Pick ffmpeg CLI option spellings based on the ffmpeg version

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.

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]