AudioContext async state transitions [chromium/src : main]
Hongchan Choi (Gerrit)
Hongchan Choi added 6 comments![]( "Open in Gerrit")
Thanks for your contribution!
The code review on the autoplay part will take some time because the WebAudio owners will need to understand the actual implementation.
It generally looks good, but I am curious about your evaluation plan on the rollout other than using usage counters.
"Cannot suspend a closed AudioContext."));Perhaps we can expand this error message to be more descriptive?
CONSOLE WARNING: The AudioContext was not allowed to start. It must be resumed (or created) after a user gesture on the page. https://developer.chrome.com/blog/autoplay/#web_audioCan you explain why this CONSOLE WARNING is shown here?
const timeout = new Promise(resolve => setTimeout(resolve, 500));I am not entirely sure what test is verifying. (The autoplay code is not implemented by the WebAudio team)
If the intention is to get the state reliably, I think we can use the `onstatechange` event listener within the promise.
await context3.resume();Is this necessary to pass the test?
Related details
- Felipe Erias
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Erias (Gerrit)
Felipe Erias added 6 comments![]( "Open in Gerrit")
Thank you very much for your feedback.
---
The way I thought about the usage counters was that we could use `kAudioContextAsyncStateTransitions` to count the number of page loads with this feature enabled.
From those, `kAudioContextAsyncTransitionToRunningStateRead` and `kAudioContextAsyncTransitionToSuspendedStateRead` will count those that are at risk of introducing a bug because they are reading the context state before it has finished transitioning.
Combining these, we would be able to measure the fraction of page loads which might be at risk.
I do not know whether it would be possible to automatically identify the specific websites where those happen.
---
Regarding the code changes:
There were some unnecessary calls to `resume()` in the tests which I have now removed. I've also reworked the comments as suggested. Please see below.
"Cannot suspend a closed AudioContext."));Perhaps we can expand this error message to be more descriptive?
I've replaced it with
"AudioContext was closed before a pending suspend() could complete."
CONSOLE WARNING: The AudioContext was not allowed to start. It must be resumed (or created) after a user gesture on the page. https://developer.chrome.com/blog/autoplay/#web_audioCan you explain why this CONSOLE WARNING is shown here?
This was caused by calling `resume()` inside `test-autoplay.html` `runWebAudioTests()`, which is not needed.
const timeout = new Promise(resolve => setTimeout(resolve, 500));I am not entirely sure what test is verifying. (The autoplay code is not implemented by the WebAudio team)
If the intention is to get the state reliably, I think we can use the `onstatechange` event listener within the promise.
Agree, we don't need to call `resume()` here.
await context3.resume();Is this necessary to pass the test?
No, it is not necessary. I have removed it.
However, this test is now checking a behaviour that is a bit different from the original:
- before this CL, this test was calling `suspend()` on a context in the "running" state
- here, `suspend()` is called on a context that is still in the "suspended" state after creation, and will transition to "running" async.
await context.resume();Ditto.
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi added 5 comments![]( "Open in Gerrit")
Looking good! I’m adding mjwilson@ as an additional reviewer since this is a significant change.
I also think this is worth a PSA to blink-dev to give other developers a heads-up.
Thank you so much for this contribution!
"Cannot suspend a closed AudioContext."));Felipe EriasPerhaps we can expand this error message to be more descriptive?
I've replaced it with
"AudioContext was closed before a pending suspend() could complete."
Acknowledged
const timeout = new Promise(resolve => setTimeout(resolve, 500));Felipe EriasI am not entirely sure what test is verifying. (The autoplay code is not implemented by the WebAudio team)
If the intention is to get the state reliably, I think we can use the `onstatechange` event listener within the promise.
Agree, we don't need to call `resume()` here.
Acknowledged
await context3.resume();Felipe EriasIs this necessary to pass the test?
No, it is not necessary. I have removed it.
However, this test is now checking a behaviour that is a bit different from the original:
- before this CL, this test was calling `suspend()` on a context in the "running" state
- here, `suspend()` is called on a context that is still in the "suspended" state after creation, and will transition to "running" async.
Acknowledged
await context.resume();Felipe EriasDitto.
Same as above.
- Felipe Erias
- Michael Wilson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wilson (Gerrit)
Michael Wilson added 8 comments![]( "Open in Gerrit")
Thank you for writing this! I still need to review the tests, but want to give some initial feedback now.
pending_suspend_resolvers_;According to the spec, suspend promises are supposed to go into "pending promises" on the BaseAudioContext (https://webaudio.github.io/web-audio-api/#dom-baseaudiocontext-pending-promises-slot) and resume promises into both this queue and "pending resume promises" (https://webaudio.github.io/web-audio-api/#dom-audiocontext-pending-resume-promises-slot) on the AudioContext, but we only have `pending_promises_resolves_` on the current Chromium BaseAudioContext.
Furthermore, suspend and resume should both use the same "control message queue" (https://webaudio.github.io/web-audio-api/#queuing) but we don't have that concept implemented directly in Chromium.
In terms of the promise queues, from my reading of the spec we should actually do the opposite here: make a queue in AudioContext for the resume promises and store the suspend promises in the BaseAudioContext queue. However, this may have unintended side-effects if we try to implement it with the current Chromium state.
What do you think? If it would be simple to align the promise queues with the spec I would prefer that. However, the suspend transition bug is a big interop problem so if it would significantly delay landing I'm fine with looking at it in a follow-up.
void ScheduleTransitionToRunning();
void PerformTransitionToRunning();It looks like these functions are only used for the initial transition to running, not for subsequent calls to resume. Let's rename them to make that more clear.
```suggestion
void ScheduleInitialTransitionToRunning();
void PerformInitialTransitionToRunning();
```
// If resume() is called before the async transition to "suspended" runs,
// it will clear this flag to make that task a no-op.
suspended_by_user_ = true;Minor nit: the spec says this should happen after the promise is appended (https://webaudio.github.io/web-audio-api/#dom-audiocontext-suspend).
// Clear this flag to cancel any pending async transition to "suspended".
suspended_by_user_ = false;
Referencing https://webaudio.github.io/web-audio-api/#dom-audiocontext-resume, it looks like this should actually happen even earlier (after the closed check, but before the interrupted checks).
// Reject all pending suspend promises.
for (auto& resolver : pending_suspend_resolvers_) {
resolver->Reject(MakeGarbageCollected<DOMException>(
DOMExceptionCode::kInvalidStateError,
"AudioContext was closed before a pending suspend() could complete."));
}
pending_suspend_resolvers_.clear();
Should we guard access to `pending_suspend_resolvers_` here by the graph lock (DeferredTaskHandler::GraphAutoLocker locker(this);)? If not, can we add a comment explaining why?
// The state transition is scheduled to happen asynchronouslly```suggestion
// The state transition is scheduled to happen asynchronously
```
virtual V8AudioContextState state() const;Let's add a comment here too that this is `virtual` only to allow adding use counters to the derived AudioContext version of `state()`.
Felipe Erias (Gerrit)
Felipe Erias added 8 comments![]( "Open in Gerrit")
Thank you very much for your feedback.
pending_suspend_resolvers_;According to the spec, suspend promises are supposed to go into "pending promises" on the BaseAudioContext (https://webaudio.github.io/web-audio-api/#dom-baseaudiocontext-pending-promises-slot) and resume promises into both this queue and "pending resume promises" (https://webaudio.github.io/web-audio-api/#dom-audiocontext-pending-resume-promises-slot) on the AudioContext, but we only have `pending_promises_resolves_` on the current Chromium BaseAudioContext.
Furthermore, suspend and resume should both use the same "control message queue" (https://webaudio.github.io/web-audio-api/#queuing) but we don't have that concept implemented directly in Chromium.
In terms of the promise queues, from my reading of the spec we should actually do the opposite here: make a queue in AudioContext for the resume promises and store the suspend promises in the BaseAudioContext queue. However, this may have unintended side-effects if we try to implement it with the current Chromium state.
What do you think? If it would be simple to align the promise queues with the spec I would prefer that. However, the suspend transition bug is a big interop problem so if it would significantly delay landing I'm fine with looking at it in a follow-up.
You are correct, but unfortunately the change does not look simple.
Chromium currently uses separate promise queues for different actions, for example:
- `BaseAudioContext::pending_promises_resolvers_` for resume promises
- `BaseAudioContext::decode_audio_resolvers_` for decodeAudioData promises
This CL adds another one:
- `AudioContext::pending_suspend_resolvers_` for suspend promises
(It is in AudioContext because OfflineAudioContext implements suspend in its own way)
However, the spec describes the opposite arrangement:
- `BaseAudioContext::[[pending promises]]` should store all promises
- `AudioContext::[[pending resume promises]]` stores only resume promises
Implementing the spec would not be straightforward because much of the existing code expects the current structure, so I would rather tackle it as a follow-up.
void ScheduleTransitionToRunning();
void PerformTransitionToRunning();It looks like these functions are only used for the initial transition to running, not for subsequent calls to resume. Let's rename them to make that more clear.
```suggestion
void ScheduleInitialTransitionToRunning();
void PerformInitialTransitionToRunning();
```
Done
// If resume() is called before the async transition to "suspended" runs,
// it will clear this flag to make that task a no-op.
suspended_by_user_ = true;Minor nit: the spec says this should happen after the promise is appended (https://webaudio.github.io/web-audio-api/#dom-audiocontext-suspend).
Done. I've moved it after the promise is appended.
// Clear this flag to cancel any pending async transition to "suspended".
suspended_by_user_ = false;
Referencing https://webaudio.github.io/web-audio-api/#dom-audiocontext-resume, it looks like this should actually happen even earlier (after the closed check, but before the interrupted checks).
Done. I've moved to the beginning of the method, after the close check.
// Reject all pending suspend promises.
for (auto& resolver : pending_suspend_resolvers_) {
resolver->Reject(MakeGarbageCollected<DOMException>(
DOMExceptionCode::kInvalidStateError,
"AudioContext was closed before a pending suspend() could complete."));
}
pending_suspend_resolvers_.clear();
Should we guard access to `pending_suspend_resolvers_` here by the graph lock (DeferredTaskHandler::GraphAutoLocker locker(this);)? If not, can we add a comment explaining why?
Done. Other accesses to the same field use a lock so it makes sense to do it here as well. I've implemented that change.
// The state transition is scheduled to happen asynchronouslly```suggestion
// The state transition is scheduled to happen asynchronously
```
Done
virtual V8AudioContextState state() const;Let's add a comment here too that this is `virtual` only to allow adding use counters to the derived AudioContext version of `state()`.
Done.
```cpp
// Virtual so AudioContext::state() can add UseCounters.
virtual V8AudioContextState state() const;
```
Related details
- Michael Wilson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wilson (Gerrit)
Michael Wilson voted and added 9 comments![]( "Open in Gerrit")
Votes added by Michael Wilson
| Code-Review | +1 |
9 comments
LGTM overall, with a nit and some concern about potential test flakiness.
pending_suspend_resolvers_;Felipe EriasAccording to the spec, suspend promises are supposed to go into "pending promises" on the BaseAudioContext (https://webaudio.github.io/web-audio-api/#dom-baseaudiocontext-pending-promises-slot) and resume promises into both this queue and "pending resume promises" (https://webaudio.github.io/web-audio-api/#dom-audiocontext-pending-resume-promises-slot) on the AudioContext, but we only have `pending_promises_resolves_` on the current Chromium BaseAudioContext.
Furthermore, suspend and resume should both use the same "control message queue" (https://webaudio.github.io/web-audio-api/#queuing) but we don't have that concept implemented directly in Chromium.
In terms of the promise queues, from my reading of the spec we should actually do the opposite here: make a queue in AudioContext for the resume promises and store the suspend promises in the BaseAudioContext queue. However, this may have unintended side-effects if we try to implement it with the current Chromium state.
What do you think? If it would be simple to align the promise queues with the spec I would prefer that. However, the suspend transition bug is a big interop problem so if it would significantly delay landing I'm fine with looking at it in a follow-up.
You are correct, but unfortunately the change does not look simple.
Chromium currently uses separate promise queues for different actions, for example:
- `BaseAudioContext::pending_promises_resolvers_` for resume promises
- `BaseAudioContext::decode_audio_resolvers_` for decodeAudioData promises
This CL adds another one:
- `AudioContext::pending_suspend_resolvers_` for suspend promises
(It is in AudioContext because OfflineAudioContext implements suspend in its own way)
However, the spec describes the opposite arrangement:
- `BaseAudioContext::[[pending promises]]` should store all promises
- `AudioContext::[[pending resume promises]]` stores only resume promises
Implementing the spec would not be straightforward because much of the existing code expects the current structure, so I would rather tackle it as a follow-up.
Thank you for considering it, and I agree with your assessment. Let's land this change first.
// If resume() is called before the async transition to "suspended" runs,
// it will clear this flag to make that task a no-op.
suspended_by_user_ = true;Felipe EriasMinor nit: the spec says this should happen after the promise is appended (https://webaudio.github.io/web-audio-api/#dom-audiocontext-suspend).
Done. I've moved it after the promise is appended.
Acknowledged
// Clear this flag to cancel any pending async transition to "suspended".
suspended_by_user_ = false;
Felipe EriasReferencing https://webaudio.github.io/web-audio-api/#dom-audiocontext-resume, it looks like this should actually happen even earlier (after the closed check, but before the interrupted checks).
Done. I've moved to the beginning of the method, after the close check.
Acknowledged
// Reject all pending suspend promises.
for (auto& resolver : pending_suspend_resolvers_) {
resolver->Reject(MakeGarbageCollected<DOMException>(
DOMExceptionCode::kInvalidStateError,
"AudioContext was closed before a pending suspend() could complete."));
}
pending_suspend_resolvers_.clear();
Felipe EriasShould we guard access to `pending_suspend_resolvers_` here by the graph lock (DeferredTaskHandler::GraphAutoLocker locker(this);)? If not, can we add a comment explaining why?
Done. Other accesses to the same field use a lock so it makes sense to do it here as well. I've implemented that change.
Acknowledged
// Flag will be cleared if resume() was called after this task was scheduled.
if (!suspended_by_user_) {
for (auto& resolver : resolvers) {
resolver->Resolve();
}
return;
}
// Stop rendering now.
if (destination()) {
SuspendRendering();
}
for (auto& resolver : resolvers) {
resolver->Resolve();
}Nit: we could combine the conditions, although it may be less clear:
```suggestion
if (suspended_by_user_ && destination()) {
SuspendRendering();
}
for (auto& resolver : resolvers) {
resolver->Resolve();
}
``` virtual V8AudioContextState state() const;Felipe EriasLet's add a comment here too that this is `virtual` only to allow adding use counters to the derived AudioContext version of `state()`.
Done.
```cpp
// Virtual so AudioContext::state() can add UseCounters.
virtual V8AudioContextState state() const;
```
Acknowledged
await Promise.race([stateChange, timeout]);This works logically, but I'm worried that using this pattern here and in test-autoplay.html will result in flakiness for the tests.
Gemini suggested that we could refactor the tests to take an expected result state, and await the state change if it is running. It suggested we could look at the result from runMediaTests to see if we should be blocked or not.
But, I'm not sure how robust that is. Do we have a strong reason for selected 500 as the timeout?
await Promise.race([stateChange, timeout]);See other comment.
Related details
- Felipe Erias
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Erias (Gerrit)
Felipe Erias added 4 comments![]( "Open in Gerrit")
// Flag will be cleared if resume() was called after this task was scheduled.
if (!suspended_by_user_) {
for (auto& resolver : resolvers) {
resolver->Resolve();
}
return;
}
// Stop rendering now.
if (destination()) {
SuspendRendering();
}
for (auto& resolver : resolvers) {
resolver->Resolve();
}Nit: we could combine the conditions, although it may be less clear:
```suggestion
if (suspended_by_user_ && destination()) {
SuspendRendering();
}for (auto& resolver : resolvers) {
resolver->Resolve();
}
```
Good suggestion. I have updated the code like this:
```cpp
// Stop rendering unless resume() was called after this task was scheduled.
if (suspended_by_user_ && destination()) {
SuspendRendering();
}
for (auto& resolver : resolvers) {
resolver->Resolve();
}
``` await Promise.race([stateChange, timeout]);This works logically, but I'm worried that using this pattern here and in test-autoplay.html will result in flakiness for the tests.
Gemini suggested that we could refactor the tests to take an expected result state, and await the state change if it is running. It suggested we could look at the result from runMediaTests to see if we should be blocked or not.
But, I'm not sure how robust that is. Do we have a strong reason for selected 500 as the timeout?
I have updated the test to await the state change (if one is forthcoming) so we don't rely on a fixed timeout:
```js
async function runWebAudioTest() {
const audioContext = new AudioContext();
if (audioContext.state === 'running') {
results.webAudio = audioContext.state;
return;
}// When autoplay is allowed, we wait for the state transition to "running".
// When autoplay is blocked, no "statechange" event will be fired.
if (results.media?.name !== 'NotAllowedError') {
await new Promise(resolve => {
audioContext.onstatechange = resolve;
});
}
results.webAudio = audioContext.state;
}
```
await Promise.race([stateChange, timeout]);See other comment.
- Hongchan Choi
- Michael Wilson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Wilson (Gerrit)
Michael Wilson voted and added 4 comments![]( "Open in Gerrit")
Votes added by Michael Wilson
| Code-Review | +1 |
LGTM overall, with a nit and some concern about potential test flakiness.
Everything LGTM now, thank you!
// Flag will be cleared if resume() was called after this task was scheduled.
if (!suspended_by_user_) {
for (auto& resolver : resolvers) {
resolver->Resolve();
}
return;
}
// Stop rendering now.
if (destination()) {
SuspendRendering();
}
for (auto& resolver : resolvers) {
resolver->Resolve();
}Felipe EriasNit: we could combine the conditions, although it may be less clear:
```suggestion
if (suspended_by_user_ && destination()) {
SuspendRendering();
}for (auto& resolver : resolvers) {
resolver->Resolve();
}
```
Good suggestion. I have updated the code like this:
```cpp
// Stop rendering unless resume() was called after this task was scheduled.
if (suspended_by_user_ && destination()) {
SuspendRendering();
}for (auto& resolver : resolvers) {
resolver->Resolve();
}
```
Done
await Promise.race([stateChange, timeout]);Felipe EriasThis works logically, but I'm worried that using this pattern here and in test-autoplay.html will result in flakiness for the tests.
Gemini suggested that we could refactor the tests to take an expected result state, and await the state change if it is running. It suggested we could look at the result from runMediaTests to see if we should be blocked or not.
But, I'm not sure how robust that is. Do we have a strong reason for selected 500 as the timeout?
I have updated the test to await the state change (if one is forthcoming) so we don't rely on a fixed timeout:
```js
async function runWebAudioTest() {
const audioContext = new AudioContext();if (audioContext.state === 'running') {
results.webAudio = audioContext.state;
return;
}// When autoplay is allowed, we wait for the state transition to "running".
// When autoplay is blocked, no "statechange" event will be fired.
if (results.media?.name !== 'NotAllowedError') {
await new Promise(resolve => {
audioContext.onstatechange = resolve;
});
}
results.webAudio = audioContext.state;
}
```
Done
await Promise.race([stateChange, timeout]);Felipe EriasSee other comment.
Same.
- Felipe Erias
- Hongchan Choi
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongchan Choi (Gerrit)
Hongchan Choi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hongchan Choi
| Code-Review | +1 |
1 comment
LGTM
Since this is landing with experimental status, it’s good to know it won't impact general users yet. Before we move toward a stable launch, we should send out a PSA to gather feedback and ensure developers are aware of the transition.
This was a complex set of async state changes. Thanks for your contribution!
Related details
- Felipe Erias
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Erias (Gerrit)
Felipe Erias added 2 comments![]( "Open in Gerrit")
Thank you very much for help to get this CL ready.
Adding Rick for approval of the changes to `runtime_enabled_features.json5`. Thanks!
CONSOLE WARNING: The AudioContext was not allowed to start. It must be resumed (or created) after a user gesture on the page. https://developer.chrome.com/blog/autoplay/#web_audioFelipe EriasCan you explain why this CONSOLE WARNING is shown here?
This was caused by calling `resume()` inside `test-autoplay.html` `runWebAudioTests()`, which is not needed.
- Rick Byers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |