From 0e7e0ecdbd35eab53ee2f5ec6e57627a367b3b4a Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Fri, 18 Sep 2026 14:40:47 +0200 Subject: [PATCH 1/2] Let a GPX override a video whose own GPS is unusable Attaching a GPX is the documented escape hatch for a video with bad embedded GPS, but for a GoPro whose GPS is rejected as noise the upload still fails with MapillaryGPSNoiseError: GPS is too noisy no matter what GPX is supplied. Reported against a GoPro MAX2 .360 recorded with no GPS fix: all 32 GPMF points carry fix=NO_FIX and a DoP of 2139 (the limit is 1000), so remove_noisy_points() drops every one. This used to work. Before the geotag refactor, NativeVideoExtractor returned an ErrorMetadata value and GPXVideoExtractor fell back to the GPX on *any* failure. It now raises instead, and only one of the three "no usable GPS" errors was being caught: except exceptions.MapillaryVideoGPSNotFoundError as ex: MapillaryGPSNoiseError and MapillaryGPXEmptyError are siblings of that class rather than subclasses, so they escape the handler and fail the whole video. That also explains the reporter's observation that the same GPX works on a video with no embedded GPS at all: that path raises MapillaryVideoGPSNotFoundError, which is caught. Rather than only widening the except clause, stop filtering in the first place when the caller is the GPX path. The noise filter is a quality gate on the track we are about to publish; once a GPX replaces that track, the video's own GPS is just a source of make/model and of a clock to sync against, and neither is improved by discarding points. Widening the except clause alone would work, but it drops the video into the bare-VIDEO fallback and loses filetype=gopro, make/model, and the sync anchor -- the GPX would be rebased to 0 instead of to its real +2.0s offset. The timestamps are still good with no fix, so they still sync. The except clause is widened as well, for the genuinely empty case where there is no clock to recover. The same bug had a second instance in factory._is_reprocessable(), which also listed only MapillaryVideoGPSNotFoundError. Chaining sources, as in --geotag_source native --geotag_source gpx failed at the native stage and never reached the GPX. Unusable GPS in one source is exactly what a later source is there to replace. The gate itself is unchanged: a noisy video with no GPX supplied is still rejected with "GPS is too noisy". Verified end to end on the reported file. Processing now reports "1 gopro read / ready", and a dry-run upload produces an mp4 whose CAMM track carries the GPX coordinates at t=2.0, 4.0, ... with GoPro/MAX2 preserved. Of the 11 new tests, 7 fail without this change and the 4 guard tests pass either way. --- mapillary_tools/geotag/factory.py | 4 + .../geotag/video_extractors/gpx.py | 13 +- .../geotag/video_extractors/native.py | 33 +++- tests/unit/test_gpx_over_noisy_gps.py | 182 ++++++++++++++++++ 4 files changed, 223 insertions(+), 9 deletions(-) create mode 100644 tests/unit/test_gpx_over_noisy_gps.py diff --git a/mapillary_tools/geotag/factory.py b/mapillary_tools/geotag/factory.py index 4d1eeea0..e9848e26 100644 --- a/mapillary_tools/geotag/factory.py +++ b/mapillary_tools/geotag/factory.py @@ -125,6 +125,10 @@ def _is_reprocessable(metadata: types.MetadataOrError) -> bool: ( exceptions.MapillaryGeoTaggingError, exceptions.MapillaryVideoGPSNotFoundError, + # Unusable GPS in this source is exactly what a later source + # (typically a user-supplied GPX) is there to replace + exceptions.MapillaryGPXEmptyError, + exceptions.MapillaryGPSNoiseError, exceptions.MapillaryExiftoolNotFoundError, exceptions.MapillaryExifToolXMLNotFoundError, ), diff --git a/mapillary_tools/geotag/video_extractors/gpx.py b/mapillary_tools/geotag/video_extractors/gpx.py index 00722bd1..70f97487 100644 --- a/mapillary_tools/geotag/video_extractors/gpx.py +++ b/mapillary_tools/geotag/video_extractors/gpx.py @@ -59,11 +59,20 @@ def extract(self) -> types.VideoMetadata: gpx_points: T.Sequence[geo.Point] = sum(gpx_tracks, []) - native_extractor = NativeVideoExtractor(self.video_path) + # The GPX track replaces the video's own GPS, so the native extractor is + # only a source of make/model and of a clock to sync against. Keep noisy + # points: they are never published, and their timestamps still sync. + native_extractor = NativeVideoExtractor( + self.video_path, filter_noisy_points=False + ) try: native_video_metadata = native_extractor.extract() - except exceptions.MapillaryVideoGPSNotFoundError as ex: + except ( + exceptions.MapillaryVideoGPSNotFoundError, + exceptions.MapillaryGPXEmptyError, + exceptions.MapillaryGPSNoiseError, + ) as ex: if self.sync_mode is SyncMode.STRICT_SYNC: raise ex self._rebase_times(gpx_points) diff --git a/mapillary_tools/geotag/video_extractors/native.py b/mapillary_tools/geotag/video_extractors/native.py index a4a329e7..d0f67986 100644 --- a/mapillary_tools/geotag/video_extractors/native.py +++ b/mapillary_tools/geotag/video_extractors/native.py @@ -22,6 +22,15 @@ class GoProVideoExtractor(BaseVideoExtractor): + def __init__(self, video_path: Path, filter_noisy_points: bool = True): + super().__init__(video_path) + # The noise filter is a quality gate on the track we are about to + # publish. Callers that only need the video's make/model and its GPS + # clock (e.g. geotagging from a GPX file) pass False: discarding noisy + # points there would throw away a usable sync anchor and, if every + # point is dropped, fail the whole video over GPS we are not using. + self.filter_noisy_points = filter_noisy_points + @override def extract(self) -> types.VideoMetadata: with self.video_path.open("rb") as fp: @@ -37,11 +46,13 @@ def extract(self) -> types.VideoMetadata: if not gps_points: raise exceptions.MapillaryGPXEmptyError("Empty GPS data found") - gps_points = T.cast( - T.List[telemetry.GPSPoint], gpmf_gps_filter.remove_noisy_points(gps_points) - ) - if not gps_points: - raise exceptions.MapillaryGPSNoiseError("GPS is too noisy") + if self.filter_noisy_points: + gps_points = T.cast( + T.List[telemetry.GPSPoint], + gpmf_gps_filter.remove_noisy_points(gps_points), + ) + if not gps_points: + raise exceptions.MapillaryGPSNoiseError("GPS is too noisy") video_metadata = types.VideoMetadata( filename=self.video_path, @@ -106,9 +117,15 @@ def extract(self) -> types.VideoMetadata: class NativeVideoExtractor(BaseVideoExtractor): - def __init__(self, video_path: Path, filetypes: set[types.FileType] | None = None): + def __init__( + self, + video_path: Path, + filetypes: set[types.FileType] | None = None, + filter_noisy_points: bool = True, + ): super().__init__(video_path) self.filetypes = filetypes + self.filter_noisy_points = filter_noisy_points @override def extract(self) -> types.VideoMetadata: @@ -116,7 +133,9 @@ def extract(self) -> types.VideoMetadata: extractor: BaseVideoExtractor if ft is None or types.FileType.VIDEO in ft or types.FileType.GOPRO in ft: - extractor = GoProVideoExtractor(self.video_path) + extractor = GoProVideoExtractor( + self.video_path, filter_noisy_points=self.filter_noisy_points + ) try: return extractor.extract() except simple_mp4_parser.BoxNotFoundError as ex: diff --git a/tests/unit/test_gpx_over_noisy_gps.py b/tests/unit/test_gpx_over_noisy_gps.py new file mode 100644 index 00000000..af1f3f72 --- /dev/null +++ b/tests/unit/test_gpx_over_noisy_gps.py @@ -0,0 +1,182 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# +# This source code is licensed under the BSD license found in the +# LICENSE file in the root directory of this source tree. + +""" +Regression tests for geotagging a video whose own GPS is unusable. + +A user-supplied GPX is the documented escape hatch for a video whose embedded +GPS is bad, so an unusable embedded track must never be what rejects the video. +The GPX replaces that track entirely; the video is then only a source of +make/model and of a clock to sync the GPX against. + +Reported as "GPS is too noisy" persisting in the Desktop Uploader even after +attaching a valid GPX (a GoPro MAX2 .360 recorded with no GPS fix, where every +point is dropped by the noise filter). +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from mapillary_tools import exceptions, types +from mapillary_tools.geotag import factory +from mapillary_tools.geotag.video_extractors.gpx import GPXVideoExtractor, SyncMode +from mapillary_tools.geotag.video_extractors.native import NativeVideoExtractor +from mapillary_tools.gpmf import gpmf_gps_filter, gpmf_parser +from mapillary_tools.telemetry import GPSFix, GPSPoint + + +# Shape of the reported capture: no GPS fix and a DoP two orders of magnitude +# over the limit, so remove_noisy_points() drops every point +A_UNIX_TIME = 1789141181.0 + +GPX_XML = """ + + + + 506.6 + + + 506.8 + + + +""" + + +def _noisy_point(time: float, epoch_time: float) -> GPSPoint: + return GPSPoint( + time=time, + lat=48.1737635, + lon=11.5972871, + alt=559.275, + angle=None, + epoch_time=epoch_time, + fix=GPSFix.NO_FIX, + precision=2139.0, + ground_speed=0.749, + ) + + +@pytest.fixture +def video_path(tmp_path: Path) -> Path: + # Contents are irrelevant: the GPMF parser is stubbed out below. The file + # only has to exist so the extractor can open it and stat its size. + path = tmp_path / "GS018205.360" + path.write_bytes(b"not a real mp4") + return path + + +@pytest.fixture +def gpx_path(tmp_path: Path) -> Path: + path = tmp_path / "GS018205.360.gpx" + path.write_text(GPX_XML) + return path + + +@pytest.fixture +def noisy_gopro(monkeypatch: pytest.MonkeyPatch): + """Make every GoPro read return a track the noise filter rejects wholesale.""" + points = [ + _noisy_point(time=i * 0.04, epoch_time=A_UNIX_TIME + i * 0.1) for i in range(32) + ] + assert not gpmf_gps_filter.remove_noisy_points(points), ( + "fixture must be noisy enough for the filter to drop every point" + ) + + info = gpmf_parser.GoProInfo(gps=points, make="GoPro", model="MAX2") + monkeypatch.setattr(gpmf_parser, "extract_gopro_info", lambda *a, **kw: info) + return info + + +class TestNoiseGateStillApplies: + """Nothing below may weaken the gate on tracks we actually publish.""" + + def test_native_extraction_still_rejects_noise(self, video_path, noisy_gopro): + with pytest.raises(exceptions.MapillaryGPSNoiseError): + NativeVideoExtractor(video_path).extract() + + def test_noise_filter_is_opt_out_only(self, video_path, noisy_gopro): + metadata = NativeVideoExtractor(video_path, filter_noisy_points=False).extract() + assert len(metadata.points) == 32 + + +class TestGPXOverridesNoisyGPS: + def test_gpx_is_used_instead_of_failing(self, video_path, gpx_path, noisy_gopro): + metadata = GPXVideoExtractor(video_path, gpx_path).extract() + + assert [(p.lat, p.lon) for p in metadata.points] == [ + (48.1731513, 11.5973752), + (48.1731692, 11.5973021), + ] + + def test_camera_identity_survives(self, video_path, gpx_path, noisy_gopro): + """Falling back to a bare VIDEO would drop make/model from the upload.""" + metadata = GPXVideoExtractor(video_path, gpx_path).extract() + + assert metadata.filetype is types.FileType.GOPRO + assert (metadata.make, metadata.model) == ("GoPro", "MAX2") + + def test_noisy_points_still_provide_the_sync_clock( + self, video_path, gpx_path, noisy_gopro + ): + """ + The GPX starts 2s after the video's first GPS sample, so it must land at + t=2.0 rather than being rebased to t=0. + """ + metadata = GPXVideoExtractor(video_path, gpx_path).extract() + + assert metadata.points[0].time == pytest.approx(2.0) + assert metadata.points[1].time == pytest.approx(4.0) + + +class TestEmptyGPSFallsBack: + """Same escape hatch, but with no timestamps to sync against.""" + + @pytest.fixture + def empty_gopro(self, monkeypatch: pytest.MonkeyPatch): + info = gpmf_parser.GoProInfo(gps=[], make="GoPro", model="MAX2") + monkeypatch.setattr(gpmf_parser, "extract_gopro_info", lambda *a, **kw: info) + return info + + def test_gpx_is_rebased_from_zero(self, video_path, gpx_path, empty_gopro): + metadata = GPXVideoExtractor(video_path, gpx_path).extract() + + assert [p.time for p in metadata.points] == [0.0, 2.0] + + def test_strict_sync_still_refuses(self, video_path, gpx_path, empty_gopro): + extractor = GPXVideoExtractor( + video_path, gpx_path, sync_mode=SyncMode.STRICT_SYNC + ) + with pytest.raises(exceptions.MapillaryGPXEmptyError): + extractor.extract() + + +class TestChainedSourcesFallThrough: + """'--geotag_source native --geotag_source gpx' must reach the gpx stage.""" + + @pytest.mark.parametrize( + "error", + [ + exceptions.MapillaryGPSNoiseError("GPS is too noisy"), + exceptions.MapillaryGPXEmptyError("Empty GPS data found"), + exceptions.MapillaryVideoGPSNotFoundError("No GPS data found"), + ], + ) + def test_unusable_gps_is_reprocessable(self, error): + metadata = types.describe_error_metadata( + error, filename=Path("/tmp/x.360"), filetype=types.FileType.VIDEO + ) + assert factory._is_reprocessable(metadata) + + def test_unrelated_errors_are_not_reprocessable(self): + metadata = types.describe_error_metadata( + exceptions.MapillaryStationaryVideoError("Stationary"), + filename=Path("/tmp/x.360"), + filetype=types.FileType.VIDEO, + ) + assert not factory._is_reprocessable(metadata) From 898988bfebbc7137e2ff4bba788f68a4d8b3b202 Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Fri, 18 Sep 2026 16:53:17 +0200 Subject: [PATCH 2/2] Only let an external GPS source overturn a noise verdict Making MapillaryGPSNoiseError reprocessable was too broad: the default chain is native, exiftool_runtime so a video that the native parser had just rejected as noise fell through to exiftool, and `mapillary_tools process` with no flags at all started *accepting* the very file this branch is about. The two readers disagree because they do not see the same fields. For the reported capture the native GPMF parser reads a DoP of ~2100 against a limit of 1000 and drops all 32 points, while exiftool reports no DoP at all (precision=None), so remove_noisy_points() skips the DoP test and keeps the 24 points that have a 3D fix. exiftool losing GPSP is a pre-existing bug, and `--geotag_source exiftool_runtime` already accepts this file on main; what changed here was only that the default chain started reaching it. Unusable GPS is a verdict on the data, not on the reader that reported it, so only a source that supplies GPS from *outside* the video can overturn it. Gate the fall-through on the remaining sources: GPX and NMEA can rescue the file, another reader of the same embedded telemetry cannot. MapillaryVideoGPSNotFoundError is unaffected, since "could not read it" really is a verdict on the reader and retrying is fair. Verified on the reported file: process (default) -> GPS is too noisy process --geotag_source native -> GPS is too noisy process --geotag_source gpx -> 1 gopro ready process --geotag_source native,gpx -> 1 gopro ready --- mapillary_tools/geotag/factory.py | 63 +++++++++++++------ tests/unit/test_gpx_over_noisy_gps.py | 88 +++++++++++++++++++++------ 2 files changed, 115 insertions(+), 36 deletions(-) diff --git a/mapillary_tools/geotag/factory.py b/mapillary_tools/geotag/factory.py index e9848e26..b4fc0e01 100644 --- a/mapillary_tools/geotag/factory.py +++ b/mapillary_tools/geotag/factory.py @@ -27,6 +27,10 @@ LOG = logging.getLogger(__name__) +# Sources that read a GPS track from outside the video file, as opposed to +# re-reading the telemetry embedded in it +EXTERNAL_GPS_SOURCES = frozenset({SourceType.GPX, SourceType.NMEA}) + def parse_source_option(source: str) -> list[SourceOption]: """ @@ -68,10 +72,13 @@ def process( final_metadatas: list[types.MetadataOrError] = [] + # Indexable, so each step can see which sources are still to come + option_list = list(options) + # Paths (image path or video path) that will be sent to the next geotag process reprocessable_paths = set(paths) - for idx, option in enumerate(options): + for idx, option in enumerate(option_list): if LOG.isEnabledFor(logging.DEBUG): LOG.info( f"==> Processing {len(reprocessable_paths)} files with source {option}..." @@ -101,10 +108,10 @@ def process( else: video_metadata_or_errors = [] - more_option = idx < len(options) - 1 + remaining_options = option_list[idx + 1 :] for metadata in image_metadata_or_errors + video_metadata_or_errors: - if more_option and _is_reprocessable(metadata): + if remaining_options and _is_reprocessable(metadata, remaining_options): # Leave what it is for the next geotag process pass else: @@ -118,22 +125,40 @@ def process( return final_metadatas -def _is_reprocessable(metadata: types.MetadataOrError) -> bool: - if isinstance(metadata, types.ErrorMetadata): - if isinstance( - metadata.error, - ( - exceptions.MapillaryGeoTaggingError, - exceptions.MapillaryVideoGPSNotFoundError, - # Unusable GPS in this source is exactly what a later source - # (typically a user-supplied GPX) is there to replace - exceptions.MapillaryGPXEmptyError, - exceptions.MapillaryGPSNoiseError, - exceptions.MapillaryExiftoolNotFoundError, - exceptions.MapillaryExifToolXMLNotFoundError, - ), - ): - return True +def _is_reprocessable( + metadata: types.MetadataOrError, + remaining_options: T.Sequence[SourceOption] = (), +) -> bool: + if not isinstance(metadata, types.ErrorMetadata): + return False + + if isinstance( + metadata.error, + ( + exceptions.MapillaryGeoTaggingError, + exceptions.MapillaryVideoGPSNotFoundError, + exceptions.MapillaryExiftoolNotFoundError, + exceptions.MapillaryExifToolXMLNotFoundError, + ), + ): + return True + + # Unusable GPS is a verdict on the data, not on the reader that happened to + # report it, so only a source that supplies GPS from *outside* the video can + # rescue the file. Handing it to another reader of the same embedded + # telemetry just asks a second opinion of the same bad data, and the readers + # do not agree: exiftool reports no DoP at all, so it silently accepts a + # track that the native parser rejects as noise. + if isinstance( + metadata.error, + ( + exceptions.MapillaryGPXEmptyError, + exceptions.MapillaryGPSNoiseError, + ), + ): + return any( + option.source in EXTERNAL_GPS_SOURCES for option in remaining_options + ) return False diff --git a/tests/unit/test_gpx_over_noisy_gps.py b/tests/unit/test_gpx_over_noisy_gps.py index af1f3f72..7de2e59b 100644 --- a/tests/unit/test_gpx_over_noisy_gps.py +++ b/tests/unit/test_gpx_over_noisy_gps.py @@ -24,9 +24,11 @@ from mapillary_tools import exceptions, types from mapillary_tools.geotag import factory +from mapillary_tools.geotag.options import SourceOption, SourceType from mapillary_tools.geotag.video_extractors.gpx import GPXVideoExtractor, SyncMode from mapillary_tools.geotag.video_extractors.native import NativeVideoExtractor from mapillary_tools.gpmf import gpmf_gps_filter, gpmf_parser +from mapillary_tools.process_geotag_properties import DEFAULT_GEOTAG_SOURCE_OPTIONS from mapillary_tools.telemetry import GPSFix, GPSPoint @@ -156,27 +158,79 @@ def test_strict_sync_still_refuses(self, video_path, gpx_path, empty_gopro): extractor.extract() +def _error(error: Exception): + return types.describe_error_metadata( + error, filename=Path("/tmp/x.360"), filetype=types.FileType.VIDEO + ) + + +def _options(*sources: SourceType) -> list[SourceOption]: + return [SourceOption(source) for source in sources] + + +UNUSABLE_GPS_ERRORS = [ + exceptions.MapillaryGPSNoiseError("GPS is too noisy"), + exceptions.MapillaryGPXEmptyError("Empty GPS data found"), +] + + class TestChainedSourcesFallThrough: """'--geotag_source native --geotag_source gpx' must reach the gpx stage.""" - @pytest.mark.parametrize( - "error", - [ - exceptions.MapillaryGPSNoiseError("GPS is too noisy"), - exceptions.MapillaryGPXEmptyError("Empty GPS data found"), - exceptions.MapillaryVideoGPSNotFoundError("No GPS data found"), - ], - ) - def test_unusable_gps_is_reprocessable(self, error): - metadata = types.describe_error_metadata( - error, filename=Path("/tmp/x.360"), filetype=types.FileType.VIDEO + @pytest.mark.parametrize("error", UNUSABLE_GPS_ERRORS) + def test_external_gps_source_can_rescue(self, error): + assert factory._is_reprocessable(_error(error), _options(SourceType.GPX)) + assert factory._is_reprocessable(_error(error), _options(SourceType.NMEA)) + + def test_unreadable_gps_is_reprocessable_by_any_source(self): + """'could not read it' is a verdict on the reader, so retrying is fair.""" + assert factory._is_reprocessable( + _error(exceptions.MapillaryVideoGPSNotFoundError("No GPS data found")), + _options(SourceType.EXIFTOOL_RUNTIME), ) - assert factory._is_reprocessable(metadata) def test_unrelated_errors_are_not_reprocessable(self): - metadata = types.describe_error_metadata( - exceptions.MapillaryStationaryVideoError("Stationary"), - filename=Path("/tmp/x.360"), - filetype=types.FileType.VIDEO, + assert not factory._is_reprocessable( + _error(exceptions.MapillaryStationaryVideoError("Stationary")), + _options(SourceType.GPX), + ) + + def test_no_remaining_sources_is_not_reprocessable(self): + assert not factory._is_reprocessable( + _error(exceptions.MapillaryGPSNoiseError("GPS is too noisy")), [] + ) + + +class TestNoiseVerdictIsNotLaunderedThroughAnotherReader: + """ + Regression: making noise errors reprocessable made the *default* chain + (native, exiftool_runtime) accept a video that native had just rejected. + + exiftool reports no DoP for GoPro tracks, so remove_noisy_points() cannot + see the very field that condemns the file -- the reported capture has a DoP + of ~2100 against a limit of 1000 -- and the second reader waves through what + the first refused. Re-reading the same embedded telemetry must never be + treated as a way to overturn a verdict on that telemetry's quality. + """ + + @pytest.mark.parametrize("error", UNUSABLE_GPS_ERRORS) + @pytest.mark.parametrize( + "source", [SourceType.EXIFTOOL_RUNTIME, SourceType.EXIFTOOL_XML] + ) + def test_embedded_readers_cannot_overturn_it(self, error, source): + assert not factory._is_reprocessable(_error(error), _options(source)) + + @pytest.mark.parametrize("error", UNUSABLE_GPS_ERRORS) + def test_the_default_chain_does_not_fall_through(self, error): + """The exact chain `mapillary_tools process` runs with no flags.""" + default = [ + SourceType(source_type) for source_type in DEFAULT_GEOTAG_SOURCE_OPTIONS + ] + assert SourceType.NATIVE == default[0] + assert not factory._is_reprocessable(_error(error), _options(*default[1:])) + + @pytest.mark.parametrize("error", UNUSABLE_GPS_ERRORS) + def test_a_later_gpx_still_rescues_it(self, error): + assert factory._is_reprocessable( + _error(error), _options(SourceType.EXIFTOOL_RUNTIME, SourceType.GPX) ) - assert not factory._is_reprocessable(metadata)