fix(recording): parallelize screen/mic capture and fade in mic gain#58
Conversation
) The mic track was lagging and clicking at the start of recordings, then running visibly behind the video track throughout. Three things combined to produce the symptom on the browser capture path: - Screen and mic were acquired sequentially in startRecording, so the mic track started capturing only after the screen track had been running for the full IPC + permission roundtrip. - The AudioContext (which routes the mic to the recorder's destination stream) was constructed only after both streams were in hand, so any audio the mic captured before then was dropped on the floor. - The mic gain was a constant 1.4 with no fade-in, so the very first packet produced a click/pop from the echo-canceller warm-up. Two changes: - Capture screen and microphone with Promise.all so they both start at the same wall-clock instant. - Extract the AudioContext + GainNode + MediaStreamDestination wiring into src/lib/audioMix.ts and add a 20 ms linearRampToValueAtTime on the mic gain from 0 -> MIC_GAIN_BOOST, so the destination stream's first audio packet doesn't click. The native Windows and macOS paths are unaffected (they already anchor audio to the first video frame).
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExtracts audio mixing into ChangesAudio Mix Extraction and Parallel Capture
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant useScreenRecorder
participant screenCapture
participant micCapture
participant mixAudioTracks
useScreenRecorder->>screenCapture: Start screen and optional system-audio capture
useScreenRecorder->>micCapture: Start microphone capture concurrently
screenCapture-->>useScreenRecorder: Return screen stream
micCapture-->>useScreenRecorder: Return microphone stream
useScreenRecorder->>mixAudioTracks: Pass system and microphone tracks
mixAudioTracks-->>useScreenRecorder: Return mixed track and optional AudioContext
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useScreenRecorder.ts (1)
1186-1204: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAwait the system-audio request before returning.
getUserMedia()returns a promise, so returning it directly from thetrybypasses thiscatch; a rejection will skip the video-only fallback and fail recording startup instead of degrading gracefully.Suggested fix
- return navigator.mediaDevices.getUserMedia({ + return await navigator.mediaDevices.getUserMedia({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useScreenRecorder.ts` around lines 1186 - 1204, The system-audio branch in useScreenRecorder is returning the getUserMedia promise directly, so the existing try/catch never handles a rejected request and the video-only fallback is skipped. Update the system-audio path to await the navigator.mediaDevices.getUserMedia call before returning, keeping the fallback logic in the catch so a failure in the system-audio request gracefully retries with audio disabled. Use the existing systemAudioEnabled block, selectedSource.id, and videoConstraints flow to keep the behavior unchanged aside from the async handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 1240-1248: The parallel startup in useScreenRecorder can leave a
successfully acquired screen or mic stream running if Promise.all() fails or the
countdown aborts before the refs are assigned. Update the startup flow around
the Promise.all and the isCountdownRunActive(countdownRunToken) early return so
any locally acquired streams are explicitly stopped on failure/cancellation
before returning, not just via teardownMedia(). Use the screenStream and
microphoneStream refs as the cleanup targets, and make sure the successful local
captures are always released even when one branch of the parallel acquisition
completes first.
In `@src/lib/audioMix.ts`:
- Around line 36-55: The mic fade-in is only applied in mixAudioTracks when both
systemAudioTrack and micAudioTrack are present, so mic-only recordings still
return the raw micAudioTrack and can pop at the start. Update mixAudioTracks to
route the mic-only branch through the same AudioContext, createGain, and
linearRampToValueAtTime flow used for mixed recordings, using the existing
MIC_GAIN_BOOST and MIC_FADE_IN_S logic, while keeping the return shape
consistent with the current MixAudioTracksResult.
---
Outside diff comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 1186-1204: The system-audio branch in useScreenRecorder is
returning the getUserMedia promise directly, so the existing try/catch never
handles a rejected request and the video-only fallback is skipped. Update the
system-audio path to await the navigator.mediaDevices.getUserMedia call before
returning, keeping the fallback logic in the catch so a failure in the
system-audio request gracefully retries with audio disabled. Use the existing
systemAudioEnabled block, selectedSource.id, and videoConstraints flow to keep
the behavior unchanged aside from the async handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 639fe4f5-9586-4a3a-a42c-7672e1daf4fa
📒 Files selected for processing (3)
src/hooks/useScreenRecorder.tssrc/lib/audioMix.test.tssrc/lib/audioMix.ts
…cordings Addresses two review findings on the parallel screen/mic capture path: - Stream leak on early exit: after `Promise.all`, the cancellation branch (and a rejected screen capture) ran before the local streams were copied into `screenStream.current` / `microphoneStream.current`, so `teardownMedia()` could not stop them and the screen/mic indicator stayed on. Assign the refs before the cancel check, and stop the parallel mic stream if the screen capture rejects. - Mic-only click: the anti-click fade-in only ran when both system audio and mic were present, so mic-only recordings (a common mode) still clicked at the first packet. Route the mic through the GainNode fade-in in that case too, ramping to unity so mic-only loudness is unchanged (the 1.4x boost still applies only when the mic competes with system audio). tsc + biome clean; 295/295 vitest pass (mic-only test updated). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Mic recordings were laggy and clicking at the start of recordings, then visibly drifting out of sync with the video track throughout (#57). Three things combined to produce the symptom on the browser capture path:
AudioContext(which routes the mic into the recorder's destination stream) was constructed only after both streams were in hand, so any audio the mic captured before then was silently dropped.1.4with no fade-in, so the first audio packet clicked from the echo-canceller warm-up.Changes
src/hooks/useScreenRecorder.ts— Capture screen and microphone withPromise.allinstartRecordingso both tracks start at the same wall-clock instant.src/lib/audioMix.ts(new) — Extract theAudioContext+GainNode+MediaStreamDestinationwiring into a small helper, and apply a 20 mslinearRampToValueAtTimeon the mic gain from0→MIC_GAIN_BOOSTso the destination stream's first packet doesn't click.src/lib/audioMix.test.ts(new) — Unit test for the mixer: verbatim passthrough when only one track is present, freshAudioContext+ fade-in + connections when both are present.Why not also fix the per-clock-domain A/V drift?
The native Windows (
audio_sample_utils.cpp:289) and macOS (main.swift:301) paths already anchor audio to the first video frame in the same clock domain. The browser path inheritsMediaRecorder's shared clock, so once the screen and mic tracks both feed it from t≈0, they stay aligned. Long-run drift correction between mismatched endpoint formats is a separate, much harder fix tracked indocs/engineering/windows-native-recorder-roadmap.md:116, 144— not in scope here.Native paths
Unchanged. They already anchor audio to the first video frame.
Test plan
npx tsc --noEmitcleannpm run lintclean (Biome)npm run test— 295/295 pass, including 5 new tests formixAudioTracksFixes #57
Summary by CodeRabbit
New Features
Bug Fixes
Tests