Repository navigation
fix(mux): apply fMP4 edit lists to frame timestamps - #6686
Conversation
Quest-Claim: 6532dbe2116b3c59
The fMP4 importer reduces each track's edit list to one shift, applies it to every frame timestamp, and drops the samples before the edit (such as AAC priming), so consumers need no edit list code. The passthrough init no longer carries the edts. Opus applies only the part beyond the OpusHead pre-skip, which its timestamps already count. An edit list with several media edits or a rate other than 1 is refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 11 minutes. View limit details
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
An Opus edit starting inside the OpusHead pre-skip saturated to no shift, misplacing every kept sample by the rest of the pre-skip; it now delays the track by that much instead. A sync sample the edit drops also drops the samples decoding from it, so a passthrough track no longer opens a group on an undecodable P frame. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merging 2de40c9 with the maintainer's approval. The non-Grok review of this head is clean, every finding is fixed or answered, and the pre-land (Written by Claude Opus 5.5) |
Quest:
quest/m1/fmp4-edit-lists.md(rewritten here to its remaining interop step).Summary
ffmpeg's fragmented MP4 carries an edit list on every AAC track (
media_time1024 attfdt0 under-movflags cmaf), and on H.264 with B-frames (media_timeequal to the first composition offset). The importer forwarded theedtsin the passthrough init and ignored it otherwise, so every subscriber played ffmpeg's AAC 1024 samples late and its B-frame video one composition delay late.The importer now reduces each track's edit list to one shift (new private
fmp4/edit.rs), applies it to every frame timestamp, and drops the samples before the edit, so no consumer needs edit list code:trunstarting at the first kept sample, so every kept sample keeps its decode and composition time. The catalog init drops theedts.media_timeequal to the pre-skip) is unchanged. An edit starting inside the pre-skip delays the track by the rest of it, since a consumer trims the whole pre-skip while in ISO BMFF the edit alone trims.Decisions made without asking
Error::UnsupportedEditList: more than one media edit, any edit after the media edit (a trailing empty edit is invalid ISO BMFF anyway), a list of only empty edits, and a media rate other than 1.Error::UnsupportedEditList(&'static str)carries a fixed reason, likeInvalidAspectandInvalidSubtitlebeside it: a caller can't act on which shape was refused, so a typed reason would be API with no consumer.tfdtwhen it led the fragment, and the restated fragment drops the per-sample tables (sbgp,subs, sample auxiliary information) that would index the wrong samples. No consumer reads them, only a stream's first fragments are restated, and encrypted sample entries are refused already.edtsstrip inextract_init: other publishers can still put one in a CMAF init.test/interop/andquest/m1/README.mdalone, since moq-dev/moq#6647 rewrites the neighbouring lines. The quest now holds only the interop step, to land after it.Interop finding: GStreamer's AAC decoder
With the
opuscondition dropped fromphase_checkedlocally,just test interop --all --publishers rustpassed the AAC phase on every phase-checked subscriber except GStreamer, which read the AAC tone+878usout, 1024 samples early. That is the decoder:faad, whichdecodebinpicks there, drops the first frame's output and stamps the rest one frame early, while ffmpeg's decoder keeps the priming at the input timestamps. An ffmpeg ADTS encode decodes with its first audible sample at 2 throughfaad, at 1026 through ffmpeg, and at 2770 throughfdkaacdec. The old 1024-sample error happened to cancelfaad's. The quest now records the choice between compensating inmoqsrcand skipping GStreamer's AAC phase with the reason.Public API and wire impact
container::fmp4::Error::UnsupportedEditList(&'static str). The enum is#[non_exhaustive], so this is additive.edts. A consumer that offset AAC priming on its own must stop, or it shifts the track twice. GStreamer playback throughfaadnow lands AAC 21 ms early (see above).Checks
just checkon the final head: passed (4087 tests).cargo nextest run -p moq-mux: passed (1726 tests), including the newedit::tests,audio_presents_from_its_edit, andvideo_drops_the_samples_before_its_edit, whose dropped-I-frame case failed in passthrough before the fix.just test interop --publishers rustwith the AAC phase check enabled locally: passed.just test interop --all --publishers rustwith the AAC phase check enabled locally: passed exceptrust -> gston AAC, as above. Not run with the committedinterop.sh, whose only difference is that check. Not rerun after the Opus and sync-sample fixes, which change nothing for ffmpeg's output (media_timeequal to the pre-skip, edits at the first I frame).Follow-ups
opuscondition after moq-dev/moq#6647, with a decision on GStreamer's AAC decoder delay. Itsquest/m1/README.mdsummary line goes stale until then.draft-lcurley-moq-hangsays nothing about anedtsin a CMAF init. It could say a consumer ignores one, matching what the importer and the export now do.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)