diff --git a/mapillary_tools/geotag/factory.py b/mapillary_tools/geotag/factory.py index 4d1eeea0..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,18 +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, - 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/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..7de2e59b --- /dev/null +++ b/tests/unit/test_gpx_over_noisy_gps.py @@ -0,0 +1,236 @@ +# 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.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 + + +# 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() + + +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", 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), + ) + + def test_unrelated_errors_are_not_reprocessable(self): + 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) + )