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

Audio Worklets #16449

Merged
merged 75 commits into from
Feb 5, 2023
Merged

Audio Worklets #16449

merged 75 commits into from
Feb 5, 2023

Conversation

juj
Copy link
Collaborator

@juj juj commented Mar 8, 2022

Add support for Wasm Audio Worklets, based on Wasm Workers.

Wasm Audio Worklets enables code to create Wasm-based Web Audio processing nodes, that run on a dedicated thread of their own. (all such nodes run sequentially on a single main audio thread, there won't be a single thread for each node)

Fix Chrome not continuing the AudioWorklet processing if a number 1 is returned from the callback - must return 'true' specifically.

Add new tone generator sample.

Adjust comment.

Use MessagePort.onmessage instead of add/removeEventListener(), since onmessage .start()s the MessagePort automatically.

Fix name noise-generator to tone-generator

Improve assertions.
@juj
Copy link
Collaborator Author

juj commented Mar 16, 2022

This PR has now been merged on top of latest main, and is good for final review and landing.

@juj juj enabled auto-merge (squash) March 16, 2022 14:26
@kripken
Copy link
Member

kripken commented Mar 25, 2022

What do you think about adding a doc page with an overview of the API and some simple examples? I think that would be useful in general for users later but also help in review.

How does this relate to WebAudio? I see webaudio.h also has methods related to that and not to audio worklets. I'm not very familiar with either WebAudio or AudioWorklets however so maybe that's something obvious, apologies if so.

Also please add a changelog entry.

@kripken
Copy link
Member

kripken commented Mar 25, 2022

cc @hoch , this might interest you (no worries if you don't have time, of course).

@tklajnscek
Copy link
Contributor

Hey @juj!

Finally got around to testing this out along with the general wasm workers support. Thanks for all the work on this!

The first realization I had was that with pure wasm workers none of the standard library code will work multithreaded (e.g. malloc will not be thread-safe etc) and we have quite a bit of code (from 3rd parties as well) that would need to change so for the foreseeable future pthreads is what we're stuck with.

Looking through the tests I realized we can run pthreads and wasm workers side by side so I set out to test running just the audio worklet bit as a wasm worker using this the code from this PR, but with pthreads enabled the JS doesn't load in an audio worklet context.

First it asserts, but removing that it tries running PThread.init and fails etc.

So I went and took this PR's branch, rebased it on latest main for ease of testing and made a few changes to skip over any remains of PThread code when not running in a PThread context.

Here's the commit - I can't do an automatically mergable PR to this branch because it's on a rebased branch:
tklajnscek@1824e25

This makes the simple example work.

I'll run some more complex setup and update here.

@tklajnscek
Copy link
Contributor

Here's quick status update...

I was able to make it work with a combination of pthreads for normal threads and wasm workers for the audio worklet code.

I used the synchronization primitives from wasm_worker.h to setup the communication between the audio worklet and the pthreads and it worked fine.

In this specific use case I had to get a middleware product ready for production so I actually trimmed down the worklet code so much that in the end the pthread part uses wasm_worker sync primitives in C++ code compiled to wasm like you'd expect, but the audio worklet part is pure javascript that uses Atomics.notify etc directly and it works fine and avoids a dependency on this PR in our deployment :)

I will schedule some time in the following weeks to look at our full blown audio playback engine in the context of this PR and to see what it would take to make the audio worklet code not depend on pthreads at all, but have some other priorities I need to deal with first (like always) :)

@juj
Copy link
Collaborator Author

juj commented Apr 12, 2022

What do you think about adding a doc page with an overview of the API and some simple examples? I think that would be useful in general for users later but also help in review.

Sure, I'll add a doc page. The added tests serve as simple examples, so pointing users to them to get started.

How does this relate to WebAudio? I see webaudio.h also has methods related to that and not to audio worklets. I'm not very familiar with either WebAudio or AudioWorklets however so maybe that's something obvious, apologies if so.

Audio Worklets are a subpart of Web Audio API. So in order to enable integration with Audio Worklets, we need to have at least a minimal integration/understanding of Web Audio AudioContext object with the PR. That is why I added a shallow shim that allows creating an AudioContext from C side, and then a C&JS mechanism to manage the Wasm AudioContext<->int opaque handle binding.

Also please add a changelog entry.

Will do, I'll address review once I get through my next round of bug PRs.

@@ -2358,7 +2358,7 @@ mergeInto(LibraryManager.library, {
" };\n" +
"} else " +
#endif
#if USE_PTHREADS
#if USE_PTHREADS && !AUDIO_WORKLET
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that just enabling AUDIO_WORKLET affects code not running in the worklet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When building with AUDIO_WORKLET enabled, we cannot unconditionally use performance.timeOrigin + performance.now(), since those don't exist in AudioWorklets.

In that case, we must use one of the forms below that feature check for presence of performance.now(), and only use that if available.

Line 2376 below will still implement the same behavior for non-Audio Worklet threads, so they get performance.timeOrigin + performance.now().

@juj
Copy link
Collaborator Author

juj commented Jan 26, 2023

CI is giving an error

em++: error: LLVM version for clang executable "C:/Users/circleci/emsdk/upstream/bin\clang.exe" appears incorrect (seeing "17.0", expected "16") [-Wversion-check] [-Werror]

upstream LLVM bumped recently?

@sbc100
Copy link
Collaborator

sbc100 commented Jan 26, 2023

CI is giving an error

em++: error: LLVM version for clang executable "C:/Users/circleci/emsdk/upstream/bin\clang.exe" appears incorrect (seeing "17.0", expected "16") [-Wversion-check] [-Werror]

upstream LLVM bumped recently?

Yup, updating to latest main should resolve those.

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.

Overall looks good aside from a few notes.

I am not really a user of these APIs so I don't have thoughts on the API usability aspect, but from all the feedback from people this seems useful so I have no concerns there.

src/library_webaudio.js Show resolved Hide resolved
src/shell.js Show resolved Hide resolved
@@ -90,6 +92,10 @@ var quit_ = (status, toThrow) => {
// Determine the runtime environment we are in. You can customize this by
// setting the ENVIRONMENT setting at compile time (see settings.js).

#if AUDIO_WORKLET
var ENVIRONMENT_IS_AUDIO_WORKLET = typeof AudioWorkletGlobalScope !== 'undefined';
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it makes sense to make this new env status be more like ENVIRONMENT_IS_PTHREAD? That is, parallel to the main environments, and not one of a disjoint set of the main states. Here is that code:

emscripten/src/shell.js

Lines 120 to 126 in 008017b

// Three configurations we can be running in:
// 1) We could be the application main() thread running in the main JS UI thread. (ENVIRONMENT_IS_WORKER == false and ENVIRONMENT_IS_PTHREAD == false)
// 2) We could be the application main() thread proxied to worker. (with Emscripten -sPROXY_TO_WORKER) (ENVIRONMENT_IS_WORKER == true, ENVIRONMENT_IS_PTHREAD == false)
// 3) We could be an application pthread running in a worker. (ENVIRONMENT_IS_WORKER == true and ENVIRONMENT_IS_PTHREAD == true)
// ENVIRONMENT_IS_PTHREAD=true will have been preset in worker.js. Make it false in the main runtime thread.
var ENVIRONMENT_IS_PTHREAD = Module['ENVIRONMENT_IS_PTHREAD'] || false;

If this is meant to be a new main environment then arguably it should go into the docs in var ENVIRONMENT in settings.js, in particular. But I think it's more like a pthread, a kind of internal state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ENVIRONMENT_IS_AUDIO_WORKLET is internal, but it can be useful for JS lib developers to access, to get an outlined short hand to be able to avoid having to type typeof AudioWorkletGlobalScope !== 'undefined' out in full in their own detection code.

I am not sure if it would make sense to have that be part of ENVIRONMENT, since it would break the Single Source of Truth rule. I.e. what would it mean to build with a mismatched

-sAUDIO_WORKLET -sENVIRONMENT=web
or
-sENVIRONMENT=web,audioworklet
command line?

With Web Workers I can see it being different that someone might want to build with pthreads disabled, but with -sENVIRONMENT=worker if they plan to run the whole code in a worker. But a similar "I'll do everything manually" deployment cannot happen (is not supported) with Wasm Audio Worklets.

Copy link
Member

@kripken kripken Feb 2, 2023

Choose a reason for hiding this comment

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

I think we might be saying the same thing. I agree that ENVIRONMENT_IS_AUDIO_WORKLET is useful for JS devs.

What I'm unsure about is line 117, below - we use ENVIRONMENT_IS_AUDIO_WORKLET there in the same way e.g. ENVIRONMENT_IS_NODE. Note that ENVIRONMENT_IS_PTHREAD is not present on line 117. I am asking whether we can treat ENVIRONMENT_IS_AUDIO_WORKLET more like ENVIRONMENT_IS_PTHREAD.

I guess it doesn't matter since audio worklets and SHELL have no overlap, but I think it would be simpler to have ENVIRONMENT_IS_AUDIO_WORKLET be like ENVIRONMENT_IS_PTHREAD.

Copy link
Collaborator Author

@juj juj Feb 3, 2023

Choose a reason for hiding this comment

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

Hmm, line 117 reads:

var ENVIRONMENT_IS_SHELL = !ENVIRONMENT_IS_WEB && !ENVIRONMENT_IS_NODE && !ENVIRONMENT_IS_WORKER && !ENVIRONMENT_IS_AUDIO_WORKLET;

Note that ENVIRONMENT_IS_PTHREAD is not present on line 117.

I think the reason that ENVIRONMENT_IS_PTHREAD is not present on that line is that ENVIRONMENT_IS_PTHREAD implies ENVIRONMENT_IS_WORKER, so testing !ENVIRONMENT_IS_PTHREAD there would be redundant.

I think it would be incorrect to omit && !ENVIRONMENT_IS_AUDIO_WORKLET on that check. I am thinking maybe someone would like to do a build that would work simultaneously on node.js and with audio worklets enabled, but they might not just ever initialize any audio worklets when they run against node.js. In such case, if && !ENVIRONMENT_IS_AUDIO_WORKLET was omitted there, then the Audio Worklet thread would incorrectly get ENVIRONMENT_IS_SHELL be true and likely execute code that will later fail.

If I now locally test just omitting && !ENVIRONMENT_IS_AUDIO_WORKLET from that check, I get test suite errors with

RuntimeError: Aborted(Assertion failed: shell environment detected but not enabled at build time.  Add 'shell' to `-sENVIRONMENT` to enable.)

so we would then have to enforce that people don't simultaneously target shell and audio worklets at the same time? I think that might be convenient to have, say, I might have a game JS file that would know to run in headless mode in node.js (e.g. for unit tests or a server), but then if the same js file is run in a browser, it would know to pick up webgl rendering and audio worklet enabled audio.

Copy link
Member

Choose a reason for hiding this comment

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

I see now, thanks. Makes sense!

@juj
Copy link
Collaborator Author

juj commented Feb 2, 2023

I noticed when merging with main that there were some regressions with compatibility against main. Updated with fixes to those, and fixed the test suites to work again. Would be be closer towards a merge? :)

emcc.py Outdated
@@ -2352,6 +2352,19 @@ def phase_linker_setup(options, state, newargs):
settings.WASM_WORKER_FILE = unsuffixed(os.path.basename(target)) + '.ww.js'
settings.JS_LIBRARIES.append((0, shared.path_from_root('src', 'library_wasm_worker.js')))

settings.SUPPORTS_GLOBALTHIS = settings.MIN_CHROME_VERSION >= 71 and settings.MIN_EDGE_VERSION >= 79 and settings.MIN_FIREFOX_VERSION >= 65 and settings.MIN_IE_VERSION == settings.TARGET_NOT_SUPPORTED and settings.MIN_SAFARI_VERSION >= 120100 # and settings.MIN_NODE_VERSION >= 120000
Copy link
Member

Choose a reason for hiding this comment

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

Rather than baking this into emcc.py, see

https://github.com/emscripten-core/emscripten/blob/main/tools/feature_matrix.py

The numbers can be in there and this place can call caniuse().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Check

@kripken
Copy link
Member

kripken commented Feb 2, 2023

Lgtm aside from the caniuse issue and the discussion around the comparison to ENVIRONMENT_IS_PTHREAD (but that discussion doesn't matter much).

@juj
Copy link
Collaborator Author

juj commented Feb 3, 2023

Lgtm aside from the caniuse issue and the discussion around the comparison to ENVIRONMENT_IS_PTHREAD (but that discussion doesn't matter much).

Updated code to utilize caniuse. On the ENVIRONMENT_IS_PTHREAD / ENVIRONMENT_IS_AUDIO_WORKLET part, I think the current code form is required.

… IN_TEST_HARNESS increased in size (added 'typeof ENVIRONMENT_IS_AUDIO_WORKLET&&ENVIRONMENT_IS_AUDIO_WORKLET)' code)
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.

lgtm!

Great to have this functionality, I know many have been waiting for it.

@juj juj merged commit 5402fc9 into emscripten-core:main Feb 5, 2023
@Jonathhhan
Copy link
Contributor

Thank you very much for implementing this. :)

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.

None yet

10 participants