-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add some debugging to audioworklet_emscripten_locks.c. NFC #24203
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
Add some debugging to audioworklet_emscripten_locks.c. NFC #24203
Conversation
I'm having some trouble getting this test to pass in emscripten-core#24190 and even having trouble getting to pass on my desktop.
Is it working fine without the changes, only failing with? I should be able to take a look tomorrow. IIRC, to run locally it needs the browser to have the same prefs/flags settings as I added for the CI, otherwise it won’t playback. Any of the non-interactive tests are designed to run without user clicks, so need the audio to automatically play. |
|
It seems to work in some situation such as when running in headless with But if I reload the test page after its done, then click to trigger playback, then it doesn't work. My guess is that it never works when the playback is triggered by clicking the on-screen button. |
|
OK to land this ? |
| case TEST_WAIT_ACQUIRE: | ||
| // Release here to acquire in process | ||
| emscripten_lock_release(&testLock); | ||
| if (!didUnlock) { |
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.
Can this be an assertion? IIUC this mode should only ever be reached once?
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.
No, it can actually be a hit continuously until the mode changes. i.e. if the worker is is non responsive then the main thread will just repeatedly call this block forever. We could probably improve the state machine but I'm not sure I want to start messing with this.
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.
Hmm, can it fail to acquire more than once? I guess maybe...
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.
emscripten_lock_release doesn't have a return code so it can't really fail. Releasing a lock you don't have I guess is just UB
But this test specifically has a button you can use to start the audio (I suppose in the event that it happens to be run interactively). Can you confirm that the test does now work when audio is started via the button click? Do you have any idea why that is? |
I'm testing it standalone, with the button, and it's working in the latest Emscripten ( I worked on the file standalone, so from the This spat out Since it expects to run in the test harness, and exits with Running as a test I'm doing: It does and will only run once, since it exits. All of the above works checking out from Emcripten's main. I didn't need to add the 10 second lock wait, it should be in a few frames at most, but I can imagine on a VM that might take some real wall-clock time. I never saw any issues with the multiple lock release that you added the guard around to run once, but don't mind fixing up the state machine so it only runs once and changes state. Testing on Mac, it works just fine for me also with your (With only To add, in the Emscripten generated HTML you only see limited debug output from the main thread: But in the console (on browsers supporting logging from other threads) you should see: |
|
Wow, thanks for investigating @cwoffenden. I will try to create some through repro steps today and report back to you. BTW my steps for running just a single test in the browser are this: When you run a single test the output always goes in If you have any more time perhaps you could try patching in #24190 to see if you can see what is going wrong with that test. I'll also try to report back with repro steps for the failure I think I am seeing on main. |
|
Following your steps, and with #24190 (and also from main) it runs fine for me. From the test runner I get: And from Chrome and FF I get the same results as earlier. I can reload the page without problem, serving from I'm out of time today, but what I'll try tomorrow is to add a button to reset the test, and fix up the state machine to only release the lock once. I'll add more logging whilst I'm in there. Send me whatever failure you have and I'll try to repro. I've built with and without optimisations, various other flags, all works for me. |
Well that is strange because this test is failing under chrome both locally and in CI with #24190. I wonder if its related to the version of chrome. I'm testing with chrome unstable and the CI uses chrome beta. Which version did you try? |
BTW that effect I observed looked like them main thread was being blocked somehow (the main loop was not firing) while the AW was doing |
Stable: Version 135.0.7049.115 (Official Build) (arm64). I can try with a beta tomorrow; what you you get with FF? |
|
Yes, I can't seem to reproduce the issue on main using FF. With chrome whenever I start the test using the button is fails: Here you can see the messages I added to main loop.. it prints a message every second until I press the button.. at which point I never see any more message from the main loop at all.. its like the main thread locks when the button is pressed. |
Hmm, odd. I'll take a look tomorrow. Does it work for you with Chrome releases? |
|
I found a solution the main thread blocking issue: Produce a few empty frames (5 seems to do it) before trying to do any interaction with the main thread. It seems like main the main thread blocks for the first few |
|
Does it block like this if run from the button? |
|
Yes, its is actually specifically when run from the button (and with DevTools open.. although I can' |
I'm having some trouble getting this test to pass in #24190 and even having trouble getting to pass on my desktop.