Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change output device for running AudioContext #232

Merged
merged 9 commits into from
Nov 6, 2022
Merged

Conversation

orottier
Copy link
Owner

This is the second of three PRs for #216

  1. Specify output device at AudioContext creation #227
  2. Change output device for running AudioContext
  3. Create AudioContext without output

You will notice a severe lack of tests for this complicated feature. That is because the functionality is only exposed in online AudioContext which need an actual audio backend. For now, because that will be changed in PR number 3.

You can test it with cargo run --release --example sink_id but notice that cpal only lists your default audio output, so you won't be able to switch from headphones to builtin speakers (on OSX). I added a virtual passthrough device to use for testing. Sad though. It is not really testable with backend cubeb because it suffers from segfaults when closing a stream. Known issue #187 :(

@@ -1,25 +1,23 @@
//! Audio IO management API
Copy link
Owner Author

@orottier orottier Oct 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thoroughly rewrote this file, so I'm closing #51 no I'm not, I misread the issue

/// This function operates synchronously and might block the current thread. An async version
/// is currently not implemented.
#[allow(clippy::needless_collect, clippy::missing_panics_doc)]
pub fn set_sink_id_sync(&self, sink_id: String) -> Result<(), Box<dyn Error>> {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the meat of the PR.

  1. check if sink is already active
  2. validate sink
  3. acquire necessary locks (avoid issues with concurrent calls)
  4. shut down current render thread and recover the audio graph
  5. boot up new thread, and ship the audio graph
  6. release locks

@orottier orottier changed the title Feature/set sink Change output device for running AudioContext Oct 31, 2022

// hotswap the backend
let options = AudioContextOptions {
sample_rate: Some(self.sample_rate()),
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this sample rate is not available on the new sink, bad things will happen. That is why #183 exists

pub fn output_latency(&self) -> f64 {
self.backend.output_latency()
self.backend.lock().unwrap().output_latency()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure of the thread layout here, but can't this lock() be a problem regarding the audio thread?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's something we might want to call periodically during rendering I guess

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize now the backend naming is confusing.
The backend object live in the control thread, so locking is no issue.
A better name would be backend_manager, because it passes instructions from the control thread (start, pause, close) and collects info from the actual backend in the render thread (latency, sink_id).
I will update the PR by renaming this struct

let message = ControlMessage::Startup { graph };
ctrl_msg_send.send(message).unwrap();

self.base().set_state(AudioContextState::Running);
Copy link
Collaborator

@b-ma b-ma Nov 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the context was suspended maybe we want to keep it suspended?

Actually, I didn't read the thing carefully, but the spec seems to say:

If wasRunning is true:

Set the [[rendering thread state]] on the AudioContext to "suspended".

https://webaudio.github.io/web-audio-api/#ref-for-dom-audiocontext-setsinkid%E2%91%A0

But as we already differ on the initial state, I don't really know...
Just keeping the state as it was could be a good tradeoff?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, very good point. I will update the PR

@b-ma
Copy link
Collaborator

b-ma commented Nov 1, 2022

That's quite a thing, I'm really not sure I understand everything (I'm pretty I don't actually :), so maybe my comments are not very relevant.

Actually it makes me think it would be quite nice to have some graph of who lives where in terms of threads, I'm always a bit lost in this part of the code.

In any case, the example seems to work on my side switching between some virtual device and real output

@orottier orottier changed the base branch from feature/sink-id to main November 5, 2022 09:57
@orottier
Copy link
Owner Author

orottier commented Nov 5, 2022

Actually it makes me think it would be quite nice to have some graph of who lives where in terms of threads, I'm always a bit lost in this part of the code.

Yes, I created a new issue for this: #235

To emphasize the object lives in the control thread, not the render
thread. This means we can block (Mutex.lock) without issues.
@github-actions
Copy link

github-actions bot commented Nov 5, 2022

Benchmark result:


bench_ctor
  Instructions:              849831 (+0.128897%)
  L1 Accesses:              1728971 (+0.133436%)
  L2 Accesses:                 7833 (+0.012768%)
  RAM Accesses:               10411 (+0.774368%)
  Estimated Cycles:         2132521 (+0.240151%)

bench_sine
  Instructions:             8546646 (-0.016343%)
  L1 Accesses:             12949528 (-0.000633%)
  L2 Accesses:                30890 (-3.266214%)
  RAM Accesses:               12514 (+0.224251%)
  Estimated Cycles:        13541968 (-0.031869%)

bench_sine_gain
  Instructions:             9042511 (-0.075475%)
  L1 Accesses:             13698391 (-0.069354%)
  L2 Accesses:                33854 (+3.003012%)
  RAM Accesses:               12669 (+0.269094%)
  Estimated Cycles:        14311076 (-0.023626%)

bench_sine_gain_delay
  Instructions:            16119673 (-0.604058%)
  L1 Accesses:             23371392 (-1.029687%)
  L2 Accesses:                74050 (-2.421990%)
  RAM Accesses:               12826 (-7.232750%)
  Estimated Cycles:        24190552 (-1.173900%)

bench_buffer_src
  Instructions:            10922118 (-0.008908%)
  L1 Accesses:             17962713 (+0.027899%)
  L2 Accesses:                53267 (-0.715737%)
  RAM Accesses:               37251 (+0.112876%)
  Estimated Cycles:        19532833 (+0.023351%)

bench_buffer_src_iir
  Instructions:            19929178 (-0.853846%)
  L1 Accesses:             29828210 (-1.552553%)
  L2 Accesses:                48994 (-8.978765%)
  RAM Accesses:               37339 (+0.099190%)
  Estimated Cycles:        31380045 (-1.547602%)

bench_buffer_src_biquad
  Instructions:            15275429 (+0.144250%)
  L1 Accesses:             23566087 (+0.206301%)
  L2 Accesses:                72430 (-6.980030%)
  RAM Accesses:               37440 (+0.090895%)
  Estimated Cycles:        25238637 (+0.089356%)


We need to handle an interesting edge case where the render thread never
processed the Startup command (e.g. when it was suspended for its entire
lifetime)
@github-actions
Copy link

github-actions bot commented Nov 6, 2022

Benchmark result:


bench_ctor
  Instructions:              845121 (-0.457946%)
  L1 Accesses:              1719757 (-0.389924%)
  L2 Accesses:                 7837 (+0.012762%)
  RAM Accesses:               10391 (+0.415539%)
  Estimated Cycles:         2122627 (-0.245412%)

bench_sine
  Instructions:             8563725 (+0.165974%)
  L1 Accesses:             12975148 (+0.179201%)
  L2 Accesses:                29591 (-1.294239%)
  RAM Accesses:               12501 (+0.080058%)
  Estimated Cycles:        13560638 (+0.159687%)

bench_sine_gain
  Instructions:             9074750 (+0.250240%)
  L1 Accesses:             13745853 (+0.256106%)
  L2 Accesses:                33098 (+7.016296%)
  RAM Accesses:               12652 (+0.102856%)
  Estimated Cycles:        14354163 (+0.324433%)

bench_sine_gain_delay
  Instructions:            16187320 (-0.215377%)
  L1 Accesses:             23477118 (-0.604015%)
  L2 Accesses:                70490 (-1.506260%)
  RAM Accesses:               12813 (-7.313368%)
  Estimated Cycles:        24278023 (-0.749922%)

bench_buffer_src
  Instructions:            10939593 (+0.137085%)
  L1 Accesses:             17987590 (+0.158995%)
  L2 Accesses:                53493 (+1.468161%)
  RAM Accesses:               37236 (+0.045676%)
  Estimated Cycles:        19558315 (+0.169109%)

bench_buffer_src_iir
  Instructions:            19968050 (-0.669638%)
  L1 Accesses:             29887897 (-1.360401%)
  L2 Accesses:                49148 (-6.736499%)
  RAM Accesses:               37327 (+0.032159%)
  Estimated Cycles:        31440082 (-1.347782%)

bench_buffer_src_biquad
  Instructions:            15303162 (+0.303983%)
  L1 Accesses:             23606977 (+0.365665%)
  L2 Accesses:                64754 (-13.49871%)
  RAM Accesses:               37455 (+0.120289%)
  Estimated Cycles:        25241672 (+0.147029%)


@orottier orottier merged commit f3d8f2c into main Nov 6, 2022
@orottier orottier deleted the feature/set-sink-id branch November 15, 2022 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants