From 32447a3e9f5693a8c28fcf2851374d191eebd8ca Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Tue, 22 Sep 2026 16:16:22 +0200 Subject: [PATCH 1/3] Read GPS DoP from GPS9 cameras, which report GPSDOP The exiftool reader looked for GPSHPositioningError only. Cameras with GPS9 telemetry (GoPro MAX 2, HERO11 and newer) do not write that tag; they write GPSDOP. So every point came back with precision=None, the DoP test in remove_noisy_points() was skipped entirely, and the reader accepted tracks the native GPMF parser rejects as noise. On a MAX 2 clip the two readers disagreed completely: native n=267 precision=185.0 / 207.0 exiftool n=267 precision=None (x267) The tags are mutually exclusive per telemetry generation and both scale by 100, though for different reasons: GPSDOP is a dilution of precision, which is what GPMF's GPSP holds, while GPSHPositioningError is a horizontal error in meters and only approximates it. Read GPSDOP first and fall back to GPSHPositioningError, so GPS5 cameras are untouched: MAX 2 (GPS9) GPSDOP 1.85, 2.07 -> 185, 207 matches native exactly hero8 (GPS5) GPSHPositioningError 99.99 -> 9999 unchanged This is why "--geotag_source exiftool_runtime" accepts the noisy MAX 2 clip from T288698491 that "--geotag_source native" rejects. The hole pre-dates PR 831; that PR only made the default chain reach it, and was revised so it no longer does. Verified on real MAX 2 footage for the tag reading. The rejection behaviour is covered by unit tests rather than a fixture: the MAX 2 files available locally all have good DoP, and the reported noisy clip is not on this machine. --- mapillary_tools/exiftool_read_video.py | 28 ++++++++----- tests/unit/test_exiftool_read_video.py | 55 +++++++++++++++++++++++++- 2 files changed, 72 insertions(+), 11 deletions(-) diff --git a/mapillary_tools/exiftool_read_video.py b/mapillary_tools/exiftool_read_video.py index 4257abca..722bd552 100644 --- a/mapillary_tools/exiftool_read_video.py +++ b/mapillary_tools/exiftool_read_video.py @@ -344,7 +344,7 @@ def _aggregate_gps_track_by_sample_time( direction_tag: str | None = None, ground_speed_tag: str | None = None, gps_fix_tag: str | None = None, - gps_precision_tag: str | None = None, + gps_precision_tags: T.Sequence[str] = (), ) -> list[GPSPoint]: track: list[GPSPoint] = [] @@ -352,9 +352,7 @@ def _aggregate_gps_track_by_sample_time( if gps_fix_tag is not None: expanded_gps_fix_tag = expand_tag(gps_fix_tag) - expanded_gps_precision_tag = None - if gps_precision_tag is not None: - expanded_gps_precision_tag = expand_tag(gps_precision_tag) + expanded_gps_precision_tags = [expand_tag(tag) for tag in gps_precision_tags] for sample_time, sample_duration, elements in sample_iterator: texts_by_tag = _index_text_by_tag(elements) @@ -369,16 +367,19 @@ def _aggregate_gps_track_by_sample_time( gps_fix = None gps_precision = None - if expanded_gps_precision_tag is not None: + for expanded_gps_precision_tag in expanded_gps_precision_tags: gps_precision_texts = texts_by_tag.get(expanded_gps_precision_tag) if gps_precision_texts: gps_precision = _maybe_float(gps_precision_texts[0]) if gps_precision is not None: - # GPS precision in ExifTool (i.e. horizontal positioning error) are in meters. - # https://exiftool.org/forum/index.php?topic=11565.0 - # Here we multiply by 100 to be compatible with the GPSP - # described in https://github.com/gopro/gpmf-parser + # Both tags are scaled by 100 to match the GPSP described in + # https://github.com/gopro/gpmf-parser, but for different + # reasons: GPSDOP is a dilution of precision, which is what + # GPSP holds, while GPSHPositioningError is a horizontal + # positioning error in meters and only an approximation of + # it. https://exiftool.org/forum/index.php?topic=11565.0 gps_precision = gps_precision * 100 + break # Aggregate GPS points in the sample points = _aggregate_gps_track( @@ -547,7 +548,14 @@ def _extract_gps_track_from_track(self) -> list[GPSPoint]: direction_tag=f"{track_ns}:GPSTrack", ground_speed_tag=f"{track_ns}:GPSSpeed", gps_fix_tag=f"{track_ns}:GPSMeasureMode", - gps_precision_tag=f"{track_ns}:GPSHPositioningError", + # Which one a camera writes depends on its telemetry + # format: GPS9 (GoPro MAX 2, HERO11 and newer) reports + # GPSDOP, while GPS5 (HERO10 and older) reports + # GPSHPositioningError. Prefer the true DOP when present. + gps_precision_tags=[ + f"{track_ns}:GPSDOP", + f"{track_ns}:GPSHPositioningError", + ], ) if track: return track diff --git a/tests/unit/test_exiftool_read_video.py b/tests/unit/test_exiftool_read_video.py index fe8c88a0..5afbbb55 100644 --- a/tests/unit/test_exiftool_read_video.py +++ b/tests/unit/test_exiftool_read_video.py @@ -8,6 +8,7 @@ import xml.etree.ElementTree as ET import pytest +from mapillary_tools import constants from mapillary_tools.exiftool_read_video import ( _aggregate_gps_track, _aggregate_gps_track_by_sample_time, @@ -785,11 +786,63 @@ def test_gps_precision_scaled(self): sample_iterator, lon_tag=f"{track_ns}:GPSLongitude", lat_tag=f"{track_ns}:GPSLatitude", - gps_precision_tag=f"{track_ns}:GPSHPositioningError", + gps_precision_tags=[f"{track_ns}:GPSHPositioningError"], ) assert len(track) == 1 assert track[0].precision == pytest.approx(219.0) + def _precision_from(self, tags: dict[str, str]) -> float | None: + """Read precision from a sample carrying the given DoP-ish tags.""" + track_ns = "Track1" + elements = [ + _make_element(f"{track_ns}:GPSLongitude", "8.0"), + _make_element(f"{track_ns}:GPSLatitude", "47.0"), + *(_make_element(f"{track_ns}:{tag}", value) for tag, value in tags.items()), + ] + track = _aggregate_gps_track_by_sample_time( + [(0.0, 1.0, elements)], + lon_tag=f"{track_ns}:GPSLongitude", + lat_tag=f"{track_ns}:GPSLatitude", + gps_precision_tags=[ + f"{track_ns}:GPSDOP", + f"{track_ns}:GPSHPositioningError", + ], + ) + assert len(track) == 1 + return track[0].precision + + def test_gps9_cameras_report_dop_instead(self): + """ + GPS9 telemetry (GoPro MAX 2, HERO11+) reports GPSDOP and no + GPSHPositioningError, so reading only the latter loses precision + entirely and the noise filter silently keeps a track it should drop. + """ + assert self._precision_from({"GPSDOP": "1.85"}) == pytest.approx(185.0) + + def test_gps5_cameras_still_work(self): + """HERO10 and older report only GPSHPositioningError.""" + assert self._precision_from({"GPSHPositioningError": "99.99"}) == pytest.approx( + 9999.0 + ) + + def test_dop_wins_when_a_camera_reports_both(self): + """GPSDOP is the quantity GPSP holds; the error in meters approximates it.""" + assert self._precision_from( + {"GPSDOP": "1.85", "GPSHPositioningError": "99.99"} + ) == pytest.approx(185.0) + + def test_no_precision_tags_at_all(self): + assert self._precision_from({}) is None + + def test_a_noisy_gps9_track_is_now_filtered(self): + """ + The reported failure: a MAX 2 whose DoP is far over the limit was + accepted by the exiftool reader while the native parser rejected it. + """ + precision = self._precision_from({"GPSDOP": "21.39"}) + assert precision == pytest.approx(2139.0) + assert precision > constants.GOPRO_MAX_DOP100 + def test_multiple_points_per_sample_get_interpolated_time(self): """Multiple GPS points within a single sample get evenly spaced times.""" track_ns = "Track1" From b2cb488beeac12af551c0b91b5e3ae66ea536584 Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Wed, 23 Sep 2026 14:47:06 +0200 Subject: [PATCH 2/3] Correct why both DoP tags scale by 100 The comment claimed the two tags scale by 100 "for different reasons", and that GPSHPositioningError is a horizontal error in meters that only approximates a dilution of precision. That is wrong, and it makes the GPS5 branch look like a soft approximation when it is exact. ExifTool's GoPro GPSHPositioningError is not the EXIF tag of that name. It is GPMF's GPSP, renamed, with a ValueConv that divides by 100 (GoPro.pm): GPSP => { Name => 'GPSHPositioningError', ValueConv => '$val / 100', }, GPS9's GPSDOP is the same quantity reached the same way: index 7 of the GPS9 table is a bare tag, scaled down by its SCAL entry of 100 (SCAL=10000000 10000000 1000 1000 100 1 1000 100 1). So both tags are a dilution of precision that ExifTool has already divided by 100, and multiplying by 100 recovers the raw GPMF value the native parser stores, byte for byte, on both telemetry generations. The noise limit of 1000 means DoP 10 either way, not "10 m on GPS5 and DoP 10 on GPS9". No behaviour change: the arithmetic was right, only the explanation was wrong. The same claim appears in the body of 32447a3 and should be dropped if these commits are squashed. --- mapillary_tools/exiftool_read_video.py | 14 ++++++++------ tests/unit/test_exiftool_read_video.py | 3 ++- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/mapillary_tools/exiftool_read_video.py b/mapillary_tools/exiftool_read_video.py index 722bd552..f666944a 100644 --- a/mapillary_tools/exiftool_read_video.py +++ b/mapillary_tools/exiftool_read_video.py @@ -372,12 +372,14 @@ def _aggregate_gps_track_by_sample_time( if gps_precision_texts: gps_precision = _maybe_float(gps_precision_texts[0]) if gps_precision is not None: - # Both tags are scaled by 100 to match the GPSP described in - # https://github.com/gopro/gpmf-parser, but for different - # reasons: GPSDOP is a dilution of precision, which is what - # GPSP holds, while GPSHPositioningError is a horizontal - # positioning error in meters and only an approximation of - # it. https://exiftool.org/forum/index.php?topic=11565.0 + # Both tags hold the dilution of precision that GPSP holds, + # already divided by 100 by ExifTool, so scaling back up + # recovers the raw GPMF value the native parser stores: + # GPS9 reports it as GPSDOP, divided by its SCAL entry of + # 100, and GPS5 as GPSP, which ExifTool renames to + # GPSHPositioningError and applies ValueConv $val/100 to. + # Despite that name it is not the EXIF horizontal error in + # meters. https://github.com/gopro/gpmf-parser gps_precision = gps_precision * 100 break diff --git a/tests/unit/test_exiftool_read_video.py b/tests/unit/test_exiftool_read_video.py index 5afbbb55..fba134c3 100644 --- a/tests/unit/test_exiftool_read_video.py +++ b/tests/unit/test_exiftool_read_video.py @@ -826,7 +826,8 @@ def test_gps5_cameras_still_work(self): ) def test_dop_wins_when_a_camera_reports_both(self): - """GPSDOP is the quantity GPSP holds; the error in meters approximates it.""" + """Both spellings carry the quantity GPSP holds, so the order only + matters for a file reporting both; GPSDOP, the GPS9 one, is read first.""" assert self._precision_from( {"GPSDOP": "1.85", "GPSHPositioningError": "99.99"} ) == pytest.approx(185.0) From e3aec1ee316b0b5798c9498bf18fd2b9487873ca Mon Sep 17 00:00:00 2001 From: Caglar Pir Date: Wed, 23 Sep 2026 14:53:09 +0200 Subject: [PATCH 3/3] Make the DoP tests actually exercise the filter and the reader Two gaps, both of which let this change be reverted with a green suite. test_a_noisy_gps9_track_is_now_filtered never filtered anything. It asserted 2139 > GOPRO_MAX_DOP100, which is arithmetic on two constants, and never called remove_noisy_points(). Reading GPSDOP is only worth something if the noise filter then drops the track, so assert that end: the noisy track empties, and the same track with precision left unread -- which is what the filter saw before this change -- survives. A clean MAX 2 DoP of 1.85 is pinned as surviving too, so the gate cannot be tightened into rejecting healthy footage without a failure. The track fixture is two points on purpose: remove_outliers() returns early below two distances, so the DoP gate is what the assertions measure rather than the outlier pass. Second gap: every DoP test drove _aggregate_gps_track_by_sample_time() with its own tag list, so none of them touched the list the reader actually asks for. Deleting GPSDOP from extract_gps_track() left all 104 tests passing -- the whole change reverted, suite green. Added a GPS9 XML fixture and a test through the real entry point; that mutation now fails. No production change. --- tests/unit/test_exiftool_read_video.py | 107 ++++++++++++++++++++++++- 1 file changed, 104 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_exiftool_read_video.py b/tests/unit/test_exiftool_read_video.py index fba134c3..9756c806 100644 --- a/tests/unit/test_exiftool_read_video.py +++ b/tests/unit/test_exiftool_read_video.py @@ -5,6 +5,7 @@ from __future__ import annotations +import dataclasses import xml.etree.ElementTree as ET import pytest @@ -21,6 +22,7 @@ ExifToolReadVideo, expand_tag, ) +from mapillary_tools.gpmf.gpmf_gps_filter import remove_noisy_points from mapillary_tools.telemetry import GPSFix, GPSPoint @@ -122,6 +124,36 @@ def _make_element(tag: str, text: str) -> ET.Element: """ +# GPS9 telemetry (GoPro MAX 2, HERO11+): GPSDOP in place of +# GPSHPositioningError. DoP values are from a real MAX 2 clip. +GPS9_XML = """\ + + + + GoPro + GoPro Max 2 + 0 + 1.001 + 47.359832 + 8.522706 + 414.9 + 2026:07:31 00:25:23.200Z + 3 + 1.85 + 1.001 + 1.001 + 47.359810 + 8.522680 + 415.2 + 2026:07:31 00:25:24.200Z + 3 + 2.07 + + +""" + INSTA360_XML = """\ @@ -835,14 +867,67 @@ def test_dop_wins_when_a_camera_reports_both(self): def test_no_precision_tags_at_all(self): assert self._precision_from({}) is None + def _track_from(self, tags: dict[str, str]) -> list[GPSPoint]: + """ + Build a two-sample track carrying the given DoP-ish tags. + + Two points keep remove_outliers() a no-op -- it returns early below + two distances -- so the DoP gate is what the assertions measure. + """ + track_ns = "Track1" + sample_iterator = [ + ( + float(idx), + 1.0, + [ + _make_element(f"{track_ns}:GPSLongitude", f"{8.0 + idx * 0.0001}"), + _make_element(f"{track_ns}:GPSLatitude", f"{47.0 + idx * 0.0001}"), + *( + _make_element(f"{track_ns}:{tag}", value) + for tag, value in tags.items() + ), + ], + ) + for idx in range(2) + ] + return list( + _aggregate_gps_track_by_sample_time( + sample_iterator, + lon_tag=f"{track_ns}:GPSLongitude", + lat_tag=f"{track_ns}:GPSLatitude", + gps_precision_tags=[ + f"{track_ns}:GPSDOP", + f"{track_ns}:GPSHPositioningError", + ], + ) + ) + def test_a_noisy_gps9_track_is_now_filtered(self): """ The reported failure: a MAX 2 whose DoP is far over the limit was accepted by the exiftool reader while the native parser rejected it. + + Reading GPSDOP is only worth anything if the noise filter then drops + the track, so assert that end rather than the parsed number alone. """ - precision = self._precision_from({"GPSDOP": "21.39"}) - assert precision == pytest.approx(2139.0) - assert precision > constants.GOPRO_MAX_DOP100 + noisy = self._track_from({"GPSDOP": "21.39"}) + assert [p.precision for p in noisy] == [ + pytest.approx(2139.0), + pytest.approx(2139.0), + ] + assert list(remove_noisy_points(noisy)) == [] + + # Leaving the tag unread is what the filter saw before this change: + # no precision to test, so the same noisy track survives + unread = [dataclasses.replace(p, precision=None) for p in noisy] + assert len(remove_noisy_points(unread)) == 2 + + def test_a_clean_gps9_track_survives_the_filter(self): + """A healthy MAX 2 DoP is far under the limit and must not be dropped.""" + clean = self._track_from({"GPSDOP": "1.85"}) + assert clean[0].precision == pytest.approx(185.0) + assert clean[0].precision < constants.GOPRO_MAX_DOP100 + assert len(remove_noisy_points(clean)) == 2 def test_multiple_points_per_sample_get_interpolated_time(self): """Multiple GPS points within a single sample get evenly spaced times.""" @@ -1224,6 +1309,22 @@ def test_gopro_track_gps(self): assert track[0].lat == pytest.approx(47.359832) assert track[0].lon == pytest.approx(8.522706) + def test_gps9_track_carries_dop_through_extract(self): + """ + The tag list extract_gps_track() asks for has to include GPSDOP. + + The other DoP tests drive _aggregate_gps_track_by_sample_time() + with their own tag list, so dropping GPSDOP from the reader would + leave them all green. This one goes through the real entry point, + and the DoP it returns is what remove_noisy_points() gates on. + """ + reader = ExifToolReadVideo(_etree_from_xml(GPS9_XML)) + track = reader.extract_gps_track() + assert [p.precision for p in track] == [ + pytest.approx(185.0), + pytest.approx(207.0), + ] + def test_empty_gps_track(self): """When no GPS data is present, returns empty list.""" xml = """\