From f344867b5da2a4fe0fceb96eee9f41b808d336ed Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Tue, 8 Sep 2026 10:22:17 +0200 Subject: [PATCH] Pick ffmpeg CLI option spellings based on the ffmpeg version 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. --- mapillary_tools/ffmpeg.py | 90 ++++++++++++++++++++++++++++++++++++- tests/unit/test_ffmpeg.py | 95 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 183 insertions(+), 2 deletions(-) diff --git a/mapillary_tools/ffmpeg.py b/mapillary_tools/ffmpeg.py index 25443c36..0264e2b2 100644 --- a/mapillary_tools/ffmpeg.py +++ b/mapillary_tools/ffmpeg.py @@ -20,6 +20,21 @@ LOG = logging.getLogger(__name__) _MAX_STDERR_LENGTH = 2048 +# ffmpeg keeps deprecated CLI options around for a few releases and then drops +# them, so the spelling we can use depends on the binary we find at runtime. + +# "-/opt", which reads an option value from a file, was added in ffmpeg 7.1. +# The same release deprecated -filter_script, and ffmpeg 9.0 removed it. +_READ_OPTION_FROM_FILE_MIN_VERSION = (7, 1) + +# -fps_mode was added in ffmpeg 5.1, deprecating -vsync. +_FPS_MODE_MIN_VERSION = (5, 1) + +# Matches "ffmpeg version 7.1.5", "ffmpeg version n7.1.5" and +# "ffmpeg version 6.1.1-3ubuntu5". Git and nightly builds report things like +# "ffmpeg version N-121246-gd52c8dbc9d", which deliberately do not match. +_FFMPEG_VERSION_RE = re.compile(r"^ffmpeg version n?(\d+)\.(\d+)") + class StreamTag(T.TypedDict): creation_time: str @@ -98,6 +113,56 @@ def __init__( self.ffmpeg_path = ffmpeg_path self.ffprobe_path = ffprobe_path self.stderr = stderr + self._version: tuple[int, int] | None = None + self._version_probed = False + + def get_version(self) -> tuple[int, int] | None: + """ + Return the (major, minor) version of the ffmpeg binary, or None if it + can not be determined (git and nightly builds do not report one). + + The result is cached, so ffmpeg is only asked once per instance. + """ + if not self._version_probed: + self._version_probed = True + self._version = self._probe_version() + return self._version + + def _probe_version(self) -> tuple[int, int] | None: + try: + completed = subprocess.run( + [self.ffmpeg_path, "-version"], + check=True, + stdout=subprocess.PIPE, + stderr=subprocess.DEVNULL, + ) + except FileNotFoundError: + raise FFmpegNotFoundError( + f'The ffmpeg command "{self.ffmpeg_path}" not found' + ) + except subprocess.CalledProcessError: + LOG.warning("Failed to read the ffmpeg version from %s", self.ffmpeg_path) + return None + + first_line = completed.stdout.decode("utf-8", errors="replace").splitlines() + matched = _FFMPEG_VERSION_RE.match(first_line[0]) if first_line else None + if matched is None: + LOG.debug("Unable to parse the ffmpeg version from %s", self.ffmpeg_path) + return None + + return (int(matched.group(1)), int(matched.group(2))) + + def _supports(self, min_version: tuple[int, int]) -> bool: + """ + Whether the ffmpeg binary is new enough for a given option spelling. + + Builds that do not report a version are assumed to be new, because in + practice they are git or nightly builds tracking master. + """ + version = self.get_version() + if version is None: + return True + return version >= min_version def probe_format_and_streams(self, video_path: Path) -> ProbeOutput: """ @@ -276,9 +341,9 @@ def extract_specified_frames( *stream_selector, # Filter videos *[ - *["-filter_script:v", select_file.name], + *self._read_filter_from_file_args(select_file.name), # Each frame is passed with its timestamp from the demuxer to the muxer - *["-vsync", "0"], + *self._passthrough_fps_args(), # Set the number of video frames to output (this is an optimization to let ffmpeg stop early) *["-frames:v", str(len(frame_indices))], ], @@ -392,6 +457,27 @@ def iterate_samples( stream_specifier, frame_idx = result yield (stream_specifier, frame_idx, sample_path) + def _read_filter_from_file_args(self, filter_path: str) -> list[str]: + """ + Arguments that apply a video filter graph read from a file. + + ffmpeg 9.0 removed -filter_script, so newer binaries get the -/opt + syntax that replaced it in 7.1. + """ + if self._supports(_READ_OPTION_FROM_FILE_MIN_VERSION): + return ["-/filter:v", filter_path] + return ["-filter_script:v", filter_path] + + def _passthrough_fps_args(self) -> list[str]: + """ + Arguments that pass every frame through with its demuxer timestamp. + + -vsync has been deprecated since ffmpeg 5.1 in favour of -fps_mode. + """ + if self._supports(_FPS_MODE_MIN_VERSION): + return ["-fps_mode", "passthrough"] + return ["-vsync", "0"] + def run_ffmpeg_non_interactive(self, cmd: list[str]) -> None: """ Execute ffmpeg command in non-interactive mode. diff --git a/tests/unit/test_ffmpeg.py b/tests/unit/test_ffmpeg.py index f9dcbf07..c79fc8ce 100644 --- a/tests/unit/test_ffmpeg.py +++ b/tests/unit/test_ffmpeg.py @@ -263,3 +263,98 @@ def test_creation_time(expected, probe_creation_time, probe_duration): "2023-03-07 01:35:34", "4.933333", ) + + +def _ffmpeg_with_version(version): + ff = ffmpeg.FFMPEG() + ff._version_probed = True + ff._version = version + return ff + + +def test_parse_ffmpeg_version(): + """Distro and build suffixes must not defeat the version match.""" + parse = ffmpeg._FFMPEG_VERSION_RE.match + + for line, expected in [ + ("ffmpeg version 9.0.1 Copyright (c) 2000-2026", (9, 0)), + ("ffmpeg version 8.1.2 Copyright (c) 2000-2026", (8, 1)), + ("ffmpeg version n7.1.5 Copyright (c) 2000-2025", (7, 1)), + ("ffmpeg version 7.0.2-static https://johnvansickle.com/ffmpeg/", (7, 0)), + ("ffmpeg version 6.1.1-3ubuntu5 Copyright (c) 2000-2023", (6, 1)), + ]: + matched = parse(line) + assert matched is not None, line + assert (int(matched.group(1)), int(matched.group(2))) == expected, line + + # Git and nightly builds do not report a release version + assert parse("ffmpeg version N-121246-gd52c8dbc9d Copyright (c) 2000-2026") is None + + +def test_ffmpeg_version_is_probed_once(): + pytest_skip_if_not_ffmpeg_installed() + + ff = ffmpeg.FFMPEG() + assert ff.get_version() == ff.get_version() + assert ff._version_probed + + # An unreadable binary must not be mistaken for an old one + with pytest.raises(ffmpeg.FFmpegNotFoundError): + ffmpeg.FFMPEG(ffmpeg_path="not_exist_ffmpeg_binary").get_version() + + +def test_option_spelling_by_ffmpeg_version(): + """ffmpeg 9.0 dropped -filter_script, ffmpeg < 7.1 never had -/filter.""" + legacy_filter = ["-filter_script:v", "/tmp/f.txt"] + modern_filter = ["-/filter:v", "/tmp/f.txt"] + + for version, expected in [ + ((6, 1), legacy_filter), + ((7, 0), legacy_filter), + ((7, 1), modern_filter), + ((9, 0), modern_filter), + # Unversioned git builds track master, so assume the modern spelling + (None, modern_filter), + ]: + ff = _ffmpeg_with_version(version) + assert ff._read_filter_from_file_args("/tmp/f.txt") == expected, version + + for version, expected in [ + ((5, 0), ["-vsync", "0"]), + ((5, 1), ["-fps_mode", "passthrough"]), + ((9, 0), ["-fps_mode", "passthrough"]), + (None, ["-fps_mode", "passthrough"]), + ]: + ff = _ffmpeg_with_version(version) + assert ff._passthrough_fps_args() == expected, version + + +def test_ffmpeg_extract_specified_frames_legacy_options_ok(setup_data: py.path.local): + """The pre-7.1 spelling must still extract the same frames. + + ffmpeg 7.1 through 8.x accept both spellings, so on those binaries this + pins the legacy branch; elsewhere it is skipped. + """ + pytest_skip_if_not_ffmpeg_installed() + + if not (7, 1) <= (ffmpeg.FFMPEG().get_version() or (0, 0)) < (9, 0): + pytest.skip("ffmpeg does not accept both the legacy and modern spellings") + + video_path = Path(setup_data.join("videos/sample-5s.mp4")) + digests = [] + + for version in [(7, 0), (7, 1)]: + sample_dir = Path(setup_data.join(f"videos/samples_{version[0]}_{version[1]}")) + sample_dir.mkdir() + + ff = _ffmpeg_with_version(version) + ff.extract_specified_frames(video_path, sample_dir, frame_indices={2, 9}) + + results = list(ff.sort_selected_samples(sample_dir, video_path)) + assert len(results) == 2, version + digests.append( + [p.read_bytes() for _, paths in results for p in paths if p is not None] + ) + + # Both spellings must select the same frames byte for byte + assert digests[0] == digests[1]