WebAudio / web-audio-api

The Web Audio API v1.0, developed by the W3C Audio WG
https://webaudio.github.io/web-audio-api/
Other
1.04k stars 165 forks source link

AudioContext.setSinkId() #2498

Closed hoch closed 1 year ago

hoch commented 1 year ago

Fixes: #2400


Preview | Diff

hoch commented 1 year ago

@padenot PTAL - I've made an attempt on setSinkId() based on our discussion. I'll add onsinkchange event handler soon, but just wanted to kick off the review first.

hoch commented 1 year ago

We probably need to integrate this to the graph processing algorithm. Maybe mentioning it happens between render quantum or something?

Since we probably are going to use "statechange" to suspend/resume rendering, so that already implicates the operation boundary is based on render quantum. What do you think?

Does onsinkchange fire if the default audio output changes? i.e. you're just playing on the default audio output device, and it's being unplugged, so that the device has changed? It would be useful: something like the sample-rate or optimal render quantum size could be different, authors might want to rebuild their audio graph.

I think it's better to limit onsinkchange to be originated from the setSinkId() call. Any device change caused by different reasons can be caught/handled by MediaDevices.ondevicechange. In this specific example, the default device will still mean "" (the empty string) so it would be no change from the Web Audio API's perspective. This would be an edge case we need to think about.

Re: the proposal we discuss in TPAC 2022:

The promise from setSinkID(...) is resolved after the audio is flowing again (i.e. it's resolved after the operation has succeeded)

Yes. This is handled by the prose "run a control message...".

The AudioContext.state is "suspended" while the graph is not being rendered because the underlying audio output device is changing onstatechange fires normally.

This is reasonable, but will trigger two onstatechange events along the way.

The old device's stream is closed A new system-level audio stream is opened, on the new device Some time passes, opening the device can take time

Also reasonable, and in the spec I think we can simply say "when the system-level audio callback is invoked from the new audio stream".

It's useful that we already defined: https://webaudio.github.io/web-audio-api/#system-level-audio-callback

hoch commented 1 year ago

PTAL again @padenot and @chrisguttandin - I think the last revision covers what @padenot suggested in https://github.com/WebAudio/web-audio-api/pull/2498#discussion_r974618408.

hoch commented 1 year ago

@chrisguttandin The {type: "none"} is handled by the step below:

  1. If |sinkId| is the empty string or a type of {{AudioSinkOptions}}, use the sample rate of the default output. Abort these substeps.

For your second question, I think the "do nothing" case only happens when |sinkId| is the same with the current one. All other case will be handled by the rejected promise.

What do you think?

chrisguttandin commented 1 year ago

Since I often run the Web Audio API in tests on CI servers I had to learn the hard way that not every computer (or VM) has an audio device. :-( I'm not sure though if that rare edge case needs to be handled.

I think you're right, the second question can probably be ignored. Calling setSinkId() with exactly the same sinkId as the one of the currently used device can be easily avoided by a user of the API with a simple comparison.

hoch commented 1 year ago

Hmm. That's an interesting case - but it easily leads into a different question. setSinkId() won't even be a problem because the AudioContext construction will fail on such system. IMO it needs a new spec issue for further discussion.

hoch commented 1 year ago

@padenot While cleaning up and preparing for mark up, I made some changes to improve the algorithm:

Other than that changes are editorial. Please feel free to take one more look - in the mean time I'll start marking things up for landing.

hoch commented 1 year ago

Okay, the markup is done and this PR is ready to land.