From 6ad4967e50c762c6a126a3f781a9568477a2bd20 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 10 Oct 2018 16:42:05 -0700 Subject: [PATCH 1/6] Handle other silence case in Block::sum() --- audio/src/block.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/audio/src/block.rs b/audio/src/block.rs index 715a1161..ca1d7b8e 100644 --- a/audio/src/block.rs +++ b/audio/src/block.rs @@ -102,13 +102,15 @@ impl Block { pub fn sum(mut self, mut other: Self) -> Self { if self.is_silence() { other + } else if other.is_silence() { + self } else { - debug_assert!(self.channels == other.channels); + debug_assert_eq!(self.channels, other.channels); if self.repeat ^ other.repeat { self.explicit_repeat(); other.explicit_repeat(); } - debug_assert!(self.buffer.len() == other.buffer.len()); + debug_assert_eq!(self.buffer.len(), other.buffer.len()); for (a, b) in self.buffer.iter_mut().zip(other.buffer.iter()) { *a += b } From 0d78e49a1ac5f12261c442370364fb5f8170b754 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 10 Oct 2018 14:26:40 -0700 Subject: [PATCH 2/6] Handle dirtying of AudioParams with SetValue events Currently, doing things like `biquad.frequency = 5` will not update the filter's internal coefficients since SetValue events bypass the other events. --- audio/src/param.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/audio/src/param.rs b/audio/src/param.rs index cf7effb0..2e6542ae 100644 --- a/audio/src/param.rs +++ b/audio/src/param.rs @@ -39,6 +39,7 @@ pub struct Param { block_mix_val: f32, /// If true, `blocks` has been summed together into a single block summed: bool, + dirty: bool, } #[derive(Copy, Clone, Eq, PartialEq, Debug)] @@ -61,6 +62,7 @@ impl Param { blocks: Vec::new(), block_mix_val: 0., summed: false, + dirty: false, } } @@ -71,6 +73,8 @@ impl Param { /// /// Returns true if anything changed pub fn update(&mut self, block: &BlockInfo, tick: Tick) -> bool { + let mut changed = self.dirty; + self.dirty = false; if tick.0 == 0 { self.summed = true; if let Some(first) = self.blocks.pop() { @@ -83,14 +87,14 @@ impl Param { self.blocks.push(block); } } else if self.kind == ParamRate::KRate { - return false; + return changed; } // Even if the timeline does nothing, it's still possible // that there were connected inputs, so we should not // directly return `false` after this point, instead returning // `changed` - let changed = if let Some(block) = self.blocks.get(0) { + changed |= if let Some(block) = self.blocks.get(0) { // store to be summed with `val` later self.block_mix_val = block.data_chan(0)[tick.0 as usize]; true @@ -176,6 +180,7 @@ impl Param { if let AutomationEvent::SetValue(val) = event { self.val = val; self.event_start_value = val; + self.dirty = true; return; } From ed56cee01499495cd3e54c641ae31ab5f0ece0be Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 10 Oct 2018 14:26:50 -0700 Subject: [PATCH 3/6] Use f64s internally in BiquadFilterNode The WPT tests exhibit differences otherwise --- audio/src/biquad_filter_node.rs | 42 +++++++++++++++++---------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/audio/src/biquad_filter_node.rs b/audio/src/biquad_filter_node.rs index 2cbd38b8..ad6b73e4 100644 --- a/audio/src/biquad_filter_node.rs +++ b/audio/src/biquad_filter_node.rs @@ -5,7 +5,7 @@ use node::BlockInfo; use node::{AudioNodeMessage, AudioNodeType, ChannelInfo}; use param::{Param, ParamType}; use smallvec::SmallVec; -use std::f32::consts::{SQRT_2, PI}; +use std::f64::consts::{SQRT_2, PI}; #[derive(Copy, Clone, Debug)] pub struct BiquadFilterNodeOptions { @@ -50,18 +50,18 @@ pub enum BiquadFilterNodeMessage { #[derive(Default, Copy, Clone, PartialEq)] struct BiquadState { /// The input value from last frame - x1: f32, + x1: f64, /// The input value from two frames ago - x2: f32, + x2: f64, /// The output value from last frame - y1: f32, + y1: f64, /// The output value from two frames ago - y2: f32, + y2: f64, } impl BiquadState { /// Update with new input/output values from this frame - fn update(&mut self, x: f32, y: f32) { + fn update(&mut self, x: f64, y: f64) { self.x2 = self.x1; self.x1 = x; self.y2 = self.y1; @@ -81,23 +81,23 @@ pub(crate) struct BiquadFilterNode { /// The computed filter parameter b0 /// This is actually b0 / a0, we pre-divide /// for efficiency - b0: f32, + b0: f64, /// The computed filter parameter b1 /// This is actually b1 / a0, we pre-divide /// for efficiency - b1: f32, + b1: f64, /// The computed filter parameter b2 /// This is actually b2 / a0, we pre-divide /// for efficiency - b2: f32, + b2: f64, /// The computed filter parameter a1 /// This is actually a1 / a0, we pre-divide /// for efficiency - a1: f32, + a1: f64, /// The computed filter parameter a2 /// This is actually a2 / a0, we pre-divide /// for efficiency - a2: f32, + a2: f64, /// Stored filter state, this contains the last two /// frames of input and output values for every /// channel @@ -144,10 +144,11 @@ impl BiquadFilterNode { /// /// See https://webaudio.github.io/web-audio-api/#filters-characteristics fn update_coefficients(&mut self, fs: f32) { - let g = self.gain.value(); - let q = self.q.value(); - let f0 = self.frequency.value() * (2.0_f32).powf(self.detune.value() / 1200.); - + let g: f64 = self.gain.value().into(); + let q: f64 = self.q.value().into(); + let freq: f64 = self.frequency.value().into(); + let f0: f64 = freq * (2.0_f64).powf(self.detune.value() as f64 / 1200.); + let fs: f64 = fs.into(); // clamp to nominal range // https://webaudio.github.io/web-audio-api/#biquadfilternode let f0 = if f0 > fs / 2. || !f0.is_finite() { @@ -158,12 +159,12 @@ impl BiquadFilterNode { f0 }; - let a = 10.0_f32.powf(g / 40.); + let a = 10.0_f64.powf(g / 40.); let omega0 = 2. * PI * f0 / fs; let sin_omega = omega0.sin(); let cos_omega = omega0.cos(); let alpha_q = sin_omega / (2. * q); - let alpha_q_db = sin_omega / (2. * 10.0_f32.powf(q / 20.)); + let alpha_q_db = sin_omega / (2. * 10.0_f64.powf(q / 20.)); let alpha_s = sin_omega / SQRT_2; // we predivide by a0 @@ -281,11 +282,12 @@ impl AudioNodeEngine for BiquadFilterNode { self.update_parameters(info, frame.tick()); frame.mutate_with(|sample, chan| { let state = &mut self.state[chan as usize]; - let x0 = *sample; - *sample = self.b0 * x0 + self.b1 * state.x1 + self.b2 * state.x2 + let x0 = *sample as f64; + let y0 = self.b0 * x0 + self.b1 * state.x1 + self.b2 * state.x2 - self.a1 * state.y1 - self.a2 * state.y2; - state.update(x0, *sample); + *sample = y0 as f32; + state.update(x0, y0); }); } } From 1716c137e1b8c77f2aa0bb4901349adaedcd8780 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 10 Oct 2018 14:27:14 -0700 Subject: [PATCH 4/6] Redo AudioScheduledSourceNode handling Fixes https://github.com/servo/media/issues/147 `.start(nonzero)` was broken before this. This fixes it, and adds support for starting and ending within a block. --- audio/src/block.rs | 1 + audio/src/buffer_source_node.rs | 35 +++++++++----------- audio/src/node.rs | 8 +++++ audio/src/oscillator_node.rs | 24 +++++++------- servo-media-derive/src/lib.rs | 57 +++++++++++++++++++++++++-------- 5 files changed, 80 insertions(+), 45 deletions(-) diff --git a/audio/src/block.rs b/audio/src/block.rs index ca1d7b8e..c8955bee 100644 --- a/audio/src/block.rs +++ b/audio/src/block.rs @@ -616,6 +616,7 @@ impl Div for Tick { } impl Tick { + pub const FRAMES_PER_BLOCK: Tick = FRAMES_PER_BLOCK; pub fn from_time(time: f64, rate: f32) -> Tick { Tick((0.5 + time * rate as f64).floor() as u64) } diff --git a/audio/src/buffer_source_node.rs b/audio/src/buffer_source_node.rs index 78475811..73850bcc 100644 --- a/audio/src/buffer_source_node.rs +++ b/audio/src/buffer_source_node.rs @@ -1,6 +1,6 @@ use block::{Block, Chunk, Tick, FRAMES_PER_BLOCK}; use node::{AudioNodeEngine, AudioScheduledSourceNodeMessage, BlockInfo, OnEndedCallback}; -use node::{AudioNodeType, ChannelInfo}; +use node::{AudioNodeType, ChannelInfo, ShouldPlay}; use param::{Param, ParamType}; /// Control messages directed to AudioBufferSourceNodes. @@ -118,34 +118,28 @@ impl AudioNodeEngine for AudioBufferSourceNode { let len = { self.buffer.as_ref().unwrap().len() as usize }; - if self.playback_offset >= len || self.should_play_at(info.frame) == (false, true) { + if self.playback_offset >= len { self.maybe_trigger_onended_callback(); inputs.blocks.push(Default::default()); return inputs; } - let buffer = self.buffer.as_ref().unwrap(); - - let samples_to_copy = match self.stop_at { - Some(stop_at) => { - let ticks_to_stop = stop_at - info.frame; - if ticks_to_stop > FRAMES_PER_BLOCK { - FRAMES_PER_BLOCK.0 as usize - } else { - ticks_to_stop.0 as usize - } - } - None => { - if self.playback_offset + (FRAMES_PER_BLOCK.0 as usize) < len { - FRAMES_PER_BLOCK.0 as usize - } else { - len - self.playback_offset - } + let (start_at, mut stop_at) = match self.should_play_at(info.frame) { + ShouldPlay::No => { + inputs.blocks.push(Default::default()); + return inputs; } + ShouldPlay::Between(start, end) => (start.0 as usize, end.0 as usize) }; - let next_offset = self.playback_offset + samples_to_copy; + let buffer = self.buffer.as_ref().unwrap(); + + if self.playback_offset + stop_at - start_at > len { + stop_at = start_at + len - self.playback_offset; + } + let samples_to_copy = stop_at - start_at; + let next_offset = self.playback_offset + samples_to_copy; if samples_to_copy == FRAMES_PER_BLOCK.0 as usize { // copy entire chan let mut block = Block::empty(); @@ -160,6 +154,7 @@ impl AudioNodeEngine for AudioBufferSourceNode { block.explicit_repeat(); for chan in 0..buffer.chans() { let data = block.data_chan_mut(chan); + let (_, data) = data.split_at_mut(start_at); let (data, _) = data.split_at_mut(samples_to_copy); data.copy_from_slice( &buffer.buffers[chan as usize][self.playback_offset..next_offset], diff --git a/audio/src/node.rs b/audio/src/node.rs index 0b5c16a2..10ba8be0 100644 --- a/audio/src/node.rs +++ b/audio/src/node.rs @@ -206,3 +206,11 @@ pub enum AudioScheduledSourceNodeMessage { /// Register onended event callback. RegisterOnEndedCallback(OnEndedCallback), } + +#[derive(Clone, Copy, PartialEq, Eq)] +pub enum ShouldPlay { + /// Don't play anything + No, + /// Play, given start and end tick offsets + Between(Tick, Tick), +} diff --git a/audio/src/oscillator_node.rs b/audio/src/oscillator_node.rs index 9249f754..bea5fb6a 100644 --- a/audio/src/oscillator_node.rs +++ b/audio/src/oscillator_node.rs @@ -1,6 +1,6 @@ use block::{Chunk, Tick}; use node::{AudioNodeEngine, AudioScheduledSourceNodeMessage, BlockInfo, OnEndedCallback}; -use node::{AudioNodeType, ChannelInfo}; +use node::{AudioNodeType, ChannelInfo, ShouldPlay}; use num_traits::cast::NumCast; use param::{Param, ParamType}; @@ -84,10 +84,12 @@ impl AudioNodeEngine for OscillatorNode { inputs.blocks.push(Default::default()); - if self.should_play_at(info.frame) == (false, true) { - self.maybe_trigger_onended_callback(); - return inputs; - } + let (start_at, stop_at) = match self.should_play_at(info.frame) { + ShouldPlay::No => { + return inputs; + } + ShouldPlay::Between(start, end) => (start, end) + }; { inputs.blocks[0].explicit_silence(); @@ -106,14 +108,12 @@ impl AudioNodeEngine for OscillatorNode { let mut step = two_pi * self.frequency.value() as f64 / sample_rate; while let Some(mut frame) = iter.next() { let tick = frame.tick(); - let (should_play_at, should_break) = self.should_play_at(info.frame + tick); - if !should_play_at { - if should_break { - self.maybe_trigger_onended_callback(); - break; - } - continue; + if tick < start_at { + continue + } else if tick > stop_at { + break; } + if self.update_parameters(info, tick) { step = two_pi * self.frequency.value() as f64 / sample_rate; } diff --git a/servo-media-derive/src/lib.rs b/servo-media-derive/src/lib.rs index bdb2e799..2d3c4786 100644 --- a/servo-media-derive/src/lib.rs +++ b/servo-media-derive/src/lib.rs @@ -18,20 +18,49 @@ fn impl_audio_scheduled_source_node(ast: &syn::DeriveInput) -> quote::Tokens { let name = &ast.ident; quote! { impl #name { - fn should_play_at(&self, tick: Tick) -> (bool, bool) { - if self.start_at.is_none() { - return (false, true); - } - - if tick < self.start_at.unwrap() { - (false, false) + fn should_play_at(&mut self, tick: Tick) -> ShouldPlay { + let start = if let Some(start) = self.start_at { + start } else { - if let Some(stop_at) = self.stop_at { - if tick >= stop_at { - return (false, true); + return ShouldPlay::No; + }; + + let frame_end = tick + Tick::FRAMES_PER_BLOCK; + if tick < start { + if frame_end < start { + ShouldPlay::No + } else { + let delta_start = start - tick; + if let Some(stop) = self.stop_at { + if stop <= start { + self.maybe_trigger_onended_callback(); + return ShouldPlay::No; + } + if stop > frame_end { + ShouldPlay::Between(delta_start, Tick::FRAMES_PER_BLOCK) + } else { + self.maybe_trigger_onended_callback(); + ShouldPlay::Between(delta_start, stop - tick) + } + } else { + ShouldPlay::Between(delta_start, Tick::FRAMES_PER_BLOCK) } } - (true, false) + } else { + let stop = if let Some(stop) = self.stop_at { + stop + } else { + return ShouldPlay::Between(Tick(0), Tick::FRAMES_PER_BLOCK); + }; + if stop > frame_end { + ShouldPlay::Between(Tick(0), Tick::FRAMES_PER_BLOCK) + } else if stop < tick { + self.maybe_trigger_onended_callback(); + ShouldPlay::No + } else { + self.maybe_trigger_onended_callback(); + ShouldPlay::Between(Tick(0), stop - tick) + } } } @@ -58,10 +87,12 @@ fn impl_audio_scheduled_source_node(ast: &syn::DeriveInput) -> quote::Tokens { fn maybe_trigger_onended_callback(&mut self) { // We cannot have an end without a start. - if self.start_at.is_none() || self.onended_callback.is_none() { + if self.start_at.is_none() { return; } - self.onended_callback.take().unwrap().0.call(); + if let Some(cb) = self.onended_callback.take() { + cb.0.call() + } } fn handle_source_node_message(&mut self, message: AudioScheduledSourceNodeMessage, sample_rate: f32) { From d5a9855da929c4feb13ad4de02771af3b161f675 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 10 Oct 2018 15:29:37 -0700 Subject: [PATCH 5/6] Add limits for biquad filter node These aren't mentioned in the spec but must be there to avoid NaNs. Copied from Servo. --- audio/src/biquad_filter_node.rs | 90 ++++++++++++++++++++++++++++++++- 1 file changed, 89 insertions(+), 1 deletion(-) diff --git a/audio/src/biquad_filter_node.rs b/audio/src/biquad_filter_node.rs index ad6b73e4..fac168d2 100644 --- a/audio/src/biquad_filter_node.rs +++ b/audio/src/biquad_filter_node.rs @@ -140,6 +140,15 @@ impl BiquadFilterNode { changed } + /// Set to the constant z-transform y[n] = b0 * x[n] + fn constant_z_transform(&mut self, b0: f64) { + self.b0 = b0; + self.b1 = 0.; + self.b2 = 0.; + self.a1 = 0.; + self.a2 = 0.; + } + /// Update the coefficients a1, a2, b0, b1, b2, given the sample_rate /// /// See https://webaudio.github.io/web-audio-api/#filters-characteristics @@ -159,8 +168,87 @@ impl BiquadFilterNode { f0 }; + let normalized = f0 / fs; let a = 10.0_f64.powf(g / 40.); - let omega0 = 2. * PI * f0 / fs; + + // the boundary values sometimes need limits to + // be taken + match self.filter { + FilterType::LowPass => { + if normalized == 1. { + self.constant_z_transform(1.); + return; + } else if normalized == 0. { + self.constant_z_transform(0.); + return; + } + } + FilterType::HighPass => { + if normalized == 1. { + self.constant_z_transform(0.); + return; + } else if normalized == 0. { + self.constant_z_transform(1.); + return; + } + } + FilterType::LowShelf => { + if normalized == 1. { + self.constant_z_transform(a * a); + return; + } else if normalized == 0. { + self.constant_z_transform(1.); + return; + } + } + FilterType::HighShelf => { + if normalized == 1. { + self.constant_z_transform(1.); + return; + } else if normalized == 0. { + self.constant_z_transform(a * a); + return; + } + } + FilterType::Peaking => { + if normalized == 0. || normalized == 1. { + self.constant_z_transform(1.); + return; + } else if q <= 0. { + self.constant_z_transform(a * a); + return; + } + } + FilterType::AllPass => { + if normalized == 0. || normalized == 1. { + self.constant_z_transform(1.); + return; + } else if q <= 0. { + self.constant_z_transform(-1.); + return; + } + } + FilterType::Notch => { + if normalized == 0. || normalized == 1. { + self.constant_z_transform(1.); + return; + } else if q <= 0. { + self.constant_z_transform(0.); + return; + } + } + FilterType::BandPass => { + if normalized == 0. || normalized == 1. { + self.constant_z_transform(0.); + return; + } else if q <= 0. { + self.constant_z_transform(1.); + return; + } + } + } + + let omega0 = 2. * PI * normalized; let sin_omega = omega0.sin(); let cos_omega = omega0.cos(); let alpha_q = sin_omega / (2. * q); From d1caba2727604434fc8622d3ba4b10763ffc4e2a Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Wed, 10 Oct 2018 16:07:19 -0700 Subject: [PATCH 6/6] Second argument of .update_parameters is a tick offset --- audio/src/biquad_filter_node.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/audio/src/biquad_filter_node.rs b/audio/src/biquad_filter_node.rs index fac168d2..a54e3158 100644 --- a/audio/src/biquad_filter_node.rs +++ b/audio/src/biquad_filter_node.rs @@ -343,7 +343,7 @@ impl AudioNodeEngine for BiquadFilterNode { debug_assert!(inputs.len() == 1); self.state .resize(inputs.blocks[0].chan_count() as usize, Default::default()); - self.update_parameters(info, info.frame); + self.update_parameters(info, Tick(0)); // XXXManishearth this node has tail time, so even if the block is silence // we must still compute things on it. However, it is possible to become