fix(mp4): only read a sound sample description from an audio track - #2692
Open
dangrier wants to merge 1 commit into
Open
fix(mp4): only read a sound sample description from an audio track#2692dangrier wants to merge 1 commit into
dangrier wants to merge 1 commit into
Conversation
Sample entries in a sample description box are track-type specific, selected by the handler type of the enclosing track (ISO/IEC 14496-12, 8.5.2): 'soun' holds an AudioSampleEntry, 'vide' a VisualSampleEntry and 'meta' a MetaDataSampleEntry. Every entry was read as a sound sample description regardless of handler type. A MetaDataSampleEntry may be no larger than the 16-byte SampleEntry base, whereas a sound sample description needs 20 bytes beyond it, so reading one threw: RangeError: Offset is outside the bounds of the DataView for a declared entry size of 17 to 35 bytes, in either SoundSampleDescriptionVersion (needs 8 bytes) or SoundSampleDescriptionV0 (a further 12 at offset 8). This completes the partial fix in Borewit#2340, which handled only an absent description. Larger non-audio entries did not throw, but were still misread: a video track was reported as an audio stream, and could be selected as the track supplying format.sampleRate, bitsPerSample and numberOfChannels. The expectation for 'Mr. Pickles S02E07 My Dear Boy.mp4' recorded that behaviour, asserting the avc1 track was audio with 0 channels and a sampling frequency of 1916.1076 Hz, and is updated accordingly. Audio properties are now derived only from a track whose handler is 'soun'. The handler is resolved during post-processing, so no box order is assumed; readers are required to accept any order. Where no handler box is present, the previous sample-description heuristic still applies. The data format of every entry is still reported, so codec names for non-audio tracks are unaffected. Both reads are additionally bounds-checked against the description length, so a truncated audio entry is skipped rather than throwing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
dangrier
commented
Aug 3, 2026
Comment on lines
-259
to
-265
| audio: { | ||
| bitDepth: 0, | ||
| channels: 0, | ||
| samplingFrequency: 1916.1076 | ||
| }, | ||
| codecName: '<avc1>', | ||
| type: TrackType.audio |
Author
There was a problem hiding this comment.
This test fixture actually demonstrates the bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #2690.
Fixes the
RangeError: Offset is outside the bounds of the DataViewthrown while parsing an MP4stsdbox, and the type confusion underneath it.Independent of #2693, which fixes a separate defect in the same box; either can merge first.
The defect
Sample entries in a sample description box are track-type specific, selected by the handler type of the enclosing track. ISO/IEC 14496-12, 8.5.2 states this as a literal switch:
MP4Parserimplements no such switch — it appliedparseSoundSampleDescriptionto every entry unconditionally. Two consequences follow.1. A crash on short entries. A
MetaDataSampleEntryisextends SampleEntry (codingname) { }— an empty body, so it may be no larger than the 16-byteSampleEntrybase. A sound sample description needs 20 bytes beyond that base (SoundSampleDescriptionVersion.len8 +SoundSampleDescriptionV0.len12), so reading one overruns:descrLenmasterSoundSampleDescriptionVersion(needs 8)SoundSampleDescriptionV0(12 bytes at offset 8 → needs 20)That is a declared entry size of 17–35 bytes, with two distinct overrun points. #2338 reported this crash and #2340 fixed only the empty-description case — its test is named "Handle empty sample entry description". It reproduces on ordinary DJI drone footage, which writes
djmd(telemetry) anddbgi(debug) metadata tracks with 20-byte sample entries.2. Silent misclassification of larger entries. Entries of 36 bytes or more never threw, but were still decoded as audio. This is visible in this repository's own corpus —
test/samples/mp4/Mr. Pickles S02E07 My Dear Boy.mp4has anavc1video track thatmasterreports as an audio stream:1916.1076 Hzis not a sampling frequency — it is the video's resolution.SoundSampleDescriptionV0computessampleRateasUINT16_BE(off + 8) + UINT16_BE(off + 10) / 10000, which lands on entry offsets 32 and 34. In aVisualSampleEntrythose arewidthandheight. That track is 1916 × 1076, giving1916 + 1076/10000.The overlay also explains why the parser did not bail out: entry offset 16 is
pre_defined = 0in aVisualSampleEntry, which decodes asversion = 0and satisfiesversion === 0 || version === 1. Offsets 24 and 26, read as channel count and sample size, fall insideunsigned int(32)[3] pre_defined = 0, hence the0ch 0bit.The change
Audio properties are now derived only from a track whose handler is
soun(oraudi).hdlrsits inmdiawhilestsdis deeper inminf > stbl— gating at parse time would have relied on an ordering the specification does not guarantee.dataFormatcontinues to supplytrackInfo.codecNameandformat.codecfor non-audio tracks. Skipping non-audio tracks outright would have regressed codec reporting.hdlris present, the previous sample-description heuristic still applies, so those files behave as before.debug()warning rather than throwing.Tests
test/test-file-mp4.tsgains aSample Description (stsd) atomsuite that builds its fixtures in memory rather than adding binary samples, as the existingTrackHeaderAtomtests do.Metadata entries of 16, 18, 24, 34 and 36 bytes all parse; a metadata track contributes no audio properties even when its bytes are filled with plausible audio values; a
sountrack still yields them; a non-audio track still reports its data format; and atrakwithhdlrdeclared afterminfbehaves identically, covering the box-order requirement.The five cases covering the defect were confirmed to fail against
masterbefore the fix; the remaining four passed throughout as controls, showing the fixtures themselves are valid.test/test-trackinfo.tsis amended: the expectation forMr. Pickles S02E07 My Dear Boy.mp4asserted theavc1track wasTrackType.audiowith 0 channels and 1916.1076 Hz. That expectation recorded the defect, so it is updated to expect a codec name only.Verification
yarn test— 595 passing, 1 pending, 0 failing.yarn typecheckclean.yarn lint:tsexits 0 (7 warnings, all pre-existing onmaster, none in the changed files).Not run:
yarn buildandyarn lint:md.