From f5bc83b82e76d6b1971d45ad58fcb1a2ef70bcc2 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 23 Apr 2026 16:14:42 +0000 Subject: [PATCH 1/4] perf(engine): content-addressed extraction cache for video frames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keys extracted frame bundles on (path, mtime, size, mediaStart, duration, fps, format) so iteration workflows (studio edit → re-render) skip the ffmpeg call when none of those inputs have changed. - New extractionCache service: SHA-256 key, `.hf-complete` sentinel, `hfcache-v2-` schema prefix, FRAME_FILENAME_PREFIX shared with the extractor. Schema bump invalidates older entries (callers gc them). - `EngineConfig.extractCacheDir` (env: HYPERFRAMES_EXTRACT_CACHE_DIR) gates the feature — undefined disables caching, preserving the prior per-workDir behaviour exactly. - Phase 3 per-video flow: compute key on the pre-preflight video (so keys are stable across renders that use workDir-local normalized files), look up; on hit rebuild ExtractedFrames from the cache dir plus Phase 2 metadata — no re-ffprobe. On miss, extract into the keyed dir then mark complete. - `ExtractedFrames.ownedByLookup` prevents `FrameLookupTable.cleanup` from deleting shared cache entries when a render finishes. - Extractor output-dir override lets cache-miss writes land directly in the keyed dir instead of the transient workDir. Validation: /tmp/hf-fixtures/cfr-sdr-cache Cold run: extractMs=69, videoExtractMs=70, totalElapsedMs=2052 Warm run: extractMs=1, videoExtractMs=2, totalElapsedMs=1964 cacheHits 0→1, cacheMisses 1→0. Adds 19 unit tests for key/sentinel/format semantics and 2 integration tests (cache hit, fps change invalidates key) in videoFrameExtractor.test.ts. --- packages/engine/src/config.ts | 14 ++ packages/engine/src/index.ts | 14 ++ .../src/services/extractionCache.test.ts | 189 ++++++++++++++++++ .../engine/src/services/extractionCache.ts | 157 +++++++++++++++ .../src/services/videoFrameExtractor.test.ts | 131 ++++++++++++ .../src/services/videoFrameExtractor.ts | 115 ++++++++++- .../src/services/renderOrchestrator.ts | 2 +- 7 files changed, 614 insertions(+), 8 deletions(-) create mode 100644 packages/engine/src/services/extractionCache.test.ts create mode 100644 packages/engine/src/services/extractionCache.ts diff --git a/packages/engine/src/config.ts b/packages/engine/src/config.ts index ea8055c4..b81764d6 100644 --- a/packages/engine/src/config.ts +++ b/packages/engine/src/config.ts @@ -72,6 +72,18 @@ export interface EngineConfig { /** Custom manifest path for Hyperframe runtime. */ runtimeManifestPath?: string; + // ── Cache ──────────────────────────────────────────────────────────── + /** + * Directory where the content-addressed extraction cache persists frame + * bundles keyed on (path, mtime, size, mediaStart, duration, fps, format). + * Undefined disables caching — extraction runs into the render's workDir + * and cleanup removes it when the render ends, preserving the pre-cache + * behaviour. + * + * Env fallback: `HYPERFRAMES_EXTRACT_CACHE_DIR`. + */ + extractCacheDir?: string; + // ── Debug ──────────────────────────────────────────────────────────── debug: boolean; } @@ -203,6 +215,8 @@ export function resolveConfig(overrides?: Partial): EngineConfig { verifyRuntime: env("PRODUCER_VERIFY_HYPERFRAME_RUNTIME") !== "false", runtimeManifestPath: env("PRODUCER_HYPERFRAME_MANIFEST_PATH"), + + extractCacheDir: env("HYPERFRAMES_EXTRACT_CACHE_DIR"), }; // Remove undefined values so they don't override defaults diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index 0bfc00bc..8eaa984b 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -125,6 +125,20 @@ export { type ExtractionPhaseBreakdown, } from "./services/videoFrameExtractor.js"; +export { + FRAME_FILENAME_PREFIX, + COMPLETE_SENTINEL, + SCHEMA_PREFIX, + cacheEntryDirName, + computeCacheKey, + ensureCacheEntryDir, + lookupCacheEntry, + markCacheEntryComplete, + type CacheEntry, + type CacheKeyInput, + type CacheLookup, +} from "./services/extractionCache.js"; + export { createVideoFrameInjector } from "./services/videoFrameInjector.js"; export { parseAudioElements, processCompositionAudio } from "./services/audioMixer.js"; diff --git a/packages/engine/src/services/extractionCache.test.ts b/packages/engine/src/services/extractionCache.test.ts new file mode 100644 index 00000000..2b30cbaa --- /dev/null +++ b/packages/engine/src/services/extractionCache.test.ts @@ -0,0 +1,189 @@ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; + +import { + COMPLETE_SENTINEL, + FRAME_FILENAME_PREFIX, + SCHEMA_PREFIX, + cacheEntryDirName, + computeCacheKey, + ensureCacheEntryDir, + lookupCacheEntry, + markCacheEntryComplete, + type CacheKeyInput, +} from "./extractionCache.js"; + +describe("extractionCache constants", () => { + it("exposes the v2 schema prefix", () => { + expect(SCHEMA_PREFIX).toBe("hfcache-v2-"); + }); + + it("exposes the frame filename prefix shared with the extractor", () => { + expect(FRAME_FILENAME_PREFIX).toBe("frame_"); + }); + + it("uses a dotfile sentinel so ls-without-A hides it", () => { + expect(COMPLETE_SENTINEL.startsWith(".")).toBe(true); + }); +}); + +describe("computeCacheKey", () => { + let tmpRoot: string; + let sourceFile: string; + + beforeEach(() => { + tmpRoot = mkdtempSync(join(tmpdir(), "hf-extract-cache-test-")); + sourceFile = join(tmpRoot, "clip.mp4"); + writeFileSync(sourceFile, "fake-video-bytes", "utf-8"); + }); + + afterEach(() => { + if (existsSync(tmpRoot)) rmSync(tmpRoot, { recursive: true, force: true }); + }); + + const base = (videoPath: string): CacheKeyInput => ({ + videoPath, + mediaStart: 0, + duration: 3, + fps: 30, + format: "jpg", + }); + + it("returns the same key for identical inputs", () => { + const a = computeCacheKey(base(sourceFile)); + const b = computeCacheKey(base(sourceFile)); + expect(a).toBe(b); + }); + + it("produces a 64-char hex SHA-256 digest", () => { + const key = computeCacheKey(base(sourceFile)); + expect(key).toMatch(/^[0-9a-f]{64}$/); + }); + + it("changes when path changes (moved files re-extract)", () => { + const other = join(tmpRoot, "other.mp4"); + writeFileSync(other, "fake-video-bytes", "utf-8"); + const a = computeCacheKey(base(sourceFile)); + const b = computeCacheKey(base(other)); + expect(a).not.toBe(b); + }); + + it("changes when mediaStart changes", () => { + const a = computeCacheKey(base(sourceFile)); + const b = computeCacheKey({ ...base(sourceFile), mediaStart: 1 }); + expect(a).not.toBe(b); + }); + + it("changes when duration changes", () => { + const a = computeCacheKey(base(sourceFile)); + const b = computeCacheKey({ ...base(sourceFile), duration: 5 }); + expect(a).not.toBe(b); + }); + + it("changes when fps changes (different frame count invalidates key)", () => { + const a = computeCacheKey(base(sourceFile)); + const b = computeCacheKey({ ...base(sourceFile), fps: 60 }); + expect(a).not.toBe(b); + }); + + it("changes when format changes", () => { + const a = computeCacheKey(base(sourceFile)); + const b = computeCacheKey({ ...base(sourceFile), format: "png" }); + expect(a).not.toBe(b); + }); + + it("normalizes non-finite duration so Infinity doesn't produce unstable keys", () => { + const a = computeCacheKey({ ...base(sourceFile), duration: Infinity }); + const b = computeCacheKey({ ...base(sourceFile), duration: Infinity }); + expect(a).toBe(b); + }); + + it("changes when file content changes (mtime+size bump)", () => { + const before = computeCacheKey(base(sourceFile)); + // Force an mtime change by waiting 5ms then overwriting with different bytes. + // 5ms is well above the Linux mtime resolution (typically nanoseconds) and + // below any Windows cache coherency window. Using a longer sleep pads against + // coarse filesystem mtime granularity without slowing the suite. + const start = Date.now(); + while (Date.now() - start < 5) { + /* spin */ + } + writeFileSync(sourceFile, "different-bytes-longer-than-before", "utf-8"); + const after = computeCacheKey(base(sourceFile)); + expect(after).not.toBe(before); + }); + + it("tolerates a missing source file (extractor surfaces the real error later)", () => { + expect(() => computeCacheKey(base(join(tmpRoot, "does-not-exist.mp4")))).not.toThrow(); + }); +}); + +describe("cacheEntryDirName", () => { + it("prefixes with the schema and truncates to 16 hex chars", () => { + const full = "a".repeat(64); + expect(cacheEntryDirName(full)).toBe(`${SCHEMA_PREFIX}${"a".repeat(16)}`); + }); +}); + +describe("lookupCacheEntry / markCacheEntryComplete", () => { + let tmpRoot: string; + let sourceFile: string; + + beforeEach(() => { + tmpRoot = mkdtempSync(join(tmpdir(), "hf-extract-cache-test-")); + sourceFile = join(tmpRoot, "clip.mp4"); + writeFileSync(sourceFile, "fake-video-bytes", "utf-8"); + }); + + afterEach(() => { + if (existsSync(tmpRoot)) rmSync(tmpRoot, { recursive: true, force: true }); + }); + + const base = (videoPath: string): CacheKeyInput => ({ + videoPath, + mediaStart: 0, + duration: 3, + fps: 30, + format: "jpg", + }); + + it("misses on an empty cache root", () => { + const lookup = lookupCacheEntry(tmpRoot, base(sourceFile)); + expect(lookup.hit).toBe(false); + expect(lookup.entry.dir.startsWith(tmpRoot)).toBe(true); + }); + + it("hits after ensureCacheEntryDir + markCacheEntryComplete", () => { + const first = lookupCacheEntry(tmpRoot, base(sourceFile)); + ensureCacheEntryDir(first.entry); + markCacheEntryComplete(first.entry); + + const second = lookupCacheEntry(tmpRoot, base(sourceFile)); + expect(second.hit).toBe(true); + expect(second.entry.dir).toBe(first.entry.dir); + }); + + it("treats an in-progress dir without the sentinel as a miss", () => { + const lookup = lookupCacheEntry(tmpRoot, base(sourceFile)); + ensureCacheEntryDir(lookup.entry); + // Simulate abandoned extraction — frames written but sentinel never marked. + writeFileSync(join(lookup.entry.dir, "frame_00001.jpg"), "x", "utf-8"); + const again = lookupCacheEntry(tmpRoot, base(sourceFile)); + expect(again.hit).toBe(false); + }); + + it("places entries under the cache root, not the source parent", () => { + const subroot = join(tmpRoot, "cache-root"); + mkdirSync(subroot, { recursive: true }); + const lookup = lookupCacheEntry(subroot, base(sourceFile)); + expect(lookup.entry.dir.startsWith(subroot)).toBe(true); + }); + + it("uses the same directory for identical inputs across lookups", () => { + const a = lookupCacheEntry(tmpRoot, base(sourceFile)); + const b = lookupCacheEntry(tmpRoot, base(sourceFile)); + expect(a.entry.dir).toBe(b.entry.dir); + }); +}); diff --git a/packages/engine/src/services/extractionCache.ts b/packages/engine/src/services/extractionCache.ts new file mode 100644 index 00000000..c8375775 --- /dev/null +++ b/packages/engine/src/services/extractionCache.ts @@ -0,0 +1,157 @@ +/** + * Content-Addressed Extraction Cache + * + * Video frame extraction is the single most expensive phase of a render + * after capture. Repeat renders of the same composition (preview → final, + * studio iteration) re-extract identical frames from the same source file, + * burning ffmpeg time that adds no value. This module keys extracted frame + * bundles on the (path, mtime, size, mediaStart, duration, fps, format) + * tuple so re-renders resolve to a pre-extracted directory instead of + * re-invoking ffmpeg. + * + * ### Scheme + * + * - The key is the SHA-256 of a stable JSON encoding of the tuple above. + * - Cache entries live under `//` so + * `ls` output and tracing logs stay short. Truncation to 16 hex chars + * leaves 64 bits of entropy — collision risk at cache scale is negligible. + * - A completed entry is marked by writing the `.hf-complete` sentinel file + * after all frames are on disk. A dir without the sentinel is treated as + * absent (stale/abandoned) and re-extracted into a fresh key (the old dir + * is left for external gc — the cache owns keys, not deletion policy). + * + * ### Versioning + * + * `SCHEMA_PREFIX` bumps when the cache-contents invariant changes (e.g. + * extraction format, frame layout). Old entries under the previous prefix + * become inert and can be gc'd by the caller. + */ + +import { createHash } from "node:crypto"; +import { existsSync, mkdirSync, statSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; + +/** Filename prefix for extracted frames. Shared with the extractor. */ +export const FRAME_FILENAME_PREFIX = "frame_"; + +/** Sentinel filename written after a cache entry is fully populated. */ +export const COMPLETE_SENTINEL = ".hf-complete"; + +/** Current schema version. Bump when cache-entry layout changes. */ +export const SCHEMA_PREFIX = "hfcache-v2-"; + +/** Truncated hex chars of SHA-256 used for the entry directory name. */ +const KEY_HEX_CHARS = 16; + +export interface CacheKeyInput { + /** Absolute path to the source video file. Path is part of the key so + * moved files re-extract rather than match by (size, mtime) alone. */ + videoPath: string; + /** Seconds into source the composition starts reading (video.mediaStart). */ + mediaStart: number; + /** Seconds of source the composition uses. Infinity is normalized to -1 + * so callers that pass an unresolved "natural duration" still produce a + * stable key across invocations. */ + duration: number; + /** Target output frames-per-second. */ + fps: number; + /** Output image format ("jpg" | "png"). */ + format: string; +} + +export interface CacheEntry { + /** Absolute path to the cache entry directory. */ + dir: string; + /** Full 64-char SHA-256 hex digest (parent of the truncated key). */ + keyHash: string; +} + +export interface CacheLookup { + /** Cache entry information — always returned even on a miss so the caller + * can extract directly into `dir` then call `markCacheEntryComplete`. */ + entry: CacheEntry; + /** True when the entry exists AND carries the completion sentinel. */ + hit: boolean; +} + +/** + * Canonical JSON serialization for the key input. Keys are listed + * explicitly to prevent property-order variance across Node versions from + * producing different hashes for the same logical input. mtime/size are + * captured from the source file at key-compute time so a file edit + * invalidates the key even when path/mediaStart/duration/fps don't change. + */ +function canonicalKeyBlob(input: CacheKeyInput): string { + let mtimeMs = 0; + let size = 0; + try { + const stat = statSync(input.videoPath); + mtimeMs = Math.floor(stat.mtimeMs); + size = stat.size; + } catch { + // If the file disappears between Phase 1 resolution and key compute, the + // missing (mtime, size) still gives a stable key — just one that won't + // match any real entry, so we'll miss and the extractor will fail with + // its normal file-not-found path. Don't throw here; the extractor is the + // right place to surface that error. + } + + const durationForKey = Number.isFinite(input.duration) ? input.duration : -1; + + // Properties are written in a fixed order. + return JSON.stringify({ + p: input.videoPath, + m: mtimeMs, + s: size, + ms: input.mediaStart, + d: durationForKey, + f: input.fps, + fmt: input.format, + }); +} + +/** + * Compute the SHA-256 hex digest for a cache key input. + */ +export function computeCacheKey(input: CacheKeyInput): string { + return createHash("sha256").update(canonicalKeyBlob(input)).digest("hex"); +} + +/** + * Derive the truncated cache-entry directory name from a full key hash. + * Exposed so tests and the entry dir resolver share one truncation rule. + */ +export function cacheEntryDirName(keyHash: string): string { + return SCHEMA_PREFIX + keyHash.slice(0, KEY_HEX_CHARS); +} + +/** + * Look up a cache entry by key input. Returns the resolved entry path plus a + * `hit` flag. On miss, callers should extract frames into `entry.dir` + * (after calling `ensureCacheEntryDir`) and then call `markCacheEntryComplete` + * once the extraction succeeds. + */ +export function lookupCacheEntry(rootDir: string, input: CacheKeyInput): CacheLookup { + const keyHash = computeCacheKey(input); + const dir = join(rootDir, cacheEntryDirName(keyHash)); + const complete = existsSync(join(dir, COMPLETE_SENTINEL)); + return { entry: { dir, keyHash }, hit: complete }; +} + +/** + * Ensure a cache entry's directory exists so the extractor can write into it. + * Idempotent — creates missing parents recursively. + */ +export function ensureCacheEntryDir(entry: CacheEntry): void { + if (!existsSync(entry.dir)) mkdirSync(entry.dir, { recursive: true }); +} + +/** + * Write the completion sentinel so subsequent lookups treat this entry as a + * hit. Must be called only after every frame has been written and the + * directory is considered durable. + */ +export function markCacheEntryComplete(entry: CacheEntry): void { + const sentinelPath = join(entry.dir, COMPLETE_SENTINEL); + writeFileSync(sentinelPath, "", "utf-8"); +} diff --git a/packages/engine/src/services/videoFrameExtractor.test.ts b/packages/engine/src/services/videoFrameExtractor.test.ts index c63e0e93..65436f3a 100644 --- a/packages/engine/src/services/videoFrameExtractor.test.ts +++ b/packages/engine/src/services/videoFrameExtractor.test.ts @@ -196,6 +196,137 @@ describe.skipIf(!HAS_FFMPEG)("extractAllVideoFrames on a VFR source", () => { expect(result.phaseBreakdown.vfrPreflightMs).toBeGreaterThan(0); }, 60_000); + it("reuses extracted frames on a warm cache hit", async () => { + const CACHE_DIR = mkdtempSync(join(tmpdir(), "hf-extract-cache-test-")); + const SRC = join(FIXTURE_DIR, "cache-src.mp4"); + + // Synthesize a clean CFR SDR clip — bypasses VFR preflight so the cache + // key is stable across the two runs. + const synth = await runFfmpeg([ + "-y", + "-hide_banner", + "-loglevel", + "error", + "-f", + "lavfi", + "-i", + "testsrc2=s=320x180:d=2:rate=30", + "-c:v", + "libx264", + "-preset", + "ultrafast", + "-pix_fmt", + "yuv420p", + SRC, + ]); + if (!synth.success) { + throw new Error(`Cache fixture synthesis failed: ${synth.stderr.slice(-400)}`); + } + + const video: VideoElement = { + id: "cv1", + src: SRC, + start: 0, + end: 2, + mediaStart: 0, + hasAudio: false, + }; + + const outDirA = join(FIXTURE_DIR, "out-cache-miss"); + mkdirSync(outDirA, { recursive: true }); + const miss = await extractAllVideoFrames( + [video], + FIXTURE_DIR, + { fps: 30, outputDir: outDirA }, + undefined, + { extractCacheDir: CACHE_DIR }, + ); + expect(miss.errors).toEqual([]); + expect(miss.phaseBreakdown.cacheHits).toBe(0); + expect(miss.phaseBreakdown.cacheMisses).toBe(1); + + const outDirB = join(FIXTURE_DIR, "out-cache-hit"); + mkdirSync(outDirB, { recursive: true }); + const hit = await extractAllVideoFrames( + [video], + FIXTURE_DIR, + { fps: 30, outputDir: outDirB }, + undefined, + { extractCacheDir: CACHE_DIR }, + ); + expect(hit.errors).toEqual([]); + expect(hit.phaseBreakdown.cacheHits).toBe(1); + expect(hit.phaseBreakdown.cacheMisses).toBe(0); + // extractMs on a hit is only the cache-lookup bookkeeping; asserting <50ms + // is loose enough to survive CI jitter but tight enough to catch a + // regression that accidentally triggered ffmpeg again. + expect(hit.phaseBreakdown.extractMs).toBeLessThan(50); + expect(hit.extracted).toHaveLength(1); + expect(hit.extracted[0]!.totalFrames).toBe(miss.extracted[0]!.totalFrames); + + rmSync(CACHE_DIR, { recursive: true, force: true }); + }, 60_000); + + it("invalidates the cache when fps changes", async () => { + const CACHE_DIR = mkdtempSync(join(tmpdir(), "hf-extract-cache-test-")); + const SRC = join(FIXTURE_DIR, "cache-fps-src.mp4"); + + const synth = await runFfmpeg([ + "-y", + "-hide_banner", + "-loglevel", + "error", + "-f", + "lavfi", + "-i", + "testsrc2=s=320x180:d=1:rate=30", + "-c:v", + "libx264", + "-preset", + "ultrafast", + "-pix_fmt", + "yuv420p", + SRC, + ]); + if (!synth.success) { + throw new Error(`Cache-fps fixture synthesis failed: ${synth.stderr.slice(-400)}`); + } + + const video: VideoElement = { + id: "cv2", + src: SRC, + start: 0, + end: 1, + mediaStart: 0, + hasAudio: false, + }; + + const outA = join(FIXTURE_DIR, "out-cache-fps-30"); + mkdirSync(outA, { recursive: true }); + const first = await extractAllVideoFrames( + [video], + FIXTURE_DIR, + { fps: 30, outputDir: outA }, + undefined, + { extractCacheDir: CACHE_DIR }, + ); + expect(first.phaseBreakdown.cacheMisses).toBe(1); + + const outB = join(FIXTURE_DIR, "out-cache-fps-60"); + mkdirSync(outB, { recursive: true }); + const second = await extractAllVideoFrames( + [video], + FIXTURE_DIR, + { fps: 60, outputDir: outB }, + undefined, + { extractCacheDir: CACHE_DIR }, + ); + expect(second.phaseBreakdown.cacheMisses).toBe(1); + expect(second.phaseBreakdown.cacheHits).toBe(0); + + rmSync(CACHE_DIR, { recursive: true, force: true }); + }, 60_000); + // Regression test for the segment-scope HDR preflight fix: pre-fix, // convertSdrToHdr re-encoded the entire source, so a 30-minute SDR source // contributing a 2-second clip took ~200× longer than needed. Post-fix the diff --git a/packages/engine/src/services/videoFrameExtractor.ts b/packages/engine/src/services/videoFrameExtractor.ts index c4ff8253..2774c422 100644 --- a/packages/engine/src/services/videoFrameExtractor.ts +++ b/packages/engine/src/services/videoFrameExtractor.ts @@ -18,6 +18,12 @@ import { import { downloadToTemp, isHttpUrl } from "../utils/urlDownloader.js"; import { runFfmpeg } from "../utils/runFfmpeg.js"; import { DEFAULT_CONFIG, type EngineConfig } from "../config.js"; +import { + FRAME_FILENAME_PREFIX, + ensureCacheEntryDir, + lookupCacheEntry, + markCacheEntryComplete, +} from "./extractionCache.js"; export interface VideoElement { id: string; @@ -37,6 +43,13 @@ export interface ExtractedFrames { totalFrames: number; metadata: VideoMetadata; framePaths: Map; + /** + * True when the extractor owns `outputDir` and cleanup should rm it when + * the render ends. Cache hits set this to false so the shared entry isn't + * deleted by a single render's cleanup — the cache dir is owned by the + * caller's gc policy, not any one render. + */ + ownedByLookup?: boolean; } export interface ExtractionOptions { @@ -181,15 +194,22 @@ export async function extractVideoFramesRange( options: ExtractionOptions, signal?: AbortSignal, config?: Partial>, + /** + * Override the output directory for this extraction. When provided, frames + * are written directly into `outputDirOverride` (no per-videoId subdir). + * Used by the cache layer to materialize frames straight into the keyed + * cache entry directory. + */ + outputDirOverride?: string, ): Promise { const ffmpegProcessTimeout = config?.ffmpegProcessTimeout ?? DEFAULT_CONFIG.ffmpegProcessTimeout; const { fps, outputDir, quality = 95, format = "jpg" } = options; - const videoOutputDir = join(outputDir, videoId); + const videoOutputDir = outputDirOverride ?? join(outputDir, videoId); if (!existsSync(videoOutputDir)) mkdirSync(videoOutputDir, { recursive: true }); const metadata = await extractMediaMetadata(videoPath); - const framePattern = `frame_%05d.${format}`; + const framePattern = `${FRAME_FILENAME_PREFIX}%05d.${format}`; const outputPattern = join(videoOutputDir, framePattern); // When extracting from HDR source, tone-map to SDR in FFmpeg rather than @@ -253,7 +273,7 @@ export async function extractVideoFramesRange( const framePaths = new Map(); const files = readdirSync(videoOutputDir) - .filter((f) => f.startsWith("frame_") && f.endsWith(`.${format}`)) + .filter((f) => f.startsWith(FRAME_FILENAME_PREFIX) && f.endsWith(`.${format}`)) .sort(); files.forEach((file, index) => { framePaths.set(index, join(videoOutputDir, file)); @@ -414,7 +434,7 @@ export async function extractAllVideoFrames( baseDir: string, options: ExtractionOptions, signal?: AbortSignal, - config?: Partial>, + config?: Partial>, compiledDir?: string, ): Promise { const startTime = Date.now(); @@ -469,6 +489,18 @@ export async function extractAllVideoFrames( breakdown.resolveMs = Date.now() - phase1Start; + // Snapshot the pre-preflight key inputs so the extraction cache keys on the + // user-visible source (original path, original mediaStart, original segment + // bounds) rather than the workDir-local normalized file produced by + // Phase 2a/2b preflight. Without this, every render would write a new + // normalized file with a fresh mtime → fresh cache key → perpetual misses. + const cacheKeyInputs = resolvedVideos.map(({ video, videoPath }) => ({ + videoPath, + mediaStart: video.mediaStart, + start: video.start, + end: video.end, + })); + // Phase 2: Probe color spaces and normalize if mixed HDR/SDR const phase2ProbeStart = Date.now(); const videoMetadata = await Promise.all( @@ -615,10 +647,14 @@ export async function extractAllVideoFrames( } breakdown.vfrPreflightMs = Date.now() - vfrPreflightStart; - // Phase 3: Extract frames (parallel) + // Phase 3: Extract frames (parallel). Optionally consults the + // content-addressed extraction cache to skip the ffmpeg call on + // repeat renders of the same composition. const phase3Start = Date.now(); + const cacheRootDir = config?.extractCacheDir; + const cacheFormat = options.format ?? "jpg"; const results = await Promise.all( - resolvedVideos.map(async ({ video, videoPath }) => { + resolvedVideos.map(async ({ video, videoPath }, i) => { if (signal?.aborted) { throw new Error("Video frame extraction cancelled"); } @@ -628,12 +664,73 @@ export async function extractAllVideoFrames( // Fallback: if no data-duration/data-end was specified (end is Infinity or 0), // probe the actual video file to get its natural duration. if (!Number.isFinite(videoDuration) || videoDuration <= 0) { - const metadata = await extractMediaMetadata(videoPath); + const metadata = videoMetadata[i] ?? (await extractMediaMetadata(videoPath)); const sourceDuration = metadata.durationSeconds - video.mediaStart; videoDuration = sourceDuration > 0 ? sourceDuration : metadata.durationSeconds; video.end = video.start + videoDuration; } + if (cacheRootDir) { + const keyInput = cacheKeyInputs[i]; + const probedMeta = videoMetadata[i]; + if (keyInput && probedMeta) { + let keyDuration = keyInput.end - keyInput.start; + if (!Number.isFinite(keyDuration) || keyDuration <= 0) { + const sourceRemaining = probedMeta.durationSeconds - keyInput.mediaStart; + keyDuration = sourceRemaining > 0 ? sourceRemaining : probedMeta.durationSeconds; + } + const lookup = lookupCacheEntry(cacheRootDir, { + videoPath: keyInput.videoPath, + mediaStart: keyInput.mediaStart, + duration: keyDuration, + fps: options.fps, + format: cacheFormat, + }); + if (lookup.hit) { + const framePattern = `${FRAME_FILENAME_PREFIX}%05d.${cacheFormat}`; + const framePaths = new Map(); + const files = readdirSync(lookup.entry.dir) + .filter((f) => f.startsWith(FRAME_FILENAME_PREFIX) && f.endsWith(`.${cacheFormat}`)) + .sort(); + files.forEach((file, idx) => { + framePaths.set(idx, join(lookup.entry.dir, file)); + }); + breakdown.cacheHits += 1; + return { + result: { + videoId: video.id, + srcPath: keyInput.videoPath, + outputDir: lookup.entry.dir, + framePattern, + fps: options.fps, + totalFrames: framePaths.size, + metadata: probedMeta, + framePaths, + ownedByLookup: true, + } satisfies ExtractedFrames, + }; + } + breakdown.cacheMisses += 1; + ensureCacheEntryDir(lookup.entry); + const result = await extractVideoFramesRange( + videoPath, + video.id, + video.mediaStart, + videoDuration, + options, + signal, + config, + lookup.entry.dir, + ); + // ownedByLookup=true so FrameLookupTable.cleanup leaves the entry + // behind for the next render. Marking complete is the last step so + // a crash mid-extract leaves the entry un-sentineled (treated as a + // miss by the next lookup and re-extracted over the partial frames). + markCacheEntryComplete(lookup.entry); + return { result: { ...result, ownedByLookup: true } }; + } + } + const result = await extractVideoFramesRange( videoPath, video.id, @@ -800,6 +897,10 @@ export class FrameLookupTable { cleanup(): void { for (const video of this.videos.values()) { + // Cache-hit / cache-write entries are owned by the extraction cache — + // a single render must not delete them, or the next render's lookup + // would miss and re-extract unnecessarily. + if (video.extracted.ownedByLookup) continue; if (existsSync(video.extracted.outputDir)) { rmSync(video.extracted.outputDir, { recursive: true, force: true }); } diff --git a/packages/producer/src/services/renderOrchestrator.ts b/packages/producer/src/services/renderOrchestrator.ts index 115ae13c..64cd5b02 100644 --- a/packages/producer/src/services/renderOrchestrator.ts +++ b/packages/producer/src/services/renderOrchestrator.ts @@ -1427,7 +1427,7 @@ export async function executeRenderJob( projectDir, { fps: job.config.fps, outputDir: join(workDir, "video-frames") }, abortSignal, - undefined, + { extractCacheDir: cfg.extractCacheDir }, compiledDir, ); assertNotAborted(); From 7c73e0c1f8a10c84a64bdc553cfc5ff7020147a8 Mon Sep 17 00:00:00 2001 From: James Date: Thu, 23 Apr 2026 18:31:52 +0000 Subject: [PATCH 2/4] refactor(engine): simplify extraction cache integration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on PR #446: - Move the readdir/filter/sort rebuild of ExtractedFrames on cache hit into a new `rehydrateCacheEntry` helper inside the cache module — the extractor no longer imports FRAME_FILENAME_PREFIX or assembles frame paths by hand, so cache layout stays owned by the cache. - Extract `resolveSegmentDuration` helper to replace two copies of the "requested duration → fallback to source natural" logic in Phase 3. - Collapse the nested Phase 3 cache logic into a `tryCachedExtract` helper that returns `ExtractedFrames | null`. Reads top-down now. - Move source-file `statSync` out of `canonicalKeyBlob` into a `readKeyStat` helper the extractor calls once per video in Phase 1. Eliminates the redundant statSync on every `computeCacheKey` / `lookupCacheEntry`. `CacheKeyInput` now requires `mtimeMs` / `size` — cleaner contract. - Drop the redundant `existsSync` in `ensureCacheEntryDir`: `mkdirSync` with `recursive:true` is already idempotent. - Tighten `CacheKeyInput.format` from `string` to `CacheFrameFormat` (`"jpg" | "png"`). Catches typos at compile time. - Add an explicit concurrency caveat on `markCacheEntryComplete` — the lookup→populate→mark sequence is non-atomic; document the tradeoff in code rather than only in the commit history. - Stop re-exporting cache internals (SCHEMA_PREFIX, COMPLETE_SENTINEL, FRAME_FILENAME_PREFIX, cacheEntryDirName, computeCacheKey, ensureCacheEntryDir, lookupCacheEntry, markCacheEntryComplete, CacheEntry, CacheKeyInput, CacheLookup) from the engine's public index.ts — they're only used by the colocated extractor and its tests, which import relatively. Behavior unchanged — same cold/warm validation on /tmp/hf-fixtures/cfr-sdr-cache: cold: extractMs=71, cacheMisses=1 warm: extractMs=0, cacheHits=1 (was 1 → 0 post-refactor) All 32 cache+extractor tests pass. --- packages/engine/src/index.ts | 14 -- .../src/services/extractionCache.test.ts | 31 ++-- .../engine/src/services/extractionCache.ts | 119 ++++++++---- .../src/services/videoFrameExtractor.ts | 174 ++++++++++-------- 4 files changed, 201 insertions(+), 137 deletions(-) diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index 8eaa984b..0bfc00bc 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -125,20 +125,6 @@ export { type ExtractionPhaseBreakdown, } from "./services/videoFrameExtractor.js"; -export { - FRAME_FILENAME_PREFIX, - COMPLETE_SENTINEL, - SCHEMA_PREFIX, - cacheEntryDirName, - computeCacheKey, - ensureCacheEntryDir, - lookupCacheEntry, - markCacheEntryComplete, - type CacheEntry, - type CacheKeyInput, - type CacheLookup, -} from "./services/extractionCache.js"; - export { createVideoFrameInjector } from "./services/videoFrameInjector.js"; export { parseAudioElements, processCompositionAudio } from "./services/audioMixer.js"; diff --git a/packages/engine/src/services/extractionCache.test.ts b/packages/engine/src/services/extractionCache.test.ts index 2b30cbaa..037c9ab5 100644 --- a/packages/engine/src/services/extractionCache.test.ts +++ b/packages/engine/src/services/extractionCache.test.ts @@ -12,9 +12,24 @@ import { ensureCacheEntryDir, lookupCacheEntry, markCacheEntryComplete, + readKeyStat, type CacheKeyInput, } from "./extractionCache.js"; +const keyFor = (videoPath: string, overrides: Partial = {}): CacheKeyInput => { + const { mtimeMs, size } = readKeyStat(videoPath); + return { + videoPath, + mtimeMs, + size, + mediaStart: 0, + duration: 3, + fps: 30, + format: "jpg", + ...overrides, + }; +}; + describe("extractionCache constants", () => { it("exposes the v2 schema prefix", () => { expect(SCHEMA_PREFIX).toBe("hfcache-v2-"); @@ -43,13 +58,7 @@ describe("computeCacheKey", () => { if (existsSync(tmpRoot)) rmSync(tmpRoot, { recursive: true, force: true }); }); - const base = (videoPath: string): CacheKeyInput => ({ - videoPath, - mediaStart: 0, - duration: 3, - fps: 30, - format: "jpg", - }); + const base = (videoPath: string): CacheKeyInput => keyFor(videoPath); it("returns the same key for identical inputs", () => { const a = computeCacheKey(base(sourceFile)); @@ -141,13 +150,7 @@ describe("lookupCacheEntry / markCacheEntryComplete", () => { if (existsSync(tmpRoot)) rmSync(tmpRoot, { recursive: true, force: true }); }); - const base = (videoPath: string): CacheKeyInput => ({ - videoPath, - mediaStart: 0, - duration: 3, - fps: 30, - format: "jpg", - }); + const base = (videoPath: string): CacheKeyInput => keyFor(videoPath); it("misses on an empty cache root", () => { const lookup = lookupCacheEntry(tmpRoot, base(sourceFile)); diff --git a/packages/engine/src/services/extractionCache.ts b/packages/engine/src/services/extractionCache.ts index c8375775..e3011b64 100644 --- a/packages/engine/src/services/extractionCache.ts +++ b/packages/engine/src/services/extractionCache.ts @@ -28,8 +28,10 @@ */ import { createHash } from "node:crypto"; -import { existsSync, mkdirSync, statSync, writeFileSync } from "node:fs"; +import { mkdirSync, readdirSync, statSync, writeFileSync } from "node:fs"; +import { existsSync } from "node:fs"; import { join } from "node:path"; +import type { VideoMetadata } from "../utils/ffprobe.js"; /** Filename prefix for extracted frames. Shared with the extractor. */ export const FRAME_FILENAME_PREFIX = "frame_"; @@ -43,10 +45,16 @@ export const SCHEMA_PREFIX = "hfcache-v2-"; /** Truncated hex chars of SHA-256 used for the entry directory name. */ const KEY_HEX_CHARS = 16; +export type CacheFrameFormat = "jpg" | "png"; + export interface CacheKeyInput { - /** Absolute path to the source video file. Path is part of the key so - * moved files re-extract rather than match by (size, mtime) alone. */ + /** Absolute path to the source video file. Part of the key so moved files + * re-extract rather than match by (size, mtime) alone. */ videoPath: string; + /** Source file modification time in ms (floored). Invalidates the key on edit. */ + mtimeMs: number; + /** Source file size in bytes. Invalidates the key on content change. */ + size: number; /** Seconds into source the composition starts reading (video.mediaStart). */ mediaStart: number; /** Seconds of source the composition uses. Infinity is normalized to -1 @@ -55,8 +63,8 @@ export interface CacheKeyInput { duration: number; /** Target output frames-per-second. */ fps: number; - /** Output image format ("jpg" | "png"). */ - format: string; + /** Output image format. */ + format: CacheFrameFormat; } export interface CacheEntry { @@ -75,34 +83,25 @@ export interface CacheLookup { } /** - * Canonical JSON serialization for the key input. Keys are listed - * explicitly to prevent property-order variance across Node versions from - * producing different hashes for the same logical input. mtime/size are - * captured from the source file at key-compute time so a file edit - * invalidates the key even when path/mediaStart/duration/fps don't change. + * Read `(mtimeMs, size)` for a path. Returns zeros if the file is missing — + * callers on the cache hot path can still compute a stable key, and the + * extractor surfaces the real file-not-found error. */ -function canonicalKeyBlob(input: CacheKeyInput): string { - let mtimeMs = 0; - let size = 0; +export function readKeyStat(videoPath: string): { mtimeMs: number; size: number } { try { - const stat = statSync(input.videoPath); - mtimeMs = Math.floor(stat.mtimeMs); - size = stat.size; + const stat = statSync(videoPath); + return { mtimeMs: Math.floor(stat.mtimeMs), size: stat.size }; } catch { - // If the file disappears between Phase 1 resolution and key compute, the - // missing (mtime, size) still gives a stable key — just one that won't - // match any real entry, so we'll miss and the extractor will fail with - // its normal file-not-found path. Don't throw here; the extractor is the - // right place to surface that error. + return { mtimeMs: 0, size: 0 }; } +} +function canonicalKeyBlob(input: CacheKeyInput): string { const durationForKey = Number.isFinite(input.duration) ? input.duration : -1; - - // Properties are written in a fixed order. return JSON.stringify({ p: input.videoPath, - m: mtimeMs, - s: size, + m: input.mtimeMs, + s: input.size, ms: input.mediaStart, d: durationForKey, f: input.fps, @@ -140,18 +139,76 @@ export function lookupCacheEntry(rootDir: string, input: CacheKeyInput): CacheLo /** * Ensure a cache entry's directory exists so the extractor can write into it. - * Idempotent — creates missing parents recursively. + * Idempotent: `mkdirSync({recursive:true})` is a no-op when the dir exists. */ export function ensureCacheEntryDir(entry: CacheEntry): void { - if (!existsSync(entry.dir)) mkdirSync(entry.dir, { recursive: true }); + mkdirSync(entry.dir, { recursive: true }); } /** * Write the completion sentinel so subsequent lookups treat this entry as a - * hit. Must be called only after every frame has been written and the - * directory is considered durable. + * hit. Must be called only after every frame has been written. + * + * Concurrency: lookup→populate→mark is non-atomic. Two concurrent renders of + * the same key may both miss, both extract into the same dir, and the later + * writer's frames win. The result is correct (identical inputs yield identical + * frames) but wasteful. Acceptable for a single-process render pipeline; + * anyone running concurrent renders against a shared cache root should front + * it with an external lock. */ export function markCacheEntryComplete(entry: CacheEntry): void { - const sentinelPath = join(entry.dir, COMPLETE_SENTINEL); - writeFileSync(sentinelPath, "", "utf-8"); + writeFileSync(join(entry.dir, COMPLETE_SENTINEL), "", "utf-8"); +} + +/** + * Rebuild the in-memory frame index for a cached entry. Called on cache hits + * so the extractor's caller receives the same `ExtractedFrames` shape it + * would get from a fresh extraction — without re-running ffmpeg or ffprobe. + * + * The `metadata` argument is the `VideoMetadata` probed in the extractor's + * Phase 2 (pre-preflight). Passing it here avoids an extra ffprobe on the + * hit path. + */ +export interface RehydrateOptions { + videoId: string; + srcPath: string; + fps: number; + format: CacheFrameFormat; + metadata: VideoMetadata; +} + +export interface RehydratedFrames { + videoId: string; + srcPath: string; + outputDir: string; + framePattern: string; + fps: number; + totalFrames: number; + metadata: VideoMetadata; + framePaths: Map; +} + +export function rehydrateCacheEntry( + entry: CacheEntry, + options: RehydrateOptions, +): RehydratedFrames { + const framePattern = `${FRAME_FILENAME_PREFIX}%05d.${options.format}`; + const framePaths = new Map(); + const suffix = `.${options.format}`; + const files = readdirSync(entry.dir) + .filter((f) => f.startsWith(FRAME_FILENAME_PREFIX) && f.endsWith(suffix)) + .sort(); + files.forEach((file, idx) => { + framePaths.set(idx, join(entry.dir, file)); + }); + return { + videoId: options.videoId, + srcPath: options.srcPath, + outputDir: entry.dir, + framePattern, + fps: options.fps, + totalFrames: framePaths.size, + metadata: options.metadata, + framePaths, + }; } diff --git a/packages/engine/src/services/videoFrameExtractor.ts b/packages/engine/src/services/videoFrameExtractor.ts index 2774c422..7ee86300 100644 --- a/packages/engine/src/services/videoFrameExtractor.ts +++ b/packages/engine/src/services/videoFrameExtractor.ts @@ -23,6 +23,9 @@ import { ensureCacheEntryDir, lookupCacheEntry, markCacheEntryComplete, + readKeyStat, + rehydrateCacheEntry, + type CacheFrameFormat, } from "./extractionCache.js"; export interface VideoElement { @@ -373,6 +376,21 @@ async function convertSdrToHdr( } } +/** + * Resolve the used-segment duration for a video, falling back to the source's + * natural duration when the caller hasn't specified bounds (end=Infinity) or + * the bounds are nonsensical (end<=start). + */ +function resolveSegmentDuration( + requested: number, + mediaStart: number, + metadata: VideoMetadata, +): number { + if (Number.isFinite(requested) && requested > 0) return requested; + const sourceRemaining = metadata.durationSeconds - mediaStart; + return sourceRemaining > 0 ? sourceRemaining : metadata.durationSeconds; +} + /** * Re-encode a VFR (variable frame rate) video segment to CFR so the downstream * fps filter can extract frames reliably. Screen recordings, phone videos, and @@ -494,12 +512,17 @@ export async function extractAllVideoFrames( // bounds) rather than the workDir-local normalized file produced by // Phase 2a/2b preflight. Without this, every render would write a new // normalized file with a fresh mtime → fresh cache key → perpetual misses. - const cacheKeyInputs = resolvedVideos.map(({ video, videoPath }) => ({ - videoPath, - mediaStart: video.mediaStart, - start: video.start, - end: video.end, - })); + const cacheKeyInputs = resolvedVideos.map(({ video, videoPath }) => { + const stat = readKeyStat(videoPath); + return { + videoPath, + mtimeMs: stat.mtimeMs, + size: stat.size, + mediaStart: video.mediaStart, + start: video.start, + end: video.end, + }; + }); // Phase 2: Probe color spaces and normalize if mixed HDR/SDR const phase2ProbeStart = Date.now(); @@ -647,89 +670,84 @@ export async function extractAllVideoFrames( } breakdown.vfrPreflightMs = Date.now() - vfrPreflightStart; - // Phase 3: Extract frames (parallel). Optionally consults the - // content-addressed extraction cache to skip the ffmpeg call on - // repeat renders of the same composition. const phase3Start = Date.now(); const cacheRootDir = config?.extractCacheDir; - const cacheFormat = options.format ?? "jpg"; + const cacheFormat: CacheFrameFormat = options.format ?? "jpg"; + + async function tryCachedExtract( + video: VideoElement, + videoPath: string, + videoDuration: number, + i: number, + ): Promise { + if (!cacheRootDir) return null; + const keyInput = cacheKeyInputs[i]; + const probedMeta = videoMetadata[i]; + if (!keyInput || !probedMeta) return null; + + const keyDuration = resolveSegmentDuration( + keyInput.end - keyInput.start, + keyInput.mediaStart, + probedMeta, + ); + const lookup = lookupCacheEntry(cacheRootDir, { + videoPath: keyInput.videoPath, + mtimeMs: keyInput.mtimeMs, + size: keyInput.size, + mediaStart: keyInput.mediaStart, + duration: keyDuration, + fps: options.fps, + format: cacheFormat, + }); + + if (lookup.hit) { + breakdown.cacheHits += 1; + const rehydrated = rehydrateCacheEntry(lookup.entry, { + videoId: video.id, + srcPath: keyInput.videoPath, + fps: options.fps, + format: cacheFormat, + metadata: probedMeta, + }); + return { ...rehydrated, ownedByLookup: true }; + } + + breakdown.cacheMisses += 1; + ensureCacheEntryDir(lookup.entry); + const result = await extractVideoFramesRange( + videoPath, + video.id, + video.mediaStart, + videoDuration, + options, + signal, + config, + lookup.entry.dir, + ); + // Mark complete only AFTER frames are on disk — a crash mid-extract + // leaves the entry un-sentineled so the next lookup re-extracts over it. + markCacheEntryComplete(lookup.entry); + return { ...result, ownedByLookup: true }; + } + const results = await Promise.all( resolvedVideos.map(async ({ video, videoPath }, i) => { if (signal?.aborted) { throw new Error("Video frame extraction cancelled"); } try { - let videoDuration = video.end - video.start; - - // Fallback: if no data-duration/data-end was specified (end is Infinity or 0), - // probe the actual video file to get its natural duration. - if (!Number.isFinite(videoDuration) || videoDuration <= 0) { - const metadata = videoMetadata[i] ?? (await extractMediaMetadata(videoPath)); - const sourceDuration = metadata.durationSeconds - video.mediaStart; - videoDuration = sourceDuration > 0 ? sourceDuration : metadata.durationSeconds; + const probedMeta = videoMetadata[i] ?? (await extractMediaMetadata(videoPath)); + const videoDuration = resolveSegmentDuration( + video.end - video.start, + video.mediaStart, + probedMeta, + ); + if (video.end - video.start !== videoDuration) { video.end = video.start + videoDuration; } - if (cacheRootDir) { - const keyInput = cacheKeyInputs[i]; - const probedMeta = videoMetadata[i]; - if (keyInput && probedMeta) { - let keyDuration = keyInput.end - keyInput.start; - if (!Number.isFinite(keyDuration) || keyDuration <= 0) { - const sourceRemaining = probedMeta.durationSeconds - keyInput.mediaStart; - keyDuration = sourceRemaining > 0 ? sourceRemaining : probedMeta.durationSeconds; - } - const lookup = lookupCacheEntry(cacheRootDir, { - videoPath: keyInput.videoPath, - mediaStart: keyInput.mediaStart, - duration: keyDuration, - fps: options.fps, - format: cacheFormat, - }); - if (lookup.hit) { - const framePattern = `${FRAME_FILENAME_PREFIX}%05d.${cacheFormat}`; - const framePaths = new Map(); - const files = readdirSync(lookup.entry.dir) - .filter((f) => f.startsWith(FRAME_FILENAME_PREFIX) && f.endsWith(`.${cacheFormat}`)) - .sort(); - files.forEach((file, idx) => { - framePaths.set(idx, join(lookup.entry.dir, file)); - }); - breakdown.cacheHits += 1; - return { - result: { - videoId: video.id, - srcPath: keyInput.videoPath, - outputDir: lookup.entry.dir, - framePattern, - fps: options.fps, - totalFrames: framePaths.size, - metadata: probedMeta, - framePaths, - ownedByLookup: true, - } satisfies ExtractedFrames, - }; - } - breakdown.cacheMisses += 1; - ensureCacheEntryDir(lookup.entry); - const result = await extractVideoFramesRange( - videoPath, - video.id, - video.mediaStart, - videoDuration, - options, - signal, - config, - lookup.entry.dir, - ); - // ownedByLookup=true so FrameLookupTable.cleanup leaves the entry - // behind for the next render. Marking complete is the last step so - // a crash mid-extract leaves the entry un-sentineled (treated as a - // miss by the next lookup and re-extracted over the partial frames). - markCacheEntryComplete(lookup.entry); - return { result: { ...result, ownedByLookup: true } }; - } - } + const cached = await tryCachedExtract(video, videoPath, videoDuration, i); + if (cached) return { result: cached }; const result = await extractVideoFramesRange( videoPath, From 483c2ba6b4bdf73cda8aac6fef49f2e5cbe89262 Mon Sep 17 00:00:00 2001 From: James Russo Date: Thu, 23 Apr 2026 22:10:41 +0000 Subject: [PATCH 3/4] perf(engine): null-return readKeyStat for missing files + document cache caveats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Miguel's review on #446: - P2: readKeyStat now returns null when the source file is missing instead of the {mtimeMs: 0, size: 0} zero-tuple sentinel. Two unrelated missing paths would otherwise share the same cache key and pollute the cache with orphaned entries. The cacheKeyInputs builder propagates the null forward; tryCachedExtract already skips the cache path for null entries so the extractor surfaces the real file-not-found error downstream. - P2: document the single-writer caveat on EngineConfig.extractCacheDir. Concurrent renders targeting the same cache dir can race on partial frame reads — the .hf-complete sentinel prevents serving an incomplete entry but individual frames are written non-atomically. Also flagged the coarser-mtime-on-network-filesystems hazard for future ops. - Updated the existing "tolerates a missing source" test to assert the new contract (readKeyStat returns null) rather than the old "zero-stat sentinel" behaviour. All 480 engine tests pass. --- packages/engine/src/config.ts | 12 ++++++++++++ .../engine/src/services/extractionCache.test.ts | 17 ++++++++++++----- packages/engine/src/services/extractionCache.ts | 12 +++++++----- .../engine/src/services/videoFrameExtractor.ts | 5 +++++ 4 files changed, 36 insertions(+), 10 deletions(-) diff --git a/packages/engine/src/config.ts b/packages/engine/src/config.ts index b81764d6..a07d3a13 100644 --- a/packages/engine/src/config.ts +++ b/packages/engine/src/config.ts @@ -80,6 +80,18 @@ export interface EngineConfig { * and cleanup removes it when the render ends, preserving the pre-cache * behaviour. * + * **Single-writer.** The cache is not safe for concurrent renders pointing + * at the same directory. A `.hf-complete` sentinel prevents another render + * from serving an entry that hasn't finished extracting, but individual + * frame files are written non-atomically — a second render reading during + * the write window can observe a truncated frame. Give each concurrent + * render pipeline its own `extractCacheDir`, or gate with an external mutex. + * + * **Network filesystems.** `mtime` resolution on NFS/SMB mounts can be + * coarser than expected (seconds rather than nanoseconds), which may + * produce spurious cache hits if a source file is overwritten within the + * same mtime tick. Local filesystems are the intended deployment target. + * * Env fallback: `HYPERFRAMES_EXTRACT_CACHE_DIR`. */ extractCacheDir?: string; diff --git a/packages/engine/src/services/extractionCache.test.ts b/packages/engine/src/services/extractionCache.test.ts index 037c9ab5..82cd09a6 100644 --- a/packages/engine/src/services/extractionCache.test.ts +++ b/packages/engine/src/services/extractionCache.test.ts @@ -17,11 +17,12 @@ import { } from "./extractionCache.js"; const keyFor = (videoPath: string, overrides: Partial = {}): CacheKeyInput => { - const { mtimeMs, size } = readKeyStat(videoPath); + const stat = readKeyStat(videoPath); + if (!stat) throw new Error(`keyFor fixture missing on disk: ${videoPath}`); return { videoPath, - mtimeMs, - size, + mtimeMs: stat.mtimeMs, + size: stat.size, mediaStart: 0, duration: 3, fps: 30, @@ -124,8 +125,14 @@ describe("computeCacheKey", () => { expect(after).not.toBe(before); }); - it("tolerates a missing source file (extractor surfaces the real error later)", () => { - expect(() => computeCacheKey(base(join(tmpRoot, "does-not-exist.mp4")))).not.toThrow(); + it("readKeyStat returns null for a missing source (callers skip the cache)", () => { + // Previously readKeyStat returned a `{mtimeMs: 0, size: 0}` sentinel for + // missing files; two unrelated missing paths then shared the same cache + // key tuple and polluted the cache. The contract now returns null so + // callers can explicitly skip the cache path and let the extractor + // surface the real file-not-found error. + const missing = join(tmpRoot, "does-not-exist.mp4"); + expect(readKeyStat(missing)).toBeNull(); }); }); diff --git a/packages/engine/src/services/extractionCache.ts b/packages/engine/src/services/extractionCache.ts index e3011b64..53859624 100644 --- a/packages/engine/src/services/extractionCache.ts +++ b/packages/engine/src/services/extractionCache.ts @@ -83,16 +83,18 @@ export interface CacheLookup { } /** - * Read `(mtimeMs, size)` for a path. Returns zeros if the file is missing — - * callers on the cache hot path can still compute a stable key, and the - * extractor surfaces the real file-not-found error. + * Read `(mtimeMs, size)` for a path. Returns `null` if the file is missing — + * callers should skip the cache path for that entry so the extractor surfaces + * the real file-not-found error. Returning a zero-stat sentinel would let two + * missing files share the same `(0, 0)` tuple and pollute the cache with an + * orphaned entry. */ -export function readKeyStat(videoPath: string): { mtimeMs: number; size: number } { +export function readKeyStat(videoPath: string): { mtimeMs: number; size: number } | null { try { const stat = statSync(videoPath); return { mtimeMs: Math.floor(stat.mtimeMs), size: stat.size }; } catch { - return { mtimeMs: 0, size: 0 }; + return null; } } diff --git a/packages/engine/src/services/videoFrameExtractor.ts b/packages/engine/src/services/videoFrameExtractor.ts index 7ee86300..f1cece17 100644 --- a/packages/engine/src/services/videoFrameExtractor.ts +++ b/packages/engine/src/services/videoFrameExtractor.ts @@ -514,6 +514,11 @@ export async function extractAllVideoFrames( // normalized file with a fresh mtime → fresh cache key → perpetual misses. const cacheKeyInputs = resolvedVideos.map(({ video, videoPath }) => { const stat = readKeyStat(videoPath); + // Missing files return null — skip the cache path for that entry. The + // extractor will surface the real file-not-found error downstream, and we + // avoid polluting the cache with a `(mtimeMs: 0, size: 0)` tuple that two + // unrelated missing paths would otherwise share. + if (!stat) return null; return { videoPath, mtimeMs: stat.mtimeMs, From ba05796793a60aa5573972a7d679a4a849429cff Mon Sep 17 00:00:00 2001 From: James Russo Date: Thu, 23 Apr 2026 22:42:56 +0000 Subject: [PATCH 4/4] perf(engine): extend HDR preflight splice to cacheKeyInputs Downstack extension of the #445 past-EOF splice fix. The parallel-array splice that removes HDR-preflight-skipped entries before Phase 3 was introduced on #445 to splice (resolvedVideos, videoMetadata, videoColorSpaces). This commit adds the fourth parallel array (cacheKeyInputs) that #446 introduces, so the cache lookup at `cacheKeyInputs[i]` in tryCachedExtract doesn't point at a stale slot after the splice. All 480 engine tests pass. --- packages/engine/src/services/videoFrameExtractor.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/engine/src/services/videoFrameExtractor.ts b/packages/engine/src/services/videoFrameExtractor.ts index f1cece17..8f81c6de 100644 --- a/packages/engine/src/services/videoFrameExtractor.ts +++ b/packages/engine/src/services/videoFrameExtractor.ts @@ -625,6 +625,10 @@ export async function extractAllVideoFrames( resolvedVideos.splice(i, 1); videoMetadata.splice(i, 1); videoColorSpaces.splice(i, 1); + // Added by the extraction-cache commit: keep cacheKeyInputs aligned + // with the other parallel arrays so Phase 3's `cacheKeyInputs[i]` + // lookup doesn't point at a stale slot after the splice. + cacheKeyInputs.splice(i, 1); } } }