Conversation
Attaching a GPX is the documented escape hatch for a video with bad
embedded GPS, but for a GoPro whose GPS is rejected as noise the upload
still fails with
MapillaryGPSNoiseError: GPS is too noisy
no matter what GPX is supplied. Reported against a GoPro MAX2 .360
recorded with no GPS fix: all 32 GPMF points carry fix=NO_FIX and a DoP
of 2139 (the limit is 1000), so remove_noisy_points() drops every one.
This used to work. Before the geotag refactor, NativeVideoExtractor
returned an ErrorMetadata value and GPXVideoExtractor fell back to the
GPX on *any* failure. It now raises instead, and only one of the three
"no usable GPS" errors was being caught:
except exceptions.MapillaryVideoGPSNotFoundError as ex:
MapillaryGPSNoiseError and MapillaryGPXEmptyError are siblings of that
class rather than subclasses, so they escape the handler and fail the
whole video. That also explains the reporter's observation that the
same GPX works on a video with no embedded GPS at all: that path raises
MapillaryVideoGPSNotFoundError, which is caught.
Rather than only widening the except clause, stop filtering in the first
place when the caller is the GPX path. The noise filter is a quality
gate on the track we are about to publish; once a GPX replaces that
track, the video's own GPS is just a source of make/model and of a clock
to sync against, and neither is improved by discarding points. Widening
the except clause alone would work, but it drops the video into the
bare-VIDEO fallback and loses filetype=gopro, make/model, and the sync
anchor -- the GPX would be rebased to 0 instead of to its real +2.0s
offset. The timestamps are still good with no fix, so they still sync.
The except clause is widened as well, for the genuinely empty case where
there is no clock to recover.
The same bug had a second instance in factory._is_reprocessable(), which
also listed only MapillaryVideoGPSNotFoundError. Chaining sources, as in
--geotag_source native --geotag_source gpx
failed at the native stage and never reached the GPX. Unusable GPS in
one source is exactly what a later source is there to replace.
The gate itself is unchanged: a noisy video with no GPX supplied is
still rejected with "GPS is too noisy".
Verified end to end on the reported file. Processing now reports
"1 gopro read / ready", and a dry-run upload produces an mp4 whose CAMM
track carries the GPX coordinates at t=2.0, 4.0, ... with GoPro/MAX2
preserved. Of the 11 new tests, 7 fail without this change and the 4
guard tests pass either way.
Making MapillaryGPSNoiseError reprocessable was too broad: the default chain is native, exiftool_runtime so a video that the native parser had just rejected as noise fell through to exiftool, and `mapillary_tools process` with no flags at all started *accepting* the very file this branch is about. The two readers disagree because they do not see the same fields. For the reported capture the native GPMF parser reads a DoP of ~2100 against a limit of 1000 and drops all 32 points, while exiftool reports no DoP at all (precision=None), so remove_noisy_points() skips the DoP test and keeps the 24 points that have a 3D fix. exiftool losing GPSP is a pre-existing bug, and `--geotag_source exiftool_runtime` already accepts this file on main; what changed here was only that the default chain started reaching it. Unusable GPS is a verdict on the data, not on the reader that reported it, so only a source that supplies GPS from *outside* the video can overturn it. Gate the fall-through on the remaining sources: GPX and NMEA can rescue the file, another reader of the same embedded telemetry cannot. MapillaryVideoGPSNotFoundError is unaffected, since "could not read it" really is a verdict on the reader and retrying is fair. Verified on the reported file: process (default) -> GPS is too noisy process --geotag_source native -> GPS is too noisy process --geotag_source gpx -> 1 gopro ready process --geotag_source native,gpx -> 1 gopro ready
|
Pushed 898988b to fix a regression in the first commit — thanks to @caglarpir for catching it. Making The two readers disagree because they do not see the same fields:
exiftool losing The fix: unusable GPS is a verdict on the data, not on the reader that reported it, so only a source supplying GPS from outside the video can overturn it. The fall-through is now gated on the remaining sources — GPX and NMEA can rescue the file, another reader of the same embedded telemetry cannot. Verified on the reported file: Tests are up to 20 (from 11), including one pinning the default chain specifically. 705 unit + 52 integration pass, mypy clean. |
Correction to the verification in the description aboveThe description offers this as evidence the fix works:
That is not a clean result, and I should not have presented it as one. The clip is 1.24 s long, so a track starting at It isn't truncated — it is a 12.5× timelapse, compressing 15.5 s of wall clock into 1.24 s of video. So the GPX (15:39:43–15:39:55) sits comfortably inside the video's real capture window; it is the mapping onto the video's time axis that is wrong:
What still standsEverything the PR actually claims to change:
What it does not doIt does not make the resulting track correctly timed on a timelapse. That is a separate fix, and this PR is a prerequisite for it: keeping the noisy points preserves the GPS clock that the correct mapping needs to invert. Same family as #832, which fixes when a video starts; this is how fast its clock runs. Worth noting that #832's reported case failed as |
1 participant
Problem
Attaching a GPX is the documented escape hatch for a video with bad embedded GPS, but for a GoPro whose GPS is rejected as noise the upload still fails with
no matter what GPX is supplied.
Reported against a GoPro MAX2
.360recorded with no GPS fix: all 32 GPMF points carryfix=NO_FIXand a DoP of 2139 (the limit is 1000), soremove_noisy_points()drops every one.Cause
This used to work. Before the geotag refactor,
NativeVideoExtractorreturned anErrorMetadatavalue andGPXVideoExtractorfell back to the GPX on any failure. It now raises instead, and only one of the three "no usable GPS" errors was being caught:MapillaryGPSNoiseErrorandMapillaryGPXEmptyErrorare siblings of that class rather than subclasses, so they escape the handler and fail the whole video.That also explains the reporter's observation that the same GPX works on a video with no embedded GPS at all: that path raises
MapillaryVideoGPSNotFoundError, which is caught.The same bug had a second instance in
factory._is_reprocessable(), which also listed onlyMapillaryVideoGPSNotFoundError. Chaining sources, as in--geotag_source native --geotag_source gpx, failed at the native stage and never reached the GPX.Fix
Rather than only widening the
exceptclause, stop filtering in the first place when the caller is the GPX path. The noise filter is a quality gate on the track we are about to publish; once a GPX replaces that track, the video's own GPS is just a source of make/model and of a clock to sync against, and neither is improved by discarding points.Widening the
exceptclause alone would work, but it drops the video into the bare-VIDEOfallback and losesfiletype=gopro, make/model, and the sync anchor — the GPX would be rebased to 0 instead of to its real +2.0s offset. The timestamps are still good with no fix, so they still sync. Theexceptclause is widened as well, for the genuinely empty case where there is no clock to recover._is_reprocessable()now treats unusable GPS as reprocessable, since that is exactly what a later source is there to replace.The gate itself is unchanged: a noisy video with no GPX supplied is still rejected with "GPS is too noisy".
Verification
Important
Correction: the
t=2.0, 4.0, ...figure below is not evidence of success — the clip is a 12.5x timelapse and that track lands past the end of its 1.24 s of video. See this comment for what still stands and what does not.Verified end to end on the reported file. Processing now reports
1 gopro read / ready, and a dry-run upload produces an mp4 whose CAMM track carries the GPX coordinates att=2.0, 4.0, ...withGoPro/MAX2preserved.Of the 11 new tests in
tests/unit/test_gpx_over_noisy_gps.py, 7 fail without this change and the 4 guard tests pass either way. Full suite: 696 unit + 52 integration tests pass, mypy clean.