fix: bound held-tail frame extraction

Author: jrusso1020

packages/engine/src/services/videoFrameExtractor.test.ts

@@ -124,34 +124,67 @@ describe("resolveVideoExtractionDuration", () => {
         2,
       ),
     ).toEqual({
-      compositionStart: -5,
-      mediaStart: 0,
-      durationSeconds: 3,
-      preserveTimelinePhase: true,
+      compositionStart: -3,
+      mediaStart: 2,
+      durationSeconds: 1,
+      preserveTimelineEnd: true,
     });
   });
 
   it.each([
-    { loop: true, label: "loop" },
-    { loop: false, label: "held tail" },
-  ])("caps a finite long slot to one short source range for $label playback", ({ loop }) => {
-    expect(resolveVideoExtractionWindow(video({ end: 60, loop }), metadata(3), 60)).toEqual({
+    { loop: true, preservation: { preserveTimelinePhase: true }, label: "loop" },
+    { loop: false, preservation: { preserveTimelineEnd: true }, label: "held tail" },
+  ])(
+    "caps a finite long slot to one short source range for $label playback",
+    ({ loop, preservation }) => {
+      expect(resolveVideoExtractionWindow(video({ end: 60, loop }), metadata(3), 60)).toEqual({
+        compositionStart: 0,
+        mediaStart: 0,
+        durationSeconds: 3,
+        ...preservation,
+      });
+    },
+  );
+
+  it("preserves authored timing when the visible interval partially crosses a held tail", () => {
+    expect(
+      resolveVideoExtractionWindow(
+        video({ start: -2, end: 10, mediaStart: 0, loop: false }),
+        metadata(3),
+        2,
+      ),
+    ).toEqual({
       compositionStart: 0,
-      mediaStart: 0,
-      durationSeconds: 3,
-      preserveTimelinePhase: true,
+      mediaStart: 2,
+      durationSeconds: 1,
+      preserveTimelineEnd: true,
     });
   });
 
-  it("preserves authored timing when the visible interval partially crosses a held tail", () => {
+  it("bounds an entirely held long source to its final-frame sample", () => {
     expect(
       resolveVideoExtractionWindow(
-        video({ start: -2, end: 10, mediaStart: 0, loop: false }),
+        video({ start: -600, end: 10, mediaStart: 0, loop: false }),
+        metadata(120),
+        2,
+      ),
+    ).toEqual({
+      compositionStart: -481,
+      mediaStart: 119,
+      durationSeconds: 1,
+      preserveTimelineEnd: true,
+    });
+  });
+
+  it("preserves a complete loop cycle when visibility ends exactly on a wrap boundary", () => {
+    expect(
+      resolveVideoExtractionWindow(
+        video({ start: -1, end: 10, mediaStart: 0, loop: true }),
         metadata(3),
         2,
       ),
     ).toEqual({
-      compositionStart: -2,
+      compositionStart: -1,
       mediaStart: 0,
       durationSeconds: 3,
       preserveTimelinePhase: true,
@@ -167,6 +200,30 @@ describe("resolveVideoExtractionDuration", () => {
     });
   });
 
+  it("never plans more extraction than the playable source range", () => {
+    for (const loop of [false, true]) {
+      for (const sourceDuration of [0.5, 3, 120]) {
+        for (const mediaStart of [0, sourceDuration / 3]) {
+          for (const start of [-600, -5, -1, 0, 2]) {
+            const window = resolveVideoExtractionWindow(
+              video({ start, end: start + 60, mediaStart, loop }),
+              metadata(sourceDuration),
+              10,
+            );
+            expect(window.durationSeconds).toBeLessThanOrEqual(sourceDuration - mediaStart);
+            expect(window.durationSeconds).toBeGreaterThanOrEqual(0);
+          }
+        }
+      }
+    }
+  });
+
+  it("rejects a media start at source EOF before planning extraction", () => {
+    expect(() =>
+      resolveVideoExtractionWindow(video({ mediaStart: 3 }), metadata(3), 10),
+    ).toThrowError(expect.objectContaining({ kind: "media_start_out_of_range", retryable: false }));
+  });
+
   it("rebases a loop phase when the visible window stays within one cycle", () => {
     expect(
       resolveVideoExtractionWindow(
@@ -1228,7 +1285,10 @@ describe.skipIf(!HAS_FFMPEG)("extractAllVideoFrames on a VFR source", () => {
     const extracted = result.extracted[0];
     if (!extracted) throw new Error("expected held-tail source frames");
     const lookup = createFrameLookupTable([video], result.extracted);
-    expect(video).toMatchObject({ start: -15, mediaStart: 0, loop: false });
+    // The authored slot remains active through end=5, but extraction is
+    // rebased to the source's final one-second sample instead of materializing
+    // the full ten-second source just to hold its last frame.
+    expect(video).toMatchObject({ start: -6, end: 5, mediaStart: 9, loop: false });
     expect(lookup.getFrame("negative-held-tail", 0)).toBe(
       extracted.framePaths.get(extracted.totalFrames - 1),
     );
@@ -1453,6 +1513,52 @@ describe.skipIf(!HAS_FFMPEG)("extractAllVideoFrames on a VFR source", () => {
     rmSync(CACHE_DIR, { recursive: true, force: true });
   }, 60_000);
 
+  it("reuses one-cycle loop extraction across different authored starts", async () => {
+    const cacheDir = mkdtempSync(join(tmpdir(), "hf-extract-loop-phase-cache-test-"));
+    const src = await synthCfrClip("cache-loop-phase-src.mp4", 3);
+    try {
+      const firstOutputDir = join(FIXTURE_DIR, "out-cache-loop-phase-first");
+      const secondOutputDir = join(FIXTURE_DIR, "out-cache-loop-phase-second");
+      mkdirSync(firstOutputDir, { recursive: true });
+      mkdirSync(secondOutputDir, { recursive: true });
+
+      const first = await extractAllVideoFrames(
+        [
+          {
+            ...cfrClipElement("loop-cache-first", src, 60),
+            loop: true,
+          },
+        ],
+        FIXTURE_DIR,
+        { fps: 30, outputDir: firstOutputDir, timelineEnd: 60 },
+        undefined,
+        { extractCacheDir: cacheDir },
+      );
+      expect(first.errors).toEqual([]);
+      expect(first.phaseBreakdown.cacheMisses).toBe(1);
+
+      const second = await extractAllVideoFrames(
+        [
+          {
+            ...cfrClipElement("loop-cache-second", src, 66),
+            start: -6,
+            loop: true,
+          },
+        ],
+        FIXTURE_DIR,
+        { fps: 30, outputDir: secondOutputDir, timelineEnd: 60 },
+        undefined,
+        { extractCacheDir: cacheDir },
+      );
+      expect(second.errors).toEqual([]);
+      expect(second.phaseBreakdown.cacheHits).toBe(1);
+      expect(second.phaseBreakdown.cacheMisses).toBe(0);
+      expect(second.extracted[0]?.totalFrames).toBe(first.extracted[0]?.totalFrames);
+    } finally {
+      rmSync(cacheDir, { recursive: true, force: true });
+    }
+  }, 60_000);
+
   it("updates the cache sentinel mtime on a hit", async () => {
     const CACHE_DIR = mkdtempSync(join(tmpdir(), "hf-extract-cache-touch-test-"));
     const SRC = await synthCfrClip("cache-touch-src.mp4", 1);

packages/engine/src/services/videoFrameExtractor.ts

@@ -740,16 +740,27 @@ export interface TimelineExtractionWindow {
   durationSeconds: number;
   /**
    * Preserve the authored timeline origin and mediaStart for lookup. This is
-   * required when a negative preroll crosses a source boundary: looped media
-   * still needs its modulo phase, while a non-looping authored slot still
-   * needs the extracted final frame for held-tail playback.
+   * required when a looped visible interval crosses a source boundary and
+   * still needs modulo phase against the complete extracted source cycle.
    */
   preserveTimelinePhase?: boolean;
+  /**
+   * Keep the authored end while rebasing start/mediaStart to the extracted
+   * source suffix. Non-looping lookup then holds the suffix's final frame
+   * through the remainder of the authored slot.
+   */
+  preserveTimelineEnd?: boolean;
 }
 
 type TimelineWindowVideo = Pick<VideoElement, "start" | "end" | "mediaStart"> &
   Partial<Pick<VideoElement, "loop">>;
 
+// A clip whose visible interval begins after source EOF only needs a reliable
+// final-frame sample, not its entire source history. One second is long enough
+// to tolerate ordinary CFR/VFR timestamp rounding while keeping raw HDR
+// scratch bounded independently of source length.
+const HELD_TAIL_EXTRACTION_MAX_SECONDS = 1;
+
 /**
  * Intersect an authored slot with the render timeline, then select the
  * smallest playable source range that preserves timeline lookup semantics.
@@ -793,7 +804,10 @@ export function resolveTimelineExtractionWindow(
     if (sourceRemaining > 0 && video.loop) {
       const phaseOffset = trimmedPreroll % sourceRemaining;
       const phaseRemaining = sourceRemaining - phaseOffset;
-      if (visibleDuration > phaseRemaining) {
+      // The element visibility contract includes its end boundary. Preserve a
+      // complete cycle on equality as well, otherwise a rebased suffix would
+      // wrap to its own first frame instead of the source cycle's first frame.
+      if (visibleDuration >= phaseRemaining) {
         return {
           compositionStart: video.start,
           mediaStart: video.mediaStart,
@@ -802,15 +816,31 @@ export function resolveTimelineExtractionWindow(
         };
       }
       mediaStart = video.mediaStart + phaseOffset;
-    } else if (
-      sourceRemaining > 0 &&
-      visibleDuration > Math.max(0, sourceRemaining - trimmedPreroll)
-    ) {
+    } else if (sourceRemaining > 0) {
+      const sourceVisibleAfterPreroll = Math.max(0, sourceRemaining - trimmedPreroll);
+      if (visibleDuration <= sourceVisibleAfterPreroll) {
+        return {
+          compositionStart,
+          mediaStart,
+          durationSeconds: visibleDuration,
+        };
+      }
+
+      // The visible interval enters (or is entirely inside) the held tail.
+      // Extract only the visible source suffix, plus a bounded final-frame
+      // sample when preroll is already at/past EOF. Rebase lookup to the
+      // source time represented by frame zero, but retain the authored end so
+      // non-loop lookup can clamp to the extracted final frame.
+      const extractionDuration = Math.min(
+        sourceRemaining,
+        Math.max(sourceVisibleAfterPreroll, HELD_TAIL_EXTRACTION_MAX_SECONDS),
+      );
+      const extractionOffset = sourceRemaining - extractionDuration;
       return {
-        compositionStart: video.start,
-        mediaStart: video.mediaStart,
-        durationSeconds: sourceRemaining,
-        preserveTimelinePhase: true,
+        compositionStart: video.start + extractionOffset,
+        mediaStart: video.mediaStart + extractionOffset,
+        durationSeconds: extractionDuration,
+        preserveTimelineEnd: true,
       };
     }
   }
@@ -827,6 +857,22 @@ export function resolveVideoExtractionWindow(
   metadata: VideoMetadata,
   timelineEnd?: number,
 ): TimelineExtractionWindow {
+  if (!(metadata.durationSeconds > 0)) {
+    throw new VideoSourceExtractionError(
+      "invalid_media",
+      false,
+      "Video source has no positive duration",
+      `Video source duration is ${metadata.durationSeconds}s`,
+    );
+  }
+  if (video.mediaStart >= metadata.durationSeconds) {
+    throw new VideoSourceExtractionError(
+      "media_start_out_of_range",
+      false,
+      "Video media start is outside the source duration",
+      `Video media start ${video.mediaStart}s is outside source duration ${metadata.durationSeconds}s`,
+    );
+  }
   const resolvedDuration = resolveSegmentDuration(
     video.end - video.start,
     video.mediaStart,
@@ -1649,7 +1695,9 @@ export async function extractAllVideoFrames(
         }
         if (!window.preserveTimelinePhase) {
           video.start = window.compositionStart;
-          video.end = window.compositionStart + videoDuration;
+          if (!window.preserveTimelineEnd) {
+            video.end = window.compositionStart + videoDuration;
+          }
           video.mediaStart = window.mediaStart;
         }
         const keyInput = cacheKeyInputs[index];