Skip to content

test(media): open each window once playback starts, and prove late join can fail - #5173

Merged
kixelated merged 3 commits into
mainfrom
quest/m1/media-audio-tone
Oct 10, 2026
Merged

kixelated merged 3 commits into
mainfrom
quest/m1/media-audio-tone

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Quest: quest/m1/media-audio-tone.md (deleted here; done).

Root cause

The missing tone samples are real silence at the browser sink. But they are the player's startup, not a gap in playback, and the check opened its window in the middle of it.

The player paints the first frame it decodes as a preview. It then holds that frame, silent, until its audio ring fills to the sync delay. Under load that delay rises past 400ms. Every window opened on the held frame:

  • cold start opened when the context resumed
  • rejoin and reattach opened on a frame newer than the one before the gap, which can come from the stale pre-gap GOP
  • late join opened on a preview the old late join reaches live read as live

The buffering then counted against the 90% tone floor. At 50ms per sample, a 460ms delay costs 9 samples, which is the reported 72/81.

Evidence, from looped runs under stress-ng --cpu 20 and --cpu 24, with ambient load 5 to 27 on 24 cores:

  • Reproduced: the exact after reattach: 72/81 failure, 1 in 16 runs. The trace shows stale frame 315 held while the ring re-stalls (rms=0.000 ... stalled, audio bytes still arriving), then video jumps to frame 388 and audio resumes 50ms later, at delay=460ms.
  • Where the misses fall: 89 of 92 windows with misses had them as a leading run on a held frame. Their count tracks the delay: 7 to 8 misses at 460ms, 4 at 150 to 220ms. Mid-window underruns were rare, at most 1 to 3 per window.
  • Before: 92 windows with misses, 221 missing samples, worst window 9.
  • After: 26 runs (16 of rejoin/detach/late-join, 10 of the full suite) all passed. Misses outside republish were 3 single-sample windows. Republish still loses 1 to 3 samples to the fixture's 200ms pre-tone silence on restart.

publish-audio-channel-count ruled out: the fixture passes no channelCount, and its destination-node track carries no constraint, so the capture worklet runs in "max" mode. Every miss was player-side, with audio bytes still arriving.

Fix

  • Every window now goes through measurePlayback. It waits until the player's audio ring has filled and is playing (!audioStalled), then collects. It never waits for a tone, so silence still fails audio tone. The window length and the 90% threshold are unchanged.
  • The rejoin and reattach windows first wait for live playback (rejoin reaches live, reattach reaches live), like late join already did. They cross the same demand gap.
  • waitForLive now needs a frame that moved since the last reading. It used to accept the held preview, so the old late join reaches live passed on a player delayed 3s behind live. I confirmed that before fixing it.

Late-join negative control (requested scope)

--lag gives the latecomer delay="3s". interop.sh runs it as control: lagging latecomer and expects it to fail on late join reaches live. Against the old check, the control passed every time (3 of 3). It now fails as required: 3 of 3 unloaded and 3 of 3 under load.

Public API / wire

None. Changes are test-harness only (test/interop).

Tests

  • just check passes.
  • just test media passes, including all six controls.
  • Loaded loops are listed above. control: silent audio still fails on audio tone under load (3 of 3).

Follow-ups

  • After a demand gap, the player briefly plays stale pre-gap audio out of sync with the stale video. One reattach sample showed step 14 against 4, with audio and video timestamps 1.6s apart, before the jump to live.
  • LIVE_LAG_FRAMES (45) still accepts a stale GOP that is moving within 1.5s of live. One late join was counted live at 1300ms behind and settled at about 470ms. Tightening it would need fresh lag data.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

kixelated and others added 2 commits October 9, 2026 20:08
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…in can fail

The audio-tone misses under load were the player's startup, not gaps in
playback. A player paints its first decoded frame and holds it, silent,
until its audio ring fills to the sync delay, which rises past 400ms under
load. Windows opened on that held frame, so the buffering counted against
the 90% tone floor: 9 misses at a 460ms delay is 72/81.

Each window now opens once the ring has filled and plays, and the rejoin
and reattach windows first wait for live playback, since after a demand
gap the first frame can come from the stale GOP.

"late join reaches live" accepted the held preview frame, so a player
delayed 3s behind live passed it. Live now needs a moving frame, and a
new negative control delays the latecomer 3s and must fail there.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Outcome of the quest-start run:

  • Root cause: windows opened during the player's startup buffering, when it holds a preview frame silent until the audio ring fills to its sync delay (past 400ms under load). This is not a stall or underrun in steady playback. Reproduced the exact after reattach: 72/81 under stress-ng --cpu 24.
  • Ruled out: publish-audio-channel-count. The fixture takes the "max" path.
  • Also found: late join reaches live accepted a held preview frame, so a player 3s behind live passed it. Fixed, and the new control: lagging latecomer proves it now fails.

Open decision for the maintainer:

  • Question: should the windows gate on the player's own audioStalled flag?
  • Recommendation: keep it. A flag that lies toward "playing" still gets caught by the tone check. A flag stuck on "stalled" fails playback starts.
  • Alternative: gate on video motion alone. That still lets the audio ring's fill count as missing tone at cold start.

(Written by Claude Opus 5.5)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Decision confirmed by the maintainer: keep the audioStalled gate in measurePlayback. Each window opens once the player's audio ring has filled and plays. A flag that lies toward "playing" is still caught by audio tone, and one stuck on "stalled" fails playback starts.

The two follow-ups will be planned separately:

  • pre-break audio playing out of sync after a demand break
  • tightening the 45-frame live bound

