[miguel-heygen]

Stacked on #6583 (base is its branch; retargets to main when that merges).

What changed

A range longer than one 120 s segment is split into segments that run side by side, and every segment seeks on its own. In a video with few keyframes they all land on the same keyframe, so segment k decodes the lead-in plus every segment before it, then throws those frames away. When the lead-in to the range start is at least one segment long, the range now runs as one ffmpeg process that decodes the lead-in once. Ranges with explicit frame ranges or a VFR frame count still split, and sources with keyframes close together still run segments in parallel.

Measured

Linux box (shared, loaded), 20-min 3840x2160 60 fps H.264 source with one keyframe, window starting 600 s in, 30 fps output, one run each:

window	segments	before wall / CPU	after wall / CPU
120 s	1	140 s / 384 s	113 s / 398 s
240 s	2	235 s / 854 s	172 s / 627 s
480 s	4	488 s / 1912 s	479 s / 1113 s

The 1-segment row runs the same path before and after, so its gap is box load. The single pass cuts CPU 27% at 2 segments and 42% at 4, and wall time 27% at 2 segments. At 4 segments wall time is flat: one process writes about 30 fps, and the parallel segments used to make up for the repeated decode with extra cores. What it buys is a render that no longer burns about twice the CPU on decoding it throws away.

Test

A 260 s 10 fps clip with one keyframe, and one with a keyframe per second, each extracted 125 s in for 130 frames: the test counts ffmpeg processes. On #6583 the one-keyframe case fails (expected "vi.fn()" to be called 1 times, but got 2 times); on this branch both cases pass 3 of 3, and the whole videoFrameExtractor.test.ts suite passes (219 tests).

Why under 100 lines

One condition and one reused lead-in value, plus the test.

Not changed

The segment length (120 s) and the lead-in threshold (one segment) are judgement calls; the threshold is where re-decoding starts to cost more than a segment extracts.

[heygengenesis]

Verdict: REQUEST CHANGES · 1 blocker · 1 important · 0 nits · 4e1551ab

The single-pass switch is correct, and the timeout is still a stall timeout, so one long pass isn't capped by wall-clock time. videoFrameExtractor.test.ts passes locally (219/219, with real ffmpeg). The only blocker is the red Comments run. ↓ inline

Scope

Read the diff plus runSegmentedExtraction, stallTimeoutMs, keyframeGapSeconds and runFfmpeg's inactivity timeout. This PR is stacked on #6583, so main CI (lint, typecheck, tests) runs only after the retarget. No prior reviews.

— Genesis

[heygengenesis]

blocker: the Comments workflow is red here: RAISED comment share: packages/engine/src/services/videoFrameExtractor.ts is 10.5% (317/3010), at the base 10.5% (315/3007). Cut these two lines, or fold them into the name (e.g. leadInCostsMoreThanASegment).

Suggested change

	// Every segment seeks back to the same keyframe and decodes the lead-in again, so a lead-in
	// as long as a segment costs more than the parallelism buys; one process decodes it once.

[heygengenesis]

✅ Fixed in 7915eb2

[heygengenesis]

important: the check looks only at the lead-in before startTime, so a range that starts on or near the one keyframe still splits. Segment k then re-decodes k×120 s, which is the waste this PR removes. I confirmed it with your test plus a case { keyint: "infinite", start: 0 }: it expected 1 process and got 2. That's no regression from #6583, but a clip used from its start is the common case. Probing the gap at the second segment's seek as well (keyframeGapSeconds(videoPath, startTime + EXTRACTION_SEGMENT_SECONDS)) would catch it. Otherwise, name this case in "Not changed".

[heygengenesis]

✅ Fixed in 7915eb2

[heygengenesis]

Verdict: REQUEST CHANGES — 0 resolved, 2 open, 0 new · 4e1551ab → ec91e2df (a rebase onto #6583 f0c1f622 with the same patch apart from the base's helper rename; Comments is still red, +2 lines in videoFrameExtractor.ts).

[heygengenesis]

Verdict: APPROVE — 2 resolved, 0 open, 0 new · ec91e2df → 7915eb23. Comments and regression are green at this head. The extractor suites pass locally (221/221), including the new start-at-keyframe case. This PR is stacked on #6583, so main CI runs after the retarget.

The base branch was changed.

[github-actions]

Edit accuracy: accurate 271 (base branch 271), smooth 142 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (2)

• seqplainkeys-none-px-r0-root-z100: tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo false, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true
• seqplainkeys-none-pct-r0-nested-z100: tracking 0.05, pressJump 0, drop 0, reload 0, render 0.02, renderKey -, undo false, teleport true / tracking 0.05, pressJump 0, drop 0, reload 0, render 0.02, renderKey -, undo true, teleport true / tracking 0.05, pressJump 0, drop 0, reload 0, render 0.02, renderKey -, undo true, teleport true