Add mp4v sample entry and decode QuickTime v2 sound descriptions - #176
Add mp4v sample entry and decode QuickTime v2 sound descriptions#176AdrianEddy wants to merge 2 commits into
Conversation
WalkthroughThe changes add QuickTime version-2 sound description support through 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/moov/trak/mdia/minf/stbl/stsd/pcm.rs (1)
172-195: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale docstring:
lpcmformat resolution now does consult version-2 flags.The doc comment for
Pcm::format()still says version-2 flags forlpcm"are not decoded here; defaults to big-endian integer,sample_sizebits" (lines 181-183), butresolve_format(below, lines 218-226) now resolves the real format fromSoundV2.format_flags/bits_per_channelwheneversound_v2isSome. This doc block wasn't updated to reflect the behavior change in this PR and will mislead API consumers.📝 Proposed doc fix
- /// - `lpcm`: QTFF format flags are only defined for version 2 sound sample - /// descriptions, which are not decoded here; defaults to big-endian - /// integer, `sample_size` bits + /// - `lpcm`: when a version-2 `SoundV2` extension is present, the real + /// storage format is resolved from its `formatSpecificFlags`; otherwise + /// defaults to big-endian integer, `sample_size` bits🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moov/trak/mdia/minf/stbl/stsd/pcm.rs` around lines 172 - 195, Update the Pcm::format documentation to state that lpcm entries use SoundV2.format_flags and bits_per_channel when sound_v2 is present, rather than claiming version-2 flags are not decoded and defaulting to big-endian integer. Keep the existing fallback description for lpcm entries without usable version-2 metadata.
🧹 Nitpick comments (3)
src/moov/trak/mdia/minf/stbl/stsd/mp4v.rs (1)
34-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
Some(atom)overatom.into()for clarity.Using
atom.into()relies on the standard library'simpl<T> From<T> for Option<T>, which obscures the fact that you are simply wrapping the parsed atom in aSomevariant. Explicitly usingSome(atom)is more idiomatic and easier to read.♻️ Proposed refactor
- Any::Esds(atom) => esds = atom.into(), - Any::Btrt(atom) => btrt = atom.into(), - Any::Colr(atom) => colr = atom.into(), - Any::Pasp(atom) => pasp = atom.into(), - Any::Fiel(atom) => fiel = atom.into(), + Any::Esds(atom) => esds = Some(atom), + Any::Btrt(atom) => btrt = Some(atom), + Any::Colr(atom) => colr = Some(atom), + Any::Pasp(atom) => pasp = Some(atom), + Any::Fiel(atom) => fiel = Some(atom),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moov/trak/mdia/minf/stbl/stsd/mp4v.rs` around lines 34 - 38, Replace the atom.into() assignments in the Any::Esds, Any::Btrt, Any::Colr, Any::Pasp, and Any::Fiel match arms with explicit Some(atom) wrapping, preserving the existing field assignments.src/moov/trak/mdia/minf/stbl/stsd/audio.rs (2)
54-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for the version-1 skip branch.
decode_with_v2now explicitly matches QuickTime sound version 1 and skips its 16-byte extension, but no test in this file (orpcm.rs) exercises this path.test_pcm_v1_chnlinpcm.rsonly coverschnl-atom versioning, not this audio sound-version-1 branch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moov/trak/mdia/minf/stbl/stsd/audio.rs` around lines 54 - 59, Add a focused test in the audio sample-description tests covering decode_with_v2 with sound version 1, supplying the 16-byte extension and asserting the decoder skips it and returns the expected result. Keep test_pcm_v1_chnl unchanged, since it covers a different versioning path.
60-77: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
constBytesPerAudioPacket/constLPCMFramesPerAudioPacketare discarded on decode and re-derived on encode, not round-tripped.
decode_with_v2reads but dropsconstBytesPerAudioPacketandconstLPCMFramesPerAudioPacket(they aren't stored inSoundV2), andencode_with_v2recomputes the former fromchannel_count * (bits_per_channel / 8)(line 114) and hardcodes the latter to1(line 115). This is safe today because the only consumer (Lpcm, uncompressed PCM) always hasframesPerPacket == 1and integer-byte-aligned samples, butAudio::decode_with_v2/encode_with_v2are generic, publicly-exposed APIs — a future caller using them for a compressed codec's version-2 sound description, or a bit depth not divisible by 8, would silently get corrupted values on a decode→encode round trip. Worth a doc note on these methods spelling out this uncompressed-PCM assumption, so future reuse doesn't hit a silent data-fidelity bug.Also applies to: 107-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/moov/trak/mdia/minf/stbl/stsd/audio.rs` around lines 60 - 77, Document the uncompressed-PCM assumption on the public Audio::decode_with_v2 and Audio::encode_with_v2 methods: the version-2 fields constBytesPerAudioPacket and constLPCMFramesPerAudioPacket are discarded during decoding and reconstructed during encoding, so decode→encode preserves values only for integer-byte-aligned PCM with one frame per packet. Make the note explicit for future callers without changing the current encoding behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/moov/trak/mdia/minf/stbl/stsd/mp4v.rs`:
- Around line 67-90: Update sample_esds() so the visual MPEG-4 configuration
does not use esds::DecoderSpecific, which is limited to the two-byte audio
AudioSpecificConfig format. Add or reuse raw-byte or MPEG-4 Visual-specific
decoder configuration support in the relevant ESDS types, then preserve the
complete decoder payload (including VOL data) during round trips and construct
the helper with that representation.
---
Outside diff comments:
In `@src/moov/trak/mdia/minf/stbl/stsd/pcm.rs`:
- Around line 172-195: Update the Pcm::format documentation to state that lpcm
entries use SoundV2.format_flags and bits_per_channel when sound_v2 is present,
rather than claiming version-2 flags are not decoded and defaulting to
big-endian integer. Keep the existing fallback description for lpcm entries
without usable version-2 metadata.
---
Nitpick comments:
In `@src/moov/trak/mdia/minf/stbl/stsd/audio.rs`:
- Around line 54-59: Add a focused test in the audio sample-description tests
covering decode_with_v2 with sound version 1, supplying the 16-byte extension
and asserting the decoder skips it and returns the expected result. Keep
test_pcm_v1_chnl unchanged, since it covers a different versioning path.
- Around line 60-77: Document the uncompressed-PCM assumption on the public
Audio::decode_with_v2 and Audio::encode_with_v2 methods: the version-2 fields
constBytesPerAudioPacket and constLPCMFramesPerAudioPacket are discarded during
decoding and reconstructed during encoding, so decode→encode preserves values
only for integer-byte-aligned PCM with one frame per packet. Make the note
explicit for future callers without changing the current encoding behavior.
In `@src/moov/trak/mdia/minf/stbl/stsd/mp4v.rs`:
- Around line 34-38: Replace the atom.into() assignments in the Any::Esds,
Any::Btrt, Any::Colr, Any::Pasp, and Any::Fiel match arms with explicit
Some(atom) wrapping, preserving the existing field assignments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a29bc2e4-63df-44e9-93d6-4a3e5e66a272
📒 Files selected for processing (5)
src/any.rssrc/moov/trak/mdia/minf/stbl/stsd/audio.rssrc/moov/trak/mdia/minf/stbl/stsd/mod.rssrc/moov/trak/mdia/minf/stbl/stsd/mp4v.rssrc/moov/trak/mdia/minf/stbl/stsd/pcm.rs
afc45ee to
34f6eda
Compare
…erate trailing bytes Three robustness fixes for MPEG-4 (ISO/IEC 14496-1) descriptor parsing in `esds`, all found on real-world files the strict decoder mishandled. 1. Preserve the DecoderSpecificInfo payload. `DecoderSpecific` parsed the first two bytes as the AAC AudioSpecificConfig (audioObjectType / samplingFrequencyIndex / channelConfiguration) and DISCARDED the rest — so a longer AAC config (e.g. a GASpecificConfig extension) or a non-AAC config (the MPEG-4 Visual VOS/VOL headers an `mp4v` `esds` carries) did not round-trip. `DecoderSpecific` now keeps the complete payload in `raw`; the AAC fields remain a parsed view of its prefix, and encode emits `raw` verbatim (or re-derives the 2-byte config from the fields when `raw` is empty, for a hand-constructed value). `DecoderSpecific` / `DecoderConfig` / `EsDescriptor` consequently lose `Copy` (the payload is a `Vec<u8>`). 2. DecoderSpecificInfo (tag 0x05) is optional. ISO/IEC 14496-1 defines it as "if available" — a stream whose config is derivable in-band (e.g. AAC carried with ADTS headers) legitimately omits it. The decoder treated it as mandatory (`MissingDescriptor(0x05)`) and failed the whole `esds`. `dec_specific` becomes an `Option`; a `None` round-trips to no tag-5 descriptor. 3. Tolerate trailing bytes within a descriptor's declared size. Descriptors are length-prefixed and forward-compatible: the size field is authoritative and a parser skips to the declared end. `Descriptor::decode` used `decode_exact`, which `ShortRead`s on unconsumed bytes — so a 2-byte `SLConfigDescriptor` failed the whole `esds` with `UnderDecode(esds)`. Each typed descriptor is now decoded within a `size`-bounded slice and advanced to the declared end. All three add regression tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…escriptions Two additions that broaden real-world sample-entry coverage. Builds on the `esds` payload-preservation change so the `mp4v` decoder configuration round-trips faithfully. 1. `mp4v` — MPEG-4 Part 2 Visual (ISO/IEC 14496-2) sample entry. A VisualSampleEntry carrying its codec configuration in an MPEG-4 `esds` (the same descriptor container as `mp4a`, with a visual object-type indication). Common in older MP4/3GP files (DivX/Xvid, MPEG-4 SP/ASP). Registered in the `Any` table and the `Codec` enum; optional `pasp`/`colr`/`btrt`/`fiel` children are carried like the other visual entries. The MPEG-4 Visual VOL config in the `esds` is preserved verbatim (via `DecoderSpecific::raw`). 2. QuickTime version-2 sound sample descriptions. `Audio::decode` read the version-1/2 sound-description extensions past the base fields but discarded them — so a `lpcm` entry (which QTFF defines ONLY as a version-2 description) lost its real sample rate, channel count, and `formatSpecificFlags`. `Audio` now surfaces the version-2 fields as `SoundV2`, exposed on the `Pcm` helper and a hand-written `Lpcm` entry, so `Lpcm::format()` resolves the true storage format from the flags. Round-trips through the version-2 encoding. Both additions include tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
34f6eda to
8b33645
Compare
|
Will look at this one once #174 lands. It would help to not have two changes in the same PR though. |
2 participants
Two additions that broaden real-world sample-entry coverage.
1.
mp4v— MPEG-4 Part 2 Visual (ISO/IEC 14496-2)A new
VisualSampleEntrywhose codec configuration rides in an MPEG-4esds(the same descriptor container asmp4a, here with a visual object-type indication). Common in older MP4/3GP files (DivX/Xvid, MPEG-4 SP/ASP), where it previously decoded asCodec::Unknown.Registered in the
Anytable and theCodecenum; optionalpasp/colr/btrt/fielchildren are carried like the other visual entries. The MPEG-4 Visual VOS/VOL config in theesdsis preserved verbatim throughDecoderSpecific::raw(from #174), so the entry round-trips faithfully.2. Decode QuickTime version-2 sound sample descriptions
Audio::decoderead the version-1/2 sound-description extensions past the base fields but discarded them. QTFF defines thelpcmfourcc only inside a version-2 description, so anlpcmentry lost its real sample rate, channel count, andformatSpecificFlags(andLpcm::format()could only return a fourcc-implied guess).Audionow surfaces the version-2 fields as aSoundV2(real rate / channels / bits-per-channel / format flags), exposed on thePcmhelper and a hand-writtenLpcmentry, soLpcm::format()resolves the true storage format — float vs integer, endianness — from the flags. A version-2 entry round-trips through encode.Both additions include tests.
🤖 Generated with Claude Code