add support for ilst "tool" box #86
Conversation
This has 4CC of ©too, and is from quicktime
WalkthroughA new Tool atom type (4CC: "©too") was added. A public Tool struct with Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/test/av1.rs (1)
262-273: Test update looks correct, but consider testing both decode variants.The explicit Ilst construction properly validates the new Tool atom support. However, as with the HEVC test, this appears to test only one of the "two different encodings" mentioned in the PR description.
Optional: Reduce verbosity with a helper method
The explicit Ilst construction is repeated across multiple test files. Consider adding a helper method to reduce boilerplate:
impl Ilst { pub fn with_tool(text: impl Into<String>) -> Self { Self { name: None, year: None, covr: None, desc: None, ctoo: Some(Tool { country_indicator: 0, language_indicator: 0, text: text.into(), }), } } }Then use:
Ilst::with_tool("Lavf61.7.100").into()src/meta/ilst/tool.rs (3)
15-21: Consider simplifying the constructor signature.The constructor always returns
Okwithout performing any validation. You could simplify the API by returningSelfdirectly instead ofResult<Self>, or add validation if there are constraints on the field values.🔎 Simplified constructor without Result wrapper
- pub fn new(country_indicator: u16, language_indicator: u16, text: String) -> Result<Self> { - Ok(Self { + pub fn new(country_indicator: u16, language_indicator: u16, text: String) -> Self { + Self { country_indicator, language_indicator, text, - }) + } }
69-79: Document the size calculation for clarity.The encode logic is correct, but the calculation on line 71 uses magic numbers that could be clearer.
🔎 Proposed documentation for clarity
fn encode_body<B: BufMut>(&self, buf: &mut B) -> Result<()> { let text_bytes = self.text.as_bytes(); + // nested_len = size field (4) + "data" fourcc (4) + type indicator (4) + // + country (2) + language (2) + text bytes let nested_len = (4 + 4 + 4 + 2 + 2 + text_bytes.len()) as u32; nested_len.encode(buf)?;Or use named constants:
+ const DATA_ATOM_HEADER_SIZE: usize = 4 + 4; // size + fourcc + const DATA_ATOM_FIELDS_SIZE: usize = 4 + 2 + 2; // type + country + language + fn encode_body<B: BufMut>(&self, buf: &mut B) -> Result<()> { let text_bytes = self.text.as_bytes(); - let nested_len = (4 + 4 + 4 + 2 + 2 + text_bytes.len()) as u32; + let nested_len = (DATA_ATOM_HEADER_SIZE + DATA_ATOM_FIELDS_SIZE + text_bytes.len()) as u32;
82-121: Consider adding error case tests.The tests cover both happy-path decode variants and round-trip encoding, which is good. Consider adding tests for error conditions to improve robustness:
- Invalid FourCC in long-style format (should return
Error::UnexpectedBox)- Non-UTF8 type indicator (should return
Error::Unsupported)- Length value < 16 (should return an error after the fix suggested above)
- Invalid UTF-8 byte sequences
🔎 Example error case test
#[test] fn test_ilst_tool_invalid_fourcc() { let buf = vec![ 0, 0, 0, 28, 0xa9, b't', b'o', b'o', 0, 0, 0, 20, // length b'b', b'a', b'd', b'!', // wrong fourcc 0, 0, 0, 1, // type 0, 0, 0, 0, // locale b'test' ]; let result = Tool::decode(&mut buf.as_slice()); assert!(matches!(result, Err(Error::UnexpectedBox(_)))); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/any.rssrc/meta/ilst/mod.rssrc/meta/ilst/tool.rssrc/test/av1.rssrc/test/bbb.rssrc/test/hevc.rssrc/test/uncompressed.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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/any.rssrc/meta/ilst/mod.rssrc/test/uncompressed.rssrc/test/av1.rssrc/meta/ilst/tool.rssrc/test/hevc.rssrc/test/bbb.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/any.rssrc/meta/ilst/mod.rssrc/test/uncompressed.rssrc/test/av1.rssrc/meta/ilst/tool.rssrc/test/hevc.rssrc/test/bbb.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
src/meta/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place metadata (HEIF/AVIF) atom implementations under src/meta/
Files:
src/meta/ilst/mod.rssrc/meta/ilst/tool.rs
src/test/**
📄 CodeRabbit inference engine (CLAUDE.md)
Put integration tests under src/test/ with sample MP4 files for different codecs
Files:
src/test/uncompressed.rssrc/test/av1.rssrc/test/hevc.rssrc/test/bbb.rs
🧠 Learnings (5)
📓 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/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: 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: 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/any.rs : Register new atom types in the Any enum (src/any.rs) to enable flexible decoding
Applied to files:
src/any.rssrc/meta/ilst/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/any.rssrc/meta/ilst/mod.rssrc/meta/ilst/tool.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/buf.rs : Define and maintain contiguous byte-slice buffer behavior via custom Buf and BufMut traits in buf.rs
Applied to files:
src/meta/ilst/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/moov/** : Place movie metadata atom implementations under src/moov/ (and its hierarchy: trak/mdia/minf/stbl/stsd)
Applied to files:
src/test/av1.rs
🧬 Code graph analysis (2)
src/meta/ilst/mod.rs (1)
src/atom.rs (1)
decode_maybe(46-73)
src/meta/ilst/tool.rs (1)
src/meta/ilst/mod.rs (2)
decode_body(28-53)encode_body(55-62)
🔇 Additional comments (10)
src/any.rs (1)
258-258: LGTM! Tool atom registration follows project conventions.The Tool atom is correctly registered in the Any enum among the basic (non-boxed) Ilst metadata atoms. The placement and comment are clear.
Based on learnings, this follows the established pattern for registering new atom types in the Any enum.
src/test/bbb.rs (1)
374-374: LGTM! Test properly validates Tool atom support.The explicit Ilst construction correctly tests the new ctoo field with a Tool instance.
src/test/uncompressed.rs (1)
291-302: Good test coverage with GPAC encoder variant.This test validates the Tool atom with GPAC-generated content, complementing the Lavf tests in other files. This demonstrates that the implementation works with Tool atoms from different encoders.
src/test/hevc.rs (1)
589-600: The review comment references claims from a PR description that do not exist in the actual commit message. The commit only states "add support for ilst 'tool' box" without mentioning multiple FFmpeg encoding variants.The Tool implementation does handle two decode variants (FFmpeg short-style and long-style), and both are already thoroughly tested in the unit tests:
test_ilst_tool_short_style()andtest_ilst_tool_long_style(). The integration test in hevc.rs appropriately validates one variant in the full MP4 roundtrip context, which is sufficient for integration testing. The unit tests provide comprehensive coverage of both decode paths.Likely an incorrect or invalid review comment.
src/meta/ilst/mod.rs (4)
4-4: LGTM! Module declaration follows established patterns.The new module and re-export follow the same pattern as other ilst metadata types.
Also applies to: 10-10
22-22: LGTM! Field addition is well-documented.The field name
ctooappropriately avoids special characters while the comment clearly documents the 4CC mapping.
33-33: LGTM! Decode integration follows the established pattern.The Tool atom is correctly integrated into the decode flow, matching the pattern used for other optional metadata fields.
Also applies to: 41-41, 51-51
60-60: LGTM! Encode integration is correct.The encode call follows the established pattern. Note that the existing unit tests (
test_ilstandtest_ilst_empty) don't exercise the newctoofield, but based on the PR context, test files elsewhere in the codebase have been updated to include Tool atoms.src/meta/ilst/tool.rs (2)
1-12: LGTM! Clean struct definition and constants.The Tool struct fields and constants are well-defined for handling the two FFmpeg encoding variants.
27-40: LGTM! Short-style decode path handles errors correctly.The short-style decoding (FFmpeg short format) properly uses
String::decode(buf)?which handles UTF-8 validation errors.
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @src/meta/ilst/tool.rs:
- Line 138: The assertion constructs a byte array manually; replace the explicit
byte array with a byte string literal to satisfy Clippy (change the argument to
FourCC::new from &[b'd', b'a', b't', b'x'] to a byte string literal like
b"datx"). Update the call site where four_cc is compared (the assert_eq!
involving four_cc and FourCC::new) so FourCC::new receives the byte string
literal.
- Line 51: Fix the typo in the error message string "Only UTF-8 text is support
in ilst tool box" by changing "support" to "supported" so it reads "Only UTF-8
text is supported in ilst tool box" (update the string literal in
src/meta/ilst/tool.rs where that exact message appears).
♻️ Duplicate comments (1)
src/meta/ilst/tool.rs (1)
58-58: Use appropriate error variant for UTF-8 decoding failure.
Error::InvalidSizeis semantically incorrect for UTF-8 decoding errors—it typically indicates size or length problems, not encoding issues. Consider usingError::Unsupportedwith a descriptive message (similar to lines 50-52) for consistency.🔎 Proposed fix
- let text = String::from_utf8(body.to_vec()).map_err(|_| Error::InvalidSize)?; + let text = String::from_utf8(body.to_vec()) + .map_err(|_| Error::Unsupported("Invalid UTF-8 in tool text"))?;
🧹 Nitpick comments (1)
src/meta/ilst/tool.rs (1)
14-22: Consider simplifying the constructor or adding validation.The constructor returns
Result<Self>but has no validation logic or error paths—it always returnsOk. If validation is planned for the future, this is fine. Otherwise, consider making this a plain function returningSelfdirectly, or use direct struct initialization.Alternative implementation without Result
- pub fn new(country_indicator: u16, language_indicator: u16, text: String) -> Result<Self> { - Ok(Self { + pub fn new(country_indicator: u16, language_indicator: u16, text: String) -> Self { + Self { country_indicator, language_indicator, text, - }) + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/meta/ilst/tool.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/meta/ilst/tool.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/meta/ilst/tool.rs
src/meta/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place metadata (HEIF/AVIF) atom implementations under src/meta/
Files:
src/meta/ilst/tool.rs
🧠 Learnings (3)
📓 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/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: Applies to src/**/*.rs : Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()
📚 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/meta/ilst/tool.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/meta/ilst/tool.rs
🧬 Code graph analysis (1)
src/meta/ilst/tool.rs (1)
src/meta/ilst/mod.rs (2)
decode_body(28-53)encode_body(55-62)
🪛 GitHub Actions: PR
src/meta/ilst/tool.rs
[error] 138-138: Clippy: can be more succinctly written as a byte str.
🪛 GitHub Check: build
src/meta/ilst/tool.rs
[failure] 138-138:
can be more succinctly written as a byte str
src/meta/ilst/tool.rs
Outdated
| assert!(parse_result.is_err()); | ||
| match parse_result.err().unwrap() { | ||
| Error::UnexpectedBox(four_cc) => { | ||
| assert_eq!(four_cc, FourCC::new(&[b'd', b'a', b't', b'x'])); |
There was a problem hiding this comment.
Fix clippy warning: use byte string literal.
The byte array can be written more succinctly as a byte string literal.
Based on pipeline failure logs.
🔎 Proposed fix
- assert_eq!(four_cc, FourCC::new(&[b'd', b'a', b't', b'x']));
+ assert_eq!(four_cc, FourCC::new(b"datx"));📝 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.
| assert_eq!(four_cc, FourCC::new(&[b'd', b'a', b't', b'x'])); | |
| assert_eq!(four_cc, FourCC::new(b"datx")); |
🧰 Tools
🪛 GitHub Actions: PR
[error] 138-138: Clippy: can be more succinctly written as a byte str.
🪛 GitHub Check: build
[failure] 138-138:
can be more succinctly written as a byte str
🤖 Prompt for AI Agents
In @src/meta/ilst/tool.rs around line 138, The assertion constructs a byte array
manually; replace the explicit byte array with a byte string literal to satisfy
Clippy (change the argument to FourCC::new from &[b'd', b'a', b't', b'x'] to a
byte string literal like b"datx"). Update the call site where four_cc is
compared (the assert_eq! involving four_cc and FourCC::new) so FourCC::new
receives the byte string literal.
bradh
force-pushed
the
unit_test_level_up_2026-01-04
branch
from
8d7b4ce to
22c5a23
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/meta/ilst/tool.rs (2)
14-21: Consider simplifying the constructor.The
new()method always returnsOk()without performing any validation. Unless validation is planned for the future, consider returningSelfdirectly instead ofResult<Self>to simplify the API.🔎 Proposed simplification
- pub fn new(country_indicator: u16, language_indicator: u16, text: String) -> Result<Self> { - Ok(Self { + pub fn new(country_indicator: u16, language_indicator: u16, text: String) -> Self { + Self { country_indicator, language_indicator, text, - }) + } }
6-12: Consider adding documentation comments.The public
Toolstruct and itsnew()constructor method lack doc comments. Adding documentation would help users understand the purpose of each field (especially the country and language indicators) and clarify which encoding style is produced byencode_body().🔎 Example documentation
/// ILST tool box (4CC: ©too) representing the encoding tool name. /// /// This atom supports two encoding styles: /// - Short-style: type indicator followed by text /// - Long-style: nested "data" atom with metadata /// /// The encoder always produces long-style format. #[derive(Debug, Clone, PartialEq, Eq)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub struct Tool { /// Country code indicator (usually 0). pub country_indicator: u16, /// Language code indicator (usually 0). pub language_indicator: u16, /// Tool name text (e.g., "Lavf61.7.100"). pub text: String, } impl Tool { /// Creates a new Tool instance. pub fn new(country_indicator: u16, language_indicator: u16, text: String) -> Result<Self> { // ... } }Also applies to: 14-21
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/meta/ilst/tool.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/meta/ilst/tool.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/meta/ilst/tool.rs
src/meta/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place metadata (HEIF/AVIF) atom implementations under src/meta/
Files:
src/meta/ilst/tool.rs
🧠 Learnings (3)
📓 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/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: Applies to src/**/*.rs : Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()
📚 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/meta/ilst/tool.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/meta/ilst/tool.rs
🧬 Code graph analysis (1)
src/meta/ilst/tool.rs (1)
src/meta/ilst/mod.rs (2)
decode_body(28-53)encode_body(55-62)
🔇 Additional comments (3)
src/meta/ilst/tool.rs (3)
27-67: Excellent handling of multiple encoding variants.The decode logic cleverly distinguishes between short-style and long-style encodings by checking if the first u32 equals 1 (type indicator) versus a larger value (nested atom length). This works because the minimum valid nested "data" atom size is 16 bytes, making 1 an impossible length value. Both paths correctly handle their respective formats and properly validate the nested atom structure in the long-style path.
All previously identified issues (unwrap, underflow, bounds validation) have been addressed.
69-83: LGTM!The encode logic correctly produces the long-style format with a nested "data" atom. The length calculation properly includes all atom components (size, 4CC, type indicator, country/language indicators, and text), and the encoding order matches the MP4 atom structure.
86-163: Comprehensive test coverage.The tests effectively cover both encoding styles, error conditions, and round-trip encoding. The test cases validate:
- Short-style decoding (FFmpeg variant)
- Long-style decoding and encoding round-trip (GPAC/FFmpeg variant)
- Invalid nested atom 4CC rejection
- Invalid type indicator rejection
All previously flagged test issues have been resolved.
1 participant
This has 4CC of ©too, and is from quicktime.
The spec is vague on exactly how this should work, and FFmpeg will output this (at least) two different ways. GPAC outputs one of those ways, so we handle both cases on decode (consistent with the inconsistency in the test data), and output the somewhat-common case on encode.
This gets us closer (but not all the way) to enabling strict mode.