fix(engine): auto-normalize VFR video inputs to CFR before frame extraction (#360)

* fix(engine): auto-normalize VFR video inputs to CFR before frame extraction Screen recordings (macOS ScreenCaptureKit, QuickTime, phone videos) are commonly variable-frame-rate. When such inputs hit the extractor's `-ss <start> -i <video> -t <dur> -vf fps=N` pipeline, the fps filter can emit fewer frames than requested — for a 4-second 30fps segment starting mid-file, the output was ~90 frames instead of 120. `FrameLookupTable.getFrameAtTime` returns null for out-of-range indices, so the compositor held the last valid frame and the user perceived the video as freezing. This matches the bug report from an X community post where a user said "all of them freezes" on their screen recording scenes. The engine already detects VFR via `metadata.isVFR` in ffprobe.ts but never acted on it — the compiler only logged a warning. This change mirrors the existing SDR→HDR normalization pattern: when a source is detected as VFR, re-encode only the used segment with `-fps_mode cfr -r <fps> -preset fast -crf 18` before extraction. Scoping the re-encode to `[mediaStart, mediaStart+duration]` means a 30-second clip cut from a 60-minute screen recording pays ~1s of transcode cost, not 18s. Benchmarked locally: Baseline (current): 32-39% duplicate frames, 25% frame-count shortfall on mid-file segments. Tier 1 (flag changes only): ~same — fps filter issue is not flag-fixable. Tier 2 (CFR preflight): 1.7-6% duplicate frames, correct frame count in every scenario tested. The compiler warning that previously told users to manually re-encode is downgraded to `console.info` since the engine now handles it. — Rames Jusso * refactor(engine): clean up VFR normalization loop after review - Drop the `vfrNormDirCreated` flag; `mkdirSync({recursive:true})` is idempotent and cheap. - Don't re-wrap the `VFR→CFR conversion failed` prefix — `convertVfrToCfr` already throws a message with that label; adding it again in the catch produced "VFR→CFR conversion failed: VFR→CFR conversion failed (exit 1)". - Shorten the Phase 2b header comment; the function docstring above `convertVfrToCfr` already explains the failure modes and rationale. - Note which frame windows the VFR fixture's select filter drops so the magic numbers are scannable. No behavior change; 311/311 engine tests still pass. — Rames Jusso * test(engine): add VFR regression unit tests Adds a describe block that synthesizes a VFR fixture via ffmpeg and asserts the extractor produces the expected frame count (no shortfall) and no long runs of duplicate frames — the user-visible "frozen screen recording" symptom. Covers both a mid-file segment and the full-file case. Guarded with describe.skipIf(!HAS_FFMPEG) because the CI Test job on ubuntu-24.04 and the Windows test-windows job don't install ffmpeg. The producer-level regression test in packages/producer/tests/vfr-screen-recording/ runs inside Dockerfile.test (which has ffmpeg) and is the primary CI signal for this bug; these unit tests are supplementary coverage for local and any ffmpeg-equipped CI environment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(producer): add vfr-screen-recording regression test End-to-end CI regression coverage for PR #360 via the existing regression-harness: renders a 3s composition containing a real macOS ScreenCaptureKit clip (r_frame_rate=120, avg≈36fps) seeked to mediaStart=1, then PSNR-compares against a committed output.mp4. Fixture src/clip.mp4 (108 KB) is a 5-second excerpt downscaled to 480×332 with -fps_mode passthrough to preserve the VFR timestamps. Content is the public hyperframes OSS repo root page — see NOTICE.md for provenance. With the fix applied, all 100 PSNR checkpoints pass. With the fix reverted, 66 of 100 fail (PSNR drops from ~43 dB to ~20 dB in the duplicate-frame windows). Tagged "regression,video,vfr" so it runs in the fast shard of .github/workflows/regression.yml automatically. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(producer): regenerate vfr-screen-recording baseline in Docker The committed golden output.mp4 was initially rendered on the host machine; CI runs the renderer inside Dockerfile.test with a different Chrome + ffmpeg build, producing pixel-level drift that failed PSNR at 54/100 checkpoints (~20 dB vs 41 dB in the VFR sparse-content windows). Both renders are valid — the VFR source has inherent sampling ambiguity in static segments, and different Chrome/ffmpeg builds make different valid choices. Regenerated the baseline via `bun run docker:test:update vfr-screen-recording` so it matches the Docker environment CI actually uses. Matches the flow the existing sub-composition-video, hdr-pq, etc. baselines were captured with. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: document that producer test baselines must be captured in Docker Hit this 2026-04-21 with the vfr-screen-recording regression test: host-generated output.mp4 baseline tripped 54/100 PSNR checkpoints in CI because Chrome + ffmpeg drift between the host and Dockerfile.test. Document the `bun run --cwd packages/producer docker:test:update <name>` flow so future contributors don't repeat the mistake. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-09-05 00:56:23 +00:00 · 2026-04-21 12:14:23 -07:00
parent b98093aa1c
commit ffc06827c4
10 changed files with 461 additions and 5 deletions
@@ -1,5 +1,25 @@
-import { describe, expect, it } from "vitest";
-import { parseVideoElements, parseImageElements } from "./videoFrameExtractor.js";
+import { afterAll, beforeAll, describe, expect, it } from "vitest";
+import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync } from "node:fs";
+import { createHash } from "node:crypto";
+import { join } from "node:path";
+import { tmpdir } from "node:os";
+import { spawnSync } from "node:child_process";
+import {
+  parseVideoElements,
+  parseImageElements,
+  extractAllVideoFrames,
+  type VideoElement,
+} from "./videoFrameExtractor.js";
+import { extractVideoMetadata } from "../utils/ffprobe.js";
+import { runFfmpeg } from "../utils/runFfmpeg.js";
+
+// ffmpeg is not preinstalled on GitHub's ubuntu-24.04 runners. The producer
+// regression test at packages/producer/tests/vfr-screen-recording/ runs inside
+// Dockerfile.test (which does include ffmpeg) and is the primary CI signal
+// for this bug. Locally and in any CI job with ffmpeg on PATH, the tests
+// below run too — they exercise the extractor in isolation against a
+// synthesized VFR fixture.
+const HAS_FFMPEG = spawnSync("ffmpeg", ["-version"]).status === 0;

 describe("parseVideoElements", () => {
  it("parses videos without an id or data-start attribute", () => {
@@ -81,3 +101,139 @@ describe("parseImageElements", () => {
    expect(images[0]!.end).toBe(5);
  });
 });
+
+// Regression test for the VFR (variable frame rate) freeze bug.
+// Screen recordings and phone videos often have irregular timestamps.
+// When such inputs hit `extractVideoFramesRange`'s `-ss <start> -i ... -t <dur>
+// -vf fps=N` pipeline, the fps filter can emit fewer frames than requested —
+// e.g. a 4-second segment at 30fps would produce ~90 frames instead of 120.
+// FrameLookupTable.getFrameAtTime then returns null for out-of-range indices
+// and the compositor holds the last valid frame, which the user perceives as
+// the video freezing. extractAllVideoFrames normalizes VFR sources to CFR
+// before extraction to fix this.
+describe.skipIf(!HAS_FFMPEG)("extractAllVideoFrames on a VFR source", () => {
+  const FIXTURE_DIR = mkdtempSync(join(tmpdir(), "hf-vfr-test-"));
+  const VFR_FIXTURE = join(FIXTURE_DIR, "vfr_screen.mp4");
+
+  beforeAll(async () => {
+    // 10s testsrc2 at 60fps, ~40% of frames dropped via select filter and
+    // encoded with -vsync vfr so timestamps are irregular. Declared fps 60,
+    // actual average ~36 — well over the 10% threshold used by isVFR.
+    // The select expression drops four 1-second windows (frames 30-89,
+    // 180-239, 330-389, 480-539) to simulate static segments in a screen
+    // recording where no pixels changed.
+    // -g/-keyint_min 600 forces a single keyframe so mid-segment seeks in the
+    // mediaStart=3 test don't snap to an intermediate IDR and drift the count.
+    const result = await runFfmpeg([
+      "-y",
+      "-hide_banner",
+      "-loglevel",
+      "error",
+      "-f",
+      "lavfi",
+      "-i",
+      "testsrc2=s=320x180:d=10:rate=60",
+      "-vf",
+      "drawtext=text='n=%{n}':fontsize=24:fontcolor=white:x=10:y=10:box=1:boxcolor=black@0.6," +
+        "select='not(between(n,30,89))*not(between(n,180,239))*not(between(n,330,389))*not(between(n,480,539))'",
+      "-vsync",
+      "vfr",
+      "-c:v",
+      "libx264",
+      "-preset",
+      "ultrafast",
+      "-pix_fmt",
+      "yuv420p",
+      "-g",
+      "600",
+      "-keyint_min",
+      "600",
+      VFR_FIXTURE,
+    ]);
+    if (!result.success) {
+      throw new Error(
+        `ffmpeg fixture synthesis failed (${result.exitCode}): ${result.stderr.slice(-400)}`,
+      );
+    }
+  }, 30_000);
+
+  afterAll(() => {
+    if (existsSync(FIXTURE_DIR)) rmSync(FIXTURE_DIR, { recursive: true, force: true });
+  });
+
+  it("detects the synthesized fixture as VFR", async () => {
+    const md = await extractVideoMetadata(VFR_FIXTURE);
+    expect(md.isVFR).toBe(true);
+  });
+
+  it("produces the expected frame count for a mid-file segment", async () => {
+    const outputDir = join(FIXTURE_DIR, "out-mid-segment");
+    mkdirSync(outputDir, { recursive: true });
+
+    const video: VideoElement = {
+      id: "v1",
+      src: VFR_FIXTURE,
+      start: 0,
+      end: 4,
+      mediaStart: 3,
+      hasAudio: false,
+    };
+
+    const result = await extractAllVideoFrames([video], FIXTURE_DIR, {
+      fps: 30,
+      outputDir,
+    });
+
+    expect(result.errors).toEqual([]);
+    expect(result.extracted).toHaveLength(1);
+    const frames = readdirSync(join(outputDir, "v1")).filter((f) => f.endsWith(".jpg"));
+    // Pre-fix behavior produced ~90 frames (a 25% shortfall).
+    expect(frames.length).toBeGreaterThanOrEqual(119);
+    expect(frames.length).toBeLessThanOrEqual(121);
+  }, 60_000);
+
+  // Asserts both frame-count correctness and that we don't emit long runs of
+  // byte-identical "duplicate" frames — the user-visible "frozen screen
+  // recording" symptom. Pre-fix duplicate rate on this fixture is ~38%
+  // (116/300); on the actual reporter's ScreenCaptureKit clip, 18–44% across
+  // segments. <10% threshold leaves margin across ffmpeg versions without
+  // letting a regression slip through.
+  it("produces the full frame count and no duplicate-frame runs on the full VFR file", async () => {
+    const outputDir = join(FIXTURE_DIR, "out-full");
+    mkdirSync(outputDir, { recursive: true });
+
+    const video: VideoElement = {
+      id: "vfull",
+      src: VFR_FIXTURE,
+      start: 0,
+      end: 10,
+      mediaStart: 0,
+      hasAudio: false,
+    };
+
+    const result = await extractAllVideoFrames([video], FIXTURE_DIR, {
+      fps: 30,
+      outputDir,
+    });
+    expect(result.errors).toEqual([]);
+
+    const frameDir = join(outputDir, "vfull");
+    const frames = readdirSync(frameDir)
+      .filter((f) => f.endsWith(".jpg"))
+      .sort();
+    expect(frames.length).toBeGreaterThanOrEqual(299);
+    expect(frames.length).toBeLessThanOrEqual(301);
+
+    let prevHash: string | null = null;
+    let duplicates = 0;
+    for (const f of frames) {
+      const hash = createHash("sha256")
+        .update(readFileSync(join(frameDir, f)))
+        .digest("hex");
+      if (hash === prevHash) duplicates += 1;
+      prevHash = hash;
+    }
+    const duplicateRate = duplicates / frames.length;
+    expect(duplicateRate).toBeLessThan(0.1);
+  }, 60_000);
+});