From f4ecf96918dbcc95bbe5ac20bf10c7d05b1e13a2 Mon Sep 17 00:00:00 2001
From: James <james.russo@heygen.com>
Date: Mon, 4 May 2026 20:16:10 -0700
Subject: [PATCH] fix(engine,cli,producer): address PR #627 review feedback
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- engine/chunkEncoder, engine/streamingEncoder: extend `-bf 0` to GPU h264
  paths (nvenc, qsv, vaapi) and `-b_strategy 0` for qsv so GPU-encoded
  outputs avoid negative-DTS freezes too — not just SW libx264.
- engine/videoFrameExtractor: detect mid-path traversal (e.g.
  `assets/../../foo.mp4`) by normalizing first and re-anchoring at the
  project root. Adds a regression test.
- engine/videoFrameExtractor: dedupe stderr "src not resolvable" warnings
  by `video.src` so a comp with N broken sources logs once, not N times.
- engine/videoFrameExtractor.test: drop dynamic `require("node:fs")`,
  use ES `import { writeFileSync } from "node:fs"`.
- engine/ffprobe: extract `readTagCI` helper for case-insensitive ffprobe
  tag reads (will recur for other libavformat-versioned sidecar tags).
- cli/background-removal/pipeline: collapse Quality / QUALITIES /
  QUALITY_CRF / DEFAULT_QUALITY / isQuality surface using
  `Quality = keyof typeof QUALITY_CRF`.
- producer/renderOrchestrator: replace `v.src.startsWith("/")` with
  `isAbsolute(v.src)` in the HDR probe path so Windows absolute paths
  (`C:\...`) aren't treated as relative — matches the audioMixer guard.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 .../cli/src/background-removal/pipeline.ts    | 12 ++--
 packages/engine/src/services/chunkEncoder.ts  | 19 ++++++
 .../engine/src/services/streamingEncoder.ts   | 13 ++++
 .../src/services/videoFrameExtractor.test.ts  | 53 ++++++++++-----
 .../src/services/videoFrameExtractor.ts       | 68 +++++++++++++------
 packages/engine/src/utils/ffprobe.ts          | 23 +++++--
 .../src/services/renderOrchestrator.ts        |  7 +-
 7 files changed, 138 insertions(+), 57 deletions(-)

diff --git a/packages/cli/src/background-removal/pipeline.ts b/packages/cli/src/background-removal/pipeline.ts
index 03ce3cb63..99b95386e 100644
--- a/packages/cli/src/background-removal/pipeline.ts
+++ b/packages/cli/src/background-removal/pipeline.ts
@@ -20,15 +20,15 @@ import { type Device, type ModelId } from "./manager.js";
 
 export type OutputFormat = "webm" | "mov" | "png";
 
-export type Quality = "fast" | "balanced" | "best";
-
-export const QUALITIES: readonly Quality[] = ["fast", "balanced", "best"] as const;
-
-export const QUALITY_CRF: Record<Quality, number> = {
+export const QUALITY_CRF = {
   fast: 30,
   balanced: 18,
   best: 12,
-};
+} as const;
+
+export type Quality = keyof typeof QUALITY_CRF;
+
+export const QUALITIES = Object.keys(QUALITY_CRF) as readonly Quality[];
 
 export const DEFAULT_QUALITY: Quality = "balanced";
 
diff --git a/packages/engine/src/services/chunkEncoder.ts b/packages/engine/src/services/chunkEncoder.ts
index a5c7ffe3e..6e9ea1b2d 100644
--- a/packages/engine/src/services/chunkEncoder.ts
+++ b/packages/engine/src/services/chunkEncoder.ts
@@ -139,6 +139,25 @@ export function buildEncoderArgs(
           else args.push("-global_quality", String(quality));
           break;
       }
