feat: Collects unexpected boxes during decoding #81

bradh · 2025-12-27T00:10:13Z

This change introduces the ability to collect and retain any unexpected boxes encountered during the decoding process.

Previously, the decoder would give an Error::unexpectedBox when encounters an unexpected box thus failing the decode process. Now, these boxes are stored in a unexpected Vec within their parent structures (Moov, Trak, Mdia, Minf, Stbl, etc.).

This allows for more robust handling of malformed or extended MP4 files, as the application can now inspect and process unexpected boxes as needed.

closes #63

replaces #71, which it is derived from.

This change introduces the ability to collect and retain any unexpected boxes encountered during the decoding process. Previously, the decoder would give an Error::unexpectedBox and failing the decode process. Now, these boxes are stored in a `unexpected` Vec within their parent structures (Moov, Trak, Mdia, Minf, Stbl, etc.). This allows for more robust handling of malformed or extended MP4 files, as the application can now inspect and process unexpected boxes as needed.

Adds documentation to README and library root regarding the fault-tolerant parsing feature. This explains how unexpected child boxes are handled during decoding and clarifies that they are not written back during encoding.

cargo fmt don't work for some reason.

Implements a new `fault-tolerant` feature that enables parsing of MP4 files with unexpected boxes. Updates documentation and tests to reflect the feature.

Introduces tests to verify the behavior of the fault-tolerant feature. When enabled, the feature collects unexpected boxes during decoding. When disabled, encountering unexpected boxes results in an error. Also adds tests for multiple unexpected boxes and ensures that unexpected boxes are not encoded.

bradh · 2025-12-27T00:11:32Z

@b01o if you have a chance, can you check that I didn't break your intent in the fixup. If no capacity, OK, just flag that.

bradh · 2025-12-27T00:12:08Z

@kixelated Are you good with the fault-tolerant feature being default?

coderabbitai · 2025-12-27T00:13:48Z

Walkthrough

Adds a new Cargo feature fault-tolerant and makes it a default. When enabled, decoding collects unknown child boxes into a feature-gated unexpected: Vec<Any> field on many public structs instead of returning Error::UnexpectedBox; when disabled decoding remains strict. Numerous structs had Eq removed from derives. Decode paths were modified to conditionally accumulate unexpected boxes, tests were updated to account for the new field, and README/docs were extended with fault-tolerant usage notes.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main feature being introduced: collecting unexpected boxes during decoding, which aligns with the core objective of the PR.
Description check	✅ Passed	The description clearly explains the change, referencing the problem (Error::UnexpectedBox), the solution (storing in unexpected Vec), and benefits. It also mentions closing issue #63.
Linked Issues check	✅ Passed	The PR implements fault-tolerant decoding for issue #63 by collecting unexpected boxes in a Vec field instead of returning Error::UnexpectedBox, enabling handling of real-world MP4 files with spec-extended boxes [#63].
Out of Scope Changes check	✅ Passed	All changes are scoped to implementing fault-tolerant decoding: feature flag addition, unexpected fields in structures, decoding logic updates, test additions, and documentation. No unrelated changes detected.
Docstring Coverage	✅ Passed	Docstring coverage is 88.37% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 905f026 and e6b7860.

📒 Files selected for processing (4)

Cargo.toml
README.md
justfile
src/lib.rs

✅ Files skipped from review due to trivial changes (1)

src/lib.rs

🚧 Files skipped from review as they are similar to previous changes (2)

README.md
justfile

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-08-31T04:06:57.364Z

Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/tokio/**/*.rs : For async IO (with the tokio feature), implement and use AsyncReadFrom and AsyncWriteTo traits

Applied to files:

Cargo.toml

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

coderabbitai

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/meta/iprp.rs (1)

6-22: Critical issue: unexpected field is decoded but never encoded, causing data loss.

The nested! macro correctly populates the unexpected field during decoding when fault-tolerant is enabled (lines 204-207), but the encode_body function (lines 226-229) completely omits this field from encoding. This breaks the decode/encode symmetry required by the Atom trait and defeats the purpose of fault-tolerant mode—unexpected boxes are silently discarded when re-encoding. The macro must be updated to encode the unexpected field back to the buffer.
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs (1)
58-66: taic field is not encoded.

The encode_body method encodes audio, esds, and optionally btrt, but the taic field (line 13) is never encoded. This appears to be a pre-existing omission unrelated to the fault-tolerant changes.
🔎 Proposed fix to encode taic
     fn encode_body<B: BufMut>(&self, buf: &mut B) -> Result<()> {
         self.audio.encode(buf)?;
         self.esds.encode(buf)?;
         if self.btrt.is_some() {
             self.btrt.encode(buf)?;
         }
+        if self.taic.is_some() {
+            self.taic.encode(buf)?;
+        }

         Ok(())
     }

🧹 Nitpick comments (1)

src/moov/trak/mdia/minf/stbl/stsd/opus.rs (1)
22-23: Consider updating the comment.

The comment "Find esds in mp4a or wave" appears to be a copy-paste artifact from another file. For Opus, the loop is looking for dOps, not esds.
Suggested fix
-        // Find esds in mp4a or wave
+        // Find dOps child box
         while let Some(atom) = Any::decode_maybe(buf)? {

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fef880e and 905f026.

📒 Files selected for processing (43)

Cargo.toml
README.md
justfile
src/atom.rs
src/lib.rs
src/meta/iprp.rs
src/meta/mod.rs
src/moof/mod.rs
src/moof/traf/mod.rs
src/moov/mod.rs
src/moov/mvex/mod.rs
src/moov/trak/edts/mod.rs
src/moov/trak/mdia/minf/dinf/mod.rs
src/moov/trak/mdia/minf/mod.rs
src/moov/trak/mdia/minf/stbl/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/ac3.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/hevc/hvcc.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.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/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/mod.rs
src/moov/trak/mod.rs
src/moov/udta/mod.rs
src/test/av1.rs
src/test/bbb.rs
src/test/esds.rs
src/test/fault_tolerant.rs
src/test/flac.rs
src/test/h264.rs
src/test/heif.rs
src/test/hevc.rs
src/test/mod.rs
src/test/uncompressed.rs
src/test/vp9.rs

🧰 Additional context used

📓 Path-based instructions (6)

**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: Code must be clippy-clean (cargo clippy --all-targets --all-features -- -D warnings)
Code must be formatted with rustfmt (cargo fmt) and pass formatting checks (cargo fmt -- --check)
Write unit tests inline within modules using #[cfg(test)]

Files:

src/test/mod.rs
src/moov/trak/edts/mod.rs
src/meta/iprp.rs
src/moov/trak/mdia/minf/stbl/mod.rs
src/moov/trak/mdia/minf/dinf/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/opus.rs
src/atom.rs
src/moov/trak/mdia/minf/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
src/moov/trak/mdia/mod.rs
src/test/hevc.rs
src/moof/traf/mod.rs
src/moof/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/flac.rs
src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
src/moov/trak/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/udta/mod.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/hevc/hvc1.rs
src/test/flac.rs
src/test/fault_tolerant.rs
src/test/uncompressed.rs
src/lib.rs
src/moov/mvex/mod.rs
src/test/av1.rs
src/test/bbb.rs
src/test/esds.rs
src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
src/test/vp9.rs
src/moov/trak/mdia/minf/stbl/stsd/av01.rs
src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
src/test/heif.rs
src/test/h264.rs
src/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rs
src/meta/mod.rs
src/moov/mod.rs

src/**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.rs: Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()
Use Header::read_from() to handle large atoms without loading entire contents into memory
All operations should return Result using custom error types defined in error.rs
Use the custom Buf/BufMut traits for contiguous byte slices instead of ad-hoc buffer handling

Files:

src/test/mod.rs
src/moov/trak/edts/mod.rs
src/meta/iprp.rs
src/moov/trak/mdia/minf/stbl/mod.rs
src/moov/trak/mdia/minf/dinf/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/opus.rs
src/atom.rs
src/moov/trak/mdia/minf/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
src/moov/trak/mdia/mod.rs
src/test/hevc.rs
src/moof/traf/mod.rs
src/moof/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/flac.rs
src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
src/moov/trak/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/udta/mod.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/hevc/hvc1.rs
src/test/flac.rs
src/test/fault_tolerant.rs
src/test/uncompressed.rs
src/lib.rs
src/moov/mvex/mod.rs
src/test/av1.rs
src/test/bbb.rs
src/test/esds.rs
src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
src/test/vp9.rs
src/moov/trak/mdia/minf/stbl/stsd/av01.rs
src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
src/test/heif.rs
src/test/h264.rs
src/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rs
src/meta/mod.rs
src/moov/mod.rs

src/test/**

📄 CodeRabbit inference engine (CLAUDE.md)

Put integration tests under src/test/ with sample MP4 files for different codecs

Files:

src/test/mod.rs
src/test/hevc.rs
src/test/flac.rs
src/test/fault_tolerant.rs
src/test/uncompressed.rs
src/test/av1.rs
src/test/bbb.rs
src/test/esds.rs
src/test/vp9.rs
src/test/heif.rs
src/test/h264.rs

src/moov/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place movie metadata atom implementations under src/moov/ (and its hierarchy: trak/mdia/minf/stbl/stsd)

Files:

src/moov/trak/edts/mod.rs
src/moov/trak/mdia/minf/stbl/mod.rs
src/moov/trak/mdia/minf/dinf/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/opus.rs
src/moov/trak/mdia/minf/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
src/moov/trak/mdia/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/flac.rs
src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
src/moov/trak/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/udta/mod.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/hevc/hvc1.rs
src/moov/mvex/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.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/hevc/hvcc.rs
src/moov/mod.rs

src/meta/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place metadata (HEIF/AVIF) atom implementations under src/meta/

Files:

src/meta/iprp.rs
src/meta/mod.rs

src/moof/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place movie fragment atom implementations under src/moof/

Files:

src/moof/traf/mod.rs
src/moof/mod.rs

🧠 Learnings (10)

📓 Common learnings

Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()

Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/any.rs : Register new atom types in the Any enum (src/any.rs) to enable flexible decoding

📚 Learning: 2025-08-31T04:06:57.364Z

Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to **/*.rs : Write unit tests inline within modules using #[cfg(test)]

Applied to files:

src/test/mod.rs
src/test/fault_tolerant.rs

📚 Learning: 2025-08-31T04:06:57.364Z

Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/any.rs : Register new atom types in the Any enum (src/any.rs) to enable flexible decoding

Applied to files:

src/moov/trak/edts/mod.rs
src/meta/iprp.rs
src/moov/trak/mdia/minf/stbl/mod.rs
src/moov/trak/mdia/minf/dinf/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/opus.rs
README.md
src/atom.rs
src/moov/trak/mdia/minf/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
src/moov/trak/mdia/mod.rs
src/moof/traf/mod.rs
src/moof/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/flac.rs
src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
src/moov/trak/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
src/moov/udta/mod.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/hevc/hvc1.rs
src/lib.rs
src/moov/mvex/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
src/test/vp9.rs
src/moov/trak/mdia/minf/stbl/stsd/av01.rs
src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
src/moov/mod.rs

📚 Learning: 2025-08-31T04:06:57.364Z

Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()

Applied to files:

src/moov/trak/edts/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/opus.rs
README.md
src/atom.rs
src/moov/trak/mdia/minf/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
src/moov/trak/mdia/mod.rs
src/test/hevc.rs
src/moof/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/flac.rs
src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
src/moov/trak/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/udta/mod.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/hevc/hvc1.rs
src/lib.rs
src/moov/mvex/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
src/test/vp9.rs
src/moov/trak/mdia/minf/stbl/stsd/av01.rs
src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
src/test/h264.rs
src/moov/mod.rs

📚 Learning: 2025-08-31T04:06:57.364Z

Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Use Header::read_from() to handle large atoms without loading entire contents into memory

Applied to files:

README.md
src/atom.rs
src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
src/moov/trak/mdia/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/flac.rs
src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
src/lib.rs
src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
src/moov/trak/mdia/minf/stbl/stsd/av01.rs

📚 Learning: 2025-08-31T04:06:57.364Z

Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/moov/** : Place movie metadata atom implementations under src/moov/ (and its hierarchy: trak/mdia/minf/stbl/stsd)

Applied to files:

src/test/hevc.rs
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
src/test/h264.rs

📚 Learning: 2025-08-31T04:06:57.364Z

Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/moof/** : Place movie fragment atom implementations under src/moof/

Applied to files:

src/moof/mod.rs

📚 Learning: 2025-08-31T04:06:57.364Z

Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to **/*.rs : Code must be formatted with rustfmt (cargo fmt) and pass formatting checks (cargo fmt -- --check)

Applied to files:

justfile

📚 Learning: 2025-08-31T04:06:57.364Z

Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to **/*.rs : Code must be clippy-clean (cargo clippy --all-targets --all-features -- -D warnings)

Applied to files:

justfile

📚 Learning: 2025-08-31T04:06:57.364Z

Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/tokio/**/*.rs : For async IO (with the tokio feature), implement and use AsyncReadFrom and AsyncWriteTo traits

Applied to files:

Cargo.toml

🧬 Code graph analysis (7)

src/test/hevc.rs (3)

src/moov/trak/mdia/minf/stbl/stsd/mod.rs (1)

decode (115-144)

src/types.rs (9)

decode (74-76)

decode (97-99)

decode (140-142)

decode (203-208)

decode (282-290)

decode (321-325)

new (18-20)

new (189-191)

new (300-302)

src/moov/trak/mdia/minf/stbl/stsd/pasp.rs (1)

new (11-16)

src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs (1)

src/atom.rs (1)

decode_maybe (41-68)

src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs (1)

src/coding.rs (1)

decode_maybe (31-31)

src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs (2)

src/atom.rs (1)

decode_maybe (41-68)

src/coding.rs (1)

decode_maybe (31-31)

src/test/fault_tolerant.rs (1)

src/meta/mod.rs (1)

decoded (344-344)

src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs (2)

src/atom.rs (1)

decode_maybe (41-68)

src/coding.rs (1)

decode_maybe (31-31)

src/moov/trak/mdia/minf/stbl/stsd/av01.rs (1)

src/atom.rs (1)

decode_maybe (41-68)

🔇 Additional comments (71)

src/moov/mvex/mod.rs (1)

9-25: LGTM!

The changes are consistent with the fault-tolerant pattern across the codebase. Removal of Eq derive is appropriate, and the unexpected field is properly gated.

src/meta/mod.rs (1)

161-163: LGTM!

Test fixtures correctly initialize the unexpected field with empty vectors when the fault-tolerant feature is enabled, ensuring tests pass in both configurations.

Also applies to: 234-236

src/moof/traf/mod.rs (1)

11-35: LGTM!

The Traf struct correctly implements the fault-tolerant pattern with the unexpected field for capturing unknown child atoms during fragment decoding.

src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs (2)

40-46: LGTM!

The fault-tolerant branching logic correctly handles unknown atoms:

Always logs a warning for visibility

Collects unknown atoms into unexpected when fault-tolerant is enabled

Returns Error::UnexpectedBox in strict mode

This matches the documented behavior where unexpected boxes are collected during decode but not re-encoded.

63-80: Confirm: unexpected boxes are intentionally not encoded.

The encode_body method correctly omits the unexpected field, which aligns with the PR documentation that unexpected boxes are collected during decoding but not written back during encoding. This is the expected behavior.

src/test/bbb.rs (2)

221-222: LGTM!

The test fixtures comprehensively initialize the unexpected fields across the entire Moov structure hierarchy, ensuring proper round-trip testing with the fault-tolerant feature.

Also applies to: 233-234, 241-242, 279-280

401-406: LGTM!

The Moof and Traf structures in the test are correctly updated with the unexpected field initialization, maintaining test coverage for fragmented MP4 structures.

Also applies to: 465-470

src/moov/trak/edts/mod.rs (1)

6-21: LGTM!

The Edts struct correctly implements the fault-tolerant pattern, consistent with other container atoms in the codebase.

src/test/mod.rs (1)

4-4: LGTM!

The new test module declaration follows the existing pattern and is properly integrated.

src/moof/mod.rs (2)

9-9: Correct removal of Eq derive.

Removing Eq is necessary because the new unexpected: Vec<Any> field may not implement Eq.

14-15: LGTM! Fault-tolerant field correctly gated.

The unexpected field is properly feature-gated and will collect unknown boxes when fault-tolerant mode is enabled.

src/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rs (1)

172-172: Good test simplification.

Using ..Default::default() makes the test more maintainable as new optional fields (like unexpected) are added to the struct.

src/moov/trak/mdia/minf/dinf/mod.rs (2)

6-6: Correct removal of Eq derive.

Removing Eq is necessary because the new unexpected: Vec<Any> field may not implement Eq.

10-11: LGTM! Fault-tolerant field correctly gated.

The unexpected field follows the same pattern as other container atoms in this PR.

justfile (2)

14-14: Good addition: testing without default features.

Adding --no-default-features check ensures the codebase compiles correctly when the fault-tolerant feature is disabled.

22-23: Excellent test coverage expansion.

Testing with both --all-features and --no-default-features ensures the fault-tolerant feature works correctly in both enabled and disabled states. This is critical for a feature that changes parsing behavior.

src/moov/trak/mdia/minf/stbl/mod.rs (2)

29-29: Correct removal of Eq derive.

Removing Eq is necessary because the new unexpected: Vec<Any> field may not implement Eq.

45-46: LGTM! Fault-tolerant field correctly gated.

The unexpected field follows the established pattern and will collect unknown boxes in this sample table container.

src/moov/trak/mdia/mod.rs (2)

11-11: LGTM: Eq removal is correct.

Removing Eq from the derive list is appropriate since Vec<Any> (used in the new unexpected field) may not implement Eq.

17-18: LGTM: Fault-tolerant field addition.

The conditional unexpected field is correctly gated by the fault-tolerant feature. The nested! macro handles the decode/encode logic appropriately.

src/test/vp9.rs (1)

159-160: LGTM: Test assertions updated correctly.

The test expectations are properly updated to include the unexpected: vec![] field under the fault-tolerant feature flag. The empty vector is correct since the test fixtures contain no unexpected boxes.

Also applies to: 198-199, 212-213, 240-241, 254-255, 266-267, 275-276, 300-301

src/moov/trak/mod.rs (1)

11-11: LGTM: Consistent fault-tolerant implementation.

The changes follow the same correct pattern: Eq removed and the unexpected field added under the feature gate.

Also applies to: 19-20

src/moov/trak/mdia/minf/stbl/stsd/ac3.rs (4)

5-5: LGTM: Struct changes are correct.

Removing Eq and adding the conditional unexpected field aligns with the fault-tolerant feature implementation.

Also applies to: 10-11

21-43: LGTM: Fault-tolerant decode logic is sound.

The decode implementation correctly:

Declares the unexpected vector under the feature flag (lines 21-22)

Conditionally handles unknown atoms: collecting them when fault-tolerant is enabled, or returning an error otherwise (lines 27-34)

Includes the unexpected field in the struct construction (lines 41-42)

46-50: LGTM: Encoding correctly omits unexpected boxes.

As documented in the PR objectives, unexpected boxes are not written back during encoding. This implementation correctly encodes only audio and dac3.

136-138: LGTM: Test updates are correct.

Tests properly initialize the unexpected field under the feature flag.

Also applies to: 160-161

src/moov/trak/mdia/minf/mod.rs (1)

13-13: LGTM: Standard fault-tolerant pattern applied correctly.

Also applies to: 20-21

src/test/heif.rs (1)

132-133: LGTM: Test fixtures updated correctly.

The Iprp struct initializations properly include the unexpected: vec![] field under the fault-tolerant feature flag.

Also applies to: 415-416

src/moov/udta/mod.rs (2)

7-7: LGTM: Consistent implementation.

Also applies to: 11-12

31-35: LGTM: Tests properly updated.

Both test cases correctly initialize the unexpected field under the feature flag.

Also applies to: 55-56

src/atom.rs (4)

178-179: LGTM: Unexpected vector declared under feature flag.

The unexpected vector is correctly initialized only when the fault-tolerant feature is enabled.

203-213: LGTM: Fault-tolerant branching is correct.

The catch-all branch properly handles unexpected boxes:

When fault-tolerant is enabled: logs a warning and accumulates the box in unexpected

When disabled: returns Error::UnexpectedBox as before

This maintains backward compatibility while enabling lenient parsing when desired.

217-223: LGTM: Struct construction includes unexpected field.

The generated struct construction correctly includes the unexpected field conditionally when the feature is enabled.

226-232: LGTM: Encoding correctly omits unexpected boxes.

The encode_body implementation only encodes required, optional, and multiple fields, deliberately excluding unexpected. This aligns with the documented behavior that unexpected boxes are not written back during encoding.

src/moov/trak/mdia/minf/stbl/stsd/av01.rs (2)

6-17: LGTM - Fault-tolerant field addition is consistent with the pattern.

The Eq removal is necessary since Vec<Any> may not implement Eq, and the feature-gated unexpected field follows the established pattern across the codebase.

40-47: LGTM - Unknown atom handling correctly branches on feature flag.

The implementation properly logs a warning, then either collects the atom (fault-tolerant) or returns an error (strict mode).

src/moov/trak/mdia/minf/stbl/stsd/flac.rs (1)

3-10: LGTM - Consistent fault-tolerant implementation for FLAC atom.

The pattern matches other codec implementations with proper feature gating.

src/moov/trak/mdia/minf/stbl/stsd/mod.rs (2)

46-50: LGTM - Stsd derive update is consistent.

The Eq removal aligns with the broader fault-tolerant changes where contained types (codecs) now may include Vec<Any> which doesn't implement Eq.

53-112: LGTM - Codec enum properly handles unknown sample entries.

The Unknown(FourCC) variant correctly captures unknown codec types while the error path at line 142 appropriately rejects atoms that aren't valid sample entries.

src/moov/trak/mdia/minf/stbl/stsd/uncv.rs (1)

5-16: LGTM - Fault-tolerant implementation for Uncv follows established pattern.

src/test/hevc.rs (2)

214-221: LGTM - Non-fault-tolerant path correctly asserts error on unexpected box.

The test properly validates that without the fault-tolerant feature, the fiel box causes an UnexpectedBox error.

642-655: LGTM - Test correctly handles non-encoding of unexpected boxes.

Clearing unexpected before assert_encode_decode is the right approach since unexpected boxes are intentionally not re-encoded.

src/moov/trak/mdia/minf/stbl/stsd/opus.rs (1)

3-10: LGTM - Fault-tolerant implementation for Opus follows established pattern.

src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs (1)

4-11: LGTM - Fault-tolerant implementation for Vp08 follows established pattern.

src/test/fault_tolerant.rs (3)

1-108: LGTM - Comprehensive test for fault-tolerant behavior.

The test properly exercises both feature configurations:

With fault-tolerant: validates unexpected boxes are collected and accessible

Without fault-tolerant: validates the appropriate error is returned

The manual construction of the moov buffer with an injected mdat box is a good approach for testing this edge case.

110-185: LGTM - Good coverage for multiple unexpected boxes scenario.

This test validates that the implementation correctly handles and preserves multiple unexpected boxes in order.

187-226: LGTM - Important test verifying non-encoding of unexpected boxes.

This test confirms the documented behavior that unexpected boxes are collected during decode but not written back during encode, which is essential for the round-trip semantics.

src/test/uncompressed.rs (1)

92-93: LGTM! Consistent fault-tolerant field additions across test expectations.

The unexpected: vec![] fields are correctly added to all nested structures (Moov, Trak, Mdia, Minf, Dinf, Stbl, Uncv codec, Udta) under the #[cfg(feature = "fault-tolerant")] gate. This aligns with the PR's design where well-formed files should decode with empty unexpected vectors.

Also applies to: 117-118, 145-146, 159-160, 171-172, 180-181, 253-254, 300-301

src/test/av1.rs (1)

114-115: LGTM! Comprehensive fault-tolerant field coverage in AV1 test.

The test correctly includes unexpected: vec![] for all container structures including Moof and Traf, ensuring the fragmented MP4 path is also covered by the fault-tolerant feature testing.

Also applies to: 138-139, 150-151, 178-179, 192-193, 204-205, 213-214, 250-251, 273-274, 290-291, 294-295

src/test/flac.rs (1)

242-243: LGTM! Fault-tolerant fields correctly added to FLAC audio codec test.

The test properly includes the feature-gated unexpected field for the Flac codec structure alongside the container structures.

Also applies to: 266-267, 278-279, 306-307, 320-321, 325-326, 334-335, 370-371

src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs (2)

4-11: LGTM! Clean implementation of fault-tolerant decoding for VP09.

The struct correctly:

Removes Eq derive (since Vec<Any> may not implement Eq)

Adds the feature-gated unexpected field

Uses Default derive for cleaner test initialization

20-42: Decoding logic is correct and follows the established pattern.

The implementation properly:

Initializes unexpected vector under feature gate

Logs a warning for unknown atoms (useful for debugging)

Conditionally collects or errors based on feature flag

Includes unexpected in the constructed struct

src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs (2)

7-16: LGTM! Fault-tolerant support correctly added to Mp4a.

The struct properly removes Eq and adds the feature-gated unexpected field.

28-55: Decoding logic correctly implements fault-tolerant pattern.

The implementation properly collects unknown atoms when the feature is enabled and returns an error otherwise.

src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs (3)

3-15: LGTM! Well-structured fault-tolerant implementation for HEV1.

The struct correctly adds Default derive alongside the feature-gated unexpected field, enabling cleaner test initialization with ..Default::default().

28-59: Decoding logic is correct and consistent with other codec implementations.

The fault-tolerant handling follows the established pattern across the codebase.

87-104: Good test simplification using struct update syntax.

Using ..Default::default() at line 103 is cleaner than explicitly setting each optional field to None and unexpected: vec![], and it's future-proof if new optional fields are added.

src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs (3)

3-15: LGTM! Fault-tolerant support correctly implemented for AVC1.

The implementation mirrors the pattern used in other codec implementations (HEV1, VP09, etc.).

28-59: Decoding logic follows the established fault-tolerant pattern.

Correctly accumulates unknown atoms when fault-tolerant is enabled and errors otherwise.

85-177: Comprehensive test coverage for AVC1 with and without optional fields.

Both test_avc1 and test_avc1_with_extras correctly include the feature-gated unexpected field. The explicit initialization style (vs. ..Default::default()) is acceptable as it makes the test expectations clearer.

src/test/esds.rs (1)

150-151: LGTM! Comprehensive fault-tolerant field coverage in ESDS test.

The test correctly adds unexpected: vec![] to all relevant structures across both video (AVC1) and audio (MP4A) tracks, as well as the fragmented container structures (Moof, Traf). The use of ..Default::default() at strategic locations (lines 315, 318) simplifies the test while maintaining clarity.

Also applies to: 162-163, 170-171, 208-209, 238-239, 254-255, 291-292, 315-316, 326-327, 330-331

src/moov/trak/mdia/minf/stbl/stsd/eac3.rs (4)

5-12: LGTM! Well-structured fault-tolerant feature implementation.

The struct correctly:

Removes Eq derivation (since Vec<Any> may not implement Eq)

Adds the unexpected field gated behind #[cfg(feature = "fault-tolerant")]

Uses public visibility to allow inspection of collected unknown boxes

21-43: Decoding logic correctly implements fault-tolerant behavior.

The implementation properly:

Logs a warning for unknown atoms in both modes (good for debugging)

Collects unknown atoms into unexpected when the feature is enabled

Returns Error::UnexpectedBox when the feature is disabled

Conditionally includes unexpected in the struct construction

46-50: Encoding intentionally omits unexpected boxes.

As documented in the PR objectives, unexpected boxes are not written back during encoding. This is the expected behavior - the unexpected field is purely for inspection/debugging of parsed content.

196-199: Test assertions correctly include the fault-tolerant field.

The use of #[cfg(feature = "fault-tolerant")] on struct field initialization in assertions is idiomatic and ensures tests compile under both feature configurations.

src/moov/mod.rs (2)

142-145: Test correctly propagates fault-tolerant fields through the hierarchy.

The test comprehensively includes unexpected: vec![] at all levels of the nested structure (Mvex, Edts, Dinf, Minf, Mdia, Trak, and Moov), ensuring the fault-tolerant feature is properly integrated throughout the module hierarchy.

21-23: Fault-tolerant field addition follows macro implementation correctly.

The nested! macro correctly handles the fault-tolerant feature: it declares and populates the unexpected vector when the feature is enabled, collects unrecognized atoms into it during decode, and includes the field in the struct construction. The pattern is consistent across all structs using this macro.

src/test/h264.rs (4)

121-122: Test correctly includes fault-tolerant field at root level.

The unexpected: vec![] field is properly placed at the top level of the Moov struct with the appropriate cfg gate.

698-706: Moof/Traf test correctly includes fault-tolerant fields.

The encrypted segment test properly includes unexpected: vec![] for both Moof and Traf structures, ensuring the fault-tolerant feature works across different MP4 atom hierarchies.

409-414: Udta test construction properly includes fault-tolerant field.

The explicit construction of Udta with unexpected: vec![] and meta: None ensures the test validates the fault-tolerant feature in the user data box as well.

275-276: Using ..Default::default() for Avc1 is a clean pattern that reduces boilerplate.

Avc1 derives Default, which correctly provides unexpected: vec![] when the fault-tolerant feature is enabled. This approach maintains consistency across the test without manual field initialization.

Cargo.toml

README.md

src/lib.rs

In general, anything derived from ISOBMFF is intended to allow other information in the file, and that can be ignored (see ISO/IEC 14496-12:2022 Section B.2.1). So while there are cases for strict behaviour, we should be chill where practical. Except where we are testing ourselves. Make sure that works.

kixelated · 2025-12-27T11:19:37Z

A feature flag is not the right approach because they're meant to be additive. Enabling fault-tolerant will break existing code (including downstream dependencies) because of the new struct member. It's a pain.

Also, it feels kind of gross to add unknown to every box. I don't know, the whole point of the library is to strongly type MP4 to the best of our ability. I feel like we've failed if library users have to actually use unknown.

I think we can instead log unknown boxes and skip them. If there's some non-standard box that users need parsed, we should parse it instead of dumping everything into unknown.

bradh · 2025-12-27T22:52:55Z

I think we can instead log unknown boxes and skip them.

Is the intent that this is the "always" case, or do you want some kind of option (feature flag, global configuration object, parse fn boolean, or something else) for this?

kixelated · 2025-12-28T00:50:18Z

I think we can instead log unknown boxes and skip them.

Is the intent that this is the "always" case, or do you want some kind of option (feature flag, global configuration object, parse fn boolean, or something else) for this?

I would turn it on globally. The application can opt-out of tracing logs on a per-crate basis. There's some performance hit of course, but not as much as Vec<Unknown>.

kixelated · 2025-12-28T00:54:04Z

The only problem with tracing is that it's an opt-in logger. Somebody using log won't see any warnings about skipped boxes. We could make it a feature, so they can choose tracing or log (or both).

Then again, they definitely won't check if every possible unknown field is populated. Opting into a logger is a visibility improvement at least.

kixelated · 2025-12-29T09:38:26Z

Does that seem reasonable @bradh ? I can make a PR to log instead of error on unknown boxes (which we already do in some places). I think that's a less abrasive way to fix #63

bradh · 2025-12-29T10:32:57Z

Does that seem reasonable @bradh ? I can make a PR to log instead of error on unknown boxes (which we already do in some places). I think that's a less abrasive way to fix #63

I am good with that. Happy for you to do it, or I can rework the PR.

b01o · 2025-12-29T11:20:45Z

log instead of error on unknown boxes (which we already do in some places)

Upvote, it's always good to keep it consistent.

@kixelated Are you good with the fault-tolerant feature being default?

Due to the breakage that could introduce, we better not make it default if we were to do it.

I don't know, the whole point of the library is to strongly type MP4 to the best of our ability. I feel like we've failed if library users have to actually use unknown.
they definitely won't check if every possible unknown field is populated.

Yes and yes, this PR is indeed too messy, and the newly added unknown variant likely won't get used. I'm wondering if there are downstream users who already depend on this fail-fast behavior. Perhaps we could make the 'log-and-continue on unknown atoms' behavior opt-in via a feature flag, rather than the default behavior? This way, we'd be more likely to receive feedback about implementation errors, and it would stay closer to the original intent of this crate: a strongly-typed, correct MP4 box implementation.

kixelated · 2025-12-29T22:39:33Z

I'm wondering if there are downstream users who already depend on this fail-fast behavior. Perhaps we could make the 'log-and-continue on unknown atoms' behavior opt-in via a feature flag, rather than the default behavior? This way, we'd be more likely to receive feedback about implementation errors, and it would stay closer to the original intent of this crate: a strongly-typed, correct MP4 box implementation.

I doubt anybody cares. I mostly decided to return an error to help catch bugs or unhandled atoms, not to be pedantic. Unit tests might be the one place I would want to error instead of log.

bradh · 2025-12-31T02:44:49Z

Closed in favour of #84

b01o and others added 12 commits December 27, 2025 09:03

doc tweaks

fab4a05

Adds documentation to README and library root regarding the fault-tolerant parsing feature. This explains how unexpected child boxes are handled during decoding and clarifies that they are not written back during encoding.

chore: fmt

6e8b6be

cargo fmt don't work for some reason.

Adds fault-tolerant parsing feature

6c6d3b3

Implements a new `fault-tolerant` feature that enables parsing of MP4 files with unexpected boxes. Updates documentation and tests to reflect the feature.

Adds unexpected field for all SampleEntry boxes

fcb4659

Doc tweaks

36c03ed

fix(hev1): unexpected boxes should not be encoded here

7c53624

Enables strict error handling when fault tolerance is off

3c497c4

fixes HEVC test to reflect fault tolerance

9edfd07

adds a comment to clarify test behavior

fb09bc3

fix up tests for fault-tolerant mode

ca64acf

bradh requested a review from kixelated December 27, 2025 00:12

coderabbitai bot reviewed Dec 27, 2025

View reviewed changes

Cargo.toml Show resolved Hide resolved

README.md Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

bradh force-pushed the feat/fault-tolerant branch from 905f026 to e6b7860 Compare December 27, 2025 00:28

This was referenced Dec 27, 2025

chore: minor comment fix #82

Merged

mp4a: fix taic encoding #83

Merged

kixelated mentioned this pull request Dec 29, 2025

Log unknown atoms instead of (sometimes) returning an error. #84

Merged

bradh closed this Dec 31, 2025

feat: Collects unexpected boxes during decoding #81

feat: Collects unexpected boxes during decoding #81

Uh oh!

Conversation

bradh commented Dec 27, 2025

Uh oh!

bradh commented Dec 27, 2025

Uh oh!

bradh commented Dec 27, 2025

Uh oh!

coderabbitai bot commented Dec 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kixelated commented Dec 27, 2025

Uh oh!

bradh commented Dec 27, 2025

Uh oh!

kixelated commented Dec 28, 2025

Uh oh!

kixelated commented Dec 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kixelated commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradh commented Dec 29, 2025

Uh oh!

b01o commented Dec 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kixelated commented Dec 29, 2025

Uh oh!

bradh commented Dec 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Dec 27, 2025 •

edited

Loading

kixelated commented Dec 28, 2025 •

edited

Loading

kixelated commented Dec 29, 2025 •

edited

Loading

b01o commented Dec 29, 2025 •

edited

Loading