fix(moq-mux): codec/container correctness fixes from #1918 review (#1923)#1925
Conversation
) Five pre-existing findings in the moq-mux codec/container lineage, surfaced by CodeRabbit's review of the #1918 backport. Each gets a focused fix plus a regression test. - h264/h265 split: a bare IDR arriving right after a delta picture in the same decode chunk folded both access units into one frame (and mis-flagged it a keyframe). The IDR arm now closes an already-open slice on the first slice of a new picture, mirroring the non-keyframe slice path. - aac: AudioSpecificConfig is bit-packed, so an explicit (index 15) 24-bit sample rate pushes channelConfiguration off byte boundaries. The old byte-aligned parser misread both fields. Parse via an MSB-first bit reader. - container/consumer: a group read error was always treated as a relay eviction and skipped. A malformed-payload decode error is not an eviction; it now propagates. The group's own terminal state (poll_finished) is the source of truth: an aborted/evicted group reports the transport error, while a decode failure leaves the group live or cleanly finished. - container/fmp4: a multi-sample fragment whose non-final samples carry no duration left every following sample stuck at the same DTS, silently collapsing timestamps. decode() now rejects such fragments, and the latency-batching producer backfills every batched sample's duration (from the next sample in decode order, or the rolling keyframe for the last) so CMAF batches stay decodable. Frames that already carry a duration keep it. - catalog/filter + catalog/target: after upstream EOF the streams retained the final snapshot and parked in Pending forever. They now end with upstream once the final filtered/selected snapshot is emitted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 26 minutes and 57 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThe PR updates EOF handling in catalog 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rs/moq-mux/src/catalog/target.rs (1)
143-158: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake EOF terminal after the final selected snapshot.
Like
Filter,Targetkeepslast_inputafter EOF. Afterpoll_nextreturnsNone, a later retarget can reuse that cached catalog and produceSome(...)again. Clear the cached input when EOF is reached/finalized, and add a post-EOF retarget assertion.Suggested fix direction
Poll::Ready(Ok((emit, epoch))) => { self.last_epoch = epoch; self.fresh_input = false; + if inner_eof { + self.last_input = None; + } Poll::Ready(Ok(Some(emit))) } @@ if inner_eof { + self.last_input = None; + self.fresh_input = false; Poll::Ready(Ok(None)) } else { Poll::PendingAlso applies to: 416-418
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/catalog/target.rs` around lines 143 - 158, Make EOF terminal in Target::poll_next by clearing the cached input when the stream reaches final EOF, so a later retarget cannot reuse stale catalog state and emit items again. Update the Target polling logic around the Poll::Ready(Ok(None)) path (and any finalize/EOF handling nearby) to drop last_input/fresh_input once EOF is determined, then add an assertion in the retarget path to verify retargeting after EOF does not reopen the stream.rs/moq-mux/src/catalog/filter.rs (1)
131-143: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear the cached snapshot when EOF becomes terminal.
last_inputremains populated after the final emit/EOF path. A laterset_videoorset_audiocan makestate_consumer.pollreturnReadyand re-emitSome(...)after this stream already returnedNone, which violates the catalogStreamEOF contract. Please clear the cached input when the final snapshot has been emitted or when returning EOF, and extend this test with a post-EOF filter change.Suggested fix direction
Poll::Ready(Ok((emit, epoch))) => { self.last_epoch = epoch; self.fresh_input = false; + if inner_eof { + self.last_input = None; + } Poll::Ready(Ok(Some(emit))) } @@ if inner_eof { + self.last_input = None; + self.fresh_input = false; Poll::Ready(Ok(None)) } else { Poll::PendingAlso applies to: 299-307
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-mux/src/catalog/filter.rs` around lines 131 - 143, The EOF handling in the catalog filter stream leaves the cached input state alive after the final emit/terminal None path, so later updates can cause `state_consumer.poll` to produce another item after EOF. Update the stream logic in the filtering path that handles `Poll::Ready(Ok((emit, epoch)))`, `Poll::Pending`, and the `inner_eof`/`Poll::Ready(Ok(None))` branches to clear the cached snapshot/input state once the final snapshot has been emitted or when returning EOF, and make sure `set_video`/`set_audio` cannot revive output after termination. Also extend the existing EOF test around the filter stream to cover a post-EOF filter change and verify no additional item is emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-mux/src/codec/aac/mod.rs`:
- Around line 77-81: `Config::parse` is draining the remaining AAC config bytes,
which makes the trailing-byte validation in `derive_from_codec_config`
ineffective. Update `Config::parse` to stop consuming unread bytes so callers
can detect malformed init data, or if the new contract intentionally ignores
extras, remove/rework the `AacTrailingBytes` check to match that behavior. Keep
the fix centered around `Config::parse` and `derive_from_codec_config`.
In `@rs/moq-mux/src/container/consumer.rs`:
- Around line 1371-1389: The async test decode_error_propagates relies on
tokio::time::timeout, so it must explicitly pause Tokio time before any timed
await. Add tokio::time::pause() at the start of the test, before creating the
Consumer or calling consumer.read(), so the timeout behavior is deterministic
and consistent with the async test guidelines.
---
Outside diff comments:
In `@rs/moq-mux/src/catalog/filter.rs`:
- Around line 131-143: The EOF handling in the catalog filter stream leaves the
cached input state alive after the final emit/terminal None path, so later
updates can cause `state_consumer.poll` to produce another item after EOF.
Update the stream logic in the filtering path that handles
`Poll::Ready(Ok((emit, epoch)))`, `Poll::Pending`, and the
`inner_eof`/`Poll::Ready(Ok(None))` branches to clear the cached snapshot/input
state once the final snapshot has been emitted or when returning EOF, and make
sure `set_video`/`set_audio` cannot revive output after termination. Also extend
the existing EOF test around the filter stream to cover a post-EOF filter change
and verify no additional item is emitted.
In `@rs/moq-mux/src/catalog/target.rs`:
- Around line 143-158: Make EOF terminal in Target::poll_next by clearing the
cached input when the stream reaches final EOF, so a later retarget cannot reuse
stale catalog state and emit items again. Update the Target polling logic around
the Poll::Ready(Ok(None)) path (and any finalize/EOF handling nearby) to drop
last_input/fresh_input once EOF is determined, then add an assertion in the
retarget path to verify retargeting after EOF does not reopen the stream.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77277292-d2a4-4679-a4c6-6304c6470e14
📒 Files selected for processing (9)
rs/moq-mux/src/catalog/filter.rsrs/moq-mux/src/catalog/target.rsrs/moq-mux/src/codec/aac/mod.rsrs/moq-mux/src/codec/h264/split.rsrs/moq-mux/src/codec/h265/split.rsrs/moq-mux/src/container/consumer.rsrs/moq-mux/src/container/fmp4/import_test.rsrs/moq-mux/src/container/fmp4/mod.rsrs/moq-mux/src/container/producer.rs
- catalog filter/target: make EOF terminal. Once the final filtered/selected snapshot is emitted and upstream has ended, drop the retained input so a later set_video/set_audio can't revive the stream after it returned None (the Stream once-None-always-None contract). The retarget-after-snapshot test now uses a still-live upstream to keep exercising the live retarget path. - msf consumer: drop the unreachable AacTrailingBytes check (and its unused error variant). AudioSpecificConfig carries valid variable-length SBR/PS extensions after the core fields, so Config::parse intentionally drains the buffer; the trailing-byte check could never fire (the parser drained before this PR too) and wrongly implied HE-AAC configs are malformed. - consumer test: pause Tokio time in decode_error_propagates (uses timeout). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the two outside-diff CodeRabbit findings (Filter/Target EOF) in a784c17:
The two inline comments (AAC trailing-bytes, test (Written by Claude) |
1 participant
Fixes the deferred findings in #1923 — pre-existing moq-mux codec/container bugs surfaced by CodeRabbit's review of the #1918 backport. Each finding gets a focused fix and a regression test.
Findings & fixes
codec/h264/split.rs+codec/h265/split.rs— consecutive bare IDRs merged. A bare IDR arriving right after a delta picture in the samedecodechunk folded two access units into one frame (and mis-flagged it a keyframe), because the IDR arm never closed an already-open slice on the first slice of a new picture. It now does, mirroring the non-keyframe slice path'sfirst_mb_in_slice/first_slice_segment_in_pic_flagcheck.codec/aac/mod.rs— explicit (index 15) sample rate misparsed. AudioSpecificConfig is bit-packed; an explicit 24-bit sample rate pusheschannelConfigurationoff byte boundaries, so the byte-aligned parser decoded bothsample_rateandchannel_countwrong. Rewritten over an MSB-first bit reader. The previously-impossible explicit-rate round-trip now passes.container/consumer.rs— decode errors treated as evictions. Anypoll_readerror skipped the group as if it had aged out of the relay cache. A malformed-payload decode error is not an eviction and now propagates. The moq_net group's own terminal state (poll_finished) is the source of truth: an aborted/evicted group reports the transport error, while a decode failure leaves the group live or cleanly finished.container/fmp4/mod.rs+container/producer.rs— duration-less multi-sample fragments lost timestamps. A CMAF fragment with several samples where the non-final samples carried no duration decoded them all at the same DTS. Per the issue's two-pronged suggestion (chosen in discussion):decode()now rejects such fragments (MissingSampleDuration) instead of silently collapsing, and the latency-batchingProducerbackfills every batched sample's duration (gap to the next sample in decode order, or the rolling keyframe for the last) so CMAF batches stay decodable end-to-end. Frames that already carry a duration (e.g. fMP4 passthrough) keep it; a backwards gap (B-frame) is left unset. This changes CMAF encoded bytes for the batched-from-duration-less-source path, which is the intended fix. Related to Support negative / signed presentation timestamps (audio priming, pacing, B-frame reorder) #1917 signed-timestamp work.catalog/filter.rs+catalog/target.rs— never returned EOF. After upstream EOF the streams kept the final snapshot and parked inPendingforever. Decision (per discussion): end with upstream — once the final filtered/selected snapshot is emitted andinneris exhausted, the stream finishes.Public API changes
moq_mux::container::fmp4::Errorgains aMissingSampleDurationvariant. The enum is#[non_exhaustive], so this is additive.pubshapes changed.Behavior changes (not API)
Producerfrom a duration-less source now carry per-sample durations (byte change vs. before; required so they decode correctly).catalog::Filter/catalog::Targetnow end when their upstream ends, rather than staying alive for post-EOF retargeting.Test plan
cargo test -p moq-mux(306 passing, incl. 10 new)just rs check,just js check,remarkmarkdown lintThe remaining
just checkfailures in my environment are confined to the worktree's git-ignored.direnv/generated flake-input sources (shellcheck/taplo), not this change.Targets
mainsince the #1918 backport landed there; reviewers can redirect todevif preferred.🤖 Generated with Claude Code
(Written by Claude)