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
fix(audio): hold a latest join's whole budget from the start - #6990
Finishes quest/m0/test-flakes-2/ffi-audio-gap.md (deleted here, with its link in the epic README).
Problem
The interop tone check failed with a timestamp gap for the python -> c and go -> c cells, and some cpp -> cells, under full-matrix load. One example from moq-dev/moq#6464: 500000us after a frame ending at 393500us. The shape fits a declared gap at group 20 (400 ms) followed by the decoder's warmup trim. Back then the C client didn't fail on a decoder Gap read itself (moq-dev/moq#6745 added that check), so tone.py caught the result instead.
Every failing cell has a C subscriber. The one C-specific path is moq-c's moq_decode_audio, which joins with Start::Latest.
Cause
A Start::Latest join on a rendition with a catalog warmup subscribed with only the warmup (80 ms for Opus) as its max delay. It widened to max_delay + warmup only once it read its first packet. The transport enforces that budget too. So when a reader was starved between subscribing and its first read, the relay reset the groups queued behind it as Old, and its first few frames were followed by a gap. The relay log of a reproduction shows the audio SUBSCRIBE carrying max_delay: 80ms although the client asked for 1 s (and later 5 s), followed by a burst of Old resets once the client resumes.
There's a second, smaller cause: the gap-checked interop readers ask for a 1 s budget. A reader starved for longer than that skips on its own, the same race moq-dev/moq#6658 fixed for the native JS reader. Since moq-dev/moq#6923 the C client also fails on a video gap, and it keeps reading video for the whole audio decode, so this now hits its 1 s video budget as well.
Fix
moq-audio: a Start::Latest join now subscribes with its whole budget (max_delay + warmup) from the start. It moves the local cursor to the live-edge group, as a join without warmup already did. It primes the decoder by trimming the warmup from that group, as every other join does. The joining_latest narrowing and widening is deleted. This mirrors moq-video, which already subscribes with its full budget and only moves the local cursor.
Interop clients: the C (audio and video), C++, Go, Python, and Rust readers whose cell fails on a gap now use a 5 s budget, the same rationale and value as the native JS audio reader. The native JS video comment no longer claims to match Go and Python. The harness comment on the Rust publisher's 5 s max-age now names every gap-checked reader.
Reproduction
A loop outside the harness, in this checkout's ignored .tmp/: an anonymous relay, a fresh Python publisher per iteration (ffmpeg H.264 plus the Opus tone, as in interop.sh), and the C subscriber started with it, judged by tone.py. I SIGSTOP the subscriber 0.4 s after it starts and resume it 1.5 s later, which lands the stall in its join window.
C client build
Failures
main (1 s budgets)
3 of 40 audio gaps (other batches: 2 of 20, 1 of 16)
moq-audio fix only
3 of 40 (1 audio gap, 2 video gaps from the 1 s budget)
moq-audio fix + 5 s budgets (this PR)
0 of 40
5 s budgets only, unfixed moq-audio
1 of 30 audio gaps (the join window)
Without stalls, 40 iterations under 48 busy loops passed both before and after. CPU contention alone doesn't stall a process long enough. Late joins (1.2 to 2.5 s) and publisher stalls during the join passed both before and after, so the trigger is a stalled reader.
Impact
Public API: none.
Behavior (moq-audio decode::Start::Latest with a catalog warmup): the consumer starts at the live-edge group instead of up to one warmup before it, and holds max_delay + warmup on the subscription from the start. Latency to live is unchanged. The first audible sample can come up to one warmup later (80 ms for Opus), since priming now decodes new media rather than cached media. Callers: moq-c moq_decode_audio, moq play, moq-scene, and moq-studio.
Wire: none.
Decisions
Latest join with warmup: prime on the live-edge group. The maintainer picked (a), implemented here: prime on the live-edge group and hold the whole budget (max_delay + warmup) from the start. moq-dev/moq#5569 had narrowed the join budget so a join could prime from cached groups up to one warmup before the live edge. Option (a) gives up that head start, costing up to one warmup of time-to-first-audio and nothing in latency. It deletes code and matches moq-video. The alternatives were (b), narrowing only the local container's budget during the join, which needs a new moq-mux API, and (c), widening once the subscription is established, which only shrinks the window.
Interaction with push-to-talk and always-on encoding
Push-to-talk (quest/m1/push-to-talk/): the first-audio cost of up to 80 ms applies once per Rust Start::Latest join. Listeners held on a disabled rendition (moq-dev/moq#7156) don't rejoin on unmute. Utterance boundaries take the declared-end restart path, which doesn't re-arm warmup.
Always-on encoding (quest/m1/eager-encode/): holding the whole budget suits always-on caches. Those are exactly where the narrowed join budget got Old resets.
Choices made without asking
5 s for every gap-checked interop read, one constant per client, rather than separate video and audio budgets. Python and Go read video only up to the first frame, and C++ reads it only after audio, so their video budget can't produce a reported gap. One constant is simpler and matches what C needs. 5 s equals the Rust publisher's --max-age 5s, which already covers the native JS reader. The FFI publishers keep hang's 30 s.
The regression test asserts the invariant, that a Latest join with warmup subscribes with max_delay + warmup and joins at the live edge. Reproducing relay-side Old resets needs real transport backpressure, so the test doesn't do that.
No lookback group for the warmup, unlike moq-video's max-delay decoder. The catalog defines warmup as "decode from the group start, present at start plus warmup". A lookback group would add up to a whole group of latency on 1 s fMP4 groups.
just test interop --publishers rust,python,go,cpp --subscribers rust,python,go,cpp,c,js-native-node,gst: all checks passed (122).
After merging main again: cargo nextest run -p moq-audio -p moq-c passed (332 tests), and quest check passes.
The browser cells weren't run: nothing here touches the browser path.
Suggested follow-ups
GStreamer moqsrc budget.rs/moq-gst/src/source/imp.rs subscribes every rendition with a hard-coded 1 s max delay, and the gst cells run tone.py's contiguity check. So a starved GStreamer subscriber can fail the same way, and there's no knob to widen it. The fix is either a max-delay property on moqsrc that interop.sh sets to 5 s, or deriving the budget from the track's max-age. This is a public element property, so it's the maintainer's call.
A transport-level test for a slow joiner. No test drives a reader that stalls before its first read over a transport with real stream-credit backpressure and checks for no Old resets within its budget. It would guard the relay path this bug went through, beyond the subscription-budget invariant tested here. It could use the moq-net mock harness with a capped stream count.
A `Start::Latest` join with a catalog warmup subscribed with the warmup alone
as its max delay and widened to `max_delay + warmup` only once it read its
first packet. The transport enforces that budget too, so a reader starved
between subscribing and its first read had the groups queued behind it reset
as `Old`, and its first frames were followed by a gap. moq-c decodes audio
this way, which is why only the `-> c` interop cells failed the tone check.
Subscribe with the whole budget, move the local cursor to the live edge as a
join without warmup already did, and prime the decoder by trimming the
warmup from that group, as every other join does.
The gap-checked interop readers also give themselves the 5 s budget the
native JS audio reader has, since a reader starved past 1 s skips on its own.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No actionable bugs found. Holding max_delay + warmup from subscription creation removes the narrow transport-budget window; moving only the local cursor to the reported live-edge group preserves shared subscription ranges. Existing warmup trimming, gap re-priming and declared-end handling remain consistent with the chosen live-edge priming policy.
Direction: deleting the temporary join state is simpler than another budget update. The 5 s interop reader budgets address scheduler stalls without removing gap assertions. The regression checks both the initial budget and output start; the documented transport-backpressure regression and GStreamer budget remain separate follow-ups.
Verification: static review of the changes and surrounding audio/container/subscriber behavior. No tests, builds, stall reproductions or interop runs performed; PR-reported results were not independently reproduced.
No actionable comments were generated in the recent review.
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: a6e77b04-1d35-4874-87c1-acabd8d48ea7
Commits
Reviewing files that changed from the base of the PR and between 4f7f7c4 and b9e7507.
Files selected for processing: 10
quest/m0/test-flakes-2/README.md
quest/m0/test-flakes-2/ffi-audio-gap.md
rs/moq-audio/src/decode/consumer.rs
test/interop/clients/c/subscribe.c
test/interop/clients/cpp/main.cpp
test/interop/clients/go/main.go
test/interop/clients/js-native/subscribe.ts
test/interop/clients/python/interop.py
test/interop/clients/rust/audio.rs
test/interop/interop.sh
Files with no reviewable changes: 2
quest/m0/test-flakes-2/ffi-audio-gap.md
quest/m0/test-flakes-2/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.
Walkthrough
Latest-start audio subscriptions now select the live-edge group regardless of decoder warmup and use the full max-delay-plus-warmup budget. Interop clients raise their maximum read delay from one second to five seconds or update related comments. The required-flakes list no longer includes the FFI audio-gap quest, and its document is deleted.
Priority: Normal
Merge Risk: Minimal · up to b9e75
The latest-start behavior matches its documented contract. No issue identified here needs resolution before merging.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description check
The description directly explains the audio join-budget fix, the interop gap issue, the behavioral impact, and the validation results.
Title check
The title clearly and concisely identifies the primary change: preserving the full budget from the start of a latest audio join.
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 75.00% which is insufficient. 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 8 functions across 8 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
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.0)
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.
Merge summary: head b9e7507628b33b6f5ee7a023501275c5db9ffc12 is clean. CI passed (12 of 12), Codex gave a thumbs-up with no findings, the OpenAI automated review found no actionable bugs, CodeRabbit generated no actionable comments, and no review threads are unresolved. quest check against origin/main passes, and the finished quest quest/m0/test-flakes-2/ffi-audio-gap.md is deleted. Decision (a), priming on the live-edge group and holding the whole budget from the start, was the maintainer's pick and is recorded in the body. No changes were made during the merge. Landing through the merge queue (squash).
…ts first frame
#6990 already subscribes a Start::Latest join with its whole
budget from the start, so the remaining work in
quest/m2/audio-join-budget.md was the session-level reproduction its Plan
asks for. The test bursts one-packet groups over a memory session before
the first decode and fails with a 320 ms gap if the wire budget is
narrowed to the warmup again. Finishes and deletes the quest.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
Finishes
quest/m0/test-flakes-2/ffi-audio-gap.md(deleted here, with its link in the epic README).Problem
The interop tone check failed with a timestamp gap for the
python -> candgo -> ccells, and somecpp ->cells, under full-matrix load. One example from moq-dev/moq#6464: 500000us after a frame ending at 393500us. The shape fits a declared gap at group 20 (400 ms) followed by the decoder's warmup trim. Back then the C client didn't fail on a decoderGapread itself (moq-dev/moq#6745 added that check), so tone.py caught the result instead.Every failing cell has a C subscriber. The one C-specific path is moq-c's
moq_decode_audio, which joins withStart::Latest.Cause
A
Start::Latestjoin on a rendition with a catalogwarmupsubscribed with only the warmup (80 ms for Opus) as its max delay. It widened tomax_delay + warmuponly once it read its first packet. The transport enforces that budget too. So when a reader was starved between subscribing and its first read, the relay reset the groups queued behind it asOld, and its first few frames were followed by a gap. The relay log of a reproduction shows the audio SUBSCRIBE carryingmax_delay: 80msalthough the client asked for 1 s (and later 5 s), followed by a burst ofOldresets once the client resumes.There's a second, smaller cause: the gap-checked interop readers ask for a 1 s budget. A reader starved for longer than that skips on its own, the same race moq-dev/moq#6658 fixed for the native JS reader. Since moq-dev/moq#6923 the C client also fails on a video gap, and it keeps reading video for the whole audio decode, so this now hits its 1 s video budget as well.
Fix
Start::Latestjoin now subscribes with its whole budget (max_delay + warmup) from the start. It moves the local cursor to the live-edge group, as a join without warmup already did. It primes the decoder by trimming the warmup from that group, as every other join does. Thejoining_latestnarrowing and widening is deleted. This mirrors moq-video, which already subscribes with its full budget and only moves the local cursor.Reproduction
A loop outside the harness, in this checkout's ignored
.tmp/: an anonymous relay, a fresh Python publisher per iteration (ffmpeg H.264 plus the Opus tone, as ininterop.sh), and the C subscriber started with it, judged by tone.py. I SIGSTOP the subscriber 0.4 s after it starts and resume it 1.5 s later, which lands the stall in its join window.main(1 s budgets)Without stalls, 40 iterations under 48 busy loops passed both before and after. CPU contention alone doesn't stall a process long enough. Late joins (1.2 to 2.5 s) and publisher stalls during the join passed both before and after, so the trigger is a stalled reader.
Impact
decode::Start::Latestwith a catalogwarmup): the consumer starts at the live-edge group instead of up to one warmup before it, and holdsmax_delay + warmupon the subscription from the start. Latency to live is unchanged. The first audible sample can come up to one warmup later (80 ms for Opus), since priming now decodes new media rather than cached media. Callers: moq-cmoq_decode_audio,moq play, moq-scene, and moq-studio.Decisions
max_delay + warmup) from the start. moq-dev/moq#5569 had narrowed the join budget so a join could prime from cached groups up to one warmup before the live edge. Option (a) gives up that head start, costing up to one warmup of time-to-first-audio and nothing in latency. It deletes code and matches moq-video. The alternatives were (b), narrowing only the local container's budget during the join, which needs a new moq-mux API, and (c), widening once the subscription is established, which only shrinks the window.Interaction with push-to-talk and always-on encoding
quest/m1/push-to-talk/): the first-audio cost of up to 80 ms applies once per RustStart::Latestjoin. Listeners held on a disabled rendition (moq-dev/moq#7156) don't rejoin on unmute. Utterance boundaries take the declared-endrestartpath, which doesn't re-arm warmup.quest/m1/eager-encode/): holding the whole budget suits always-on caches. Those are exactly where the narrowed join budget gotOldresets.Choices made without asking
--max-age 5s, which already covers the native JS reader. The FFI publishers keep hang's 30 s.max_delay + warmupand joins at the live edge. Reproducing relay-sideOldresets needs real transport backpressure, so the test doesn't do that.Checks
just check(rs, js, markdown, gh, shell, quest, ...): passed, 1534 nextest tests.just fix: no changes.just test interop --publishers rust,python,go,cpp --subscribers rust,python,go,cpp,c,js-native-node,gst: all checks passed (122).mainagain:cargo nextest run -p moq-audio -p moq-cpassed (332 tests), andquest checkpasses.Suggested follow-ups
moqsrcbudget.rs/moq-gst/src/source/imp.rssubscribes every rendition with a hard-coded 1 s max delay, and thegstcells run tone.py's contiguity check. So a starved GStreamer subscriber can fail the same way, and there's no knob to widen it. The fix is either amax-delayproperty onmoqsrcthatinterop.shsets to 5 s, or deriving the budget from the track's max-age. This is a public element property, so it's the maintainer's call.Oldresets within its budget. It would guard the relay path this bug went through, beyond the subscription-budget invariant tested here. It could use the moq-net mock harness with a capped stream count.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code