Fix Closure breaking AUDIO_WORKLET

[cwoffenden]

If building with -sAUDIO_WORKLET and --closure=1 the generated glue JS will minimise the property names passed to the worklet's message handler:

emscripten/src/audio_worklet.js

Line 179 in 9445d6a

p.postMessage({'_wsc': d['callback'], 'x': [d['contextHandle'], 1/*EM_TRUE*/, d['userData']] }); // "WaSm Call"

E.g. these are received as:

A: 3,
B: 1,
F: 0,
s: "noise-generator"

No sound will be generated as a result, since calls to emscripten_create_wasm_audio_worklet_processor_async() will never call the passed function.

The quick fix being to add the names to the literal notation for the message (as per the commit). It might be worthwhile renaming the property names to something shorter though whilst making the changes.

Compiling any of the webaudio tests with --closure will reproduce the error:

emcc -O2 --closure=1 -sAUDIO_WORKLET=1 -sWASM_WORKERS=1 -o index.html tone_generator.c

Links here for precompiled before and after. On the before test the 'Toggle playback' button doesn't get added because AudioWorkletProcessorCreated never gets called.

[juj]

Looks great. Something that seems odd is that the browser test suite does have several AudioWorklet tests that do enable Closure, e.g.

emscripten/test/test_browser.py

Lines 5313 to 5350 in a19f7df

	# Tests the AudioWorklet demo
	@parameterized({
	'': ([],),
	'memory64': (['-sMEMORY64', '-Wno-experimental'],),
	'with_fs': (['--preload-file', test_file('hello_world.c') + '@/'],),
	'closure': (['--closure', '1', '-Oz'],),
	'asyncify': (['-sASYNCIFY'],),
	'pthreads': (['-pthread', '-sPTHREAD_POOL_SIZE=2'],),
	'pthreads_and_closure': (['-pthread', '--closure', '1', '-Oz'],),
	'minimal_runtime': (['-sMINIMAL_RUNTIME'],),
	'minimal_runtime_pthreads_and_closure': (['-sMINIMAL_RUNTIME', '-pthread', '--closure', '1', '-Oz'],),
	'pthreads_es6': (['-pthread', '-sPTHREAD_POOL_SIZE=2', '-sEXPORT_ES6'],),
	'es6': (['-sEXPORT_ES6'],),
	'strict': (['-sSTRICT'],),
	})
	def test_audio_worklet(self, args):
	if '-sMEMORY64' in args and is_firefox():
	self.skipTest('https://github.com/emscripten-core/emscripten/issues/19161')
	self.btest_exit('webaudio/audioworklet.c', args=['-sAUDIO_WORKLET', '-sWASM_WORKERS'] + args)
	# Tests that audioworklets and workers can be used at the same time
	def test_audio_worklet_worker(self):
	self.btest('webaudio/audioworklet_worker.c', args=['-sAUDIO_WORKLET', '-sWASM_WORKERS'], expected='1')
	# Tests that posting functions between the main thread and the audioworklet thread works
	@parameterized({
	'': ([],),
	'closure': (['--closure', '1', '-Oz'],),
	})
	def test_audio_worklet_post_function(self, args):
	self.btest('webaudio/audioworklet_post_function.c', args=['-sAUDIO_WORKLET', '-sWASM_WORKERS'] + args, expected='1')
	@parameterized({
	'': ([],),
	'closure': (['--closure', '1', '-Oz'],),
	})
	def test_audio_worklet_modularize(self, args):
	self.btest_exit('webaudio/audioworklet.c', args=['-sAUDIO_WORKLET', '-sWASM_WORKERS', '-sMODULARIZE=1', '-sEXPORT_NAME=MyModule', '--shell-file', test_file('shell_that_launches_modularize.html')] + args)

Any chance you'd be able to double check why those tests didn't catch this issue? And/or add/modify the tests there so that they would start failing before this fix?

The fix itself looks solid.

[cwoffenden]

Any chance you'd be able to double check why those tests didn't catch this issue?

I'll take a look tomorrow (at a guess it silently fails, which is what the basic tests do).

[cwoffenden]

@juj I took a look at the tests you linked to and some of the individual parameterised tests fail to initialise the audio without this fix, e.g.:

