From d24771db649d0a16e3c8e27736dd014db8ac0ec2 Mon Sep 17 00:00:00 2001 From: Dermot Duffy Date: Sun, 30 Aug 2026 11:39:56 -0700 Subject: [PATCH] fix: Stop reporting a stall when a live provider replaces its media element (#2728) - Related: #2718 --- src/components-lib/media-player/image.ts | 44 ++++-- src/components-lib/media-player/video.ts | 54 +++++-- .../components-lib/media-player/image.test.ts | 149 ++++++++++++++++++ .../components-lib/media-player/video.test.ts | 119 +++++++++++++- 4 files changed, 339 insertions(+), 27 deletions(-) diff --git a/src/components-lib/media-player/image.ts b/src/components-lib/media-player/image.ts index 9e400573..556da420 100644 --- a/src/components-lib/media-player/image.ts +++ b/src/components-lib/media-player/image.ts @@ -1,4 +1,4 @@ -import type { LitElement } from 'lit'; +import type { LitElement, ReactiveController } from 'lit'; import type { FullscreenElement, @@ -40,13 +40,19 @@ interface ImageMediaPlayerControllerOptions { } // Image player composed from opt-in capabilities. -export class ImageMediaPlayerController implements MediaPlayerController { +export class ImageMediaPlayerController + implements MediaPlayerController, ReactiveController +{ private _host: LitElement; private _getImageCallback: () => HTMLImageElement | null; private _screenshotProvider: ImageScreenshotProvider | null; private _stallWatchdog: FrameStallWatchdog | null = null; - private _loadListener: (() => void) | null = null; + + // The frame source: the image the load listener was added to, and the + // listener itself. + private _frameCallback: { image: HTMLImageElement; listener: () => void } | null = + null; public readonly playback?: PlaybackControl; public readonly subscribeLiveness?: ( @@ -89,6 +95,26 @@ export class ImageMediaPlayerController implements MediaPlayerController { this.subscribeLiveness = (callback): UnsubscribeCallback => stallWatchdog.subscribe(callback); } + + host.addController(this); + } + + // The host can replace its image element so the frame callback has to move + // with it. Left on the old element it would be watching something detached + // that never presents another frame. + // + // See: https://github.com/dermotduffy/advanced-camera-card/issues/2726 + // See: https://github.com/dermotduffy/advanced-camera-card/issues/2718 + public hostUpdated(): void { + if (!this._frameCallback) { + return; + } + const image = this._getImageCallback(); + if (!image || image === this._frameCallback.image) { + return; + } + this._stopFrameSource(); + this._startFrameSource(); } public async mute(): Promise { @@ -135,18 +161,16 @@ export class ImageMediaPlayerController implements MediaPlayerController { if (!image) { return false; } - this._loadListener = (): void => { + const listener = (): void => { this._stallWatchdog?.notifyFrame(); }; - image.addEventListener('load', this._loadListener); + this._frameCallback = { image, listener }; + image.addEventListener('load', listener); return true; } private _stopFrameSource(): void { - const image = this._getImageCallback(); - if (image && this._loadListener) { - image.removeEventListener('load', this._loadListener); - } - this._loadListener = null; + this._frameCallback?.image.removeEventListener('load', this._frameCallback.listener); + this._frameCallback = null; } } diff --git a/src/components-lib/media-player/video.ts b/src/components-lib/media-player/video.ts index 35bab595..5450876f 100644 --- a/src/components-lib/media-player/video.ts +++ b/src/components-lib/media-player/video.ts @@ -1,4 +1,4 @@ -import type { LitElement } from 'lit'; +import type { LitElement, ReactiveController } from 'lit'; import type { FullscreenElement, @@ -12,12 +12,16 @@ import { hideMediaControlsTemporarily, setControlsOnVideo } from '../../utils/co import { screenshotVideo } from '../../utils/screenshot'; import { FrameStallWatchdog } from './frame-stall-watchdog'; -export class VideoMediaPlayerController implements MediaPlayerController { +export class VideoMediaPlayerController + implements MediaPlayerController, ReactiveController +{ private _host: LitElement; private _getVideoCallback: () => HTMLVideoElement | null; private _getControlsDefaultCallback: (() => boolean) | null; - private _rvfcHandle: number | null = null; + // The frame callback registration: the video it was made on and the handle to + // cancel it with. + private _frameCallback: { video: HTMLVideoElement; handle: number } | null = null; private _stallWatchdog = new FrameStallWatchdog({ // Playback is expected unless the video is legitimately idle. Seeking / // ended is idle. A poster shown with no media loaded is a still-image @@ -50,6 +54,26 @@ export class VideoMediaPlayerController implements MediaPlayerController { this._host = host; this._getVideoCallback = getVideoCallback; this._getControlsDefaultCallback = getControlsDefaultCallback ?? null; + + host.addController(this); + } + + // A player can replace its video element (e.g. go2rtc rebuilding its player), + // so the frame callback has to move with it. Left on the old element it would + // be watching something detached that never presents another frame. + // + // See: https://github.com/dermotduffy/advanced-camera-card/issues/2726 + // See: https://github.com/dermotduffy/advanced-camera-card/issues/2718 + public hostUpdated(): void { + if (!this._frameCallback) { + return; + } + const video = this._getVideoCallback(); + if (!video || video === this._frameCallback.video) { + return; + } + this._stopFrameSource(); + this._startFrameSource(); } public readonly playback: PlaybackControl = { @@ -161,29 +185,29 @@ export class VideoMediaPlayerController implements MediaPlayerController { if (!video || !('requestVideoFrameCallback' in video)) { return false; } - this._rvfcHandle = video.requestVideoFrameCallback(this._onVideoFrame); + this._frameCallback = { + video, + handle: video.requestVideoFrameCallback(this._onVideoFrame), + }; return true; } private _stopFrameSource(): void { - const video = this._getVideoCallback(); - if (video && this._rvfcHandle !== null) { - video.cancelVideoFrameCallback(this._rvfcHandle); - } - this._rvfcHandle = null; + this._frameCallback?.video.cancelVideoFrameCallback(this._frameCallback.handle); + this._frameCallback = null; } private _onVideoFrame = (): void => { this._stallWatchdog.notifyFrame(); // Re-arm only if still watching: `notifyFrame`'s live notification may have - // dropped the last subscriber, which stops the source and nulls the handle. - if (this._rvfcHandle === null) { + // dropped the last subscriber, which stops the source and clears the + // registration. + const callback = this._frameCallback; + if (!callback) { return; } - const video = this._getVideoCallback(); - if (video) { - this._rvfcHandle = video.requestVideoFrameCallback(this._onVideoFrame); - } + + callback.handle = callback.video.requestVideoFrameCallback(this._onVideoFrame); }; } diff --git a/tests/components-lib/media-player/image.test.ts b/tests/components-lib/media-player/image.test.ts index 5eaf1b68..35bd557f 100644 --- a/tests/components-lib/media-player/image.test.ts +++ b/tests/components-lib/media-player/image.test.ts @@ -300,6 +300,155 @@ describe('ImageMediaPlayerController', () => { expect(callback).toHaveBeenCalledTimes(1); }); + it('should not report a stall while a replacement image keeps loading', () => { + const original = document.createElement('img'); + const replacement = document.createElement('img'); + let current = original; + const controller = createImageMediaPlayerWithLiveness( + () => true, + () => current, + ); + const subscribe = controller.subscribeLiveness; + assert(subscribe); + const callback = vi.fn(); + + subscribe(callback); + original.dispatchEvent(new Event('load')); + + current = replacement; + controller.hostUpdated(); + + // Twice the stall window, with the replacement loading throughout. + for (let i = 0; i < 2 * STALL_SECONDS; i++) { + replacement.dispatchEvent(new Event('load')); + vi.advanceTimersByTime(1000); + } + + expect(callback).toHaveBeenCalledExactlyOnceWith(true); + }); + + it('should not give a replacement longer than the stall window', () => { + const original = document.createElement('img'); + const replacement = document.createElement('img'); + let current = original; + const controller = createImageMediaPlayerWithLiveness( + () => true, + () => current, + ); + const subscribe = controller.subscribeLiveness; + assert(subscribe); + const callback = vi.fn(); + + subscribe(callback); + original.dispatchEvent(new Event('load')); + + current = replacement; + controller.hostUpdated(); + + vi.advanceTimersByTime(STALL_MS); + + expect(callback).toHaveBeenNthCalledWith(1, true); + expect(callback).toHaveBeenNthCalledWith(2, false); + }); + + it('should stop watching an image the host has replaced', () => { + const original = document.createElement('img'); + const replacement = document.createElement('img'); + let current = original; + const controller = createImageMediaPlayerWithLiveness( + () => true, + () => current, + ); + const subscribe = controller.subscribeLiveness; + assert(subscribe); + const callback = vi.fn(); + + subscribe(callback); + current = replacement; + controller.hostUpdated(); + + // The abandoned image is no longer listened to, so its loads say nothing. + original.dispatchEvent(new Event('load')); + + expect(callback).not.toHaveBeenCalled(); + }); + + it('should keep watching the same image when a render changes nothing', () => { + const image = document.createElement('img'); + const addEventListener = vi.spyOn(image, 'addEventListener'); + const controller = createImageMediaPlayerWithLiveness( + () => true, + () => image, + ); + const subscribe = controller.subscribeLiveness; + assert(subscribe); + + subscribe(vi.fn()); + controller.hostUpdated(); + controller.hostUpdated(); + + expect(addEventListener).toHaveBeenCalledOnce(); + }); + + it('should keep watching the old image when a render offers no replacement', () => { + const image = document.createElement('img'); + const removeEventListener = vi.spyOn(image, 'removeEventListener'); + let current: HTMLImageElement | null = image; + const controller = createImageMediaPlayerWithLiveness( + () => true, + () => current, + ); + const subscribe = controller.subscribeLiveness; + assert(subscribe); + + subscribe(vi.fn()); + current = null; + controller.hostUpdated(); + + expect(removeEventListener).not.toHaveBeenCalled(); + }); + + it('should ignore a render before anything is being watched', () => { + const image = document.createElement('img'); + const addEventListener = vi.spyOn(image, 'addEventListener'); + const controller = createImageMediaPlayerWithLiveness( + () => true, + () => image, + ); + + controller.hostUpdated(); + + expect(addEventListener).not.toHaveBeenCalled(); + }); + + it('should stop watching the replaced image, not its replacement', () => { + const original = document.createElement('img'); + const replacement = document.createElement('img'); + const removeOriginal = vi.spyOn(original, 'removeEventListener'); + const removeReplacement = vi.spyOn(replacement, 'removeEventListener'); + let current = original; + const controller = createImageMediaPlayerWithLiveness( + () => true, + () => current, + ); + const subscribe = controller.subscribeLiveness; + assert(subscribe); + + const unsubscribe = subscribe(vi.fn()); + current = replacement; + unsubscribe(); + + expect(removeOriginal).toHaveBeenCalled(); + expect(removeReplacement).not.toHaveBeenCalled(); + }); + + it('should register itself with its host', () => { + const host = createLitElement(); + new ImageMediaPlayerController(host, () => null); + + expect(host.addController).toHaveBeenCalled(); + }); + it('should honor a custom stall window', () => { const shortSeconds = 3; const image = document.createElement('img'); diff --git a/tests/components-lib/media-player/video.test.ts b/tests/components-lib/media-player/video.test.ts index 6c253e60..8fa7b415 100644 --- a/tests/components-lib/media-player/video.test.ts +++ b/tests/components-lib/media-player/video.test.ts @@ -444,6 +444,121 @@ describe('VideoMediaPlayerController', () => { expect(callback).toHaveBeenCalledWith(false); }); + it('should not report a stall while a replacement video keeps delivering frames', () => { + const original = createVideo(); + const replacement = createVideo(); + let current = original; + + const controller = new VideoMediaPlayerController( + createLitElement(), + () => current.video, + ); + const callback = vi.fn(); + + controller.subscribeLiveness(callback); + original.deliverFrame(); + + current = replacement; + controller.hostUpdated(); + + // Twice the stall window, with the replacement delivering frames + // throughout. + for (let i = 0; i < 2 * FRAME_STALL_SECONDS; i++) { + replacement.deliverFrame(); + vi.advanceTimersByTime(1000); + } + + expect(callback).toHaveBeenCalledExactlyOnceWith(true); + }); + + it('should not give a replacement longer than the stall window', () => { + const original = createVideo(); + const replacement = createVideo(); + let current = original; + + const controller = new VideoMediaPlayerController( + createLitElement(), + () => current.video, + ); + const callback = vi.fn(); + + controller.subscribeLiveness(callback); + original.deliverFrame(); + + current = replacement; + controller.hostUpdated(); + + vi.advanceTimersByTime(STALL_MS); + + expect(callback).toHaveBeenNthCalledWith(1, true); + expect(callback).toHaveBeenNthCalledWith(2, false); + }); + + it('should keep watching the same video when a render changes nothing', () => { + const { video, deliverFrame, cancel } = createVideo(); + const controller = new VideoMediaPlayerController(createLitElement(), () => video); + const callback = vi.fn(); + + controller.subscribeLiveness(callback); + deliverFrame(); + controller.hostUpdated(); + + expect(cancel).not.toHaveBeenCalled(); + expect(callback).toHaveBeenCalledExactlyOnceWith(true); + }); + + it('should keep watching the old video when a render offers no replacement', () => { + const { video, cancel } = createVideo(); + let current: HTMLVideoElement | null = video; + const controller = new VideoMediaPlayerController( + createLitElement(), + () => current, + ); + + controller.subscribeLiveness(vi.fn()); + current = null; + controller.hostUpdated(); + + expect(cancel).not.toHaveBeenCalled(); + }); + + it('should ignore a render before anything is being watched', () => { + const { video, cancel } = createVideo(); + const controller = new VideoMediaPlayerController(createLitElement(), () => video); + + controller.hostUpdated(); + + expect(vi.mocked(video.requestVideoFrameCallback)).not.toHaveBeenCalled(); + expect(cancel).not.toHaveBeenCalled(); + }); + + it('should stop watching the replaced video, not its replacement', () => { + // A handle is only valid on the video it came from, so it is cancelled + // there rather than on whatever the player is showing now. + const original = createVideo(); + const replacement = createVideo(); + let current = original; + + const controller = new VideoMediaPlayerController( + createLitElement(), + () => current.video, + ); + + const unsubscribe = controller.subscribeLiveness(vi.fn()); + current = replacement; + unsubscribe(); + + expect(original.cancel).toHaveBeenCalled(); + expect(replacement.cancel).not.toHaveBeenCalled(); + }); + + it('should register itself with its host', () => { + const host = createLitElement(); + new VideoMediaPlayerController(host, () => null); + + expect(host.addController).toHaveBeenCalled(); + }); + it('should report no stall when requestVideoFrameCallback is unavailable', () => { const { video } = createVideo({ rvfc: false }); expect('requestVideoFrameCallback' in video).toBe(false); @@ -514,7 +629,7 @@ describe('VideoMediaPlayerController', () => { expect(callback).not.toHaveBeenCalledWith(false); }); - it('should cancel nothing on unsubscribe when the video is already gone', () => { + it('should stop watching the old video once the player has dropped it', () => { const { video, cancel } = createVideo(); let currentVideo: HTMLVideoElement | null = video; const controller = new VideoMediaPlayerController( @@ -526,7 +641,7 @@ describe('VideoMediaPlayerController', () => { currentVideo = null; unsubscribe(); - expect(cancel).not.toHaveBeenCalled(); + expect(cancel).toHaveBeenCalled(); }); }); });