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.