fix: Stop reporting a stall when a live provider replaces its media element (#2728)

- Related: #2718

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<void> {
@@ -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();
+    this._frameCallback?.image.removeEventListener('load', this._frameCallback.listener);
-    if (image && this._loadListener) {
+    this._frameCallback = null;
-      image.removeEventListener('load', this._loadListener);
-    }
-    this._loadListener = null;
   }
 }

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();
+    this._frameCallback?.video.cancelVideoFrameCallback(this._frameCallback.handle);
-    if (video && this._rvfcHandle !== null) {
+    this._frameCallback = null;
-      video.cancelVideoFrameCallback(this._rvfcHandle);
-    }
-    this._rvfcHandle = 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.
+    // dropped the last subscriber, which stops the source and clears the
-    if (this._rvfcHandle === null) {
+    // registration.
+    const callback = this._frameCallback;
+    if (!callback) {
       return;
     }
-    const video = this._getVideoCallback();
+
-    if (video) {
+    callback.handle = callback.video.requestVideoFrameCallback(this._onVideoFrame);
-      this._rvfcHandle = video.requestVideoFrameCallback(this._onVideoFrame);
-    }
   };
 }

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');

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();
     });
   });
 });