fix(engine): harden ffprobe parsing and command arguments

[santhiprakash]

What

Hardens packages/engine/src/utils/ffprobe.ts against three edge cases:

• Malformed frame-rate strings (e.g. "30/", "30/0", "abc/def") were returning NaN from parseFrameRate.
• File paths starting with - were passed as bare arguments to ffprobe, which can be misinterpreted as option flags.
• The PNG cICP color-space handler returned immediately, so a cICP chunk seen before IHDR could emit width: 0, height: 0.

Why

These bugs caused downstream metadata (fps, colorSpace) to become NaN/null or, in the path case, allowed accidental option injection when user-provided paths begin with a dash.

How

• parseFrameRate now checks Number.isFinite on both numerator and denominator and requires den !== 0; invalid ratios fall back to 0 instead of NaN.
• Every runFfprobe invocation now inserts -- before the user-supplied filePath.
• extractPngMetadataFromBuffer stores any cICP color space in a local variable and only emits it after IHDR has supplied valid width/height.
• Added regression tests for all three behaviors.

Test plan

• Unit tests added/updated
• Manual testing performed
• Documentation updated (if applicable)

Ran bunx vitest run src/utils/ffprobe.test.ts and bunx oxlint / bunx oxfmt --check on the changed files. Three existing fixture-dependent tests fail locally because hdr-photo-pq.png is stored via Git LFS and is not present in this working tree; they pass in CI with the LFS object.

[jrusso1020]

Reviewed packages/engine/src/utils/ffprobe.ts end-to-end at HEAD (e740f1c) plus the new tests. No prior reviews/comments on the PR.

Strengths

• parseFrameRate (fix #1, ~L239) is correct and thoroughly table-tested — Number.isFinite on both operands + den !== 0, with "30/", "30/0", "0/0", "abc/def", and bare "60" all pinned. 0 is the right fallback: downstream avgFps || rFps and the still-image path already tolerate fps: 0, whereas NaN poisoned every frame-count calc.
• cICP ordering (fix #3, L184/L208) is correct — the color space is stashed in a local and only emitted once IHDR supplies valid dims. Handles cICP-before-IHDR, normal cICP-after-IHDR, and cICP-with-no-IHDR (→ null). All three covered by the new test.

Blocker

• Fix #2 is incomplete. The PR body says "Every runFfprobe invocation now inserts -- before the user-supplied filePath," but the AAC packet-count probe misses it — ffprobe.ts:388 passes filePath bare (right after -print_format json). Three of the four call sites got -- (L274, L366, L457); this fourth did not. So the same audio file at a --prefixed path is separator-protected on the main audio probe (L366) but not on the AAC sub-probe just below it — the exact arg-injection class this PR sets out to close, left open on one path. The "uses -- for audio and keyframe probes too" test can't catch it because the audio fixture is pcm_s16le, so the if (audioCodec === "aac") branch never executes (it asserts 2 --, not 3).
  Fix: add "--" before filePath at L388, and add an aac-codec case to that test so the count becomes 3.

Important — merge-readiness, not code

• CI has not actually run. Every real job — CI, regression, preview-regression, Player perf, CodeQL, Windows render verification — is in action_required (0s, awaiting maintainer approval to run); the only completed check is WIP (pass). So the body's claim that the 3 LFS-fixture tests "pass in CI with the LFS object" is currently unverified — nothing green confirms it, and this is why the PR shows BLOCKED. A maintainer needs to approve the workflow runs and confirm the vitest CI job (with the hdr-photo-pq.png LFS object) is green before merge.

Verdict: REQUEST CHANGES
Reasoning: Fixes #1 and #3 are correct and well-tested, but the headline command-arg hardening (#2) misses the AAC packet-count call site — so the "every invocation" claim is false and the injection path stays open there — and CI hasn't executed, so the test-pass claim is unconfirmed.

— Jerrai

[jrusso1020]

Fix is incomplete: the AAC packet-count sub-probe a few lines down (ffprobe.ts:388, inside if (audioCodec === "aac" && sampleRate > 0)) still passes filePath bare — no --. The same audio file at a --prefixed path is separator-protected on this probe but not on that one, which contradicts the PR body's "every runFfprobe invocation" claim. The "uses -- for audio and keyframe probes too" test can't catch it because its fixture is pcm_s16le (the aac branch never runs). Add "--" before filePath at L388, plus an aac case that bumps the assertion to 3 --.

[santhiprakash]

Good catch — I missed the -- in the AAC packet-count probe. I added it before filePath there and updated the regression test so it exercises the AAC path and asserts all three -- separators. The relevant tests now pass locally. PTAL.