fix: missing samples due to out-of-sync indices (#666)

* fix: missing samples due to out-of-sync indices we ask ffmpeg to extract A,B,C frames but it extracts X,Y,Z frames * fix tests * fix comment
mapillary · Jan 30, 2024 · 5b39288 · 5b39288
1 parent 6cc9bf5
commit 5b39288
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 20 deletions.
diff --git a/mapillary_tools/ffmpeg.py b/mapillary_tools/ffmpeg.py
@@ -252,12 +252,18 @@ def extract_specified_frames(
                     *[
                         *["-filter_script:v", select_file.name],
                         # Each frame is passed with its timestamp from the demuxer to the muxer
-                        # vsync is deprecated but -fps_mode is not avaliable on some versions ;(
                         *["-vsync", "0"],
+                        # vsync is deprecated by fps_mode,
+                        # but fps_mode is not avaliable on some older versions ;(
                         # *[f"-fps_mode:{stream_specifier}", "passthrough"],
                         # Set the number of video frames to output
                         *[f"-frames:{stream_specifier}", str(len(frame_indices))],
-                        *["-frame_pts", "1"],
+                        # Disabled because it doesn't always name the sample images as expected
+                        # For example "select(n\,1)" we expected the first sample to be IMG_001.JPG
+                        # but it could be IMG_005.JPG
+                        # https://www.ffmpeg.org/ffmpeg-formats.html#Options-21
+                        # If set to 1, expand the filename with pts from pkt->pts. Default value is 0.
+                        # *["-frame_pts", "1"],
                     ],
                     # video quality level (or the alias -q:v)
                     *[f"-qscale:{stream_specifier}", "2"],

diff --git a/mapillary_tools/sample_video.py b/mapillary_tools/sample_video.py
@@ -320,35 +320,37 @@ def _sample_single_video_by_distance(
     sample_points_by_frame_idx = _sample_video_stream_by_distance(
         video_metadata.points, video_track_parser, sample_distance
     )
+    sorted_sample_indices = sorted(sample_points_by_frame_idx.keys())
 
     with wip_dir_context(wip_sample_dir(sample_dir), sample_dir) as wip_dir:
         ffmpeg.extract_specified_frames(
             video_path,
             wip_dir,
-            frame_indices=set(sample_points_by_frame_idx.keys()),
+            frame_indices=set(sorted_sample_indices),
             stream_idx=video_stream_idx,
         )
 
         frame_samples = ffmpeglib.sort_selected_samples(
             wip_dir, video_path, [video_stream_idx]
         )
-        # extract_specified_frames() produces 0-based frame indices
-        for frame_idx_0based, sample_paths in frame_samples:
-            assert len(sample_paths) == 1
-            if sample_paths[0] is None:
-                continue
-
-            sample_point = sample_points_by_frame_idx.get(frame_idx_0based)
-            if sample_point is None:
-                # this should not happen
-                LOG.warning(
-                    "The specified frame index %d was not extracted from stream index %d",
-                    frame_idx_0based,
-                    video_stream_idx,
+        if len(frame_samples) != len(sorted_sample_indices):
+            raise exceptions.MapillaryVideoError(
+                f"Expect {len(sorted_sample_indices)} samples but extracted {len(frame_samples)} samples"
+            )
+        for idx, (frame_idx_1based, sample_paths) in enumerate(frame_samples):
+            assert (
+                len(sample_paths) == 1
+            ), "Expect 1 sample path at {frame_idx_1based} but got {sample_paths}"
+            if idx + 1 != frame_idx_1based:
+                raise exceptions.MapillaryVideoError(
+                    f"Expect {sample_paths[0]} to be {idx + 1}th sample but got {frame_idx_1based}"
                 )
+
+        for (_, sample_paths), sample_idx in zip(frame_samples, sorted_sample_indices):
+            if sample_paths[0] is None:
                 continue
 
-            video_sample, interp = sample_point
+            video_sample, interp = sample_points_by_frame_idx[sample_idx]
             assert (
                 interp.time == video_sample.composition_time_offset
             ), f"interpolated time {interp.time} should match the video sample time {video_sample.composition_time_offset}"

diff --git a/tests/integration/test_process.py b/tests/integration/test_process.py
@@ -703,7 +703,7 @@ def test_video_process_sample_with_distance(setup_data: py.path.local):
                         Path(
                             sample_dir,
                             "max-360mode.mp4",
-                            "max-360mode_0_000000.jpg",
+                            "max-360mode_0_000001.jpg",
                         )
                     ),
                     "filetype": "image",
@@ -725,7 +725,7 @@ def test_video_process_sample_with_distance(setup_data: py.path.local):
                         Path(
                             sample_dir,
                             "max-360mode.mp4",
-                            "max-360mode_0_000127.jpg",
+                            "max-360mode_0_000002.jpg",
                         )
                     ),
                     "filetype": "image",
@@ -747,7 +747,7 @@ def test_video_process_sample_with_distance(setup_data: py.path.local):
                         Path(
                             sample_dir,
                             "max-360mode.mp4",
-                            "max-360mode_0_000250.jpg",
+                            "max-360mode_0_000003.jpg",
                         )
                     ),
                     "filetype": "image",