Repository navigation
quest: import #7965, flag the TS PCR only where its clock jumps - #7972
Conversation
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 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. |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: be0bfd2
Findings: No actionable findings in this quest-only change. The plan correctly distinguishes padded delivery gaps from skipped schedule slots, preserves declared-switch handling, and explicitly supersedes the earlier loss-coalescing decision. I checked the premise against Schedule::index / next, PCR flag emission, and a_skipped_group_flags_the_clock at this head.
Direction: Deriving the flag from the actual schedule jump is appropriate. When implementing, cover a timestamp gap with consecutive group sequences as well as delivery loss, and retain the short-gap/no-flag and long-gap/first-PCR-only assertions across padded and unpadded output.
Verification limits: Static GitHub review only; no tests executed. This PR changes two planning documents and does not implement the behavior. CI was still in progress when checked.
There was a problem hiding this comment.
ℹ️ One plan detail worth tightening — nothing blocks the import.
Reviewed changes
- New quest
quest/m1/ts-reliability/pcr-jump-flag.md— imports #7965: flagdiscontinuity_indicatoronly where the schedule skips slots, delete #7785's per-track loss tracking, rework the tests and the docs describing the skip flag. - Epic README — marks the 2026-11-05 coalescing decision reversed and lists the quest under
## Required.
Verified: BACKFILL is 40 slots at 25 ms (1 s), Schedule::index jumps to first - BACKFILL, a_skipped_group_flags_the_clock runs 6 s holes, every symbol in the deletion list exists on main, doc/bin/cli.md:235 and test/ts/README.md:759 describe the skip flag, and quest check passes (598 documents).
ℹ️ The deletion list drops the schedule's send-ahead reset along with the loss signal
Schedule::skip does two jobs and the plan's list removes both. Besides marking the flag, queue clears self.last so the resumed unit keeps its own send-ahead; without that, release clamps to unit.start = (last + 1).min(due) (schedule.rs:976, 1089), so the first unit after a hole goes out in the slot right after the old one and index (schedule.rs:1093) jumps far less than the real gap. Since the new derivation takes the flag from that jump, the schedule-side replacement needs an equivalent reset derived from unit timestamps rather than deleting it with the per-track signal.
Technical details
# Preserve the resume reset when the skip signal goes
## Affected sites
- `rs/moq-mux/src/container/ts/schedule.rs:886` — `queue` sets `self.last = None` when `gap` is set, so the resumed unit's start is its own due slot.
- `rs/moq-mux/src/container/ts/schedule.rs:976` — `start = (last + 1).min(due)`; `:1089` — `release` returns `.min(unit.start)`.
- `rs/moq-mux/src/container/ts/schedule.rs:1093` — `index` jumps only when the first release is more than `BACKFILL` ahead of `next`.
- `rs/moq-mux/src/container/ts/schedule.rs` — `a_skip_flags_the_jump_to_the_tables_leading_a_placed_unit` drives its jump with `skip()`.
- `rs/moq-mux/src/container/ts/export_test.rs:8465` — `a_skipped_group_flags_the_clock` reaches the 6 s jump via `PesUnit::skipped` to `Schedule::skip`; AAC is not `placed()`, so its release is the clamped `start`.
## Required outcome
- The schedule still jumps past a hole longer than `BACKFILL` and still sends the resumed unit at its own send-ahead point, with the skip signal gone.
Summary
Imports moq-dev/moq#7965 as a new quest,
quest/m1/ts-reliability/pcr-jump-flag.md, under the TS reliability epic.The issue corrects #7741: in a live relay-kill run the semantic TS export padded the outage with nulls on a continuous PCR, so the
discontinuity_indicatorthat moq-dev/moq#7785 sets on every delivery skip declared an unchanged time base. Checked againstmain: true for holes the schedule pads, butSchedule::indexskips empty slots pastBACKFILL(1 s), and there the PCR does jump (a_skipped_group_flags_the_clockcovers it with 6 s holes).Decisions
Direction for #7965:
Milestone:
The epic README's 2026-11-05 coalescing decision is marked as reversed by the new quest.
Review
Codex
gpt-6.1-sol(medium), head be0bfd2: no findings.Impact
Quest tree only; no code, API, or wire change.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code