Read exiftool_xml from %g.xml by default, as the README documents

README.md

@@ -414,7 +414,7 @@ The new video processing is triggered with the `--video_geotag_source SOURCE` op
 In this case, each source will be tried in turn, until one returns good quality data.
 
 `SOURCE` can be:
-1. the plain name of the source - one of `video, camm, gopro, blackvue, gpx, nmea, exiftool_xml, exiftool_runtime`
+1. the plain name of the source - one of `native, video, camm, gopro, blackvue, gpx, nmea, exiftool_xml, exiftool_runtime`
 2. a JSON object that includes optional parameters
 ```json5
 {
@@ -430,7 +430,8 @@ PATTERN specifies how the data source file is named, starting from the video fil
 
 Supported sources and their default pattern are:
 
-* `video`: parse the video, in order, as `camm, gopro, blackvue`. Pattern: `%f`
+* `native`, or `video`: parse the video looking for geolocation in any format that mapillary_tools reads: GoPro, CAMM,
+BlackVue and others. Pattern: `%f`
 * `camm`: parse the video looking for a CAMM track. Pattern: `%f`
 * `gopro`: parse the video looking for geolocation in GoPro format. Pattern: `%f`
 * `blackvue`: parse the video looking for geolocation in BlackVue format. Pattern: `%f`

mapillary_tools/geotag/factory.py

@@ -31,19 +31,32 @@
     SourceOption,
     SourcePathOption,
     SourceType,
+    VIDEO_SOURCE_TYPE_ALIAS,
 )
 
 
 LOG = logging.getLogger(__name__)
 
 _ALL_VIDEO_TYPES = {types.FileType.VIDEO, *types.NATIVE_VIDEO_FILETYPES}
 
+# As a --video_geotag_source, each of the legacy sources that geotag images from
+# videos reads the GPS of the videos of its type, as it did before v0.14
+_VIDEO_TYPE_OF_LEGACY_SOURCE = {
+    SourceType.GOPRO: types.FileType.GOPRO,
+    SourceType.BLACKVUE: types.FileType.BLACKVUE,
+    SourceType.CAMM: types.FileType.CAMM,
+}
+
+# The XML that the exiftool_xml source reads for a file, without a source path
+# or a pattern: the output of `exiftool -X` for it, named after it
+_EXIFTOOL_XML_DEFAULT_PATTERN = "%g.xml"
+
 # Sources that read a GPS track from outside the video file, as opposed to
 # re-reading the telemetry embedded in it
 EXTERNAL_GPS_SOURCES = frozenset({SourceType.GPX, SourceType.NMEA})
 
 
-def parse_source_option(source: str) -> list[SourceOption]:
+def parse_source_option(source: str, video: bool = False) -> list[SourceOption]:
     """
     Given a source string, parse it into a list of GeotagOptions objects.
 
@@ -53,13 +66,17 @@ def parse_source_option(source: str) -> list[SourceOption]:
         "exif,gpx" -> [SourceOption(SourceType.EXIF), SourceOption(SourceType.GPX)]
         '{"source": "gpx"}' -> [SourceOption(SourceType.GPX)]
 
+    A --video_geotag_source (**video**) takes the names of
+    VIDEO_SOURCE_TYPE_ALIAS too.
+
     Raise MapillaryBadParameterError for an unknown source type, and for a
     JSON object that does not match SourceOptionSchema.
     """
+    aliases = {**SOURCE_TYPE_ALIAS, **VIDEO_SOURCE_TYPE_ALIAS} if video else None
 
     try:
-        source_type = SourceType(SOURCE_TYPE_ALIAS.get(source, source))
-    except ValueError:
+        source_type = _parse_source_type(source, aliases)
+    except exceptions.MapillaryBadParameterError:
         pass
     else:
         return [SourceOption(source_type)]
@@ -69,6 +86,10 @@ def parse_source_option(source: str) -> list[SourceOption]:
     except json.JSONDecodeError:
         pass
     else:
+        if aliases and isinstance(payload, dict):
+            name = payload.get("source")
+            if isinstance(name, str) and name in VIDEO_SOURCE_TYPE_ALIAS:
+                payload = {**payload, "source": VIDEO_SOURCE_TYPE_ALIAS[name].value}
         try:
             return [SourceOption.from_dict(payload)]
         except jsonschema.ValidationError as ex:
@@ -78,17 +99,21 @@ def parse_source_option(source: str) -> list[SourceOption]:
 
     sources = source.split(",")
 
-    return [SourceOption(_parse_source_type(s)) for s in sources]
+    return [SourceOption(_parse_source_type(s, aliases)) for s in sources]
 
 
-def _parse_source_type(source: str) -> SourceType:
+def _parse_source_type(
+    source: str, aliases: T.Mapping[str, SourceType] | None = None
+) -> SourceType:
+    if aliases is None:
+        aliases = SOURCE_TYPE_ALIAS
     try:
-        return SourceType(SOURCE_TYPE_ALIAS.get(source, source))
+        return SourceType(aliases.get(source, source))
     except ValueError:
         raise exceptions.MapillaryBadParameterError(
             f"Invalid geotag source {source!r}. Supported source types: "
             f"{', '.join(s.value for s in SourceType)} "
-            f"(aliases: {', '.join(SOURCE_TYPE_ALIAS)})"
+            f"(aliases: {', '.join(aliases)})"
         ) from None
 
 
@@ -109,11 +134,8 @@ def parse_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 video_option in parse_source_option(s, video=True):
+            parsed_options.append(_as_video_source(video_option))
 
     for s in geotag_source:
         parsed_options.extend(parse_source_option(s))
@@ -137,6 +159,31 @@ def parse_source_options(
     return parsed_options
 
 
+def _as_video_source(option: SourceOption) -> SourceOption:
+    """
+    Limit a --video_geotag_source option to videos. The legacy gopro, blackvue
+    and camm sources, which otherwise geotag images from videos, read the
+    video's own GPS as the native source does, from the videos of their type
+
+    >>> option = _as_video_source(SourceOption(SourceType.GOPRO))
+    >>> option.source, option.filetypes
+    (<SourceType.NATIVE: 'native'>, {<FileType.GOPRO: 'gopro'>})
+    >>> option = _as_video_source(SourceOption(SourceType.GPX))
+    >>> option.source, option.filetypes
+    (<SourceType.GPX: 'gpx'>, {<FileType.VIDEO: 'video'>})
+    """
+    video_type = _VIDEO_TYPE_OF_LEGACY_SOURCE.get(option.source)
+    if video_type is not None:
+        option.source = SourceType.NATIVE
+        option.filetypes = types.combine_filetype_filters(
+            option.filetypes, {video_type}
+        )
+    option.filetypes = types.combine_filetype_filters(
+        option.filetypes, {types.FileType.VIDEO}
+    )
+    return option
+
+
 def can_geotag_videos(option: SourceOption) -> bool:
     """
     Whether process() can read positions for videos from the source: it has a
@@ -197,6 +244,9 @@ def process(
     # those sources, to report if no other source geotags them or fails to
     unread_videos: dict[Path, list[SourceType]] = {}
 
+    # Paths let through to any source
+    handed_paths: set[Path] = set()
+
     for option in options:
         if option.source in EXTERNAL_GPS_SOURCES:
             option_paths = reprocessable_paths
@@ -215,6 +265,7 @@ def process(
         image_videos, video_paths = _filter_images_and_videos(
             option_paths, option.filetypes
         )
+        handed_paths.update(image_videos, video_paths)
 
         if image_videos:
             image_geotag = _build_image_geotag(option)
@@ -291,18 +342,125 @@ def process(
             break
 
     final_metadatas.extend(pending_errors.values())
-    # Limited to some types of video, a video that no source read may not be
-    # of any of them, and is left out as the videos found to be of other types
-    if video_types is None:
-        final_metadatas.extend(
-            _describe_unread_video(path, sources)
-            for path, sources in unread_videos.items()
-            if path in reprocessable_paths and path not in pending_errors
+
+    unread_paths = [
+        path
+        for path in unread_videos
+        if path in reprocessable_paths and path not in pending_errors
+    ]
+
+    # Files that no source was let through, as the filetypes of every source
+    # leave them out (e.g. sources for images only, or a source for BlackVue
+    # videos with --filetypes gopro), but those that --filetypes leaves out
+    sourceless_images, sourceless_videos = _filter_images_and_videos(
+        sorted(reprocessable_paths - handed_paths), filetypes
+    )
+
+    if video_types is not None and (unread_paths or sourceless_videos):
+        # Limited to some types of video, a video that no source read may not
+        # be of any of them. Those of other types are left out, as the videos
+        # found to be of other types
+        kept_paths = set(
+            _videos_not_of_other_types(
+                unread_paths + sourceless_videos,
+                video_types,
+                # The same for every option (see process_geotag_properties)
+                num_processes=next(iter(options)).num_processes,
+            )
         )
+        unread_paths = [path for path in unread_paths if path in kept_paths]
+        sourceless_videos = [path for path in sourceless_videos if path in kept_paths]
+
+    final_metadatas.extend(
+        _describe_unread_video(path, unread_videos[path]) for path in unread_paths
+    )
+    final_metadatas.extend(
+        _describe_sourceless_file(path, options, filetypes)
+        for path in sourceless_images + sourceless_videos
+    )
 
     return final_metadatas
 
 
+def _videos_not_of_other_types(
+    video_paths: T.Sequence[Path],
+    video_types: set[types.FileType],
+    num_processes: int | None = None,
+) -> list[Path]:
+    """
+    The videos of **video_paths** that are of the types **video_types**, or of
+    a type that cannot be told: all but those in which the native reader, given
+    those types, recognizes none of them, as process() tells while geotagging
+    (see _is_of_other_type)
+    """
+    LOG.info(
+        f"==> Reading the type of {len(video_paths)} videos that no source can geotag..."
+    )
+    native = SourceOption(
+        SourceType.NATIVE, filetypes=video_types, num_processes=num_processes
+    )
+    video_geotag = geotag_videos_from_video.GeotagVideosFromVideo(
+        filetypes=native.filetypes, num_processes=native.num_processes
+    )
+    other_types = {
+        metadata.filename
+        for metadata in video_geotag.to_description(video_paths)
+        if _is_of_other_type(metadata, native, video_types)
+    }
+    return [path for path in video_paths if path not in other_types]
+
+
+def _describe_sourceless_file(
+    path: Path,
+    options: T.Iterable[SourceOption],
+    filetypes: set[types.FileType] | None,
+) -> types.ErrorMetadata:
+    """
+    Describe a file that no source of **options** was let through, limited to
+    --filetypes **filetypes**, rather than leave it out of the results as if it
+    had not been there
+
+    >>> video_only = SourceOption(SourceType.NATIVE, filetypes={types.FileType.VIDEO})
+    >>> _describe_sourceless_file(Path("i.jpg"), [video_only], None).error
+    MapillaryGeoTaggingError('No geotag source is given for this image: the geotag source native is limited to other file types')
+    >>> # For BlackVue videos, limited to --filetypes gopro
+    >>> blackvue_only = SourceOption(SourceType.NATIVE, filetypes=set())
+    >>> _describe_sourceless_file(Path("v.mp4"), [blackvue_only], {types.FileType.GOPRO}).error
+    MapillaryVideoGPSNotFoundError('--filetypes gopro leaves no geotag source for this video: the geotag source native is limited to other file types')
+    >>> images_only = [
+    ...     SourceOption(SourceType.NATIVE, filetypes={types.FileType.IMAGE}),
+    ...     SourceOption(SourceType.EXIFTOOL_RUNTIME, filetypes={types.FileType.IMAGE}),
+    ... ]
+    >>> filetypes = {types.FileType.IMAGE, types.FileType.GOPRO}
+    >>> _describe_sourceless_file(Path("v.mp4"), images_only, filetypes).error
+    MapillaryVideoGPSNotFoundError('--filetypes gopro,image leaves no geotag source for this video: the geotag sources native, exiftool_runtime are limited to other file types')
+    """
+    kind = "image" if utils.is_image_file(path) else "video"
+    if filetypes is None:
+        no_source = f"No geotag source is given for this {kind}"
+    else:
+        asked_for = ",".join(sorted(filetype.value for filetype in filetypes))
+        no_source = f"--filetypes {asked_for} leaves no geotag source for this {kind}"
+    names = list(dict.fromkeys(option.source.value for option in options))
+    sources = (
+        f"the geotag sources {', '.join(names)} are"
+        if len(names) > 1
+        else f"the geotag source {names[0]} is"
+    )
+    message = f"{no_source}: {sources} limited to other file types"
+    if kind == "image":
+        return types.describe_error_metadata(
+            exceptions.MapillaryGeoTaggingError(message),
+            path,
+            filetype=types.FileType.IMAGE,
+        )
+    return types.describe_error_metadata(
+        exceptions.MapillaryVideoGPSNotFoundError(message),
+        path,
+        filetype=types.FileType.VIDEO,
+    )
+
+
 def _describe_unread_video(
     video_path: Path, sources: T.Sequence[SourceType]
 ) -> types.ErrorMetadata:
@@ -489,6 +647,20 @@ def _ensure_source_path(option: SourceOption) -> Path:
     return option.source_path.source_path
 
 
+def _exiftool_xml_source_path(option: SourceOption) -> SourcePathOption:
+    """
+    Where the exiftool_xml source finds the XML of an image or a video: at its
+    source path or by its pattern, or by default, as the README documents, in
+    the XML named after the file beside it
+
+    >>> _exiftool_xml_source_path(SourceOption(SourceType.EXIFTOOL_XML))
+    SourcePathOption(pattern='%g.xml', source_path=None)
+    """
+    if option.source_path is None:
+        return SourcePathOption(pattern=_EXIFTOOL_XML_DEFAULT_PATTERN)
+    return option.source_path
+
+
 def _build_image_geotag(option: SourceOption) -> base.GeotagImagesFromGeneric | None:
     """
     Build a GeotagImagesFromGeneric object based on the provided option.
@@ -509,14 +681,8 @@ def _build_image_geotag(option: SourceOption) -> base.GeotagImagesFromGeneric |
         )
 
     elif option.source is SourceType.EXIFTOOL_XML:
-        # This is to ensure 'video_process --geotag={"source": "exiftool_xml", "source_path": "/tmp/xml_path"}'
-        # to work
-        if option.source_path is None:
-            raise exceptions.MapillaryBadParameterError(
-                "source_path must be provided for EXIFTOOL_XML source"
-            )
         return geotag_images_from_exiftool.GeotagImagesFromExifToolWithSamples(
-            source_path=option.source_path,
+            source_path=_exiftool_xml_source_path(option),
             num_processes=option.num_processes,
         )
 
@@ -604,12 +770,8 @@ def _build_video_geotag(option: SourceOption) -> base.GeotagVideosFromGeneric |
         )
 
     elif option.source is SourceType.EXIFTOOL_XML:
-        if option.source_path is None:
-            raise exceptions.MapillaryBadParameterError(
-                "source_path must be provided for EXIFTOOL_XML source"
-            )
         return geotag_videos_from_exiftool.GeotagVideosFromExifToolXML(
-            source_path=option.source_path,
+            source_path=_exiftool_xml_source_path(option),
         )
 
     elif option.source is SourceType.GPX:

mapillary_tools/geotag/geotag_images_from_video.py

@@ -20,7 +20,7 @@
 from .base import GeotagImagesFromGeneric
 from .geotag_images_from_gpx import GeotagImagesFromGPX
 from .geotag_videos_from_video import GeotagVideosFromVideo
-from .video_extractors.gpx import GPSTimeline, GPXVideoExtractor
+from .video_extractors.gpx import GPXVideoExtractor
 
 
 LOG = logging.getLogger(__name__)
@@ -109,21 +109,15 @@ def _on_capture_timeline(
     ) -> T.Sequence[geo.Point]:
         """
         Time the video's GPS points in GPS seconds from the start of the video,
-        like the samples, which are stamped with their capture times, as its
-        GPS timestamps tell: a timelapse packs several into each second of
+        like the samples, which are stamped with their capture times, at the
+        pace of its GPS clock: a timelapse packs several into each second of
         video, and some clocks gain on the video.
         """
         clock = GPXVideoExtractor._read_video_clock(points)
-        if clock is None:
+        if clock is None or clock.rate == 1.0:
             return points
 
-        timeline = GPSTimeline.read(points, clock.rate)
-        assert timeline is not None, "a GPS clock is read from GPS timestamps"
-        start = timeline.to_unix_time(0.0)
-        return [
-            dataclasses.replace(p, time=timeline.to_unix_time(p.time) - start)
-            for p in points
-        ]
+        return [dataclasses.replace(p, time=p.time * clock.rate) for p in points]
 
 
 class GeotagImageSamplesFromVideo(GeotagImagesFromGeneric):