[kixelated]

Problem

An fMP4 header refusal dropped the refusing call's unread tail. A later arbitrary chunk could then report success after losing the next box's prefix. This PR implements the framing fix from the quest, re-measures the interleaved import benchmark, and finishes the quest.

Approach

• Choice made under the recorded plan: decode refusals are terminal, matching MKV's simpler policy. Later decodes and controls return the first error without reading input or changing tracks. Remove refusal-specific skip/recovery and deferred-control settlement; release parser state and abort any partial frame. finish() still closes accepted media without clearing the refusal.
• Verify HLS with real muxed segments, unwrapped and passthrough: a refused importer retires immediately and the next pass can import a repaired source under fresh track names. This skips the MAX_RENDITION_FAILURES count that transient fetch failures still get. The refusal is not settled, so run still retries the step within its backoff budget. A timestamp rewind still ends the HLS import (Error::Rewind). Existing init-map replacement, discontinuity, and timestamp-refusal tests pass. CLI chunked input already propagates decode errors and aborts the instance.
• The benchmark re-measurement (below) meets the quest's goal, so this PR deletes quest/m1/fmp4-refusal-framing.md and its links.

Impact

• Breaking behavior: after container::fmp4::Import::decode fails, decoding and controls keep returning that error; replace the importer to resume, including through import::Container. Box-aligned input no longer recovers on the same importer. Migration notes are under moq-mux/MIGRATE.md's Unreleased section. No signatures or exported items change.
• Wire: formats and refusal abort codes unchanged.

Alternatives

Draining the refusing call would preserve recovery but require tracking the first error while processing more input and more failure/control paths. The recorded plan explicitly permits terminal refusal; HLS already owns importer replacement, so the smaller state machine meets its needs.

Performance

cmaf-interleaved-import/4tracks/16runs-4096bytes, the 4,232,444-byte workload the quest names. The historical whole-input ID lacks today's -<chunk>chunk suffix, so the cases are paired explicitly. The bench body is the same at every revision apart from that suffix.

