Fail the sample instead of silently producing no frames

README.md

@@ -303,6 +303,16 @@ mapillary_tools sample_video MY_VIDEO_DIR MY_SAMPLES --video_sample_distance 5
 mapillary_tools process MY_SAMPLES
 ```
 
+If a video can not be sampled, for example because it has no GPS track to locate the frames along,
+mapillary_tools still samples the other videos and processes their sample images, then exits with an error.
+To skip such videos instead, specify `--skip_sample_errors`. It covers sampling only: errors from processing
+the sample images are skipped with `--skip_process_errors`.
+
+```sh
+# Sample and process the videos that can be sampled, and skip the others
+mapillary_tools video_process MY_VIDEO_DIR MY_SAMPLES --skip_sample_errors
+```
+
 To process and upload the sample images consecutively, run:
 
 ```sh

mapillary_tools/commands/sample_video.py

@@ -61,7 +61,7 @@ def add_basic_arguments(self, parser: argparse.ArgumentParser):
         )
         group.add_argument(
             "--skip_sample_errors",
-            help="Skip errors from the video sampling.",
+            help="Skip the videos that fail to sample. Otherwise the command fails after sampling the other videos.",
             action="store_true",
             default=False,
             required=False,

mapillary_tools/commands/video_process.py

@@ -3,8 +3,11 @@
 # This source code is licensed under the BSD license found in the
 # LICENSE file in the root directory of this source tree.
 
+from __future__ import annotations
+
 import logging
 
+from .. import exceptions
 from ..types import FileType
 from .process import Command as ProcessCommand
 from .sample_video import Command as SampleCommand
@@ -22,7 +25,14 @@ def add_basic_arguments(self, parser):
         ProcessCommand().add_basic_arguments(parser)
 
     def run(self, args: dict):
-        SampleCommand().run(args)
+        sample_error: exceptions.MapillaryVideoSampleError | None = None
+        try:
+            SampleCommand().run(args)
+        except exceptions.MapillaryVideoSampleError as ex:
+            # Like the process command, which writes out the files it did
+            # process before failing: process the frames of the videos that
+            # did sample rather than leave them behind, then fail
+            sample_error = ex
 
         option = "filetypes"
         if args[option] != {FileType.IMAGE}:
@@ -34,3 +44,6 @@ def run(self, args: dict):
             args[option] = {FileType.IMAGE}
 
         ProcessCommand().run(args)
+
+        if sample_error is not None:
+            raise sample_error

mapillary_tools/commands/video_process_and_upload.py

@@ -4,12 +4,17 @@
 # LICENSE file in the root directory of this source tree.
 
 import inspect
+import logging
 
+from .. import exceptions
 from ..authenticate import fetch_user_items
 from .upload import Command as UploadCommand
 from .video_process import Command as VideoProcessCommand
 
 
+LOG = logging.getLogger(__name__)
+
+
 class Command:
     name = "video_process_and_upload"
     help = "sample video into images, process the images and upload to Mapillary"
@@ -33,5 +38,17 @@ def run(self, vars_args: dict):
                 }
             )
 
-        VideoProcessCommand().run(vars_args)
+        try:
+            VideoProcessCommand().run(vars_args)
+        except exceptions.MapillaryVideoSampleError:
+            # Upload only some of the videos when asked to, as process_and_upload
+            # does with the files it fails to process. But say so: the last thing
+            # logged before this is the process summary of the sample images that
+            # were ready to upload
+            LOG.error(
+                "Nothing was uploaded, because some videos failed to sample. "
+                "To upload the sample images of the others, specify --skip_sample_errors"
+            )
+            raise
+
         UploadCommand().run(vars_args)

mapillary_tools/exceptions.py

@@ -40,6 +40,12 @@ class MapillaryVideoError(MapillaryUserError):
     exit_code = 7
 
 
+class MapillaryVideoSampleError(MapillaryVideoError):
+    """
+    Raised after sampling every video if any of them failed
+    """
+
+
 class MapillaryFFmpegNotFoundError(MapillaryUserError):
     exit_code = 8
 

mapillary_tools/sample_video.py

@@ -27,8 +27,9 @@ def _normalize_path(
     video_import_path: Path, skip_subfolders: bool
 ) -> tuple[Path, list[Path]]:
     if video_import_path.is_dir():
-        video_list = utils.find_videos(
-            [video_import_path], skip_subfolders=skip_subfolders
+        # Sorted so that videos are sampled and reported in a stable order
+        video_list = sorted(
+            utils.find_videos([video_import_path], skip_subfolders=skip_subfolders)
         )
         video_dir = video_import_path.resolve()
         LOG.debug("Found %d videos in %s", len(video_list), video_dir)
@@ -92,6 +93,15 @@ def sample_video(
             elif sample_dir.is_file():
                 os.remove(sample_dir)
 
+    if video_list:
+        # Created even if no video samples, so that the process command that
+        # follows in video_process finds an empty import path rather than none
+        os.makedirs(import_path, exist_ok=True)
+
+    num_sampled = 0
+    num_already_sampled = 0
+    failures: list[tuple[Path, Exception]] = []
+
     for video_path in video_list:
         # need to resolve video_path because video_dir might be absolute
         sample_dir = Path(import_path).joinpath(
@@ -103,6 +113,7 @@ def sample_video(
                 video_path.name,
                 sample_dir,
             )
+            num_already_sampled += 1
             continue
 
         try:
@@ -129,15 +140,56 @@ def sample_video(
             raise exceptions.MapillaryFFmpegNotFoundError(str(ex)) from ex
 
         except Exception as ex:
-            if skip_sample_errors:
-                LOG.warning(
-                    "Skipping the error sampling %s: %s",
-                    video_path,
-                    str(ex),
-                    exc_info=LOG.isEnabledFor(logging.DEBUG),
-                )
-            else:
-                raise
+            # Carry on with the other videos, as the process command does with
+            # the other files, and fail after the summary. Log the reason now,
+            # because unlike process, sampling writes no description file
+            (LOG.warning if skip_sample_errors else LOG.error)(
+                "Failed to sample %s: %s",
+                video_path,
+                str(ex),
+                exc_info=LOG.isEnabledFor(logging.DEBUG),
+            )
+            failures.append((video_path, ex))
+
+        else:
+            num_sampled += 1
+
+    _show_stats(
+        len(video_list),
+        num_sampled=num_sampled,
+        num_already_sampled=num_already_sampled,
+        failures=failures,
+        skip_sample_errors=skip_sample_errors,
+    )
+
+
+def _show_stats(
+    num_videos: int,
+    num_sampled: int,
+    num_already_sampled: int,
+    failures: T.Sequence[tuple[Path, Exception]],
+    skip_sample_errors: bool,
+) -> None:
+    LOG.info("==> Sample summary")
+    LOG.info("%d videos found", num_videos)
+    if num_sampled:
+        LOG.info("\t%d sampled", num_sampled)
+    if num_already_sampled:
+        LOG.info("\t%d already sampled", num_already_sampled)
+
+    failures_by_type: dict[type[Exception], int] = {}
+    for _, ex in failures:
+        failures_by_type[type(ex)] = failures_by_type.get(type(ex), 0) + 1
+
+    for error_type, count in failures_by_type.items():
+        (LOG.warning if skip_sample_errors else LOG.error)(
+            "\t%d %s", count, error_type.__name__
+        )
+
+    if failures and not skip_sample_errors:
+        raise exceptions.MapillaryVideoSampleError(
+            f"Failed to sample {len(failures)} videos. To skip these errors, specify --skip_sample_errors"
+        )
 
 
 @contextmanager
@@ -300,17 +352,26 @@ def _sample_single_video_by_distance(
     )
     assert len(video_metadatas) == 1, "expect 1 video metadata"
     video_metadata = video_metadatas[0]
+
+    # Distance sampling needs positions to decide which frames to cut, so
+    # failing to read them is a failed sample, not something to carry on past.
+    # Warning and returning left the caller with a success exit code, an empty
+    # (or missing) sample directory and nothing to upload. Raised as is, so
+    # that the sample summary counts it by its own type
     if isinstance(video_metadata, types.ErrorMetadata):
-        LOG.warning(str(video_metadata.error))
-        return
-    assert video_metadata.points, "expect non-empty points"
+        raise video_metadata.error
+
+    if not video_metadata.points:
+        raise exceptions.MapillaryVideoGPSNotFoundError("No GPS points found")
+
     LOG.info("Found total %d GPS points", len(video_metadata.points))
 
     # find the video stream with maximum resolution
     video_stream = probe.probe_video_with_max_resolution()
     if not video_stream:
-        LOG.warning("no video streams found from ffprobe")
-        return
+        raise exceptions.MapillaryVideoError(
+            f"No video streams found in {video_path} by ffprobe"
+        )
 
     LOG.info("Extracting video samples")
     video_stream_idx = video_stream["index"]
@@ -319,6 +380,11 @@ def _sample_single_video_by_distance(
     sample_points_by_frame_idx = _sample_video_stream_by_distance(
         video_metadata.points, video_track_parser, sample_distance
     )
+    if not sample_points_by_frame_idx:
+        # A track that covers none of the video, e.g. a GPX from another trip
+        raise exceptions.MapillaryVideoError(
+            f"No video frames found within the GPS track from {video_metadata.points[0].time:.3f}s to {video_metadata.points[-1].time:.3f}s"
+        )
     sorted_sample_indices = sorted(sample_points_by_frame_idx.keys())
 
     with wip_dir_context(wip_sample_dir(sample_dir), sample_dir) as wip_dir: