Honour --geotag_source in distance-based sampling

mapillary_tools/geotag/factory.py

@@ -22,7 +22,13 @@
     geotag_videos_from_gpx,
     geotag_videos_from_video,
 )
-from .options import InterpolationOption, SOURCE_TYPE_ALIAS, SourceOption, SourceType
+from .options import (
+    InterpolationOption,
+    SOURCE_TYPE_ALIAS,
+    SourceOption,
+    SourcePathOption,
+    SourceType,
+)
 
 
 LOG = logging.getLogger(__name__)
@@ -58,6 +64,51 @@ def parse_source_option(source: str) -> list[SourceOption]:
     return [SourceOption(SourceType(SOURCE_TYPE_ALIAS.get(s, s))) for s in sources]
 
 
+def parse_source_options(
+    geotag_source: T.Sequence[str],
+    video_geotag_source: T.Sequence[str],
+    geotag_source_path: Path | None,
+) -> list[SourceOption]:
+    """
+    Turn the raw --geotag_source / --video_geotag_source / --geotag_source_path
+    arguments into the option list that process() consumes.
+    """
+    parsed_options: list[SourceOption] = []
+
+    if video_geotag_source and geotag_source:
+        LOG.warning(
+            "Video source options will be processed BEFORE the generic source options"
+        )
+
+    for s in video_geotag_source:
+        for video_option in parse_source_option(s):
+            video_option.filetypes = types.combine_filetype_filters(
+                video_option.filetypes, {types.FileType.VIDEO}
+            )
+            parsed_options.append(video_option)
+
+    for s in geotag_source:
+        parsed_options.extend(parse_source_option(s))
+
+    if geotag_source_path is not None:
+        for parsed_option in parsed_options:
+            if parsed_option.source_path is None:
+                parsed_option.source_path = SourcePathOption(
+                    source_path=Path(geotag_source_path)
+                )
+            else:
+                source_path_option = parsed_option.source_path
+                if source_path_option.source_path is None:
+                    source_path_option.source_path = Path(geotag_source_path)
+                else:
+                    LOG.warning(
+                        "The option --geotag_source_path is ignored for source %s",
+                        parsed_option,
+                    )
+
+    return parsed_options
+
+
 def process(
     # Collection: ABC for sized iterable container classes
     paths: T.Iterable[Path],

mapillary_tools/process_geotag_properties.py

@@ -14,11 +14,9 @@
 from tqdm import tqdm
 
 from . import constants, exceptions, exif_write, types, utils
-from .geotag.factory import parse_source_option, process
+from .geotag.factory import parse_source_options, process
 from .geotag.options import (
     InterpolationOption,
-    SourceOption,
-    SourcePathOption,
     SourceType,
 )
 from .serializer.description import (
@@ -44,47 +42,6 @@ def _normalize_import_paths(import_path: Path | T.Sequence[Path]) -> T.Sequence[
     return import_paths
 
 
-def _parse_source_options(
-    geotag_source: list[str],
-    video_geotag_source: list[str],
-    geotag_source_path: Path | None,
-) -> list[SourceOption]:
-    parsed_options: list[SourceOption] = []
-
-    if video_geotag_source and geotag_source:
-        LOG.warning(
-            "Video source options will be processed BEFORE the generic source options"
-        )
-
-    for s in video_geotag_source:
-        for video_option in parse_source_option(s):
-            video_option.filetypes = types.combine_filetype_filters(
-                video_option.filetypes, {types.FileType.VIDEO}
-            )
-            parsed_options.append(video_option)
-
-    for s in geotag_source:
-        parsed_options.extend(parse_source_option(s))
-
-    if geotag_source_path is not None:
-        for parsed_option in parsed_options:
-            if parsed_option.source_path is None:
-                parsed_option.source_path = SourcePathOption(
-                    source_path=Path(geotag_source_path)
-                )
-            else:
-                source_path_option = parsed_option.source_path
-                if source_path_option.source_path is None:
-                    source_path_option.source_path = Path(geotag_source_path)
-                else:
-                    LOG.warning(
-                        "The option --geotag_source_path is ignored for source %s",
-                        parsed_option,
-                    )
-
-    return parsed_options
-
-
 def process_geotag_properties(
     import_path: Path | T.Sequence[Path],
     filetypes: set[types.FileType] | None,
@@ -115,7 +72,7 @@ def process_geotag_properties(
     if not geotag_source and not video_geotag_source:
         geotag_source = [*DEFAULT_GEOTAG_SOURCE_OPTIONS]
 
-    options = _parse_source_options(
+    options = parse_source_options(
         geotag_source=geotag_source or [],
         video_geotag_source=video_geotag_source or [],
         geotag_source_path=geotag_source_path,

mapillary_tools/sample_video.py

@@ -16,12 +16,18 @@
 
 from . import constants, exceptions, ffmpeg as ffmpeglib, geo, types, utils
 from .exif_write import ExifEdit
-from .geotag import geotag_videos_from_video
+from .geotag import factory
+from .geotag.options import SourceOption, SourceType
 from .mp4 import mp4_sample_parser
 from .serializer.description import parse_capture_time
 
 LOG = logging.getLogger(__name__)
 
+# Sampling errors are suppressed by --skip_sample_errors, not by
+# --skip_process_errors, which governs the later geotagging stage. Say so in
+# the message: the two flags are easy to reach for the wrong one.
+_SKIP_HINT = "To skip these errors, specify --skip_sample_errors"
+
 
 def _normalize_path(
     video_import_path: Path, skip_subfolders: bool
@@ -49,6 +55,36 @@ def xor(a: bool, b: bool):
     return bool(a) ^ bool(b)
 
 
+def _parse_geotag_options(
+    geotag_source: list[str] | None,
+    geotag_source_path: Path | None,
+    video_geotag_source: list[str] | None,
+    video_import_path: Path,
+) -> list[SourceOption]:
+    """
+    Resolve which sources distance sampling may take its positions from.
+
+    Defaults to the video's own telemetry rather than to
+    DEFAULT_GEOTAG_SOURCE_OPTIONS: the default chain continues on to
+    exiftool_runtime, which reads the same telemetry through a parser that
+    cannot see every field the noise filter rejects on, so routing sampling
+    through it would accept tracks the native parser refuses.
+    """
+    if not geotag_source and not video_geotag_source:
+        return [SourceOption(SourceType.NATIVE)]
+
+    # Mirrors process_geotag_properties(): a sidecar is looked for next to the
+    # video when no explicit path is given
+    if geotag_source_path is None:
+        geotag_source_path = video_import_path
+
+    return factory.parse_source_options(
+        geotag_source=geotag_source or [],
+        video_geotag_source=video_geotag_source or [],
+        geotag_source_path=geotag_source_path,
+    )
+
+
 def sample_video(
     video_import_path: Path,
     import_path: Path,
@@ -60,9 +96,18 @@ def sample_video(
     video_start_time: str | None = None,
     skip_sample_errors: bool = False,
     rerun: bool = False,
+    # Absent when called from the sample_video command, which does not register
+    # the process command's arguments
+    geotag_source: list[str] | None = None,
+    geotag_source_path: Path | None = None,
+    video_geotag_source: list[str] | None = None,
 ) -> None:
     video_dir, video_list = _normalize_path(video_import_path, skip_subfolders)
 
+    geotag_options = _parse_geotag_options(
+        geotag_source, geotag_source_path, video_geotag_source, video_import_path
+    )
+
     if not xor(0 <= video_sample_distance, 0 < video_sample_interval):
         raise exceptions.MapillaryBadParameterError(
             f"Expect either non-negative video_sample_distance or positive video_sample_interval but got {video_sample_distance} and {video_sample_interval} respectively"
@@ -112,6 +157,7 @@ def sample_video(
                     sample_dir,
                     sample_distance=video_sample_distance,
                     start_time=video_start_time_dt,
+                    geotag_options=geotag_options,
                 )
             else:
                 assert 0 < video_sample_interval, (
@@ -281,7 +327,11 @@ def _sample_single_video_by_distance(
     sample_dir: Path,
     sample_distance: float,
     start_time: datetime.datetime | None = None,
+    geotag_options: T.Sequence[SourceOption] | None = None,
 ) -> None:
+    if geotag_options is None:
+        geotag_options = [SourceOption(SourceType.NATIVE)]
+
     ffmpeg = ffmpeglib.FFMPEG(constants.FFMPEG_PATH, constants.FFPROBE_PATH)
 
     probe = ffmpeglib.Probe(ffmpeg.probe_format_and_streams(video_path))
@@ -295,22 +345,43 @@ def _sample_single_video_by_distance(
 
     LOG.info("Extracting video metdata")
 
-    video_metadatas = geotag_videos_from_video.GeotagVideosFromVideo().to_description(
-        [video_path]
-    )
+    # Go through the factory rather than reading the video's own telemetry
+    # directly, so that --geotag_source is honoured here as well: a GPX is the
+    # documented answer for a camera whose embedded GPS is unusable, and
+    # distance sampling needs positions just as much as geotagging does.
+    video_metadatas = factory.process([video_path], geotag_options)
     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. sample_video()
+    # already funnels these through --skip_sample_errors for callers who do
+    # want to tolerate them.
     if isinstance(video_metadata, types.ErrorMetadata):
-        LOG.warning(str(video_metadata.error))
-        return
-    assert video_metadata.points, "expect non-empty points"
+        raise exceptions.MapillaryVideoError(
+            f"Unable to sample {video_path} by distance: {video_metadata.error}. {_SKIP_HINT}"
+        ) from video_metadata.error
+
+    # Only a video path was passed in, so only video metadata can come back
+    assert isinstance(video_metadata, types.VideoMetadata), (
+        f"expect VideoMetadata but got {type(video_metadata).__name__}"
+    )
+
+    if not video_metadata.points:
+        raise exceptions.MapillaryVideoError(
+            f"Unable to sample {video_path} by distance: no GPS points found. {_SKIP_HINT}"
+        )
+
     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. {_SKIP_HINT}"
+        )
 
     LOG.info("Extracting video samples")
     video_stream_idx = video_stream["index"]

tests/integration/test_video_process.py

@@ -31,13 +31,24 @@
 def test_sample_video_relpath():
     pytest_skip_if_not_ffmpeg_installed()
 
+    # hero8.mp4's embedded GPS is filtered out entirely as noise, so it cannot
+    # be sampled by distance. This test is about relative paths, not about that,
+    # hence --skip_sample_errors.
     with tempfile.TemporaryDirectory() as dir:
-        run_sample_video(["--rerun", "tests/data/gopro_data/hero8.mp4", str(dir)])
+        run_sample_video(
+            [
+                "--rerun",
+                "--skip_sample_errors",
+                "tests/data/gopro_data/hero8.mp4",
+                str(dir),
+            ]
+        )
 
     with tempfile.TemporaryDirectory() as dir:
         run_sample_video(
             [
                 "--rerun",
+                "--skip_sample_errors",
                 "--video_start_time",
                 "2021_10_10_10_10_10_123",
                 "tests/data",
@@ -46,6 +57,37 @@ def test_sample_video_relpath():
         )
 
 
+def test_sample_video_by_distance_without_usable_gps(setup_data: py.path.local):
+    """
+    A video whose GPS cannot be read is not silently sampled into nothing.
+
+    hero8.mp4 has 32 embedded GPS points and remove_noisy_points() drops all of
+    them, so distance sampling has no positions to choose frames with. It used
+    to log a warning, write no frames and still exit 0, which left the caller a
+    success code and an empty import directory.
+    """
+    pytest_skip_if_not_ffmpeg_installed()
+
+    video_path = setup_data.join("gopro_data").join("hero8.mp4")
+
+    with tempfile.TemporaryDirectory() as dir:
+        with pytest.raises(subprocess.CalledProcessError) as ex:
+            run_sample_video(["--video_sample_distance=6", "--rerun", video_path, dir])
+        assert 7 == ex.value.returncode, ex.value.stderr
+        assert not list(Path(dir).iterdir())
+
+    with tempfile.TemporaryDirectory() as dir:
+        run_sample_video(
+            [
+                "--video_sample_distance=6",
+                "--skip_sample_errors",
+                "--rerun",
+                video_path,
+                dir,
+            ]
+        )
+
+
 def test_sample_video_without_video_time(setup_data: py.path.local):
     pytest_skip_if_not_ffmpeg_installed()
 
@@ -143,6 +185,9 @@ def test_video_process_sample_with_multiple_distances(setup_data: py.path.local)
             [
                 "--video_sample_distance",
                 str(distance),
+                # gopro_data also holds hero8.mp4, whose GPS is all noise and so
+                # cannot be distance-sampled; this test is about max-360mode.mp4
+                "--skip_sample_errors",
                 "--rerun",
                 str(video_dir),
                 str(video_dir.join("my_samples")),
@@ -167,6 +212,8 @@ def test_video_process_sample_with_distance(setup_data: py.path.local):
         descs = run_video_process_for_descs(
             [
                 *options,
+                # see test_video_process_sample_with_multiple_distances
+                "--skip_sample_errors",
                 str(video_dir),
                 str(video_dir.join("my_samples")),
             ]