Skip to content

[AUDIO_WORKLET] Fix spinlocks for audio worklets (and add tests) #22995

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

Closed
wants to merge 0 commits into from

Conversation

cwoffenden
Copy link
Contributor

@cwoffenden cwoffenden commented Nov 23, 2024

Fix for #22962 and adds tests:

  • adds tests for various spinlock cases
  • spinlocks fixed so they work in audio worklets
  • test for atomic wait exposed as a public function
  • test renamed to match its new use
  • test converted to C-only (like the other tests, plus it wasn't using C++ features)

(The git mv lost the old to new mapping at some point)

@cwoffenden cwoffenden force-pushed the cw-audio-tweaks-4 branch 2 times, most recently from b70f88c to 81bcaa2 Compare November 26, 2024 00:28
@cwoffenden cwoffenden force-pushed the cw-audio-tweaks-4 branch 8 times, most recently from cf23f0e to aa78b84 Compare December 10, 2024 17:43
@cwoffenden cwoffenden marked this pull request as ready for review December 10, 2024 17:43
@cwoffenden cwoffenden force-pushed the cw-audio-tweaks-4 branch 3 times, most recently from cbd6be7 to 2dcfb14 Compare December 11, 2024 04:11
@cwoffenden cwoffenden requested a review from sbc100 December 11, 2024 06:05
@tklajnscek
Copy link
Contributor

@sbc100 this looks like it's good to go?

@cwoffenden
Copy link
Contributor Author

Same with #22753, I'd still like to get this merged (and unlike #22753 this fixed something broken).

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Can you update the PR title? Perhaps something like "Fix testing of emscripten locks in audio worklets".

Perhaps you could add to the description why the existing test wasn't actually working?

@sbc100
Copy link
Collaborator

sbc100 commented Jan 2, 2025

Same with #22753, I'd still like to get this merged (and unlike #22753 this fixed something broken).

Sure this change LGTM if we split out or remove the API change (which makes this a test-only change).

Just to be clear when you say "fixed something broken" you mean the it fixes the testing.. not an actual fix in emscripten itself? Or am I missunderstanding?

@cwoffenden cwoffenden changed the title [AUDIO_WORKLET] Add compatible spinlock tests [AUDIO_WORKLET] Fix spinlocks for audio worklets (and add tests) Jan 13, 2025
@cwoffenden cwoffenden closed this Jan 14, 2025
@cwoffenden
Copy link
Contributor Author

cwoffenden commented Jan 14, 2025

Closed to recreate a new PR (since it had too many changes to keep track and undo, and GitHub closed it for me once I reset Git to the start of the changes).

New PR in #23729.

sbc100 pushed a commit that referenced this pull request Feb 25, 2025
…#23729)

- spinlocks fixed so they work in audio worklets
- adds tests for various spinlock cases
- test converted to C-only (still in a C++ file for now, see below)

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
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.

3 participants