Repository navigation
Honour --geotag_source in distance-based sampling - #841
Merged
caglarpir merged 13 commits intoOct 20, 2026
Merged
Conversation
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
mapillary#839 now samples every video before failing and reports the failures together (38ded6a), and fails distance sampling that selects no frames (3649f34). Both meet this branch in sample_video.py: - 38ded6a removed _sampling_error(): errors are raised as they are, and the summary error names --skip_sample_errors. The one call this branch added, in its check for a track that misses the video, now raises a plain MapillaryVideoError, and leaves out the video path, which the summary logs. - 3649f34 added the same check at the same spot. Keep this branch's, whose message says where the track lies relative to the video. Three tests follow: 3649f34's test of the message; this branch's unit test of a missed track, which reads the log now that the error raised is the summary; and its integration test, which checks for no frames rather than no sample directory, now that the directory is created up front.
The no-frames test matched "which starts a minute after the video starts", but humanize words 100 seconds as "a minute" in 4.14 and as "2 minutes" in the version CI installs, so CI failed. Match the message without the duration, and move two doctests off values that versions round differently.
Brings in 7cd3025, which has video_process_and_upload log that it uploaded nothing when a video fails to sample. No conflicts.
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.
mapillary#839 reached main through its own merge commit, so this branch and main had two merge bases, mapillary#839's tip and the main merged in before it. git merges them cleanly, but GitHub reported the pull request as conflicting. Merging main again leaves it a single base. It brings in mapillary#843, which merges without conflicts, and nothing else changes.
caglarpir
added a commit
to caglarpir/mapillary_tools
that referenced
this pull request
Oct 20, 2026
The previous commit reads a video's start time from its telemetry and otherwise takes creation_time as the start. That fixes the reported dashcam, and Sony and Insta360, which also stamp the start. But it moves every video from a camera that stamps the end one full duration late. Those are most dashcams: Viofo, Vantrue, AZDome and others without GPS telemetry, which people geotag from a GPX, plus the Ricoh Theta X and the Labpano PanoX V2. With interval sampling and a GPX of their own track, sample videos from a Viofo A129 and an AZDome GS63H went from 119 and 57 geotagged frames to 1 and 0. Nothing in the metadata says which of the two a camera writes, so look for evidence, in this order: - a camera known to stamp the end: the Theta X and the PanoX V2 by their make and model tags, BlackVue by the boxes it writes its GPS log into, which it does whether or not it ever gets a fix - a date and time in the file name that matches only one of the two, allowing for a whole UTC offset because cameras name files in local time - otherwise the start, with a warning that prints the --video_start_time value to use if the camera stamps the end Also: - Take the median of the first 5 plausible GPS timestamps instead of the first one. The PanoX V2 can record a stale first fix seconds older than the rest, and a GPS clock that was never set (GoPro writes 2000-01-01 until its first fix) must not become the start time. The hero8 fixture moves by 0.1s because the GPS5 samples of one payload share a timestamp. - Fall back to the creation time when the telemetry fails to parse, instead of failing to sample the video. - Measure BlackVue point times from the first NMEA line, which is the start of the recording, instead of from the first valid fix. Until the receiver gets a fix the camera logs lines without positions, so every point, and with the previous commit every frame sampled by interval, was late by the time to first fix. Across 433 BlackVue sample videos 7 took more than 5s to get a fix, one 60s. - Distance sampling resolves the start time only when a sampled frame has no absolute GPS time of its own, instead of requiring a creation time up front for every video. - Ignore a malformed creation_time with a warning instead of raising. On top of mapillary#839 and mapillary#841: the start time raises a plain MapillaryVideoError, which the sample summary reports, as mapillary#839's other sampling errors do. mapillary#839's test of an unreadable start time patched probe_video_start_time(), which this removes; it now covers interval sampling through _extract_video_start_time(), and distance sampling through a track without a GPS clock, which is when distance sampling looks the start time up. mapillary#841's comment on why only --video_start_time places a GPX gave creation_time - duration as the reason, which no longer holds; the reason now is that a video without a GPS clock would get its start time from its creation time, which is stamped at the start or the end of the recording, some in local time.
This was referenced Oct 20, 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
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, and for GPS without a clock2fb6b22). 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
--interpolation_offset_timein GPS seconds, which a timelapse packs several of into each second of video. A timelapse without a GPS clock is still placed as if its video ran in real time.--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.