Pick ffmpeg CLI option spellings based on the ffmpeg version - #829
Open
caglarpir wants to merge 1 commit into
Open
Pick ffmpeg CLI option spellings based on the ffmpeg version#829caglarpir wants to merge 1 commit into
caglarpir wants to merge 1 commit into
Conversation
ffmpeg 9.0 removed -filter_script, which extract_specified_frames() uses to pass a large select filter without hitting the 32767 character Windows command line limit. On ffmpeg 9 the whole invocation now fails during argument parsing: Error splitting the argument list: Option not found so frame sampling is broken -- video_process and sample_video cannot extract anything. This is not platform specific; CI only caught it on Windows because setup-ffmpeg resolves "release" to 9.0.1 there while Ubuntu gets a 7.0.2 static build and macOS installs no ffmpeg at all. -filter_script was deprecated in ffmpeg 7.1, replaced by the -/opt syntax for reading an option value from a file. The supported spellings therefore do not overlap across the versions we care about: <= 7.0 only -filter_script 7.1-8.x both >= 9.0 only -/filter so this cannot be a straight substitution. Probe the version once per FFMPEG instance, cache it, and pick accordingly. While here, do the same for -vsync, deprecated since ffmpeg 5.1 in favour of -fps_mode. It is still accepted in 8.x, but it is the only other deprecated option we pass, and the CLI aborts on the first unrecognized option, so fixing -filter_script alone risks simply surfacing -vsync next on some future release. Builds that report no release version (git and nightly builds, e.g. "ffmpeg version N-121246-gd52c8dbc9d") are assumed to be new, since in practice they track master. Verified on ffmpeg 8.1.2, which accepts both spellings: -vsync 0 and -fps_mode passthrough produce byte identical output, as do -filter_script:v and -/filter:v. The new test pins that equivalence on any 7.1-8.x binary.
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
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 #826
Summary
FFMPEG.extract_specified_frames()passes-filter_script:vto hand ffmpeg a largeselectfilter via a file, avoiding the 32767 character Windows command line limit. ffmpeg 9.0 removed that option, so on ffmpeg 9 the whole invocation dies during argument parsing:Frame sampling is then completely broken —
video_processandsample_videocannot extract anything.This is not platform specific. #826 was reported on Windows 11 and reproduced independently on macOS, with the same command and the same error. It shows up only on Windows in CI because
setup-ffmpegresolvesreleaseto a different build per platform, in the same workflow run:if: matrix.platform != 'macos-latest')Any user on ffmpeg 9 is affected on any OS. This is also why
mainis currently red — the same 4 tests fail ona23dbef.Why it can't be a straight substitution
-filter_scriptwas deprecated in ffmpeg 7.1 and replaced by the-/optsyntax for reading an option value from a file. The supported spellings do not overlap:-filter_script-/filterSwapping unconditionally would break CI's own Ubuntu runner on 7.0.2. So
FFMPEGnow probesffmpeg -versiononce per instance, caches it, and picks the spelling.-/filteris a syntax prefix rather than an enumerable option, so it cannot be feature-detected from-h full— the version is the only signal available short of a trial invocation.Also:
-vsync-vsynchas been deprecated since ffmpeg 5.1 in favour of-fps_mode, and it is the only other deprecated option we pass. It is present in both reporters' failing command lines, immediately after-filter_script:v. It still works in 8.x, but the CLI aborts on the first unrecognized option, so fixing-filter_scriptalone risks simply surfacing-vsyncnext on a future release. Same version-gated treatment (-fps_mode passthroughfor ≥ 5.1).Unversioned builds
Git and nightly builds report e.g.
ffmpeg version N-121246-gd52c8dbc9dwith no release number. Those are assumed to be new and get the modern spellings, since in practice they track master.FFmpegNotFoundErroris still raised for a missing binary, so an absent ffmpeg is never silently treated as an old one.Testing
Verified against ffmpeg 8.1.2, which accepts both spellings, so both branches can be compared directly on one binary:
-vsync 0and-fps_mode passthroughproduce byte-identical output-filter_script:vand-/filter:vproduce byte-identical outputNew tests:
test_parse_ffmpeg_version— distro/build suffixes (n7.1.5,7.0.2-static,6.1.1-3ubuntu5) parse; git builds deliberately do not.test_option_spelling_by_ffmpeg_version— boundaries at 7.0/7.1 and 5.0/5.1, plus the unversioned case.test_ffmpeg_version_is_probed_once— caching, and that a missing binary raises rather than degrading.test_ffmpeg_extract_specified_frames_legacy_options_ok— forces the pre-7.1 branch and asserts it selects the same frames byte for byte. Runs on any 7.1–8.x binary, skips elsewhere.Full suite locally: 715 passed, 18 skipped, 1 xfailed, 0 failures.
ruff check/ruff format --check/usort diff/mypyall clean.CI is green on all 16 checks. On the Windows / ffmpeg 9.0.1 jobs the four tests that fail on
mainnow pass:Both code paths get real coverage across the matrix: Ubuntu on 7.0.2 exercises the legacy spelling, Windows on 9.0.1 the modern one.
A note on the CI matrix
I deliberately left
.github/workflows/python-package.ymlalone. The accidental spread — Ubuntu on 7.0.2 and Windows on 9.0.1 — happens to exercise both sides of the 7.1 boundary, which is exactly the coverage this change needs. Worth being aware it is incidental rather than pinned, and that macOS installs no ffmpeg so it validates none of this — which is precisely why the macOS report in #826 went unnoticed by CI.