+
+      // Same B-frame story as the SW branch below — nvenc emits B-frames
+      // by default (qsv via b_strategy, vaapi too), and the negative-DTS
+      // freeze hits the same downstream players. The unconditional
+      // `-avoid_negative_ts make_zero` near the bottom of this function
+      // covers the mux level, but we belt-and-suspenders the encoder too
+      // so even tools that consume the chunk file directly (without going
+      // through our mux step) play correctly. videotoolbox doesn't accept
+      // `-bf` so it's skipped — videotoolbox h264 also doesn't emit
+      // negative DTS in practice on macOS Sonoma+.
+      if (
+        codec === "h264" &&
+        (gpuEncoder === "nvenc" || gpuEncoder === "qsv" || gpuEncoder === "vaapi")
+      ) {
+        args.push("-bf", "0");
+        if (gpuEncoder === "qsv") {
+          args.push("-b_strategy", "0");
+        }
+      }
     } else {
       const encoderName = codec === "h264" ? "libx264" : "libx265";
       args.push("-c:v", encoderName, "-preset", preset);
diff --git a/packages/engine/src/services/streamingEncoder.ts b/packages/engine/src/services/streamingEncoder.ts
index a527df830..03c0d0735 100644
--- a/packages/engine/src/services/streamingEncoder.ts
+++ b/packages/engine/src/services/streamingEncoder.ts
@@ -221,6 +221,19 @@ export function buildStreamingArgs(
           else args.push("-global_quality", String(quality));
           break;
       }
+
+      // Mirror SW branch: GPU h264 paths emit B-frames by default (nvenc, qsv,
+      // vaapi) and produce the same negative-DTS freeze for downstream players.
+      // See chunkEncoder.buildEncoderArgs for the full explanation.
+      if (
+        codec === "h264" &&
+        (gpuEncoder === "nvenc" || gpuEncoder === "qsv" || gpuEncoder === "vaapi")
+      ) {
+        args.push("-bf", "0");
+        if (gpuEncoder === "qsv") {
+          args.push("-b_strategy", "0");
+        }
+      }
     } else {
       const encoderName = codec === "h264" ? "libx264" : "libx265";
       args.push("-c:v", encoderName, "-preset", preset);
diff --git a/packages/engine/src/services/videoFrameExtractor.test.ts b/packages/engine/src/services/videoFrameExtractor.test.ts
index e83ca1254..29fd98eed 100644
--- a/packages/engine/src/services/videoFrameExtractor.test.ts
+++ b/packages/engine/src/services/videoFrameExtractor.test.ts
@@ -1,5 +1,13 @@
 import { afterAll, beforeAll, describe, expect, it } from "vitest";
-import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync } from "node:fs";
+import {
+  existsSync,
+  mkdirSync,
+  mkdtempSync,
+  readFileSync,
+  readdirSync,
+  rmSync,
+  writeFileSync,
+} from "node:fs";
 import { createHash } from "node:crypto";
 import { join } from "node:path";
 import { tmpdir } from "node:os";
@@ -26,19 +34,11 @@ import { runFfmpeg } from "../utils/runFfmpeg.js";
 // synthesized VFR fixture.
 const HAS_FFMPEG = spawnSync("ffmpeg", ["-version"]).status === 0;
 
-// Regression: a long-standing footgun where `<video src="../assets/foo">`
-// inside a sub-composition silently dropped the video from extraction. The
-// browser's URL resolver clamps `..` at the served origin's root (so the
-// page renders fine in the studio), but `path.join(projectDir, "../assets/foo")`
-// normalizes to <parentOfProjectDir>/assets/foo, which doesn't exist. Result:
-// no extracted frames, no per-frame injection, the rendered output shows the
-// <video>'s first decoded frame for the whole clip duration. The resolver
-// now mirrors browser semantics by stripping leading `..` segments as a
-// fallback when the literal join doesn't exist.
-// Codec-based alpha defaulting is the deeper fix for the
-// `alpha_mode`-vs-`ALPHA_MODE` tag-detection bug (see ffprobe.test.ts). The
-// extractor uses these helpers to decide:
-//   1. whether to force the alpha-aware decoder (libvpx-vp9)
+// Codec-based alpha defaulting replaces tag-based detection (the
+// alpha_mode/ALPHA_MODE case bug — see ffprobe.test.ts for the regression
+// pin on that). The extractor uses these helpers for two decisions:
+//   1. whether to force the alpha-aware decoder (libvpx-vp9 for VP9, libvpx
+//      for VP8)
 //   2. whether to default the cached frame format to PNG (with alpha) vs JPG
 // The "default to capable" trade is small file-size growth on opaque VP9
 // content for correctness on alpha-having content even when the sidecar tag
@@ -70,14 +70,21 @@ describe("codec alpha capability", () => {
   });
 });
 
+// Regression: a long-standing footgun where `<video src="../assets/foo">`
+// inside a sub-composition silently dropped the video from extraction. The
+// browser's URL resolver clamps `..` at the served origin's root (so the
+// page renders fine in the studio), but `path.join(projectDir, "../assets/foo")`
+// normalizes to <parentOfProjectDir>/assets/foo, which doesn't exist —
+// extraction skipped, no frame injection, rendered output shows the video's
+// first decoded frame for the whole clip duration. The resolver now mirrors
+// browser semantics by clamping any traversal that escapes the project root.
 describe("resolveProjectRelativeSrc — sub-composition path clamping", () => {
   let tmp: string;
 
   beforeAll(() => {
     tmp = mkdtempSync(join(tmpdir(), "hf-resolver-"));
     mkdirSync(join(tmp, "project", "assets"), { recursive: true });
-    // Empty file is enough for existsSync — this test is about path resolution.
-    require("node:fs").writeFileSync(join(tmp, "project", "assets", "foo.mp4"), "");
+    writeFileSync(join(tmp, "project", "assets", "foo.mp4"), "");
   });
   afterAll(() => {
     rmSync(tmp, { recursive: true, force: true });
@@ -90,7 +97,7 @@ describe("resolveProjectRelativeSrc — sub-composition path clamping", () => {
     );
   });
 
-  it("clamps a leading `../` (sub-comp authoring) so `../assets/foo.mp4` resolves to assets/foo.mp4", () => {
+  it("clamps a leading `../` so `../assets/foo.mp4` resolves to assets/foo.mp4", () => {
     const projectDir = join(tmp, "project");
     expect(resolveProjectRelativeSrc("../assets/foo.mp4", projectDir)).toBe(
       join(projectDir, "assets/foo.mp4"),
@@ -104,6 +111,16 @@ describe("resolveProjectRelativeSrc — sub-composition path clamping", () => {
     );
   });
 
+  it("clamps mid-path traversal that escapes baseDir (not just leading `..`)", () => {
+    // `assets/../../foo.mp4` collapses past projectDir via path.join — this
+    // case used to silently escape; the resolver now strips embedded `..`
+    // segments and re-anchors at the project root.
+    const projectDir = join(tmp, "project");
+    expect(resolveProjectRelativeSrc("assets/../../assets/foo.mp4", projectDir)).toBe(
+      join(projectDir, "assets/foo.mp4"),
+    );
+  });
+
   it("returns the (non-existent) base-dir path on miss so callers get a stable error message", () => {
     const projectDir = join(tmp, "project");
     expect(resolveProjectRelativeSrc("../assets/missing.mp4", projectDir)).toBe(
@@ -115,7 +132,7 @@ describe("resolveProjectRelativeSrc — sub-composition path clamping", () => {
     const projectDir = join(tmp, "project");
     const compiledDir = join(tmp, "compiled");
     mkdirSync(join(compiledDir, "assets"), { recursive: true });
-    require("node:fs").writeFileSync(join(compiledDir, "assets", "foo.mp4"), "");
+    writeFileSync(join(compiledDir, "assets", "foo.mp4"), "");
     expect(resolveProjectRelativeSrc("assets/foo.mp4", projectDir, compiledDir)).toBe(
       join(compiledDir, "assets/foo.mp4"),
     );
diff --git a/packages/engine/src/services/videoFrameExtractor.ts b/packages/engine/src/services/videoFrameExtractor.ts
index 33d3173da..67681df04 100644
--- a/packages/engine/src/services/videoFrameExtractor.ts
+++ b/packages/engine/src/services/videoFrameExtractor.ts
@@ -7,7 +7,7 @@
 
 import { spawn } from "child_process";
 import { existsSync, mkdirSync, readdirSync, rmSync } from "fs";
-import { isAbsolute, join } from "path";
+import { isAbsolute, join, posix, resolve, sep } from "path";
 import { parseHTML } from "linkedom";
 import { extractMediaMetadata, type VideoMetadata } from "../utils/ffprobe.js";
 import {
@@ -498,27 +498,44 @@ async function convertVfrToCfr(
  * `<projectDir>/assets/foo`) but the filesystem-side resolver lands at
  * `<parentOfProjectDir>/assets/foo` — file missing, extraction skipped,
  * the rendered output shows the video's first frame for the whole clip.
- * Mirror the clamp here.
+ *
+ * The clamp covers two escape patterns: leading `..` (`../assets/foo`) AND
+ * mid-path escapes (`assets/../../foo`) that `path.join` collapses past the
+ * project root silently. Both fall back to a project-rooted candidate that
+ * strips traversal from the resolved path.
  *
  * Returns the first existing candidate, or the base-dir join on miss so
- * the caller's existsSync check produces a stable error path.
+ * the caller's `existsSync` check produces a stable error path.
  */
 export function resolveProjectRelativeSrc(
   src: string,
   baseDir: string,
   compiledDir?: string,
 ): string {
-  const candidates = [
-    compiledDir && join(compiledDir, src),
-    join(baseDir, src),
-    ...(src.startsWith("..")
-      ? (() => {
-          const clamped = src.replace(/^(\.\.[\\/])+/, "");
-          return [compiledDir && join(compiledDir, clamped), join(baseDir, clamped)];
-        })()
-      : []),
-  ].filter(Boolean) as string[];
-  return candidates.find(existsSync) ?? join(baseDir, src);
+  const fromCompiled = compiledDir ? join(compiledDir, src) : null;
+  const fromBase = join(baseDir, src);
+  const candidates: string[] = [];
+  if (fromCompiled) candidates.push(fromCompiled);
+  candidates.push(fromBase);
+  // If the joined result escapes the project root (either via leading `..`
+  // or mid-path traversal that path.join collapsed past baseDir), retry
+  // with the basename re-anchored at the project root. This mirrors the
+  // browser URL clamp without relying on a particular `..` shape.
+  const baseAbs = resolve(baseDir);
+  const fromBaseAbs = resolve(fromBase);
+  if (!fromBaseAbs.startsWith(baseAbs + sep) && fromBaseAbs !== baseAbs) {
+    // Normalize first (`assets/../../assets/foo.mp4` → `../assets/foo.mp4`)
+    // then strip any remaining leading `..` segments. Stripping `..` from the
+    // raw input would leave dangling siblings (`assets/../../assets/foo`
+    // would become `assets/assets/foo` instead of `assets/foo`).
+    const normalized = posix.normalize(src.replace(/\\/g, "/"));
+    const stripped = normalized.replace(/^(\.\.\/)+/, "");
+    if (stripped && stripped !== src && !stripped.startsWith("..")) {
+      if (compiledDir) candidates.push(join(compiledDir, stripped));
+      candidates.push(join(baseDir, stripped));
+    }
+  }
+  return candidates.find(existsSync) ?? fromBase;
 }
 
 export async function extractAllVideoFrames(
@@ -549,6 +566,9 @@ export async function extractAllVideoFrames(
   // Phase 1: Resolve paths and download remote videos
   const phase1Start = Date.now();
   const resolvedVideos: Array<{ video: VideoElement; videoPath: string }> = [];
+  // Dedupe missing-src warnings: a composition with N <video> elements all
+  // pointing at the same broken src should only print one warning, not N.
+  const warnedSrcs = new Set<string>();
   for (const video of videos) {
     if (signal?.aborted) break;
     try {
@@ -569,14 +589,18 @@ export async function extractAllVideoFrames(
 
       if (!existsSync(videoPath)) {
         // Loud: silent miss leaves the rendered video frozen at frame 0 with
-        // no error in stdout — extremely confusing for authors.
-        process.stderr.write(
-          `[hyperframes:render] WARNING: video <${video.id}> src="${video.src}" ` +
-            `could not be resolved on disk (looked for ${videoPath}). ` +
-            `The rendered output will show this video's first frame for the entire clip duration. ` +
-            `If your <video> lives inside a sub-composition, prefer project-root-relative paths ` +
-            `(e.g. src="assets/foo.mp4") over "../assets/foo.mp4".\n`,
-        );
+        // no error in stdout — extremely confusing for authors. Dedupe by
+        // src so 50 broken videos pointing at the same path don't spam.
+        if (!warnedSrcs.has(video.src)) {
+          warnedSrcs.add(video.src);
+          process.stderr.write(
+            `[hyperframes:render] WARNING: video src="${video.src}" ` +
+              `could not be resolved on disk (looked for ${videoPath}). ` +
+              `The rendered output will show this video's first frame for the entire clip duration. ` +
+              `If your <video> lives inside a sub-composition, prefer project-root-relative paths ` +
+              `(e.g. src="assets/foo.mp4") over "../assets/foo.mp4".\n`,
+          );
+        }
         errors.push({ videoId: video.id, error: `Video file not found: ${videoPath}` });
         continue;
       }
diff --git a/packages/engine/src/utils/ffprobe.ts b/packages/engine/src/utils/ffprobe.ts
index fb8667e2c..c60d8c49d 100644
--- a/packages/engine/src/utils/ffprobe.ts
+++ b/packages/engine/src/utils/ffprobe.ts
@@ -203,6 +203,21 @@ function extractStillImageMetadata(filePath: string): StillImageMetadata | null
   }
 }
 
+/**
+ * Read an ffprobe tag case-insensitively. ffmpeg/libavformat versions disagree
+ * on tag casing — VP9 alpha is `alpha_mode` in older builds and `ALPHA_MODE`
+ * in newer ones; HDR tags vary similarly. Use this for any sidecar tag where
+ * you want to be resilient across muxer versions.
+ */
+function readTagCI(tags: Record<string, string | undefined> | undefined, name: string): string {
+  if (!tags) return "";
+  const target = name.toLowerCase();
+  for (const [key, value] of Object.entries(tags)) {
+    if (key.toLowerCase() === target && typeof value === "string") return value;
+  }
+  return "";
+}
+
 function parseFrameRate(frameRateStr: string | undefined): number {
   if (!frameRateStr) return 0;
   const parts = frameRateStr.split("/");
@@ -277,13 +292,7 @@ export async function extractMediaMetadata(filePath: string): Promise<VideoMetad
         : null;
     const colorSpace = ffprobeColorSpace ?? stillImageMeta?.colorSpace ?? null;
     const pixelFormat = videoStream.pix_fmt || "";
-    // ffmpeg writes the VP9-alpha sidecar tag as either `alpha_mode` (older
-    // builds) or `ALPHA_MODE` (libavformat ≥ 6.x). Read case-insensitively
-    // so the alpha-aware extraction path triggers on both — without this
-    // the producer extracted alpha-having webms as opaque JPGs and the
-    // injected <img> covered everything below it on the z-stack.
-    const tags = (videoStream.tags ?? {}) as Record<string, string | undefined>;
-    const alphaMode = tags.alpha_mode ?? tags.ALPHA_MODE ?? "";
+    const alphaMode = readTagCI(videoStream.tags, "alpha_mode");
     const hasAlpha =
       /(^|[^a-z])yuva|rgba|argb|bgra|gbrap|gray[a-z0-9]*a/i.test(pixelFormat) || alphaMode === "1";
 
diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts
index 16a73f782..af7157cca 100644
--- a/packages/producer/src/services/renderOrchestrator.ts
+++ b/packages/producer/src/services/renderOrchestrator.ts
@@ -2316,10 +2316,9 @@ export async function executeRenderJob(
           // Use the shared resolver so a `<video src="../assets/foo">` in a
           // sub-composition resolves the same way the browser would (see
           // resolveProjectRelativeSrc in videoFrameExtractor for the full
-          // explanation). Without this, the HDR probe silently no-ops on
-          // those videos and the downstream extraction bug shows up
-          // amplified — videos register but never extract.
-          const videoPath = v.src.startsWith("/")
+          // explanation). isAbsolute (not `startsWith("/")`) so Windows
+          // absolute paths like `C:\...` skip the join correctly.
+          const videoPath = isAbsolute(v.src)
             ? v.src
             : resolveProjectRelativeSrc(v.src, projectDir, compiledDir);
           if (!existsSync(videoPath)) return;