fix: stream large local recordings so the editor and export don't exhaust memory#74
Conversation
Exporting a recording larger than ~2 GiB failed with "Failed to read binary file". The renderer loaded the whole source into memory via the read-binary-file IPC (Node `fs.readFile`), which throws ERR_FS_FILE_TOO_LARGE above 2 GiB, so any long recording (e.g. a 2h 1080p60 capture at ~6.6 GB) could never be exported. Even under the cap, a multi-GB ArrayBuffer/Blob would exhaust memory on a typical machine. web-demuxer reads a File on demand, so it never needs the bytes up front. Add a chunked range-read IPC (`get-readable-file-info` + `read-file-chunk`) and a renderer helper that streams large recordings into an OPFS-backed File, handing web-demuxer a disk-backed File instead of an in-memory one. Memory stays flat regardless of length. Small recordings keep the existing single-shot path. Wire the streaming loader into the export decoder and the captions audio extractor; skip the in-memory source-copy fast path and the waveform ArrayBuffer read for oversized files so they degrade gracefully. Verified against a real 6.6 GB / 2h11m recording: OPFS copy peaked at ~137 MB heap, web-demuxer parsed metadata and decoded 1920x1080 frames with heap staying ~27 MB. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Electron file-info and chunk-read IPC, introduces OPFS-backed local source materialization for large recordings, and updates captioning, exporter, audio peak, and export UI flows to use size-aware loading and a new preparing phase. ChangesChunked local file access and large-file streaming
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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: 1
🧹 Nitpick comments (3)
src/lib/exporter/localSourceFile.test.ts (1)
1-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the OPFS cache eviction logic.
Tests here are solid for the small-file path, but
pruneStaleEntries/cache-reuse behavior incopyToOpfsFile(flagged separately inlocalSourceFile.ts) has no automated coverage. A test simulating two concurrently-cached large sources would have caught the single-slot pruning issue.🤖 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/exporter/localSourceFile.test.ts` around lines 1 - 86, Add test coverage for the OPFS cache eviction and reuse behavior that lives in materializeLocalSourceFile/copyToOpfsFile, since the current localSourceFile.test.ts only covers the small-file branch. Create a scenario with two large sources being cached concurrently so the second cache operation exercises pruneStaleEntries and verifies the earlier entry is not incorrectly evicted or reused. Use the existing materializeLocalSourceFile and copyToOpfsFile flow, along with the Electron API stubs, to assert the intended multi-source cache behavior.src/lib/exporter/streamingDecoder.ts (1)
202-224: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo progress feedback during large local-file OPFS materialization.
materializeLocalSourceFilesupports anonProgresscallback (used for the multi-GB streaming copy), butloadLocalSourceFiledoesn't pass one through. GivenLOCAL_SOURCE_LOAD_TIMEOUT_MSallows up to 15 minutes for this step, a large export could appear to hang with no feedback while the file streams into OPFS.♻️ Proposed wiring for progress feedback
- static async loadLocalSourceFile(videoUrl: string): Promise<File> { + static async loadLocalSourceFile( + videoUrl: string, + onProgress?: (progress: { copiedBytes: number; totalBytes: number }) => void, + ): Promise<File> { const filename = (videoUrl.split(/[\\/]/).pop() || "video").replace(/^file:/, ""); - return materializeLocalSourceFile(videoUrl, filename); + return materializeLocalSourceFile(videoUrl, filename, onProgress); }🤖 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/exporter/streamingDecoder.ts` around lines 202 - 224, `loadLocalSourceFile` currently calls `materializeLocalSourceFile` without exposing its progress callback, so large OPFS copies have no feedback. Update `streamingDecoder.ts` so `loadLocalSourceFile` accepts and forwards an `onProgress` callback to `materializeLocalSourceFile`, and thread that through any caller path that triggers local-file loading via `loadSourceFile`/`loadMetadata` so multi-GB materialization can report progress.src/lib/exporter/localSourceFile.ts (1)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThreshold constant duplicated across 3 files.
The same
1.5 * 1024 * 1024 * 1024threshold is independently redefined here, asMAX_DECODE_AUDIO_DATA_BYTESinextractMono16k.ts, and asSOURCE_COPY_MAX_BYTESinvideoExporter.ts. If one is tuned later without updating the others, the size-based fallback behavior will silently diverge between captions, export fast-path, and OPFS materialization.♻️ Proposed fix: export and reuse a single constant
-const LARGE_FILE_THRESHOLD_BYTES = 1.5 * 1024 * 1024 * 1024; +// Shared with extractMono16k.ts and videoExporter.ts — keep one source of truth. +export const LARGE_FILE_THRESHOLD_BYTES = 1.5 * 1024 * 1024 * 1024;Then import
LARGE_FILE_THRESHOLD_BYTESfromextractMono16k.tsandvideoExporter.tsinstead of redeclaring it.Also applies to: 28-28
🤖 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/exporter/localSourceFile.ts` around lines 22 - 24, The file-size threshold is duplicated across localSourceFile, extractMono16k, and videoExporter, which can cause the fallback behavior to drift if one value changes. Move the shared 1.5 * 1024 * 1024 * 1024 limit into a single exported constant, then update the LARGE_FILE_THRESHOLD_BYTES, MAX_DECODE_AUDIO_DATA_BYTES, and SOURCE_COPY_MAX_BYTES usages to import and reuse that shared symbol instead of redefining it.
🤖 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/lib/exporter/localSourceFile.ts`:
- Line 106: The pruning step in localSourceFile’s export flow is deleting OPFS
cache entries that are still needed by the same export. Update the cache cleanup
around pruneStaleEntries in localSourceFile so it does not remove entries while
the demuxer is still active; keep live cache names until the decoder/export is
destroyed, or defer pruning until no later read via demuxer.read("video", ...)
can occur. Use the localSourceFile export path and pruneStaleEntries call as the
main touchpoints.
---
Nitpick comments:
In `@src/lib/exporter/localSourceFile.test.ts`:
- Around line 1-86: Add test coverage for the OPFS cache eviction and reuse
behavior that lives in materializeLocalSourceFile/copyToOpfsFile, since the
current localSourceFile.test.ts only covers the small-file branch. Create a
scenario with two large sources being cached concurrently so the second cache
operation exercises pruneStaleEntries and verifies the earlier entry is not
incorrectly evicted or reused. Use the existing materializeLocalSourceFile and
copyToOpfsFile flow, along with the Electron API stubs, to assert the intended
multi-source cache behavior.
In `@src/lib/exporter/localSourceFile.ts`:
- Around line 22-24: The file-size threshold is duplicated across
localSourceFile, extractMono16k, and videoExporter, which can cause the fallback
behavior to drift if one value changes. Move the shared 1.5 * 1024 * 1024 * 1024
limit into a single exported constant, then update the
LARGE_FILE_THRESHOLD_BYTES, MAX_DECODE_AUDIO_DATA_BYTES, and
SOURCE_COPY_MAX_BYTES usages to import and reuse that shared symbol instead of
redefining it.
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 202-224: `loadLocalSourceFile` currently calls
`materializeLocalSourceFile` without exposing its progress callback, so large
OPFS copies have no feedback. Update `streamingDecoder.ts` so
`loadLocalSourceFile` accepts and forwards an `onProgress` callback to
`materializeLocalSourceFile`, and thread that through any caller path that
triggers local-file loading via `loadSourceFile`/`loadMetadata` so multi-GB
materialization can report progress.
🪄 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: e0a0f3bd-1f0d-473e-b207-81e8bbaaf1dd
📒 Files selected for processing (8)
electron/electron-env.d.tselectron/ipc/handlers.tselectron/preload.tssrc/lib/captioning/extractMono16k.tssrc/lib/exporter/localSourceFile.test.tssrc/lib/exporter/localSourceFile.tssrc/lib/exporter/streamingDecoder.tssrc/lib/exporter/videoExporter.ts
Address review feedback on the >2GB export fix: - OPFS cache pruning now reference-counts live sources and never deletes an entry a demuxer is still reading. releaseLocalSourceFile() is called from StreamingVideoDecoder.destroy() and the captions extractor's finally. Previously pruning kept only the newest entry, which could remove a file still in use by a concurrent export/caption pass. - Extract the shared in-memory size threshold into sourceFileLimits.ts (was duplicated across localSourceFile, extractMono16k, videoExporter). - Report OPFS copy progress as a "preparing" export phase so the dialog reflects the multi-GB copy instead of sitting at 0%. - Add unit coverage for the OPFS streaming/eviction path: chunked copy, cache reuse, and the in-use pruning guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/exporter/localSourceFile.test.ts (1)
155-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for concurrent copies of the same cache key.
The new suite covers sequential reuse and prune/refcount semantics well, but there's no test exercising two concurrent
materializeLocalSourceFilecalls racing to write the samecacheName(e.g.Promise.all([materialize(url), materialize(url)])), which is exactly the scenario in the concurrency finding oncopyToOpfsFile's catch-block cleanup.FakeWritable/FakeFileHandlewould need to model OPFS's siloed multi-writer semantics (or at least a forced failure on one of two concurrent writers) to catch that regression.🤖 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/exporter/localSourceFile.test.ts` around lines 155 - 244, The large-file OPFS test suite in materializeLocalSourceFile is missing coverage for two concurrent calls racing to write the same cache entry. Add a test around materializeLocalSourceFile/copyToOpfsFile that starts two Promise.all copies for the same url/cacheName and forces one writer to fail so the catch-block cleanup is exercised. Update FakeWritable/FakeFileHandle or the test harness to simulate OPFS multi-writer behavior, then assert the surviving copy is correct and no stale partial cache entry remains.
🤖 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/lib/exporter/localSourceFile.ts`:
- Around line 51-70: The refcount tracking in retainSource and
releaseLocalSourceFile is keyed only by videoUrl, so a new cacheName can
overwrite an older still-active revision and cause releases to decrement the
wrong entry. Update the tracking in localSourceFile to distinguish revisions by
both videoUrl and cacheName, and make releaseLocalSourceFile decrement the
matching retained revision instead of the latest one. Ensure activeSources and
any related bookkeeping preserve outstanding refs for older revisions until
their own releases arrive.
---
Nitpick comments:
In `@src/lib/exporter/localSourceFile.test.ts`:
- Around line 155-244: The large-file OPFS test suite in
materializeLocalSourceFile is missing coverage for two concurrent calls racing
to write the same cache entry. Add a test around
materializeLocalSourceFile/copyToOpfsFile that starts two Promise.all copies for
the same url/cacheName and forces one writer to fail so the catch-block cleanup
is exercised. Update FakeWritable/FakeFileHandle or the test harness to simulate
OPFS multi-writer behavior, then assert the surviving copy is correct and no
stale partial cache entry remains.
🪄 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: 3f857ae1-91cd-41e6-a6e1-bb7ba7f85ab0
📒 Files selected for processing (8)
src/components/video-editor/ExportDialog.tsxsrc/lib/captioning/extractMono16k.tssrc/lib/exporter/localSourceFile.test.tssrc/lib/exporter/localSourceFile.tssrc/lib/exporter/sourceFileLimits.tssrc/lib/exporter/streamingDecoder.tssrc/lib/exporter/types.tssrc/lib/exporter/videoExporter.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/exporter/sourceFileLimits.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/exporter/streamingDecoder.ts
Address the follow-up review on the OPFS cache guard: - Reference-count cache entries by cache-entry name (the returned File's .name) instead of by source URL. Keying by URL discarded an older revision's refcount when a new revision (size/mtime change) superseded it, so releasing the old revision could decrement the wrong entry and prematurely drop a still-in-use copy. Callers now release with the File's name (StreamingVideoDecoder.destroy and the captions extractor). - Add a test that a failed chunk read mid-copy throws and removes the partial cache entry, exercising the catch-block cleanup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the follow-up review — addressed in 655cf00: Refcount keyed only by Concurrent copies of the same All 299 unit tests, |
|
@rainyflash Thanks for your PR! |
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
Here is a code review of the chunked file streaming solution. Overall, this is an excellent and necessary addition to support > 2 GiB files, but there is a major race condition in copyToOpfsFile's cache pruning, along with a few minor security/robustness recommendations.
The 1.5 GiB threshold was too high. read-binary-file returns the whole file over IPC, which Electron structured-clones (copies) in the main process, so a ~1 GB recording transiently needs ~2x its size there and hard-crashes a memory-constrained machine (observed on a 16 GB Mac) — well below the 2 GiB fs.readFile cap. Two paths hit this: - Export/demux source load and captions now stream via OPFS above 256 MiB (lowered MAX_IN_MEMORY_SOURCE_BYTES), so moderate recordings no longer ship a giant buffer over IPC. - The trim waveform (loadFileAsArrayBuffer) cannot stream — decodeAudioData needs the whole file — so it now skips recordings above the limit; useAudioPeaks already degrades to no waveform on throw. Fixes an editor-entry crash when opening a ~1 GB recording. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rainyflash
changed the title
|
Hi @rainyflash! We are currently working on a PR to resolve the crash/freeze during finalization/saving of large recordings (issue #41) by streaming the WebM duration patching on disk rather than loading the file entirely in memory, and adding a \saving\ loading state to the recording window UI. Your PR (#74) for streaming the files during edit and export is highly complementary to this, as together they will fully solve OOM issues for large recordings! We will cross-reference this once our PR is opened. |
Review feedback from getopenscreen#74: - Fix the concurrent-prune race: retain the OPFS cache entry BEFORE pruning/writing, so a concurrent materialization's prune pass can no longer delete an entry that is still mid-write. On failure the reference is released and the partial copy removed only if no other in-flight operation still references it. Regression test included. - Cap read-file-chunk requests at 64 MiB on the main-process side so a buggy or compromised renderer cannot force an arbitrarily large Buffer.allocUnsafe. - Infer the small-file MIME type from the extension (mp4/webm/mov/...) instead of hardcoding video/mp4. - Restore the trim waveform for large recordings instead of skipping it: new streaming peaks path demuxes the audio track and folds each decoded AudioData frame straight into min/max buckets (same output format as audioPeaksWorker), so memory stays flat. Verified on a 288 MB file: all 24000 blocks populated, renderer heap 25→27 MB. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the review @EtienneLescot! All three points are addressed in 35937a9 (replied inline with details): the prune race is fixed by retaining the cache entry before pruning/writing (with a concurrent-copy regression test), Re: platform testing — honest answer: macOS (arm64) only. That's the machine I have. What was tested there:
The changed code paths are platform-agnostic by construction — OPFS, web-demuxer, WebCodecs, and two thin One more improvement in the same commit: instead of skipping the trim waveform for large recordings (a silent feature regression of my earlier commit), there is now a streaming peaks path — the audio track is demuxed and each decoded |
… paths Findings from an adversarial multi-agent cross-review of this branch: - Cancelling an export during the "preparing" phase used to leak the OPFS cache reference forever (destroy() ran before sourceCacheName was set) while the multi-GB copy kept streaming in the background. Copies are now abortable: materializeLocalSourceFile takes an AbortSignal, the decoder's cancel()/destroy() and the load timeout abort it, and references are only taken when a caller actually receives the File — no success, no retain, nothing to leak. GifExporter benefits via the same decoder hook. - Concurrent materializations of the same recording (waveform + export) used to race two writables on one OPFS handle and could invalidate the File another consumer was reading. In-flight copies are now deduplicated per cache entry: joiners share one stream (and its progress events), and the underlying copy aborts only when every joined caller has aborted. - The streaming waveform now falls back to a demux-only packet-timestamp scan when the container duration is missing/bogus (MediaRecorder WebM), and always closes its AudioDecoder on error/abort paths. - The caption demuxer path buffers decoded PCM in memory, so oversized sources now cap decoded audio at 30 min and surface `truncated` instead of exhausting the renderer heap on multi-hour recordings. - Stale multi-GB OPFS copies from previous sessions are reclaimed at app startup (previously only pruned during the next large-file load). Tests: in-flight dedup, mid-copy abort + cleanup, and shared-copy survival when one of two joined callers aborts (307 total passing). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
(Also: great to see #77 — save-time streaming plus this PR's edit/export streaming covers the full large-recording lifecycle, and the two PRs touch disjoint files so they can land independently.) After addressing your review, we ran a systematic adversarial cross-review of the whole branch (multiple independent passes: correctness / concurrency / security / memory / behavior-regressions) — the prune race you caught suggested the concurrency surface deserved a full audit. It did turn up more issues in the same family, all fixed in
Three concurrency/abort regression tests added; 307 tests passing. PR description updated to match. I realize this grew the PR — if you'd prefer to review it in smaller pieces, I'm happy to split |
Review feedback from #74: - Fix the concurrent-prune race: retain the OPFS cache entry BEFORE pruning/writing, so a concurrent materialization's prune pass can no longer delete an entry that is still mid-write. On failure the reference is released and the partial copy removed only if no other in-flight operation still references it. Regression test included. - Cap read-file-chunk requests at 64 MiB on the main-process side so a buggy or compromised renderer cannot force an arbitrarily large Buffer.allocUnsafe. - Infer the small-file MIME type from the extension (mp4/webm/mov/...) instead of hardcoding video/mp4. - Restore the trim waveform for large recordings instead of skipping it: new streaming peaks path demuxes the audio track and folds each decoded AudioData frame straight into min/max buckets (same output format as audioPeaksWorker), so memory stays flat. Verified on a 288 MB file: all 24000 blocks populated, renderer heap 25→27 MB. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Problem
Large local recordings crash Openscreen in two places:
Failed to read binary file. A 2‑hour 1080p60 capture (~6.6 GB) can never be exported after editing.EXC_BREAKPOINTin the main process) before you can edit it.Root cause
The renderer loads the source through the
read-binary-fileIPC, which reads the whole file with Node'sfs.readFileand returns the bytes over IPC. That has two failure modes:fs.readFilethrowsERR_FS_FILE_TOO_LARGEabove 2 GiB, so large files can't be read at all.web-demuxerreads aFileon demand, so the export never needed the bytes up front.Fix
Chunked range-read IPC (
get-readable-file-info+read-file-chunk), reusing the existing path allow-listing, with a 64 MiB per-chunk cap enforced on the main-process side.Stream large sources into an OPFS-backed File (
localSourceFile.ts): recordings above 256 MiB are streamed in 32 MB chunks and handed to web-demuxer as a disk-backed File, so peak memory stays flat regardless of length. Copies are cache-keyed and reference-counted — entries are retained before the prune/write, so a concurrent materialization can never prune an entry that is mid-write or still being read. Copy progress surfaces as apreparingexport phase.Export decoder and captions route through the streaming loader; the in-memory source-copy fast path is skipped for oversized files. Small-file MIME types are inferred from the extension.
Trim waveform streams too: a new peaks path demuxes the audio track and folds each decoded
AudioDataframe straight into min/max buckets (same output format asaudioPeaksWorker), so large recordings keep their waveform instead of losing it — with flat memory.Robustness (from an adversarial cross-review of this branch): OPFS copies are abortable (cancelling an export or unmounting the waveform stops the multi-GB copy; cache references are only taken on successful delivery, so nothing can leak) and deduplicated (waveform + export of the same recording share one in-flight copy instead of racing two writables). The streaming waveform falls back to a packet-timestamp scan when the container duration is bogus (MediaRecorder WebM) and always closes its AudioDecoder. Caption decode is capped at 30 min for oversized sources (surfaced as
truncated) since its PCM buffers are in-memory. Stale cache copies from previous sessions are reclaimed at startup.No new dependencies.
Verification
Tested on a 16 GB Mac (arm64) against real recordings:
Failed to read binary filetsc, Biome, and the unit suite pass (304 tests incl. OPFS streaming, eviction, refcount, and concurrent-prune race coverage).Platform note: hands-on testing was macOS arm64 only; the changed paths are platform-agnostic (OPFS / web-demuxer / WebCodecs / thin
fsIPC), but Windows/Linux runs would be welcome.Notes / possible follow-ups
Known tradeoff: unedited MP4s between 256 MiB and ~1.5 GiB previously hit the in-memory source-copy fast path (instant export); they now take the streaming re-encode path. The old path OOM-crashed the main process at ~1 GB in testing, so this trades speed for reliability; a zero-copy fast path via a main-process file copy would restore the speed and is a natural follow-up.
The OPFS copy trades temporary disk for flat memory. A zero-copy path — serving the file over a Range-capable protocol and passing web-demuxer a URL source (it already does
Range: bytes=requests) — would avoid the copy; happy to explore separately.🤖 Generated with Claude Code