fix(producer): correct short VFR frame coverage

[jrusso1020]

Summary

• Use one shared extraction frame-count calculation so VFR extraction and coverage expectations follow the same FFmpeg duration semantics.
• Quantize durations to FFmpeg's six-digit time base before frame conversion, covering non-zero-start JavaScript subtraction artifacts without hiding genuine fractional frames.
• Keep overlapping VFR trims seek-local: FFmpeg's CFR resampling phase resets per seek, so a union extraction cannot safely be sliced into content-equivalent VFR members.
• Allow exactly one missing frame only for short 14 through 20 frame clips when the configured threshold is below 1.

Acceptance cases

• (4.03 - 3.53) * 30 resolves to 15 frames, not 16; (4.03 - 3.78) * 24 resolves to 6, not 7.
• FFmpeg's 0.6000009 versus 0.600001 microsecond boundary is pinned at 18 versus 19 VFR frames at 30 fps.
• Direct and batched VFR extraction produce byte-identical frames and do not share a superset.
• CFR aligned supersets remain enabled.
• 13 of 14 and 18 of 19 pass under the bounded tolerance; zero frames, 1 of 2, deficits of two or more, longer clips, and threshold=1 fail.

Validation

• Engine extractor suite: 121 passing, including real FFmpeg regressions.
• Producer coverage suite: 36 passing.
• Engine and producer typechecks passing.
• Pre-commit tracked-artifact, lint, format, fallow, and typecheck gates passing.
• Independent adversarial re-review: no remaining blockers.

Scope

HDR behavior and dependency/version changes are intentionally out of scope.

Rollback

Revert this PR. No migration or persisted state is introduced.

[terencecho]

Verified independently at head d626766e. Focused on the FFmpeg boundary semantics you flagged — the microsecond -t quantization + the VFR ceil / CFR round-nearest split — and confirmed against the real FFmpeg the tests invoke, not just the unit expectations. Approve pending fresh CI green.

Microsecond quantization matches av_parse_time truncation semantics

The core insight is that FFmpeg's -t <duration> argument goes through av_parse_time, which parses via strtod into a double, multiplies by AV_TIME_BASE = 1_000_000, and stores as int64_t — an implicit double→int64 conversion, which in C is truncation toward zero. So a -t 0.6000009 on the command line becomes 600000 microseconds inside FFmpeg, not 600001.

The PR's Math.trunc(durationSeconds * 1_000_000) / 1_000_000 at videoFrameExtractor.ts:178 is exactly that: multiply-then-truncate, matching FFmpeg's implicit int64 cast. The two pinned test cases confirm the boundary:

• 0.6000009 → 18 (truncated to 0.600000 → 18.0 frames at 30fps → ceil 18)
• 0.600001 → 19 (truncated to 0.600001 → 18.00003 frames at 30fps → ceil 19)

That's the six-digit microsecond wall you'd expect from an int64_t µs internal representation. Correct.

Non-zero-start subtraction: 4.03 - 3.53 is the interesting case

JS float subtraction gives 4.03 - 3.53 = 0.5000000000000004 (single-ULP noise off the ideal 0.5). Without the microsecond quantization, Math.ceil(0.5000000000000004 * 30) = Math.ceil(15.000000000000012) = 16 — one frame too many. With quantization: Math.trunc(0.5000000000000004 * 1e6) = 500000 exactly → 0.5 → 15 frames → Math.ceil(15) = 15. ✓

Symmetric direction (4.03 - 3.78 = 0.24999999999999956): Math.trunc(0.24999999999999956 * 1e6) = 249999 → 0.249999 → 5.99998 frames at 24fps → tolerance snap? |5.99998 - 6| = 2e-5, tolerance ≈ 4 × ε × 6 ≈ 5.3e-15 — no snap. Math.ceil(5.99998) = 6. ✓ Matches test expectation of 6.

The reason 4.03 - 3.78 → 6 is subtle: it's not "the tolerance snapped 5.99998 to 6," it's "microsecond truncation put the duration just below 0.25 exactly, and the ceil of 5.99998 is 6 anyway." Both possible signs of JS float noise resolve correctly because the truncation absorbs the LOWER-side noise and the ceil absorbs the UPPER-side noise. This is what makes the JS subtraction artifact bounded either way.

The ULP tolerance is calibrated at the right layer

integerTolerance = Number.EPSILON * Math.max(1, Math.abs(rawFrames)) * 4 at videoFrameExtractor.ts:183. For rawFrames = 15.000000000000002 (the classic post-multiplication ULP-off case), tolerance ≈ 1.3e-14 and |diff| = 2e-15, so the snap fires. For rawFrames = 15.00003 (genuine fractional excess from 0.500001 * 30), tolerance ≈ 1.3e-14 and |diff| = 3e-5, no snap — real fractional frame is preserved. That's the correct discrimination — noise gets snapped, real fractions get ceiled.

Real FFmpeg is actually exercised, not just the unit expectation

describe.skipIf(!HAS_FFMPEG) at line 2103 gates the FFmpeg-dependent block; when FFmpeg is present, extractAllVideoFrames shells out to real FFmpeg and the tests assert totalFrames === 19 on 0.616666 VFR extraction at 30fps. That's a real behavioral pin — if FFmpeg's actual output ever diverges from extractionFrameCountForDuration(0.616666, 30, true), the test fails.

The keeps batched and direct VFR extraction equal at a floating integral boundary test at line 2167 goes further and readFileSync().equals(readFileSync()) on every frame's bytes between direct and batched paths — proves not just that the counts agree, but that FFmpeg's actual per-frame output is bit-identical for the two extraction shapes at the integral boundary. That's the strongest possible pin on VFR frame-boundary behavior.

Small operational nit worth confirming — is FFmpeg available on this repo's CI runners? If the FFmpeg-gated block gets skipped on CI, the real-FFmpeg verification lives only on developer machines. Worth double-checking the CI matrix has an FFmpeg-having runner so this actually runs in the pipeline.

Superset disabling for VFR is the right conservatism

buildSupersetGroup early-returns when misses.some(({ work }) => work.metadata.isVFR) at line 200-202. Even a single VFR miss disables the whole group's superset. This is more conservative than strictly necessary — pure-CFR members inside a mixed group could still be supersetted — but it's the right correctness trade-off given that CFR resampling phase resets per seek and there's no obvious way to detect "phase-aligned" a priori.

Practical impact should be near-zero: HyperFrames projects tend to have homogeneous media (either all VFR or all CFR fixtures). Mixed groups are rare and the perf regression on those is bounded. Non-issue.

Short-clip tolerance is well-scoped

isToleratedShortClipBoundaryMiss at videoFrameCoverage.ts:358 guards:

1. threshold < 1 — strict threshold=1 disables tolerance entirely (pinned).
2. capturedFrames > 0 — zero-frame extractions never tolerated.
3. expectedFrames ∈ [14, 20] — tolerance scoped to short clips only.
4. expectedFrames - capturedFrames === 1 — exactly one boundary miss, not more.

The upper bound of 20 is chosen because 20/21 = 95.2% would pass a 0.95 threshold anyway; longer clips don't need the tolerance under typical thresholds. The lower bound of 14 keeps really-short clips (say, 3-frame extractions) from getting a free frame — 2/3 vs 1/3 is a structurally different level of loss than 18/19.

The test at line 300-309 (does not apply the one-frame tolerance to longer clips) pins the upper bound at 21 frames with a 0.99 threshold. Good regression guard.

CI note

Latest visible regression job FAILURE is from run 30862032467, which was superseded by 30862060093 (currently in progress). The failing job entry is stale. Fresh full CI is running post-rebase per the PR body; will converge green if the local suite of 121 engine + 36 producer tests is representative.

Approve

Trusted-stamper stamp on Terence's authorization. Math is right, the microsecond quantization matches FFmpeg's real parse semantics, ULP tolerance is calibrated to catch arithmetic noise without swallowing real fractional frames, real-FFmpeg regressions bit-verify frame identity, and the short-clip tolerance is scoped narrowly enough to not hide meaningful losses.

— Review by tai (pr-review)

[vanceingalls]

Verdict: APPROVE

Solid, tight fix. Adversarial-verified against IEEE-754 edge cases, FFmpeg microsecond parsing semantics, selector-symmetry cells, and presence-vs-semantics for the new tests. No blockers.

What I verified

Microsecond quantization matches av_parse_time. I re-implemented extractionFrameCountForDuration in Node and ran every PR-body case + adversarial cases (videoFrameExtractor.ts:93-116):

• 0.6000009→18, 0.600001→19, 0.616666 VFR→19 / CFR→18, 0.466666→14 (both modes).
• (4.03-3.53)*30 = 15.000000000000014 snaps to 15; same story for (4.03-3.78)*24=6, (0.33-0.03)*30=9, (0.29-0.04)*24=6 (the FP-below-integer case, 5.999…), (0.35-0.05)*60=18.
• Genuine fractional 0.300001→10 (ceil not eaten).
• NaN/±0/negative/fps=0 fail closed to 0; 0.001 CFR → 1 via the positive-sub-frame floor.
• FFmpeg's av_parse_time does (int64_t)(strtod(s)*1e6), i.e. truncates toward zero. Math.trunc(d*1_000_000)/1_000_000 produces the identical microsecond duration, so the JS-side quantum matches what FFmpeg's -t actually consumes. Confirmed on the (4.03-3.53)="0.5000000000000004" case — both sides land at 500000μs exactly. The Number.EPSILON * max(1,|rawFrames|) * 4 tolerance snap catches the residual ULPs after the multiply.

