mp4a: find esds inside the QuickTime wave box#170
Conversation
WalkthroughUpdated 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mp4a/mod.rs`:
- Around line 134-198: In test_mp4a_quicktime_wave, move audio_bytes directly
into mp4a_body instead of cloning it. Also extend the wave fixture with a btrt
child and assert the decoded result preserves the expected bitrate data,
covering decode_wave’s btrt handling.
🪄 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: bfb205fa-6378-42ed-b8cb-ad7562b048d2
📒 Files selected for processing (1)
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
11ad4b7 to
2547b0e
Compare
FFmpeg and camera muxers (MotionCam, etc.) emit AAC-in-MOV with the esds nested inside a QuickTime `wave` (siDecompressionParam) box rather than as a direct child of `mp4a`. The decode loop already claimed to "find esds in mp4a or wave" but only ever saw `wave` as an unknown atom and skipped it, so Mp4a::decode_body returned MissingBox(esds) and the whole moov decode failed. Descend into the wave box, scanning its children by header for esds/btrt only. Other children (frma, a nested mp4a terminator stub, the trailing zero box) are skipped by size and never typed-decoded — the nested mp4a stub is 4 bytes and is not a valid audio sample entry.
2547b0e to
d073717
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mp4a/mod.rs`:
- Around line 68-85: Update the size guard in decode_wave so only child sizes
greater than zero and within wave.remaining() are accepted. Treat Some(0) like
other invalid or over-long sizes by breaking the scan, ensuring
wave.advance(size) always makes progress.
🪄 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: 2bf5cc98-1306-46d0-be12-31e7ca41de8f
📒 Files selected for processing (1)
src/moov/trak/mdia/minf/stbl/stsd/mp4a/mod.rs
Problem
Mp4a::decode_bodyhas a comment saying "Find esds in mp4a or wave", but it never actually descends into the QuickTimewavebox — it only iterates the direct children ofmp4a. When theesdsis nested inside awave(siDecompressionParam) box, the loop seeswaveas an unknown atom, skips it viadecode_unknown, and then returnsError::MissingBox(esds), failing the wholemoovdecode.This is a very common layout: FFmpeg's
movmuxer and many camera apps (e.g. MotionCam) emit AAC-in-MOV with theesdsinsidewave:Such files currently fail to parse entirely with
missing box: esds.Fix
Descend into the
wavebox and scan its children by header foresds(andbtrt), which is what the existing comment already promised. Every other child —frma, the nestedmp4aterminator stub, the trailing zero box — is skipped by size and deliberately not typed-decoded: the nestedmp4ais a 4-byte stub that is not a valid audio sample entry, so decoding it as one would error.The change is additive and does not affect the ISO-BMFF path (esds as a direct child of
mp4a). Re-encoding normalises to the ISO layout, as before.Test
Adds
test_mp4a_quicktime_wave, which builds the QuickTimemp4a → wave → { frma, mp4a-stub, esds, terminator }layout and assertsMp4a::decoderecovers theesds. Full suite passes (227 tests).