Skip to content

Conversation

@bradh
Copy link
Collaborator

@bradh bradh commented Jan 15, 2026

nmhd is the null media header box, and is used for metadata and text tracks (14496-12, Sections 8.4.5.2, 12.3.2 and 12.5.2)

sthd is the subtitle header box (14496-12, Section 12.6.2)

These boxes are very similar, and the test changes would affect both.

nmhd is the null media header box, and is used for metadata and text
tracks (14496-12, Sections 8.4.5.2, 12.3.2 and 12.5.2

sthd is the subtitle header box (14496-12, Section 12.6.2)

These boxes are very similar, and the test changes would affect both.
@coderabbitai
Copy link

coderabbitai bot commented Jan 15, 2026

Walkthrough

This pull request adds support for two new atom types, Nmhd and Sthd, to the media container library. The changes introduce two new enum variants to the Any enum, create corresponding module implementations in the minf hierarchy with full AtomExt trait support, and extend the Minf struct with optional fields for both atom types. Multiple test files are updated to use Default::default() pattern for Minf initialization instead of explicitly setting certain fields to None. The implementation includes encoding/decoding logic for the new atoms and updates the atom nesting rules to recognize Nmhd and Sthd as optional components within Minf.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 73.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'add nmhd and sthd boxes' directly and clearly summarizes the main changes, which involve adding two new atom variants (Nmhd and Sthd) and their implementations.
Description check ✅ Passed The description is clearly related to the changeset, explaining what nmhd and sthd boxes are, citing relevant ISO standards, and noting their similarity.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

🧹 Recent nitpick comments
src/moov/trak/mdia/minf/sthd.rs (1)

25-38: Good test coverage; minor style note.

The test correctly validates the encode/decode roundtrip. The use of character literals (b's', b't', b'h', b'd') for the FourCC is functionally equivalent to the hex approach in nmhd.rs ([0x6e, 0x6d, 0x68, 0x64]).

For consistency across the codebase, consider using the same style in both tests—the character literal approach here is arguably more readable.


📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1676d9a and 18690b9.

📒 Files selected for processing (13)
  • src/any.rs
  • src/moov/mod.rs
  • src/moov/trak/mdia/minf/mod.rs
  • src/moov/trak/mdia/minf/nmhd.rs
  • src/moov/trak/mdia/minf/sthd.rs
  • src/test/av1.rs
  • src/test/bbb.rs
  • src/test/esds.rs
  • src/test/flac.rs
  • src/test/h264.rs
  • src/test/hevc.rs
  • src/test/uncompressed.rs
  • src/test/vp9.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/test/uncompressed.rs
  • src/test/hevc.rs
  • src/moov/trak/mdia/minf/mod.rs
  • src/test/bbb.rs
  • src/test/esds.rs
  • src/moov/trak/mdia/minf/sthd.rs
  • src/test/av1.rs
  • src/moov/mod.rs
  • src/any.rs
  • src/moov/trak/mdia/minf/nmhd.rs
  • src/test/flac.rs
  • src/test/vp9.rs
  • src/test/h264.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/uncompressed.rs
  • src/test/hevc.rs
  • src/moov/trak/mdia/minf/mod.rs
  • src/test/bbb.rs
  • src/test/esds.rs
  • src/moov/trak/mdia/minf/sthd.rs
  • src/test/av1.rs
  • src/moov/mod.rs
  • src/any.rs
  • src/moov/trak/mdia/minf/nmhd.rs
  • src/test/flac.rs
  • src/test/vp9.rs
  • src/test/h264.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.rs
  • src/test/hevc.rs
  • src/test/bbb.rs
  • src/test/esds.rs
  • src/test/av1.rs
  • src/test/flac.rs
  • src/test/vp9.rs
  • src/test/h264.rs
src/moov/**

📄 CodeRabbit inference engine (CLAUDE.md)

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

Files:

  • src/moov/trak/mdia/minf/mod.rs
  • src/moov/trak/mdia/minf/sthd.rs
  • src/moov/mod.rs
  • src/moov/trak/mdia/minf/nmhd.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 (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: Applies to src/moov/** : Place movie metadata atom implementations under src/moov/ (and its hierarchy: trak/mdia/minf/stbl/stsd)
📚 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/mod.rs
  • src/moov/trak/mdia/minf/sthd.rs
  • src/moov/mod.rs
  • src/moov/trak/mdia/minf/nmhd.rs
📚 Learning: 2025-08-31T04:06:57.364Z
Learnt from: CR
Repo: kixelated/mp4-atom PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T04:06:57.364Z
Learning: Applies to src/**/*.rs : Each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body()

Applied to files:

  • src/moov/trak/mdia/minf/sthd.rs
  • src/any.rs
  • src/moov/trak/mdia/minf/nmhd.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/sthd.rs
  • src/any.rs
  • src/moov/trak/mdia/minf/nmhd.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/moov/mod.rs
  • src/moov/trak/mdia/minf/nmhd.rs
🧬 Code graph analysis (2)
src/moov/trak/mdia/minf/sthd.rs (1)
src/moov/trak/mdia/minf/nmhd.rs (2)
  • decode_body_ext (12-14)
  • encode_body_ext (16-18)
src/moov/trak/mdia/minf/nmhd.rs (2)
src/moov/trak/mdia/minf/sthd.rs (2)
  • decode_body_ext (12-14)
  • encode_body_ext (16-18)
src/meta/mod.rs (1)
  • decoded (340-340)
🔇 Additional comments (17)
src/test/vp9.rs (1)

175-176: LGTM!

The use of ..Default::default() is appropriate here to accommodate the new optional nmhd and sthd fields in Minf without explicitly listing them. This keeps the test focused on the fields relevant to the VP9 video track while allowing the struct to evolve with new optional components.

src/test/hevc.rs (1)

382-383: LGTM!

Consistent with the broader test refactoring to use ..Default::default() for unspecified Minf fields. The video track correctly specifies vmhd while new optional fields (nmhd, sthd) default to None.

src/test/uncompressed.rs (1)

214-215: LGTM!

The change aligns with the test suite's shift to default-based initialization for Minf, properly accommodating the new nmhd and sthd optional fields while maintaining explicit values for video-relevant fields (vmhd).

src/test/esds.rs (1)

122-122: LGTM!

The video track correctly specifies vmhd while defaulting other optional fields. This test properly validates both video (Avc1) and audio (Mp4a) tracks, each using their appropriate media header (vmhd and smhd respectively).

src/test/h264.rs (2)

181-181: LGTM!

The video track's Minf correctly specifies vmhd and defaults new optional fields. The explicit field assignments for stbl components remain intact, ensuring the test continues to validate AVCC extension handling.


283-283: LGTM!

The audio track's Minf correctly specifies smhd and defaults new optional fields, maintaining consistency with the video track pattern and the broader test suite refactoring.

src/test/bbb.rs (1)

122-122: LGTM!

Using ..Default::default() for Minf is appropriate here since this is a video track (uses vmhd), and the new nmhd/sthd fields should default to None.

src/any.rs (1)

319-322: LGTM!

The new Nmhd and Sthd atom types are correctly registered in the Any enum alongside the other media header types (Smhd, Vmhd). This follows the repository convention of grouping related atoms within the Minf section. Based on learnings, this registration enables flexible decoding for these new atom types.

src/moov/mod.rs (1)

227-228: LGTM!

The test correctly uses ..Default::default() for the Minf struct. Since this is a video track (with vmhd explicitly set), using the default for remaining fields (smhd, nmhd, sthd) is appropriate and future-proofs the test against additional optional fields.

src/test/flac.rs (1)

153-154: LGTM!

The use of ..Default::default() is appropriate for this audio track test. The smhd field is explicitly set (as required for audio tracks), while the new optional nmhd/sthd fields and vmhd correctly default to None.

src/test/av1.rs (1)

165-166: LGTM!

The use of ..Default::default() is appropriate for this video track test. The vmhd field is explicitly set (as required for video tracks), while the new optional nmhd/sthd fields and smhd correctly default to None.

src/moov/trak/mdia/minf/nmhd.rs (2)

1-19: LGTM! Clean implementation following established patterns.

The Nmhd (null media header) atom implementation correctly:

  • Uses AtomExt with Ext = () for a version-0 fullbox with no body data
  • Matches the pattern used by Sthd and other similar atoms
  • Is correctly placed under src/moov/trak/mdia/minf/ per the project structure guidelines

Based on learnings, ensure the Nmhd type is registered in the Any enum in src/any.rs for flexible decoding.


25-38: Good test coverage for the encode/decode roundtrip.

The test correctly validates:

  • 12-byte fullbox structure (4-byte size + 4-byte type + 4-byte version/flags)
  • The FourCC bytes [0x6e, 0x6d, 0x68, 0x64] correctly spell "nmhd"
  • Successful roundtrip decode back to the original struct
src/moov/trak/mdia/minf/mod.rs (3)

2-5: Module declarations properly organized.

The new nmhd and sthd modules are correctly declared and maintain alphabetical ordering with the existing modules.


9-12: Re-exports correctly expose the new types.

Public re-exports follow the established pattern and make Nmhd and Sthd accessible from the minf module.


19-26: Struct fields and nested macro correctly updated.

The Minf struct now includes optional nmhd and sthd fields, and the nested! macro properly registers Nmhd and Sthd in the optional list alongside Vmhd and Smhd. This aligns with ISO/IEC 14496-12, where these are mutually exclusive media header boxes within minf.

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

1-19: LGTM! Implementation mirrors Nmhd as expected.

The Sthd (subtitle header) atom implementation is correct and follows the same pattern as Nmhd. Both are empty fullbox atoms per ISO/IEC 14496-12.

Based on learnings, ensure the Sthd type is registered in the Any enum in src/any.rs for flexible decoding.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

@bradh bradh merged commit 24bc34a into kixelated:main Jan 16, 2026
1 check passed
@bradh bradh deleted the nmhd_2026-01-15 branch January 16, 2026 10:49
@github-actions github-actions bot mentioned this pull request Jan 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants