Tolerate trailing box padding after child boxes by AdrianEddy · Pull Request #173 · kixelated/mp4-atom

AdrianEddy · 2026-07-15T01:01:59Z

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 can't be a box; ffmpeg, GPAC and others skip it. The strict remaining-bytes check otherwise fails the whole moov with UnderDecode.

A skip_trailing_padding helper 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 applied after the child-box loop everywhere one occurs (per the note about #112): 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.

Tests cover the sample-entry boundary (1/4/7 tolerated, 8 rejected) and the container paths (edts via the macro, meta hand-written).

Split from the original two-in-one PR — the udta namespace dispatch is now #222. Rebased onto main.

coderabbitai · 2026-07-15T01:05:17Z

Walkthrough

Added skip_trailing_padding to consume buffer remainders shorter than eight bytes and applied it across sample-entry decoders. Added Avc1 coverage for four-byte trailing padding. Reworked udta decoding to explicitly dispatch supported children, skip padding boxes, handle unknown or truncated children, and encode optional children in a fixed order. Added QuickTime-focused tests for trailing padding and name child dispatch.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 31.58% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly summarizes the main change: tolerating trailing padding after child boxes.
Description check	✅ Passed	The description is directly related to the padding-handling and decode changes in the patch.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.}

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs (1)
107-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover both sides of the eight-byte boundary.

This test only exercises a four-byte remainder. Add cases for 1 and 7 bytes, plus an 8-byte remainder that remains on the existing rejection path, to verify the helper’s exact < 8 contract.
🤖 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/trak/mdia/minf/stbl/stsd/h264/avc1.rs` around lines 107 - 147, The
test test_avc1_trailing_padding only validates a 4-byte remainder; extend it to
cover 1-byte and 7-byte trailing padding cases, confirming both decode
successfully and produce the original Avc1 value. Also add an 8-byte remainder
case that continues to reject decoding, preserving the helper’s exact
less-than-8-byte contract and existing rejection behavior.

🤖 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`:
- Line 224: Run cargo fmt to format the Rust test code around the buf slice
assignment, then verify cargo fmt -- --check passes without changes.
- Around line 40-45: In the child parsing loop around the size check, replace
the truncated-child break path with an immediate decoding error when size
exceeds buf.remaining(). Ensure the already-consumed header and leftover bytes
are not passed to skip_trailing_padding, while preserving normal parsing for
children whose declared body fits.

---

Nitpick comments:
In `@src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs`:
- Around line 107-147: The test test_avc1_trailing_padding only validates a
4-byte remainder; extend it to cover 1-byte and 7-byte trailing padding cases,
confirming both decode successfully and produce the original Avc1 value. Also
add an 8-byte remainder case that continues to reject decoding, preserving the
helper’s exact less-than-8-byte contract and existing rejection behavior.

🪄 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

Run ID: e60959ab-d3f0-4be3-ba3f-9117d0dff04b

📥 Commits

Reviewing files that changed from the base of the PR and between 2a82fb8 and b545e3a.

📒 Files selected for processing (17)

src/buf.rs
src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
src/moov/trak/mdia/minf/stbl/stsd/amr/samr.rs
src/moov/trak/mdia/minf/stbl/stsd/av01.rs
src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
src/moov/trak/mdia/minf/stbl/stsd/flac.rs
src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/opus.rs
src/moov/trak/mdia/minf/stbl/stsd/tx3g.rs
src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs
src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs
src/moov/trak/mdia/minf/stbl/stsd/wvtt.rs
src/moov/udta/mod.rs

coderabbitai · 2026-07-15T01:05:20Z

+        let mut buf = ENCODED_UDTA_WITH_CPRT.to_vec();
+        buf.extend_from_slice(&[0, 0, 0, 0]);
+        let size = (buf.len() as u32).to_be_bytes();
+        buf[0..4].copy_from_slice(&size);


📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Run cargo fmt before merging.

The build check reports a rustfmt diff in this test hunk. As per coding guidelines, Rust files must pass cargo fmt -- --check.

🧰 Tools

🪛 GitHub Check: build

[warning] 224-224:
Diff in /home/runner/work/mp4-atom/mp4-atom/src/moov/udta/mod.rs

