You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(audio): moq play decodes audio only when the ring has room - #8439
moq play decoded every audio packet as it was read. A shared delay past the sink's budget, such as a slow video encoder's, was held as decoded PCM in play_audio's queue until it was due (moq-dev/moq#8362), so native memory grew with the delay. Separately, a shared catalog delay that fit the sink skipped that path's lead padding, so the first write played behind the sink's opening 50 ms cushion and audio started up to the catalog delay early.
Approach
moq_audio::decode::Backpressure, from Consumer::backpressure(), gates when Consumer::read decodes, mirroring Backpressure in js/watch/src/audio/buffer.ts for live playback. The consumer keeps reading packets as they arrive, so target.observe still times each arrival, and holds them encoded. A packet decodes once the media queued behind it covers the hold, or nothing more continues it (a gap, an end, or the track's end is queued after it), or once the playhead is within the floor of where its output ends, and in any case once it has waited the hold since it arrived. That last rule bounds what the gate keeps by arrival time, whatever the timestamps say. With no hold, everything decodes at once, as in the browser. Dropping the handle stops gating, so other users of Consumer see no change.
Consumer::arrival() reports when the packet behind the latest decoded frame arrived. play_audio anchors the shared clock on that rather than on when read returned, so the hold is not counted twice.
A declared silence now reaches the estimator as its packets arrive rather than as they decode, so the silence corpus case still holds.
play_audio splits the shared delay at the sink's cap (max_delay, as before): floor min(delay, cap), hold delay - cap. After each write it reports the playhead as where the shared clock says that write sounds. fix(cli): moq play holds each rendition's catalog delay #8362's wait stays only as a backstop: the consumer releases media early only when its timeline ends (a declared end, a gap, or the track's end) before the media behind it covers the hold. That short tail is the only decoded PCM held outside the ring.
A fresh sink (the first, or one opened after a reset) pads its first write to its due time. This is the decided fix for starting early, with audio_holds_a_catalog_delay_within_its_budget.
Choices made without asking:
The playhead is the shared clock's, not the sink's. The quest suggested Sink::buffered() plus the last write. Native audio follows the shared clock, so a clock playhead holds a raised delay encoded until the media is due, which is what the browser's park does, without porting the park and epoch state. Using the sink's own playhead would keep the ring on its floor and play audio ahead of a delay raised past the cap. After a stall, the clock playhead can let the ring hold the lag on top of the floor while the stretch catches up. The ring's trim still bounds that.
The floor cap stays max_delay (2 s under --delay auto), not the browser's 500 ms. Lowering it would change sink sizing for every native rendition, which is a separate behavior change that needs its own measurement.
A stale playhead is ignored by position, not by epoch. After the consumer hands over a gap or end at b, it ignores playheads at or before b, so a write from the old timeline cannot hold back the new one.
Impact
moq-audio: adds decode::Backpressure, decode::Consumer::backpressure(), and decode::Consumer::arrival(). Additive. Consumer::read decodes as before unless a gate is held.
moq play: holds a shared delay past the sink's cap encoded instead of as PCM. The first audio after a catalog delay under the cap now starts on time instead of early.
Wire: none.
Alternatives
Splitting Consumer into public fetch and decode calls, with the gate in moq-cli. That is a larger API surface, and it would expose the consumer's boundary handling.
Porting the browser's park and epoch messages. The shared clock already provides both.
Follow-ups
None.
Caused by
moq-dev/moq#8362 - held the shared delay past the sink's budget as decoded PCM, and placed no lead padding when the delay fit the sink, so audio started early
Models
Size: M
Implementer: Claude Code, Claude Opus 5.5, medium effort
Review
mechanical fix by codex, gpt-6.1-sol, medium effort: Merged base and re·solved quest/m0/README.md conflict by preserving removal of the Required index; ran git diff --check and nix develop --command just ci base (passed). (head ed93b8a)
mechanical fix by codex, gpt-6.1-sol, medium effort: Merged base, re·solved quest index conflicts preserving PR removals and new base entries, and retained a Finished handle to satisfy the updated base API; ran git diff --check base HEAD and nix develop --command just ci base (passed). (head 697fdb6)
mechanical fix by codex, gpt-6.1-sol, medium effort: Merged base and re·solved three quest index conflicts by preserving section removals and base quest-file updates; ran git diff --check base..HEAD and nix develop --command just ci base (passed). (head a7ad8dc)
mechanical fix by quest land watch: merged main at 88572c583e2f64c7f5fc9bfa59df90706308497e with the quest merge driver (head f7b6e2d)
Fixed: high, rs/moq-cli/src/play/media.rs:651: Preserve packet arrival times across the decode gate; Consumer::arrival() preserves the encoded packet's arrival time, and play_audio passes it to the presentation clock instead of using gated decode completion time.
Fixed: high, rs/moq-audio/src/decode/consumer.rs:636: Keep held packets within subscription-owned retention; admit releases packets once their arrival age reaches the hold regardless of timestamps or playhead, and boundary counting plus newest-packet comparison removes the growing queue scan.
… tail
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
Native playback decodes audio only when the ring has room
feat(audio): moq play decodes audio only when the ring has room
Nov 10, 2026
The reason will be displayed to describe this comment to others. Learn more.
Automated review by review (OpenAI)
Reviewed the full patch at b30af87, including the decode gate, arrival/boundary handling, and native playback integration. No actionable findings.
Direction: keeping arrival observation ahead of decoding preserves the jitter estimator, while carrying the packet arrival into the presentation clock avoids counting the gate's hold twice. Admission includes arrival-age and timeline-end escape paths, and fresh-sink lead padding addresses catalog delays that fit within the sink budget. The new tests cover encoded holding, playhead release, age-limited release, and first-sound timing.
Verification limits: GitHub-only static review. No builds, audio tests, device playback, or memory measurements performed; runtime timing and memory behavior were not independently verified.
No actionable comments were generated in the recent review.
Walkthrough
The audio consumer now buffers encoded packets and applies a backpressure gate based on queued media, boundaries, packet arrival time, and the observed playhead. Audio playback configures the gate using shared presentation delay and reports the due time of written media. Playback timing logic and tests were updated. The quest README and stretch document no longer list the native backpressure plan, and that planning document was removed.
Merge Risk: Low · previous review 697fd
The change is mergeable with awareness that unusual packet durations may briefly produce silence and a narrowly timed gate drop may delay decoding.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly and concisely describes the primary change: audio decoding is gated until the playback ring has room.
Description check
The description is directly related to the changeset. It explains the audio backpressure design, arrival-time handling, playback changes, expected impact, and validation results.
Docstring Coverage
Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 9 functions across 4 files.
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.
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: de8b5bd4-e961-4bbe-af08-637644d72bd8
Commits
Reviewing files that changed from the base of the PR and between b30af87 and 697fdb6.
Files selected for processing: 44
quest/README.md
quest/m0/README.md
quest/m0/ci/README.md
quest/m0/nightly/README.md
quest/m0/test-flakes-2/README.md
quest/m1/README.md
quest/m1/audio-jitter-target/README.md
quest/m1/audio-jitter-target/stretch.md
quest/m1/stream-credit/README.md
quest/m2/README.md
quest/m2/cluster-routing/README.md
quest/m2/cluster-routing/routes/README.md
quest/m2/probe/README.md
quest/m2/subscribe-ranges/README.md
quest/m2/time-addressing/README.md
quest/m2/unbounded-groups/README.md
quest/m3/README.md
quest/m3/c/README.md
quest/m3/cache-routing/README.md
quest/m3/intra-refresh/README.md
quest/m3/layers/README.md
quest/m3/mesh/README.md
quest/m3/obs-moq-video/README.md
quest/m3/obs-studio/README.md
quest/m3/one-port/README.md
quest/m3/qos/README.md
quest/m3/quic-realtime/README.md
quest/m3/release/README.md
quest/m3/stats/README.md
quest/m3/stream/README.md
quest/m3/vendor-sync/README.md
quest/m3/video-quality/README.md
quest/m3/webrtc/README.md
quest/m3/wire-stats/README.md
quest/m4/README.md
quest/m4/kio-channel-contention/README.md
quest/m4/p2p/README.md
quest/m4/perf/README.md
quest/m4/stream-transports/README.md
quest/m5/README.md
quest/m5/e2ee/README.md
quest/m5/net-wrap/README.md
rs/moq-audio/src/decode/consumer.rs
rs/moq-cli/src/play/media.rs
Files with no reviewable changes: 42
quest/m3/obs-studio/README.md
quest/m3/quic-realtime/README.md
quest/m5/net-wrap/README.md
quest/m3/vendor-sync/README.md
quest/m2/cluster-routing/README.md
quest/m0/test-flakes-2/README.md
quest/m3/cache-routing/README.md
quest/m2/unbounded-groups/README.md
quest/m3/intra-refresh/README.md
quest/m0/ci/README.md
quest/m3/mesh/README.md
quest/m4/perf/README.md
quest/m3/c/README.md
quest/m2/probe/README.md
quest/m3/stats/README.md
quest/m1/audio-jitter-target/stretch.md
quest/m4/kio-channel-contention/README.md
quest/m4/p2p/README.md
quest/m3/one-port/README.md
quest/m5/e2ee/README.md
quest/m3/stream/README.md
quest/m3/wire-stats/README.md
quest/m0/nightly/README.md
quest/m4/stream-transports/README.md
quest/m2/subscribe-ranges/README.md
quest/m3/webrtc/README.md
quest/m1/stream-credit/README.md
quest/m3/qos/README.md
quest/m0/README.md
quest/m2/cluster-routing/routes/README.md
quest/m3/layers/README.md
quest/m3/obs-moq-video/README.md
quest/m4/README.md
quest/m5/README.md
quest/README.md
quest/m3/release/README.md
quest/m2/time-addressing/README.md
quest/m3/README.md
quest/m2/README.md
quest/m1/README.md
quest/m3/video-quality/README.md
quest/m1/audio-jitter-target/README.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.
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Minor suggestions only.
Reviewed changes
Decode gate (moq-audio): adds decode::Backpressure and Consumer::backpressure(); Consumer::read now reads through next(), which observes every arrival, queues packets encoded, and admits one via admit() once the queued media covers the hold, the playhead is within the floor of the packet's end, or it has waited the hold since arrival.
Consumer::arrival(): exposes the encoded arrival of the packet behind the latest decoded frame, so a gated handover does not count the hold twice in the shared clock.
Arrival-time estimator bookkeeping: target.observe and target.end moved from read() into next() at arrival, with a declared group carried until a later group or a loss arrives.
moq play audio: splits the shared delay at the sink cap (floor = min(delay, cap), the rest held encoded), steers the gate's playhead from the shared clock after each write, keys the clock's anchor on Consumer::arrival(), and pads a fresh sink's first write to its due time.
Tests: a paused-clock test covering all three release rules of the gate, and audio_holds_a_catalog_delay_within_its_budget sharing first_sound/assert_on_time with the existing shared-delay test.
Quest docs: native-backpressure.md removed along with both references to it; no dangling links.
I traced the ungated path against origin/main and found no behavioral difference for consumers that never take a Backpressure handle (moq-ffi, moq-c, the integration and conformance suites), including packet order, observe/end timing and end/error handling. The new gate test passes locally (cargo test -p moq-audio --lib backpressure, 0.01 s on a paused clock). The one finding is a doc claim the new arrival-based timing falsifies.
The reason will be displayed to describe this comment to others. Learn more.
The arrival-versus-handover distinction this adds is exactly what Consumer::delay() (line 322) still contradicts: it says the estimate "measures when read hands each packet over" and that a caller must therefore read promptly rather than pacing to the speaker. With a gate held, next() times the container handover and read() resolves up to the hold later, so that sentence should say the gated mode times arrivals instead.
Technical details
# The playout estimate no longer depends on read promptness when gated## Affected sites-`rs/moq-audio/src/decode/consumer.rs:322-325` — "The estimate measures when `read` hands each packet over, so it is only as honest as the caller is prompt: read as soon as the previous frame is handled ... rather than pacing reads to the speaker." False for a `Consumer` that holds a `Backpressure` handle: `next()` (line 637) observes at the handover and `read()` may resolve up to `hold` later.
-`doc/concept/audio-jitter.md:47-50` — "`read()` is timed when it resolves, so a player drains it as packets come ... rather than pacing reads to the speaker." The concept doc the two implementations are held to.
-`doc/lib/index.md:71` — same advice in the end-user feature list.
-`rs/moq-ffi/src/audio.rs:605-608` — same wording on `MoqAudioDecoder::delay`.
## Required outcome- Each site states which moment is timed: arrival when a gate holds the consumer (packets are read and observed ahead of the handover), and the `read()` handover otherwise. The existing "read promptly" advice stays correct for the ungated case and should not be deleted, only scoped.
## Open questions for the human (optional)- Whether the concept doc should describe the gate as the recommended way to consume live audio, or stay silent on it until `moq-ffi`/`moq-c` can wire it too.
quest land watch: the merge queue removed this pull request at b30af87 because it conflicts with main (merge_conflict).
The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.
quest land watch: armed auto-merge on this pull request at f7b6e2d, so it joins the back of the merge queue once its required checks pass, with main at 88572c5 merged in by the quest merge driver.
quest land watch: the merge queue removed this pull request at f7b6e2d because it conflicts with main (merge_conflict).
The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.
codex, gpt-6.1-sol, medium effort land fixer note:
Merged the current base and resolved the three quest index conflicts by preserving the PR’s removal of the index sections. Base updates to the underlying quest files are retained. The fix changes no runtime code. Validation passed with nix develop --command just ci base, including 1,739 Rust tests (60 skipped).
quest land watch: armed auto-merge on this pull request at a7ad8dc, so it joins the back of the merge queue once its required checks pass, with the land fixer's fix (codex, gpt-6.1-sol, medium effort): Merged base and re·solved three quest index conflicts by preserving section removals and base quest-file updates; ran git diff --check base..HEAD and nix develop --command just ci base (passed)..
quest land watch: the merge queue removed this pull request at a7ad8dc because it conflicts with main (merge_conflict).
The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.
codex, gpt-6.1-sol, medium effort land fixer note:
Resolved the quest index conflicts by preserving the PR’s section removals, keeping new base entries, and respecting base’s deleted explicit-state README. The merged base also requires retaining the Finished handle in one added audio test; it now matches the existing tests. The full queue gate passed with nix develop --command just ci base, including lint, formatting, compilation, tooling tests, and all 1,737 selected Rust tests (60 existing skips).
quest land watch: re-enqueued this pull request at 697fdb6, at the back of the merge queue, with the land fixer's fix (codex, gpt-6.1-sol, medium effort): Merged base, re·solved quest index conflicts preserving PR removals and new base entries, and retained a Finished handle to satisfy the updated base API; ran git diff --check base HEAD and nix develop --command just ci base (passed)..
quest land watch: the merge queue removed this pull request at 697fdb6 because it conflicts with main (merge_conflict).
The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.
quest land watch: armed auto-merge on this pull request at ed93b8a, so it joins the back of the merge queue once its required checks pass, with the land fixer's fix (codex, gpt-6.1-sol, medium effort): Merged base and re·solved quest/m0/README.md conflict by preserving removal of the Required index; ran git diff --check and nix develop --command just ci base (passed)..
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
kixelated
changed the title
kixelated
left a comment
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Finishes quest/m1/audio-jitter-target/native-backpressure.md.
Problem
moq playdecoded every audio packet as it was read. A shared delay past the sink's budget, such as a slow video encoder's, was held as decoded PCM inplay_audio's queue until it was due (moq-dev/moq#8362), so native memory grew with the delay. Separately, a shared catalog delay that fit the sink skipped that path's lead padding, so the first write played behind the sink's opening 50 ms cushion and audio started up to the catalog delay early.Approach
moq_audio::decode::Backpressure, fromConsumer::backpressure(), gates whenConsumer::readdecodes, mirroringBackpressureinjs/watch/src/audio/buffer.tsfor live playback. The consumer keeps reading packets as they arrive, sotarget.observestill times each arrival, and holds them encoded. A packet decodes once the media queued behind it covers the hold, or nothing more continues it (a gap, an end, or the track's end is queued after it), or once the playhead is within the floor of where its output ends, and in any case once it has waited the hold since it arrived. That last rule bounds what the gate keeps by arrival time, whatever the timestamps say. With no hold, everything decodes at once, as in the browser. Dropping the handle stops gating, so other users ofConsumersee no change.Consumer::arrival()reports when the packet behind the latest decoded frame arrived.play_audioanchors the shared clock on that rather than on whenreadreturned, so the hold is not counted twice.silencecorpus case still holds.play_audiosplits the shared delay at the sink's cap (max_delay, as before): floormin(delay, cap), holddelay - cap. After each write it reports the playhead as where the shared clock says that write sounds. fix(cli): moq play holds each rendition's catalog delay #8362's wait stays only as a backstop: the consumer releases media early only when its timeline ends (a declared end, a gap, or the track's end) before the media behind it covers the hold. That short tail is the only decoded PCM held outside the ring.audio_holds_a_catalog_delay_within_its_budget.Choices made without asking:
Sink::buffered()plus the last write. Native audio follows the shared clock, so a clock playhead holds a raised delay encoded until the media is due, which is what the browser's park does, without porting the park and epoch state. Using the sink's own playhead would keep the ring on its floor and play audio ahead of a delay raised past the cap. After a stall, the clock playhead can let the ring hold the lag on top of the floor while the stretch catches up. The ring's trim still bounds that.max_delay(2 s under--delay auto), not the browser's 500 ms. Lowering it would change sink sizing for every native rendition, which is a separate behavior change that needs its own measurement.b, it ignores playheads at or beforeb, so a write from the old timeline cannot hold back the new one.Impact
moq-audio: addsdecode::Backpressure,decode::Consumer::backpressure(), anddecode::Consumer::arrival(). Additive.Consumer::readdecodes as before unless a gate is held.moq play: holds a shared delay past the sink's cap encoded instead of as PCM. The first audio after a catalog delay under the cap now starts on time instead of early.Alternatives
Consumerinto public fetch and decode calls, with the gate inmoq-cli. That is a larger API surface, and it would expose the consumer's boundary handling.Follow-ups
None.
Caused by
Models
Size: M
Implementer: Claude Code, Claude Opus 5.5, medium effort
Review
mechanical fix by codex, gpt-6.1-sol, medium effort:
Merged base and re·solved quest/m0/README.md conflict by preserving removal of the Required index; ran git diff --check and nix develop --command just ci base (passed).(head ed93b8a)mechanical fix by codex, gpt-6.1-sol, medium effort:
Merged base, re·solved quest index conflicts preserving PR removals and new base entries, and retained a Finished handle to satisfy the updated base API; ran git diff --check base HEAD and nix develop --command just ci base (passed).(head 697fdb6)mechanical fix by codex, gpt-6.1-sol, medium effort:
Merged base and re·solved three quest index conflicts by preserving section removals and base quest-file updates; ran git diff --check base..HEAD and nix develop --command just ci base (passed).(head a7ad8dc)mechanical fix by quest land watch:
merged main at 88572c583e2f64c7f5fc9bfa59df90706308497e with the quest merge driver(head f7b6e2d)Rewritten by
quest reviewafter each round.01a1c646-24c0-73c3-948e-69d94420a380, round 2The incremental change fixes both reported problems, and code inspection found no new blocking issues.rs/moq-cli/src/play/media.rs:651:Preserve packet arrival times across the decode gate;Consumer::arrival() preserves the encoded packet's arrival time, and play_audio passes it to the presentation clock instead of using gated decode completion time.rs/moq-audio/src/decode/consumer.rs:636:Keep held packets within subscription-owned retention;admit releases packets once their arrival age reaches the hold regardless of timestamps or playhead, and boundary counting plus newest-packet comparison removes the growing queue scan.(Written by Claude Opus 5.5 with medium)