From 5a3f896652a597195698c642fca2991558c7a3b6 Mon Sep 17 00:00:00 2001 From: Filip131311 Date: Sat, 1 Aug 2026 08:30:33 +0200 Subject: [PATCH] fix(screen-recording): pick an ffmpeg that can actually encode, not just one that exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolution trusted the first `command -v ffmpeg` hit unconditionally and only consulted the fallback list when ffmpeg was ABSENT. A conda-forge build (`--disable-gpl`, so no libx264) ahead of Homebrew on PATH therefore won every time, and every recording died with `Unrecognized option 'preset'` — `-preset` being a libx264-private option — while a working build sat in the fallback list, unreachable. Each candidate is now asked whether it can encode, and the first that says yes wins. The probe has to read ffmpeg's OUTPUT: `-h encoder=libx264` exits 0 whether or not the encoder exists, so the obvious implementation — try/catch around execFile — would look like a fix and change nothing. It matches the success header rather than the failure sentence, because the header comes from ffmpeg's help formatter and has been stable for a decade while the failure text is prose any release may reword; keying on the failure string would silently re-break recording, and in the direction that breaks setups which currently work. `-hide_banner` is load-bearing rather than cosmetic: without it the build banner goes to stderr carrying the literal `--enable-libx264`. Everything inconclusive — a timeout, a kill, no output at all — is treated as usable, so the probe can only ever demote a build that positively said it lacks libx264. On any host where recording works today, ffmpeg still gets to speak for itself. Two things the failure path was getting wrong: - "ffmpeg was not found on PATH. Install it" was shown to someone with three ffmpegs installed, which is what sent the reporter looking in the wrong place. That case now has its own message and its own failure code, since the code is rendered to the user verbatim. An EACCES binary counts as found, not missing, for the same reason. - There was no way to override the choice. ARGENT_FFMPEG adds one, and the probe is deliberately ADVISORY for it: the probe's one new failure mode is a false negative on a build whose help output we don't recognise, and an escape hatch subject to the filter it exists to escape would rescue nobody. A pinned binary is never silently swapped for a different one either — it fails loudly instead. `resolveBinary` is deleted rather than extended. Its generic half duplicated `commandOnPath`, worse: the hand-rolled `/bin/sh -c command -v` could never match on Windows, where Android recording is reachable. Going through `commandOnPath` also means the binary we validate is the binary we spawn, instead of resolving PATH twice and proving nothing. Not cached on purpose — the tool-server has no idle shutdown by default, so a cached "no usable ffmpeg" would outlive the user acting on our own error message. One ~33ms probe per recording start, against a path that already waits ~800ms. `resolveFfmpeg` keeps its name and stays reachable from ./watermark so the existing test mock seam still disarms it. Verified end to end on a host in the reported state (conda's ffmpeg first on PATH): recording now produces a valid 87KB h264 mp4 at native resolution where it previously failed instantly. Fixes #621 --- .github/workflows/windows-e2e.yml | 1 + packages/registry/src/failure-codes.ts | 5 + .../skills/argent-screen-recording/SKILL.md | 2 +- .../src/tools/screen-recording/capture.ts | 33 +- .../tools/screen-recording/ffmpeg-binary.ts | 292 +++++++++++++ .../screen-recording-start.ts | 2 +- .../src/tools/screen-recording/watermark.ts | 44 +- .../tool-server/test/ffmpeg-resolver.test.ts | 394 ++++++++++++++++++ .../tool-server/test/screen-recording.test.ts | 39 +- 9 files changed, 757 insertions(+), 55 deletions(-) create mode 100644 packages/tool-server/src/tools/screen-recording/ffmpeg-binary.ts create mode 100644 packages/tool-server/test/ffmpeg-resolver.test.ts diff --git a/.github/workflows/windows-e2e.yml b/.github/workflows/windows-e2e.yml index f91c3ce1c..6075e4935 100644 --- a/.github/workflows/windows-e2e.yml +++ b/.github/workflows/windows-e2e.yml @@ -87,6 +87,7 @@ jobs: run: > npx vitest run test/command-on-path.test.ts + test/ffmpeg-resolver.test.ts test/android-binary-windows.test.ts test/adb-resolve-avd-path.test.ts test/check-deps.test.ts diff --git a/packages/registry/src/failure-codes.ts b/packages/registry/src/failure-codes.ts index 598a080a8..2ca7dfb37 100644 --- a/packages/registry/src/failure-codes.ts +++ b/packages/registry/src/failure-codes.ts @@ -216,6 +216,11 @@ export const FAILURE_CODES = { SCREEN_RECORDING_SERVER_SHUTTING_DOWN: "SCREEN_RECORDING_SERVER_SHUTTING_DOWN", SCREEN_RECORDING_STREAM_UNAVAILABLE: "SCREEN_RECORDING_STREAM_UNAVAILABLE", SCREEN_RECORDING_FFMPEG_NOT_FOUND: "SCREEN_RECORDING_FFMPEG_NOT_FOUND", + // ffmpeg is installed but the build cannot encode H.264 — a `--disable-gpl` + // build has no libx264. Distinct from NOT_FOUND because the code is shown to + // the user and the fix is different: install a full build or point + // ARGENT_FFMPEG at one, NOT "install ffmpeg". + SCREEN_RECORDING_FFMPEG_UNUSABLE: "SCREEN_RECORDING_FFMPEG_UNUSABLE", FLOW_PROJECT_ROOT_REQUIRED: "FLOW_PROJECT_ROOT_REQUIRED", FLOW_PROJECT_ROOT_INVALID: "FLOW_PROJECT_ROOT_INVALID", diff --git a/packages/skills/skills/argent-screen-recording/SKILL.md b/packages/skills/skills/argent-screen-recording/SKILL.md index 216f6e96a..0fccada9a 100644 --- a/packages/skills/skills/argent-screen-recording/SKILL.md +++ b/packages/skills/skills/argent-screen-recording/SKILL.md @@ -44,5 +44,5 @@ A recording does not stop itself before its `timeLimitSeconds` cap, so a forgott - **The timeline is paced to a steady 30 fps**: a device only emits a frame when its screen changes, so captured frames are re-paced onto a fixed timeline rather than bunching up. With static-frame trimming off (`trimStatic: false`) that timeline is wall-clock accurate — a completely still screen still comes back as a full-length video (compressing to almost nothing) and `durationMs` matches the time you actually recorded. With trimming on (the default, see §3) still stretches past the grace window are collapsed, so `durationMs` is the trimmed video length and `wallClockMs` carries the real elapsed time. - **Android**: records at the device's native resolution; secure screens (DRM, some password fields) come out black. - **Unsupported**: tvOS simulators, physical iPhones, Chromium apps, Vega/Fire TV, and remote (`remote:`-prefixed) simulators — none of them expose a readable frame stream. For a single still frame use `screenshot`; for a replayable interaction script use `argent-create-flow` instead of a video. -- **ffmpeg is required**: it is the encoder, so `screen-recording-start` fails up front with an install hint if it is missing (`brew install ffmpeg` on macOS, `apt install ffmpeg` on Debian/Ubuntu). It is resolved from `PATH` plus the usual Homebrew prefixes. It must be a build with libx264 — on Fedora the default `ffmpeg-free` package lacks it and encoding fails after start. +- **ffmpeg is required**: it is the encoder, so `screen-recording-start` fails up front with an install hint if it is missing (`brew install ffmpeg` on macOS, `apt install ffmpeg` on Debian/Ubuntu). It is resolved from `PATH` plus the usual Homebrew prefixes, and each candidate is checked for the `libx264` encoder — a `--disable-gpl` build cannot record, which is what conda-forge's ffmpeg and Fedora's default `ffmpeg-free` package are, so a good build further down the list is used instead. Set `ARGENT_FFMPEG=/path/to/ffmpeg` to pick one explicitly. - **Watermark**: the Argent logo + "By @swmansion" is stamped bottom-left while encoding, faint (20% opacity) and per-pixel contrast-matched to the background (light logo over dark UI, dark logo over light UI). On by default — turn it off with `argent disable video-watermark` (re-enable with `argent enable video-watermark`). The flag is read when the recording starts. diff --git a/packages/tool-server/src/tools/screen-recording/capture.ts b/packages/tool-server/src/tools/screen-recording/capture.ts index 76704f362..e41a06b70 100644 --- a/packages/tool-server/src/tools/screen-recording/capture.ts +++ b/packages/tool-server/src/tools/screen-recording/capture.ts @@ -20,7 +20,12 @@ import { type StartRecordingResult, type StopRecordingFile, } from "./session-guards"; -import { buildWatermarkGraph, resolveFfmpeg, writeLogoTemp } from "./watermark"; +import { + buildWatermarkGraph, + ffmpegUnavailableMessage, + resolveFfmpeg, + writeLogoTemp, +} from "./watermark"; /** * Platform-agnostic screen capture, driven entirely by simulator-server — the @@ -290,17 +295,19 @@ async function startCaptureLocked( } ): Promise { const ffmpeg = await resolveFfmpeg(); - if (!ffmpeg) { - throw new FailureError( - "`ffmpeg` was not found on PATH. Install it with your system package manager (`brew install ffmpeg` on macOS, `apt install ffmpeg` on Debian/Ubuntu; on Fedora use RPM Fusion's `ffmpeg`, since the default `ffmpeg-free` build has no libx264) or see https://ffmpeg.org/download.html, then retry.", - { - error_code: FAILURE_CODES.SCREEN_RECORDING_FFMPEG_NOT_FOUND, - failure_stage: "screen_recording_resolve_ffmpeg", - failure_area: "tool_server", - error_kind: "dependency_missing", - failure_command: "ffmpeg", - } - ); + if (!ffmpeg.ok) { + throw new FailureError(ffmpegUnavailableMessage(ffmpeg), { + // "found but cannot encode" is a different problem with a different fix + // than "not installed", and the code is rendered to the user verbatim. + error_code: + ffmpeg.reason === "unusable" + ? FAILURE_CODES.SCREEN_RECORDING_FFMPEG_UNUSABLE + : FAILURE_CODES.SCREEN_RECORDING_FFMPEG_NOT_FOUND, + failure_stage: "screen_recording_resolve_ffmpeg", + failure_area: "tool_server", + error_kind: "dependency_missing", + failure_command: "ffmpeg", + }); } const outputFile = path.join( @@ -333,7 +340,7 @@ async function startCaptureLocked( // (shutdown) while this start was suspended above, abort now rather than // spawn an encoder the teardown can no longer reap. assertNotDisposed(api, "screen_recording_start"); - child = spawn(ffmpeg, ffmpegArgs({ outputFile, logoFile, graph }), { + child = spawn(ffmpeg.path, ffmpegArgs({ outputFile, logoFile, graph }), { stdio: ["pipe", "ignore", "pipe"], }); // Visible to dispose() while the fail-fast grace is pending (captureProcess diff --git a/packages/tool-server/src/tools/screen-recording/ffmpeg-binary.ts b/packages/tool-server/src/tools/screen-recording/ffmpeg-binary.ts new file mode 100644 index 000000000..37de48d85 --- /dev/null +++ b/packages/tool-server/src/tools/screen-recording/ffmpeg-binary.ts @@ -0,0 +1,292 @@ +import { execFile } from "node:child_process"; +import { promisify } from "node:util"; +import { access, realpath } from "node:fs/promises"; +import { constants } from "node:fs"; +import * as path from "node:path"; +import { commandOnPath } from "../../utils/command-on-path"; + +const execFileAsync = promisify(execFile); + +/** + * Which ffmpeg to record with. + * + * ffmpeg IS the recorder — it encodes simulator-server's frame stream straight + * to mp4 with `-c:v libx264` (see `ffmpegArgs` in capture.ts), so a build + * without libx264 can never work. Resolving by name alone is not enough: a + * `--disable-gpl` build (conda-forge ships one) has no libx264, and when it sits + * ahead of a good build on PATH every recording dies with + * `Unrecognized option 'preset'` — `-preset` being a libx264-private option. + * That is issue #621, and the known-good build was sitting in the fallback list + * the whole time, unreachable because the list was only consulted when ffmpeg + * was *absent*. + * + * So each candidate is asked whether it can actually encode, and the first one + * that says yes wins. + */ + +/** Package-manager prefixes to try when PATH has no usable ffmpeg. */ +const FFMPEG_FALLBACK_PATHS = [ + "/opt/homebrew/bin/ffmpeg", + "/usr/local/bin/ffmpeg", + "/usr/bin/ffmpeg", +]; + +/** + * `-hide_banner` is load-bearing, NOT cosmetic: without it ffmpeg writes its + * build banner to stderr, and that banner's `configuration:` line contains the + * literal `--enable-libx264`. Since the verdict is taken from stdout AND stderr + * combined, dropping this flag would put a libx264-shaped string in front of any + * looser matcher. (The marker below happens to survive it, but the next person + * to relax the regex should not have to discover that.) + * + * Deliberately no `-loglevel`: it does not gate help output either way + * (measured: `-loglevel error` and even `-loglevel quiet` still print the full + * encoder help), so it would add a version-dependent variable for nothing. + */ +const PROBE_ARGS = ["-hide_banner", "-h", "encoder=libx264"]; + +/** + * Generous purely as hang insurance — the probe measures ~33ms. It is longer + * than `commandOnPath`'s 2s on purpose: this execs the real binary, which may be + * on a stalled network mount or paying a first-run translation cost. + */ +const PROBE_TIMEOUT_MS = 5_000; + +/** + * ffmpeg answers `-h encoder=libx264` with `Encoder libx264 [libx264 H.264 …]:` + * when it has the encoder, and `Codec 'libx264' is not recognized by FFmpeg.` + * when it does not — **exiting 0 either way**. The verdict therefore has to come + * from the output, never from the exit status. + * + * Matching the success header rather than the failure sentence is deliberate: + * `Encoder []:` comes from ffmpeg's help formatter and has been + * stable for a decade, while the failure text is ordinary prose that any release + * may reword. Keying on the failure string would mean a future ffmpeg silently + * re-breaks recording — and it would break it for people whose setup works, + * which is the one direction this must never fail in. + * + * `\bencoder\s+` (rather than a bare `libx264`) keeps `--enable-libx264` and + * `libx264rgb` from counting. + */ +const LIBX264_MARKER = /\bencoder\s+libx264\b/i; + +/** Point argent at a specific ffmpeg when discovery cannot find a usable one. */ +const FFMPEG_OVERRIDE_ENV = "ARGENT_FFMPEG"; + +export type FfmpegResolution = + | { ok: true; path: string; origin: "override" | "path" | "fallback" } + | { ok: false; reason: "missing" | "unusable"; override: string | null; tried: string[] }; + +type Verdict = + /** Answered with the encoder header — it can record. */ + | "supported" + /** Answered, but without the header: a real "I don't have libx264". */ + | "unsupported" + /** Nothing to execute at that path. */ + | "absent" + /** Something is there but this user cannot execute it. */ + | "unrunnable" + /** No trustworthy answer — timed out, was killed, or said nothing. */ + | "inconclusive"; + +/** + * Ask one binary whether it can encode H.264. + * + * Anything short of a clear "no" is inconclusive, and an inconclusive candidate + * is still usable (see {@link resolveFfmpeg}). The probe exists to demote a + * build that positively told us it lacks libx264 — it must never be the reason + * a working setup stops recording. + */ +async function probeLibx264(binary: string): Promise { + try { + const { stdout, stderr } = await execFileAsync(binary, PROBE_ARGS, { + timeout: PROBE_TIMEOUT_MS, + }); + return LIBX264_MARKER.test(`${stdout}\n${stderr}`) ? "supported" : "unsupported"; + } catch (err) { + const e = err as NodeJS.ErrnoException & { + stdout?: string; + stderr?: string; + killed?: boolean; + signal?: string | null; + }; + // execFile still captures output when the child exits non-zero, so a build + // that answers correctly and *then* exits non-zero is still supported. + const output = `${e.stdout ?? ""}\n${e.stderr ?? ""}`; + if (LIBX264_MARKER.test(output)) return "supported"; + + // A spawn failure sets a STRING code (ENOENT/EACCES); a non-zero exit sets a + // NUMBER. Only the string form tells us anything about the binary itself. + if (typeof e.code === "string") { + if (e.code === "ENOENT") return "absent"; + // Not "absent": the file is there. Calling it missing would make the error + // tell a user who HAS ffmpeg installed to go and install it. + if (e.code === "EACCES" || e.code === "EPERM") return "unrunnable"; + return "inconclusive"; + } + + // Checked before the output test on purpose: a killed process may have + // printed something first, but a partial answer from a run that never + // finished is not evidence that libx264 is absent. + if (e.killed || e.signal) return "inconclusive"; + + return output.trim() ? "unsupported" : "inconclusive"; + } +} + +/** Resolve the override, which may be a path or a bare command name. */ +async function resolveOverridePath(value: string): Promise { + if (value.includes("/") || value.includes("\\") || path.isAbsolute(value)) { + return (await isExecutable(value)) ? value : null; + } + // Bare name: go through commandOnPath, which validates the name before it + // reaches a shell. The env value is user input and must never be interpolated. + return commandOnPath(value); +} + +async function isExecutable(p: string): Promise { + try { + // X_OK, not F_OK: a present-but-unexecutable file would only surface as an + // opaque EACCES at spawn time. + await access(p, constants.X_OK); + return true; + } catch { + return false; + } +} + +/** Canonical identity for dedup, so one binary is never probed twice. */ +async function canonical(p: string): Promise { + const resolved = await realpath(p).catch(() => p); + return process.platform === "win32" ? resolved.toLowerCase() : resolved; +} + +/** + * Candidates in priority order, deduplicated. + * + * `/opt/homebrew/bin/ffmpeg` is both the usual PATH hit and the first fallback, + * and on Intel macs `/usr/local/bin/ffmpeg` symlinks to the same Cellar binary, + * so without dedup the healthy host probes one file two or three times. Dedup on + * the realpath but keep — and later spawn — the path we started from: that is + * the name the user recognises, and a wrapper script must not be bypassed. + */ +async function collectCandidates(): Promise> { + const out: Array<{ path: string; origin: "path" | "fallback" }> = []; + const seen = new Set(); + + const add = async (p: string, origin: "path" | "fallback") => { + const key = await canonical(p); + if (seen.has(key)) return; + seen.add(key); + out.push({ path: p, origin }); + }; + + // commandOnPath returns an ABSOLUTE path and works on Windows, where the old + // hand-rolled `/bin/sh -c command -v` could never match. Resolving to an + // absolute path also means the binary we validate is the binary we spawn — + // otherwise the probe proves nothing about what actually runs. + const onPath = await commandOnPath("ffmpeg"); + if (onPath) await add(onPath, "path"); + + for (const p of FFMPEG_FALLBACK_PATHS) { + if (await isExecutable(p)) await add(p, "fallback"); + } + return out; +} + +/** + * Pick an ffmpeg that can record, or explain why none can. + * + * Not cached, deliberately. The tool-server has no idle shutdown by default, so + * a cached "no usable ffmpeg" would outlive the user installing one — they would + * follow the advice in our own error message and watch it keep failing. One + * probe per `screen-recording-start` (a human action, minutes apart) is nothing + * against a start path that already waits ~800ms before it declares success. + */ +export async function resolveFfmpeg(): Promise { + const override = (process.env[FFMPEG_OVERRIDE_ENV] ?? "").trim(); + if (override) { + const resolved = await resolveOverridePath(override); + if (!resolved) return { ok: false, reason: "missing", override, tried: [] }; + + const verdict = await probeLibx264(resolved); + if (verdict === "absent") return { ok: false, reason: "missing", override, tried: [] }; + if (verdict === "unrunnable") { + return { ok: false, reason: "unusable", override, tried: [resolved] }; + } + // The probe is ADVISORY here, and that is the whole point of the override. + // Its job is to rescue the user whose ffmpeg the probe misjudges — a fork + // whose help output we don't recognise reads as "unsupported", and refusing + // it would make the escape hatch subject to the very filter it exists to + // escape. If the binary really is libx264-less they get ffmpeg's own error, + // which is exactly what they got before this change. + return { ok: true, path: resolved, origin: "override" }; + } + + const candidates = await collectCandidates(); + const tried: string[] = []; + let fallbackToInconclusive: { path: string; origin: "path" | "fallback" } | null = null; + + for (const candidate of candidates) { + const verdict = await probeLibx264(candidate.path); + if (verdict === "supported") + return { ok: true, path: candidate.path, origin: candidate.origin }; + if (verdict === "absent") continue; // vanished between the check and the exec + if (verdict === "inconclusive") { + fallbackToInconclusive ??= candidate; + continue; + } + tried.push(candidate.path); + } + + // Nothing said yes, but something never gave a straight answer — use it. On + // any host where recording worked before, this is the branch that keeps it + // working: ffmpeg gets to speak for itself, exactly as it did previously. + if (fallbackToInconclusive) { + return { ok: true, path: fallbackToInconclusive.path, origin: fallbackToInconclusive.origin }; + } + + return tried.length > 0 + ? { ok: false, reason: "unusable", override: null, tried } + : { ok: false, reason: "missing", override: null, tried: [] }; +} + +/** + * The user-facing explanation. Pure, so it can be tested without mocking + * anything — and so the wording is decided in one place rather than at a throw + * site. + */ +export function ffmpegUnavailableMessage(result: Extract): string { + const { override, reason, tried } = result; + + if (override) { + return reason === "missing" + ? `\`${FFMPEG_OVERRIDE_ENV}\` is set to \`${override}\`, but there is no executable there. ` + + `Point it at an ffmpeg binary, or unset it to let argent search PATH.` + : `\`${FFMPEG_OVERRIDE_ENV}\` points at \`${override}\`, but it could not be run (check its ` + + `permissions). Point it at an executable ffmpeg, or unset it to let argent search PATH.`; + } + + if (reason === "missing") { + return ( + "`ffmpeg` was not found on PATH or at " + + `${FFMPEG_FALLBACK_PATHS.join(", ")}. ` + + "Install it with your system package manager (`brew install ffmpeg` on macOS, " + + "`apt install ffmpeg` on Debian/Ubuntu; on Fedora use RPM Fusion's `ffmpeg`, since the " + + "default `ffmpeg-free` build has no libx264) or see https://ffmpeg.org/download.html, " + + "then retry." + ); + } + + // The case that made this message worth building: saying "ffmpeg was not + // found" to someone with three ffmpegs installed is what sent the reporter + // looking in the wrong place. + return ( + `Found ffmpeg at ${tried.join(", ")}, but none of them can record: recording needs the ` + + "`libx264` encoder to write H.264, and a `--disable-gpl` build does not have it — " + + "conda-forge's and Fedora's default `ffmpeg-free` are both built that way. Install a full " + + "build ahead of it on PATH (`brew install ffmpeg` on macOS, `apt install ffmpeg` on " + + "Debian/Ubuntu, RPM Fusion's `ffmpeg` on Fedora), or point argent straight at one with " + + `\`${FFMPEG_OVERRIDE_ENV}=/path/to/ffmpeg\`.` + ); +} diff --git a/packages/tool-server/src/tools/screen-recording/screen-recording-start.ts b/packages/tool-server/src/tools/screen-recording/screen-recording-start.ts index 608d0d8ff..5e1eca2ad 100644 --- a/packages/tool-server/src/tools/screen-recording/screen-recording-start.ts +++ b/packages/tool-server/src/tools/screen-recording/screen-recording-start.ts @@ -77,7 +77,7 @@ By default every tap, swipe, drag, pinch and rotate is drawn into the video as a The recording keeps running across other tool calls (every result carries a reminder) until \`screen-recording-stop\` is called or timeLimitSeconds elapses — immediately after starting, set yourself a reminder/wakeup for the expected end of the recording so it is never left running. Use when the user wants a video of an interaction, animation, or app behavior — for a single still frame use \`screenshot\` instead. Returns { status: "recording", timeLimitSeconds, outputFile } — the video is retrieved later by \`screen-recording-stop\`, not by reading outputFile directly. -Fails if a recording is already running on the device, the device is not booted, ffmpeg is not installed, or the platform cannot be recorded (tvOS, Chromium, Vega and remote simulators are unsupported).`, +Fails if a recording is already running on the device, the device is not booted, ffmpeg is missing or cannot encode H.264, or the platform cannot be recorded (tvOS, Chromium, Vega and remote simulators are unsupported).`, searchHint: "record video screen capture movie mp4 start filming screencast", zodSchema, // simulator-server is resolved inside execute, not declared here: a tvOS diff --git a/packages/tool-server/src/tools/screen-recording/watermark.ts b/packages/tool-server/src/tools/screen-recording/watermark.ts index 775e2a57a..015a4e0dc 100644 --- a/packages/tool-server/src/tools/screen-recording/watermark.ts +++ b/packages/tool-server/src/tools/screen-recording/watermark.ts @@ -1,24 +1,18 @@ import { promises as fs } from "fs"; -import { execFile } from "child_process"; -import { promisify } from "util"; import os from "os"; import path from "path"; import { WATERMARK_PNG_BASE64, WATERMARK_PNG_WIDTH, WATERMARK_PNG_HEIGHT } from "./watermark-asset"; -const execFileAsync = promisify(execFile); - /** Frame rate of the recorded video; every input in the graph runs at it. */ const OUTPUT_FPS = 30; -// ffmpeg IS the recorder (it encodes simulator-server's frame stream straight -// to mp4), so resolve it from PATH first, then the usual package-manager -// prefixes for hosts where the tool-server's PATH is sanitized (launchd / -// login-shell differences). -const FFMPEG_FALLBACK_PATHS = [ - "/opt/homebrew/bin/ffmpeg", - "/usr/local/bin/ffmpeg", - "/usr/bin/ffmpeg", -]; +// ffmpeg resolution lives in ./ffmpeg-binary, but is re-exported here because +// capture.ts imports it from this module and the recording tests mock this +// module path. Keeping the seam means those mocks still disarm the resolver — +// without it they would silently stop intercepting and CI would exec a real +// ffmpeg inside a fake-timers test. +export { resolveFfmpeg, ffmpegUnavailableMessage } from "./ffmpeg-binary"; +export type { FfmpegResolution } from "./ffmpeg-binary"; // Watermark geometry, all relative to the frame WIDTH so it scales with any // device resolution. @@ -49,30 +43,6 @@ interface WatermarkBox { y: number; } -/** Locate a binary on PATH, falling back to common install prefixes. */ -async function resolveBinary(name: string, fallbacks: string[]): Promise { - try { - await execFileAsync("/bin/sh", ["-c", `command -v ${name}`], { timeout: 2_000 }); - return name; - } catch { - // not on PATH - } - for (const p of fallbacks) { - try { - await fs.access(p); - return p; - } catch { - // keep looking - } - } - return null; -} - -/** Absolute path (or bare name) of the ffmpeg to record with; null if absent. */ -export function resolveFfmpeg(): Promise { - return resolveBinary("ffmpeg", FFMPEG_FALLBACK_PATHS); -} - // yuv420p (what the encoder writes) subsamples chroma 2x, so crop/scale // dimensions AND offsets must be even - otherwise ffmpeg rounds the video crop // down to even while the rgba logo scales to the odd value, and maskedmerge diff --git a/packages/tool-server/test/ffmpeg-resolver.test.ts b/packages/tool-server/test/ffmpeg-resolver.test.ts new file mode 100644 index 000000000..ab2593518 --- /dev/null +++ b/packages/tool-server/test/ffmpeg-resolver.test.ts @@ -0,0 +1,394 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; + +/** + * Issue #621: a `--disable-gpl` ffmpeg on PATH (conda-forge ships one) has no + * libx264, so every recording dies with `Unrecognized option 'preset'` while a + * perfectly good build sits unreachable in the fallback list. + * + * The trap these tests exist to keep shut: ffmpeg reports the missing encoder in + * its OUTPUT and exits 0 either way, so a probe written as + * `try { await execFileAsync(...) } catch { next }` looks like a fix and changes + * nothing. + */ + +const execFileMock = vi.fn(); + +// The variant that attaches stdout/stderr to the rejection (as execFile really +// does) — without it the "answered correctly, then exited non-zero" case cannot +// be expressed at all. +vi.mock("node:child_process", async () => { + const actual = await vi.importActual("node:child_process"); + return { + ...actual, + execFile: ( + cmd: string, + args: readonly string[], + opts: unknown, + cb?: (err: Error | null, out: { stdout: string; stderr: string }) => void + ) => { + const callback = typeof opts === "function" ? opts : cb!; + const result = execFileMock(cmd, args); + if (result instanceof Error) { + const e = result as Error & { stdout?: string; stderr?: string }; + callback(e, { stdout: e.stdout ?? "", stderr: e.stderr ?? "" }); + } else callback(null, result ?? { stdout: "", stderr: "" }); + }, + }; +}); + +const commandOnPathMock = vi.fn(async (_name: string): Promise => null); +vi.mock("../src/utils/command-on-path", () => ({ + commandOnPath: (name: string) => commandOnPathMock(name), +})); + +// Pinned so a real /opt/homebrew/bin/ffmpeg on the developer's machine cannot +// leak into a result. +const executablePaths = new Set(); +const realpathMap = new Map(); +vi.mock("node:fs/promises", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + access: async (p: string) => { + if (!executablePaths.has(p)) throw Object.assign(new Error("ENOENT"), { code: "ENOENT" }); + }, + realpath: async (p: string) => realpathMap.get(p) ?? p, + }; +}); + +import { + resolveFfmpeg, + ffmpegUnavailableMessage, +} from "../src/tools/screen-recording/ffmpeg-binary"; + +const CONDA = "/opt/miniconda3/bin/ffmpeg"; +const BREW = "/opt/homebrew/bin/ffmpeg"; +const USR_LOCAL = "/usr/local/bin/ffmpeg"; + +/** What a build WITH libx264 prints (stdout), verbatim from ffmpeg 7.1.1. */ +const SUPPORTED_OUT = { + stdout: + "Encoder libx264 [libx264 H.264 / AVC / MPEG-4 AVC / MPEG-4 part 10]:\n General capabilities: dr1 delay threads\n", + stderr: "", +}; +/** What a --disable-gpl build prints — note it EXITS 0. */ +const UNSUPPORTED_OUT = { + stdout: "Codec 'libx264' is not recognized by FFmpeg.\n", + stderr: "", +}; + +function spawnError(code: string): Error { + return Object.assign(new Error(`spawn ${code}`), { code, stdout: "", stderr: "" }); +} + +let savedOverride: string | undefined; + +beforeEach(() => { + execFileMock.mockReset(); + commandOnPathMock.mockReset(); + commandOnPathMock.mockResolvedValue(null); + executablePaths.clear(); + realpathMap.clear(); + savedOverride = process.env.ARGENT_FFMPEG; + delete process.env.ARGENT_FFMPEG; +}); + +afterEach(() => { + if (savedOverride === undefined) delete process.env.ARGENT_FFMPEG; + else process.env.ARGENT_FFMPEG = savedOverride; +}); + +describe("resolveFfmpeg — capability, not just presence", () => { + it("uses the PATH ffmpeg when it can encode, probing exactly once", async () => { + commandOnPathMock.mockResolvedValue(BREW); + execFileMock.mockReturnValue(SUPPORTED_OUT); + + await expect(resolveFfmpeg()).resolves.toEqual({ ok: true, path: BREW, origin: "path" }); + // Pins the short-circuit: a healthy host pays one exec, not four. + expect(execFileMock).toHaveBeenCalledTimes(1); + }); + + it("skips a PATH ffmpeg that lacks libx264 and reaches the fallback", async () => { + // The reported host, exactly: conda first on PATH, Homebrew in the fallbacks. + commandOnPathMock.mockResolvedValue(CONDA); + executablePaths.add(BREW); + execFileMock.mockImplementation((cmd: string) => + cmd === BREW ? SUPPORTED_OUT : UNSUPPORTED_OUT + ); + + await expect(resolveFfmpeg()).resolves.toEqual({ ok: true, path: BREW, origin: "fallback" }); + }); + + it("never consults the exit status — a good build that exits non-zero still wins", async () => { + commandOnPathMock.mockResolvedValue(BREW); + execFileMock.mockReturnValue( + Object.assign(new Error("Command failed"), { + code: 3, // NUMBER: a non-zero exit, not a spawn failure + stdout: SUPPORTED_OUT.stdout, + stderr: "", + }) + ); + + await expect(resolveFfmpeg()).resolves.toMatchObject({ ok: true, path: BREW }); + }); + + it("reads stderr as well as stdout", async () => { + commandOnPathMock.mockResolvedValue(BREW); + execFileMock.mockReturnValue({ stdout: "", stderr: SUPPORTED_OUT.stdout }); + + await expect(resolveFfmpeg()).resolves.toMatchObject({ ok: true, path: BREW }); + }); + + it("probes with the exact argv, so nobody quietly changes the question", async () => { + commandOnPathMock.mockResolvedValue(BREW); + execFileMock.mockReturnValue(SUPPORTED_OUT); + + await resolveFfmpeg(); + + // -hide_banner is load-bearing: without it ffmpeg writes its build banner to + // stderr, and that banner contains the literal `--enable-libx264`. + expect(execFileMock).toHaveBeenCalledWith(BREW, ["-hide_banner", "-h", "encoder=libx264"]); + }); +}); + +describe("resolveFfmpeg — an inconclusive answer must never break a working host", () => { + it("uses a candidate whose probe timed out", async () => { + commandOnPathMock.mockResolvedValue(BREW); + execFileMock.mockReturnValue( + Object.assign(new Error("timeout"), { + killed: true, + signal: "SIGTERM", + stdout: "", + stderr: "", + }) + ); + + await expect(resolveFfmpeg()).resolves.toEqual({ ok: true, path: BREW, origin: "path" }); + }); + + it("treats a killed probe as inconclusive even when it printed something first", async () => { + // Partial output from a run that never finished is not evidence of absence. + commandOnPathMock.mockResolvedValue(BREW); + execFileMock.mockReturnValue( + Object.assign(new Error("timeout"), { + killed: true, + signal: "SIGTERM", + stdout: "Codec 'libx26", + stderr: "", + }) + ); + + await expect(resolveFfmpeg()).resolves.toMatchObject({ ok: true, path: BREW }); + }); + + it("still prefers a candidate that positively supports libx264", async () => { + commandOnPathMock.mockResolvedValue(CONDA); + executablePaths.add(BREW); + execFileMock.mockImplementation((cmd: string) => + cmd === BREW + ? SUPPORTED_OUT + : Object.assign(new Error("timeout"), { killed: true, stdout: "", stderr: "" }) + ); + + await expect(resolveFfmpeg()).resolves.toMatchObject({ ok: true, path: BREW }); + }); +}); + +describe("resolveFfmpeg — candidate collection", () => { + it("probes one binary once when PATH and the fallback are the same file", async () => { + commandOnPathMock.mockResolvedValue(BREW); + executablePaths.add(BREW); + execFileMock.mockReturnValue(SUPPORTED_OUT); + + await resolveFfmpeg(); + + expect(execFileMock).toHaveBeenCalledTimes(1); + }); + + it("dedups two prefixes that symlink to the same binary", async () => { + // The Intel-mac shape: /usr/local/bin/ffmpeg -> the Homebrew Cellar binary. + const cellar = "/opt/homebrew/Cellar/ffmpeg/7.1.1_3/bin/ffmpeg"; + commandOnPathMock.mockResolvedValue(USR_LOCAL); + executablePaths.add(BREW); + realpathMap.set(USR_LOCAL, cellar); + realpathMap.set(BREW, cellar); + execFileMock.mockReturnValue(UNSUPPORTED_OUT); + + const result = await resolveFfmpeg(); + + expect(execFileMock).toHaveBeenCalledTimes(1); + // And the path reported back is the one the user recognises, not the Cellar + // realpath — a wrapper at that prefix must not be bypassed either. + expect(result).toEqual({ + ok: false, + reason: "unusable", + override: null, + tried: [USR_LOCAL], + }); + }); + + it("reports 'missing' — and probes nothing — when there is no ffmpeg at all", async () => { + const result = await resolveFfmpeg(); + + expect(result).toEqual({ ok: false, reason: "missing", override: null, tried: [] }); + expect(execFileMock).not.toHaveBeenCalled(); + }); + + it("does not count a vanished binary as one that was tried and rejected", async () => { + // Deleted between the executable check and the exec. + commandOnPathMock.mockResolvedValue(BREW); + execFileMock.mockReturnValue(spawnError("ENOENT")); + + await expect(resolveFfmpeg()).resolves.toEqual({ + ok: false, + reason: "missing", + override: null, + tried: [], + }); + }); + + it("does not tell a user with an unrunnable ffmpeg that ffmpeg is missing", async () => { + // EACCES means the file is right there. "Install it" would be nonsense. + commandOnPathMock.mockResolvedValue(BREW); + execFileMock.mockReturnValue(spawnError("EACCES")); + + await expect(resolveFfmpeg()).resolves.toEqual({ + ok: false, + reason: "unusable", + override: null, + tried: [BREW], + }); + }); + + it("does not cache, so installing ffmpeg mid-session recovers", async () => { + // The tool-server has no idle shutdown by default; a sticky negative would + // outlive the user following the advice in our own error message. + const first = await resolveFfmpeg(); + expect(first.ok).toBe(false); + + commandOnPathMock.mockResolvedValue(BREW); + execFileMock.mockReturnValue(SUPPORTED_OUT); + + await expect(resolveFfmpeg()).resolves.toMatchObject({ ok: true, path: BREW }); + }); +}); + +describe("ARGENT_FFMPEG — the escape hatch must actually escape", () => { + it("wins over PATH without consulting it", async () => { + process.env.ARGENT_FFMPEG = "/opt/custom/ffmpeg"; + executablePaths.add("/opt/custom/ffmpeg"); + execFileMock.mockReturnValue(SUPPORTED_OUT); + + await expect(resolveFfmpeg()).resolves.toEqual({ + ok: true, + path: "/opt/custom/ffmpeg", + origin: "override", + }); + expect(commandOnPathMock).not.toHaveBeenCalled(); + }); + + it("is honoured even when the probe does not recognise the build", async () => { + // THE POINT OF THE OVERRIDE. The probe's one new failure mode is a false + // negative on a build whose help output we don't recognise; if the override + // were subject to the probe, the user it exists to rescue would have no way + // out. Worst case they get ffmpeg's own error — what they got before. + process.env.ARGENT_FFMPEG = "/opt/custom/ffmpeg"; + executablePaths.add("/opt/custom/ffmpeg"); + execFileMock.mockReturnValue({ stdout: "some fork's unfamiliar help text\n", stderr: "" }); + + await expect(resolveFfmpeg()).resolves.toEqual({ + ok: true, + path: "/opt/custom/ffmpeg", + origin: "override", + }); + }); + + it("never silently falls through to PATH", async () => { + // An override that is quietly ignored is its own bug: the user pinned a + // binary and must be told it is wrong, not handed a different one. + process.env.ARGENT_FFMPEG = "/opt/custom/ffmpeg"; + commandOnPathMock.mockResolvedValue(BREW); + executablePaths.add(BREW); + execFileMock.mockReturnValue(SUPPORTED_OUT); + + const result = await resolveFfmpeg(); + + expect(result).toEqual({ + ok: false, + reason: "missing", + override: "/opt/custom/ffmpeg", + tried: [], + }); + expect(execFileMock).not.toHaveBeenCalled(); + }); + + it("resolves a bare command name through PATH lookup", async () => { + process.env.ARGENT_FFMPEG = "ffmpeg7"; + commandOnPathMock.mockResolvedValue("/usr/bin/ffmpeg7"); + execFileMock.mockReturnValue(SUPPORTED_OUT); + + await expect(resolveFfmpeg()).resolves.toMatchObject({ + ok: true, + path: "/usr/bin/ffmpeg7", + origin: "override", + }); + expect(commandOnPathMock).toHaveBeenCalledWith("ffmpeg7"); + }); + + it("ignores a blank value rather than failing on it", async () => { + process.env.ARGENT_FFMPEG = " "; + commandOnPathMock.mockResolvedValue(BREW); + execFileMock.mockReturnValue(SUPPORTED_OUT); + + await expect(resolveFfmpeg()).resolves.toMatchObject({ origin: "path" }); + }); +}); + +describe("ffmpegUnavailableMessage", () => { + it("does not say 'not found' to someone who has ffmpeg installed", async () => { + const msg = ffmpegUnavailableMessage({ + ok: false, + reason: "unusable", + override: null, + tried: [CONDA, BREW], + }); + + expect(msg).not.toMatch(/not found/i); + expect(msg).toContain(CONDA); + expect(msg).toContain(BREW); + expect(msg).toContain("libx264"); + // The false-negative victim reads THIS message, so it has to name the way out. + expect(msg).toContain("ARGENT_FFMPEG"); + }); + + it("still tells someone with no ffmpeg how to install it", async () => { + const msg = ffmpegUnavailableMessage({ + ok: false, + reason: "missing", + override: null, + tried: [], + }); + + expect(msg).toMatch(/not found/i); + expect(msg).toContain("brew install ffmpeg"); + }); + + it("names the override when the override is the problem", async () => { + const missing = ffmpegUnavailableMessage({ + ok: false, + reason: "missing", + override: "/opt/custom/ffmpeg", + tried: [], + }); + expect(missing).toContain("ARGENT_FFMPEG"); + expect(missing).toContain("/opt/custom/ffmpeg"); + + const unusable = ffmpegUnavailableMessage({ + ok: false, + reason: "unusable", + override: "/opt/custom/ffmpeg", + tried: ["/opt/custom/ffmpeg"], + }); + expect(unusable).toContain("ARGENT_FFMPEG"); + }); +}); diff --git a/packages/tool-server/test/screen-recording.test.ts b/packages/tool-server/test/screen-recording.test.ts index 04a3f0336..7abf40fa2 100644 --- a/packages/tool-server/test/screen-recording.test.ts +++ b/packages/tool-server/test/screen-recording.test.ts @@ -20,7 +20,10 @@ vi.mock("../src/tools/screen-recording/watermark", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - resolveFfmpeg: vi.fn(async () => "/fake/ffmpeg"), + resolveFfmpeg: vi.fn(async () => ({ ok: true, path: "/fake/ffmpeg", origin: "path" })), + // Mocked too: the real one is pure, but leaving it live would run inside + // vi.useFakeTimers() and obscure which branch a failing assertion took. + ffmpegUnavailableMessage: vi.fn(() => "no usable ffmpeg"), writeLogoTemp: vi.fn(async () => "/tmp/fake-logo.png"), }; }); @@ -195,7 +198,7 @@ beforeEach(() => { mockSpawn.mockReset(); mockOpenStream.mockReset(); mockResolveFfmpeg.mockReset(); - mockResolveFfmpeg.mockResolvedValue("/fake/ffmpeg"); + mockResolveFfmpeg.mockResolvedValue({ ok: true, path: "/fake/ffmpeg", origin: "path" }); vi.useFakeTimers(); }); @@ -374,7 +377,12 @@ describe("screen recording capture", () => { it("fails the start when ffmpeg is not installed", async () => { const api = await makeSession(iosDevice); - mockResolveFfmpeg.mockResolvedValue(null); + mockResolveFfmpeg.mockResolvedValue({ + ok: false, + reason: "missing", + override: null, + tried: [], + }); try { await startAndSettle(api); @@ -389,6 +397,31 @@ describe("screen recording capture", () => { expect(api.startPending).toBe(false); }); + // Issue #621: ffmpeg IS installed — several of them — but none can encode + // H.264. Reporting that as "not found" is what sent the reporter looking in + // the wrong place, so it gets its own code. + it("fails the start with a distinct code when no ffmpeg can encode H.264", async () => { + const api = await makeSession(iosDevice); + mockResolveFfmpeg.mockResolvedValue({ + ok: false, + reason: "unusable", + override: null, + tried: ["/opt/miniconda3/bin/ffmpeg"], + }); + + try { + await startAndSettle(api); + expect.unreachable(); + } catch (err) { + expect(getFailureSignal(err)?.error_code).toBe( + FAILURE_CODES.SCREEN_RECORDING_FFMPEG_UNUSABLE + ); + } + expect(mockSpawn).not.toHaveBeenCalled(); + expect(getActiveScreenRecordings()).toHaveLength(0); + expect(api.startPending).toBe(false); + }); + it("fails the start (and closes the stream) when no frame ever arrives", async () => { const api = await makeSession(iosDevice); const streamError = new Error("no frame");