feat: Collects unexpected boxes during decoding #71
Conversation
WalkthroughAdds a Cargo feature Pre-merge checks and finishing touches✅ Passed checks (5 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 |
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.
b01o
force-pushed
the
feat/fault-tolerant
branch
from
e63f244 to
2654b8e
Compare
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.
b01o
force-pushed
the
feat/fault-tolerant
branch
from
c01bdb7 to
0677cad
Compare
b01o
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/moov/mod.rs (1)
13-22: Bump major version for semver-breakingunexpectedfield addition
Addingunexpected: Vec<Any>to public atom structs (e.g.,Moov) is semver-breaking; please increment the crate’s major version (or gate it behind a feature) and document this change in the changelog.
🧹 Nitpick comments (11)
src/test/av1.rs (1)
114-114: Add tests for fault-tolerant parsing of unexpected boxes.Current tests correctly assert
unexpected: vec![]for well-formed MP4 files, but no cases cover inputs with unknown boxes. Add unit or integration tests that feed data containing unexpected atoms and verify they populate theunexpectedfield.src/moov/udta/mod.rs (1)
7-13: Consider serde skip/default for unexpectedAvoid serializing empty vectors and ensure defaults when deserializing.
Apply:
#[derive(Debug, Clone, PartialEq, Default)] pub struct Udta { pub meta: Option<Meta>, pub skip: Option<Skip>, - pub unexpected: Vec<Any>, + #[cfg_attr(feature = "serde", serde(default, skip_serializing_if = "Vec::is_empty"))] + pub unexpected: Vec<Any>, }src/moof/traf/mod.rs (1)
11-11: Add serde attrs and module test forunexpectedinTraf
- Add
#[cfg_attr(feature = "serde", serde(default, skip_serializing_if = "Vec::is_empty"))]to theunexpectedfield.- Add a doc comment clarifying it’s populated on decode for unknown children and not re-encoded.
- Add a
#[cfg(test)]module test insrc/moof/traf/mod.rsthat decodes aTrafbox with an unknown child, asserts it appears inunexpected, and verifies re-encoding omits it.src/test/h264.rs (2)
119-163: Add a targeted test with a non‑emptyunexpectedto prove fault‑tolerance.Right now we only assert empty
unexpected. Please add a small fixture (e.g., anhdlrinsideminf, or any extra child likefree) and assert it lands inunexpected, and that re‑encoding omits it.I can help craft a minimal bytestream that places
hdlrunderminfand validateMinf.unexpected.len() == 1. Want me to add that test?Also applies to: 164-206, 216-226, 290-341, 395-402, 684-727
410-415: Consider asserting encode/decode round‑trip once stable.The final equality is commented out. Keep it if flakiness exists, otherwise it’s a strong guard that unexpected boxes aren’t re‑emitted.
src/test/hevc.rs (2)
214-251: Cover non‑emptyunexpectedand omission on re‑encode.Mirror H.264 suggestion: add a tiny unknown/legacy child under a container (e.g.,
minfortraf) and assert:
- decode collects it into
.unexpected- encode+decode drops it (field becomes empty)
Also applies to: 252-296, 300-312, 585-596
603-607: Optionally restore end‑to‑end byte equality.If encode stability is acceptable, re‑enable the final buffer equality for stronger regression protection.
src/moov/mod.rs (2)
13-22: Document theunexpectedfield semantics.Add a brief doc comment stating it’s populated on decode and never encoded back. Helps users and serde consumers.
Apply this diff:
#[derive(Debug, Clone, PartialEq, Default)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub struct Moov { @@ pub udta: Option<Udta>, - pub unexpected: Vec<Any>, + /// Unexpected child atoms encountered during decoding. Populated by the decoder and not emitted during encoding. + pub unexpected: Vec<Any>, }
132-143: Tests updated correctly; consider adding one with an actual unexpected child at Moov level.Great to see defaults asserted. A small Moov containing an extra child (e.g.,
free) would validate collection at the top level too.Also applies to: 154-166, 187-235, 236-241
src/test/esds.rs (2)
149-175: Good alignment with the new field; add one test exercisingMoof/Traf.unexpected.Decode a
moofcarrying an extra box undertrafand assert it lands inunexpectedand is omitted on re‑encode.Also applies to: 232-255, 315-349
102-115: Optional: factor helpers to reduce test churn across added fields.Introduce small helpers/macros to assert empty
unexpectedrecursively, or builder helpers with..Default::default()where possible, to reduce future edits.Also applies to: 352-359
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
README.md(1 hunks)src/atom.rs(2 hunks)src/lib.rs(1 hunks)src/meta/iprp.rs(1 hunks)src/meta/mod.rs(2 hunks)src/moof/mod.rs(1 hunks)src/moof/traf/mod.rs(2 hunks)src/moov/mod.rs(5 hunks)src/moov/mvex/mod.rs(1 hunks)src/moov/trak/edts/mod.rs(1 hunks)src/moov/trak/mdia/minf/dinf/mod.rs(1 hunks)src/moov/trak/mdia/minf/mod.rs(1 hunks)src/moov/trak/mdia/minf/stbl/mod.rs(2 hunks)src/moov/trak/mdia/mod.rs(1 hunks)src/moov/trak/mod.rs(1 hunks)src/moov/udta/mod.rs(3 hunks)src/test/av1.rs(8 hunks)src/test/bbb.rs(6 hunks)src/test/esds.rs(5 hunks)src/test/flac.rs(5 hunks)src/test/h264.rs(11 hunks)src/test/heif.rs(2 hunks)src/test/hevc.rs(7 hunks)src/test/uncompressed.rs(6 hunks)src/test/vp9.rs(6 hunks)
🧰 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/moof/mod.rssrc/moov/trak/mdia/minf/dinf/mod.rssrc/moov/trak/mod.rssrc/test/vp9.rssrc/atom.rssrc/test/heif.rssrc/test/av1.rssrc/moov/trak/edts/mod.rssrc/test/flac.rssrc/test/hevc.rssrc/test/uncompressed.rssrc/lib.rssrc/test/h264.rssrc/moov/udta/mod.rssrc/moov/trak/mdia/mod.rssrc/moov/mvex/mod.rssrc/meta/mod.rssrc/moov/mod.rssrc/moof/traf/mod.rssrc/meta/iprp.rssrc/test/esds.rssrc/moov/trak/mdia/minf/stbl/mod.rssrc/test/bbb.rssrc/moov/trak/mdia/minf/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/moof/mod.rssrc/moov/trak/mdia/minf/dinf/mod.rssrc/moov/trak/mod.rssrc/test/vp9.rssrc/atom.rssrc/test/heif.rssrc/test/av1.rssrc/moov/trak/edts/mod.rssrc/test/flac.rssrc/test/hevc.rssrc/test/uncompressed.rssrc/lib.rssrc/test/h264.rssrc/moov/udta/mod.rssrc/moov/trak/mdia/mod.rssrc/moov/mvex/mod.rssrc/meta/mod.rssrc/moov/mod.rssrc/moof/traf/mod.rssrc/meta/iprp.rssrc/test/esds.rssrc/moov/trak/mdia/minf/stbl/mod.rssrc/test/bbb.rssrc/moov/trak/mdia/minf/mod.rs
src/moof/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place movie fragment atom implementations under src/moof/
Files:
src/moof/mod.rssrc/moof/traf/mod.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/mdia/minf/dinf/mod.rssrc/moov/trak/mod.rssrc/moov/trak/edts/mod.rssrc/moov/udta/mod.rssrc/moov/trak/mdia/mod.rssrc/moov/mvex/mod.rssrc/moov/mod.rssrc/moov/trak/mdia/minf/stbl/mod.rssrc/moov/trak/mdia/minf/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/vp9.rssrc/test/heif.rssrc/test/av1.rssrc/test/flac.rssrc/test/hevc.rssrc/test/uncompressed.rssrc/test/h264.rssrc/test/esds.rssrc/test/bbb.rs
src/meta/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place metadata (HEIF/AVIF) atom implementations under src/meta/
Files:
src/meta/mod.rssrc/meta/iprp.rs
🧠 Learnings (2)
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
PR: kixelated/mp4-atom#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/lib.rs
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
PR: kixelated/mp4-atom#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/mdia/mod.rsREADME.md
🔇 Additional comments (30)
src/moov/trak/mdia/minf/dinf/mod.rs (1)
6-11: LGTM! Consistent fault-tolerant parsing pattern.The addition of the
unexpectedfield and removal ofEqalign with the broader PR objective to collect unexpected boxes during decoding. TheEqremoval is necessary becauseVec<Any>cannot implementEqifAnycontains variants that don't implementEq.src/moov/trak/mdia/minf/mod.rs (1)
13-21: LGTM! Consistent with the fault-tolerant parsing pattern.The changes mirror the pattern applied to other container boxes throughout the codebase. The
unexpectedfield will collect child boxes that don't match the expected schema during decoding.src/meta/mod.rs (2)
155-162: LGTM! Test initialization is correct.The test correctly initializes the new
unexpectedfield for theDinfstruct, ensuring the equality assertion passes with the expanded public API.
177-234: LGTM! Test initialization is correct.The test correctly initializes the new
unexpectedfield for theIprpstruct, maintaining consistency with the expanded public API.src/moof/mod.rs (1)
9-15: LGTM! Fault-tolerant parsing for movie fragments.The
Moof(movie fragment) box now collects unexpected child boxes during decoding, aligning with the PR's objective to support malformed or extended MP4 files. TheEqremoval is necessary due to theVec<Any>field.src/atom.rs (2)
178-214: Fault-tolerant parsing implementation looks solid.The macro now collects unexpected child boxes into the
unexpectedfield instead of returningError::UnexpectedBox. The implementation correctly:
- Initializes an empty
unexpectedvector (lines 178-179)- Logs warnings for unknown and unexpected boxes (lines 199, 202)
- Aggregates unexpected boxes for inspection (line 203)
- Includes the field in the decoded structure (line 212)
This enables post-processing of malformed or extended MP4 files as intended.
216-222: Verify that droppingunexpectedboxes during encoding is intentional.The
encode_bodyimplementation does not write back theunexpectedboxes (they're omitted from the encoding logic at line 219). This means boxes collected during fault-tolerant decoding will be lost on re-encoding.According to the PR objectives and AI summary, this is documented as intentional behavior. However, please confirm:
- Is this the desired behavior for all use cases?
- Should there be an option to preserve these boxes during round-trip encoding?
- Is this behavior clearly documented in the public API docs?
src/moov/trak/edts/mod.rs (1)
6-11: LGTM! Consistent pattern applied to Edts.The Edit Box (
Edts) now supports fault-tolerant parsing by collecting unexpected child boxes. The changes are consistent with the broader PR pattern.src/moov/trak/mdia/minf/stbl/mod.rs (1)
29-46: LGTM! Sample table box now supports fault-tolerant parsing.The
Stbl(Sample Table Box) gains theunexpectedfield, enabling collection of non-standard or extended child boxes during decoding. This is particularly useful for handling vendor-specific extensions.src/test/vp9.rs (7)
159-159: LGTM! Test correctly initializes the new field.The
Moovstruct in the test expectation correctly includesunexpected: vec![], ensuring the assertion passes with the expanded public API.
197-197: LGTM! Test correctly initializes Mvex.
210-210: LGTM! Test correctly initializes Trak.
237-237: LGTM! Test correctly initializes Mdia.
250-250: LGTM! Test correctly initializes Minf.
261-261: LGTM! Test correctly initializes Dinf.
269-269: LGTM! Test correctly initializes Stbl.All nested container structures in the VP9 test now properly initialize the
unexpectedfield, maintaining test correctness with the fault-tolerant parsing changes.README.md (1)
20-25: LGTM! Clear documentation of fault-tolerant parsing behavior.The documentation clearly explains the new fault-tolerant parsing feature, including the important caveat that unexpected boxes are not re-encoded. This helps users understand the implications of using this feature.
src/moov/trak/mod.rs (2)
11-11: Correct removal ofEqderive.The removal of
Eqis necessary because the newunexpected: Vec<Any>field only supportsPartialEq. This is the correct approach.
19-19: Publicunexpectedfield enables fault-tolerant parsing.The new public field allows users to inspect unexpected boxes encountered during decoding. This is a breaking change for code that constructs
Trakusing struct literals, but theDefaultimplementation provides a migration path.src/lib.rs (1)
19-24: LGTM! Crate-level documentation clearly explains fault-tolerant parsing.The documentation appropriately describes the new fault-tolerant parsing behavior at the crate level, making it discoverable in the generated docs.
src/meta/iprp.rs (1)
11-11: LGTM! Consistent addition ofunexpectedfield.The
unexpectedfield addition toIprpfollows the same pattern as other container boxes in this PR, enabling fault-tolerant parsing for HEIF/AVIF metadata structures.src/moov/mvex/mod.rs (1)
9-9: LGTM! Consistent changes to enable fault-tolerant parsing.The removal of
Eqand addition of theunexpectedfield follow the established pattern for container boxes in this PR.Also applies to: 14-14
src/moov/trak/mdia/mod.rs (1)
11-11: LGTM! Changes enable fault-tolerant parsing for media box.These changes allow the
Mdiacontainer to collect unexpected child boxes, directly addressing issue #63 where anhdlrbox inminfwas treated as unexpected.Also applies to: 17-17
src/test/uncompressed.rs (1)
92-92: LGTM! Test expectations correctly updated.All test assertions properly initialize the
unexpectedfield with empty vectors, consistent with the changes in other test files.Also applies to: 116-116, 143-143, 156-156, 167-167, 175-175, 292-292
src/test/heif.rs (1)
132-132: LGTM: tests updated for Iprp.unexpectedInitializing unexpected: vec![] matches the new API and keeps assertions precise.
Also applies to: 414-414
src/test/flac.rs (1)
242-242: LGTM: tests reflect unexpected field across containersAccurately initializes unexpected: vec![] throughout the expected structure.
Also applies to: 266-266, 276-276, 303-303, 316-316, 320-320, 328-328
src/moov/udta/mod.rs (1)
29-35: LGTM: unit tests updated for unexpectedBoth tests correctly include unexpected: vec![] and validate round‑trip.
Also applies to: 46-57
src/test/bbb.rs (1)
221-221: LGTM: tests updated to include unexpected across mdia/minf/dinf and moof/trafMatches the new fault‑tolerant surface; assertions remain precise.
Also applies to: 233-233, 240-240, 305-305, 320-320, 392-392, 395-395, 454-454, 457-457
src/test/esds.rs (1)
149-175: Minf already collects unknown boxes intounexpected– thenested!macro pushes any non‐declared child (e.g. anhdlrunderMinf) intoMinf.unexpectedrather than erroring.src/moov/mod.rs (1)
24-32: All container Atom impls usenested!to collect unknown children
Thenested!macro inatom.rsinitializes and aggregates anunexpected: Vec<Any>for any unmatched boxes, and it’s invoked in every container (Moov, Trak, Mdia, Minf, Stbl, Udta, Mvex, Moof, Traf, etc.).
bradh
left a comment
bradh
left a comment
There was a problem hiding this comment.
If we are going to do this, it probably should work the same for meta (which is a pseudo-container).
It possibly should be the same way we handle extra stuff below SampleEntry. That is, given this part from ISO/IEC 14496-12 8.5.2.1:
All boxes may contain “extra boxes” not explicitly defined in the box syntax of this or
derived specifications. When present, such boxes shall follow all defined fields and should follow any defined contained boxes. Decoders shall presume a sample entry box could contain extra boxes and shall continue parsing as though they are present until the containing box length is exhausted.
|
The bot review comments include a part about skipping serialisation, which I'm not confident about, but might be worth considering more seriously. |
|
I am not sure about the skipping serialization part either. The |
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.
The MetaBox (14496-12 8.11.1.2) is kind of special IMO, it currently haven't been well-implemented yet1. Maybe we should leave it to another dedicated pr?
I don't see that paragraph from my specs(14496-12/2012), can you double confirm is that the case before I go ahead and put the logic for these boxes? Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/av1.rs (1)
114-115: Good use of cfg gating for unexpected fields; consider a helper to reduce repetition.Pattern is correct and safe (field first avoids comma pitfalls). To cut noise across tests, consider a tiny helper macro:
#[cfg(feature = "fault-tolerant")] macro_rules! ft_unexpected { () => { unexpected: vec![], } } #[cfg(not(feature = "fault-tolerant"))] macro_rules! ft_unexpected { () => {} }Then use
ft_unexpected!()inside struct literals. Keeps tests concise and consistent.Also applies to: 138-139, 150-151, 178-179, 192-193, 204-205, 213-214, 271-272, 289-290, 293-294
src/test/fault_tolerant.rs (2)
10-34: Remove unused initial encoding to reduce noise.The Moov encoded into
bufisn’t used later. Safe to drop these lines.Apply:
- let mut buf = Vec::new(); - // First encode a valid Moov with mvhd - let moov = Moov { - mvhd: Mvhd { - creation_time: 0, - modification_time: 0, - timescale: 1000, - duration: 0, - rate: 1.into(), - volume: 1.into(), - next_track_id: 1, - ..Default::default() - }, - meta: None, - mvex: None, - trak: vec![], - udta: None, - #[cfg(feature = "fault-tolerant")] - unexpected: vec![], - }; - - // Encode the moov - moov.encode(&mut buf).unwrap(); + // Define a valid mvhd to embed in a moov payload + let moov = Moov { + mvhd: Mvhd { + creation_time: 0, + modification_time: 0, + timescale: 1000, + duration: 0, + rate: 1.into(), + volume: 1.into(), + next_track_id: 1, + ..Default::default() + }, + meta: None, + mvex: None, + trak: vec![], + udta: None, + #[cfg(feature = "fault-tolerant")] + unexpected: vec![], + };
61-61: Prefer Cursor for clarity when decoding from a slice.This works, but Cursor is clearer and avoids borrowing surprises:
- let result = Moov::decode(&mut final_buf.as_slice()); + let result = Moov::decode(&mut std::io::Cursor::new(&final_buf));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
Cargo.toml(1 hunks)README.md(1 hunks)src/atom.rs(2 hunks)src/lib.rs(1 hunks)src/meta/iprp.rs(1 hunks)src/meta/mod.rs(2 hunks)src/moof/mod.rs(1 hunks)src/moof/traf/mod.rs(2 hunks)src/moov/mod.rs(5 hunks)src/moov/mvex/mod.rs(1 hunks)src/moov/trak/edts/mod.rs(1 hunks)src/moov/trak/mdia/minf/dinf/mod.rs(1 hunks)src/moov/trak/mdia/minf/mod.rs(1 hunks)src/moov/trak/mdia/minf/stbl/mod.rs(2 hunks)src/moov/trak/mdia/mod.rs(1 hunks)src/moov/trak/mod.rs(1 hunks)src/moov/udta/mod.rs(3 hunks)src/test/av1.rs(8 hunks)src/test/bbb.rs(6 hunks)src/test/esds.rs(5 hunks)src/test/fault_tolerant.rs(1 hunks)src/test/flac.rs(5 hunks)src/test/h264.rs(11 hunks)src/test/heif.rs(2 hunks)src/test/hevc.rs(7 hunks)src/test/mod.rs(1 hunks)src/test/uncompressed.rs(6 hunks)src/test/vp9.rs(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/moov/trak/edts/mod.rs
- src/test/uncompressed.rs
- src/test/flac.rs
- src/moov/trak/mod.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/moof/mod.rssrc/test/fault_tolerant.rssrc/moov/trak/mdia/mod.rssrc/meta/iprp.rssrc/moov/trak/mdia/minf/mod.rssrc/moov/udta/mod.rssrc/test/bbb.rssrc/moov/mvex/mod.rssrc/test/heif.rssrc/test/h264.rssrc/moov/trak/mdia/minf/stbl/mod.rssrc/moov/mod.rssrc/test/hevc.rssrc/moof/traf/mod.rssrc/moov/trak/mdia/minf/dinf/mod.rssrc/test/mod.rssrc/atom.rssrc/test/vp9.rssrc/meta/mod.rssrc/test/esds.rssrc/test/av1.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/moof/mod.rssrc/test/fault_tolerant.rssrc/moov/trak/mdia/mod.rssrc/meta/iprp.rssrc/moov/trak/mdia/minf/mod.rssrc/moov/udta/mod.rssrc/test/bbb.rssrc/moov/mvex/mod.rssrc/test/heif.rssrc/test/h264.rssrc/moov/trak/mdia/minf/stbl/mod.rssrc/moov/mod.rssrc/test/hevc.rssrc/moof/traf/mod.rssrc/moov/trak/mdia/minf/dinf/mod.rssrc/test/mod.rssrc/atom.rssrc/test/vp9.rssrc/meta/mod.rssrc/test/esds.rssrc/test/av1.rs
src/moof/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place movie fragment atom implementations under src/moof/
Files:
src/moof/mod.rssrc/moof/traf/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/fault_tolerant.rssrc/test/bbb.rssrc/test/heif.rssrc/test/h264.rssrc/test/hevc.rssrc/test/mod.rssrc/test/vp9.rssrc/test/esds.rssrc/test/av1.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/mdia/mod.rssrc/moov/trak/mdia/minf/mod.rssrc/moov/udta/mod.rssrc/moov/mvex/mod.rssrc/moov/trak/mdia/minf/stbl/mod.rssrc/moov/mod.rssrc/moov/trak/mdia/minf/dinf/mod.rs
src/meta/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place metadata (HEIF/AVIF) atom implementations under src/meta/
Files:
src/meta/iprp.rssrc/meta/mod.rs
🧠 Learnings (2)
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
PR: kixelated/mp4-atom#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/mdia/mod.rsREADME.md
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
PR: kixelated/mp4-atom#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/atom.rs
🪛 LanguageTool
README.md
[grammar] ~20-~20: There might be a mistake here.
Context: ...rs/mp4-atom). ## Fault-Tolerant Parsing Enable the fault-tolerant feature to s...
(QB_NEW_EN)
🔇 Additional comments (35)
Cargo.toml (1)
29-29: LGTM!The
fault-tolerantfeature flag is correctly defined following Cargo conventions. The empty dependency list is appropriate since this feature only gates conditional compilation via#[cfg(feature = "fault-tolerant")].src/meta/mod.rs (2)
161-162: LGTM!The test correctly initializes the
unexpectedfield behind the feature gate, maintaining consistency with the struct definition changes across the codebase.
234-235: LGTM!The test correctly initializes the
unexpectedfield behind the feature gate, maintaining consistency with the struct definition changes across the codebase.src/moof/mod.rs (2)
9-9: LGTM!Removing
Eqis correct becauseVec<Any>cannot implementEq(theAnytype contains trait objects). The struct retainsPartialEqfor comparison operations where possible.
14-15: LGTM!The
unexpectedfield is correctly added behind the feature gate, following the consistent pattern across all container structs in this PR.src/moov/mvex/mod.rs (2)
9-9: LGTM!Removing
Eqis correct becauseVec<Any>cannot implementEq. The struct retainsPartialEqfor comparison operations.
14-15: LGTM!The
unexpectedfield is correctly added behind the feature gate, following the consistent pattern across all container structs.src/moov/udta/mod.rs (4)
7-7: LGTM!Removing
Eqis correct becauseVec<Any>cannot implementEq. The struct retainsPartialEqfor comparison operations.
12-13: LGTM!The
unexpectedfield is correctly added behind the feature gate, following the consistent pattern across all container structs.
35-36: LGTM!The test correctly initializes the
unexpectedfield behind the feature gate, ensuring the test compiles and passes with the feature enabled.
58-59: LGTM!The test correctly initializes the
unexpectedfield behind the feature gate, maintaining consistency with other test updates.src/moof/traf/mod.rs (2)
11-11: LGTM!Removing
Eqis correct becauseVec<Any>cannot implementEq. The struct retainsPartialEqfor comparison operations.
24-25: LGTM!The
unexpectedfield is correctly added behind the feature gate, following the consistent pattern across all container structs.src/moov/trak/mdia/minf/dinf/mod.rs (2)
6-6: LGTM!Removing
Eqis correct becauseVec<Any>cannot implementEq. The struct retainsPartialEqfor comparison operations.
10-11: LGTM!The
unexpectedfield is correctly added behind the feature gate, following the consistent pattern across all container structs.src/test/mod.rs (1)
4-4: Tests in fault_tolerant.rs are properly feature-gated
All fault-tolerant tests are annotated with#[cfg(feature = "fault-tolerant")]and the fallback tests with#[cfg(not(feature = "fault-tolerant"))].README.md (1)
20-32: LGTM! Clear documentation of the fault-tolerant feature.The documentation accurately describes the fault-tolerant parsing behavior, including the important detail that encoding does not write unexpected boxes back. The distinction between enabled and disabled modes is clear.
src/test/hevc.rs (1)
218-219: LGTM! Test initializations correctly updated.The test structs are properly initialized with the new
unexpectedfield under the fault-tolerant feature gate, maintaining consistency with the updated struct definitions.Also applies to: 242-243, 254-255, 282-283, 296-297, 308-309, 592-593
src/meta/iprp.rs (1)
11-12: LGTM! Consistent addition of fault-tolerant field.The
unexpectedfield is correctly added with appropriate feature gating, following the same pattern used throughout the codebase.src/moov/trak/mdia/mod.rs (1)
11-11: LGTM! Correct derive updates for fault-tolerant support.The removal of
Eqis necessary sinceVec<Any>does not implementEq. The newunexpectedfield is properly feature-gated and follows the established pattern.Also applies to: 17-18
src/moov/trak/mdia/minf/mod.rs (1)
13-13: LGTM! Consistent derive and field updates.Same pattern as other structs:
Eqremoved due toVec<Any>, andunexpectedfield properly added under the fault-tolerant feature gate.Also applies to: 20-21
src/test/heif.rs (1)
132-133: LGTM! Test data correctly updated.The Iprp test initializations are properly updated with the
unexpectedfield under the fault-tolerant feature gate.Also applies to: 415-416
src/moov/trak/mdia/minf/stbl/mod.rs (1)
29-29: LGTM! Consistent struct updates.The derive and field changes follow the established pattern for fault-tolerant support.
Also applies to: 45-46
src/atom.rs (4)
178-179: LGTM! Proper initialization of fault-tolerant storage.The
unexpectedVec is correctly initialized under the fault-tolerant feature flag to collect unexpected boxes during decoding.
198-211: LGTM! Correct handling of Unknown vs Unexpected boxes.The implementation properly distinguishes between:
- Unknown boxes (line 198-200): Completely unrecognized box types that are always logged but never cause errors or get stored. This provides leniency for proprietary/experimental boxes.
- Unexpected boxes (line 201-211): Recognized box types appearing in the wrong context. When fault-tolerant is enabled, these are collected for inspection; otherwise, they cause an error.
This design addresses the use case from issue #63 where known boxes (like
hdlr) appear in unexpected locations. The opt-in nature via feature flag addresses the previous review concern from bradh.
219-220: LGTM! Conditional field inclusion.The
unexpectedfield is correctly included in the constructed Self only when the fault-tolerant feature is enabled, maintaining backward compatibility.
224-229: LGTM! Intentional omission of unexpected boxes during encoding.The
encode_bodyimplementation correctly does not encode theunexpectedboxes, as documented in the README. This ensures that round-trip encoding produces clean output containing only explicitly defined fields.src/test/bbb.rs (1)
221-222: LGTM! Test assertions properly updated for fault-tolerant feature.The test assertions correctly initialize the new
unexpectedfield with empty vectors, matching the expected behavior when no unexpected boxes are encountered during decoding. The feature gating is consistent throughout.Also applies to: 233-234, 241-242, 307-308, 323-324, 396-397, 400-401, 460-461, 464-465
src/test/h264.rs (1)
121-122: LGTM! Comprehensive test updates for fault-tolerant feature.The test assertions have been thoroughly updated across all nested MP4 box structures (Moov, Mvex, Trak, Mdia, Minf, Dinf, Stbl, Udta, Moof, Traf) to include the
unexpectedfield. The initialization pattern is consistent and correct.Also applies to: 145-146, 167-168, 195-196, 209-210, 221-222, 228-229, 297-298, 325-326, 339-340, 344-345, 351-352, 408-409, 699-700, 705-706
src/test/esds.rs (1)
150-151: LGTM! Test assertions correctly updated.The test code properly initializes the
unexpectedfield for all affected structures (Mdia, Minf, Dinf, Moof, Traf), maintaining consistency with the fault-tolerant feature implementation.Also applies to: 162-163, 170-171, 236-237, 252-253, 324-325, 328-329
src/test/vp9.rs (1)
159-160: LGTM! VP9 test properly updated for fault-tolerant parsing.The test assertions correctly include the
unexpectedfield for all container boxes in the VP9 test case, with proper empty vector initialization.Also applies to: 198-199, 212-213, 240-241, 254-255, 266-267, 275-276
src/moov/mod.rs (2)
21-22: LGTM! Clean addition of fault-tolerant field to Moov.The
unexpectedfield is properly feature-gated and follows the established pattern for collecting unexpected boxes during fault-tolerant decoding. The field is public, allowing inspection of unexpected boxes after decoding.
143-144: LGTM! Test assertions correctly updated.The test code properly initializes the
unexpectedfield throughout the nested structure hierarchy (Mvex, Edts, Dinf, Stbl, Minf, Mdia, Trak, Moov), ensuring test compatibility with the fault-tolerant feature.Also applies to: 164-165, 194-195, 235-240, 243-244, 247-248
src/test/fault_tolerant.rs (2)
1-226: Tests cover success/failure paths, ordering, and non-encoding. LGTM.
1-226: Add test forhdlrhandling when encountered insideminfto verify fault-tolerant behavior.The
Minfstruct (src/moov/trak/mdia/minf/mod.rs) does not includehdlrin its nested atom declaration (onlyDinfandStblare required;VmhdandSmhdare optional). TheHdlrbox is a required child ofMdia, notMinf.The fault-tolerant test file does not currently cover the scenario where
hdlrappears insideminf. To ensure coverage: add a test that decodes aminfcontaining anhdlrbox and verify it is collected as unexpected when the feature is enabled, and errors when disabled—similar to the existingmdat-in-moovtest.
I'm OK with that.
Here is a snip from ISO/IEC 14496-12:2022: It is also in some derived specs, for example the last paragraph in this section of the AV1 bindings: |
b01o
force-pushed
the
feat/fault-tolerant
branch
from
7032c60 to
6b8902b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/moov/trak/mdia/minf/stbl/stsd/mod.rs (1)
118-136: Bug: Encoding Unknown sample entries writes an invalid boxSelf::Unknown(kind) => kind.encode(buf) only emits a FourCC, not a valid sample entry (missing header/body). Either prevent encoding Unknown or round‑trip its full payload.
Suggested minimal fix: fail fast on encode to avoid corrupt output.
impl Encode for Codec { fn encode<B: BufMut>(&self, buf: &mut B) -> Result<()> { match self { - Self::Unknown(kind) => kind.encode(buf), + // Do not attempt to encode unknown sample entries; we don't retain their bodies. + Self::Unknown(kind) => return Err(Error::UnexpectedBox(*kind)), Self::Avc1(atom) => atom.encode(buf), Self::Hev1(atom) => atom.encode(buf), Self::Hvc1(atom) => atom.encode(buf), Self::Vp08(atom) => atom.encode(buf), Self::Vp09(atom) => atom.encode(buf), Self::Mp4a(atom) => atom.encode(buf), Self::Tx3g(atom) => atom.encode(buf), Self::Av01(atom) => atom.encode(buf), Self::Opus(atom) => atom.encode(buf), Self::Uncv(atom) => atom.encode(buf), Self::Flac(atom) => atom.encode(buf), Self::Ac3(atom) => atom.encode(buf), Self::Eac3(atom) => atom.encode(buf), } } }If you want to preserve and re‑emit unknown entries later, consider changing the variant to carry the full Any (breaking change) and implement encode accordingly.
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs (1)
55-63: Bug: Mp4a::encode_body drops taic and redundantly guards btrttaic is decoded but never encoded (data loss). Also, guarding an Option before calling encode is unnecessary if you pattern‑match.
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 let Some(btrt) = &self.btrt { + btrt.encode(buf)?; + } + if let Some(taic) = &self.taic { + taic.encode(buf)?; + } Ok(()) }Note: Intentionally omitting unexpected from encoding is reasonable for now; consider documenting this explicitly in the type or module docs.
🧹 Nitpick comments (14)
src/moov/trak/mdia/minf/stbl/stsd/flac.rs (2)
3-10: Consider documenting theunexpectedfield.The removal of
Eqis correct sinceVec<Any>implements onlyPartialEq. However, the new publicunexpectedfield lacks documentation explaining its purpose and when it's populated.Add a doc comment to clarify the field's role:
pub audio: Audio, pub dfla: Dfla, #[cfg(feature = "fault-tolerant")] + /// Unrecognized child boxes encountered during decoding. + /// Only populated when the `fault-tolerant` feature is enabled. pub unexpected: Vec<Any>,
41-45: Consider adding test coverage for fault-tolerant decoding.The encoding correctly omits
unexpectedboxes (per PR design), but there's no test verifying the fault-tolerant decode path forFlac. While encoding behavior is intentional, a test would document this and verify unknown atoms are collected without breaking decoding.Consider adding a test similar to:
#[cfg(feature = "fault-tolerant")] #[test] fn test_flac_decode_with_unexpected() { // Test that unknown atoms are collected in unexpected field // and don't cause decode to fail }src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs (1)
54-72: Consider adding a test case for unexpected box collection.The current test verifies the happy path with an empty
unexpectedvector. Consider adding a test that decodes a VP09 box containing an unknown child atom to verify that:
- The unknown box is collected into
unexpectedwhenfault-tolerantis enabled- The unknown box is not re-encoded (confirming the one-way collection behavior)
This would strengthen confidence in the fault-tolerant feature's behavior.
Example test structure:
#[cfg(feature = "fault-tolerant")] #[test] fn test_vp09_unexpected_box() { // Encode a VP09 with visual, vpcc, and an extra unknown box // Decode it and verify the unknown box is in `unexpected` // Re-encode and verify the unknown box is omitted }src/moov/trak/mdia/minf/stbl/stsd/uncv.rs (1)
297-456: Consider adding tests for fault-tolerant behavior.The existing tests verify basic encode/decode functionality but don't test the fault-tolerant feature. Consider adding tests that:
- Verify unknown atoms are captured in
unexpectedwhen the feature is enabled- Confirm that encoding does not write back unexpected boxes
- Validate the behavior when unknown atoms are encountered with the feature disabled
Example test structure:
#[cfg(feature = "fault-tolerant")] #[test] fn test_uncv_captures_unexpected_atoms() { // Create test data with an unknown child box // Decode and verify unknown box is in unexpected vec // Encode and verify unknown box is not written back }src/moov/trak/mdia/minf/stbl/stsd/mod.rs (1)
145-154: Preallocate codecs vectorSaves one allocation and avoids growth on large stsd tables.
- let codec_count = u32::decode(buf)?; - let mut codecs = Vec::new(); + let codec_count = u32::decode(buf)?; + let mut codecs = Vec::with_capacity(codec_count as usize);src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs (2)
7-16: Derive changes look good; add serde skip-if-empty on unexpectedEq removal is fine. To keep JSON lean, skip empty unexpected and default it on deserialize (only when serde is enabled).
#[derive(Debug, Clone, PartialEq)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub struct Mp4a { pub audio: Audio, pub esds: Esds, pub btrt: Option<Btrt>, pub taic: Option<Taic>, - #[cfg(feature = "fault-tolerant")] - pub unexpected: Vec<Any>, + #[cfg(feature = "fault-tolerant")] + #[cfg_attr(feature = "serde", serde(skip_serializing_if = "Vec::is_empty", default))] + pub unexpected: Vec<Any>, }
37-41: Reduce log noise for unknown atomsLogging the full Any may dump large payloads. Prefer logging only kind (and maybe size) at debug level.
- tracing::warn!("unknown atom: {:?}", atom); + tracing::debug!("unknown atom: kind={:?}", atom.kind());src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs (2)
14-15: Hide emptyunexpectedin JSON and make it backward-compatible to deserialize.Add serde attributes so empty vectors aren’t emitted and missing fields deserialize to empty.
#[cfg(feature = "fault-tolerant")] + #[cfg_attr(feature = "serde", serde(default, skip_serializing_if = "Vec::is_empty"))] pub unexpected: Vec<Any>,
30-31: Minor: avoid first reallocation for rare additions.If you want to micro-opt, pre-allocate a slot (cheap and harmless).
- let mut unexpected = Vec::new(); + let mut unexpected = Vec::with_capacity(1);src/test/hevc.rs (1)
218-219: LGTM: fault-tolerant expectations added and Unknown(fiel) captured.
- The cfg-gated empty unexpected vectors keep tests stable across features.
- Capturing 'fiel' under Hev1 via Any::Unknown is a good real-world case; round‑trip via assert_encode_decode should ignore it on encode as intended.
Consider a small helper/macro to reduce repeated #[cfg(feature = "fault-tolerant")] unexpected: vec![] boilerplate in tests.
Also applies to: 242-243, 254-255, 282-283, 296-297, 308-309, 581-585, 597-598
src/test/bbb.rs (1)
221-223: LGTM: tests aligned with fault-tolerant surface.Empty unexpected fields are correctly asserted where containers gained the field.
If this pattern appears across many tests, a helper to construct expected structs with empty unexpected could trim duplication.
Also applies to: 234-235, 242-243, 280-281, 309-311, 326-327, 363-364, 400-402, 405-406, 465-466, 469-470
src/moov/mod.rs (1)
21-22: Skip empty unexpected in serde output.Add serde defaults to omit empty arrays from JSON while preserving decode semantics.
Apply:
#[cfg(feature = "fault-tolerant")] - pub unexpected: Vec<Any>, + #[cfg_attr(feature = "serde", serde(skip_serializing_if = "Vec::is_empty", default))] + pub unexpected: Vec<Any>,As per coding guidelines.
src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs (2)
9-10: Omit emptyunexpectedin serde and default it.This keeps JSON clean without affecting decode.
Apply:
#[cfg(feature = "fault-tolerant")] - pub unexpected: Vec<Any>, + #[cfg_attr(feature = "serde", serde(skip_serializing_if = "Vec::is_empty", default))] + pub unexpected: Vec<Any>,
26-31: Reduce log noise: log kind at debug level, not full atom at warn.Dumping the full atom with {:?} at warn can spam logs and leak large payloads. Prefer concise debug context.
Apply:
- tracing::warn!("unknown atom: {:?}", atom); + tracing::debug!(kind = %atom.kind(), parent = %Self::KIND, "collected unexpected child atom");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/moov/mod.rs(5 hunks)src/moov/trak/mdia/minf/stbl/stsd/ac3.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/av01.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/eac3.rs(7 hunks)src/moov/trak/mdia/minf/stbl/stsd/flac.rs(2 hunks)src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs(6 hunks)src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs(6 hunks)src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rs(1 hunks)src/moov/trak/mdia/minf/stbl/stsd/mod.rs(1 hunks)src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/opus.rs(2 hunks)src/moov/trak/mdia/minf/stbl/stsd/uncv.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs(2 hunks)src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs(3 hunks)src/test/av1.rs(9 hunks)src/test/bbb.rs(8 hunks)src/test/esds.rs(7 hunks)src/test/flac.rs(6 hunks)src/test/h264.rs(13 hunks)src/test/hevc.rs(8 hunks)src/test/uncompressed.rs(7 hunks)src/test/vp9.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/uncompressed.rs
- src/test/esds.rs
- src/test/vp9.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rssrc/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/mod.rssrc/test/h264.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rssrc/test/bbb.rssrc/test/av1.rssrc/moov/trak/mdia/minf/stbl/stsd/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/opus.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rssrc/moov/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/eac3.rssrc/test/hevc.rssrc/moov/trak/mdia/minf/stbl/stsd/ac3.rssrc/moov/trak/mdia/minf/stbl/stsd/uncv.rssrc/test/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rssrc/moov/trak/mdia/minf/stbl/stsd/av01.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/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rssrc/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/mod.rssrc/test/h264.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rssrc/test/bbb.rssrc/test/av1.rssrc/moov/trak/mdia/minf/stbl/stsd/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/opus.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rssrc/moov/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/eac3.rssrc/test/hevc.rssrc/moov/trak/mdia/minf/stbl/stsd/ac3.rssrc/moov/trak/mdia/minf/stbl/stsd/uncv.rssrc/test/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rssrc/moov/trak/mdia/minf/stbl/stsd/av01.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/mdia/minf/stbl/stsd/hevc/hvcc.rssrc/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rssrc/moov/trak/mdia/minf/stbl/stsd/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/opus.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rssrc/moov/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/eac3.rssrc/moov/trak/mdia/minf/stbl/stsd/ac3.rssrc/moov/trak/mdia/minf/stbl/stsd/uncv.rssrc/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rssrc/moov/trak/mdia/minf/stbl/stsd/av01.rs
src/test/**
📄 CodeRabbit inference engine (CLAUDE.md)
Put integration tests under src/test/ with sample MP4 files for different codecs
Files:
src/test/h264.rssrc/test/bbb.rssrc/test/av1.rssrc/test/hevc.rssrc/test/flac.rs
🧠 Learnings (2)
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
PR: kixelated/mp4-atom#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/mdia/minf/stbl/stsd/mp4a/mod.rs
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
PR: kixelated/mp4-atom#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/mdia/minf/stbl/stsd/hevc/hvc1.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rssrc/moov/trak/mdia/minf/stbl/stsd/uncv.rssrc/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rssrc/moov/trak/mdia/minf/stbl/stsd/av01.rs
🧬 Code graph analysis (6)
src/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rs (1)
src/moov/trak/mdia/minf/stbl/stsd/colr.rs (1)
default(97-105)
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs (4)
src/atom.rs (1)
decode_maybe(41-68)src/coding.rs (1)
decode_maybe(31-31)src/header.rs (1)
decode_maybe(58-69)src/test/esds.rs (1)
esds(4-370)
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/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.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/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 (44)
src/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rs (1)
172-172: LGTM! Clean use of Default pattern.Using
..Default::default()is idiomatic and cleaner than explicitly setting each optional field. The test remains functionally equivalent while adapting to the newunexpectedfield added by the fault-tolerant feature.src/moov/trak/mdia/minf/stbl/stsd/flac.rs (3)
19-20: LGTM!The feature-gated initialization is correctly placed before the decode loop.
25-29: LGTM!The unknown atom handling correctly collects unexpected boxes when the feature is enabled while maintaining the warning for observability. The fallback behavior (discarding) when the feature is disabled preserves backward compatibility.
36-37: LGTM!The
unexpectedfield is correctly included in the struct initialization, maintaining consistency with the feature-gated definition.src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs (5)
20-21: LGTM!The initialization of the
unexpectedvector is correctly feature-gated and positioned before the atom decoding loop.
26-30: LGTM!The unknown atom handling correctly logs a warning and collects the unexpected box when the feature is enabled, aligning with the fault-tolerant objective.
37-38: LGTM!The
unexpectedfield is correctly included in struct construction with proper feature gating.
42-47: Confirm that omittingunexpectedboxes from encoding is documented.The
encode_bodymethod intentionally does not write back the collectedunexpectedboxes. While this aligns with the PR description's note that "encoding does not write these back," users may expect round-trip fidelity. Ensure this "collect but don't re-encode" behavior is clearly documented in the module or struct-level docs.
4-11: Eq removal fromVp09is necessary and correct.The
Anyenum derives onlyCloneandPartialEq(notEq), and includes anUnknown(FourCC, Vec<u8>)variant. SinceAnydoes not implementEq, theVec<Any>field inVp09cannot implementEqeither. The removal ofEqfrom the derives is therefore necessary to maintain correctness and reflect the actual trait bounds of the contained types.src/moov/trak/mdia/minf/stbl/stsd/opus.rs (3)
3-3: Removal ofEqand addition of fault-tolerant field look good.The removal of
Eqfrom the derive list is necessary becauseVec<Any>does not implementEq. Theunexpectedfield is properly feature-gated and follows the pattern applied across other codec types in this PR.Also applies to: 8-9
26-30: Verify intended behavior when feature is disabled.When the
fault-tolerantfeature is disabled, unknown atoms are logged but silently consumed without returning an error. This makes the decoder always fault-tolerant, with the feature flag only controlling whether unexpected boxes are collected for inspection.Is this the intended behavior? If fault tolerance should be truly optional, the code should return an error when the feature is disabled:
_ => { - tracing::warn!("unknown atom: {:?}", atom); #[cfg(feature = "fault-tolerant")] + { + tracing::warn!("unknown atom: {:?}", atom); + unexpected.push(atom); + } + #[cfg(not(feature = "fault-tolerant"))] - unexpected.push(atom); + return Err(Error::UnexpectedBox(atom.kind())); }If always-tolerant behavior is intentional, consider renaming the feature to better reflect that it controls collection/retention rather than tolerance itself (e.g.,
collect-unexpected-boxes).
42-46: Correctly omits unexpected boxes during encoding.The encode method intentionally does not write back unexpected boxes, which is the correct behavior. Writing unknown boxes could introduce unintended data or corruptions.
src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs (4)
13-14: LGTM: Feature-gated field correctly added.The
unexpectedfield is properly feature-gated and provides a clean way to collect unrecognized child atoms without failing the decode.
20-56: LGTM: Decode path correctly implements fault-tolerant collection.The decode logic properly:
- Initializes the
unexpectedvector when the feature is enabled- Logs warnings for unknown atoms (good for debugging)
- Collects unexpected atoms without failing the decode
- Preserves validation for required boxes (e.g.,
hvcc)
88-112: LGTM: Test correctly uses Default pattern.The test appropriately uses
..Default::default()to initialize remaining fields, which will setunexpectedto an emptyVecwhen thefault-tolerantfeature is enabled. The encode/decode roundtrip validation remains intact.
3-3: No issues found—Eq removal is consistent with codebase patterns and correct.The removal of
Eqis correct: floating point types cannot implementEqbecause NaN != NaN. The script output confirms that all other codec sample entry types (av01, avc1, hvc1, vp09, mp4a, ac3, eac3, flac) also omitEqfrom their derives, using onlyDebug, Clone, PartialEq(withDefaultwhere applicable). This change harmonizesHev1with the established codebase pattern rather than introducing a breaking inconsistency. TheVisualparent structure likely contains floating-point fields that preventEqderivation.src/moov/trak/mdia/minf/stbl/stsd/uncv.rs (3)
29-31: LGTM!The conditional initialization of the
unexpectedvector is properly gated and follows the expected pattern.
47-56: LGTM!The conditional inclusion of the
unexpectedfield in the struct construction is correct and properly feature-gated.
59-76: Verify that not encodingunexpectedboxes aligns with user expectations.The
encode_bodymethod does not write back any boxes stored in theunexpectedfield. Per the PR description, this is intentional—the fault-tolerant feature is for reading non-standard boxes, not round-tripping them. However, users might reasonably expectdecode -> encodeto preserve all boxes, including unexpected ones.Consider adding a note in the documentation or a code comment explaining this behavior to prevent confusion.
src/moov/trak/mdia/minf/stbl/stsd/mod.rs (2)
42-46: Removing Eq is appropriateEq on Stsd isn’t required and would be brittle with nested types; PartialEq is sufficient.
49-53: Eq removal on Codec is fineEnum variants contain complex types; PartialEq suffices and avoids unnecessary constraints.
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs (1)
104-106: Test scaffolding OKIncluding an empty unexpected under the feature gate is consistent with decode/encode expectations.
src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs (2)
3-3: Derive set looks good after addingunexpected.Dropping
Eqis appropriate (Vec typically can’t derive Eq).DefaultandPartialEqare sufficient for tests/serde.
56-57: LGTM on wiring the field through the result.Once the serde attributes are added on the field, this remains compatible.
src/test/h264.rs (1)
121-123: LGTM: consistent cfg-gated unexpected fields in expectations.Matches the feature-gated decoding behavior; no issues spotted.
Also applies to: 145-147, 167-169, 195-197, 209-211, 221-223, 229-231, 297-299, 325-327, 339-341, 345-347, 351-353, 386-388, 411-413
src/test/flac.rs (1)
242-244: LGTM: fault-tolerant expectations wired for FLAC path.Consistent with the rest of the suite.
Also applies to: 266-268, 278-280, 306-308, 320-322, 325-327, 334-336, 370-372
src/test/av1.rs (1)
114-116: LGTM: AV1 tests updated for fault-tolerant containers and sample entry.The cfg-gated unexpected fields and unchanged encode behavior look correct.
Also applies to: 138-140, 150-152, 178-180, 192-194, 204-206, 213-215, 250-252, 273-275, 291-293, 295-297
src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs (1)
16-39: Handle duplicatevpcCdeterministically.If multiple
vpcCatoms appear, current code keeps the last silently. Consider: error on duplicate (strict), or keep first and push subsequent tounexpected(fault-tolerant). Add a small test to lock behavior.Would you like me to draft a test vector and patch to treat duplicate
vpcCexplicitly?src/moov/mod.rs (1)
142-146: Confirm whether Minf should accept optional Hdlr per Issue #63.Codebase inspection reveals that
Minfstruct (src/moov/trak/mdia/minf/mod.rs:15–22) has noHdlrfield, and itsnested!macro (lines 27–31) lists onlyVmhdandSmhdas optional, with noHdlrentry. TheHdlratom is currently handled only at theMdialevel (required).The review asks to verify that
Minfstructurally allowsHdlroptionally without the feature gate, but the current code does not support this. Either the PR is incomplete and must addHdlrtoMinf, or the review comment's interpretation of Issue #63 is incorrect. Manual verification required: review Issue #63 specification and confirm the intended behavior.src/moov/trak/mdia/minf/stbl/stsd/eac3.rs (4)
5-11: LGTM! Feature-gated field addition is correct.The removal of
Eqfrom the derive and addition of the conditionalunexpectedfield follows the fault-tolerant pattern correctly.Eqcannot be derived when the struct containsVec<Any>sinceAnylikely doesn't implementEqdue to its variant-based nature.
21-31: LGTM! Unknown atom handling is correctly implemented.The decode path properly initializes the
unexpectedvector when the feature is enabled and pushes unknown atoms with appropriate warning logs. This aligns with the fault-tolerant design objective.
43-47: LGTM! Intentional non-encoding of unexpected boxes.The encode path correctly does NOT write back unexpected boxes. This is the intended behavior per the PR objectives—unexpected boxes are collected for inspection/debugging but not re-encoded to avoid propagating unknown data.
193-196: LGTM! Test updates are consistent.All test cases properly initialize the
unexpectedfield with an empty vector when the fault-tolerant feature is enabled, ensuring tests pass under both feature configurations.Also applies to: 222-224, 255-257, 306-308, 363-366
src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs (4)
3-14: LGTM! Feature-gated field addition follows the pattern.The derive changes and conditional field addition are consistent with the fault-tolerant design. The removal of
Eqis necessary as discussed in the eac3.rs review.
29-43: LGTM! Unknown atom handling is correctly implemented.The fault-tolerant decode path properly collects unknown atoms with warning logs, matching the pattern established across other codec implementations.
59-75: LGTM! Encoding intentionally omits unexpected boxes.Consistent with the fault-tolerant design, the encode path does not write back unexpected boxes, preventing propagation of unrecognized data.
112-114: LGTM! Tests updated for feature gate.Both test cases properly handle the conditional
unexpectedfield, ensuring compatibility with and without the fault-tolerant feature.Also applies to: 165-167
src/moov/trak/mdia/minf/stbl/stsd/ac3.rs (4)
5-11: LGTM! Feature-gated field addition is correct.The derive changes and conditional field follow the established fault-tolerant pattern. Note that
Ac3SpecificBox(lines 51-60) correctly retainsEqsince it doesn't contain theunexpectedfield—it's a leaf box, not a container.
21-31: LGTM! Unknown atom handling matches the pattern.The decode implementation for fault tolerance is consistent with the other codec files reviewed.
43-47: LGTM! Encoding behavior is correct.The encode path intentionally omits unexpected boxes, consistent with the fault-tolerant design.
133-136: LGTM! Test scaffolding updated correctly.Tests properly handle the conditional field for the fault-tolerant feature.
Also applies to: 157-159
src/moov/trak/mdia/minf/stbl/stsd/av01.rs (3)
6-16: LGTM! Feature-gated field addition is correct.The derive changes and conditional field are consistent with the fault-tolerant pattern. Note that
Av1c(lines 81-95) correctly retainsEqsince it's a leaf configuration box without anunexpectedfield.
30-44: LGTM! Unknown atom handling is correctly implemented.The fault-tolerant decode path properly collects unknown atoms with warning logs, following the established pattern.
60-77: LGTM! Encoding intentionally omits unexpected boxes.Consistent with the fault-tolerant design across all reviewed files, the encode path does not write back unexpected boxes.
| use super::{Btrt, Pasp, Visual}; | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| #[derive(Debug, Clone, PartialEq)] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consider conditional Eq derive when feature is disabled.
The Eq derive was removed from Uncv, likely because Vec<Any> doesn't implement Eq. However, when the fault-tolerant feature is disabled, all fields of Uncv support Eq, so the derive should still be possible. Unconditionally removing Eq is a breaking change for users who don't enable the feature.
Apply this diff to conditionally derive Eq:
-#[derive(Debug, Clone, PartialEq)]
+#[derive(Debug, Clone, PartialEq)]
+#[cfg_attr(not(feature = "fault-tolerant"), derive(Eq))]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
pub struct Uncv {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Debug, Clone, PartialEq)] | |
| #[derive(Debug, Clone, PartialEq)] | |
| #[cfg_attr(not(feature = "fault-tolerant"), derive(Eq))] | |
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | |
| pub struct Uncv { |
🤖 Prompt for AI Agents
In src/moov/trak/mdia/minf/stbl/stsd/uncv.rs around line 5, the Uncv type
unconditionally removed the Eq derive causing a breaking change for builds
without the "fault-tolerant" feature; restore Eq only when that feature is not
enabled by adding a cfg_attr to conditionally derive Eq (e.g., keep
derive(Debug, Clone, PartialEq) and add #[cfg_attr(not(feature =
"fault-tolerant"), derive(Eq))] so Eq is compiled in for non-fault-tolerant
builds).
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/moov/trak/mdia/minf/stbl/stsd/uncv.rs (1)
121-127: Avoid panic in encode; return an error instead.expect() here can crash callers. Encode should be fallible.
- if component.component_type >= 0x8000 { - let component_type_uri = component - .component_type_uri - .as_ref() - .expect("Expected valid URI when component_type is >= 0x8000"); - component_type_uri.as_str().encode(buf)?; - } + if component.component_type >= 0x8000 { + let Some(uri) = component.component_type_uri.as_deref() else { + return Err(Error::MissingContent( + "Cmpd.component_type_uri required when component_type >= 0x8000", + )); + }; + uri.encode(buf)?; + }As per coding guidelines.
♻️ Duplicate comments (2)
src/test/hevc.rs (1)
1-620: Run cargo fmt on testsThere are minor indentation inconsistencies across this file; please run cargo fmt to keep diffs tight.
You can verify locally with:
#!/bin/bash cargo fmt -- --checksrc/test/bbb.rs (1)
1-513: Format the test moduleSome spacing/indent diverges from rustfmt; please run cargo fmt.
#!/bin/bash cargo fmt -- --check
🧹 Nitpick comments (12)
src/moov/trak/mdia/minf/stbl/stsd/av01.rs (3)
6-6: Keep Eq when the feature is offEq was dropped unconditionally. You can retain Eq for non–fault-tolerant builds via cfg_attr.
-#[derive(Debug, Clone, PartialEq, Default)] +#[derive(Debug, Clone, PartialEq, Default)] +#[cfg_attr(not(feature = "fault-tolerant"), derive(Eq))]
15-16: Skip empty unexpected in serde JSONAvoid noisy JSON and keep backward compatibility on deserialize.
#[cfg(feature = "fault-tolerant")] +#[cfg_attr(feature = "serde", serde(default, skip_serializing_if = "Vec::is_empty"))] pub unexpected: Vec<Any>,
41-41: Reduce log volume and cost for unknown atomsLogging the full atom with {:?} can flood logs and allocate. Log just kind/size.
Example:
match &atom { Any::Unknown(kind, bytes) => tracing::warn!(parent=%Self::KIND, %kind, len=bytes.len(), "unexpected child"), _ => tracing::warn!(parent=%Self::KIND, kind=%atom.kind(), "unexpected child"), }src/test/hevc.rs (2)
218-220: Trim test boilerplate for unexpected and prefer defaultsRepeated #[cfg(feature = "fault-tolerant")] unexpected: vec![] can be avoided by using ..Default::default() on parent structs where possible to reduce noise and maintenance.
Example:
Minf { vmhd: Some(Vmhd { ..Default::default() }), dinf: Dinf { dref: Dref { urls: vec![Url { location: "".into() }] } , ..Default::default() }, stbl: Stbl { ..Default::default() }, ..Default::default() // covers fault-tolerant unexpected }Also applies to: 243-244, 255-256, 283-284, 297-298, 309-310, 321-322, 343-346, 579-586, 597-599
619-619: Re-enable exact roundtrip when stableConsider re-enabling assert_eq!(buf, ENCODED) once all writer orderings are finalized to catch regressions.
src/test/bbb.rs (1)
221-223: Reduce duplication for unexpected fields in test expectationsWhere structs implement Default, prefer ..Default::default() to avoid per-struct unexpected: vec![] initializers.
Also applies to: 234-235, 241-247, 279-281, 309-311, 325-327, 335-336, 362-364, 400-406, 464-466
src/moov/trak/mdia/minf/stbl/stsd/eac3.rs (3)
5-5: Keep Eq derive when feature is offRetain Eq for non–fault-tolerant builds using cfg_attr.
-#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, PartialEq)] +#[cfg_attr(not(feature = "fault-tolerant"), derive(Eq))]
10-11: Serde: skip empty unexpected and default on deserializeMirror other structs’ JSON behavior and avoid empty arrays.
#[cfg(feature = "fault-tolerant")] +#[cfg_attr(feature = "serde", serde(default, skip_serializing_if = "Vec::is_empty"))] pub unexpected: Vec<Any>,
128-149: Bound substream count to 3-bit fieldnum_ind_sub is 3 bits (value = len-1). Encoding >8 substreams will truncate. Validate and error early.
Suggestion:
let count = self.substreams.len(); assert!(count >= 1 && count <= 8, "ec-3: substreams must be in 1..=8"); let header = (self.data_rate << 3) | ((count - 1) as u16 & 0b111);Optionally return an Error instead of assert.
src/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rs (1)
137-145: Minor: precompute lengths onceNot required, but caching arr.nalus.len() into a u16 can shave repeated casts in the loop.
src/moov/trak/mdia/minf/stbl/stsd/uncv.rs (1)
59-75: Prefer if-let over is_some()+encode on Option fields.Cleaner and clippy-friendly; avoids relying on Option:Encode blanket impl.
self.visual.encode(buf)?; -if self.cmpd.is_some() { - self.cmpd.encode(buf)?; -} +self.cmpd.as_ref().map(|x| x.encode(buf)).transpose()?; self.uncc.encode(buf)?; -if self.btrt.is_some() { - self.btrt.encode(buf)?; -} -if self.ccst.is_some() { - self.ccst.encode(buf)?; -} -if self.pasp.is_some() { - self.pasp.encode(buf)?; -} +self.btrt.as_ref().map(|x| x.encode(buf)).transpose()?; +self.ccst.as_ref().map(|x| x.encode(buf)).transpose()?; +self.pasp.as_ref().map(|x| x.encode(buf)).transpose()?;As per coding guidelines.
src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs (1)
62-73: Use if-let for Option encodes.Small cleanup; consistent with clippy strictness.
- if self.btrt.is_some() { - self.btrt.encode(buf)?; - } - if self.colr.is_some() { - self.colr.encode(buf)?; - } - if self.pasp.is_some() { - self.pasp.encode(buf)?; - } - if self.taic.is_some() { - self.taic.encode(buf)? - } + self.btrt.as_ref().map(|x| x.encode(buf)).transpose()?; + self.colr.as_ref().map(|x| x.encode(buf)).transpose()?; + self.pasp.as_ref().map(|x| x.encode(buf)).transpose()?; + self.taic.as_ref().map(|x| x.encode(buf)).transpose()?;As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/moov/mod.rs(5 hunks)src/moov/trak/mdia/minf/stbl/stsd/ac3.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/av01.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/eac3.rs(7 hunks)src/moov/trak/mdia/minf/stbl/stsd/flac.rs(2 hunks)src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs(6 hunks)src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs(6 hunks)src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rs(1 hunks)src/moov/trak/mdia/minf/stbl/stsd/mod.rs(1 hunks)src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/opus.rs(2 hunks)src/moov/trak/mdia/minf/stbl/stsd/uncv.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs(2 hunks)src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs(3 hunks)src/test/av1.rs(9 hunks)src/test/bbb.rs(8 hunks)src/test/esds.rs(7 hunks)src/test/flac.rs(6 hunks)src/test/h264.rs(13 hunks)src/test/hevc.rs(8 hunks)src/test/uncompressed.rs(7 hunks)src/test/vp9.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/uncompressed.rs
- src/test/av1.rs
- src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
- src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/bbb.rssrc/moov/trak/mdia/minf/stbl/stsd/uncv.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rssrc/test/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rssrc/test/hevc.rssrc/moov/trak/mdia/minf/stbl/stsd/ac3.rssrc/moov/trak/mdia/minf/stbl/stsd/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/eac3.rssrc/test/h264.rssrc/test/esds.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rssrc/moov/mod.rssrc/test/vp9.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rssrc/moov/trak/mdia/minf/stbl/stsd/opus.rssrc/moov/trak/mdia/minf/stbl/stsd/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/av01.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/bbb.rssrc/moov/trak/mdia/minf/stbl/stsd/uncv.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rssrc/test/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rssrc/test/hevc.rssrc/moov/trak/mdia/minf/stbl/stsd/ac3.rssrc/moov/trak/mdia/minf/stbl/stsd/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/eac3.rssrc/test/h264.rssrc/test/esds.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rssrc/moov/mod.rssrc/test/vp9.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rssrc/moov/trak/mdia/minf/stbl/stsd/opus.rssrc/moov/trak/mdia/minf/stbl/stsd/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/av01.rs
src/test/**
📄 CodeRabbit inference engine (CLAUDE.md)
Put integration tests under src/test/ with sample MP4 files for different codecs
Files:
src/test/bbb.rssrc/test/flac.rssrc/test/hevc.rssrc/test/h264.rssrc/test/esds.rssrc/test/vp9.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/mdia/minf/stbl/stsd/uncv.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rssrc/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rssrc/moov/trak/mdia/minf/stbl/stsd/ac3.rssrc/moov/trak/mdia/minf/stbl/stsd/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/eac3.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rssrc/moov/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rssrc/moov/trak/mdia/minf/stbl/stsd/opus.rssrc/moov/trak/mdia/minf/stbl/stsd/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/av01.rs
🧠 Learnings (1)
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
PR: kixelated/mp4-atom#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/mdia/minf/stbl/stsd/uncv.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rssrc/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rssrc/moov/trak/mdia/minf/stbl/stsd/av01.rs
🧬 Code graph analysis (5)
src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.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/h264/avc1.rs (2)
src/atom.rs (1)
decode_maybe(41-68)src/coding.rs (1)
decode_maybe(31-31)
src/test/hevc.rs (1)
src/types.rs (3)
new(18-20)new(189-191)new(300-302)
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/moov/trak/mdia/minf/stbl/stsd/av01.rs (1)
src/atom.rs (1)
decode_maybe(41-68)
🔇 Additional comments (21)
src/moov/trak/mdia/minf/stbl/stsd/av01.rs (1)
60-77: LGTM on encode pathNot writing unexpected back out is correct; optional members are encoded in canonical order.
src/test/hevc.rs (1)
173-181: Nice use of Default spread in testsThe ..Default::default() on Hev1 keeps tests concise while remaining explicit where needed.
src/moov/trak/mdia/minf/stbl/stsd/eac3.rs (1)
120-126: OK to skip reserved tailAdvancing remaining bytes to ignore reserved bits is fine and matches spec padding expectations.
src/moov/trak/mdia/minf/stbl/stsd/hevc/hvcc.rs (1)
172-172: Good use of Default spreadUsing ..Default::default() keeps tests concise and resilient to added optional fields.
src/moov/trak/mdia/minf/stbl/stsd/mod.rs (2)
42-46: Derive updates look good.Dropping Eq on Stsd is consistent with feature-gated fields impacting total equality.
49-93: Codec derive set is fine.Keeping From and dropping Eq is reasonable; no functional risk spotted.
src/test/esds.rs (1)
150-152: Code structure is sound; manual feature verification required.Static inspection confirms all cfg-gated
unexpectedfields are correctly placed and consistently initialized across all specified line ranges (150-152, 162-164, 170-176, 208-210, 238-240, 254-256, 291-293, 328-334). The attribute guards are syntactically correct and non-intrusive.However, automated verification cannot be performed in this environment. Please manually run the following to verify tests pass and clippy is clean in both feature modes:
# Without fault-tolerant feature cargo clippy --all-targets -- -D warnings cargo test --all-targets # With fault-tolerant feature enabled cargo clippy --all-targets --features fault-tolerant -- -D warnings cargo test --all-targets --features fault-tolerantsrc/test/flac.rs (1)
242-243: LGTM: Test updates for fault-tolerant feature.The test properly initializes the
unexpectedfield to an empty vector across all nested structures when thefault-tolerantfeature is enabled. The pattern is consistent and correctly gated.Also applies to: 267-268, 279-280, 307-308, 321-322, 326-327, 335-336, 370-371
src/moov/mod.rs (2)
21-22: LGTM: Fault-tolerant field added to Moov.The
unexpectedfield is properly added to theMoovstruct with correct feature gating. The field will collect unrecognized atoms during decoding without failing the parse.
142-144: LGTM: Test expectations updated for fault-tolerant mode.All test initializations properly include
unexpected: vec![]under the feature gate across nested structures (Mvex, Edts, Dinf, Stbl, Mdia, Minf, Trak, Udta). The pattern is consistent and comprehensive.Also applies to: 163-165, 194-195, 230-232, 237-242, 244-246, 249-250
src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs (3)
4-4: Eq derive removed correctly.The
Eqderive was removed because the newunexpected: Vec<Any>field cannot implementEq(onlyPartialEq). This is the correct approach for maintaining type consistency.
20-30: LGTM: Fault-tolerant atom collection implemented correctly.The implementation properly:
- Initializes the
unexpectedvector under the feature gate- Logs a warning when encountering unknown atoms
- Collects unknown atoms without failing the decode
42-47: LGTM: Encoding correctly excludes unexpected atoms.The
encode_bodymethod does not serialize theunexpectedfield, which aligns with the PR's design—unexpected atoms are collected during decode but not re-encoded. This prevents corrupting the output with potentially malformed data.src/test/h264.rs (1)
121-122: LGTM: Comprehensive test updates for fault-tolerant parsing.All test expectations are properly updated to include
unexpected: vec![]under the feature gate across all container structures. The changes maintain test correctness for both fault-tolerant and non-fault-tolerant builds.Also applies to: 145-146, 167-168, 195-196, 209-210, 221-222, 228-229, 275-275, 297-298, 325-326, 339-340, 344-345, 351-352, 386-387, 410-411, 701-702, 707-708
src/test/vp9.rs (1)
159-160: LGTM: VP9 test updates are consistent.The VP9 test properly initializes the
unexpectedfield across all nested structures (Moov, Mvex, Trak, Mdia, Minf, Dinf, Stbl, Vp09) when the fault-tolerant feature is enabled. Pattern matches other test files.Also applies to: 198-199, 212-213, 240-241, 254-255, 266-267, 275-276, 300-301
src/moov/trak/mdia/minf/stbl/stsd/ac3.rs (2)
5-5: LGTM: AC-3 codec follows fault-tolerant pattern correctly.The implementation properly:
- Removes
Eqfrom derives (necessary forVec<Any>)- Adds the
unexpectedfield with feature gating- Collects unknown atoms with warnings
- Includes the field in the decoded result
Note:
Ac3SpecificBoxcorrectly retainsEq(line 51) since it doesn't contain theunexpectedfield.Also applies to: 10-11, 21-31, 38-39
43-47: LGTM: Encoding correctly excludes unexpected atoms.Consistent with the overall design, the
encode_bodymethod does not serialize theunexpectedfield, preventing potentially malformed atoms from being written back.src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs (2)
3-3: LGTM: H.264 AVC1 codec implements fault-tolerant parsing correctly.The implementation follows the established pattern:
- Removes
Eq(required forVec<Any>)- Adds
Defaultderive for convenience- Properly gates the
unexpectedfield- Collects unknown atoms with warnings during decode
- Includes the field in the decoded result
Also applies to: 13-14, 29-30, 39-43, 54-55
59-75: LGTM: Encoding correctly excludes unexpected atoms.The
encode_bodymethod properly serializes only the known fields (visual, avcc, btrt, colr, pasp, taic) and skips theunexpectedcollection, maintaining safe output.src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs (2)
3-3: LGTM: HEVC HVC1 codec implements fault-tolerant parsing correctly.The implementation mirrors the pattern used in other codecs:
- Removes
Eqderive (necessary forVec<Any>)- Adds the
unexpectedfield with proper feature gating- Collects unknown atoms with warnings
- Includes the field in the decoded result
Consistent with the overall PR design.
Also applies to: 14-15, 30-31, 40-44, 56-57
61-78: LGTM: Encoding correctly excludes unexpected atoms.The encode implementation properly serializes only known fields (visual, hvcc, btrt, colr, pasp, taic) and does not write back the
unexpectedcollection, maintaining safe and predictable output.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/moov/trak/mdia/minf/stbl/stsd/uncv.rs (1)
5-5: RestoreEqderive when the feature is off to avoid a breaking change.Re-raising prior feedback: keep
Eqfor non–fault-tolerant builds via cfg_attr.-#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, PartialEq)] +#[cfg_attr(not(feature = "fault-tolerant"), derive(Eq))]
🧹 Nitpick comments (13)
src/moov/trak/mdia/minf/stbl/stsd/eac3.rs (1)
27-34: Past review concern addressed; consider enhancing the warning message.The code now correctly returns
Error::UnexpectedBoxwhen thefault-tolerantfeature is disabled, addressing the previous review feedback. However, the warning message could be more informative for debugging by including the parent context.Consider this enhancement for better observability:
_ => { - tracing::warn!("unknown atom: {:?}", atom); + tracing::warn!(parent=%Self::KIND, "unexpected child: {:?}", atom); #[cfg(feature = "fault-tolerant")] unexpected.push(atom); #[cfg(not(feature = "fault-tolerant"))] return Err(Error::UnexpectedBox(atom.kind())); }src/moov/trak/mdia/minf/stbl/stsd/ac3.rs (2)
5-12: Consider adding documentation for theunexpectedfield.The new
unexpectedfield is user-facing but lacks a doc comment explaining its purpose. Consider adding a brief comment such as:/// Unexpected boxes encountered during decoding (fault-tolerant mode only). #[cfg(feature = "fault-tolerant")] pub unexpected: Vec<Any>,This helps users understand what the field contains and when it's populated.
27-34: Warning logged regardless of feature flag.The warning at line 28 is logged even when
fault-tolerantis disabled, meaning users will see a warning log followed by an error. While this might be intentional for debugging visibility, consider whether the warning should be conditional:_ => { + #[cfg(feature = "fault-tolerant")] tracing::warn!("unknown atom: {:?}", atom); #[cfg(feature = "fault-tolerant")] unexpected.push(atom); #[cfg(not(feature = "fault-tolerant"))] return Err(Error::UnexpectedBox(atom.kind())); }This would suppress the warning when fault-tolerant is disabled and the operation will fail anyway.
src/moov/trak/mdia/minf/stbl/stsd/flac.rs (1)
15-42: Previous concern addressed; optional refinement available.The critical issue from the previous review has been resolved—lines 30-31 now correctly return an error when the
fault-tolerantfeature is disabled, ensuring the decoder fails fast on unexpected atoms instead of silently dropping them.The warning at line 26 could be made more specific and gated to avoid logging in the error path, as originally suggested:
Any::Dfla(atom) => dfla = atom.into(), _ => { - tracing::warn!("unknown atom: {:?}", atom); #[cfg(feature = "fault-tolerant")] - unexpected.push(atom); - + { + tracing::warn!("fLaC: unexpected child {}", atom.kind()); + unexpected.push(atom); + } #[cfg(not(feature = "fault-tolerant"))] - return Err(Error::UnexpectedBox(atom.kind())); + { + return Err(Error::UnexpectedBox(atom.kind())); + } }src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs (4)
13-15: Skip serializing emptyunexpectedin serde output.Avoid emitting
unexpected: []in JSON; add serde defaults and skip-if-empty.#[cfg(feature = "fault-tolerant")] + #[cfg_attr( + feature = "serde", + serde(default, skip_serializing_if = "Vec::is_empty") + )] pub unexpected: Vec<Any>,
39-46: Make logging quieter and more structured; preserve strict-by-default flow.Don’t log the whole atom at warn level. Log the FourCC at debug inside the FT block; return Err in strict mode without pre-logging.
- _ => { - tracing::warn!("unknown atom: {:?}", atom); - #[cfg(feature = "fault-tolerant")] - unexpected.push(atom); - - #[cfg(not(feature = "fault-tolerant"))] - return Err(Error::UnexpectedBox(atom.kind())); - } + _ => { + #[cfg(feature = "fault-tolerant")] + { + tracing::debug!(kind = %atom.kind(), "hev1: unexpected child retained"); + unexpected.push(atom); + } + #[cfg(not(feature = "fault-tolerant"))] + { + return Err(Error::UnexpectedBox(atom.kind())); + } + }
62-79: Optional: encode Option fields idiomatically with if-let.Clearer and avoids reliance on an Option::::encode impl.
- if self.btrt.is_some() { - self.btrt.encode(buf)?; - } - if self.colr.is_some() { - self.colr.encode(buf)?; - } - if self.pasp.is_some() { - self.pasp.encode(buf)?; - } - if self.taic.is_some() { - self.taic.encode(buf)? - } + if let Some(btrt) = &self.btrt { btrt.encode(buf)?; } + if let Some(colr) = &self.colr { colr.encode(buf)?; } + if let Some(pasp) = &self.pasp { pasp.encode(buf)?; } + if let Some(taic) = &self.taic { taic.encode(buf)?; }
86-111: Nice round-trip test; consider adding strict/FT semantics tests.Add:
- strict mode: unknown child → Err(UnexpectedBox)
- FT mode: same input retained in
unexpected.I can draft minimal inputs and tests if desired.
src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs (4)
14-15: Skip serializing emptyunexpectedin serde output.Mirror the
hev1suggestion to keep JSON lean.#[cfg(feature = "fault-tolerant")] + #[cfg_attr( + feature = "serde", + serde(default, skip_serializing_if = "Vec::is_empty") + )] pub unexpected: Vec<Any>,
40-46: Reduce log noise; log FourCC at debug only in FT path.Keeps strict mode clean and avoids dumping entire atoms.
- _ => { - tracing::warn!("unknown atom: {:?}", atom); - #[cfg(feature = "fault-tolerant")] - unexpected.push(atom); - #[cfg(not(feature = "fault-tolerant"))] - return Err(Error::UnexpectedBox(atom.kind())); - } + _ => { + #[cfg(feature = "fault-tolerant")] + { + tracing::debug!(kind = %atom.kind(), "hvc1: unexpected child retained"); + unexpected.push(atom); + } + #[cfg(not(feature = "fault-tolerant"))] + { + return Err(Error::UnexpectedBox(atom.kind())); + } + }
63-77: Optional: encode Option fields with if-let for clarity.Same rationale as
hev1.- if self.btrt.is_some() { - self.btrt.encode(buf)?; - } - if self.colr.is_some() { - self.colr.encode(buf)?; - } - if self.pasp.is_some() { - self.pasp.encode(buf)?; - } - if self.taic.is_some() { - self.taic.encode(buf)?; - } + if let Some(btrt) = &self.btrt { btrt.encode(buf)?; } + if let Some(colr) = &self.colr { colr.encode(buf)?; } + if let Some(pasp) = &self.pasp { pasp.encode(buf)?; } + if let Some(taic) = &self.taic { taic.encode(buf)?; }
1-81: Add a minimal round‑trip test forHvc1(mirrorsHev1).Keeps parity and catches regressions.
Example to append:
#[cfg(test)] mod tests { use super::*; #[test] fn hvc1_roundtrip_minimal() { let expected = Hvc1 { visual: Visual { data_reference_index: 1, width: 1920, height: 1080, horizresolution: 0x48.into(), vertresolution: 0x48.into(), frame_count: 1, compressor: "hevc".into(), depth: 24, }, hvcc: Hvcc { configuration_version: 1, ..Default::default() }, ..Default::default() }; let mut buf = Vec::new(); expected.encode(&mut buf).unwrap(); let mut buf = buf.as_ref(); let decoded = Hvc1::decode(&mut buf).unwrap(); assert_eq!(decoded, expected); } }src/moov/trak/mdia/minf/stbl/stsd/uncv.rs (1)
14-15: Apply serde attributes tounexpectedfield for cleaner serialization.The atom registration (Cmpd/UncC in the Any enum) is already complete. The serde attribute suggestion is valid—it will skip serializing empty
unexpectedvectors and allow deserialization when the field is absent, keeping JSON output clean and maintaining backward compatibility.Note: This pattern should be applied consistently across all 26 files containing
pub unexpected: Vec<Any>fields in the codebase for uniform behavior.#[cfg(feature = "fault-tolerant")] - pub unexpected: Vec<Any>, + #[cfg_attr(feature = "serde", serde(skip_serializing_if = "Vec::is_empty", default))] + pub unexpected: Vec<Any>,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.md(1 hunks)src/lib.rs(1 hunks)src/moov/trak/mdia/minf/stbl/stsd/ac3.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/av01.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/eac3.rs(7 hunks)src/moov/trak/mdia/minf/stbl/stsd/flac.rs(2 hunks)src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs(6 hunks)src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs(5 hunks)src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/opus.rs(2 hunks)src/moov/trak/mdia/minf/stbl/stsd/uncv.rs(4 hunks)src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs(2 hunks)src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- src/moov/trak/mdia/minf/stbl/stsd/opus.rs
- src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
- src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
- src/moov/trak/mdia/minf/stbl/stsd/av01.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/moov/trak/mdia/minf/stbl/stsd/ac3.rssrc/moov/trak/mdia/minf/stbl/stsd/eac3.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rssrc/moov/trak/mdia/minf/stbl/stsd/uncv.rssrc/moov/trak/mdia/minf/stbl/stsd/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.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/moov/trak/mdia/minf/stbl/stsd/ac3.rssrc/moov/trak/mdia/minf/stbl/stsd/eac3.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rssrc/moov/trak/mdia/minf/stbl/stsd/uncv.rssrc/moov/trak/mdia/minf/stbl/stsd/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.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/mdia/minf/stbl/stsd/ac3.rssrc/moov/trak/mdia/minf/stbl/stsd/eac3.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rssrc/moov/trak/mdia/minf/stbl/stsd/uncv.rssrc/moov/trak/mdia/minf/stbl/stsd/flac.rssrc/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
🧠 Learnings (1)
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
PR: kixelated/mp4-atom#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/mdia/minf/stbl/stsd/hevc/hvc1.rssrc/moov/trak/mdia/minf/stbl/stsd/uncv.rssrc/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
🧬 Code graph analysis (2)
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/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs (2)
src/atom.rs (1)
decode_maybe(41-68)src/coding.rs (1)
decode_maybe(31-31)
🔇 Additional comments (22)
src/moov/trak/mdia/minf/stbl/stsd/eac3.rs (3)
5-12: LGTM! Struct definition and feature gating are correct.The removal of
Eqis appropriate sinceVec<Any>doesn't implementEq, and theunexpectedfield is properly feature-gated.
46-50: LGTM! Encoding correctly omits unexpected boxes.The encoding path intentionally skips serializing the
unexpectedfield, which aligns with the design decision discussed in the PR comments.
196-198: LGTM! Tests properly updated for fault-tolerant feature.All test cases correctly initialize the
unexpectedfield under the feature gate, ensuring test coverage for both feature-enabled and feature-disabled scenarios.Also applies to: 225-226, 258-259, 309-310, 367-368
src/moov/trak/mdia/minf/stbl/stsd/ac3.rs (2)
46-50: Encoding intentionally skips unexpected boxes.The
encode_bodymethod correctly does not serialize theunexpectedfield, which aligns with the design decision discussed in the PR comments. This ensures that re-encoded MP4 files only contain recognized, expected boxes.
136-138: Test updates look correct.The tests are properly updated to include the
unexpected: vec![]field under the fault-tolerant feature gate. The tests verify the expected behavior when no unexpected boxes are present during decoding and encoding.Also applies to: 160-161
src/moov/trak/mdia/minf/stbl/stsd/flac.rs (2)
3-10: LGTM: Struct definition correctly updated for fault-tolerant feature.The removal of
Eqis necessary sinceVec<Any>doesn't implementEq, and theunexpectedfield is properly gated behind the feature flag.
44-48: LGTM: Encoding correctly omits unexpected boxes.The encoder properly serializes only the expected fields (
audio,dfla), omitting theunexpectedcollection as intended.src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs (7)
9-10: LGTM!The conditional
unexpectedfield correctly implements the fault-tolerant feature. The public visibility allows consumers to inspect unexpected boxes when the feature is enabled.
20-21: LGTM!The initialization correctly prepares the
unexpectedcollection before the decode loop.
26-33: LGTM!The handling of unknown atoms is well-implemented:
- Logs a warning in both modes for observability
- Collects atoms when fault-tolerant is enabled
- Maintains backward-compatible error behavior when disabled
37-42: LGTM!The struct construction correctly includes the
unexpectedfield under the feature gate.
45-50: LGTM!The encoding correctly excludes the
unexpectedfield. This is the intended behavior per the PR design—unexpected boxes are collected during decoding for diagnostics but not re-encoded.
66-67: LGTM!The test correctly initializes the
unexpectedfield as an empty vector under the feature gate, ensuring the equality assertion passes.
4-4: Eq removal is necessary and does not break internal code.Verification confirms that removing
EqfromVp09is necessary becauseVec<Any>(fromstd::any::Any) does not implementEq. No code within the repository depends onVp09implementingEq. The test usesassert_eq!, which requires onlyPartialEq, and passes successfully. While this is a breaking change for external consumers, it is unavoidable and justified by the fault-tolerant feature requirement.src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs (4)
9-10: LGTM: Field addition is properly feature-gated.The
unexpectedfield is correctly gated behind thefault-tolerantfeature and uses the appropriate type.
26-33: LGTM: Excellent fault-tolerant handling with backward compatibility.The implementation correctly:
- Logs unknown atoms for observability
- Collects unexpected boxes when the feature is enabled
- Preserves the original error behavior when the feature is disabled
4-4: LGTM:Eqremoval is correct and consistently applied.Removing
Eqfrom structs withunexpected: Vec<Any>field is correct becauseVec<Any>only implementsPartialEq, notEq. This pattern is consistently applied across all 27+ codec and container atom structs in the codebase that gained this field (Vp08, Vp09, Opus, Flac, Av01, Ac3, Eac3, Uncv, Hvc1, Hev1, Avc1, Mp4a, Moof, Moov, Traf, Trak, Mdia, Minf, Stbl, Edts, Udta, Mvex, Dinf, and Iprp).
45-50: Now I need to inspect the actual code to verify if the design is documented and consistent.Now I need to inspect the actual vp08.rs file to verify the implementation:
Now let me inspect the actual vp08.rs file to verify the implementation and documentation:
Based on my research, I can now provide a comprehensive rewritten review comment. The ISO base media file format is designed as an extensible file format, and it is designed as a flexible, extensible format that facilitates interchange, management, editing and presentation of the media. The practice of discarding unexpected/unknown boxes during remuxing is standard industry behavior for MP4 muxers.
Clarify whether skipping
unexpectedboxes during encoding is intentional and documented.The
unexpectedfield is not serialized during encoding. This behavior—dropping unknown boxes during remuxing—aligns with how the ISO base media file format is designed as an extensible file format with a list of all registered extensions published on the official registration authority website, www.mp4ra.org. However, this design choice should be explicitly documented in the type's doc comments to clarify that round-trip fidelity (decode → encode) will not preserve unrecognized boxes, which is intentional per the extensible format philosophy.Confirm:
- The rationale for not encoding
unexpectedis intentional and acceptable for this codebase's use case- This design pattern is documented in the module or type documentation
src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs (1)
62-79: No issues found — unexpected atoms are correctly NOT encoded.The
encode_bodymethod in hev1.rs only encodes the defined fields (visual, hvcc, btrt, colr, pasp, taic) and omits theunexpectedfield. The codebase includes a test that explicitly verifies unexpected boxes are not written back during re-encoding. The ripgrep search found zero instances ofunexpected.encode()across the entire codebase, confirming consistent behavior repo-wide.src/moov/trak/mdia/minf/stbl/stsd/uncv.rs (3)
29-31: LGTM: feature-gated allocation.Correctly allocates
unexpectedonly when needed.
39-46: Correct gating for unknown atoms.Returning
Error::UnexpectedBoxwhen the feature is disabled and collecting when enabled matches spec and prior behavior. Nice fix.
57-58: LGTM: wiresunexpectedinto the returned struct.Consistent with the decode path.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/hevc.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/hevc.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/hevc.rs
src/test/**
📄 CodeRabbit inference engine (CLAUDE.md)
Put integration tests under src/test/ with sample MP4 files for different codecs
Files:
src/test/hevc.rs
🧬 Code graph analysis (1)
src/test/hevc.rs (2)
src/moov/trak/mdia/minf/stbl/stsd/mod.rs (1)
decode(96-115)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)
🔇 Additional comments (3)
src/test/hevc.rs (3)
214-221: LGTM: Non-fault-tolerant error handling is correct.The test properly verifies that without the fault-tolerant feature, an
UnexpectedBoxerror is returned for the "fiel" fourcc as expected.
223-641: LGTM: Comprehensive fault-tolerant mode verification.The assertion correctly verifies that:
- Decoding succeeds when the fault-tolerant feature is enabled
- The unexpected "fiel" box is captured in the
Hev1.unexpectedfield with correct data- All expected fields are properly decoded
- Feature gates are correctly applied to all
unexpectedfield references
643-649: Good approach: Clearing unexpected before round-trip test.The test correctly clears the
unexpectedfield before testing encode/decode round-tripping, since unexpected boxes are not encoded back. The explanatory comment is helpful.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/test/hevc.rs (1)
651-656: Thanks for clarifying the encode-without-assert intent.The added comment resolves the earlier ambiguity.
🧹 Nitpick comments (2)
src/test/hevc.rs (2)
223-641: Reduce repeatedunexpected: vec![]boilerplate for readability.The struct literal repeats empty
unexpectedfields under many nodes. Consider a small helper/macro or use struct update with..Default::default()where possible to cut repetition and cfg noise, keeping tests concise.
306-316: Add a focused test coveringminfwith anhdlrchild (issue #63).This HEVC sample doesn’t exercise
minf->hdlr. A small additional fixture (or synthetic bytes) that includeshdlrinminfwould concretely verify the new acceptance logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/hevc.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/hevc.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/hevc.rs
src/test/**
📄 CodeRabbit inference engine (CLAUDE.md)
Put integration tests under src/test/ with sample MP4 files for different codecs
Files:
src/test/hevc.rs
🧬 Code graph analysis (1)
src/test/hevc.rs (2)
src/moov/trak/mdia/minf/stbl/stsd/mod.rs (1)
decode(96-115)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)
🔇 Additional comments (1)
src/test/hevc.rs (1)
214-221: Negative-path check looks good.Asserting
Moov::decodeerrors withUnexpectedBox('fiel')when fault-tolerant is disabled is the right guardrail.
| // Make sure the hev1 atom encodes/decodes to the exact same content. | ||
| // Note: We need to clear unexpected atoms because they are not encoded back | ||
| let mut hev1_for_test = moov.trak[0].mdia.minf.stbl.stsd.codecs[0].clone(); | ||
| if let Codec::Hev1(ref mut hev1) = hev1_for_test { | ||
| hev1.unexpected.clear(); | ||
| } | ||
| hev1_for_test.assert_encode_decode(); |
There was a problem hiding this comment.
Unconditional access to hev1.unexpected breaks non-fault-tolerant builds; also avoid unused_mut.
When the fault-tolerant feature is off, Hev1 has no unexpected field, so Lines 646–648 won’t compile. Additionally, let mut on Line 645 triggers unused_mut in that config.
Apply this diff to cfg‑gate both the mutable binding and the field mutation:
- let mut hev1_for_test = moov.trak[0].mdia.minf.stbl.stsd.codecs[0].clone();
- if let Codec::Hev1(ref mut hev1) = hev1_for_test {
- hev1.unexpected.clear();
- }
+ #[cfg(feature = "fault-tolerant")]
+ let mut hev1_for_test = moov.trak[0].mdia.minf.stbl.stsd.codecs[0].clone();
+ #[cfg(not(feature = "fault-tolerant"))]
+ let hev1_for_test = moov.trak[0].mdia.minf.stbl.stsd.codecs[0].clone();
+ #[cfg(feature = "fault-tolerant")]
+ {
+ if let Codec::Hev1(ref mut hev1) = hev1_for_test {
+ hev1.unexpected.clear();
+ }
+ }Optional: Assert the variant to fail fast if the test vector changes:
assert!(matches!(hev1_for_test, Codec::Hev1(_)));🤖 Prompt for AI Agents
In src/test/hevc.rs around lines 643 to 649, the test unconditionally binds a
mutable copy and accesses hev1.unexpected which only exists with the
fault-tolerant feature and causes an unused_mut warning when that feature is
off; gate the mutable binding and the field-clear behind the same cfg(feature =
"fault-tolerant") so the code only makes a mut copy and calls
hev1.unexpected.clear() when the feature is enabled, and remove the unnecessary
mut otherwise (i.e., keep an immutable clone for the non-fault-tolerant path);
optionally add an assert!(matches!(hev1_for_test, Codec::Hev1(_))); before the
cfg-gated clear to fail fast if the test vector changes.
2 participants
closes #63
This change introduces the ability to collect and retain any unexpected boxes encountered during the decoding process.
Previously, the decoder would give an
Error::unexpectedBoxwhen encounters an unexpected box thus failing the decode process. Now, these boxes are stored in aunexpectedVec 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.
Dones
unexpectedfield to any parent boxes, aggregating unexpected boxes during decodingSummary by CodeRabbit
New Features
Documentation
Public API
Tests