Skip to content

refactor!: one exported 10 s playout ceiling - #8588

Merged
kixelated merged 4 commits into
mainfrom
quest/m1/audio-jitter-target/delay-ceiling
Nov 12, 2026
Merged

kixelated merged 4 commits into
mainfrom
quest/m1/audio-jitter-target/delay-ceiling

Conversation

@kixelated

@kixelated kixelated commented Nov 12, 2026 •

Copy link
Copy Markdown
Collaborator

One exported 10 s playout ceiling replaces four copies under three names.

Changes

  • moq_mux::container::DELAY_MAX replaces moq_mux::container::WARMUP_MAX and moq_audio::playback::Input::LATENCY_MAX; every Rust user (moq-audio, moq-video, moq-cli, moq-gst) imports it.
  • moq-cli's private duplicate and the test pinning it are gone: moq-cli always depends on moq-mux.
  • @moq/hang exports Container.DELAY_MAX; @moq/watch's internal DELAY_MAX is now an alias of it (one line in sync.ts, so the parallel push-to-talk quest's edits there should merge cleanly).
  • Deletes the quest.

Impact

Public API, breaking:

  • Rust: moq_mux::container::WARMUP_MAX and moq_audio::playback::Input::LATENCY_MAX → moq_mux::container::DELAY_MAX (same 10 s value).
  • JS: new Container.DELAY_MAX export in @moq/hang.

Wire: none.

Decisions made without asking

  • Kept the 10 s value and the name DELAY_MAX as the quest recommended; defined the JS constant beside Consumer in js/hang/src/container/consumer.ts.
  • Left rs/moq-audio/MIGRATE.md to the release cut.
  • @moq/hang's Container.Consumer now clamps warmup to DELAY_MAX, like Rust's Playout::reach (before, only @moq/watch clamped).

Validation

  • just check (Rust, JS, biome, api reports) passes.
  • cargo test -p moq-cli --features play the_delay_is_bounded_by_the_speakers_ring passes: --delay 11s is refused, 10s accepted.
  • grep finds no remaining WARMUP_MAX/LATENCY_MAX outside MIGRATE.md history, and one Duration::from_secs(10) / Duration.secs(10) definition per language.

Caused by

No single PR: the copies accumulated, most recently moq-dev/moq#8549 adding WARMUP_MAX.

Follow-ups

None.

Models

Size: S
Implementer: Claude Code, claude-opus-5-5, unknown effort

Review

Rewritten by quest review after each round.

  • Head: 8048684, named in full once no reviewer reports a finding

  • Base: origin/main at 3628a31

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

  • Session: 01a1d17a-94aa-7cb3-921d-ac490ed3b7d2, round 1

  • Score: 9/10, Source review found the refactor preserves the existing 10-second bounds while consolidating their definitions across Rust and JavaScript; builds and tests were not run as instructed.

  • Findings: none

  • Reviewer: opencode, opencode-go/deepseek-v4.1-flash, high effort

  • Session: ses_e2e856547ffeViGT6ZdV7m5AeC, round 1

  • Score: 8/10, The constant unification is complete and behavior-preserving, but the shipped migration guide still points at the removed API and the new JS constant documents a bound its own package does not apply.

  • Findings, open until the next round settles them:

    • Low, rs/moq-audio/MIGRATE.md:30: Migration guide still names the removed Input::LATENCY_MAX
    • Low, js/hang/src/container/consumer.ts:8: DELAY_MAX documents a cap @moq/hang itself does not enforce

Head 13187c5: finding 1 refuted (repo's check migrate allows only the release cut to edit MIGRATE.md; the migration lives in Impact above). Finding 2 fixed: Container.Consumer now clamps warmup to DELAY_MAX, mirroring Rust's Playout::reach.

(Written by Opus 5.5)

kixelated and others added 2 commits November 12, 2026 12:29
…AY_MAX

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title One exported 10 s playout ceiling refactor!: one exported 10 s playout ceiling Nov 12, 2026
kixelated and others added 2 commits November 12, 2026 12:49
…constant

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

@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

I reviewed the change that merges four copies of the 10 s playout ceiling into one exported constant per language.

  • Rust constant: moq_mux::container::DELAY_MAX replaces WARMUP_MAX and moq_audio::playback::Input::LATENCY_MAX. Every user in moq-audio, moq-video, moq-cli and moq-gst now uses the new name. moq-mux is a non-optional dependency of each of these crates, so Input::validate behind the playback feature still compiles.
  • moq-cli duplicate removed: args.rs now imports the shared constant. The test that pinned the duplicate is gone because nothing is duplicated any more.
  • JS export: @moq/hang exports Container.DELAY_MAX, and @moq/watch's internal DELAY_MAX is now an alias of it. @moq/watch already depends on @moq/hang.
  • Container.Consumer warmup clamp: warmup is now clamped to DELAY_MAX, matching Rust's Playout::reach. @moq/watch already clamps before it builds the consumer, so the clamp only changes behavior for code that uses @moq/hang directly.

No references to the old names remain except the historical line at rs/moq-audio/MIGRATE.md:30, which the release cut owns. No moq-ffi binding exposed the removed constants.

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

@coderabbitai

coderabbitai Bot commented Nov 12, 2026

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The changes define and export a shared 10-second delay maximum in the container APIs. Rust and JavaScript consumers use the maximum to cap catalog warmup. Rust playback paths use it to validate latency and cap playback delays. Tests and API documentation are updated to use or describe the shared maximum.

Merge Risk: Minimal · up to 13187

This change unifies the 10-second playout delay ceiling under one shared constant with the same value, so playback behavior is unchanged. The Rust API rename is breaking for downstream users but is stated as intended. No actionable merge-blocking risk remains.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title "refactor!: one exported 10 s playout ceiling" matches the main change. The PR replaces duplicate 10-second delay/warmup constants with a single exported DELAY_MAX in moq_mux::container and …
Description check Passed The description is related to the changeset. It explains that DELAY_MAX replaces WARMUP_MAX and Input::LATENCY_MAX across the Rust crates, that moq-cli's private duplicate is removed, and that @moq/ha…
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 "refactor!: one exported 10 s playout ceiling" matches the main change. The PR replaces duplicate 10-second delay/warmup constants with a single exported DELAY_MAX in moq_mux::container and mirrors it in @moq/hang. The title is short, specific, and flags the breaking change with the refactor! prefix.

Full details: Description check

Explanation

The description is related to the changeset. It explains that DELAY_MAX replaces WARMUP_MAX and Input::LATENCY_MAX across the Rust crates, that moq-cli's private duplicate is removed, and that @moq/hang exports Container.DELAY_MAX with @moq/watch aliasing it. It also lists the breaking API impact and validation steps.

Finishing Touches

Simplify code

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

Warning

Some tools did not complete. Review the errors below.

Clippy (1.99.1)

Clippy execution timed out

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: 1d8959fe-bfbb-4c86-baaf-a5de0509df9c
Commits

Reviewing files that changed from the base of the PR and between 18fc33e and 13187c5.

Files selected for processing: 13

  • js/hang/api/container.api.md
  • js/hang/src/container/consumer.ts
  • js/hang/src/container/index.ts
  • js/watch/src/sync.ts
  • quest/m1/audio-jitter-target/delay-ceiling.md
  • rs/moq-audio/src/decode/consumer.rs
  • rs/moq-audio/src/playback/sink.rs
  • rs/moq-cli/src/play/args.rs
  • rs/moq-cli/src/play/media.rs
  • rs/moq-gst/src/source/imp.rs
  • rs/moq-mux/src/container/consumer.rs
  • rs/moq-mux/src/container/mod.rs
  • rs/moq-video/src/decode/consumer.rs
Files with no reviewable changes: 1

  • quest/m1/audio-jitter-target/delay-ceiling.md

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

@kixelated
kixelated added this pull request to the merge queue Nov 12, 2026
Merged via the queue into main with commit 5e5859a Nov 12, 2026
7 checks passed
@kixelated
kixelated deleted the quest/m1/audio-jitter-target/delay-ceiling branch November 12, 2026 22:14
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