From f70b4c51e154fa7c143d98593a1cc32a0316b076 Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Wed, 23 Sep 2026 12:09:38 +0200 Subject: [PATCH 1/4] Fix CAMM GPS epoch follow-ups from #828 #828 converts CAMM type 6 timestamps from GPS time to Unix time on read for Labpano, keyed on an exact make match, and writes Unix time back out. Review of what shipped found five problems. 1. Double conversion on write. camm_builder copies the source make into the CAMM track it writes, but wrote Unix time into the GPS time field, so our own upload of a 2024 Labpano video parsed back as 2034. denormalize_gps_epochs() now converts back to GPS time for makes that record it, which is also what the camera itself wrote and what v0.14.7 and earlier wrote. 2. Exact make match. "Labpano Technology Co.,Ltd" and similar variants were read ten years off, silently now that the elst widens to 64 bits instead of overflowing. The make now matches as a substring, and the mvhd creation_time decides ahead of the make when it can: GPS time sits one GPS epoch before it, Unix time sits close to it, ten years apart. That also reads the Unix-time outputs main has written since #828 correctly, whatever their make. 3. Silent GPX mis-sync. GPXVideoExtractor now raises MapillaryOutsideGPXTrackError when the GPX track, once synced, does not overlap the video's GPS track at all, like image geotagging already does. This replaces the warning #828 added. The error is made picklable, since video geotagging raises it in a worker process. 4. Mixed type 5 and type 6 samples. The native extractor dropped the type 5 points when a track had both; it now merges them in time order, and the GPX offset anchors on the first point that carries a timestamp rather than on the first point. 5. ExifTool 18 s behind native. ExifTool adds the GPS epoch offset without the leap seconds. Native is right: on a PanoX V2 file the first GPS fix plus the mvhd duration ends 0.02 s from the creation time, where ExifTool's reading ends 18 s after the file was created. The ExifTool reader now subtracts the leap seconds for CAMM tracks from makes that record GPS time. Two integration tests paired the 2019 GoPro sample video with a 2025 GPX file and asserted success, which is the mis-sync in 3. They now shift the GPX to the video's time, and a new test asserts that the unrelated pairing fails. --- mapillary_tools/camm/camm_builder.py | 8 +- mapillary_tools/camm/camm_parser.py | 145 +++++-- mapillary_tools/exceptions.py | 8 + mapillary_tools/exiftool_read_video.py | 34 +- .../geotag/video_extractors/gpx.py | 77 +++- .../geotag/video_extractors/native.py | 6 +- mapillary_tools/telemetry.py | 24 +- tests/cli/camm_parser.py | 4 +- tests/integration/test_process.py | 58 ++- tests/unit/test_camm_parser.py | 7 +- tests/unit/test_gps_epoch.py | 375 +++++++++++++++++- 11 files changed, 667 insertions(+), 79 deletions(-) diff --git a/mapillary_tools/camm/camm_builder.py b/mapillary_tools/camm/camm_builder.py index d8f6a6b6..12b1574d 100644 --- a/mapillary_tools/camm/camm_builder.py +++ b/mapillary_tools/camm/camm_builder.py @@ -255,9 +255,13 @@ def _f( creation_time = mvhd_data.get("creation_time", 0) modification_time = mvhd_data.get("modification_time", 0) + # Write GPS timestamps in the epoch the make records, so that the + # output parses back to the same Unix times + gps = camm_parser.denormalize_gps_epochs(camm_info.gps or [], camm_info.make) + # Multiplex points for creating elst track: list[geo.Point] = [ - *(camm_info.gps or []), + *gps, *(camm_info.mini_gps or []), ] track.sort(key=lambda p: p.time) @@ -267,7 +271,7 @@ def _f( # Multiplex telemetry measurements measurements: list[camm_parser.TelemetryMeasurement] = [ - *(camm_info.gps or []), + *gps, *(camm_info.mini_gps or []), *(camm_info.accl or []), *(camm_info.gyro or []), diff --git a/mapillary_tools/camm/camm_parser.py b/mapillary_tools/camm/camm_parser.py index b5f42049..c8cd5016 100644 --- a/mapillary_tools/camm/camm_parser.py +++ b/mapillary_tools/camm/camm_parser.py @@ -131,19 +131,83 @@ def extract_camm_info(fp: T.BinaryIO, telemetry_only: bool = False) -> CAMMInfo # Makes whose CAMM type 6 samples record GPS time (seconds since 1980-01-06), -# as the CAMM spec describes. Everything else -- Insta360, and the CAMM tracks -# mapillary_tools writes itself -- records Unix time in the same field, so -# converting unconditionally would push those ~10 years into the future. -_GPS_EPOCH_MAKES = frozenset(["labpano"]) +# as the CAMM spec describes. Everything else -- Insta360, for one -- records +# Unix time in the same field, so converting unconditionally would push those +# ~10 years into the future. Matched as a substring of the lowercased make, +# because firmware reports the same vendor in more than one form. +_GPS_EPOCH_MAKES = ("labpano",) # Seconds between the mp4 epoch (1904-01-01) and the Unix epoch. _MP4_EPOCH_UNIX_OFFSET = 2082844800 -# Tolerance for recognizing a gap as "off by exactly one GPS epoch". Checking -# for that specific distance rather than for general implausibility matters: -# some cameras write a meaningless mvhd creation_time (a GoPro HERO7 recorded -# in 2022 reports 2016), so a generic bound would fire constantly. -_GPS_EPOCH_GAP_TOLERANCE = 30 * 24 * 3600 +# How close the first GPS timestamp has to be to the mvhd creation_time, read +# either as Unix time or as GPS time, for the creation time to decide the +# epoch. The two readings are ten years apart, so this can be generous enough +# to absorb local-time creation times and long recordings without ever being +# ambiguous. +_CREATION_TIME_TOLERANCE = 30 * 24 * 3600 + + +def make_records_gps_time(make: str) -> bool: + """ + Whether cameras of this make record GPS time in CAMM type 6 samples. + + >>> make_records_gps_time("Labpano") + True + >>> make_records_gps_time("Labpano Technology Co.,Ltd") + True + >>> make_records_gps_time("Insta360") + False + """ + normalized = make.strip().lower() + return any(m in normalized for m in _GPS_EPOCH_MAKES) + + +def _records_gps_time( + first_epoch_time: float, make: str, creation_time: float | None +) -> bool: + """ + Decide the epoch of raw CAMM type 6 timestamps. + + The creation time of the video decides when it can: GPS time reads as one + GPS epoch before it, Unix time reads close to it. The make decides only + when the creation time settles neither, because it is missing or + meaningless (a GoPro HERO7 recorded in 2022 reports 2016). + + >>> creation_time = 1705574637 # 2024-01-18T10:43:57Z + >>> _records_gps_time(1389609661, "", creation_time) # GPS time + True + >>> _records_gps_time(1705574443, "Labpano", creation_time) # Unix time + False + >>> _records_gps_time(1389609661, "Labpano", None) + True + >>> _records_gps_time(1705574443, "Insta360", None) + False + """ + if creation_time: + gap = creation_time - first_epoch_time + if abs(gap - telemetry.GPS_EPOCH_UNIX_OFFSET) < _CREATION_TIME_TOLERANCE: + return True + if abs(gap) < _CREATION_TIME_TOLERANCE: + return False + + return make_records_gps_time(make) + + +def _extract_creation_time(moov: MovieBoxParser | None) -> float | None: + """Return the mvhd creation_time as Unix time, or None if unset.""" + if moov is None: + return None + + try: + creation_time = moov.extract_mvhd_boxdata().get("creation_time", 0) + except Exception: + return None + + if not creation_time: + return None + + return creation_time - _MP4_EPOCH_UNIX_OFFSET def _normalize_gps_epochs( @@ -153,37 +217,49 @@ def _normalize_gps_epochs( Rewrite CAMMGPSPoint.epoch_time in place so it is Unix time regardless of which epoch the producer used. - This is the only place CAMM GPS timestamps change epoch. Everything - downstream, including the serializer, treats them as Unix time. + This is the only place CAMM GPS timestamps are read into Unix time. + Everything downstream treats them as Unix time, until + denormalize_gps_epochs() converts them back for writing. """ - if not gps: - return - - if make.strip().lower() in _GPS_EPOCH_MAKES: - for point in gps: - if point.epoch_time > 0: - point.epoch_time = telemetry.gps_epoch_to_unix(point.epoch_time) - first = next((p.epoch_time for p in gps if p.epoch_time > 0), None) - if first is None or moov is None: + if first is None: return - try: - creation_time = moov.extract_mvhd_boxdata().get("creation_time", 0) - except Exception: + if not _records_gps_time(first, make, _extract_creation_time(moov)): return - if not creation_time: - return + for point in gps: + if point.epoch_time > 0: + point.epoch_time = telemetry.gps_epoch_to_unix(point.epoch_time) + + +def denormalize_gps_epochs( + gps: T.Sequence[telemetry.CAMMGPSPoint], make: str +) -> list[telemetry.CAMMGPSPoint]: + """ + Return copies of the points with epoch_time converted to the epoch this + make records, for writing. The inverse of _normalize_gps_epochs(). + + The source make is copied into every CAMM track mapillary_tools writes, so + a Labpano video written with Unix time in its GPS track would read back + ten years in the future. Writing GPS time for these makes keeps our output + parseable, and leaves the camera's own timestamps as the camera wrote them. + + Only the make is available here, and that is enough: the reader decides by + the creation time first, which is copied from the source, and by the make + otherwise, so either way it reads back what was written. + """ + if not make_records_gps_time(make): + return list(gps) - gap = abs(first - (creation_time - _MP4_EPOCH_UNIX_OFFSET)) - if abs(gap - telemetry.GPS_EPOCH_UNIX_OFFSET) < _GPS_EPOCH_GAP_TOLERANCE: - LOG.warning( - "CAMM GPS timestamps are one GPS epoch away from the creation time " - "of the video. The camera (make %r) may record GPS time where Unix " - "time is expected, or the reverse; please report this video", - make, + return [ + dataclasses.replace( + point, epoch_time=telemetry.unix_to_gps_epoch(point.epoch_time) ) + if point.epoch_time > 0 + else point + for point in gps + ] def extract_camera_make_and_model(fp: T.BinaryIO) -> tuple[str, str]: @@ -305,9 +381,8 @@ def serialize(cls, data: telemetry.CAMMGPSPoint) -> bytes: { "type": cls.serialized_camm_type.value, "data": { - # Written as Unix time, which is what every released - # version of mapillary_tools has written and what readers - # of our output expect. Do not convert here. + # Written as is. camm_builder converts to the epoch the + # make records beforehand (see denormalize_gps_epochs). "time_gps_epoch": data.epoch_time, "gps_fix_type": data.gps_fix_type, "latitude": data.lat, diff --git a/mapillary_tools/exceptions.py b/mapillary_tools/exceptions.py index 5474fd1b..60777edf 100644 --- a/mapillary_tools/exceptions.py +++ b/mapillary_tools/exceptions.py @@ -81,6 +81,14 @@ def __init__( self.gpx_start_time = gpx_start_time self.gpx_end_time = gpx_end_time + def __reduce__(self): + # Pickle with every argument, so that the error survives the trip back + # from a worker process (video geotagging raises it in one) + return ( + self.__class__, + (self.args[0], self.image_time, self.gpx_start_time, self.gpx_end_time), + ) + class MapillaryDuplicationError(MapillaryDescriptionError): def __init__( diff --git a/mapillary_tools/exiftool_read_video.py b/mapillary_tools/exiftool_read_video.py index 4257abca..f08958ed 100644 --- a/mapillary_tools/exiftool_read_video.py +++ b/mapillary_tools/exiftool_read_video.py @@ -11,7 +11,8 @@ import typing as T import xml.etree.ElementTree as ET -from . import exif_read, exiftool_read, geo +from . import exif_read, exiftool_read, geo, telemetry +from .camm import camm_parser from .telemetry import GPSFix, GPSPoint from .utils import sanitize_serial @@ -51,6 +52,17 @@ def _maybe_float(text: str | None) -> float | None: return None +def _exiftool_gps_time_to_unix(exiftool_time: float) -> float: + """ + Convert a CAMM GPS timestamp as ExifTool renders it -- GPS time plus the + epoch difference, with no leap-second correction -- to Unix time (UTC). + + >>> _exiftool_gps_time_to_unix(1705574461.6) # 2024-01-18T10:41:01.6 + 1705574443.6 + """ + return telemetry.gps_epoch_to_unix(exiftool_time - telemetry.GPS_EPOCH_UNIX_OFFSET) + + def _index_text_by_tag(elements: T.Iterable[ET.Element]) -> dict[str, list[str]]: texts_by_tag: dict[str, list[str]] = {} for element in elements: @@ -550,9 +562,29 @@ def _extract_gps_track_from_track(self) -> list[GPSPoint]: gps_precision_tag=f"{track_ns}:GPSHPositioningError", ) if track: + if self._is_camm_track_in_gps_time(track_ns): + for point in track: + if point.epoch_time is not None: + point.epoch_time = _exiftool_gps_time_to_unix( + point.epoch_time + ) return track return [] + def _is_camm_track_in_gps_time(self, track_ns: str) -> bool: + """ + Whether the track is CAMM from a camera that records GPS time. + + ExifTool converts those timestamps by the epoch difference alone, so + they read 18s (the leap seconds since 1980) ahead of what the native + CAMM parser returns for the same video. + """ + meta_format = self._extract_alternative_fields([f"{track_ns}:MetaFormat"], str) + if meta_format != "camm": + return False + make = self.extract_make() + return make is not None and camm_parser.make_records_gps_time(make) + def _extract_alternative_fields( self, fields: T.Sequence[str], diff --git a/mapillary_tools/geotag/video_extractors/gpx.py b/mapillary_tools/geotag/video_extractors/gpx.py index 00722bd1..4e8f207c 100644 --- a/mapillary_tools/geotag/video_extractors/gpx.py +++ b/mapillary_tools/geotag/video_extractors/gpx.py @@ -6,6 +6,7 @@ from __future__ import annotations import dataclasses +import datetime import enum import logging import sys @@ -18,6 +19,7 @@ from typing_extensions import override from ... import exceptions, geo, types, utils +from ...serializer.description import build_capture_time from ..utils import parse_gpx from .base import BaseVideoExtractor from .native import NativeVideoExtractor @@ -25,11 +27,6 @@ LOG = logging.getLogger(__name__) -# A GPX track and the video it is synced against should overlap in time. Warn -# above a day, which no legitimate pairing needs and an epoch mix-up exceeds by -# orders of magnitude. -_IMPLAUSIBLE_OFFSET_SECONDS = 24 * 3600 - class SyncMode(enum.Enum): # Sync by video GPS timestamps if found, otherwise rebase @@ -78,19 +75,45 @@ def extract(self) -> types.VideoMetadata: self._rebase_times(gpx_points) else: offset = self._gpx_offset(gpx_points, native_video_metadata.points) - if abs(offset) > _IMPLAUSIBLE_OFFSET_SECONDS: - LOG.warning( - "Syncing %s against %s requires an offset of %.0f seconds (%.1f days). " - "The GPX file probably does not belong to this video", - self.video_path, - self.gpx_path, - offset, - offset / 86400, - ) + if offset: + self._check_overlap(gpx_points, native_video_metadata.points, offset) self._rebase_times(gpx_points, offset=offset) return dataclasses.replace(native_video_metadata, points=gpx_points) + @classmethod + def _check_overlap( + cls, + gpx_points: T.Sequence[geo.Point], + video_gps_points: T.Sequence[geo.Point], + offset: float, + ) -> None: + """ + Raise if the GPX track, once synced by offset, does not overlap the + video in time. + + Nothing downstream can use such a sync: every point would fall outside + the video, and at upload the edit list would carry the whole offset. + That used to overflow and abort the upload. Now that the edit list + widens instead, this is what keeps an epoch mix-up or a GPX file from + another day from failing silently. + """ + # The Unix time of video time 0, in the convention _rebase_times() uses + video_start_time = gpx_points[0].time - offset + video_first = video_start_time + min(p.time for p in video_gps_points) + video_last = video_start_time + max(p.time for p in video_gps_points) + gpx_first = min(p.time for p in gpx_points) + gpx_last = max(p.time for p in gpx_points) + + if gpx_last < video_first or video_last < gpx_first: + raise exceptions.MapillaryOutsideGPXTrackError( + f"The video GPS track ({_isoformat(video_first)} to {_isoformat(video_last)}) " + f"does not overlap the GPX track ({_isoformat(gpx_first)} to {_isoformat(gpx_last)}) in time", + image_time=build_capture_time(video_first), + gpx_start_time=build_capture_time(gpx_first), + gpx_end_time=build_capture_time(gpx_last), + ) + @classmethod def _rebase_times(cls, points: T.Sequence[geo.Point], offset: float = 0.0) -> None: """ @@ -121,13 +144,25 @@ def _gpx_offset( if not gpx_points or not video_gps_points: return offset - # Both sides must be Unix time here. Video GPS timestamps are stored in - # whatever epoch their container uses (CAMM records GPS time, GoPro - # records Unix time), so go through get_unix_time() rather than reading - # the raw attributes -- that also skips zero/invalid timestamps. - video_unix_time = video_gps_points[0].get_unix_time() - - if video_unix_time is not None: + # Both sides must be Unix time here. get_unix_time() skips + # zero/invalid timestamps, and points that carry none at all, like + # CAMM type 5 points, which a track can start with. + anchor = next( + (p for p in video_gps_points if p.get_unix_time() is not None), None + ) + + if anchor is not None: + anchor_unix_time = T.cast(float, anchor.get_unix_time()) + # The Unix time the first video point would have + video_unix_time = anchor_unix_time - ( + anchor.time - video_gps_points[0].time + ) offset = gpx_points[0].time - video_unix_time return offset + + +def _isoformat(unix_time: float) -> str: + return datetime.datetime.fromtimestamp( + unix_time, tz=datetime.timezone.utc + ).isoformat() diff --git a/mapillary_tools/geotag/video_extractors/native.py b/mapillary_tools/geotag/video_extractors/native.py index a4a329e7..301bd1a6 100644 --- a/mapillary_tools/geotag/video_extractors/native.py +++ b/mapillary_tools/geotag/video_extractors/native.py @@ -69,11 +69,15 @@ def extract(self) -> types.VideoMetadata: if not camm_info.gps and not camm_info.mini_gps: raise exceptions.MapillaryGPXEmptyError("Empty GPS data found") + # A track may mix type 6 and type 5 samples, so use both + points: list[geo.Point] = [*(camm_info.gps or []), *(camm_info.mini_gps or [])] + points.sort(key=lambda p: p.time) + return types.VideoMetadata( filename=self.video_path, filesize=utils.get_file_size(self.video_path), filetype=types.FileType.CAMM, - points=T.cast(T.List[geo.Point], camm_info.gps or camm_info.mini_gps), + points=points, make=camm_info.make, model=camm_info.model, ) diff --git a/mapillary_tools/telemetry.py b/mapillary_tools/telemetry.py index d6084a05..7521ea14 100644 --- a/mapillary_tools/telemetry.py +++ b/mapillary_tools/telemetry.py @@ -64,7 +64,7 @@ def gps_epoch_to_unix(gps_epoch_time: float) -> float: """ Convert seconds since the GPS epoch (GPS time) to Unix time (UTC). - Only called at the parse boundary, for producers known to record GPS time. + Only called at the CAMM parse boundary, for tracks that record GPS time. >>> gps_epoch_to_unix(1470558405.9798455) 1786523187.9798455 @@ -75,6 +75,19 @@ def gps_epoch_to_unix(gps_epoch_time: float) -> float: return approx_unix_time - _gps_utc_offset_at(approx_unix_time) +def unix_to_gps_epoch(unix_time: float) -> float: + """ + Convert Unix time (UTC) to seconds since the GPS epoch (GPS time). + + The inverse of gps_epoch_to_unix(). Only called when writing a CAMM track + for a camera that records GPS time. + + >>> unix_to_gps_epoch(1786523187.9798455) + 1470558405.9798455 + """ + return unix_time - GPS_EPOCH_UNIX_OFFSET + _gps_utc_offset_at(unix_time) + + @unique class GPSFix(Enum): NO_FIX = 0 @@ -160,10 +173,11 @@ class CAMMGPSPoint(TimestampedMeasurement, Point): # # The corresponding CAMM box field is named time_gps_epoch, but what # producers actually store there varies: Labpano cameras record GPS time, - # while Insta360 and mapillary_tools itself record Unix time. Whatever the - # producer wrote is normalized to Unix time once, when the CAMM track is - # parsed (see camm_parser.extract_camm_info), so that everything - # downstream can rely on a single meaning. + # while Insta360 records Unix time. Whatever the producer wrote is + # normalized to Unix time once, when the CAMM track is parsed, and + # converted back only when mapillary_tools writes a CAMM track for a make + # that records GPS time (see camm_parser.denormalize_gps_epochs), so that + # everything in between can rely on a single meaning. epoch_time: float gps_fix_type: int horizontal_accuracy: float diff --git a/tests/cli/camm_parser.py b/tests/cli/camm_parser.py index 1acd0155..ff8f7ed8 100644 --- a/tests/cli/camm_parser.py +++ b/tests/cli/camm_parser.py @@ -44,7 +44,9 @@ def _convert(path: pathlib.Path): track.description = "Invalid CAMM video" return track - points = T.cast(T.List[geo.Point], camm_info.gps or camm_info.mini_gps) + # A track may mix type 6 and type 5 samples, so use both + points: T.List[geo.Point] = [*(camm_info.gps or []), *(camm_info.mini_gps or [])] + points.sort(key=lambda p: p.time) track.segments.append(_convert_points_to_gpx_segment(points)) make_model = json.dumps({"make": camm_info.make, "model": camm_info.model}) diff --git a/tests/integration/test_process.py b/tests/integration/test_process.py index 765fad80..df09b3d7 100644 --- a/tests/integration/test_process.py +++ b/tests/integration/test_process.py @@ -9,6 +9,7 @@ import subprocess from pathlib import Path +import gpxpy import py.path import pytest @@ -623,11 +624,32 @@ def test_process_video_geotag_source_gpx_not_found(setup_data: py.path.local): assert descs[0]["error"]["type"] == "MapillaryVideoGPSNotFoundError" +# The GPS track of gopro_data/max-360mode.mp4 starts at 2019-11-18T23:44:42.59Z +_GOPRO_MAX_GPS_START = datetime.datetime( + 2019, 11, 18, 23, 44, 40, tzinfo=datetime.timezone.utc +) + + +def _copy_gpx_shifted_to( + src: py.path.local, dst: py.path.local, start: datetime.datetime +) -> None: + """Copy a GPX file with its times shifted to begin at start.""" + with open(src) as fp: + gpx = gpxpy.parse(fp) + start_time = gpx.get_time_bounds().start_time + assert start_time is not None + gpx.adjust_time(start - start_time) + dst.write(gpx.to_xml()) + + def test_process_video_geotag_source_with_gopro_gpx_specified( setup_data: py.path.local, ): video_path = setup_data.join("gopro_data").join("max-360mode.mp4") - gpx_file = setup_data.join("gpx").join("sf_30km_h.gpx") + gpx_file = setup_data.join("gpx").join("max-360mode.gpx") + _copy_gpx_shifted_to( + setup_data.join("gpx").join("sf_30km_h.gpx"), gpx_file, _GOPRO_MAX_GPS_START + ) descs = run_process_for_descs( [ @@ -645,6 +667,33 @@ def test_process_video_geotag_source_with_gopro_gpx_specified( assert len(descs[0]["MAPGPSTrack"]) > 0 +def test_process_video_geotag_source_with_gpx_outside_video( + setup_data: py.path.local, +): + """A GPX file recorded at another time than the video must not sync.""" + video_path = setup_data.join("gopro_data").join("max-360mode.mp4") + # Recorded in 2025, five years after the video + gpx_file = setup_data.join("gpx").join("sf_30km_h.gpx") + + descs = run_process_for_descs( + [ + *[ + "--video_geotag_source", + json.dumps({"source": "gpx", "source_path": str(gpx_file)}), + ], + str(video_path), + ] + ) + + assert len(descs) == 1 + assert descs[0]["error"]["type"] == "MapillaryOutsideGPXTrackError" + assert descs[0]["error"]["vars"] == { + "image_time": "2019_11_18_23_44_42_590", + "gpx_start_time": "2025_03_14_07_00_00_000", + "gpx_end_time": "2025_03_14_07_01_33_624", + } + + def test_process_geotag_with_gpx_pattern_not_found(setup_data: py.path.local): video_path = setup_data.join("gopro_data").join("max-360mode.mp4") @@ -661,8 +710,11 @@ def test_process_geotag_with_gpx_pattern_not_found(setup_data: py.path.local): def test_process_geotag_with_gpx_pattern(setup_data: py.path.local): video_path = setup_data.join("gopro_data").join("max-360mode.mp4") - gpx_file = setup_data.join("gpx").join("sf_30km_h.gpx") - gpx_file.copy(setup_data.join("gopro_data").join("max-360mode.gpx")) + _copy_gpx_shifted_to( + setup_data.join("gpx").join("sf_30km_h.gpx"), + setup_data.join("gopro_data").join("max-360mode.gpx"), + _GOPRO_MAX_GPS_START, + ) descs = run_process_for_descs( [ diff --git a/tests/unit/test_camm_parser.py b/tests/unit/test_camm_parser.py index 112a1df5..37c54686 100644 --- a/tests/unit/test_camm_parser.py +++ b/tests/unit/test_camm_parser.py @@ -802,10 +802,9 @@ def test_extract_camm_info_routes_plain_points_to_mini_gps(): def test_camm_gps_timestamps_round_trip_as_unix(): """process -> build CAMM -> re-read must return the input timestamps. - mapillary_tools has always written Unix time into the CAMM type 6 - time_gps_epoch field, and released versions read it back as Unix time. - Writing anything else would make our output unreadable by them, so the - serializer must not convert. + Without a make that records GPS time, mapillary_tools writes Unix time + into the CAMM type 6 time_gps_epoch field, as every released version has. + Makes that record GPS time are covered in test_gps_epoch.py. """ unix_times = [1655503450.5, 1655503451.5] points = [ diff --git a/tests/unit/test_gps_epoch.py b/tests/unit/test_gps_epoch.py index 87627b70..6984fdb4 100644 --- a/tests/unit/test_gps_epoch.py +++ b/tests/unit/test_gps_epoch.py @@ -12,22 +12,31 @@ is a ~315,964,800s (10 year) error. The invariant these tests protect: ``CAMMGPSPoint.epoch_time`` is *always* -Unix time in memory. The conversion happens exactly once, when the CAMM track -is parsed, and never again -- in particular not in the serializer, which must -keep writing Unix time so files stay readable by released versions. +Unix time in memory. It is converted from GPS time once, when the CAMM track is +parsed, and back once, when mapillary_tools writes a CAMM track for a make that +records GPS time -- so that whatever it writes reads back to the same instants. + +Which epoch a track records is decided by the mvhd creation time of the video +when that is conclusive, and by the make otherwise. """ from __future__ import annotations +import datetime +import io +import pickle +import typing as T +import xml.etree.ElementTree as ET from pathlib import Path import pytest - -from mapillary_tools import geo, telemetry +from mapillary_tools import exceptions, geo, telemetry, types, uploader from mapillary_tools.camm import camm_builder, camm_parser +from mapillary_tools.exiftool_read_video import ExifToolReadVideo from mapillary_tools.geotag.options import SourceOption, SourceType from mapillary_tools.geotag.video_extractors.gpx import GPXVideoExtractor -from mapillary_tools.mp4 import construct_mp4_parser as cparser +from mapillary_tools.geotag.video_extractors.native import CAMMVideoExtractor +from mapillary_tools.mp4 import construct_mp4_parser as cparser, simple_mp4_builder # Seconds between the two epochs, i.e. the size of the bug @@ -37,6 +46,9 @@ A_GPS_TIME = 1470558405.9798455 A_UNIX_TIME = 1786523187.9798455 +# Seconds between the mp4 epoch (1904-01-01) and the Unix epoch +MP4_UNIX_DELTA = 2082844800 + def _camm_point(time: float, epoch_time: float) -> telemetry.CAMMGPSPoint: return telemetry.CAMMGPSPoint( @@ -70,6 +82,67 @@ def _gps_point(time: float, epoch_time: float | None) -> telemetry.GPSPoint: ) +def _write_camm_mp4( + points: T.Sequence[geo.Point], make: str, creation_time: float | None +) -> bytes: + """ + Write points as a CAMM track into an empty mp4, the way the uploader does. + + creation_time is the Unix time to put in mvhd, or None to leave it unset. + """ + mp4_creation_time = ( + 0 if creation_time is None else int(creation_time) + MP4_UNIX_DELTA + ) + mvhd: cparser.BoxDict = { + "type": b"mvhd", + "data": { + "creation_time": mp4_creation_time, + "modification_time": mp4_creation_time, + "timescale": 1000, + "duration": 36000 * 1000, + }, + } + src = cparser.MP4WithoutSTBLBuilderConstruct.build_boxlist( + [ + {"type": b"ftyp", "data": b"test"}, + {"type": b"moov", "data": [mvhd]}, + ] + ) + metadata = types.VideoMetadata( + Path(""), + filetype=types.FileType.CAMM, + points=list(points), + make=make, + model="PanoX V2", + ) + camm_info = uploader.VideoUploader.prepare_camm_info(metadata) + target_fp = simple_mp4_builder.transform_mp4( + io.BytesIO(src), camm_builder.camm_sample_generator2(camm_info) + ) + return target_fp.read() + + +def _read_camm(data: bytes) -> camm_parser.CAMMInfo: + camm_info = camm_parser.extract_camm_info(io.BytesIO(data)) + assert camm_info is not None + return camm_info + + +def _unix_times(camm_info: camm_parser.CAMMInfo) -> list[float]: + return [p.epoch_time for p in camm_info.gps or []] + + +# A two point track at A_UNIX_TIME, as parse_gpx() or the parser would return it +UNIX_TIMES = [A_UNIX_TIME, A_UNIX_TIME + 1] + + +def _unix_track() -> list[telemetry.CAMMGPSPoint]: + return [ + _camm_point(time=float(idx), epoch_time=epoch_time) + for idx, epoch_time in enumerate(UNIX_TIMES) + ] + + class TestEpochConversion: def test_known_instant(self): assert telemetry.gps_epoch_to_unix(A_GPS_TIME) == A_UNIX_TIME @@ -82,6 +155,13 @@ def test_leap_seconds_accumulate(self): telemetry.gps_epoch_to_unix(A_GPS_TIME) == A_GPS_TIME + GPS_UNIX_DELTA - 18 ) + def test_unix_to_gps_epoch_is_the_inverse(self): + assert telemetry.unix_to_gps_epoch(A_UNIX_TIME) == A_GPS_TIME + # One instant per leap second era, from before the first leap second + for unix_time in [400000000.5, 1000000000.5, 1500000000.5, A_UNIX_TIME]: + gps_time = telemetry.unix_to_gps_epoch(unix_time) + assert telemetry.gps_epoch_to_unix(gps_time) == unix_time + class TestParseBoundaryNormalization: """The one place an epoch conversion is allowed to happen.""" @@ -112,6 +192,120 @@ def test_invalid_timestamps_are_not_converted(self): assert points[0].epoch_time == 0.0 assert points[0].get_unix_time() is None + @pytest.mark.parametrize( + "make", ["Labpano Technology Co.,Ltd", "LABPANO TECHNOLOGY", "Labpano Pilot"] + ) + def test_make_variants_are_matched(self, make: str): + """Firmware reports the vendor in more than one form.""" + points = [_camm_point(time=0.0, epoch_time=A_GPS_TIME)] + camm_parser._normalize_gps_epochs(points, make) + assert points[0].epoch_time == A_UNIX_TIME + + +class TestCreationTimeEvidence: + """The creation time decides the epoch whenever it is conclusive.""" + + # Labpano stamps the creation time at the end of the recording + CREATION_TIME = A_UNIX_TIME + 600 + + @pytest.mark.parametrize("make", ["", "Insta360", "Some Future Camera"]) + def test_gps_time_is_recognized_whatever_the_make(self, make: str): + assert camm_parser._records_gps_time(A_GPS_TIME, make, self.CREATION_TIME) + + @pytest.mark.parametrize("make", ["Labpano", "Labpano Technology Co.,Ltd"]) + def test_unix_time_is_recognized_whatever_the_make(self, make: str): + assert not camm_parser._records_gps_time(A_UNIX_TIME, make, self.CREATION_TIME) + + def test_inconclusive_creation_time_falls_back_to_make(self): + # A GoPro HERO7 recorded in 2022 reports 2016 + meaningless = A_UNIX_TIME - 6 * 365 * 24 * 3600 + assert camm_parser._records_gps_time(A_GPS_TIME, "Labpano", meaningless) + assert not camm_parser._records_gps_time(A_UNIX_TIME, "Insta360", meaningless) + + def test_unknown_make_in_gps_time_is_read_from_the_file(self): + """The mvhd creation time reaches the decision through the parser.""" + data = _write_camm_mp4( + [_camm_point(time=0.0, epoch_time=A_GPS_TIME)], + make="", + creation_time=self.CREATION_TIME, + ) + assert _unix_times(_read_camm(data)) == [A_UNIX_TIME] + + +class TestWriteRoundTrip: + """ + Whatever mapillary_tools writes must read back to the same Unix times. + + The source make is copied into the output, so writing Unix time for a + Labpano video used to read back one GPS epoch later: 2026 became 2036. + """ + + @pytest.mark.parametrize("creation_time", [A_UNIX_TIME + 600, None]) + @pytest.mark.parametrize( + "make", ["Labpano", "Labpano Technology Co.,Ltd", "Insta360", ""] + ) + def test_round_trip(self, make: str, creation_time: float | None): + data = _write_camm_mp4(_unix_track(), make, creation_time) + assert _unix_times(_read_camm(data)) == UNIX_TIMES + + def test_reprocessing_output_is_stable(self): + """process_and_upload output, processed again, is the same track.""" + data = _write_camm_mp4(_unix_track(), "Labpano", A_UNIX_TIME + 600) + for _ in range(2): + camm_info = _read_camm(data) + assert _unix_times(camm_info) == UNIX_TIMES + data = _write_camm_mp4( + camm_info.gps or [], camm_info.make, A_UNIX_TIME + 600 + ) + + @pytest.mark.parametrize( + "make, raw_times", + [ + ("Labpano", [A_GPS_TIME, A_GPS_TIME + 1]), + ("Insta360", UNIX_TIMES), + ], + ) + def test_written_in_the_epoch_the_make_records( + self, monkeypatch, make: str, raw_times: list[float] + ): + """Labpano output carries GPS time, just like the camera's own file.""" + data = _write_camm_mp4(_unix_track(), make, A_UNIX_TIME + 600) + monkeypatch.setattr(camm_parser, "_normalize_gps_epochs", lambda *_: None) + assert _unix_times(_read_camm(data)) == raw_times + + +class TestMixedCAMMTypes: + """A track may mix type 6 and type 5 samples; neither may be dropped.""" + + def _mixed_track(self) -> list[geo.Point]: + return [ + geo.Point(time=0.0, lat=37.0, lon=14.0, alt=None, angle=None), + _camm_point(time=1.0, epoch_time=A_UNIX_TIME + 1), + geo.Point(time=2.0, lat=37.0, lon=14.0, alt=None, angle=None), + _camm_point(time=3.0, epoch_time=A_UNIX_TIME + 3), + ] + + def test_extractor_returns_both_types_in_order(self, tmp_path: Path): + video_path = tmp_path / "mixed.mp4" + video_path.write_bytes( + _write_camm_mp4(self._mixed_track(), "Labpano", A_UNIX_TIME + 600) + ) + + points = CAMMVideoExtractor(video_path).extract().points + + assert [p.time for p in points] == [0.0, 1.0, 2.0, 3.0] + assert [type(p) for p in points] == [ + geo.Point, + telemetry.CAMMGPSPoint, + geo.Point, + telemetry.CAMMGPSPoint, + ] + + def test_gpx_offset_anchors_on_the_first_timestamped_point(self): + # The GPX starts 30s before the video does + gpx_points = [_camm_point(time=A_UNIX_TIME - 30, epoch_time=A_UNIX_TIME - 30)] + assert GPXVideoExtractor._gpx_offset(gpx_points, self._mixed_track()) == -30.0 + class TestPointAccessors: def test_both_point_types_report_unix_time(self): @@ -153,6 +347,175 @@ def test_missing_video_timestamp_yields_no_offset(self): assert GPXVideoExtractor._gpx_offset(gpx_points, video_points) == 0.0 +def _gpx_track(start: float, duration: int) -> list[telemetry.CAMMGPSPoint]: + return [ + _camm_point(time=start + t, epoch_time=start + t) for t in range(duration + 1) + ] + + +def _write_gpx(path: Path, points: T.Sequence[geo.Point]) -> None: + trkpts = "".join( + f'' + for p in points + ) + path.write_text( + '' + '' + f"{trkpts}" + ) + + +def _isoformat(unix_time: float) -> str: + return datetime.datetime.fromtimestamp(unix_time, datetime.timezone.utc).strftime( + "%Y-%m-%dT%H:%M:%SZ" + ) + + +class TestGPXOverlap: + """ + A GPX track that does not overlap the video must fail, not sync. + + The edit list no longer overflows on a huge offset, so this is what keeps + an epoch mix-up or a GPX file from another day from being uploaded. + """ + + # A 10s video track starting at A_UNIX_TIME + VIDEO = [_camm_point(time=float(t), epoch_time=A_UNIX_TIME + t) for t in range(11)] + + def _check(self, gpx_points: list[telemetry.CAMMGPSPoint]) -> None: + offset = GPXVideoExtractor._gpx_offset(gpx_points, self.VIDEO) + GPXVideoExtractor._check_overlap(gpx_points, self.VIDEO, offset) + + @pytest.mark.parametrize( + "start, duration", + [ + # Starts 30s before the video and ends during it + (A_UNIX_TIME - 30, 35), + # Starts during the video + (A_UNIX_TIME + 5, 60), + # A long recording that spans the video + (A_UNIX_TIME - 3 * 24 * 3600, 6 * 24 * 3600), + ], + ) + def test_overlapping_gpx_passes(self, start: float, duration: int): + self._check(_gpx_track(start, duration)) + + @pytest.mark.parametrize( + "start", + [ + # Ended a minute before the video started + A_UNIX_TIME - 70, + # The next day + A_UNIX_TIME + 24 * 3600, + ], + ) + def test_disjoint_gpx_raises(self, start: float): + with pytest.raises(exceptions.MapillaryOutsideGPXTrackError): + self._check(_gpx_track(start, 10)) + + def test_epoch_mixup_raises(self): + """Video timestamps left in GPS time sync ten years off.""" + video = [ + _camm_point(time=float(t), epoch_time=A_GPS_TIME + t) for t in range(11) + ] + gpx_points = _gpx_track(A_UNIX_TIME, 10) + offset = GPXVideoExtractor._gpx_offset(gpx_points, video) + assert abs(offset - (GPS_UNIX_DELTA - 18)) < 1 + with pytest.raises(exceptions.MapillaryOutsideGPXTrackError): + GPXVideoExtractor._check_overlap(gpx_points, video, offset) + + def test_error_survives_a_worker_process(self): + """Videos are geotagged in a process pool, so the error gets pickled.""" + with pytest.raises(exceptions.MapillaryOutsideGPXTrackError) as info: + self._check(_gpx_track(A_UNIX_TIME + 24 * 3600, 10)) + + unpickled = pickle.loads(pickle.dumps(info.value)) + + assert str(unpickled) == str(info.value) + assert vars(unpickled) == vars(info.value) + + def test_extract_syncs_labpano_video_to_its_gpx(self, tmp_path: Path): + video_path = tmp_path / "labpano.mp4" + video_path.write_bytes( + _write_camm_mp4(self.VIDEO, "Labpano", A_UNIX_TIME + 600) + ) + gpx_path = tmp_path / "labpano.gpx" + _write_gpx(gpx_path, _gpx_track(int(A_UNIX_TIME) - 5, 20)) + + points = GPXVideoExtractor(video_path, gpx_path).extract().points + + # The GPX starts ~5s before the video, whose GPS starts at video time 0 + assert -6 < points[0].time < -4 + + def test_extract_rejects_gpx_from_another_day(self, tmp_path: Path): + video_path = tmp_path / "labpano.mp4" + video_path.write_bytes( + _write_camm_mp4(self.VIDEO, "Labpano", A_UNIX_TIME + 600) + ) + gpx_path = tmp_path / "yesterday.gpx" + _write_gpx(gpx_path, _gpx_track(A_UNIX_TIME - 24 * 3600, 20)) + + with pytest.raises(exceptions.MapillaryOutsideGPXTrackError): + GPXVideoExtractor(video_path, gpx_path).extract() + + +def _camm_exiftool_xml(make: str, meta_format: str = "camm") -> ET.ElementTree: + """ExifTool XML for a CAMM track, trimmed from a PanoX V2 capture.""" + xml = f""" + + + 2024:01:18 10:43:57 + {meta_format} + 0 + 0 + 2024:01:18 10:41:01.600768Z + 3 + 47.36061891 + 8.52077651 + 448.905395507812 + {make} + PanoX V2 + + +""" + root = ET.fromstring(xml) + desc = root.find("{http://www.w3.org/1999/02/22-rdf-syntax-ns#}Description") + assert desc is not None + return ET.ElementTree(desc) + + +class TestExifToolAgreesWithNativeParser: + """ + ExifTool converts CAMM GPS time by the epoch difference alone, so it + reads 18s (the leap seconds since 1980) later than the native parser. + The native reading is the right one: the PanoX V2 capture this is taken + from lasts 193.4s and its mvhd creation time, stamped when recording ends, + is 10:43:57 -- 10:40:43.6 plus 193.4s, where 10:41:01.6 would overshoot it. + """ + + # 2024-01-18T10:41:01.600768Z, as ExifTool renders it + EXIFTOOL_TIME = 1705574461.600768 + NATIVE_TIME = EXIFTOOL_TIME - 18 + + @pytest.mark.parametrize("make", ["Labpano", "Labpano Technology Co.,Ltd"]) + def test_gps_time_camm_track_is_leap_corrected(self, make: str): + track = ExifToolReadVideo(_camm_exiftool_xml(make)).extract_gps_track() + epoch_time = T.cast(telemetry.GPSPoint, track[0]).epoch_time + assert epoch_time == pytest.approx(self.NATIVE_TIME, abs=1e-3) + + @pytest.mark.parametrize( + "make, meta_format", [("Insta360", "camm"), ("Labpano", "gpmd")] + ) + def test_other_tracks_are_left_alone(self, make: str, meta_format: str): + xml = _camm_exiftool_xml(make, meta_format=meta_format) + track = ExifToolReadVideo(xml).extract_gps_track() + epoch_time = T.cast(telemetry.GPSPoint, track[0]).epoch_time + assert epoch_time == pytest.approx(self.EXIFTOOL_TIME, abs=1e-3) + + class TestEditListOverflow: """An oversized initial gap must not abort the upload.""" From 384f40684795ad114b071c0034ffd8e0fc8709ba Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Wed, 23 Sep 2026 15:01:20 +0200 Subject: [PATCH 2/4] Write CAMM GPS timestamps as Unix time, and loosen the GPX time check Addresses review of the CAMM epoch follow-ups: - Write Unix time whatever the make, as main has since #828, instead of converting back to GPS time for Labpano. This is not what released versions wrote: v0.14.7 passed a Labpano camera's GPS time through unconverted, 1389017092.966 for the first sample of a PanoX V2 capture from 2024-01-11, where this writes the Unix time 1704981874.966, one GPS epoch less 18 leap seconds later. Our output copies the source creation time, which the reader already uses to tell Unix time from GPS time. Removes denormalize_gps_epochs and unix_to_gps_epoch. - Decide the epoch of a CAMM track by the median of its timestamps, so that one stray sample cannot flip the whole track. - Sync GPX to video time 0 rather than to the first video GPS point, which put the track early by that point's time when the video's GPS started late. - Replace the overlap check with a gap check: silent on overlap, a warning naming both files for a gap up to a day, and an error only beyond that, where no clock or time zone mistake can explain it. The error is now a geotagging error, so the next geotag source gets its turn. - ExifTool leap correction: match the CAMM format case-insensitively, log when it applies, and test it against real ExifTool output of a file we write. - Document the cost of mixing CAMM type 5 and type 6 samples. --- mapillary_tools/camm/camm_builder.py | 8 +- mapillary_tools/camm/camm_parser.py | 57 ++-- mapillary_tools/exceptions.py | 4 +- mapillary_tools/exiftool_read_video.py | 16 +- .../geotag/video_extractors/gpx.py | 56 ++-- .../geotag/video_extractors/native.py | 6 +- mapillary_tools/telemetry.py | 24 +- tests/unit/test_camm_parser.py | 6 +- tests/unit/test_gps_epoch.py | 252 +++++++++++++----- 9 files changed, 275 insertions(+), 154 deletions(-) diff --git a/mapillary_tools/camm/camm_builder.py b/mapillary_tools/camm/camm_builder.py index 12b1574d..d8f6a6b6 100644 --- a/mapillary_tools/camm/camm_builder.py +++ b/mapillary_tools/camm/camm_builder.py @@ -255,13 +255,9 @@ def _f( creation_time = mvhd_data.get("creation_time", 0) modification_time = mvhd_data.get("modification_time", 0) - # Write GPS timestamps in the epoch the make records, so that the - # output parses back to the same Unix times - gps = camm_parser.denormalize_gps_epochs(camm_info.gps or [], camm_info.make) - # Multiplex points for creating elst track: list[geo.Point] = [ - *gps, + *(camm_info.gps or []), *(camm_info.mini_gps or []), ] track.sort(key=lambda p: p.time) @@ -271,7 +267,7 @@ def _f( # Multiplex telemetry measurements measurements: list[camm_parser.TelemetryMeasurement] = [ - *gps, + *(camm_info.gps or []), *(camm_info.mini_gps or []), *(camm_info.accl or []), *(camm_info.gyro or []), diff --git a/mapillary_tools/camm/camm_parser.py b/mapillary_tools/camm/camm_parser.py index c8cd5016..15600638 100644 --- a/mapillary_tools/camm/camm_parser.py +++ b/mapillary_tools/camm/camm_parser.py @@ -10,6 +10,7 @@ import dataclasses import io import logging +import statistics import typing as T from enum import Enum @@ -164,10 +165,11 @@ def make_records_gps_time(make: str) -> bool: def _records_gps_time( - first_epoch_time: float, make: str, creation_time: float | None + epoch_time: float, make: str, creation_time: float | None ) -> bool: """ - Decide the epoch of raw CAMM type 6 timestamps. + Decide the epoch of raw CAMM type 6 timestamps, given one representative + of them. The creation time of the video decides when it can: GPS time reads as one GPS epoch before it, Unix time reads close to it. The make decides only @@ -185,7 +187,7 @@ def _records_gps_time( False """ if creation_time: - gap = creation_time - first_epoch_time + gap = creation_time - epoch_time if abs(gap - telemetry.GPS_EPOCH_UNIX_OFFSET) < _CREATION_TIME_TOLERANCE: return True if abs(gap) < _CREATION_TIME_TOLERANCE: @@ -217,15 +219,17 @@ def _normalize_gps_epochs( Rewrite CAMMGPSPoint.epoch_time in place so it is Unix time regardless of which epoch the producer used. - This is the only place CAMM GPS timestamps are read into Unix time. - Everything downstream treats them as Unix time, until - denormalize_gps_epochs() converts them back for writing. + This is the only place CAMM GPS timestamps change epoch. Everything + downstream, including the serializer, treats them as Unix time. """ - first = next((p.epoch_time for p in gps if p.epoch_time > 0), None) - if first is None: + epoch_times = [p.epoch_time for p in gps if p.epoch_time > 0] + if not epoch_times: return - if not _records_gps_time(first, make, _extract_creation_time(moov)): + # Decide by the median rather than by any one sample, so that a stray + # timestamp cannot flip the epoch of the whole track + median_epoch_time = statistics.median(epoch_times) + if not _records_gps_time(median_epoch_time, make, _extract_creation_time(moov)): return for point in gps: @@ -233,35 +237,6 @@ def _normalize_gps_epochs( point.epoch_time = telemetry.gps_epoch_to_unix(point.epoch_time) -def denormalize_gps_epochs( - gps: T.Sequence[telemetry.CAMMGPSPoint], make: str -) -> list[telemetry.CAMMGPSPoint]: - """ - Return copies of the points with epoch_time converted to the epoch this - make records, for writing. The inverse of _normalize_gps_epochs(). - - The source make is copied into every CAMM track mapillary_tools writes, so - a Labpano video written with Unix time in its GPS track would read back - ten years in the future. Writing GPS time for these makes keeps our output - parseable, and leaves the camera's own timestamps as the camera wrote them. - - Only the make is available here, and that is enough: the reader decides by - the creation time first, which is copied from the source, and by the make - otherwise, so either way it reads back what was written. - """ - if not make_records_gps_time(make): - return list(gps) - - return [ - dataclasses.replace( - point, epoch_time=telemetry.unix_to_gps_epoch(point.epoch_time) - ) - if point.epoch_time > 0 - else point - for point in gps - ] - - def extract_camera_make_and_model(fp: T.BinaryIO) -> tuple[str, str]: moov = MovieBoxParser.parse_stream(fp) udta_boxdata = moov.extract_udta_boxdata() @@ -381,8 +356,10 @@ def serialize(cls, data: telemetry.CAMMGPSPoint) -> bytes: { "type": cls.serialized_camm_type.value, "data": { - # Written as is. camm_builder converts to the epoch the - # make records beforehand (see denormalize_gps_epochs). + # Written as Unix time, whatever the make. Readers tell it + # from the GPS time some cameras record by the creation + # time the file carries (see _records_gps_time). Do not + # convert here. "time_gps_epoch": data.epoch_time, "gps_fix_type": data.gps_fix_type, "latitude": data.lat, diff --git a/mapillary_tools/exceptions.py b/mapillary_tools/exceptions.py index 60777edf..af1f34cb 100644 --- a/mapillary_tools/exceptions.py +++ b/mapillary_tools/exceptions.py @@ -72,7 +72,9 @@ class MapillaryStationaryVideoError(MapillaryDescriptionError): pass -class MapillaryOutsideGPXTrackError(MapillaryDescriptionError): +# A geotagging error, so that the next geotag source gets its turn: a GPX track +# that misses the capture time says nothing about the file itself +class MapillaryOutsideGPXTrackError(MapillaryGeoTaggingError): def __init__( self, message: str, image_time: str, gpx_start_time: str, gpx_end_time: str ): diff --git a/mapillary_tools/exiftool_read_video.py b/mapillary_tools/exiftool_read_video.py index f08958ed..a9834c11 100644 --- a/mapillary_tools/exiftool_read_video.py +++ b/mapillary_tools/exiftool_read_video.py @@ -563,6 +563,9 @@ def _extract_gps_track_from_track(self) -> list[GPSPoint]: ) if track: if self._is_camm_track_in_gps_time(track_ns): + LOG.debug( + f"Correcting the CAMM GPS timestamps in {track_ns} by the leap seconds" + ) for point in track: if point.epoch_time is not None: point.epoch_time = _exiftool_gps_time_to_unix( @@ -578,12 +581,21 @@ def _is_camm_track_in_gps_time(self, track_ns: str) -> bool: ExifTool converts those timestamps by the epoch difference alone, so they read 18s (the leap seconds since 1980) ahead of what the native CAMM parser returns for the same video. + + Only camera originals qualify. Cameras put their CAMM track under a + meta handler, which ExifTool reports as MetaFormat. The CAMM tracks + mapillary_tools writes use a camm handler, which ExifTool reports as + OtherFormat, and hold Unix time, which ExifTool reads as is. """ meta_format = self._extract_alternative_fields([f"{track_ns}:MetaFormat"], str) - if meta_format != "camm": + if (meta_format or "").strip().lower() != "camm": return False + make = self.extract_make() - return make is not None and camm_parser.make_records_gps_time(make) + if not make: + return False + + return camm_parser.make_records_gps_time(make) def _extract_alternative_fields( self, diff --git a/mapillary_tools/geotag/video_extractors/gpx.py b/mapillary_tools/geotag/video_extractors/gpx.py index 4e8f207c..47a2a555 100644 --- a/mapillary_tools/geotag/video_extractors/gpx.py +++ b/mapillary_tools/geotag/video_extractors/gpx.py @@ -27,6 +27,11 @@ LOG = logging.getLogger(__name__) +# A GPX track that misses the video by more than this cannot belong to it. No +# camera clock or time zone mistake comes close, while an epoch mix-up exceeds +# it by orders of magnitude. +_IMPLAUSIBLE_GAP_SECONDS = 24 * 3600 + class SyncMode(enum.Enum): # Sync by video GPS timestamps if found, otherwise rebase @@ -76,44 +81,55 @@ def extract(self) -> types.VideoMetadata: else: offset = self._gpx_offset(gpx_points, native_video_metadata.points) if offset: - self._check_overlap(gpx_points, native_video_metadata.points, offset) + self._check_time_gap(gpx_points, native_video_metadata.points, offset) self._rebase_times(gpx_points, offset=offset) return dataclasses.replace(native_video_metadata, points=gpx_points) - @classmethod - def _check_overlap( - cls, + def _check_time_gap( + self, gpx_points: T.Sequence[geo.Point], video_gps_points: T.Sequence[geo.Point], offset: float, ) -> None: """ - Raise if the GPX track, once synced by offset, does not overlap the - video in time. - - Nothing downstream can use such a sync: every point would fall outside - the video, and at upload the edit list would carry the whole offset. - That used to overflow and abort the upload. Now that the edit list - widens instead, this is what keeps an epoch mix-up or a GPX file from - another day from failing silently. + Check the GPX track, once synced by offset, against the video in time. + + Silent when the two overlap. A gap only warns, because it can be + legitimate, as when the GPX starts after the video's own GPS gives out. + A gap too large for any clock or time zone mistake to explain raises: + the GPX cannot belong to the video, and syncing to it would put every + point far outside the video. """ # The Unix time of video time 0, in the convention _rebase_times() uses video_start_time = gpx_points[0].time - offset - video_first = video_start_time + min(p.time for p in video_gps_points) + # From video time 0, since the frames start there even when the video's + # own GPS starts later video_last = video_start_time + max(p.time for p in video_gps_points) gpx_first = min(p.time for p in gpx_points) gpx_last = max(p.time for p in gpx_points) - if gpx_last < video_first or video_last < gpx_first: + gap = max(gpx_first - video_last, video_start_time - gpx_last) + if gap <= 0: + return + + message = ( + f"The GPX track in {self.gpx_path} ({_isoformat(gpx_first)} to {_isoformat(gpx_last)}) " + f"misses the video {self.video_path} ({_isoformat(video_start_time)} to {_isoformat(video_last)}) " + f"by {gap:.0f} seconds ({gap / 86400:.1f} days). " + "Check the camera clock, and the time zone of the GPX timestamps" + ) + + if gap > _IMPLAUSIBLE_GAP_SECONDS: raise exceptions.MapillaryOutsideGPXTrackError( - f"The video GPS track ({_isoformat(video_first)} to {_isoformat(video_last)}) " - f"does not overlap the GPX track ({_isoformat(gpx_first)} to {_isoformat(gpx_last)}) in time", - image_time=build_capture_time(video_first), + message, + image_time=build_capture_time(video_start_time), gpx_start_time=build_capture_time(gpx_first), gpx_end_time=build_capture_time(gpx_last), ) + LOG.warning(message) + @classmethod def _rebase_times(cls, points: T.Sequence[geo.Point], offset: float = 0.0) -> None: """ @@ -153,10 +169,8 @@ def _gpx_offset( if anchor is not None: anchor_unix_time = T.cast(float, anchor.get_unix_time()) - # The Unix time the first video point would have - video_unix_time = anchor_unix_time - ( - anchor.time - video_gps_points[0].time - ) + # The Unix time of video time 0 + video_unix_time = anchor_unix_time - anchor.time offset = gpx_points[0].time - video_unix_time return offset diff --git a/mapillary_tools/geotag/video_extractors/native.py b/mapillary_tools/geotag/video_extractors/native.py index 301bd1a6..f4a60af3 100644 --- a/mapillary_tools/geotag/video_extractors/native.py +++ b/mapillary_tools/geotag/video_extractors/native.py @@ -69,7 +69,11 @@ def extract(self) -> types.VideoMetadata: if not camm_info.gps and not camm_info.mini_gps: raise exceptions.MapillaryGPXEmptyError("Empty GPS data found") - # A track may mix type 6 and type 5 samples, so use both + # A track may mix type 6 and type 5 samples, so use both. No camera is + # known to interleave them, and it would cost the absolute timestamps + # of sampled frames: interpolating between the two types returns a + # plain geo.Point (see CAMMGPSPoint.interpolate_with), so sample_video + # falls back to the container start time. points: list[geo.Point] = [*(camm_info.gps or []), *(camm_info.mini_gps or [])] points.sort(key=lambda p: p.time) diff --git a/mapillary_tools/telemetry.py b/mapillary_tools/telemetry.py index 7521ea14..d6084a05 100644 --- a/mapillary_tools/telemetry.py +++ b/mapillary_tools/telemetry.py @@ -64,7 +64,7 @@ def gps_epoch_to_unix(gps_epoch_time: float) -> float: """ Convert seconds since the GPS epoch (GPS time) to Unix time (UTC). - Only called at the CAMM parse boundary, for tracks that record GPS time. + Only called at the parse boundary, for producers known to record GPS time. >>> gps_epoch_to_unix(1470558405.9798455) 1786523187.9798455 @@ -75,19 +75,6 @@ def gps_epoch_to_unix(gps_epoch_time: float) -> float: return approx_unix_time - _gps_utc_offset_at(approx_unix_time) -def unix_to_gps_epoch(unix_time: float) -> float: - """ - Convert Unix time (UTC) to seconds since the GPS epoch (GPS time). - - The inverse of gps_epoch_to_unix(). Only called when writing a CAMM track - for a camera that records GPS time. - - >>> unix_to_gps_epoch(1786523187.9798455) - 1470558405.9798455 - """ - return unix_time - GPS_EPOCH_UNIX_OFFSET + _gps_utc_offset_at(unix_time) - - @unique class GPSFix(Enum): NO_FIX = 0 @@ -173,11 +160,10 @@ class CAMMGPSPoint(TimestampedMeasurement, Point): # # The corresponding CAMM box field is named time_gps_epoch, but what # producers actually store there varies: Labpano cameras record GPS time, - # while Insta360 records Unix time. Whatever the producer wrote is - # normalized to Unix time once, when the CAMM track is parsed, and - # converted back only when mapillary_tools writes a CAMM track for a make - # that records GPS time (see camm_parser.denormalize_gps_epochs), so that - # everything in between can rely on a single meaning. + # while Insta360 and mapillary_tools itself record Unix time. Whatever the + # producer wrote is normalized to Unix time once, when the CAMM track is + # parsed (see camm_parser.extract_camm_info), so that everything + # downstream can rely on a single meaning. epoch_time: float gps_fix_type: int horizontal_accuracy: float diff --git a/tests/unit/test_camm_parser.py b/tests/unit/test_camm_parser.py index 37c54686..b687c349 100644 --- a/tests/unit/test_camm_parser.py +++ b/tests/unit/test_camm_parser.py @@ -802,9 +802,9 @@ def test_extract_camm_info_routes_plain_points_to_mini_gps(): def test_camm_gps_timestamps_round_trip_as_unix(): """process -> build CAMM -> re-read must return the input timestamps. - Without a make that records GPS time, mapillary_tools writes Unix time - into the CAMM type 6 time_gps_epoch field, as every released version has. - Makes that record GPS time are covered in test_gps_epoch.py. + mapillary_tools writes Unix time into the CAMM type 6 time_gps_epoch + field whatever the make, as every released version has. Makes that record + GPS time are covered in test_gps_epoch.py. """ unix_times = [1655503450.5, 1655503451.5] points = [ diff --git a/tests/unit/test_gps_epoch.py b/tests/unit/test_gps_epoch.py index 6984fdb4..af5f5616 100644 --- a/tests/unit/test_gps_epoch.py +++ b/tests/unit/test_gps_epoch.py @@ -4,7 +4,7 @@ # LICENSE file in the root directory of this source tree. """ -Regression tests for the epoch of CAMM GPS timestamps. +Tests for the epoch of CAMM GPS timestamps. The CAMM box field is called ``time_gps_epoch``, but producers disagree about what goes in it: Labpano cameras write GPS time (seconds since 1980-01-06), @@ -13,27 +13,33 @@ The invariant these tests protect: ``CAMMGPSPoint.epoch_time`` is *always* Unix time in memory. It is converted from GPS time once, when the CAMM track is -parsed, and back once, when mapillary_tools writes a CAMM track for a make that -records GPS time -- so that whatever it writes reads back to the same instants. +parsed, and never on the way out: mapillary_tools writes Unix time whatever the +make. Which epoch a track records is decided by the mvhd creation time of the video -when that is conclusive, and by the make otherwise. +when that is conclusive, and by the make otherwise. The CAMM tracks +mapillary_tools writes carry the make and the creation time of their source, +so for a Labpano video it is the creation time that says Unix time. """ from __future__ import annotations import datetime import io +import logging import pickle +import shutil import typing as T import xml.etree.ElementTree as ET from pathlib import Path import pytest -from mapillary_tools import exceptions, geo, telemetry, types, uploader +from mapillary_tools import exceptions, exiftool_read, geo, telemetry, types, uploader from mapillary_tools.camm import camm_builder, camm_parser from mapillary_tools.exiftool_read_video import ExifToolReadVideo -from mapillary_tools.geotag.options import SourceOption, SourceType +from mapillary_tools.exiftool_runner import ExiftoolRunner +from mapillary_tools.geotag import factory +from mapillary_tools.geotag.options import SourceOption, SourcePathOption, SourceType from mapillary_tools.geotag.video_extractors.gpx import GPXVideoExtractor from mapillary_tools.geotag.video_extractors.native import CAMMVideoExtractor from mapillary_tools.mp4 import construct_mp4_parser as cparser, simple_mp4_builder @@ -155,13 +161,6 @@ def test_leap_seconds_accumulate(self): telemetry.gps_epoch_to_unix(A_GPS_TIME) == A_GPS_TIME + GPS_UNIX_DELTA - 18 ) - def test_unix_to_gps_epoch_is_the_inverse(self): - assert telemetry.unix_to_gps_epoch(A_UNIX_TIME) == A_GPS_TIME - # One instant per leap second era, from before the first leap second - for unix_time in [400000000.5, 1000000000.5, 1500000000.5, A_UNIX_TIME]: - gps_time = telemetry.unix_to_gps_epoch(unix_time) - assert telemetry.gps_epoch_to_unix(gps_time) == unix_time - class TestParseBoundaryNormalization: """The one place an epoch conversion is allowed to happen.""" @@ -232,20 +231,51 @@ def test_unknown_make_in_gps_time_is_read_from_the_file(self): assert _unix_times(_read_camm(data)) == [A_UNIX_TIME] +class TestOneSampleDoesNotDecide: + """The whole track decides its epoch, not its first timestamp.""" + + CREATION_TIME = A_UNIX_TIME + 600 + + def _read_back(self, raw_times: list[float]) -> list[float]: + points = [ + _camm_point(time=float(idx), epoch_time=raw_time) + for idx, raw_time in enumerate(raw_times) + ] + # Written as is, so the raw times are what the reader sees + data = _write_camm_mp4(points, "Labpano", self.CREATION_TIME) + return _unix_times(_read_camm(data)) + + def test_stray_unix_time_does_not_flip_a_gps_time_track(self): + raw_times = [A_UNIX_TIME] + [A_GPS_TIME + t for t in range(1, 10)] + unix_times = self._read_back(raw_times) + assert unix_times[1:] == [A_UNIX_TIME + t for t in range(1, 10)] + + def test_stray_gps_time_does_not_flip_a_unix_time_track(self): + raw_times = [A_GPS_TIME] + [A_UNIX_TIME + t for t in range(1, 10)] + unix_times = self._read_back(raw_times) + assert unix_times[1:] == [A_UNIX_TIME + t for t in range(1, 10)] + + class TestWriteRoundTrip: """ Whatever mapillary_tools writes must read back to the same Unix times. - The source make is copied into the output, so writing Unix time for a - Labpano video used to read back one GPS epoch later: 2026 became 2036. + It writes Unix time whatever the make. The source make is copied into the + output, so for a Labpano video it is the creation time, copied from the + source too, that keeps the reader from converting it one GPS epoch later: + 2026 used to become 2036. """ - @pytest.mark.parametrize("creation_time", [A_UNIX_TIME + 600, None]) @pytest.mark.parametrize( "make", ["Labpano", "Labpano Technology Co.,Ltd", "Insta360", ""] ) - def test_round_trip(self, make: str, creation_time: float | None): - data = _write_camm_mp4(_unix_track(), make, creation_time) + def test_round_trip(self, make: str): + data = _write_camm_mp4(_unix_track(), make, A_UNIX_TIME + 600) + assert _unix_times(_read_camm(data)) == UNIX_TIMES + + @pytest.mark.parametrize("make", ["Insta360", ""]) + def test_round_trip_without_creation_time(self, make: str): + data = _write_camm_mp4(_unix_track(), make, None) assert _unix_times(_read_camm(data)) == UNIX_TIMES def test_reprocessing_output_is_stable(self): @@ -258,20 +288,11 @@ def test_reprocessing_output_is_stable(self): camm_info.gps or [], camm_info.make, A_UNIX_TIME + 600 ) - @pytest.mark.parametrize( - "make, raw_times", - [ - ("Labpano", [A_GPS_TIME, A_GPS_TIME + 1]), - ("Insta360", UNIX_TIMES), - ], - ) - def test_written_in_the_epoch_the_make_records( - self, monkeypatch, make: str, raw_times: list[float] - ): - """Labpano output carries GPS time, just like the camera's own file.""" + @pytest.mark.parametrize("make", ["Labpano", "Insta360", ""]) + def test_written_as_unix_time_whatever_the_make(self, monkeypatch, make: str): data = _write_camm_mp4(_unix_track(), make, A_UNIX_TIME + 600) monkeypatch.setattr(camm_parser, "_normalize_gps_epochs", lambda *_: None) - assert _unix_times(_read_camm(data)) == raw_times + assert _unix_times(_read_camm(data)) == UNIX_TIMES class TestMixedCAMMTypes: @@ -346,6 +367,24 @@ def test_missing_video_timestamp_yields_no_offset(self): video_points = [_camm_point(time=0.0, epoch_time=0.0)] assert GPXVideoExtractor._gpx_offset(gpx_points, video_points) == 0.0 + def test_video_gps_starting_late_syncs_to_video_time(self): + """ + The offset is to video time 0, not to the first video GPS point, or + the GPX track would land early by that point's time. + """ + gpx_points = _gpx_track(A_UNIX_TIME, 20) + # The video's GPS starts 5s into the video + video_points = [ + _camm_point(time=5.0 + t, epoch_time=A_UNIX_TIME + 5 + t) for t in range(10) + ] + + offset = GPXVideoExtractor._gpx_offset(gpx_points, video_points) + GPXVideoExtractor._rebase_times(gpx_points, offset=offset) + + # Recorded at the same instant as the first video GPS point, so it + # lands at the same video time + assert gpx_points[5].time == 5.0 + def _gpx_track(start: float, duration: int) -> list[telemetry.CAMMGPSPoint]: return [ @@ -371,20 +410,26 @@ def _isoformat(unix_time: float) -> str: ) -class TestGPXOverlap: +class TestGPXTimeGap: """ - A GPX track that does not overlap the video must fail, not sync. + A GPX track that misses the video by more than a day must fail, not sync. The edit list no longer overflows on a huge offset, so this is what keeps - an epoch mix-up or a GPX file from another day from being uploaded. + an epoch mix-up or a GPX file from another day from being uploaded. A + smaller gap only warns, since it can be legitimate. """ # A 10s video track starting at A_UNIX_TIME VIDEO = [_camm_point(time=float(t), epoch_time=A_UNIX_TIME + t) for t in range(11)] - def _check(self, gpx_points: list[telemetry.CAMMGPSPoint]) -> None: - offset = GPXVideoExtractor._gpx_offset(gpx_points, self.VIDEO) - GPXVideoExtractor._check_overlap(gpx_points, self.VIDEO, offset) + def _check( + self, + gpx_points: list[telemetry.CAMMGPSPoint], + video: T.Sequence[geo.Point] = VIDEO, + ) -> None: + extractor = GPXVideoExtractor(Path("video.mp4"), Path("track.gpx")) + offset = extractor._gpx_offset(gpx_points, video) + extractor._check_time_gap(gpx_points, video, offset) @pytest.mark.parametrize( "start, duration", @@ -397,21 +442,38 @@ def _check(self, gpx_points: list[telemetry.CAMMGPSPoint]) -> None: (A_UNIX_TIME - 3 * 24 * 3600, 6 * 24 * 3600), ], ) - def test_overlapping_gpx_passes(self, start: float, duration: int): - self._check(_gpx_track(start, duration)) + def test_overlapping_gpx_passes_silently(self, caplog, start: float, duration: int): + with caplog.at_level(logging.WARNING): + self._check(_gpx_track(start, duration)) + assert not caplog.records @pytest.mark.parametrize( "start", [ # Ended a minute before the video started A_UNIX_TIME - 70, - # The next day - A_UNIX_TIME + 24 * 3600, + # Naive GPX timestamps read in a time zone 9h off + A_UNIX_TIME + 9 * 3600, + # Starts after the video's own GPS gives out + A_UNIX_TIME + 60, ], ) - def test_disjoint_gpx_raises(self, start: float): - with pytest.raises(exceptions.MapillaryOutsideGPXTrackError): + def test_small_gap_warns(self, caplog, start: float): + with caplog.at_level(logging.WARNING): + self._check(_gpx_track(start, 10)) + [record] = caplog.records + assert "video.mp4" in record.getMessage() + assert "track.gpx" in record.getMessage() + + @pytest.mark.parametrize( + "start", + [A_UNIX_TIME + 2 * 24 * 3600, A_UNIX_TIME - 3 * 24 * 3600], + ) + def test_gap_of_days_raises(self, start: float): + with pytest.raises(exceptions.MapillaryOutsideGPXTrackError) as info: self._check(_gpx_track(start, 10)) + assert "video.mp4" in str(info.value) + assert "track.gpx" in str(info.value) def test_epoch_mixup_raises(self): """Video timestamps left in GPS time sync ten years off.""" @@ -422,12 +484,12 @@ def test_epoch_mixup_raises(self): offset = GPXVideoExtractor._gpx_offset(gpx_points, video) assert abs(offset - (GPS_UNIX_DELTA - 18)) < 1 with pytest.raises(exceptions.MapillaryOutsideGPXTrackError): - GPXVideoExtractor._check_overlap(gpx_points, video, offset) + self._check(gpx_points, video) def test_error_survives_a_worker_process(self): """Videos are geotagged in a process pool, so the error gets pickled.""" with pytest.raises(exceptions.MapillaryOutsideGPXTrackError) as info: - self._check(_gpx_track(A_UNIX_TIME + 24 * 3600, 10)) + self._check(_gpx_track(A_UNIX_TIME + 2 * 24 * 3600, 10)) unpickled = pickle.loads(pickle.dumps(info.value)) @@ -447,20 +509,54 @@ def test_extract_syncs_labpano_video_to_its_gpx(self, tmp_path: Path): # The GPX starts ~5s before the video, whose GPS starts at video time 0 assert -6 < points[0].time < -4 - def test_extract_rejects_gpx_from_another_day(self, tmp_path: Path): + def _write_video_and_gpx( + self, tmp_path: Path, gpx_start: float + ) -> tuple[Path, Path]: video_path = tmp_path / "labpano.mp4" video_path.write_bytes( _write_camm_mp4(self.VIDEO, "Labpano", A_UNIX_TIME + 600) ) - gpx_path = tmp_path / "yesterday.gpx" - _write_gpx(gpx_path, _gpx_track(A_UNIX_TIME - 24 * 3600, 20)) + gpx_path = tmp_path / "other.gpx" + _write_gpx(gpx_path, _gpx_track(gpx_start, 20)) + return video_path, gpx_path + def test_extract_rejects_gpx_from_days_before(self, tmp_path: Path): + video_path, gpx_path = self._write_video_and_gpx( + tmp_path, A_UNIX_TIME - 3 * 24 * 3600 + ) with pytest.raises(exceptions.MapillaryOutsideGPXTrackError): GPXVideoExtractor(video_path, gpx_path).extract() + def test_next_source_gets_its_turn(self, tmp_path: Path): + """The GPX misses the video, which says nothing about the video itself.""" + video_path, gpx_path = self._write_video_and_gpx( + tmp_path, A_UNIX_TIME - 3 * 24 * 3600 + ) + options = [ + SourceOption( + SourceType.GPX, + num_processes=0, + source_path=SourcePathOption(source_path=gpx_path), + ), + SourceOption(SourceType.NATIVE, num_processes=0), + ] + + [metadata] = factory.process([video_path], options) + + assert isinstance(metadata, types.VideoMetadata) + assert [p.time for p in metadata.points] == [p.time for p in self.VIDEO] + -def _camm_exiftool_xml(make: str, meta_format: str = "camm") -> ET.ElementTree: - """ExifTool XML for a CAMM track, trimmed from a PanoX V2 capture.""" +def _camm_exiftool_xml( + make: str, meta_format: str = "camm", format_tag: str = "MetaFormat" +) -> ET.ElementTree: + """ + ExifTool XML for a CAMM track, trimmed from a PanoX V2 capture. + + ExifTool reports the format of a camera's CAMM track, under a meta + handler, as MetaFormat, and that of the CAMM tracks mapillary_tools + writes, under a camm handler, as OtherFormat. + """ xml = f""" ET.ElementTree: xmlns:Track1='http://ns.exiftool.org/QuickTime/Track1/1.0/' xmlns:UserData='http://ns.exiftool.org/QuickTime/UserData/1.0/'> 2024:01:18 10:43:57 - {meta_format} + {meta_format} 0 0 2024:01:18 10:41:01.600768Z @@ -491,30 +587,64 @@ class TestExifToolAgreesWithNativeParser: """ ExifTool converts CAMM GPS time by the epoch difference alone, so it reads 18s (the leap seconds since 1980) later than the native parser. - The native reading is the right one: the PanoX V2 capture this is taken - from lasts 193.4s and its mvhd creation time, stamped when recording ends, - is 10:43:57 -- 10:40:43.6 plus 193.4s, where 10:41:01.6 would overshoot it. + The native reading is the right one: in the PanoX V2 capture this is taken + from, the last GPS sample reads 0.4s before the mvhd creation time, stamped + when the file is finalized, where ExifTool's reading of it would be 17.6s + after. """ # 2024-01-18T10:41:01.600768Z, as ExifTool renders it EXIFTOOL_TIME = 1705574461.600768 NATIVE_TIME = EXIFTOOL_TIME - 18 - @pytest.mark.parametrize("make", ["Labpano", "Labpano Technology Co.,Ltd"]) - def test_gps_time_camm_track_is_leap_corrected(self, make: str): - track = ExifToolReadVideo(_camm_exiftool_xml(make)).extract_gps_track() - epoch_time = T.cast(telemetry.GPSPoint, track[0]).epoch_time + def _first_epoch_time(self, xml: ET.ElementTree) -> float | None: + track = ExifToolReadVideo(xml).extract_gps_track() + return T.cast(telemetry.GPSPoint, track[0]).epoch_time + + @pytest.mark.parametrize( + "make, meta_format", + [ + ("Labpano", "camm"), + ("Labpano Technology Co.,Ltd", "camm"), + ("Labpano", "CAMM"), + ("Labpano", " camm "), + ], + ) + def test_gps_time_camm_track_is_leap_corrected(self, make: str, meta_format: str): + epoch_time = self._first_epoch_time(_camm_exiftool_xml(make, meta_format)) assert epoch_time == pytest.approx(self.NATIVE_TIME, abs=1e-3) @pytest.mark.parametrize( - "make, meta_format", [("Insta360", "camm"), ("Labpano", "gpmd")] + "make, meta_format", [("Insta360", "camm"), ("Labpano", "gpmd"), ("", "camm")] ) def test_other_tracks_are_left_alone(self, make: str, meta_format: str): - xml = _camm_exiftool_xml(make, meta_format=meta_format) - track = ExifToolReadVideo(xml).extract_gps_track() - epoch_time = T.cast(telemetry.GPSPoint, track[0]).epoch_time + epoch_time = self._first_epoch_time(_camm_exiftool_xml(make, meta_format)) assert epoch_time == pytest.approx(self.EXIFTOOL_TIME, abs=1e-3) + def test_camm_tracks_written_by_mapillary_tools_are_left_alone(self): + """They hold Unix time, which ExifTool reads as is.""" + xml = _camm_exiftool_xml("Labpano", format_tag="OtherFormat") + epoch_time = self._first_epoch_time(xml) + assert epoch_time == pytest.approx(self.EXIFTOOL_TIME, abs=1e-3) + + @pytest.mark.skipif(shutil.which("exiftool") is None, reason="needs ExifTool") + def test_real_exiftool_reads_our_output_as_written(self, tmp_path: Path): + video_path = tmp_path / "labpano.mp4" + video_path.write_bytes( + _write_camm_mp4(_unix_track(), "Labpano", A_UNIX_TIME + 600) + ) + + xml = ExiftoolRunner(T.cast(str, shutil.which("exiftool"))).extract_xml( + [video_path] + ) + [rdf] = exiftool_read.index_rdf_description_by_path_from_xml_element( + ET.fromstring(xml) + ).values() + track = ExifToolReadVideo(ET.ElementTree(rdf)).extract_gps_track() + + epoch_times = [T.cast(telemetry.GPSPoint, p).epoch_time for p in track] + assert epoch_times == pytest.approx(UNIX_TIMES, abs=1e-3) + class TestEditListOverflow: """An oversized initial gap must not abort the upload.""" From 246e579f2bbd2129d9b90ad1d82ab7c5a848b33d Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Wed, 23 Sep 2026 15:41:43 +0200 Subject: [PATCH 3/4] Reject GPX tracks that miss the video, and drop stray CAMM timestamps Addresses further review of the CAMM epoch follow-ups: - Check the GPX track against the whole video, from video time 0 to the mvhd duration, rather than to the video's last GPS point. A track that misses the video now raises MapillaryOutsideGPXTrackError whatever the gap, since it has no position for any moment of the video. This catches the most common mistake, naive GPX timestamps read in the wrong time zone: main syncs a GPX 2 h off silently, and the previous commit only warned. A track that covers only part of the video warns. When the duration cannot be read, the previous commit's rule applies: a gap warns up to 24 h and raises beyond. - Without a conclusive creation time, do not read a CAMM timestamp as GPS time if that would put it in the future. Unix time read as GPS time lands ten years late, so our output of a Labpano source without a creation time no longer reads back ten years late, for recordings less than ten years old when read. - Drop CAMM GPS points whose timestamps are more than 30 days from the median of the track, and warn. A stray was converted with the track, so it ended up years off and corrupted the timestamps interpolated next to it. - Test that images outside a GPX track fall through to the next geotag source. The previous commit made MapillaryOutsideGPXTrackError a geotagging error, and images raise it too: with --geotag_source gpx --geotag_source exif, an image outside the GPX track now gets its EXIF location instead of failing. With GPX as the only or last source, nothing changes. --- mapillary_tools/camm/camm_parser.py | 47 ++- .../geotag/video_extractors/gpx.py | 81 +++++- tests/unit/test_gps_epoch.py | 275 ++++++++++++++---- 3 files changed, 331 insertions(+), 72 deletions(-) diff --git a/mapillary_tools/camm/camm_parser.py b/mapillary_tools/camm/camm_parser.py index 15600638..65934000 100644 --- a/mapillary_tools/camm/camm_parser.py +++ b/mapillary_tools/camm/camm_parser.py @@ -11,6 +11,7 @@ import io import logging import statistics +import time import typing as T from enum import Enum @@ -141,13 +142,17 @@ def extract_camm_info(fp: T.BinaryIO, telemetry_only: bool = False) -> CAMMInfo # Seconds between the mp4 epoch (1904-01-01) and the Unix epoch. _MP4_EPOCH_UNIX_OFFSET = 2082844800 -# How close the first GPS timestamp has to be to the mvhd creation_time, read -# either as Unix time or as GPS time, for the creation time to decide the -# epoch. The two readings are ten years apart, so this can be generous enough -# to absorb local-time creation times and long recordings without ever being -# ambiguous. +# How close the median GPS timestamp of a track has to be to the mvhd +# creation_time, read either as Unix time or as GPS time, for the creation time +# to decide the epoch. The two readings are ten years apart, so this can be +# generous enough to absorb local-time creation times and long recordings +# without ever being ambiguous. _CREATION_TIME_TOLERANCE = 30 * 24 * 3600 +# A timestamp this far from the median of its track cannot belong to it: even +# a time-lapse spans days, not months +_STRAY_TOLERANCE = 30 * 24 * 3600 + def make_records_gps_time(make: str) -> bool: """ @@ -174,7 +179,9 @@ def _records_gps_time( The creation time of the video decides when it can: GPS time reads as one GPS epoch before it, Unix time reads close to it. The make decides only when the creation time settles neither, because it is missing or - meaningless (a GoPro HERO7 recorded in 2022 reports 2016). + meaningless (a GoPro HERO7 recorded in 2022 reports 2016). Even then, a + timestamp that GPS time would put in the future is Unix time: that is what + mapillary_tools writes for a Labpano source without a creation time. >>> creation_time = 1705574637 # 2024-01-18T10:43:57Z >>> _records_gps_time(1389609661, "", creation_time) # GPS time @@ -193,7 +200,14 @@ def _records_gps_time( if abs(gap) < _CREATION_TIME_TOLERANCE: return False - return make_records_gps_time(make) + if not make_records_gps_time(make): + return False + + # Read as GPS time, Unix time lands ten years late, so in the future for + # any recording less than ten years old + return ( + telemetry.gps_epoch_to_unix(epoch_time) < time.time() + _CREATION_TIME_TOLERANCE + ) def _extract_creation_time(moov: MovieBoxParser | None) -> float | None: @@ -217,7 +231,8 @@ def _normalize_gps_epochs( ) -> None: """ Rewrite CAMMGPSPoint.epoch_time in place so it is Unix time regardless of - which epoch the producer used. + which epoch the producer used, and drop the points whose timestamps are + strays. This is the only place CAMM GPS timestamps change epoch. Everything downstream, including the serializer, treats them as Unix time. @@ -229,6 +244,22 @@ def _normalize_gps_epochs( # Decide by the median rather than by any one sample, so that a stray # timestamp cannot flip the epoch of the whole track median_epoch_time = statistics.median(epoch_times) + + # A stray timestamp would still be wrong by years once converted with the + # track, and would corrupt the timestamps interpolated next to it + kept = [ + p + for p in gps + if p.epoch_time <= 0 or abs(p.epoch_time - median_epoch_time) < _STRAY_TOLERANCE + ] + if len(kept) < len(gps): + LOG.warning( + "Dropped %d of %d CAMM GPS points whose timestamps are more than 30 days from the rest of the track", + len(gps) - len(kept), + len(gps), + ) + gps[:] = kept + if not _records_gps_time(median_epoch_time, make, _extract_creation_time(moov)): return diff --git a/mapillary_tools/geotag/video_extractors/gpx.py b/mapillary_tools/geotag/video_extractors/gpx.py index 47a2a555..931bac53 100644 --- a/mapillary_tools/geotag/video_extractors/gpx.py +++ b/mapillary_tools/geotag/video_extractors/gpx.py @@ -13,12 +13,15 @@ import typing as T from pathlib import Path +import construct as C + if sys.version_info >= (3, 12): from typing import override else: from typing_extensions import override from ... import exceptions, geo, types, utils +from ...mp4 import construct_mp4_parser as cparser, simple_mp4_parser as sparser from ...serializer.description import build_capture_time from ..utils import parse_gpx from .base import BaseVideoExtractor @@ -27,11 +30,17 @@ LOG = logging.getLogger(__name__) -# A GPX track that misses the video by more than this cannot belong to it. No -# camera clock or time zone mistake comes close, while an epoch mix-up exceeds -# it by orders of magnitude. +# When the duration of the video is unknown, a GPX track that misses the +# video's GPS by more than this cannot belong to it. No camera clock or time +# zone mistake comes close, while an epoch mix-up exceeds it by orders of +# magnitude. _IMPLAUSIBLE_GAP_SECONDS = 24 * 3600 +# How much of the video a GPX track may leave uncovered without a warning. A +# logger that records whole seconds once a second, started and stopped with +# the camera, can leave a second at each end. +_UNCOVERED_TOLERANCE_SECONDS = 2.0 + class SyncMode(enum.Enum): # Sync by video GPS timestamps if found, otherwise rebase @@ -81,46 +90,88 @@ def extract(self) -> types.VideoMetadata: else: offset = self._gpx_offset(gpx_points, native_video_metadata.points) if offset: - self._check_time_gap(gpx_points, native_video_metadata.points, offset) + self._check_time_gap( + gpx_points, + native_video_metadata.points, + offset, + self._video_duration(), + ) self._rebase_times(gpx_points, offset=offset) return dataclasses.replace(native_video_metadata, points=gpx_points) + def _video_duration(self) -> float | None: + """ + The duration of the video in seconds, from its mvhd box, or None if it + cannot be read. + """ + try: + with self.video_path.open("rb") as fp: + data = sparser.parse_box_data_first(fp, [b"moov", b"mvhd"]) + if data is None: + return None + mvhd = cparser.MovieHeaderBox.parse(data) + except (OSError, sparser.ParsingError, C.ConstructError) as ex: + LOG.debug("Failed to read the duration of %s: %s", self.video_path, ex) + return None + + # All 1s means the duration is unknown + if not mvhd.timescale or mvhd.duration in (0, 0xFFFFFFFF, 0xFFFFFFFFFFFFFFFF): + return None + + return mvhd.duration / mvhd.timescale + def _check_time_gap( self, gpx_points: T.Sequence[geo.Point], video_gps_points: T.Sequence[geo.Point], offset: float, + video_duration: float | None, ) -> None: """ Check the GPX track, once synced by offset, against the video in time. - Silent when the two overlap. A gap only warns, because it can be - legitimate, as when the GPX starts after the video's own GPS gives out. - A gap too large for any clock or time zone mistake to explain raises: - the GPX cannot belong to the video, and syncing to it would put every - point far outside the video. + A GPX track that misses the video raises: positions outside the track + are extrapolated, so syncing to it would give every frame a made-up + position. A track that covers only part of the video warns, and one + that covers all of it is silent. + + Without the duration of the video, the video is known only up to its + last GPS point, and a GPX that starts after it may still overlap the + video. Then a gap only warns, unless it is too large for any clock or + time zone mistake to explain. """ # The Unix time of video time 0, in the convention _rebase_times() uses video_start_time = gpx_points[0].time - offset # From video time 0, since the frames start there even when the video's # own GPS starts later - video_last = video_start_time + max(p.time for p in video_gps_points) + video_end_time = video_start_time + max(p.time for p in video_gps_points) + if video_duration is not None: + video_end_time = max(video_end_time, video_start_time + video_duration) gpx_first = min(p.time for p in gpx_points) gpx_last = max(p.time for p in gpx_points) - gap = max(gpx_first - video_last, video_start_time - gpx_last) + gpx_track = f"The GPX track in {self.gpx_path} ({_isoformat(gpx_first)} to {_isoformat(gpx_last)})" + video = f"the video {self.video_path} ({_isoformat(video_start_time)} to {_isoformat(video_end_time)})" + + gap = max(gpx_first - video_end_time, video_start_time - gpx_last) if gap <= 0: + uncovered = max(gpx_first - video_start_time, 0) + max( + video_end_time - gpx_last, 0 + ) + if uncovered > _UNCOVERED_TOLERANCE_SECONDS: + LOG.warning( + f"{gpx_track} covers only part of {video}: {uncovered:.0f} seconds " + "of the video fall outside the track, where positions are extrapolated" + ) return message = ( - f"The GPX track in {self.gpx_path} ({_isoformat(gpx_first)} to {_isoformat(gpx_last)}) " - f"misses the video {self.video_path} ({_isoformat(video_start_time)} to {_isoformat(video_last)}) " - f"by {gap:.0f} seconds ({gap / 86400:.1f} days). " + f"{gpx_track} misses {video} by {gap:.0f} seconds ({gap / 86400:.1f} days). " "Check the camera clock, and the time zone of the GPX timestamps" ) - if gap > _IMPLAUSIBLE_GAP_SECONDS: + if video_duration is not None or gap > _IMPLAUSIBLE_GAP_SECONDS: raise exceptions.MapillaryOutsideGPXTrackError( message, image_time=build_capture_time(video_start_time), diff --git a/tests/unit/test_gps_epoch.py b/tests/unit/test_gps_epoch.py index af5f5616..deac40aa 100644 --- a/tests/unit/test_gps_epoch.py +++ b/tests/unit/test_gps_epoch.py @@ -55,6 +55,21 @@ # Seconds between the mp4 epoch (1904-01-01) and the Unix epoch MP4_UNIX_DELTA = 2082844800 +# When the tests read tracks: two months after A_UNIX_TIME +NOW = A_UNIX_TIME + 60 * 24 * 3600 + + +class _PinnedClock: + @staticmethod + def time() -> float: + return NOW + + +@pytest.fixture +def pinned_now(monkeypatch): + """Pin the clock the make fallback reads, so the tests do not expire.""" + monkeypatch.setattr(camm_parser, "time", _PinnedClock) + def _camm_point(time: float, epoch_time: float) -> telemetry.CAMMGPSPoint: return telemetry.CAMMGPSPoint( @@ -89,12 +104,16 @@ def _gps_point(time: float, epoch_time: float | None) -> telemetry.GPSPoint: def _write_camm_mp4( - points: T.Sequence[geo.Point], make: str, creation_time: float | None + points: T.Sequence[geo.Point], + make: str, + creation_time: float | None, + duration: float = 10.0, ) -> bytes: """ Write points as a CAMM track into an empty mp4, the way the uploader does. creation_time is the Unix time to put in mvhd, or None to leave it unset. + duration is the duration of the video in seconds, or 0 for unknown. """ mp4_creation_time = ( 0 if creation_time is None else int(creation_time) + MP4_UNIX_DELTA @@ -105,7 +124,7 @@ def _write_camm_mp4( "creation_time": mp4_creation_time, "modification_time": mp4_creation_time, "timescale": 1000, - "duration": 36000 * 1000, + "duration": int(duration * 1000), }, } src = cparser.MP4WithoutSTBLBuilderConstruct.build_boxlist( @@ -231,8 +250,35 @@ def test_unknown_make_in_gps_time_is_read_from_the_file(self): assert _unix_times(_read_camm(data)) == [A_UNIX_TIME] -class TestOneSampleDoesNotDecide: - """The whole track decides its epoch, not its first timestamp.""" +@pytest.mark.usefixtures("pinned_now") +class TestWithoutCreationTime: + """ + Without a conclusive creation time the make decides, but GPS time must + not put a timestamp in the future: Unix time read as GPS time lands ten + years late. That keeps our output of a Labpano source without a creation + time from reading back as 2036, for recordings less than ten years old + when read. + """ + + @pytest.mark.parametrize("make", ["Labpano", "Labpano Technology Co.,Ltd"]) + def test_unix_time_is_not_converted(self, make: str): + assert not camm_parser._records_gps_time(A_UNIX_TIME, make, None) + + def test_gps_time_is_converted(self): + assert camm_parser._records_gps_time(A_GPS_TIME, "Labpano", None) + + def test_labpano_original_is_converted(self): + data = _write_camm_mp4( + [_camm_point(time=0.0, epoch_time=A_GPS_TIME)], "Labpano", None + ) + assert _unix_times(_read_camm(data)) == [A_UNIX_TIME] + + +class TestStraySamples: + """ + The whole track decides its epoch, not its first timestamp, and a + timestamp more than 30 days from the rest of the track is dropped. + """ CREATION_TIME = A_UNIX_TIME + 600 @@ -247,13 +293,32 @@ def _read_back(self, raw_times: list[float]) -> list[float]: def test_stray_unix_time_does_not_flip_a_gps_time_track(self): raw_times = [A_UNIX_TIME] + [A_GPS_TIME + t for t in range(1, 10)] - unix_times = self._read_back(raw_times) - assert unix_times[1:] == [A_UNIX_TIME + t for t in range(1, 10)] + assert self._read_back(raw_times) == [A_UNIX_TIME + t for t in range(1, 10)] def test_stray_gps_time_does_not_flip_a_unix_time_track(self): raw_times = [A_GPS_TIME] + [A_UNIX_TIME + t for t in range(1, 10)] - unix_times = self._read_back(raw_times) - assert unix_times[1:] == [A_UNIX_TIME + t for t in range(1, 10)] + assert self._read_back(raw_times) == [A_UNIX_TIME + t for t in range(1, 10)] + + def test_stray_is_dropped_wherever_it_is(self, caplog): + """Kept, it would sit ten years off, amid the track it interrupts.""" + raw_times = [A_UNIX_TIME + t for t in range(10)] + raw_times[5] = A_GPS_TIME + 5 + + with caplog.at_level(logging.WARNING): + unix_times = self._read_back(raw_times) + + assert unix_times == [A_UNIX_TIME + t for t in range(10) if t != 5] + [record] = caplog.records + assert "Dropped 1 of 10" in record.getMessage() + + def test_points_without_a_timestamp_are_not_strays(self): + points = [_camm_point(time=0.0, epoch_time=0.0)] + [ + _camm_point(time=float(t), epoch_time=A_GPS_TIME + t) for t in range(1, 5) + ] + camm_parser._normalize_gps_epochs(points, "Labpano") + assert [p.epoch_time for p in points] == [0.0] + [ + A_UNIX_TIME + t for t in range(1, 5) + ] class TestWriteRoundTrip: @@ -273,7 +338,10 @@ def test_round_trip(self, make: str): data = _write_camm_mp4(_unix_track(), make, A_UNIX_TIME + 600) assert _unix_times(_read_camm(data)) == UNIX_TIMES - @pytest.mark.parametrize("make", ["Insta360", ""]) + @pytest.mark.usefixtures("pinned_now") + @pytest.mark.parametrize( + "make", ["Labpano", "Labpano Technology Co.,Ltd", "Insta360", ""] + ) def test_round_trip_without_creation_time(self, make: str): data = _write_camm_mp4(_unix_track(), make, None) assert _unix_times(_read_camm(data)) == UNIX_TIMES @@ -412,70 +480,112 @@ def _isoformat(unix_time: float) -> str: class TestGPXTimeGap: """ - A GPX track that misses the video by more than a day must fail, not sync. + A GPX track that misses the video must fail, not sync. - The edit list no longer overflows on a huge offset, so this is what keeps - an epoch mix-up or a GPX file from another day from being uploaded. A - smaller gap only warns, since it can be legitimate. + Positions outside a GPX track are extrapolated, so a track that misses + the video would give every frame a made-up position: an epoch mix-up, a + GPX file from another day, or naive GPX timestamps read in the wrong time + zone. A track that covers only part of the video warns. """ - # A 10s video track starting at A_UNIX_TIME + # A 10s video track starting at A_UNIX_TIME, in a 12s video VIDEO = [_camm_point(time=float(t), epoch_time=A_UNIX_TIME + t) for t in range(11)] + VIDEO_DURATION = 12.0 def _check( self, gpx_points: list[telemetry.CAMMGPSPoint], video: T.Sequence[geo.Point] = VIDEO, + duration: float | None = VIDEO_DURATION, ) -> None: extractor = GPXVideoExtractor(Path("video.mp4"), Path("track.gpx")) offset = extractor._gpx_offset(gpx_points, video) - extractor._check_time_gap(gpx_points, video, offset) + extractor._check_time_gap(gpx_points, video, offset, duration) @pytest.mark.parametrize( "start, duration", [ - # Starts 30s before the video and ends during it - (A_UNIX_TIME - 30, 35), - # Starts during the video - (A_UNIX_TIME + 5, 60), + # Starts 30s before the video and ends after it + (A_UNIX_TIME - 30, 60), + # Started a second into the video, and ends with it + (A_UNIX_TIME + 1, 11), # A long recording that spans the video (A_UNIX_TIME - 3 * 24 * 3600, 6 * 24 * 3600), ], ) - def test_overlapping_gpx_passes_silently(self, caplog, start: float, duration: int): + def test_covering_gpx_passes_silently(self, caplog, start: float, duration: int): with caplog.at_level(logging.WARNING): self._check(_gpx_track(start, duration)) assert not caplog.records @pytest.mark.parametrize( - "start", + "start, duration, uncovered", [ - # Ended a minute before the video started - A_UNIX_TIME - 70, - # Naive GPX timestamps read in a time zone 9h off - A_UNIX_TIME + 9 * 3600, - # Starts after the video's own GPS gives out - A_UNIX_TIME + 60, + # Ends 5s into the video + (A_UNIX_TIME - 30, 35, 7), + # Starts 5s into the video + (A_UNIX_TIME + 5, 60, 5), + # Starts after the video's own GPS gives out, before the video ends + (A_UNIX_TIME + 11, 20, 11), ], ) - def test_small_gap_warns(self, caplog, start: float): + def test_partial_cover_warns( + self, caplog, start: float, duration: int, uncovered: int + ): with caplog.at_level(logging.WARNING): - self._check(_gpx_track(start, 10)) + self._check(_gpx_track(start, duration)) [record] = caplog.records + assert "covers only part of" in record.getMessage() + assert f"{uncovered} seconds" in record.getMessage() assert "video.mp4" in record.getMessage() assert "track.gpx" in record.getMessage() @pytest.mark.parametrize( "start", - [A_UNIX_TIME + 2 * 24 * 3600, A_UNIX_TIME - 3 * 24 * 3600], + [ + # Ended a minute before the video started + A_UNIX_TIME - 70, + # Starts after the video ends, though within a minute of it + A_UNIX_TIME + 13, + # Naive GPX timestamps read in a time zone 2h off + A_UNIX_TIME + 2 * 3600, + # A GPX file from another day + A_UNIX_TIME - 3 * 24 * 3600, + ], ) - def test_gap_of_days_raises(self, start: float): + def test_missing_the_video_raises(self, start: float): with pytest.raises(exceptions.MapillaryOutsideGPXTrackError) as info: self._check(_gpx_track(start, 10)) assert "video.mp4" in str(info.value) assert "track.gpx" in str(info.value) + assert "time zone" in str(info.value) + + @pytest.mark.parametrize( + "start", + [ + A_UNIX_TIME - 70, + A_UNIX_TIME + 2 * 3600, + # Starts after the video's own GPS gives out, which the video + # itself may not + A_UNIX_TIME + 60, + ], + ) + def test_without_duration_a_gap_warns(self, caplog, start: float): + with caplog.at_level(logging.WARNING): + self._check(_gpx_track(start, 10), duration=None) + [record] = caplog.records + assert "misses" in record.getMessage() + + @pytest.mark.parametrize( + "start", + [A_UNIX_TIME + 2 * 24 * 3600, A_UNIX_TIME - 3 * 24 * 3600], + ) + def test_without_duration_a_gap_of_days_raises(self, start: float): + with pytest.raises(exceptions.MapillaryOutsideGPXTrackError): + self._check(_gpx_track(start, 10), duration=None) - def test_epoch_mixup_raises(self): + @pytest.mark.parametrize("duration", [VIDEO_DURATION, None]) + def test_epoch_mixup_raises(self, duration: float | None): """Video timestamps left in GPS time sync ten years off.""" video = [ _camm_point(time=float(t), epoch_time=A_GPS_TIME + t) for t in range(11) @@ -484,7 +594,7 @@ def test_epoch_mixup_raises(self): offset = GPXVideoExtractor._gpx_offset(gpx_points, video) assert abs(offset - (GPS_UNIX_DELTA - 18)) < 1 with pytest.raises(exceptions.MapillaryOutsideGPXTrackError): - self._check(gpx_points, video) + self._check(gpx_points, video, duration) def test_error_survives_a_worker_process(self): """Videos are geotagged in a process pool, so the error gets pickled.""" @@ -496,36 +606,62 @@ def test_error_survives_a_worker_process(self): assert str(unpickled) == str(info.value) assert vars(unpickled) == vars(info.value) - def test_extract_syncs_labpano_video_to_its_gpx(self, tmp_path: Path): + def _write_video_and_gpx( + self, tmp_path: Path, gpx_start: float, duration: float = VIDEO_DURATION + ) -> tuple[Path, Path]: video_path = tmp_path / "labpano.mp4" video_path.write_bytes( - _write_camm_mp4(self.VIDEO, "Labpano", A_UNIX_TIME + 600) + _write_camm_mp4(self.VIDEO, "Labpano", A_UNIX_TIME + 600, duration) ) gpx_path = tmp_path / "labpano.gpx" - _write_gpx(gpx_path, _gpx_track(int(A_UNIX_TIME) - 5, 20)) + _write_gpx(gpx_path, _gpx_track(gpx_start, 20)) + return video_path, gpx_path + + def test_extract_syncs_labpano_video_to_its_gpx(self, tmp_path: Path, caplog): + video_path, gpx_path = self._write_video_and_gpx(tmp_path, A_UNIX_TIME - 5) - points = GPXVideoExtractor(video_path, gpx_path).extract().points + with caplog.at_level(logging.WARNING): + points = GPXVideoExtractor(video_path, gpx_path).extract().points # The GPX starts ~5s before the video, whose GPS starts at video time 0 assert -6 < points[0].time < -4 + assert not caplog.records - def _write_video_and_gpx( + @pytest.mark.parametrize( + "gpx_start", + [ + # From days before + A_UNIX_TIME - 3 * 24 * 3600, + # In a time zone 2h off + A_UNIX_TIME + 2 * 3600, + ], + ) + def test_extract_rejects_gpx_that_misses_the_video( self, tmp_path: Path, gpx_start: float - ) -> tuple[Path, Path]: - video_path = tmp_path / "labpano.mp4" - video_path.write_bytes( - _write_camm_mp4(self.VIDEO, "Labpano", A_UNIX_TIME + 600) - ) - gpx_path = tmp_path / "other.gpx" - _write_gpx(gpx_path, _gpx_track(gpx_start, 20)) - return video_path, gpx_path + ): + video_path, gpx_path = self._write_video_and_gpx(tmp_path, gpx_start) + with pytest.raises(exceptions.MapillaryOutsideGPXTrackError): + GPXVideoExtractor(video_path, gpx_path).extract() - def test_extract_rejects_gpx_from_days_before(self, tmp_path: Path): + def test_extract_without_duration_only_warns_of_hours(self, tmp_path: Path, caplog): video_path, gpx_path = self._write_video_and_gpx( - tmp_path, A_UNIX_TIME - 3 * 24 * 3600 + tmp_path, A_UNIX_TIME + 2 * 3600, duration=0 ) - with pytest.raises(exceptions.MapillaryOutsideGPXTrackError): + with caplog.at_level(logging.WARNING): GPXVideoExtractor(video_path, gpx_path).extract() + [record] = caplog.records + assert "misses" in record.getMessage() + + def test_video_duration_is_read_from_the_file(self, tmp_path: Path): + video_path, gpx_path = self._write_video_and_gpx(tmp_path, A_UNIX_TIME) + assert GPXVideoExtractor(video_path, gpx_path)._video_duration() == 12.0 + + @pytest.mark.parametrize("content", [b"", b"not a real mp4"]) + def test_unreadable_duration_is_unknown(self, tmp_path: Path, content: bytes): + video_path = tmp_path / "video.mp4" + video_path.write_bytes(content) + extractor = GPXVideoExtractor(video_path, tmp_path / "track.gpx") + assert extractor._video_duration() is None def test_next_source_gets_its_turn(self, tmp_path: Path): """The GPX misses the video, which says nothing about the video itself.""" @@ -547,6 +683,47 @@ def test_next_source_gets_its_turn(self, tmp_path: Path): assert [p.time for p in metadata.points] == [p.time for p in self.VIDEO] +class TestImagesOutsideGPXTrack: + """ + Images outside a GPX track fail with the same error, a geotagging error, + so they too fall through to the next geotag source. + """ + + # Captured 2018-06-08T20:24:11Z at 45.5169, -122.5728 + IMAGE = Path(__file__).parent.parent / "data" / "images" / "DSC00001.JPG" + + def _process(self, tmp_path: Path, sources: list[SourceType]): + image_path = tmp_path / self.IMAGE.name + shutil.copyfile(self.IMAGE, image_path) + # A track recorded years after the image + gpx_path = tmp_path / "track.gpx" + _write_gpx(gpx_path, _gpx_track(A_UNIX_TIME, 10)) + options = [ + SourceOption( + source, + num_processes=0, + source_path=SourcePathOption(source_path=gpx_path), + ) + if source is SourceType.GPX + else SourceOption(source, num_processes=0) + for source in sources + ] + [metadata] = factory.process([image_path], options) + return metadata + + def test_next_source_gets_its_turn(self, tmp_path: Path): + metadata = self._process(tmp_path, [SourceType.GPX, SourceType.EXIF]) + + assert isinstance(metadata, types.ImageMetadata) + assert (round(metadata.lat, 4), round(metadata.lon, 4)) == (45.5169, -122.5728) + + def test_gpx_as_the_only_source_fails(self, tmp_path: Path): + metadata = self._process(tmp_path, [SourceType.GPX]) + + assert isinstance(metadata, types.ErrorMetadata) + assert isinstance(metadata.error, exceptions.MapillaryOutsideGPXTrackError) + + def _camm_exiftool_xml( make: str, meta_format: str = "camm", format_tag: str = "MetaFormat" ) -> ET.ElementTree: From d761dace84e33dac0aba6ee8bd235db4af1c4cda Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Wed, 23 Sep 2026 16:45:38 +0200 Subject: [PATCH 4/4] Check GPX tracks synced to video time 0, and report sub-second gaps Addresses review of the previous commit: - The GPX time check ran only for a non-zero offset. When the video's GPS has no timestamps, as on the Ricoh Theta X, the offset is 0 and nothing was checked, and neither was a GPX that syncs to exactly video time 0 but covers only part of the video. Check whenever there are GPX and video GPS points. A GPX synced to 0 starts with the video, so the check can only warn that it ends before the video does. - Report a gap in seconds, minutes, hours or days, whichever fits, with significant digits: a GPX starting 0.089 s after the video ended was reported as missing it "by 0 seconds (0.0 days)". The hint now starts with checking that the GPX file belongs to the video, which fits a gap of any size, such as the GPX of the next clip. - Say what is known about the part of a video outside a GPX track: the track has no position for it. The warning and docstrings said that positions there are extrapolated. The video sampler drops frames outside its track instead, and what happens after upload cannot be checked here. --- .../geotag/video_extractors/gpx.py | 44 +++++++++--- tests/unit/test_gps_epoch.py | 69 +++++++++++++++++-- 2 files changed, 100 insertions(+), 13 deletions(-) diff --git a/mapillary_tools/geotag/video_extractors/gpx.py b/mapillary_tools/geotag/video_extractors/gpx.py index 931bac53..cd8973e0 100644 --- a/mapillary_tools/geotag/video_extractors/gpx.py +++ b/mapillary_tools/geotag/video_extractors/gpx.py @@ -89,7 +89,7 @@ def extract(self) -> types.VideoMetadata: self._rebase_times(gpx_points) else: offset = self._gpx_offset(gpx_points, native_video_metadata.points) - if offset: + if gpx_points and native_video_metadata.points: self._check_time_gap( gpx_points, native_video_metadata.points, @@ -131,10 +131,12 @@ def _check_time_gap( """ Check the GPX track, once synced by offset, against the video in time. - A GPX track that misses the video raises: positions outside the track - are extrapolated, so syncing to it would give every frame a made-up - position. A track that covers only part of the video warns, and one - that covers all of it is silent. + A GPX track that misses the video raises: it has no position for any + moment of the video. A track that covers only part of the video warns, + and one that covers all of it is silent. + + When the video's GPS has no timestamps, the offset is 0 and the track + starts at video time 0, so only the end of the video can go uncovered. Without the duration of the video, the video is known only up to its last GPS point, and a GPX that starts after it may still overlap the @@ -161,14 +163,15 @@ def _check_time_gap( ) if uncovered > _UNCOVERED_TOLERANCE_SECONDS: LOG.warning( - f"{gpx_track} covers only part of {video}: {uncovered:.0f} seconds " - "of the video fall outside the track, where positions are extrapolated" + f"{gpx_track} covers only part of {video}: " + f"{_format_duration(uncovered)} of the video fall outside the track" ) return message = ( - f"{gpx_track} misses {video} by {gap:.0f} seconds ({gap / 86400:.1f} days). " - "Check the camera clock, and the time zone of the GPX timestamps" + f"{gpx_track} misses {video} by {_format_duration(gap)}. Check that " + "the GPX file belongs to this video, then the camera clock and the " + "time zone of the GPX timestamps" ) if video_duration is not None or gap > _IMPLAUSIBLE_GAP_SECONDS: @@ -231,3 +234,26 @@ def _isoformat(unix_time: float) -> str: return datetime.datetime.fromtimestamp( unix_time, tz=datetime.timezone.utc ).isoformat() + + +def _format_duration(seconds: float) -> str: + """ + >>> _format_duration(0.089) + '0.089 seconds' + >>> _format_duration(1) + '1 second' + >>> _format_duration(13) + '13 seconds' + >>> _format_duration(90) + '1.5 minutes' + >>> _format_duration(2 * 3600) + '2.0 hours' + >>> _format_duration(3 * 86400) + '3.0 days' + """ + for unit, size in (("days", 86400), ("hours", 3600), ("minutes", 60)): + if seconds >= size: + return f"{seconds / size:.1f} {unit}" + # Significant digits, so that a gap under a second does not read as 0 + text = f"{seconds:.3g}" + return f"{text} second" if text == "1" else f"{text} seconds" diff --git a/tests/unit/test_gps_epoch.py b/tests/unit/test_gps_epoch.py index deac40aa..8741f5b5 100644 --- a/tests/unit/test_gps_epoch.py +++ b/tests/unit/test_gps_epoch.py @@ -482,16 +482,25 @@ class TestGPXTimeGap: """ A GPX track that misses the video must fail, not sync. - Positions outside a GPX track are extrapolated, so a track that misses - the video would give every frame a made-up position: an epoch mix-up, a - GPX file from another day, or naive GPX timestamps read in the wrong time - zone. A track that covers only part of the video warns. + A track that misses the video has no position for any moment of it: an + epoch mix-up, a GPX file from another day, or naive GPX timestamps read + in the wrong time zone. A track that covers only part of the video warns. """ # A 10s video track starting at A_UNIX_TIME, in a 12s video VIDEO = [_camm_point(time=float(t), epoch_time=A_UNIX_TIME + t) for t in range(11)] VIDEO_DURATION = 12.0 + # GPS without timestamps, so the GPX can only be aligned to video time 0 + UNTIMED_VIDEO = [ + geo.Point(time=float(t), lat=37.0, lon=14.0, alt=None, angle=None) + for t in range(11) + ] + # Timestamps in whole seconds, as in the GPX, so that it syncs to exactly 0 + WHOLE_SECOND_VIDEO = [ + _camm_point(time=float(t), epoch_time=int(A_UNIX_TIME) + t) for t in range(11) + ] + def _check( self, gpx_points: list[telemetry.CAMMGPSPoint], @@ -558,8 +567,26 @@ def test_missing_the_video_raises(self, start: float): self._check(_gpx_track(start, 10)) assert "video.mp4" in str(info.value) assert "track.gpx" in str(info.value) + assert "belongs to this video" in str(info.value) assert "time zone" in str(info.value) + @pytest.mark.parametrize( + "gap, reported", + [ + # The GPX of the next clip, starting just after the video ends + (0.089, "by 0.089 seconds."), + (1, "by 1 second."), + (70, "by 1.2 minutes."), + (2 * 3600, "by 2.0 hours."), + (3 * 24 * 3600, "by 3.0 days."), + ], + ) + def test_gap_is_reported_in_a_readable_unit(self, gap: float, reported: str): + video_end = A_UNIX_TIME + self.VIDEO_DURATION + with pytest.raises(exceptions.MapillaryOutsideGPXTrackError) as info: + self._check(_gpx_track(video_end + gap, 10)) + assert reported in str(info.value) + @pytest.mark.parametrize( "start", [ @@ -652,6 +679,40 @@ def test_extract_without_duration_only_warns_of_hours(self, tmp_path: Path, capl [record] = caplog.records assert "misses" in record.getMessage() + def _extract_synced_to_video_time_0( + self, tmp_path: Path, video: T.Sequence[geo.Point], gpx_duration: int + ) -> list[geo.Point]: + video_path = tmp_path / "video.mp4" + video_path.write_bytes( + _write_camm_mp4(video, "Labpano", A_UNIX_TIME + 600, self.VIDEO_DURATION) + ) + gpx_path = tmp_path / "track.gpx" + _write_gpx(gpx_path, _gpx_track(int(A_UNIX_TIME), gpx_duration)) + + points = GPXVideoExtractor(video_path, gpx_path).extract().points + + assert points[0].time == 0.0 + return points + + @pytest.mark.parametrize("video", [UNTIMED_VIDEO, WHOLE_SECOND_VIDEO]) + def test_extract_warns_of_partial_cover_at_video_time_0( + self, tmp_path: Path, caplog, video: T.Sequence[geo.Point] + ): + """An offset of 0 is a sync like any other, not a reason to skip the check.""" + with caplog.at_level(logging.WARNING): + # Covers the first 6 of the video's 12 seconds + self._extract_synced_to_video_time_0(tmp_path, video, 6) + [record] = caplog.records + assert "6 seconds of the video fall outside the track" in record.getMessage() + + @pytest.mark.parametrize("video", [UNTIMED_VIDEO, WHOLE_SECOND_VIDEO]) + def test_extract_covering_at_video_time_0_is_silent( + self, tmp_path: Path, caplog, video: T.Sequence[geo.Point] + ): + with caplog.at_level(logging.WARNING): + self._extract_synced_to_video_time_0(tmp_path, video, 20) + assert not caplog.records + def test_video_duration_is_read_from_the_file(self, tmp_path: Path): video_path, gpx_path = self._write_video_and_gpx(tmp_path, A_UNIX_TIME) assert GPXVideoExtractor(video_path, gpx_path)._video_duration() == 12.0