feat(mp4): defragment hybrid files with a progressive part - #561
feat(mp4): defragment hybrid files with a progressive part#561nchitkara-xai wants to merge 2 commits into
Conversation
9dcc61d to
addadb3
Compare
b603735 to
262d9e0
Compare
A fragment whose tfdt re-declares an earlier decode time supersedes the earlier samples of its track (a retransmission): the fragment appearing later in the file wins, whole re-sent fragments disappear, superseded tails are trimmed at sample granularity, and the timeline continues from the declaring fragment, whether or not the re-sent bytes are identical. Non-overlapping input converts byte-identically to before. Ambiguous overlaps keep failing closed instead of being guessed at: an overlap that starts inside a sample (unless it only shrinks earlier tfdt-gap padding), any abandoned time range that no surviving later fragment declares again (whether the superseded fragment was trimmed or dropped whole, and with voided declarations not counting as coverage), and overlapping files whose fragments use absolute base data offsets are rejected, so no declared content is ever silently dropped. The payload size bound applies to the surviving samples, so re-sent declarations larger than the input file do not reject a file that resolves cleanly. The work is linear in the number of fragments. Coverage checking merges each track's surviving declaration windows once and binary searches them. dropTrackSamplesFrom walked track.frags backwards until a record ended at or below the cut; records voided by earlier cuts sit above later live records in collection order and never satisfy that test once the cut walks below them, so each later cut rescanned the whole voided tail: n one-sample fragments followed by n-1 fragments each rewinding one sample took O(n^2). A parallel live slice keeps the records with kept > 0; their sample ranges tile the samples in order, so a cut pops the suffix with firstSample >= keep and trims at most the record straddling keep. The firstSample+kept <= keep guard stays so a cut landing exactly on a record boundary inside gap padding does not advance its cutoff. Gap padding is tracked as a stack indexed by sample so truncation pops instead of scanning. Each record and padding entry leaves once, making every cut amortized O(1).
A fragmented file may carry progressive samples in its moov sample tables before the first fragment. These samples now come first in the converted output with their chunk structure preserved, and the fragment samples are appended to the same tracks. Per-track chunk order follows the sample tables even when stco offsets are not monotone, and the tracks' chunk runs are interleaved by their input file position. Hostile sample tables are validated before any count-proportional allocation, and the progressive payload counts toward the total payload bound. The progressive part takes part in overlap resolution like a fragment: a later fragment that re-declares its decode times supersedes the progressive samples under the same later-declarer-wins, sample-boundary, and coverage rules, so no declared content is ever silently dropped. A track with a progressive prefix starts at decode time zero, anchors the empty-edit alignment of later-starting fragment-only tracks, and gets its edit list validated like any other track. The sample table expansion lives on the box types as SttsBox.SampleDurations, CttsBox.CompositionTimeOffsets, and StssBox.SampleIsSync, each returning one value per sample after validating the entries against the declared sample count.
262d9e0 to
1717dd3
Compare
|
I've never heard about this strange hybrid case, but it is of course possible to generate something like that. There are a few holes that I'll let Claude comment on. |
There was a problem hiding this comment.
Reviewed the second commit only, rebased onto the current master (eac43c8, #560 merged this morning).
Merge state
GitHub reports CONFLICTING and git merge-tree agrees, but that is the stacked-PR artifact: this PR's first commit has the same tree as eac43c8 with a different SHA, so cherry-picking the hybrid commit onto master applies cleanly. Nothing to do beyond a rebase.
Validation
go test ./...,go vet,gofmtclean.- All 12 convertible files under
testdata/convert byte-identically to the pre-#561 binary; error messages match on the rest. - Real media end to end: built a hybrid from
init.mp4+1.m4s, defragmented it, then re-declared its 60 samples as an appended fragment. Converts to 120 frames / 4.0 s andffmpeg -f null -decodes with zero errors. - A fragment fully superseding the progressive part (same samples re-declared from decode time 0) produces output byte-identical to the pure progressive conversion.
The mechanism looks right — per-track sample-table chunk order, the cross-track merge by srcOffset, the firstSample/start zero values for the progressive defragFragRec, and the interaction with dropTrackSamplesFrom/survivingWindows all check out. Two findings below, both reproduced with probe tests.
1. A missing stsz silently drops the progressive samples
Line 290 treats stbl.Stsz == nil as "this track has no progressive part":
if stbl == nil || stbl.Stsz == nil || stbl.Stsz.GetNrSamples() == 0 {
return nil, nil
}But stsz.go documents that the stz2 variant is not supported, so an stz2-coded progressive part decodes with stbl.Stsz == nil while stts/stsc/stco still describe the chunks. Those samples are then dropped with no error.
Reproduced with a track carrying 2 progressive + 1 fragment sample and no stsz in the tree: output has 1 sample and mdhd.Duration 512 instead of 3 samples and 1536.
The check right below it — erroring when stsz declares samples but a sibling table is missing (line 305) — covers exactly this class of disagreement in the other direction; only this direction is unguarded.
Suggested fix, in progressiveChunks:
func progressiveChunks(track *defragTrack, fileSize uint64) ([]*defragChunk, error) {
stbl := trackStbl(track.trak)
- if stbl == nil || stbl.Stsz == nil || stbl.Stsz.GetNrSamples() == 0 {
+ if stbl == nil {
+ return nil, nil
+ }
+ if stbl.Stsz == nil || stbl.Stsz.GetNrSamples() == 0 {
+ // stsz carries the only sample count mp4ff reads (stz2 is not
+ // supported), so chunks declared without it would be dropped silently.
+ nrChunks := 0
+ if stbl.Stco != nil {
+ nrChunks = len(stbl.Stco.ChunkOffset)
+ }
+ if stbl.Co64 != nil {
+ nrChunks += len(stbl.Co64.ChunkOffset)
+ }
+ if nrChunks > 0 {
+ return nil, fmt.Errorf("%d chunks are declared but stsz declares no samples (stz2 is not supported)", nrChunks)
+ }
return nil, nil
}
nrSamples := stbl.Stsz.GetNrSamples()With this the suite still passes, gofmt is clean, all 12 testdata files still convert byte-identically, and the real-media hybrid output is unchanged.
2. An encrypted progressive part loses its saiz/saio
collectTraf rejects senc/saiz/saio in a traf, but progressiveChunks never checks stbl.Saiz/stbl.Saio — and fillSampleTables nils both (lines 1125-1126). A hybrid track whose progressive part carries CENC auxiliary information therefore converts with exit status 0 into a file whose stsd still declares encv/enca + sinf while the per-sample IVs and subsample ranges are gone: undecryptable output, no error.
Reproduced by adding saiz/saio to the video stbl of a hybrid file whose appended fragment belongs to the other track (so the traf check never fires): conversion succeeds, output has saiz=<nil> saio=<nil>.
But saiz/saio is the wrong signal for "is this encrypted", and that is a pre-existing question
The authoritative signal is a sinf in the sample entry (encv/enca/encs/enct/encm, with schm naming cenc/cbcs/cens/cbc1). senc/saiz/saio only say "per-sample auxiliary data exists". So the traf check on master is not an encryption test either — both are really "would I silently drop per-sample aux data?" tests.
That distinction has teeth, because full-sample encryption with a constant IV has no senc, saiz, or saio at all. mp4ff says so itself, in DecryptFragmentWithKeys:
A missing senc is fine for full-sample encryption with a constant IV (no sample auxiliary information); CMAF (ISO/IEC 23000-19 Section 8.2.2.1) recommends omitting senc, saiz, and saio in that case.
I built that shape — InitProtect(..., "cbcs", ...) on an AAC track, so tenc.DefaultPerSampleIVSize=0 with a 16-byte DefaultConstantIV, plus plain fragments carrying no senc/saiz/saio — and it defragments with err=<nil> on master as well as on this branch. So the Defragment doc comment's claim that encrypted content is rejected is already inaccurate today; #561 does not introduce that.
Worth noting that this particular conversion is lossless: the output keeps enca + sinf + schm + tenc, the sample data is copied verbatim, and there was no per-sample aux data to lose, so the result is still correctly encrypted and decryptable.
So there is a contract decision here, and it is yours rather than mine:
- A — reject protected tracks outright. One check per kept track in
newDefragmenteron the stsd sample entry (sinf != nil, or a type inencv/enca/encs/enct/encm). Spec-anchored, and it subsumes both the traf check and any progressivesaiz/saiocheck. Cost: constant-IV files that convert correctly today start being refused. - B — keep the "reject only when something would be lost" rule, adding
if stbl.Saiz != nil || stbl.Saio != niltoprogressiveChunksto mirror the traf check, and reword theDefragmentdoc comment to say per-sample encryption auxiliary information rather than encrypted content. Constant-IV pass-through then becomes documented behaviour, and it genuinely works.
I would lean B plus the doc fix, since A gives up working functionality to satisfy a sentence in a comment. One argument for adding the stsd check even under B: SencBox is modelled only on TrafBox, never on StblBox, so a stbl-level senc decodes into stbl.Children unnamed and fillSampleTables drops it when it rebuilds that list from a fixed set. In practice CENC pairs senc with saiz/saio so the saiz/saio check catches it, but only indirectly.
Nit
SttsBox.SampleDurations and CttsBox.CompositionTimeOffsets panic on a hand-built box whose parallel slices disagree — CompositionTimeOffsets loops to NrSampleCount() (len(SampleOffset)) while indexing EndSampleNr[i+1], so e.g. CttsBox{EndSampleNr: []uint32{0, 3}, SampleOffset: []int32{100, 200}} panics with an index-out-of-range. Not reachable from a decoded box, and GetDecodeTime and CttsBox.SampleCount already behave the same way, so this is house style rather than a regression — but these are new exported API, so bounding the loop by len(b.EndSampleNr)-1 would be cheap.
2 participants
Stacked on #560 (the first commit is #560); please review the last commit only.
Problem
Hybrid files carry a progressive part in the moov sample tables followed by
fragments (a recorder that finalizes periodically, or a progressive file
extended by appending fragments). #557 rejects them; converting requires
dropping the hybrid track via track selection.
Fix
The moov sample tables are expanded into the same sample list the fragments
feed (stts/ctts/stsc/stsz/stss/stco/co64, with hostile-table guards so
declared counts a file cannot physically hold error out before any
count-proportional allocation). Per-track chunk order follows the sample
tables unconditionally; across tracks, chunk runs are interleaved by input
file position, so non-monotonic chunk offsets (legal, if unusual) cannot
corrupt the sample-to-bytes mapping.
The progressive part counts as each track's earliest declaration and flows
through the overlap machinery from #560 unchanged: a later fragment may
supersede progressive samples under the same later-declarer-wins and
no-silently-dropped-ranges rules, and mid-sample cuts reject. A hybrid track
starts at decode time 0 by construction and anchors the cross-track
alignment of #557 for later-starting fragment-only tracks. Purely
progressive input (no fragments) still rejects as not fragmented.
Tests
Hybrid round-trips with multi-entry stsc, sync-sample subsets, and negative
composition offsets; non-monotonic chunk offsets round-trip byte-correct;
fragments superseding progressive samples (covered resolves, uncovered and
mid-sample reject); hybrid track anchoring a fragment-only track's empty
edit; hostile stts/stsc/stsz/ctts tables. All fragmented testdata files
convert byte-identically to #560 output.
go test ./...,go vet,gofmt,golangci-lintpass.