Repository navigation
fix(moq-mux): move a sub-tick presentation tie up one tick on the declared clock - #8715
Merged
Merged
Conversation
…lared clock Two frames with distinct source timestamps can round onto one container tick (one FLV millisecond, one 90 kHz tick). The declared clock then authored two equal DTS and failed the export. The later-presented frame now moves up to the next free tick, and the FLV, MKV, and MPEG-TS exports present it there. Identical source timestamps still fail. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…view Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
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 18 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. |
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
I reviewed how DeclaredClock now resolves presentation ties that come from rounding, and how the FLV, MKV, and MPEG-TS exports write the moved PTS.
- Tie lift in
DeclaredClock::push:highestnow carries each frame's source nanoseconds. A frame moves to the tick after every retained frame with an earlier source at or above its tick. Because the loop walkshighestin ascending order, a run of ties keeps moving up. Ticks inhighestandslotsstay unique, sopop's DTS invariant holds. - New
ClockError::Tie: identical source timestamps, or a frame presented below a later-sourced frame already on its tick, fail explicitly. Before, they reached thePresentationerror. - Exports rewrite the moved PTS: FLV writes
from_millis(pts)and MKV writesfrom_scale(pts, 90_000). TS writesceil(pts·100/9)µs, which I checked always reads back throughto_ticksaspts. - Tests: there are new clock cases at depth 0 and 1, and the FLV test helper now takes µs. I ran
cargo test -p moq-mux --lib -- declared export_declared_reorderlocally. All 27 tests that can run here pass, includingsub_tick_ties_move_to_the_next_free_tickand bothexport_declared_reorder_decodes_each_group_from_its_framestests. The one failure ists::recovery_test::a_declared_discontinuity_survives_loss_either_side, which panics with "ffmpeg is in the dev shell" because ffmpeg isn't installed in this environment.
ℹ️ Remaining tie cases still end the export
These cases don't block the merge, but they behave the same as before:
- MKV: the clock runs at 90 kHz, but
feed_framewrites blocks in ms. An 11 µs lift therefore lands both frames on the same MKV block timestamp. MKV no longer fails, but it still can't present the two frames apart. MKV already collapsed sub-ms timestamps like this before this PR. - TS
steps_back: it checksreorders_past(to_ticks(pts))with the tick before any lift. A backward step smaller than one tick is therefore not treated as a timeline restart.pushrefuses it asTie, so the export fails, as it did before throughPresentation. - Identical source timestamps: an MP4 published directly, not through ffmpeg's TS path, can repeat a sample's timestamp exactly. That still ends the FLV play. At depth 0, decode order would decide which frame presents first, if you ever want to cover that case too.
claude-opus-5-5 | 𝕏
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.

Problem
A rendition that declares
reorderclocks each group withDeclaredClockat its container's tick: 1 ms for FLV, 90 kHz for MKV and MPEG-TS. Two frames whose source timestamps are distinct but round onto one tick take the same presentation slot. The clock then authors two equal DTS and fails the export withvideo presentation slots cannot author increasing DTS at or before PTS, which ends the FLV/RTMP play.This is easy to hit: an MP4 with a sample that repeats its predecessor's timestamp, looped through
ffmpeg -c copyinto MPEG-TS, publishes the repeat one 90 kHz tick (11 µs) later. Every FLV viewer then drops each time the stream reaches that frame. The FLV export authored a tie one tick apart withauthor_dtsbefore moq-dev/moq#6453 moved it onto the declared clock.Approach
DeclaredClock::pushtakes the frame's source timestamp along with its tick. A frame moves up to the tick after every frame still in the clock's lookback window that is presented before it, so a run of frames inside one tick presents one tick apart, in source order. Identical source timestamps still fail, as does a frame presented below one already on its tick, since neither leaves a slot to take.popreturns the PTS the frame presents at, and the FLV, MKV, and MPEG-TS exports write it.Tests: a clock case at depth 0 and 1 (pairs, runs, identical and unplaceable ties), and an FLV export case, which fails on
mainwith the error above.Impact
Alternatives
author_dtsdid: at depth 0 that decodes after presenting, a negative composition time.Caused by
author_dtsbumped.Follow-ups
None.
Models
Size: S
Implementer: Claude Code, claude-opus-5-5, medium effort (iterate, S)
(Written by Claude Opus 5.5 with medium)
Review
Rewritten by
quest reviewafter each round.01a1d82c-4208-7c41-b3bc-86d81e14df82, round 2The updated tick advancement fixes ascending collision runs, and code inspection found no new issues in the incremental change.rs/moq-mux/src/container/declared.rs:96:Preserve collision history when advancing presentation ticks;push now advances past every retained frame with an earlier source timestamp, preserving displacement across evictions; the added depth-zero case maps [40000, 40011, 40022, 40033, 80000] µs to ticks [40, 41, 42, 43, 80].