Prepare public API for variable render quantum size #242
Conversation
This file contains hidden or 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
This comment was marked as outdated.
This comment was marked as outdated.
orottier
force-pushed
the
feature/prepare-api-for-variable-render-quantum-size
branch
from
5fa5bc8 to
20e6a6f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
orottier
commented
Nov 25, 2022
| @@ -75,9 +75,12 @@ impl AllocInner { | |||
|
|
|||
| /// Render thread channel buffer | |||
There was a problem hiding this comment.
AudioRenderQuantumChannel now has no public methods, except the Deref(Mut) to [f32]
|
orottier
commented
Jan 7, 2023
| @@ -135,10 +138,10 @@ impl AudioRenderQuantumChannel { | |||
| use std::ops::{Deref, DerefMut}; | |||
|
|
|||
| impl Deref for AudioRenderQuantumChannel { | |||
There was a problem hiding this comment.
This here is the motivation of the PR. We stop hardcoding the length of the render quantum to 128, but leave it unspecified (we return a slice, instead of an array).
This incurs a small performance hit (a percent, looking at the benchmarks) because more bound checks need to be performed at runtime.
However, this opens the way for the "dynamic render quantum size" feature without breaking the current API. With this PR merged, the RENDER_QUANTUM_SIZE (or, the value 128) is no longer present in the API surface
In preparation of 'variable render quantum size', it is important to never explicitly expose the buffer size in the public API. This has a small performance hit though because the new deref signature forces more bound checks at runtime. It is in the order of a few percent for some benchmarks.
In the future `RENDER_QUANTUM_SIZE` will not be a constant, but a property of the BaseAudioContext instead that is based on the `render_size_hint`
orottier
force-pushed
the
feature/prepare-api-for-variable-render-quantum-size
branch
from
f7b914c to
a5bcf86
Compare
|
|
Ok I think I see the idea, looks good to me! |
5 tasks
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.
I think we have settled on most of the public API surface. However in preparation of #217 I think one more modification must be made: remove the constant
RENDER_QUANTUM_SIZE.Relatest to #76 and #140
Might have performance impact. I will add more benchmarks