fix(engine): reject non-finite, negative and malformed frame rates

[vanceingalls]

Stack 3/6, on top of #2913. Extends the Number.isFinite guards #2740 added — they check the operands but not the result.

The gap

Nothing downstream catches a bad frame rate. Callers use meta.fps || 30, which only rescues 0 and NaN. Everything below is truthy, so it flows into buildEncoderArgs as -r <value> — rejected by ffmpeg mid-render — and into frameCount arithmetic that goes negative or non-finite.

Executed against current main:

input	result	why
1e308/1e-10	Infinity	finite operands, infinite quotient
2/1e-320	Infinity	same
-30/1, 30/-1, -60	negative	sign never checked
30/1/2	30	parts.length !== 2 fell through
60fps	30	parseFloat stops at trailing garbage

Now: the quotient is checked rather than the operands, non-positive is rejected, more than two parts is rejected, and the single-part path uses Number() so trailing garbage fails the whole string.

Sub-0.005 rates collapsed to zero

2dp rounding sent any rate below 0.005 to exactly 0, and the caller's 30fps default took over. Verified end-to-end with a real 1/300-fps H.264 file: a 300-second timelapse re-encodes as a ~1/30-second clip, and frameCount becomes 9000 for a 1-frame file. Those floor to 0.01 instead.

On the tests

parseFrameRate is now exported and tested directly. The table added in #2740 drove it through extractMediaMetadata behind a spawn mock, costing a vi.resetModules() plus a re-import of core's 238-file barrel per row — 74.9 ms vs 0.094 ms.

More importantly, 4 of its 7 rows produced identical values against the pre-fix implementation (30/1, 30000/1001, 0/0, 60), so reverting the guards it was written to protect left it green. The replacement fails 9 against that implementation.

Verification

Engine suite: 1280 pass.

🤖 Generated with Claude Code

[miga-heygen]

Stack 3/6: Frame rate validation — Review

Clean fix, no concerns.

Every guard is load-bearing

Downstream consumers use meta.fps || 30, which only rescues 0 and NaN — Infinity and negatives are truthy and flow into buildEncoderArgs as -r Infinity / -r -30. The fix correctly:

• Checks the quotient, not just the operands (1e308/1e-10 has finite parts but an infinite result)
• Rejects non-positive: raw <= 0 catches -30/1, 30/-1, -60
• Rejects malformed: parts.length > 2 catches 30/1/2; Number() instead of parseFloat catches 60fps (parseFloat stops at trailing garbage)
• Floors sub-0.005 to 0.01 instead of collapsing to 0 (which would trigger the 30fps fallback and re-encode a 300-second timelapse as ~1/30s)

Tests

Rewritten from integration (spawn mock + module re-import per row, 74.9ms) to direct function tests (0.094ms). The old table had 4 of 7 rows that passed against the pre-fix implementation — reverting the guards left the suite green. The new table has 9 rows that fail against the pre-fix code.

Not a duplication concern

Producer's parseFrameRate in audioPadTrim.ts:408 returns { fpsNum, fpsDen } (exact rational for audio pad/trim) and throws on bad input — different signature, different semantics, different consumers. Not the same predicate.

Verified

• Infinity, negative, multi-slash, trailing-garbage all return 0
• Sub-0.005 rates floor to 0.01 (not 0)
• Valid rates: 30/1 → 30, 30000/1001 → 29.97, 60 → 60, 25.5 → 25.5
• Engine test suite: 1280 pass per PR body

Ships clean.

[miguel-heygen]

Reviewed exact head 0c71c89b107dd37eedb8fc59e45fcc3d8fb6e621 against stack base #2913. The direct parser tests and nonpositive/quotient guards are improvements, but two malformed/non-finite paths remain:

