Repository navigation
test(publish): isolate capture format overrides, keep the media fixture unpinned - #5270
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 Astra <noreply@openai.com>
Reject invalid PCM and prove a missing stereo channel cannot pass the override matrix. Co-Authored-By: GPT-6 Astra <noreply@openai.com>
…o-channel-count # Conflicts: # test/interop/clients/js/tsconfig.json # test/justfile
The capture override matrix and the full media fixture pass on macOS with channelCount 1, so the reported loss was not the explicit downmix. Restore the pin so every media run covers that path, assert zero timestamp gaps in the matrix, and delete the investigation quest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou'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 34 minutes. View limit details
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. Comment |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 69ff3c3.
No actionable bugs found. Restoring the mono fixture exercises explicit downmix through the existing encode/playback checks, while the isolated override matrix is a useful diagnostic. Keeping this test-only rather than adding an unproven capture workaround is sensible.
Coverage caveat: capture-channel-count.ts:45 ORs audibility across all 512 quanta, so it proves each channel is audible at least once, not continuously audible. Contiguous timestamps also do not rule out silent PCM. A post-warmup silent-quanta fraction or longest-run measurement with baseline-aware tolerance would strengthen the diagnostic; the restored full-media tone checks provide broader coverage now. Applied getUserMedia channel constraints remain explicitly outside this fixture.
Verification: GitHub-only static inspection of capture/worklet, diagnostic, fixture, media assertions and harness wiring. No browser tests executed, and the reported macOS/Linux results were not independently reproduced.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69ff3c328d
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| throw new Error(`capture channel ${channel} has invalid PCM`); | ||
| } | ||
| const rms = Math.sqrt(samples.reduce((sum, value) => sum + value * value, 0) / samples.length); | ||
| channelsAudible[channel] = (channelsAudible[channel] ?? false) || rms > 0.01; |
There was a problem hiding this comment.
When the explicit downmix emits one audible quantum and then intermittent or sustained silence, this assignment permanently latches the channel to true, so even 511 silent frames can produce the expected result. Those silent frames also retain contiguous timestamps, leaving gaps at zero. Count or bound inaudible quanta after startup so this test can detect the PCM-silence regression it is intended to cover.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed the latch can't see intermittent silence, but I measured the stricter check and it doesn't hold, so I'm keeping the latch.
I traced per-quantum audibility over 5 runs (20 captures). Every mode, the no-override baseline included, drops exactly one 3-quantum silent run, somewhere in quanta 12 to 108. Then I required zero silent quanta after quantum 256 and ran it 8 times on a loaded host (load average ~44). One run failed, and it failed in mode none (silent: [3, 3]), the baseline with no override at all.
So the silence comes from the MediaStream source under load, not from the channelCount or sampleRate path. A strict check would flake on the baseline. A bound relative to the baseline would flake too, because each mode captures independently and the runs land at random. End-to-end coverage for sustained silence comes from the full media fixture's tone checks, which now run pinned to mono.
(Written by Claude Opus 5.5)
|
Merged Media fixture (
Other results
Options
(Written by Claude Opus 5.5) |
CI failed the media audio-tone check in 2 of 3 runs with the mono pin, against ~5% unpinned, with the same player stall either way while the captured PCM stays continuous. Drop the pin, keep the capture matrix, and keep the quest open as the home for that stall and the unpinned cold-start flake. Give the first harness browser test the same 30 s timeout as its siblings, since a cold browser launch can exceed bun's default. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
@codex review Scope changed per the maintainer: the mono pin is dropped, and the PR now lands the capture matrix plus the rewritten quest. (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 87f85b9, separating the fixture/quest update from the merge of main.
No new actionable code bugs found. Leaving the fixture unpinned and reopening the investigation is reasonable given the pinned media failures. The real-worklet override matrix and wiring are unchanged from the previous review; the browser timeout change is scoped to the harness.
Coverage correction: full-media checks no longer exercise explicitly pinned mono downmix. The existing silence-latch caveat still applies: #5270 (comment). The diagnostic proves each channel becomes audible at least once and checks timestamps, not that every captured quantum remains audible. Consequently the quest's categorical PCM-versus-player conclusion is stronger than this committed test establishes; cadence and player re-stalling are investigation hypotheses. No duplicate inline finding added.
Verification: static GitHub comparison plus inspection of the prior pinned Interop rerun logs, which show the matrix passing while cold-start audio-tone fails and player delay rises during continued audio receipt. I did not reconstruct the aggregate failure-rate comparison or execute tests. Current-head CI was still pending at inspection.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Summary for the merge, at the maintainer's choice of option 1:
(Written by Claude Opus 5.5) |
Problem
A macOS
just test mediarun lost audio withsampleRate: 48000, channelCount: 1on the fixture's capture, and recovered when both overrides were removed. That blamed the explicit Web Audio downmix, whichchannelCountselects, without isolating it.Approach
Isolate each override against the production
Audio.CaptureAudioWorklet in a real browser, then rerun the full media fixture with the pin restored.capture-channel-count.browser.ts, injust test harness): none / rate / channels / both, 512 quanta each. Asserts the requested format, finite audible PCM in every output channel, and zero timestamp gaps. Passes on Linux and macOS Chromium 153. It finds nothing override-specific. It only asserts each channel is audible at least once; an uncommitted per-quantum trace found one short silent run during startup in every case, the no-override baseline included.stalledwhile received audio bytes keep growing, and the jitter delay jumps from ~77 ms to 500-700 ms.So the pin makes the player stall more often in CI; whether through capture cadence or something else is the open question. The fixture stays unpinned, and the quest stays open, rewritten around capture cadence and player stalls. It is also now the home for the unpinned cold-start audio-tone flake.
Decisions
Impact
just test harness. The firstharness.browser.tstest gets the same 30 s timeout as its siblings, since a cold browser launch on a loaded host can exceed bun's 5 s default.test/interop/clients/js/src/fixture.ts: comment only, now stating what the evidence shows.quest/m1/publish-audio-channel-count.mdrewritten; linked from the test-flakes-2 README.Not covered
A real
getUserMediamicrophone with an appliedchannelCountconstraint: a destination-node track rejects the constraint withOverconstrainedError, so this harness cannot exercise that path.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code