Skip to content

fix(preview): stop leaking video sources on layout changes - #892

Open
stevenahhh wants to merge 1 commit into
webadderallorg:mainfrom
stevenahhh:fix/preview-video-source-lifetime
Open

fix(preview): stop leaking video sources on layout changes#892
stevenahhh wants to merge 1 commit into
webadderallorg:mainfrom
stevenahhh:fix/preview-video-source-lifetime

Conversation

@stevenahhh

@stevenahhh stevenahhh commented Sep 7, 2026

Copy link
Copy Markdown

Description

Changing the preview radius or padding recreated VideoSource through the layoutVideoContent effect dependency. Cleanup called videoTexture.destroy(false), leaving each source's GPU allocation and play/pause/seeked listeners 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: false at construction to avoid starting playback before the old post-construction assignment.

The owner is necessary because Pixi's VideoSource.destroy() also clears video.src and reloads it. Calling texture.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:

Measurement Existing lifecycle Updated lifecycle
GPU process physical footprint 3.8 GiB 252.4 MiB
GPU process peak footprint 3.9 GiB 385.6 MiB
Retained listeners per video event (play, pause, seeked) 30 1

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

  • Bug Fix

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).
  • Biome lint on all three changed files: passed.
  • New tests use real Pixi sources with a test video element to cover 100 texture lifecycles, bounded event listeners, GPU unload events, paused seeks, media dimension changes, frame callback cancellation, and element detachment.
  • Manual integration check: open a high-resolution recording, repeatedly adjust radius/padding and scrub, verify geometry and playback still work, then open another recording and close the editor. GPU memory should remain bounded. Check process physical footprint with 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 f0faa06a with 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.

  • Changed radius 120 times (two rounds of 30 increments and 30 decrements), changed padding 20 times, played the video, and scrubbed/seeked repeatedly in both directions. The preview rendered correctly and playback continued to advance.
  • Imported a 1920×1080 / 2-second clip into the same editor and verified its preview and playback through the end.
  • Closed the editor, discarding only test edits, and observed GPU memory release.

All measurements below are physical footprint of the same GPU helper process, from vmmap -summary:

Checkpoint GPU physical footprint
After first 30 radius changes 988.1 MiB
After 60 radius changes, padding changes, playback and scrubbing 928.4 MiB
After 120 radius changes and additional repeated seeks 907.2 MiB
After switching to the 1080p clip 734.1 MiB
After closing the editor 99.5 MiB

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

  • I have performed a self-review of my code.
  • No visual design changes; screenshots are not applicable.
  • Related issues and changelog requirements considered; none applicable.

Summary by CodeRabbit

  • Bug Fixes

    • Improved video preview stability when switching media or reopening the editor.
    • Video previews now update more reliably after media loads and resume correctly after being paused or suspended.
    • Improved cleanup when preview videos are detached, reducing stale playback behavior and unnecessary resource usage.
  • Tests

    • Added coverage for video source reuse, suspension and resumption, media updates, and video element replacement.

@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 5b0402e5-75c0-4159-9ccd-831c295d295f

📥 Commits

Reviewing files that changed from the base of the PR and between 68bca43 and f0faa06.

📒 Files selected for processing (3)
  • src/components/video-editor/VideoPlayback.tsx
  • src/components/video-editor/videoPlayback/previewVideoSource.test.ts
  • src/components/video-editor/videoPlayback/previewVideoSource.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The preview now uses PreviewVideoSource to manage Pixi video sources for its persistent HTML video element. It adds lifecycle tests for reuse, suspension, detachment, frame callbacks, and source replacement.

Changes

Video source lifecycle

Layer / File(s) Summary
PreviewVideoSource lifecycle
src/components/video-editor/videoPlayback/previewVideoSource.ts, src/components/video-editor/videoPlayback/previewVideoSource.test.ts
Adds PreviewVideoSource with lazy source creation, suspension, detachment, replacement, and lifecycle tests.
Preview integration and cleanup
src/components/video-editor/VideoPlayback.tsx
Routes the video element through PreviewVideoSource, uses its source for texture creation, stabilizes media-ready layout handling, and suspends the source during cleanup.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to f0faa

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
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: fixing leaked video sources during preview layout changes.
Description check ✅ Passed The description is complete and follows the template. It explains the bug, motivation, change type, testing, validation results, and checklist status.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@stevenahhh
stevenahhh marked this pull request as ready for review September 7, 2026 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant