Read GPS DoP from GPS9 cameras, which report GPSDOP - #840
Merged
Merged
Conversation
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The exiftool reader looks for
GPSHPositioningErroronly. Cameras with GPS9 telemetry (GoPro MAX 2, HERO11 and newer) do not write that tag — they writeGPSDOP.So every point came back with
precision=None, the DoP test inremove_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 disagree completely:This is why
--geotag_source exiftool_runtimeaccepts the noisy MAX 2 clip from the report that--geotag_source nativerejects.Fix
The tags are mutually exclusive per telemetry generation, and both scale by 100 — though for different reasons:
GPSDOPis a dilution of precision, which is what GPMF'sGPSPholds, whileGPSHPositioningErroris a horizontal error in metres and only approximates it. ReadGPSDOPfirst, fall back toGPSHPositioningError:GPSDOP1.85 / 2.07GPSHPositioningError99.99GPS5 cameras are untouched.
Relationship to #831
This hole pre-dates #831 —
mainforced through--geotag_source exiftool_runtimealready yields the unfiltered track. #831 briefly made the default chain reach it, and was revised (898988b) so it no longer does. This PR closes the hole itself, independently.Verification
Tag reading verified on real MAX 2 footage. The rejection behaviour is covered by unit tests rather than a fixture: the MAX 2 files available locally all happen to have good DoP, and the reported noisy clip is not on this machine.
Full suite passes, mypy and ruff clean. (
test_persistent_cache::test_multiprocess_shared_cache_comprehensiveis flaky under parallel load on this machine — it passes in isolation both with and without this change.)