fix(producer): align frame coverage with extraction rounding

[jrusso1020]

What

• model expected video-frame counts using the same rounding contract as the extraction branch:
  • CFR -vf fps=<fps>: nearest output-frame boundary
  • VFR -fps_mode cfr -r <fps>: ceil
• retain fail-closed ceil behavior when extraction metadata is missing
• keep positive sub-frame clips at a minimum of one expected frame
• preserve the existing 95% truncation gate and source-duration credit

Why

The coverage gate universally used ceil(duration * fps), but FFmpeg's CFR fps filter rounds to the nearest boundary. This made successfully extracted short CFR clips such as 0.616666s at 30 fps look truncated (18 captured vs 19 expected).

In the dashboard window, 124 of 335 video-coverage failures were exactly one frame short. Historical logs do not include isVFR, so that is the maximum addressable cohort rather than a guaranteed reduction. Zero-frame and materially truncated extraction failures remain fail-closed.

Safety

• VFR still uses ceil and the existing strict coverage threshold.
• Missing extraction metadata still uses ceil.
• No retry, fallback, Temporal workflow, or render-plan behavior changes.
• Intended rollout is through the producer sidecar canary with explicit internal in-process jobs before dev and production promotion.

Test

• bunx vitest run packages/producer/src/services/render/videoFrameCoverage.test.ts (28 passed)
• bun run --filter @hyperframes/producer typecheck
• bunx oxlint ...videoFrameCoverage.ts ...videoFrameCoverage.test.ts
• bunx oxfmt --check ...videoFrameCoverage.ts ...videoFrameCoverage.test.ts
• bunx fallow audit --base origin/main --fail-on-issues
• repository pre-commit gates

[jrusso1020]

• fix(producer): align frame coverage with extraction rounding #2770 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[vanceingalls]

PR #2770 — Adversarial R1 Review

Head SHA: c850fd94aa198fa4c2c4fab3c25eafc58ffda87a
Title: fix(producer): align frame coverage with extraction rounding
Scope: +97 / −24 across 2 files (videoFrameCoverage.ts + its test)
Verdict: APPROVE (no P0/P1 blockers)

---

Summary

This is the smallest and most tightly focused PR in the stack. It aligns the coverage-gate's expected-frame count with the actual FFmpeg extraction contract:

• CFR path (-vf fps=<fps>) → Math.round(duration * fps) (nearest boundary).
• VFR path (-fps_mode cfr -r <fps>) → Math.ceil(duration * fps).
• Missing entry → ceil (fail-closed).
• Positive sub-frame clips lift to a floor of 1 expected frame.

The change eliminates the 124-of-335 "exactly one frame short" false-positive cohort cited in the PR body, without weakening the VFR / missing-metadata fail-closed paths.

---

Alignment mechanism — VERIFIED

Extraction source of truth (packages/engine/src/services/videoFrameExtractor.ts at head):

• Line 308-309: CFR branch — if (!metadata.isVFR) vfFilters.push('fps=${fps}') → FFmpeg fps filter default round=near → delivers ≈ round(d * f) output frames.
• Line 322: VFR branch — if (metadata.isVFR) args.push('-fps_mode', 'cfr', '-r', String(fps)) → ceil-like resampling → delivers ≈ ceil(d * f).
• Line 608 (sibling in same file, superset slicing): const requestedFrames = Math.round(work.videoDuration * fps) — already uses Math.round for the CFR case. This PR brings coverage into parity with a pattern extractor internals were already using; no third divergent rounding site remains (sibling-precision lens PASSES).

PR-side implementation (videoFrameCoverage.ts at head):

• expectedFramesForClip(start, end, fps, rounding="ceil") — default keeps the historical fail-closed behavior; opt-in "nearest" for aligned callers only.
• expectedFramesForVideo(video, entry, fps) — picks rounding = "nearest" iff entry && !entry.metadata.isVFR. Every fail-closed leg (no entry, VFR entry) stays on ceil.
• Behavior equivalence for the source-vs-slot min: OLD min(slotFrames, sourceFrames) semantics preserved; only the rounding mode inside changes.

Numerical spot-checks (all pass on the head file):

Input	Expected (test)	Verified
(0, 0.616666, 30, "nearest")	18	Math.round(18.49998) = 18 ✓
(0, 0.316666, 30, "nearest")	9	Math.round(9.49998) = 9 ✓
(0, 0.633333, 30, "nearest")	19	Math.round(18.99999) = 19 ✓
(0, 0.616666, 30) (ceil)	19	Math.ceil(18.49998) = 19 ✓
(0, 1, 29.97)	30	Math.ceil(29.97) = 30 ✓
(0, 0.001, 30, "nearest")	1	Math.max(1, round(0.03)) ✓
(1, 1, 30)	0	0-duration guard ✓

---

Adversarial angles worked

1. Rounding-alignment mechanism — CLEAN

Two rounding modes on the extraction side match two rounding modes in the coverage helper. Source of truth is the extractor's actual FFmpeg invocation, cited above.

2. One-frame off-by-one boundaries — WELL-COVERED

Tests exercise exactly-integer, near-integer, and sub-frame boundary cases:

• Exactly-integer: (0, 5, 30) = 150 and (0, 1, 29.97) = 30 (ceil test).
• Near-integer: three nearest cases at 0.316666 / 0.616666 / 0.633333, each 30fps.
• Sub-frame: (0, 0.001, 30) → 1 in both rounding modes.
• Zero-duration: (1, 1, 30) → 0 (duration===0 short-circuit).

3. Coverage threshold sensitivity vs field signal — SAFE

The field signal cited in the task (coverage 0.0% hard-abort on 0.7.72 win32, cron 141 ts=1785076772) is captured==0 with expected>0. This PR only lowers expectedFrames on the CFR path (nearest ≤ ceil), so it can only raise the ratio for real clips — cannot introduce new 0% failures. The 0% signal reflects a genuine extraction shortfall and remains fail-closed.

4. Sibling-precision divergence — CLEAN

• expectedFramesForClip is the sole external entry point (grep confirms only 2 callers: this file + its test).
• The other producer-side "frame count" formulas (planValidation.ts line 150 Math.ceil(MAX_RENDER_DURATION_SECONDS * fps), distributed/plan.ts chunk sizing) operate on unrelated axes (limits, chunk shapes) and do not model per-clip capture expectation.
• Extractor's own requestedFrames = Math.round(work.videoDuration * fps) (line 608) already matches the new coverage-side "nearest" — this PR consolidates parity rather than introducing a third mode.

5. Test asserts realness — CLEAN (per default-value-assertion-realness lens)

Every new test pins exact numeric values, not .toBeGreaterThan(0) or .toBeDefined():

• expectedFrames: 18, capturedFrames: 18, ratio: 1 (short CFR clip)
• expectedFrames: 19, capturedFrames: 18, ratio: 18 / 19 (short VFR clip — pinned as a fraction, not a truthy)
• expectedFrames: 1, capturedFrames: 0, ratio: 0 (blank sub-frame)
• .toThrow(VideoFrameCoverageError) — type-pinned, not .toThrow() bare.

6. In-thesis calibration — HELD

Scope is: frame-count rounding parity between extraction and coverage. All notes below are nits inside that scope; nothing gates on out-of-scope infrastructure.

---

Notes / nits (non-blocking)

N1. Half-boundary JS-vs-FFmpeg rounding mode (theoretical). Math.round(1.5) = 2, Math.round(2.5) = 3 (half toward +∞). FFmpeg's fps filter with round=near uses nearbyint-style banker's rounding (half-to-even): nearbyint(2.5) = 2, nearbyint(4.5) = 4. So for a hypothetical d*fps exactly at an odd-half boundary (e.g. d = 0.0833…s at 30fps → 2.5), JS predicts 3 while FFmpeg emits 2 — a 1-frame over-estimate that could false-positive fail a 0.667 ratio. In practice this requires an exact X.5 boundary; floating-point noise at composition-time authored durations makes hitting one exactly extremely unlikely. Not worth blocking; worth a comment in expectedFramesForClip if the author wants belt-and-suspenders. If it ever becomes a field-signal cohort, Math.round(x - Number.EPSILON) or a half-to-even helper would resolve it.

N2. Source-duration rounding-divergence test gap. Existing test credits a non-looping held tail against the source portion only uses durationSeconds: 3 → both ceil and nearest return 90 (no divergence). A test with durationSeconds picked so that ceil and nearest disagree by 1 (e.g. 3.007 at 30fps → nearest=90, ceil=91) would lock the sourceDuration-side rounding decision more tightly. Suggested addition, not a blocker.

N3. Fallow-ignore directives. // fallow-ignore-file code-duplication on the test file and // fallow-ignore-next-line unused-class-member on hyperframesVideoFrameCoverageError are both justified in context (repeated makeExtracted calls; structurally-read discriminant). Fine.

---

Verdict

APPROVE.