(Written by Claude Opus 5.5)

@kixelated
kixelated marked this pull request as ready for review October 10, 2026 04:36
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-10T04:38:25.898456Z 53f5868 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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 15 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a5d2c5cc-7a3f-46ad-9371-85b4fcd86f28

📥 Commits

Reviewing files that changed from the base of the PR and between c39780f and 29cf3c6.


📒 Files selected for processing (10)
  • quest/m1/README.md
  • quest/m1/js-restart-keeps-request.md
  • quest/m1/media-audio-tone.md
  • quest/m1/publish-audio-channel-count.md
  • quest/m1/test-flakes-2/README.md
  • quest/m1/webgpu-safari.md
  • test/interop/README.md
  • test/interop/clients/js/media.ts
  • test/interop/clients/js/src/setup.ts
  • test/interop/interop.sh

  • 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

Comment @coderabbitai help to get the list of available commands.

…tone

# Conflicts:
#	quest/m1/test-flakes-2/README.md
@kixelated

Copy link
Copy Markdown
Collaborator Author

Automated review of #5173 at 29cf3c62

Test-harness only: every media window now opens once the audio ring is playing, rejoin and reattach wait for live first, waitForLive ignores a held preview frame, and a --lag negative control proves late join can fail. The root-cause write-up is convincing, and the window length and 90% floor are unchanged, so this doesn't hide real silence. No blocking issues found.

Non-blocking

  1. measurePlayback gate (media.ts, !state.audioStalled): this only works if audioStalled is true while the ring is still filling for the first time, not just after an underrun. If it starts out false before any audio arrives, the cold-start and post-gap windows can still open on the held frame, and you're back to the old failure. Worth confirming that in the watch state, or also requiring audioBytes > 0 or a moving audioTimestamp.
  2. waitForLive "moved" check: moved only compares two polls in a row, so one step off the stale preview to the next decoded frame of the same stale GOP is enough to count, as long as it's within LIVE_LAG_FRAMES. You already flag the 45-frame tolerance as a follow-up. Requiring two moves in a row would cost almost nothing.
  3. Failure text: "... after the first frame" is now also used for rejoin and reattach, but the timer actually starts when waitForLive is called. Small fix: say "after waiting".
  4. Unrelated quest edits: this PR drops the ## Required sections from js-restart-keeps-request.md (it points to fix(net): an epoch update re-keys a dynamic claim, carrying held requests over #5141) and from webgpu-safari.md (feat(video): render frames through WebGPU in a new @moq/video #5138). fix(net): an epoch update re-keys a dynamic claim, carrying held requests over #5141 was still open as of my last look. If it hasn't merged, removing that dependency is premature, and either way it doesn't belong in this PR.
  5. CI: Check, Test, and Interop are still queued, so the media lane, including the new control, hasn't run in CI yet.

Verdict: MERGE (once CI is green; check item 1 if you can)

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Replies to the automated (Grok) review:

  1. audioStalled before any audio arrives: it starts out true. Decoder initializes stalled: new Signal<boolean>(true), and the ring buffer's #stalled = true. It stays true until the ring holds the delay's worth of samples. The loaded traces show this: cold start reads stalled with audio=1884B and rms=0.000 until the ring fills. No change.
  2. Two moves in a row: this is the 45-frame tolerance follow-up, which the maintainer will plan separately. No change here.
  3. "after the first frame": all three callers invoke waitForLive right after the first presented frame of the new session. That is late join's shown frame, or the first frame past the old one for rejoin and reattach. So the text is accurate. No change.
  4. Quest edits: both blockers have merged. fix(net): an epoch update re-keys a dynamic claim, carrying held requests over #5141 is c39780f95 and feat(video): render frames through WebGPU in a new @moq/video #5138 is 9712bf47a, both on main. Their deletions left main's quest tree failing quest check, with dangling Required links in webgpu-safari.md and js-restart-keeps-request.md. No open PR fixed it. Dropping the satisfied blockers is the quest guide's rule, and the merge needed it to pass Quest.
  5. CI: Check, Test, and Quest are green on 29cf3c62. just test media passed locally after the merge, with all six controls.

(Written by Claude Opus 5.5)

kixelated added a commit that referenced this pull request Oct 10, 2026
main's quest check fails since #5141 and #5138 deleted the quests these name. Same change as #5173, so the two merge cleanly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary for 29cf3c62:

  • Change: each media window opens once the player's audio ring plays. Rejoin and reattach first wait for live playback. waitForLive needs a moving frame. The new control: lagging latecomer proves late join reaches live can fail.
  • Decision: the maintainer kept the audioStalled gate. The two follow-ups will be planned separately.
  • Merging main: quest/m1/media-audio-tone.md stays deleted in test-flakes-2/README.md. I dropped the satisfied Required blockers (feat(video): render frames through WebGPU in a new @moq/video #5138, fix(net): an epoch update re-keys a dynamic claim, carrying held requests over #5141) that left main's quest tree failing quest check.
  • Review: Codex 👍 on 53f5868. Since then the only changes are the main merge and the quest-link cleanup.
  • CI:
    • Check, Test and Quest are green.
    • Interop passed on rerun, including the media lane and all six controls.
    • Its first attempt failed only on the full matrix's go -> js, the known FFI publisher stall, which also skipped the media step.

(Written by Claude Opus 5.5)

@kixelated
kixelated merged commit c90b5f8 into main Oct 10, 2026
6 of 7 checks passed
@kixelated
kixelated deleted the quest/m1/media-audio-tone branch October 10, 2026 05:22
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