[kixelated]

Completes quest/m1/fmp4-metadata-limits.md: an fMP4 moov or moof within the importer's bounds now parses and plans within MAX_HELD_BYTES (32 MiB), and track setup costs linear time.

Measured

Peak live heap of one Import::decode call, from the counting allocator (release build; the tests assert the same numbers in debug):

Input	Before	After
116 B moof of field-less truns at MAX_FRAGMENT_SAMPLES, then an mdat header	584 MB	14 MB (2^17 samples)
moof of 4-byte trun entries at MAX_FRAGMENT_SAMPLES	667 MB / 684 MB passthrough	19 MB / 25 MB
moof of 4-byte entries filling the byte bound	369 MB, then TooManySamples	32 MiB refused at its header; 2 MiB parses in 23 MB, then TooManySamples
moov of repeated audio traks	quadratic: 1.4 GB allocated for 2,000	linear: 7.2 MB allocated for 256, twice 128's; 257+ refused before any track is created

Changes

• Parse bound. mp4-atom expands a parsed body up to tenfold, on any box it knows and wherever that box appears. A 4-byte trun entry becomes a 40-byte TrunEntry, subs grows about 8x, and traks about 4.4x. So a 32 MiB moov or moof could cost about 320 MB to parse. A new fmp4::MAX_METADATA_BYTES (MAX_HELD_BYTES / 16, 2 MiB) bounds a moov or moof body at its header, and MAX_HELD_BYTES now bounds only a text sample.
• Sample plan. MAX_FRAGMENT_SAMPLES is now MAX_METADATA_BYTES / 16 (2^17, down from 2^21): a moof at the byte bound still holds that many full 16-byte entries. The plan is presized from the declared count instead of growing. Text samples moved out of the per-sample Chunk queue into one list on the Mdat, so a Chunk shrank to a byte range and an index. The mdat byte-ownership map extends a range that ends where the next sample starts, so samples that sit back to back take one entry per run.
• Tracks. Rendition::set and stage checked each new catalog entry by cloning and serializing the whole catalog, so a moov of N tracks cost N². They now check the entry in a catalog that holds only it and the extension, then write it in place. This applies to every importer, not just fMP4. A new fmp4::MAX_TRACKS (256) refuses a moov declaring more with Error::TooManyTracks before any track is created.
• Tests. fragment_plans_within_the_held_bound covers field-less runs, full entries, 4-byte entries filling the byte bound, and one-sample runs whose bytes are never adjacent, each unwrapped and passthrough. moov_tracks_are_bounded checks linear allocation (it fails on the old catalog code, at a 3.3x ratio), the peak, and refusal both past MAX_TRACKS and at the byte bound. Together they run in under a second in debug. allocation::Usage gains peak for them.
• Docs: doc/lib/rs/moq-mux.md, the CHANGELOG, and the m2 policy quest state the new bounds.

Choices made without asking

• The Plan offered two ways to bound trun expansion: read the counts from the held body, or bound a moof body separately. I chose the byte bound and applied it to moov too. mp4-atom decodes a known box (a trun, a trak) wherever it appears, before rejecting it, so a pre-scan of the expected paths would leak. The byte bound caps every box's expansion at once, including subs and trak. Trade-off: a trun count is still refused after a parse, but that parse is capped at about 20 MB.
• Values: MAX_METADATA_BYTES = 2 MiB, MAX_FRAGMENT_SAMPLES = 2^17, MAX_TRACKS = 256 (the quest sets them). A moov with more than 2 MiB of metadata, such as large embedded cover art, is now refused. Real fMP4 metadata is kilobytes.
• A rendition write is checked in a catalog holding only its entry plus a clone of the catalog's extension. The media and data sections serialize each entry on its own, so skipping them keeps writes linear. The extension stays whole because a custom CatalogExt serializer may depend on its combined entries, as the OpenAI review pointed out.

Impact