The PR is precisely what the title says: extraction-coverage rounding parity, scoped to two files, backed by a real field-signal cohort (124/335 "one-frame short"), with the fail-closed contract preserved for VFR / missing-metadata / positive sub-frame legs. Test assertions pin exact values. No new sibling-precision divergence — the extractor's own Math.round(d*fps) already existed on the CFR-adjacent path (line 608), so this PR is consolidating rather than diverging.

The only theoretical concern (half-boundary banker's-rounding drift, N1) is unreachable in typical floating-point composition durations and belongs in a follow-up if it ever surfaces in field data.

— Review by Via

[james-russo-rames-d-jusso]

Reviewed at c850fd94aa198fa4c2c4fab3c25eafc58ffda87a.

Clean rounding-alignment fix, defended in exactly the right places. The expectedFramesForClip signature adds rounding: "ceil" | "nearest" = "ceil" as an optional param — the default preserves fail-closed behavior byte-identically for every existing caller. expectedFramesForVideo selects "nearest" only when entry && !entry.metadata.isVFR (missing entry → ceil, VFR → ceil), so the tighter rounding is applied to exactly the cohort where FFmpeg's -vf fps matches it. Math.max(1, frameCount) guards the sub-frame case for both rounding modes — the duration === 0 early-return preserves the correct zero for zero/negative windows, so the floor doesn't leak into "empty clip demands one frame" territory. VFR test at videoFrameCoverage.test.ts:74-85 pins the strict-ceil behavior with an assertion that DOES throw on the exact same 0.616666s / 30fps case that passes for CFR — a good structural regression against future accidental "unify to nearest" refactors.

Verified VideoMetadata.isVFR is required (not optional) in packages/engine/src/utils/ffprobe.ts:75, and both concrete assignments at :291 and :329 populate it as a boolean — so the !entry.metadata.isVFR check reads a guaranteed-defined field. Also cross-checked the numeric examples: 0.616666 * 30 = 18.4999… → Math.round = 18, 0.633333 * 30 = 18.9999… → 19, and 0.001 * 30 = 0.03 → 0 → floored to 1. All PR-body examples land correctly.

Source-duration credit logic (short source in a longer slot returning min(slot, source)) is preserved verbatim in the new expectedFramesForVideo helper — same short-source ceiling, same non-looping-holds-last-frame rationale from #2516/#2606/#2665.

Nit

• The old inline explanation of "short source in a longer slot has a legitimate delivery ceiling" now lives inside expectedFramesForVideo — good move. Nothing to change; noting so a future reader sees the rationale is deliberately co-located with the min/source-vs-slot decision, not lost in the refactor.

What I didn't verify

• Historical dashboard cohort ("124 of 335 video-coverage failures were exactly one frame short") — trusting your dashboard query; the change scope matches the causal explanation.
• FFmpeg -vf fps exact-half-frame edge case (e.g. duration * fps === 15.5) — JS Math.round rounds .5 toward positive infinity (15.5 → 16), and depending on FFmpeg's internal tie-break for -vf fps the two could diverge. For realistic sub-microsecond-precision clip durations, exact-half boundaries are essentially impossible to hit, so this is theoretical.

— Review by Rames D Jusso

[miguel-heygen]

No additive code defect found at exact head c850fd94aa198fa4c2c4fab3c25eafc58ffda87a. CFR selects nearest-boundary expectations only when extraction metadata is present and non-VFR; VFR and missing entries retain ceil/fail-closed behavior, positive sub-frame windows still require one frame, and source-duration credit plus the 95% gate remain intact. Via and Rames already covered the theoretical half-tie and source-duration-test nits, so I am not repeating them.

Focused local verification passed: 28/28 coverage tests, producer typecheck, oxlint, and oxfmt. Exact-head regression shards are still in progress, so this is a code-clear comment rather than an early approval.

Verdict: Comment — code clear, CI pending.
Reasoning: The rounding change is internally consistent and already approved by peers, but the exact-head check inventory is not terminal.

— Magi

[jrusso1020]

Final Graphite rebase onto current main completed. Exact review head is bfaaab662380e599e5ee46a71a13dbf706da3249; focused frame-coverage tests remain green (28/28) and producer typecheck passes. Please re-stamp this head after fresh CI.

[james-russo-rames-d-jusso]

Re-reviewed at bfaaab6623 — verified pure rebase on new main (5bfda4e08737…).

git diff <merge-base>..<head> | git patch-id --stable returns 44c5c9257dcb25514a93625683cea150ef5aedb9 on BOTH the prior c850fd94aa and the new bfaaab6623 — so #2770's own contribution is byte-identical to R1. R1 LGTM stands.

— Review by Rames D Jusso

[vanceingalls]

Byte-clean re-stamp at bfaaab6623. Verified via blob-SHA equivalence: videoFrameCoverage.ts and videoFrameCoverage.test.ts are byte-identical to their content at prior R1 head c850fd94aa (same git blob SHAs). The Graphite rebase is pure main catch-up; no content drift on the PR's own scope.

Prior R1 APPROVE at c850fd94aa stands. Rounding-alignment (Math.round on CFR, Math.ceil on VFR/missing/fail-closed) still matches the extraction source of truth in videoFrameExtractor.ts at head; sibling-precision lens passes; no new as T / non-null / unguarded .message on the changed files.

— Review by Via

[miguel-heygen]

Exact-head approval at bfaaab662380e599e5ee46a71a13dbf706da3249.

The Graphite rebase is patch-identical to the code-clear head previously reviewed. packages/producer/src/services/render/videoFrameCoverage.ts still selects nearest-boundary expectations only for metadata-backed CFR extraction; VFR and missing metadata remain ceil/fail-closed, positive sub-frame windows retain the one-frame floor, and source-duration credit plus the 95% coverage gate are unchanged. The exact-value regressions in videoFrameCoverage.test.ts continue to pin both sides of the rounding split.

Via independently verified byte-identical blobs and approved this exact head; Rames independently verified the stable PR patch-id.

Verdict: APPROVE
Reasoning: The rebase introduces no PR-owned drift, the coverage math remains aligned with the extractor without weakening fail-closed paths, and exact-head required CI is green.

— Magi

[james-russo-rames-d-jusso]

R2 (pure-rebase-only on new main; patch-id 44c5c9257… identical to R1 slice) stands. Applying stamp per James's ask at 1785090928.968079.

[james-russo-rames-d-jusso]

Re-reviewed at f0c2c7d23384de589f54c11ff093f128c9a39e56 — verified pure rebase on merged-main (f500a42d9…, containing #2776).

git diff <merge-base>..<head> | git patch-id --stable returns 44c5c9257dcb25514a93625683cea150ef5aedb9 on BOTH bfaaab6623 and f0c2c7d233 — #2770's own contribution byte-identical to R2. R2 LGTM stands.

— Review by Rames D Jusso

[vanceingalls]

Byte-clean re-stamp at f0c2c7d233. Pure post-#2776-merge Graphite rebase.

Verified blob-SHA equivalence on both PR-scoped files vs prior head bfaaab6623:

• videoFrameCoverage.ts — MATCH (455ccfcf42…)
• videoFrameCoverage.test.ts — MATCH (254607a1dc…)

Genuinely patch-identical rebase. No content drift on #2770's own scope; rebase is pure post-#2776-merge main catch-up.

All prior R1 verification stands: rounding-alignment (Math.round on CFR + Math.ceil on VFR/missing/fail-closed) matches the extraction source of truth at videoFrameExtractor.ts; sibling-precision lens passes; every fail-closed leg stays on ceil; test assertions pin exact numerical values (18/18/1, 19/18/(18/19), 1/0/0).

— Review by Via

[miguel-heygen]

Final exact-head re-stamp at f0c2c7d23384de589f54c11ff093f128c9a39e56.

This is a pure post-#2776-merge rebase of the previously audited child slice:

• Stable child patch ID matches the prior head bfaaab6623: 44c5c9257dcb25514a93625683cea150ef5aedb9.
• The child diff contains the same two PR-scoped files.
• Both videoFrameCoverage.ts and videoFrameCoverage.test.ts are byte-identical by blob SHA.
• git diff --check passes.
• Every required exact-head check is terminal green, including Build, Test, Typecheck, runtime contract, and both Windows jobs.

The previously reviewed contract is unchanged: metadata-backed CFR extraction uses nearest-boundary expectations, while VFR and missing metadata retain ceil/fail-closed behavior; positive sub-frame windows still require one frame, and source-duration credit plus the 95% coverage gate remain intact.

Three non-required regression shards are still running at submission time; they remain a merge/release gate under the stated full-matrix policy, not an approval gate.

Verdict: APPROVE
Reasoning: The final restack introduces no PR-owned drift, the reviewed coverage semantics are unchanged, and all required exact-head checks pass.

— Magi

[jrusso1020]

Merge activity

• Jul 26, 9:20 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Jul 26, 9:20 PM UTC: @jrusso1020 merged this pull request with Graphite.

[james-russo-rames-d-jusso]

R3 (pure-rebase-on-merged-#2776; patch-id 44c5c9257… identical to R2 slice) stands. Fresh CI settled clean. Applying stamp per James's ask at 1785098053.767169 — final in the render-success batch.