Skip to content

refactor(mux): sync resets at a segment transition, as JS does - #8856

Open
kixelated wants to merge 3 commits into
mainfrom
quest/m2/sync-segments
Open

kixelated wants to merge 3 commits into
mainfrom
quest/m2/sync-segments

Conversation

@kixelated

@kixelated kixelated commented Nov 15, 2026 •

Copy link
Copy Markdown
Collaborator

Completes quest/m2/sync-segments.md and deletes it.

Problem

moq_mux::sync mapped encoder segments onto the shared clock across declared ends. It assumed a replacement encoder might restart its timestamps, which the protocol forbids (doc/concept/identity.md: keep timestamps moving past the old track's). The model had these parts:

  • a Transition per restart, shared by roles and paired by their earliest plan;
  • a Mapping that rebased each replacement;
  • a Resume that waited for the old tail;
  • a hold that shifted held arrivals.

Its state was split two ways:

  • Pending kept three parallel members/resumed/plans arrays.
  • One track's segment sat behind its own lock as well as the clock's.

@moq/watch runs none of this, so Rust ran a third model that the deleted mux-sync quest said not to invent. The maintainer picked option (a) on moq-dev/moq#8764: delete the transitions and reset the way JS does.

Approach

moq_mux::sync (1,894 → 979 lines):

  • Gone: Transition, Mapping, Resume, sync::Error/Result, and Track::{declare, end, plan, apply, abandon, gap, retire, map, resume}, with the state behind them.
  • Track keeps received, reset, set_target, and poll_delay_changed, and holds only the clock and its role. Each value now sits behind one lock.

moq play follows @moq/watch's rule:

  • Video resets its part of the clock (Track::reset) at the first frame of a group later than a declared end's, as js/watch/src/video/decoder.ts #onBreak does.
    • It keeps one buffer per encoder, so the codec still drains before a fresh encoder's keyframe.
    • The next buffer starts as soon as the previous one drains, without waiting for a mapping or for the window to empty.
    • Every arrival folds into the clock.
    • The max-delay decoder no longer fences on segments.
    • Receive still stops reading while a segment behind the front holds media older than the age budget, as behind a decoder slower than real time, so the subscription evicts the excess. Each segment ages out only its own media.
  • Audio keeps the clock through a declared end, as js/watch/src/audio/decoder.ts does. The sink plays comfort noise after end() and starts the next utterance with next(). An end already queued past that utterance's first frame still declares its silence (Crossing::End { ahead }).
  • Removed with the model: Buffer::end, Buffer::clear, and the hold behind a retired tail.

JS is unchanged: it already ran this rule.

Impact

  • Public API, moq-mux: removes sync::{Transition, Mapping, Resume, Error, Result} and the transition methods on sync::Track. moq_mux::sync is unreleased (the release branch has no sync.rs), so this needs no ! and no MIGRATE entry. The CHANGELOG's poll_changed line drops its mention of transition bookkeeping.
  • Behavior, moq play: media after a declared end plays on the timestamps it carries.
    • A publisher that restarts its timestamps across an end breaks the timestamp contract. Its media now lands in the past and is skipped onto the clock instead of being remapped. Only its own broadcast suffers, and nothing panics.
    • Video's reset can move the reference later by up to the arrival jitter at that point, as it already does in the browser.
  • Wire: none.

Choices made without asking

  • Each kind follows its own JS rule: video resets at a later group after a declared end, and audio carries the clock on. Making audio reset too would be a third rule; JS audio's comment says the clock stays anchored.
  • Kept Track::reset's name (it matches JS Track.reset()). retire became plain drop, since nothing waits on a tail any more.
  • Deleted the tests whose premise was a held replacement: a_held_video_resume_leaves_the_excess_to_the_subscription, a_held_audio_resume_leaves_the_excess_to_the_subscription, and every transition test in sync.rs. Added segments_behind_a_slow_decoder_leave_the_excess_to_the_subscription for the backpressure (it decodes all 100 pictures with the check disabled, under 50 with it). Rewrote the rest to the new model:
    • an_emptied_superseded_segment_is_discarded and an_evicted_replacement_gop_gives_way_to_the_next_replacement now publish live, with no audio hold.
    • a_restart_the_max_delay_decoder_reads_first_plays_on_the_clock asserts the replacement shows at its due time.
    • quick_resumes sends the replacement live, since timestamps run on through the end.
  • Renamed video.rs's Mapped to Ordered, since nothing maps any more.

Validation

  • just test push-to-talk passes. It runs the native moq play against the JS push-to-talk publisher, 22 presses. For every press, the native player resumed on the utterance's head. The head lag stayed flat at -551 to -555 ms, with two at -531 and -534 and no drift; fix(mux): back-to-back push-to-talk presses keep their latency #8589 measured -608 flat. Native playback had 0 underruns and 0 stalls, and every separated mute played comfort noise.
  • cargo test -p moq-cli --features play --bin moq play::: 86 passed, 1 failed. The failure is a_wide_delay_observes_the_whole_tune_in_burst, which this host can't build: NoEncoder("nothing compiled in for H264 at Software"). The test doesn't touch sync.
  • cargo test -p moq-mux --lib sync: 52 passed.
  • just check: passes.

Caused by

  • moq-dev/moq#8272: kept and generalized the Rust-only segment-transition model when porting js/watch's sync, rather than porting JS's reset (found by reviewer: the pre-cut feature review, moq-dev/moq#8764).

Follow-ups

None.

Models

Size: M
Implementer: Claude Code, claude-opus-5-5, unknown effort (arm A)

(Written by Claude Opus 5.5)

Review

Rewritten by quest review after each round.

  • Head: e265b74

  • Base: origin/main at 0e6e469

  • Risk: medium, the base has no RISK.md to name what is always high or customer-facing, so every change is medium

  • Classified: medium by default

  • Reviewer: Codex, gpt-6.1-sol, high effort

  • Session: 01a1dfe6-16ec-7dd2-af0a-f4a001f71390, round 2

  • Score: 10/10, The restored queue-wide backpressure fixes the retention issue, and source review found no new problems in the incremental change.

  • Rounds: 2; first round 1 (high 1); findings 1 (high 1): fixed 1, refuted 0, open 0

  • Findings: none

  • The last round's findings, at ec5c2dc:

    • Fixed: high, rs/moq-cli/src/play/video.rs:235: Preserve backpressure across queued video segments; In rs/moq-cli/src/play/video.rs:228, receive pauses when nonempty segments behind the front exceed the age budget and resumes on queue advancement, leaving excess media upstream for subscription eviction; the added slow-decoder regression test covers this scenario, though tests were not run during this review.

kixelated and others added 2 commits November 15, 2026 07:04
Quest-Claim: b05d29a00866f339
Timestamps keep moving forward across a declared end, so the shared clock
needs no segment mapping. Delete moq_mux::sync's transitions (Transition,
Mapping, Resume, declare/end/plan/apply/abandon/gap/retire/map) and the
segment state behind a second lock. moq play's video track resets its part
of the clock at the first later group after a declared end, and audio
starts the next utterance on the same clock, as @moq/watch does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title moq_mux::sync resets at a segment transition, as JS does refactor(mux): sync resets at a segment transition, as JS does Nov 15, 2026
…ecoder

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review November 15, 2026 16:13

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

