-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[CI] Enable audio tests in Chrome #23665
Conversation
7c2c85f
to
065759d
Compare
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.
nice!
Does the test pass locally for you now?
Yes, running with the browser flags documented earlier (or having interaction). I'll test all of the parameterised tests tomorrow, I didn't check all variants. |
Now checked, and yes, all the parameterised I'll work my way through the other browser tests next week now. It would be useful to have #23659 merged (which doesn't affect anything other than my code) so I can move all the browser audio tests to use the same pattern.
emscripten/.circleci/config.yml Line 309 in 6fd5a1f
I removed EDIT: answering myself, it looks like there's no virtualised audio, given the assert: I'll add code to enumerate the hardware (if JS allows this). More notes:
|
The latest flags for Chrome I tried appear to work, with the AW tests for regular Chrome passing: For Firefox it's failing as expected since I've not added its fake audio flags ( |
command: | | ||
export EMTEST_BROWSER="/usr/bin/google-chrome $CHROME_FLAGS_BASE $CHROME_FLAGS_HEADLESS $CHROME_FLAGS_WASM $CHROME_FLAGS_NOCACHE" | ||
export EMTEST_BROWSER="/usr/bin/google-chrome $CHROME_FLAGS_BASE $CHROME_FLAGS_HEADLESS $CHROME_FLAGS_WASM $CHROME_FLAGS_NOCACHE $CHROME_FLAGS_AUDIO" |
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.
If we make this change should we also remove the EMTEST_LACKS_SOUND_HARDWARE
setting above?
I'm kind of inclined to make this change as its one separate PR, e.g. [CI] Enable audio testing on chrome
?
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 didn't have much time today so it was mostly experimenting and coming back to it, but:
- I'll remove the
EMTEST_LACKS_SOUND_HARDWARE
✅ - I'll rename to match its new purpose (I wasn't sure what I'd achieve when starting) ✅
- I'll disable all the wasm64 and 2GB tests with audio for now
Is it fine to always enable the dummy sound hardware and bypass the media interaction? I can't think what this would break (and where real sound hardware is present its used as a preference, at least from my manual testing).
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 imagine enabling dummy sounds hardware and bypass the interaction check should be fine for all out tests..
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 limited removing EMTEST_LACKS_SOUND_HARDWARE
to Chrome (FF I can't get to work, documented in the comments).
5483e24
to
f9a781b
Compare
Hmm, FF still broken. To do:
|
test/webaudio/audioworklet.c
Outdated
#ifdef TEST_AND_EXIT | ||
// We're in the test harness and exiting is via main_thread_tls_access() | ||
emscripten_exit_with_live_runtime(); | ||
#endif |
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 was thinking or a separate PR to enable audio testing CI.
PR1: enable audio testing in CI
PR2: fix audio worklet test so they fail correctly
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.
Sure, NP. I’ll see how I get on with FF on Monday but I might split it for Chrome first too (I set up a VM with the same Debian and FF and so far, even with no audio device, it doesn’t fail; left to try is headless).
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.
After getting an audio-less Debian VM set-up with all the fallback sound devices removed, I finally managed to recreate what Firefox does. It fails here playing manually, or the test times out otherwise:
emscripten/test/webaudio/audioworklet.c
Line 60 in f9a781b
audioContext.resume(); |
The promise calls neither the success nor failure route, it just silently fails and the audio remains permanently suspended. I'll call this a Firefox bug and keep the EMTEST_LACKS_SOUND_HARDWARE
(I'm satisfied that I've spent enough time going down all avenues and what remains is creating a repro in minimal JS for Firefox).
I'll just enable audio testing in CI for Chrome for now.
test/test_browser.py
Outdated
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) | ||
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'), '-DTEST_AND_EXIT'] + args) |
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.
Revert the -DTEST_AND_EXIT
part of this change
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'm just doing so (and moving it to the other PR). 😀
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.
Great!
Please update the PR description too
Tested locally, this doesn't break anything, whereas #23695 breaks the CI as expected. |
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.
LGTM with update PR description. Do you want to also remove the "Draft" status of this PR?
Funny, wasm64 tests like |
Ah.. good to know. I can take a look at |
There seems to be quite a few. I'll make a list tomorrow and mark them as |
They're parameterised so it's only |
test/test_browser.py
Outdated
@@ -3244,6 +3245,7 @@ def test_sdl2_mixer_wav(self, flags): | |||
# TODO: need to source freepats.cfg and a midi file | |||
# 'mod': (['mid'], 'MIX_INIT_MID', 'midi.mid'), | |||
}) | |||
@no_wasm64('TODO: SDL2 audio memory64') |
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 just submitted a PR on the SDL side: libsdl-org/SDL#12332
Can you use that URL as the string argument to no_wasm64
here.
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.
Done.
…udio works) (#23719) Now that the audio tests are running in CI (see #23665) this existing AudioWorklet/futex test can be moved from `interactive` to `browser`: https://github.com/emscripten-core/emscripten/blob/32832575e0d200afcccd1d892ff3555baf54bc83/test/webaudio/audioworklet_emscripten_futex_wake.cpp It's had the bare minimum to enable it to compile but is otherwise broken and therefore disabled. The issue is here #22962. A fix is [already done](https://github.com/cwoffenden/emscripten/tree/cw-audio-spinlock-fix) with more advanced lock tests, but needs this to land first to get things in the right order. To confirm the test is broken, comment out the `@disabled` in line 5518 and run with: ``` test/runner browser.test_audio_worklet_emscripten_futex_wake ``` It will fail with: ``` test.js:1003 Uncaught RuntimeError: Aborted(Assertion failed: emscripten_is_main_browser_thread(), at: /emsdk/emscripten/system/lib/pthread/emscripten_futex_wait.c,26,futex_wait_main_browser_thread) at abort (test.js:1003:11) at ___assert_fail (test.js:1945:7) at test.wasm:0x10a8 at test.wasm:0xf34 at test.wasm:0xa78 at WasmAudioWorkletProcessor.process (test.aw.js:113:34) ```
…tests) (#23659) This is splits out the test changes from #23508 adding an interactive parameter test: ``` test/runner interactive.test_audio_worklet_params_mixing ``` This test is also added to the browser tests now #23665 has merged, offering a single test that touches all the various structs and their offsets. When run as part of the `browser` tests it plays for 1 second, for interactive without the `TEST_AND_EXIT` macro it will play continuously. It also tidies a little the shared code (and related tests) in the process. Interactive tests had `_2gb` and `_4gb` options added (which will fail until #23508 lands, but they're not part of the CI). All features of these tests as interactive now work fully on Chrome, Firefox and Safari (Safari has issues with looping, but it doesn't affect the spirit of the test).
Step one of a fix for #23131 (the second part in #23695).
Enabled Chrome's
FakeAudioOutputStream
for the CI machines and bypassed the need for user interaction. The Chrome tests are now enabled with@requires_sound_hardware
.Many attempts at replicating the same functionally were tried with Firefox (details below) but audio would never run so its
EMTEST_LACKS_SOUND_HARDWARE
was left unchanged.