Add async OfflineAudioContext startRendering, suspend and resume #412
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It is a temporary fix because this simplifies the node bindings. As long as OfflineContext rendering is synchronous, the bounds are not necessary. The tests fail however because we require OfflineAudioContext to be Send and Sync. To be dealt with later.
…d callbacks" This reverts commit 1c66068.
Only at the beginning, and when the context was suspended
|
/bench |
|
I'm ready for a first round of reviews! |
orottier
changed the base branch from
feature/offline-audiocontext-suspend
to
main
|
|
Ok cool! I will try to have a closer look this week, and (hopefully) try to see where this gets us with the bindings |
b-ma
reviewed
Dec 21, 2023
src/context/offline.rs
Outdated
| /// assert_eq!(buffer.number_of_channels(), 1); | ||
| /// assert_eq!(buffer.length(), 512); | ||
| /// ``` | ||
| pub async fn suspend_at(&self, suspend_time: f64) { |
There was a problem hiding this comment.
Maybe we could just call it suspend as there is no alternative signature
b-ma
reviewed
Dec 21, 2023
src/context/offline.rs
Outdated
| /// ``` | ||
| pub async fn suspend_at(&self, suspend_time: f64) { | ||
| let quantum = (suspend_time * self.base.sample_rate() as f64 / RENDER_QUANTUM_SIZE as f64) | ||
| .ceil() as usize; |
There was a problem hiding this comment.
should check the other conditions as well, cf. https://webaudio.github.io/web-audio-api/#dom-offlineaudiocontext-suspend, but maybe for another PR (this one may be a bit tricky "is less than or equal to the current time"
b-ma
reviewed
Dec 21, 2023
src/context/offline.rs
Outdated
| /// assert_eq!(buffer.number_of_channels(), 1); | ||
| /// assert_eq!(buffer.length(), 512); | ||
| /// ``` | ||
| pub fn suspend_at_sync<F: FnOnce(&mut Self) + Send + Sync + 'static>( |
There was a problem hiding this comment.
Same, maybe suspend_sync is clear enough?
|
Hey, except my small comments, look all good to me! I will try to implement the bindings, I let you know |
|
Thanks for the comments. I will address those, plus the leftover performance regression. |
This is slightly different from the spec, where it says we should throw if the suspend time is less than or equal to the current time. However, this is more or less the same because rendering happens so fast the user does not even know about the precise 'current time' anyway.
|
/bench |
|
|
Cool, the baseline performance regression is gone and even exceeds performance of the previous release. I guess due to ab48aff |
This was referenced Dec 28, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Todo:
syncinterface this way