From a5e49e4979d94a5275d5ea788db7633240d902e6 Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Mon, 21 Sep 2026 16:37:31 +0200 Subject: [PATCH 1/3] Fail the sample instead of silently producing no frames Distance sampling warned and returned when it could not read a video's GPS, so the command exited 0 having written nothing: WARNING - GPS is too noisy ==> Processing 0 files with source gpx... ==> Validating 0 metadatas... ==> Process summary No error, no frames, no non-zero exit -- the sample directory is never created and the geotag stage then runs over zero files. The user is told the run succeeded and has nothing to upload. Reported for a GoPro MAX 2 whose embedded GPS is rejected as noise, where attaching a GPX made no difference, but nothing about it is specific to noisy GPS: a video with no GPS at all takes the same path, which is every "camera without embedded GPS, bring your own GPX" workflow. Distance sampling needs positions to decide which frames to cut, so failing to read them is a failed sample. Raise MapillaryVideoError, the same error the rest of this function already raises for an unreadable start time or a frame count mismatch, and which exits 7 rather than dumping a traceback. sample_video() already funnels sampling errors through --skip_sample_errors, so callers who want to tolerate this keep a supported way to do it and the default stops lying. Two neighbouring silent paths get the same treatment: a missing video stream, which also returned after a warning, and an empty point list, which was an assert and so disappeared under `python -O`, leaving an IndexError further down instead. Note this changes batch behaviour. Sampling a directory containing one unreadable video now aborts unless --skip_sample_errors is passed. That matches what every other sampling error in this function already does, and the alternative is continuing to hide the failure, but it is a behaviour change for callers who relied on the skip. Three integration tests covered directories containing hero8.mp4, whose 32 embedded points are all dropped by remove_noisy_points(); they were passing while it contributed no frames at all. They now pass --skip_sample_errors, which is what they always meant. A new test pins the loud behaviour, mirroring test_sample_video_without_video_time. This does not make --geotag_source reach distance sampling; that call site still hardcodes GeotagVideosFromVideo() and is a separate fix. It only stops the failure from being silent. --- mapillary_tools/sample_video.py | 24 ++++-- tests/integration/test_video_process.py | 49 ++++++++++- tests/unit/test_sample_video.py | 104 +++++++++++++++++++++++- 3 files changed, 170 insertions(+), 7 deletions(-) diff --git a/mapillary_tools/sample_video.py b/mapillary_tools/sample_video.py index 1e3e7764..b641e56d 100644 --- a/mapillary_tools/sample_video.py +++ b/mapillary_tools/sample_video.py @@ -300,17 +300,31 @@ def _sample_single_video_by_distance( ) assert len(video_metadatas) == 1, "expect 1 video metadata" video_metadata = video_metadatas[0] + + # Distance sampling needs positions to decide which frames to cut, so + # failing to read them is a failed sample, not something to carry on past. + # Warning and returning left the caller with a success exit code, an empty + # (or missing) sample directory and nothing to upload. sample_video() + # already funnels these through --skip_sample_errors for callers who do + # want to tolerate them. if isinstance(video_metadata, types.ErrorMetadata): - LOG.warning(str(video_metadata.error)) - return - assert video_metadata.points, "expect non-empty points" + raise exceptions.MapillaryVideoError( + f"Unable to sample {video_path} by distance: {video_metadata.error}" + ) from video_metadata.error + + if not video_metadata.points: + raise exceptions.MapillaryVideoError( + f"Unable to sample {video_path} by distance: no GPS points found" + ) + LOG.info("Found total %d GPS points", len(video_metadata.points)) # find the video stream with maximum resolution video_stream = probe.probe_video_with_max_resolution() if not video_stream: - LOG.warning("no video streams found from ffprobe") - return + raise exceptions.MapillaryVideoError( + f"No video streams found in {video_path} by ffprobe" + ) LOG.info("Extracting video samples") video_stream_idx = video_stream["index"] diff --git a/tests/integration/test_video_process.py b/tests/integration/test_video_process.py index a2a2a05e..aff97e68 100644 --- a/tests/integration/test_video_process.py +++ b/tests/integration/test_video_process.py @@ -31,13 +31,24 @@ def test_sample_video_relpath(): pytest_skip_if_not_ffmpeg_installed() + # hero8.mp4's embedded GPS is filtered out entirely as noise, so it cannot + # be sampled by distance. This test is about relative paths, not about that, + # hence --skip_sample_errors. with tempfile.TemporaryDirectory() as dir: - run_sample_video(["--rerun", "tests/data/gopro_data/hero8.mp4", str(dir)]) + run_sample_video( + [ + "--rerun", + "--skip_sample_errors", + "tests/data/gopro_data/hero8.mp4", + str(dir), + ] + ) with tempfile.TemporaryDirectory() as dir: run_sample_video( [ "--rerun", + "--skip_sample_errors", "--video_start_time", "2021_10_10_10_10_10_123", "tests/data", @@ -46,6 +57,37 @@ def test_sample_video_relpath(): ) +def test_sample_video_by_distance_without_usable_gps(setup_data: py.path.local): + """ + A video whose GPS cannot be read is not silently sampled into nothing. + + hero8.mp4 has 32 embedded GPS points and remove_noisy_points() drops all of + them, so distance sampling has no positions to choose frames with. It used + to log a warning, write no frames and still exit 0, which left the caller a + success code and an empty import directory. + """ + pytest_skip_if_not_ffmpeg_installed() + + video_path = setup_data.join("gopro_data").join("hero8.mp4") + + with tempfile.TemporaryDirectory() as dir: + with pytest.raises(subprocess.CalledProcessError) as ex: + run_sample_video(["--video_sample_distance=6", "--rerun", video_path, dir]) + assert 7 == ex.value.returncode, ex.value.stderr + assert not list(Path(dir).iterdir()) + + with tempfile.TemporaryDirectory() as dir: + run_sample_video( + [ + "--video_sample_distance=6", + "--skip_sample_errors", + "--rerun", + video_path, + dir, + ] + ) + + def test_sample_video_without_video_time(setup_data: py.path.local): pytest_skip_if_not_ffmpeg_installed() @@ -143,6 +185,9 @@ def test_video_process_sample_with_multiple_distances(setup_data: py.path.local) [ "--video_sample_distance", str(distance), + # gopro_data also holds hero8.mp4, whose GPS is all noise and so + # cannot be distance-sampled; this test is about max-360mode.mp4 + "--skip_sample_errors", "--rerun", str(video_dir), str(video_dir.join("my_samples")), @@ -167,6 +212,8 @@ def test_video_process_sample_with_distance(setup_data: py.path.local): descs = run_video_process_for_descs( [ *options, + # see test_video_process_sample_with_multiple_distances + "--skip_sample_errors", str(video_dir), str(video_dir.join("my_samples")), ] diff --git a/tests/unit/test_sample_video.py b/tests/unit/test_sample_video.py index 0743eeb4..0aa609ba 100644 --- a/tests/unit/test_sample_video.py +++ b/tests/unit/test_sample_video.py @@ -22,9 +22,10 @@ geo, sample_video, ) +from mapillary_tools.geotag import geotag_videos_from_video from mapillary_tools.mp4 import mp4_sample_parser from mapillary_tools.serializer import description -from mapillary_tools.types import FileType, VideoMetadata +from mapillary_tools.types import describe_error_metadata, FileType, VideoMetadata _PWD = Path(os.path.dirname(os.path.abspath(__file__))) @@ -334,6 +335,107 @@ def test_empty_samples(self) -> None: assert len(result) == 0 +# --------------------------------------------------------------------------- +# A failed distance sample must not look like a successful one +# --------------------------------------------------------------------------- + + +class TestDistanceSamplingFailsLoudly: + """ + Distance sampling used to warn and return when it could not read the GPS, + which left the caller a success exit code, no sample directory and nothing + to upload -- `video_process` went on to geotag zero files and printed an + empty summary. Reported for a GoPro whose embedded GPS is all noise, where + an attached GPX made no difference. + """ + + VIDEO = _PWD.joinpath("data/mock_sample_video/videos/hello.mp4") + + @pytest.fixture + def unreadable_gps(self, monkeypatch): + """Every GPS read fails the way a fully filtered noisy track does.""" + error = exceptions.MapillaryGPSNoiseError("GPS is too noisy") + monkeypatch.setattr( + geotag_videos_from_video.GeotagVideosFromVideo, + "to_description", + lambda _self, paths: [ + describe_error_metadata( + error, filename=paths[0], filetype=FileType.GOPRO + ) + ], + ) + return error + + def _sample(self, tmpdir: py.path.local, **kwargs): + return sample_video.sample_video( + self.VIDEO, Path(tmpdir), video_sample_distance=2, rerun=True, **kwargs + ) + + def test_it_raises_instead_of_returning(self, tmpdir, setup_mock, unreadable_gps): + with pytest.raises(exceptions.MapillaryVideoError) as excinfo: + self._sample(tmpdir) + + # the underlying reason has to survive into the message the user sees + assert "GPS is too noisy" in str(excinfo.value) + assert excinfo.value.__cause__ is unreadable_gps + + def test_the_exit_code_is_a_clean_one(self, tmpdir, setup_mock, unreadable_gps): + """Not a MapillaryUserError means a traceback instead of an exit code.""" + with pytest.raises(exceptions.MapillaryUserError) as excinfo: + self._sample(tmpdir) + + assert excinfo.value.exit_code == 7 + + def test_nothing_is_left_behind(self, tmpdir, setup_mock, unreadable_gps): + with pytest.raises(exceptions.MapillaryVideoError): + self._sample(tmpdir) + + assert not Path(tmpdir).joinpath(self.VIDEO.name).exists() + + def test_skip_sample_errors_still_tolerates_it( + self, tmpdir, setup_mock, unreadable_gps + ): + """The opt-out that several existing callers rely on.""" + self._sample(tmpdir, skip_sample_errors=True) + + def test_an_empty_track_is_also_an_error(self, tmpdir, setup_mock, monkeypatch): + """Previously an assert, so it vanished under `python -O`.""" + monkeypatch.setattr( + geotag_videos_from_video.GeotagVideosFromVideo, + "to_description", + lambda _self, paths: [ + VideoMetadata( + filename=paths[0], filesize=0, filetype=FileType.GOPRO, points=[] + ) + ], + ) + + with pytest.raises(exceptions.MapillaryVideoError): + self._sample(tmpdir) + + def test_a_missing_video_stream_is_also_an_error( + self, tmpdir, setup_mock, monkeypatch + ): + monkeypatch.setattr( + geotag_videos_from_video.GeotagVideosFromVideo, + "to_description", + lambda _self, paths: [ + VideoMetadata( + filename=paths[0], + filesize=0, + filetype=FileType.GOPRO, + points=_make_gps_points(3, time_step=1.0), + ) + ], + ) + monkeypatch.setattr( + ffmpeglib.Probe, "probe_video_with_max_resolution", lambda _self: None + ) + + with pytest.raises(exceptions.MapillaryVideoError): + self._sample(tmpdir) + + # --------------------------------------------------------------------------- # sample_video() parameter validation & rerun # --------------------------------------------------------------------------- From bb5873c99dc8a02afe7af4d550587c50443d261e Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Tue, 22 Sep 2026 15:54:14 +0200 Subject: [PATCH 2/3] Name --skip_sample_errors in the error it suppresses The raise told users the sample failed but not how to get past it, and the obvious guess is wrong: --skip_process_errors governs the later geotagging stage and does not cover sampling, so reaching for it leaves the run failing with the same message. Append the hint, matching the existing wording in process_geotag_properties.py. The message now reads: MapillaryVideoError: Unable to sample GS018205.360 by distance: GPS is too noisy. To skip these errors, specify --skip_sample_errors --- mapillary_tools/sample_video.py | 11 ++++++++--- tests/unit/test_sample_video.py | 13 +++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/mapillary_tools/sample_video.py b/mapillary_tools/sample_video.py index b641e56d..752fc36d 100644 --- a/mapillary_tools/sample_video.py +++ b/mapillary_tools/sample_video.py @@ -22,6 +22,11 @@ LOG = logging.getLogger(__name__) +# Sampling errors are suppressed by --skip_sample_errors, not by +# --skip_process_errors, which governs the later geotagging stage. Say so in +# the message: the two flags are easy to reach for the wrong one. +_SKIP_HINT = "To skip these errors, specify --skip_sample_errors" + def _normalize_path( video_import_path: Path, skip_subfolders: bool @@ -309,12 +314,12 @@ def _sample_single_video_by_distance( # want to tolerate them. if isinstance(video_metadata, types.ErrorMetadata): raise exceptions.MapillaryVideoError( - f"Unable to sample {video_path} by distance: {video_metadata.error}" + f"Unable to sample {video_path} by distance: {video_metadata.error}. {_SKIP_HINT}" ) from video_metadata.error if not video_metadata.points: raise exceptions.MapillaryVideoError( - f"Unable to sample {video_path} by distance: no GPS points found" + f"Unable to sample {video_path} by distance: no GPS points found. {_SKIP_HINT}" ) LOG.info("Found total %d GPS points", len(video_metadata.points)) @@ -323,7 +328,7 @@ def _sample_single_video_by_distance( video_stream = probe.probe_video_with_max_resolution() if not video_stream: raise exceptions.MapillaryVideoError( - f"No video streams found in {video_path} by ffprobe" + f"No video streams found in {video_path} by ffprobe. {_SKIP_HINT}" ) LOG.info("Extracting video samples") diff --git a/tests/unit/test_sample_video.py b/tests/unit/test_sample_video.py index 0aa609ba..c7ad376d 100644 --- a/tests/unit/test_sample_video.py +++ b/tests/unit/test_sample_video.py @@ -379,6 +379,19 @@ def test_it_raises_instead_of_returning(self, tmpdir, setup_mock, unreadable_gps assert "GPS is too noisy" in str(excinfo.value) assert excinfo.value.__cause__ is unreadable_gps + def test_it_names_the_flag_that_suppresses_it( + self, tmpdir, setup_mock, unreadable_gps + ): + """ + --skip_process_errors governs the later geotagging stage and does not + cover sampling, so the message has to name --skip_sample_errors or + users reach for the wrong flag. + """ + with pytest.raises(exceptions.MapillaryVideoError) as excinfo: + self._sample(tmpdir) + + assert "--skip_sample_errors" in str(excinfo.value) + def test_the_exit_code_is_a_clean_one(self, tmpdir, setup_mock, unreadable_gps): """Not a MapillaryUserError means a traceback instead of an exit code.""" with pytest.raises(exceptions.MapillaryUserError) as excinfo: From 976c13562eef29f863f2181ea5ad0e026a1f0847 Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Tue, 22 Sep 2026 16:34:09 +0200 Subject: [PATCH 3/3] Name the flag in every error it suppresses, not just some Four other raises sit inside the same try block that --skip_sample_errors guards and said nothing about it: _sample_single_video_by_interval unable to extract video start time _sample_single_video_by_distance unable to extract video start time _sample_single_video_by_distance expect N samples but extracted M _sample_single_video_by_distance expect X to be Nth sample but got M Half the errors naming the flag is worse than none naming it: a user who lands on a silent one reaches for --skip_process_errors, which governs the later geotagging stage and leaves the run failing with the same message. Rather than append the hint at seven call sites, where it can drift out of sync again, construct these errors through _sampling_error(), so a raise added later cannot forget it. MapillaryFFmpegNotFoundError keeps its plain message. It is re-raised by its own handler before the skip check, so --skip_sample_errors really does not suppress it and it must not say otherwise. There is a test for that, alongside ones covering both newly hinted start-time paths. --- mapillary_tools/sample_video.py | 32 ++++++++++----- tests/unit/test_sample_video.py | 71 +++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 11 deletions(-) diff --git a/mapillary_tools/sample_video.py b/mapillary_tools/sample_video.py index 752fc36d..b6548685 100644 --- a/mapillary_tools/sample_video.py +++ b/mapillary_tools/sample_video.py @@ -28,6 +28,18 @@ _SKIP_HINT = "To skip these errors, specify --skip_sample_errors" +def _sampling_error(message: str) -> exceptions.MapillaryVideoError: + """ + Build an error for a failed sample, naming the flag that skips it. + + Everything raised out of the per-video body of sample_video() is + suppressible by --skip_sample_errors, so every one of those messages should + say so. Going through here rather than appending the hint at each raise + keeps that true of raises added later. + """ + return exceptions.MapillaryVideoError(f"{message}. {_SKIP_HINT}") + + def _normalize_path( video_import_path: Path, skip_subfolders: bool ) -> tuple[Path, list[Path]]: @@ -198,7 +210,7 @@ def _sample_single_video_by_interval( ffmpeg.probe_format_and_streams(video_path) ).probe_video_start_time() if start_time is None: - raise exceptions.MapillaryVideoError( + raise _sampling_error( f"Unable to extract video start time from {video_path}" ) @@ -294,7 +306,7 @@ def _sample_single_video_by_distance( if start_time is None: start_time = probe.probe_video_start_time() if start_time is None: - raise exceptions.MapillaryVideoError( + raise _sampling_error( f"Unable to extract video start time from {video_path}" ) @@ -313,13 +325,13 @@ def _sample_single_video_by_distance( # already funnels these through --skip_sample_errors for callers who do # want to tolerate them. if isinstance(video_metadata, types.ErrorMetadata): - raise exceptions.MapillaryVideoError( - f"Unable to sample {video_path} by distance: {video_metadata.error}. {_SKIP_HINT}" + raise _sampling_error( + f"Unable to sample {video_path} by distance: {video_metadata.error}" ) from video_metadata.error if not video_metadata.points: - raise exceptions.MapillaryVideoError( - f"Unable to sample {video_path} by distance: no GPS points found. {_SKIP_HINT}" + raise _sampling_error( + f"Unable to sample {video_path} by distance: no GPS points found" ) LOG.info("Found total %d GPS points", len(video_metadata.points)) @@ -327,9 +339,7 @@ def _sample_single_video_by_distance( # find the video stream with maximum resolution video_stream = probe.probe_video_with_max_resolution() if not video_stream: - raise exceptions.MapillaryVideoError( - f"No video streams found in {video_path} by ffprobe. {_SKIP_HINT}" - ) + raise _sampling_error(f"No video streams found in {video_path} by ffprobe") LOG.info("Extracting video samples") video_stream_idx = video_stream["index"] @@ -352,7 +362,7 @@ def _sample_single_video_by_distance( wip_dir, video_path, selected_stream_specifiers=[str(video_stream_idx)] ) if len(frame_samples) != len(sorted_sample_indices): - raise exceptions.MapillaryVideoError( + raise _sampling_error( f"Expect {len(sorted_sample_indices)} samples but extracted {len(frame_samples)} samples" ) for idx, (frame_idx_1based, sample_paths) in enumerate(frame_samples): @@ -360,7 +370,7 @@ def _sample_single_video_by_distance( "Expect 1 sample path at {frame_idx_1based} but got {sample_paths}" ) if idx + 1 != frame_idx_1based: - raise exceptions.MapillaryVideoError( + raise _sampling_error( f"Expect {sample_paths[0]} to be {idx + 1}th sample but got {frame_idx_1based}" ) diff --git a/tests/unit/test_sample_video.py b/tests/unit/test_sample_video.py index c7ad376d..15dcc6f3 100644 --- a/tests/unit/test_sample_video.py +++ b/tests/unit/test_sample_video.py @@ -723,3 +723,74 @@ def test_exif_lat_lon_written(self, tmp_path: Path) -> None: # First GPS point is at (40.0, -74.0) assert abs(lat - 40.0) < 0.01 assert abs(lon - (-74.0)) < 0.01 + + +class TestEverySuppressibleErrorNamesTheFlag: + """ + Every error raised out of the per-video body of sample_video() is + suppressed by --skip_sample_errors, so every one of those messages has to + name it. Half of them naming it is worse than none: a user who hits one of + the silent ones reaches for --skip_process_errors, which governs the later + geotagging stage and leaves the run failing with the same message. + """ + + VIDEO = _PWD.joinpath("data/mock_sample_video/videos/hello.mp4") + + @pytest.fixture + def no_start_time(self, monkeypatch): + monkeypatch.setattr( + ffmpeglib.Probe, "probe_video_start_time", lambda _self: None + ) + + @pytest.mark.parametrize( + "kwargs", + [ + pytest.param({"video_sample_distance": 2}, id="by_distance"), + pytest.param( + {"video_sample_distance": -1, "video_sample_interval": 2}, + id="by_interval", + ), + ], + ) + def test_unreadable_start_time_names_the_flag( + self, tmpdir, setup_mock, no_start_time, kwargs + ): + with pytest.raises(exceptions.MapillaryVideoError) as excinfo: + sample_video.sample_video(self.VIDEO, Path(tmpdir), rerun=True, **kwargs) + + assert "Unable to extract video start time" in str(excinfo.value) + assert "--skip_sample_errors" in str(excinfo.value) + + def test_the_helper_appends_it_once(self): + error = sample_video._sampling_error("something went wrong") + + assert str(error) == ( + "something went wrong. To skip these errors, specify --skip_sample_errors" + ) + assert isinstance(error, exceptions.MapillaryUserError) + + def test_a_missing_ffmpeg_does_not_claim_to_be_skippable( + self, tmpdir, setup_mock, monkeypatch + ): + """ + FFmpegNotFoundError is re-raised by its own handler before the skip + check, so --skip_sample_errors does not suppress it and its message + must not offer the flag. + """ + + def boom(*args, **kwargs): + raise ffmpeglib.FFmpegNotFoundError("ffmpeg not found") + + monkeypatch.setattr(MOCK_FFMPEG, "extract_frames_by_interval", boom) + + with pytest.raises(exceptions.MapillaryFFmpegNotFoundError) as excinfo: + sample_video.sample_video( + self.VIDEO, + Path(tmpdir), + video_sample_distance=-1, + video_sample_interval=2, + rerun=True, + skip_sample_errors=True, + ) + + assert "--skip_sample_errors" not in str(excinfo.value)