Skip to content

Ignore stco chunks with no samples to fill them - #455

Open
kinetiknz wants to merge 2 commits into
masterfrom
surplus-stco-entries
Open

Ignore stco chunks with no samples to fill them#455
kinetiknz wants to merge 2 commits into
masterfrom
surplus-stco-entries

Conversation

@kinetiknz

Copy link
Copy Markdown
Collaborator

Fixes bug 2026607, where videos on etoland.co.kr fail in Firefox but play in Chrome and Safari.

The file

The reported MP4 has one video track with an inconsistent sample table:

boxcontents
stszsample_size=0, sample_count=112, sizes summing to 529972
stscone entry: first_chunk=1, samples_per_chunk=112
stcoentry_count=2: 0x90E (the mdat payload start) and 0xC000009D (3GB past the 532290-byte file)

The 112 sample sizes exactly fill the mdat, so the second chunk offset is garbage that no sample maps into.

The failure

mp4parse_new succeeds; mp4parse_get_indice_table returns Invalid. SampleToChunkIterator extends the last stsc entry across every chunk stco declares, yielding 2 × 112 = 224 samples. At sample 113 the stsz size iterator is exhausted, end_offset falls through to 0, and create_sample_table returns None, rejecting the whole track. Gecko surfaces that as NS_ERROR_DOM_MEDIA_METADATA_ERR.

The fix

Treat stsz as authoritative for the sample count and stop once every sample is placed. This is what ffmpeg's mov_build_index() does — it iterates chunks driven by stco just like we do, but bounds the inner loop by the stsz sample count and keeps the entries already built:

for (j=0; j<sc->stsc_data[stsc_index].count; j++) {
if (current_sample >= sc->sample_count) {
av_log(mov->fc, AV_LOG_ERROR, "wrong sample count\n");
return; /* stop, but keep the index built so far */
}

sc->sample_count is assigned in mov_read_stsz before the if (sample_size) return 0; early-out, so it bounds both the size-table and constant-sample-size cases. SampleSizeBox now retains sample_count for the same reason: it was previously read and discarded when sample_size != 0.

This also fixes a quieter variant. With a constant sample size there is no size table to run out of, so the surplus chunk produced a phantom sample at the bogus offset instead of an error — confirmed on a synthetic file, where the old code emitted a second indice ending at 3221226371.

ffmpeg comparison

nb_read_frames for ffmpeg, indice-table result for mp4parse:

caseffmpegbeforeafter
A · the reported file112 + wrong sample countInvalid112 ✅
B · synthetic surplus stco3 + wrong sample countInvalid3 ✅
B0 · B's unmodified parent333
C · const-size surplus stco1 + wrong sample count2 (phantom @3221226371)1 ✅
C0 · C's unmodified parent111
D · chunk_out_of_range.mp4fatalInvalid ✅Invalid ✅
D0 · D with its flipped bit repaired171717
E · too few chunks (mirror case)222
F · stsc mid-table overshootfatalInvalid ✅Invalid ✅
G · zero-sample trackopens, empty trackInvalidOk, 0 samples ✅

We diverged from ffmpeg in four rows before; we now match in all ten.

What stays strict

Tables inconsistent in the other direction — stsc naming a chunk stco doesn't have — remain an error. ffmpeg draws the same line in sanity_checks(), and deliberately distinguishes the two failures:

ret=sanity_checks(c->fc, sc, st->index);
if (ret)
returnret>1 ? AVERROR_INVALIDDATA : 0;

stsc_data[stsc_count - 1].first > chunk_count returns 2 and fails the whole header (cases D and F); a chunk-bearing track with no samples returns 1 and is kept as an empty track (case G).

The second commit documents this on test_chunk_out_of_range.rs. That test arrived in 7563263 as a regression test against indexing stco out of bounds, which used to panic before it was changed to use get() — the Invalid assertion was just what not panicking happened to produce, so it was worth recording that the strictness is now deliberate. That file's only defect is a single flipped bit: first_chunk is 16777217 (0x0100_0001) rather than 1. Repairing that bit makes it decode 17 frames everywhere (row D0).

Behaviour change beyond the reported case

Row G: a track with stsz sample_count=0 but a non-empty stco now yields an empty sample table instead of Invalid. This moves us toward ffmpeg, and Gecko already receives empty indice tables from fragmented init segments, so the shape is not new.

Testing

New fixture stco_extra_chunk.mp4 (1670 bytes), derived from the existing video_colr_nclx_hdr10.mp4 by adding one bogus stco entry — it has the same single-chunk stsc/stsz shape as the reported file. It returns Invalid without the fix and the same 3-sample table as its unmodified parent with it.

211 tests pass; cargo fmt --check and cargo clippy --all-targets clean.

🤖 Generated with Claude Code

Some files declare more chunks in 'stco' than the samples in 'stsz' can
fill, and the surplus chunk offsets are bogus. Treat 'stsz' as
authoritative for the sample count and stop once every sample is placed,
rather than rejecting the track. Matches ffmpeg's mov_build_index().
Fixes bug 2026607.
@kinetiknzkinetiknz self-assigned this Aug 26, 2026
@kinetiknz

Copy link
Copy Markdown
CollaboratorAuthor

@alastor0325 Would you mind reviewing this please?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@kinetiknz