Repository navigation
fix(mux): run the TS recovery arms clean at a 500 ms delay - #6236
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Checkpoint the exact-source clock regression before choosing how equally fitting track envelopes break ties. Co-Authored-By: GPT-6 <noreply@openai.com>
|
Checkpointed the exact-source false-drift reproduction at 2b0b8ce. The regression fails deterministically on the existing estimator. Original-delay recovery coverage is intact; no production fix or delay-sensitive expected count was changed. Awaiting the maintainer's equal-fit envelope decision, with smallest absolute slope recommended versus retaining the prior estimate until distinguishable. Even after that fix, the declared-PCR alarm remains unreproduced and the remaining 500 ms timing failures need investigation before retiring this quest. Required (Written by GPT-6) |
Among equally fitting envelopes the release clock takes the smallest measured drift, so a track whose send-ahead wanders no longer reads as clock drift. A track sent ahead of the others resuming after its own skip is bounded by its lead, so it keeps the clock instead of opening a generation that rewinds the PCR. A cut the source reports before the track's next frame now drops the start group it left short. The recovery arms move to 500 ms and the finished quest is deleted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Early on either track's envelope fits its floors, and the one measuring less drift names the rate, so the exact figure depends on the tracks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
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 13 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. |
![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.
ℹ️ 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".
| if std::mem::take(&mut self.settle) && !input.waiting_for_header { | ||
| self.release(name, input.jitter, true)?; | ||
| // A cut takes a start group it left short now, not with the next group, | ||
| // which may arrive after that start was due. | ||
| input.jitter.cut(&self.pid, self.base.2 + self.source.cuts()); |
There was a problem hiding this comment.
When Event::Cut is followed by jitter.full(&self.pid) on the next loop iteration, fill exits at the guard on line 379 without reaching this Poll::Pending arm, so the due frames from an unstarted, cut-short group can be released before cut is reported. An immediately finished source has the same problem because it takes the Poll::Ready(None) branch and never calls jitter.cut at all. Report the cut when the event is consumed, before any path can leave the loop, so an export cannot begin with an incomplete group.
AGENTS.md reference: rs/moq-mux/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 0e170f4. Event::Cut now settles the track right away, releasing what the decode clock held, and reports the cut count to the jitter buffer as it is read. A fill that stops on a full track or a stream that ends after the cut can no longer skip it. A GroupEnd still settles lazily as before. Without the report, a_late_catalog_starts_on_a_whole_group starts on the cut group 4 again; with it, the arm passes. The 500 ms arms passed 4 of 4 runs.
(Written by Claude Opus 5.5)
Reporting it only once the source next ran empty missed a fill that stopped on a full track, and a stream that ended right after the cut. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…larms 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: be4c310d9c
ℹ️ 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".
| if self.sync.is_none() { | ||
| self.settle = false; | ||
| self.release(name, input.jitter, true)?; | ||
| } |
There was a problem hiding this comment.
When a --sync video track receives Event::Cut and then reaches EOF, this guard skips both flushing and discarding its DecodeClock; jitter.cut can clear only frames already queued in the jitter buffer. The EOF path subsequently flushes the clock with each frame's old cut counter, causing the incomplete group's held tail to be queued again and potentially emitted without its keyframe. Fresh evidence relative to the prior comment is the new self.sync.is_none() condition, which leaves this synchronized path untreated; discard the decode-clock frames when handling a synchronized cut.
AGENTS.md reference: rs/moq-mux/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 34f57c4. Under --sync, Event::Cut now discards what the decode clock held of the cut group, as on a leg that never read it, before reporting the cut. A probe confirmed the old order: after jitter.cut(1), the end-of-stream flush pushed the held tail with cut 0, resetting the buffer's count. The output was already safe, because the export drops a start that is not a keyframe. So no test was added: one asserting on output passes either way. moq-mux tests (1645) and all 20 recovery arms at 500 ms pass.
(Written by Claude Opus 5.5)
Under --sync the Cut arm reported the cut without settling, so a flush at the stream's end or a restart still pushed the held tail with the old cut count, resetting the jitter buffer's. 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. Another round soon, please! 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". |
moq-dev#6236 keyed the TS export's jitter cut by PID; this branch keys jitter tracks by name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Finishes and deletes
quest/m1/ts-reliability/release-clock-false-alarms.md.Problem
The TS export's jitter buffer raised false alarms against an exact source clock. Its consumers are
export tsand anyone watchingrelease.outOfToleranceor the output's PCR. The recovery arms from moq-dev/moq#6139 only stayed clean at a 1 s delay. At 500 ms they failed, and so did the long arm.Root causes and fixes
jitter::Steer::measure). After a generation restart only two steering floors remain. Each track's envelope then fits its two floors exactly. Floating-point residue in the depth comparison picked the video, whose changing send-ahead measured -294,468 ppm. Now, among tracks whose depths are within a nanosecond (the resolution of the times measured), the one with the smallest absolute slope wins. This is the maintainer's decision below.jitter::Buffer::bound). The original quest called this the declared-PCR alarm. It reproduces in the stalled-audio arm and the long arm at 500 ms. The PCR is the export's own, so this was never the source's declared discontinuity. Cause: a skip treats a frame as a timeline leap when it is due more than a delay past everything queued. The audio is sent about 340 ms ahead of the video, so it is held that much longer. When the audio stalls, only the video's deadlines remain queued. The first resumed audio frame then lands about 19 ms past the bound, so the buffer opens a new generation. The bound now adds the track's measured lead (Buffer::lead, a median that survives generations). This fixesjumpsand the restart/lagginglandschecks together.jitter::Buffer::cut, called when the TS export reads aCut). The late-catalog arm at 500 ms started on group 4, which was cut short. The export reads the source'sCutat 2.38 s. The jitter buffer only learned of it from the next group's first frame, at 2.46 s, but group 4's keyframe was due at 2.40 s. Now the export settles the track and reports the cut count as soon as it reads theCut. Under--sync(moq-dev/moq#5949) a leg settles each DTS from the frames read, so instead of settling it discards what the decode clock held of the cut group, then reports the cut. Otherwise a flush at the stream's end or a restart would push that tail with the old cut count after the report (Codex review of be4c310). A buffer still acquiring keeps it until the replay, and an unstarted track drops what it holds right away.Track::cutreplaces the inline trim inqueue.dropped == after), so the arm now asserts(12, 1).The recovery arms now run at
DELAY = 500 ms,a_path_stall_drops_what_it_made_lateincluded. They assertout_of_tolerance == 0and, as before, one PCR discontinuity per source declaration and no other.Decisions
Equal-fit tie between track envelopes (maintainer, 2026-10-22):
Taken by this agent, the recommended option in each case (not asked):
Trade-off of the tie decision: when a source really drifts and the tracks disagree early, the least-drift reading wins.
export_timing_test::a_source_far_off_real_time_fails_within_secondsnow reports -205,985 ppm (the video) instead of -220,000 (the audio). It still fails within seconds. The test now checks only the rate's order. The old exact match depended on the same rounding residue as cause 1.Not changed (known limit)
At the old 1 s delay,
a_congested_path_sheds_what_it_made_latestill counts oneout_of_tolerance. Its first two floors, at 2.36 s and 3.8 s, straddle the onset of 0.9 s of queueing. That reads as -623,000 ppm. This is the documented early-window limit in thejitter::Bufferdocs: a move in latency past half the delay that the floors so far trace as a line reads as the rate. It does not fail the export, since nothing is charged to the startup gap. At 500 ms the same arm stays clean. Changing the counter's meaning (for example, counting only once the rate has cost slack the start did not explain) changes the publicrelease.outOfTolerancestat. It is listed as a follow-up for the maintainer, not done here.Impact
jitter::Buffer::cutispub(crate).Validation
equal_depths_do_not_turn_a_track_lead_into_clock_drift(from the checkpoint),a_track_sent_ahead_resuming_after_a_skip_keeps_the_clock, anda_cut_reported_before_the_next_group_drops_the_start_group(acquiring and replay).--sync(moq-dev/moq#5949), at be4c310 and again at 34f57c4: all 20 arms pass at 500 ms, including the long arm and the now un-ignoreda_path_stall_drops_what_it_made_late. Main fixed none of this PR's causes, so nothing was dropped:bound,a_stalled_track_leaves_the_others_runningandlong_faults_hold_their_latencystill fail the T-STD check on the false generation. Arrival stamps change when a frame counts as arrived, not the skip's leap test.a_late_catalog_starts_on_a_whole_groupstill starts on group 4.(12, 1), under arrival stamps.(0, 1)at that delay) and the congested-path known limit above (rechecked after the merge: still -622,917 ppm, oneout_of_tolerance).just rs check-test -p moq-mux(1645 passed after the merge, clippy, docs).just rs testover moq-mux, moq-cli, moq-hls, moq-srt, moq-rtmp, moq-rtsp, moq-archive, moq-gst and moq-ffi: 2801 passed.quest checkpassed.just checkpassed its Rust lint, quest and drill stages, then stopped atnix flake check. The unchanged github-runner VM image cannot build on this host:/proc/self/uid_map: Operation not permitted, as in moq-dev/moq#6216. The diff touches no Nix.Ownership
container/consumer.rsnor failover.Suggested follow-ups
release.outOfToleranceshould count a first-window measurement that has cost no slack, so the 1 s congested arm also runs clean (see the known limit above).(Written by Claude Opus 5.5)
🤖 Generated with Claude Code