Skip to content

fix(moq-mux): reject frames before the first keyframe instead of dropping#1766

Closed
kixelated wants to merge 2 commits into
mainfrom
claude/romantic-lovelace-133ded
Closed

fix(moq-mux): reject frames before the first keyframe instead of dropping#1766
kixelated wants to merge 2 commits into
mainfrom
claude/romantic-lovelace-133ded

Conversation

@kixelated

@kixelated kixelated commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

A MoQ group must start with a keyframe. The container Producer papered over this with a with_lenient_start opt-in that silently dropped non-keyframes arriving before the first group. That let a malformed stream produce an undecodable group with no signal, and the leniency was load-bearing in ways that weren't obvious at the call sites.

This removes with_lenient_start and makes the invariant explicit, with a single legitimate exception (MPEG-TS, which can join mid-GOP).

What changed

  • Producer owns and enforces the invariant (producer.rs): a non-keyframe with no open group is rejected with a new typed Error::MissingKeyframe. write now returns crate::Error directly (the bound crate::Error: From<C::Error> holds for every container error, incl. fmp4::Error via the Cmaf variant), so importers just ?-propagate the error rather than re-deriving the decision. Removed with_lenient_start.
  • New typed error (error.rs): Error::MissingKeyframe plus Error::is_missing_keyframe(&anyhow::Error) to detect it through an anyhow chain.
  • Codec importers (h264, h265) reset per-frame state before writing, so a propagated MissingKeyframe leaves them ready for the next access unit. The h264 avc3 path additionally errors on a keyframe it cannot configure (no inline SPS, no avcC seed) — the genuinely undecodable case, kept distinct from MissingKeyframe so it is never swallowed.
  • TS is the sole exception (ts/import.rs): a transport stream legitimately joins mid-GOP, so Stream::write ignores MissingKeyframe. This is the single choke point, so it also covers the post-seek discontinuity case.
  • FLV propagates it (flv/import.rs): a valid FLV is expected to open on a keyframe, so a leading delta surfaces as a malformed-stream error to the caller.

Why

The old behavior silently swallowed a real protocol violation. Making it a typed error that the Producer returns by default — with mid-stream tolerance opt-in at exactly one importer — means a group can never start without a keyframe, the one format where mid-GOP join is normal (TS) is explicitly the special case, and the keyframe decision lives in one place instead of being re-derived at every call site.

Public API note

Removing pub fn with_lenient_start from moq-mux's container Producer is technically a breaking change, but the only callers were internal to the crate. Producer::write/finish/seek/finish_group now return crate::Error instead of the (always-crate::Error in practice) associated C::Error. Error::MissingKeyframe (a #[non_exhaustive] enum) and Error::is_missing_keyframe are additive. moq-mux isn't in the explicit breaking-change list, so this targets main — happy to retarget dev if reviewers prefer.

Test plan

  • cargo test -p moq-mux — 236 passed, incl. survives_midstream_join and kyrion_dirtystart_extracts_real_cues
  • New: keyframe_without_sps_errors, delta_before_init_returns_missing_keyframe; updated first_frame_must_be_keyframe to expect MissingKeyframe
  • cargo clippy -p moq-mux --all-targets clean
  • cargo fmt --check clean (via nix toolchain)
  • Dependents compile (hang, moq-cli, moq-native)

No cross-package sync needed: the JS side (js/hang) has no equivalent leniency concept.

(Written by Claude)

…ping

A MoQ group must start with a keyframe. The container `Producer` papered over
this with a `with_lenient_start` opt-in that silently dropped non-keyframes
arriving before the first group, which let a malformed stream produce an
undecodable group without anyone noticing.

Remove `with_lenient_start` and make the rule explicit:

- The `Producer` stays strict: a delta with no open group is a protocol
  violation. Add `has_group()` so importers can check before writing.
- Codec/format importers (h264 avc1+avc3, h265, flv) return a new typed
  `Error::MissingKeyframe` when a delta arrives before a keyframe anchors a
  group, plus `Error::is_missing_keyframe` to detect it through an anyhow chain.
- The h264 avc3 path additionally errors on a keyframe it cannot configure
  (no inline SPS, no avcC seed), the genuinely undecodable case.
- The TS importer is the sole exception: a transport stream can legitimately
  join mid-GOP, so `Stream::write` ignores `MissingKeyframe` (covering the
  post-seek discontinuity case too). FLV lets it propagate, since a valid FLV
  is expected to open on a keyframe.

Tests: add `keyframe_without_sps_errors` and
`delta_before_init_returns_missing_keyframe`; `survives_midstream_join` and
`kyrion_dirtystart` continue to pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The pull request removes the with_lenient_start() builder method and the associated lenient_start field from Producer<C>, replacing the silent delta-drop behavior with an explicit Error::MissingKeyframe variant and a new has_group() -> bool query method. Every importer (H.264 avc3 and avc1 paths, H.265, FLV) removes its .with_lenient_start() call and adds a has_group() guard that returns MissingKeyframe when a delta arrives before any group is open. The H.264 avc3 path also hard-fails when a keyframe arrives before SPS-derived configuration. The TS importer captures the decode result and suppresses MissingKeyframe errors, returning Ok(()) for late-joining streams. Two tests validate the avc3 IDR-before-SPS and delta-before-keyframe error paths.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'reject frames before the first keyframe instead of dropping' accurately summarizes the main change—converting from silent frame dropping to explicit rejection of non-keyframes before the first keyframe.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description clearly explains the changes, rationale, and implementation details aligned with the changeset modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch claude/romantic-lovelace-133ded

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…_group

The producer is the single owner of the keyframe invariant, so make `write`
return `Error::MissingKeyframe` directly instead of having each importer
re-derive the decision with a `has_group()` pre-check.

`container::Producer::write` now returns `crate::Error` (the bound
`crate::Error: From<C::Error>` holds for every container error, including
`fmp4::Error` via the `Cmaf` variant), so the orphan-delta case returns
`MissingKeyframe` and importers just propagate it. Removes `has_group()` and
all the duplicated checks in the h264/h265/flv importers.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

@arielmol I missed with_lenient_start in the original PR.

I'll fix this on the dev branch, but IMO we should error on invalid input instead of skipping. The TS importer can be a special case that allows joining mid-stream, but like we should tell the user they doing stuff wrong instead of silently accepting it IMO.

I think on the dev branch I'll add a MissingKeyframe error that the TS importer will ignore.

@kixelated kixelated closed this Jun 16, 2026
@kixelated kixelated deleted the claude/romantic-lovelace-133ded branch June 26, 2026 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant