Document ffmpeg 5.1+ for -fps_mode and fail the suite once when older

[HarshRajSinghania]

Fixes #491.

Summary

The test suite uses -fps_mode (src/hflow/video.py, src/hflow/episode.py). That option is not present in FFmpeg 4.4.x (Ubuntu 22.04 default), so a contributor on the distro binary gets encode-path assertion failures that look like their branch broke something.

Motivation

#491 asked for two things: name the minimum in CONTRIBUTING, and replace those three obscure failures with one message that states the required and found versions.

Implementation

• CONTRIBUTING now requires ffmpeg 5.1 or newer and names -fps_mode so the floor can be re-derived.
• A session-scoped autouse fixture in tests/conftest.py probes ffmpeg -version once. If the parsed version is older than 5.1 it calls pytest.exit with required vs found. Missing ffmpeg or an unparseable banner is left alone so existing skip/fail paths still apply.
• No check was added to the encode path.

How the 5.1 floor was established: -fps_mode replaced -vsync in the FFmpeg 5.1 CLI. This environment's ffmpeg is 6.1.1-3ubuntu5, which lists -fps_mode in ffmpeg -h full. Ubuntu 22.04's 4.4.2 does not.

Testing

• PYTHONPATH=. pytest -q tests/test_ffmpeg_version_guard.py — 2 passed
• Parser checks for Ubuntu 4.4.2, n5.1.2, and 6.1.1 banners
• Full uv sync --locked --all-extras / ruff / ty / full pytest were not run here (no project venv). Local ffmpeg is 6.1.1, which is above the floor.

[github-actions]

👋 Hi @HarshRajSinghania — thank you so much for your first contribution to HFlow!

A maintainer will review your pull request as soon as possible. In the meantime:

• ⭐ Star the repo: https://github.com/Hebbian-Robotics/hflow
• 💬 Join our Discord: https://discord.gg/vacepQvjmg
• 📖 Check CONTRIBUTING.md for the validation commands CI will run
• 📣 Share the project with your network

💡 Tip: one open pull request per contributor at a time. Issues with an assignee are taken; everything else is fair game.

We are excited to have you here and appreciate your help making the project better! 🙌

[kstonekuan]

ruff format --check fails on tests/conftest.py:51, so this would turn main red.

pytest.exit ends the whole session. Someone working on curation with an old ffmpeg now cannot run any test. #491 asked for the ffmpeg-dependent tests to skip or fail, not the run to abort.

Rest looks right, and naming -fps_mode in CONTRIBUTING so the floor can be re-derived is the part that keeps it true.

[HarshRajSinghania]

Thanks for the review.

Addressed both points:

• Replaced the session-wide pytest.exit with a collection hook that skips only ffmpeg-dependent tests (video/episode/encode/ffmpeg paths) when the suite ffmpeg is older than 5.1. Curation and other non-media tests can still run on FFmpeg 4.4.
• Reformatted the version-line parsing in tests/conftest.py so ruff format --check no longer fails on that file.

Pushed to this branch: 2f8629c.

[kstonekuan]

Formatting is fixed and the session no longer aborts, thanks.

The substring match is too blunt in both directions. On an old ffmpeg it skips 473 of 1928 tests, including tests/test_episode_to_numpy.py (10 of 10), the four packages/hflow-server/tests/test_server_episode_*.py files (59 tests), and 10 in test_catalog_curation.py. Those are the ones you set out to keep runnable. It also misses tests/test_topic_cache_collision.py, which calls ffmpeg_path() and would still fail the way #491 describes.

tests/test_video_concurrency.py:15 and tests/test_lerobot_converter.py:60 already carry _requires_system_ffmpeg. A registered marker applied to the media tests is the same idea without guessing from names.

CONTRIBUTING is good as-is.

[HarshRajSinghania]

Thanks — the path-substring skip was too blunt, as you measured.

Pushed:

• Dropped _FFMPEG_MEDIA_PATH_MARKERS / nodeid guessing.
• Registered a requires_system_ffmpeg pytest marker.
• The collection hook now skips only tests that carry that marker when the suite ffmpeg is older than 5.1.

That leaves test_episode_to_numpy, the server episode tests, and test_catalog_curation runnable on old ffmpeg. Tests that actually need -fps_mode should opt in with @pytest.mark.requires_system_ffmpeg (same idea as the existing _requires_system_ffmpeg skipif). CONTRIBUTING is unchanged.

[kstonekuan]

The marker is the right mechanism, and the blast radius is gone.

It is applied to zero tests, though. requires_system_ffmpeg is registered in pyproject.toml and read by the hook, but no test carries it, so on ffmpeg 4.4 the hook skips nothing and #491's three assertion failures happen exactly as before. The PR touches only CONTRIBUTING, pyproject.toml, and tests/conftest.py.

-fps_mode reaches ffmpeg from episode.py:715, video.py:205, and source_sampling.py:256. The tests covering those are what need the marker. test_lerobot_converter.py and test_video_concurrency.py already carry _requires_system_ffmpeg for a related reason and are the obvious starting point.

Worth confirming on a 4.4 binary that the marked set is the set that actually fails, rather than marking by eye.

[HarshRajSinghania]

Thanks — the marker was registered but unused.

Applied pytest.mark.requires_system_ffmpeg to the tests that actually hit -fps_mode:

• module-wide on tests/test_video.py (encode_images_to_h264 / video.py)
• test_canonical_episode_extracts_exact_source_frame_indices in tests/test_end_to_end.py (episode.py frame extract)
• test_converter_output_remuxes_without_tail_loss in tests/test_lerobot_converter.py (alongside the existing _requires_system_ffmpeg skipif)

test_video_concurrency.py / source_sampling.py are not on this branch. I did not mark the frames_at_indices validation tests that fail before ffmpeg runs.

Could not confirm the marked set against an ffmpeg 4.4 binary in this environment.

[kstonekuan]

The mechanism works now. I built a shim that reports the Ubuntu 22.04 4.4.2 banner, rejects -fps_mode the way 4.4 does, and forwards everything else to a real binary, then ran the suite against it. Skips go from 8 to 38, so the marker is doing its job.

23 tests still fail, in three files the marked set misses:

• tests/test_source_sampling.py (17)
• tests/test_egocentric_prepare.py (5)
• tests/test_lerobot_converter.py::test_converter_slices_exactly_the_declared_frame_count (1)

Sample failure, so you can see it is the real thing and not the shim misbehaving:

RuntimeError: ffmpeg failed (... -fps_mode passthrough -f h264 pipe:1):
Unrecognized option 'fps_mode'.
examples/egocentric/prepare.py:506

That points at something my last review got wrong. I listed three -fps_mode call sites from src/; there are four, and the fourth is examples/egocentric/prepare.py:498, which is what test_egocentric_prepare.py goes through. The complete set is episode.py:715, video.py:205, source_sampling.py:256, examples/egocentric/prepare.py:498.

test_source_sampling.py landed on main after your branch, so that one is not on you. test_egocentric_prepare.py and the lerobot converter test were both reachable.

Reproduce it with the shim rather than marking by eye:

printf '%s\n' '#!/bin/sh' \
  'case "$1" in -version) echo "ffmpeg version 4.4.2-0ubuntu0.22.04.1"; exit 0;; esac' \
  'for a in "$@"; do [ "$a" = "-fps_mode" ] && { echo "Unrecognized option '\''fps_mode'\''." >&2; exit 1; }; done' \
  'exec "$HFLOW_REAL_FFMPEG" "$@"' > /tmp/shim/ffmpeg && chmod +x /tmp/shim/ffmpeg
HFLOW_REAL_FFMPEG=$(command -v ffmpeg) PATH=/tmp/shim:$PATH uv run pytest -q

Merge main in first, then mark until that run is green.

[HarshRajSinghania]

Thanks for the shim run.

Marked tests/test_egocentric_prepare.py with pytest.mark.requires_system_ffmpeg (covers examples/egocentric/prepare.py).

Could not merge main onto this branch from here without a force-push, so tests/test_source_sampling.py is still absent on the branch. test_converter_slices_exactly_the_declared_frame_count is also not on this branch. Happy to mark those after main is merged in.

[kstonekuan]

Merged main into your branch and pushed it (d172326), so the two files you could not reach are there now. That commit is mine; everything else on the branch is yours.

The egocentric marking worked. Those five failures are gone and skips went from 38 to 45 under the shim.

18 left, and they are exactly the two you named:

• tests/test_source_sampling.py (17)
• tests/test_lerobot_converter.py::test_converter_slices_exactly_the_declared_frame_count (1)

