mux: emit avcC/hvcC in catalog description for inline-SPS/PPS importers#1437
Closed
kixelated wants to merge 4 commits into
Closed
mux: emit avcC/hvcC in catalog description for inline-SPS/PPS importers#1437kixelated wants to merge 4 commits into
kixelated wants to merge 4 commits into
Conversation
The Avc3 and Hev1 importers cache parameter set NALs but previously left the catalog `description` field empty, leaving downstream consumers to synthesize an AVCDecoderConfigurationRecord (or HEVC equivalent) on their own. KVS- and CMAF-style muxers that need codec-private bytes had no out-of-band record to work from and would either reject the rendition or require keyframe scraping. Build the configuration record once both SPS+PPS (or VPS+SPS+PPS for H.265) have been observed and republish the catalog with the populated description. The cache update order changes so init() sees the latest NAL before deciding whether the catalog needs republishing. For Hev1, splitting "codec changed" (which requires a new track) from "description appeared" (in-place rendition update) keeps subscribers from re-fetching the track every time the description becomes available. avcC builder emits the standard ISO/IEC 14496-15 §5.3.3.1.2 layout without the high-profile extension fields; players that need them re-derive from the SPS we ship inline. hvcC builder pulls chroma/bit-depth/temporal-id fields from the parsed SPS per §8.3.3.
Contributor
WalkthroughThis PR adds codec-specific description fields to H.264 and HEVC renditions by populating them with configuration records (avcC for AVC3 and hvcC for HEV1). Avc3 now caches SPS before initial init(), re-parses cached SPS when PPS arrives, stores a BroadcastProducer to allow track replacement, and builds avcC when SPS+PPS are present. Hev1 gates VPS/SPS/PPS caching and computes hvcC when VPS+SPS+PPS exist; init() now reinitializes tracks only when codec-bearing fields differ (same_codec). Both modules include helpers and tests to serialize/validate the config records. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ 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 |
Three follow-ups on top of the avcC/hvcC description emission: 1. Avc3 now drops + recreates its track when the codec config actually changes (resolution/profile flip), matching Hev1's existing behavior. Description-only updates (PPS arriving after SPS) keep the existing track via a shared `same_codec` predicate hoisted into `import::mod`. Previously the avc3 track persisted across codec changes and mixed incompatible samples — a pre-existing bug surfaced by this PR's description-emission flow. 2. `build_avcc` and `build_hvcc` now error if any NAL exceeds the 16-bit length field in their respective configuration records, instead of silently truncating. SPS/PPS/VPS NALs are normally a few dozen bytes but range/SCC extensions can produce larger ones. 3. `build_hvcc` parses its own SPS internally instead of accepting an already-parsed `SpsNALUnit`, simplifying both the call site and the tests. 4. Unit tests for the new code paths: avcC layout, oversize rejection for both records, and array-record structure for hvcC.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-mux/src/import/avc3.rs (1)
401-441:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid emitting incomplete avcC for high profiles.
build_avccalways writes the baseline layout, but profiles like 100/110/122/144 require the extra avcC extension fields. Emitting the shorter form for those profiles can produce an invalid description for strict consumers. Either append the required extension fields or fail fast for those profile IDs.Suggested guard (fail fast until extension bytes are implemented)
fn build_avcc(sps_nal: &[u8], pps_nal: &[u8]) -> anyhow::Result<Bytes> { use bytes::BufMut; @@ let profile_idc = sps_nal.get(1).copied().unwrap_or(0); let constraints = sps_nal.get(2).copied().unwrap_or(0); let level_idc = sps_nal.get(3).copied().unwrap_or(0); + anyhow::ensure!( + !matches!(profile_idc, 100 | 110 | 122 | 144), + "avcC high-profile extension fields are not implemented (profile_idc={profile_idc})" + );🤖 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/import/avc3.rs` around lines 401 - 441, build_avcc currently always emits the baseline avcC layout but must fail for high profiles that require the avcC extension fields; after extracting profile_idc (in function build_avcc) add a guard that checks if profile_idc is one of the high-profile IDs (e.g. 100, 110, 122, 144 — include other known high profiles if needed) and return an error (using anyhow::bail or anyhow::ensure) with a clear message like "profile X requires avcC extension fields; not implemented" so we fail fast until the extension bytes are implemented; place this check right after the profile_idc/constraints/level_idc extraction and before allocating out/serializing the avcC fields.
🧹 Nitpick comments (1)
rs/moq-mux/src/import/hev1.rs (1)
525-557: ⚡ Quick winAdd one positive hvcC layout test in addition to overflow tests.
The new tests only cover error paths. A single “known input → exact hvcC bytes/fields” assertion would protect the bitfield packing and array ordering from regressions.
🤖 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/import/hev1.rs` around lines 525 - 557, Add a positive unit test in the existing tests module (in hev1.rs) that calls build_hvcc with a small, known VPS/SPS/PPS triplet (use the same test harness as the overflow tests) and asserts success and that the returned hvcC bytes/fields match an exact expected layout; specifically reference build_hvcc and the tests mod, construct deterministic vps/sps/pps vectors, call build_hvcc(...).unwrap(), and compare the resulting byte vector (or parsed hvcC struct fields) against a precomputed expected byte slice to lock in bitfield packing and array ordering.
🤖 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.
Outside diff comments:
In `@rs/moq-mux/src/import/avc3.rs`:
- Around line 401-441: build_avcc currently always emits the baseline avcC
layout but must fail for high profiles that require the avcC extension fields;
after extracting profile_idc (in function build_avcc) add a guard that checks if
profile_idc is one of the high-profile IDs (e.g. 100, 110, 122, 144 — include
other known high profiles if needed) and return an error (using anyhow::bail or
anyhow::ensure) with a clear message like "profile X requires avcC extension
fields; not implemented" so we fail fast until the extension bytes are
implemented; place this check right after the profile_idc/constraints/level_idc
extraction and before allocating out/serializing the avcC fields.
---
Nitpick comments:
In `@rs/moq-mux/src/import/hev1.rs`:
- Around line 525-557: Add a positive unit test in the existing tests module (in
hev1.rs) that calls build_hvcc with a small, known VPS/SPS/PPS triplet (use the
same test harness as the overflow tests) and asserts success and that the
returned hvcC bytes/fields match an exact expected layout; specifically
reference build_hvcc and the tests mod, construct deterministic vps/sps/pps
vectors, call build_hvcc(...).unwrap(), and compare the resulting byte vector
(or parsed hvcC struct fields) against a precomputed expected byte slice to lock
in bitfield packing and array ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80bb2ef3-45c4-4901-a66f-44034d05a2cc
📒 Files selected for processing (3)
rs/moq-mux/src/import/avc3.rsrs/moq-mux/src/import/hev1.rsrs/moq-mux/src/import/mod.rs
kixelated
commented
May 21, 2026
| tracing::debug!(name = ?old_name, "codec changed; replacing track"); | ||
| catalog.video.renditions.remove(&old_name); | ||
|
|
||
| let new_track = self.broadcast.unique_track(".avc3")?; |
Collaborator
Author
There was a problem hiding this comment.
We should make this before removing the old one right?
kixelated
commented
May 21, 2026
| catalog.video.renditions.remove(&track.name); | ||
| } | ||
|
|
||
| let track = self.broadcast.unique_track(".hev1")?; |
Collaborator
Author
There was a problem hiding this comment.
We should do this before removing right?
Per review on #1437: if `unique_track(...)` errors after we've already removed the old rendition (avc3) or taken the old track (hev1), the catalog is left in a half-state and the old track is lost. Reorder both importers so the replacement track is constructed first and the catalog mutations only run once we have it in hand. Also lifts the fallible network call out from under the catalog lock.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/moq-mux/src/import/hev1.rs (1)
540-565: 💤 Low valueConsider adding a layout verification test for hvcC.
Unlike
avc3.rswhich hasavcc_layout_matches_iso_14496_15, this file only tests overflow rejection. A layout test with a minimal valid SPS would improve confidence in the hvcC byte layout. This would require crafting a parseable SPS NAL, which is more involved than avc3's simple byte extraction.🤖 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/import/hev1.rs` around lines 540 - 565, Add a layout verification unit test for hvcC similar to avcc_layout_matches_iso_14496_15: create a new test (e.g., hvcc_layout_matches_iso_14496_15) that uses build_hvcc with a minimal valid VPS/SPS/PPS byte sequences (construct a parseable SPS NAL rather than just arbitrary bytes), then assert the produced hvcC byte layout matches the ISO/IEC 14496-15 expected structure (or round-trip by parsing the output with your hvcC parser). Locate build_hvcc and the existing tests hvcc_errors_on_oversized_vps/sps/pps to mirror their pattern and ensure the SPS chosen is a minimal valid HEVC SPS so the test verifies layout correctness rather than just overflow rejection.
🤖 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.
Nitpick comments:
In `@rs/moq-mux/src/import/hev1.rs`:
- Around line 540-565: Add a layout verification unit test for hvcC similar to
avcc_layout_matches_iso_14496_15: create a new test (e.g.,
hvcc_layout_matches_iso_14496_15) that uses build_hvcc with a minimal valid
VPS/SPS/PPS byte sequences (construct a parseable SPS NAL rather than just
arbitrary bytes), then assert the produced hvcC byte layout matches the ISO/IEC
14496-15 expected structure (or round-trip by parsing the output with your hvcC
parser). Locate build_hvcc and the existing tests
hvcc_errors_on_oversized_vps/sps/pps to mirror their pattern and ensure the SPS
chosen is a minimal valid HEVC SPS so the test verifies layout correctness
rather than just overflow rejection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1812920c-4d59-44eb-988a-3f942c2e6a2f
📒 Files selected for processing (2)
rs/moq-mux/src/import/avc3.rsrs/moq-mux/src/import/hev1.rs
… description Once we ship an avcC/hvcC in the catalog description, the bitstream shouldn't redundantly carry the parameter sets — that's the avc1/hvc1 contract. Previously the importers were a hybrid: out-of-band avcC plus inline NALs plus Annex-B framing, which matches no codec convention cleanly. This commit commits to avc1/hvc1: - SPS/PPS (and VPS for HEVC) are cached for description but stripped from sample data; the defensive insertion before IDR slices is gone. - Sample data is length-prefixed (4-byte big-endian NAL length) instead of Annex-B start codes, matching `lengthSizeMinusOne = 3` in the configuration records. - `H264.inline` and `H265.in_band` flip to false, so the catalog codec string becomes `avc1.*` / `hvc1.*`. - The `contains_sps`/`contains_pps`/`contains_vps` flags and the unused `START_CODE` constant in `annexb` go away. Importer struct names (`Avc3`, `Hev1`) describe the INPUT format (Annex-B with inline parameter sets) — the output is now consistently avc1/hvc1.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/moq-mux/src/import/hev1.rs (1)
337-341: ⚡ Quick winDead code:
contains_vps,contains_sps,contains_ppsflags are never set.The
Framestruct includescontains_vps,contains_sps, andcontains_ppsfields that are reset inmaybe_start_frame()but never set totrueanywhere. These appear to be remnants from the prior implementation that inserted parameter sets before keyframes. Consider removing them.♻️ Proposed cleanup
In
Framestruct:struct Frame { chunks: BytesMut, contains_idr: bool, contains_slice: bool, - contains_vps: bool, - contains_sps: bool, - contains_pps: bool, }In
maybe_start_frame():self.current.contains_idr = false; self.current.contains_slice = false; - self.current.contains_vps = false; - self.current.contains_sps = false; - self.current.contains_pps = false;Also applies to: 450-457
🤖 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/import/hev1.rs` around lines 337 - 341, Frame contains dead boolean flags contains_vps, contains_sps, contains_pps that are reset in maybe_start_frame() but never set or used elsewhere; remove these fields from the Frame struct and delete their resets in maybe_start_frame() (and the duplicated reset block around the other occurrence), then run a repo-wide search to remove any remaining references to contains_vps/contains_sps/contains_pps and adjust any logic that assumed their presence (ensure only contains_idr and contains_slice remain as needed).
🤖 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.
Nitpick comments:
In `@rs/moq-mux/src/import/hev1.rs`:
- Around line 337-341: Frame contains dead boolean flags contains_vps,
contains_sps, contains_pps that are reset in maybe_start_frame() but never set
or used elsewhere; remove these fields from the Frame struct and delete their
resets in maybe_start_frame() (and the duplicated reset block around the other
occurrence), then run a repo-wide search to remove any remaining references to
contains_vps/contains_sps/contains_pps and adjust any logic that assumed their
presence (ensure only contains_idr and contains_slice remain as needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cba06e6-315a-4845-8051-397371440044
📒 Files selected for processing (3)
rs/moq-mux/src/import/annexb.rsrs/moq-mux/src/import/avc3.rsrs/moq-mux/src/import/hev1.rs
💤 Files with no reviewable changes (1)
- rs/moq-mux/src/import/annexb.rs
Collaborator
Author
|
Closing — review feedback led us to the right architectural conclusion: the avc3/hev1 importers shouldn't transcode their own output. If a downstream consumer needs avc1/hvc1 (length-prefixed samples + out-of-band avcC/hvcC), that's the consumer's transcode and belongs in an exporter layer. Codec-aware writers like the upcoming Not landing the avcC/hvcC emission in the importers means consumers that today need an out-of-band record will get it from the export layer instead. Re-open or salvage commits if useful for the MKV exporter work. |
kixelated
pushed a commit
that referenced
this pull request
May 21, 2026
Addresses the qsx0004 / KVS pilot blockers from #1438 review: 1. **Avc3/Hev1 sources now work end-to-end.** The MKV exporter no longer demands a populated catalog `description`. New `export::Avc1` / `export::Hvc1` helpers cache inline parameter sets per track, synthesize `AVCDecoderConfigurationRecord` / `HEVCDecoderConfigurationRecord`, strip SPS/PPS/VPS from sample data, and length-prefix the remaining NALs. `Container::Legacy` sources with avc1/hvc1 shape (description populated) pass through unchanged. Per the close comment on #1437, the codec-shape transcode belongs in the exporter (codec-aware writer), not the importer. These helpers live in `export/` because that's their natural home; they're public so other container exporters (e.g. fmp4) can reuse them later. 2. **Header emission is deferred.** When any video track lacks a codec config (Avc3 with description=None), the file header waits until each transmuxer has observed SPS+PPS (typically the first keyframe). After that, the header is emitted with CodecPrivate populated; sample bytes that arrived during the wait are already transformed and flow normally. 3. **Configurable fragment duration.** `Mkv::with_fragment_duration(d)` batches frames into Clusters of roughly `d` (default 2s) and rolls on video keyframes or i16 timestamp overflow. `Duration::ZERO` keeps the old one-cluster-per-frame behavior. This unblocks KVS PutMedia, which throttles at much lower fragment rates than per-frame. 4. **Graceful close on dropped producer.** Both catalog and track polls now treat `moq_net::Error::Dropped` as EOS. Producers that drop without calling `finish()` no longer surface as exporter errors. Restructuring: - `pub use mkv::*;` tightened to `pub use mkv::Mkv;` (and `Fmp4`, `Avc1`, `Hvc1`); only intended types are re-exported from `export`. - `mkv_test.rs` moved under `export/test/` to match the import-side layout. - `import/annexb` is now `pub(crate)` so the export-side transmuxers can share its NAL iterator. New tests: - `export_avc3_source_synthesizes_avcc_and_length_prefixes` — end-to-end proof that an Avc3 broadcast (inline=true, no description, Annex-B samples) yields valid MKV with avcC in CodecPrivate and length-prefixed blocks. - `export_fragment_duration_batches_blocks` — 5 frames within 100ms batch into a single Cluster when fragment_duration=2s. - `avc1_*` / `hvc1_*` unit tests for the transmuxer state machine (parameter caching, codec config build, passthrough, idempotency). Mid-stream Tracks updates are still a known limitation; out of scope here. https://claude.ai/code/session_013rPJFCfTMyf71MqzqAXo45
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
The
Avc3andHev1importers cache parameter set NALs for re-insertion before keyframes, but until now they left the catalogdescriptionfield empty. Downstream consumers that need an out-of-bandAVCDecoderConfigurationRecord/HEVCDecoderConfigurationRecord(KVS via MKV CodecPrivate; CMAF muxers; anything not willing to scrape the bitstream) had no way to get one from anavc3/hev1broadcast.This PR:
description, and republishes the catalog. Same flow for hvcC once VPS+SPS+PPS are all cached.init()— that way the description builder sees the latest NAL, not the previous one.Hev1, splits "codec changed" (which requires a new track) from "description appeared" (in-place rendition update) so subscribers don't re-fetch the track when only the description becomes available.The avcC layout follows ISO/IEC 14496-15 §5.3.3.1.2 (standard, no high-profile extension fields — those are inferable from the inline SPS we ship anyway). The hvcC layout follows §8.3.3; chroma/bit-depth/temporal-id fields come from the parsed SPS.
Test plan
cargo test --libon moq-mux — 84 tests passdescriptionpopulated (KVS MKV bridge inquartermaster/moq-pilotworker)description(not just inline NALs) can decode🤖 Generated with Claude Code