Revisions: base = 038ecfa6b3 (parent of moq-dev/moq#7232), stream = 951cca678c (#7232), main = 96244dd854 (this branch's merge parent), current = c147c2caf3 (this branch at that merge; the later commit only deletes the quest). Each binary was built from a git archive of its revision with its own target dir. Each pass ran 6 rounds over every order of its sides, each run with Criterion at 1 s warmup, 2 s measurement and 30 samples (the repo's paired-comparison settings), pinned to the P-cores 0-7. Both builds and measurement ran through quest gate, which admitted them at a 1-minute load of 17-19 on 24 CPUs. Load then rose to 39-45 during the runs from other sessions, so the host was shared, not quiet. Medians of the per-round paired deltas follow; "held" means every round had the same sign.

Case	Pair	Median Δ	Range	Sign	Median means
whole input	stream vs base	−81.0%	−86.9…−77.9%	held	7.60 → 1.49 ms
whole input	current vs base	−78.7%	−86.7…−76.2%	held	7.60 → 1.67 ms
whole input	current vs main	+3.5% / +8.7%	−32.8…+33.3%	mixed	1.36-1.43 → 1.33-1.38 ms
1024-byte chunks	current vs main	−3.5%	−13.9…+34.2%	mixed	1.68 → 1.88 ms
1-byte chunks	current vs main	+4.4% / +3.1%	+1.0…+10.1%	held	339-341 → 351-353 ms
1-byte chunks	main vs stream	+19.4%	+16.4…+22.1%	held	283 → 341 ms

• Quest goal met: the streaming change did not slow this case. It is about 5× faster than before. The earlier "possible ~25% slowdown" does not reproduce, and no acceptance of a cost is needed.
• This PR's own cost, isolated against its merge parent, is not measurable at whole-input or 1 KiB chunks. At 1-byte chunks, it adds a consistent 2-4%, about 2-3 ns per decode call, from the terminal-error check and wrapper. A variant that moved the failure handling into a #[cold] function and rebuilt Ok(()) measured −1.1% against this branch with a mixed sign (fix pass: −2.3…+5.4%), so it was dropped as unproven. Choice made without asking: this per-call cost is accepted. One-byte chunks are pathological (HLS feeds whole segments, CLI reads buffered chunks), and the cost is below the noise at any realistic chunk size.
• Separately, main is 19% slower than fix(mux)!: stream fMP4 media payloads and bound held metadata #7232 at 1-byte chunks. That change landed on main between 951cca678c and 96244dd854 and is outside this PR (see Follow-ups).
• The earlier shared-host checkpoint's base/current means (7.99/1.04, 7.54/1.07, 6.37/1.03 ms) agree with these.

Validation

• Before the fix, a refused mdat plus four bytes of the next styp header, followed by its remaining four bytes, returned success without declaring the segment boundary. The reproduction failed with the styp prefix was silently dropped. The permanent test sweeps splits through the refused body and next header, including empty input and decoding after finish().
• After merging origin/main (d26e2aac82) on head 317662a81b: quest gate nix develop --ignore-environment --keep HOME --keep USER --keep PATH --command just ci origin/main: passed, including 4,416 Rust tests (67 skipped) and every selected check.
• quest check --base origin/main: 586 documents OK.

Review

• Latest: Codex, gpt-6.1-sol, medium effort, round 2 of the Codex session, head 317662a81b44e52aad5e4a9b553a0eacd28c492a: 0 findings. Changes since round 1 were a docstring rewording and a merge of origin/main.
• Pullfrog (thread): the HLS test docstring's "transient fetch retry budget" read as run's STEP_RETRY_BUDGET, which a refusal still spends. Fixed: the docstring now names the MAX_RENDITION_FAILURES count the refusal skips, and says run still retries it within its backoff budget.
• Codex round 1, head 7d0717fc48: 0 findings.
• Earlier rounds, on head 3ed95cc36a before the main merge and quest deletion (Claude Code, claude-opus-5-5, medium): round 1 preserved MalformedTrack abort codes, made HLS retire refused importers immediately, kept failed controls inert, and documented terminal behavior on shared import::Container::decode. Broader binding-doc duplication was refuted: the FFI write describes arbitrary chunk splitting and directly propagates MoqError::Codec (rs/moq-ffi/src/media.rs:1182-1196), and makes no recovery-after-error promise. Round 2 reported 0 findings.

Follow-ups

• Find which main commit between 951cca678c and 96244dd854 made 1-byte-chunk fMP4 import ~19% slower (283 → 341 ms), and whether any realistic chunk size feels it. Candidates touching the path include moq-dev/moq#7491, moq-dev/moq#7398, moq-dev/moq#6985 (feature unification) and moq-dev/moq#7218. Low priority: whole-input and 1 KiB chunks show no change.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[kixelated]

Design change: HLS now retires a refused fMP4 importer immediately instead of spending the transient-fetch retry budget on a terminal decoder. Rewinds still terminate the import, transient fetch failures retain their existing handling, and subsequent decode/control calls on a refused importer keep the original error. This addresses the local review while preserving the quest's authorized terminal-refusal choice.

(Written by GPT-6.1 Sol)

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 34 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 39e99f56-6c79-42f0-bca9-92649a5f8de9

Commits

Reviewing files that changed from the base of the PR and between d26e2aa and 317662a.

Files selected for processing: 8

• quest/m1/README.md
• quest/m1/fmp4-refusal-framing.md
• quest/m1/mkv-leaf-limits.md
• rs/moq-hls/src/import.rs
• rs/moq-mux/MIGRATE.md
• rs/moq-mux/src/container/fmp4/import.rs
• rs/moq-mux/src/container/fmp4/import_test.rs
• rs/moq-mux/src/import/container.rs

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

• X
• Mastodon
• Reddit
• LinkedIn

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 7d0717f

No actionable bugs found. Making decode refusals terminal fixes arbitrary-chunk framing at its source and removes recovery/deferred-control complexity. The first error remains sticky across decode and controls, partial media is aborted, and HLS retires refused generations while retaining fatal rewind handling. Migration notes and split-boundary/replacement tests cover the behavior change.

Verification limits: static source review only. I did not run tests, HLS playback, or benchmarks; the reported performance measurements were not independently reproduced.

(Written by OpenAI)

[pullfrog]

ℹ️ No critical issues — one test comment claims a retry-budget behavior the code does not have.

Reviewed changes

• Terminal fMP4 refusals — Import::decode now delegates to a new private drain; on any error it aborts a held Body::Media, clears buffer/moof/deferred, latches the error, and every later decode/cut/seek/discontinuity returns it without reading input or changing tracks. finish still ends the accepted media. The old refused-box skip and post-refusal settle() paths are gone, fixing the lost-framing bug where a refused box's unread tail could be silently dropped.
• HLS retires refused importers — a decode error other than a timestamp reset now calls evict() before returning, so the next pass rebuilds the generation from its init segment under fresh track names. The new test covers both passthrough modes and a repaired source.
• Docs & tests — MIGRATE.md records the breaking behavior; import/container.rs documents terminal decode errors; the quest and its links are deleted; the fragment/oversized-metadata/text tests are reworked to the terminal contract with a split-sweep test.

The fMP4 terminal-state cleanup is complete: every early return in drain leaves self.body None or a writers-cleared Media, so no open frame leaks, no double-abort occurs, and finish() never synthesizes a spurious UnexpectedEof. HLS eviction is bounded (run retries the non-settled error with backoff rather than looping), and group numbering stays anchored to the HLS sequence so sibling renditions cannot desync.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}