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

AudioContext.setSinkId() #2498

Merged
merged 18 commits into from Oct 4, 2022
Merged

AudioContext.setSinkId() #2498

merged 18 commits into from Oct 4, 2022

Conversation

hoch
Copy link
Member

@hoch hoch commented Jul 20, 2022

Fixes: #2400


Preview | Diff

@hoch hoch requested a review from padenot July 20, 2022 00:50
@hoch
Copy link
Member Author

hoch commented Jul 20, 2022

@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.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
aarongable pushed a commit to chromium/chromium that referenced this pull request Sep 1, 2022
This implements: WebAudio/web-audio-api#2498
sinkId and setSinkId are added to support selecting an audio output
device. setSinkId() will take in sinkId as a DOMString and return a
pending promise. This CL adds IDL / attribute / method for setSinkId and
will be gated behind AudioContextSetSinkId/test.

Related CL: 3838685
Reference document: go/webaudio-setsinkid-api

Bug: 1216187
Change-Id: I001743125bbf0dce8104c23d34f10d33fb562f48
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3862684
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Alvin Ji <alvinji@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1042229}
index.bs Outdated Show resolved Hide resolved
aarongable pushed a commit to chromium/chromium that referenced this pull request Sep 13, 2022
This implements: WebAudio/web-audio-api#2498
sinkId and setSinkId are added to support selecting an audio output device. setSinkId() will take in sinkId as a DOMString and return a pending promise. Multiple setSinkId requests are queued and resolved sequentially.


Reference document: go/webaudio-setsinkid-api

Bug: 1216187
Change-Id: I4125a0b434b45a9364cf1a69ad9b858a6d6fb7a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3838685
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Commit-Queue: Alvin Ji <alvinji@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1046125}
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@hoch
Copy link
Member Author

hoch commented Sep 20, 2022

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
Copy link
Member Author

hoch commented Sep 26, 2022

PTAL again @padenot and @chrisguttandin - I think the last revision covers what @padenot suggested in #2498 (comment).

Copy link
Member

@padenot padenot left a comment

Choose a reason for hiding this comment

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

A couple nits, but looks good, please merge at your convenience!

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
Copy link
Contributor

@chrisguttandin chrisguttandin left a comment

Choose a reason for hiding this comment

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

Thanks for pinging me again. I only spotted a few "tabs".

While reading the text I was wondering what would be the sampleRate of an AudioContext with { type: 'none' } as the sinkId? Do we need to define a default sampleRate for that case?

const audioContext = new AudioContext({ sinkId: { type: 'none' } });

console.log(audioContext.sampleRate); // ?

I was also wondering if the algorithm for changing the output device should be aborted if the the device doesn't change. As far as I understand it the following code (which is of course a silly example) would still trigger the algorithm although it's basically not changing anything.

audioContext.setSinkId(audioContext.sinkId);

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@hoch
Copy link
Member Author

hoch commented Sep 30, 2022

@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
Copy link
Contributor

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
Copy link
Member Author

hoch commented Sep 30, 2022

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
Copy link
Member Author

hoch commented Oct 3, 2022

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

  • When |sinkId| is the same with the current one, exit early (on constructor) or resolve it right away (on setSinkId()).
  • Now the ID validation subroutine returns a DOMException or null. (Previously it was returning a DOMException or boolean. I thought that could be confusing)

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
Copy link
Member Author

hoch commented Oct 3, 2022

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

@hoch hoch merged commit cc718a3 into main Oct 4, 2022
tidoust added a commit to tidoust/web-audio-api that referenced this pull request Oct 5, 2022
Changes in WebAudio#2498 introduce a weird phrasing for "fire an event", e.g.
"Fire an event to onstatechange EventHandler". Correct phrasing should rather
be "Fire an event named statechange".

This made me realize that the spec never associates the event handler IDL
attributes that it defines with the actual event handler event type that gets
fired. The spec also seems to assume that there will be at most one event
handler (e.g. when it says that "This AudioBuffer is only valid while in the
scope of the onaudioprocess function"), whereas `onxxx` is just one way to
listen to `xxx` events, `addEventListener` may also be used.

Specs typically make the association between event handler IDL attributes and
event handler event types explicit, e.g. as done in HTML through tables such as:
https://html.spec.whatwg.org/multipage/webappapis.html#event-handlers-on-elements,-document-objects,-and-window-objects

... or in WebRTC through text in the definition of the IDL attribute:
https://w3c.github.io/webrtc-pc/#ref-for-event-datachannel-bufferedamountlow-1

This pull request adds definitions of event types next to the definitions of
the `onxxx` IDL attributes with which they are associated and uses references
to these event types whenever the spec fires an event or mentions event
handlers.

I tried to keep the changes minimal. I was going to argue that these changes are
editorial in nature as they merely clarify something that did not create
ambiguities for implementations. Now, you seem to track all changes with
"proposed corrections", regardless of whether they're editorial or substantive
so I suspect you cannot accept these changes as-is...

At a minimum, I think that the occurrences of "fire an event to onxxx" should be
fixed (they only appear in proposed corrections so that seems doable without
creating additional proposed corrections). I can prepare a separate PR to that
effect. I can also look into creating appropriate "proposed corrections"
structures for the other changes if they seem useful. That may not be worth the
hassle.
tidoust added a commit to tidoust/web-audio-api that referenced this pull request Oct 5, 2022
Changes in WebAudio#2498 introduce a weird phrasing for "fire an event", e.g.
"Fire an event to onstatechange EventHandler". Correct phrasing should rather
be "Fire an event named statechange".

This made me realize that the spec never associates the event handler IDL
attributes that it defines with the actual event handler event type that gets
fired. The spec also seems to assume that there will be at most one event
handler (e.g. when it says that "This AudioBuffer is only valid while in the
scope of the onaudioprocess function"), whereas `onxxx` is just one way to
listen to `xxx` events, `addEventListener` may also be used.

Specs typically make the association between event handler IDL attributes and
event handler event types explicit, e.g. as done in HTML through tables such as:
https://html.spec.whatwg.org/multipage/webappapis.html#event-handlers-on-elements,-document-objects,-and-window-objects

... or in WebRTC through text in the definition of the IDL attribute:
https://w3c.github.io/webrtc-pc/#ref-for-event-datachannel-bufferedamountlow-1

This pull request adds definitions of event types next to the definitions of
the `onxxx` IDL attributes with which they are associated and uses references
to these event types whenever the spec fires an event or mentions event
handlers.

I tried to keep the changes minimal. I was going to argue that these changes are
editorial in nature as they merely clarify something that did not create
ambiguities for implementations. Now, you seem to track all changes with
"proposed corrections", regardless of whether they're editorial or substantive
so I suspect you cannot accept these changes as-is...

At a minimum, I think that the occurrences of "fire an event to onxxx" should be
fixed (they only appear in proposed corrections so that seems doable without
creating additional proposed corrections). I can prepare a separate PR to that
effect. I can also look into creating appropriate "proposed corrections"
structures for the other changes if they seem useful. That may not be worth the
hassle.
padenot pushed a commit that referenced this pull request Oct 12, 2022
Changes in #2498 introduce a weird phrasing for "fire an event", e.g.
"Fire an event to onstatechange EventHandler". Correct phrasing should rather
be "Fire an event named statechange".

This made me realize that the spec never associates the event handler IDL
attributes that it defines with the actual event handler event type that gets
fired. The spec also seems to assume that there will be at most one event
handler (e.g. when it says that "This AudioBuffer is only valid while in the
scope of the onaudioprocess function"), whereas `onxxx` is just one way to
listen to `xxx` events, `addEventListener` may also be used.

Specs typically make the association between event handler IDL attributes and
event handler event types explicit, e.g. as done in HTML through tables such as:
https://html.spec.whatwg.org/multipage/webappapis.html#event-handlers-on-elements,-document-objects,-and-window-objects

... or in WebRTC through text in the definition of the IDL attribute:
https://w3c.github.io/webrtc-pc/#ref-for-event-datachannel-bufferedamountlow-1

This pull request adds definitions of event types next to the definitions of
the `onxxx` IDL attributes with which they are associated and uses references
to these event types whenever the spec fires an event or mentions event
handlers.

I tried to keep the changes minimal. I was going to argue that these changes are
editorial in nature as they merely clarify something that did not create
ambiguities for implementations. Now, you seem to track all changes with
"proposed corrections", regardless of whether they're editorial or substantive
so I suspect you cannot accept these changes as-is...

At a minimum, I think that the occurrences of "fire an event to onxxx" should be
fixed (they only appear in proposed corrections so that seems doable without
creating additional proposed corrections). I can prepare a separate PR to that
effect. I can also look into creating appropriate "proposed corrections"
structures for the other changes if they seem useful. That may not be worth the
hassle.
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
This implements: WebAudio/web-audio-api#2498
sinkId and setSinkId are added to support selecting an audio output
device. setSinkId() will take in sinkId as a DOMString and return a
pending promise. This CL adds IDL / attribute / method for setSinkId and
will be gated behind AudioContextSetSinkId/test.

Related CL: 3838685
Reference document: go/webaudio-setsinkid-api

Bug: 1216187
Change-Id: I001743125bbf0dce8104c23d34f10d33fb562f48
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3862684
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Alvin Ji <alvinji@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1042229}
NOKEYCHECK=True
GitOrigin-RevId: 7aa2e143bc33131d4fd9080da0ba97e6c8e6a345
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
This implements: WebAudio/web-audio-api#2498
sinkId and setSinkId are added to support selecting an audio output device. setSinkId() will take in sinkId as a DOMString and return a pending promise. Multiple setSinkId requests are queued and resolved sequentially.

Reference document: go/webaudio-setsinkid-api

Bug: 1216187
Change-Id: I4125a0b434b45a9364cf1a69ad9b858a6d6fb7a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3838685
Reviewed-by: Hongchan Choi <hongchan@chromium.org>
Commit-Queue: Alvin Ji <alvinji@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1046125}
NOKEYCHECK=True
GitOrigin-RevId: 12306d136e14a4498f240b99d90573b0c48952d0
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.

Access to a different output device: AudioContext.setSinkId()
4 participants