🤖 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` at line 224, Run cargo fmt to format the Rust test code around the buf slice assignment, then verify cargo fmt -- --check passes without changes.

Sources: Coding guidelines, Linters/SAST tools

bradh

#112 was an earlier attempt at the same problem, so skip_trailing_padding may be needed in other places.

Can you fix the format problem.

I'd also prefer not to have the "before the fix" type noise added by the AI. That doesn't make sense in the code. If you want to have it, just put it in the PR text or a comment.

bradh · 2026-07-15T10:46:03Z

+/// other demuxers. A remainder of 8 or more bytes is left untouched so genuine
+/// trailing corruption is still reported.
+pub(crate) fn skip_trailing_padding<B: Buf>(buf: &mut B) {
+    if buf.remaining() < 8 {


Could this be

Suggested change

if buf.remaining() < 8 {

if buf.remaining() > 0 && buf.remaining() < 8 {

to save the common case of buf.advance(0) ?

bradh · 2026-07-15T10:46:59Z

+                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)?),
+                // `free`/`skip` are padding boxes — drop them silently, as the


This kind of AI noise is better removed.

bradh · 2026-07-15T10:47:38Z

+            }
+        }
+        // QuickTime appends a 4-byte zero terminator after `udta`'s children
+        // (e.g. after an `apmd` box), just as it does inside sample entries.


What is apmd and why does that matter here?

bradh · 2026-07-15T10:49:17Z

+    // terminator) after a sample entry's child boxes. Before the fix the
+    // strict remaining-bytes check rejected the whole entry with
+    // `UnderDecode(avc1)`; now the sub-header remainder is skipped as padding.


Suggested change

// terminator) after a sample entry's child boxes. Before the fix the

// strict remaining-bytes check rejected the whole entry with

// `UnderDecode(avc1)`; now the sub-header remainder is skipped as padding.

// terminator) after a sample entry's child boxes.

bradh · 2026-07-15T10:53:17Z

Also, its easier to review one fix at a time - this has two.

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.

AdrianEddy · 2026-07-22T15:42:15Z

Split as requested — this PR is now just the trailing-padding fix; the udta namespace dispatch moved to #222.

Addressed the rest:

Make remaining bytes validation conditional on strict mode #112 — skip_trailing_padding is now applied everywhere a child-box loop is followed by the strict emptiness check: the nested! container macro, the hand-written meta/mfra containers, and the sample entries. Added edts (macro) and meta container tests alongside the avc1 boundary ones.
buf.rs guarded on n > 0 && n < 8 to skip the advance(0).
Applied your avc1 comment; removed the "before the fix" narration from the code.
cargo fmt --check clean.

Rebased onto current main.

coderabbitai Bot reviewed Jul 15, 2026

View reviewed changes

AdrianEddy force-pushed the fix-quicktime-udta-and-trailing-padding branch 3 times, most recently from 0fe5fd1 to 540aa7f Compare July 15, 2026 03:13

bradh reviewed Jul 15, 2026

View reviewed changes

AdrianEddy force-pushed the fix-quicktime-udta-and-trailing-padding branch from 540aa7f to 6d3fa1a Compare July 22, 2026 15:41

AdrianEddy mentioned this pull request Jul 22, 2026

udta: dispatch children by namespace #222

Merged

AdrianEddy changed the title ~~Tolerate trailing box padding; decode udta by namespace~~ Tolerate trailing box padding after child boxes Jul 22, 2026

bradh approved these changes Jul 23, 2026

View reviewed changes

Navigation Menu

Tolerate trailing box padding after child boxes - #173

Tolerate trailing box padding after child boxes#173
AdrianEddy wants to merge 1 commit into
kixelated:mainfrom
AdrianEddy:fix-quicktime-udta-and-trailing-padding

AdrianEddy commented Jul 15, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 15, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot Jul 15, 2026

Uh oh!

bradh left a comment

Uh oh!

bradh Jul 15, 2026

Uh oh!

bradh Jul 15, 2026

Uh oh!

bradh Jul 15, 2026

Uh oh!

bradh Jul 15, 2026

Uh oh!

bradh commented Jul 15, 2026

Uh oh!

AdrianEddy commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	if buf.remaining() < 8 {
	if buf.remaining() > 0 && buf.remaining() < 8 {

Conversation

AdrianEddy commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

bradh left a comment

Choose a reason for hiding this comment

Uh oh!

bradh Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

bradh Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

bradh Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

bradh Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

bradh commented Jul 15, 2026

Uh oh!

AdrianEddy commented Jul 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AdrianEddy commented Jul 15, 2026 •

edited

Loading

coderabbitai Bot commented Jul 15, 2026 •

edited

Loading