Tonejs / Tone.js

A Web Audio framework for making interactive music in the browser.
https://tonejs.github.io
MIT License
13.41k stars 976 forks source link

fix attempt #2 for AudioBufferSourceNode stop time miscalculation, with updated tests #968

Closed marcelblum closed 2 years ago

marcelblum commented 2 years ago

2nd try at a fix for #912, with tests updated to use the corrected time reference. All tests now pass on Mac (though not on Windows but that's a separate can of worms that has nothing to do with the changes made in this pr, see comments at #942).

I still plan to come up with a test to repro the conditions that lead to #912, will commit that when I get a chance.

marcelblum commented 2 years ago

@tambien please hold off on merging this as I am finding other lookAhead/stop time miscalculation bugs I'd like to fix first. But I have a question for you. What is the desired handling of lookAhead when scheduling events in the future? Is it that the event occurs at the exact scheduled time regardless of lookAhead, or the event occurs at time + lookAhead? For example, if we have lookAhead = 0.25 and then player.start("+2"), should the start occur at 2.25secs in the future, or 2.0secs? And then how do we handle a scenario where the start time is slightly in the future but < lookAhead, like lookAhead = 0.5 and player.start("+0.25")? Should it start at 0.25secs, or 0.75secs, or round up to 0.5secs...?

tambien commented 2 years ago

@marcelblum thanks for continuing down this route. i'll hold off on merging for now.

Good question, the desired behavior for "+2" = "now + 2" = "currentTime + lookAhead + 2". So 'now' relative scheduling should include the lookAhead.

As far as the second question, if lookAhead = 0.5, then player.start("+0.25") should be scheduled for currentTime + 0.5 + 0.25.

marcelblum commented 2 years ago

Hey @tambien here's a better fix for #912. Turns out the original theory of it being a currentTime vs now() issue was a red herring, though that did help me solve some other timing issues by accident.

Anyway for #912 I realized it was only happening on repeated calls to player.restart() and when there are multiple player._activeSources. player.restart() was leading to a call to player._stop which indiscriminately sets all _activeSources to the same stop time. When there were multiple _activeSources with different delayed stop times due to lookAhead - such as when restart() is called rapidly at increments < lookAhead - this caused a pileup of increasingly erroneously delayed stops. My fix is to only stop the most recently created source on player.restart(), rather than stop all _activeSources (let me know if you think there might be a more elegant way to go about this).

marcelblum commented 2 years ago

I've added a test for this fix (fails on dev, passes on this branch)

tambien commented 2 years ago

Thank you! Really appreciate the contributions