From f89c17fd81f7f26531bfbd867b7fb94638816f71 Mon Sep 17 00:00:00 2001 From: Lirian Su Date: Mon, 25 May 2026 12:56:01 +0800 Subject: [PATCH] fix(engine): harden ancestor-hidden video skip against mask + caller cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups to the ancestor-visibility skip in `injectVideoFramesBatch` and `syncVideoFrameVisibility`. 1. **Mask defence.** Both ancestor-hidden branches previously wrote a plain `img.style.visibility = "hidden"`. `applyDomLayerMask` writes the stylesheet rule `#${showId} *{visibility:visible !important}`, and CSS cascade puts important stylesheet author above non-important inline author — so a sub-comp host landing in the active layer's `show` set would revive a stale `__render_frame__` and let it bleed onto the layer composite. Write the hide via `style.setProperty("visibility", "hidden", "important")` instead; important inline beats important stylesheet. 2. **Caller cache hygiene.** `createVideoFrameInjector` unconditionally wrote `lastInjectedFrameByVideo.set(id, frameIndex)` after calling `injectVideoFramesBatch`, even for videos the page silently skipped due to a hidden visual ancestor. On the next call at the same frameIndex — common with source-fps < output-fps, paused source frames, or non-frame-aligned host starts — the cache short-circuited the second inject and the host's first visible frame painted blank because the replacement `` was never created. Make `injectVideoFramesBatch` return `string[]` (the subset of ids it actually painted) and have the caller cache only those. The cli-side `snapshot.ts` consumer is unaffected: its local `InjectFn` types the return as `Promise`, which is structurally compatible with `Promise` under TS void-return assignment rules. Tests: linkedom doesn't preserve `!important` in cssText, so the two new mask-defence cases spy on the live ``'s `style.setProperty` and assert the 3-arg call shape. The cache-hygiene case stubs the page-side primitives via `vi.mock`, drives the hook twice at the same frameIndex with a stubbed "injected nothing" first response, and verifies the second call still issues an inject. A counter-test pins the happy-path cache hit so a future refactor can't trade the skip bug for a never-cache regression. --- .../src/services/screenshotService.test.ts | 54 +++++++++ .../engine/src/services/screenshotService.ts | 32 ++++- .../src/services/videoFrameInjector.test.ts | 113 +++++++++++++++++- .../engine/src/services/videoFrameInjector.ts | 18 ++- 4 files changed, 206 insertions(+), 11 deletions(-) diff --git a/packages/engine/src/services/screenshotService.test.ts b/packages/engine/src/services/screenshotService.test.ts index e6f96d4c6..4332c359d 100644 --- a/packages/engine/src/services/screenshotService.test.ts +++ b/packages/engine/src/services/screenshotService.test.ts @@ -453,4 +453,58 @@ describe("video-frame injection respects ancestor visibility", () => { // to `visibility: visible` so it overrides the ancestor. expect(seededImg.style.visibility).toBe("visible"); }); + + // Regression for the layered/HDR mask path: `applyDomLayerMask` writes an + // `!important` stylesheet rule `#${showId} *{visibility:visible !important}` + // which, if a sub-comp host id appears in the show set, would revive a + // plain (non-important) inline `visibility: hidden` on a descendant + // `__render_frame__` — the cascade rule is "important stylesheet author + // beats non-important inline author". To stay safe regardless of which + // layer ends up in `show`, the ancestor-hidden hide must be written with + // `!important` so inline `!important` beats stylesheet `!important`. + // + // linkedom strips `!important` from `cssText`/`getPropertyPriority`, so we + // pin the contract on the API call site instead: a `setProperty(name, + // value, "important")` invocation on the live ``'s style. + it("injectVideoFramesBatch hides a stale with !important so the layer mask cannot revive it", async () => { + const { teardown, setup } = withGlobals(setupHostHiddenScenario({ visibility: "hidden" })); + const seededImg = setup.document.createElement("img"); + seededImg.classList.add("__render_frame__"); + seededImg.style.visibility = "visible"; + setup.video.parentNode?.insertBefore(seededImg, setup.video.nextSibling); + const setPropertySpy = vi.spyOn(seededImg.style, "setProperty"); + + try { + await injectVideoFramesBatch(passthroughPage(), [ + { + videoId: "pip", + dataUri: + "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNkAAIAAAoAAv/lxKUAAAAASUVORK5CYII=", + }, + ]); + } finally { + teardown(); + } + + expect(seededImg.style.visibility).toBe("hidden"); + expect(setPropertySpy).toHaveBeenCalledWith("visibility", "hidden", "important"); + }); + + it("syncVideoFrameVisibility hides an existing with !important so the layer mask cannot revive it", async () => { + const { teardown, setup } = withGlobals(setupHostHiddenScenario({ visibility: "hidden" })); + const seededImg = setup.document.createElement("img"); + seededImg.classList.add("__render_frame__"); + seededImg.style.visibility = "visible"; + setup.video.parentNode?.insertBefore(seededImg, setup.video.nextSibling); + const setPropertySpy = vi.spyOn(seededImg.style, "setProperty"); + + try { + await syncVideoFrameVisibility(passthroughPage(), ["pip"]); + } finally { + teardown(); + } + + expect(seededImg.style.visibility).toBe("hidden"); + expect(setPropertySpy).toHaveBeenCalledWith("visibility", "hidden", "important"); + }); }); diff --git a/packages/engine/src/services/screenshotService.ts b/packages/engine/src/services/screenshotService.ts index a73423f87..7af0481ad 100644 --- a/packages/engine/src/services/screenshotService.ts +++ b/packages/engine/src/services/screenshotService.ts @@ -369,13 +369,22 @@ export async function removeDomLayerMask(page: Page, extraHideIds: string[]): Pr ); } +/** + * Returns the subset of `updates.videoId`s that were actually painted in + * this call. Videos skipped because of a hidden visual ancestor are NOT + * included — the caller relies on this to avoid recording a `lastInjected` + * cache entry for a frame that never reached the page, which would otherwise + * short-circuit the next inject at the same frameIndex and leave the host's + * first visible frame blank. + */ export async function injectVideoFramesBatch( page: Page, updates: Array<{ videoId: string; dataUri: string }>, -): Promise { - if (updates.length === 0) return; - await page.evaluate( +): Promise { + if (updates.length === 0) return []; + return await page.evaluate( async (items: Array<{ videoId: string; dataUri: string }>, visualProperties: string[]) => { + const injectedIds: string[] = []; const pendingDecodes: Array> = []; const replacementLayoutProperties = new Set([ "width", @@ -435,7 +444,13 @@ export async function injectVideoFramesBatch( // Don't paint a frame over a hidden host — if an existing replacement // is still around from when the host was visible, hide it so it // doesn't bleed through a sibling host that *is* visible on this seek. - if (hasImg && img) img.style.visibility = "hidden"; + // + // Use `!important` so the inline hide survives `applyDomLayerMask`'s + // stylesheet `#${showId} *{visibility:visible !important}` when the + // sub-comp host happens to land in the active layer's `show` set — + // important stylesheet beats non-important inline, but important + // inline beats important stylesheet. + if (hasImg && img) img.style.setProperty("visibility", "hidden", "important"); continue; } @@ -522,10 +537,12 @@ export async function injectVideoFramesBatch( // GSAP-controlled value. video.style.setProperty("visibility", "hidden", "important"); video.style.setProperty("pointer-events", "none", "important"); + injectedIds.push(item.videoId); } if (pendingDecodes.length > 0) { await Promise.all(pendingDecodes); } + return injectedIds; }, updates, [...MEDIA_VISUAL_STYLE_PROPERTIES], @@ -577,11 +594,16 @@ export async function syncVideoFrameVisibility( } else { // Inactive (or ancestor-hidden) video: hide both. Use visibility only // (never opacity) so we never clobber GSAP-controlled inline opacity. + // Use `!important` on the hide so `applyDomLayerMask`'s + // important stylesheet rule (`#${showId} *{visibility:visible !important}`) + // cannot revive a stale frame when the sub-comp host lands in the + // active layer's `show` set — same mask-defense reasoning as the + // `isVisualAncestorHidden` branch in `injectVideoFramesBatch`. video.style.removeProperty("display"); video.style.setProperty("visibility", "hidden", "important"); video.style.setProperty("pointer-events", "none", "important"); if (hasImg) { - img.style.visibility = "hidden"; + img.style.setProperty("visibility", "hidden", "important"); } } } diff --git a/packages/engine/src/services/videoFrameInjector.test.ts b/packages/engine/src/services/videoFrameInjector.test.ts index 28c813641..5a1c793b1 100644 --- a/packages/engine/src/services/videoFrameInjector.test.ts +++ b/packages/engine/src/services/videoFrameInjector.test.ts @@ -1,9 +1,30 @@ // @vitest-environment node -import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { __testing } from "./videoFrameInjector.js"; +import { type Page } from "puppeteer-core"; + +// Hoist mocks before importing the module under test so the mock factory wins. +// The cache-hygiene block exercises createVideoFrameInjector against stubbed +// page-side primitives so we can assert on Node-side state (cache poisoning) +// without standing up a real browser. +const { injectVideoFramesBatchMock, syncVideoFrameVisibilityMock } = vi.hoisted(() => ({ + injectVideoFramesBatchMock: vi.fn< + (page: Page, updates: Array<{ videoId: string; dataUri: string }>) => Promise + >(async (_page, updates) => updates.map((u) => u.videoId)), + syncVideoFrameVisibilityMock: vi.fn<(page: Page, ids: string[]) => Promise>( + async () => undefined, + ), +})); + +vi.mock("./screenshotService.js", () => ({ + injectVideoFramesBatch: injectVideoFramesBatchMock, + syncVideoFrameVisibility: syncVideoFrameVisibilityMock, +})); + +import { __testing, createVideoFrameInjector } from "./videoFrameInjector.js"; +import { type FrameLookupTable } from "./videoFrameExtractor.js"; import { DEFAULT_CONFIG } from "../config.js"; const { createFrameSourceCache } = __testing; @@ -143,3 +164,91 @@ describe("frame source cache eviction", () => { expect(cache.stats()).toMatchObject({ ...SHARED_STATS, entries: 0, bytes: 0 }); }); }); + +describe("createVideoFrameInjector cache hygiene against page-side skips", () => { + // Build a minimal FrameLookupTable stand-in that returns one fixed payload + // for every time so we can drive the hook deterministically. The real + // table is exercised exhaustively in videoFrameExtractor.test.ts. + function fakeTable(payload: { videoId: string; framePath: string; frameIndex: number }) { + return { + getActiveFramePayloads: () => + new Map([ + [payload.videoId, { framePath: payload.framePath, frameIndex: payload.frameIndex }], + ]), + } as unknown as FrameLookupTable; + } + + // Bypass the on-disk frame cache by handing back a synthetic data URI. + function inlineResolver(framePath: string): string { + return `data:image/png;base64,fake-${framePath}`; + } + + beforeEach(() => { + injectVideoFramesBatchMock.mockReset(); + syncVideoFrameVisibilityMock.mockReset(); + syncVideoFrameVisibilityMock.mockResolvedValue(undefined); + }); + + it("does not poison the lastInjected cache when the page reports zero ids injected", async () => { + // Regression for the agentic-finecut scenario after PR #1028's ancestor + // skip: when injectVideoFramesBatch silently drops a video (its sub-comp + // host is hidden), the caller used to record `lastInjectedFrame[v] = N` + // anyway. On the next frame, if the source frameIndex is unchanged + // (low-fps source, multiple output frames per source frame, or + // non-frame-aligned host start), the cache short-circuits the second + // call and the host's first visible frame paints blank because the + // replacement was never created. + // + // Pin the contract: when the page returns `[]` (no ids actually + // injected), the cache must not record those frameIndexes, so a follow- + // up call at the same frameIndex still issues an inject. + const fakePage = {} as Page; + const hook = createVideoFrameInjector( + fakeTable({ videoId: "pip", framePath: "/p", frameIndex: 5 }), + { + frameSrcResolver: inlineResolver, + }, + ); + expect(hook).not.toBeNull(); + + // First call: simulate the ancestor-hidden skip — page-side reports it + // injected nothing. + injectVideoFramesBatchMock.mockResolvedValueOnce([]); + await hook!(fakePage, 0); + expect(injectVideoFramesBatchMock).toHaveBeenCalledTimes(1); + expect(injectVideoFramesBatchMock).toHaveBeenLastCalledWith(fakePage, [ + { videoId: "pip", dataUri: "data:image/png;base64,fake-/p" }, + ]); + + // Second call: same frameIndex, but the previous call did not really + // paint. The cache must NOT short-circuit; the inject must run again. + injectVideoFramesBatchMock.mockResolvedValueOnce(["pip"]); + await hook!(fakePage, 0); + expect(injectVideoFramesBatchMock).toHaveBeenCalledTimes(2); + expect(injectVideoFramesBatchMock).toHaveBeenLastCalledWith(fakePage, [ + { videoId: "pip", dataUri: "data:image/png;base64,fake-/p" }, + ]); + }); + + it("does cache normally when the page reports the id as injected", async () => { + // Counter-test: when injection succeeds for a videoId, the cache must + // record it and a second call at the same frameIndex must short-circuit. + // This pins the happy path so a future refactor can't trade the skip + // bug for a never-cache regression. + const fakePage = {} as Page; + const hook = createVideoFrameInjector( + fakeTable({ videoId: "pip", framePath: "/p", frameIndex: 5 }), + { + frameSrcResolver: inlineResolver, + }, + ); + + injectVideoFramesBatchMock.mockResolvedValueOnce(["pip"]); + await hook!(fakePage, 0); + expect(injectVideoFramesBatchMock).toHaveBeenCalledTimes(1); + + await hook!(fakePage, 0); + // Cache hit — no second inject for the same frameIndex. + expect(injectVideoFramesBatchMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/engine/src/services/videoFrameInjector.ts b/packages/engine/src/services/videoFrameInjector.ts index ddc79722a..d8b9e0e56 100644 --- a/packages/engine/src/services/videoFrameInjector.ts +++ b/packages/engine/src/services/videoFrameInjector.ts @@ -197,12 +197,22 @@ export function createVideoFrameInjector( await syncVideoFrameVisibility(page, Array.from(activeIds)); if (updates.length > 0) { - await injectVideoFramesBatch( - page, - updates.map((u) => ({ videoId: u.videoId, dataUri: u.dataUri })), + // Only record cache entries for videos the page actually painted. + // injectVideoFramesBatch skips any video whose visual ancestor is + // hidden (sub-comp host out-of-window) and returns the subset of ids + // it really wrote — recording the rest would short-circuit the next + // call at the same frameIndex and leave the host's first visible + // frame blank. + const injectedIds = new Set( + await injectVideoFramesBatch( + page, + updates.map((u) => ({ videoId: u.videoId, dataUri: u.dataUri })), + ), ); for (const update of updates) { - lastInjectedFrameByVideo.set(update.videoId, update.frameIndex); + if (injectedIds.has(update.videoId)) { + lastInjectedFrameByVideo.set(update.videoId, update.frameIndex); + } } } };