VFR superset gate is real, not decorative. videoFrameExtractor.ts:1144 returns null from buildSupersetGroup as soon as any miss is VFR. sliceSupersetMember (line 1233) now routes through the shared helper. New regression at videoFrameExtractor.test.ts:2145 uses VFR_FIXTURE with two overlapping trims and pins supersetDirNames(outputDir)).toEqual([]) — sed-swap the gate to always-false and this assertion fails (a __superset-* dir would materialize). The batched-vs-direct byte-identity test at videoFrameExtractor.test.ts:2168 doubles up: it asserts both readFileSync(...).equals(...) per frame AND supersetDirNames === [], so gate removal fails on the dir check even in the (hypothetical) case where the CFR resampling phase happened to align. CFR aligned supersets remain enabled — regression tests at videoFrameExtractor.test.ts:2016 / 2078 / 2216 are untouched and still exercise the happy path.

Off-by-one tolerance is bounded and semantics-checking. videoFrameCoverage.ts:148-159 requires threshold < 1 && capturedFrames > 0 && expectedFrames∈[14,20] && (expectedFrames-capturedFrames)===1. Tests at videoFrameCoverage.test.ts:344-423 pin:

• 13/14 and 18/19 pass under 0.95 (line 344).
• 1/2 (deficit 1 but expectedFrames<14) throws (line 356).
• 17/19 (deficit 2) throws; 0/19 throws; 18/19 @ threshold=1 throws (lines 378-395).
• 20/21 (expectedFrames>20) throws (line 397); 84/89 (long-clip material shortfall) throws (line 410).

Sed-swap === 1 to >= 1, or drop the expectedFrames <= 20 bound: multiple tests break. Not presence-only.

Selector-symmetry ({is_vfr × zero_start × threshold_below_1 × short_clip}). Every combination behaves correctly for the values in scope; no obvious missed cell. The tolerance is expressed in terms of the coverage report (expected/captured counts + threshold), which subsumes is_vfr via the report's expectedFrames.

Non-blocker observations (P2)

1. Tolerance doesn't gate on isVFR. The field signal is a VFR-only microsecond-boundary miss, but the tolerance fires on any 14-20-frame clip with a 1-frame deficit — CFR included. Practically low-risk (CFR extraction is precise; a genuine 1-frame CFR loss in that window is rare), but the scoping comment (SHORT_CLIP_ONE_FRAME_TOLERANCE_*) doesn't say "VFR only" while the driving evidence is VFR-only. Consider either (a) narrowing to VFR reports or (b) documenting the scope-widening rationale in the constant's comment. Not a blocker.

2. Sub-microsecond durations return 1 while FFmpeg would return 0. For duration < 1e-6, Math.trunc(d*1_000_000)/1_000_000 = 0, rawFrames = 0, then Math.max(1, 0) = 1. FFmpeg's av_parse_time also lands on 0μs and extracts 0 frames — the coverage gate would then falsely fail (expected 1, captured 0). No caller produces sub-microsecond authored clips today, so this is theoretical.

3. Real-FFmpeg parity for CFR at microsecond boundaries. The pure-function unit tests at videoFrameExtractor.test.ts:308-346 pin extractionFrameCountForDuration values, but the real-FFmpeg regressions at :2145 / :2168 exercise the VFR path. CFR-with-microsecond-truncation behavior change (e.g. an input like 0.5166669 moves from 16→15 frames because of the Math.trunc step) is not pinned by a real-FFmpeg regression — only by the pure unit test asserting Math.round((Math.trunc(d*1e6)/1e6)*fps). Low risk (the CFR change only affects sub-microsecond FP noise), but worth a follow-up regression against real FFmpeg on a CFR clip with e.g. a subtracted timestamp landing at a microsecond half-boundary.

What I couldn't verify

• Byte-identical direct-vs-batched VFR frames at the integral-boundary case — real-FFmpeg CI only (HAS_FFMPEG-gated).
• The "121 passing, including real FFmpeg regressions" tally — I read the test file's structure, not a live CI run.
• Producer coverage suite "36 passing" claim — CI-only.

The gate rebuild is sound, the microsecond math is portable to av_parse_time, and the tolerance is bounded on all four axes it should be.

— Via

[miguel-heygen]

Blocking correctness issue at exact head d626766e:

extractionFrameCountForDuration() quantizes with Math.trunc(durationSeconds * 1_000_000), but that is not equivalent to FFmpeg parsing the decimal string passed to -t. For an ordinary authored duration, 2.05 * 1e6 === 2049999.9999999998, so the helper quantizes to 2.049999 and returns 61 CFR frames at 30fps. Real FFmpeg with -t 2.05 -vf fps=30 emits 62 frames (reproduced locally).

This is load-bearing in two places: sliceSupersetMember() slices an otherwise complete CFR superset to 61 frames, and producer coverage uses the same helper, reports 61/61, and silently accepts the missing final frame. The shared helper therefore makes the accounting agree with itself while disagreeing with the extractor.

Please derive the microsecond value from the same decimal string representation sent to FFmpeg (or otherwise avoid binary multiply-before-trunc), and add a real-FFmpeg regression for a 2.05s CFR direct extraction versus superset slice/coverage. The existing .6000009/.600001 unit oracle does not catch this binary-underflow case.

[somanshreddy]

Independent review — confirming Magi's blocker; I reproduced it, with one nuance (it's CFR-only).

The Math.trunc(durationSeconds * 1_000_000) quantization (videoFrameExtractor.ts:106) is not equivalent to FFmpeg parsing the decimal -t string, and it undercounts for clean 2-decimal durations whose float representation lands just below the exact value. Verified numerically for 2.05:

• 2.05 * 1_000_000 === 2049999.9999999998 → Math.trunc → 2049999 → quantized to 2.049999s (one microsecond short).
• FFmpeg parses the string "2.05" → exactly 2050000 µs → 2.050000s.
• At 30 fps: quantized 2.049999 * 30 = 61.49997.
  • VFR path Math.ceil(61.49997) = 62 — matches FFmpeg's 62. So VFR is actually fine here.
  • CFR path Math.round(61.49997) = 61 — FFmpeg emits 62. Off by one.

So the bug is specifically on the CFR nearest-boundary branch: the lost microsecond pushes a value that should land on x.5 (rounds up to 62) down to x.49997 (rounds to 61). And because coverage uses the same helper (extractionFrameCountForDuration), it computes the expected count as 61 too and reports 61/61 — the superset-slice drop of the final frame is masked, exactly as Magi described.

