diff --git a/packages/engine/src/utils/ffprobe.test.ts b/packages/engine/src/utils/ffprobe.test.ts index 899ffc90b..ab1cf211f 100644 --- a/packages/engine/src/utils/ffprobe.test.ts +++ b/packages/engine/src/utils/ffprobe.test.ts @@ -6,6 +6,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { extractMediaMetadata, extractPngMetadataFromBuffer, + parseFrameRate, pixelFormatHasAlpha, } from "./ffprobe.js"; @@ -578,47 +579,58 @@ describe("ffprobe option separator", () => { }); }); -describe("ffprobe frame rate parsing", () => { - afterEach(() => { - vi.resetModules(); - vi.doUnmock("child_process"); +describe("parseFrameRate", () => { + // Direct against the exported function. The previous table drove this + // through extractMediaMetadata behind a spawn mock, which cost a + // vi.resetModules() plus a dynamic 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 + // was written to catch. + it.each([ + ["30/1", 30], + ["30000/1001", 29.97], + ["24000/1001", 23.98], + ["60", 60], + ["25.5", 25.5], + ])("parses %s as %s", (input, expected) => { + expect(parseFrameRate(input)).toBe(expected); }); it.each([ - { r: "30/1", avg: "30/1", expected: 30 }, - { r: "30000/1001", avg: "30000/1001", expected: 29.97 }, - { r: "30/", avg: undefined, expected: 0 }, - { r: "30/0", avg: undefined, expected: 0 }, - { r: "0/0", avg: undefined, expected: 0 }, - { r: "abc/def", avg: undefined, expected: 0 }, - { r: "60", avg: undefined, expected: 60 }, - ])("parses r=$r avg=$avg as fps=$expected", async ({ r, avg, expected }) => { - const { spawn } = createSpawnSpy([ - { - kind: "exit", - code: 0, - stdout: JSON.stringify({ - streams: [ - { - codec_type: "video", - codec_name: "h264", - width: 320, - height: 180, - r_frame_rate: r, - avg_frame_rate: avg, - }, - ], - format: { duration: "1.5" }, - }), - }, - ]); - vi.resetModules(); - vi.doMock("child_process", () => ({ spawn })); + ["30/", 0], + ["30/0", 0], + ["0/0", 0], + ["abc/def", 0], + ["", 0], + [undefined, 0], + ])("returns 0 for unusable input %s", (input, expected) => { + expect(parseFrameRate(input)).toBe(expected); + }); - const { extractMediaMetadata } = await import("./ffprobe.js"); - const meta = await extractMediaMetadata("/tmp/frame-rate.mp4"); + // Finite operands, infinite quotient — the operand-only guard missed these. + it.each(["1e308/1e-10", "2/1e-320"])("returns 0 for overflowing quotient %s", (input) => { + expect(parseFrameRate(input)).toBe(0); + }); - expect(meta.fps).toBe(expected); + // Negatives were truthy, so `meta.fps || 30` did not rescue them and + // buildEncoderArgs emitted `-r -30`. + it.each(["-30/1", "30/-1", "-60"])("returns 0 for negative rate %s", (input) => { + expect(parseFrameRate(input)).toBe(0); + }); + + // Fell through to a bare parseFloat that stops at trailing garbage. + it.each(["30/1/2", "60fps"])("returns 0 for malformed input %s", (input) => { + expect(parseFrameRate(input)).toBe(0); + }); + + // 2dp rounding collapsed these to 0, and the caller's `|| 30` then + // re-encoded a 300-second timelapse as a ~1/30-second clip. + it.each([ + ["1/300", 0.01], + ["1/1000", 0.01], + ["1/200", 0.01], + ])("floors sub-0.005 rate %s to %s rather than 0", (input, expected) => { + expect(parseFrameRate(input)).toBe(expected); }); }); diff --git a/packages/engine/src/utils/ffprobe.ts b/packages/engine/src/utils/ffprobe.ts index 08e07a597..217f0807e 100644 --- a/packages/engine/src/utils/ffprobe.ts +++ b/packages/engine/src/utils/ffprobe.ts @@ -326,19 +326,54 @@ function readTagCI(tags: Record | undefined, name: s return ""; } -function parseFrameRate(frameRateStr: string | undefined): number { +/** + * Parse an ffprobe rational frame rate ("30000/1001") or plain number. + * + * Returns 0 for anything not a usable positive rate. Exported so tests + * exercise the shipped function directly instead of re-importing the module + * behind a spawn mock. + * + * Every guard here is load-bearing, because a bad value is NOT caught + * downstream: callers use `meta.fps || 30`, which only rescues 0 and NaN. + * Infinity and negatives are truthy and flow into buildEncoderArgs as + * `-r Infinity` / `-r -30`, which ffmpeg rejects mid-render, and into + * frameCount arithmetic that then goes negative or non-finite. + * + * - the QUOTIENT is checked, not just the operands: "1e308/1e-10" and + * "2/1e-320" have finite parts and an infinite result; + * - the sign is checked: "-30/1", "30/-1" and "-60" all parsed clean; + * - more than two parts is rejected: "30/1/2" used to fall through to the + * bare parseFloat below and return 30, as did "60fps", because parseFloat + * stops at trailing garbage; + * - sub-0.005 rates round to 0 at 2dp and would be replaced by the caller's + * 30fps default, re-encoding a 300-second 1/300-fps timelapse as a + * ~1/30-second clip. Kept as 0 is wrong too, so they are floored to the + * smallest representable 2dp rate instead. + */ +export function parseFrameRate(frameRateStr: string | undefined): number { if (!frameRateStr) return 0; + const parts = frameRateStr.split("/"); - if (parts.length === 2) { - const num = parseFloat(parts[0] ?? ""); - const den = parseFloat(parts[1] ?? ""); - if (Number.isFinite(num) && Number.isFinite(den) && den !== 0) { - return Math.round((num / den) * 100) / 100; - } - return 0; - } - const parsed = parseFloat(frameRateStr); - return Number.isFinite(parsed) ? parsed : 0; + if (parts.length > 2) return 0; + + const raw = + parts.length === 2 + ? (() => { + const num = parseFloat(parts[0] ?? ""); + const den = parseFloat(parts[1] ?? ""); + if (!Number.isFinite(num) || !Number.isFinite(den) || den === 0) return NaN; + return num / den; + })() + : // Not parseFloat: it stops at trailing garbage, so "60fps" parsed as + // 60. Number() rejects the whole string. + Number(frameRateStr.trim()); + + if (!Number.isFinite(raw) || raw <= 0) return 0; + + const rounded = Math.round(raw * 100) / 100; + // A real but very slow rate must not collapse to 0 and inherit the + // caller's 30fps default. + return rounded > 0 ? rounded : 0.01; } /**