From 37c75a3a81df62f2c443f443c89c6844c7b4407b Mon Sep 17 00:00:00 2001 From: Otto Date: Wed, 17 Apr 2024 20:23:32 +0200 Subject: [PATCH 1/3] Change ScriptProcessorNode::onaudioprocess API - use owned buffer value This is easier for the node bindings. We hook into the Drop call of the event to ship the output buffer to the render thread. TODO: I found a gnarly bug where the ScriptProcessorRenderer would be cleared from the audio graph erroneously. AudioContextRegistration should not be allowed to implement Clone because it has a very meaningful Drop implementation. That's why I now resort to an `Arc` hack to prevent the Drop method to run. --- examples/script_processor.rs | 2 +- src/events.rs | 9 +++++++++ src/node/script_processor.rs | 12 +++++++----- src/render/processor.rs | 1 + 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/examples/script_processor.rs b/examples/script_processor.rs index f37c525c..e23e6ea6 100644 --- a/examples/script_processor.rs +++ b/examples/script_processor.rs @@ -28,7 +28,7 @@ fn main() { }); let node = context.create_script_processor(512, 1, 1); - node.set_onaudioprocess(|e| { + node.set_onaudioprocess(|mut e| { let mut rng = rand::thread_rng(); e.output_buffer .get_channel_data_mut(0) diff --git a/src/events.rs b/src/events.rs index 42beb282..d2774287 100644 --- a/src/events.rs +++ b/src/events.rs @@ -52,6 +52,15 @@ pub struct AudioProcessingEvent { /// The time when the audio will be played in the same time coordinate system as the /// AudioContext's currentTime. pub playback_time: f64, + pub(crate) registration: Option>, +} + +impl Drop for AudioProcessingEvent { + fn drop(&mut self) { + if let Some(registration) = self.registration.take() { + registration.post_message(self.output_buffer.clone()); + } + } } /// The OfflineAudioCompletionEvent Event interface diff --git a/src/node/script_processor.rs b/src/node/script_processor.rs index 37431fea..212e4b3d 100644 --- a/src/node/script_processor.rs +++ b/src/node/script_processor.rs @@ -7,6 +7,7 @@ use crate::render::{ use crate::{AudioBuffer, RENDER_QUANTUM_SIZE}; use std::any::Any; +use std::sync::Arc; /// Options for constructing an [`ScriptProcessorNode`] #[derive(Clone, Debug)] @@ -139,18 +140,19 @@ impl ScriptProcessorNode { /// /// Only a single event handler is active at any time. Calling this method multiple times will /// override the previous event handler. - pub fn set_onaudioprocess( + pub fn set_onaudioprocess( &self, mut callback: F, ) { - let registration = self.registration.clone(); + // TODO, hack: use Arc to prevent drop of AudioContextRegistration + let registration = Arc::new(self.registration.clone()); let callback = move |v| { let mut payload = match v { EventPayload::AudioProcessing(v) => v, _ => unreachable!(), }; - callback(&mut payload); - registration.post_message(payload.output_buffer); + payload.registration = Some(Arc::clone(®istration)); + callback(payload); }; self.context().set_event_handler( @@ -301,7 +303,7 @@ mod tests { let node = context.create_script_processor(BUFFER_SIZE, 0, 1); node.connect(&context.destination()); - node.set_onaudioprocess(|e| { + node.set_onaudioprocess(|mut e| { e.output_buffer.get_channel_data_mut(0).fill(1.); // set all samples to 1. }); diff --git a/src/render/processor.rs b/src/render/processor.rs index 3376dea6..a543f9f2 100644 --- a/src/render/processor.rs +++ b/src/render/processor.rs @@ -68,6 +68,7 @@ impl AudioWorkletGlobalScope { input_buffer, output_buffer, playback_time, + registration: None, }; let dispatch = EventDispatch::audio_processing(self.node_id.get(), event); let _ = self.event_sender.try_send(dispatch); From 701be64209b77cf0c21a7952f76c0b8b7c68e573 Mon Sep 17 00:00:00 2001 From: Otto Date: Sat, 20 Apr 2024 12:08:46 +0200 Subject: [PATCH 2/3] No Clone for AudioContextRegistration because the Drop has side effects Rewrite the AudioProcessingEvent a bit to circumvent this limitation --- src/context/mod.rs | 5 ++--- src/events.rs | 11 ++++++++--- src/node/script_processor.rs | 12 ++++++++---- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/context/mod.rs b/src/context/mod.rs index d96477bb..621b632b 100644 --- a/src/context/mod.rs +++ b/src/context/mod.rs @@ -88,9 +88,8 @@ impl From for AudioContextState { /// Only when implementing the AudioNode trait manually, this struct is of any concern. /// /// This object allows for communication with the render thread and dynamic lifetime management. -// -// The only way to construct this object is by calling [`BaseAudioContext::register`] -#[derive(Clone)] +// The only way to construct this object is by calling [`BaseAudioContext::register`]. +// This struct should not derive Clone because of the Drop handler. pub struct AudioContextRegistration { /// the audio context in which nodes and connections lives context: ConcreteBaseAudioContext, diff --git a/src/events.rs b/src/events.rs index d2774287..a1009c31 100644 --- a/src/events.rs +++ b/src/events.rs @@ -1,3 +1,4 @@ +use crate::context::ConcreteBaseAudioContext; use crate::context::{AudioContextState, AudioNodeId}; use crate::{AudioBuffer, AudioRenderCapacityEvent}; @@ -52,13 +53,17 @@ pub struct AudioProcessingEvent { /// The time when the audio will be played in the same time coordinate system as the /// AudioContext's currentTime. pub playback_time: f64, - pub(crate) registration: Option>, + pub(crate) registration: Option<(ConcreteBaseAudioContext, AudioNodeId)>, } impl Drop for AudioProcessingEvent { fn drop(&mut self) { - if let Some(registration) = self.registration.take() { - registration.post_message(self.output_buffer.clone()); + if let Some((context, id)) = self.registration.take() { + let wrapped = crate::message::ControlMessage::NodeMessage { + id, + msg: llq::Node::new(Box::new(self.output_buffer.clone())), + }; + context.send_control_msg(wrapped); } } } diff --git a/src/node/script_processor.rs b/src/node/script_processor.rs index 212e4b3d..9989e784 100644 --- a/src/node/script_processor.rs +++ b/src/node/script_processor.rs @@ -7,7 +7,6 @@ use crate::render::{ use crate::{AudioBuffer, RENDER_QUANTUM_SIZE}; use std::any::Any; -use std::sync::Arc; /// Options for constructing an [`ScriptProcessorNode`] #[derive(Clone, Debug)] @@ -138,20 +137,25 @@ impl ScriptProcessorNode { /// the inputBuffer attribute. The audio data which is the result of the processing (or the /// synthesized data if there are no inputs) is then placed into the outputBuffer. /// + /// The output buffer is shipped back to the render thread when the AudioProcessingEvent goes + /// out of scope, so be sure not to store it somewhere. + /// /// Only a single event handler is active at any time. Calling this method multiple times will /// override the previous event handler. pub fn set_onaudioprocess( &self, mut callback: F, ) { - // TODO, hack: use Arc to prevent drop of AudioContextRegistration - let registration = Arc::new(self.registration.clone()); + // We need these fields to ship the output buffer to the render thread + let base = self.registration().context().clone(); + let id = self.registration().id(); + let callback = move |v| { let mut payload = match v { EventPayload::AudioProcessing(v) => v, _ => unreachable!(), }; - payload.registration = Some(Arc::clone(®istration)); + payload.registration = Some((base.clone(), id)); callback(payload); }; From 5b4874787987177b04dd64685ef5b27abec91028 Mon Sep 17 00:00:00 2001 From: Otto Date: Sat, 20 Apr 2024 12:15:59 +0200 Subject: [PATCH 3/3] Fix API call for ScriptProcessorNode after merging main --- src/node/script_processor.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node/script_processor.rs b/src/node/script_processor.rs index d4cfe180..be993a3a 100644 --- a/src/node/script_processor.rs +++ b/src/node/script_processor.rs @@ -342,7 +342,7 @@ mod tests { // 2 input channels, 2 output channels let node = context.create_script_processor(BUFFER_SIZE, 2, 2); node.connect(&context.destination()); - node.set_onaudioprocess(|e| { + node.set_onaudioprocess(|mut e| { // left output buffer is left input * 2 e.output_buffer .get_channel_data_mut(0)