fix(moq-mux): carry all distinct SPS/PPS/VPS through transmux, not just the last seen#1812
Conversation
A real broadcast TS round-tripped through moq-cli (publish ts, subscribe
--format ts) produced undecodable H.264: ffplay reported "non-existing PPS 0
referenced" continuously. The source defines two PPS (ids 0 and 1) and ~97% of
slices reference PPS 0, but the transmux kept a single parameter set each, so
only the last-seen PPS (id 1) survived.
avcC/hvcC store SPS/PPS (and VPS) as count-prefixed arrays, and slices reference
them by id, so a single-slot cache is wrong. Keep an ordered set of distinct
parameter-set NALs and emit/re-inject all of them:
- Avc1/Hvc1 transforms accumulate distinct SPS/PPS (VPS too); build_avcc and
build_hvcc emit numOf{Sequence,Picture}ParameterSets > 1.
- The avc3/hev1 importers re-inject every cached parameter set not already
inline in a keyframe AU, with per-AU seen tracking, instead of just the most
recent one.
The mid-stream reconfiguration reset (resolution/profile change) is preserved
via a reconfigured signal so a genuine config change still drops stale params.
This changes inline keyframe bytes and the synthesized avcC/hvcC only, not the
wire or catalog format, so it is additive and non-breaking. Covers issue #1798
work item 1; the DTS and opaque-carriage items are separate and out of scope.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis change extends H.264 (avc1/avc3) and H.265 (hvc1) codec parameter-set handling from single cached values ( 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
… entries The first pass accumulated every distinct parameter set forever, so a mid-stream reconfiguration left superseded SPS/PPS in the avcC/hvcC and re-injection. Scope the active set to the latest keyframe instead: the set a keyframe presents is the active set, and a keyframe presenting a different set reinits, dropping the ones the new GOP no longer uses. - Avc1/Hvc1 transforms collect each frame's parameter sets and, when a frame carries any (a keyframe), adopt them per type (replace, not accumulate), rebuilding the avcC/hvcC only when the set actually changes. - The avc3/hev1 importers reconcile at each keyframe: adopt the inline set when present (dropping deprecated), else re-inject the retained set for tune-in. - ExportSource::refresh_description now tracks the transform's record after it is first set, so a reinit's new avcC/hvcC reaches the muxer's per-keyframe re-injection instead of a stale one. Single-keyframe multi-PPS (the original #1798 case) is unchanged: both PPS in one keyframe are still kept. Added tests for dropping a superseded PPS on reinit in both the transform and the importer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/h265/import.rs`:
- Around line 233-241: When H.265 reconfiguration is detected at the SPS stage
in the reconfigured block, the code incorrectly clears current.vps_seen which
loses the new VPS that was already collected in the current AU before the SPS
arrived. Remove the line that clears current.vps_seen (line 237) while keeping
the other clear() calls, since VPS precedes SPS in H.265 NAL order unlike H.264
where SPS is the first parameter set. This preserves the newly seen VPS so it
can be properly injected by reconcile_keyframe_params when the IDR frame is
processed.
🪄 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: 8c03ff66-68c1-444e-800d-cf64c39dfb50
📒 Files selected for processing (7)
rs/moq-mux/src/codec/annexb.rsrs/moq-mux/src/codec/h264/import.rsrs/moq-mux/src/codec/h264/mod.rsrs/moq-mux/src/codec/h265/import.rsrs/moq-mux/src/codec/h265/mod.rsrs/moq-mux/src/container/source.rsrs/moq-mux/src/container/ts/export_test.rs
In H.265 the VPS precedes the SPS, so a reconfiguration's new VPS is collected into the current access unit before the reconfiguring SPS arrives. The reset on reconfiguration cleared current.chunks and vps_seen, dropping that VPS, so reconcile_keyframe_params had nothing to inject and the keyframe went out without a VPS (decoder failure). H.264 is unaffected because SPS is the first parameter set. Keep vps_seen across the reset and re-append it to the cleared chunks so the new VPS survives into the keyframe. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Fixes moq-dev/moq#1798 (work item 1).
Publishing a real broadcast MPEG-TS feed and subscribing back with
--format tsproduced undecodable H.264: ffplay/ffprobe reportednon-existing PPS 0 referenced/no frame!continuously. The source defines two PPS (ids 0 and 1) and ~97% of slices reference PPS 0, but the H.264/H.265 transmux held a single parameter set each, so only the last-seen PPS (id 1) survived the round-trip.avcC/hvcCstore SPS/PPS (and VPS for HEVC) as count-prefixed arrays, and slices reference them by id, so a single-slot cache is wrong by construction. This keeps an ordered set of parameter-set NALs and emits/re-injects all of them.What changed
Avc1/Hvc1transforms (h264/mod.rs,h265/mod.rs): keep the parameter sets carried by a keyframe and emit them all.build_avcc/build_hvccnow emitnumOf{Sequence,Picture}ParameterSets > 1(and multi-NAL hvcC arrays). The egressavcc_params/hvcc_paramsalready loop over the counts, so TS re-injection needed no further change.h264/import.rs,h265/import.rs): re-inject every parameter set the keyframe references for mid-stream tune-in, with per-AU "seen" tracking.push_distinct/reconcile_keyframe_paramshelpers added toannexb.rs.Reinit: dropping superseded parameter sets
The active parameter set is scoped to the latest keyframe rather than accumulated for the life of the stream:
ExportSource::refresh_descriptionnow tracks the transform's record after it is first set, so a reinit's new avcC/hvcC reaches the muxer's per-keyframe re-injection instead of a stale one.So a mid-stream reconfiguration (resolution change, a PPS dropped from the rotation) drops the superseded entries instead of letting them linger. A single keyframe carrying multiple PPS (the original #1798 case) still keeps all of them.
Why
mainThis changes inline keyframe bytes and the synthesized
avcC/hvcConly, not the wire or catalog format (avcC/hvcCcarrying multiples is standard), so it is additive and non-breaking.Out of scope
Issue items 2 (proper DTS / B-frame timing) and 3 (opaque MPEG-TS carriage) are larger,
dev-targeted (wire /Frame/ catalog changes), and independent of this blocker. Left for separate PRs as the issue recommends.Test plan
cargo test -p moq-mux(240 passing) including:build_avcc_carries_multiple_ppsavc3_keyframe_with_two_pps_keeps_bothavc3_reinit_drops_superseded_pps(transform drops on reinit)avc3_reinjects_all_cached_pps_on_keyframe(importer re-injection)avc3_reinit_drops_superseded_pps_on_keyframe(importer drops on reinit)export_avc3_preserves_multiple_pps(end-to-end TS export round-trip)cargo clippy -p moq-mux --all-targetscleancargo fmtmoq-cliandhangbuild against the changeCross-package sync
No
js/netorjs/hangmirror needed: this is internal moq-mux transmux, not amoq-netwire change or ahangcatalog/container format change.(Written by Claude)