Skip to content

Commit 19dc83b

Browse files
vanceingallsclaude
andcommitted
fix(engine): reject non-finite, negative and malformed frame rates
parseFrameRate guarded its operands but not its result, so several inputs produced values that are not usable frame rates — and nothing downstream catches them, because callers use `meta.fps || 30`, which only rescues 0 and NaN. Everything below was truthy and flowed into buildEncoderArgs as `-r <value>` (rejected by ffmpeg mid-render) and into frameCount arithmetic. "1e308/1e-10", "2/1e-320" -> Infinity (finite operands, infinite quotient) "-30/1", "30/-1", "-60" -> negative (sign never checked) "30/1/2" -> 30 (parts.length !== 2 fell through) "60fps" -> 60 (parseFloat stops at 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() rather than parseFloat so trailing garbage fails the whole string. Separately, 2dp rounding collapsed any rate below 0.005 to exactly 0, and the caller's 30fps default then re-encoded a 300-second 1/300-fps timelapse as a ~1/30-second clip with frameCount 9000 for a 1-frame file. Those floor to 0.01 instead. parseFrameRate is now exported and tested directly. The previous table 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) — and 4 of its 7 rows produced identical values against the pre-fix implementation, so it could not fail for the bugs it existed to catch. The replacement fails 9 against that implementation. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
1 parent 62b96c2 commit 19dc83b

2 files changed

Lines changed: 94 additions & 47 deletions

File tree

‎packages/engine/src/utils/ffprobe.test.ts‎

Lines changed: 48 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { afterEach, describe, expect, it, vi } from "vitest";
66
import {
77
extractMediaMetadata,
88
extractPngMetadataFromBuffer,
9+
parseFrameRate,
910
pixelFormatHasAlpha,
1011
} from "./ffprobe.js";
1112

@@ -578,47 +579,58 @@ describe("ffprobe option separator", () => {
578579
});
579580
});
580581

