SourcesQueueOutput can peek metadata from next source #812
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
channels and sample_rate currently return the old metadata on source boundaries, which is problematic for UniformSourceIterator, which checks metadata on span boundaries. This commit takes advantage of current_span_len's behavior to use it as a cheap "peek" to hopefully find out if we're done with the current source or not.
do_skip_duration's math was prone to rounding errors -- this commit shuffles the math around a bit so that the sample skip and the duration subtraction are each holding onto as much precision as possible.
The queue will now change samples and sample rate immediately on source boundaries that correctly report their current_span_len, so this test passes
unrelated to the rest of the PR, but since I'm fixing current_span_len, might as well do this too thanks @max-m
roderickvd
requested changes
Dec 10, 2025
Member
roderickvd
left a comment
roderickvd
left a comment
There was a problem hiding this comment.
Thanks a lot for tackling this one! Sorry for the late response. Here are a few points and questions for further improvement.
| #[inline] | ||
| fn current_span_len(&self) -> Option<usize> { | ||
| None | ||
| Some(self.data.len() - self.pos) |
Member
There was a problem hiding this comment.
The contract is that current_span_len returns the length of the span / buffer regardless of its current position. That's for size_hint to do.
| #[inline] | ||
| fn size_hint(&self) -> (usize, Option<usize>) { | ||
| (self.data.len(), Some(self.data.len())) | ||
| (self.data.len() - self.pos, Some(self.data.len() - self.pos)) |
Member
There was a problem hiding this comment.
SamplesBuffer could therefore also implement ExactSizeIterator. I know that's not directly related to this change, but see it also doesn't implement it currently.
| fn channels(&self) -> ChannelCount { | ||
| self.current.channels() | ||
| // current_span_len() should never return 0 unless the source is empty, so this is a cheeky hint | ||
| if self.current.current_span_len() != Some(0) { |
Member
There was a problem hiding this comment.
I like the cheekiness 😄
How much sense do you think it would make to add an is_empty helper method to Source?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #811
SourcesQueueOutput currently violates the span interface by returning incorrect values for sample_rate and channels while the iterator is positioned on a source boundary (i.e. at the end of one source, before the next source has started.).
This PR adjusts its sample_rate and channels implementations so that they use current_span_len == 0 as an admissible heuristic to check if the current source is empty. This means that it can obey the span interface contract as long as the sources are returning Some current_span_len values. (If they aren't, there's probably no way we can possibly obey that contract.)
In order to actually uncomment the
basictest for the queue, I also had to adjust theSamplesBufferimplementation to return a Some current_span_len value. This showed some unrelated rounding errors inSkipDuration, so I made unrelated adjustments to theSkipDurationmath to reduce rounding errors, just to keep tests passing.