Mark those and the shim run should be green, which is the bar for this PR.

[HarshRajSinghania]

Thanks for merging main onto the branch.

Marked the two remaining shim failures:

• module-wide pytestmark = pytest.mark.requires_system_ffmpeg on tests/test_source_sampling.py
• @pytest.mark.requires_system_ffmpeg on test_converter_slices_exactly_the_declared_frame_count

Pushed in 380a3b3.

[kstonekuan]

Both markers landed and the set you built was complete: 18 shim failures down to 1, skips up to 84.

Two things left, both small.

uv run ruff check fails with four E402s. pytestmark sits above the imports in tests/test_source_sampling.py, so everything after it reads as a late import. Move the assignment below the import block; the other marked files already do it that way.

The remaining shim failure is tests/test_blur.py, which is not yours. #579 landed a few hours ago and added src/hflow/blur.py:81 as a fifth -fps_mode call site with its own test. Mark that one too and the shim run is green.

Worth saying out loud: this will keep happening. The marker is the right mechanism, but nothing makes a new ffmpeg-dependent test carry it, so the set decays the moment someone adds one. A line in CONTRIBUTING next to the ffmpeg requirement would be enough. Entirely optional for this PR, and a fine follow-up issue if you would rather not widen it now.

[HarshRajSinghania]

Thanks — addressed the remaining items from the latest review.

