diff --git a/mapillary_tools/sample_video.py b/mapillary_tools/sample_video.py index 1e3e7764..b6548685 100644 --- a/mapillary_tools/sample_video.py +++ b/mapillary_tools/sample_video.py @@ -22,6 +22,23 @@ 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 _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 @@ -193,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}" ) @@ -289,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}" ) @@ -300,17 +317,29 @@ 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 _sampling_error( + f"Unable to sample {video_path} by distance: {video_metadata.error}" + ) from video_metadata.error + + if not video_metadata.points: + 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)) # 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 _sampling_error(f"No video streams found in {video_path} by ffprobe") LOG.info("Extracting video samples") video_stream_idx = video_stream["index"] @@ -333,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): @@ -341,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/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..15dcc6f3 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,120 @@ 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_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: + 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 # --------------------------------------------------------------------------- @@ -608,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)