fix: correct Source trait semantics and span tracking bugs #831
Conversation
Clarify Source::current_span_len() returns total span length (not remaining), while size_hint() returns remaining samples. Fix multiple bugs in span boundary detection, seeking, and iterator implementations. Opportunistic fixes: - fix division by zero, off-by-one error, and zero case handling - prevent counter overflows - optimize vector allocations Mid-span seeking: best-effort tracking after try_seek() to detect span boundaries when seeking lands mid-span. Fixes #691
|
I did not fix the decoders because I had already done so with the infamous #786. |
roderickvd
force-pushed
the
fix/span-fixes-clean
branch
from
16c90ba to
7538971
Compare
Parameter updates (channels/sample_rate) occur only at span boundaries or during post-seek detection. Reset counters and enter a detection mode on try_seek (Duration::ZERO is treated as start-of-span).
Use detect_span_boundary and reset_seek_span_tracking to detect span boundaries and parameter changes.
|
Verifying span boundary behavior in the other sources, latest commits I also added:
w.r.t. the first, I wonder how many people use that, given it only ever worked correctly on mono sources. |
|
I want to take a final look at a couple of other sources, that may need to deal with mid-stream endings as well. In the meantime, feel free to review what we've got already, as I might apply the same patterns that we've already got. |
Document that Sources must emit complete frames and pad with silence when ending mid-frame, and ensure that decoders do so.
|
This thing is getting big but I think I've got it down now. Took me a good week's worth of evenings! I've left the intermediate commits intact so you can see my journey. I've been going back-and-forth to defend against the fact that when source iterators end mid-frame, multi-channel audio and queuing blows up. First I added defensive measures to "filter" sources that depend on proper frame alignment. Then, I removed it from the filters in favor of placing it with the "producer" sources: the decoders, mostly, and documenting the frame-alignment requirement. I think it's better this way. |
|
|
||
| // Check if duration has expired. | ||
| if self.remaining_duration < self.duration_per_sample { | ||
| self.silence_samples_remaining = |
There was a problem hiding this comment.
this breaks the promise of take duration, should it not instead pass on a modified last span accounting for the target duration?
There was a problem hiding this comment.
a similar thing could work for lineargainramp (there we would cut a span into two)
There was a problem hiding this comment.
Actually, thinking about it more, is the contract for TakeDuration that get you get:
a. precisely the duration you've asked for (zero-padded if necessary)
b. up to the duration you asked for, or less if the source is exhausted before that?
|
Indeed! And the sad part is that I expect that 90% of use cases will never use it. Two things I can think of:
Point 2 is easily done and I had already put in... well you know. Thanks for your review. |
|
Those perf. changes are indeed not insignificant, bummer. I would like to merge this sooner rather then later though (as it's not really isolated). How about we cut a release this week and then merge this after? That gives us until the next release to provide more performance either through 1 or 2. I feel confident we can land the experimental source architecture within half a year, so before the next release. I'm using it right now in a side project and I'm really happy with how that code looks. example: |
OK for me. |
2 participants
Clarify Source::current_span_len() returns total span length (not remaining), while size_hint() returns remaining samples of the entire iterator. Fix multiple bugs in span boundary detection, seeking, and iterator implementations.
Opportunistic fixes:
Mid-span seeking: best-effort tracking after try_seek() to detect span boundaries when seeking lands mid-span.
Fixes #691