From f1396a920b0c35f2a7501e891e782ac11754ac40 Mon Sep 17 00:00:00 2001 From: Breakthrough Date: Fri, 22 Nov 2024 22:22:27 -0500 Subject: [PATCH 1/3] [backends] Fix behavior of MoviePy backend There are still some fixes required in MoviePy itself to make corrupt video tests pass. --- scenedetect/backends/moviepy.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/scenedetect/backends/moviepy.py b/scenedetect/backends/moviepy.py index e85f37c4..99698fbe 100644 --- a/scenedetect/backends/moviepy.py +++ b/scenedetect/backends/moviepy.py @@ -189,7 +189,10 @@ def seek(self, target: Union[FrameTimecode, float, int]): raise SeekError("Target frame is beyond end of video!") from ex raise self._last_frame = self._reader.lastread - self._frame_number = target.frame_num + self._frame_number = min( + target.frame_num, + FrameTimecode(self._reader.infos["duration"], self.frame_rate).frame_num - 1, + ) def reset(self): """Close and re-open the VideoStream (should be equivalent to calling `seek(0)`).""" @@ -215,10 +218,10 @@ def read(self, decode: bool = True, advance: bool = True) -> Union[np.ndarray, b return self._last_frame_rgb if not hasattr(self._reader, "lastread"): return False + # TODO: In Moviepy2.0 this is broken - lastread is updated in-place in some cases. self._last_frame = self._reader.lastread - self._reader.read_frame() - if self._last_frame is self._reader.lastread: - # Didn't decode a new frame, must have hit EOF. + frame = self._reader.read_frame() + if frame is self._last_frame: if self._eof: return False self._eof = True @@ -226,5 +229,5 @@ def read(self, decode: bool = True, advance: bool = True) -> Union[np.ndarray, b if decode: if self._last_frame is not None: self._last_frame_rgb = cv2.cvtColor(self._last_frame, cv2.COLOR_BGR2RGB) - return self._last_frame_rgb - return True + return self._last_frame_rgb if not self._eof else False + return not self._eof From 0e1df2b4a7b6681c70960585d850ea51e34622f7 Mon Sep 17 00:00:00 2001 From: Breakthrough Date: Fri, 22 Nov 2024 22:31:55 -0500 Subject: [PATCH 2/3] [backends] Fix MoviePy 2.0 EOF behavior Skip corrupt video test on MoviePy awaiting Zulko/moviepy#2253 --- scenedetect/_cli/controller.py | 14 +++++++++-- scenedetect/backends/moviepy.py | 43 +++++++++++++++++++++------------ tests/test_video_stream.py | 13 ++++++++-- 3 files changed, 50 insertions(+), 20 deletions(-) diff --git a/scenedetect/_cli/controller.py b/scenedetect/_cli/controller.py index 28d52b9d..8859d60b 100644 --- a/scenedetect/_cli/controller.py +++ b/scenedetect/_cli/controller.py @@ -16,8 +16,10 @@ import os import time import typing as ty +import warnings from scenedetect._cli.context import CliContext +from scenedetect.backends import VideoStreamCv2, VideoStreamMoviePy from scenedetect.frame_timecode import FrameTimecode from scenedetect.platform import get_and_create_path from scenedetect.scene_manager import CutList, SceneList, get_scenes_from_cuts @@ -39,6 +41,11 @@ def run_scenedetect(context: CliContext): logger.debug("No input specified.") return + # Suppress warnings when reading past EOF in MoviePy. + is_debug = context.config.get_value("global", "verbosity") != "debug" + if isinstance(context.video_stream, VideoStreamMoviePy) and not is_debug: + warnings.filterwarnings("ignore", module="moviepy") + if context.load_scenes_input: # Skip detection if load-scenes was used. logger.info("Skipping detection, loading scenes from: %s", context.load_scenes_input) @@ -49,7 +56,10 @@ def run_scenedetect(context: CliContext): logger.info("Loaded %d scenes.", len(scenes)) else: # Perform scene detection on input. - scenes, cuts = _detect(context) + result = _detect(context) + if result is None: + return + scenes, cuts = result scenes = _postprocess_scene_list(context, scenes) # Handle -s/--stats option. _save_stats(context) @@ -110,7 +120,7 @@ def _detect(context: CliContext) -> ty.Optional[ty.Tuple[SceneList, CutList]]: # Handle case where video failure is most likely due to multiple audio tracks (#179). # TODO(#380): Ensure this does not erroneusly fire. - if num_frames <= 0 and context.video_stream.BACKEND_NAME == "opencv": + if num_frames <= 0 and isinstance(context.video_stream, VideoStreamCv2): logger.critical( "Failed to read any frames from video file. This could be caused by the video" " having multiple audio tracks. If so, try installing the PyAV backend:\n" diff --git a/scenedetect/backends/moviepy.py b/scenedetect/backends/moviepy.py index 99698fbe..ac368cc4 100644 --- a/scenedetect/backends/moviepy.py +++ b/scenedetect/backends/moviepy.py @@ -174,13 +174,19 @@ def seek(self, target: Union[FrameTimecode, float, int]): SeekError: An error occurs while seeking, or seeking is not supported. ValueError: `target` is not a valid value (i.e. it is negative). """ + success = False if not isinstance(target, FrameTimecode): target = FrameTimecode(target, self.frame_rate) try: - self._reader.get_frame(target.get_seconds()) + self._last_frame = self._reader.get_frame(target.get_seconds()) + if hasattr(self._reader, "last_read") and target >= self.duration: + raise SeekError("MoviePy > 2.0 does not have proper EOF semantics.") + self._frame_number = min( + target.frame_num, + FrameTimecode(self._reader.infos["duration"], self.frame_rate).frame_num - 1, + ) + success = True except OSError as ex: - # Leave the object in a valid state. - self.reset() # TODO(#380): Other backends do not currently throw an exception if attempting to seek # past EOF. We need to ensure consistency for seeking past end of video with respect to # errors and behaviour, and should probably gracefully stop at the last frame instead @@ -188,18 +194,18 @@ def seek(self, target: Union[FrameTimecode, float, int]): if target >= self.duration: raise SeekError("Target frame is beyond end of video!") from ex raise - self._last_frame = self._reader.lastread - self._frame_number = min( - target.frame_num, - FrameTimecode(self._reader.infos["duration"], self.frame_rate).frame_num - 1, - ) + finally: + # Leave the object in a valid state on any errors. + if not success: + self.reset() - def reset(self): + def reset(self, print_infos=False): """Close and re-open the VideoStream (should be equivalent to calling `seek(0)`).""" - self._reader.initialize() - self._last_frame = self._reader.read_frame() + self._last_frame = False + self._last_frame_rgb = None self._frame_number = 0 self._eof = False + self._reader = FFMPEG_VideoReader(self._path, print_infos=print_infos) def read(self, decode: bool = True, advance: bool = True) -> Union[np.ndarray, bool]: """Read and decode the next frame as a np.ndarray. Returns False when video ends. @@ -213,13 +219,17 @@ def read(self, decode: bool = True, advance: bool = True) -> Union[np.ndarray, b If decode = False, a bool indicating if advancing to the the next frame succeeded. """ if not advance: + last_frame_valid = self._last_frame is not None and self._last_frame is not False + if not last_frame_valid: + return False if self._last_frame_rgb is None: self._last_frame_rgb = cv2.cvtColor(self._last_frame, cv2.COLOR_BGR2RGB) return self._last_frame_rgb - if not hasattr(self._reader, "lastread"): + if not hasattr(self._reader, "lastread") or self._eof: return False - # TODO: In Moviepy2.0 this is broken - lastread is updated in-place in some cases. - self._last_frame = self._reader.lastread + has_last_read = hasattr(self._reader, "last_read") + self._last_frame = self._reader.last_read if has_last_read else self._reader.lastread + # Read the *next* frame for the following call to read, and to check for EOF. frame = self._reader.read_frame() if frame is self._last_frame: if self._eof: @@ -227,7 +237,8 @@ def read(self, decode: bool = True, advance: bool = True) -> Union[np.ndarray, b self._eof = True self._frame_number += 1 if decode: - if self._last_frame is not None: + last_frame_valid = self._last_frame is not None and self._last_frame is not False + if last_frame_valid: self._last_frame_rgb = cv2.cvtColor(self._last_frame, cv2.COLOR_BGR2RGB) - return self._last_frame_rgb if not self._eof else False + return self._last_frame_rgb return not self._eof diff --git a/tests/test_video_stream.py b/tests/test_video_stream.py index 1d2074b0..161ed6cd 100644 --- a/tests/test_video_stream.py +++ b/tests/test_video_stream.py @@ -20,6 +20,7 @@ from dataclasses import dataclass from typing import List, Type +import moviepy import numpy import pytest @@ -41,6 +42,8 @@ # The warning occurs when reading the last frame, which VideoStreamMoviePy handles gracefully. MOVIEPY_WARNING_FILTER = "ignore:.*Using the last valid frame instead.:UserWarning" +MOVIEPY_MAJOR_VERSION = int(moviepy.__version__.split(".")[0]) + def calculate_frame_delta(frame_a, frame_b, roi=None) -> float: if roi: @@ -354,10 +357,16 @@ def test_corrupt_video(vs_type: Type[VideoStream], corrupt_video_file: str): """Test that backend handles video with corrupt frame gracefully with defaults.""" if vs_type == VideoManager: pytest.skip(reason="VideoManager does not support handling corrupt videos.") + if vs_type == VideoStreamMoviePy and MOVIEPY_MAJOR_VERSION >= 2: + # Due to changes in MoviePy 2.0, loading this file causes an exception to be thrown. + # See https://github.com/Zulko/moviepy/pull/2253 for a PR that attempts to more gracefully + # handle this case, however even once that is fixed, we will be unable to run this test + # on certain versions of MoviePy. + pytest.skip(reason="https://github.com/Zulko/moviepy/pull/2253") stream = vs_type(corrupt_video_file) - # OpenCV usually fails to read the video at frame 45, so we make sure all backends can - # get to 60 without reporting a failure. + # OpenCV usually fails to read the video at frame 45, but the remaining frames all seem to + # decode just fine. Make sure all backends can get to 60 without reporting a failure. for frame in range(60): assert stream.read() is not False, "Failed on frame %d!" % frame From 10fba27d074d9d4a471396adb6773a5e069d4355 Mon Sep 17 00:00:00 2001 From: Breakthrough Date: Sat, 23 Nov 2024 13:20:35 -0500 Subject: [PATCH 3/3] [build] Enable Moviepy tests for builds and document workarounds for #461 --- .github/workflows/build.yml | 6 ++++++ scenedetect/_cli/controller.py | 9 +++++---- scenedetect/backends/moviepy.py | 3 ++- tests/test_video_stream.py | 11 +++++++---- 4 files changed, 20 insertions(+), 9 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 9a05953e..c21ce269 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -62,6 +62,12 @@ jobs: pip install av opencv-python-headless --only-binary :all: pip install -r requirements_headless.txt + - name: Install MoviePy + # TODO: We can only run MoviePy tests on systems that have ffmpeg. + if: ${{ runner.arch == 'X64' }} + run: | + pip install moviepy + - name: Checkout test resources run: | git fetch --depth=1 https://github.com/Breakthrough/PySceneDetect.git refs/heads/resources:refs/remotes/origin/resources diff --git a/scenedetect/_cli/controller.py b/scenedetect/_cli/controller.py index 8859d60b..d9947d92 100644 --- a/scenedetect/_cli/controller.py +++ b/scenedetect/_cli/controller.py @@ -41,10 +41,11 @@ def run_scenedetect(context: CliContext): logger.debug("No input specified.") return - # Suppress warnings when reading past EOF in MoviePy. - is_debug = context.config.get_value("global", "verbosity") != "debug" - if isinstance(context.video_stream, VideoStreamMoviePy) and not is_debug: - warnings.filterwarnings("ignore", module="moviepy") + # Suppress warnings when reading past EOF in MoviePy (#461). + if VideoStreamMoviePy and isinstance(context.video_stream, VideoStreamMoviePy): + is_debug = context.config.get_value("global", "verbosity") != "debug" + if not is_debug: + warnings.filterwarnings("ignore", module="moviepy") if context.load_scenes_input: # Skip detection if load-scenes was used. diff --git a/scenedetect/backends/moviepy.py b/scenedetect/backends/moviepy.py index ac368cc4..c3fb0935 100644 --- a/scenedetect/backends/moviepy.py +++ b/scenedetect/backends/moviepy.py @@ -180,7 +180,7 @@ def seek(self, target: Union[FrameTimecode, float, int]): try: self._last_frame = self._reader.get_frame(target.get_seconds()) if hasattr(self._reader, "last_read") and target >= self.duration: - raise SeekError("MoviePy > 2.0 does not have proper EOF semantics.") + raise SeekError("MoviePy > 2.0 does not have proper EOF semantics (#461).") self._frame_number = min( target.frame_num, FrameTimecode(self._reader.infos["duration"], self.frame_rate).frame_num - 1, @@ -228,6 +228,7 @@ def read(self, decode: bool = True, advance: bool = True) -> Union[np.ndarray, b if not hasattr(self._reader, "lastread") or self._eof: return False has_last_read = hasattr(self._reader, "last_read") + # In MoviePy 2.0 there is a separate property we need to read named differently (#461). self._last_frame = self._reader.last_read if has_last_read else self._reader.lastread # Read the *next* frame for the following call to read, and to check for EOF. frame = self._reader.read_frame() diff --git a/tests/test_video_stream.py b/tests/test_video_stream.py index 161ed6cd..894d60f6 100644 --- a/tests/test_video_stream.py +++ b/tests/test_video_stream.py @@ -20,7 +20,6 @@ from dataclasses import dataclass from typing import List, Type -import moviepy import numpy import pytest @@ -42,7 +41,11 @@ # The warning occurs when reading the last frame, which VideoStreamMoviePy handles gracefully. MOVIEPY_WARNING_FILTER = "ignore:.*Using the last valid frame instead.:UserWarning" -MOVIEPY_MAJOR_VERSION = int(moviepy.__version__.split(".")[0]) + +def get_moviepy_major_version() -> int: + import moviepy + + return int(moviepy.__version__.split(".")[0]) def calculate_frame_delta(frame_a, frame_b, roi=None) -> float: @@ -357,8 +360,8 @@ def test_corrupt_video(vs_type: Type[VideoStream], corrupt_video_file: str): """Test that backend handles video with corrupt frame gracefully with defaults.""" if vs_type == VideoManager: pytest.skip(reason="VideoManager does not support handling corrupt videos.") - if vs_type == VideoStreamMoviePy and MOVIEPY_MAJOR_VERSION >= 2: - # Due to changes in MoviePy 2.0, loading this file causes an exception to be thrown. + if vs_type == VideoStreamMoviePy and get_moviepy_major_version() >= 2: + # Due to changes in MoviePy 2.0 (#461), loading this file causes an exception to be thrown. # See https://github.com/Zulko/moviepy/pull/2253 for a PR that attempts to more gracefully # handle this case, however even once that is fixed, we will be unable to run this test # on certain versions of MoviePy.