fix(recording): parallelize screen/mic capture and fade in mic gain#58
Open
EtienneLescot wants to merge 1 commit into
Open
fix(recording): parallelize screen/mic capture and fade in mic gain#58EtienneLescot wants to merge 1 commit into
EtienneLescot wants to merge 1 commit into
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).
📝 WalkthroughWalkthroughExtracts audio-mixing logic from ChangesAudio Mix Extraction and Parallel Capture
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
Comment on lines
+1240
to
+1248
| const [screenMediaStream, micMediaStream] = await Promise.all([screenCapture, micCapture]); | ||
|
|
||
| if (!isCountdownRunActive(countdownRunToken)) { | ||
| teardownMedia(); | ||
| return; | ||
| } | ||
|
|
||
| if (microphoneEnabled) { | ||
| try { | ||
| microphoneStream.current = await navigator.mediaDevices.getUserMedia({ | ||
| audio: microphoneDeviceId | ||
| ? { | ||
| deviceId: { exact: microphoneDeviceId }, | ||
| echoCancellation: true, | ||
| noiseSuppression: true, | ||
| autoGainControl: true, | ||
| } | ||
| : { | ||
| echoCancellation: true, | ||
| noiseSuppression: true, | ||
| autoGainControl: true, | ||
| }, | ||
| video: false, | ||
| }); | ||
| } catch (audioError) { | ||
| console.warn("Failed to get microphone access:", audioError); | ||
| toast.error(t("recording.microphoneDenied")); | ||
| setMicrophoneEnabled(false); | ||
| } | ||
| } | ||
| screenStream.current = screenMediaStream; | ||
| microphoneStream.current = micMediaStream; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Stop locally acquired streams before any early exit from parallel startup.
At Line 1240, Promise.all() can reject after one capture has already succeeded, and the cancellation branch at Line 1242 returns before either local stream is copied into screenStream.current / microphoneStream.current. teardownMedia() only stops ref-held streams, so the successful local capture can stay live with the screen/mic indicator on after startup fails or the countdown is cancelled.
Suggested fix
- const [screenMediaStream, micMediaStream] = await Promise.all([screenCapture, micCapture]);
-
- if (!isCountdownRunActive(countdownRunToken)) {
- teardownMedia();
- return;
- }
-
- screenStream.current = screenMediaStream;
- microphoneStream.current = micMediaStream;
+ let screenMediaStream: MediaStream | null = null;
+ let micMediaStream: MediaStream | null = null;
+ try {
+ [screenMediaStream, micMediaStream] = await Promise.all([screenCapture, micCapture]);
+ if (!isCountdownRunActive(countdownRunToken)) {
+ screenMediaStream.getTracks().forEach((track) => track.stop());
+ micMediaStream?.getTracks().forEach((track) => track.stop());
+ return;
+ }
+
+ screenStream.current = screenMediaStream;
+ microphoneStream.current = micMediaStream;
+ } catch (error) {
+ screenMediaStream?.getTracks().forEach((track) => track.stop());
+ micMediaStream?.getTracks().forEach((track) => track.stop());
+ throw error;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const [screenMediaStream, micMediaStream] = await Promise.all([screenCapture, micCapture]); | |
| if (!isCountdownRunActive(countdownRunToken)) { | |
| teardownMedia(); | |
| return; | |
| } | |
| if (microphoneEnabled) { | |
| try { | |
| microphoneStream.current = await navigator.mediaDevices.getUserMedia({ | |
| audio: microphoneDeviceId | |
| ? { | |
| deviceId: { exact: microphoneDeviceId }, | |
| echoCancellation: true, | |
| noiseSuppression: true, | |
| autoGainControl: true, | |
| } | |
| : { | |
| echoCancellation: true, | |
| noiseSuppression: true, | |
| autoGainControl: true, | |
| }, | |
| video: false, | |
| }); | |
| } catch (audioError) { | |
| console.warn("Failed to get microphone access:", audioError); | |
| toast.error(t("recording.microphoneDenied")); | |
| setMicrophoneEnabled(false); | |
| } | |
| } | |
| screenStream.current = screenMediaStream; | |
| microphoneStream.current = micMediaStream; | |
| let screenMediaStream: MediaStream | null = null; | |
| let micMediaStream: MediaStream | null = null; | |
| try { | |
| [screenMediaStream, micMediaStream] = await Promise.all([screenCapture, micCapture]); | |
| if (!isCountdownRunActive(countdownRunToken)) { | |
| screenMediaStream.getTracks().forEach((track) => track.stop()); | |
| micMediaStream?.getTracks().forEach((track) => track.stop()); | |
| return; | |
| } | |
| screenStream.current = screenMediaStream; | |
| microphoneStream.current = micMediaStream; | |
| } catch (error) { | |
| screenMediaStream?.getTracks().forEach((track) => track.stop()); | |
| micMediaStream?.getTracks().forEach((track) => track.stop()); | |
| throw error; | |
| } |
🤖 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 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.
Comment on lines
+36
to
+55
| export function mixAudioTracks({ | ||
| systemAudioTrack, | ||
| micAudioTrack, | ||
| }: MixAudioTracksInput): MixAudioTracksResult { | ||
| if (systemAudioTrack && micAudioTrack) { | ||
| const context = new AudioContext(); | ||
| const systemSource = context.createMediaStreamSource(new MediaStream([systemAudioTrack])); | ||
| const micSource = context.createMediaStreamSource(new MediaStream([micAudioTrack])); | ||
| const micGain = context.createGain(); | ||
| micGain.gain.setValueAtTime(0, context.currentTime); | ||
| micGain.gain.linearRampToValueAtTime(MIC_GAIN_BOOST, context.currentTime + MIC_FADE_IN_S); | ||
| const destination = context.createMediaStreamDestination(); | ||
| systemSource.connect(destination); | ||
| micSource.connect(micGain).connect(destination); | ||
| return { context, track: destination.stream.getAudioTracks()[0] }; | ||
| } | ||
| return { | ||
| context: null, | ||
| track: systemAudioTrack ?? micAudioTrack ?? null, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply the fade-in on mic-only recordings too.
Lines 40-50 only ramp the gain when both inputs exist. If the user records microphone-only, Line 54 returns the raw micAudioTrack, so the start-of-stream click/pop this PR is meant to suppress still survives in that supported mode. Route the mic-only path through the same AudioContext/GainNode flow as well.
🤖 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/lib/audioMix.ts` around lines 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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