Repository navigation
fix(web-voice): play agent audio on a dedicated AudioContext at the stream rate - #4240
Conversation
…tream rate The browser transport created one AudioContext at the device rate (48 kHz on Windows) and played each 24 kHz PCM chunk through an AudioBufferSourceNode whose buffer rate differed from the context rate. Chromium and WebKit resample that case with linear interpolation, which mirrors every component above a few kHz to (24 kHz - f): a WASAPI loopback capture of the speakers shows 16-20 kHz energy at -10..-16 dB re total during every sibilant, while the source PCM has -66 dB there. Listeners who can hear 17-19 kHz perceive it as a "beep" a few seconds into every answer. Playback now runs on its own AudioContext created at the configured output rate (24 kHz by default), so the buffer and context rates match and the browser's band-limited device resampler does the only conversion. The capture context and the recovery FSM are untouched. Where the constructor rejects the rate the transport falls back to the device rate with a debug warning, and a one-time warning marks any remaining buffer/context rate mismatch. A session.config that renegotiates the output rate retires the playback context so the next chunk plays at the new rate. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TqRPjsrR2kmfcWwX1uAb8H
be795a7 to
16c87b0
Compare
Coverage Gate✅ No Python changes — nothing to gate (bar: 95% on changed lines). |
|
Reviewed at head The bug and the fix are real. Playback was sharing the capture Verified myself, not just trusting the tests:
Worst-case disruption: this can only improve the existing behavior (mismatched-rate interpolation happens today, unconditionally) — the fallback path when the browser rejects the requested rate reproduces today's exact behavior (device-rate context), so there's no new failure mode, only a removed one. I RECOMMEND APPROVE #4240 — Bassil Khilo (bassilkhilo-ag2), fixing a real audio-quality bug in voice playback with a well-scoped, well-tested change. Not casting a formal review (needs current owner authorization). No merge, approval, or restart action taken. Stand: Echo Act IV Pro |
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE at 16c87b0bd — first review on this. The diagnosis is right, the fix is the correct one, and the reasoning in the body is the kind I wish more audio bugs got. One finding below that I think matters more than its size suggests.
The diagnosis holds
A 24 kHz AudioBuffer played in a 48 kHz context is resampled per AudioBufferSourceNode by linear interpolation, and linear interpolation without an anti-imaging filter leaves images at fs − f. For a 24 kHz source that puts a sibilant's 4–8 kHz energy back at 16–20 kHz — audible to anyone whose hearing reaches there, inaudible to everyone else, which is exactly the shape of "two Windows testers hear a beep and nobody else does." Matching the context rate to the buffer rate removes the interpolation entirely rather than masking it.
ctx.createBuffer(1, f32.length, this.outputRate) inside a context constructed at outputRate is the whole fix, and the rest is the care around it: a constructor fallback with a warn, a one-time mismatch warn if it still diverges, retirePlaybackCtx() on a renegotiated session.config, and closing the new context on teardown. The analyser follows playback correctly — playChunk builds it against ctx and retirePlaybackCtx nulls it so it is rebuilt against the successor.
The finding: playback no longer has a state-change handler
adoptCtx() installs onstatechange, and handleCtxStateChange opens with:
if (ctx !== this.audioCtx) return; // stale contextBoth adoptCtx call sites pass the capture context — :829 and :1461 (const ctx = this.audioCtx!). playbackCtx is never adopted. So once playback moves to its own context:
ctxSuspendCountandctxLastTransitionno longer observe the playback context at all.- The only thing that notices a suspended playback context is
if (ctx.state === 'suspended') ctx.resume();at the top ofplayChunk— which runs only when the next chunk arrives, and is not awaited.
Before this PR playback shared audioCtx, which was adopted, so a suspension on the context carrying playback was counted. That coverage is gone, and it is gone for the failure mode this PR is adjacent to: playback going silent.
I checked the suite — the suspension tests drive FakeAudioContext.created[0], which is the capture context, so nothing exercises a suspended playback context.
Why I am not blocking: the user-facing bug is real and this fixes it, the gap is observability rather than correctness, and holding a working audio fix for missing telemetry would be the wrong trade. But this.adoptCtx(this.playbackCtx) at creation, plus widening that identity check to accept either context and tagging which one transitioned, is small and worth doing soon.
Sharper half: the stats object now spans two contexts, labelled as one
ctxState: this.audioCtx?.state ?? null, // capture
ctxTimeMs: this.playbackCtx ? … : null, // playback <- changedThe heartbeat legend was updated for one of these (x [playback ctxTimeMs…]) and not the other — cs still reads "ctx state initial", and sc ctxSuspendCount is now capture-only without saying so.
That is a trap for whoever debugs the next audio incident: cs=r beside a stalled x[0] now means "capture is running and playback is not", which reads as a contradiction rather than a diagnosis. And "capture suspended while playback runs" has become a normal state that the field names do not admit. Renaming to captureCtxState / playbackCtxTimeMs, or reporting both states, would cost a line and remove the ambiguity permanently.
CI: 18 check-runs SUCCESS, 1 entry without a conclusion (license/cla, which reports on commits/<sha>/status rather than the rollup — worth confirming there rather than reading the rollup as pending).
|
@cla-assistant check |
john-the-dev
left a comment
There was a problem hiding this comment.
APPROVE — re-affirming at the current head. Stale by timestamp; the content is unchanged.
18 commits arrived since my approval, which looks alarming until you separate the PR's own work from main flowing in. I fingerprinted this PR's own diff (merge-base → head) at both heads:
src/web-voice-transport.ts— identicaltests/web-voice-transport.test.ts— identicalCHANGELOG-PENDING.md— flagged as changed by my first pass, then not: the added line is byte-identical, and only the surrounding context lines moved because other entries landed above it frommain.
Worth recording that correction, since it is a trap for anyone doing this check: hashing the whole patch counts context drift as a content change. The +/- lines are the right granularity, and by that measure this PR has not moved since I read it.
Everything else in those 18 commits is main — #4230, #4235, #4238, #4244, #4245, #4248, #4249, #4252, #4253, #4131 and friends, which I reviewed individually.
My original finding stands and is still non-blocking: playbackCtx is never passed to adoptCtx, so its suspensions are invisible to ctxSuspendCount/ctxLastTransition, and the stats object now reports capture state beside playback clock under one legend.
|
Re-checked at the new head
Nothing material moved since my first pass. Verdict stands: ready to merge on the bar (currently 1/2 standing approvals — @john-the-dev's — short by one). |
yixuan-ag2
left a comment
There was a problem hiding this comment.
Read the diff and the surrounding transport at 3204576ec205. The change is right, and the part I went looking for a bug in is the part that is best covered.
What I actually checked, rather than the diff's own story. Splitting one AudioContext into two retires an invariant the old code stated out loud — "playback and capture share one clock" — so my question was what else depended on it.
- The analyser follows playback, correctly. It is created inside
playChunkagainstctx(nowplaybackCtx), connected toctx.destination, and re-handed throughthis.ev.onAnalyser?.(...)on every recreation. It visualises the agent's audio, so playback is where it belongs, andretirePlaybackCtx()nulls it so the next chunk rebuilds it against the new context. - Stats.
it('stats report the playback clock, not the capture clock')— that is the consequence of the split I would most expect to be missed, and it is the one you tested. - Teardown symmetry. I suspected the retire path flushed while the disconnect path did not, since only
retirePlaybackCtxnamesflushPlayback()in the diff. Wrong: the teardown callsthis.flushPlayback()before closing either context, so thecancelled/D7.1 bookkeeping holds on both paths. Checking beat publishing the suspicion.
The lazy-recreate path is genuinely lazy in the right places. connect() swallows a construction failure and leaves playbackCtx = null for playChunk to retry; createPlaybackContext() falls back to the device rate on NotSupportedError and says which happened; rateMismatchLogged latches so a degraded session warns once rather than per chunk. The test asserting one mismatch line across two chunks is what makes that latch a property rather than an intention.
Pair I'd point at if anyone reads only one thing: session.config with a new output rate retires the playback context alongside session.config confirming the same output rate keeps the playback context. A retire-on-config test alone would pass for code that retired unconditionally; the same-rate case is what makes if (this.outputRate !== prevOutputRate) load-bearing.
pr-checks.py rc 0 at this head: 18 distinct checks from 18 entries, 0 superseded, nothing failing, nothing running.
Scope, so this is not read as more than it is: I read web-voice-transport.ts and the new tests. I did not run the suite and did not exercise browser audio — every claim above is from the tree, and a rate-interpolation fix ultimately wants an ear on it, which I do not have.
What changed, and why?
The browser voice transport played Gemini's 24 kHz PCM through one
AudioContextcreated with plainnew AudioContext(), i.e. at the device rate (48 kHz on Windows). Each chunk became anAudioBufferat 24 kHz inside a 48 kHz context, and Chromium/WebKit resample that case perAudioBufferSourceNodewith linear interpolation, which mirrors every component above a few kHz to24 kHz − f. Every "s"/"t" in the agent's speech therefore came with a 16–20 kHz image only ~15 dB below the sibilant. Listeners who can hear that range (two Windows-laptop testers so far) perceive it as a short "beep" a few seconds into every answer.Fix: playback now runs on its own
AudioContextcreated at the configured output rate (24 kHz by default), so buffer and context rates match and the only conversion left is the browser's band-limited device resampler. The capture (microphone) context and the D7.5 recovery FSM are untouched. If the browser rejects the rate, playback falls back to the device rate with onewarndebug line; a one-timewarnmarks any remaining buffer/context rate mismatch; asession.configthat renegotiates the output rate retires the playback context so the next chunk plays at the new rate; theaudio_healthheartbeat's playback-clock evidence now reads the playback context.Surfaced while testing #1789 (Windows support) on Windows 11, but the code is identical on
main(git diff origin/main..pr-1789 -- src/web-voice-transport.tsis empty). This PR is independent of #1789; #1789 inherits it on rebase.Review first
src/web-voice-transport.ts—playbackCtx+createPlaybackContext()/retirePlaybackCtx(), theconnect()block,playChunk(),teardownAudio(), thesession.configbranch,buildStats()/ heartbeatx[0].tests/web-voice-transport.test.ts—FakeAudioContextnow honours{ sampleRate }and can reject it; new describe "playback runs on a dedicated AudioContext at the stream rate" (12 tests); one existing ledger test now indexescreated[1]because buffer sources live on the playback context.CHANGELOG-PENDING.md.What user behavior or bug does this prove?
Agent speech in the web client on a 48 kHz output device (every Windows machine; many Macs) no longer carries 16–20 kHz imaging on sibilants — the "beep" is gone. Measured below.
Feature/documentation consistency
N/A — no user-visible feature, command or config changes;
docs/does not describe the transport's audio-graph internals.Before/after evidence
1. Live measurement — what the speakers played vs the PCM the server sent
Method: the voice agent's outgoing PCM was dumped to disk (24 kHz int16, per chunk, with arrival timestamps) and the default render endpoint was captured with a WASAPI loopback recorder (48 kHz) while talking to the agent in the web client. Each utterance's capture was aligned to its source PCM by cross-correlation; over all 50 ms frames where the source is a sibilant (>90 % of energy above 1.5 kHz), the median band energy relative to the frame total. Windows 11, Chromium 152 (Edge/Chrome), Realtek speakers.
Before: 16–20 kHz at −14.7 dB on the speakers while the source has −66 dB there (a simulation of 2× linear interpolation on the source reproduces the speaker spectrum within ~1 dB in every band, which is how the cause was pinned). After: −78 to −86 dB — below the source's own noise floor, because the browser's device resampler is band-limited.
Listening A/B on the same 200 ms sibilant region: the tester heard the "beep" in the speaker capture and not in the source PCM played directly.
2. Unit tests — fail at the parent source, pass at HEAD
The one remaining failure (
bounded fallback: a wedged self-close handshake…) is pre-existing and Windows-environmental — it fails identically on the parent with the parent tests; CI on Linux/macOS is the authoritative signal for it.What tests did you run?
npx tsx --test --test-force-exit tests/web-voice-transport.test.ts tests/web-voice-transport-delivery.test.ts— see above (the delivery file's two serve-path subtests fail on this Windows host withspawn npx ENOENT, also pre-existing).npm run typecheck→tsc --noEmitexit 0.npx eslint src/web-voice-transport.ts tests/web-voice-transport.test.ts→ 0 errors (pre-existingno-explicit-anywarnings in the test file only).bash scripts/review-checks.sh --diff <(git diff origin/main...HEAD)→hardcoded-paths + root-artifacts clean(prose-caphad no in-scope.pyfiles).Live path — real post-restart round trip (voice, Windows 11)
The web client was restarted on the fixed transport (
/web-voice-transport.jsnow servescreatePlaybackContext), the page hard-reloaded, and the same math questions asked as in the "before" session.data/conversation.sqlite,voicetable, sessionsession_1789293804655:The tester reported no beep in this session; the three "AFTER" utterances in the measurement above are from it. Mic capture, barge-in and the avatar analyser behaved as before.
Edge cases / non-happy paths checked
{ sampleRate }→ no-arg fallback, onewarn, session goes live, playback continues (interpolated, as today) with exactly one mismatchwarnper attempt.suspended→ resumed on the next chunk;closed→ recreated at the stream rate on the next chunk.session.configwith a differentoutputSampleRatewhile chunks are draining → sources flushed, old context closed, analyser re-handed to the surface on the next chunk; same rate or absent rate → context kept (a device-rate fallback context is not churned by a confirming config).connect()→ attempt still goes live withplaybackCtx = null;playChunkretries lazily.disconnect()/ teardown closes both contexts; the D7.5 fence ordering for the capture context is unchanged.Worst-case disruption to existing users, and mitigation
AudioContextper voice session (capture + playback + the page's tool-cue context). Created inside the same user-gesture call stack as before; iOS/WebKit context limits are far above three.AnalyserNodethat now lives on a 24 kHz context (bins span 0–12 kHz instead of 0–24 kHz), so its fixed threshold may trigger slightly more eagerly — cosmetic; looked normal in the live session.speech_speed≠ 1.0 (playbackRate≠ 1) still makes the browser interpolate per chunk; the measurement above is at unity speed.Known gaps / follow-ups
warnincreatePlaybackContext()is not latched per attempt (only reachable if both constructor forms throw repeatedly).🤖 Generated with Claude Code
https://claude.ai/code/session_01TqRPjsrR2kmfcWwX1uAb8H