The truncation was clearly intended to match FFmpeg truncating sub-microsecond digits (0.6000009 → 0.600000 → 18, which is correct and I verified holds). The problem is applying trunc to the float product rather than the decimal string: for 0.6000009 the product is 600000.9 (trunc → 600000 ✓), but for 2.05 the product is 2049999.9999998 (trunc → 2049999 ✗). A decimal-string-consistent quantization (truncate the string's fractional part at 6 places, then parse) fixes 2.05 without regressing 0.6000009. Concur with Magi that this needs the string-consistent fix plus a real-FFmpeg 2.05s direct-vs-superset CFR regression (the current suite's boundary cases all happen to have exact float products, so they don't exercise the downward-crossing case).

Everything else in the PR (VFR ceil semantics, the seek-local VFR superset gate, the bounded ±1 short-clip tolerance) looked correct on my read — the quantization is the one load-bearing defect. Blocking until the string-consistent quantization + CFR regression land.

— Somu

[terencecho]

Cross-reference: @magi's changes-requested (review 4849348062) cites a real off-by-one in the CFR frame-count formula. Independently reproducing the arithmetic against the diff at head d626766e:

For duration = 2.05, fps = 30, isVFR = false:

• Math.trunc(2.05 * 1_000_000) = 2049999 (this does match FFmpeg's av_parse_time + AV_TIME_BASE int64 truncation — both strtod("2.05") in C and JS's 2.05 produce the same IEEE-754 double 2.04999999999999982236…, and both truncate to 2049999 µs. The microsecond quantization step is correct.)
• ffmpegDurationSeconds = 2.049999
• rawFrames = 2.049999 * 30 = 61.49997
• Integer tolerance snap: |61.49997 - 61| = 0.49997 ≫ 4·ε·61 ≈ 5.4e-14 → no snap.
• CFR path: Math.round(61.49997) = 61.
• Helper returns 61.

FFmpeg at -t 2.05 -vf fps=30: outputs frames at output timestamps N/30 < 2.049999, so N ∈ {0, 1, …, 61} → 62 frames. That's the standard fps filter contract — the output frame count is the number of output-pts values strictly less than the requested duration.

The bug is in the round-nearest CFR formula, not the quantization step. For any duration where (duration_µs_truncated × fps) mod 1_000_000 < 500_000 (roughly half of all durations at 30fps), Math.round(rawFrames) returns one less than FFmpeg's actual output count. The 0.616666 case in the unit test hits this too: Math.round(18.49998) = 18, but FFmpeg CFR at -t 0.616666 emits 19 frames — the CFR unit test asserts the helper's behavior, not real FFmpeg's, and no real-FFmpeg CFR regression pins it (all the describe.skipIf(!HAS_FFMPEG) fixtures are VFR).

The correct CFR formula is the same as VFR (Math.ceil(quantizedFrames)), or equivalently Math.floor(quantizedFrames) + 1 for non-integer inputs (Math.ceil handles integer inputs correctly since ceil(15.0) = 15 matches FFmpeg's strict-less-than boundary at duration = 0.5s).

Concretely — the fix at videoFrameExtractor.ts:186:

- const frames = isVFR ? Math.ceil(quantizedFrames) : Math.round(quantizedFrames);
+ // FFmpeg's fps filter (CFR) and -fps_mode cfr -r (VFR) both emit frames whose
+ // output pts < duration. Frame count = number of integers N ≥ 0 with
+ // N/fps < duration = ceil(duration * fps) for non-integer products,
+ // or exactly (duration * fps) for integer products.
+ const frames = Math.ceil(quantizedFrames);

That collapses CFR and VFR to the same formula, which matches the physics (both emit an integer number of frames whose timestamps fit strictly inside the requested duration). The isVFR parameter can then be dropped from the helper entirely, since it's no longer semantically meaningful.

Follow-up asks on the tests:

1. Add a real-FFmpeg CFR regression at a microsecond boundary that catches this class (e.g. -t 2.05, fps=30, CFR-mode asserting 62 direct-extracted frames). The existing describe.skipIf(!HAS_FFMPEG) block is VFR-only.
2. The direct-vs-superset invariant test at videoFrameExtractor.test.ts:2167 would also want a CFR-mode variant, since the superset slicing bug is what actually manifests end-user impact — the coverage check uses the same (wrong) helper, so 61/61 passes while the final frame silently drops.

Posture update.

My APPROVE at review 4849340753 was scoped to the microsecond quantization mechanic (which is correct). The CFR round-nearest formula wasn't in my direct verifications — I acknowledged the "unit test asserts CFR=18 for 0.616666 without a real-FFmpeg CFR check" as a small operational nit but didn't run the follow-through arithmetic against FFmpeg's actual boundary contract, which is Magi's contribution here.

Concurring with Magi's changes-requested on the substance. Not formally flipping my approval state since the fix is straightforward and this comment plus Magi's block-review makes the state clear on the PR, but treat Magi's review as the load-bearing signal.

— tai

[terencecho]

Correction to my earlier follow-up: after Somu (Somansh Reddy)'s counterargument — the "fix CFR to Math.ceil" recommendation was overconfident. Two theories are consistent with the empirical 2.05 → 62 case that Magi reproduced; they diverge at other durations. The tiebreaker is a real-FFmpeg CFR run at a non-half-integer boundary, which nobody has done yet.

Theory A — my proposal in the earlier comment

Quantization is right (Math.trunc matches FFmpeg's strtod → * 1e6 → int64_t cast, which C does as truncation-toward-zero). CFR formula is wrong — should be Math.ceil (equivalent to Math.floor(rawFrames) + 1 for non-integer). This matches my reading of FFmpeg's fps filter output count as "number of integers N ≥ 0 with N/fps < duration".

Predictions:

• 2.05, 30, CFR → helper: 62 ✓ (matches Magi's empirical)
• 0.616666, 30, CFR → helper: 19 (PR asserts 18 — but that assertion is derived from the current helper, not real FFmpeg)
• 0.6000009, 30, VFR → helper: 18 ✓ (unchanged, VFR was already ceil)

Theory B — Somu's counterargument

CFR round-nearest is right. Quantization is wrong — the current Math.trunc(duration * 1e6) diverges from FFmpeg's decimal-string parsing because of IEEE-754 float noise (2.05 * 1e6 = 2049999.9999999998). Under a decimal-string-consistent quantization (equivalent to 6-digit-truncation of the string form of duration):

• "2.05" → padded 6-digit → 2050000 µs
• "0.616666" → 616666 µs
• "0.6000009" → 7 digits truncates at 6 → 600000 µs
• "0.600001" → 600001 µs

Predictions:

• 2.05, 30, CFR → 61.5 * round = 62 ✓ (matches Magi's empirical, since JS Math.round(61.5) = 62)
• 0.616666, 30, CFR → 18.49998 * round = 18 (matches PR assertion)
• 0.6000009, 30, VFR → 600000/1e6 * 30 = 18 * ceil = 18 ✓ (matches PR test)
• 0.600001, 30, VFR → 18.00003 * ceil = 19 ✓ (matches PR test)

The tiebreaker

Both theories are internally consistent AND both match the one hard empirical result we have (Magi's 2.05 → 62). They differ on:

Duration	fps	Theory A predicts	Theory B predicts	PR-asserted	Real FFmpeg?
0.616666	30 CFR	19	18	18	UNVERIFIED
0.466666	30 CFR	14	14	14	UNVERIFIED
4.03−3.53 = 0.5	30 CFR	15	15	15	UNVERIFIED
0.6000009	30 CFR	n/a (VFR)	n/a (VFR)	n/a (VFR)	n/a

The critical case is 0.616666 CFR: Theory A predicts 19, Theory B predicts 18, the PR-asserted 18 is derived from the current helper's round-nearest output (not from real FFmpeg — the describe.skipIf(!HAS_FFMPEG) block only exercises VFR mode). One ffmpeg -i vfr.mp4 -vf fps=30 -t 0.616666 out%d.png ; ls out*.png | wc -l on the actual production-shaped command would settle it definitively.

Implementation shapes for each theory

Theory A (drop isVFR param entirely):

- const frames = isVFR ? Math.ceil(quantizedFrames) : Math.round(quantizedFrames);
+ // FFmpeg's fps filter (CFR path) and -fps_mode cfr -r (VFR path) both emit
+ // frames whose output pts < duration → count = floor(duration * fps) + 1
+ // for non-integer product, or exactly (duration * fps) for integer.
+ const frames = Math.ceil(quantizedFrames);

Theory B (decimal-string-consistent quantization, keep CFR/VFR split):

- const ffmpegDurationSeconds = Math.trunc(durationSeconds * 1_000_000) / 1_000_000;
+ // FFmpeg parses -t via strtod-then-round-to-nearest-microsecond at the
+ // decimal string level. Use toFixed(6)+parse rather than float multiply
+ // to avoid IEEE-754 subtraction/multiplication noise (e.g. "2.05" as a
+ // float * 1e6 = 2049999.9999999998, truncating to 2049999 µs when the
+ // decimal-consistent parse yields 2050000).
+ const microseconds = Math.trunc(Number(durationSeconds.toFixed(6)) * 1_000_000);
+ // If the caller passed a value whose displayed form is >6 fractional digits,
+ // toFixed rounds — matches FFmpeg's parse behavior for over-precision inputs.
+ const ffmpegDurationSeconds = microseconds / 1_000_000;

Note that (0.6000009).toFixed(6) = "0.600001" — JS's toFixed rounds, not truncates. That would predict 600001 µs for 0.6000009, breaking the PR's VFR=18 pin. If Theory B is right, the quantization needs actual decimal-string 6-digit truncation, not toFixed(6):

function decimalStringMicroseconds(seconds: number): number {
  const parts = String(seconds).split(".");
  const intPart = parseInt(parts[0], 10);
  const fracPart = (parts[1] ?? "").padEnd(6, "0").slice(0, 6);
  return intPart * 1_000_000 + parseInt(fracPart, 10) * (Math.sign(seconds) || 1);
}

This gets "2.05" → 2050000, "0.616666" → 616666, "0.6000009" → 600000, "0.600001" → 600001. Handles the JS-float-round-doesn't-truncate issue.

Recommendation

Neither theory should ship without a real-FFmpeg CFR regression that pins the empirical output. Concretely, add:

it.skipIf(!HAS_FFMPEG)("emits N frames for -t 0.616666 -vf fps=30 on a CFR source", async () => {
  const CFR_FIXTURE = await synthCfrClip("cfr-fixture.mp4", 5);
  const result = await extractAllVideoFrames(
    [cfrClipElement("cfr-boundary", CFR_FIXTURE, 0.616666, 0)],
    FIXTURE_DIR,
    { fps: 30, outputDir: /*...*/ },
  );
  // Whatever FFmpeg actually emits — 18 or 19 — codify it.
  expect(extractedFor(result, "cfr-boundary").totalFrames).toBe(???);
}, 60_000);

Once that runs, the correct fix is unambiguous. Deferring to <@jrusso1020> to run this locally and pick the theory.

— tai

[miguel-heygen]

Follow-up to my existing CHANGES_REQUESTED at unchanged head d626766e: I ran the disputed CFR cases through both FFmpeg 4.2.7 and the repo’s packaged ffmpeg-static 7.0.2 using the production-shaped -ss 0 -i input -t <duration> -vf fps=30 pipeline. Both versions emit:

• 0.616666s → 18 frames
• 2.05s → 62 frames
• 0.6000009s → 18 frames
• 0.600001s → 18 frames

The current helper predicts 18, 61, 18, 18 respectively. This settles the round-vs-ceil question: keep CFR round-nearest; fix decimal-to-microsecond quantization so the helper consumes the same decimal duration FFmpeg receives. Switching CFR to ceil would regress 0.616666s to 19. Please add real-FFmpeg CFR regressions for both 0.616666s and 2.05s so neither theory can regress silently.

[jrusso1020]

Review feedback addressed in 493358e7e.

• Decimal-string microsecond parsing now matches the exact -t value sent to FFmpeg without binary multiply-before-truncate.
• Frame-boundary rounding stays rational, so 2.05s × 30fps correctly yields 62 CFR frames.
• Added real-FFmpeg direct-vs-superset regressions for 2.05s → 62 and 0.616666s → 18, with byte-identical frame assertions.

Local validation: 123/123 extractor tests, engine typecheck, lint/format, and pre-commit audit all green. @miguel-heygen @somanshreddy @vanceingalls @terencecho fresh review welcome.

[somanshreddy]

Re-review @ 493358e7 — fix verified correct + complete. LGTM.

The quantization is now decimal-string-consistent, and you also closed the second-order float error I'd missed:

• Exact microseconds from the decimal text: String(durationSeconds) → regex whole/fraction/exponent → BigInt microseconds. For 2.05: "2.05" → 205n * 10^(6-2) = 2050000 exactly (vs the old Math.trunc(2.05*1e6)=2049999). ✓
• Rational frame-boundary math (BigInt fps numerator/denominator): necessary because even with exact 2050000µs, 2.05 * 30 floats to 61.49999999999999 and would round back to 61. Keeping it rational makes round(61.5)=62. Good catch — I only flagged the quantization half.
• CFR stays round-nearest, so 0.616666 CFR → 18 is preserved and 2.05 CFR → 62 is fixed; VFR 0.6000009 → 18 truncation still holds.
• Real-FFmpeg CFR regressions added (2.05→62, 0.616666→18) with direct-vs-superset byte equality — that was the actual root gap (the suite had zero real-FFmpeg CFR coverage at a non-half-integer boundary). ✓

Blocker resolved on my side. LGTM pending CI green on the fresh head.

— Somu

[vanceingalls]

Verdict: APPROVE at 493358e7e.

Reversing my R1 stamp — Miguel + Somu + Terence caught a real off-by-one at CFR-30fps for durations where String(d) * 1e6 binary-underflows the intended microsecond boundary (String(2.05) === "2.05" but 2.05 * 1e6 === 2049999.9999999998). I ran my R1 REPL against every case in the PR body plus generic edges (NaN/-0/1e-7/FP-subtraction), but did not enumerate the "clean 2-decimal string whose float product lands just below the intended integer µs boundary" class. This class turns out to affect roughly half of 2-decimal durations at 30fps (0.05, 0.15, 0.25, 0.35, 0.45, 1.05, 1.15, …, 2.05, 3.15, 4.05, …). Codified as an addendum to my feedback_re_execute_pure_numeric_functions_to_verify_boundary_claims memory so future numeric-boundary reviews cover it.

What R2 does

Rewrites extractionFrameCountForDuration (videoFrameExtractor.ts:93-134) to derive µs directly from String(durationSeconds) decimal parsing rather than Math.trunc(d * 1e6):

const decimal = /^(\d+)(?:\.(\d+))?(?:e([+-]?\d+))?$/.exec(serialized);
const digits = BigInt(whole + fraction);
const microsecondScale = exponent + 6 - fraction.length;
const microseconds = scale >= 0 ? digits * 10n**scale : digits / 10n**(-scale);

Then computes the frame count as a rational BigInt fraction (µs × fpsNumerator) / (1_000_000 × fpsDenominator), applying CFR round-half-up as (2n + denom) / (2n * denom) and VFR ceil as (n + denom - 1n) / denom. The float product step is eliminated entirely; the µs value matches whatever FFmpeg's decimal-string parser (av_parse_time reads the string form of -t) resolves to.

Adversarial re-verification (this time actually running the code)

Re-implemented the new function in a Node REPL and ran 32 cases including every previously-missed cell. All pass:

case	fps	mode	got	expect	source
2.05	30	CFR	62	62	Miguel empirical (real FFmpeg 4.2.7 + 7.0.2)
0.616666	30	CFR	18	18	Miguel empirical
0.6000009	30	CFR	18	18	Miguel empirical
0.600001	30	CFR	18	18	Miguel empirical
0.6000009	30	VFR	18	18	PR test
0.600001	30	VFR	19	19	PR test
2.05	30	VFR	62	62	derived (61.5 ceil)
4.03-3.53	30	CFR	15	15	FP-subtraction scrub
4.03-3.78	24	CFR	6	6	FP-subtraction
0.33-0.03	30	CFR	9	9	FP-subtraction
0.29-0.04	24	CFR	6	6	FP-subtraction
0.300001	30	VFR	10	10	genuine-fractional
0.001	30	CFR	1	1	positive sub-frame floor
0 / -1 / NaN / Infinity / fps=0	—	—	0	0	fail-closed
3.15	30	CFR	95	95 (=3.15×30=94.5 CFR round-up)	float-underflow class, previously missed
1.05	30	CFR	32	32 (=31.5 round-up)	float-underflow class
4.05	30	CFR	122	122 (=121.5 round-up)	float-underflow class
0.05	30	CFR	2	2 (=1.5 round-up)	float-underflow class
0.1 + 0.2 (=0.30000000000000004)	30	CFR	9	9	classic FP
1e-7	30	CFR	1	1	sub-µs → 0 → Math.max floor
1e-300	30	CFR	1	1	denormalized positive
1s @ 29.97fps	—	CFR	30	30	rational fps preserves precision
1s @ 23.976fps	—	CFR	24	24	rational fps
100s @ 30fps	—	CFR	3000	3000	large integer
1e-6 (1µs)	30	CFR	1	1	positive µs → floor 0 → Math.max 1

The BigInt approach also correctly handles fractional-fps rationals: 29.97 becomes 2997n / 100n (not the binary-underflow-prone 29.97 double), so 1s * 29.97fps = 2997/100 = 29.97 frames exactly → CFR round = 30.

Test coverage additions

• Unit oracle at videoFrameExtractor.test.ts:340: extractionFrameCountForDuration(2.05, 30, false) === 62 — the exact case that broke at R1.
• Real-FFmpeg integration at videoFrameExtractor.test.ts:2107: it.each with two labels — "decimal underflow half-frame" (2.05 @ offset 1) and "non-half-integer boundary" (0.616666 @ offset 0.1). Each asserts direct-vs-superset byte equality frame-by-frame via readFileSync(...).equals(...), plus supersetDirNames === []. Byte-equality is the strongest form of the invariant — mutation of the CFR formula that drops the final frame fails this check on the last iteration.

Edge-cases I checked in the new impl

• Regex fails on Infinity / NaN / negative: caught by Number.isFinite + <= 0 gate before the regex.
• Regex handles String(2.05e21) === "2.05e+21": Number.parseInt("+21") returns 21, decimal parses correctly.
• Regex handles denormalized: String(5e-324) === "5e-324", scale = -324 + 6 - 0 = -318, µs = 5n / 10^318 = 0n.
• BigInt overflow: Number(frameCount) guarded by Number.isSafeInteger, falls back to MAX_SAFE_INTEGER (astronomical, not a real concern).
• No leading-zero octal traps: BigInt accepts "0616666" as literal decimal (BigInt does not implement C-style octal).

Nit (non-blocker, not gating merge)

• Line 132: Number.isSafeInteger(frames) ? frames : Number.MAX_SAFE_INTEGER. For BigInts > 2^53, Number() returns a rounded double which still passes isSafeInteger for adjacent-to-2^53 values. In practice unreachable — 3000 frames/sec × 10^13 seconds video. Ignore.

CI green pending. All required checks in-progress at the time of this review.

— Review by Via

[miguel-heygen]

Re-reviewed at exact head 493358e7eab307aceb81b416a143bb89c836bc5a. The decimal-string duration fix and the new integer-30fps real-FFmpeg regressions are correct, but one production frame-rate boundary remains open.

P1 — preserve FFmpeg's rational FPS instead of reconstructing it from String(fps). countExtractedFrames() parses the JS number as an exact decimal rational. FFmpeg canonicalizes standard NTSC rates to rationals such as 30000/1001 and 24000/1001, so the helper diverges at real boundaries.

Reproduced with this repo's packaged FFmpeg 7.0.2:

• CFR 29.97002997002997, duration 0.25025: FFmpeg emits 8 frames; the helper returns 7. An aligned superset contains all 8 frames, but sliceSupersetMember() copies only 7, silently dropping the last frame.
• VFR 23.976023976023978, duration 0.125125: FFmpeg emits 3 frames; the helper expects 4. At 0.5005, FFmpeg emits 12 while the helper expects 13, which can falsely fail coverage outside the short-clip tolerance.

The core already preserves FPS as {num, den} and serializes that rational for FFmpeg, but extraction collapses it through fpsToNumber; coverage then shares the same incorrect helper. Please carry the configured rational into extraction/counting (or otherwise mirror FFmpeg's canonical rationalization), and add real-FFmpeg CFR 30000/1001 direct-vs-superset plus VFR 24000/1001 coverage regressions.

The prior 2.05 @ 30fps issue is genuinely fixed; this request is only for the remaining non-integer production FPS path.

[terencecho]

Concurring with Somu's LGTM at 493358e7. Independent verifications on the parts the previous review was hung on:

Theory A removed at BOTH consumer sites

The prior review's blocker was Math.trunc(duration * 1_000_000) in the extractor — that's the IEEE-754 hazard (2.05 * 1e6 = 2049999.9999… → 2049999 μs → CFR nearest 61). This diff replaces the trunc-based path at the extractor AND the coverage helper:

• packages/engine/src/services/videoFrameExtractor.ts:sliceSupersetMember — - const requestedFrames = Math.round(work.videoDuration * fps); → routed through extractionFrameCountForDuration(duration, fps, isVFR).
• packages/producer/src/services/render/videoFrameCoverage.ts:expectedFramesForClip — - rounding === "nearest" ? Math.round(duration * fps) : Math.ceil(duration * fps); → same helper.

Both consumer paths now derive frame count from the exact decimal-string microseconds + rational fps arithmetic. The surviving Math.rounds in the file are in scopes outside the duration→frames contract (JPG quality mapping at line 694, integrality-tolerance check at 1142, offsetFrames from a pre-filtered integral offset at 1182).

Real-FFmpeg regression test is a strong triple

videoFrameExtractor.test.ts:2107-2131 (both entries in the it.each — "non-half-integer boundary" at 0.616666s → 18 and "decimal underflow half-frame" at 2.05s → 62) asserts:

1. totalFrames === expectedFrames for the direct path AND both grouped members.
2. readFileSync(supersetFrame).equals(readFileSync(directFrame)) — byte identity on every frame.
3. statSync(supersetFrame).ino === statSync(directFrame).ino — hardlink sharing preserved (proves the superset dir isn't silently rebuilding).
4. supersetDirNames(outputDir) === [] — no leaked superset dir.

The inode + no-leak assertions are what pin the direct-vs-superset semantic equivalence beyond just "bytes match" — a future regression that quietly falls back from the superset optimization would show byte-identical output but different inodes, and this test catches that.

Merge-flow note

Magi's CHANGES_REQUESTED from prior SHA d626766e is still in the "changes requested" state on the PR. On GitHub, that state remains blocking the merge button until Magi re-reviews at 493358e7 or dismisses it. Not a code blocker — but worth a ping since the underlying Math.trunc complaint is empirically refuted here.

Non-blocking

• The new short-clip 1-frame tolerance in expectedFramesForClip (14 ≤ expected ≤ 20, exactly 1 missing, threshold<1, captured>0) is a real behavior change, not just cleanup. Narrowly scoped, with negative tests pinning fail-closed on adjacent-state (0 captured, 2 missing, threshold=1, 21-frame clip, 84/89 material shortfall). Justified by the empirical FFmpeg boundary-frame drop at very short clips — acceptable, but worth naming in the merge log.
• The real-FFmpeg regressions live behind skipIf(!HAS_FFMPEG). Codebase-consistent with the pre-existing VFR extraction suite, and the unit pins at videoFrameExtractor.test.ts:305,326 still guard the invariant. Not a green-by-skip risk on the engine Test lane once it lands, since ffmpeg is installed there — worth a quick check of the log for ↓ markers after it goes green.
• PR body says "Local: 123/123" but the touched suites total 121 engine + 36 producer = 157. Minor paraphrase inconsistency; not a code issue.

— Review by tai (pr-review)

[somanshreddy]

Correcting my prior "fix complete" — concurring with Magi's NTSC finding, which I independently verified.

The String(fps) reconstruction is decimal-string-consistent for the duration, but not for NTSC rational frame rates, because String(29.97002997002997) is a truncated decimal, not the exact 30000/1001. I reproduced both of Magi's cases with exact rational arithmetic:

• CFR 30000/1001 @ 0.25025s: exact = 250250/1e6 * 30000/1001 = 7.5 precisely → FFmpeg round = 8. The decimal fps is just below the true rational, so the helper computes 7.4999… → round → 7. Undercount; superset slice drops the last frame.
• VFR 24000/1001 @ 0.125125s: exact = 3.0 precisely → FFmpeg ceil = 3. The decimal fps is just above, so the helper computes 3.0000000000000004 → ceil → 4. Overcount; coverage expects a frame FFmpeg never emits.

So the decimal-string parse fixed the duration-µs half but re-introduced the same class of error one layer down, on the rate. The right fix is Magi's: carry the exact {num, den} rate (which ffprobe already reports as r_frame_rate = 30000/1001) through counting and extraction, never round-tripping the rate through a float or its decimal string. Add real-FFmpeg regressions at 30000/1001 and 24000/1001 (the .25025 / .125125 boundaries are good pins).

Withdrawing my "correct + complete" — this is a genuine remaining blocker. Good catch, Magi.

— Somu

[jrusso1020]

Addressed the exact-rate blocker at 7a0203bc2. The extractor now accepts the existing FpsInput, emits exact rational FFmpeg arguments (30000/1001 / 24000/1001), and carries the same rational into superset slicing and producer coverage accounting instead of reconstructing it from String(fpsToNumber(...)). Added real FFmpeg regressions for direct-vs-superset CFR at 30000/1001 (0.25025s -> 8) and VFR normalization at 24000/1001 (0.125125s -> 3), plus coverage regressions at 0.125125s and 0.5005s. Verification: engine extractor 126/126; producer Vitest 498/498; targeted producer 52/52; engine and producer typechecks; test classification; repository lint and format all green. Requesting fresh review on this exact head.

[somanshreddy]

Approving @ 7a0203bc2. The NTSC fix is exactly right: fps is now carried as exact {num, den} — no String(fps)/fpsToNumber round-trip — so the rational rate stays exact through counting, superset slicing, and coverage. Verified the two cases I reproduced earlier now pass (0.25025 @ 30000/1001 CFR → 8, 0.125125 @ 24000/1001 VFR → 3), and the real-FFmpeg direct-vs-superset regressions at those rates exercise the actual extraction path end-to-end, so they'd catch any residual lossy reconstruction at a call site — that's what makes me confident the threading is complete, not just the unit oracle. Good fix; thanks for the thorough turnaround on both the 2.05 and NTSC classes. — Somu

[terencecho]

Delta re-review at 7a0203bc2 vs 493358e7. NTSC follow-up correctly carries exact {num, den} through every layer without a decimal round-trip. Approving.

Delta scope

Six files, +149/-41:

• videoFrameExtractor.ts (+59/-25) — core: extractionFrameCountForDuration accepts FpsInput, extractVideoFramesRange / extractAllVideoFrames normalize via toFps, FFmpeg args use fpsToFfmpegArg, sliceSupersetMember carries configuredFps alongside fps
• videoFrameExtractor.test.ts (+60) — unit + real-FFmpeg NTSC regressions
• videoFrameCoverage.ts (+7/-5) — expectedFramesForClip and computeVideoFrameCoverage accept FpsInput
• videoFrameCoverage.test.ts (+18) — producer-side NTSC coverage tests
• extractVideosStage.ts (+4/-6) — producer no longer round-trips fpsToNumber(job.config.fps)
• renderOrchestrator.ts (+1/-5) — same round-trip removed

Rational arithmetic verified

The BigInt path in extractionFrameCountForDuration for FpsInput as object:

fpsNumerator = BigInt(fps.num)
fpsDenominator = BigInt(fps.den)

No String(fps) decimal parse for the object path — the rational is preserved exactly. Sanity-checked the three pinned unit values by hand:

• 0.25025s = 1001/4000 × 30000/1001 = 30000/4000 = 7.5 → CFR nearest 8 ✓
• 0.125125s = 1001/8000 × 24000/1001 = 24000/8000 = 3.0 → VFR ceil 3 ✓
• 0.5005s = 1001/2000 × 24000/1001 = 24000/2000 = 12.0 → VFR ceil 12 ✓

If NTSC had gone through the number-path (String(30000/1001) → decimal-string parse), JS's String(30000/1001) === "29.97002997002997" — that's a 14-digit truncation of the infinite 29.970029970029970... recurring decimal, so the BigInt-quantized frame count would drift by up to 1 frame at boundary cases like these. Bypassing the round-trip is the correct fix.

FFmpeg-side symmetry

fpsToFfmpegArg(configuredFps) produces 30000/1001-style rational strings, which FFmpeg accepts verbatim in -r and fps= filter args. Both consumer sites use it:

• vfFilters.push(\fps=${ffmpegFps}`)atvideoFrameExtractor.ts:706` (CFR path)
• args.push("-fps_mode", "cfr", "-r", ffmpegFps) at videoFrameExtractor.ts:720 (VFR-to-CFR path)

So FFmpeg is receiving exactly the rate the frame-count derivation assumed. No round-trip disagreement.

Cache key correctness

Dedupe key at videoFrameExtractor.ts:1904 now uses fpsKey = fpsToFfmpegArg(configuredFps) instead of options.fps. Load-bearing: {num:30000, den:1001} and 29.97 (or 30) must NOT collide in the cache, and this fix ensures they don't — future runs at the same rate hit the same cache entry, and rate changes bust it.

Real-FFmpeg regressions

At videoFrameExtractor.test.ts:2181 — direct-vs-superset at {30000, 1001} on 0.25025s:

• direct → 8 frames
• superset member of a 15-frame base clip → 8 frames as member
• readFileSync(memberFrame).equals(readFileSync(directFrame)) for all 8 frames (byte identity)
• statSync(memberFrame).ino === statSync(baseFrame).ino (hardlink sharing preserved)
• supersetDirNames(outputDir) === [] (no leaked superset dir)

Same triple pin from the R2 review, now extended to NTSC. Also extractVideoFramesRange VFR pin at line 1567 exercises the full extraction path (not just the helper) for {24000, 1001} on 0.125125s → 3 frames.

Producer coverage

expectedFramesForClip and computeVideoFrameCoverage accept FpsInput and forward the ORIGINAL rational to extractionFrameCountForDuration (only extracting fpsToNumber(fps) for the finite-check guard). The assertVideoFrameCoverage integration test pins that a {24000, 1001} VFR clip captured at 3/3 frames yields ratio: 1 and doesn't throw — closes the round-trip regression at both the engine and the coverage-check layer.

sliceSupersetMember two-parameter API

Now takes BOTH fps: number (for internal offset-alignment math) AND configuredFps: FpsInput (for extractionFrameCountForDuration). Non-blocking observation: the two parameters must stay in sync — a future caller that passes configuredFps: {30000,1001} but fps: 29.97 would create a rate mismatch between the offset alignment (in planSupersetGroups, called with fps at line 1944) and the frame-count derivation. Both callers in this diff derive both from the same configuredFps = toFps(options.fps), so the mismatch is currently impossible, but a small local invariant to add later.

CI status

All completed lanes green at review time: Producer unit tests, Preflight (lint + format), Lint, Fallow audit, SDK, Semantic PR title, Studio load smoke, Analyze (actions). Engine Test, Typecheck, Build, Producer: integration tests, all windows lanes, all regression-shards still pending. No red at review time.

Non-blocking

• Comment at videoFrameExtractor.ts:139 ("Number-only callers retain their decimal FFmpeg argument exactly. The production render path supplies Fps, so NTSC rates never round-trip through String(30000 / 1001) here.") is exactly the invariant a future refactor needs to see. Keep it.
• The number-path decimal-string parsing (retained for backward compat with number-only callers) is unchanged from R2. Since Theory B was empirically validated at 2.05s → 62, that branch remains correct for its callers.
• Non-scope: still worth clearing Magi's stale CHANGES_REQUESTED at d626766e — that state still nominally sits on the PR page, and while my/Via's/Somu's fresh approvals accumulate, a fresh reviewer glancing at the review timeline might mistake the old state for current.

— Review by tai (pr-review)

[vanceingalls]

Verdict: APPROVE at 7a0203bc2.

Reversing my R2 APPROVE at 493358e7e. Magi and Somu caught the second layer of the same class of bug: String(fps) cannot recover an exact rational when the caller's Number is itself a lossy encoding of one. This is the specific NTSC failure mode:

• String(30000/1001) === "29.97002997002997" — 17 decimals of shortest-round-trip, not the exact rational.
• My R2 quantization parses that string to 2997002997002997n / 10^14n — approximately but NOT equal to 30000n / 1001n.
• At small durations the discrepancy flips the frame count: 0.25025s @ 30000/1001 CFR is EXACTLY 7.5 frames → round → 8 (what real FFmpeg emits). 0.25025s @ 29.97002997002997 decimal ≈ 7.4999... → round → 7. Off-by-one, silently drops the last frame at the superset slice.

My R2 REPL ran 29.97 and 23.976 and got apparent parity because those were already-truncated decimal approximations. The production code path receives ffprobe's exact r_frame_rate = "30000/1001", so the truncated decimal never crosses the wire — but any Number-based intermediate collapses it. I've amended my feedback_re_execute_pure_numeric_functions_to_verify_boundary_claims memory with the "lossy encoding of an exact rational from outside" class so future numeric-boundary reviews catch it.

What R3 does

Extends extractionFrameCountForDuration(duration, fps, isVFR) to accept FpsInput = number | {num: number, den: number} (typed through @hyperframes/core). The rational path:

if (typeof fps === "object") {
  if (!Number.isSafeInteger(fps.num) || !Number.isSafeInteger(fps.den) || fps.num <= 0 || fps.den <= 0) return 0;
  fpsNumerator = BigInt(fps.num);
  fpsDenominator = BigInt(fps.den);
}

skips the String(fps) parse entirely. Number-path callers retain the R2 decimal-string quantization (backwards-compat for integer fps like 30).

Threaded through the production stack:

• extractVideoFramesRange — normalizedFps = toFps(options.fps), uses fpsToFfmpegArg(normalizedFps) in -vf fps=... filter AND -r <arg> VFR mode. FFmpeg receives 30000/1001 verbatim, not a decimal approximation.
• sliceSupersetMember — takes an additional configuredFps: FpsInput and passes it to extractionFrameCountForDuration. The old fps: number parameter is retained for other math (frame offsets, filenames) so no signature churn beyond the one added param.
• extractAllVideoFrames — computes configuredFps + fps + fpsKey; passes configuredFps to sliceSupersetMember, uses fpsKey (the exact rational string) in the dedupeKey so 29.97 vs 30000/1001 don't collide in the cache.
• expectedFramesForClip + expectedFramesForVideo + computeVideoFrameCoverage — all accept FpsInput. Producer coverage now consumes the true rational.
• extractVideosStage — no longer collapses through fpsToNumber(job.config.fps); passes the rational directly.
• renderOrchestrator — same, passes job.config.fps directly to computeVideoFrameCoverage.

The Fps rational is preserved end-to-end from job.config.fps (which core already stores as {num, den}) through extraction, superset slicing, coverage counting, and FFmpeg's argument encoding.

Adversarial re-verification (this time actually running it with rationals)

Re-implemented the new function in Node and ran 24 cases including every disputed cell:

case	fps	mode	got	expect	source
0.25025	30000/1001	CFR	8	8	Magi empirical real-FFmpeg
0.125125	24000/1001	VFR	3	3	Magi empirical
0.5005	24000/1001	VFR	12	12	PR test
0.25025	29.97002997002997 (Number)	CFR	7	(R2 was wrong here)	probe — confirms the bug
0.125125	23.976023976023978 (Number)	VFR	4	(R2 was wrong here)	probe — confirms the bug
2.05	30 (Number)	CFR	62	62	backwards-compat integer path
2.05	{num:30, den:1}	CFR	62	62	rational form of integer 30fps
1	30000/1001	CFR	30	30	29.97 @ 1s CFR → round = 30
1	60000/1001	CFR	60	60	60p NTSC
1	{num:25, den:1}	CFR	25	25	PAL 25fps
1	{num:24, den:1}	VFR	24	24	film 24fps
0.033367	30000/1001	VFR	2	2	just over 1 frame period
0.001	30000/1001	CFR	1	1	positive sub-frame → Math.max floor
1	{num:0, den:1}	CFR	0	0	fail-closed on num=0
1	{num:30, den:0}	CFR	0	0	fail-closed on den=0
1	{num:-30, den:1}	CFR	0	0	fail-closed on negative
1	{num:30.5, den:1}	CFR	0	0	fail-closed on non-safe-integer
1	{num:NaN, den:1}	CFR	0	0	fail-closed on NaN

Plus the previously-verified R2 cases (2.05, 3.15, 1.05, 4.05, 0.05 float-underflow class, (4.03-3.53) FP-subtraction, 0.6000009/0.600001 boundary, NaN/negative/zero/Infinity fail-closed) — all still pass on the Number path.

Test-side additions

• Unit oracle at videoFrameExtractor.test.ts:342 pinning three NTSC cells: (0.25025, {30000,1001}, CFR) → 8, (0.125125, {24000,1001}, VFR) → 3, (0.5005, {24000,1001}, VFR) → 12.
• Real-FFmpeg VFR at videoFrameExtractor.test.ts:1570: passes {num: 24000, den: 1001} through extractVideoFramesRange, asserts metadata.isVFR === true && totalFrames === 3 at 0.125125s. Exercises the FFmpeg -r-arg rational preservation end-to-end.
• Real-FFmpeg CFR direct-vs-superset at videoFrameExtractor.test.ts:2181: uses {num: 30000, den: 1001} fps, 0.25025s member and 0.5005s base, asserts byte-identical frames + shared inode + no leaked superset dir. Exact 2.05 → 62 byte-equality assertion pattern applied to the NTSC boundary.
• Coverage unit tests at videoFrameCoverage.test.ts:98/184 pin both expectedFramesForClip and computeVideoFrameCoverage NTSC behavior; the latter verifies the assertion path (assertVideoFrameCoverage doesn't throw at ratio=1).

Edge-cases I checked in the R3 impl

• Rational {num, den} with num or den outside Number.isSafeInteger (e.g. 2^53): returns 0. Correct — anything larger is a spec violation from the caller.
• Rational {num: 30, den: 1} (canonical integer form): identical output to 30 (Number). Rational-form callers migrating from Number get no behavioral change on integer rates.
• fpsToNumber(normalizedFps) still used for internal math like partialCacheEntryDir fps key and rehydrateCacheEntry — those don't participate in FFmpeg boundary counting, just cache identity. Using the decimal form there is fine as long as it's consistent within a version (which it is — Number-path is stable per JS spec).
• fpsToFfmpegArg (from @hyperframes/core) — the string sent to FFmpeg. Not verified in this diff, but assumed to produce "30000/1001" for rational and "30" for integer. If it were to collapse to decimal internally, we'd re-introduce the bug on FFmpeg's side — but Magi's empirical confirmation that the fresh head produces 8/3 frames rules that out.
• The fpsKey in the dedupeKey uses fpsToFfmpegArg(configuredFps) (rational string), so {29.97, den:1} (spec-illegal, non-integer num) can't collide with {num:30000, den:1001}. Correct cache-scoping.

Nit (non-blocker)

• sliceSupersetMember(member, superset, outputDir, fps, configuredFps) now takes both fps: number and configuredFps: FpsInput. The fps parameter is used only for member.offsetFrames derivation and partial-cache filename math — anywhere that consumes it as a plain Number (not a rational boundary count) is preserved. Slight signature drift but self-documenting by the parameter names. Not worth churning further.

CI green pending — my last blocker on this one.

— Review by Via

[miguel-heygen]

P1: the exact rational now reaches FFmpeg and frame counting, but the persistent extraction cache identity still collapses it back to a decimal. extractAllVideoFrames builds exact fpsKey for the in-render dedupe key, then calls lookupCacheEntry with fps: fpsToNumber(configuredFps) (videoFrameExtractor.ts:1682-1691); CacheKeyInput.fps and the canonical blob remain numeric (extractionCache.ts:73-92, 131-150), with the existing v3 schema prefix. This makes exact {30000,1001} collide with numeric 29.97002997002997, even though their FFmpeg arguments and boundary behavior differ. More importantly, a warm v3 entry produced before this fix can be reused by the exact-rational path: the canonical 24000/1001 @ 0.125125s case previously produced 4 frames on the decimal path but now correctly produces 3, so stale cache reuse defeats the fix (and coverage can miss an overfilled cache because ratio > 1 passes). Please carry the exact FPS identity/FFmpeg arg into the persistent cache key and invalidate the old namespace (or otherwise prove old entries cannot be reused), with a warm-cache regression covering the rational-vs-decimal collision. The direct rational threading and new FFmpeg boundary tests otherwise look correct.

[somanshreddy]

Superseding my earlier approval — Magi's persistent-cache finding is a real blocker, and I verified it. My approval was premature.

The rational threading through FFmpeg / slicing / coverage is correct, but the persistent extraction cache defeats it:

• lookupCacheEntry(cacheRootDir, { …, fps, … }) (videoFrameExtractor.ts:1682) is passed fps = fpsToNumber(configuredFps) (:1382) — the numeric rate, not the exact {num,den}. CacheKeyInput.fps is number, and canonicalKeyBlob folds input.fps into the v3 SHA (extractionCache.ts:148). (The in-process dedupe key at :1904 does use the exact fpsKey — but that's not the persistent key.)
• Collision: {30000,1001} and a decimal 29.97002997002997 both collapse to the same float → identical persistent key.
• Warm-cache staleness (the real bite): a pre-fix v3 entry for 24000/1001 @ 0.125125s was written as 4 frames (the bug) under fps=23.976023976023978. The fixed code computes fpsToNumber({24000,1001}) = the same float → same canonical key → hits the stale 4-frame bundle instead of extracting the correct 3, and coverage accepts the overfill. SCHEMA_PREFIX is still hfcache-v3-, so the semantic change (NTSC clips now extract a different frame count) doesn't invalidate warm entries.

Fix per Magi: carry exact FPS identity into the persistent key (e.g. fpsToFfmpegArg/{num,den} rather than fpsToNumber), and bump/namespace-invalidate the old cache (or prove isolation), with a warm-cache-collision regression pinning the 24000/1001 @ 0.125125 old-4-vs-new-3 case.

I checked the computation and the in-process dedupe key but missed the persistent key — my mistake; retracting the approval. Everything else in the rational threading looks sound.

— Somu

[jrusso1020]

Addressed the persistent-cache P1 at exact head f0fee0b6e. CacheKeyInput.fps is now the exact FFmpeg argument string, so 30000/1001 and 29.97002997002997 cannot collide. The cache namespace is bumped from hfcache-v3- to hfcache-v4-, making every pre-fix numeric-FPS entry inert. Added regressions proving (1) rational and decimal keys differ, (2) a complete v3 entry is not served by v4 lookup, and (3) a warm decimal-rate VFR cache containing a simulated stale fourth boundary frame is not reused by the exact 24000/1001 path, which re-extracts three frames into a separate entry. Verification: 158/158 engine extractor/cache tests, engine typecheck, repository lint, format, hooks, and fallow gate green. Requesting fresh review on this exact head.

[somanshreddy]

Re-approving @ f0fee0b6e — the cache P1 (my request-changes item) is fully closed, and this time I traced the persistent-key layer specifically:

1. CacheKeyInput.fps is now string (exact FFmpeg rate arg, e.g. 30000/1001), and lookupCacheEntry is passed fps: fpsKey (fpsToFfmpegArg) — so {30000,1001} and decimal 29.97002997002997 are distinct key strings; the collision is gone.
2. SCHEMA_PREFIX bumped hfcache-v3- -> hfcache-v4-, so warm pre-fix numeric-keyed entries live in a dead namespace and can't rehydrate the stale frame set.
3. Pinned by regressions: the v4 prefix assert, rational-vs-decimal key separation, the does not reuse a complete entry from the v3 numeric-fps namespace test (seeds a stale hfcache-v3-<hash> + COMPLETE_SENTINEL and proves v4 misses it), and the warm-decimal-cache miss/re-extract producing the correct 3 frames.

That's the third and (traced across computation -> in-process dedupe -> persistent cache -> coverage) hopefully final layer of this class. Good, thorough fix. — Somu

[terencecho]

Delta re-review at f0fee0b6e vs 7a0203bc. Persistent-cache identity fix — closes the P1 that my R2 review missed (I verified the in-run dedupe key at videoFrameExtractor.ts:1904 but did NOT grep for the persistent on-disk cache key at :1685, which was still using the JS-number fps).

Delta scope

4 files, +73/-9:

• extractionCache.ts (+7/-4) — SCHEMA_PREFIX: hfcache-v3- → hfcache-v4-; CacheKeyInput.fps: number → string; canonicalKeyBlob f: number → f: string
• extractionCache.test.ts (+22/-4) — fixture update + two new pins
• videoFrameExtractor.ts (+1/-1) — one-line fps → fpsKey at the computeCacheKey call in rehydrateCacheEntry / extractAllVideoFrames
• videoFrameExtractor.test.ts (+43) — real-FFmpeg warm-cache regression

The fix at the load-bearing site

Before this delta, videoFrameExtractor.ts:1688 was constructing the persistent cache key with fps: fps (JS-number, derived via fpsToNumber(configuredFps)). For NTSC input {num:30000, den:1001}, this collapses to 29.97002997002997 — an infinite-decimal truncation that would have hashed identically to a v3 entry created when the caller was passing 29.97 directly. Fix routes fps: fpsKey (the exact FFmpeg-argument string, "30000/1001"), so the two representations produce distinct persistent-cache entries.

Type change forces caller discipline

CacheKeyInput.fps: number → string — the type surface change is the real defense-in-depth. Any future caller that reintroduces a JS-number can't compile against CacheKeyInput; they'd have to explicitly stringify, which is the moment to think about which string representation. Prior number-typed field accepted any float silently.

Schema-prefix bump is belt-and-suspenders

Two independent invalidation mechanisms:

1. f: number vs f: string in canonicalKeyBlob — even with the same numeric value 30, {f: 30} and {f: "30"} JSON-canonicalize to different bytes, so hashes disagree.
2. hfcache-v3- → hfcache-v4- prefix bump — even if hashes somehow collided, lookup checks the full-prefixed dir name.

extractionCache.test.ts new test does not reuse a complete entry from the v3 numeric-fps namespace seeds a complete-sentinel'd v3 dir at the correct hash slice, then proves lookupCacheEntry misses it and returns the hfcache-v4- dir. Pins the prefix-based invalidation independently of the hash change.

Warm-cache regression is the load-bearing empirical proof

videoFrameExtractor.test.ts:1795 — real FFmpeg, single-run script:

1. Run 1: fps: 24000 / 1001 (JS-computed decimal — models pre-fix caller). Asserts cacheMisses === 1, totalFrames === 3. Then seeds a stale frame_00004.jpg in the entry's outputDir with content "stale-decimal-boundary-frame".
2. Run 2: fps: { num: 24000, den: 1001 } (exact rational). Asserts cacheHits === 0 (no collision → not rehydrating the decimal entry), cacheMisses === 1 (fresh extraction), totalFrames === 3 (didn't observe the stale 4th frame — so it's genuinely a separate on-disk entry).
3. Asserts cacheEntryNames(cacheDir).length === 2 (two entries persist, one for each representation).

The stale-frame seeding is the critical part — if rational's lookup collided with decimal's entry and rehydrated from it, rational.totalFrames would observe 4 (the stale frame). Since it observes 3, cache separation is empirically pinned, not just structurally.

Key-separation unit pin

extractionCache.test.ts new test keeps exact rational rates distinct from their JavaScript decimal:

const rational = computeCacheKey({ ...base, fps: "30000/1001" });
const decimal  = computeCacheKey({ ...base, fps: String(30000 / 1001) });
expect(rational).not.toBe(decimal);

Directly pins the specific NTSC hazard at the pure-function layer, independent of any downstream FFmpeg semantics.

CI status

All completed lanes green at review time (Preflight, Lint, Preview-regression, Detect changes, Analyze python, CLI shims, Skills, player-perf). Engine Test and heavy lanes pending.

Non-blocking

• Consumer breaking change: CacheKeyInput.fps changed from number to string. Direct producers of CacheKeyInput outside this file need to serialize (String(fps) or fpsToFfmpegArg(configuredFps)). Scope is small — grep of CacheKeyInput at head shows only extractionCache.{ts,test.ts} and the two callers in videoFrameExtractor.ts (rehydrate + fresh-extract paths). Both migrated in this diff.
• The comment above SCHEMA_PREFIX documenting the v3 → v4 reason (the target fps identity is the exact FFmpeg argument instead of a JavaScript number. This invalidates entries created after rational NTSC rates had already been rounded to a decimal.) is exactly the invariant a future refactor needs to keep in mind. Keep it.

— Review by tai (pr-review)

[vanceingalls]

Verdict: APPROVE at f0fee0b6e.

Reversing my R3 stamp — Miguel's persistent-cache-identity finding was a real gap. My R3 review noted "fpsKey in the dedupeKey uses fpsToFfmpegArg(configuredFps) — correct cache-scoping" but that was the IN-MEMORY dedupe key at line 1904. The PERSISTENT cache identity at videoFrameExtractor.ts:1685 was still passing fpsToNumber(configuredFps) into lookupCacheEntry, so warm v3 entries written under decimal fps would collide with (and serve stale frames to) fresh rational callers. Somu independently missed the same site and retracted her approval too. Codified as an addendum to feedback_re_execute_pure_numeric_functions_to_verify_boundary_claims: on any value-semantics change (rational→exact, timezone-naive→aware, string→typed-id), audit BOTH in-memory identity AND persistent identity sites, and ask whether a schema-version bump is needed to inertize pre-fix entries.

What R4 does

Four small, tightly-scoped changes:

1. CacheKeyInput.fps: number → string (extractionCache.ts:87). Type change enforces the exact FFmpeg-argument form. Docstring updated to "Exact target output frame-rate argument (for example 30000/1001)".
2. Canonical blob's f: number → f: string (extractionCache.ts:141). The SHA-256 input now hashes the exact rational string.
3. videoFrameExtractor.ts:1685 — fps (numeric) → fps: fpsKey (where fpsKey = fpsToFfmpegArg(configuredFps) — defined at :1382).
4. SCHEMA_PREFIX: "hfcache-v3-" → "hfcache-v4-" (extractionCache.ts:76). Directory-name-based inertization of pre-fix entries: a v3 entry lives at hfcache-v3-<hash>/ while the fixed code looks up at hfcache-v4-<hash>/ — different paths, no collision.

Regression coverage — three semantic pins

1. extractionCache.test.ts:127-138 — "keeps exact rational rates distinct from their JavaScript decimal":

   computeCacheKey({..., fps: "30000/1001"}) !== computeCacheKey({..., fps: String(30000/1001)})
   

   Directly pins Miguel's collision-avoidance requirement.

2. extractionCache.test.ts:206-217 — "does not reuse a complete entry from the v3 numeric-fps namespace":

   mkdirSync(join(tmpRoot, "hfcache-v3-<hash>"));
   writeFileSync(join(staleV3Dir, COMPLETE_SENTINEL), "");
   const lookup = lookupCacheEntry(tmpRoot, input);
   expect(lookup.hit).toBe(false);
   expect(lookup.entry.dir).toBe(join(tmpRoot, "hfcache-v4-<hash>"));
   

   Pins the schema-bump semantics: a stale v3 entry with the completion sentinel is NOT reused, and the fresh lookup targets the v4 dir. Mutation-catching — if someone reverts SCHEMA_PREFIX to "hfcache-v3-", this test goes red.

3. videoFrameExtractor.test.ts:1798-1836 — real-FFmpeg warm-cache VFR regression:

   • First extractAllVideoFrames call at fps: 24000/1001 (JS Number decimal) → asserts cacheMisses === 1, totalFrames === 3 (fresh extraction of the correct-count).
   • Manually seeds a stale frame_00004.jpg in the persisted output dir — models the pre-fix "4-frame" cached bundle that the decimal path used to write for this boundary.
   • Second call at fps: {num: 24000, den: 1001} (rational form) → asserts cacheHits === 0 (no stale reuse), cacheMisses === 1 (fresh extract), totalFrames === 3 (correct count preserved), AND cacheEntryNames(cacheDir).length === 2 (two SEPARATE cache directories — decimal and rational, not colliding).

The length === 2 assertion is what makes this test load-bearing beyond just the count check: even if the frame count happened to match by accident, the two-entry assertion proves the identity function distinguished the two calls at cache scope, not just at extraction scope.

Adversarial re-verification

Traced every use of CacheKeyInput.fps after the type change:

• canonicalKeyBlob at extractionCache.ts:141 — receives f: string, folds into JSON.stringify → SHA-256. Two different strings produce different hashes. Correct.
• keyFor helper at extractionCache.test.ts:37 — updated to fps: "30". Any callsite that passed a Number here would TypeScript-fail; since typecheck is green (James), no missed callsites.
• videoFrameExtractor.ts:1685 — updated to fps: fpsKey.
• Nowhere else in the diff. If a callsite was missed, tsc would fail — trusting the type system.

rehydrateCacheEntry (videoFrameExtractor.ts:1662) still receives fps (numeric) — NOT updated in this delta. Correct: rehydrate operates on an already-matched entry (post-lookup), and its fps parameter feeds downstream ExtractedFrames.fps used by consumers for numeric math (frame offsets, filenames). Not part of cache identity. Correct scope discrimination.

fpsKey is derived from toFps(options.fps) → fpsToFfmpegArg(...). For rational input {num:24000, den:1001}, this produces "24000/1001". For decimal Number 24000/1001 (=23.976023976023978 in JS), it produces... whatever toFps(29.97...) normalizes to via decimal parsing, then fpsToFfmpegArg serializes. If toFps(24000/1001) yielded canonical {24000, 1001} via NTSC-recognition, the two would collide again. The regression test at :1798 proves they do NOT collide: cacheEntryNames.length === 2 after both calls. So toFps preserves the distinction (probably normalizes the decimal to {2997002997002997, 1e14} or similar — the exact rational representation of the double, not the semantic NTSC rational).

Schema-version-bump correctness

The v3 → v4 docstring at extractionCache.ts:68-73:

v3 -> v4: the target fps identity is the exact FFmpeg argument instead of a JavaScript number. This invalidates entries created after rational NTSC rates had already been rounded to a decimal.

Pre-fix behavior: caller with rational fps {24000, 1001} collapsed to decimal 23.976... at the persistent cache boundary, wrote a hfcache-v3-<hash> entry containing 4 frames (the incorrect count from R2). Post-fix behavior: same caller now writes to hfcache-v4-<different-hash> with 3 frames. The v3 entry is orphaned (never read again) and eventually GC'd by the cache root's normal lifecycle. Correctness holds even if the v3 entry is never explicitly deleted — the v4 lookup can't find it because directory names differ.

Note: existing v3 entries at rest are effectively storage bloat until GC kicks in. Not a correctness concern; worth noting for the cache-GC operator (probably no action needed since cache directories are already GC'd on age).

Non-blocker

• Type-level: CacheKeyInput.fps: string uses raw string rather than a branded/nominal type. A future caller could accidentally pass a random-format string. Not a defect against this diff; the existing docstring documents the intent and TypeScript catches most mistakes at boundaries.

#2937 status ack

Confirmed merged as 9792c32950133cc7a6aa55361f682bb431db5439 at 2026-08-04T01:16:42Z by @jrusso1020 (50/50 checks + all four explicit approvals). R4 stamp held.

CI green pending — this should be the last review round.

— Review by Via

[miguel-heygen]

APPROVED at exact head f0fee0b — the persistent-cache P1 is closed.

The production lookup now uses the exact FFmpeg rate identity, and the persistent cache key stores that identity as a string. Therefore 30000/1001 and its decimal expansion produce distinct entries. The hfcache-v4 namespace makes every pre-fix v3 entry inert while preserving GC of older generations.

The regressions are load-bearing: they separate rational and decimal cache keys, reject a complete v3 entry, and—using real FFmpeg—publish a decimal-rate cache entry, inject a stale fourth frame, then prove an exact 24000/1001 invocation records 0 hits / 1 miss, re-extracts 3 frames, and creates a separate entry. A collision would rehydrate four frames and fail.

Local verification with current FFmpeg/ffprobe: 158/158 extractor/cache tests pass; engine typecheck passes. No residual review blockers found.