implement 3GPP narrowband codec sample entry (samr)#135
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTwo new AMR atom kinds were added: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/moov/trak/mdia/minf/stbl/stsd/amr/amrsampleentry.rs`:
- Around line 11-22: The encode implementation in AmrSampleEntry writes a 32-bit
value (16u32) causing a 30-byte output while decode expects two 16-bit fields
(total 28 bytes); replace the 16u32.encode(buf)? with two u16 writes matching
decode (e.g., 0u16.encode(buf)? followed by 16u16.encode(buf)?), update the
trailing comments to reflect Reserved_2 for each u16, and keep the rest of
AmrSampleEntry::encode unchanged so the encoded byte count matches the decoder.
In `@src/moov/trak/mdia/minf/stbl/stsd/amr/samr.rs`:
- Line 41: Update the stale comment in samr.rs that reads "Streaminfo metadata
block only" to accurately describe the AMR test data used here (e.g., mention
that this is AMR sample/frames or AMR-specific stream header/test vector), so
replace the FLAC-specific text with a concise AMR-specific description near the
top of the AMR test block in samr.rs.
- Line 19: The match arm using Any::Damr(atom) incorrectly calls atom.into() to
produce an Option; change the arm to wrap the extracted value with Some(atom)
and assign it to damr (i.e., use Some(atom) instead of atom.into()), updating
the match in the code handling Any::Damr and ensuring damr has type Option<...>
as intended.
🧹 Nitpick comments (1)
src/moov/trak/mdia/minf/stbl/stsd/amr/samr.rs (1)
49-71: Consider adding a round-trip test.The current test only validates decoding. Adding an encode test or a decode→encode→decode round-trip test would help catch serialization mismatches (like the encode bug in
AmrSampleEntry).💡 Example round-trip test
#[test] fn test_samr_roundtrip() { let original = Samr { amrsampleentry: AmrSampleEntry { data_reference_index: 1, timescale: 8000, }, damr: Damr { vendor: b"erat".into(), decoder_version: 0, mode_set: 128, mode_change_period: 0, frames_per_sample: 1, }, }; let mut buf = Vec::new(); original.encode(&mut buf).expect("failed to encode"); let decoded = Samr::decode(&mut std::io::Cursor::new(&buf)) .expect("failed to decode"); assert_eq!(original, decoded); }
bradh
force-pushed
the
samr
branch
3 times, most recently
from
a0fc0ef to
a6499d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/moov/trak/mdia/minf/stbl/stsd/amr/damr.rs (1)
1-39: No inline unit tests forDamr.The
damr.rsmodule has no#[cfg(test)]block. Whilesamr.rsexercisesDamrindirectly in its decode test, a standalone round-trip test in this module (encode → decode → assert field equality) would directly guardDamr's correctness in isolation.As per coding guidelines, write unit tests inline within modules using
#[cfg(test)].🧪 Suggested inline test
+ +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_damr_roundtrip() { + let damr = Damr { + vendor: FourCC::new(b"FFMP"), + decoder_version: 0, + mode_set: 0x81FF, + mode_change_period: 1, + frames_per_sample: 1, + }; + + let mut buf = Vec::new(); + damr.encode(&mut buf).unwrap(); + let decoded = Damr::decode(&mut buf.as_slice()).unwrap(); + assert_eq!(damr, decoded); + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/moov/trak/mdia/minf/stbl/stsd/amr/damr.rs` around lines 1 - 39, Add an inline unit test under a #[cfg(test)] mod that constructs a Damr instance, round-trips it through the Atom impl by calling Damr::encode_body (or the public encode helper) into a buffer and then decoding it back via Damr::decode_body (or the public decode helper), and assert that all fields (vendor, decoder_version, mode_set, mode_change_period, frames_per_sample) match the original; name the test something like round_trip_damr and place it in the same file near the Damr definition so it exercises the Atom::encode_body and Atom::decode_body implementations directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/moov/trak/mdia/minf/stbl/stsd/mod.rs`:
- Around line 121-126: The file contains unresolved merge conflict markers
around the Codec enum and another later block; remove the conflict markers
(<<<<<<<, =======, >>>>>>>) and restore both enum variants so Codec includes
both Wvtt(Wvtt) and Samr(Samr), and likewise ensure any match arms referencing
Self::Wvtt(atom) remain valid; repeat the same resolution for the other conflict
(lines referencing the same areas around 159-163) so no variants are dropped and
the code compiles cleanly.
---
Nitpick comments:
In `@src/moov/trak/mdia/minf/stbl/stsd/amr/damr.rs`:
- Around line 1-39: Add an inline unit test under a #[cfg(test)] mod that
constructs a Damr instance, round-trips it through the Atom impl by calling
Damr::encode_body (or the public encode helper) into a buffer and then decoding
it back via Damr::decode_body (or the public decode helper), and assert that all
fields (vendor, decoder_version, mode_set, mode_change_period,
frames_per_sample) match the original; name the test something like
round_trip_damr and place it in the same file near the Damr definition so it
exercises the Atom::encode_body and Atom::decode_body implementations directly.
|
@coderabbitai: full review please |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/moov/trak/mdia/minf/stbl/stsd/mod.rs (1)
121-123: Optional: Add a descriptive comment for theSamrvariant.Every other
Codecvariant is accompanied by a brief comment identifying the codec and spec reference.Samr(Samr)is the sole exception.✏️ Suggested addition
// WebVTT, ISO/IEC 14496-30 Wvtt(Wvtt), - Samr(Samr), + // 3GPP Narrowband AMR (3GPP TS 26.244) + Samr(Samr),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/moov/trak/mdia/minf/stbl/stsd/mod.rs` around lines 121 - 123, Add a brief descriptive comment for the Samr variant in the Codec enum (next to Samr(Samr)) similar to other variants (e.g., Wvtt), identifying it as the Microsoft SAMR audio codec and optionally citing the relevant spec or RFC; update the comment style to match surrounding comments so it is consistent with the other Codec variant annotations.src/moov/trak/mdia/minf/stbl/stsd/amr/damr.rs (1)
13-39: Add dedicated unit tests forDamr.Per coding guidelines, unit tests should be written inline within modules using
#[cfg(test)].damr.rshas no tests of its own; coverage exists only transitively throughsamr.rs. An isolated test makes future regressions easier to diagnose.✅ Suggested test block
#[cfg(test)] mod tests { use super::*; use crate::coding::Decode; // AMRSpecificBox body (no outer header): vendor="erat", version=0, mode_set=0x0080, period=0, fps=1 const ENCODED_DAMR_BODY: &[u8] = &[ 0x65, 0x72, 0x61, 0x74, // vendor = "erat" 0x00, // decoder_version = 0 0x00, 0x80, // mode_set = 128 0x00, // mode_change_period = 0 0x01, // frames_per_sample = 1 ]; #[test] fn test_damr_decode() { let buf = &mut std::io::Cursor::new(ENCODED_DAMR_BODY); let damr = Damr::decode_body(buf).expect("failed to decode damr body"); assert_eq!(damr.vendor, b"erat".into()); assert_eq!(damr.decoder_version, 0); assert_eq!(damr.mode_set, 128); assert_eq!(damr.mode_change_period, 0); assert_eq!(damr.frames_per_sample, 1); } #[test] fn test_damr_encode() { let damr = Damr { vendor: b"erat".into(), decoder_version: 0, mode_set: 128, mode_change_period: 0, frames_per_sample: 1, }; let mut buf = Vec::new(); damr.encode_body(&mut buf).unwrap(); assert_eq!(buf.as_slice(), ENCODED_DAMR_BODY); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/moov/trak/mdia/minf/stbl/stsd/amr/damr.rs` around lines 13 - 39, Add an inline unit-test module under #[cfg(test)] in damr.rs that exercises Damr::decode_body and Damr::encode_body: create a constant byte slice representing the AMRSpecificBox body (vendor "erat", decoder_version=0, mode_set=0x0080, mode_change_period=0, frames_per_sample=1), write a test that decodes that slice via Damr::decode_body and asserts each field (vendor, decoder_version, mode_set, mode_change_period, frames_per_sample), and a test that constructs the same Damr struct, calls encode_body into a Vec, and asserts the produced bytes equal the constant; import crate::coding::Decode where needed and keep the tests inside mod tests with #[cfg(test)] so they run only in test builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/moov/trak/mdia/minf/stbl/stsd/amr/amrsampleentry.rs`:
- Around line 16-18: Replace the misleading "Reserved" comments with the correct
ISO 14496-12 field names: the two u16 values should be labeled channelcount (2)
and samplesize (16) instead of Reserved_2/Reserved_2, and the 4-byte block
before samplerate should be split/labeled as pre_defined + reserved (not a
single Reserved_4). Update the comments (and, if present, any helper constants
or variables used in amrsampleentry.rs) to reflect channelcount, samplesize, and
pre_defined/reserved so the encoded fields match the AudioSampleEntry spec.
---
Nitpick comments:
In `@src/moov/trak/mdia/minf/stbl/stsd/amr/damr.rs`:
- Around line 13-39: Add an inline unit-test module under #[cfg(test)] in
damr.rs that exercises Damr::decode_body and Damr::encode_body: create a
constant byte slice representing the AMRSpecificBox body (vendor "erat",
decoder_version=0, mode_set=0x0080, mode_change_period=0, frames_per_sample=1),
write a test that decodes that slice via Damr::decode_body and asserts each
field (vendor, decoder_version, mode_set, mode_change_period,
frames_per_sample), and a test that constructs the same Damr struct, calls
encode_body into a Vec, and asserts the produced bytes equal the constant;
import crate::coding::Decode where needed and keep the tests inside mod tests
with #[cfg(test)] so they run only in test builds.
In `@src/moov/trak/mdia/minf/stbl/stsd/mod.rs`:
- Around line 121-123: Add a brief descriptive comment for the Samr variant in
the Codec enum (next to Samr(Samr)) similar to other variants (e.g., Wvtt),
identifying it as the Microsoft SAMR audio codec and optionally citing the
relevant spec or RFC; update the comment style to match surrounding comments so
it is consistent with the other Codec variant annotations.
See 3GPP TS 26.244 or ETSI TS 126 244
|
@coderabbitai: can you provide objective evidence (e.g. a specification reference) that the samr codec type derives from AudioSampleEntry? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query: 💡 Result: In 3GPP TS 26.244, the AMR AudioSampleEntry for narrowband AMR uses sample entry type The decoder-specific box referenced above is defined in §6.7 as Notes from §6.7:
Here's the specification structure from §6.5: The key fields are:
This custom structure explains why your implementation uses |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/moov/trak/mdia/minf/stbl/stsd/amr/samr.rs (1)
5-8: Preferamr_sample_entryoveramrsampleentryfor idiomatic field naming.The field name
amrsampleentryis a concatenation of three words without underscores. Rust convention uses underscores to separate words in compound identifiers (amr_sample_entry). Since this is a public field on a new type, renaming before merge is cheap and avoids a future breaking change.♻️ Proposed rename
pub struct Samr { - pub amrsampleentry: AmrSampleEntry, + pub amr_sample_entry: AmrSampleEntry, pub damr: Damr, }The same rename would need to propagate to the
decode_bodyconstructor andencode_body:Ok(Self { - amrsampleentry, + amr_sample_entry: amrsampleentry, damr: damr.ok_or(Error::MissingBox(Damr::KIND))?, })- self.amrsampleentry.encode(buf)?; + self.amr_sample_entry.encode(buf)?;And the two test structs (lines 57 and 75).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/moov/trak/mdia/minf/stbl/stsd/amr/samr.rs` around lines 5 - 8, The public field name in struct Samr is non-idiomatic: rename the field amrsampleentry to amr_sample_entry in the Samr struct, and propagate that change to all uses — update the constructor in decode_body, the encoder in encode_body, and the two test structs referenced (replace amrsampleentry -> amr_sample_entry) so references compile; ensure any pattern matches, struct construction, and field accesses use the new amr_sample_entry identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/moov/trak/mdia/minf/stbl/stsd/amr/samr.rs`:
- Around line 5-8: The public field name in struct Samr is non-idiomatic: rename
the field amrsampleentry to amr_sample_entry in the Samr struct, and propagate
that change to all uses — update the constructor in decode_body, the encoder in
encode_body, and the two test structs referenced (replace amrsampleentry ->
amr_sample_entry) so references compile; ensure any pattern matches, struct
construction, and field accesses use the new amr_sample_entry identifier.
1 participant
See 3GPP TS 26.244 or ETSI TS 126 244
Resolves #134