From b0d6486bbb6dff0ef232b64b4d1a074cda7eaf3e Mon Sep 17 00:00:00 2001 From: Patrick Lu Date: Tue, 8 Sep 2026 18:24:37 -0700 Subject: [PATCH] fix(preview): preserve active video buffers after preload timeout --- packages/freecut-editor/package.json | 2 +- .../preview/hooks/use-custom-player.test.tsx | 117 +++++++++++++++++- .../preview/hooks/use-custom-player.ts | 37 +++--- .../player/video/VideoSourcePool.test.ts | 104 ++++++++++++++++ src/runtime/player/video/VideoSourcePool.ts | 42 ++++++- 5 files changed, 280 insertions(+), 22 deletions(-) diff --git a/packages/freecut-editor/package.json b/packages/freecut-editor/package.json index 76cf36a7e..c0b4384ff 100644 --- a/packages/freecut-editor/package.json +++ b/packages/freecut-editor/package.json @@ -1,6 +1,6 @@ { "name": "@quantfive/freecut-editor-surface", - "version": "0.3.15", + "version": "0.3.16", "description": "The host-backed FreeCut browser editor surface.", "license": "MIT", "repository": { diff --git a/src/features/preview/hooks/use-custom-player.test.tsx b/src/features/preview/hooks/use-custom-player.test.tsx index d693b5981..cdadd612a 100644 --- a/src/features/preview/hooks/use-custom-player.test.tsx +++ b/src/features/preview/hooks/use-custom-player.test.tsx @@ -1,10 +1,41 @@ -import { cleanup, renderHook } from '@testing-library/react' -import { afterEach, describe, expect, it, vi } from 'vite-plus/test' +import { act, cleanup, renderHook } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vite-plus/test' import { usePlaybackStore } from '@/shared/state/playback' import { useEditorStore } from '@/shared/state/editor' +import { useTimelineSettingsStore } from '@/features/preview/deps/timeline-store' +import { ClockBridgeProvider, useClock } from '../deps/player' import { useCustomPlayer } from './use-custom-player' +function createPlayer(initialFrame = 0) { + const { result } = renderHook(() => useClock(), { + wrapper: ({ children }) => ( + + {children} + + ), + }) + const clock = result.current + return { + clock, + player: { + seekTo: vi.fn((frame: number) => clock.seekToFrame(frame)), + play: vi.fn(() => clock.play()), + pause: vi.fn(() => clock.pause()), + getCurrentFrame: () => clock.currentFrame, + isPlaying: () => clock.isPlaying, + setPlaybackRate: vi.fn((rate: number) => { + clock.playbackRate = rate + }), + }, + } +} + +beforeEach(() => { + useTimelineSettingsStore.setState({ isTimelineLoading: false }) +}) + afterEach(() => { + vi.useRealTimers() cleanup() usePlaybackStore.setState({ isPlaying: false }) useEditorStore.setState({ hostMode: false }) @@ -32,3 +63,85 @@ describe('preview mount transport ownership', () => { expect(usePlaybackStore.getState().currentFrame).toBe(42) }) }) + +describe('preview Player transport reconciliation', () => { + it('starts a fresh host Player at the preserved frame and advances its Clock', () => { + vi.useFakeTimers() + useEditorStore.setState({ hostMode: true }) + usePlaybackStore.setState({ isPlaying: true, currentFrame: 42 }) + const { player, clock } = createPlayer() + renderHook(() => useCustomPlayer({ current: player })) + expect(clock.isPlaying).toBe(true) + expect(clock.currentFrame).toBe(42) + act(() => { + vi.advanceTimersByTime(200) + }) + expect(clock.currentFrame).toBeGreaterThan(42) + expect(player.play).toHaveBeenCalledTimes(1) + act(() => { + usePlaybackStore.getState().pause() + }) + expect(clock.isPlaying).toBe(false) + }) + + it('reconciles a Player that becomes available after the former readiness timeout', () => { + vi.useFakeTimers() + useEditorStore.setState({ hostMode: true }) + usePlaybackStore.setState({ isPlaying: false, currentFrame: 42 }) + const { player, clock } = createPlayer() + const playerRef: { current: typeof player | null } = { current: null } + renderHook(() => useCustomPlayer(playerRef)) + act(() => { + usePlaybackStore.setState({ isPlaying: true, currentFrame: 75 }) + vi.advanceTimersByTime(1500) + }) + playerRef.current = player + act(() => { + vi.advanceTimersByTime(50) + }) + expect(clock.isPlaying).toBe(true) + expect(clock.currentFrame).toBe(75) + expect(player.play).toHaveBeenCalledTimes(1) + }) + + it('uses the latest paused transport when a late Player becomes ready', () => { + vi.useFakeTimers() + useEditorStore.setState({ hostMode: true }) + usePlaybackStore.setState({ isPlaying: true, currentFrame: 42 }) + const { player, clock } = createPlayer() + const playerRef: { current: typeof player | null } = { current: null } + renderHook(() => useCustomPlayer(playerRef)) + act(() => { + usePlaybackStore.setState({ isPlaying: false, currentFrame: 75 }) + }) + clock.play() + playerRef.current = player + act(() => { + vi.advanceTimersByTime(50) + }) + expect(clock.isPlaying).toBe(false) + expect(clock.currentFrame).toBe(75) + expect(player.play).not.toHaveBeenCalled() + expect(player.pause).toHaveBeenCalledTimes(1) + }) + + it('keeps a fresh standalone Player paused without starting it during mount', () => { + usePlaybackStore.setState({ isPlaying: true, currentFrame: 42 }) + const { player, clock } = createPlayer() + renderHook(() => useCustomPlayer({ current: player })) + expect(usePlaybackStore.getState().isPlaying).toBe(false) + expect(clock.isPlaying).toBe(false) + expect(clock.currentFrame).toBe(42) + expect(player.play).not.toHaveBeenCalled() + }) + + it('does not replay a Player already following host transport', () => { + useEditorStore.setState({ hostMode: true }) + usePlaybackStore.setState({ isPlaying: true, currentFrame: 42 }) + const { player, clock } = createPlayer(42) + clock.play() + renderHook(() => useCustomPlayer({ current: player })) + expect(player.play).not.toHaveBeenCalled() + expect(clock.isPlaying).toBe(true) + }) +}) diff --git a/src/features/preview/hooks/use-custom-player.ts b/src/features/preview/hooks/use-custom-player.ts index 7fb105db8..787af331c 100644 --- a/src/features/preview/hooks/use-custom-player.ts +++ b/src/features/preview/hooks/use-custom-player.ts @@ -189,22 +189,20 @@ export function useCustomPlayer( // Detect when Player becomes ready useEffect(() => { - if (playerRef.current && !playerReady) { + if (playerReady) return + if (playerRef.current) { setPlayerReady(true) + return } + // Lazy preview mounts can take longer than a second. Keep checking until + // the actual Player exists, and stop on readiness or unmount. const checkReady = setInterval(() => { - if (playerRef.current && !playerReady) { + if (playerRef.current) { setPlayerReady(true) clearInterval(checkReady) } }, 50) - - const timeout = setTimeout(() => clearInterval(checkReady), 1000) - - return () => { - clearInterval(checkReady) - clearTimeout(timeout) - } + return () => clearInterval(checkReady) }, [playerRef, playerReady]) // Timeline → Player: Sync play/pause state @@ -219,17 +217,19 @@ export function useCustomPlayer( if (!playerRef.current) return const wasPlaying = wasPlayingRef.current - wasPlayingRef.current = isPlaying - const { currentFrame, setPreviewFrame } = usePlaybackStore.getState() + // Read live transport: the standalone mount effect may already have paused + // it, while this render still captured the previous playing value. + const { currentFrame, setPreviewFrame, isPlaying: shouldPlay } = usePlaybackStore.getState() + wasPlayingRef.current = shouldPlay const playbackPlan = planPlaybackStateCommand({ - wasPlaying, - isPlaying, + wasPlaying: playerRef.current.isPlaying(), + isPlaying: shouldPlay, currentFrame, playerFrame: getPlayerFrame(), }) try { - if (isPlaying && !wasPlaying) { + if (shouldPlay && !wasPlaying) { flushPreviewWarmSeek() } if (playbackPlan.clearPreviewFrame) { @@ -239,7 +239,14 @@ export function useCustomPlayer( } catch (error) { logger.error('Failed to control playback:', error) } - }, [isPlaying, playerRef, executePlayerCommand, flushPreviewWarmSeek, getPlayerFrame]) + }, [ + isPlaying, + playerReady, + playerRef, + executePlayerCommand, + flushPreviewWarmSeek, + getPlayerFrame, + ]) // Wait for timeline to finish loading before syncing frame position. // Without this, the Player would seek to frame 0 (the default) before diff --git a/src/runtime/player/video/VideoSourcePool.test.ts b/src/runtime/player/video/VideoSourcePool.test.ts index 889c5f9b3..343ab2c8f 100644 --- a/src/runtime/player/video/VideoSourcePool.test.ts +++ b/src/runtime/player/video/VideoSourcePool.test.ts @@ -8,6 +8,7 @@ type MutableVideoElement = HTMLVideoElement & { } function installVideoElementMocks() { + let autoLoad = true const createdVideos: MutableVideoElement[] = [] const originalCreateElement = document.createElement.bind(document) @@ -63,6 +64,7 @@ function installVideoElementMocks() { } video.load = vi.fn(() => { + if (!autoLoad) return queueMicrotask(() => { readyStateValue = 2 video.dispatchEvent(new Event('loadedmetadata')) @@ -82,6 +84,9 @@ function installVideoElementMocks() { return { createdVideos, + stallLoads: () => { + autoLoad = false + }, restore: () => createElementSpy.mockRestore(), } } @@ -98,6 +103,105 @@ describe('VideoSourcePool', () => { vi.useRealTimers() }) + it('keeps an assigned pending video intact after preload timeout so late data can play', async () => { + vi.useFakeTimers() + videoMocks.stallLoads() + const pool = new VideoSourcePool() + const loading = pool.preloadSource('blob:slow').catch((error: Error) => error) + const video = pool.acquireForClip('clip', 'blob:slow') as MutableVideoElement + video.__setPaused(false) + await vi.advanceTimersByTimeAsync(15_000) + expect(await loading).toMatchObject({ message: expect.stringContaining('timed out') }) + expect(video.getAttribute('src')).toBe('blob:slow') + expect(video.pause).not.toHaveBeenCalled() + expect(video.load).toHaveBeenCalledTimes(1) + expect(pool.getClipElement('clip')).toBe(video) + video.__setReadyState(4) + video.dispatchEvent(new Event('canplay')) + await pool.preloadSource('blob:slow') + pool.releaseClip('clip') + expect(pool.acquireForClip('next', 'blob:slow')).toBe(video) + expect(video.getAttribute('src')).toBe('blob:slow') + pool.dispose() + }) + + it('discards an unassigned timed-out video and permits a fresh preload', async () => { + vi.useFakeTimers() + videoMocks.stallLoads() + const pool = new VideoSourcePool() + const loading = pool.preloadSource('blob:slow').catch((error: Error) => error) + const first = videoMocks.createdVideos[0]! + await vi.advanceTimersByTimeAsync(15_000) + await loading + expect(first.getAttribute('src')).toBe('') + expect(pool.getStats().totalElements).toBe(0) + const retry = pool.preloadSource('blob:slow') + const next = videoMocks.createdVideos[1]! + next.dispatchEvent(new Event('canplay')) + await retry + expect(pool.acquireForClip('clip', 'blob:slow')).toBe(next) + pool.dispose() + }) + + it('defers failed assigned video disposal until its owner releases it', async () => { + videoMocks.stallLoads() + const pool = new VideoSourcePool() + const loading = pool.preloadSource('blob:broken').catch((error: Error) => error) + const video = pool.acquireForClip('clip', 'blob:broken')! + video.dispatchEvent(new Event('error')) + expect(await loading).toBeInstanceOf(Error) + expect(video.getAttribute('src')).toBe('blob:broken') + expect(pool.getClipElement('clip')).toBe(video) + pool.releaseClip('clip') + expect(video.getAttribute('src')).toBe('') + expect(pool.getStats().totalElements).toBe(0) + const retry = pool.preloadSource('blob:broken') + videoMocks.createdVideos[1]!.dispatchEvent(new Event('canplay')) + await retry + pool.dispose() + }) + + it('cleans up an unassigned media error before retrying', async () => { + videoMocks.stallLoads() + const pool = new VideoSourcePool() + const loading = pool.preloadSource('blob:broken').catch((error: Error) => error) + const video = videoMocks.createdVideos[0]! + video.dispatchEvent(new Event('error')) + expect(await loading).toBeInstanceOf(Error) + expect(video.getAttribute('src')).toBe('') + expect(pool.getStats().totalElements).toBe(0) + const retry = pool.preloadSource('blob:broken') + videoMocks.createdVideos[1]!.dispatchEvent(new Event('canplay')) + await retry + pool.dispose() + }) + + it('does not promote a ready video when disposal wins the completion microtask race', async () => { + videoMocks.stallLoads() + const pool = new VideoSourcePool() + const controller = pool.getSource('blob:pending') + const loading = pool.preloadSource('blob:pending').catch((error: Error) => error) + const video = videoMocks.createdVideos[0]! + video.dispatchEvent(new Event('canplay')) + pool.dispose() + expect(await loading).toMatchObject({ name: 'AbortError' }) + expect(controller.getElementCount()).toBe(0) + expect(video.getAttribute('src')).toBe('') + expect(video.pause).toHaveBeenCalledTimes(1) + }) + + it('settles a pending preload on disposal without reviving or disposing its video twice', async () => { + videoMocks.stallLoads() + const pool = new VideoSourcePool() + const loading = pool.preloadSource('blob:pending').catch((error: Error) => error) + const video = pool.acquireForClip('clip', 'blob:pending')! + pool.dispose() + expect(await loading).toMatchObject({ name: 'AbortError' }) + expect(video.pause).toHaveBeenCalledTimes(1) + expect(video.load).toHaveBeenCalledTimes(2) + expect(pool.getStats()).toEqual({ sourceCount: 0, totalElements: 0, activeClips: 0 }) + }) + it('ensures ready lanes and warms idle elements near transition boundaries', async () => { const pool = new VideoSourcePool() diff --git a/src/runtime/player/video/VideoSourcePool.ts b/src/runtime/player/video/VideoSourcePool.ts index 30be05208..962169f06 100644 --- a/src/runtime/player/video/VideoSourcePool.ts +++ b/src/runtime/player/video/VideoSourcePool.ts @@ -44,6 +44,9 @@ class SourceController { private overflow: HTMLVideoElement[] = [] private assignments: Map = new Map() private loadPromise: Promise | null = null + private cancelPendingLoad: (() => void) | null = null + private disposed = false + private failedAssignedElements = new Set() // Element being loaded by ensureLoaded() but not yet promoted to primary. // Allows acquire() to reuse it instead of creating a redundant overflow element. private _pendingPrimary: HTMLVideoElement | null = null @@ -81,6 +84,7 @@ class SourceController { * redundant overflow element (the common race on first mount). */ async ensureLoaded(): Promise { + if (this.disposed) throw createVideoPoolAbortError('source-disposed') if (this.primary) { return this.primary } @@ -120,6 +124,12 @@ class SourceController { } element.removeEventListener('canplay', onCanPlay) element.removeEventListener('error', onError) + this.cancelPendingLoad = null + } + + this.cancelPendingLoad = () => { + cleanup() + reject(createVideoPoolAbortError('source-disposed-during-load')) } element.addEventListener('canplay', onCanPlay) @@ -147,17 +157,25 @@ class SourceController { element.load() }) .then(() => { + if (this.disposed) throw createVideoPoolAbortError('source-disposed-before-ready') // acquire() may have already promoted _pendingPrimary to primary; // this is a harmless no-op in that case. this.primary = element this._pendingPrimary = null }) .catch((err) => { - // Tear down the failed element so it doesn't linger in memory - if (this._pendingPrimary === element) { - this._pendingPrimary = null + // acquire() can promote the loading element and mount it before + // preload settles. A timeout must not clear src or pause that owner's + // live element: late network/decode data may still make it playable. + if (!this.disposed && this.isElementInUse(element)) { + if (element.readyState < 3 || element.error) { + this.failedAssignedElements.add(element) + } + } else if (!this.disposed) { + if (this._pendingPrimary === element) this._pendingPrimary = null + if (this.primary === element) this.primary = null + this.disposeElement(element) } - this.disposeElement(element) // Allow retries by clearing the rejected promise this.loadPromise = null throw err @@ -254,7 +272,16 @@ class SourceController { * Release a clip's element back to the pool */ release(clipId: string): void { + const element = this.assignments.get(clipId) this.assignments.delete(clipId) + // Failed live lanes remain owned until unmounted. Remove them only after + // release; a late canplay clears this marker and preserves their buffers. + if (element && this.failedAssignedElements.has(element) && !this.isElementInUse(element)) { + if (this.primary === element) this.primary = null + if (this._pendingPrimary === element) this._pendingPrimary = null + this.overflow = this.overflow.filter((candidate) => candidate !== element) + this.disposeElement(element) + } this.pruneIdleOverflowElements() } @@ -316,6 +343,8 @@ class SourceController { * Dispose all elements */ dispose(): void { + this.disposed = true + this.cancelPendingLoad?.() // Cancel any in-flight load timeout so it can't reject after disposal if (this._loadTimeoutId !== null) { clearTimeout(this._loadTimeoutId) @@ -345,6 +374,7 @@ class SourceController { // --- Private methods --- private disposeElement(element: HTMLVideoElement): void { + this.failedAssignedElements.delete(element) element.pause() element.src = '' element.load() @@ -451,6 +481,10 @@ class SourceController { element.playsInline = true element.muted = true // Start muted, unmute when needed + element.addEventListener('canplay', () => { + this.failedAssignedElements.delete(element) + }) + element.addEventListener('loadedmetadata', () => { this.onElementReady?.(element) })