Add support for raw audio as per ISO/IEC 23003-5 #78

SanchayanMaity · 2025-12-23T13:04:29Z

While at it, also implement the channel layout box as per ISO/IEC 14496-12 which is mandatory when the PCM channel count is larger than one.

coderabbitai · 2025-12-23T13:08:26Z

Walkthrough

Adds uncompressed audio support to stsd by introducing PCM sample-entry types (Sowt, Twos, Lpcm, Ipcm, Fpcm, In24, In32, Fl32, Fl64, S16l) implemented via a shared Pcm/PcmC representation with optional Chnl. Adds a new chnl module with channel structure enums, explicit speaker positions, versioned AtomExt encoding/decoding, and tests. Extends Codec and Any enums and stsd encode/decode paths to handle the new atoms.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 41.03% 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 'Add support for raw audio as per ISO/IEC 23003-5' directly describes the main change: adding PCM/raw audio support with 12 new audio codec variants and comprehensive encoding/decoding infrastructure.
Description check	✅ Passed	The description 'While at it, also implement the channel layout box as per ISO/IEC 14496-12 which is mandatory when the PCM channel count is larger than one' is directly related to the changeset, detailing the channel layout implementation added in chnl.rs.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/moov/trak/mdia/minf/stbl/stsd/pcm.rs (1)
90-134: Consider minor efficiency improvement in macro-generated encode.

The macro correctly generates Atom implementations for all PCM sample entry types. However, the encode_body method (lines 112-119) clones all fields to create a temporary Pcm struct. This could be slightly more efficient by having Pcm::encode_with_fourcc take a reference.
🔎 Potential optimization

