Refactor message passing #337

b-ma · 2023-07-19T09:06:21Z

Work in progress regarding #315

b-ma · 2023-07-21T13:06:56Z

Ok I think this is all done, just two notes on the PannerNode:

What I did is kind of dirty with the HrtfState, didn't want to fight with the borrow checker, and should be cleaned. But I think this is a good first step and the message channel has been removed.
Also added the set_orientation and set_position methods. I know they are marked as deprecated by they are used in the wpt, so it sounds quite logical to implement them I guess. In any case this is very easy to revert.

Edit: I tried to follow the pattern proposed by @uklotzde in the AudioBufferSourceNode, let me know if I misunderstood something

uklotzde

It would be easier for review to refactor the node types one after another. All these changes are not dependent on each other.

uklotzde · 2023-07-22T08:16:54Z

src/node/convolver.rs

-    atomic::{AtomicBool, Ordering},
-    Arc, Mutex,
-};
+use super::{AudioNode, ChannelConfig, ChannelConfigOptions, ChannelInterpretation};

 /// Scale buffer by an equal-power normalization
 fn normalization(buffer: &AudioBuffer) -> f32 {


Unrelated: Why does this function name does not start with a verb as usual?

uklotzde · 2023-07-22T08:23:38Z

src/node/panner.rs

-    ref_distance: Arc<AtomicF64>,
-    max_distance: Arc<AtomicF64>,
-    rolloff_factor: Arc<AtomicF64>,
+    cone_inner_angle: AtomicF64,


Why still individual atomics instead of an inner typed wrapped into a single RefCell as demonstrated in in the AudioBufferSourceNode? This also applies to all other node types as far as I could see.

uklotzde · 2023-07-22T08:29:29Z

This PR does not seem to fix the soundness errors that are potentially causing inconsistencies in multi-threaded contexts.

orottier · 2023-07-22T09:41:16Z

This PR does not seem to fix the soundness errors that are potentially causing inconsistencies in multi-threaded contexts.

Yeah, I think @b-ma is keeping these things separate for now.
This PR addresses the leftover nodes that do not use message passing yet, as described in #315
Indeed this introduces soundness issues, but let's decide on the way forward first in #344 . I won't release in the meantime
(I'm a bit busy at the moment but will propose something shortly)

Edit, I just read

Edit: I tried to follow the pattern proposed by @uklotzde in the AudioBufferSourceNode, let me know if I misunderstood something

but I still think it is fine to keep matters separate for now

b-ma · 2023-07-22T09:46:05Z

Edit, I just read

Edit: I tried to follow the pattern proposed by @uklotzde in the AudioBufferSourceNode, let me know if I misunderstood something

but I still think it is fine to keep matters separate for now

Yup that was the idea, I was actually thinking about the post_message / onmessage pattern used for the AudioBuffer and control messages

orottier · 2023-07-22T09:56:02Z

src/node/constant_source.rs

@@ -11,8 +11,11 @@ use super::{AudioNode, AudioScheduledSourceNode, ChannelConfig};
 // dictionary ConstantSourceOptions {
 //   float offset = 1;
 // };
+// @note: this does not extend AudioNodeOptions in the spec, why?
+// https://webaudio.github.io/web-audio-api/#ConstantSourceOptions


AudioNodeOptions are useless for source nodes, because they instruct how to upmix the inputs.
This is a common source of confusion, see e.g. mdn/content#18472
Also for the spec editors because they accidentally let OscillatorOptions extend AudioNodeOptions

Added your note and link to the comment, so we don't forget again. I will try to align this to other nodes as well later

orottier · 2023-07-22T09:57:42Z

src/node/convolver.rs

+    // dummy convolver used to init renderer
+    fn tombstone() -> Self {
+        // just use arbitrary common sample rate
+        let padded_buffer = AudioBuffer::from(vec![vec![0.; 0]; 1], 48000.);


This allocates (non-empty vec). I think you can reuse the tombstone from AudioBufferSourceRenderer

Edit: oh this is not very important because we don't use it in the renderer..

Found a way to make it more clean in any case

orottier · 2023-07-22T19:25:20Z

I have improved the HRTF-loading in the panner node.
You ran into issues because the onmessage handler receives an &mut of the message. So if you need to take out a value that is not Copy or Clone, you need to place back a tombstone or, when that's not easily created, wrap the inner value in an Option so you can take it out.

I will have a closer look at the other changes tomorrow.

b-ma · 2023-07-23T05:56:49Z

I have improved the HRTF-loading in the panner node.
You ran into issues because the onmessage handler receives an &mut of the message. So if you need to take out a value that is not Copy or Clone, you need to place back a tombstone or, when that's not easily created, wrap the inner value in an Option so you can take it out.

Ok thanks! Think I got it (until next time borrow checker shows up :)

orottier

Thanks for the hard work. I will pick up the stuff I have mentioned.

orottier · 2023-07-24T19:01:33Z

src/node/convolver.rs

-        })
+        });
+
+        // renderer has been sent to render thread, we can sent it messages


Suggested change

// renderer has been sent to render thread, we can sent it messages

// renderer has been sent to render thread, we can send it messages

orottier · 2023-07-24T19:04:35Z

src/node/convolver.rs

@@ -359,6 +345,17 @@ impl ConvolverRendererInner {
        }
    }

+    // dummy convolver used to init renderer


I think I could better rewrite this like the HRTF-renderer. I'm not too sure what Fft::new(0), does behind the scenes. Also, it will make it more succinct and clear.

Stay tuned for updates

orottier · 2023-07-24T19:05:30Z

src/node/convolver.rs

@@ -414,6 +411,11 @@ impl ConvolverRendererInner {
    }
 }

+struct ConvolverRenderer {


As said, I will rewrite this.
A pattern of is_set and a tombstone value is way better represented as simply Option<ConvolverRendererInner>

sure this is a bit redondant

orottier · 2023-07-24T19:07:12Z

src/node/waveshaper.rs

+    /// distortion curve
+    curve: Vec<f32>,
+    /// whether the distortion curve has been set
+    curve_set: bool,


Similar, I will rewrite this to simply curve: Option<Vec<f32>>

orottier · 2023-07-24T19:08:50Z

src/node/waveshaper.rs

@@ -135,9 +128,7 @@ pub struct WaveShaperNode {
    /// distortion curve
    curve: OnceCell<Vec<f32>>,


I missed this node in #347 . I will need to rewrite this one to use a OnceLock instead

orottier · 2023-07-24T19:35:20Z

/bench

orottier · 2023-07-24T19:36:17Z

Ready to merge but I want to publish a release before merging as we still have some bugfixes lined up.
Merging this will force us to address #344 to fix the logic bugs from the atomics

github-actions · 2023-07-24T19:38:21Z

Benchmark result:


bench_ctor
  Instructions:             4525410 (-1.308950%)
  L1 Accesses:              6722228 (-1.754016%)
  L2 Accesses:                54313 (+0.012890%)
  RAM Accesses:               61555 (+0.021124%)
  Estimated Cycles:         9148218 (-1.289678%)

bench_sine
  Instructions:            70988666 (-0.159642%)
  L1 Accesses:            103561106 (-0.177325%)
  L2 Accesses:               261179 (-0.916933%)
  RAM Accesses:               62423 (-0.182292%)
  Estimated Cycles:       107051806 (-0.186515%)

bench_sine_gain
  Instructions:            75902391 (-0.384479%)
  L1 Accesses:            110928371 (-0.448659%)
  L2 Accesses:               278610 (+1.632778%)
  RAM Accesses:               62527 (+0.017595%)
  Estimated Cycles:       114509866 (-0.414975%)

bench_sine_gain_delay
  Instructions:           150748566 (-0.258071%)
  L1 Accesses:            213321276 (-0.313888%)
  L2 Accesses:               557720 (+4.565311%)
  RAM Accesses:               64175 (+0.020261%)
  Estimated Cycles:       218356001 (-0.251019%)

bench_buffer_src
  Instructions:            17751855 (-0.743735%)
  L1 Accesses:             25542998 (-0.909105%)
  L2 Accesses:                86855 (-2.304733%)
  RAM Accesses:              100719 (+0.007944%)
  Estimated Cycles:        29502438 (-0.821293%)

bench_buffer_src_delay
  Instructions:            91325956 (-0.238735%)
  L1 Accesses:            126259309 (-0.259935%)
  L2 Accesses:               163412 (+0.060620%)
  RAM Accesses:              100913 (+0.013875%)
  Estimated Cycles:       130608324 (-0.250551%)

bench_buffer_src_iir
  Instructions:            41798280 (-0.568978%)
  L1 Accesses:             60559810 (-0.674354%)
  L2 Accesses:                87508 (+0.491502%)
  RAM Accesses:              100820 (+0.004960%)
  Estimated Cycles:        64526050 (-0.629623%)

bench_buffer_src_biquad
  Instructions:            37997279 (-1.323566%)
  L1 Accesses:             53218794 (-1.653817%)
  L2 Accesses:               136919 (+3.773685%)
  RAM Accesses:              100951 (+0.006935%)
  Estimated Cycles:        57436674 (-1.491776%)

bench_stereo_positional
  Instructions:            45023336 (-2.320743%)
  L1 Accesses:             66775237 (-2.693669%)
  L2 Accesses:               244593 (-7.271404%)
  RAM Accesses:              101035 (-0.049463%)
  Estimated Cycles:        71534427 (-2.648518%)

bench_stereo_panning_automation
  Instructions:            32367298 (-0.230654%)
  L1 Accesses:             48100703 (-0.280920%)
  L2 Accesses:               136328 (+0.529459%)
  RAM Accesses:              100830 (+0.009919%)
  Estimated Cycles:        52311393 (-0.250873%)

bench_analyser_node
  Instructions:            39553046 (+0.122918%)
  L1 Accesses:             55381910 (+0.174286%)
  L2 Accesses:               179841 (+0.356021%)
  RAM Accesses:              101376 (-0.000986%)
  Estimated Cycles:        59829275 (+0.166600%)

This comment was marked as off-topic.

Sign in to view

b-ma marked this pull request as ready for review July 21, 2023 12:59

uklotzde reviewed Jul 22, 2023

View reviewed changes

orottier reviewed Jul 22, 2023

View reviewed changes

orottier reviewed Jul 24, 2023

View reviewed changes

b-ma added 14 commits July 24, 2023 21:14

refactor: align message passing strategy with AudioBufferSourceNode

fb985f9

refactor: harmonize biquad filter message passing

5c2ea5e

refactor: renaming variables

575bded

refactor: update to message passing API

87a74d3

refactor: move to message passing API

95e7cc1

refactor: more simple init

c7391ef

typo

c48da0a

refactor: simplify initialization

66ca300

refactor: add explicit dummy constructor for inner convolver

42aa3d1

cleaning

3235d24

refactor: wip message passing

d432715

refactor: use message passing

d086a96

feat: add set_orientation and set_position methods

2fe4278

refactor: clean dummy ConvolverRendererInner

f704a1b

orottier added 5 commits July 24, 2023 21:16

Fix loading of HRTF-sphere for panner node in onmessage handler

5bc2acc

Ensure WaveShaperNode is Send+Sync, use OnceLock

6e0c3ed

Simplify ConvolverNode message passing

7d45b1c

Simplify WaveShaperNode message passing

72b2a1e

Spelling

13cda2c

orottier force-pushed the refactor/message-passing branch from 483a22c to 13cda2c Compare July 24, 2023 19:34

orottier marked this pull request as draft July 24, 2023 19:35

orottier marked this pull request as ready for review July 27, 2023 18:54

orottier merged commit 98bf467 into orottier:main Jul 27, 2023

orottier mentioned this pull request Jul 27, 2023

Rewrite audio nodes to use proper message passing instead of atomics #315

Closed

5 tasks

b-ma deleted the refactor/message-passing branch November 4, 2023 06:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor message passing #337

Refactor message passing #337

b-ma commented Jul 19, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

b-ma commented Jul 21, 2023 •

edited

Loading

uklotzde left a comment

uklotzde Jul 22, 2023

uklotzde Jul 22, 2023

uklotzde commented Jul 22, 2023

orottier commented Jul 22, 2023 •

edited

Loading

b-ma commented Jul 22, 2023

orottier Jul 22, 2023

b-ma Jul 23, 2023

orottier Jul 22, 2023 •

edited

Loading

b-ma Jul 23, 2023

orottier commented Jul 22, 2023

b-ma commented Jul 23, 2023

orottier left a comment

orottier Jul 24, 2023

orottier Jul 24, 2023

orottier Jul 24, 2023

b-ma Jul 24, 2023 •

edited

Loading

orottier Jul 24, 2023

orottier Jul 24, 2023

orottier commented Jul 24, 2023

orottier commented Jul 24, 2023

github-actions bot commented Jul 24, 2023

	// renderer has been sent to render thread, we can sent it messages
	// renderer has been sent to render thread, we can send it messages

		@@ -135,9 +128,7 @@ pub struct WaveShaperNode {
		/// distortion curve
		curve: OnceCell<Vec<f32>>,

Refactor message passing #337

Refactor message passing #337

Conversation

b-ma commented Jul 19, 2023

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

b-ma commented Jul 21, 2023 • edited Loading

uklotzde left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Jul 22, 2023

orottier commented Jul 22, 2023 • edited Loading

b-ma commented Jul 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orottier Jul 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orottier commented Jul 22, 2023

b-ma commented Jul 23, 2023

orottier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-ma Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orottier commented Jul 24, 2023

orottier commented Jul 24, 2023

github-actions bot commented Jul 24, 2023

b-ma commented Jul 21, 2023 •

edited

Loading

orottier commented Jul 22, 2023 •

edited

Loading

orottier Jul 22, 2023 •

edited

Loading

b-ma Jul 24, 2023 •

edited

Loading