fix(screen-recording): pick an ffmpeg that can actually encode, not just one that exists - #680
Open
filip131311 wants to merge 1 commit into
Open
fix(screen-recording): pick an ffmpeg that can actually encode, not just one that exists#680filip131311 wants to merge 1 commit into
filip131311 wants to merge 1 commit into
Conversation
…ust one that exists 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
filip131311
force-pushed
the
filip/ffmpeg-capability-probe
branch
from
August 3, 2026 07:51
6d40425 to
5a3f896
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #621.
resolveBinarytrusted the firstcommand -v ffmpeghit unconditionally and consultedFFMPEG_FALLBACK_PATHSonly when ffmpeg was absent. A conda-forge build (--disable-gpl, no libx264) ahead of Homebrew on PATH therefore won every time, and the known-good build sat in the fallback list unreachable.Reproduced
This host happens to be in the reported state by accident, which is what makes the bug insidious:
-presetis a libx264-private option, which is why a missing encoder surfaces as an unknown option.The issue proposes "probe the resolved candidate (e.g.
ffmpeg -hide_banner -h encoder=libx264) and fall through". Measured on both builds:Both exit 0. ffmpeg reports the missing codec in its output, not its status. Implemented the natural way —
try { await execFileAsync(...) } catch { next }— the fix would look right and change nothing. The probe reads output, and there is a test asserting exactly that (never consults the exit status).Design decisions worth reviewing
Match the success header, not the failure text.
Encoder libx264 [...]comes from ffmpeg's help formatter and has been stable for a decade;Codec 'libx264' is not recognized by FFmpeg.is prose 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, the one direction this must never fail in.-hide_banneris load-bearing, not cosmetic. Without it ffmpeg writes its build banner to stderr, and that banner'sconfiguration:line contains the literal--enable-libx264. Since the verdict comes from stdout+stderr combined, dropping the flag would put a libx264-shaped string in front of any looser matcher. (The current regex survives it; the next person to relax it shouldn't have to find that out.) Related:-loglevelis deliberately not passed — measured, it does not gate help output either way, so it would add a version-dependent variable for nothing.Everything inconclusive is usable. A timeout, a kill, or empty output all mean "no trustworthy answer", and the candidate is used anyway. The probe can only ever demote a build that positively said it lacks libx264, so on any host where recording works today ffmpeg still gets to speak for itself.
ARGENT_FFMPEGexists because the probe does. The probe introduces a failure mode that did not exist before — a false negative on a fork whose help output we don't recognise. An override that were itself subject to the probe would rescue nobody, so it is deliberately advisory: an explicitly pinned binary is honoured even when the probe doesn't recognise it (worst case the user gets ffmpeg's own error, exactly what they got before). It is never silently swapped for a different binary either — a pinned path that isn't there fails loudly rather than falling through.A found-but-unusable ffmpeg is not "missing". The old message told a user with three ffmpegs installed to go and install ffmpeg. That case gets its own message and its own failure code, because
screen-recording-startrenders the code to the user verbatim. AnEACCESbinary counts as found for the same reason.resolveBinarydeleted, not extended. Its generic half duplicatedcommandOnPath— worse, its hand-rolled/bin/sh -c command -vcould never match on Windows, where Android recording is reachable. Routing throughcommandOnPathalso means the binary we validate is the binary we spawn, rather than resolving PATH twice and proving nothing about what runs.Not cached. The tool-server has no idle shutdown by default (
ARGENT_IDLE_TIMEOUT_MINUTESdefaults to0), so a cached "no usable ffmpeg" would outlive the user acting on the advice in our own error message. One ~33 ms probe per recording start, against a start path that already waits ~800 ms.Verified end to end
Ran a tool-server built from this branch against the still-broken host:
Tests
22 new cases in
test/ffmpeg-resolver.test.ts, hermetic (node:child_process,command-on-pathandnode:fs/promisesall mocked, so a real ffmpeg on the dev machine can't leak in): the reported host pinned exactly, exit-status-independence in both directions, stdout/stderr combining, the exact probe argv, fail-open on timeout/kill/partial output, dedup (same file via PATH+fallback, and two prefixes symlinked to one Cellar binary),ENOENT-vs-EACCES, no-caching, and all five override behaviours. Plus a newscreen-recording.test.tscase for the distinct failure code, and the message builder asserted not to say "not found" to someone who has ffmpeg.Mutation-verified: replacing the output check with an exit-status check fails 2 tests; making the override subject to the probe fails 1.
Full tool-server suite green (3109 passed / 298 files).
Notes for the reviewer
screen-recording-start.ts:80andargent-screen-recording/SKILL.md:47, both of which feat(screen-recording): record through simulator-server instead of host ffmpeg #587 also replaces — plus its@@ -281,13 @@hunk incapture.ts, whose trailing context includesif (!ffmpeg) {. All mechanical.resolveFfmpegkeeps its name and stays exported from./watermarkspecifically so feat(screen-recording): record through simulator-server instead of host ffmpeg #587's (and main's)vi.mockseam still disarms it.ARGENT_FFMPEGis undocumented inargent-private/docs/environment-variables.md— that file is in a submodule, so documenting it there is a separate PR. That table already omitsARGENT_TRACE_PROCESSOR_WASMandARGENT_NATIVE_DEVTOOLS_DIR, and nothing enforces completeness.ffmpegArgsis the only ffmpeg invocation in the repo (trimStaticis frame-dropping in the Node pump, not a second pass).child.kill("SIGINT")is an abrupt terminate on Windows. The new test is added to the curatedwindows-e2e.ymllist.