-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[AUDIO_WORKLET] Wait for Wasm instance to start using audio worklet. #24778
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
base: main
Are you sure you want to change the base?
Conversation
Fixes intermittent test failures where the AudioWorklet was used before the Wasm module was loaded in MINIMAL_RUNTIME.
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.
How is this solved in the regular runtime?
Regular runtime instantiates on the worker synchronously. We could potentially do the same for minimal, but it will remove some of the shared instantiation code. |
@@ -92,6 +92,8 @@ var wasmExports; | |||
var wasmModule; | |||
#endif | |||
|
|||
var instantiatePromise; |
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.
How about declaring this in src/audio_worklet.js
Also, the instantiatePromise =
usage below should be wrapped in if AUDIO_WORKET
I guess? Although no code size tests were effected maybe because closure can delete this? Still best not to generate it in the first place though.. especially with MINIMAL_RUNTIME.
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'd prefer to leave it as is since adding more ifdefs is going to make this even more unreadable. Closure/rollup/webpack are all able to get rid of unused variables. Are people running minimal runtime without some minification step?
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.
IIRC the goal of minimal runtime is to have the default output be minimal, not depend on DCE steps
@juj, is that correct?
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.
Yes absolutely, it is definitely something that MINIMAL_RUNTIME should achieve: smallest output possible.
I can understand that the #ifdef jungle can look a bit messy, but it is worth it in MINIMAL_RUNTIME philosophy. Also the intent is that we should not rely on CLOSURE_COMPILER to optimize code in the "easy cases" that we can deal with ourselves. This is because not every user is able to utilize closure: it requires adhering to defining a heavier set of internal/external public code boundary with the externs model. So not all users can utilize it.
Note that Closure also has challenges of optimizing away variables at global scope.
Fixes intermittent test failures where the AudioWorklet was used before the Wasm module was loaded in MINIMAL_RUNTIME.