fix(preview): stop leaking video sources on layout changes - #892
fix(preview): stop leaking video sources on layout changes#892stevenahhh wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe preview now uses ChangesVideo source lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Preview video lifecycle handling now reuses sources during layout changes and releases GPU and event resources during cleanup or detachment, with no concrete current-head merge risk identified. Sequence Diagram(s)sequenceDiagram
participant VideoPlayback
participant PreviewVideoSource
participant HTMLVideoElement
participant PixiVideoSource
VideoPlayback->>PreviewVideoSource: setVideo(video)
VideoPlayback->>PreviewVideoSource: getSource()
PreviewVideoSource->>PixiVideoSource: create or update source
HTMLVideoElement->>PixiVideoSource: emit media events and frame callbacks
VideoPlayback->>PreviewVideoSource: suspend()
PreviewVideoSource->>PixiVideoSource: disable auto-update and unload
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Description
Changing the preview radius or padding recreated
VideoSourcethrough thelayoutVideoContenteffect dependency. Cleanup calledvideoTexture.destroy(false), leaving each source's GPU allocation andplay/pause/seekedlisteners alive.VideoSource.from(video)creates a fresh source on every call; it does not cache by video element.Reuse one source for React's persistent video element, unload GPU data and suspend updates when the sprite effect cleans up, and destroy the source when React detaches the element. Keep geometric layout changes independent of the texture/event-handler effect. Set
autoPlay: falseat construction to avoid starting playback before the old post-construction assignment.The owner is necessary because Pixi's
VideoSource.destroy()also clearsvideo.srcand reloads it. Callingtexture.destroy(true)on every effect cleanup would disrupt React's persistent media element.Motivation
Investigated a 19.8-second 5120×2880 recording on macOS with Recordly 1.4.0-beta.1. After radius adjustment and scrubbing, the GPU helper retained 12.9 GiB physical footprint (13.6 GiB peak), including about 12 GiB in IOAccelerator graphics allocations, while the editor renderer footprint was 334 MiB.
An isolated Electron/Pixi reproduction using a synthetic 5120×2880 video and 30 texture creation/render/cleanup cycles confirmed the source lifetime leak:
play,pause,seeked)The table above measures the isolated reproduction. Full editor validation with the built application is recorded below. No recordings or raw user diagnostic dumps are included.
Type of Change
Related Issue(s)
No existing issue linked.
Testing Guide
npm test: 121 test files / 1,086 tests passed.npx tsc --noEmit: passed.npx vite build: passed (renderer and Electron bundles; not a packaged installer).vmmap -summary <GPU helper PID>; RSS alone misses much of the graphics allocation on macOS.Full editor validation on macOS
Validated the built renderer and Electron main process from commit
f0faa06awith Electron 43.1.0, a separate test profile, and a local copy of the original 5120×2880 / 19.8-second recording. This ran the actual Recordly editor rather than the synthetic lifecycle harness; the installed app was not replaced.All measurements below are physical footprint of the same GPU helper process, from
vmmap -summary:Peak GPU footprint for this process was 1.3 GiB. There was no monotonic memory growth during the repeated edits. This validates the preview regression on the tested machine; packaged distribution/signing and recording/export paths were not part of this test.
Checklist
Summary by CodeRabbit
Bug Fixes
Tests