Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
bradh merged 3 commits into from
Dec 31, 2025
Merged

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

bradh merged 3 commits into from
Dec 31, 2025

Conversation

@kixelated
Copy link
Owner

@kixelated kixelated commented Dec 29, 2025 •
edited
Loading

We should enable this for unit tests, but currently the hevc test would fail. I don't want to create a feature flag until there's test coverage.

cargo test --features strict

---- test::hevc::hevc stdout ----

thread 'test::hevc::hevc' panicked at src/test/hevc.rs:214:34: failed to decode moov: UnexpectedBox(fiel)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Alternatively, we could use tracing_test to ensure that none of the unit tests have log output. That might be cleaner if we don't want a feature flag in the future.

Fixes #63
Replaces #81 #71

@kixelated
Log unknown atoms instead of returning an error.
20d9251 
We should enable this for unit tests, but currently the hevc test would
fail. I don't want to create a feature flag until there's test coverage.

cargo test --features strict

---- test::hevc::hevc stdout ----

thread 'test::hevc::hevc' panicked at src/test/hevc.rs:214:34:
failed to decode moov: UnexpectedBox(fiel)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Alternatively, we could use `tracing_test` to ensure that none of the
unit tests have log output. That might be cleaner if we don't want a
feature flag in the future.
@kixelated kixelated requested a review from bradh December 29, 2025 23:25
@coderabbitai
Copy link

coderabbitai bot commented Dec 29, 2025 •
edited
Loading

Walkthrough

A new associated method fn decode_unknown(atom: &crate::Any) -> Result<()> was added to the Atom trait (defaulting to crate::decode_unknown). A new internal helper crate::decode_unknown(&Any, parent: FourCC) was added in src/error.rs. Many decoders (ilst, multiple stsd sample-entry modules and codec-specific parsers) now delegate unknown atoms to Self::decode_unknown(&unknown)? instead of emitting non-fatal warning logs; several skip/free logs were changed to include a structured size field. stsd::Codec now routes unmatched atoms through the centralized decode_unknown path before producing Unknown variants.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title describes logging unknown atoms instead of returning errors, which contradicts the actual changes that convert silent warnings into error returns via decode_unknown. Correct the title to accurately reflect that unknown atoms now propagate errors instead of being silently logged; consider: 'Centralize unknown atom handling and propagate errors via decode_unknown'.
Docstring Coverage ⚠️ Warning Docstring coverage is 11.76% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Linked Issues check ❓ Inconclusive The PR changes do not fully address issue #63. While decode_unknown is added to prevent UnexpectedBox in some cases, the changes convert warnings into errors rather than enabling fault-tolerant decoding as requested. Clarify whether this PR is a step toward fault-tolerant decoding [#63] or a refactoring; if a step, document how decode_unknown (which logs then returns Ok) will enable future strict/lenient modes.
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed The description is related to the changeset as it discusses the implications of stricter error handling for unknown atoms and test coverage.
Out of Scope Changes check ✅ Passed All changes systematically replace warning logs with error propagation via decode_unknown across multiple codec files, which aligns with the stated objective of centralizing unknown atom handling.
✨ Finishing touches
  • 📝 Generate docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai bot reviewed Dec 29, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/error.rs (1)

72-78: Consider tracking the TODOs as GitHub issues.

The implementation currently logs and returns Ok(()), making the ? operator at call sites effectively a no-op. While this prepares the infrastructure for future strict mode, the TODOs indicate incomplete work that should be tracked.

Would you like me to open tracking issues for:

  1. Implementing the strict feature flag behavior
  2. Fixing unit tests to handle unknown atoms appropriately
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2f60395 and 20d9251.

📒 Files selected for processing (17)
  • src/atom.rs
  • src/error.rs
  • src/meta/ilst/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/av01.rs
  • src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
  • src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs
🧰 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/meta/ilst/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs
  • src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/atom.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/av01.rs
  • src/error.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/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs
  • src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/atom.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/av01.rs
  • src/error.rs
src/meta/**

📄 CodeRabbit inference engine (CLAUDE.md)

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

Files:

  • src/meta/ilst/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/stbl/stsd/vp9/vp08.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs
  • src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/av01.rs
🧠 Learnings (7)
📓 Common learnings 
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()

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

Applied to files:

  • src/meta/ilst/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs
  • src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/atom.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/av01.rs
  • src/error.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/meta/ilst/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs
  • src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/atom.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/av01.rs
  • src/error.rs
📚 Learning: 2025-08-31T04:06:57.364Z 
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Use Header::read_from() to handle large atoms without loading entire contents into memory

Applied to files:

  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs
  • src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/atom.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/av01.rs
📚 Learning: 2025-08-31T04:06:57.364Z 
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/moov/** : Place movie metadata atom implementations under src/moov/ (and its hierarchy: trak/mdia/minf/stbl/stsd)

Applied to files:

  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mp4a/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/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.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 : All operations should return Result<T> using custom error types defined in error.rs

Applied to files:

  • src/error.rs
🧬 Code graph analysis (15)
src/meta/ilst/mod.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/atom.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/moov/trak/mdia/minf/stbl/stsd/opus.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/moov/trak/mdia/minf/stbl/stsd/uncv.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/moov/trak/mdia/minf/stbl/stsd/flac.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/moov/trak/mdia/minf/stbl/stsd/eac3.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/moov/trak/mdia/minf/stbl/stsd/ac3.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
src/moov/trak/mdia/minf/stbl/stsd/av01.rs (1)
src/error.rs (1)
  • unexpected_atom (81-83)
🔇 Additional comments (9)
src/error.rs (1)

80-83: LGTM!

Clean delegation pattern that centralizes unknown-atom handling.

src/atom.rs (2)

196-196: LGTM - Improved logging.

Structured logging with named field %kind is more query-friendly than formatted messages.

198-200: LGTM - Consistent unknown-atom handling.

The changes properly route unhandled atoms through the centralized error path while maintaining structured logging for Skip and Free boxes.

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

32-32: LGTM - Centralized unknown-atom handling.

The change routes unknown atoms through the centralized error path, aligning with the broader refactoring pattern across all decoders.

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

34-34: LGTM - Consistent with the refactoring pattern.

Unknown atoms are now routed through the centralized error handler.

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

35-35: LGTM - Consistent unknown-atom handling.

The centralized error path is properly applied.

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

20-20: LGTM - Centralized error handling applied.

The change aligns with the broader refactoring pattern.

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

110-110: LGTM - Centralized error handling applied.

The change routes unknown atoms through the centralized error handler, consistent with the pattern used in other decoders.

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

32-32: LGTM - Consistent unknown-atom handling.

The centralized error path is properly applied.

@bradh
Copy link
Collaborator

bradh commented Dec 30, 2025

UnexpectedBox(fiel)

That is some Quicktime legacy usage. The only ISO standard I could find that does it is Motion JPEG 2000.

Won't be hard to add though. Any reason not to have it?

bradh
bradh reviewed Dec 30, 2025
View reviewed changes
Copy link
Collaborator

@bradh bradh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LTGM, apart from the codec case which would be good to treat in the same way.

The only other thing I can think of is that it might be useful to include what the parent box is. That will make it easier to work out what is happening, and ensure that the missing child gets added to the right struct.

@kixelated
Copy link
Owner Author

kixelated commented Dec 30, 2025

UnexpectedBox(fiel)

That is some Quicktime legacy usage. The only ISO standard I could find that does it is Motion JPEG 2000.

Won't be hard to add though. Any reason not to have it?

Certainly worth adding in another PR.

coderabbitai[bot]
coderabbitai bot reviewed Dec 30, 2025
View reviewed changes
Copy link

@coderabbitai coderabbitai bot left a 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

♻️ Duplicate comments (1)
src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs (1)

33-33: LGTM! The previous review concern was a false positive.

The ? operator here does not cause fatal errors. Looking at the actual decode_unknown implementation in src/error.rs (lines 72-77), it logs a warning and returns Ok(()). The ? operator simply unwraps the Ok and continues execution—this is fault-tolerant behavior as intended by the PR.

The ? is intentionally kept for forward compatibility: when the strict feature flag is implemented (per the TODO comments), the function can return Err to make unknown atoms fatal in strict mode.

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

110-111: Consider preserving unknown atom data for round-trip encoding.

The Unknown variant stores only the FourCC, discarding the atom's body. When encoded (line 152), this writes just the four-character code without the original data, preventing faithful round-trip preservation of unknown codecs.

If the goal includes preserving unrecognized codecs during decode→encode cycles, consider storing the full Any atom or its serialized bytes:

// In the enum definition
Unknown(Any),  // or Unknown(FourCC, Bytes) if you want to avoid the full Any

// In decode
unknown => {
    crate::decode_unknown(&unknown, Stsd::KIND)?;
    Self::Unknown(unknown)  // preserve full atom
}

// In encode  
Self::Unknown(atom) => atom.encode(buf),

This is a pre-existing design choice, so deferring is reasonable if unknown codecs are intentionally stripped.

Also applies to: 141-144, 152-152

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 65dd76c and a58f296.

📒 Files selected for processing (17)
  • src/atom.rs
  • src/error.rs
  • src/meta/ilst/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/av01.rs
  • src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
  • src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs
🚧 Files skipped from review as they are similar to previous changes (7)
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp09.rs
  • src/moov/trak/mdia/minf/stbl/stsd/uncv.rs
  • src/moov/trak/mdia/minf/stbl/stsd/ac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hev1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/av01.rs
  • src/moov/trak/mdia/minf/stbl/stsd/eac3.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.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/pcm.rs
  • src/meta/ilst/mod.rs
  • src/atom.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/error.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.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/pcm.rs
  • src/meta/ilst/mod.rs
  • src/atom.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/error.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.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/pcm.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs
src/meta/**

📄 CodeRabbit inference engine (CLAUDE.md)

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

Files:

  • src/meta/ilst/mod.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/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: Applies to src/**/*.rs : Use Header::read_from() to handle large atoms without loading entire contents into memory
📚 Learning: 2025-08-31T04:06:57.364Z 
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()

Applied to files:

  • src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
  • src/meta/ilst/mod.rs
  • src/atom.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/error.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs
📚 Learning: 2025-08-31T04:06:57.364Z 
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/any.rs : Register new atom types in the Any enum (src/any.rs) to enable flexible decoding

Applied to files:

  • src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
  • src/meta/ilst/mod.rs
  • src/atom.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/error.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs
📚 Learning: 2025-08-31T04:06:57.364Z 
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Use Header::read_from() to handle large atoms without loading entire contents into memory

Applied to files:

  • src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
  • src/meta/ilst/mod.rs
  • src/atom.rs
  • src/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/opus.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.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/moov/trak/mdia/minf/stbl/stsd/flac.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.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 : All operations should return Result<T> using custom error types defined in error.rs

Applied to files:

  • src/error.rs
🧬 Code graph analysis (10)
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs (2)
src/atom.rs (1)
  • decode_unknown (13-15)
src/error.rs (1)
  • decode_unknown (73-78)
src/meta/ilst/mod.rs (2)
src/atom.rs (1)
  • decode_unknown (13-15)
src/error.rs (1)
  • decode_unknown (73-78)
src/atom.rs (1)
src/error.rs (1)
  • decode_unknown (73-78)
src/moov/trak/mdia/minf/stbl/stsd/flac.rs (2)
src/atom.rs (1)
  • decode_unknown (13-15)
src/error.rs (1)
  • decode_unknown (73-78)
src/moov/trak/mdia/minf/stbl/stsd/opus.rs (2)
src/atom.rs (1)
  • decode_unknown (13-15)
src/error.rs (1)
  • decode_unknown (73-78)
src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs (2)
src/atom.rs (1)
  • decode_unknown (13-15)
src/error.rs (1)
  • decode_unknown (73-78)
src/moov/trak/mdia/minf/stbl/stsd/mod.rs (2)
src/atom.rs (1)
  • decode_unknown (13-15)
src/error.rs (1)
  • decode_unknown (73-78)
src/moov/trak/mdia/minf/stbl/stsd/hevc/hvc1.rs (2)
src/atom.rs (1)
  • decode_unknown (13-15)
src/error.rs (1)
  • decode_unknown (73-78)
src/error.rs (1)
src/atom.rs (1)
  • decode_unknown (13-15)
src/moov/trak/mdia/minf/stbl/stsd/vp9/vp08.rs (2)
src/atom.rs (1)
  • decode_unknown (13-15)
src/error.rs (1)
  • decode_unknown (73-78)
🔇 Additional comments (10)
src/error.rs (1)

72-78: LGTM! Well-designed centralized unknown-atom handler.

The function correctly implements fault-tolerant decoding by logging a warning and returning Ok(()). The structured logging with kind and parent fields provides useful diagnostic context. The TODO comments clearly document the planned evolution toward strict mode.

src/atom.rs (2)

11-15: LGTM! Clean trait extension for centralized unknown handling.

The default implementation provides consistent behavior across all Atom implementors while allowing per-type overrides if needed. Passing Self::KIND as the parent provides useful context in log messages.

200-202: LGTM! Improved logging and unified unknown handling in the macro.

The structured logging with size field for Skip/Free boxes improves debuggability. The centralized Self::decode_unknown(&unknown)? pattern ensures consistent handling across all nested atom decoders.

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

20-20: LGTM!

Correctly delegates unknown atom handling to the centralized decode_unknown path.

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

32-32: LGTM!

Correctly delegates unknown atom handling to the centralized decode_unknown path, maintaining fault-tolerant decoding behavior.

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

110-110: LGTM!

Correctly uses crate::decode_unknown directly with the fourcc parameter as parent context. This is appropriate since decode_with_fourcc is a helper method rather than an Atom trait implementation.

src/meta/ilst/mod.rs (1)

37-37: LGTM!

This change correctly transitions from returning UnexpectedBox errors to fault-tolerant logging, which aligns with the PR objectives and issue #63 requirements.

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

22-22: LGTM!

Correctly delegates unknown atom handling to the centralized decode_unknown path.

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

21-21: LGTM! Unknown atom handling correctly centralized.

The change properly delegates unknown atoms to the trait's decode_unknown method, which logs with parent context and allows decoding to continue. This aligns with the PR's fault-tolerant decoding objectives.

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

141-144: Correct pattern for Codec enum's unknown handling.

The direct call to crate::decode_unknown(&unknown, Stsd::KIND)? is appropriate here since Codec is an enum implementing Decode (not an Atom), so it cannot use Self::decode_unknown(&unknown)? like other atom types do. The pattern logs the unknown codec with parent context before gracefully accepting it as Unknown(kind).

@bradh
Copy link
Collaborator

bradh commented Dec 31, 2025

Certainly worth adding in another PR.

#85

@bradh bradh merged commit 59dd10a into main Dec 31, 2025
1 check passed
@bradh bradh deleted the log-unknowns branch December 31, 2025 02:40
@github-actions github-actions bot mentioned this pull request Dec 27, 2025
Open
This was referenced Dec 31, 2025
Closed
Closed
@bradh bradh restored the log-unknowns branch January 4, 2026 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@bradh bradh bradh approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature request: fault-tolerant decoding

3 participants

@kixelated @bradh