Repository navigation
fix(ts): start an export on a whole group and count every drop - #5713
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A live-edge export subscribes with a zero budget, so the publisher can reset the group it joined at (`Old`) before the raised budget reaches it, and the export emitted that group's cut-off start ahead of a hole (#5565). The jitter buffer now holds a track's start group whole: until a frame of the track has gone out, a loss (a source gap, or a frame it drops) takes everything the track holds, and it starts at its next sync frame. The TS export's tune-in follows the video's start when it moves. Drops are counted by cause: `dropped` (after a track's output began, including a new generation's acquisition, previously uncounted), `trimmed` (before it), and `gaps` (media skipped upstream, each logged with its span). The TS compliance harness prints them beside a failing decoded-content check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Quest outcome: the confirmed CI cause (#5565, a zero-budget live-edge join whose start group the publisher resets Left as draft for the open decisions in the description: a separate check for host-starvation drops (recommended), the trim's scope for sparse tracks (recommend keeping it uniform), and the field names. Suggested follow-up: a loaded (Written by Claude Opus 5.5) |
A frame late before its track's first went out counts as trimmed, not dropped, so the warning carries both counts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # quest/m1/ts-drift-startup.md
…es-2/ts-video-content
…es-2/ts-video-content # Conflicts: # quest/m1/test-flakes-2/README.md # quest/m1/test-flakes-2/ts-resume-late-frames.md
…es-2/ts-video-content # Conflicts: # quest/m1/test-flakes-2/README.md
A data track marks a discontinuity by skipping a sequence, which the consumer reads as media skipped upstream. Document that in stats::Export::gaps and the warning, and add quests for telling the break apart on the wire and for audio leading a keyframe after a trimmed video start. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
|
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 3 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 765f99b2f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The jitter buffer marked a track started on release, before the export dropped the frame for leading the video keyframe. A later loss on that track then counted as dropped from the output and skipped the start trim. Buffer::discard now counts the dropped release and leaves a track whose first frame it was not started. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Combine the start trim with #5666's leading-picture drop. A resumed sync frame's leading pictures count as dropped once the track has started and as trimmed before, and they no longer clear the queued sync frame. #5666's export test showed the start trim also dropped whole groups ahead of a clean sequence hole before anything went out. ExportSource now counts cuts, gaps that lose the rest of a group it had begun, and the jitter buffer trims a track's start only on a cut. A data track's declared break follows a closed group, so it no longer trims either. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cfef1cb8a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A delivery gap arms the leading floor, and the pictures below it were discarded before the jitter buffer saw them, so they counted nowhere. They are now dropped when the output holds frames ahead of the hole, and trimmed when they lead the track's first keyframe. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9bd0e6182
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .filter_map(|track| self.jitter.peek(&track.pid)) | ||
| .map(|queued| queued.frame.timestamp) | ||
| .min(); | ||
| timestamp < next.map_or(start, |next| next.max(start)) |
There was a problem hiding this comment.
When acquisition trims every frame from the initial video group before the next keyframe arrives, peek returns None and this fallback compares audio/data against the stale video_start. Frames at or after that timestamp are therefore muxed before any video, potentially leaving a long audio-only preamble that prevents live decoders from acquiring the in-band video configuration. While video_out is false and no video frame is queued, non-video releases should remain trimmed until the next video keyframe is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Known, and deliberately deferred. This is decision 4 in the PR description. The maintainer chose to land the fix (trim non-video releases while no video went out and none is queued) only with a regression test that reproduces it. Three bounded paused-clock attempts did not get the acquisition to drop the whole start group before the next keyframe arrived, partly because a frame's arrived is when its source was last found empty. The exception is named in the description, and quest/m1/ts-reliability/audio-lead.md tracks the fix together with the fixture it needs.
(Written by Claude Opus 5.5)
|
Ready to land at Changes: an export now starts each track on a whole group. Until a track has sent a frame, a gap that cut its group short, or a frame dropped as late or stale, takes everything the track holds, and the track starts at its next sync frame. That fixes the #5565 flake, where a zero-budget live-edge join had its start group reset Decisions (details in the description):
Checks: CI is green on the final head, and Follow-up worth a quest: a loaded (Written by Claude Opus 5.5) |
#5713 merged with the cold-join regressions this draft gated on, so drop the prerequisite (its quest is gone) and record what the recovery tests reuse instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolve quest/m2/qos-publisher-timeliness.md by taking main's m2 paths and dropping its Related link to the schema quest this PR completes. Record the export re-timing section decision in the TS health stats quest, reconciled with ts::stats::Export's trimmed and gaps from #5713. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-dev#5746) Co-authored-by: Luke Curley <432854+kixelated@users.noreply.github.com> Co-authored-by: GPT-6 <noreply@openai.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Closes #5565
Problem
The TS compliance
video-contentcheck flaked on unrelated PRs: the capture kept pictures 25 to ~33, then 50 onward. Root cause (confirmed in #5565 from the #5553 and #5566 runs):ExportSource::live()subscribes withmax_delay = 0to pick the live edge, then raises the budget after the first frame. When the next group lands before the raised budget crosses the relay, the publisher resets the joined groupOld. The export already held that group's first pictures and wrote them out ahead of a hole. The loss went unreported apart from the consumer's eviction warning.Changes
rs/moq-mux/src/jitter.rs). Until a frame of a track has gone out, a loss takes everything the track holds, and the track starts at its next sync frame. A loss here is a cut (a gap that loses the rest of a group the source had begun) or a frame the buffer drops (stale at acquisition, late).ExportSourcecounts cuts (cuts(), crate-private) and they reach the buffer asjitter::Arrival::cut. Whole groups missing after one that ended are not a cut, so they trim nothing. The same goes for a data track's declared break, which follows a closed group. fix(video): trim orphan leading pictures after delivery breaks #5666's leading-picture drop combines with it: a resumed sync frame's leading pictures count asdroppedonce the track has started, and astrimmedbefore. This covers both candidates in the quest's hypothesis 1: the upstream cut, and a held IDR that passes while the rest of its group is stale at acquisition. The zero-budget live-edge pick stays as it is, as the quest recommends.ts/export.rs). The stream still begins on the video keyframe, with one exception below. Until video goes out, audio that presents before the earliest frame the video holds is dropped. Without this, a second of audio would lead the moved keyframe.just test tsloses most of the first video group when it is evicted asOldjust after subscribing (4 of 51 CI runs) #5565 is unaffected, because the next keyframe arrives with the gap. Tracked inquest/m1/ts-reliability/audio-lead.md.stats::Exportgainstrimmed(dropped before a stream's output began) andgaps(media skipped upstream).droppednow also counts frames that fall due before a new generation's acquisition ends after output began; those were silent before.ExportSourcewarns on every gap with its position and span. A data track's declared break is a skipped sequence, so it also counts as a gap and gets the warning; the doc and the warning say so (quest/m1/ts-reliability/data-break.mdtracks telling them apart).moq export tslogs the new counts in itsTS export release clockline.jitter.rs, from Codex review).jitter::Readysays whether it is its track's first frame out. A release the export drops ahead of the keyframe goes throughBuffer::discard, so a later loss on that track is still trimmed rather than counted asdropped.ts/export.rs, from Codex review). fix(video): trim orphan leading pictures after delivery breaks #5666 drops the leading pictures below a recovery keyframe before they reach the jitter buffer. They now count throughBuffer::lose: asdroppedwhen the output already holds frames ahead of the hole, and astrimmedwhen they lead the track's first keyframe.test/ts/run.sh). A failingdecode.pynow prints the exporter's release-clock counts and loss warnings beside the failure, where it used to exit with no context.Regressions, all on the paused clock:
a_start_group_cut_short_starts_at_the_next(export): reproduces the CI signature, pictures 0..4 then 15+.a_start_group_cut_by_a_gap_is_dropped_whole(jitter).a_join_drops_its_group_whole(jitter): the held-IDR case.audio_dropped_ahead_of_a_moved_start_is_not_started(export): audio dropped ahead of a moved start, then late, is trimmed, not dropped.Each one fails without the fix.
only_a_gap_inside_a_group_cuts_it(source): an eviction mid-group is a cut; a missing whole group is not.Existing tests:
export_drops_an_ac3_frame_cut_by_a_markerkeeps main's expectation, and its doc now says why the break trims nothing.a_resumed_sync_frame_drops_its_leading_frames(#5666) now lets the first frame go out before the late one, so its leading-frame loss still counts asdropped.Public API / wire
moq_mux::container::ts::stats::Export(#[non_exhaustive]): newtrimmedandgapsfields.droppedis now documented as "after the stream's output began", and it also counts frames that fall due before a new generation's acquisition ends.ExportSource::cuts,jitter::Arrival::cut,jitter::Ready::first,jitter::Buffer::discardandjitter::Buffer::loseare crate-private.Checks
just check: passed.cargo nextest run -p moq-mux: 1406 passed on the final head, merged with main (fix(video): trim orphan leading pictures after delivery breaks #5666 included). moq-cli tests passed on the merge.clippy -D warningson moq-mux and moq-cli, andquest check: passed.TS full multiplexfailure on the drift exit from flaky:just test ts --fullexits on the release clock's first rate estimates (~1 in 5 local runs) #5564, which fix(moq-mux): judge a source's clock on slack the floors saw #5714 fixed. The final head is green.just test ts: passed twice locally, loggingtrimmed=2 gaps=0 dropped=0. Not looped under load.Decisions
Settled with the maintainer during iteration.
arrived, which is when the source was last found empty. So starving only the exporter cannot make a frame late; only late delivery upstream can, and that is real loss.decode.pykeeps failing on any loss after output began, and the harness prints the exporter's counts beside the failure.export-dropscheck, withvideo-contentandaudio-contentexcusing holes whendropped> 0. CI fails either way, and this could hide corruption that coincides with a drop.Read::Gap. So every declared break before such a track starts trims what it holds and counts ingaps.gapsdoc and the "media skipped upstream" warning for data-track breaks, and add a quest to tell the break apart on the wire (quest/m1/ts-reliability/data-break.md). Since the merge with fix(video): trim orphan leading pictures after delivery breaks #5666, the trim follows only cuts, so a declared data-track break no longer trims anything. It still counts ingaps.expect.stats::Exportfields.trimmedandgaps.startupfortrimmed.skipsforgaps. Rejected becauseExportSource::skipsincludes markers.quest/m1/ts-reliability/audio-lead.mdtracks the fix.Follow-ups
test/ts/run.shloop should name the cause. I recommend a quest to run that loop and fix what it shows.drift_ppmof -100 to -250 without_of_tolerancerising during the default round. fix(moq-mux): judge a source's clock on slack the floors saw #5714 (merged) addressed that start-up drift; this branch is merged with it.The quest file is deleted in this PR.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code