Reviewed the full diff: the segment-transition model is removed from moq_mux::sync, and moq play's video and audio paths now follow @moq/watch's reset rule.

  • moq_mux::sync slimmed to the clock — Transition, Mapping, Resume, sync::Error/Result and the transition methods on Track are gone. Track now holds only {clock, role}, so each value sits behind one lock. No other crate in the workspace uses the removed API, and release has no sync.rs, so no migration note is needed.
  • Video resets at a later group after a declared end — receive opens a new Segment only for group > end, which matches JS #onRead's read.group <= ended guard, and calls role.reset() there. Step::Next advances once the front drains and the codec has flushed. Generation bookkeeping stays sound because Step::Next is reachable only after Flush clears the decoder's generation.
  • Backpressure kept across queued segments — receive still pauses while a segment behind the front has waited past max_delay + held. The new slow-decoder test covers this.
  • Max-delay decoder unfenced — the fence/origin/advancing gating is removed, since nothing waits on a mapping any more.
  • Audio carries its clock through a declared end — Crossing::End { ahead } replaces the resume wait. ahead reproduces the old deferred-end sink.end() path for an utterance whose end was read before its first PCM.
  • Tests rewritten to the new model — tests built on held replacements are deleted, and the remaining ones now publish live with continuing timestamps.

Pullfrog  | View workflow run | Using claude-opus-5-5 | 𝕏

@coderabbitai

coderabbitai Bot commented Nov 15, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The sync module removes encoder-segment mapping and transition APIs. Tracks contribute received timestamps directly to the shared clock. Audio playback handles declared ends using original frame timestamps. Video playback advances through buffered segments without presentation transition resumption. Tests and supporting documentation were updated to reflect these changes.

Merge Risk: Minimal · up to e265b

This change simplifies how audio and video stay in sync when a stream restarts or switches renditions, following the web player's approach. When audio finishes, video may briefly run slightly ahead of the last buffered audio until the replacement audio starts. This brief overlap appears to be intended. No blocking playback, integrity or availability problems were found, and the change looks ready to merge.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title names the main change: moq_mux::sync resets at a segment transition, following the JS behavior. The changeset removes the Rust transition/remapping model and makes moq play follow the @moq/w…
Description check Passed The description explains the problem, the approach, user-visible impact, validation runs, and the upstream PR that caused the issue. It matches the changeset: it describes removing the Transition/Mapp…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title names the main change: moq_mux::sync resets at a segment transition, following the JS behavior. The changeset removes the Rust transition/remapping model and makes moq play follow the @moq/watch reset rule, so the title is related to the primary change. The title uses a refactor prefix and is specific enough for a teammate scanning history. It is 62 characters, within a short single-sentence limit.

Full details: Description check

Explanation

The description explains the problem, the approach, user-visible impact, validation runs, and the upstream PR that caused the issue. It matches the changeset: it describes removing the Transition/Mapping/Resume model from moq_mux::sync, the video reset and audio clock behavior in moq play, and the deleted quest document. The description is detailed and on-topic.

Finishing Touches

Simplify code

  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

Recent review info

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6742b80a-9c14-43b4-8d7c-785de24b2399
Commits

Reviewing files that changed from the base of the PR and between ca88fc1 and e265b74.

Files selected for processing: 6

  • quest/m2/sync-segments.md
  • rs/moq-cli/src/play/buffer.rs
  • rs/moq-cli/src/play/media.rs
  • rs/moq-cli/src/play/video.rs
  • rs/moq-mux/CHANGELOG.md
  • rs/moq-mux/src/sync.rs
Files with no reviewable changes: 2

  • quest/m2/sync-segments.md
  • rs/moq-cli/src/play/buffer.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant