From de21f059c0835a0371c259681fe547ae2f39a206 Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Tue, 27 Oct 2026 15:56:30 +0100 Subject: [PATCH] Report why a GPX track was not used when no other source geotags the file A GPX track that misses a file lets the later geotag sources take their turn. When none of them geotagged the file, the error reported was that of the last one to fail, which hid the miss. A video whose own GPS is too noisy, given a GPX of another day, failed as "GPS is too noisy": the very problem the GPX was given to work around. The Desktop Uploader, whose chain runs two GPX sources, exiftool_xml, native and exiftool_runtime, shows only that error. Report the miss instead: "The GPX track in X (...) misses the video Y (...) by 105.1 days. Check that the GPX file belongs to this video, ...". An error that ends the chain, such as "Stationary video", is still reported, as it says something about the file itself, and a file that a later source geotags still only gets the warning it gets now. --- mapillary_tools/geotag/factory.py | 11 ++++- tests/unit/test_gpx_over_noisy_gps.py | 61 +++++++++++++++++++++++++++ tests/unit/test_gpx_video_coverage.py | 24 +++++++++++ 3 files changed, 95 insertions(+), 1 deletion(-) diff --git a/mapillary_tools/geotag/factory.py b/mapillary_tools/geotag/factory.py index 87c0016d4..e11984ecc 100644 --- a/mapillary_tools/geotag/factory.py +++ b/mapillary_tools/geotag/factory.py @@ -289,7 +289,8 @@ def process( rejected_gps_paths: set[Path] = set() # Paths whose GPX track missed them, with the source that rejected them, to - # warn about if a later source geotags them instead + # warn about if a later source geotags them instead, and to report if none + # does gpx_rejections: dict[Path, tuple[SourceOption, types.ErrorMetadata]] = {} # Videos let through to sources that cannot geotag videos (e.g. exif), with @@ -399,6 +400,14 @@ def process( if not reprocessable_paths: break + # Of a file that a GPX track missed, and that no later source geotagged, + # report the miss: why the track given for it was not used says more than + # the later sources failing too, such as "GPS is too noisy" for a video + # whose GPS the track was given to replace + for path, (_, gpx_miss) in gpx_rejections.items(): + if path in pending_errors: + pending_errors[path] = gpx_miss + final_metadatas.extend(pending_errors.values()) unread_paths = [ diff --git a/tests/unit/test_gpx_over_noisy_gps.py b/tests/unit/test_gpx_over_noisy_gps.py index f8c494112..ac586847a 100644 --- a/tests/unit/test_gpx_over_noisy_gps.py +++ b/tests/unit/test_gpx_over_noisy_gps.py @@ -374,6 +374,13 @@ def build_video_geotag(option: SourceOption): GPX_NOT_FOUND = exceptions.MapillaryVideoGPSNotFoundError( "GPX file not found for video" ) +GPX_MISS = exceptions.MapillaryOutsideGPXTrackError( + "The GPX track misses the video by 105.1 days", + image_time="2026_09_11_15_39_41_000", + gpx_start_time="2026_05_29_12_47_06_000", + gpx_end_time="2026_05_29_12_53_00_000", +) +XML_NOT_FOUND = exceptions.MapillaryExifToolXMLNotFoundError("No XML file found") DEFAULT_CHAIN = [SourceType(source) for source in DEFAULT_GEOTAG_SOURCE_OPTIONS] @@ -505,6 +512,60 @@ def test_gpx_that_fails_replaces_the_rejection(self, monkeypatch): assert isinstance(result, types.ErrorMetadata) assert result.error is mismatch + def test_gpx_that_misses_is_reported_over_rejected_gps(self, monkeypatch): + """ + The chain of the Desktop Uploader, with a GPX that misses a video whose + GPS is too noisy: the miss says why the GPX was not used, while "GPS + is too noisy" is what the GPX was given to work around. + """ + results, called = _run_chain( + monkeypatch, + [ + SourceType.GPX, + SourceType.EXIFTOOL_XML, + SourceType.NATIVE, + SourceType.EXIFTOOL_RUNTIME, + ], + { + SourceType.GPX: GPX_MISS, + SourceType.EXIFTOOL_XML: XML_NOT_FOUND, + SourceType.NATIVE: NOISE, + SourceType.EXIFTOOL_RUNTIME: None, + }, + ) + result = _only_result(results) + assert isinstance(result, types.ErrorMetadata) + assert result.error is GPX_MISS + assert called == [SourceType.GPX, SourceType.EXIFTOOL_XML, SourceType.NATIVE] + + @pytest.mark.parametrize( + "later_error", + [NOISE, EMPTY, NOT_FOUND, exceptions.MapillaryExiftoolNotFoundError("no")], + ) + def test_gpx_that_misses_is_reported_over_later_failures( + self, monkeypatch, later_error + ): + results, _ = _run_chain( + monkeypatch, + [SourceType.GPX, SourceType.NATIVE], + {SourceType.GPX: GPX_MISS, SourceType.NATIVE: later_error}, + ) + result = _only_result(results) + assert isinstance(result, types.ErrorMetadata) + assert result.error is GPX_MISS + + def test_gpx_that_misses_does_not_hide_a_final_error(self, monkeypatch): + """An error that ends the chain says something about the file itself""" + stationary = exceptions.MapillaryStationaryVideoError("Stationary") + results, _ = _run_chain( + monkeypatch, + [SourceType.GPX, SourceType.NATIVE], + {SourceType.GPX: GPX_MISS, SourceType.NATIVE: stationary}, + ) + result = _only_result(results) + assert isinstance(result, types.ErrorMetadata) + assert result.error is stationary + def test_not_found_still_falls_through_to_exiftool(self, monkeypatch): """Finding nothing is a verdict on the reader, so retrying is fair.""" results, called = _run_chain( diff --git a/tests/unit/test_gpx_video_coverage.py b/tests/unit/test_gpx_video_coverage.py index 8fa98c199..2e657e30c 100644 --- a/tests/unit/test_gpx_video_coverage.py +++ b/tests/unit/test_gpx_video_coverage.py @@ -527,6 +527,30 @@ def test_video_is_geotagged_by_the_next_source_with_a_warning( assert "misses the video" in warning and "by 2.0 hours" in warning assert warning.endswith("Geotagged it from the native source instead") + def test_the_miss_is_reported_if_no_later_source_geotags_the_video(self, tmp_path): + video_path = _write_camm_mp4(tmp_path / "video.mp4", _video_track()) + gpx_path = _write_gpx(tmp_path / "track.gpx", A_UNIX_TIME + 2 * 3600, 10) + options = [ + SourceOption( + SourceType.GPX, + num_processes=0, + source_path=SourcePathOption(source_path=gpx_path), + ), + # Finds no GPX file to read + SourceOption( + SourceType.GPX, + num_processes=0, + source_path=SourcePathOption(source_path=tmp_path / "none.gpx"), + ), + ] + + [metadata] = factory.process([video_path], options) + + assert isinstance(metadata, types.ErrorMetadata) + assert isinstance(metadata.error, exceptions.MapillaryOutsideGPXTrackError) + assert "misses the video" in str(metadata.error) + assert "by 2.0 hours" in str(metadata.error) + @pytest.mark.parametrize( "gpx_start, source", [(A_UNIX_TIME, SourceType.GPX), (A_UNIX_TIME + 2 * 3600, SourceType.NATIVE)],