Skip to content

refactor(web): replace unbounded preview frame buffering with bounded, disposable frame access #758

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chinhkrb113 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor(web): replace unbounded preview frame buffering with bounded, disposable frame access #758

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions apps/web/src/core/managers/media-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,20 @@ import { generateUUID } from "@/utils/id";
import { videoCache } from "@/services/video-cache/service";
import { BatchCommand, RemoveMediaAssetCommand } from "@/lib/commands";

const FRAME_CACHE_MAX_SIZE = 8;

interface CachedFrame {
frame: ImageBitmap;
assetId: string;
timeMs: number;
lastAccessed: number;
}

export class MediaManager {
private assets: MediaAsset[] = [];
private isLoading = false;
private listeners = new Set<() => void>();
private frameCache: CachedFrame[] = [];

constructor(private editor: EditorCore) {}

Expand Down Expand Up @@ -120,6 +130,7 @@ export class MediaManager {

clearAllAssets(): void {
videoCache.clearAll();
this.evictAllFrames();

this.assets.forEach((asset) => {
if (asset.url) {
Expand Down Expand Up @@ -147,6 +158,70 @@ export class MediaManager {
return this.isLoading;
}

/**
* Returns a cached ImageBitmap for the given asset and time, or decodes and
* caches a new one. The caller must NOT close the returned bitmap — the cache
* owns its lifetime. Old entries are evicted (and closed) when the cache
* exceeds FRAME_CACHE_MAX_SIZE.
*/
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;
}
Comment on lines +167 to +192
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


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;
}
Comment on lines +194 to +204
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 ts

Repository: OpenCut-app/OpenCut

Length of output: 173

🏁 Script executed:

# Check the RemoveMediaAssetCommand implementation
cat -n apps/web/src/lib/commands/media/remove-media-asset.ts

Repository: 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.ts

Repository: OpenCut-app/OpenCut

Length of output: 129

🏁 Script executed:

# Check how mediaManager is used in the codebase
rg -n 'mediaManager\.(clearVideo|evictFrames)' --type ts

Repository: 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.ts

Repository: 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 -30

Repository: 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.ts

Repository: 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.ts

Repository: 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.ts

Repository: 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.


private evictLruFrame(): void {
if (this.frameCache.length === 0) return;
let lruIndex = 0;
for (let i = 1; i < this.frameCache.length; i++) {
if (this.frameCache[i].lastAccessed < this.frameCache[lruIndex].lastAccessed) {
lruIndex = i;
}
}
this.frameCache[lruIndex].frame.close();
this.frameCache.splice(lruIndex, 1);
}

private evictAllFrames(): void {
for (const entry of this.frameCache) {
entry.frame.close();
}
this.frameCache = [];
}

subscribe(listener: () => void): () => void {
this.listeners.add(listener);
return () => this.listeners.delete(listener);
Expand Down