Modify Pcm::encode_with_fourcc signature to accept &self:
-    pub fn encode_with_fourcc<B: BufMut>(&self, buf: &mut B) -> Result<()> {
+    pub fn encode_with_fourcc<B: BufMut>(&self, buf: &mut B) -> Result<()> {
Then update the macro's encode_body:
             fn encode_body<B: BufMut>(&self, buf: &mut B) -> Result<()> {
                 let entry = Pcm {
                     fourcc: Self::KIND,
-                    audio: self.audio.clone(),
-                    pcmc: self.pcmc.clone(),
-                    chnl: self.chnl.clone(),
+                    audio: self.audio,
+                    pcmc: self.pcmc,
+                    chnl: self.chnl,
                 };
-                entry.encode_with_fourcc(buf)
+                entry.encode_with_fourcc(buf)
             }
Note: This assumes Audio, PcmC, and Chnl implement Copy or you'd use references throughout.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3e24dfb and 50922fa.

📒 Files selected for processing (4)

src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.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/any.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/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/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/**/*.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/any.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.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.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/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 : Use Header::read_from() to handle large atoms without loading entire contents into memory

Applied to files:

src/moov/trak/mdia/minf/stbl/stsd/mod.rs

🧬 Code graph analysis (2)

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

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

decode_body (123-188)

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

decode (112-140)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (4)

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

value (41-46)

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

decode_body (13-28)

encode_body (30-43)

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

decode (112-140)

src/meta/mod.rs (1)

decoded (340-340)

🔇 Additional comments (11)

src/any.rs (1)

298-300: LGTM! New PCM atom types properly registered.

The 12 new atom variants are correctly added to the Any enum's macro invocation, following the established pattern for codec sample entries and their nested atoms. The macro will generate all necessary trait implementations for encoding, decoding, and type conversions.

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

6-6: LGTM! Module structure follows the established hierarchy.

The new chnl and pcm modules are properly declared and publicly exported, following the project's module organization pattern for stsd codec implementations.

Also applies to: 15-15, 27-27, 36-36

95-105: LGTM! PCM codec variants properly added.

All 10 uncompressed audio codec variants are correctly added to the Codec enum with clear documentation.

160-169: LGTM! Encode path properly handles all PCM codecs.

All 10 PCM codec variants correctly delegate to their respective atom encoding implementations.

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

1-44: LGTM! PcmC atom implementation is correct.

The PcmC struct properly implements the Atom trait with:

FullBox-style header handling (version and flags)

Version 0 validation

Correct endianness flag interpretation (0 = big-endian, 1 = little-endian)

Proper error handling

Based on learnings, this follows the required pattern for MP4 atom implementations.

46-88: LGTM! Pcm decode/encode implementation is correct.

The Pcm struct properly:

Decodes the base Audio sample entry

Uses Any::decode_maybe to flexibly decode nested atoms

Returns an error if the mandatory PcmC box is missing

Handles the optional Chnl box

Logs warnings for unknown atoms (good defensive programming)

136-185: LGTM! Comprehensive test coverage.

The tests properly validate:

Round-trip encoding/decoding for PcmC

Full Pcm entry with channel layout

Correct integration of all nested atoms

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (4)

1-85: LGTM! AudioChannelPosition enum is comprehensive and spec-compliant.

The enum properly defines:

All standard channel positions from ISO/IEC 23091-3

Special positions (ExplicitPosition=126, Unknown=127)

Reserved positions (32-42) documented in comments

A robust from_u8 method that returns None for invalid/reserved values

87-111: LGTM! Channel structure types are well-designed.

The supporting types properly model:

Explicit speaker positions with azimuth/elevation

Both standard and explicit position variants

Channel structures with explicit positions or pre-defined layouts

190-240: LGTM! Encode implementation correctly mirrors decode path.

The encode_body method properly:

Encodes the FullBox header

Handles stream_structure flags

Encodes explicit positions with the 126 marker

Encodes defined layouts with omitted channels map

Handles optional object count

242-281: LGTM! Tests provide good coverage.

The tests validate round-trip encoding/decoding for:

Explicit channel positions

Pre-defined layouts with omitted channels map

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs

src/moov/trak/mdia/minf/stbl/stsd/mod.rs

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

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

242-281: Consider adding tests for object_count and explicit azimuth/elevation positions.

Current tests cover the main paths, but consider adding:

A test with object_count: Some(n) to verify object-structured decoding
A test with SpeakerPosition::Explicit to cover azimuth/elevation serialization

These additional tests would help catch the decode bug noted above.

🔎 Example additional test

#[test]
fn test_chnl_with_explicit_position() {
    let chnl = Chnl {
        channel_structure: Some(ChannelStructure::ExplicitPositions {
            positions: vec![
                SpeakerPosition::Standard(AudioChannelPosition::FrontLeft),
                SpeakerPosition::Explicit(ExplicitSpeakerPosition {
                    azimuth: 45,
                    elevation: 10,
                }),
            ],
        }),
        object_count: None,
    };

    let mut buf = Vec::new();
    chnl.encode(&mut buf).unwrap();

    let decoded = Chnl::decode(&mut &buf[..]).unwrap();
    assert_eq!(chnl, decoded);
}

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 50922fa and c80eb18.

📒 Files selected for processing (4)

src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs

🚧 Files skipped from review as they are similar to previous changes (1)

src/moov/trak/mdia/minf/stbl/stsd/pcm.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/mod.rs
src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/mod.rs
src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/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/**/*.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/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/mod.rs
src/any.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/mod.rs

🔇 Additional comments (7)

src/any.rs (1)

298-300: LGTM! New PCM and channel layout atoms correctly registered.

The new variants for uncompressed audio (Sowt, Twos, Lpcm, Ipcm, Fpcm, In24, In32, Fl32, Fl64, S16l) along with their nested atoms (PcmC, Chnl) are properly added to the basic variant list. This follows the coding guidelines for registering new atom types in the Any enum.

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

6-6: LGTM! Module declarations and re-exports properly structured.

The new chnl and pcm modules are correctly declared and re-exported, following the existing pattern for other codec modules in this directory.

Also applies to: 15-15, 27-27, 36-36

95-106: LGTM! Codec enum extended with all uncompressed audio variants.

The 10 PCM codec variants are correctly added with appropriate comments identifying them as uncompressed audio formats. The naming follows ISO/IEC 23003-5 conventions.

128-137: LGTM! Decode path now includes all PCM variants including Twos.

The previous review concern about the missing Any::Twos decode arm has been addressed—line 129 now correctly handles Twos atoms.

161-170: LGTM! Encode path is symmetric with decode path.

All 10 PCM codec variants have matching encode arms, maintaining consistency with the decode implementation.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (2)

1-85: LGTM! AudioChannelPosition enum correctly implements ISO/IEC 23091-3.

The enum covers standard channel positions (0-31), explicit position marker (126), and unknown position (127). The from_u8 conversion properly returns None for reserved values (32-125).

190-239: Encode implementation is correct.

The encoding logic properly serializes the channel structure followed by the optional object_count. The structure matches the decode expectations (when the decode bug is fixed).

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c80eb18 and c914eb0.

📒 Files selected for processing (4)

src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.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/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/any.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/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/any.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/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/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/**/*.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/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/any.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/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/any.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/mod.rs

🧬 Code graph analysis (2)

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

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (2)

decode_body (123-188)

encode_body (190-239)

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

decode (112-141)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (2)

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

decode_body (13-28)

encode_body (30-43)

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

decode (112-141)

🔇 Additional comments (13)

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

6-6: LGTM! Module integration follows established patterns.

The new chnl and pcm modules are properly declared and re-exported, consistent with how other codec modules are integrated in this file.

Also applies to: 15-15, 27-27, 36-36

95-105: LGTM! Complete and consistent codec integration.

All 10 new uncompressed audio codec variants are properly integrated:

Added to the Codec enum

Handled in decode path (lines 128-137)

Handled in encode path (lines 161-170)

The implementation follows the established pattern and the previous issue with the missing Twos decode case has been addressed.

Also applies to: 128-137, 161-170

src/any.rs (1)

298-300: LGTM! Atom registration follows established patterns.

All 12 new atom types (10 PCM sample entries + PcmC + Chnl) are properly registered in the Any enum under the Stsd hierarchy. The placement in the basic list (rather than boxed) is appropriate for these relatively small metadata structures.

Based on learnings, registering new atom types in the Any enum enables flexible decoding throughout the codebase.

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

3-44: LGTM! PcmC implementation correctly follows FullBox pattern.

The PcmC atom properly implements version/flags handling and format_flags interpretation (0 for big-endian, 1 for little-endian). The encode/decode logic is consistent and correct.

56-76: LGTM! Decode logic correctly handles nested atoms.

The decode_with_fourcc method properly:

Decodes Audio first (per specification order)

Collects nested PcmC and Chnl atoms via decode_maybe loop

Enforces that PcmC is mandatory (returns error if missing)

Treats Chnl as optional

Logs unknown atoms for forward compatibility

78-87: LGTM! Encode logic maintains correct atom order.

The encode_with_fourcc method correctly writes atoms in the proper order: Audio, PcmC, then optional Chnl.

90-123: Macro effectively reduces code duplication.

The define_pcm_sample_entry! macro successfully generates 10 PCM sample entry types with consistent behavior. The encode path (lines 112-120) creates a temporary Pcm and clones fields, which adds minor overhead but ensures consistency with Pcm::encode_with_fourcc. For these small metadata structures, the simplicity and maintainability benefits outweigh the performance cost.

125-134: LGTM! All 10 PCM formats properly generated.

The macro invocations create all required PCM sample entry types with correct FourCC mappings, consistent with the registrations in Any and Codec enums.

136-184: LGTM! Adequate test coverage for PCM atoms.

The tests provide good coverage:

test_pcmc_encode_decode: Validates PcmC round-trip

test_pcm_encode_decode: Validates full PCM sample entry with optional Chnl

Both tests verify encode/decode symmetry, which is essential for ensuring wire format compatibility.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (4)

3-85: LGTM! AudioChannelPosition enum correctly implements ISO/IEC 23091-3.

The enum comprehensively covers standard channel positions (0-31), reserves positions 32-125, and includes special values for explicit positioning (126) and unknown (127). The from_u8 helper properly returns None for reserved/invalid values.

87-118: LGTM! Supporting types are well-structured.

The type hierarchy (ExplicitSpeakerPosition, SpeakerPosition, ChannelStructure, Chnl) correctly models the ISO/IEC 14496-12 channel layout box specification. The use of Option fields allows for the various configuration modes.

143-163: LGTM! Explicit positions loop correctly addressed previous review concern.

The loop limit has been corrected from 64 to 65535 (line 146), properly implementing the ISO/IEC 14496-12 specification for version 0 chnl boxes. The loop correctly handles both standard channel positions and explicit positions with azimuth/elevation coordinates.

242-281: LGTM! Tests adequately cover supported configurations.

The tests validate both channel structure modes:

test_chnl_explicit_positions: Verifies explicit speaker positions

test_chnl_defined_layout: Verifies predefined layout mode

Both tests confirm encode/decode symmetry for their respective cases.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs

coderabbitai

Actionable comments posted: 1

♻️ Duplicate comments (1)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (1)
140-176: Critical: Decode fails when both channel_structure and object_count are present.

When channel_structured=true with defined_layout=0 AND object_structured=true, the while buf.remaining() > 0 loop at line 146 consumes all remaining bytes, leaving nothing for the object_count byte that must be decoded at line 179. This causes decode failure on valid MP4 files where both structures coexist (which ISO/IEC 14496-12 explicitly permits).

This issue was flagged in a past review as "Addressed in commit c914eb0", but the current code still exhibits the problem.
🔎 Recommended fix

Reserve 1 byte for object_count before entering the positions loop when object_structured is true:
             if defined_layout == 0 {
                 let mut positions = Vec::new();
+                // Reserve space for object_count if object_structured
+                let reserved_bytes = if object_structured { 1 } else { 0 };
 
-                while buf.remaining() > 0 {
+                while buf.remaining() > reserved_bytes {
                     let speaker_position = u8::decode(buf)?;
Alternatively, if the channel count is available from the parent AudioSampleEntry, pass it as a parameter to terminate the loop deterministically rather than relying on buffer exhaustion.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c914eb0 and 8b8514f.

📒 Files selected for processing (4)

src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs

🚧 Files skipped from review as they are similar to previous changes (1)

src/moov/trak/mdia/minf/stbl/stsd/pcm.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/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/any.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/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/any.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/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.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

📚 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/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/any.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/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/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/mod.rs
src/any.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/mod.rs

🔇 Additional comments (7)

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

6-7: LGTM! Module organization is clean.

The new chnl and pcm modules are properly declared and re-exported following the established pattern for this codebase.

Also applies to: 15-15, 27-27, 36-36

95-105: LGTM! Comprehensive PCM format coverage.

The new uncompressed audio codec variants cover the standard PCM formats (signed/unsigned, little/big endian, various bit depths, integer and floating point). The naming follows MP4 conventions.

128-137: LGTM! Decode and encode paths are symmetric and complete.

All 10 new PCM variants are properly handled in both decode and encode paths. The past issue with the missing Twos variant has been resolved.

Also applies to: 161-170

src/any.rs (1)

298-300: LGTM! New atom types properly registered.

The 12 new atom variants (10 PCM sample entries, PcmC configuration box, and Chnl channel layout box) are correctly registered in the Any enum under the Stsd subtree. The macro will generate all necessary decode/encode implementations and conversions.

Based on learnings, this follows the established pattern for registering new atom types.

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

3-85: LGTM! AudioChannelPosition implementation is comprehensive.

The enum covers all standard positions from ISO/IEC 23091-3, includes special values (126 for explicit position, 127 for unknown), and the from_u8 constructor properly handles reserved/invalid values by returning None.

87-111: LGTM! Data structures align with ISO/IEC 14496-12.

The ExplicitSpeakerPosition, SpeakerPosition, and ChannelStructure types cleanly model the channel layout box specification, supporting both explicit positions and predefined layouts.

190-239: LGTM! Encode logic is correct.

The encoding properly writes version/flags, stream_structure, channel structures (explicit positions or defined layouts), and optional object_count. The logic correctly handles all combinations.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/moov/trak/mdia/minf/stbl/stsd/pcm.rs (1)
13-19: Consider validating flags field in PcmC decode.

The Chnl::decode_body (in chnl.rs) validates that flags must be zero and returns an error otherwise. However, PcmC::decode_body only validates the version but ignores the lower 24 bits of version_and_flags. If the spec requires flags to be zero for PcmC as well, consider adding validation for consistency.
🔎 Proposed addition for flags validation
     fn decode_body<B: Buf>(buf: &mut B) -> Result<Self> {
         let version_and_flags = u32::decode(buf)?;
         let version = (version_and_flags >> 24) as u8;
+        let flags = version_and_flags & 0xFFFFFF;

         if version != 0 {
             return Err(Error::UnknownVersion(version));
         }

+        if flags != 0 {
+            return Err(Error::Unsupported("pcmC box with non-zero flags"));
+        }
+
         let format_flags = u8::decode(buf)?;

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8b8514f and 61825dd.

📒 Files selected for processing (4)

src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.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/any.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/any.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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

📚 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/any.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/any.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/chnl.rs

🧬 Code graph analysis (2)

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

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (2)

decode_body (193-260)

decode_body_with_channel_count (124-187)

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

decode (112-141)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (4)

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

value (41-46)

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

decode (112-141)

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

decode_body (13-28)

encode_body (30-43)

src/meta/mod.rs (1)

decoded (340-340)

🔇 Additional comments (12)

src/any.rs (1)

298-300: LGTM! New atom variants correctly registered.

The PCM sample entry types (Sowt, Twos, Lpcm, Ipcm, Fpcm, In24, In32, Fl32, Fl64, S16l) along with their nested atoms (PcmC, Chnl) are properly registered in the Any enum under the Stsd subtree. This follows the established pattern for other audio codecs and aligns with the coding guidelines for registering new atom types.

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

6-6: Module declarations and exports properly added.

The chnl and pcm modules are correctly declared and re-exported, following the established alphabetical ordering pattern.

Also applies to: 15-15, 27-27, 36-36

95-106: Codec enum variants for uncompressed audio correctly added.

All 10 PCM sample entry types are properly integrated into the Codec enum with clear grouping and documentation comment.

128-137: Decode and encode paths are symmetric and complete.

All PCM variants have matching arms in both Decode and Encode implementations. The previously missing Any::Twos case (flagged in prior review) is now correctly included.

Also applies to: 161-170

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

62-91: Decode loop correctly handles child atoms.

The implementation properly uses Header::decode_maybe and buf.slice(size) pattern for safe child atom parsing. The special handling for Chnl::KIND to use decode_body_with_channel_count addresses the channel count requirement correctly.

113-146: Macro generates correct Atom implementations.

The define_pcm_sample_entry! macro efficiently generates the boilerplate for all 10 PCM types. Each generated struct properly implements Atom with KIND, decode_body, and encode_body as required by the coding guidelines.

159-239: Good test coverage for encode/decode roundtrips.

Tests cover PcmC alone, Pcm with Chnl (DefinedLayout), and Pcm with both channel_structure and object_count. This addresses the prior review feedback requesting test coverage for the combined case.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (5)

3-85: AudioChannelPosition enum correctly maps ISO/IEC 23091-3 positions.

The enum values and from_u8 converter properly handle standard positions (0-31), explicit position marker (126), and unknown (127). Reserved values return None as expected.

120-188: decode_body_with_channel_count provides spec-compliant decoding.

This method correctly uses the channel_count parameter from the parent Audio sample entry to determine exactly how many positions to read, avoiding the buffer-exhaustion heuristic. This is the preferred decoding path used by Pcm::decode_with_fourcc.

262-311: encode_body correctly serializes all channel structure variants.

The encoding logic properly handles:

Stream structure flags (bits 0 and 1)

Explicit positions with marker byte 126 followed by azimuth/elevation

Defined layouts with omitted_channels_map

Optional object_count at the end

376-393: Test for combined channel_structure and object_count added.

This test addresses the prior review feedback and validates that both fields can coexist and round-trip correctly.

210-248: Buffer-exhaustion heuristic with reserved byte handling is acceptable for standalone Chnl decoding.

The reserved_bytes approach (line 213) correctly reserves 1 byte for object_count when object_structured is true. While this heuristic can theoretically fail in edge cases where an explicit position straddles the boundary, it is acceptable here since standalone Chnl atom decoding is limited to unit tests—production code uses Chnl::decode_body_with_channel_count() in the PCM decoding path, which provides channel count context for proper buffer parsing.

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 61825dd and e06ce46.

📒 Files selected for processing (4)

src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.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/any.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/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/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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

📚 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/chnl.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.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/chnl.rs

🧬 Code graph analysis (1)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (2)

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

decode (112-141)

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

decode_body (13-33)

encode_body (35-48)

🔇 Additional comments (13)

src/any.rs (1)

298-300: LGTM!

The new PCM-related atom variants (Sowt, Twos, Lpcm, Ipcm, Fpcm, In24, In32, Fl32, Fl64, S16l) and their sub-atoms (PcmC, Chnl) are correctly registered in the Any enum under the Stsd subtree, enabling flexible decoding as per the coding guidelines.

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

6-6: LGTM!

Module declarations and re-exports for chnl and pcm follow the established patterns in this file.

Also applies to: 15-15, 27-27, 36-36

95-106: LGTM!

All ten uncompressed audio codec variants are added to the Codec enum with appropriate comments.

128-137: LGTM!

The decode path correctly handles all new PCM variants including Twos (previously noted as missing in an earlier review, now addressed).

161-170: LGTM!

The encode path correctly handles all new PCM variants, maintaining symmetry with the decode path.

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

118-162: LGTM!

The macro cleanly generates ten PCM sample entry types, each implementing the Atom trait by delegating to shared Pcm encoding/decoding logic. The approach reduces code duplication effectively.

164-244: LGTM!

Good test coverage for PcmC and Pcm encode/decode round-trips, including the case with both channel layout and object count.

106-116: Atom header wrapping is correctly implemented via the blanket impl<T: Atom> Encode for T in src/atom.rs. self.pcmc.encode(buf) calls this blanket impl (lines 13-31), which wraps encode_body() with a 4-byte size + 4-byte kind header. The integration tests (test_pcm_encode_decode and test_pcm_encode_decode_with_both_channel_and_object) verify round-trip encoding/decoding works correctly.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (5)

3-85: LGTM!

The AudioChannelPosition enum correctly maps ISO/IEC 23091-3 speaker positions with proper handling of reserved values (32-42 not defined), explicit position marker (126), and unknown (127).

120-188: LGTM!

The decode_body_with_channel_count method correctly uses the channel count from the audio sample entry to read exactly the expected number of speaker positions, addressing the earlier review concern about buffer exhaustion.

190-260: LGTM!

The Atom implementation for Chnl correctly:

Validates version 0 and zero flags

Parses stream_structure bitfield for channel/object flags

Reserves bytes for object_count when object_structured is true (addressing the earlier review concern)

Handles both explicit positions and defined layouts

262-311: LGTM!

The encode_body implementation correctly:

Writes FullBox header (version 0, flags 0)

Computes stream_structure from presence of channel_structure and object_count

Encodes either explicit positions (with marker 126 for explicit speaker positions) or defined layout with omitted channels map

Appends object_count when present

314-394: LGTM!

Good test coverage including the critical case with both channel_structure and object_count present (lines 376-393), which was flagged as missing in a previous review.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

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

140-148: Consider avoiding clones during encoding.

The encode path clones audio, pcmc, and chnl to create a temporary Pcm struct. Since encode_with_fourcc only needs references, you could refactor to accept references instead.

🔎 Proposed optimization

Add a reference-based encode method to avoid cloning:

 impl Pcm {
+    pub fn encode_fields<B: BufMut>(
+        audio: &Audio,
+        pcmc: &PcmC,
+        chnl: Option<&Chnl>,
+        buf: &mut B,
+    ) -> Result<()> {
+        audio.encode(buf)?;
+        pcmc.encode(buf)?;
+        if let Some(chnl) = chnl {
+            chnl.encode(buf)?;
+        }
+        Ok(())
+    }
 }

Then in the macro:

             fn encode_body<B: BufMut>(&self, buf: &mut B) -> Result<()> {
-                let entry = Pcm {
-                    fourcc: Self::KIND,
-                    audio: self.audio.clone(),
-                    pcmc: self.pcmc.clone(),
-                    chnl: self.chnl.clone(),
-                };
-                entry.encode_with_fourcc(buf)
+                Pcm::encode_fields(&self.audio, &self.pcmc, self.chnl.as_ref(), buf)
             }

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

122-262: Consider extracting shared decode logic to reduce duplication.

The decode_body_with_channel_count and decode_body methods share significant logic (version/flags parsing, stream_structure parsing, DefinedLayout handling). You could extract a common helper that accepts a position-reading strategy.

🔎 Possible refactoring approach

impl Chnl {
    fn decode_header<B: Buf>(buf: &mut B) -> Result<(bool, bool, Option<u8>)> {
        let version_and_flags = u32::decode(buf)?;
        let version = (version_and_flags >> 24) as u8;
        let flags = version_and_flags & 0xFFFFFF;

        if version != 0 {
            return Err(Error::UnknownVersion(version));
        }
        if flags != 0 {
            return Err(Error::Unsupported("chnl box with non-zero flags"));
        }

        let stream_structure = u8::decode(buf)?;
        let channel_structured = (stream_structure & 0x01) != 0;
        let object_structured = (stream_structure & 0x02) != 0;
        
        let defined_layout = if channel_structured {
            Some(u8::decode(buf)?)
        } else {
            None
        };

        Ok((channel_structured, object_structured, defined_layout))
    }
    
    // Then use this in both decode methods
}

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e06ce46 and 7d57132.

📒 Files selected for processing (4)

src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.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/any.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/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/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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

📚 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/chnl.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.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/chnl.rs

🧬 Code graph analysis (2)

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

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

decode_body (195-262)

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

decode (112-141)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (2)

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

decode (112-141)

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

decode_body (13-33)

encode_body (35-48)

🔇 Additional comments (12)

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

95-106: LGTM! PCM codec variants correctly integrated.

All ten uncompressed audio variants are properly added to the Codec enum with consistent naming and structure matching the existing codec patterns.

128-137: Decode paths correctly implemented.

All PCM variants are properly handled in the decode match, including Twos which was missing in an earlier revision. The pattern is consistent with other codecs.

161-170: Encode paths correctly implemented.

All PCM variants delegate to their respective atom.encode(buf) implementations, consistent with the existing codec encoding pattern.

src/any.rs (1)

298-300: LGTM! New atom types correctly registered.

All 12 new types (10 PCM sample entries, PcmC, and Chnl) are properly registered in the Any enum under the Stsd subtree. This enables flexible decoding as per coding guidelines.

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

3-49: LGTM! PcmC atom implementation is correct.

The PcmC struct properly implements the Atom trait with:

Version 0 / flags 0 handling per ISO/IEC 23003-5

Correct endianness flag encoding (0 = big-endian, 1 = little-endian)

Proper sample size field handling

60-104: Decoding logic handles nested atoms correctly.

The decode_with_fourcc method properly:

Reads the Audio sample entry first

Iterates through nested atoms using Header::decode_maybe

Uses decode_body_with_channel_count for Chnl to access audio.channel_count

Returns Error::MissingBox if required PcmC is absent

118-151: Macro correctly generates PCM sample entry types.

The define_pcm_sample_entry! macro properly:

Creates a struct with audio, pcmc, and optional chnl fields

Implements Atom trait with appropriate KIND constant

Delegates to shared Pcm decoding/encoding logic to avoid duplication

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (5)

3-84: LGTM! AudioChannelPosition enum is well-designed.

The enum correctly maps ISO/IEC 23091-3 channel positions with #[repr(u8)] for safe casting. The from_u8 conversion properly handles known positions and returns None for reserved/invalid values. Good decision to exclude value 126 (ExplicitPosition marker) from the enum to prevent encoding ambiguity.

122-190: Channel count-aware decoding correctly implemented.

The decode_body_with_channel_count method properly uses the channel_count from the parent Audio sample entry to read exactly the right number of speaker positions when defined_layout == 0. This resolves the ISO/IEC 14496-12 compliance issue where version 0 doesn't encode an explicit count in the box itself.

212-250: Buffer-based decoding handles both structures correctly.

The reserved_bytes logic on line 215/220 properly reserves 1 byte for object_count when object_structured is true, preventing the explicit positions loop from consuming the final byte. This addresses the previous encode/decode asymmetry concern.

264-314: Encoding implementation is correct and complete.

The encode_body method properly:

Sets stream_structure flags based on which optional fields are present

Writes defined_layout = 0 for explicit positions

Encodes explicit positions with the 126 marker followed by azimuth/elevation

Writes object_count after channel structure data

378-395: Good test coverage for both channel structure and object count.

This test validates that the encode/decode roundtrip works correctly when both channel_structure and object_count are present, which was a previously identified edge case.

bradh · 2025-12-24T03:03:29Z

We have an AtomExt trait for FullBox. Can that be used here instead of open-coding the header flag and version parsing and serialisation?

bradh · 2025-12-24T03:03:56Z

Sorry for power cycling. My bad.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (1)
321-401: Test coverage gaps for different decode paths.

The tests verify round-trip correctness for various scenarios, but they all use Chnl::decode() which goes through the AtomExt path with channel_count=None. This doesn't exercise the decode_body_with_channel_count method that's used by the PCM sample entries.

Consider adding tests that directly call decode_body_with_channel_count to verify that the channel-count-aware decode path works correctly.
🔎 Suggested additional test
#[test]
fn test_chnl_decode_with_channel_count() {
    let chnl = Chnl {
        channel_structure: Some(ChannelStructure::ExplicitPositions {
            positions: vec![
                SpeakerPosition::Standard(AudioChannelPosition::FrontLeft),
                SpeakerPosition::Standard(AudioChannelPosition::FrontRight),
            ],
        }),
        object_count: Some(2),
    };

    let mut buf = Vec::new();
    chnl.encode(&mut buf).unwrap();
    
    // Skip the FullBox header (4 bytes: size) and (4 bytes: type) and (4 bytes: version+flags)
    let body_start = 12;
    let mut body_buf = &buf[body_start..];
    
    let decoded = Chnl::decode_body_with_channel_count(&mut body_buf, 2).unwrap();
    assert_eq!(chnl, decoded);
}

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7d57132 and f340e14.

📒 Files selected for processing (4)

src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs

🚧 Files skipped from review as they are similar to previous changes (1)

src/moov/trak/mdia/minf/stbl/stsd/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/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/any.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/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/any.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/chnl.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/**/*.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/**/*.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/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/any.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/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/any.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/chnl.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/chnl.rs

🧬 Code graph analysis (1)

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

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

decode_body_ext (244-267)

encode_body_ext (269-318)

decode_body_with_channel_count (201-236)

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

decode_body_ext (180-190)

decode (112-141)

encode_body_ext (192-199)

🔇 Additional comments (11)

src/any.rs (1)

298-300: LGTM!

The PCM atom variants are properly registered in the Any enum and correctly nested under the Stsd hierarchy. This enables flexible decoding of uncompressed audio atoms as specified in ISO/IEC 23003-5.

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

3-7: LGTM!

The use of the ext! macro for PcmC correctly leverages the AtomExt trait pattern for FullBox handling, addressing bradh's suggestion to avoid manually parsing version/flags.

16-44: LGTM!

The AtomExt implementation for PcmC correctly handles endianness flags and sample size encoding/decoding. The format_flags logic (0 for big-endian, 1 for little-endian) is clear and symmetric.

72-127: LGTM!

The custom decode/encode methods correctly handle the special relationship between Audio.channel_count and Chnl decoding. The decode loop properly limits buffer scope and advances correctly. Good use of tracing::warn! for unknown atoms to maintain forward compatibility.

129-167: LGTM!

The define_pcm_sample_entry! macro elegantly generates 10 PCM sample entry types with consistent Atom trait implementations. Each type correctly delegates to the shared Pcm encode/decode logic, following DRY principles.

169-355: LGTM!

Comprehensive test coverage for PCM encoding/decoding, including edge cases with channel layouts, object counts, and mixed speaker position types. All tests verify round-trip correctness.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (5)

3-84: LGTM!

The AudioChannelPosition enum correctly maps ISO/IEC 23091-3 channel positions. The exclusion of value 126 from the enum (handled separately as a marker for explicit positions) properly prevents the encode/decode ambiguity mentioned in previous reviews.

86-110: LGTM!

The supporting types (ExplicitSpeakerPosition, SpeakerPosition, ChannelStructure) accurately model the ISO/IEC 14496-12 channel layout specification with appropriate type safety.

112-116: LGTM!

The ext! macro correctly defines the Chnl FullBox structure with versions 0 and 1, following the AtomExt pattern as recommended by bradh.

130-237: LGTM!

The decode implementation correctly addresses the channel count issue mentioned in past reviews by accepting channel_count as a parameter. The decode_channel_structure method properly handles both known (read exactly N positions) and unknown (read until exhaustion with reserved bytes) channel counts, preventing the buffer consumption bug when both channel_structure and object_count are present.

239-319: LGTM!

The AtomExt implementation correctly handles both channel_structured and object_structured flags independently (addressing the mutual exclusivity bug from past reviews). Version 1 is appropriately rejected with a clear error. The encode/decode logic is symmetric and correctly handles all variants.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs

src/moov/trak/mdia/minf/stbl/stsd/mod.rs

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (2)

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

112-116: Clarify version 1 support declaration.

The ext! macro declares support for versions [0, 1], but the decode_body_ext implementation at line 245 explicitly rejects version 1 with Error::Unsupported. Consider either:

Removing version 1 from the versions array if it's not implemented

Adding a comment explaining that version 1 is declared but deferred for future implementation

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

95-105: Clarify the standard sources for these PCM codec FourCC codes.

These codes come from multiple standards, not just ISO/IEC 23003-5 as the PR description suggests:

sowt, twos, in24, in32, fl32, fl64, s16l, fpcm are QuickTime/MOV FourCC codes (mp4ra.org, Telestream audio format specs)

ipcm is explicitly the ISO/IEC 23003-5 integer PCM tag

lpcm is used in both QuickTime and ISO/MP4 contexts (endianness handling varies)

Update the enum comment (or add a doc comment) to clarify that these implementations support both QuickTime and ISO/IEC 23003-5 uncompressed audio formats.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f340e14 and df3f73f.

📒 Files selected for processing (4)

src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs

🚧 Files skipped from review as they are similar to previous changes (1)

src/any.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/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/**/*.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/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/chnl.rs

🧬 Code graph analysis (1)

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

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

decode_body_ext (244-267)

encode_body_ext (269-318)

decode_body_with_channel_count (201-236)

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

decode_body_ext (180-190)

decode (112-141)

encode_body_ext (192-199)

🔇 Additional comments (14)

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

6-6: LGTM! Module declarations follow the established pattern.

The chnl and pcm modules are correctly declared and exported, consistent with other modules in this file.

Also applies to: 15-15, 27-27, 36-36

128-137: LGTM! All PCM variants properly handled in decode path.

All 10 uncompressed audio variants are correctly registered in the decode match statement, following the established pattern.

161-170: LGTM! All PCM variants properly handled in encode path.

All 10 uncompressed audio variants are correctly registered in the encode match statement, maintaining symmetry with the decode path.

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

3-14: LGTM! Correctly uses AtomExt pattern for FullBox.

The implementation follows bradh's recommendation to use the AtomExt trait rather than manually handling FullBox headers. The structure is clean and consistent with the codebase patterns.

16-44: LGTM! PcmC encode/decode logic is correct.

The endianness handling is properly implemented with format_flags: 0 for big-endian, 1 for little-endian. The encode and decode paths are symmetric.

46-70: LGTM! Pcm structure and helper methods are well-designed.

The encode_fields method is appropriately public as it's used by the macro-generated types. The structure cleanly separates the common PCM logic from the specific FourCC variants.

72-115: LGTM! Decode logic correctly handles Chnl with channel_count.

The implementation properly addresses the requirement to pass the channel count from the Audio sample entry to the Chnl decoder. The decode loop uses appropriate buffer management and error handling.

129-167: LGTM! Macro-generated PCM sample entries follow best practices.

The macro correctly generates Atom trait implementations for each PCM variant, reducing code duplication while maintaining type safety. Each generated type properly implements KIND, decode_body, and encode_body as required by the learnings. Based on learnings.

169-355: LGTM! Comprehensive test coverage.

The tests thoroughly cover different combinations of PcmC, Chnl with defined layouts, explicit positions, object counts, and mixed speaker position types. The round-trip encode/decode pattern ensures correctness.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (5)

86-110: LGTM! Channel structure types are well-designed.

The type hierarchy (ChannelStructure, SpeakerPosition, ExplicitSpeakerPosition) cleanly represents both explicit positions and defined layouts as specified in ISO/IEC 14496-12.

130-197: LGTM! Channel structure decoding correctly handles all scenarios.

The implementation properly:

Reserves space for object_count when both structures are present (line 163)

Uses channel_count when available to read exact positions (lines 153-160)

Falls back to buffer exhaustion when channel_count is unknown (lines 161-167)

Correctly distinguishes between standard and explicit speaker positions

These fixes address the critical issues identified in previous reviews.

199-237: LGTM! Custom decode method correctly provides channel count context.

This public method elegantly solves the problem of needing channel count from the parent Audio sample entry, which the standard Atom trait's decode_body cannot provide. The approach enables proper decoding of explicit channel positions without buffer exhaustion issues.

239-319: LGTM! AtomExt implementation correctly handles encode/decode.

The implementation properly:

Rejects version 1 explicitly (as bradh noted, version 1 requires additional handling)

Encodes/decodes stream structure bits independently (allowing both channel and object structures)

Handles both explicit positions and defined layouts

Maintains encode/decode symmetry

The marker value 126 for explicit positions is handled correctly since no AudioChannelPosition enum variant uses that value.

321-425: LGTM! Comprehensive test coverage including critical scenarios.

The tests thoroughly cover:

Explicit positions and defined layouts

Mixed standard and explicit speaker positions

Both channel structure and object count simultaneously (line 383-400)

The custom decode_body_with_channel_count method (line 402-424)

These tests validate the fixes for the critical issues identified in previous reviews.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs

coderabbitai

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (1)
7-84: Add missing audio channel positions 36-42 from ISO/IEC 23091-3:2018 Amendment 1:2022.

The enum and from_u8 method omit positions 36-42, which are defined (not reserved) in Amendment 1:2022:

36: Lfe3 (Low frequency enhancement 3)

37: Leos (Left edge of screen)

38: Reos (Right edge of screen)

39: Hwbcal (Half-way between centre and left edge)

40: Hwbcar (Half-way between centre and right edge)

41: Lbs (Left back surround)

42: Rbs (Right back surround)

Only positions 32-35 are reserved. Without these positions, valid MP4 files using Amendment 1:2022 will fail to decode with Error::Unsupported("invalid speaker position") at line 196.
🔎 Proposed fix
     TopSurroundLeft = 30,
     TopSurroundRight = 31,
-    // 32-42: reserved or special positions
+    // 32-35: reserved
+    Lfe3 = 36,
+    Leos = 37,
+    Reos = 38,
+    Hwbcal = 39,
+    Hwbcar = 40,
+    Lbs = 41,
+    Rbs = 42,
     // ExplicitPosition is handled separately with SpeakerPosition::Standard.
     // ExplicitPosition = 126, // Followed by azimuth (i16) and elevation (i8)
     Unknown = 127, // Unknown/undefined position (unpositioned)
Update from_u8:
             30 => Some(Self::TopSurroundLeft),
             31 => Some(Self::TopSurroundRight),
+            36 => Some(Self::Lfe3),
+            37 => Some(Self::Leos),
+            38 => Some(Self::Reos),
+            39 => Some(Self::Hwbcal),
+            40 => Some(Self::Hwbcar),
+            41 => Some(Self::Lbs),
+            42 => Some(Self::Rbs),
             127 => Some(Self::Unknown),

🧹 Nitpick comments (1)

src/moov/trak/mdia/minf/stbl/stsd/pcm.rs (1)
56-70: Consider making encode_fields private or removing it.

The encode_fields method is public but only used internally by encode_with_fourcc (line 152). Since it's not part of the Atom trait and doesn't appear to be needed externally, consider making it private or inlining it.
🔎 Option 1: Make private
-    pub fn encode_fields<B: BufMut>(
+    fn encode_fields<B: BufMut>(

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between df3f73f and b0cf1d3.

📒 Files selected for processing (4)

src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs

🚧 Files skipped from review as they are similar to previous changes (1)

src/any.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/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.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/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/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/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.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/**/*.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/**/*.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/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/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/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/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/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 : Use Header::read_from() to handle large atoms without loading entire contents into memory

Applied to files:

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/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/moov/trak/mdia/minf/stbl/stsd/chnl.rs

🧬 Code graph analysis (2)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (2)

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

decode (112-141)

decode_body_ext (180-190)

encode_body_ext (192-199)

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

decode_body_ext (21-29)

encode_body_ext (31-43)

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

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

decode_body_ext (246-269)

encode_body_ext (271-320)

decode_body_with_channel_count (202-237)

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

decode_body_ext (180-190)

decode (112-141)

encode_body_ext (192-199)

🔇 Additional comments (13)

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

6-6: LGTM: Module declarations and re-exports are correctly structured.

The new chnl and pcm modules are properly declared and re-exported, following the codebase pattern for module organization under src/moov/trak/mdia/minf/stbl/stsd/.

Also applies to: 15-15, 27-27, 36-36

95-106: LGTM: Codec enum extensions are consistent and complete.

All 10 uncompressed audio codec variants are properly added to the enum. The additions align with the PR objective to support raw audio per ISO/IEC 23003-5 and QuickTime formats.

128-137: LGTM: Decode implementation handles all new codec variants.

All new uncompressed audio variants are correctly mapped in the decode match statement. The previous critical issue with missing Twos decode case has been properly addressed.

161-170: LGTM: Encode implementation is complete and consistent.

All new codec variants are handled in the encode path, maintaining symmetry with the decode implementation.

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

3-44: LGTM: PcmC implementation is clean and follows AtomExt pattern.

The FullBox-based implementation correctly encodes/decodes endianness and sample size. The logic is straightforward: format_flags == 0 indicates big-endian, non-zero indicates little-endian.

72-115: Excellent handling of Chnl decoding with channel count.

The implementation correctly addresses the ISO/IEC 14496-12 requirement by passing audio.channel_count to Chnl::decode_body_with_channel_count (lines 95-98). This solves the buffer consumption issue when explicit positions are used. The fallback to Any::decode_atom for other boxes is also appropriate.

129-167: LGTM: Macro cleanly generates all PCM sample entry types.

The define_pcm_sample_entry! macro eliminates code duplication across 10 PCM format variants while maintaining type safety. Each generated type correctly implements the Atom trait by delegating to shared Pcm logic.

Based on learnings, each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body().

169-355: LGTM: Comprehensive test coverage validates encode/decode symmetry.

Tests cover:

Basic PcmC round-trip

Pcm with defined layout

Pcm with both channel_structure and object_count (lines 220-249)

Explicit positions with standard and custom speaker positions

The test at lines 220-249 specifically validates the scenario where both channel and object structures coexist, ensuring the decode fix is working correctly.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (5)

143-180: LGTM: Channel structure decoding elegantly handles both known and unknown channel counts.

The decode_channel_structure method correctly:

Uses exact count when channel_count is provided (lines 156-160), preventing over-reading

Reserves 1 byte for object_count when channel count is unknown but objects are present (line 164)

Handles both explicit positions and defined layouts (lines 150-178)

This addresses the previous critical issue where the decode loop would consume bytes intended for object_count.

202-237: Public decode method with channel count is well-designed.

The decode_body_with_channel_count method appropriately handles the special case where Pcm needs to decode Chnl with channel count information from the parent Audio sample entry. Manual version/flags parsing (lines 203-213) is necessary here since this bypasses the standard AtomExt decode path. The standard decode_body_ext (line 246) correctly uses AtomExt for normal decoding scenarios.

246-269: LGTM: AtomExt implementation correctly uses extension-based decoding.

The decode_body_ext method properly leverages the ext! macro and ChnlExt parameter for version/flags handling, following the codebase pattern. Version 1 rejection (lines 247-249) is appropriate given the complexity mentioned in the ISO spec.

Based on learnings, the AtomExt trait is being used correctly for FullBox handling.

271-320: LGTM: Encoding correctly supports independent channel and object structures.

The encode implementation properly handles all combinations:

Channel-only (lines 285-310)

Object-only (lines 313-314)

Both structures simultaneously (lines 272-281 set flags independently)

Neither structure

This correctly implements ISO/IEC 14496-12 which states the box "can describe channel-structured streams, object-structured streams, both, or neither."

323-427: LGTM: Test suite validates all critical scenarios.

Tests cover:

Explicit positions (lines 328-344)

Defined layout (lines 347-361)

Mixed explicit and standard positions (lines 364-383)

Both channel and object structures (lines 386-402) - addresses previous missing coverage

Decode with channel count (lines 405-426)

The test at lines 386-402 specifically validates the scenario where both structures coexist, confirming the mutual exclusivity bug has been fixed.

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b0cf1d3 and 7b97478.

📒 Files selected for processing (4)

src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs

🚧 Files skipped from review as they are similar to previous changes (2)

src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/any.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/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/moov/trak/mdia/minf/stbl/stsd/chnl.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/chnl.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/**/*.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/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/moov/trak/mdia/minf/stbl/stsd/chnl.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/chnl.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/chnl.rs

🧬 Code graph analysis (2)

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

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (2)

decode_body_ext (265-288)

encode_body_ext (290-339)

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

decode_body_ext (183-193)

decode (115-144)

encode_body_ext (195-202)

src/meta/mod.rs (1)

decoded (340-340)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (2)

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

decode (115-144)

decode_body_ext (183-193)

encode_body_ext (195-202)

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

decode_body_ext (21-29)

encode_body_ext (31-43)

🔇 Additional comments (10)

src/moov/trak/mdia/minf/stbl/stsd/pcm.rs (5)
3-44: LGTM: PcmC implementation follows AtomExt pattern correctly.

The endianness encoding (0 = big endian, 1 = little endian) is handled consistently across decode and encode paths, and the use of the ext! macro aligns with the project's pattern for FullBox types.

72-115: Verify ISO spec compliance for optional Chnl.

According to the PR description, "the channel layout box is mandatory when the PCM channel count is greater than one." However, the decode path makes chnl optional regardless of audio.channel_count. Consider adding validation to ensure compliance:
let chnl_required = audio.channel_count > 1;
if chnl_required && chnl.is_none() {
    return Err(Error::MissingBox(Chnl::KIND));
}
This would catch non-compliant MP4 files during decode rather than silently accepting them.

Based on coding guidelines and ISO/IEC 14496-12 requirements.

91-98: Good design: Custom Chnl decoding correctly passes channel count.

The special handling for Chnl is necessary because the standard Atom trait's decode_body doesn't accept parameters. Passing audio.channel_count to decode_body_with_channel_count ensures correct parsing of explicit channel positions as discussed in previous reviews.

129-167: Excellent use of macro to eliminate boilerplate for PCM types.

The define_pcm_sample_entry! macro generates 10 structurally identical PCM sample entry types that differ only in their FourCC codes. This approach maintains DRY principles while ensuring each type properly implements the required Atom trait.

169-355: Comprehensive test coverage for PCM and channel layout scenarios.

The tests cover all major code paths including PcmC roundtrip, defined layouts, explicit positions, explicit speaker positions, and various combinations of channel structure with object counts. The encode/decode symmetry validation ensures correctness.
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (5)

162-199: Correct handling of reserved bytes for object_count.

The decode logic properly reserves 1 byte for object_count when object_structured is true (line 183), preventing the consumption of bytes intended for the object count. When channel_count is provided, it reads exactly that many positions, ensuring deterministic decoding. This addresses the issues raised in previous reviews.

221-256: Custom decode method necessary to pass channel count parameter.

The decode_body_with_channel_count method provides a way for the parent Pcm sample entry to pass the audio.channel_count value during decoding. While this duplicates some header parsing logic, it's necessary because the standard Atom and AtomExt traits don't support parameterized decoding. This design was discussed and agreed upon in previous reviews.

265-288: Standard AtomExt decode path correctly handles both structure flags.

The decode_body_ext implementation properly checks channel_structured and object_structured flags independently (lines 272 and 278), allowing both to be present simultaneously. This fixes the mutual exclusivity issue raised in previous reviews. The version 1 rejection (line 266) is documented as intentional.

290-340: Encode path correctly serializes both structure types.

The encoding logic properly sets stream structure flags, then conditionally writes channel structure (explicit positions or defined layout) and object count. The logic matches the decode path and handles the special marker 126 for explicit speaker positions.

342-446: Comprehensive test coverage addresses previous review feedback.

The test suite covers all major scenarios including explicit positions, defined layouts, explicit speaker positions, simultaneous channel structure and object count (line 405), and the custom decode_body_with_channel_count method (line 424). This addresses the test coverage gaps identified in previous reviews.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs

While at it, also implement the channel layout box as per ISO/IEC 14496-12 which is mandatory when the PCM channel count is larger than one.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (1)
7-100: Minor: Fix reserved range comment.

The AudioChannelPosition enum correctly includes positions 36-42 from ISO/IEC 23091-3:2018 Amendment 1:2022 and excludes the previously invalid positions 43-44. However, the comment on line 48 should read "// 43-125: reserved" instead of "// 45-125: reserved" since the gap starts at position 43 (the position after Rbs = 42).
🔎 Suggested fix
-    // 45-125: reserved
+    // 43-125: reserved
Note: Position 126 is the explicit position marker (handled separately), not a reserved value.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b97478 and f2d1051.

📒 Files selected for processing (4)

src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/mod.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs

🚧 Files skipped from review as they are similar to previous changes (1)

src/moov/trak/mdia/minf/stbl/stsd/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/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/chnl.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/**/*.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/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/any.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/chnl.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/chnl.rs

🧬 Code graph analysis (2)

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

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

decode_body_with_channel_count (598-612)

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

decode (115-144)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (2)

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

decode (115-144)

decode_body_ext (183-193)

encode_body_ext (195-202)

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

decode_body_ext (21-29)

encode_body_ext (31-43)

🔇 Additional comments (8)

src/any.rs (1)

298-300: LGTM! PCM atom variants properly registered.

The new PCM-related atoms (Sowt, Twos, Lpcm, Ipcm, Fpcm, In24, In32, Fl32, Fl64, S16l, PcmC, Chnl) are correctly added to the Any enum's basic group. The macro-driven approach ensures all necessary traits (Decode, Encode, From, TryFrom, AnyAtom) are automatically implemented. Indentation properly reflects the stsd hierarchy.

Based on coding guidelines, new atom types must be registered in the Any enum to enable flexible decoding.

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

3-44: LGTM! PcmC correctly implements AtomExt.

The PcmC struct properly uses the AtomExt trait with the ext! macro to handle FullBox headers (version/flags). The format_flags encoding (0 = big endian, 1 = little endian) is symmetric with decoding.

As per coding guidelines, each MP4 atom type must implement the Atom trait (or AtomExt for FullBox).

72-127: LGTM! Pcm decode/encode logic is correct.

The decode_with_fourcc method properly:

Decodes the Audio sample entry first to obtain channel_count

Uses Chnl::decode_body_with_channel_count to pass channel_count (required per ISO/IEC 14496-12 for v0 explicit positions)

Falls back to Any::decode_atom for other atoms like PcmC

Returns an error if the mandatory PcmC box is missing

The encode_with_fourcc method is symmetric with decoding.

129-167: LGTM! Macro effectively generates PCM sample entries.

The define_pcm_sample_entry! macro generates 10 PCM sample entry structs (Sowt, Twos, Lpcm, Ipcm, Fpcm, In24, In32, Fl32, Fl64, S16l) with identical structure but unique FourCC values. All delegate to the shared Pcm decoding/encoding logic, eliminating code duplication.

As per coding guidelines, each MP4 atom type must implement the Atom trait with KIND, decode_body(), and encode_body().

169-402: LGTM! Comprehensive test coverage.

The test suite covers:

PcmC roundtrip encoding/decoding

Pcm with various Chnl configurations (DefinedLayout, ExplicitPositions)

Both channel_structure and object_count scenarios

Mix of Standard and Explicit speaker positions

Both v0 and v1 Chnl box versions

All tests verify encode/decode symmetry.

As per coding guidelines, write unit tests inline within modules using #[cfg(test)].

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

290-612: LGTM! Decode logic correctly handles both v0 and v1.

The decode implementation properly:

Handles both channel_structured and object_structured flags independently (past review issue resolved)

Provides decode_body_with_channel_count for PCM integration, allowing the parent Audio sample entry to supply channel_count for v0 explicit positions

Validates computed vs decoded object_count in v1 (lines 581-585)

Handles explicit speaker positions (marker 126) and standard positions

Documents the workaround for synthetic test cases when channel_count is unavailable (lines 356-368)

615-697: LGTM! AtomExt implementation correctly handles FullBox.

The Chnl struct properly implements AtomExt to handle FullBox headers (version/flags). The encode_body_ext method intelligently determines the version based on the presence of v1-specific fields (format_ordering and base_channel_count). Validation ensures format_ordering is within the valid range (0-2) per ISO/IEC 14496-12:2022 Section 12.2.4.3.

Based on past review comments, using the AtomExt trait for FullBox is preferred over manually handling version/flags.

699-873: LGTM! Comprehensive test coverage.

The test suite thoroughly covers:

v0 and v1 versions

ExplicitPositions and DefinedLayout channel structures

Combinations of channel_structure and object_count (past review comment addressed)

decode_body_with_channel_count integration

Omitted channels in defined layouts

Unknown/unpositioned audio

Computed object_count validation in v1 (lines 854-872)

All tests verify encode/decode symmetry.

As per coding guidelines, write unit tests inline within modules using #[cfg(test)].

bradh

Partial review. Will do more on this tomorrow.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs

bradh · 2025-12-26T03:19:44Z

Some minor updates in my PR. If those look OK to you, I'm good with this one.

SanchayanMaity · 2025-12-26T08:07:41Z

@bradh Yes, those updates look good. Thank you.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (2)
31-31: Fix documentation typos in channel position comments.

Line 31: The comment should say "Right front centre (Rc)" not "(Lc)" — Lc is left front centre.

Line 113: The comment should say "Right edge of screen (Reos)" not "(Leos)" — the variant name is Reos.
🔎 Proposed fixes
-    /// Right front centre (Lc).
+    /// Right front centre (Rc).
     FrontRightOfCenter = 7,
-    /// Right edge of screen (Leos).
+    /// Right edge of screen (Reos).
     Reos = 38,
Also applies to: 113-113

898-1072: Good test coverage, consider adding tests for explicit positions.

The existing tests cover the main paths well, including both v0 and v1 variants, defined layouts, and object count computation. Consider adding test cases for:

Explicit speaker positions (marker 126 with azimuth/elevation)

Mixed Standard and Explicit positions in the same structure

These would strengthen robustness testing of the speaker position decode/encode logic.
🔎 Suggested test case for explicit positions
#[test]
fn test_chnl_explicit_speaker_positions() {
    let chnl = Chnl {
        channel_structure: Some(ChannelStructure::ExplicitPositions {
            positions: vec![
                SpeakerPosition::Standard(AudioChannelPosition::FrontLeft),
                SpeakerPosition::Explicit(ExplicitSpeakerPosition {
                    azimuth: 30,
                    elevation: 0,
                }),
                SpeakerPosition::Standard(AudioChannelPosition::FrontRight),
            ],
        }),
        object_count: None,
        format_ordering: None,
        base_channel_count: None,
    };

    let mut buf = Vec::new();
    chnl.encode(&mut buf).unwrap();

    let decoded = Chnl::decode(&mut &buf[..]).unwrap();
    assert_eq!(chnl, decoded);
}

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f2d1051 and e135f90.

📒 Files selected for processing (1)

src/moov/trak/mdia/minf/stbl/stsd/chnl.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/moov/trak/mdia/minf/stbl/stsd/chnl.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/chnl.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/chnl.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/chnl.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/chnl.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/chnl.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/chnl.rs

🔇 Additional comments (10)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (10)

204-427: LGTM: Channel layout definitions are comprehensive.

The predefined channel layouts correctly implement ISO/IEC 23091-3:2018/Amd 1:2022 Table 3, including the new binaural layout (21) for headphone playback.

457-465: Note: Truncation in channel_count() is acceptable given practical limits.

The cast to u8 on line 459 truncates if there are more than 255 positions. This aligns with the v1 spec (layout_channel_count is u8) and practical audio channel limits. While v0's AudioSampleEntry.channelcount is u16, real-world audio doesn't exceed 255 channels, so the truncation is reasonable.

468-481: LGTM: AtomExt integration follows best practices.

The use of the ext! macro and struct design properly supports both v0 and v1 chnl boxes, with optional fields clearly indicating version-specific data.

Based on learnings, this correctly implements the Atom trait pattern.

490-534: LGTM: Decode helper methods correctly implement the specification.

The speaker position decoding properly handles the explicit position marker (126), and the stream structure decoding for both v0 and v1 matches the ISO/IEC 14496-12 specification.

536-617: LGTM: Channel structure decoding properly handles both versions.

The v0 decode correctly addresses the buffer consumption issue from past reviews by accepting an optional channel_count parameter. The comment on lines 555-558 clearly explains the workaround for synthetic test cases. The v1 decode is clean and validates the channel_order_definition range per ISO/IEC 14496-12 Section 12.2.4.3.

619-644: LGTM: Object count computation follows the specification.

The v1 object count logic correctly implements the formula from ISO/IEC 14496-12:2022 Section 12.2.4.3, using saturating arithmetic to prevent underflow.

646-728: LGTM: Encoding logic correctly mirrors decoding.

The channel structure encoding for both v0 and v1 properly implements the specification, including the requirement for omitted_channels_map in v0 defined layouts and the optional map in v1.

730-792: LGTM: Body decode methods correctly implement both versions.

The v1 decode includes proper validation that the computed object count matches the decoded value (lines 780-784), ensuring data consistency per the specification.

794-812: LGTM: Public decode method properly supports PCM integration.

The decode_body_with_channel_count method correctly addresses the architectural constraint discussed in past review comments, allowing the parent PCM box to pass channel_count for accurate v0 decoding. The v1 path ignores the parameter since it's self-describing.

814-896: LGTM: AtomExt implementation is correct and complete.

The decode/encode logic properly determines version from field presence and validates v1 constraints (format_ordering 0-2). The unwrap() calls are safe due to the version check on line 829.

Based on learnings, this correctly implements the AtomExt trait with decode_body_ext and encode_body_ext.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/moov/trak/mdia/minf/stbl/stsd/pcm.rs (1)
31-43: Consider simplifying the format_flags assignment.

The conditional assignment of format_flags can be written more concisely using an if-expression, which is idiomatic in Rust and avoids the mutable variable.
🔎 Suggested simplification
-    let mut format_flags = 0u8;
-    if !self.big_endian {
-        format_flags = 1u8;
-    }
+    let format_flags = if self.big_endian { 0u8 } else { 1u8 };
 
     format_flags.encode(buf)?;
     self.sample_size.encode(buf)?;

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e135f90 and 665de9b.

📒 Files selected for processing (2)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.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/moov/trak/mdia/minf/stbl/stsd/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/moov/trak/mdia/minf/stbl/stsd/chnl.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/chnl.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/pcm.rs
src/moov/trak/mdia/minf/stbl/stsd/chnl.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/moov/trak/mdia/minf/stbl/stsd/chnl.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/chnl.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/chnl.rs

🧬 Code graph analysis (2)

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

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (2)

encode_body_ext (827-895)

decode_body_with_channel_count (797-811)

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

decode (115-144)

encode_body_ext (195-202)

src/meta/mod.rs (1)

decoded (340-340)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (2)

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

decode (115-144)

decode_body_ext (183-193)

encode_body_ext (195-202)

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

decode_body_ext (21-29)

encode_body_ext (31-43)

🔇 Additional comments (8)

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

72-115: LGTM! Well-integrated channel layout decoding.

The implementation correctly passes audio.channel_count to the Chnl decoder, which is essential for properly decoding explicit channel positions when defined_layout=0. The buffer management with slice() and advance() is appropriate, and the mandatory PcmC validation is correct per ISO/IEC 23003-5.

129-167: LGTM! Clean macro-based code generation.

The macro effectively reduces boilerplate while maintaining type safety for the 10 PCM sample-entry variants. Each generated type properly implements the Atom trait with KIND, decode_body(), and encode_body() as required by the coding guidelines.

169-402: Excellent test coverage for PCM variants.

The tests comprehensively cover critical scenarios including:

Basic PcmC encode/decode roundtrips

PCM with optional Chnl

Simultaneous channel_structure and object_count (addressing past review concerns)

Mixed standard and explicit speaker positions

Both v0 and v1 channel layout versions

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs (5)

3-199: LGTM! AudioChannelPosition enum correctly implements ISO/IEC 23091-3:2018/Amd. 1:2022.

The enum properly includes all standard positions and the Amendment 1 additions (positions 36-44, including the binaural LeftEar/RightEar positions). The documentation for headphone-only positions is appropriately detailed. The from_u8() conversion correctly handles all defined positions and returns None for reserved ranges.

536-579: LGTM! Decode logic correctly handles buffer boundaries.

The implementation properly addresses the concern from past reviews about consuming bytes intended for object_count. When channel_count is provided (the normal case from PCM sample entries), it reads exactly that many positions deterministically. The fallback path with reserved_bytes is well-documented as a workaround for synthetic test cases.

757-792: LGTM! Version 1 support correctly implements ISO/IEC 14496-12:2022.

The v1 decode/encode logic properly:

Computes objectCount as baseChannelCount - channel_count per Section 12.2.4.3

Validates computed vs decoded object counts match (lines 780-784), catching malformed data

Validates format_ordering is in the valid range 0-2 (line 860)

Determines version based on presence of v1-specific fields during encoding

Also applies to: 855-891

646-728: LGTM! Channel structure encoding correctly implements spec requirements.

The encode logic properly:

Distinguishes between v0 and v1 format requirements

Enforces that v0 DefinedLayout requires omitted_channels_map (line 675-677)

Allows optional omitted_channels_map for v1 DefinedLayout

Uses protocol marker value 126 for explicit speaker positions

Includes layout_channel_count for v1 explicit positions

898-1097: Excellent test coverage for dual-version channel layout support.

The tests thoroughly validate:

Both v0 and v1 encoding/decoding roundtrips

Explicit positions and defined layouts

Optional and required omitted_channels_map for respective versions

Mixed standard and explicit speaker positions

Computed object count for v1 (test at line 1053 validates 3 = 5 - 2)

Integration with channel_count parameter via decode_body_with_channel_count

These tests demonstrate that the issues flagged in past reviews have been properly addressed.

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 665de9b and ac0e61a.

📒 Files selected for processing (2)

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs

🚧 Files skipped from review as they are similar to previous changes (1)

src/moov/trak/mdia/minf/stbl/stsd/chnl.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/moov/trak/mdia/minf/stbl/stsd/pcm.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/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

🧠 Learnings (1)

📚 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

🧬 Code graph analysis (1)

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

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

decode_body_ext (819-824)

encode_body_ext (826-894)

decode_body_with_channel_count (796-810)

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

decode_body_ext (183-193)

decode (115-144)

encode_body_ext (195-202)

🔇 Additional comments (2)

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

129-167: LGTM! Excellent use of macro for code generation.

The define_pcm_sample_entry! macro effectively eliminates code duplication across the 10 PCM sample entry types. The generated implementations correctly delegate to the shared Pcm logic, maintaining consistency and reducing maintenance burden. The macro-generated structs integrate cleanly with the Any enum variants referenced in mod.rs.

169-402: LGTM! Comprehensive test coverage.

The test suite thoroughly validates:

PcmC encode/decode roundtrips

Various Chnl configurations (DefinedLayout, ExplicitPositions, v0/v1 versions)

Both channel-structured and object-based audio

Standard and explicit speaker positions

Multiple PCM sample entry types (Fpcm, Ipcm, Lpcm)

The tests follow consistent patterns and provide strong confidence in the implementation.

coderabbitai · 2025-12-26T08:30:28Z

src/moov/trak/mdia/minf/stbl/stsd/pcm.rs

+    fn decode_body_ext<B: Buf>(buf: &mut B, _ext: PcmCExt) -> Result<Self> {
+        let format_flags = u8::decode(buf)?;
+        let sample_size = u8::decode(buf)?;
+
+        Ok(Self {
+            big_endian: format_flags == 0,
+            sample_size,
+        })
+    }


⚠️ Potential issue | 🟡 Minor

Validate format_flags to prevent incorrect decoding.

The current decode logic treats any non-zero format_flags value as little-endian. According to ISO/IEC 23003-5, format_flags should only be 0 (big-endian) or 1 (little-endian). Consider validating the value to catch malformed data.

🔎 Proposed validation

fn decode_body_ext<B: Buf>(buf: &mut B, _ext: PcmCExt) -> Result<Self> { let format_flags = u8::decode(buf)?; + if format_flags > 1 { + return Err(Error::Unsupported("invalid PCM format_flags")); + } let sample_size = u8::decode(buf)?; Ok(Self { big_endian: format_flags == 0, sample_size, }) }

🤖 Prompt for AI Agents

In src/moov/trak/mdia/minf/stbl/stsd/pcm.rs around lines 21 to 29, validate the decoded format_flags so only 0 or 1 are accepted: after decoding format_flags, check if it equals 0 or 1 and return an Err (e.g. InvalidData/Parse error) for any other value; keep big_endian = format_flags == 0 and sample_size as decoded, and ensure the error carries context about invalid format_flags to make malformed files detectable.

coderabbitai · 2025-12-26T08:30:29Z

src/moov/trak/mdia/minf/stbl/stsd/pcm.rs

+    pub fn decode_with_fourcc<B: Buf>(fourcc: FourCC, buf: &mut B) -> Result<Self> {
+        let audio = Audio::decode(buf)?;
+
+        let mut chnl = None;
+        let mut pcmc = None;
+
+        while buf.remaining() > 0 {
+            let header = match Header::decode_maybe(buf)? {
+                Some(h) => h,
+                None => break,
+            };
+
+            let size = header.size.unwrap_or(buf.remaining());
+            if size > buf.remaining() {
+                break;
+            }
+
+            let mut limited = buf.slice(size);
+
+            if header.kind == Chnl::KIND {
+                // Decode channel layout by using the channel count
+                // information. We cannot rely on the decode_body
+                // implementation of Atom for channel layout box.
+                chnl = Some(Chnl::decode_body_with_channel_count(
+                    &mut limited,
+                    audio.channel_count,
+                )?);
+            } else {
+                match Any::decode_atom(&header, &mut limited)? {
+                    Any::PcmC(atom) => pcmc = Some(atom),
+                    _ => tracing::warn!("unknown atom in PCM sample entry: {:?}", header.kind),
+                }
+            }
+
+            buf.advance(size);
+        }
+
+        Ok(Self {
+            fourcc,
+            audio,
+            pcmc: pcmc.ok_or(Error::MissingBox(PcmC::KIND))?,
+            chnl,
+        })
+    }


⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# Search for validation patterns related to channel_count and chnl rg -nP 'channel_count.*>.*1|chnl.*required|mandatory' --type rust -C 3

Repository: kixelated/mp4-atom

Length of output: 777

🏁 Script executed:

# Check the encode_body or related methods in PCM implementation rg -A 20 'fn encode_body|fn encode|impl.*Atom.*PCM' src/moov/trak/mdia/minf/stbl/stsd/pcm.rs

Repository: kixelated/mp4-atom

Length of output: 2335

🏁 Script executed:

# Look for any validation in the Pcm struct or Audio struct rg -B 5 -A 10 'struct Pcm\|struct Audio' src/moov/trak/mdia/minf/stbl/stsd/pcm.rs --type rust

Repository: kixelated/mp4-atom

Length of output: 44

🏁 Script executed:

# Check if there's validation in the parent module or encoder fd -e rs . src/moov/trak/mdia/minf/stbl/stsd/ | xargs rg -l 'channel_count'

Repository: kixelated/mp4-atom

Length of output: 358

🏁 Script executed:

# Check encode_body implementation in the Atom trait for Pcm rg -B 5 -A 15 'impl.*Atom.*Pcm|fn encode_body.*Pcm' src/moov/trak/mdia/minf/stbl/stsd/pcm.rs --type rust

Repository: kixelated/mp4-atom

Length of output: 882

🏁 Script executed:

# Check if there's any validation in tests for mandatory chnl rg -B 5 -A 15 'mod tests|#\[test\]' src/moov/trak/mdia/minf/stbl/stsd/pcm.rs --type rust

Repository: kixelated/mp4-atom

Length of output: 4686

🏁 Script executed:

# Search for any error checking related to chnl or channel_count in the entire stsd module rg 'chnl.*missing|missing.*chnl|channel.*mandatory' src/moov/trak/mdia/minf/stbl/stsd/ --type rust

Repository: kixelated/mp4-atom

Length of output: 44

🏁 Script executed:

# Check if Audio struct or channel_count validation happens elsewhere rg -B 3 -A 10 'pub channel_count|fn.*channel_count' src/moov/trak/mdia/minf/stbl/stsd/audio.rs --type rust

Repository: kixelated/mp4-atom

Length of output: 498

🏁 Script executed:

# Final check: Look for any validation in the Atom trait implementation or encode methods rg -B 10 -A 5 'impl.*Atom.*for.*Pcm|impl.*Atom.*for.*Sowt|impl.*Atom.*for.*Twos' src/moov/trak/mdia/minf/stbl/stsd/pcm.rs --type rust | head -50

Repository: kixelated/mp4-atom

Length of output: 444

🏁 Script executed:

# Check if there's validation during sample entry construction rg -B 5 -A 15 'fn encode_body\|fn decode_body' src/moov/trak/mdia/minf/stbl/stsd/pcm.rs --type rust | head -80

Repository: kixelated/mp4-atom

Length of output: 44

🏁 Script executed:

# Check if there's a parent encoder/decoder that validates this requirement rg -B 5 -A 10 'channel_count.*chnl|chnl.*channel' src/moov/trak/mdia/minf/stbl/ --type rust

Repository: kixelated/mp4-atom

Length of output: 2669

🏁 Script executed:

# Look for any Result validation or Result propagation that might validate this rg 'channel_count' src/moov/trak/mdia/minf/stbl/stsd/pcm.rs -B 2 -A 2 --type rust

Repository: kixelated/mp4-atom

Length of output: 2333

Add validation to enforce mandatory channel layout box when channel count exceeds one.

The current implementation allows chnl to be None regardless of audio.channel_count. Per the PR description and ISO/IEC 14496-12, the channel layout box is mandatory when audio.channel_count > 1. Add validation in decode_with_fourcc to return an error if chnl is missing when this condition is met.

🤖 Prompt for AI Agents

In src/moov/trak/mdia/minf/stbl/stsd/pcm.rs around lines 72 to 115, after the loop that parses child atoms and before returning Ok(Self { ... }), add validation to enforce the ISO rule: if audio.channel_count > 1 and chnl is None, return Err(Error::MissingBox(Chnl::KIND)); keep the existing error for missing pcmc as-is, but ensure the new check runs so the function fails when a required channel layout box is absent for multi-channel audio.

bradh

LGTM.

coderabbitai bot reviewed Dec 23, 2025

View reviewed changes

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs Outdated Show resolved Hide resolved

src/moov/trak/mdia/minf/stbl/stsd/mod.rs Show resolved Hide resolved

SanchayanMaity force-pushed the raw-audio branch from 50922fa to c80eb18 Compare December 23, 2025 13:30

coderabbitai bot reviewed Dec 23, 2025

View reviewed changes

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs Outdated Show resolved Hide resolved

SanchayanMaity force-pushed the raw-audio branch from c80eb18 to c914eb0 Compare December 23, 2025 13:55

coderabbitai bot reviewed Dec 23, 2025

View reviewed changes

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs Outdated Show resolved Hide resolved

SanchayanMaity force-pushed the raw-audio branch from c914eb0 to 8b8514f Compare December 23, 2025 14:24

coderabbitai bot reviewed Dec 23, 2025

View reviewed changes

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs Show resolved Hide resolved

SanchayanMaity force-pushed the raw-audio branch from 8b8514f to 61825dd Compare December 23, 2025 16:31

coderabbitai bot reviewed Dec 23, 2025

View reviewed changes

SanchayanMaity force-pushed the raw-audio branch from 61825dd to e06ce46 Compare December 23, 2025 16:38

coderabbitai bot reviewed Dec 23, 2025

View reviewed changes

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs Outdated Show resolved Hide resolved

SanchayanMaity force-pushed the raw-audio branch from e06ce46 to 7d57132 Compare December 23, 2025 16:56

coderabbitai bot reviewed Dec 23, 2025

View reviewed changes

bradh closed this Dec 24, 2025

bradh reopened this Dec 24, 2025

SanchayanMaity force-pushed the raw-audio branch from 7d57132 to f340e14 Compare December 24, 2025 09:22

coderabbitai bot reviewed Dec 24, 2025

View reviewed changes

bradh reviewed Dec 24, 2025

View reviewed changes

SanchayanMaity force-pushed the raw-audio branch 2 times, most recently from df3f73f to b0cf1d3 Compare December 24, 2025 09:40

coderabbitai bot reviewed Dec 24, 2025

View reviewed changes

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs Show resolved Hide resolved

coderabbitai bot reviewed Dec 24, 2025

View reviewed changes

SanchayanMaity force-pushed the raw-audio branch from b0cf1d3 to 7b97478 Compare December 24, 2025 10:02

coderabbitai bot reviewed Dec 24, 2025

View reviewed changes

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs Outdated Show resolved Hide resolved

SanchayanMaity force-pushed the raw-audio branch from 7b97478 to de653b9 Compare December 24, 2025 10:09

Add support for raw audio as per ISO/IEC 23003-5

2515680

While at it, also implement the channel layout box as per ISO/IEC 14496-12 which is mandatory when the PCM channel count is larger than one.

SanchayanMaity force-pushed the raw-audio branch from de653b9 to f2d1051 Compare December 25, 2025 09:03

coderabbitai bot reviewed Dec 25, 2025

View reviewed changes

SanchayanMaity requested a review from bradh December 25, 2025 09:43

bradh reviewed Dec 25, 2025

View reviewed changes

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs Show resolved Hide resolved

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs Show resolved Hide resolved

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs Outdated Show resolved Hide resolved

coderabbitai bot reviewed Dec 26, 2025

View reviewed changes

SanchayanMaity force-pushed the raw-audio branch from e135f90 to 665de9b Compare December 26, 2025 08:14

coderabbitai bot reviewed Dec 26, 2025

View reviewed changes

src/moov/trak/mdia/minf/stbl/stsd/chnl.rs Show resolved Hide resolved

SanchayanMaity and others added 2 commits December 26, 2025 13:51

Support version 1 for channel layout box

3b83043

Update channel definition to match ISO 23091-3 Amd 1.

ac0e61a

SanchayanMaity force-pushed the raw-audio branch from 665de9b to ac0e61a Compare December 26, 2025 08:26

coderabbitai bot reviewed Dec 26, 2025

View reviewed changes

bradh approved these changes Dec 26, 2025

View reviewed changes

bradh merged commit a912528 into kixelated:main Dec 26, 2025
1 check passed

github-actions bot mentioned this pull request Dec 26, 2025

chore: release v0.10.0 #79

Open

Add support for raw audio as per ISO/IEC 23003-5 #78

Add support for raw audio as per ISO/IEC 23003-5 #78

Uh oh!

Conversation

SanchayanMaity commented Dec 23, 2025

Uh oh!

coderabbitai bot commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

bradh commented Dec 24, 2025

Uh oh!

bradh commented Dec 24, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

bradh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bradh commented Dec 26, 2025

Uh oh!

SanchayanMaity commented Dec 26, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot commented Dec 23, 2025 •

edited

Loading