docs: generalize note about AudioNodeOptions not useful in source nodes

b-ma · b-ma · commit 4bb48b75ea27 · 2023-07-23T08:53:16.000+02:00
diff --git a/src/node/audio_buffer_source.rs b/src/node/audio_buffer_source.rs
@@ -1,10 +1,12 @@
 use std::any::Any;
-use std::cell::{OnceCell, RefCell};
+use std::cell::OnceCell;
+use std::sync::atomic::{AtomicBool, Ordering};
 
 use crate::buffer::AudioBuffer;
 use crate::context::{AudioContextRegistration, AudioParamId, BaseAudioContext};
 use crate::param::{AudioParam, AudioParamDescriptor, AutomationRate};
 use crate::render::{AudioParamValues, AudioProcessor, AudioRenderQuantum, RenderScope};
+use crate::AtomicF64;
 use crate::RENDER_QUANTUM_SIZE;
 
 use super::{AudioNode, AudioScheduledSourceNode, ChannelConfig};
@@ -18,6 +20,10 @@ use super::{AudioNode, AudioScheduledSourceNode, ChannelConfig};
 //   double loopStart = 0;
 //   float playbackRate = 1;
 // };
+//
+// @note - Does not extend AudioNodeOptions because AudioNodeOptions are
+// useless for source nodes as they instruct how to upmix the inputs.
+// This is a common source of confusion, see e.g. https://github.com/mdn/content/pull/18472
 #[derive(Clone, Debug)]
 pub struct AudioBufferSourceOptions {
     pub buffer: Option<AudioBuffer>,
@@ -47,15 +53,54 @@ struct PlaybackInfo {
     k: f32,
 }
 
-#[derive(Debug, Clone)]
-struct LoopState {
-    pub is_looping: bool,
-    pub start: f64,
-    pub end: f64,
+// The strategy for loop parameters is as follow: store the given values
+// in the `loop_control` thread safe instance which only lives in control
+// thread(s) and send a message to the render thread which stores the raw values.
+// Values between control and render side might be desynchronised for little while
+// but the developer experience will appear more logical, i.e.
+// ```no_run
+// node.set_loop(true);
+// println!("{:?}", node.loop_());
+// > true // is guaranteed
+// ```
+// Note that this seems to be the strategy used by Firefox
+#[derive(Debug)]
+struct LoopControl {
+    loop_: AtomicBool,
+    loop_start: AtomicF64,
+    loop_end: AtomicF64,
+}
+
+// Uses the canonical ordering for handover of values,
+// i.e. `Acquire` on load and `Release` on store.
+impl LoopControl {
+    fn loop_(&self) -> bool {
+        self.loop_.load(Ordering::Acquire)
+    }
+
+    fn set_loop(&self, loop_: bool) {
+        self.loop_.store(loop_, Ordering::Release);
+    }
+
+    fn loop_start(&self) -> f64 {
+        self.loop_start.load(Ordering::Acquire)
+    }
+
+    fn set_loop_start(&self, loop_start: f64) {
+        self.loop_start.store(loop_start, Ordering::Release);
+    }
+
+    fn loop_end(&self) -> f64 {
+        self.loop_end.load(Ordering::Acquire)
+    }
+
+    fn set_loop_end(&self, loop_end: f64) {
+        self.loop_end.store(loop_end, Ordering::Release);
+    }
 }
 
 /// Instructions to start or stop processing
-#[derive(Debug, Clone)]
+#[derive(Debug, Copy, Clone)]
 enum ControlMessage {
     StartWithOffsetAndDuration(f64, f64, f64),
     Stop(f64),
@@ -98,17 +143,12 @@ enum ControlMessage {
 ///
 pub struct AudioBufferSourceNode {
     registration: AudioContextRegistration,
+    loop_control: LoopControl,
     channel_config: ChannelConfig,
     detune: AudioParam,        // has constraints, no a-rate
     playback_rate: AudioParam, // has constraints, no a-rate
     buffer: OnceCell<AudioBuffer>,
-    inner_state: RefCell<InnerState>,
-}
-
-#[derive(Debug, Clone)]
-struct InnerState {
-    loop_state: LoopState,
-    source_started: bool,
+    source_started: AtomicBool,
 }
 
 impl AudioNode for AudioBufferSourceNode {
@@ -145,10 +185,9 @@ impl AudioScheduledSourceNode for AudioBufferSourceNode {
     }
 
     fn stop_at(&self, when: f64) {
-        assert!(
-            self.inner_state.borrow().source_started,
-            "InvalidStateError cannot stop before start"
-        );
+        if !self.source_started.load(Ordering::SeqCst) {
+            panic!("InvalidStateError cannot stop before start");
+        }
 
         self.registration.post_message(ControlMessage::Stop(when));
     }
@@ -191,36 +230,35 @@ impl AudioBufferSourceNode {
             pr_param.set_automation_rate_constrained(true);
             pr_param.set_value(playback_rate);
 
-            let loop_state = LoopState {
-                is_looping: loop_,
-                start: loop_start,
-                end: loop_end,
+            let loop_control = LoopControl {
+                loop_: AtomicBool::new(loop_),
+                loop_start: AtomicF64::new(loop_start),
+                loop_end: AtomicF64::new(loop_end),
             };
 
             let renderer = AudioBufferSourceRenderer {
                 start_time: f64::MAX,
                 stop_time: f64::MAX,
                 duration: f64::MAX,
                 offset: 0.,
+                loop_,
+                loop_start,
+                loop_end,
                 buffer: None,
                 detune: d_proc,
                 playback_rate: pr_proc,
-                loop_state: loop_state.clone(),
                 render_state: AudioBufferRendererState::default(),
                 ended_triggered: false,
             };
 
-            let inner_state = InnerState {
-                loop_state,
-                source_started: false,
-            };
             let node = Self {
                 registration,
+                loop_control,
                 channel_config: ChannelConfig::default(),
                 detune: d_param,
                 playback_rate: pr_param,
                 buffer: OnceCell::new(),
-                inner_state: RefCell::new(inner_state),
+                source_started: AtomicBool::new(false),
             };
 
             if let Some(buf) = buffer {
@@ -246,12 +284,9 @@ impl AudioBufferSourceNode {
     ///
     /// Panics if the source was already started
     pub fn start_at_with_offset_and_duration(&self, start: f64, offset: f64, duration: f64) {
-        let source_started = &mut self.inner_state.borrow_mut().source_started;
-        assert!(
-            !*source_started,
-            "InvalidStateError: Cannot call `start` twice"
-        );
-        *source_started = true;
+        if self.source_started.swap(true, Ordering::SeqCst) {
+            panic!("InvalidStateError: Cannot call `start` twice");
+        }
 
         let control = ControlMessage::StartWithOffsetAndDuration(start, offset, duration);
         self.registration.post_message(control);
@@ -298,32 +333,32 @@ impl AudioBufferSourceNode {
 
     /// Defines if the playback the [`AudioBuffer`] should be looped
     pub fn loop_(&self) -> bool {
-        self.inner_state.borrow().loop_state.is_looping
+        self.loop_control.loop_()
     }
 
     pub fn set_loop(&self, value: bool) {
-        self.inner_state.borrow_mut().loop_state.is_looping = value;
+        self.loop_control.set_loop(value);
         self.registration.post_message(ControlMessage::Loop(value));
     }
 
     /// Defines the loop start point, in the time reference of the [`AudioBuffer`]
     pub fn loop_start(&self) -> f64 {
-        self.inner_state.borrow().loop_state.start
+        self.loop_control.loop_start()
     }
 
     pub fn set_loop_start(&self, value: f64) {
-        self.inner_state.borrow_mut().loop_state.start = value;
+        self.loop_control.set_loop_start(value);
         self.registration
             .post_message(ControlMessage::LoopStart(value));
     }
 
     /// Defines the loop end point, in the time reference of the [`AudioBuffer`]
     pub fn loop_end(&self) -> f64 {
-        self.inner_state.borrow().loop_state.end
+        self.loop_control.loop_end()
     }
 
     pub fn set_loop_end(&self, value: f64) {
-        self.inner_state.borrow_mut().loop_state.end = value;
+        self.loop_control.set_loop_end(value);
         self.registration
             .post_message(ControlMessage::LoopEnd(value));
     }
@@ -354,26 +389,28 @@ struct AudioBufferSourceRenderer {
     stop_time: f64,
     offset: f64,
     duration: f64,
+    loop_: bool,
+    loop_start: f64,
+    loop_end: f64,
     buffer: Option<AudioBuffer>,
     detune: AudioParamId,
     playback_rate: AudioParamId,
-    loop_state: LoopState,
     render_state: AudioBufferRendererState,
     ended_triggered: bool,
 }
 
 impl AudioBufferSourceRenderer {
-    fn handle_control_message(&mut self, control: &ControlMessage) {
+    fn handle_control_message(&mut self, control: ControlMessage) {
         match control {
             ControlMessage::StartWithOffsetAndDuration(when, offset, duration) => {
-                self.start_time = *when;
-                self.offset = *offset;
-                self.duration = *duration;
+                self.start_time = when;
+                self.offset = offset;
+                self.duration = duration;
             }
-            ControlMessage::Stop(when) => self.stop_time = *when,
-            ControlMessage::Loop(is_looping) => self.loop_state.is_looping = *is_looping,
-            ControlMessage::LoopStart(loop_start) => self.loop_state.start = *loop_start,
-            ControlMessage::LoopEnd(loop_end) => self.loop_state.end = *loop_end,
+            ControlMessage::Stop(when) => self.stop_time = when,
+            ControlMessage::Loop(loop_) => self.loop_ = loop_,
+            ControlMessage::LoopStart(loop_start) => self.loop_start = loop_start,
+            ControlMessage::LoopEnd(loop_end) => self.loop_end = loop_end,
         }
     }
 }
@@ -394,11 +431,10 @@ impl AudioProcessor for AudioBufferSourceRenderer {
         let block_duration = dt * RENDER_QUANTUM_SIZE as f64;
         let next_block_time = scope.current_time + block_duration;
 
-        let LoopState {
-            is_looping,
-            start: loop_start,
-            end: loop_end,
-        } = self.loop_state.clone();
+        // grab all timing information
+        let loop_ = self.loop_;
+        let loop_start = self.loop_start;
+        let loop_end = self.loop_end;
 
         // these will only be used if `loop_` is true, so no need for `Option`
         let mut actual_loop_start = 0.;
@@ -453,7 +489,7 @@ impl AudioProcessor for AudioBufferSourceRenderer {
         }
 
         // 3. the end of the buffer has been reached.
-        if !is_looping {
+        if !loop_ {
             if computed_playback_rate > 0. && self.render_state.buffer_time >= buffer_duration {
                 output.make_silent(); // also converts to mono
                 if !self.ended_triggered {
@@ -558,15 +594,15 @@ impl AudioProcessor for AudioBufferSourceRenderer {
                             *o = if buffer_index < end_index {
                                 buffer_channel[buffer_index]
                             } else {
-                                if is_looping && buffer_index == end_index {
+                                if loop_ && buffer_index == end_index {
                                     loop_point_index = Some(index);
                                     // reset values for the rest of the block
                                     start_index = 0;
                                     offset = index;
                                     buffer_index = 0;
                                 }
 
-                                if is_looping {
+                                if loop_ {
                                     buffer_channel[buffer_index]
                                 } else {
                                     0.
@@ -607,7 +643,7 @@ impl AudioProcessor for AudioBufferSourceRenderer {
         // ---------------------------------------------------------------
         // Slow track
         // ---------------------------------------------------------------
-        if is_looping {
+        if loop_ {
             if loop_start >= 0. && loop_end > 0. && loop_start < loop_end {
                 actual_loop_start = loop_start;
                 actual_loop_end = loop_end.min(buffer_duration);
@@ -639,19 +675,19 @@ impl AudioProcessor for AudioBufferSourceRenderer {
             if !self.render_state.started {
                 self.offset += current_time - self.start_time;
 
-                if is_looping && computed_playback_rate >= 0. && self.offset >= actual_loop_end {
+                if loop_ && computed_playback_rate >= 0. && self.offset >= actual_loop_end {
                     self.offset = actual_loop_end;
                 }
 
-                if is_looping && computed_playback_rate < 0. && self.offset < actual_loop_start {
+                if loop_ && computed_playback_rate < 0. && self.offset < actual_loop_start {
                     self.offset = actual_loop_start;
                 }
 
                 self.render_state.buffer_time = self.offset;
                 self.render_state.started = true;
             }
 
-            if is_looping {
+            if loop_ {
                 if !self.render_state.entered_loop {
                     // playback began before or within loop, and playhead is now past loop start
                     if self.offset < actual_loop_end
@@ -740,7 +776,7 @@ impl AudioProcessor for AudioBufferSourceRenderer {
 
     fn onmessage(&mut self, msg: &mut dyn Any) {
         if let Some(control) = msg.downcast_ref::<ControlMessage>() {
-            self.handle_control_message(control);
+            self.handle_control_message(*control);
             return;
         };
 
diff --git a/src/node/constant_source.rs b/src/node/constant_source.rs
@@ -11,6 +11,10 @@ use super::{AudioNode, AudioScheduledSourceNode, ChannelConfig};
 // dictionary ConstantSourceOptions {
 //   float offset = 1;
 // };
+//
+// @note - Does not extend AudioNodeOptions because AudioNodeOptions are
+// useless for source nodes as they instruct how to upmix the inputs.
+// This is a common source of confusion, see e.g. https://github.com/mdn/content/pull/18472
 #[derive(Clone, Debug)]
 pub struct ConstantSourceOptions {
     pub offset: f32,
diff --git a/src/node/media_element_source.rs b/src/node/media_element_source.rs
@@ -6,6 +6,13 @@ use crate::RENDER_QUANTUM_SIZE;
 use super::{AudioNode, ChannelConfig, MediaStreamRenderer};
 
 /// Options for constructing a [`MediaElementAudioSourceNode`]
+// dictionary MediaElementAudioSourceOptions {
+//     required HTMLMediaElement mediaElement;
+// };
+//
+// @note - Does not extend AudioNodeOptions because AudioNodeOptions are
+// useless for source nodes as they instruct how to upmix the inputs.
+// This is a common source of confusion, see e.g. https://github.com/mdn/content/pull/18472
 pub struct MediaElementAudioSourceOptions<'a> {
     pub media_element: &'a mut MediaElement,
 }
diff --git a/src/node/media_stream_source.rs b/src/node/media_stream_source.rs
@@ -9,6 +9,10 @@ use super::{AudioNode, ChannelConfig, MediaStreamRenderer};
 // dictionary MediaStreamAudioSourceOptions {
 //   required MediaStream mediaStream;
 // };
+//
+// @note - Does not extend AudioNodeOptions because AudioNodeOptions are
+// useless for source nodes as they instruct how to upmix the inputs.
+// This is a common source of confusion, see e.g. https://github.com/mdn/content/pull/18472
 pub struct MediaStreamAudioSourceOptions<'a> {
     pub media_stream: &'a MediaStream,
 }
diff --git a/src/node/media_stream_track_source.rs b/src/node/media_stream_track_source.rs
@@ -6,9 +6,13 @@ use crate::RENDER_QUANTUM_SIZE;
 use super::{AudioNode, ChannelConfig, MediaStreamRenderer};
 
 /// Options for constructing a [`MediaStreamTrackAudioSourceNode`]
-// dictionary MediaStreamAudioSourceOptions {
-//   required MediaStream mediaStream;
+// dictionary MediaStreamTrackAudioSourceOptions {
+//     required MediaStreamTrack mediaStreamTrack;
 // };
+//
+// @note - Does not extend AudioNodeOptions because AudioNodeOptions are
+// useless for source nodes as they instruct how to upmix the inputs.
+// This is a common source of confusion, see e.g. https://github.com/mdn/content/pull/18472
 pub struct MediaStreamTrackAudioSourceOptions<'a> {
     pub media_stream_track: &'a MediaStreamTrack,
 }
diff --git a/src/node/oscillator.rs b/src/node/oscillator.rs
