Repository navigation
Let a GPX override a video whose own GPS is unusable - #831
Conversation
|
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
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 mapillary_tools process VIDEO --geotag_source gpx --geotag_source_path GPX still failed with "GPS is too noisy", whatever the GPX. Reported against a GoPro MAX2 .360 timelapse: all 32 of its GPS points report a DoP of 21.0 to 21.4 against a limit of 10, so remove_noisy_points() drops every one of them, the 24 with a 3D fix included. This used to work. Before mapillary#743, NativeVideoExtractor returned an ErrorMetadata and GPXVideoExtractor fell back to the GPX on any of them. It now raises instead, and the GPX extractor catches only MapillaryVideoGPSNotFoundError. MapillaryGPXEmptyError and MapillaryGPSNoiseError are siblings of that class rather than subclasses, so they escaped and failed the video. That is also why the same GPX worked on a video with no GPS at all. Make the three "no usable GPS in the video" errors subclasses of a new MapillaryVideoGPSUnusableError and catch that, so that a fourth cannot be missed the same way. They stay MapillaryDescriptionError subclasses, so nothing that catches them changes. A video whose GPS is unusable is geotagged from the GPX as one without GPS is: synced from the start of the video, as a plain VIDEO. The noise filter itself is unchanged: without a GPX, the video is still rejected with "GPS is too noisy".
The previous commit lets --geotag_source gpx geotag a video whose own GPS is empty or too noisy, but a chain such as --geotag_source native --geotag_source gpx still stopped at native: only MapillaryVideoGPSNotFoundError passes a video on to the next source. Passing every unusable-GPS error on is not enough by itself: - The default chain is native, exiftool_runtime. A video that native rejected would go on to exiftool, which reads the same telemetry and can only repeat the verdict or overturn it by reading the data differently: until mapillary#840 taught it the GPS9 DoP, exiftool accepted the reported clip that native rejects. Where exiftool is not installed, "exiftool not found" would replace "GPS is too noisy". - A source that cannot geotag videos (NMEA, EXIF) skips them, and a video left for the next source is then dropped from the results: no error, and exit code 0. Main already drops a video without GPS this way with --geotag_source native,nmea or native,exif. So once a video's own GPS is rejected as empty or noisy, only a source that reads GPS from outside the video (GPX, NMEA) is given it. Every error is held until a later source geotags the path, and reported if none does, so no source can drop a video by skipping it. A fallback that finds nothing, such as a missing GPX, does not replace the reason the video's own GPS was rejected; any other error from it does. Held errors are reported after the other results, so the order of the results can change.
898988b to
12f7d2f
Compare
|
Rewritten on current main and narrowed to the original fix; the description is updated. The two comments above describe an earlier version: its look-ahead in Syncing the reported timelapse correctly is now #849 (stacked on this and #847), and rejecting a GPX that misses the video is #848. |
main reworked GPXVideoExtractor (mapillary#831, mapillary#847, mapillary#849): it syncs a GPX through a VideoClock, with a rate for timelapses, falls back to the start of the video when the video's GPS has no usable clock, and reports a GPX it cannot parse. This branch changed the same method, so take main's and re-apply this branch's changes to it: - Drop GPX points timestamped before 1980, and sort the rest. - Place the GPX by --video_start_time wherever the video has no clock to sync it with: no usable GPS, as before, and now also GPS without a clock, such as noise with a placeholder date, which main places at the start of the video. - Apply --interpolation_offset_time after the sync. The offset is in GPS seconds, so with a timelapse clock the track moves by offset / rate seconds of video; by the whole offset, frames would be picked offset * (rate - 1) seconds of track away from where they are geotagged. In factory.py, keep both main's EXTERNAL_GPS_SOURCES and this branch's _ALL_VIDEO_TYPES. Tests cover the clockless placement and the timelapse offset.
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,
--geotag_source gpxstill fails withMapillaryGPSNoiseError: GPS is too noisy, whatever the GPX.Reported against a GoPro MAX2
.360timelapse: all 32 of its GPS points report a DoP of 21.0 to 21.4 against a limit of 10, soremove_noisy_points()drops every one of them, the 24 with a 3D fix included.A GoPro HERO12, which has no GPS, fails the same way with any GPX: its telemetry has a GPS stream with no points, and the video is rejected with
MapillaryGPXEmptyError: Empty GPS data found.Cause
Before #743, the GPX extractor fell back to the GPX on any error from the native extractor. Now it catches only
MapillaryVideoGPSNotFoundError, andMapillaryGPXEmptyErrorandMapillaryGPSNoiseErrorare siblings of that class, not subclasses. The geotag source chain has the same gap: only "not found" passes a video on to the next source, so--geotag_source native,gpxstops at native.Changes
MapillaryVideoGPSUnusableError, which the GPX extractor catches. Such a video is geotagged from the GPX as one without GPS is: synced from the start of the video, as a plainVIDEO.native,nmeaornative,exif, with exit code 0.The noise filter is unchanged: without a GPX, the video is still rejected.
Not in this PR
Verification
processon the reported clip and its GPX:nativeexiftool_runtimegpxnative,gpxnative,exiftool_runtime,gpxnative,nmeaA noisy GoPro HERO8 sample behaves the same. A HERO12 video with the GPX recorded alongside it fails on main with "Empty GPS data found", and here is geotagged with all 3,810 points of the GPX. A GoPro MAX clip with good GPS and a GPX syncs exactly as on main, and a Sony clip without GPS now reports "No GPS data found" with
native,nmeaandnative,exifinstead of vanishing. Across 168 runs over sample files from over 20 cameras, with this PR and the two stacked on it applied, that HERO12 run is the only output that differs from main.Unit: 727 passed, 18 skipped, 1 xfailed. Integration: 56 passed.
mypy,ruff checkandruff format --checkare clean. 11 of the 24 new tests fail on main; the others guard what must not change.