From 7db84fc0ad9e642c4890dd0d99e32b1126c065d8 Mon Sep 17 00:00:00 2001
From: Tzuhany <122426901+Tzuhany@users.noreply.github.com>
Date: Fri, 26 Jun 2026 12:44:17 +0800
Subject: [PATCH] fix(producer): rebuildExtractedFramesFromPlanDir off-by-one
 in framePaths key indexing (#1730)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix(producer): rebuildExtractedFramesFromPlanDir off-by-one in framePaths key indexing

In distributed chunk-lambda render mode, every <video>'s first-paint
frame (the moment a vid first becomes visible on the composition
timeline) renders as PRISTINE Y=16 black. For a 3-vid back-to-back
composition (v1: 0-4s, v2: 4-8s, v3: 8-12s at 30fps), frames 0, 121,
242 are all PRISTINE black; the render then either stays black for 1
frame, or shows body bg + persistent overlays only (Y~22 with sparse
highlights from text/logo). The symptom only reproduces in distributed
mode — local single-process renders are unaffected.

Root cause: rebuildExtractedFramesFromPlanDir builds the framePaths Map
with 1-based keys, but the consumer (getFrameAtTime at
engine/videoFrameExtractor.ts:958) computes a 0-based frame index via
Math.floor(localTime * fps + 1e-9). For each vid's first-paint frame
(localTime === 0 → frameIndex === 0), framePaths.get(0) returns
undefined; the vid is silently dropped from activePayloads,
videoFrameInjector doesn't fire, syncVideoFrameVisibility hides
everything, and BeginFrame screenshots an empty composition.

Every other site in the codebase builds/consumes framePaths with
0-based keys:

  - engine/videoFrameExtractor.ts:317     framePaths.set(index, ...)
  - engine/extractionCache.ts:204          framePaths.set(idx, ...)
  - engine/videoFrameExtractor.test.ts:264/1066  framePaths.set(i, ...)
  - engine/videoFrameExtractor.ts:958 (consumer) Math.floor 0-based
  - producer/renderOrchestrator.test.ts:287/315/349  framePaths.get(0)

Only producer/distributed/renderChunk.ts:198 was 1-based, with a
stale comment claiming FrameLookupTable indexes frames 1-based —
which the surrounding evidence contradicts. This is why local tests
pass while distributed-lambda renders always had cold black at each
vid first paint.

Verified locally against a 3-vid composition and a single-vid 4-worker
case in a Lambda render fleet. Before fix: every vid first-paint frame
is PRISTINE Y=16 black. After fix: all frames are valid source content,
blackdetect reports zero black regions outside legitimate source video
content (intentional fade-ins / hard cuts in source mp4).

* test(producer): pin rebuildExtractedFramesFromPlanDir 0-based framePaths contract

Regression guard for the off-by-one fix in HF#1730. The pre-fix code
indexed framePaths 1-based while the consumer (getFrameAtTime in
engine/videoFrameExtractor.ts:958) reads 0-based, dropping every
<video>'s first-paint frame in distributed chunk-lambda renders.

Asserts framePaths.get(0) resolves to the first extracted frame, and
framePaths.get(N-1) resolves to the last — pre-fix the keys were
shifted to [1..N], so get(0) returned undefined and get(N) resolved.
Verified to fail against the previous i+1 indexing.

Also exports rebuildExtractedFramesFromPlanDir (was module-local) so the
test can call it directly. Pure logic worth testing in isolation — the
bug only reproduces under distributed mode and the existing
renderChunk.test.ts already pays a multi-second Chrome smoke probe in
its module-level beforeAll, so the regression check lives in its own
file (rebuildExtractedFrames.test.ts) and runs Chrome-free in ~10ms.

The function's doc comment said "1-based framePaths" — updated to
"0-based" with a pointer to the consumer site and the bug context.

Per Miguel's REQUEST_CHANGES on HF#1730.

— Jerrai (https://claude.com/claude-code)

---------

Co-authored-by: James <james.russo@heygen.com>
---
 .../rebuildExtractedFrames.test.ts            | 153 ++++++++++++++++++
 .../src/services/distributed/renderChunk.ts   |  18 ++-
 2 files changed, 166 insertions(+), 5 deletions(-)
 create mode 100644 packages/producer/src/services/distributed/rebuildExtractedFrames.test.ts

diff --git a/packages/producer/src/services/distributed/rebuildExtractedFrames.test.ts b/packages/producer/src/services/distributed/rebuildExtractedFrames.test.ts
new file mode 100644
index 000000000..d45fa1c79
--- /dev/null
+++ b/packages/producer/src/services/distributed/rebuildExtractedFrames.test.ts
@@ -0,0 +1,153 @@
+/**
+ * Regression guard for HF#1731 / HF#1730 — pins the 0-based `framePaths`
+ * key convention on `rebuildExtractedFramesFromPlanDir`.
+ *
+ * Why a separate file from `renderChunk.test.ts`: that file's top-level
+ * `beforeAll` boots a Chrome smoke probe so the byte-identical-retry
+ * assertions can soft-skip on hosts where chrome-headless-shell can't
+ * initialize. The probe is a 5-15s tax on the whole module even when no
+ * Chrome-dependent test runs. This file is pure-filesystem and stays
+ * Chrome-free so the regression check runs in ~10ms on every PR.
+ *
+ * The bug: the consumer of `framePaths` is
+ * `videoFrameExtractor.ts:getFrameAtTime`, which computes
+ * `Math.floor(localTime * fps + 1e-9)` (0-based) and reads
+ * `framePaths.get(frameIndex)`. The pre-fix code in
+ * `rebuildExtractedFramesFromPlanDir` wrote `framePaths.set(i + 1, …)`
+ * instead of `framePaths.set(i, …)`, so `framePaths.get(0)` returned
+ * `undefined` at every `<video>`'s first-paint frame: the vid silently
+ * dropped out of activePayloads, the injector didn't fire, and
+ * BeginFrame screenshotted an empty composition (Y≈22 black flash for
+ * one frame at each first-paint boundary). Symptom only reproduced in
+ * distributed mode — local single-process renders take a different
+ * code path (`videoFrameExtractor.ts:317`) that was already 0-based.
+ *
+ * This test must FAIL against the previous `i + 1` indexing and PASS
+ * against the fix. Don't soften it into a "key set has expected size"
+ * shape — the bug was specifically that `get(0)` returned undefined,
+ * so assert that directly.
+ */
+
+import { describe, expect, it } from "bun:test";
+import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { rebuildExtractedFramesFromPlanDir } from "./renderChunk.js";
+import type { PlanVideosJson } from "./shared.js";
+
+function makeFramesDir(planDir: string, videoId: string, frameNames: string[]): void {
+  const outputDir = join(planDir, "video-frames", videoId);
+  mkdirSync(outputDir, { recursive: true });
+  for (const name of frameNames) {
+    // Contents don't matter — the function only lists + sorts the dir
+    // and maps names to absolute paths. A single byte keeps the test
+    // fast and stays well clear of any filesystem reservation quirks.
+    writeFileSync(join(outputDir, name), "x", "utf-8");
+  }
+}
+
+// The function never reads inside `metadata`, it just forwards it onto
+// the returned `ExtractedFrames`. A bare cast spares us the full
+// `VideoMetadata` shape per test.
+const VIDEO_METADATA_STUB = {} as PlanVideosJson["extracted"][number]["metadata"];
+
+describe("rebuildExtractedFramesFromPlanDir", () => {
+  it("indexes framePaths 0-based (regression guard for HF#1731)", () => {
+    const planDir = mkdtempSync(join(tmpdir(), "hf-rebuild-frames-0based-"));
+    try {
+      const videoId = "vid-0";
+      // Zero-padded monotonic names — same shape `extractVideoFramesRange`
+      // produces (`frame_%05d.jpg`).
+      const frameNames = [
+        "frame_00001.jpg",
+        "frame_00002.jpg",
+        "frame_00003.jpg",
+        "frame_00004.jpg",
+        "frame_00005.jpg",
+      ];
+      makeFramesDir(planDir, videoId, frameNames);
+
+      const result = rebuildExtractedFramesFromPlanDir(planDir, [
+        {
+          videoId,
+          srcPath: "/does/not/matter.mp4",
+          framePattern: "frame_%05d.jpg",
+          fps: 30,
+          totalFrames: frameNames.length,
+          metadata: VIDEO_METADATA_STUB,
+        },
+      ]);
+
+      expect(result).toHaveLength(1);
+      const extracted = result[0]!;
+      expect(extracted.framePaths.size).toBe(frameNames.length);
+
+      // The load-bearing assertion. `getFrameAtTime` calls
+      // `framePaths.get(0)` for every video's first-paint frame
+      // (localTime === 0 → frameIndex === 0). The pre-fix code's `i + 1`
+      // indexing meant this returned `undefined` and the vid silently
+      // dropped from activePayloads — that's HF#1731.
+      const first = extracted.framePaths.get(0);
+      expect(first).toBeDefined();
+      expect(first).toBe(join(planDir, "video-frames", videoId, "frame_00001.jpg"));
+
+      // Last frame at index N-1 (0-based) must resolve; index N must not.
+      // Together with `get(0)` defined this fully pins the 0-based
+      // contract — the pre-fix shape would have `get(N)` defined and
+      // `get(0)` undefined.
+      const last = extracted.framePaths.get(frameNames.length - 1);
+      expect(last).toBe(join(planDir, "video-frames", videoId, "frame_00005.jpg"));
+      expect(extracted.framePaths.get(frameNames.length)).toBeUndefined();
+    } finally {
+      rmSync(planDir, { recursive: true, force: true });
+    }
+  });
+
+  it("preserves 0-based indexing across multiple videos in the same planDir", () => {
+    // Multi-video shape — the HF#1731 repro was a back-to-back composition
+    // (v1: 0-4s, v2: 4-8s, v3: 8-12s) and EVERY vid's first-paint frame
+    // was PRISTINE black. The function must produce the 0-based contract
+    // for every video in the manifest, not just the first.
+    const planDir = mkdtempSync(join(tmpdir(), "hf-rebuild-frames-multi-"));
+    try {
+      makeFramesDir(planDir, "vid-a", ["frame_00001.jpg", "frame_00002.jpg"]);
+      makeFramesDir(planDir, "vid-b", ["frame_00001.jpg", "frame_00002.jpg", "frame_00003.jpg"]);
+
+      const result = rebuildExtractedFramesFromPlanDir(planDir, [
+        {
+          videoId: "vid-a",
+          srcPath: "/a.mp4",
+          framePattern: "frame_%05d.jpg",
+          fps: 30,
+          totalFrames: 2,
+          metadata: VIDEO_METADATA_STUB,
+        },
+        {
+          videoId: "vid-b",
+          srcPath: "/b.mp4",
+          framePattern: "frame_%05d.jpg",
+          fps: 30,
+          totalFrames: 3,
+          metadata: VIDEO_METADATA_STUB,
+        },
+      ]);
+
+      expect(result).toHaveLength(2);
+      // Every video's `framePaths.get(0)` must resolve — pre-fix, all of
+      // them returned undefined and every vid's first-paint dropped.
+      expect(result[0]!.framePaths.get(0)).toBe(
+        join(planDir, "video-frames", "vid-a", "frame_00001.jpg"),
+      );
+      expect(result[1]!.framePaths.get(0)).toBe(
+        join(planDir, "video-frames", "vid-b", "frame_00001.jpg"),
+      );
+      // Cleanup ownership flag — chunk workers don't own the planDir's
+      // video-frames tree (the controller does); a flip would cause the
+      // injector cleanup to rm bytes another worker may still be reading.
+      expect(result[0]!.ownedByLookup).toBe(false);
+      expect(result[1]!.ownedByLookup).toBe(false);
+    } finally {
+      rmSync(planDir, { recursive: true, force: true });
+    }
+  });
+});
diff --git a/packages/producer/src/services/distributed/renderChunk.ts b/packages/producer/src/services/distributed/renderChunk.ts
index 33d4186c3..c77d98b3a 100644
--- a/packages/producer/src/services/distributed/renderChunk.ts
+++ b/packages/producer/src/services/distributed/renderChunk.ts
@@ -166,10 +166,19 @@ export interface ChunkResult {
  * Rebuild the engine's in-memory `ExtractedFrames[]` from the on-disk
  * planDir layout. `<planDir>/video-frames/<videoId>/` holds the numbered
  * frame files plan() extracted; this lists each dir and rebuilds the
- * 1-based `framePaths` Map that `FrameLookupTable` / `videoFrameInjector`
- * both index against.
+ * 0-based `framePaths` Map that `FrameLookupTable` / `videoFrameInjector`
+ * both index against — the consumer is
+ * `videoFrameExtractor.ts:getFrameAtTime`, which floors `localTime * fps`
+ * to a 0-based index and reads `framePaths.get(frameIndex)`. Any drift
+ * from that key convention silently drops every `<video>`'s first-paint
+ * frame; see HF#1731 / HF#1730.
+ *
+ * Exported so a unit test can pin the 0-based contract without spinning
+ * up the heavyweight Docker fixture — the bug surfaces only under
+ * distributed mode and only at video first-paint, so this primitive is
+ * the right granularity to guard.
  */
-function rebuildExtractedFramesFromPlanDir(
+export function rebuildExtractedFramesFromPlanDir(
   planDir: string,
   videos: PlanVideosJson["extracted"],
 ): ExtractedFrames[] {
@@ -194,8 +203,7 @@ function rebuildExtractedFramesFromPlanDir(
     for (let i = 0; i < frames.length; i++) {
       const frameName = frames[i];
       if (!frameName) continue;
-      // FrameLookupTable indexes frames 1-based.
-      framePaths.set(i + 1, join(outputDir, frameName));
+      framePaths.set(i, join(outputDir, frameName));
     }
     result.push({
       videoId: v.videoId,