Skip to content
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

Fix pc.SoundInstance end event suspension #4234

Merged
merged 1 commit into from
May 11, 2022
Merged

Fix pc.SoundInstance end event suspension #4234

merged 1 commit into from
May 11, 2022

Conversation

rafern
Copy link
Contributor

@rafern rafern commented May 10, 2022

When the currentTime value of a SoundInstance is quickly changed, the sound can stop unintentionally. Setting currentTime first stops the playback, sets the playback offset and starts the playback again. Stopping is asynchronous but currentTime doesn't wait for the playback to be stopped, so multiple end events may be fired at once. To suspend end events, a flag is set to true so that the next end event is ignored, however, because multiple end events may happen at once, only one of the events is suspended, leading to the playback unintentionally being stopped. To fix this issue, a counter is used instead of a flag: increment when the next end event needs to be ignored, decrement when an event was ignored, don't do anything if the counter is 0

Tested on Firefox 100.0 Linux x64

Example project demonstrating bug: https://playcanvas.com/project/924909/overview/currenttime-bug

  1. Press O to play
  2. Quickly spam the left and right arrow to seek at the same time
  3. In the current version, playback will eventually stop. In the patch, playback will continue as normal

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@yaustar yaustar requested review from jpauloruschel and a team May 10, 2022 10:07
Copy link
Contributor

@jpauloruschel jpauloruschel left a comment

Choose a reason for hiding this comment

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

I really like this approach and the test project - very easy to verify the fix!

I'm happy to go ahead and merge this, but I do wonder if it makes sense to reset _suspendEndEvent to 0 at some point (maybe on play?) just to be safe and avoid SoundInstance to ever start playing with a != 0 value.

@rafern
Copy link
Contributor Author

rafern commented May 10, 2022

I really like this approach and the test project - very easy to verify the fix!

I'm happy to go ahead and merge this, but I do wonder if it makes sense to reset _suspendEndEvent to 0 at some point (maybe on play?) just to be safe and avoid SoundInstance to ever start playing with a != 0 value.

I think it's best not to do that because _suspendEndEvent will normally be > 0 when playing if, for example, you start playing immediately after stopping (like in the currentTime setter). Clearing it would reintroduce the bug and the end event handler should always be called anyway (unless there is a browser bug that cancels it somehow), so the counter will lead to zero eventually

@jpauloruschel jpauloruschel merged commit 5133027 into playcanvas:main May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants