-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[AUDIO_WORKLET] Enable AW spinlocks, verify them in the browser tests #23729
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_WORKLET] Enable AW spinlocks, verify them in the browser tests #23729
Conversation
0b71d81
to
af3d551
Compare
test/test_browser.py
Outdated
def test_audio_worklet_emscripten_futex_wake(self): | ||
self.btest('webaudio/audioworklet_emscripten_futex_wake.cpp', expected='0', args=['-sAUDIO_WORKLET', '-sWASM_WORKERS', '-pthread', '-sPTHREAD_POOL_SIZE=2']) | ||
def test_audio_worklet_emscripten_locks(self): | ||
self.btest_exit('webaudio/audioworklet_emscripten_futex_wake.cpp', args=['-sAUDIO_WORKLET', '-sWASM_WORKERS', '-pthread', '-sPTHREAD_POOL_SIZE=2']) |
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.
Does this test actually start any pthreads? I.e. can we remove -sPTHREAD_POOL_SIZE
?
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’ll take a look tomorrow, I kept this from the original interactive test.
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.
We can remove -sPTHREAD_POOL_SIZE
but not -pthread
(otherwise _emscripten_thread_supports_atomics_wait
won't link).
Done, and also rebased to bring in the Firefox CI changes.
a0ca317
to
eed0ca2
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.
lgtm.
CC'ing @juj in case he has some input.
Performed as a separate step from #23729 to not lose the history. The C++ file had already been converted to C as part of the previous PR, this now updates the filename and its extension.
This was unmerged last year (#22995), visited again now the browser tests are running with audio. It's been moved over to
btest_exit()
and verified that it runs (and will only exit after all tests have run or asserted).The only change I'd make is to rename the original test file, which has grown beyond its original use. A
git mv
command won't survive multiple commits, so I want to either leave it until last, live with the resulting deletion, or rename as a separate PR.Fixes: #22962