Repository navigation
Conversation
Distance sampling warned and returned when it could not read a video's GPS, so the command exited 0 having written nothing: WARNING - GPS is too noisy ==> Processing 0 files with source gpx... ==> Validating 0 metadatas... ==> Process summary No error, no frames, no non-zero exit -- the sample directory is never created and the geotag stage then runs over zero files. The user is told the run succeeded and has nothing to upload. Reported for a GoPro MAX 2 whose embedded GPS is rejected as noise, where attaching a GPX made no difference, but nothing about it is specific to noisy GPS: a video with no GPS at all takes the same path, which is every "camera without embedded GPS, bring your own GPX" workflow. Distance sampling needs positions to decide which frames to cut, so failing to read them is a failed sample. Raise MapillaryVideoError, the same error the rest of this function already raises for an unreadable start time or a frame count mismatch, and which exits 7 rather than dumping a traceback. sample_video() already funnels sampling errors through --skip_sample_errors, so callers who want to tolerate this keep a supported way to do it and the default stops lying. Two neighbouring silent paths get the same treatment: a missing video stream, which also returned after a warning, and an empty point list, which was an assert and so disappeared under `python -O`, leaving an IndexError further down instead. Note this changes batch behaviour. Sampling a directory containing one unreadable video now aborts unless --skip_sample_errors is passed. That matches what every other sampling error in this function already does, and the alternative is continuing to hide the failure, but it is a behaviour change for callers who relied on the skip. Three integration tests covered directories containing hero8.mp4, whose 32 embedded points are all dropped by remove_noisy_points(); they were passing while it contributed no frames at all. They now pass --skip_sample_errors, which is what they always meant. A new test pins the loud behaviour, mirroring test_sample_video_without_video_time. This does not make --geotag_source reach distance sampling; that call site still hardcodes GeotagVideosFromVideo() and is a separate fix. It only stops the failure from being silent.
The raise told users the sample failed but not how to get past it, and the obvious guess is wrong: --skip_process_errors governs the later geotagging stage and does not cover sampling, so reaching for it leaves the run failing with the same message. Append the hint, matching the existing wording in process_geotag_properties.py. The message now reads: MapillaryVideoError: Unable to sample GS018205.360 by distance: GPS is too noisy. To skip these errors, specify --skip_sample_errors
Four other raises sit inside the same try block that --skip_sample_errors guards and said nothing about it: _sample_single_video_by_interval unable to extract video start time _sample_single_video_by_distance unable to extract video start time _sample_single_video_by_distance expect N samples but extracted M _sample_single_video_by_distance expect X to be Nth sample but got M Half the errors naming the flag is worse than none naming it: a user who lands on a silent one reaches for --skip_process_errors, which governs the later geotagging stage and leaves the run failing with the same message. Rather than append the hint at seven call sites, where it can drift out of sync again, construct these errors through _sampling_error(), so a raise added later cannot forget it. MapillaryFFmpegNotFoundError keeps its plain message. It is re-raised by its own handler before the skip check, so --skip_sample_errors really does not suppress it and it must not say otherwise. There is a test for that, alongside ones covering both newly hinted start-time paths.
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
caglarpir
force-pushed
the
fix-geotag-source-in-distance-sampling
branch
from
September 23, 2026 07:07
b95d573 to
fff6550
Compare
exif, nmea and the legacy gopro/blackvue/camm sources have no video extractor, so process() leaves a video they are asked about out of its result altogether. Now that distance sampling goes through the factory, that trips the sampler's assert, with a traceback, for a video with good GPS that used to sample fine: video_process max-360mode.mp4 --geotag_source exif AssertionError: expect 1 video metadata In video_process these sources are there to geotag the frames, as the legacy gopro source pointed at the video itself is. Sample by the requested sources that can read videos, or by the video's own GPS, as before, when none of them can. can_geotag_videos() asks _build_video_geotag(), so the map from source to extractor stays in one place, and also honours a source's filetypes, which can rule videos out too. With only such sources left, process() always returns the video and the assert holds.
The comment claimed a GPX sidecar is looked for next to the video when no --geotag_source_path is given, and the test was named after that, but neither is what happens: every source gets the video import path itself, as process_geotag_properties() gives it when geotagging the frames. A GPX is then read from the video file, or not found in the directory. The default is still right, for exiftool_xml, which finds the XML files in the video directory through it. A GPX beside the video would not help anyway: the geotagging stage reads its GPX from the same default path, so a GPX needs --geotag_source_path either way.
For a video with no GPS of its own, GPXVideoExtractor assumes the GPX track starts with the video, and --video_start_time never reaches it. The points keep their absolute times, so the sampler stamped each frame with the GPX start plus the frame's time, and the geotagging stage, which places the frames by those stamps, agreed. The result exited 0, looked consistent, and was wrong whenever the track did not start with the video: two clips of one drive, recorded hours apart and each given its own --video_start_time, came out as the same frames, stamped with the first minutes of the drive's GPX. Distance sampling now hands the extractor a VideoSyncOption carrying the start time the user gave, and the GPX on a video without GPS is placed by it. Only a given start time: the one probed from the container is creation_time - duration, a whole video length early on cameras whose creation_time marks the start, which would move their videos off their GPX. A video with GPS is still synced by its own GPS clock. process passes no VideoSyncOption and places a GPX exactly as before. Without a start time the track is still assumed to start with the video, but that is now checked. A track lasting more than twice as long as the video was not recorded for it alone, and the video could start anywhere along it, so the sample fails and asks for --video_start_time. A track a little longer than the video warns, since every frame is then up to that much too early along it. MapillaryGPXVideoStartUnknownError is not reprocessable: a source after the GPX in the chain could only report that the video has no GPS, hiding why the GPX was refused. A track that misses the video, such as one placed by a wrong --video_start_time, selected no frames and left an empty sample directory behind with a success exit code. That now fails the sample too. Checked on sample videos from several cameras: a clip whose GPX covers a whole drive is placed by its start time, a start time outside the GPX fails loudly, and clips whose GPX was recorded with them sample as before.
video_process registers --interpolation_offset_time and
--interpolation_use_gpx_start_time for its geotagging stage, which
applies them to the frames, and the README documents both for lining up
a GPX with a video. Distance sampling ignored them, and so picked frames
by positions along the GPX that the frames did not end up at. With an
offset, the geotagging stage moved every frame that many seconds along
the track, and pushed frames near its end past it, where they were
rejected and failed the command:
video_process V.mp4 OUT --video_start_time T --interpolation_offset_time 2 \
--geotag_source gpx --geotag_source_path V.gpx
MapillaryProcessError: Failed to process 5 files
VideoSyncOption now carries both to the GPX extractor, which applies
them as the geotagging stage will. use_gpx_start_time starts the track
with the video, whatever the start time says; the user has said that the
two started together, so a track much longer than the video is not
refused either. The offset shifts the track back, and its absolute times
with it, so that a frame is stamped with its capture time and the
geotagging stage, adding the offset back, finds the position the frame
was picked by.
process still ignores both for videos. It sets them on every source, for
the images; applying them to a video there would change the track that
is uploaded with it, which is a separate decision.
For a clip at the end of its GPX with a 2 second offset, the positions
the sampler picked frames by and the ones they were geotagged at
differed by 16.7 m, and five frames were rejected. Now they agree, and
none are. With use_gpx_start_time they can still differ by the
composition time of the first frame, since the geotagging stage puts the
first sampled frame, not video time 0, at the start of the GPX.
Two new tests expect video_process to fail, and ran it through run_process_for_descs(). On Windows, that helper removes its description file while still holding it open when the command fails, and the PermissionError replaced the CalledProcessError the tests expect. Run them through run_command(), as the other tests expecting a failure do. Also find ffmpeg for the creation-time copy the way pytest_skip_if_not_ffmpeg_installed() does, through MAPILLARY_TOOLS_FFMPEG_PATH.
A GPX converted from a dashcam's log listed its 42 tracks out of time order, and 26 of them carried 1969-12-31T15:59:59Z on every point. GPXVideoExtractor merged the tracks as they came, so the points were out of time order, and distance sampling, which now reads the GPX, crashed in the interpolator with a traceback: ValueError: Expect points to be sorted by time, ... process did not crash, but uploaded the video with all of those points in its track, out of order, 25,250 of them some 55 years before the video. No GPS fix can be timestamped before the GPS epoch, 1980-01-06, so drop those points, with a warning, and sort the rest. Sorting alone would have kept the junk at the start of the track, where --interpolation_use_gpx_start_time would pin the video, and where the check of an assumed start would find a track lasting 55 years. This changes process too, for a GPX with such points or with its tracks out of order. A GPX whose points are in order and after 1980 gives the same track as before.
Moving _parse_source_options() out of process_geotag_properties left two names in a parenthesised import, which `usort diff mapillary_tools` reports. CI does not fail on it, but it should print nothing.
This was referenced Oct 19, 2026
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
Distance sampling called
GeotagVideosFromVideo()directly, bypassing the geotag factory, so--geotag_sourcenever 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, announced in the log, then ignored:Fix
Go through
factory.process()(fff6550). The consumer was already generic —_sample_video_stream_by_distance()takes a plainSequence[geo.Point]and doesn't care about provenance. Only the producer was hardcoded.Routing rather than adding a fallback keeps the source-selection policy in one place:
sample_video.pyhas no business re-deciding which source may rescue a file,_build_video_geotag()stays the only map from source to extractor, and chaining (native,gpx) comes for free.Sampling defaults to
NATIVEalone, notDEFAULT_GEOTAG_SOURCE_OPTIONS, which continues on toexiftool_runtime. That reader cannot see every field the noise filter rejects on (see #840), 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 fromprocess_geotag_propertiestogeotag.factory, next toparse_source_option(), so both callers share one implementation instead of the sampler reaching for a private helper.commands/sample_video.pyneeds no change: it splatsvars_argsfiltered by the signature ofsample_video(), so the new parameters flow through fromvideo_process, which registers the process command's arguments into the same parser. The standalonesample_videocommand doesn't register them, so they stay absent and the defaults apply.Picking frames where the geotagging stage will put them
Routing alone was not enough. The sampler now picks frames by positions from the GPX, and the geotagging stage of
video_processthen places each frame along the same GPX by the timestamp the sampler stamped it with. Unless the two line up the GPX with the video the same way, frames are picked by positions they do not end up at. Five follow-ups:--video_start_time(c599ddb). For a video with no GPS of its own,GPXVideoExtractorassumed the track starts with the video, and--video_start_timenever reached it. The frames were stamped with the GPX start plus their own time, and the geotagging stage agreed, so the result exited 0, looked consistent, and was wrong whenever the track did not start with the video: two clips of one drive, recorded hours apart and each given its own--video_start_time, came out as the same frames, from the first minutes of the drive's GPX.Distance sampling now hands the extractor a
VideoSyncOptioncarrying the start time the user gave. Only a given one: the probed start time iscreation_time - duration, a whole video length early on cameras whosecreation_timemarks the start (Read a video's start time from its GPS clock, and tell whether creation_time marks the start or the end #832). A video with GPS is still synced by its own GPS clock.--video_start_time, or--interpolation_use_gpx_start_timeif the GPX was started with the video.MapillaryGPXVideoStartUnknownErroris not reprocessable, so a source after the GPX in the chain cannot hide it behind "no GPS found". A track more than 2 s longer than the video warns.61b2a6a).video_processregisters--interpolation_offset_timeand--interpolation_use_gpx_start_timefor its geotagging stage, and the README documents both for lining up a GPX with a video. Sampling ignored them: with an offset, frames near the end of the track were pushed past it and rejected, failing the command. The extractor now applies both, and shifts the absolute times with the track, so that a frame is stamped with its capture time and the geotagging stage, adding the offset back, finds the position the frame was picked by.641ba60). exif, nmea and the legacy gopro/blackvue/camm sources have no video extractor, soprocess()returned nothing for the video and the sampler failed an assert, for videos it used to sample fine. Sampling now uses the requested sources that can read videos, or the video's own GPS, as before, when none can.544b21e). A GPX converted from a dashcam's log listed its 42 tracks out of time order, and 26 of them carried1969-12-31T15:59:59Zon every point.GPXVideoExtractormerged the tracks as they came, and distance sampling, which now reads the GPX, crashed in the interpolator withValueError: Expect points to be sorted by time. No GPS fix can predate the GPS epoch, so those points are dropped, with a warning, and the rest sorted. Sorting alone would have left the junk at the start of the track, where--interpolation_use_gpx_start_timewould pin the video.a0ef901). A comment and a test claimed that a GPX beside the video is found without--geotag_source_path. It is not: every source gets the video import path, which is howexiftool_xmlfinds XML files in the video directory. A GPX needs--geotag_source_path, which the geotagging stage needs as well.processis unchanged, except for untidy GPX files. It passes noVideoSyncOption, so it places a GPX on a video as before, and keeps ignoring the interpolation options for videos: it sets them on every source for the images, and applying them to a video there would change the track uploaded with it, which is a separate decision. The one change it shares is544b21e: for the GPX above it uploaded all 29,440 points out of order, 25,250 of them some 55 years before the video, and now uploads the 4,190 real ones in order. A GPX whose points are in order and after 1980 gives the same track as before.Verification
Against
main:The noisy-GoPro row needs #831 as well: without it
GPXVideoExtractorstill letsMapillaryGPSNoiseErrorescape before the GPX is consulted.The follow-ups, against
fff6550:--video_start_timegiven--video_start_time--interpolation_offset_time 2--geotag_source exif, video with good GPSAssertionError--video_start_timegivenValueErrortraceback, exit 1The default paths are untouched: bare
video_processandsample_video, andprocesson a video with a GPX, with and without the interpolation options, give the same descriptions and byte-identical frames asfff6550. Every new test fails without its fix, except the two that pin behaviour that stays the same:process, and the default source path.Full suite passes, mypy, ruff and usort clean. (
test_persistent_cache::test_multiprocess_shared_cache_comprehensiveis flaky under parallel load on this machine — passes in isolation, with and without this change.)Not fixed here
GPXVideoExtractor— see the correction on Let a GPX override a video whose own GPS is unusable #831.--interpolation_use_gpx_start_time, the geotagging stage puts the first sampled frame at the start of the GPX, while sampling puts video time 0 there. They differ by the composition time of the first frame, under a metre on the test clip; it cannot push frames off the track.creation_timestill needs--video_start_time, even with--interpolation_use_gpx_start_time: the start-time check runs before the GPX is read._video_duration()ingpx.pyduplicates the one Fix CAMM GPS epoch follow-ups from #828 #842 adds; whichever lands second should keep one.