Repository navigation
fix(moq-mux): judge a source's clock on slack the floors saw - #5714
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The release clock read the slack now from the floors' upper envelope extrapolated to the present. Once a move in latency stops, the edge it moved along runs on past the floors that hold after it, so `lost` grew with no frame losing or gaining slack and a source on our clock failed with "the source's clock runs ... ppm off ours". The slack is now kept within the floors seen. Closes #5564. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Outcome: implemented as a draft. I reproduced the mechanism in mocked time (the extrapolated envelope charges the rate for slack no floor showed) and fixed it by keeping the slack within the floors seen. (Written by Claude Opus 5.5) |
|
@codex review (Written by Claude Opus 5.5) |
|
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 54 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. |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
|
Merging. Summary:
(Written by Claude Opus 5.5) |
Completes
quest/m1/ts-drift-startup.md. Closes #5564.Summary
jitter::Buffer::steerread the slack now (gap) from the floors' upper envelope extrapolated tonow. Once a move in latency stops, the hull edge it moved along runs on past the floors that hold after it.lostthen kept growing although no frame lost or gained any slack, and a source on our clock failed with "the source's clock runs ... ppm off ours". The slack is now clamped to the range of floors seen in the window. Past that range the envelope is a guess at the rate, not observed slack. Steering, the startup gap andlostall read the clamped value. The rate estimate and the exit rule are unchanged.Reproduction and evidence
just test ts --full, debug, loaded machine) with a temporary per-floor trace. None of the runs exited on drift. Two failed on an unrelatedtstd"PID 257 B underflow". The traces still show the mechanism. All three tracks' floors move together by up to 0.44 s over about 12 s: one run built from +0.09 to -0.35 between 13 s and 25 s, then recovered. While a move like that spans the window's mean, the hull reads it as -44000 ppm. That run reachedlost-0.406 against the -0.45 limit.just test ts --fullexits on the release clock's first rate estimates (~1 in 5 local runs) #5564 exits on a positive rate (+32045, +9750, +21347.9 ppm). That is the draining direction. In mocked time, a single-track 0.3 s drain over 20 s from 3 s fails at 37.6 s on unfixed code. Its floors hold at 0.300 while the extrapolatedatclimbs from 0.30 to 0.499. A 0.4 s build over 5 s from 3 s also fails (-80000 ppm at 9.1 s), because the extrapolation runs ahead of the last floor.Tests
jitter::start_up_latency_never_fails_one_trackandjitter::start_up_latency_never_fails_a_full_multiplexcover a 0 ppm source through a 900 ms buffer. Its latency builds or drains by 0.3 or 0.4 s, over 5, 10 or 20 s, starting at 0, 3 or 6 s, and then holds. The multiplex variant is shaped like--full: video sent 0.35 to 0.65 s ahead and wandering, plus two audio tracks sent as they decode. No case fails or drops a frame. Both tests fail without the fix.feednow takes aShape(delay and tracks). Existing callers passPAIR, which is the old 500 ms buffer with two tracks.a_source_far_off_real_time_fails_within_seconds,a_source_past_the_drift_limit_fails, and the day-long drift test.just checkpasses. I re-ranjust test ts --full11 times; 0 runs exited on drift, but that is also true onmainlocally, so this does not prove the CI flake is gone.Public API / wire impact
jitter::Bufferispub(crate).Decisions
--full's 900 ms delay. These tests cover 300 and 400 ms. A move past half the delay (450 ms) after the rate's first measurement is a real change in observed slack. If the floors so far trace it as a line (a build starting at the first floor, or a drain after a flat start, whose flat floors the upper hull treats as queueing), it is identical to a source that far off at the moment it crosses the limit. Exiting later would need a minimum span or sample count, or a wider limit, and the plan rules out all three. I prototyped "exit only when every hull edge in the window reads past 30 ppm on the same side". It saves a 0.5 s build after a flat start, but not a 0.5 s drain after a flat start or a line from the first floor, so I left it out.Buffer.out_of_tolerancecount still counts the point estimate. Early in a run it reads thousands of ppm from latency wander. The sibling fix(ts): start an export on a whole group and count every drop #5713 saw -100 to -250 ppm and a climbing count in default runs, which matches this misfit. Left as is here.Suggested follow-ups
--fullruns, if fix(ts): start an export on a whole group and count every drop #5713 does not cover it.quest/m1/ts-drift-startup.md, which this PR deletes, so whichever lands second resolves that conflict.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code