From aa27e7cbd1735e60dc9b1677e1e6abd0502c8fe0 Mon Sep 17 00:00:00 2001 From: webadderall <131426131+webadderall@users.noreply.github.com> Date: Fri, 17 Apr 2026 00:17:54 +1000 Subject: [PATCH] fix: address remaining export pipeline review feedback --- electron/cursorHider.ts | 5 +- src/lib/exporter/audioEncoder.ts | 26 +++++--- src/lib/exporter/forwardFrameSource.ts | 13 ++++ src/lib/exporter/frameRenderer.test.ts | 76 +++++++++++++++++------ src/lib/exporter/modernVideoExporter.ts | 5 ++ src/lib/exporter/videoExporter.ts | 82 +++++++++++++++++++++++-- 6 files changed, 170 insertions(+), 37 deletions(-) diff --git a/electron/cursorHider.ts b/electron/cursorHider.ts index 35dbef8b..266d111a 100644 --- a/electron/cursorHider.ts +++ b/electron/cursorHider.ts @@ -133,11 +133,12 @@ export function showCursor() { try { const didShow = runPythonSnippet(PY_SHOW_WIN) || runPowerShellSnippet(getPowerShellCommand(true)); + if (didShow) { + cursorHidden = false; + } return didShow; } catch (error) { console.error("[cursorHider] Failed to show Windows cursor:", error); return false; - } finally { - cursorHidden = false; } } diff --git a/src/lib/exporter/audioEncoder.ts b/src/lib/exporter/audioEncoder.ts index 2604fbd2..16d919f3 100644 --- a/src/lib/exporter/audioEncoder.ts +++ b/src/lib/exporter/audioEncoder.ts @@ -64,11 +64,12 @@ export class AudioProcessor { return false; } - const reader = audioStream.getReader(); + let reader: ReadableStreamDefaultReader | null = null; let wroteAudio = false; let passthroughTimestampOffsetUs: number | null = null; try { + reader = audioStream.getReader(); while (!this.cancelled) { const { done, value: chunk } = await reader.read(); if (done || !chunk) break; @@ -97,10 +98,12 @@ export class AudioProcessor { wroteAudio = true; } } finally { - try { - await reader.cancel(); - } catch { - // reader already closed + if (reader) { + try { + await reader.cancel(); + } catch { + // reader already closed + } } } @@ -424,9 +427,10 @@ export class AudioProcessor { }); decoder.configure(audioConfig); - const reader = audioStream.getReader(); + let reader: ReadableStreamDefaultReader | null = null; try { + reader = audioStream.getReader(); while (!this.cancelled) { failIfNeeded(); @@ -473,10 +477,12 @@ export class AudioProcessor { await pendingMuxing; failIfNeeded(); } finally { - try { - await reader.cancel(); - } catch { - // reader already closed + if (reader) { + try { + await reader.cancel(); + } catch { + // reader already closed + } } cleanupPendingFrames(); diff --git a/src/lib/exporter/forwardFrameSource.ts b/src/lib/exporter/forwardFrameSource.ts index d73a96f9..eeb9f7e6 100644 --- a/src/lib/exporter/forwardFrameSource.ts +++ b/src/lib/exporter/forwardFrameSource.ts @@ -46,6 +46,9 @@ export class ForwardFrameSource { try { const url = new URL(resourceUrl); let filePath = decodeURIComponent(url.pathname); + if (url.host && url.host !== "localhost") { + return `//${url.host}${filePath}`; + } if (/^\/[A-Za-z]:/.test(filePath)) { filePath = filePath.slice(1); } @@ -345,6 +348,16 @@ export class ForwardFrameSource { cancel(): void { this.cancelled = true; + if (this.frameResolve) { + const resolve = this.frameResolve; + this.frameResolve = null; + resolve(null); + } + if (this.reader) { + void this.reader.cancel().catch(() => { + // Ignore cancellation errors during shutdown. + }); + } } async destroy(): Promise { diff --git a/src/lib/exporter/frameRenderer.test.ts b/src/lib/exporter/frameRenderer.test.ts index f3e87ac8..3d6629d7 100644 --- a/src/lib/exporter/frameRenderer.test.ts +++ b/src/lib/exporter/frameRenderer.test.ts @@ -59,6 +59,38 @@ vi.mock("@/components/video-editor/videoPlayback/cursorRenderer", () => ({ import { FrameRenderer } from "./frameRenderer"; +type MockFunction = ReturnType; +type MockContext = { + beginPath: MockFunction; + moveTo: MockFunction; + lineTo: MockFunction; + closePath: MockFunction; + clip: MockFunction; + drawImage: MockFunction; + save: MockFunction; + restore: MockFunction; + translate: MockFunction; + scale: MockFunction; + clearRect: MockFunction; + filter: string; +}; +type MockCanvas = ReturnType; +type FrameRendererTestAccess = { + webcamVideoElement: FakeVideoElement | null; + webcamSeekPromise: Promise | null; + webcamFrameCacheCanvas: MockCanvas | null; + webcamFrameCacheCtx: CanvasRenderingContext2D | null; + lastSyncedWebcamTime: number | null; + currentVideoTime: number; + animationState: { appliedScale: number }; + syncWebcamFrame: (targetTimeSec: number) => Promise; + drawWebcamOverlay: ( + outputCtx: CanvasRenderingContext2D, + outputWidth: number, + outputHeight: number, + ) => void; +}; + type Listener = { callback: () => void; once: boolean; @@ -169,7 +201,7 @@ function createMockContext() { scale: vi.fn(), clearRect: vi.fn(), filter: "", - } as unknown as CanvasRenderingContext2D; + }; } function createMockCanvas() { @@ -178,7 +210,7 @@ function createMockCanvas() { width: 0, height: 0, context, - getContext: vi.fn((_type?: string) => context), + getContext: vi.fn((_type?: string) => context as unknown as CanvasRenderingContext2D), }; } @@ -235,7 +267,7 @@ describe("FrameRenderer webcam export path", () => { }); it("clamps webcam sync seeks to the media duration", async () => { - const renderer = createRenderer() as any; + const renderer = createRenderer() as unknown as FrameRendererTestAccess; const webcamVideo = new FakeVideoElement({ duration: 4.5, currentTime: 0.25 }); renderer.webcamVideoElement = webcamVideo; @@ -247,7 +279,7 @@ describe("FrameRenderer webcam export path", () => { }); it("falls back to animation frame when requestVideoFrameCallback does not fire", async () => { - const renderer = createRenderer() as any; + const renderer = createRenderer() as unknown as FrameRendererTestAccess; const webcamVideo = new FakeVideoElement({ duration: 4.5, currentTime: 0.25, @@ -269,7 +301,7 @@ describe("FrameRenderer webcam export path", () => { }); it("uses the cached webcam frame when the live video is out of sync", () => { - const renderer = createRenderer() as any; + const renderer = createRenderer() as unknown as FrameRendererTestAccess; const outputContext = createMockContext(); const webcamVideo = new FakeVideoElement({ currentTime: 2, @@ -288,16 +320,18 @@ describe("FrameRenderer webcam export path", () => { renderer.currentVideoTime = 2; renderer.animationState.appliedScale = 1; - renderer.drawWebcamOverlay(outputContext, 1280, 720); + renderer.drawWebcamOverlay(outputContext as unknown as CanvasRenderingContext2D, 1280, 720); const bubbleCanvas = createdCanvases[0]; expect(bubbleCanvas).toBeDefined(); - expect((bubbleCanvas.context.drawImage as any).mock.calls[0][0]).toBe(cachedFrameCanvas); - expect((outputContext.drawImage as any).mock.calls[0][0]).toBe(bubbleCanvas); + expect((bubbleCanvas.context as MockContext).drawImage.mock.calls[0][0]).toBe( + cachedFrameCanvas, + ); + expect((outputContext as MockContext).drawImage.mock.calls[0][0]).toBe(bubbleCanvas); }); it("keeps drawing the cached webcam frame when the live element temporarily has no current data", () => { - const renderer = createRenderer() as any; + const renderer = createRenderer() as unknown as FrameRendererTestAccess; const outputContext = createMockContext(); const webcamVideo = new FakeVideoElement({ currentTime: 2, @@ -316,16 +350,18 @@ describe("FrameRenderer webcam export path", () => { renderer.currentVideoTime = 2; renderer.animationState.appliedScale = 1; - renderer.drawWebcamOverlay(outputContext, 1280, 720); + renderer.drawWebcamOverlay(outputContext as unknown as CanvasRenderingContext2D, 1280, 720); const bubbleCanvas = createdCanvases[0]; expect(bubbleCanvas).toBeDefined(); - expect((bubbleCanvas.context.drawImage as any).mock.calls[0][0]).toBe(cachedFrameCanvas); - expect((outputContext.drawImage as any).mock.calls[0][0]).toBe(bubbleCanvas); + expect((bubbleCanvas.context as MockContext).drawImage.mock.calls[0][0]).toBe( + cachedFrameCanvas, + ); + expect((outputContext as MockContext).drawImage.mock.calls[0][0]).toBe(bubbleCanvas); }); it("uses the live webcam frame and refreshes the cache when the video is synchronized", () => { - const renderer = createRenderer() as any; + const renderer = createRenderer() as unknown as FrameRendererTestAccess; const outputContext = createMockContext(); const webcamVideo = new FakeVideoElement({ currentTime: 2, @@ -339,18 +375,18 @@ describe("FrameRenderer webcam export path", () => { renderer.currentVideoTime = 2; renderer.animationState.appliedScale = 1; - renderer.drawWebcamOverlay(outputContext, 1280, 720); + renderer.drawWebcamOverlay(outputContext as unknown as CanvasRenderingContext2D, 1280, 720); const bubbleCanvas = createdCanvases[0]; const cacheCanvas = createdCanvases[1]; expect(cacheCanvas).toBeDefined(); - expect((cacheCanvas.context.drawImage as any).mock.calls[0][0]).toBe(webcamVideo); - expect((bubbleCanvas.context.drawImage as any).mock.calls[0][0]).toBe(cacheCanvas); - expect((outputContext.drawImage as any).mock.calls[0][0]).toBe(bubbleCanvas); + expect((cacheCanvas.context as MockContext).drawImage.mock.calls[0][0]).toBe(webcamVideo); + expect((bubbleCanvas.context as MockContext).drawImage.mock.calls[0][0]).toBe(cacheCanvas); + expect((outputContext as MockContext).drawImage.mock.calls[0][0]).toBe(bubbleCanvas); }); it("reuses the webcam bubble canvas across frames", () => { - const renderer = createRenderer() as any; + const renderer = createRenderer() as unknown as FrameRendererTestAccess; const outputContext = createMockContext(); const webcamVideo = new FakeVideoElement({ currentTime: 2, @@ -364,8 +400,8 @@ describe("FrameRenderer webcam export path", () => { renderer.currentVideoTime = 2; renderer.animationState.appliedScale = 1; - renderer.drawWebcamOverlay(outputContext, 1280, 720); - renderer.drawWebcamOverlay(outputContext, 1280, 720); + renderer.drawWebcamOverlay(outputContext as unknown as CanvasRenderingContext2D, 1280, 720); + renderer.drawWebcamOverlay(outputContext as unknown as CanvasRenderingContext2D, 1280, 720); expect(createdCanvases).toHaveLength(2); }); diff --git a/src/lib/exporter/modernVideoExporter.ts b/src/lib/exporter/modernVideoExporter.ts index 05a17054..e2cc41cc 100644 --- a/src/lib/exporter/modernVideoExporter.ts +++ b/src/lib/exporter/modernVideoExporter.ts @@ -912,6 +912,11 @@ export class ModernVideoExporter { throw new Error(`${NATIVE_EXPORT_ENGINE_NAME} export session is not active`); } if (this.nativeEncoderError) throw this.nativeEncoderError; + while (this.nativeWritePromises.size >= this.maxNativeWriteInFlight) { + await this.awaitOldestNativeWrite(); + if (this.cancelled) return; + if (this.nativeEncoderError) throw this.nativeEncoderError; + } while ( this.nativeH264Encoder.encodeQueueSize >= ModernVideoExporter.NATIVE_ENCODER_QUEUE_LIMIT diff --git a/src/lib/exporter/videoExporter.ts b/src/lib/exporter/videoExporter.ts index 2ba64712..c81d6c79 100644 --- a/src/lib/exporter/videoExporter.ts +++ b/src/lib/exporter/videoExporter.ts @@ -103,6 +103,9 @@ export class VideoExporter { private nativeExportSessionId: string | null = null; private nativeH264Encoder: VideoEncoder | null = null; private nativePendingWrite: Promise = Promise.resolve(); + private nativeWritePromises = new Set>(); + private nativeWriteError: Error | null = null; + private maxNativeWriteInFlight = 1; private nativeEncoderError: Error | null = null; constructor(config: VideoExporterConfig) { @@ -116,6 +119,12 @@ export class VideoExporter { this.encoderError = null; this.nativeEncoderError = null; this.nativePendingWrite = Promise.resolve(); + this.nativeWritePromises = new Set(); + this.nativeWriteError = null; + this.maxNativeWriteInFlight = Math.max( + 1, + Math.floor(this.config.maxInFlightNativeWrites ?? 1), + ); this.exportStartTimeMs = this.getNowMs(); this.progressSampleStartTimeMs = this.exportStartTimeMs; this.progressSampleStartFrame = 0; @@ -260,7 +269,7 @@ export class VideoExporter { if (useNativeEncoder && nativeAudioPlan) { if (this.nativeH264Encoder) { await this.nativeH264Encoder.flush(); - await this.nativePendingWrite; + await this.awaitPendingNativeWrites(); if (this.nativeEncoderError) { throw this.nativeEncoderError; } @@ -541,6 +550,12 @@ export class VideoExporter { this.nativeExportSessionId = result.sessionId; this.nativePendingWrite = Promise.resolve(); + this.nativeWritePromises = new Set(); + this.nativeWriteError = null; + this.maxNativeWriteInFlight = Math.max( + 1, + Math.floor(this.config.maxInFlightNativeWrites ?? 1), + ); // Initialize the browser-side H.264 encoder (hardware-accelerated where available). // Encoded Annex B chunks are sent over IPC and FFmpeg stream-copies them into MP4. @@ -550,7 +565,7 @@ export class VideoExporter { if (this.cancelled || !this.nativeExportSessionId) return; const buffer = new ArrayBuffer(chunk.byteLength); chunk.copyTo(buffer); - this.nativePendingWrite = this.nativePendingWrite + const writePromise = (this.nativePendingWrite = this.nativePendingWrite .then(async () => { const writeResult = await window.electronAPI.nativeVideoExportWriteFrame( sessionId, @@ -567,7 +582,12 @@ export class VideoExporter { this.nativeEncoderError = error instanceof Error ? error : new Error(String(error)); } - }); + if (!this.cancelled && !this.nativeWriteError) { + this.nativeWriteError = + error instanceof Error ? error : new Error(String(error)); + } + })); + this.trackNativeWritePromise(writePromise); }, error: (e) => { this.nativeEncoderError = e; @@ -607,16 +627,37 @@ export class VideoExporter { } if (this.nativeEncoderError) throw this.nativeEncoderError; + if (this.nativeWriteError) throw this.nativeWriteError; + + while (this.nativeWritePromises.size >= this.maxNativeWriteInFlight && !this.cancelled) { + await this.awaitOldestNativeWrite(); + if (this.nativeEncoderError) throw this.nativeEncoderError; + if (this.nativeWriteError) throw this.nativeWriteError; + } // Apply backpressure: don't queue too far ahead of FFmpeg's stdin pipe - while (this.nativeH264Encoder.encodeQueueSize >= 32) { + while ( + this.nativeH264Encoder.encodeQueueSize >= + Math.max(1, Math.floor(this.config.maxEncodeQueue ?? DEFAULT_MAX_ENCODE_QUEUE)) + ) { await new Promise((r) => setTimeout(r, 2)); if (this.cancelled) return; if (this.nativeEncoderError) throw this.nativeEncoderError; + if (this.nativeWriteError) throw this.nativeWriteError; } const canvas = this.renderer!.getCanvas(); - const frame = new VideoFrame(canvas, { timestamp, duration: frameDuration }); + // @ts-expect-error - colorSpace not in TypeScript definitions but works at runtime + const frame = new VideoFrame(canvas, { + timestamp, + duration: frameDuration, + colorSpace: { + primaries: "bt709", + transfer: "iec61966-2-1", + matrix: "rgb", + fullRange: true, + }, + }); this.nativeH264Encoder.encode(frame, { keyFrame: frameIndex % 300 === 0 }); frame.close(); } @@ -787,6 +828,37 @@ export class VideoExporter { exportFrame.close(); } + private trackNativeWritePromise(writePromise: Promise): void { + this.nativeWritePromises.add(writePromise); + + void writePromise.finally(() => { + this.nativeWritePromises.delete(writePromise); + }); + } + + private async awaitOldestNativeWrite(): Promise { + const oldestWritePromise = this.nativeWritePromises.values().next().value; + if (!oldestWritePromise) { + return; + } + + await oldestWritePromise; + + if (this.nativeWriteError) { + throw this.nativeWriteError; + } + } + + private async awaitPendingNativeWrites(): Promise { + while (this.nativeWritePromises.size > 0) { + await this.awaitOldestNativeWrite(); + } + + if (this.nativeWriteError) { + throw this.nativeWriteError; + } + } + private reportFinalizingProgress( totalFrames: number, renderProgress: number,