test/runner browser.test_audio_worklet_closure
test/runner browser.test_audio_worklet_pthreads_and_closure
test/runner browser.test_audio_worklet_minimal_runtime_pthreads_and_closure

And other tests fail for other reasons, e.g., missing stackAlloc:

test/runner browser.test_audio_worklet_minimal_runtime

But running the original code with the test runner, none seems to flag as a failure. Though if I look at a complete run output I see that the worklet doesn't appear to be loaded for any of the tests:

test_audio_worklet_closure (test_browser.browser) ... cache:INFO: generating system asset: symbol_lists/ed0c6582c55ebab55869b56f918759080dad520a.json... (this will be cached in "/root/cache/symbol_lists/ed0c6582c55ebab55869b56f918759080dad520a.json" for subsequent builds)
cache:INFO:  - ok
127.0.0.1 - - [29/Aug/2024 15:40:47] code 404, message File not found: /root/project/test.aw.js

In the end I ran each test individually, passed --save-dir to the test runner and used emrun to look at each.

[sbc100]

In the end I ran each test individually, passed --save-dir to the test runner and used emrun to look at each.

FYI, when you run just a single test, the output is always saved in out/test, so you don't need --save-dir in this case.

[sbc100]

@juj I took a look at the tests you linked to and some of the individual parameterised tests fail to initialise the audio without this fix, e.g.:

This seems bad, I guess we need to improve the robustness of these test. Its hard I suppose because I'm not sure that we can actually run any actual, audio tests in CI (we run the browser tests with EMTEST_LACKS_SOUND_HARDWARE in CI).

[juj]

some of the individual parameterised tests fail to initialise

none seems to flag as a failure

Yikes, something has definitely gone wrong then. I'll try to look at this tomorrow on how to make this more robust. Was this Closure change enough to get those tests to pass for you?

[cwoffenden]

Was this Closure change enough to get those tests to pass for you?

The minimal runtime ones also fail, I’ll try to look at that tomorrow and roll it into this PR.

[juj]

I see there are multiple regressions to Audio Worklet usage from different PRs in the past months. I've got them fixed now in local tree, with interactive tests passing... I'll see what a good way to land these fixes will be.

[sbc100]

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?

[juj]

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.

[sbc100]

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?

[juj]

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.

[sbc100]

Fair enough, lgtm

[sbc100]

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.

[cwoffenden]

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.

[sbc100]

Move the trailing comment here up above too?

Also, should we use a leading underscore for all of these or none of them? @juj do you remember why you add the underscore?

[cwoffenden]

Isn't _wpn part of the message name so could clash? Same as _wpn, the worklet processor name.

There was already an existing x in there so I didn't add underscores.

I did tidy the comments though because they repeat the same thing.

[juj]

@juj do you remember why you add the underscore?

Commented in #22472 (comment)

[cwoffenden]

@juj Fixes also added for MINIMAL_RUNTIME (dependencies plus missing float heap view).

Tested manually but this works with combinations of Closure and minimal runtime.

[sbc100]

This change actually inspired me to do exactly the opposite here for pthreads: #22555

If we even manage to implement the single-file approach like #21701 for wasm workers then we can do that same here too.

[cwoffenden]

This change actually inspired me to do exactly the opposite here for pthreads: #22555

A better solution all round, sure.

[sbc100]

I'd love to see a test for this in browser_tests.py. Can we wait on this change until @juj has updated the audio_worklet tests to be more robust/accurate. IIUC they are currently passing erroneously despite some recent breakages.

lgtm though, @juj if you want to land this first thats fine with me to.

[juj]

This GitHub/CircleCI keeps on giving..

there was a test failure on build-test and test-browser-chrome due to the single offset converter test failure, which we know to be flaky.

So I opened the test page, and clicked on "Re-run failed tests" to rerun the test-browser-chrome suite again.

However that caused the test runner to re-run tons of other (already passing) suites, except that failing test-browser-chrome-suite that I wanted to rerun.

And now other suites are failing with HTTP 500 No Fun for You.

To me it seems that all relevant tests are passing, I'd be happy to land this PR.

[cwoffenden]

This GitHub/CircleCI keeps on giving..

Run it enough times and it finally all passed!