From 52e6471a0a93325b59368fb23eaf11891724c3df Mon Sep 17 00:00:00 2001 From: 4gray Date: Mon, 24 Aug 2026 10:31:11 +0200 Subject: [PATCH] fix(playback): run embedded MPV position poll outside the Angular zone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With provideZoneChangeDetection a zone-registered interval runs app-wide change detection every 500 ms for the entire stream. Register the drift poll via NgZone.runOutsideAngular and never re-enter the zone — the drift path is rAF → setEmbeddedMpvBounds IPC and touches no Angular state. Frame-copy sessions skip the measurement entirely: the canvas is laid out by the DOM and moves with the layout, so only the native-view child window can go stale on a position-only shift. Addresses the Codex P2 review finding on #1476. Co-Authored-By: Claude Fable 5 --- docs/architecture/embedded-mpv-native.md | 8 +- ...ed-mpv-session-controller.position.spec.ts | 77 +++++++++++++++++++ .../embedded-mpv-session-controller.ts | 33 ++++++-- 3 files changed, 109 insertions(+), 9 deletions(-) diff --git a/docs/architecture/embedded-mpv-native.md b/docs/architecture/embedded-mpv-native.md index 7b11179ef..b4dce7818 100644 --- a/docs/architecture/embedded-mpv-native.md +++ b/docs/architecture/embedded-mpv-native.md @@ -675,7 +675,13 @@ stale coordinates and rendered offset from the DOM stage. The session controller therefore polls the host bounds every 500 ms while a session is active, compares them against the last synced bounds with a half-pixel tolerance, and schedules a re-sync only on drift — idle cost is one -`getBoundingClientRect` per tick with no IPC. +`getBoundingClientRect` per tick with no IPC. The interval is registered +outside Angular's zone (a zone timer would run app-wide change detection +every tick for the whole stream) and never re-enters it, because the drift +path is rAF → `setEmbeddedMpvBounds` IPC and touches no Angular state. +Frame-copy skips the measurement entirely: its canvas is laid out by the +DOM and moves with the layout, so only the native-view child window can go +stale on a position-only shift. ### Controls ownership by engine diff --git a/libs/ui/playback/src/lib/embedded-mpv-player/embedded-mpv-session-controller.position.spec.ts b/libs/ui/playback/src/lib/embedded-mpv-player/embedded-mpv-session-controller.position.spec.ts index dbdf7e30b..cc875c1c1 100644 --- a/libs/ui/playback/src/lib/embedded-mpv-player/embedded-mpv-session-controller.position.spec.ts +++ b/libs/ui/playback/src/lib/embedded-mpv-player/embedded-mpv-session-controller.position.spec.ts @@ -1,3 +1,4 @@ +import { NgZone } from '@angular/core'; import { TestBed } from '@angular/core/testing'; import { EmbeddedMpvSession, @@ -24,6 +25,8 @@ describe('EmbeddedMpvSessionController position drift poll', () => { disposeEmbeddedMpvSession: jest.Mock; setEmbeddedMpvBounds: jest.Mock; onEmbeddedMpvSessionUpdate: jest.Mock; + attachEmbeddedMpvFrameView?: jest.Mock; + detachEmbeddedMpvFrameView?: jest.Mock; }; beforeEach(() => { @@ -126,8 +129,82 @@ describe('EmbeddedMpvSessionController position drift poll', () => { await jest.advanceTimersByTimeAsync(2000); expect(electron.setEmbeddedMpvBounds).not.toHaveBeenCalled(); }); + + it('registers the poll outside the Angular zone', async () => { + // With zone change detection, a zone-registered interval would run + // app-wide change detection every 500 ms for the whole stream. + const zone = TestBed.inject(NgZone); + const runOutsideAngular = jest.spyOn(zone, 'runOutsideAngular'); + + const controller = TestBed.inject(EmbeddedMpvSessionController); + const teardown = controller.startSession( + createHost(), + createPlayback(), + 0.5 + ); + await waitFor( + () => controller.sessionId() === 'mpv-1', + 'session to start' + ); + + expect(runOutsideAngular).toHaveBeenCalled(); + teardown(); + }); + + it('skips drift measurement for the frame-copy engine', async () => { + // Frame-copy paints into a DOM canvas that moves with the layout, so + // a position-only shift needs no re-sync (size changes still arrive + // through ResizeObserver). + electron.getEmbeddedMpvSupport.mockResolvedValue({ + supported: true, + platform: 'linux', + engine: 'frame-copy', + }); + electron.attachEmbeddedMpvFrameView = jest + .fn() + .mockResolvedValue(true); + electron.detachEmbeddedMpvFrameView = jest.fn(); + + const rect = { left: 10, top: 20, width: 640, height: 360 }; + const host = { + getBoundingClientRect: () => ({ ...rect }), + } as HTMLElement; + + const controller = TestBed.inject(EmbeddedMpvSessionController); + await waitFor( + () => controller.support()?.engine === 'frame-copy', + 'support to load' + ); + const teardown = controller.startSession(host, createPlayback(), 0.5); + await waitFor( + () => controller.sessionId() === 'mpv-1', + 'session to start' + ); + await waitFor( + () => electron.setEmbeddedMpvBounds.mock.calls.length > 0, + 'initial bounds sync' + ); + electron.setEmbeddedMpvBounds.mockClear(); + + rect.left = 29; + await jest.advanceTimersByTimeAsync(2000); + expect(electron.setEmbeddedMpvBounds).not.toHaveBeenCalled(); + + teardown(); + }); }); +function createHost(): HTMLElement { + return { + getBoundingClientRect: () => ({ + left: 10, + top: 20, + width: 640, + height: 360, + }), + } as HTMLElement; +} + function createPlayback(): ResolvedPortalPlayback { return { streamUrl: 'https://example.com/movie.mp4', diff --git a/libs/ui/playback/src/lib/embedded-mpv-player/embedded-mpv-session-controller.ts b/libs/ui/playback/src/lib/embedded-mpv-player/embedded-mpv-session-controller.ts index 6c4ab1f4f..2b4256bb6 100644 --- a/libs/ui/playback/src/lib/embedded-mpv-player/embedded-mpv-session-controller.ts +++ b/libs/ui/playback/src/lib/embedded-mpv-player/embedded-mpv-session-controller.ts @@ -1,6 +1,7 @@ import { DestroyRef, Injectable, + NgZone, computed, effect, inject, @@ -75,6 +76,7 @@ export class EmbeddedMpvSessionController { ); private readonly destroyRef = inject(DestroyRef); + private readonly zone = inject(NgZone); private readonly unsubscribeSessionUpdate?: () => void; private boundsProvider: EmbeddedMpvBoundsProvider = (host) => @@ -182,14 +184,29 @@ export class EmbeddedMpvSessionController { // the measured bounds against the last synced ones and re-syncs only // on drift, so the idle cost is one getBoundingClientRect per tick // with no IPC. - const positionPoll = window.setInterval(() => { - if (!activeSessionId || !lastSyncedBounds) { - return; - } - if (boundsDiffer(this.boundsProvider(host), lastSyncedBounds)) { - scheduleBoundsSync(); - } - }, POSITION_POLL_INTERVAL_MS); + // + // The interval runs outside Angular's zone: with zone change + // detection a zone-registered timer would run app-wide change + // detection every tick for the whole stream. It never re-enters the + // zone — the drift path is rAF → setEmbeddedMpvBounds IPC and + // touches no Angular state. Frame-copy paints into a DOM canvas + // that moves with the layout, so only native-view can go stale on a + // position-only shift; the poll skips the measurement there. + const positionPoll = this.zone.runOutsideAngular(() => + window.setInterval(() => { + if (!activeSessionId || !lastSyncedBounds) { + return; + } + if (untracked(() => this.isFrameCopyEngine())) { + return; + } + if ( + boundsDiffer(this.boundsProvider(host), lastSyncedBounds) + ) { + scheduleBoundsSync(); + } + }, POSITION_POLL_INTERVAL_MS) + ); // Page zoom and monitor DPI rescale the CSS→native-pixel mapping the // backend applies to these bounds. Moving the window to a display