From 00533cd755906f599f4f3c208872ba48d1280b52 Mon Sep 17 00:00:00 2001 From: Tao Peng Date: Mon, 29 Jan 2024 18:06:12 -0800 Subject: [PATCH 1/3] 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 --- mapillary_tools/ffmpeg.py | 10 ++++++++-- mapillary_tools/sample_video.py | 32 +++++++++++++++++--------------- 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/mapillary_tools/ffmpeg.py b/mapillary_tools/ffmpeg.py index 0da5ef86..462f6b3d 100644 --- 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\,0)" we expected the first sample to be IMG_000.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 index 6cfdfe12..e70a370b 100644 --- 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}" From 2935c959ca7ad1ce228cabe4e23df7ef1b3e31aa Mon Sep 17 00:00:00 2001 From: Tao Peng Date: Mon, 29 Jan 2024 22:24:13 -0800 Subject: [PATCH 2/3] fix tests --- tests/integration/test_process.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/integration/test_process.py b/tests/integration/test_process.py index ff0be6fc..68083571 100644 --- 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", From 4fca47102290d1e436012f53238080862351d8b0 Mon Sep 17 00:00:00 2001 From: Tao Peng Date: Mon, 29 Jan 2024 23:11:18 -0800 Subject: [PATCH 3/3] fix comment --- mapillary_tools/ffmpeg.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mapillary_tools/ffmpeg.py b/mapillary_tools/ffmpeg.py index 462f6b3d..9ade31ee 100644 --- a/mapillary_tools/ffmpeg.py +++ b/mapillary_tools/ffmpeg.py @@ -259,7 +259,7 @@ def extract_specified_frames( # Set the number of video frames to output *[f"-frames:{stream_specifier}", str(len(frame_indices))], # Disabled because it doesn't always name the sample images as expected - # For example "select(n\,0)" we expected the first sample to be IMG_000.JPG + # 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.