Skip to content

Fail the sample instead of silently producing no frames - #839

Open
caglarpir wants to merge 1 commit into
mapillary:mainfrom
caglarpir:fix-silent-sample-failure
Open

caglarpir wants to merge 1 commit into
mapillary:mainfrom
caglarpir:fix-silent-sample-failure

Conversation

@caglarpir

Copy link
Copy Markdown
Contributor

Problem

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, so 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, but nothing about it is specific to noisy GPS — a video with no GPS at all takes the same path. Same clip, same GPX, only the sampling mode differs:

exit frames
--video_sample_distance 3 0 0
--video_sample_interval 2 0 3

Fix

Distance sampling needs positions to decide which frames to cut, so failing to read them is a failed sample. Raise MapillaryVideoError — the same error this function already raises for an unreadable start time or a frame-count mismatch, and a MapillaryUserError, so it exits 7 instead of dumping a traceback.

sample_video() already funnels sampling errors through --skip_sample_errors, so tolerating this stays supported and the default stops lying. This is exactly the shape of the existing test_sample_video_without_video_time.

Two neighbouring silent paths get the same treatment: a missing video stream (also returned after a warning), and an empty point list (an assert, so it disappeared under python -O, leaving an IndexError further down).

Behaviour change worth flagging

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 real change for anyone relying on the silent 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, and a new test pins the loud behaviour.

Scope

This does not make --geotag_source reach distance sampling — that call site still hardcodes GeotagVideosFromVideo() and bypasses factory.process() entirely. That is a separate fix. This one only stops the failure from being silent.

Independent of #831 (different file, branches off main).

Verification

759 unit + integration tests pass, mypy clean. Of the 7 new unit tests, 5 fail without the change; the --skip_sample_errors guard passes either way.

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.
@meta-cla meta-cla Bot added the cla signed label Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant