-
Notifications
You must be signed in to change notification settings - Fork 14
implement mfra container and nested tfra and mfro boxes #98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
See ISO/IEC 14496-12:2022 Section 8.8
WalkthroughThis change introduces support for Movie Fragment Random Access (MFRA) box structures in the ISO BMFF format. New modules are added for 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mfra/tfra.rs (1)
70-95: Minor: Consider renaming parameter for clarity.The parameter
num_bits_minus_oneis technicallynum_bytes_minus_one(values 0-3 map to 1-4 bytes). However, this aligns with how the ISO spec uses "length_size_of_*" fields, so keeping it consistent with the specification terminology is reasonable.The
.expect()on line 89 is safe becausedetermine_required_lengthsensures the value fits within 24 bits before selectingnum_bits_minus_one = 2.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/any.rssrc/lib.rssrc/mfra/mfro.rssrc/mfra/mod.rssrc/mfra/tfra.rssrc/types.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/types.rssrc/lib.rssrc/mfra/mfro.rssrc/any.rssrc/mfra/mod.rssrc/mfra/tfra.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/types.rssrc/lib.rssrc/mfra/mfro.rssrc/any.rssrc/mfra/mod.rssrc/mfra/tfra.rs
src/any.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Register new atom types in the Any enum (src/any.rs) to enable flexible decoding
Files:
src/any.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/moov/** : Place movie metadata atom implementations under src/moov/ (and its hierarchy: trak/mdia/minf/stbl/stsd)
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/moof/** : Place movie fragment atom implementations under src/moof/
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/any.rs : Register new atom types in the Any enum (src/any.rs) to enable flexible decoding
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Place top-level atoms (e.g., ftyp, mdat, free) at the top-level of src/
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/moov/** : Place movie metadata atom implementations under src/moov/ (and its hierarchy: trak/mdia/minf/stbl/stsd)
Applied to files:
src/lib.rssrc/mfra/mod.rs
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Place top-level atoms (e.g., ftyp, mdat, free) at the top-level of src/
Applied to files:
src/lib.rs
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/moof/** : Place movie fragment atom implementations under src/moof/
Applied to files:
src/lib.rssrc/mfra/mfro.rssrc/mfra/mod.rs
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()
Applied to files:
src/lib.rssrc/mfra/mfro.rssrc/any.rssrc/mfra/mod.rssrc/mfra/tfra.rs
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/any.rs : Register new atom types in the Any enum (src/any.rs) to enable flexible decoding
Applied to files:
src/mfra/mfro.rssrc/any.rs
🧬 Code graph analysis (3)
src/mfra/mfro.rs (2)
src/types.rs (9)
new(18-20)new(193-195)new(304-306)decode(74-76)decode(101-103)decode(144-146)decode(207-212)decode(286-294)decode(325-329)src/mfra/tfra.rs (2)
decode_body_ext(102-127)encode_body_ext(129-159)
src/mfra/mod.rs (1)
src/types.rs (3)
new(18-20)new(193-195)new(304-306)
src/mfra/tfra.rs (1)
src/types.rs (9)
decode(74-76)decode(101-103)decode(144-146)decode(207-212)decode(286-294)decode(325-329)new(18-20)new(193-195)new(304-306)
🔇 Additional comments (13)
src/types.rs (1)
90-92: LGTM!The
MAXconstant correctly represents the maximum 24-bit unsigned value (2²⁴ - 1). This is appropriately used intfra.rsfor determining variable-length field encoding sizes.src/lib.rs (1)
153-153: LGTM!The
mframodule declaration and public re-export follow the established codebase patterns. Placement at the top-levelsrc/directory is appropriate for the Movie Fragment Random Access box per the project's conventions for top-level atoms.Also applies to: 174-174
src/any.rs (1)
336-338: LGTM!The new
Mfra,Tfra, andMfroatom types are properly registered in theAnyenum's basic variants list, enabling flexible decoding and encoding through the commonAnyinterface. Based on learnings, registering new atom types insrc/any.rsis required for flexible decoding.src/mfra/mfro.rs (2)
1-26: LGTM!The
Mfro(MovieFragmentRandomAccessOffsetBox) implementation correctly follows theAtomExtpattern withExt = ()since this box uses the default version 0 with no flags. Theparent_sizefield aligns with ISO/IEC 14496-12:2022 section 8.8.11 specification.
28-41: LGTM!Good round-trip test coverage for the
Mfroatom. The test validates that encoding and decoding produce identical results.src/mfra/mod.rs (4)
14-17: LGTM!The
Mfrastruct correctly models the Movie Fragment Random Access box with a vector ofTfraentries and a singleMfro. The comment on line 19 appropriately explains why thenested!macro cannot be used here—mfromust be written last per the specification.
23-47: LGTM!The
decode_bodyimplementation properly handles:
- Duplicate
mfrodetection withError::DuplicateBox- Missing
mfrovalidation withError::MissingBox- Collection of multiple
tfraentries- Skip/Free boxes with debug logging
- Unknown boxes via
decode_unknown
49-52: LGTM!The
encode_bodycorrectly writestfraentries beforemfro, ensuringmfrois the last box in the container as required by the specification.
55-176: LGTM!Excellent test coverage using data from the MPEG File Format Conformance suite. The bidirectional round-trip tests (
test_mfra_decodeandtest_mfra_encode) validate both encoding and decoding paths against the conformance data.src/mfra/tfra.rs (4)
9-24: LGTM!The
FragmentInfoandTfrastructs correctly model the Track Fragment Random Access box per ISO/IEC 14496-12. Usingu64fortimeandmoof_offsetaccommodates both V0 (32-bit) and V1 (64-bit) versions internally.
25-68: LGTM!The
determine_required_lengthsmethod correctly calculates the minimum encoding sizes for variable-length fields and selects the appropriate version (V0 or V1) based on whethertimeormoof_offsetvalues exceed 32-bit limits. Thedetermine_required_lengthhelper properly usesu24::MAXfor the 24-bit threshold.
97-160: LGTM!The
AtomExtimplementation forTfracorrectly handles:
- Bit-field extraction/packing for the variable-length size indicators
- Version-dependent encoding of
timeandmoof_offset(64-bit for V1, 32-bit for V0)- Defensive capacity allocation (line 111) that doesn't blindly trust the entry count
The encode/decode symmetry is well-maintained.
162-213: LGTM!Good test organization—V1 format is explicitly tested here while V0 is exercised through the parent
mframodule tests using the conformance suite data. The test data includes interesting edge cases likesample_delta: 65535(requiring 2-byte encoding).
See ISO/IEC 14496-12:2022 Section 8.8