udta: dispatch children by namespace - #222
Conversation
QuickTime muxers append a few bytes of padding — commonly a 4-byte zero terminator — after the child boxes of a container atom or sample entry (e.g. `avc1 → avcC + colr + 00000000`). The remainder is shorter than an 8-byte box header, so it cannot be a box; ffmpeg, GPAC and others skip it. The strict remaining-bytes check otherwise fails the whole `moov` with `UnderDecode`. `skip_trailing_padding` drains a sub-header (`< 8` byte) remainder — 8+ zero bytes already decode as a null-fourcc box, so only a 1–7 byte remainder is affected — and is now called after the child-box loop everywhere one occurs: the `nested!` container macro (moov, trak, mdia, minf, stbl, dinf, edts, mvex, moof, traf, iprp, udta), the hand-written `meta` and `mfra` containers, and every sample entry. A remainder of 8+ bytes is left untouched so genuine trailing corruption is still reported.
`udta` is a free-form user-data container whose children are context-specific. QuickTime writes a track-name `name` box in a track's `udta`, and its fourcc collides with the iTunes `ilst` Name item in the global atom table; routing `udta` children through that table misparses the QuickTime `name` (its string decoder stops at the first NUL of an empty name) and fails the whole `moov` with `UnderDecode(name)`. `Udta::decode_body` now dispatches children by header against the udta namespace only (`cprt`, `kind`, `meta`, `rtng`; `free`/`skip` dropped as padding; everything else via `decode_unknown`), the same shape as `Ilst::decode_body`. A child whose declared size exceeds what remains is rejected as truncated.
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe decoder gains shared handling for 1–7 trailing padding bytes after child atoms. Nested containers, metadata, movie fragments, and multiple audio/video/text sample entries now invoke this handling. Udta uses explicit child dispatch with size validation, known-field encoding, padding-box omission, and unknown-box routing. Tests cover trailing padding for metadata, editing lists, Avc1, and Udta, including rejection of a QuickTime track-name box. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/moov/udta/mod.rs`:
- Around line 44-47: Update the known-child handling in the udta nested decoder
to reject a second Cprt, Kind, Meta, or Rtng when its corresponding option is
already populated, returning the existing Error::DuplicateBox instead of
overwriting it. Preserve normal first-child decoding and add a regression test
covering duplicate known children and the expected error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d91f962a-0874-439f-a90f-bdb540778228
📒 Files selected for processing (21)
src/atom.rssrc/buf.rssrc/meta/mod.rssrc/mfra/mod.rssrc/moov/trak/edts/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/ac3.rssrc/moov/trak/mdia/minf/stbl/stsd/amr/samr.rssrc/moov/trak/mdia/minf/stbl/stsd/av01.rssrc/moov/trak/mdia/minf/stbl/stsd/eac3.rssrc/moov/trak/mdia/minf/stbl/stsd/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rssrc/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/opus.rssrc/moov/trak/mdia/minf/stbl/stsd/tx3g.rssrc/moov/trak/mdia/minf/stbl/stsd/uncv.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rssrc/moov/trak/mdia/minf/stbl/stsd/wvtt.rssrc/moov/udta/mod.rs
| Cprt::KIND => cprt = Some(Cprt::decode_atom(&header, buf)?), | ||
| Kind::KIND => kind = Some(Kind::decode_atom(&header, buf)?), | ||
| Meta::KIND => meta = Some(Meta::decode_atom(&header, buf)?), | ||
| Rtng::KIND => rtng = Some(Rtng::decode_atom(&header, buf)?), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve duplicate-child rejection.
nested! previously returned Error::DuplicateBox for repeated known children. These assignments silently overwrite the first atom, so decoding and re-encoding a malformed udta loses data. Reject duplicates for cprt, kind, meta, and rtng before assignment, and add a regression test.
🤖 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 `@src/moov/udta/mod.rs` around lines 44 - 47, Update the known-child handling
in the udta nested decoder to reject a second Cprt, Kind, Meta, or Rtng when its
corresponding option is already populated, returning the existing
Error::DuplicateBox instead of overwriting it. Preserve normal first-child
decoding and add a regression test covering duplicate known children and the
expected error.
There was a problem hiding this comment.
@AdrianEddy I think checking for duplicates (i.e. that each of cprt, kind, meta and rtng is None before assigning) is a valid suggestion. Your view?
There was a problem hiding this comment.
Good catch that the rewrite dropped the nested! duplicate check — but restoring DuplicateBox for all four children would reject spec-valid files. The specs disagree with the old behavior for three of the four boxes:
cprt— ISO/IEC 14496-12 §8.10.2, quantity zero or more: "There may be multiple copyright boxes using different language codes" (that's why the box carries alanguagefield).kind— ISO/IEC 14496-12 §8.10.4, quantity zero or more: a track carrying several roles gets one KindBox per role value, and multiple schemes may each contribute boxes (DASH role signaling relies on this).rtng— 3GPP TS 26.244 clause 8 (asset information): the udta may hold "zero or more sub-boxes of each kind, zero or one for each language".meta— ISO/IEC 14496-12 §8.11.1, quantity zero or one per container: a duplicate here genuinely is malformed.
So the old nested! behavior was already wrong — it failed a whole moov over e.g. a bilingual copyright notice, which is exactly the class of over-strictness this PR is about, and keep-last silently dropped data.
Fixed in 09daed6: cprt/kind/rtng are now Vec fields that accumulate repeats (encode iterates them), while a second meta is rejected with Error::DuplicateBox. Regression tests cover both directions: repeated children round-trip losslessly, duplicate meta errors. Note this is a breaking change to the Udta field types (Option<T> → Vec<T>).
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| Cprt::KIND => cprt = Some(Cprt::decode_atom(&header, buf)?), | ||
| Kind::KIND => kind = Some(Kind::decode_atom(&header, buf)?), | ||
| Meta::KIND => meta = Some(Meta::decode_atom(&header, buf)?), | ||
| Rtng::KIND => rtng = Some(Rtng::decode_atom(&header, buf)?), |
There was a problem hiding this comment.
@AdrianEddy I think checking for duplicates (i.e. that each of cprt, kind, meta and rtng is None before assigning) is a valid suggestion. Your view?
CopyrightBox and KindBox are quantity "zero or more" (ISO/IEC 14496-12 §8.10.2 and §8.10.4 — one copyright notice per language, one kind box per role value), and 3GPP TS 26.244 clause 8 admits "zero or more sub-boxes of each kind, zero or one for each language" of the asset boxes, rtng included. The previous nested!-based decode rejected such spec-valid repeats with DuplicateBox, and the first namespace-dispatch revision silently kept only the last one; store them as Vec instead. MetaBox stays quantity "zero or one" per container (§8.11.1), so a second meta is rejected with DuplicateBox rather than overwritten.
dbeae0e to
09daed6
Compare
|
Thanks. |
2 participants
udtais a free-form user-data container whose children are context-specific. QuickTime writes a track-namenamebox in a track'sudta, and its fourcc collides with the iTunesilstNameitem in the global atom table; routingudtachildren through that table misparses the QuickTimename(its string decoder stops at the first NUL of an empty name) and fails themoovwithUnderDecode(name).Udta::decode_bodynow dispatches children by header against the udta namespace only (cprt,kind,meta,rtng;free/skiptreated as padding; everything else viadecode_unknown), the same shape asIlst::decode_bodyfrom #172. A child whose declared size exceeds what remains is rejected as truncated.Review follow-up (breaking): child multiplicity now follows the specs —
cprtandkindare quantity "zero or more" (ISO/IEC 14496-12 §8.10.2 / §8.10.4: one copyright notice per language, one kind box per role value) andrtngmay repeat per language (3GPP TS 26.244 clause 8), so thoseUdtafields changed fromOption<T>toVec<T>and repeats accumulate.metastays "zero or one" (§8.11.1): a second one is rejected withError::DuplicateBox. The oldnested!decode rejected repeats of all four, failing wholemoovs over spec-valid files such as bilingual copyright notices.Split out of the trailing-padding PR (#173) and stacked on it — the diff shows the trailing-padding commit until #173 merges.