-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[AUDIO WORKLET] Support proper shutdown of audio node #25888
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
Open
lindell
wants to merge
7
commits into
emscripten-core:main
Choose a base branch
from
lindell:better-destroy
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+168
−2
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
00cc27d
POC of ensuring Audio worklet destroying
lindell 1d45ba9
Add emscripten_destroy_web_audio_node_async
lindell ab99713
Fixed so that the .data is checked instead of directly on the message
lindell 90d6bee
Add missing function def
lindell bfcc86b
Added interactive.test_audio_worklet_destroy_async test
lindell c5588c6
Fixed filename
lindell 1c19091
Added `this.stopped = false;` in contructor
lindell File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| #include <stdio.h> | ||
| #include <stdlib.h> | ||
| #include <emscripten.h> | ||
| #include <emscripten/webaudio.h> | ||
| #include <emscripten/threading.h> | ||
| #include <stdatomic.h> | ||
|
|
||
| #ifdef REPORT_RESULT | ||
| volatile int audioProcessedCount = 0; | ||
| int valueAfterDestroy; | ||
| #endif | ||
|
|
||
| EMSCRIPTEN_AUDIO_WORKLET_NODE_T node_id; | ||
|
|
||
| bool ProcessAudio(int numInputs, const AudioSampleFrame *inputs, int numOutputs, AudioSampleFrame *outputs, int numParams, const AudioParamFrame *params, void *userData) { | ||
| #ifdef REPORT_RESULT | ||
| ++audioProcessedCount; | ||
| #endif | ||
|
|
||
| // Produce noise in all output channels. | ||
| for(int i = 0; i < numOutputs; ++i) | ||
| for(int j = 0; j < outputs[i].samplesPerChannel*outputs[i].numberOfChannels; ++j) | ||
| outputs[i].data[j] = (rand() / (float)RAND_MAX * 2.0f - 1.0f) * 0.3f; | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| void observe_after_destroy(void * userData) { | ||
| printf("Expected processed count to be %d, was %d\n", valueAfterDestroy, audioProcessedCount); | ||
|
|
||
| #ifdef REPORT_RESULT | ||
| if (audioProcessedCount == valueAfterDestroy) { | ||
| printf("Test PASSED!\n"); | ||
| REPORT_RESULT(0); | ||
| } else { | ||
| printf("Test FAILED!\n"); | ||
| REPORT_RESULT(1); | ||
| } | ||
| #endif | ||
| } | ||
|
|
||
| void AudioWorkletDestroyed(void* userData) { | ||
| emscripten_out("AudioWorkletDestroyed"); | ||
| #ifdef REPORT_RESULT | ||
| valueAfterDestroy = audioProcessedCount; | ||
| #endif | ||
| emscripten_set_timeout(observe_after_destroy, 1000, 0); | ||
| } | ||
|
|
||
| void observe_after_start(void *userData) { | ||
| #ifdef REPORT_RESULT | ||
| if (audioProcessedCount == 0) { | ||
| printf("Test FAILED!\n"); | ||
| REPORT_RESULT(1); | ||
| } | ||
| #endif | ||
|
|
||
| emscripten_destroy_web_audio_node_async(node_id, &AudioWorkletDestroyed, 0); | ||
| } | ||
|
|
||
| // This callback will fire after the Audio Worklet Processor has finished being | ||
| // added to the Worklet global scope. | ||
| void AudioWorkletProcessorCreated(EMSCRIPTEN_WEBAUDIO_T audioContext, bool success, void *userData) { | ||
| if (!success) return; | ||
|
|
||
| emscripten_out("AudioWorkletProcessorCreated"); | ||
|
|
||
| // Specify the input and output node configurations for the Wasm Audio | ||
| // Worklet. A simple setup with single mono output channel here, and no | ||
| // inputs. | ||
| int outputChannelCounts[1] = { 1 }; | ||
|
|
||
| EmscriptenAudioWorkletNodeCreateOptions options = { | ||
| .numberOfInputs = 0, | ||
| .numberOfOutputs = 1, | ||
| .outputChannelCounts = outputChannelCounts | ||
| }; | ||
|
|
||
| // Instantiate the counter-incrementer Audio Worklet Processor. | ||
| node_id = emscripten_create_wasm_audio_worklet_node(audioContext, "counter-incrementer", &options, &ProcessAudio, 0); | ||
| emscripten_audio_node_connect(node_id, audioContext, 0, 0); | ||
|
|
||
| // Wait 1s to check that the counter has started incrementing | ||
| emscripten_set_timeout(observe_after_start, 1000, 0); | ||
| } | ||
|
|
||
| // This callback will fire when the audio worklet thread has been initialized. | ||
| void WebAudioWorkletThreadInitialized(EMSCRIPTEN_WEBAUDIO_T audioContext, bool success, void *userData) { | ||
| if (!success) return; | ||
|
|
||
| emscripten_out("WebAudioWorkletThreadInitialized"); | ||
|
|
||
| WebAudioWorkletProcessorCreateOptions opts = { | ||
| .name = "counter-incrementer", | ||
| }; | ||
| emscripten_create_wasm_audio_worklet_processor_async(audioContext, &opts, AudioWorkletProcessorCreated, 0); | ||
| } | ||
|
|
||
| uint8_t wasmAudioWorkletStack[4096]; | ||
|
|
||
| int main() { | ||
| EMSCRIPTEN_WEBAUDIO_T context = emscripten_create_audio_context(NULL); | ||
|
|
||
| emscripten_start_wasm_audio_worklet_thread_async(context, wasmAudioWorkletStack, sizeof(wasmAudioWorkletStack), WebAudioWorkletThreadInitialized, 0); | ||
|
|
||
| return 0; | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not use a shared memory location here to guarantee that the callback will never fire again once this function returns?
Then we would not need
emscripten_destroy_web_audio_node_asyncat all I think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that shared location will then be leaked memory? As it will never be able to tell when the last process callback will be (at least according to the WebAudio spec)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, interesting yes. What about this sequence of operations:
Assuming the audio worklet always make progress during the
emscripten_destroy_web_audio_nodeI think it should be OK with block like this. WDYT @cwoffenden ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type of blocking in the main thread should be fine, since we'd already be in the main thread, waiting on the AW, so at most it should be spinning for 3ms with the default quantum size of 128 samples (as this becomes adjustable this blocking time will increase).
It would need some testing to prove that spinning for 3ms doesn't cause Chrome to start delaying timeouts or frames, so I think in general the async approach is cleaner.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would very much prefer if we could avoid the callback here.
Assuming process will always be called afterwards, that should work. But is that really guaranteed to happen? Should at least not be when using an Offline Audio context?
Assuming we can guarantee it.
Do we even need the spinlock? If we set the shutdown bit, we can already then guarantee that the process callback wont be called. So there is no need to wait at that point?
As soon as the shutdown bit is set, we should be free to free any resource that is connected to the Process callback?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lindell I've never run timings between process callbacks so will take your word for it that they're unevenly spaced (compounding that the audio worklet's design is terrible 😀). I guess it depends on how often and where closing the audio device is expected, whether a 10ms spin is acceptable.
@sbc100 It's not something I'd think of using, nothing is ever released after acquiring by design (except clean-up of WebGL contexts on page unload, but that's more to do with a long standing issue of browsers keeping them around if the debugger is open). If I look at cross platform shipping code going back 20+ years, some of the
close()implementations are empty (which I guess makes me a cowboy, so ye-ha?!).Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cwoffenden https://ui.perfetto.dev is really useful when understanding how the code is running.
Here are some chrome traces of a (non emscripten) audio worklet:
playbacklatency hint (23ms between)process())0.3(seconds) as latency hint (190ms between)process())Demo page here: https://lindell.me/audio-context-demos/noise-generator.html
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbc100 Your suggestion would work from a free perspective (being sure you can free when the
shutdown_competedbit is set. I was referring to my modified suggestion which will not since the process callback might already be running.While a predictable 3ms spin might be acceptable, we can see this might take hundreds of milliseconds.
Furthermore, if the Audio Context is suspended, the process callback may never fire, causing the main thread to spinlock indefinitely (deadlock), or if we have a max timeout, cause a use after free when it resumes. Suspended audio contexts are simply no longer requesting audio through the audio graph and thus not calling the
processcallback on worklet nodes. While MessagePorts are still handled:https://lindell.me/audio-context-demos/suspended-messages.html
Similarly, with OfflineAudioContexts. Will only call
process, when a chunk of audio is actively requested. While MessagePorts will still be processed: https://lindell.me/audio-context-demos/offline-messages.htmlBecause we cannot guarantee the process scheduler's behavior, we cannot safely block the main thread waiting for it.
Reusing the existing API function without any new functions is definitely to preferred if it can be done without other consequences, which I do unfortunately not think we can do. We do not know
If we implement this on top of the existing API, hoping the spinlocking will be fine, and where users can expect the userData will not be used after the destroy. And then some time later realises that this will be unacceptable for some new user. We will not be able to reverse it without breaking existing uses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very valid point, suspended either explicitly or simply because the tab is backgrounded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there more information needed here?