fix(recording): stream the native webcam to disk instead of losing it - #259
Merged
Merged
Conversation
On the macOS and Linux native capture paths the webcam handle was built with no file name, which selects in-memory buffering. Nothing reached disk during capture, and finalize had to flatten the whole clip into one ArrayBuffer to hand it across IPC. Past ~2GB — a take of roughly 20 minutes at BITRATE_BASE — that allocation throws. The throw was swallowed twice over: fixWebmDuration catches its own FileReader failure and returns the unpatched blob, then the finalize catch logged to console and returned undefined, which made the attach guard skip attachNative*WebcamRecording entirely. The session was written screen-only and the editor opened as if nothing had happened, with the camera simply absent. A 23-minute take lost its webcam this way; shorter takes in the same app session saved fine. Pass the webcam file name on both native paths so chunks stream to disk as they arrive, the way the legacy path and the Windows helper already do. Finalize now branches on isStreaming(): a streamed clip hands over its name alone and the main process closes the stream and patches the WebM duration on disk, so nothing multi-gigabyte is ever flattened or sent across IPC. Buffered short takes keep the existing behaviour. Every failure now comes back with a reason and reaches the user as a toast. Silently discarding a completed take is the worst available outcome, and it was the one that shipped. Because the bytes now land on disk during capture, a webcam stream that isn't folded into a saved session is closed and its partial file removed — otherwise a discarded or failed take orphans a half-written .webm. The macOS and Linux finalizers were line-for-line copies, so the shared logic moves into finalizeWebcamAsset() rather than being duplicated again. Its tests pin both halves of the fix: that a streamed clip is never read into memory, and that a failure is never silent. Fixes #253
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
thank you! |
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
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.
Fixes #253.
The bug
On the macOS and Linux native capture paths the webcam handle is built with no file name, which selects in-memory buffering — nothing reaches disk during capture. Finalize then has to flatten the whole clip into a single
ArrayBufferto hand it across IPC. Past ~2 GB (roughly 20 minutes atBITRATE_BASE) that allocation throws.The throw was swallowed twice:
fixWebmDurationcatches its own failure and returns the unpatched blob —WebmFile.fromBlobdoes a full-blobFileReader.readAsArrayBuffer, so it dies first and silently.catchlogged to console and returnedundefined, which made theif (webcamAsset && result.path)guard skip the attach entirely.The session was written screen-only and the editor opened as if nothing had happened, camera simply absent. No toast, no failure state.
One correction to the issue's diagnosis: this is not memory exhaustion. Chromium handled the buffering correctly and spilled the ~1380 one-second blobs to
blob_storage— that's what the reporter's 2149 MB disk-write warning actually shows. What has a hard ceiling is the mandatory flatten. The old design needed a contiguous multi-GB allocation four times over: theFileReaderread,.arrayBuffer(), the structured clone throughipcRenderer.invoke, andBuffer.from()in the main process.Windows is unaffected — its helper owns webcam capture directly.
The fix
Pass the webcam file name on both native paths so chunks stream to disk as they arrive, the way the legacy path and the Windows helper already do. All the plumbing existed (
RecordingStreamRegistry,finalizeRecordingFile,repairRecordingContainer) — the native paths just weren't wired to it.isStreaming(): a streamed clip hands over its name alone; the main process closes the stream and patches the WebM duration on disk. Nothing multi-gigabyte is flattened or crosses IPC. Buffered short takes keep the existing behaviour.catchand thesize === 0early return used to exit without saying anything..webm.finalizeWebcamAsset()instead of being duplicated a second time.Tests
Six new cases in
src/hooks/webcamAsset.test.ts, following thewebcamOffset.test.tsconvention. Verified they have teeth: removing theisStreaming()branch fails two of them.Full suite: 1625 passing. The 6 failures in
electron/recording/webm-seek-index.test.tsare pre-existing and unrelated — that suite asserts remux behaviour behind aprocess.platform !== "linux"guard, so it only passes on Linux (confirmed failing identically on a clean tree).What I could not verify
The streaming path is exercised by unit tests, not by a real >2 GB recording — I have no way to run a 20-minute webcam capture here. Worth one manual long take on macOS before release.