Prevent out-of-order arrival of audio node settings #311

orottier · 2023-06-25T18:37:44Z

This is the direction I am thinking of now, to prevent settings passed via atomics 'overtaking' settings passed by the various message channels. Fixes #297

It adds a new method to the public API of the AudioProcessor:
fn onmessage(&mut self, msg: Box<dyn std::any::Any + Send + 'static>)
And to the mysterious AudioContextRegistration:
pub fn post_message(&self, msg: Box<dyn std::any::Any + Send + 'static>)

But the AudioProcessor::onmessage fn comes with a default no-op implementation so I think it will not really complicate things. The AudioProcessor interface is not part of the specification anyway (just modeled after AudioWorkletProcessor ) and only advanced users will encounter it.

I have rewritten the AudioParam and OscillatorNode to use it so far. All other nodes still need to be ported. In the end, none of the nodes should have their own crossbeam sender/receiver.

Besides fixing the arrival order, I also expect a modest performance gain because nodes do not need to poll for changed settings on each render quantum. Instead the render thread pushes them on demand.

Some tests are still failing. I need to rethink the way we write the AudioParam test suite. And there are some subtle issues with the AudioListener which is only loaded lazily which confuses the AudioParam events we ship.

orottier · 2023-06-26T07:30:11Z

src/context/mod.rs

+    ///
+    /// The message will be handled by
+    /// [`AudioProcessor::onmessage`](crate::render::AudioProcessor::onmessage).
+    pub fn post_message(&self, msg: Box<dyn std::any::Any + Send + 'static>) {


A de-allocation will happen every time this method is called (we're consuming the Box by value). I'm not sure how we could prevent it - except for shipping the box back to the control thread when we are done with it..

I implemented the shipping back pattern for Moire based on llq: https://codeberg.org/moire/moire/src/branch/main/crates/ctrl/src/circular_fifo.rs. Not sure if this is suitable here.

The other option would be to hand over handled messages to basedrop from the real-time thread.

It makes me think we could return the renderers back to the control thread as well when they are removed from the graph?
Let me know if I'm wrong or badly understand something, but then the memory would be cleared within the control thread and it would also open the door for implementing some pools of renderers?

This is tricky. The renderer may have stored/cached AudioRenderQuanta (e.g. DelayRenderer) which are not Send so are not allowed to be moved outside of the render thread.

A generic system to decommision AudioProcessors and control messages without touching the global allocator is something worth to explore though.

I will create a separate issue, also taking the recommendations of @uklotzde into account

b-ma · 2023-06-26T09:13:51Z

Actually, regarding the spec, this could be explicitly modelled according to the AudioWorklet MessagePort interface, isn't it?

related specs:

…f-order-arrival

orottier · 2023-06-27T05:55:30Z

Actually, regarding the spec, this could be explicitly modelled according to the AudioWorklet MessagePort interface

Hah, I had not realized it was actually this close. The only difference is that the spec allows for bi-directional comms.

Otherwise, AudioParam automation events will be routed to a non-yet existing processor

…f-order-arrival

orottier · 2023-07-08T11:38:51Z

@b-ma @uklotzde Apologies for the large PR. But this should lay the groundwork to prevent out of order processing of changes to the audio graph. Fixes #311

Nodes to rewrite still (in a new PR):

BiquadFilterNode
ConvolverNode
PannerNode
WaveShaperNode

For further consideration

also rewrite the ChannelConfig to use message passing

uklotzde · 2023-07-09T17:47:23Z

src/control.rs

-    pub fn get_start_at(&self) -> f64 {
-        self.inner.start.load(Ordering::Acquire)
+impl Controller {
+    pub fn shared(&self) -> Arc<ControllerShared> {


Only return a reference in such situations. The caller knows better if and when to clone it. The caller might even decide to obtain only a weak reference.

Thanks, I have changed it to return &Arc

uklotzde · 2023-07-09T17:56:22Z

src/render/processor.rs

+    /// [`AudioContextRegistration::post_message`](crate::context::AudioContextRegistration::post_message).
+    /// This will not be necessary for most processors.
+    #[allow(unused_variables)]
+    fn onmessage(&mut self, msg: Box<dyn Any + Send + 'static>) {


Why onmessage instead of on_message? This looks odd. Either change the name or leave a comment that explains why this naming is the right choice here.

Odd indeed, it's from https://developer.mozilla.org/en-US/docs/Web/API/MessagePort/message_event
I added a comment to clarify this

github-actions · 2023-07-11T06:01:17Z

Benchmark result:


bench_ctor
  Instructions:             4295905 (-0.430619%)
  L1 Accesses:              6418795 (-0.626834%)
  L2 Accesses:                54022 (-0.120177%)
  RAM Accesses:               61399 (-0.297165%)
  Estimated Cycles:         8837870 (-0.531441%)

bench_sine
  Instructions:            77054551 (-1.098305%)
  L1 Accesses:            113380795 (-0.901524%)
  L2 Accesses:               265222 (-9.907333%)
  RAM Accesses:               62271 (-0.617639%)
  Estimated Cycles:       116886390 (-1.008517%)

bench_sine_gain
  Instructions:            81866631 (-1.017497%)
  L1 Accesses:            120654370 (-0.828858%)
  L2 Accesses:               263349 (-9.118862%)
  RAM Accesses:               62367 (-0.710044%)
  Estimated Cycles:       124153960 (-0.922625%)

bench_sine_gain_delay
  Instructions:           156002857 (-0.482918%)
  L1 Accesses:            221993342 (-0.363321%)
  L2 Accesses:               569510 (-12.31279%)
  RAM Accesses:               64020 (-0.728795%)
  Estimated Cycles:       227081592 (-0.536901%)

bench_buffer_src
  Instructions:            17525840 (-2.026301%)
  L1 Accesses:             25342403 (-1.938690%)
  L2 Accesses:                86207 (-6.691273%)
  RAM Accesses:              100445 (+4.532209%)
  Estimated Cycles:        29289013 (-1.279164%)

bench_buffer_src_delay
  Instructions:            90783783 (-0.155646%)
  L1 Accesses:            125540772 (-0.057697%)
  L2 Accesses:               165357 (-23.91397%)
  RAM Accesses:              100632 (+4.522321%)
  Estimated Cycles:       129889677 (-0.138345%)

bench_buffer_src_iir
  Instructions:            41578873 (-0.582583%)
  L1 Accesses:             60382332 (-0.475390%)
  L2 Accesses:                87523 (-15.04024%)
  RAM Accesses:              100545 (+4.530758%)
  Estimated Cycles:        64339022 (-0.330527%)

bench_buffer_src_biquad
  Instructions:            38075272 (-1.420590%)
  L1 Accesses:             53553935 (-1.417608%)
  L2 Accesses:               111411 (-24.68362%)
  RAM Accesses:              100659 (+4.534078%)
  Estimated Cycles:        57634055 (-1.368823%)

bench_stereo_positional
  Instructions:            45594233 (-2.838222%)
  L1 Accesses:             67917716 (-2.471454%)
  L2 Accesses:               235640 (-61.16940%)
  RAM Accesses:              100821 (+4.525379%)
  Estimated Cycles:        72624651 (-4.502784%)

bench_stereo_panning_automation
  Instructions:            31612416 (-1.331616%)
  L1 Accesses:             47117702 (-1.109327%)
  L2 Accesses:               134175 (-25.60547%)
  RAM Accesses:              100567 (+4.522117%)
  Estimated Cycles:        51308422 (-1.169539%)

bench_analyser_node
  Instructions:            38055551 (-0.889869%)
  L1 Accesses:             53257024 (-0.808277%)
  L2 Accesses:               184591 (-1.788745%)
  RAM Accesses:              101091 (+4.446878%)
  Estimated Cycles:        57718164 (-0.517323%)

b-ma · 2023-07-11T06:30:49Z

Hey, looks pretty good to me

Just one thing I don't really understand: why did you keep the Controller? It seems that it could be dropped just as the Scheduler, no?

Let me know if I can help fixing the other nodes!

orottier · 2023-07-11T07:48:32Z

why did you keep the Controller

Good point. It can be inlined into audio_buffer_source.rs. I will create a separate issue for it because I'm already quite tired from this PR :)

b-ma · 2023-07-11T09:35:25Z

I'm already quite tired from this PR :)

Huhu I get it, I will try to propose something these next days

orottier added 3 commits June 25, 2023 20:20

Experiment with new method of message passing for AudioParams

acb8015

Use new message passing system for OscillatorNode

acd5f2a

Rename message methods to resemble MessagePort interface

7f1ce0b

This comment was marked as outdated.

Sign in to view

orottier commented Jun 26, 2023

View reviewed changes

orottier added 2 commits June 26, 2023 15:36

AudioParam: pass automation_rate changes also via main message bus

0a200c5

Code style

f9dbbff

This comment was marked as outdated.

Sign in to view

orottier mentioned this pull request Jun 26, 2023

Controller & Scheduler: use single Arc instead of 1 per field #312

Merged

Oscillator, pass start_at/stop_at also via the main message bus

bc35ad9

This comment was marked as outdated.

Sign in to view

orottier added 4 commits June 26, 2023 16:42

Rewrite constant_source to use proper scheduling

84a8a3f

Merge remote-tracking branch 'origin/main' into feature/prevent-out-o…

814c2df

…f-order-arrival

Rewrite AudioBufferSource to use new style message passing for controls

8df23ef

AudioBufferSourceNode: pass audio-buffer via main message channel

6dab397

This comment was marked as outdated.

Sign in to view

orottier mentioned this pull request Jun 27, 2023

Prevent needless clone in offline rendering + in the online happy path #310

Merged

orottier added 4 commits July 5, 2023 21:07

Make AudioParam tests work again, but at what cost..?

80583e3

When instantiating the AudioListener, add it to the audio graph already

c0bd5b3

Otherwise, AudioParam automation events will be routed to a non-yet existing processor

Merge remote-tracking branch 'origin/main' into feature/prevent-out-o…

05a7283

…f-order-arrival

Rewrite Arc::clone

fda9d9b

This comment was marked as outdated.

Sign in to view

DelayNode: remove unnecessary usage of atomics

db36c6a

This comment was marked as outdated.

Sign in to view

Improve AudioParam test calls

3fe764b

This comment was marked as outdated.

Sign in to view

orottier added 2 commits July 8, 2023 13:27

DynamicsCompressor: use Relaxed atomics for info sharing

9b1e30c

Make post_message generic, so users don't need to Box themselves

f36bc48

orottier marked this pull request as ready for review July 8, 2023 11:38

This comment was marked as outdated.

Sign in to view

uklotzde reviewed Jul 9, 2023

View reviewed changes

orottier added 2 commits July 11, 2023 07:23

AudioProcessor trait: document the origin of the onmessage method

4d24bc1

Controller::shared(), return &Arc to let caller decide to clone

6eaac05

orottier merged commit f9a25ab into main Jul 11, 2023

orottier mentioned this pull request Jul 11, 2023

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

Closed

5 tasks

orottier deleted the feature/prevent-out-of-order-arrival branch July 13, 2023 18:31

b-ma mentioned this pull request Jul 17, 2023

Merge Controller into AudioBufferSourceNode #329

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent out-of-order arrival of audio node settings #311

Prevent out-of-order arrival of audio node settings #311

orottier commented Jun 25, 2023 •

edited

Loading

This comment was marked as outdated.

orottier Jun 26, 2023

uklotzde Jul 8, 2023

b-ma Jul 11, 2023

orottier Jul 11, 2023

b-ma commented Jun 26, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

orottier commented Jun 27, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

orottier commented Jul 8, 2023

This comment was marked as outdated.

uklotzde Jul 9, 2023

orottier Jul 11, 2023

uklotzde Jul 9, 2023

orottier Jul 11, 2023

github-actions bot commented Jul 11, 2023

b-ma commented Jul 11, 2023

orottier commented Jul 11, 2023

b-ma commented Jul 11, 2023

Prevent out-of-order arrival of audio node settings #311

Prevent out-of-order arrival of audio node settings #311

Conversation

orottier commented Jun 25, 2023 • edited Loading

This comment was marked as outdated.

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 commented Jun 26, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

orottier commented Jun 27, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

orottier commented Jul 8, 2023

This comment was marked as outdated.

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

github-actions bot commented Jul 11, 2023

b-ma commented Jul 11, 2023

orottier commented Jul 11, 2023

b-ma commented Jul 11, 2023

orottier commented Jun 25, 2023 •

edited

Loading