refactor(web): replace unbounded preview frame buffering with bounded, disposable frame access#758
Open
chinhkrb113 wants to merge 1 commit intoOpenCut-app:mainfrom
Conversation
…, disposable frame access Playback crash symptoms (short freeze, then browser/tab crash) strongly match an OOM pattern from preloading or retaining too many decoded video frames (`ImageBitmap`/raw RGBA buffers). The media manager should stop eager full-clip frame extraction and move to bounded caching with explicit disposal. This change should make video decoding demand-driven (current/nearby frames only), and ensure old frames are evicted and closed. Affected files: media-manager.ts Signed-off-by: ChinhLee <76194645+chinhkrb113@users.noreply.github.com>
|
@chinhkrb113 is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughAdded an in-memory LRU frame cache to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/core/managers/media-manager.ts (1)
161-166: Consider defensive check for closed bitmaps on cache hit.If a caller mistakenly closes the returned
ImageBitmap, subsequent cache hits will return an invalid bitmap. Adding a check forframe.width === 0(closed bitmaps have zero dimensions) could defensively handle this edge case.🛡️ Optional defensive check
const existing = this.frameCache.find( (c) => c.assetId === assetId && c.timeMs === timeMs, ); - if (existing) { + if (existing && existing.frame.width > 0) { existing.lastAccessed = Date.now(); return existing.frame; } + // If found but closed, remove the stale entry + if (existing) { + this.frameCache = this.frameCache.filter((c) => c !== existing); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/core/managers/media-manager.ts` around lines 161 - 166, The cache-return path in the media manager that returns a cached ImageBitmap (the method described by the comment and using FRAME_CACHE_MAX_SIZE) should defensively detect a closed bitmap by checking its dimensions (e.g., frame.width === 0) on cache hit; if the cached ImageBitmap is closed, remove/evict that entry from the cache, close it if necessary, and proceed to decode/create a fresh ImageBitmap before returning it so callers never receive an invalid (zero-dimension) bitmap. Ensure this check is added where the cached frame is accessed (the function that retrieves/returns the cached ImageBitmap) and that eviction/cleanup uses the same cache removal logic currently tied to FRAME_CACHE_MAX_SIZE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/core/managers/media-manager.ts`:
- Around line 167-192: Concurrent callers to getFrameAt can both miss frameCache
and decode/push duplicates; add a pending-decode map (e.g., pendingDecodes:
Map<string, Promise<ImageBitmap>>) keyed by `${assetId}:${timeMs}` to coalesce
inflight decodes: first check frameCache, then check pendingDecodes and if
present await it and update lastAccessed on the cached entry before returning;
if not present, create and store a decode promise in pendingDecodes, await it,
then remove the key, perform the bounded-cache eviction via evictLruFrame only
once before pushing the single resolved frame into frameCache (respecting
FRAME_CACHE_MAX_SIZE), and ensure pendingDecodes entries are removed in finally
to avoid leaks.
- Around line 194-204: Remove the leaked ImageBitmap frames by calling
MediaManager.evictFramesForAsset from RemoveMediaAssetCommand.execute: after the
spot where videoCache.clearVideo(...) is invoked, add a call to
editor.media.evictFramesForAsset({ assetId: this.assetId }) so cached frames for
the removed asset are closed and removed; ensure you reference the existing
methods evictFramesForAsset and videoCache.clearVideo and pass this.assetId
exactly as the assetId parameter.
---
Nitpick comments:
In `@apps/web/src/core/managers/media-manager.ts`:
- Around line 161-166: The cache-return path in the media manager that returns a
cached ImageBitmap (the method described by the comment and using
FRAME_CACHE_MAX_SIZE) should defensively detect a closed bitmap by checking its
dimensions (e.g., frame.width === 0) on cache hit; if the cached ImageBitmap is
closed, remove/evict that entry from the cache, close it if necessary, and
proceed to decode/create a fresh ImageBitmap before returning it so callers
never receive an invalid (zero-dimension) bitmap. Ensure this check is added
where the cached frame is accessed (the function that retrieves/returns the
cached ImageBitmap) and that eviction/cleanup uses the same cache removal logic
currently tied to FRAME_CACHE_MAX_SIZE.
🪄 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
Run ID: af45e0ec-47fe-4dfe-9c91-a19c091eddbd
📒 Files selected for processing (1)
apps/web/src/core/managers/media-manager.ts
Comment on lines
+167
to
+192
| async getFrameAt({ | ||
| assetId, | ||
| timeMs, | ||
| decode, | ||
| }: { | ||
| assetId: string; | ||
| timeMs: number; | ||
| decode: () => Promise<ImageBitmap>; | ||
| }): Promise<ImageBitmap> { | ||
| const existing = this.frameCache.find( | ||
| (c) => c.assetId === assetId && c.timeMs === timeMs, | ||
| ); | ||
| if (existing) { | ||
| existing.lastAccessed = Date.now(); | ||
| return existing.frame; | ||
| } | ||
|
|
||
| const frame = await decode(); | ||
|
|
||
| if (this.frameCache.length >= FRAME_CACHE_MAX_SIZE) { | ||
| this.evictLruFrame(); | ||
| } | ||
|
|
||
| this.frameCache.push({ frame, assetId, timeMs, lastAccessed: Date.now() }); | ||
| return frame; | ||
| } |
Contributor
There was a problem hiding this comment.
Race condition allows duplicate entries and unbounded cache growth.
Concurrent calls to getFrameAt() for the same (assetId, timeMs) can both miss the cache check, both invoke decode(), and both push to frameCache. This breaks the bounded-size guarantee and wastes decode work.
Consider tracking in-flight decode operations to coalesce concurrent requests:
🛠️ Proposed fix using a pending-decode map
export class MediaManager {
private assets: MediaAsset[] = [];
private isLoading = false;
private listeners = new Set<() => void>();
private frameCache: CachedFrame[] = [];
+ private pendingDecodes = new Map<string, Promise<ImageBitmap>>();
+
+ private frameCacheKey(assetId: string, timeMs: number): string {
+ return `${assetId}:${timeMs}`;
+ }
async getFrameAt({
assetId,
timeMs,
decode,
}: {
assetId: string;
timeMs: number;
decode: () => Promise<ImageBitmap>;
}): Promise<ImageBitmap> {
const existing = this.frameCache.find(
(c) => c.assetId === assetId && c.timeMs === timeMs,
);
if (existing) {
existing.lastAccessed = Date.now();
return existing.frame;
}
+ const key = this.frameCacheKey(assetId, timeMs);
+ const pending = this.pendingDecodes.get(key);
+ if (pending) {
+ return pending;
+ }
+
- const frame = await decode();
+ const decodePromise = decode().then((frame) => {
+ this.pendingDecodes.delete(key);
+ if (this.frameCache.length >= FRAME_CACHE_MAX_SIZE) {
+ this.evictLruFrame();
+ }
+ this.frameCache.push({ frame, assetId, timeMs, lastAccessed: Date.now() });
+ return frame;
+ });
+ this.pendingDecodes.set(key, decodePromise);
+ return decodePromise;
-
- if (this.frameCache.length >= FRAME_CACHE_MAX_SIZE) {
- this.evictLruFrame();
- }
-
- this.frameCache.push({ frame, assetId, timeMs, lastAccessed: Date.now() });
- return frame;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/core/managers/media-manager.ts` around lines 167 - 192,
Concurrent callers to getFrameAt can both miss frameCache and decode/push
duplicates; add a pending-decode map (e.g., pendingDecodes: Map<string,
Promise<ImageBitmap>>) keyed by `${assetId}:${timeMs}` to coalesce inflight
decodes: first check frameCache, then check pendingDecodes and if present await
it and update lastAccessed on the cached entry before returning; if not present,
create and store a decode promise in pendingDecodes, await it, then remove the
key, perform the bounded-cache eviction via evictLruFrame only once before
pushing the single resolved frame into frameCache (respecting
FRAME_CACHE_MAX_SIZE), and ensure pendingDecodes entries are removed in finally
to avoid leaks.
Comment on lines
+194
to
+204
| evictFramesForAsset({ assetId }: { assetId: string }): void { | ||
| const remaining: CachedFrame[] = []; | ||
| for (const entry of this.frameCache) { | ||
| if (entry.assetId === assetId) { | ||
| entry.frame.close(); | ||
| } else { | ||
| remaining.push(entry); | ||
| } | ||
| } | ||
| this.frameCache = remaining; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that evictFramesForAsset is not called in RemoveMediaAssetCommand
rg -n 'evictFramesForAsset' apps/web/src/lib/commands/media/remove-media-asset.ts
# Check all call sites of evictFramesForAsset to confirm it's only called internally
rg -n 'evictFramesForAsset' --type tsRepository: OpenCut-app/OpenCut
Length of output: 173
🏁 Script executed:
# Check the RemoveMediaAssetCommand implementation
cat -n apps/web/src/lib/commands/media/remove-media-asset.tsRepository: OpenCut-app/OpenCut
Length of output: 3518
🏁 Script executed:
# Also check if there are other places where assets are removed
rg -n 'clearVideo|evictFrames' apps/web/src/core/managers/media-manager.tsRepository: OpenCut-app/OpenCut
Length of output: 129
🏁 Script executed:
# Check how mediaManager is used in the codebase
rg -n 'mediaManager\.(clearVideo|evictFrames)' --type tsRepository: OpenCut-app/OpenCut
Length of output: 45
🏁 Script executed:
# Check where mediaManager is instantiated and used
rg -n 'mediaManager' apps/web/src/core/managers/media-manager.ts | head -20
# Find all references to mediaManager in the codebase
rg -n 'mediaManager' --type ts | head -30
# Check how frameCache is populated
rg -n 'frameCache' apps/web/src/core/managers/media-manager.ts
# Search for where frame caching happens
rg -n 'new CachedFrame\|frameCache\.push\|this\.frameCache' apps/web/src/core/managers/media-manager.tsRepository: OpenCut-app/OpenCut
Length of output: 728
🏁 Script executed:
# Search for mediaManager instantiation and exports
rg -n 'export.*mediaManager|new MediaManager' --type ts
# Check the EditorCore to see if mediaManager is a property
rg -n 'mediaManager' apps/web/src/core/*.ts
# Find where frames are actually being cached (cacheFrame method call)
rg -n 'cacheFrame|getFrame' --type ts | grep -v node_modules | head -30Repository: OpenCut-app/OpenCut
Length of output: 1899
🏁 Script executed:
# Check the getFrameAt method implementation in media-manager to see if it calls cacheFrame
sed -n '167,190p' apps/web/src/core/managers/media-manager.ts
# Search for all calls to cacheFrame
rg -n 'cacheFrame' --type ts
# Check video-node.ts to see how getFrameAt is used
sed -n '15,35p' apps/web/src/services/renderer/nodes/video-node.ts
# Check blur-background-node.ts similarly
sed -n '60,75p' apps/web/src/services/renderer/nodes/blur-background-node.tsRepository: OpenCut-app/OpenCut
Length of output: 1402
🏁 Script executed:
# Check the relationship between videoCache and mediaManager
cat -n apps/web/src/services/video-cache/service.ts | head -50
# See if videoCache is related to mediaManager
rg -n 'MediaManager\|frameCache' apps/web/src/services/video-cache/service.ts
# Check the imports and exports from media-manager.ts
head -30 apps/web/src/core/managers/media-manager.tsRepository: OpenCut-app/OpenCut
Length of output: 2391
🏁 Script executed:
# Verify how videoCache is cleared and whether it relates to MediaManager
sed -n '1,100p' apps/web/src/services/video-cache/service.ts | grep -A 10 "clearVideo"
# Check if mediaManager is accessible from RemoveMediaAssetCommand
grep -n "editor.media" apps/web/src/lib/commands/media/remove-media-asset.ts
# Verify the complete clearVideo implementation
rg -A 15 "clearVideo" apps/web/src/services/video-cache/service.tsRepository: OpenCut-app/OpenCut
Length of output: 789
Call evictFramesForAsset() in RemoveMediaAssetCommand to clean up cached frames.
The evictFramesForAsset() method correctly clears ImageBitmap frames from the MediaManager's cache, but RemoveMediaAssetCommand.execute() never calls it when removing an asset. While it calls videoCache.clearVideo() to clear the video processing cache, the frame cache in MediaManager is left untouched, causing ImageBitmap objects for deleted assets to leak memory. Add editor.media.evictFramesForAsset({ assetId: this.assetId }) in RemoveMediaAssetCommand.execute() after line 43.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/core/managers/media-manager.ts` around lines 194 - 204, Remove
the leaked ImageBitmap frames by calling MediaManager.evictFramesForAsset from
RemoveMediaAssetCommand.execute: after the spot where videoCache.clearVideo(...)
is invoked, add a call to editor.media.evictFramesForAsset({ assetId:
this.assetId }) so cached frames for the removed asset are closed and removed;
ensure you reference the existing methods evictFramesForAsset and
videoCache.clearVideo and pass this.assetId exactly as the assetId parameter.
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.
Code Quality
Problem
Playback crash symptoms (short freeze, then browser/tab crash) strongly match an OOM pattern from preloading or retaining too many decoded video frames (
ImageBitmap/raw RGBA buffers). The media manager should stop eager full-clip frame extraction and move to bounded caching with explicit disposal. This change should make video decoding demand-driven (current/nearby frames only), and ensure old frames are evicted and closed.Severity:
criticalFile:
apps/web/src/core/managers/media-manager.tsSolution
Introduce a frame-provider API in
MediaManagerthat:Changes
apps/web/src/core/managers/media-manager.ts(modified)We are not currently accepting PRs except for critical bugs.
If this is a bug fix:
If this is a feature:
This PR will be closed. Please open an issue to discuss first.Contributed by Lê Thành Chỉnh
Code is a tool. Mindset is the real value.
Closes #663
Summary by CodeRabbit
New Features
Performance Improvements