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 |
Distance sampling called GeotagVideosFromVideo() directly, bypassing the
geotag factory, so --geotag_source never reached it. Attaching a GPX is
the documented answer for a camera whose embedded GPS is unusable, but
the sampler needs positions just as much as geotagging does, and it only
ever looked at the video's own telemetry. The GPX was read, reported in
the log, and then ignored:
video_process V.mp4 OUT --video_sample_distance 3 \
--geotag_source gpx --geotag_source_path V.gpx
-> 0 frames
Go through factory.process() instead. The consumer was already generic:
_sample_video_stream_by_distance() takes a plain Sequence[geo.Point] and
does not care where they came from, and GPXVideoExtractor already
returns points rebased onto the video timeline. Only the producer was
hardcoded.
Routing rather than adding a fallback here keeps the source-selection
policy in one place. sample_video.py has no business re-deciding which
source may rescue a file; _build_video_geotag() stays the only map from
a source to an extractor, and chaining (native,gpx) comes for free.
Sampling defaults to NATIVE alone rather than to
DEFAULT_GEOTAG_SOURCE_OPTIONS, which continues on to exiftool_runtime.
That reader cannot see every field the noise filter rejects on, so
defaulting to the full chain would make sampling accept tracks the
native parser refuses. NATIVE-only is also exactly today's behaviour.
_parse_source_options() moves from process_geotag_properties to
geotag.factory, next to parse_source_option(), so both callers share one
implementation instead of the sampler reaching for a private helper.
commands/sample_video.py needs no change: it splats vars_args filtered
by the signature of sample_video(), so the new parameters flow through
from video_process, which registers the process command's arguments into
the same parser. The standalone sample_video command does not register
them, so they stay absent and the defaults apply.
Verified end to end:
no-GPS video + overlapping GPX 0 frames -> 86 frames
noisy GoPro + overlapping GPX 0 frames -> 299 frames (with mapillary#831)
noisy GoPro, no GPX still rejected
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.