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

Fix AUDIO_WORKLET + MODULARIZE #21958

Merged
merged 1 commit into from
May 17, 2024

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented May 16, 2024

Fixes: #21908

@sbc100
Copy link
Collaborator Author

sbc100 commented May 16, 2024

Verified fix using interactive tests

@sbc100 sbc100 requested review from kripken and juj May 16, 2024 19:22
@sbc100 sbc100 enabled auto-merge (squash) May 16, 2024 19:25
@kripken
Copy link
Member

kripken commented May 16, 2024

Can you please explain why renaming this makes a difference? I can't figure it out.

@MuTsunTsai
Copy link

@kripken Looks like the key is adding await in this line:

globalThis.Module = await AudioWorkletModuleFactory(Module);

@juj
Copy link
Collaborator

juj commented May 17, 2024

LGTM, but has there been a semantics change to how MODULARIZE works? The await there is surprising to me.

@sbc100
Copy link
Collaborator Author

sbc100 commented May 17, 2024

LGTM, but has there been a semantics change to how MODULARIZE works? The await there is surprising to me.

With MODULARIZE the constructor functions has always returned a Promise of a module (IIUC, thats the main difference between MODULARIZE and now removed MODULARIZE_INSTANCE).

There was a semantic change back in #21775 where the argument passed into the module constructor was not modified in-place.. this avoids leaking the module before the promise resolves.

@sbc100
Copy link
Collaborator Author

sbc100 commented May 17, 2024

Can you please explain why renaming this makes a difference? I can't figure it out.

Sorry the rename is unrelated. I will revert that.

@sbc100 sbc100 force-pushed the autoworklet_modularize branch 2 times, most recently from fb2b5f9 to fd3daca Compare May 17, 2024 16:03
The change that was made back in emscripten-core#21775 means that its no longer OK to
ignore the return value of the `MODULARIZE` constructor function.

Fixes: emscripten-core#21908
@sbc100
Copy link
Collaborator Author

sbc100 commented May 17, 2024

Hopefully all these code will be removed soon anyway once I figure out how to inline the ww.js and aw.js files like I did for the pthread worker.js

@sbc100
Copy link
Collaborator Author

sbc100 commented May 17, 2024

Simplified this PR by removing the rename. PTAL.

@kripken
Copy link
Member

kripken commented May 17, 2024

Thanks @MuTsunTsai , I missed that part among the rest of the diff...

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Can we add a test for this?

@sbc100 sbc100 merged commit 4cf3682 into emscripten-core:main May 17, 2024
29 checks passed
@sbc100 sbc100 deleted the autoworklet_modularize branch May 17, 2024 21:28
@sbc100
Copy link
Collaborator Author

sbc100 commented May 17, 2024

Can we add a test for this?

This failure was already showing up but only in the interactive tests... so it was already tests, just not by CI tests (that same goes for pretty much all our audio tests).

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.

AUDIO_WORKLET + MODULARIZE broken in 3.1.59
4 participants