Leave out ExifTool GPS points timestamped far from the others

mapillary_tools/exiftool_read_video.py

@@ -11,7 +11,7 @@
 import typing as T
 import xml.etree.ElementTree as ET
 
-from . import exif_read, exiftool_read, geo
+from . import constants, exif_read, exiftool_read, geo
 from .telemetry import GPSFix, GPSPoint
 from .utils import sanitize_serial
 
@@ -121,6 +121,45 @@ def _deduplicate_gps_points(
     return deduplicated_track
 
 
+def _split_off_far_points(
+    track: list[GPSPoint], max_gap_seconds: float
+) -> tuple[list[GPSPoint], list[GPSPoint]]:
+    """
+    Split the track, sorted by GPS time, where two consecutive points are more
+    than **max_gap_seconds** apart, and return the segment with the most
+    points (the earliest of them on a tie) and the points of the others.
+
+    A receiver at cold start, or a wrong date, can write a GPS timestamp far
+    from the others, which would put every other point that far into the
+    video once the times are made relative to the first point.
+
+    >>> def point(time):
+    ...     return GPSPoint(time=time, lat=1, lon=1, alt=None, angle=None, epoch_time=time, fix=None, precision=None, ground_speed=None)
+    >>> kept, far = _split_off_far_points([point(t) for t in [0, 79738, 79739, 79740]], 3600)
+    >>> [p.time for p in kept], [p.time for p in far]
+    ([79738, 79739, 79740], [0])
+    >>> kept, far = _split_off_far_points([point(t) for t in [0, 3600, 7200, 7300]], 3600)
+    >>> [p.time for p in kept], [p.time for p in far]
+    ([0, 3600, 7200, 7300], [])
+    >>> kept, far = _split_off_far_points([point(t) for t in [0, 3601]], 3600)
+    >>> [p.time for p in kept], [p.time for p in far]
+    ([0], [3601])
+    """
+    segments: list[list[GPSPoint]] = []
+    for point in track:
+        if not segments or max_gap_seconds < point.time - segments[-1][-1].time:
+            segments.append([])
+        segments[-1].append(point)
+
+    if len(segments) <= 1:
+        return track, []
+
+    kept = max(segments, key=len)
+    return kept, [
+        point for segment in segments if segment is not kept for point in segment
+    ]
+
+
 def _aggregate_float_values_same_length(
     texts_by_tag: dict[str, list[str]],
     tag: str | None,
@@ -215,12 +254,16 @@ def _aggregate_gps_track(
     gps_time_tag: str | None = None,
     direction_tag: str | None = None,
     ground_speed_tag: str | None = None,
+    far_points: list[GPSPoint] | None = None,
 ) -> list[GPSPoint]:
     """
     Aggregate all GPS data by the tags.
     It requires lat, lon to be present, and their lengths must match.
     Some cameras store time information in the SimpleTime tag (and each simple has multiple GPS data points),
     therefore the time_tag is optional. If it is None, then all returned points will have time = 0.0.
+    With the time_tag, the points whose GPS times are far from the most points
+    are left out (see _split_off_far_points()), and added to **far_points**
+    if it is given.
     """
 
     # aggregate coordinates (required)
@@ -303,6 +346,9 @@ def _aggregate_gps_track(
     track = _deduplicate_gps_points(track, same_gps_point=_same_gps_point)
 
     if time_tag is not None:
+        track, far = _split_off_far_points(track, constants.MAX_GPX_GAP_SECONDS)
+        if far_points is not None:
+            far_points.extend(far)
         if track:
             first_time = track[0].time
             for point in track:
@@ -371,11 +417,14 @@ def _aggregate_gps_track_by_record(
     alt_tag: str,
     direction_tag: str,
     record_start_tag: str | None = None,
+    far_points: list[GPSPoint] | None = None,
 ) -> list[GPSPoint]:
     """
     Aggregate the GPS records that ExifTool lists one after another, each with
     its GPS time in **time_tag**, into a track whose times start at 0. Every
-    point takes its altitude and direction from its own record.
+    point takes its altitude and direction from its own record. The points
+    whose GPS times are far from the most points are left out (see
+    _split_off_far_points()), and added to **far_points** if it is given.
     """
     expanded_time_tag = expand_tag(time_tag)
     expanded_lon_tag = expand_tag(lon_tag)
@@ -418,6 +467,10 @@ def _aggregate_gps_track_by_record(
     # recording embedded in it, which ExifTool lists as well
     track = _deduplicate_gps_points(track, same_gps_point=_same_gps_point)
 
+    track, far = _split_off_far_points(track, constants.MAX_GPX_GAP_SECONDS)
+    if far_points is not None:
+        far_points.extend(far)
+
     if track:
         first_time = track[0].time
         for point in track:
@@ -535,8 +588,13 @@ def __init__(
             raise ValueError("ElementTree root is None")
         self._texts_by_tag = _index_text_by_tag(root)
         self._all_tags = set(self._texts_by_tag.keys())
+        # The points that extract_gps_track() left out for GPS times far from
+        # the most points
+        self.far_gps_points: list[GPSPoint] = []
 
     def extract_gps_track(self) -> list[geo.Point]:
+        self.far_gps_points = []
+
         # blackvue and many other cameras
         track_with_fix = self._extract_gps_track_from_quicktime()
         if track_with_fix:
@@ -719,4 +777,5 @@ def _extract_gps_track_from_quicktime(
             alt_tag=f"{namespace}:GPSAltitude",
             direction_tag=f"{namespace}:GPSTrack",
             record_start_tag=f"{namespace}:SampleTime",
+            far_points=self.far_gps_points,
         )

mapillary_tools/geotag/video_extractors/exiftool.py

@@ -16,7 +16,7 @@
 else:
     from typing_extensions import override
 
-from ... import exceptions, exiftool_read_video, geo, telemetry, types, utils
+from ... import constants, exceptions, exiftool_read_video, geo, telemetry, types, utils
 from ...gpmf import gpmf_gps_filter
 from .base import BaseVideoExtractor
 from .gpx import describe_long_track
@@ -47,6 +47,9 @@ def extract(self) -> types.VideoMetadata:
 
         points = exif.extract_gps_track()
 
+        if exif.far_gps_points:
+            self._warn_of_far_points(len(exif.far_gps_points), len(points))
+
         # ExifTool has no idea if GPS is not found or found but empty
         if is_gopro:
             if not points:
@@ -89,6 +92,18 @@ def extract(self) -> types.VideoMetadata:
 
         return video_metadata
 
+    def _warn_of_far_points(self, far_count: int, kept_count: int) -> None:
+        LOG.warning(
+            "%d GPS %s of %s %s left out: %s more than %g s from those of the other %s",
+            far_count,
+            "point" if far_count == 1 else "points",
+            self.video_path.name,
+            "was" if far_count == 1 else "were",
+            "its GPS timestamp is" if far_count == 1 else "their GPS timestamps are",
+            constants.MAX_GPX_GAP_SECONDS,
+            "point" if kept_count == 1 else f"{kept_count} points",
+        )
+
     def _warn_of_a_long_track(self, points: T.Sequence[geo.Point]) -> None:
         """
         Warn of a track read from another file that runs on for longer than

tests/unit/test_exiftool_read_video.py

@@ -6,7 +6,9 @@
 from __future__ import annotations
 
 import dataclasses
+import logging
 import xml.etree.ElementTree as ET
+from pathlib import Path
 
 import pytest
 from mapillary_tools import constants
@@ -22,6 +24,7 @@
     ExifToolReadVideo,
     expand_tag,
 )
+from mapillary_tools.geotag.video_extractors.exiftool import VideoExifToolExtractor
 from mapillary_tools.gpmf.gpmf_gps_filter import remove_noisy_points
 from mapillary_tools.telemetry import GPSFix, GPSPoint
 
@@ -440,6 +443,28 @@ def test_basic_quicktime_track(self):
         for p in track:
             assert p.epoch_time is not None
 
+    def test_far_timestamp_left_out_before_times_start_at_0(self):
+        """A stale first GPS timestamp does not push the others into the video"""
+        texts = {
+            expand_tag("QuickTime:GPSLongitude"): ["28.1", "28.2", "28.3"],
+            expand_tag("QuickTime:GPSLatitude"): ["37.1", "37.2", "37.3"],
+            expand_tag("QuickTime:GPSDateTime"): [
+                "2019:09:01 12:14:30.00Z",
+                "2019:09:02 10:23:29.00Z",
+                "2019:09:02 10:23:30.00Z",
+            ],
+        }
+        far_points: list[GPSPoint] = []
+        track = _aggregate_gps_track(
+            texts,
+            time_tag="QuickTime:GPSDateTime",
+            lon_tag="QuickTime:GPSLongitude",
+            lat_tag="QuickTime:GPSLatitude",
+            far_points=far_points,
+        )
+        assert [(p.time, p.lat) for p in track] == [(0.0, 37.2), (1.0, 37.3)]
+        assert [p.lat for p in far_points] == [37.1]
+
     def test_mismatched_lon_lat_returns_empty(self):
         texts = {
             expand_tag("QuickTime:GPSLongitude"): ["28.0", "29.0"],
@@ -1509,6 +1534,112 @@ def test_a_record_without_a_time_keeps_the_next_time_off_it(self):
             (2.0, 37.4, 28.4),
         ]
 
+    @staticmethod
+    def _records_xml(records: list[tuple[str, float]], copies: int = 1) -> str:
+        body = "".join(
+            f" <QuickTime:GPSDateTime>{time}</QuickTime:GPSDateTime>\n"
+            f" <QuickTime:GPSLatitude>{lat}</QuickTime:GPSLatitude>\n"
+            " <QuickTime:GPSLongitude>28.2</QuickTime:GPSLongitude>\n"
+            for time, lat in records
+        )
+        return f"""\
+<?xml version='1.0' encoding='UTF-8'?>
+<rdf:RDF xmlns:rdf='http://www.w3.org/1999/02/22-rdf-syntax-ns#'>
+<rdf:Description rdf:about='/tmp/test.mp4'
+ xmlns:QuickTime='http://ns.exiftool.org/QuickTime/QuickTime/1.0/'>
+{body * copies}</rdf:Description>
+</rdf:RDF>
+"""
+
+    def test_a_stale_first_timestamp_is_left_out(self):
+        """
+        The first GPS timestamp, 22.1 hours older than the others, no longer
+        puts every other point 22.1 hours into the video
+        """
+        xml = self._records_xml(
+            [
+                ("2019:09:01 12:14:30.00Z", 37.1),
+                ("2019:09:02 10:23:29.00Z", 37.2),
+                ("2019:09:02 10:23:30.00Z", 37.3),
+            ]
+        )
+        reader = ExifToolReadVideo(_etree_from_xml(xml))
+        track = reader.extract_gps_track()
+        assert [(p.time, p.lat) for p in track] == [(0.0, 37.2), (1.0, 37.3)]
+        assert [p.lat for p in reader.far_gps_points] == [37.1]
+
+    def test_a_stale_timestamp_in_one_copy_of_a_doubled_log(self):
+        """
+        A BlackVue lists its GPS log twice; the copy of the stale point keeps
+        its right timestamp, and the other points are not doubled
+        """
+        records = [
+            ("2019:09:02 10:23:28.00Z", 37.1),
+            ("2019:09:02 10:23:29.00Z", 37.2),
+            ("2019:09:02 10:23:30.00Z", 37.3),
+        ]
+        xml = self._records_xml(records, copies=2).replace(
+            "2019:09:02 10:23:28.00Z", "2019:09:01 12:14:30.00Z", 1
+        )
+        reader = ExifToolReadVideo(_etree_from_xml(xml))
+        track = reader.extract_gps_track()
+        assert [(p.time, p.lat) for p in track] == [
+            (0.0, 37.1),
+            (1.0, 37.2),
+            (2.0, 37.3),
+        ]
+        assert len(reader.far_gps_points) == 1
+
+    def test_a_real_gap_keeps_the_larger_segment(self):
+        xml = self._records_xml(
+            [
+                ("2019:09:02 10:00:00.00Z", 37.1),
+                ("2019:09:02 10:00:01.00Z", 37.2),
+                ("2019:09:02 11:30:00.00Z", 37.3),
+            ]
+        )
+        reader = ExifToolReadVideo(_etree_from_xml(xml))
+        track = reader.extract_gps_track()
+        assert [(p.time, p.lat) for p in track] == [(0.0, 37.1), (1.0, 37.2)]
+        assert [p.lat for p in reader.far_gps_points] == [37.3]
+
+    def test_a_gap_of_the_limit_is_kept(self, monkeypatch):
+        records = [
+            ("2019:09:02 10:00:00.00Z", 37.1),
+            ("2019:09:02 11:00:00.00Z", 37.2),
+            ("2019:09:02 11:00:01.00Z", 37.3),
+        ]
+        reader = ExifToolReadVideo(_etree_from_xml(self._records_xml(records)))
+        assert [p.time for p in reader.extract_gps_track()] == [0.0, 3600.0, 3601.0]
+        assert reader.far_gps_points == []
+
+        # The limit is MAPILLARY_TOOLS_MAX_GPX_GAP_SECONDS
+        monkeypatch.setattr(constants, "MAX_GPX_GAP_SECONDS", 60)
+        assert [p.time for p in reader.extract_gps_track()] == [0.0, 1.0]
+        assert [p.lat for p in reader.far_gps_points] == [37.1]
+
+    def test_the_extractor_warns_once_of_the_points_left_out(
+        self, tmp_path: Path, caplog
+    ):
+        video_path = tmp_path / "video.mp4"
+        video_path.write_bytes(b"")
+        xml = self._records_xml(
+            [
+                ("2019:09:01 12:14:30.00Z", 37.1),
+                ("2019:09:02 10:23:29.00Z", 37.2),
+                ("2019:09:02 10:23:30.00Z", 37.3),
+            ]
+        )
+        element = _etree_from_xml(xml).getroot()
+        assert element is not None
+        with caplog.at_level(logging.WARNING):
+            metadata = VideoExifToolExtractor(video_path, element).extract()
+        assert [p.time for p in metadata.points] == [0.0, 1.0]
+        assert [m for m in caplog.messages if "left out" in m] == [
+            "1 GPS point of video.mp4 was left out: its GPS timestamp is more "
+            "than 3600 s from those of the other 2 points"
+        ]
+
     def test_records_without_sample_times(self):
         """Insta360 lists its records with no SampleTime between them."""
         xml = """\