• Moved pytestmark below the import block in tests/test_source_sampling.py so ruff check no longer reports E402.
• Applied pytest.mark.requires_system_ffmpeg on tests/test_blur.py (the fifth -fps_mode call site from Add reusable video preparation and raw measurement APIs #579).
• Merged latest main into this branch so that file is actually on the PR.
• Added a line next to the ffmpeg requirement in CONTRIBUTING.md that new ffmpeg-dependent tests must carry the marker.

Commit: HarshRajSinghania@a19f520

[kstonekuan]

The shim run is green now: 2198 passed, 88 skipped, nothing failing on a 4.4 binary. That was the bar, and the CONTRIBUTING line is what stops the set rotting the next time someone adds an ffmpeg test.

One thing left. tests/test_video.py still has pytestmark above its imports, so ruff check reports two E402s and this would redden main. You fixed the identical placement in test_source_sampling.py; test_video.py has it at line 9 with imports running to line 12.

I checked all four module-wide marks so this is the last of them:

ok   tests/test_source_sampling.py   mark 26, last import 24
ok   tests/test_blur.py              mark 15, last import 13
ok   tests/test_egocentric_prepare.py mark 23, last import 21
BAD  tests/test_video.py             mark  9, last import 12

Move that one below the import block and this is done.

[victorwon2001]

tests/conftest.py:22 — The version gate probes the PATH-resolved _system_ffmpeg instead of the effective HFLOW_FFMPEG executable. Because tests/conftest.py uses os.environ.setdefault, a pre-existing HFLOW_FFMPEG override is preserved while _ffmpeg_below_fps_mode_floor() still executes _system_ffmpeg -version.

The skip decision can describe a different binary from the one HFlow is configured to use. For example, a newer PATH ffmpeg with HFLOW_FFMPEG pointing at 4.4 can leave marked tests runnable even though their effective binary rejects -fps_mode; the inverse combination can skip tests even when the configured binary is new enough.

Static inspection establishes the mismatch between the preserved environment override and the executable passed to subprocess.run. README documents HFLOW_FFMPEG as the supported way to select a managed binary. The supplied guard tests cover version-string parsing and the floor but not override selection. No execution was performed.

tests/test_blur.py:15 — tests/test_blur.py applies requires_system_ffmpeg to the entire module even though its first two tests only exercise summarize_blur_scores over in-memory numeric values and do not invoke ffmpeg.

On ffmpeg older than 5.1, dependency-free blur-summary coverage is unnecessarily skipped. This is broader than the documented rule that tests which invoke ffmpeg must carry the marker and reduces unrelated coverage on the compatibility configuration the PR is trying to preserve.

The module-level marker is visible before both pure summary tests, whose bodies only call summarize_blur_scores and inspect its result. CONTRIBUTING states the marker requirement in terms of tests that invoke ffmpeg. No execution was performed.

file:tests/conftest.py:head

_system_ffmpeg = shutil.which("ffmpeg")
if _system_ffmpeg is not None:
os.environ.setdefault("HFLOW_FFMPEG", _system_ffmpeg)

file:tests/conftest.py:head

def _ffmpeg_below_fps_mode_floor() -> tuple[bool, str]:
"""Return (too_old, reason) for the suite ffmpeg, if it can be parsed."""
if _system_ffmpeg is None:
return False, ""
completed = subprocess.run(
[_system_ffmpeg, "-version"],
check=False,
capture_output=True,
text=True,
)
raw = completed.stdout or completed.stderr
version_line = raw.splitlines()[0] if raw else ""
parsed = _ffmpeg_version_tuple(version_line)
if parsed is None or parsed >= _MIN_FFMPEG_VERSION:
return False, ""

context:README.md

• On Linux x86_64/aarch64, the first video operation downloads a checksum-verified, pinned ffmpeg/ffprobe build into the user cache. Set HFLOW_FFMPEG and HFLOW_FFPROBE to use binaries you manage instead.

file:tests/test_ffmpeg_version_guard.py:head

def test_parses_ubuntu_lts_and_git_n_prefix() -> None:
assert _ffmpeg_version_tuple("ffmpeg version 4.4.2-0ubuntu0.22.04.1 Copyright") == (4, 4)
assert _ffmpeg_version_tuple("ffmpeg version n5.1.2 Copyright") == (5, 1)
assert _ffmpeg_version_tuple("ffmpeg version 6.1.1-3ubuntu5 Copyright") == (6, 1)

def test_old_distro_ffmpeg_is_below_fps_mode_floor() -> None:
parsed = _ffmpeg_version_tuple("ffmpeg version 4.4.2-0ubuntu0.22.04.1 Copyright")
assert parsed is not None
assert parsed < _MIN_FFMPEG_VERSION

file:tests/test_blur.py:head

pytestmark = pytest.mark.requires_system_ffmpeg

def test_summary_preserves_raw_scores_and_excludes_unavailable_frames() -> None:
summary = summarize_blur_scores(iter((120.0, math.nan, 240.0, math.inf, -math.inf)))

assert summary.frame_count == 5
assert summary.scored_frame_count == 2
assert summary.mean_blur_score == pytest.approx(180.0)

@pytest.mark.parametrize("scores", [(), (math.nan, math.inf, -math.inf)])
def test_missing_blur_evidence_has_no_score(scores: tuple[float, ...]) -> None:
summary = summarize_blur_scores(iter(scores))

assert summary.frame_count == len(scores)
assert summary.scored_frame_count == 0
assert summary.mean_blur_score is None

file:CONTRIBUTING.md:head

• ffmpeg and ffprobe on PATH for the test suite (ffmpeg 5.1 or newer; the suite uses -fps_mode, which is not in the 4.4.x builds Ubuntu 22.04 ships). Tests that invoke ffmpeg must carry @pytest.mark.requires_system_ffmpeg so older binaries skip them instead of failing the suite.

file:tests/test_video.py:head

import pytest

pytestmark = pytest.mark.requires_system_ffmpeg

from hflow.ffmpeg import ffmpeg_path, ffprobe_path
from hflow.video import (

review:5264051839

One thing left. tests/test_video.py still has pytestmark above its imports, so ruff check reports two E402s and this would redden main. You fixed the identical placement in test_source_sampling.py; test_video.py has it at line 9 with imports running to line 12.

I checked all four module-wide marks so this is the last of them:

ok   tests/test_source_sampling.py   mark 26, last import 24
ok   tests/test_blur.py              mark 15, last import 13
ok   tests/test_egocentric_prepare.py mark 23, last import 21
BAD  tests/test_video.py             mark  9, last import 12

head_content truncated at 50000 characters: tests/test_lerobot_converter.py

base_content truncated at 50000 characters: tests/test_lerobot_converter.py

Caller/test context uses filename and changed-code references; transitive callers and tests are unverified

CI diagnostic logs are not bound to the immutable head/base check snapshots

head_content truncated at 50000 characters: tests/test_lerobot_converter.py

base_content truncated at 50000 characters: tests/test_lerobot_converter.py

Caller/test context uses filename and changed-code references; transitive callers and tests are unverified

CI diagnostic logs are not bound to the immutable head/base check snapshots

Update the gate to probe the effective ffmpeg executable, add regression coverage for an existing HFLOW_FFMPEG override, narrow the blur marker to tests that actually require ffmpeg, and move the tests/test_video.py module marker below its import block before re-running the lint and FFmpeg-4.4 compatibility checks.

SHA: a19f520fd8dc10c242e77799053527cd617fd262

[HarshRajSinghania]

Thanks for the follow-up reviews.

Addressed the remaining points:

• Moved pytestmark in tests/test_video.py below the import block (the last E402 kstonekuan called out).
• Version gate now probes the effective suite ffmpeg (HFLOW_FFMPEG when set, otherwise PATH) instead of only _system_ffmpeg.
• Added a regression test that an existing HFLOW_FFMPEG override is the binary the gate uses.
• Dropped the module-wide marker on tests/test_blur.py and marked only test_video_adapter_distinguishes_defocus_from_missing_detail, so the in-memory summarize_blur_scores tests still run on ffmpeg 4.4.

Pushed: HarshRajSinghania@b41810e

I could not re-run the full 4.4 shim suite in this environment.

[greptile-apps]

Confidence Score: 4/5

[Low risk] Documents ffmpeg version requirement and marks tests accordingly.

The PR does not yet appear safe to merge because an older FFmpeg can cause marked tests to be skipped without failing the suite.

Findings

1.  Old FFmpeg Still Passes ▶
2.  Marker Skips Unrelated Tests ▶
3.  Version Probe Can Stall ▶

Summary

The PR documents an FFmpeg 5.1 minimum and adds a version-based pytest marker that skips selected tests on older binaries.

• The current hook skips marked tests rather than failing the suite once, so the original blocking finding remains outstanding.
• The broad marker in tests/test_video.py and the unbounded version probe also remain outstanding in their existing threads.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Pytest collection] --> B[Probe suite FFmpeg version]
  B --> C{Older than 5.1?}
  C -->|No or unparseable| D[Run collected tests]
  C -->|Yes| E[Skip marked tests]
  E --> F[Run remaining tests]

Loading

_{Reviews (5) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..."}

[greptile-apps]

Old FFmpeg Still Passes

When FFmpeg is older than 5.1, this hook only adds skip markers to selected tests. Pytest can therefore exit successfully after reporting skips, making an unsupported development environment appear healthy instead of failing once with the required and detected versions.

[greptile-apps]

Marker Skips Unrelated Tests

This module-wide marker skips every test in the file on FFmpeg 4.4, including in-memory H.264 parsing, splitting, delimiter, and unescape tests that never invoke FFmpeg. Marking only the tests or fixtures that execute the binary would preserve this unrelated coverage on older systems.

[greptile-apps]

Version Probe Can Stall

If HFLOW_FFMPEG points to a stale or non-executable path, this unhandled call raises an OSError during collection. If it points to a hanging wrapper, the lack of a timeout blocks the entire suite indefinitely. Using the existing bounded media-command path or containing these probe failures would keep the suite's normal failure handling reachable.

[kstonekuan]

Merged main in and pushed it (5e169d2, mine). The only conflict was tests/test_video.py, where #581 added imports while you moved pytestmark below the block; both are wanted, so the resolution keeps main's imports with the marker after them.

ruff check still fails, so this cannot merge yet. Three things, all in what this round added:

I001   tests/test_blur.py          import block un-sorted (a third blank line before the first test)
ANN001 tests/test_ffmpeg_version_guard.py:18  monkeypatch has no annotation
ANN001 tests/test_ffmpeg_version_guard.py:18  tmp_path has no annotation

ruff format --check fails on the same test_blur.py blank line. The annotations are monkeypatch: pytest.MonkeyPatch and tmp_path: Path, matching the other tests in that file.

[HarshRajSinghania]

Thanks — fixed the remaining ruff findings from the last review.

• tests/test_blur.py: removed the extra blank line after the import block (I001 / ruff format).
• tests/test_ffmpeg_version_guard.py: annotated monkeypatch: pytest.MonkeyPatch and tmp_path: Path (ANN001), matching the other tests in that file.

Pushed as 1f3c7fb.

[kstonekuan]

LGTM, merging. I merged main in (af6347f, mine): the only conflict was a test that now needs both main's skip-without-ffmpeg decorator and your marker, so it keeps both. Thanks for sticking with this through the rounds.