Skip to content

Simplify AUDIO_WORKLET checks. NFC #24164

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

Merged
merged 1 commit into from
Apr 22, 2025
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Apr 21, 2025

I assume the idea here was that there would be an AUDIO_WORKLET=2 mode but that was never added.

@sbc100 sbc100 requested review from kripken and juj April 21, 2025 22:57
@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 21, 2025

@cwoffenden

@@ -137,9 +137,6 @@ var WASM_BINARY_FILE = '';
// name of the file containing the Wasm Worker *.ww.js, if relevant
var WASM_WORKER_FILE = '';

// name of the file containing the Audio Worklet *.aw.js, if relevant
var AUDIO_WORKLET_FILE = '';
Copy link
Member

Choose a reason for hiding this comment

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

Is this a separate change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, its just an internal setting that is no longer needed after this change.

I assume the idea here was that there would be an AUDIO_WORKLET=2 mode
but that was never added.
@sbc100 sbc100 requested a review from kripken April 22, 2025 00:45
@cwoffenden
Copy link
Contributor

I assume the idea here was that there would be an AUDIO_WORKLET=2 mode but that was never added.

I did useAUDIO_WORKLET=2 for the updated API I had in development, but I think it better to run the two side-by-side marking parts with __attribute__((deprecated)).

Removing the need for a custom AUDIO_WORKLET_FILE makes sense.

@juj
Copy link
Collaborator

juj commented Apr 22, 2025

I recall I would have originally intended AUDIO_WORKLET=2 to be used for an embedded mode where the file a.aw.js would be emdedded into the main JS (like with Wasm Workers). It is a tradeoff between CSP-eval policy safety to choose whether to embed the file, or have it separate.

But this change LGTM, I presume we can still add a AUDIO_WORKLET=2 mode later if we wanted to.

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 22, 2025

I recall I would have originally intended AUDIO_WORKLET=2 to be used for an embedded mode where the file a.aw.js would be emdedded into the main JS (like with Wasm Workers). It is a tradeoff between CSP-eval policy safety to choose whether to embed the file, or have it separate.

But this change LGTM, I presume we can still add a AUDIO_WORKLET=2 mode later if we wanted to.

Yup, that makes sense to me.

BTW, for wasm workers I'm looking at removed completely the separate .ww.js file now: #24163

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 22, 2025

Can someone lgtm?

@sbc100 sbc100 enabled auto-merge (squash) April 22, 2025 15:12
@sbc100 sbc100 merged commit 5ab7f5e into emscripten-core:main Apr 22, 2025
28 checks passed
@sbc100 sbc100 deleted the audio_worklet_2 branch April 22, 2025 17:40
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.

4 participants