1. P1 — rounding can recreate Infinity after the finite guard. At ffprobe.ts:350, a finite raw value such as 1e307 or 1e307/1 overflows in raw * 100; rounded becomes Infinity, and line 353 returns it because it is positive. This reaches the same downstream meta.fps || 30 / -r Infinity failure the PR is intended to close. Check the rounded result too, or round without an overflowing multiply, and add both plain/rational regressions.

2. P2 — rational operands still accept trailing garbage. Lines 339-340 use parseFloat, so 60fps/1, 60/1fps, and 30garbage/1garbage all return valid rates even though the plain-number path correctly switched to Number() and the contract says malformed frame rates fail closed. Parse the two rational components strictly and pin them in the direct table.

The exact-head regression run is also red, but the logs show external Docker Hub/buildx setup failure (registry-1.docker.io context deadline) before tests; fail-fast then stopped sibling shards. That is infrastructure, not evidence of a product regression, but the head is not terminal green. No merge or deployment performed.

[james-russo-rames-d-jusso]

Independent read at 0c71c89b1, third perspective following @miga-heygen. Agree with their verified matrix — the || 30 fallback path is confirmed at cli/src/background-removal/pipeline.ts:123 (feeds into buildEncoderArgs verbatim), so every one of the six guards Vance adds really is load-bearing. And Miga's carve-out for audioPadTrim.parseFrameRate at :397 holds: different return shape ({ fpsNum, fpsDen }), different failure mode (throws), different consumer (audio pad/trim rational rate), not a duplication concern.

One observation, not a finding:

Nits

• 2-decimal rounding is inherited, not introduced, but worth locking in a comment. Math.round(raw * 100) / 100 predates this PR — 30000/1001 → 29.97 and 24000/1001 → 23.98 both lose their exact rational form here, and any downstream that stringifies the result and passes it to -r on ffmpeg's CLI loses ~3.3ppm of frame-rate accuracy relative to the container's rational value. Not a regression (identical to the pre-fix code), and probably indistinguishable for anything under ~30 minutes of content, but the new docstring is silent about it while enumerating every other guard. Half a line noting "returns a 2dp rounding, callers wanting exact rationals should read the raw ffprobe field" would leave a breadcrumb for future readers.

What I didn't verify

• Whether the floored-to-0.01 case (sub-0.005 real rates) survives cleanly through the rest of the encoder pipeline — -r 0.01 on ffmpeg CLI is technically valid but I didn't trace whether every consumer downstream of meta.fps copes with sub-1 fps values. The pre-fix behavior collapsed these to 0 and inherited 30fps by accident; this fix keeps them as 0.01 which is closer to right, and either way the guard is a strict improvement.

Otherwise clean. LGTM from my side.

— Review by Rames D Jusso

[vanceingalls]

Both fixed in 339179913.

P1 — rounding recreated Infinity. Correct, and it defeated the guard I'd just added: raw * 100 overflows for 1e307, so rounded was Infinity and passed the positivity check. The rounded result is now checked too. Pinned with 1e307, 1e307/1 and 1e308/0.5.

P2 — rational operands still used parseFloat. Also correct, and an inconsistency I introduced: I switched the plain-number path to Number() precisely so trailing garbage fails the whole string, then left the numerator and denominator on parseFloat. Both are strict now, and an empty operand (/, /1, 30/) is rejected rather than coerced. The direct table covers 60fps/1, 60/1fps and 30garbage/1garbage.

Reverting either fix fails 5 tests.

On the red regression run: agreed it was the Docker Hub registry-1.docker.io deadline before any test ran, with fail-fast stopping the sibling shards. Re-pushed, so it should get a clean lane.

[miguel-heygen]

Re-review

Approved at exact head 339179913089c69e4a6757fc86f3c9df2e456785.

Both prior parsing blockers are fixed: rational operands now use the same strict whole-string conversion as the plain-number branch, including empty-operand rejection, and the post-round value is checked for finiteness before it can reach downstream ffmpeg arguments. The new table covers trailing garbage and finite-input overflow paths. Exact-head CI is terminal green. No remaining P0-P2 findings.

The base branch was changed.