• Public API: new fmp4::MAX_METADATA_BYTES, fmp4::MAX_TRACKS, and fmp4::Error::TooManyTracks (the enum is #[non_exhaustive]). MAX_FRAGMENT_SAMPLES changes value, and MAX_HELD_BYTES keeps its value but now covers only text samples. The MetadataTooLarge message names the new bound. No signatures change.
• Behavior: fMP4 / HLS ingest refuses a moov or moof over 2 MiB, a moof of more than 131,072 samples, or a moov of more than 256 tracks.
• Wire: none.

Checks

• cargo test -p moq-mux: 1991 passed, 3 ignored, including the peak-memory tests fragment_plans_within_the_held_bound and moov_tracks_are_bounded, and the new an_entry_that_breaks_the_extension_is_refused (fails without the fix).
• cargo clippy -p moq-mux --all-targets --all-features -- -D warnings, cargo fmt, and RUSTDOCFLAGS=-D warnings cargo doc -p moq-mux --no-deps: clean.
• quest check --base origin/main: ok.
• Not run: the Markdown lint (sh/markdown.sh), which needs bun install on this disk-constrained host.

Not in this PR

• mp4-atom's sgpd v0 entries that cost no input (21 bytes → 251 MB at 4M entries) are already fixed upstream in kixelated/mp4-atom#264. Picking up that fix is tracked by quest/m1/fuzz-findings/mp4-atom-bump.md and waits on a release.
• The subscriber's CMAF Wire::decode parses a moof from a frame of up to 32 MiB, so it has the same tenfold expansion. I'm suggesting it as a follow-up.

Review

Codex gpt-6.1-sol, medium effort (quest review), new session round 1 on fb90bcf87437826db0df112d6fb7d9ae45a2b164 against origin/main at 151ed3820d: no findings.

Online reviews on 3917bd8583:

• OpenAI P2, the rendition write misses a custom extension whose serializer depends on its combined entries: fixed in 5fc9276. The scratch catalog now holds the real extension, and a regression test covers it.
• Pullfrog, the MAX_METADATA_BYTES doc overstates the tenfold bound for sgpd: fixed in 5fc9276. The doc names the exception and links fix: reject sgpd entry counts past the box kixelated/mp4-atom#264.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

[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 1 minute.

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: db1bc982-75af-4d50-a247-b0840faad9c8

Commits

Reviewing files that changed from the base of the PR and between c4f8abc and fb90bcf.

Files selected for processing: 11

• doc/lib/rs/moq-mux.md
• quest/m1/README.md
• quest/m1/fmp4-metadata-limits.md
• quest/m2/import-metadata-policy.md
• rs/moq-mux/CHANGELOG.md
• rs/moq-mux/src/allocation.rs
• rs/moq-mux/src/catalog/tracks.rs
• rs/moq-mux/src/container/fmp4/import.rs
• rs/moq-mux/src/container/fmp4/mod.rs
• rs/moq-mux/src/container/fmp4/sample.rs
• rs/moq-mux/src/container/fmp4/text.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

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 3917bd8

• P2 — Validate the resulting catalog for custom extensions (rs/moq-mux/src/catalog/tracks.rs:390–397): write serializes a catalog containing only the new entry before mutating the real catalog. Public CatalogExt implementations can serialize conditionally on the combined entries, so an insertion may return success yet leave the actual catalog unserializable; subsequent publication then aborts instead of rejecting the edit atomically as before. Preserve full aggregate validation for custom extensions or expose an equivalent validation contract, and add a cross-entry serializer regression test.

Bounding headers and sample plans is a useful direction. The disclosed upstream sgpd allocation remains a separate limit on the overall memory guarantee until the dependency fix lands. Static review only; I did not run tests or measure memory.

[pullfrog]

ℹ️ Minor suggestions only. The new bounds, the text-staging rework, the plan presizing, and the catalog linearization all check out; one documentation caveat inline.

Reviewed changes

• Parse and sample bounds (rs/moq-mux/src/container/fmp4/mod.rs): MAX_METADATA_BYTES (2 MiB) caps a moov/moof body at its header, MAX_FRAGMENT_SAMPLES drops to 2^17 and now presizes the plan, and MAX_TRACKS (256) refuses an over-large moov with Error::TooManyTracks before any track is created. MAX_HELD_BYTES now bounds only a text sample.
• Sample plan (fmp4/import.rs, fmp4/sample.rs, fmp4/text.rs): text samples move out of the per-sample Chunk into one Mdat::text list plus a single shared Mdat::staged buffer, shrinking Chunk to a range and ChunkOutput; Claimed::take extends an adjacent range rather than inserting; Vec/VecDeque are presized from the already-checked sample count via sample::count.
• Catalog writes (rs/moq-mux/src/catalog/tracks.rs): the shared write helper validates a rendition's identity and serialization in a catalog holding only its own entry, replacing the whole-catalog clone-and-serialize that made declaring N renditions cost N². This applies to set, replace, and stage across every importer.
• Allocator accounting (rs/moq-mux/src/allocation.rs): Usage::peak records peak live bytes, accounting for the both-blocks-live window during realloc.
• Tests and docs: fragment_plans_within_the_held_bound and moov_tracks_are_bounded assert peak allocation and refusal; the CHANGELOG, doc/lib/rs/moq-mux.md, and the m1/m2 quest docs record the new limits.

I traced the two subtle behavioral changes: the shared staged buffer is safe because write_mdat only ever feeds the front chunk and drains staged on each completion, so no two text samples are staged at once; and Claimed::take's merge preserves cross-track disjointness. The write refactor is equivalent to the old whole-catalog check, since check_identity inspects only the named video/audio entry and insert touches exactly one category.

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

[kixelated]

Re the OpenAI review's P2 on rs/moq-mux/src/catalog/tracks.rs (write checks only the new entry): agreed, fixed in 5fc9276.

The scratch catalog is now seeded with a clone of the real extension (alone.ext = catalog.ext.clone()), so everything a custom CatalogExt serializer sees is checked after the insert, and a failing edit is refused before the catalog keeps it. Built-in configs never write to ext, so their writes stay linear in track count. The new test catalog::tracks::tests::custom::an_entry_that_breaks_the_extension_is_refused uses a section that serializes only while it holds a single entry. It fails without the fix and passes with it.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: fb90bcf

• P2 — Validate custom insertions against the real catalog context (rs/moq-mux/src/catalog/tracks.rs:395–400): Cloning the extension fixes the previously reported cross-entry serializer case, but the scratch catalog still has empty video, audio, text, and data sections. RenditionConfig::insert is public and receives the whole catalog. For example, a custom insertion can write only its own extension while reading whether a video rendition already exists; its extension serializer can reject the resulting value. The scratch insertion sees no video and passes, while the real insertion sees video and leaves a catalog that cannot serialize, so an edit that previously failed atomically can be accepted and later abort publication. Validate custom insertions against equivalent full context, or define and enforce a narrower insertion contract, and cover this cross-section dependency in a regression test.

The new extension clone addresses the earlier extension-only example. Static review only; I did not run tests or measure memory.

[pullfrog]

✅ No new issues found.

Reviewed changes

• Extension validated whole (rs/moq-mux/src/catalog/tracks.rs): write now clones catalog.ext into the scratch catalog, so a custom CatalogExt serializer that depends on its combined entries is checked before the real catalog is mutated. Media/data entries are still excluded, keeping N renditions from re-serializing each other. This closes the OpenAI P2 finding.
• Regression test: an_entry_that_breaks_the_extension_is_refused adds a Solo section that refuses more than one entry and asserts the second write fails while the first is kept.
• Docs: MAX_METADATA_BYTES now names the sgpd case that expands past the bound until the mp4-atom count fix lands.
• Merge of origin/main: no behavioral change to the touched files.

The extension clone is bounded by application-supplied catalog data, not by the untrusted moov/moof, so it does not reopen the attacker-controlled quadratic path this PR closes.