fix(desktop): remove the voice mic re-arm delay - #54067
Conversation
alt-glitch
added
type/bug
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Removes voice mic re-arm delay and gap between TTS sentences in the desktop voice conversation. Key changes: (1) keeps mic stream alive between turns instead of releasing/re-requesting, (2) pre-synthesizes upcoming TTS chunks for gapless playback, (3) re-arms mic immediately after speaking instead of waiting for re-render.
Changes
apps/desktop/src/app/chat/composer/hooks/use-mic-recorder.ts: Soft cleanup (keep stream) vs full release, AudioContext rebuilt per turnapps/desktop/src/app/chat/composer/hooks/use-voice-conversation.ts: peekUpcomingChunks lookahead, prefetchSpeechText pre-synth, immediate mic re-armapps/desktop/src/lib/voice-playback.ts: Speech prefetch cache (Map with size bound)
Looks Good
- Keeping stream alive eliminates the costly getUserMedia round-trip
- AudioContext intentionally rebuilt per turn (browser suspends idle contexts)
- Prefetch cache bounded at 6 entries to prevent accumulation
- 600ms timeout on canplaythrough prevents blocking on flaky codec decode
- Immediate startListening() eliminates re-render lag
Reviewed by Hermes Agent
Linux 测试结果之前 Cherry-pick 了 测试环境
结果
说明修复前的 mic re-arm 延迟（3-4 秒甚至 8-9 秒）在 5 轮中均未复现。两句之间的间隙也正常，无停顿感。 |
|
Thanks for tracing this through the desktop voice loop. The reported behavior is still present on current main: Problems
Suggested changes
This is an automated hermes-sweeper review. |
teknium1
added
sweeper:risk-compatibility
Every normal cleanup stopped the MediaStream tracks, so each turn re-ran `requestMicrophoneAccess` + `getUserMedia`. That round-trip is most of the pause after the assistant stops speaking. `stop()` now drops the MediaRecorder and the audio graph but keeps the stream open, and the next turn arms off it. Cancel, unmount and a recorder error still release the tracks, and a retained stream whose track has ended is re-requested rather than reused. The AudioContext is still rebuilt per turn on purpose: the browser suspends an idle context, after which the analyser reads silence and speech detection stops without a word. Five contracts cover the split. The retention one fails on main; the four release ones pass there, so they hold the existing teardown still. Consequence worth knowing: the OS microphone indicator now stays lit for the whole conversation instead of flickering off between turns, and a device change takes effect on the next release rather than the next turn.
Once the whole reply had been spoken the effect set `pendingStartRef`, called
`setStatus('idle')` and returned. `status` is already 'idle' at that point,
because the last speak() set it in its finally, so the setState is a no-op, React
bails out, and the re-render that would run this effect again never happens. The
restart guard at the bottom is only three lines below, and nothing reaches it
until some unrelated render arrives. That was the 4-8s wait before recording
resumed, and the variance is other components re-rendering.
Dropping the early return lets the same pass fall through to the guard, which
already checks `busy` and `status` before starting.
NousResearch#38411 diagnosed this first, and independently. Its fix removes the same return
but also removes the `setStatus('idle')` calls, which the sibling paths rely on:
where `status` is still 'thinking' the setState is a real change, and the
re-render it triggers is what reaches the guard. Removing it there strands the
hook in 'thinking' with a dead mic, so this keeps both calls and only drops the
one return that is unreachable-by-design.
Three contracts, driven through the real path rather than by setting refs. The
first fails on main. The two covering the sibling paths pass on main and are here
to stop a fix trading one stall for another.
Two things the splitter got wrong, both audible on a local backend. A short leading fragment was voiced on its own. A list marker like "1." became its own synth call, which sounds clipped and pauses before the item it introduces. Chunks now accumulate to MIN_SPEAK_CHARS, so a marker rides along with its text. Sentences ending in a number were split mid-number. The buffer here is still growing, so a `.` at its end doesn't mean the sentence ended: "Revenue grew to 1." was spoken as "Revenue grew to one", with "2 billion." following as its own chunk. `!?。！？` and a `.` after a non-digit may still end at the buffer end, while a `.` after a digit waits for whitespace to prove the number is finished. That costs one character of latency on "grew 12." and keeps "1.2" whole. The splitter is pure, so it moves to module scope, out of the hook body where it made the loop effect depend on two functions rebuilt every render. The contracts stream the reply in a character at a time and collect what would be spoken, because passing a finished string hides both bugs: a decimal is only at risk while the text is still arriving.
94d2e59 to
f58f229
Compare
laurinaitis
changed the title
|
Tests are in, and the reconciliation went further than expected: this PR now stops at the microphone. #44308. I should have found it before filing and didn't. It's been open since 11 June rewriting What's left here is the part nobody else touches: the mic. Three commits. The re-arm. Once the reply is fully spoken the effect set The stream. Every cleanup stopped the MediaStream tracks, so each turn re-ran The splitter, which is where the audible pauses came from: a bare list marker was voiced as its own synth #55581, and the answer isn't the one I expected. Its #38411 got to the re-arm three weeks before me and diagnosed it correctly. Its fix removes the same Tests. 19 contracts: the re-arm paths (3), the mic stream lifecycle (5), and the splitter (11). The Two things worth a sanity check. Keeping the stream open means the OS microphone indicator stays lit for the Rebased onto |
alt-glitch
removed
the
sweeper:risk-compatibility
Related to #38411: both address the mic re-arm path, while this PR also adds separate MediaStream reuse and streamed speech chunking. It is therefore a broader competing repair rather than a duplicate. Playback-specific latency work is tracked separately in #44308. |
|
Closing as superseded by the merged voice playback/barge-in rework #69511 ( If anything in this area still misbehaves on current main, please open a fresh PR against the new stack. |
5 participants
What
Three fixes to the desktop voice loop, all on the microphone side.
getUserMedia.Why
Two separate delays after the assistant stopped talking, both measured on macOS with a local
Kokoro + Whisper backend.
The effect never re-ran. Once the reply was fully spoken, the loop set
pendingStartRef, calledsetStatus('idle')and returned.statusis already'idle'at that point, because the lastspeak()setit in its
finally, so the setState is a no-op, React bails out, and the re-render that would run the effectagain never arrives. The restart guard is three lines below the return. Nothing reaches it until some other
component happens to render, which is where the 4-8s wait and its variance came from.
Every turn re-acquired the microphone. Cleanup stopped the stream tracks on every normal stop, so the
next turn paid
requestMicrophoneAccess+getUserMediaagain.The splitter mishandled short fragments and numbers. A bare list marker like
1.was voiced as its ownsynth call, which sounds clipped and pauses before the item it introduces. And because the splitter runs on a
buffer that is still growing, a
.at the end of the buffer doesn't mean the sentence ended:Revenue grew to 1.matched, so the reply was spoken as "Revenue grew to one" followed by "2 billion."Notes
The AudioContext is still rebuilt every turn on purpose. The browser suspends an idle context, after which
the analyser reads silence and speech detection stops without any error.
Keeping the stream open has two visible consequences: the OS microphone indicator stays lit for the whole
conversation rather than flickering off between turns, and a microphone device change takes effect on the
next release instead of the next turn. Cancel, unmount, mute and recorder errors all still release the
tracks.
#38411 diagnosed the re-arm first and independently. Its fix removes the same early return, and also removes
the
setStatus('idle')calls; the sibling paths rely on those, so removing them strands the hook in'thinking'with a dead microphone. This keeps both and drops only the one return that can't be reachedusefully. Evidence and a correction are on that PR. If it merges first, the re-arm commit here comes out.
voice-playback.tsis deliberately untouched. #44308 has been rewritingplaySpeechTextfor the samelatency goal since 11 June; the prefetch work that used to be in this PR is offered there instead.
Testing
19 contracts.
use-voice-conversation.test.tsx(3) anduse-mic-recorder.test.tsx(5) run against currentmainunchanged: 2 fail there (the already-idle re-arm, and stream retention across a normal stop) and 6pass, holding the existing teardown and sibling behaviour still.
speech-chunking.test.ts(11) importsextractChunk, which this PR adds, and streams the reply in a character at a time, since passing a finishedstring hides the decimal case.
Desktop UI suite green apart from
markdown-text.test.ts, which fails the same way on unmodifiedmainhere. eslint clean on the touched files. tsc shows the same errors as
main, none in these files. Verifiedon macOS with local Kokoro + Whisper. Not tested on Windows; @wen0531 tested an earlier revision on Linux
(comment below).
Fixes #51265.