581-
describe("ffprobe frame rate parsing", () => {
582-
afterEach(() => {
583-
vi.resetModules();
584-
vi.doUnmock("child_process");
582+
describe("parseFrameRate", () => {
583+
// Direct against the exported function. The previous table drove this
584+
// through extractMediaMetadata behind a spawn mock, which cost a
585+
// vi.resetModules() plus a dynamic re-import of core's 238-file barrel per
586+
// row (74.9 ms vs 0.094 ms) — and 4 of its 7 rows produced identical values
587+
// against the pre-fix implementation, so it could not fail for the bugs it
588+
// was written to catch.
589+
it.each([
590+
["30/1", 30],
591+
["30000/1001", 29.97],
592+
["24000/1001", 23.98],
593+
["60", 60],
594+
["25.5", 25.5],
595+
])("parses %s as %s", (input, expected) => {
596+
expect(parseFrameRate(input)).toBe(expected);
585597
});
586598

587599
it.each([
588-
{ r: "30/1", avg: "30/1", expected: 30 },
589-
{ r: "30000/1001", avg: "30000/1001", expected: 29.97 },
590-
{ r: "30/", avg: undefined, expected: 0 },
591-
{ r: "30/0", avg: undefined, expected: 0 },
592-
{ r: "0/0", avg: undefined, expected: 0 },
593-
{ r: "abc/def", avg: undefined, expected: 0 },
594-
{ r: "60", avg: undefined, expected: 60 },
595-
])("parses r=$r avg=$avg as fps=$expected", async ({ r, avg, expected }) => {
596-
const { spawn } = createSpawnSpy([
597-
{
598-
kind: "exit",
599-
code: 0,
600-
stdout: JSON.stringify({
601-
streams: [
602-
{
603-
codec_type: "video",
604-
codec_name: "h264",
605-
width: 320,
606-
height: 180,
607-
r_frame_rate: r,
608-
avg_frame_rate: avg,
609-
},
610-
],
611-
format: { duration: "1.5" },
612-
}),
613-
},
614-
]);
615-
vi.resetModules();
616-
vi.doMock("child_process", () => ({ spawn }));
600+
["30/", 0],
601+
["30/0", 0],
602+
["0/0", 0],
603+
["abc/def", 0],
604+
["", 0],
605+
[undefined, 0],
606+
])("returns 0 for unusable input %s", (input, expected) => {
607+
expect(parseFrameRate(input)).toBe(expected);
608+
});
617609

618-
const { extractMediaMetadata } = await import("./ffprobe.js");
619-
const meta = await extractMediaMetadata("/tmp/frame-rate.mp4");
610+
// Finite operands, infinite quotient — the operand-only guard missed these.
611+
it.each(["1e308/1e-10", "2/1e-320"])("returns 0 for overflowing quotient %s", (input) => {
612+
expect(parseFrameRate(input)).toBe(0);
613+
});
614+
615+
// Negatives were truthy, so `meta.fps || 30` did not rescue them and
616+
// buildEncoderArgs emitted `-r -30`.
617+
it.each(["-30/1", "30/-1", "-60"])("returns 0 for negative rate %s", (input) => {
618+
expect(parseFrameRate(input)).toBe(0);
619+
});
620620

621-
expect(meta.fps).toBe(expected);
621+
// Fell through to a bare parseFloat that stops at trailing garbage.
622+
it.each(["30/1/2", "60fps"])("returns 0 for malformed input %s", (input) => {
623+
expect(parseFrameRate(input)).toBe(0);
624+
});
625+
626+
// 2dp rounding collapsed these to 0, and the caller's `|| 30` then
627+
// re-encoded a 300-second timelapse as a ~1/30-second clip.
628+
it.each([
629+
["1/300", 0.01],
630+
["1/1000", 0.01],
631+
["1/200", 0.01],
632+
])("floors sub-0.005 rate %s to %s rather than 0", (input, expected) => {
633+
expect(parseFrameRate(input)).toBe(expected);
622634
});
623635
});
624636

‎packages/engine/src/utils/ffprobe.ts‎

Lines changed: 46 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -326,19 +326,54 @@ function readTagCI(tags: Record<string, string | undefined> | undefined, name: s
326326
return "";
327327
}
328328

329-
function parseFrameRate(frameRateStr: string | undefined): number {
329+
/**
330+
* Parse an ffprobe rational frame rate ("30000/1001") or plain number.
331+
*
332+
* Returns 0 for anything not a usable positive rate. Exported so tests
333+
* exercise the shipped function directly instead of re-importing the module
334+
* behind a spawn mock.
335+
*
336+
* Every guard here is load-bearing, because a bad value is NOT caught
337+
* downstream: callers use `meta.fps || 30`, which only rescues 0 and NaN.
338+
* Infinity and negatives are truthy and flow into buildEncoderArgs as
339+
* `-r Infinity` / `-r -30`, which ffmpeg rejects mid-render, and into
340+
* frameCount arithmetic that then goes negative or non-finite.
341+
*
342+
* - the QUOTIENT is checked, not just the operands: "1e308/1e-10" and
343+
* "2/1e-320" have finite parts and an infinite result;
344+
* - the sign is checked: "-30/1", "30/-1" and "-60" all parsed clean;
345+
* - more than two parts is rejected: "30/1/2" used to fall through to the
346+
* bare parseFloat below and return 30, as did "60fps", because parseFloat
347+
* stops at trailing garbage;
348+
* - sub-0.005 rates round to 0 at 2dp and would be replaced by the caller's
349+
* 30fps default, re-encoding a 300-second 1/300-fps timelapse as a
350+
* ~1/30-second clip. Kept as 0 is wrong too, so they are floored to the
351+
* smallest representable 2dp rate instead.
352+
*/
353+
export function parseFrameRate(frameRateStr: string | undefined): number {
330354
if (!frameRateStr) return 0;
355+
331356
const parts = frameRateStr.split("/");
332-
if (parts.length === 2) {
333-
const num = parseFloat(parts[0] ?? "");
334-
const den = parseFloat(parts[1] ?? "");
335-
if (Number.isFinite(num) && Number.isFinite(den) && den !== 0) {
336-
return Math.round((num / den) * 100) / 100;
337-
}
338-
return 0;
339-
}
340-
const parsed = parseFloat(frameRateStr);
341-
return Number.isFinite(parsed) ? parsed : 0;
357+
if (parts.length > 2) return 0;
358+
359+
const raw =
360+
parts.length === 2
361+
? (() => {
362+
const num = parseFloat(parts[0] ?? "");
363+
const den = parseFloat(parts[1] ?? "");
364+
if (!Number.isFinite(num) || !Number.isFinite(den) || den === 0) return NaN;
365+
return num / den;
366+
})()
367+
: // Not parseFloat: it stops at trailing garbage, so "60fps" parsed as
368+
// 60. Number() rejects the whole string.
369+
Number(frameRateStr.trim());
370+
371+
if (!Number.isFinite(raw) || raw <= 0) return 0;
372+
373+
const rounded = Math.round(raw * 100) / 100;
374+
// A real but very slow rate must not collapse to 0 and inherit the
375+
// caller's 30fps default.
376+
return rounded > 0 ? rounded : 0.01;
342377
}
343378

344379
/**

0 commit comments

Comments
 (0)