Skip to content

Conversation

@fcancela
Copy link

@fcancela fcancela commented Nov 18, 2025

This PR aims to add avc3 support.

It's an initial implementation, reusing all the avc1 codebase. Tested it with a bbc test stream.

Tested with this live stream: https://vs-dash-ww-rd-live.akamaized.net/pl/testcard2020/avc-full.m3u8

@coderabbitai
Copy link

coderabbitai bot commented Nov 18, 2025

Walkthrough

Adds an Avc3 sample-entry variant and wires it throughout the codebase. Introduces Avc3 as a public type alias (AvcSampleEntry<{ AVC3_CODE }>), the private AVC3_CODE, and unit tests exercising base and extended forms. Exposes avc3 in the H.264 module and adds Any::Avc3 handling. Extends the Codec enum with Avc3(Avc3) and updates decode/encode paths to convert between Any::Avc3 and Codec::Avc3. Also generalizes AVC sample entries via AvcSampleEntry<const KIND_CODE: u32> and adds FourCC::from_u32.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Initial avc3 support' directly summarizes the main change: adding avc3 atom support to the MP4 parser.
Description check ✅ Passed The description explains the PR's purpose (adding avc3 support), implementation approach (reusing avc1 codebase), and testing validation with live streams.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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.

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: 1

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

55-57: Codec::Avc3 integration matches existing patterns

Defining Codec::Avc3(Avc3) and wiring it through Codec::decode (Any::Avc3(atom) => atom.into()) and Codec::encode (Self::Avc3(atom) => atom.encode(buf)) is consistent with the other video codecs and leverages the existing derive(From) nicely. Only minor nit: the “SPS/PPS/VPS is inline” comment mentions VPS, which is HEVC terminology; if you want to be precise, you might drop “VPS” from the H.264 comment, but that’s purely cosmetic.

Also applies to: 98-105, 122-129

src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs (3)

3-12: Avc3 Atom shape and decode logic look solid

The Avc3 struct layout (visual + avcc + optional btrt/colr/pasp/taic) and Atom impl with KIND: "avc3" are consistent with the existing stsd/H.264 design. decode_body correctly:

  • decodes the leading Visual,
  • iterates child atoms via Any::decode_maybe(buf)?,
  • populates the optional fields, and
  • enforces presence of Avcc with Error::MissingBox(Avcc::KIND).

Logging unknown children with tracing::warn! is a reasonable compromise for forward‑compatibility. If you ever want stricter behavior, a future enhancement could be to optionally preserve unknown children instead of dropping them, but the current behavior matches typical MP4 parsing patterns.

As per coding guidelines.

Also applies to: 14-44


46-62: Encoding optional child boxes is correct; can simplify slightly

Encoding visual and avcc first, followed by the optional btrt, colr, pasp, and taic boxes, mirrors the decode path and keeps the layout straightforward. The explicit is_some() checks around self.*.encode(buf)? are correct; if Encode is already implemented for Option<T> as “encode if Some, otherwise no-op”, you could drop the if guards and just call self.btrt.encode(buf)?; etc. directly to reduce repetition, but that’s purely stylistic.


65-157: Tests cover primary paths; consider an extra negative case

The two tests provide good coverage:

  • test_avc3 validates a minimal Avc3 with only visual and avcc.
  • test_avc3_with_extras covers round‑tripping with all optional boxes populated.

Together they exercise both decode and encode and rely on PartialEq for a strong equality check. As an optional improvement, you might add a test that decodes an Avc3 without an Avcc child and asserts Error::MissingBox(Avcc::KIND) to lock in that invariant, but the current tests are already useful and aligned with the guidelines of keeping tests inline.

As per coding guidelines.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 16ef4e6 and df6e102.

📒 Files selected for processing (4)
  • src/any.rs (1 hunks)
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs (1 hunks)
  • src/moov/trak/mdia/minf/stbl/stsd/h264/mod.rs (2 hunks)
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs (3 hunks)
🧰 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/h264/mod.rs
  • src/any.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.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/h264/mod.rs
  • src/any.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.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/h264/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.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 (4)
📓 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()
📚 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.rs
  • src/moov/trak/mdia/minf/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.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/stbl/stsd/mod.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.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/h264/avc3.rs
🧬 Code graph analysis (1)
src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs (1)
src/moov/trak/mdia/minf/stbl/stsd/mod.rs (1)
  • decode (99-119)
🪛 GitHub Actions: PR
src/moov/trak/mdia/minf/stbl/stsd/h264/mod.rs

[warning] 1-1: Rustfmt diff detected: 'mod avc1;' vs '-mod avcc;' indicates formatting changes needed in mod.rs.


[warning] 8-8: Rustfmt diff detected: public re-export lines differ; formatting changes needed in mod.rs.


[error] 16-16: Rustfmt check failed. Run 'cargo fmt' to fix formatting.

🔇 Additional comments (1)
src/any.rs (1)

271-277: Any registration of Avc3 is consistent with existing H.264 atoms

Adding Avc3 alongside Avc1 in the any! macro’s basic list correctly plugs it into Any, DecodeAtom, AnyAtom, and the conversion impls. This matches how other stsd entries are wired.

Based on learnings.

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.

Looks generally OK. Didn't check the specs yet.

Its a bit annoying that we have to duplicate code like this, but not clear there is a significantly better way.

Can you fix the formatting issue?

@kixelated
Copy link
Owner

kixelated commented Nov 18, 2025

Looks generally OK. Didn't check the specs yet.

Its a bit annoying that we have to duplicate code like this, but not clear there is a significantly better way.

Can you fix the formatting issue?

I said they should start by copy-pasting AVC1 and sure enough, it decodes properly. But it's definitely not implementing the full spec and yeah the code reuse is frustrating.

Maybe until there's interest in full Avc3 support, a type alias might just work? pub type Avc3 = Avc1?

- Introduced a new AvcSampleEntry struct to encapsulate common fields for AVC sample entries.
- Updated Avc1 and Avc3 to utilize the new AvcSampleEntry type, improving code reusability.
- Added a from_u32 method to the FourCC struct for better FourCC initialization from u32 values.
@fcancela
Copy link
Author

Maybe until there's interest in full Avc3 support, a type alias might just work? pub type Avc3 = Avc1?

Not an expert in Rust, so after a quick Cursor investigation, I got the following answer:

  • Atom::KIND must be b"avc3" for Avc3 while Avc1 stays b"avc1". With a type alias there is only a single impl block, so we can’t publish two different constants or decode_body registrations. The dispatcher (Any, Codec) would see both boxes as avc1 and treat avc3 bytes as unknown.
  • Traits implemented for an alias are the same as for the underlying type, so we couldn’t even add a second impl Atom for Avc3. Rust forbids duplicate impls on aliases.

My latest commit looks to minimize code reuse and plan for future additions to other avc types by:

  • Adding one data structure AvcSampleEntry and one implementation for AVC sample entries, eliminating the previous wrapper struct and duplicate logic.
  • Adding future FourCCs (e.g., avc2/avc4) is a single-line alias instead of a new module or macro.
  • Public API stays the same (Avc1 { … } literals still work), so there’s no user-facing churn.

Happy to defer the decision to you guys! Or if we want to leave this open for now...

@fcancela fcancela requested a review from bradh November 21, 2025 14:01
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.

Minor suggestions only, I can do those as a follow-up if you'd prefer.

@bradh
Copy link
Collaborator

bradh commented Nov 21, 2025

Not an expert in Rust, so after a quick Cursor investigation, I got the following answer:

* Atom::KIND must be b"avc3" for Avc3 while Avc1 stays b"avc1". With a type alias there is only a single impl block, so we can’t publish two different constants or decode_body registrations. The dispatcher (Any, Codec) would see both boxes as avc1 and treat avc3 bytes as unknown.

* Traits implemented for an alias are the same as for the underlying type, so we couldn’t even add a second impl Atom for Avc3. Rust forbids duplicate impls on aliases.

I'm going to guess you have some background in an object-oriented language. Trying to force everything to be an object is making it hard to see alternatives. (Maybe I'm just projecting here, because I sometimes still have this problem).

I haven't actually tried it, but we can probably just turn the Visual parsing into a fn outside the impl block and call that.

@fcancela
Copy link
Author

You guessed correctly 😂

I'll take a look at those suggestions during the weekend.

Thanks for the suggestions!

- Introduced a constant for BBC AVC3 sample data extracted from a test stream.
- Implemented a function to create an expected Avc3 structure for comparison.
- Added a test to validate the decoding of the BBC AVC3 sample, ensuring correct structure and child atom handling.
- Refactored AVC1 code to use a more explicit byte array initialization.
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: 1

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/h264/avc1.rs (1)

50-66: Fix encode_body to encode inner optional atoms instead of the Option wrapper

self.btrt, self.colr, self.pasp, and self.taic are Option<...> fields. Calling .encode on the Option itself (even behind an is_some() check) is incorrect and will either fail to compile or rely on an unintended trait impl.

Encode the inner atom only when present, e.g. using if let Some(...).

Suggested diff:

     fn encode_body<B: BufMut>(&self, buf: &mut B) -> Result<()> {
         self.visual.encode(buf)?;
         self.avcc.encode(buf)?;
-        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)?;
+        }
         Ok(())
     }
🧹 Nitpick comments (1)
src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs (1)

12-39: Avc3 round‑trip tests are solid; shared fixture abstraction is optional

The base() constructor plus roundtrip() helper and the two tests (test_avc3, test_avc3_with_extras) give good coverage of both minimal and fully-populated Avc3 entries and mirror the Avc1 coverage.

If more AVC-style entries appear later, consider extracting a shared test fixture helper (e.g., in a small tests helper module) to avoid repeating the same synthetic Visual/Avcc setup in multiple files, but it's fine as-is for now.

Also applies to: 83-117

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 347a02a and ed6cedb.

