Skip to content

Fix Closure breaking AUDIO_WORKLET #22472

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Sep 18, 2024
+25 −17
Merged

Fix Closure breaking AUDIO_WORKLET #22472

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e016ad3
Ensure Closure doesn't remove the property names
cwoffenden Aug 29, 2024
34dea82
Shortened property names
cwoffenden Sep 6, 2024
71f5a87
Added missing float view
cwoffenden Sep 6, 2024
92914d5
Added missing dependencies
cwoffenden Sep 6, 2024
0139b57
Tidied comments
cwoffenden Sep 6, 2024
db7028f
Clarify the idea of a 'key' message property
cwoffenden Sep 11, 2024
a5eb47f
Removed unused HEAP32
cwoffenden Sep 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions src/audio_worklet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,18 +168,22 @@ class BootstrapMessages extends AudioWorkletProcessor {
}
#endif
// Register a real AudioWorkletProcessor that will actually do audio processing.
registerProcessor(d['_wpn'], createWasmAudioWorkletProcessor(d['audioParams']));
// 'ap' being the audio params
registerProcessor(d['_wpn'], createWasmAudioWorkletProcessor(d['ap']));
#if WEBAUDIO_DEBUG
console.log(`Registered a new WasmAudioWorkletProcessor "${d['_wpn']}" with AudioParams: ${d['audioParams']}`);
console.log(`Registered a new WasmAudioWorkletProcessor "${d['_wpn']}" with AudioParams: ${d['ap']}`);
#endif
// Post a Wasm Call message back telling that we have now registered the
// AudioWorkletProcessor class, and should trigger the user onSuccess
// callback of the
// emscripten_create_wasm_audio_worklet_processor_async() call.
p.postMessage({'_wsc': d['callback'], 'x': [d['contextHandle'], 1/*EM_TRUE*/, d['userData']] }); // "WaSm Call"
} else if (d['_wsc']) {
// AudioWorkletProcessor, and should trigger the user onSuccess callback
// of the emscripten_create_wasm_audio_worklet_processor_async() call.
//
// '_wsc' is short for 'wasm call', using an identifier that will never
// conflict with user messages
// 'cb' the callback function
// 'ch' the context handle
// 'ud' the passed user data
p.postMessage({'_wsc': d['cb'], 'x': [d['ch'], 1/*EM_TRUE*/, d['ud']] });
} else if (d['_wsc']) {
Module['wasmTable'].get(d['_wsc'])(...d['x']);
};
}
Expand Down
22 changes: 13 additions & 9 deletions src/library_webaudio.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,12 @@ let LibraryWebAudio = {
},

#if AUDIO_WORKLET
// emscripten_start_wasm_audio_worklet_thread_async() doesn't use stackAlloc,
// etc., but the created worklet does.
emscripten_start_wasm_audio_worklet_thread_async__deps: [
'$_wasmWorkersID',
'$_EmAudioDispatchProcessorCallback'],
'$_EmAudioDispatchProcessorCallback',
'$stackAlloc', '$stackRestore', '$stackSave'],
emscripten_start_wasm_audio_worklet_thread_async: (contextHandle, stackLowestAddress, stackSize, callback, userData) => {

#if ASSERTIONS
Expand Down Expand Up @@ -249,14 +252,15 @@ let LibraryWebAudio = {
#endif

EmAudio[contextHandle].audioWorklet.bootstrapMessage.port.postMessage({
// '_wpn' == 'Worklet Processor Name', use a deliberately mangled name so
// that this field won't accidentally be mixed with user submitted
// messages.
_wpn: UTF8ToString(HEAPU32[options]),
audioParams,
contextHandle,
callback,
userData
// Deliberately mangled and short names used here ('_wpn', the 'Worklet
// Processor Name' used as a 'key' to verify the message type so as to
// not get accidentally mixed with user submitted messages, the remainder
// for space saving reasons, abbreviated from their variable names).
'_wpn': UTF8ToString(HEAPU32[options]),
'ap': audioParams,
'ch': contextHandle,
'cb': callback,
'ud': userData
Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes me a bit sad that we have to do this in the source code rather than have the minifier tool do it... but I guess we don't have a better way.

Do we have any codesize test for audioworklets that can means that code size improvement here? If not maybe we should add one.

I wonder if we should do this in the pthreads code too? @juj what do you think of this technique?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks fine to me.

In general end users should never interact/snoop these postMessage()s, so their format should be strictly internal to the implementation. The only thing here is to make sure we generate messages that are not easy to confuse with user-sent messages - which I think manually mangled names make pretty clear it won't be anything that a user might have written.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, IIUC, there is no difference between these mangled names and the existing _wsc and _wpn mangled names. Can we be consistent and use underscores either for all of them, or none?

Copy link
Collaborator

Choose a reason for hiding this comment

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

One difference was that the _wsc and _wpn fields were the "keys" of the data object, used to identify what the postMessaged struct was. These other fields are the satellite data, that are assumed to be present when that one key field matches. That is why I initially only mangled and underscored that one field to make it clear that "that's the key, others are satellite" disposition.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair enough, lgtm

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I'm still a little confused. It looks like _wsc and _wpn are both fields the same object that ap and ch are part of.

For example I see registerProcessor(d['_wpn'], createWasmAudioWorkletProcessor(d['ap'])); where they are both fields of d.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the importance of _wpn is that no other message is going to have such a property. There's a test, equivalent to

if (msg.data['_wpn']) {

I can imagine minified properties being called ap, which might trigger a false positive, but it's unlikely there'd be a _wpn.

So my take, I can see reasoning for the underscore here, but for anything else it's just extra characters.

});
},

Expand Down
2 changes: 1 addition & 1 deletion src/runtime_shared.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
if (!shouldExport) {
if (MODULARIZE && EXPORT_ALL) {
shouldExport = true;
} else if (AUDIO_WORKLET && (x == 'HEAP32' || x == 'HEAPU32')) {
} else if (AUDIO_WORKLET && (x == 'HEAPU32' || x == 'HEAPF32')) {
// Export to the AudioWorkletGlobalScope the needed variables to access
// the heap. AudioWorkletGlobalScope is unable to access global JS vars
// in the compiled main JS file.
Expand Down
Loading