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
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
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(). The consumer was already generic —_sample_video_stream_by_distance()takes a plainSequence[geo.Point]and doesn't care about provenance, andGPXVideoExtractoralready returns points rebased onto the video timeline. 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.Verification
The noisy-GoPro row needs #831 as well: without it
GPXVideoExtractorstill letsMapillaryGPSNoiseErrorescape before the GPX is consulted. This PR is what gets the GPX to the sampler at all.Full suite passes, mypy and ruff 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
A GPX is still mapped onto the video's timeline by a constant offset, which is wrong for a timelapse, where the video clock runs slower than wall time. The reported clip is a 12.5x timelapse, so it still yields nothing even with this PR. That is a separate change in
GPXVideoExtractor— see the correction on #831.