⛔ Files ignored due to path filters (1)
  • tests/data/bbc_avc3.bin is excluded by !**/*.bin
📒 Files selected for processing (2)
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs (3 hunks)
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs (1 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/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.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/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.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/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs
🧠 Learnings (4)
📚 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/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.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/h264/avc1.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/h264/avc1.rs
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.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/test/** : Put integration tests under src/test/ with sample MP4 files for different codecs

Applied to files:

  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs
🧬 Code graph analysis (2)
src/moov/trak/mdia/minf/stbl/stsd/h264/avc1.rs (1)
src/types.rs (1)
  • from_u32 (22-24)
src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs (2)
src/types.rs (9)
  • new (18-20)
  • new (193-195)
  • new (304-306)
  • decode (78-80)
  • decode (101-103)
  • decode (144-146)
  • decode (207-212)
  • decode (286-294)
  • decode (325-329)
src/moov/trak/mdia/minf/stbl/stsd/mod.rs (1)
  • decode (99-119)
🪛 GitHub Actions: PR
src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs

[error] 40-40: Rustfmt check failed. Formatting differences detected in the file. Run 'cargo fmt' to fix formatting issues.


[warning] 1-1: Diff in code formatting detected by cargo fmt --check (reported as a warning in logs) but caused the step to fail with exit code 1.

🪛 GitHub Check: build
src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs

[warning] 40-40:
Diff in /home/runner/work/mp4-atom/mp4-atom/src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs

🔇 Additional comments (3)
src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs (2)

1-6: Avc3 alias over AvcSampleEntry cleanly reuses the generic Atom implementation

The AVC3_CODE constant plus pub type Avc3 = AvcSampleEntry<{ AVC3_CODE }>; ties Avc3 into the generic AvcSampleEntry<const KIND_CODE: u32> and its Atom impl nicely, so Avc3 gets the correct KIND and behavior without duplicate logic.


41-143: Real BBC avc3 sample test is a very useful spec/interop check

Using include_bytes! for the BBC-derived BBC_AVC3_SAMPLE and then:

  • Parsing the header and asserting header.kind == Avc3::KIND,
  • Decoding Visual and walking child atoms via Any::decode_maybe,
  • Finally doing a full Avc3::decode and comparing to bbc_expected(),

gives strong confidence that the generic AvcSampleEntry + Atom wiring matches real-world avc3 boxes and that child atoms (avcC, pasp, colr) are handled correctly.

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

3-3: Generic AvcSampleEntry<const KIND_CODE> design cleanly unifies Avc1/Avc3 Atom handling

Defining AVC1_CODE once and using:

  • pub struct AvcSampleEntry<const KIND_CODE: u32> { ... }
  • pub type Avc1 = AvcSampleEntry<{ AVC1_CODE }>;
  • impl<const KIND_CODE: u32> Atom for AvcSampleEntry<KIND_CODE> { const KIND: FourCC = FourCC::from_u32(KIND_CODE); ... }

is a nice way to satisfy the Atom trait requirement (KIND/decode_body/encode_body) for all AVC-style sample entries while keeping Avc1 and Avc3 as simple aliases. This will also make future fourcc variants trivial to add.

Also applies to: 7-7, 16-16, 18-20, 21-48

@bradh
Copy link
Collaborator

bradh commented Nov 27, 2025

I've fixed the formatting nit. The other parts are messier than I'd hoped, and won't get looked at until the weekend (UTC+11 time).

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/h264/avc3.rs (1)

40-40: Verify rustfmt compliance.

A past review comment flagged a rustfmt issue around this line. Please ensure cargo fmt has been run and the file passes cargo fmt -- --check to comply with project formatting requirements.

As per coding guidelines, code must be formatted with rustfmt and pass formatting checks.

Run the following to verify:

#!/bin/bash
# Verify rustfmt compliance for this file
cargo fmt -- --check src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs
🧹 Nitpick comments (1)
src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs (1)

5-5: Consider adding documentation for the public Avc3 type.

Since Avc3 is a public type alias that will be part of the crate's API, adding a doc comment would help users understand its purpose and relationship to the AVC3 codec specification.

Example documentation:

+/// AVC3 sample entry.
+///
+/// Similar to AVC1, but parameter sets are stored in-band rather than in the sample entry.
+/// See ISO/IEC 14496-15 for the AVC file format specification.
 pub type Avc3 = AvcSampleEntry<{ AVC3_CODE }>;
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ed6cedb and f5427b4.

📒 Files selected for processing (1)
  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs (1 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/moov/trak/mdia/minf/stbl/stsd/h264/avc3.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/h264/avc3.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/h264/avc3.rs
🧠 Learnings (5)
📚 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/h264/avc3.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/h264/avc3.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/test/** : Put integration tests under src/test/ with sample MP4 files for different codecs

Applied to files:

  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.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 **/*.rs : Code must be formatted with rustfmt (cargo fmt) and pass formatting checks (cargo fmt -- --check)

Applied to files:

  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.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 **/*.rs : Code must be clippy-clean (cargo clippy --all-targets --all-features -- -D warnings)

Applied to files:

  • src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs
🧬 Code graph analysis (1)
src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs (1)
src/moov/trak/mdia/minf/stbl/stsd/mod.rs (1)
  • decode (99-119)
🔇 Additional comments (2)
src/moov/trak/mdia/minf/stbl/stsd/h264/avc3.rs (2)

48-92: LGTM!

The bbc_expected() function provides comprehensive expected data, and the roundtrip() helper follows standard encode/decode test patterns.


94-145: Excellent test coverage!

The tests are comprehensive and well-structured:

  • Basic roundtrip validation
  • Extended testing with all optional fields (btrt, colr, pasp, taic)
  • Real BBC stream test with thorough structural validation

The BBC test (lines 121-145) directly addresses the past review comment requesting validation with real avc3 boxes, and it carefully verifies both the structure and decoded content.

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.

3 participants