moq-mux: pre-existing codec/container correctness findings (from #1918 review)

[kixelated]

CodeRabbit's review of #1918 (the moq-mux backport to main) surfaced several Major-severity findings that are pre-existing in the dev moq-mux lineage — they are not regressions introduced by the backport. The two low-risk ones were fixed in #1918; the rest are tracked here because they're either risky to change blindly (could affect the byte-exact roundtrip tests), architectural, or ambiguous. They should be fixed on dev (and flow back to main) or in a focused follow-up with dedicated tests.

Deferred findings

• codec/h264/split.rs / codec/h265/split.rs — consecutive bare IDRs can merge into one access unit. The IDR/keyframe arm reconciles SPS/PPS and sets contains_idr/contains_slice but never closes an already-open slice-bearing frame on the first slice of a new picture (unlike the non-keyframe slice path which checks first_slice…). A stream that doesn't repeat parameter sets before each keyframe, with a keyframe arriving right after a delta picture in the same decode chunk, can fold two AUs into one frame (and mis-flag it keyframe). Streaming-only; the decode+flush-per-AU tests don't surface it.

• codec/aac/mod.rs — explicit (15) sample-rate index misparses. When freq_index == 15 the 24-bit explicit rate begins in bits already consumed from the preceding bytes; the current path reads the next three whole bytes, so both sample_rate and channel_count decode wrong. Breaks Config::encode() → Config::parse() round-trips for non-standard rates. Needs a bit-reader.

• container/consumer.rs — group read errors all treated as evictions. The poll_read error branch skips forward on any F::Error, but a malformed container payload (decode error) is not the same as a relay group eviction. Only known group-abort / old-cache errors should skip; decode errors should propagate.

• container/fmp4/mod.rs — duration-less multi-sample fragments lose timestamps. Encoding omits the sample duration whenever Frame.duration is None, but decode() advances DTS by 0 for those, so a fragment with frames at e.g. 0ms and 33ms but no explicit durations decodes both at 0ms. Should infer adjacent durations for non-last samples or reject such multi-sample fragments. Related to the signed/duration work in Support negative / signed presentation timestamps (audio priming, pacing, B-frame reorder) #1917.

• catalog/filter.rs / catalog/target.rs — stream never returns EOF after the final snapshot. After upstream EOF, last_input stays Some, so once the retained final snapshot is emitted the next poll parks in Pending forever. Ambiguous: the Pending may be intentional (re-filter the retained snapshot on a later target change after upstream ends). Needs a decision on whether a filter/target stream should end with upstream or stay alive for post-EOF target changes.

Fixed in #1918 (low-risk)

• codec/av1/import.rs: av1C size gate relaxed from >= 16 to the 4-byte fixed header.
• container/fmp4/import.rs: checked arithmetic for PTS / sample offsets / DTS advance.

(Written by Claude)

[coderabbitai]

🔗 Related PRs

#1782 - fix(ci): unbreak the self-hosted A1 runner (libmoq clippy + ffmpeg bindgen headers) [merged]
#1812 - fix(moq-mux): carry all distinct SPS/PPS/VPS through transmux, not just the last seen [merged]
#1815 - feat(moq-mux): generic verbatim MPEG-TS carriage (mpegts catalog section) [merged]
#1842 - feat(moq-cli): wire verbatim MPEG-TS carriage through publish/subscribe [merged]
#1879 - test: move moq-cli's TS verbatim coverage into moq-mux [merged]
#1884 - feat(moq-mux): clear consumer buffer when group timestamps rewind [merged]
#1897 - feat(json): group-scoped DEFLATE compression with browser support [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[kixelated]

Status: all five findings fixed on `main`

All five deferred findings have since been fixed on `main` by two PRs:

• fix(moq-mux): codec/container correctness fixes from #1918 review (#1923) #1925 — `fix(moq-mux): codec/container correctness fixes from Backport moq-mux to main (adapted to main's moq-net, no wire/API breaks) #1918 review` covered every finding file: `codec/h264/split.rs`, `codec/h265/split.rs`, `codec/aac/mod.rs`, `container/consumer.rs`, `container/fmp4/mod.rs`, `catalog/filter.rs`, `catalog/target.rs`.
• Fix fMP4 zero-duration samples #1933 — `Fix fMP4 zero-duration samples` reinforced the duration-less multi-sample fragment finding.

Spot-checked on current `main`:

• h264/h265 split — the IDR arm now closes an open slice-bearing AU on the first slice of a new picture (`first_mb_in_slice` high-bit guard), with a `bare_idr_after_delta_splits` regression test.
• AAC explicit (15) sample rate — rewritten on a real bit-reader so the 24-bit rate and channel count parse from the correct bit offsets.
• container/consumer poll_read — only group-abort / old-cache errors skip forward; decode errors propagate.
• fMP4 duration-less fragments — non-last sample durations are inferred (fix(moq-mux): codec/container correctness fixes from #1918 review (#1923) #1925 + Fix fMP4 zero-duration samples #1933).
• catalog filter/target EOF — the stream now terminates after the retained final snapshot.

Not yet on `dev`

The fixes landed `main`-first (the inverse of the order this issue suggested). `dev`'s moq-mux still carries the original bugs — confirmed by a direct content diff (e.g. `dev`'s h264 IDR arm lacks the split guard, its AAC still reads three whole bytes for `freq_index == 15`, and its fMP4 still does `dts += sample_duration.unwrap_or(0)`). The two lineages also differ structurally (`dev` uses `moq_net::Timestamp` where `main` uses `crate::container::Timestamp`), so it is not a clean cherry-pick.

No separate port is planned: `dev` inherits these on the next routine "Merge main into dev". Safe to close once that merge lands (or now, tracking the dev arrival via the merge).

(Written by Claude Opus 4.8)