fix(engine): stop compositing phantom duplicates on captureBeyondViewport (#2607)

* fix(core): stop the async media-metadata rebind once render capture starts seeking scheduleMetadataDurationHydration re-resolves and can swap the captured GSAP timeline off a debounced loadedmetadata/durationchange event, fully uncoordinated with the producer's own per-frame renderSeek calls. When a full-length <video>'s metadata resolves after capture has already begun (slow I/O, Docker), this races the deterministic BeginFrame capture loop and can reflow sub-composition state mid-render, producing phase-offset duplicate content in captured frames (#2550). Render-mode duration correction already happens deterministically during the probe stage before capture starts, so once renderSeek has been called once there is nothing left for this self-correction to do — gate it off for the rest of the session. * fix(core): scope the metadata-rebind guard to actual render/export pages renderSeek isn't capture-exclusive — Studio's own preview iframe falls back to it for compositions whose timeline overhangs every native adapter's duration. Gating the HF#2550 fix on renderCaptureSeekStarted alone silently disabled the metadata-driven duration self-correction for that live-scrub case too, where it's still needed. Require the render/ export page signal (window.__HF_EXPORT_RENDER_SEEK_CONFIG, set only by the producer's fileServer.ts) alongside it, and add a regression test covering the Studio-preview case. * fix(engine): stop requesting beyond-viewport capture for video comps that don't need it Root-caused HF#2550 by reproducing the reporter's public repro end-to-end (not just the timeline-rebind mechanism from the earlier commits in this branch) on native Linux: instrumented the actual DOM state during a real capture session and confirmed the sub-composition never double-mounts — getBoundingClientRect and the timeline's own local time both match the single, correct DOM tree throughout. The phantom second copy only exists in the captured screenshot pixels. Bisected it to captureBeyondViewport: resolveVideoCaptureBeyondViewport (#1094's tall-portrait fix) forces `Page.captureScreenshot`'s beyond-viewport path on for any render with a native <video>, regardless of whether the page's content actually overflows the declared capture height. On SwiftShader that beyond-viewport path can composite a stale, vertically offset paint of the page alongside the fresh one for content that fits entirely within the viewport — producing exactly the reported phase-offset duplicate. Disabling captureBeyondViewport (repro's video still present) eliminates the duplicate outright; re-enabling it reproduces the duplicate byte-for-byte, isolating it as the actual cause. Adds pageContentExceedsCaptureHeight, a ground-truth measurement of the page's actual scrollHeight against the requested capture height, and wires it into initializeSession to downgrade captureBeyondViewport back to false once the page is settled and it's confirmed unnecessary — the "reliable clip predictor" the original #1094 fix's ponytail comment flagged as missing. This keeps #1094's fix intact for content that genuinely overflows while closing the SwiftShader ghosting hazard for the (common) case of video that fits inside its own viewport. * test(producer): add HF#2550 video+sub-composition regression fixture Checks in the reporter's confirmed real-world reproduction (media regenerated via ffmpeg testsrc2, matching their public repro repo) as a regression fixture, with a golden baseline rendered against the fix. Verified end-to-end via the project's own Docker regression harness: - Rendering this fixture with the fix produces the golden baseline (clean, single flowchart instance, captureBeyondViewport correctly downgraded). - Direct CLI renders (not through this harness) against unpatched code reproduce the reported phantom-duplicate artifact reliably (10/10). Caveat documented in meta.json: the underlying bug is timing-dependent. Two harness runs against unpatched code, using this same fixture, did not reproduce the artifact (0/2) — the harness's in-process render path apparently doesn't hit the same race window a direct CLI process does on this host. This fixture is a best-effort regression guard and a preserved real-world repro, not the sole protection — the deterministic guard is packages/engine/src/services/screenshotService.test.ts's pageContentExceedsCaptureHeight unit tests, which exercise the actual fix logic directly. Also adds an .gitattributes LFS rule for this fixture's source index.html (744 KB — carries the real project's embedded base64 assets, over the largefiles hook's 500 KB non-LFS limit). * fix: route HF#2550 fixture binaries through LFS (were committed raw) filter.lfs.clean/smudge were locally configured as a no-op "cat" in this repo's shared .git/config, silently disabling LFS filtering for every worktree. The previous commit's large binaries (output.mp4, compiled.html, source index.html, source video) landed as raw blobs instead of LFS pointers as a result. Ran `git lfs install --local --force` to restore the correct filter commands, then re-staged the affected files so they commit as proper LFS pointers. * fix(engine): address capture viewport review feedback
2026-09-24 00:08:03 +00:00 · 2026-07-17 11:59:45 -04:00
parent ae1c3f6c46
commit 52e518d356
15 changed files with 1100 additions and 6 deletions
@@ -17,6 +17,12 @@ packages/producer/tests/**/*.webm filter=lfs diff=lfs merge=lfs -text
 packages/producer/tests/**/*.png filter=lfs diff=lfs merge=lfs -text
 packages/producer/tests/**/output/compiled.html filter=lfs diff=lfs merge=lfs -text

+# HF#2550 fixture's source index.html carries the real-world project's
+# embedded base64 assets (744 KB) — over the largefiles hook's 500 KB
+# non-LFS limit. Scoped to this one file, not a broad tests/**/*.html
+# pattern, since every other fixture's source HTML is small plain git.
+packages/producer/tests/hf2550-video-subcomposition-ghost/src/index.html filter=lfs diff=lfs merge=lfs -text
+
 # ONNX models must ALWAYS use LFS regardless of location: a 31 MB ppmattingv2
 # model was once committed raw into skills/ then deleted — but a raw commit
 # lives in history forever. No path in the repo legitimately ships a raw .onnx.
@@ -37,13 +37,18 @@ function createMockTimeline(duration: number): RuntimeTimelineLike {

 function createPaddableMockTimeline(duration: number): RuntimeTimelineLike {
  const timeline = createMockTimeline(duration) as RuntimeTimelineLike & {
-    to: (_target: object, vars: { duration: number }, position: number) => void;
+    to: (_target: object, vars: { duration: number }, position?: number) => void;
  };
  const baseDuration = timeline.duration;
  let paddedDuration = baseDuration();
  timeline.duration = () => paddedDuration;
+  // Mirrors GSAP: an omitted position appends sequentially at the current end.
  timeline.to = (_target, vars, position) => {
-    paddedDuration = Math.max(paddedDuration, position + Math.max(0, Number(vars.duration) || 0));
+    const resolvedPosition = position ?? paddedDuration;
+    paddedDuration = Math.max(
+      paddedDuration,
+      resolvedPosition + Math.max(0, Number(vars.duration) || 0),
+    );
  };
  return timeline;
 }
@@ -1391,6 +1396,118 @@ describe("initSandboxRuntimeModular", () => {
    },
  );

+  it("ignores the async media-metadata duration rebind once render capture has started seeking frames (regression HF#2550)", async () => {
+    const root = document.createElement("div");
+    root.setAttribute("data-composition-id", "main");
+    root.setAttribute("data-root", "true");
+    root.setAttribute("data-start", "0");
+    root.setAttribute("data-width", "1920");
+    root.setAttribute("data-height", "1080");
+    document.body.appendChild(root);
+
+    const video = document.createElement("video");
+    video.setAttribute("data-start", "0");
+    document.body.appendChild(video);
+
+    // A root timeline with no usable duration yet — mirrors a composition
+    // whose length is derived from a full-length <video> that hasn't reported
+    // its metadata. window.gsap is needed because resolveRootTimelineFromDocument
+    // builds a fresh duration-floor wrapper timeline via gsap.timeline().
+    (
+      window as unknown as {
+        gsap?: { timeline: () => ReturnType<typeof createPaddableMockTimeline> };
+      }
+    ).gsap = {
+      timeline: () => createPaddableMockTimeline(0),
+    };
+    window.__timelines = { main: createMockTimeline(0) };
+    // Only a real producer render/export page sets this (fileServer.ts's
+    // pre-head script) — required alongside renderCaptureSeekStarted so the
+    // gate doesn't also disable Studio's own preview-iframe rebind.
+    window.__HF_EXPORT_RENDER_SEEK_CONFIG = { fps: 30, fpsSource: "default" };
+
+    const postMessageSpy = vi.spyOn(window, "postMessage");
+
+    try {
+      initSandboxRuntimeModular();
+
+      // The render/producer capture protocol has claimed the timeline and is
+      // now driving frames deterministically (mirrors the engine's
+      // window.__hf.seek(t) -> player.renderSeek(t) bridge).
+      window.__player?.renderSeek(0);
+
+      // Let the runtime's own deferred re-bind attempt (init.ts's
+      // `setTimeout(() => maybePublishRenderReady(), 0)`, unrelated to media
+      // metadata) settle first, so only the metadata path below is under test.
+      await new Promise((resolve) => setTimeout(resolve, 10));
+
+      // Video metadata resolves late — after capture has started, the exact
+      // HF#2550 race (Docker/slow-I/O environments hit this; fast native
+      // environments resolve metadata before capture begins and never do).
+      Object.defineProperty(video, "duration", { value: 12, configurable: true });
+      video.dispatchEvent(new Event("loadedmetadata"));
+
+      // Clears init.ts's internal METADATA_REBIND_DEBOUNCE_MS (100ms, not exported).
+      await new Promise((resolve) => setTimeout(resolve, 150));
+
+      const rebindMessages = postMessageSpy.mock.calls
+        .map(([message]) => message as { code?: string } | undefined)
+        .filter((message) => message?.code === "timeline_rebind_after_media_metadata");
+      expect(rebindMessages).toHaveLength(0);
+    } finally {
+      delete (window as { gsap?: unknown }).gsap;
+    }
+  });
+
+  it("still applies the media-metadata duration rebind after renderSeek in Studio preview (no export render-seek config)", async () => {
+    const root = document.createElement("div");
+    root.setAttribute("data-composition-id", "main");
+    root.setAttribute("data-root", "true");
+    root.setAttribute("data-start", "0");
+    root.setAttribute("data-width", "1920");
+    root.setAttribute("data-height", "1080");
+    document.body.appendChild(root);
+
+    const video = document.createElement("video");
+    video.setAttribute("data-start", "0");
+    document.body.appendChild(video);
+
+    (
+      window as unknown as {
+        gsap?: { timeline: () => ReturnType<typeof createPaddableMockTimeline> };
+      }
+    ).gsap = {
+      timeline: () => createPaddableMockTimeline(0),
+    };
+    window.__timelines = { main: createMockTimeline(0) };
+    // No window.__HF_EXPORT_RENDER_SEEK_CONFIG here — Studio's preview iframe
+    // never sets it, and useTimelinePlayer's overhang fallback drives
+    // renderSeek there too. The rebind must still fire for this case.
+
+    const postMessageSpy = vi.spyOn(window, "postMessage");
+
+    try {
+      initSandboxRuntimeModular();
+
+      window.__player?.renderSeek(0);
+
+      await new Promise((resolve) => setTimeout(resolve, 10));
+
+      Object.defineProperty(video, "duration", { value: 12, configurable: true });
+      video.dispatchEvent(new Event("loadedmetadata"));
+
+      // Clears init.ts's internal METADATA_REBIND_DEBOUNCE_MS (100ms, not exported).
+      await new Promise((resolve) => setTimeout(resolve, 150));
+
+      const rebindMessages = postMessageSpy.mock.calls
+        .map(([message]) => message as { code?: string } | undefined)
+        .filter((message) => message?.code === "timeline_rebind_after_media_metadata");
+      expect(rebindMessages).toHaveLength(1);
+    } finally {
+      delete (window as { gsap?: unknown }).gsap;
+    }
+  });
+
  it("sets __renderReady only after timeline is bound, not at __playerReady time", async () => {
    const root = document.createElement("div");
    root.setAttribute("data-composition-id", "main");
@@ -1593,6 +1593,12 @@ export function initSandboxRuntimeModular(): void {

  let metadataRebindDebounceTimerId: number | null = null;
  let metadataRebindApplied = false;
+  // Flips true on the first renderSeek call — the render/producer capture
+  // protocol's signal that it has started deterministically driving frames.
+  // One-way for this page lifetime; every producer render gets a fresh runtime.
+  // See scheduleMetadataDurationHydration for why this gates the async
+  // metadata rebind off once set.
+  let renderCaptureSeekStarted = false;
  const metadataBoundMedia = new Set<HTMLMediaElement>();
  const volumeKeyframeCache = new WeakMap<HTMLMediaElement, VolumeKeyframe[]>();

@@ -1604,6 +1610,23 @@ export function initSandboxRuntimeModular(): void {
    metadataRebindDebounceTimerId = window.setTimeout(() => {
      if (state.tornDown) return;
      metadataRebindDebounceTimerId = null;
+      // The render/producer capture protocol drives frames deterministically
+      // via renderSeek — once it has claimed the timeline, an async
+      // loadedmetadata/durationchange rebind racing that loop is exactly the
+      // "double composite" hazard from HF#2550: this handler runs off its own
+      // debounced browser-side timer, uncoordinated with the capture loop's
+      // own seeks, so a rebind mid-capture can reflow the DOM between one
+      // BeginFrame call and the next. Render-mode duration correction has
+      // already happened deterministically during the probe stage before
+      // capture starts, so once frames are being driven there is nothing left
+      // for this self-correction to usefully do.
+      //
+      // renderSeek is also the entrypoint Studio's own preview iframe falls
+      // back to for overhanging timelines (useTimelinePlayer), so gate on
+      // both signals — renderCaptureSeekStarted alone would silently disable
+      // this self-correction for a live Studio scrub too, where duration
+      // hasn't been pre-resolved by a probe stage and still needs it.
+      if (renderCaptureSeekStarted && window.__HF_EXPORT_RENDER_SEEK_CONFIG) return;
      const resolution = resolveRootTimelineFromDocument();
      if (!resolution.timeline) return;
      const hasResolvedMediaFloor = isUsableTimelineDuration(
@@ -2254,6 +2277,7 @@ export function initSandboxRuntimeModular(): void {
      postState(true);
    },
    renderSeek: (timeSeconds, options) => {
+      renderCaptureSeekStarted = true;
      const quantized = quantizeTimeToFrame(
        Math.max(0, Number(timeSeconds) || 0),
        state.canonicalFps,
@@ -28,6 +28,7 @@ import {
  beginFrameCapture,
  ensureRenderFrameSiblings,
  getCdpSession,
+  pageContentExceedsCaptureHeight,
  pageScreenshotCapture,
  initTransparentBackground,
  shouldDefaultCaptureBeyondViewport,
@@ -2004,6 +2005,23 @@ export async function initializeSession(session: CaptureSession): Promise<void>

    await recordSessionInitTelemetry(session, initStart);

+    // Ground-truth-check the upstream captureBeyondViewport request (see
+    // pageContentExceedsCaptureHeight) now that the page is fully settled —
+    // downgrade it when the page doesn't actually overflow the requested
+    // capture height, since the beyond-viewport CDP path is otherwise pure
+    // downside (HF#2550: phantom duplicate content on SwiftShader) for
+    // content it was never needed for.
+    if (session.options.captureBeyondViewport) {
+      const needsBeyondViewport = await pageContentExceedsCaptureHeight(
+        page,
+        session.options.height,
+      );
+      if (!needsBeyondViewport) {
+        session.options.captureBeyondViewport = false;
+        logInitPhase("captureBeyondViewport downgraded: page content fits the capture viewport");
+      }
+    }
+
    // drawElement or transparent-background init — runs after page is fully ready.
    await initDrawElementOrTransparentBackground(session, page, logInitPhase);

@@ -5,6 +5,7 @@ import { parseHTML } from "linkedom";
 import { type Page } from "puppeteer-core";
 import {
  pageScreenshotCapture,
+  pageContentExceedsCaptureHeight,
  cdpSessionCache,
  ensureRenderFrameSiblings,
  applyDomLayerMask,
@@ -143,6 +144,37 @@ describe("shouldDefaultCaptureBeyondViewport", () => {
  });
 });

+describe("pageContentExceedsCaptureHeight", () => {
+  function makeFakePageWithScrollHeight(scrollHeight: number): Page {
+    return { evaluate: vi.fn().mockResolvedValue(scrollHeight) } as unknown as Page;
+  }
+
+  it("is false when the page fits exactly within the requested height", async () => {
+    const page = makeFakePageWithScrollHeight(1920);
+    await expect(pageContentExceedsCaptureHeight(page, 1920)).resolves.toBe(false);
+  });
+
+  it("tolerates subpixel rounding just past the requested height", async () => {
+    const page = makeFakePageWithScrollHeight(1920.5);
+    await expect(pageContentExceedsCaptureHeight(page, 1920)).resolves.toBe(false);
+  });
+
+  it("tolerates exactly one CSS pixel past the requested height", async () => {
+    const page = makeFakePageWithScrollHeight(1921);
+    await expect(pageContentExceedsCaptureHeight(page, 1920)).resolves.toBe(false);
+  });
+
+  it("detects content just beyond the rounding tolerance", async () => {
+    const page = makeFakePageWithScrollHeight(1922);
+    await expect(pageContentExceedsCaptureHeight(page, 1920)).resolves.toBe(true);
+  });
+
+  it("is true when the page genuinely overflows the requested height", async () => {
+    const page = makeFakePageWithScrollHeight(2007);
+    await expect(pageContentExceedsCaptureHeight(page, 1920)).resolves.toBe(true);
+  });
+});
+
 describe("injectVideoFramesBatch replacement layout", () => {
  it("does not copy opposing inset constraints onto the injected frame image", async () => {
    const { window, document } = parseHTML(
@@ -182,6 +182,30 @@ export async function beginFrameCapture(
  };
 }

+/**
+ * True if the page's actual rendered content is taller than the requested
+ * capture height. `captureBeyondViewport` exists for exactly one reason
+ * (#1094): a native `<video>` surface whose content genuinely overflows the
+ * viewport-bound capture path clips its bottom edge to black. A video that
+ * fits entirely inside its composition's declared viewport doesn't have that
+ * problem — ground-truth measurement beats the coarser "has a video, so
+ * always request beyond-viewport" heuristic, which also unnecessarily routes
+ * every video render through a CDP capture path prone to producing phantom
+ * duplicate content on SwiftShader (#2550).
+ *
+ * Callers measure once after page settle. Hyperframes compositions have a
+ * fixed-height, overflow-clipped render surface; timeline animation may move
+ * pixels within that surface but must not grow document flow during capture.
+ */
+export async function pageContentExceedsCaptureHeight(
+  page: Page,
+  requestedHeight: number,
+): Promise<boolean> {
+  const scrollHeight = await page.evaluate(() => document.documentElement.scrollHeight);
+  // Small tolerance for subpixel layout rounding, not a real overflow signal.
+  return scrollHeight > requestedHeight + 1;
+}
+
 /**
 * Capture a screenshot using standard Page.captureScreenshot CDP call.
 * Fallback for environments where BeginFrame is unavailable (macOS, Windows).
@@ -12,10 +12,13 @@
 * over the software perf optimization: enable beyond-viewport for any render
 * that has a native video surface, regardless of GPU mode.
 *
- * ponytail: blanket-on for video. If the software re-raster tax proves
- * significant on the distributed fleet, narrow it back to comps whose content
- * actually reaches the bottom edge — but that needs a reliable clip predictor
- * first, and a black band is unshippable in the meantime.
+ * This is a candidate, not a final decision: the engine downgrades it back to
+ * `false` once the page is loaded and `pageContentExceedsCaptureHeight`
+ * (screenshotService.ts) measures that the content doesn't actually overflow
+ * the requested capture height — the "reliable clip predictor" this used to
+ * lack. That measurement matters beyond the software re-raster tax: on
+ * SwiftShader, requesting beyond-viewport for content that doesn't need it
+ * can produce phantom duplicate content in the captured frame (HF#2550).
 */
 export function resolveVideoCaptureBeyondViewport(videoCount: number): boolean | undefined {
  if (videoCount <= 0) return undefined;
@@ -0,0 +1,13 @@
+{
+  "name": "HF#2550: video + sub-composition phantom duplicate on captureBeyondViewport",
+  "description": "Regression for HF#2550: a full-length <video> forced captureBeyondViewport on for every render (resolveVideoCaptureBeyondViewport), regardless of whether the page actually needed it. On SwiftShader, requesting beyond-viewport capture for content that fits entirely within the viewport could composite a stale, vertically-offset paint of the page alongside the fresh one — visible here as a phantom second copy of the flowchart-vertical sub-composition around t=11.3-14.3s. Fixed by pageContentExceedsCaptureHeight downgrading captureBeyondViewport back to false once the page is confirmed not to overflow the requested capture height. Caveat: the underlying bug is timing-dependent — it reproduced reliably (10/10) via a direct CLI render on the same host, but did NOT reproduce (0/2) when run through this harness's in-process mode on unpatched code. Keep this fixture as documentation of a confirmed real-world repro and a best-effort guard, not the sole regression protection — packages/engine/src/services/screenshotService.test.ts's pageContentExceedsCaptureHeight tests are the deterministic guard for the actual fix logic.",
+  "tags": ["video", "sub-composition", "regression", "slow"],
+  "minPsnr": 30,
+  "maxFrameFailures": 0,
+  "minAudioCorrelation": 0,
+  "maxAudioLagWindows": 1,
+  "renderConfig": {
+    "fps": 30,
+    "workers": 1
+  }
+}
@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:26bfa531f6a7b4a31ad6e99e838f743b552d3b3c7c461a7c3f7b9c85d02d27cf
+size 1129788
@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:697405f1eaa0f52dbe265d4c2a7674d70d18a05072a14921ded5c3237d377d1c
+size 67061526
@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:df2daf89b0fe9646669bebf269b5fc60ff0bc4923df07d0683c5f68ba730ebcf
+size 761352
@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:11874bb366edf2414d39c0375d9607c5557108712f89cb7ab83a5ebfc9ffd999
+size 28509566