From c7c25f18ca216949f7935998ef07996e3ebfcecb Mon Sep 17 00:00:00 2001 From: vishal-tambi Date: Wed, 21 Oct 2026 17:34:16 +0530 Subject: [PATCH 1/2] [backends] Add VideoStream.close() and context manager support #568 --- scenedetect/backends/__init__.py | 9 ++ scenedetect/backends/concat.py | 70 ++++++++++---- scenedetect/backends/moviepy.py | 14 ++- scenedetect/backends/opencv.py | 22 +++++ scenedetect/backends/pyav.py | 62 ++++++++---- scenedetect/video_stream.py | 53 ++++++++++ tests/helpers.py | 27 +----- tests/test_backend_opencv.py | 12 +++ tests/test_backend_pyav.py | 28 ++++++ tests/test_concat.py | 160 ++++++++++++++++++++++++++----- tests/test_video_stream.py | 69 +++++++++++++ website/pages/changelog.md | 2 + 12 files changed, 440 insertions(+), 88 deletions(-) diff --git a/scenedetect/backends/__init__.py b/scenedetect/backends/__init__.py index bad4b972..cdcbb734 100644 --- a/scenedetect/backends/__init__.py +++ b/scenedetect/backends/__init__.py @@ -50,6 +50,15 @@ In both examples above, the resulting ``video`` can be used with :meth:`SceneManager.detect_scenes() `. +Every backend can be closed with :meth:`close() ` +to release the file handles and decoders it holds, or used as a context manager to close it +automatically: + +.. code:: python + + with open_video("video.mp4") as video: + scene_manager.detect_scenes(video=video) + Multiple videos can be opened as one continuous stream by passing a list of paths to :func:`open_video`, which returns a :class:`VideoStreamConcat `: diff --git a/scenedetect/backends/concat.py b/scenedetect/backends/concat.py index af335a2e..f6aa73fa 100644 --- a/scenedetect/backends/concat.py +++ b/scenedetect/backends/concat.py @@ -136,26 +136,37 @@ def __init__( # Probe all inputs up front for validation and metadata, then only keep one source # open at a time for decoding. The handle probed for the first source is kept as the - # initial decode source to avoid re-opening it. + # initial decode source to avoid re-opening it, and the others are closed once probed. self._sources: list[_SourceMetadata] = [] first_cap: VideoStream | None = None - for index in range(len(self._paths)): - cap = self._open_source(index) - duration = cap.duration - declared_seconds = _exact_seconds(duration) if duration is not None else Fraction(0) - self._sources.append( - _SourceMetadata( - path=self._paths[index], - frame_size=cap.frame_size, - frame_rate=cap.frame_rate, - duration=declared_seconds, - frames=duration.frame_num if duration is not None else 0, - aspect_ratio=cap.aspect_ratio, + cap: VideoStream | None = None + try: + for index in range(len(self._paths)): + cap = self._open_source(index) + duration = cap.duration + declared_seconds = _exact_seconds(duration) if duration is not None else Fraction(0) + self._sources.append( + _SourceMetadata( + path=self._paths[index], + frame_size=cap.frame_size, + frame_rate=cap.frame_rate, + duration=declared_seconds, + frames=duration.frame_num if duration is not None else 0, + aspect_ratio=cap.aspect_ratio, + ) ) - ) - if index == 0: - first_cap = cap - self._validate_sources() + if index == 0: + first_cap = cap + else: + cap.close() + self._validate_sources() + except BaseException: + # Don't leave any probed source open if probing or validation fails. Closing is + # idempotent, so it doesn't matter if `cap` was already closed or is `first_cap`. + for opened in (cap, first_cap): + if opened is not None: + opened.close() + raise # Global start time of each source in exact rational seconds. Has one extra entry at # the end holding the total (declared) duration. Values after the current source are @@ -203,6 +214,13 @@ def _open_source(self, index: int) -> VideoStream: str(self._paths[index]), self._frame_rate_override, **self._backend_kwargs ) + def _switch_source(self, index: int): + """Make the source at `index` the one being decoded, closing the outgoing source. The + new source is opened first, so the outgoing one is kept if that fails.""" + cap = self._open_source(index) + self._cap.close() + self._cap = cap + def _child_position_seconds(self) -> Fraction: """Position of the current source as exact rational seconds (local timeline).""" return _exact_seconds(self._cap.position) @@ -226,6 +244,7 @@ def _finish_current_source(self): def read(self, decode: bool = True) -> np.ndarray | bool: """Read/decode the next frame. Returns False when all inputs have been processed.""" + self._check_open() while True: result = self._cap.read(decode=decode) if result is not False: @@ -236,11 +255,12 @@ def read(self, decode: bool = True) -> np.ndarray | bool: self._finish_current_source() self._index += 1 logger.debug("Processing complete, opening next video: %s", self._paths[self._index]) - self._cap = self._open_source(self._index) + self._switch_source(self._index) def seek(self, target: TimecodeLike): """Seek to `target` on the global timeline. Supports seeking across sources in either direction.""" + self._check_open() if not isinstance(target, FrameTimecode): target = FrameTimecode(target, self.frame_rate) if target < 0: @@ -253,16 +273,26 @@ def seek(self, target: TimecodeLike): self._decode_failures_prior += self._cap.decode_failures self._frames_prior = sum(source.frames for source in self._sources[:index]) self._index = index - self._cap = self._open_source(index) + self._switch_source(index) local_seconds = target_seconds - self._offsets[index] self._cap.seek(float(local_seconds)) def reset(self): """Close and re-open the stream (equivalent to seeking back to the beginning).""" + self._check_open() self._index = 0 self._frames_prior = 0 self._decode_failures_prior = 0 - self._cap = self._open_source(0) + self._switch_source(0) + + def close(self) -> None: + """Close the stream, closing the input video currently open for decoding (only one is + kept open at a time). Has no effect if the stream is already closed. See + :meth:`VideoStream.close` for details.""" + if self._closed: + return + self._closed = True + self._cap.close() # # VideoStream Properties diff --git a/scenedetect/backends/moviepy.py b/scenedetect/backends/moviepy.py index d3167b26..5afa27ad 100644 --- a/scenedetect/backends/moviepy.py +++ b/scenedetect/backends/moviepy.py @@ -239,8 +239,10 @@ def seek(self, target: TimecodeLike): If int, interpreted as frame number. Raises: SeekError: An error occurs while seeking, or seeking is not supported. - ValueError: `target` is not a valid value (i.e. it is negative). + ValueError: `target` is not a valid value (i.e. it is negative), or the stream is + closed. """ + self._check_open() success = False if not isinstance(target, FrameTimecode): target = FrameTimecode(target, self.frame_rate) @@ -273,6 +275,7 @@ def seek(self, target: TimecodeLike): def reset(self, print_infos=False): """Close and re-open the VideoStream (should be equivalent to calling `seek(0)`).""" + self._check_open() self._last_frame = False self._last_frame_rgb = None self._frame_number = 0 @@ -281,7 +284,16 @@ def reset(self, print_infos=False): "reset", lambda: FFMPEG_VideoReader(self._path, print_infos=print_infos) ) + def close(self) -> None: + """Close the video, terminating the ffmpeg process used to decode it. Has no effect if + the stream is already closed. See :meth:`VideoStream.close` for details.""" + if self._closed: + return + self._closed = True + self._reader.close() + def read(self, decode: bool = True) -> np.ndarray | bool: + self._check_open() if not hasattr(self._reader, "lastread") or self._eof: return False has_last_read = hasattr(self._reader, "last_read") diff --git a/scenedetect/backends/opencv.py b/scenedetect/backends/opencv.py index 5abe78c6..3b470c08 100644 --- a/scenedetect/backends/opencv.py +++ b/scenedetect/backends/opencv.py @@ -215,6 +215,8 @@ def is_seekable(self) -> bool: @property def frame_size(self) -> tuple[int, int]: """Size of each video frame in pixels as a tuple of (width, height).""" + if self._closed: + return self._closed_frame_size return ( math.trunc(self._cap.get(cv2.CAP_PROP_FRAME_WIDTH)), math.trunc(self._cap.get(cv2.CAP_PROP_FRAME_HEIGHT)), @@ -225,11 +227,15 @@ def duration(self) -> FrameTimecode | None: """Duration of the stream as a FrameTimecode, or None if non terminating.""" if self._is_device: return None + if self._closed: + return self.base_timecode + self._closed_frame_count return self.base_timecode + math.trunc(self._cap.get(cv2.CAP_PROP_FRAME_COUNT)) @property def aspect_ratio(self) -> float: """Display/pixel aspect ratio as a float (1.0 represents square pixels).""" + if self._closed: + return self._closed_aspect_ratio return _get_aspect_ratio(self._cap) @property @@ -266,6 +272,7 @@ def frame_number(self) -> int: return math.trunc(self._cap.get(cv2.CAP_PROP_POS_FRAMES)) def seek(self, target: TimecodeLike): + self._check_open() if self._is_device: raise SeekError("Cannot seek if input is a device!") if not isinstance(target, FrameTimecode): @@ -307,10 +314,20 @@ def seek(self, target: TimecodeLike): def reset(self): """Close and re-open the VideoStream (should be equivalent to calling `seek(0)`).""" + self._check_open() self._cap.release() self._open_capture(float(self._frame_rate)) + def close(self) -> None: + """Close the video, releasing the underlying `cv2.VideoCapture`. Has no effect if the + stream is already closed. See :meth:`VideoStream.close` for details.""" + if self._closed: + return + self._closed = True + self._cap.release() + def read(self, decode: bool = True) -> np.ndarray | bool: + self._check_open() if not self._cap.isOpened(): return False # At the end of some VFR streams OpenCV reports an earlier CAP_PROP_POS_MSEC once grab() @@ -401,6 +418,11 @@ def _open_capture(self, frame_rate: FrameRate | None = None): self._has_grabbed = False self._eof_timecode: Timecode | None = None cap.set(cv2.CAP_PROP_ORIENTATION_AUTO, 1.0) # https://github.com/opencv/opencv/issues/26795 + # A released capture reports 0 for every property, so record those describing the video + # for after `close()`. While open, they are still read from the capture. + self._closed_frame_size = self.frame_size + self._closed_frame_count = math.trunc(cap.get(cv2.CAP_PROP_FRAME_COUNT)) + self._closed_aspect_ratio = self.aspect_ratio class VideoCaptureAdapter(VideoStream): diff --git a/scenedetect/backends/pyav.py b/scenedetect/backends/pyav.py index 4656bbd1..70caeaaf 100644 --- a/scenedetect/backends/pyav.py +++ b/scenedetect/backends/pyav.py @@ -11,6 +11,7 @@ # """:class:`VideoStreamAv` provides an adapter for the PyAV av.InputContainer object.""" +import contextlib import os import typing as ty import warnings @@ -116,7 +117,7 @@ def __init__( self._reopened = True self._decode_failures = 0 self._warning_displayed = False - # True if `_io` was opened by this object (from a path) and must be closed in `__del__`. + # True if `_io` was opened by this object (from a path) and must be closed in `close()`. # Set before anything below can raise so a partially constructed object is safe to finalize. self._owns_io = False @@ -135,7 +136,7 @@ def __init__( try: if isinstance(path_or_io, (str, os.PathLike)): self._path: str = os.fspath(path_or_io) - # File handle is tied to the lifetime of this object and released in `__del__`. + # File handle is owned by this object and released in `close()` (or `__del__`). self._io = open(self._path, "rb") # noqa: SIM115 self._owns_io = True if not self._name: @@ -172,27 +173,40 @@ def __init__( # Calculate duration after we have set the framerate. self._duration_frames = self._get_duration() + # Properties read from the container are recorded so they remain available after `close()`. + self._closed_frame_size = self.frame_size + self._closed_aspect_ratio = self.aspect_ratio def __del__(self): # Finalizers must never raise - an exception here becomes an unraisable error. During # interpreter shutdown the underlying handles are reclaimed by the OS anyway. - try: - # Close the decode generator first to break its cycle with the container. - decoder = getattr(self, "_decoder", None) - if decoder is not None: - decoder.close() - # `_container` is unset if `__init__` raised before `av.open()` succeeded. - container = getattr(self, "_container", None) - if container is not None: - container.close() - # Closing the container does not close the file object backing it. Only close `_io` - # if we opened it ourselves; file-like objects passed by the caller remain theirs. - if getattr(self, "_owns_io", False): - io = getattr(self, "_io", None) - if io is not None: - io.close() - except Exception: - pass + with contextlib.suppress(Exception): + self.close() + + def close(self) -> None: + """Close the video, releasing the decoder, the container, and the file handle opened if + `path_or_io` is a path. File-like objects passed by the caller are left open. Has no + effect if the stream is already closed. See :meth:`VideoStream.close` for details.""" + if self._closed: + return + self._closed = True + self._frame = None + # Attributes below may be unset if `__init__` raised, since `__del__` calls this method. + # Close the decode generator first to break its cycle with the container. + decoder = getattr(self, "_decoder", None) + if decoder is not None: + decoder.close() + self._decoder = None + # `_container` is unset if `__init__` raised before `av.open()` succeeded. + container = getattr(self, "_container", None) + if container is not None: + container.close() + # Closing the container does not close the file object backing it. Only close `_io` + # if we opened it ourselves; file-like objects passed by the caller remain theirs. + if getattr(self, "_owns_io", False): + io = getattr(self, "_io", None) + if io is not None: + io.close() # # VideoStream Methods/Properties @@ -219,6 +233,8 @@ def is_seekable(self) -> bool: @property def frame_size(self) -> tuple[int, int]: """Size of each video frame in pixels as a tuple of (width, height).""" + if self._closed: + return self._closed_frame_size return (self._codec_context.width, self._codec_context.height) @property @@ -276,6 +292,8 @@ def time_base(self) -> Fraction | None: @property def aspect_ratio(self) -> float: """Pixel aspect ratio as a float (1.0 represents square pixels).""" + if self._closed: + return self._closed_aspect_ratio # Unset is None on PyAV < 19 and a falsy AVRational(0, 1) on PyAV 19+. dar = getattr(self._codec_context, "display_aspect_ratio", None) if not dar or dar.denominator <= 0: @@ -301,8 +319,10 @@ def seek(self, target: TimecodeLike) -> None: If float, interpreted as time in seconds. If int, interpreted as frame number. Raises: - ValueError: `target` is not a valid value (i.e. it is negative). + ValueError: `target` is not a valid value (i.e. it is negative), or the stream is + closed. """ + self._check_open() if not isinstance(target, FrameTimecode): target = FrameTimecode(target, self.frame_rate) if target < 0: @@ -326,6 +346,7 @@ def seek(self, target: TimecodeLike) -> None: def reset(self): """Close and re-open the VideoStream (should be equivalent to calling `seek(0)`).""" + self._check_open() self._container.close() self._frame = None self._decoder = None @@ -335,6 +356,7 @@ def reset(self): raise VideoOpenFailure() from ex def read(self, decode: bool = True) -> np.ndarray | bool: + self._check_open() consecutive_failures = 0 while True: # Reuse a persistent decoder generator so the codec's internal frame buffer (used for diff --git a/scenedetect/video_stream.py b/scenedetect/video_stream.py index 71af9950..8912817b 100644 --- a/scenedetect/video_stream.py +++ b/scenedetect/video_stream.py @@ -27,6 +27,18 @@ You can also optionally specify a framerate and a specific backend library to use. Unless specified, OpenCV will be used as the video backend. See :mod:`scenedetect.backends` for a detailed example. +A :class:`VideoStream` holds resources such as file handles and decoders until it is closed. +Call :meth:`VideoStream.close` once done with it, or use it as a context manager to close it +automatically: + +.. code:: python + + from scenedetect import open_video + with open_video('video.mp4') as video: + while video.read() is not False: + pass + print("Read %d frames" % video.frame_number) + New :class:`VideoStream ` implementations can be tested by adding it to the test suite in `tests/test_video_stream.py`. """ @@ -34,6 +46,7 @@ import typing as ty from abc import ABC, abstractmethod from fractions import Fraction +from types import TracebackType import numpy as np @@ -75,6 +88,8 @@ def __init__(self): ## VideoStream Interface (Base Class) ## +_VideoStreamT = ty.TypeVar("_VideoStreamT", bound="VideoStream") + class VideoStream(ABC): """Interface which all video backends must implement.""" @@ -87,6 +102,10 @@ class VideoStream(ABC): """Cumulative count of frames which failed to decode. Backends which can detect and skip corrupt frames increment this as an instance attribute.""" + _closed: bool = False + """True once the stream has been closed. Backends which implement `close()` set this as an + instance attribute, and call `_check_open()` from methods which require an open stream.""" + @property def base_timecode(self) -> FrameTimecode: """FrameTimecode object to use as a time base.""" @@ -98,6 +117,40 @@ def decode_failures(self) -> int: corruption). Always 0 for backends which do not track decode failures.""" return self._decode_failures + def close(self) -> None: # noqa: B027 - Not abstract, so existing subclasses remain valid. + """Close the stream, releasing any resources it holds (e.g. file handles, decoders). + + Calling `close()` more than once has no effect. Once closed, `read()`, `seek()`, and + `reset()` raise `ValueError`; the stream is never re-opened. Properties describing the + video itself (`path`, `name`, `frame_rate`, `duration`, `frame_size`, and `aspect_ratio`) + remain available, but the values of all others (e.g. `position`) are undefined. + + A stream can also be used as a context manager, which closes it on exit: + + .. code:: python + + with open_video("video.mp4") as video: + scene_manager.detect_scenes(video=video) + + The default implementation does nothing. Backends which hold resources override it. + """ + + def __enter__(self: _VideoStreamT) -> _VideoStreamT: + return self + + def __exit__( + self, + exc_type: type[BaseException] | None, + exc_value: BaseException | None, + traceback: TracebackType | None, + ) -> None: + self.close() + + def _check_open(self) -> None: + """Raise `ValueError` if the stream has been closed.""" + if self._closed: + raise ValueError("I/O operation on closed video stream.") + # # Backend Identification # diff --git a/tests/helpers.py b/tests/helpers.py index 00d33add..2500b4e5 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -11,7 +11,6 @@ # """Shared test helpers.""" -import contextlib import typing as ty from click.testing import CliRunner @@ -24,30 +23,10 @@ def close_video_stream(stream: ty.Any) -> None: """Deterministically release a VideoStream's native resources. - `VideoStream` has no public close()/context-manager API, so tests release the - backend-specific handles directly. Closing while the interpreter is healthy avoids - ResourceWarnings (and native teardown work) at interpreter shutdown. Safe to call - multiple times; never raises. + Closing while the interpreter is healthy avoids ResourceWarnings (and native teardown + work) at interpreter shutdown. Safe to call multiple times, as `close()` is idempotent. """ - backend = getattr(stream, "BACKEND_NAME", None) - if backend == "pyav": - # Close the decode generator first to break its cycle with the container. `_io` is - # the file handle backing the container (opened by the stream when given a path). - for attr in ("_decoder", "_container", "_io"): - handle = getattr(stream, attr, None) - if handle is not None: - with contextlib.suppress(Exception): - handle.close() - elif backend == "opencv": - cap = getattr(stream, "_cap", None) - if cap is not None: - with contextlib.suppress(Exception): - cap.release() - elif backend == "moviepy": - reader = getattr(stream, "_reader", None) - if reader is not None: - with contextlib.suppress(Exception): - reader.close() + stream.close() def invoke_cli(args: list[str], catch_exceptions: bool = False) -> tuple[int, str]: diff --git a/tests/test_backend_opencv.py b/tests/test_backend_opencv.py index 4b0f2dac..63cb5c37 100644 --- a/tests/test_backend_opencv.py +++ b/tests/test_backend_opencv.py @@ -77,6 +77,18 @@ def test_capture_adapter(test_movie_clip: str): assert [start.frame_num for (start, _) in scenes] == GROUND_TRUTH_CAPTURE_ADAPTER_TEST +def test_capture_adapter_close_leaves_capture_open(test_movie_clip: str): + """The VideoCapture wrapped by a VideoCaptureAdapter belongs to the caller, so closing the + adapter (here by leaving a `with` block) must not release it.""" + cap = cv2.VideoCapture(test_movie_clip) + try: + with VideoCaptureAdapter(cap) as adapter: + assert adapter.read() is not False + assert cap.isOpened() + finally: + cap.release() + + def test_capture_adapter_framerate_legacy_alias(test_movie_clip: str): """`framerate=` is the deprecated alias for `frame_rate=` on VideoCaptureAdapter.""" cap = cv2.VideoCapture(test_movie_clip) diff --git a/tests/test_backend_pyav.py b/tests/test_backend_pyav.py index cec9382a..c8654c2c 100644 --- a/tests/test_backend_pyav.py +++ b/tests/test_backend_pyav.py @@ -55,6 +55,34 @@ def test_video_stream_pyav_del_closes_owned_file_handle(test_video_file: str): assert handle.closed +def test_close_leaves_caller_file_open(test_video_file: str): + """`close()` must not close a file-like object supplied by the caller (#568).""" + with open(test_video_file, mode="rb") as video_file: + stream = VideoStreamAv(path_or_io=video_file, threading_mode=None) + assert stream.read() is not False + stream.close() + assert not video_file.closed + + +def test_close_releases_owned_file_handle(test_video_file: str, monkeypatch: pytest.MonkeyPatch): + """`close()` must release the file handle the stream opened from a path (#568), without + waiting for the stream to be finalized.""" + opened = [] + + # Record the handle as the backend opens it, rather than reaching into the stream. + def recording_open(*args, **kwargs): + handle = open(*args, **kwargs) # noqa: SIM115 + opened.append(handle) + return handle + + monkeypatch.setattr("scenedetect.backends.pyav.open", recording_open, raising=False) + stream = VideoStreamAv(test_video_file, threading_mode=None) + assert len(opened) == 1 + assert not opened[0].closed + stream.close() + assert opened[0].closed + + def _make_invalid_data_error() -> Exception: # AVERROR_INVALIDDATA ("Invalid data found when processing input"). return av.error.InvalidDataError( # type: ignore[attr-defined] diff --git a/tests/test_concat.py b/tests/test_concat.py index 9e3c18b7..b247d255 100644 --- a/tests/test_concat.py +++ b/tests/test_concat.py @@ -13,11 +13,14 @@ Validates the multi-video concatenation logic in `scenedetect.backends.concat`.""" +import typing as ty + import pytest from scenedetect import SceneManager, ThresholdDetector, open_video from scenedetect.backends import AVAILABLE_BACKENDS from scenedetect.backends.concat import VideoStreamConcat +from scenedetect.backends.opencv import VideoStreamCv2 from scenedetect.video_stream import VideoOpenFailure FADES_TOTAL_FRAMES = 250 @@ -26,10 +29,33 @@ BACKENDS = [backend for backend in ("opencv", "pyav") if backend in AVAILABLE_BACKENDS] +class _TrackedStream(VideoStreamCv2): + """OpenCV backend which records every stream it opens, and whether it has been closed.""" + + instances: ty.ClassVar[list["_TrackedStream"]] = [] + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.is_closed = False + _TrackedStream.instances.append(self) + + def close(self) -> None: + super().close() + self.is_closed = True + + +@pytest.fixture +def tracked_streams(monkeypatch: pytest.MonkeyPatch) -> list[_TrackedStream]: + """Registers `_TrackedStream` as the `tracked` backend, returning the streams it opens.""" + monkeypatch.setattr(_TrackedStream, "instances", []) + monkeypatch.setitem(AVAILABLE_BACKENDS, "tracked", _TrackedStream) + return _TrackedStream.instances + + @pytest.mark.parametrize("backend", BACKENDS) -def test_decode_single(test_fades_clip, backend): +def test_decode_single(test_fades_clip, backend, auto_close): """Decode a single video and validate the reported frame count and position.""" - video = VideoStreamConcat([test_fades_clip], backend=backend) + video = auto_close(VideoStreamConcat([test_fades_clip], backend=backend)) while video.read(decode=False) is not False: pass assert video.frame_number == FADES_TOTAL_FRAMES @@ -37,10 +63,10 @@ def test_decode_single(test_fades_clip, backend): @pytest.mark.parametrize("backend", BACKENDS) -def test_decode_multiple(test_fades_clip, backend): +def test_decode_multiple(test_fades_clip, backend, auto_close): """Decode multiple videos and validate the reported frame count.""" splice_amount = 3 - video = VideoStreamConcat([test_fades_clip] * splice_amount, backend=backend) + video = auto_close(VideoStreamConcat([test_fades_clip] * splice_amount, backend=backend)) while video.read(decode=False) is not False: pass assert video.frame_number == FADES_TOTAL_FRAMES * splice_amount @@ -48,9 +74,9 @@ def test_decode_multiple(test_fades_clip, backend): @pytest.mark.parametrize("backend", BACKENDS) -def test_seam_monotonicity(test_fades_clip, backend): +def test_seam_monotonicity(test_fades_clip, backend, auto_close): """Position must be strictly increasing across the file seam.""" - video = VideoStreamConcat([test_fades_clip] * 2, backend=backend) + video = auto_close(VideoStreamConcat([test_fades_clip] * 2, backend=backend)) last_seconds = -1.0 max_delta = 0.0 while video.read(decode=False) is not False: @@ -65,9 +91,9 @@ def test_seam_monotonicity(test_fades_clip, backend): @pytest.mark.parametrize("backend", BACKENDS) -def test_seek(test_fades_clip, backend): +def test_seek(test_fades_clip, backend, auto_close): """Seeking should work on the global timeline, in either direction, across sources.""" - video = VideoStreamConcat([test_fades_clip] * 2, backend=backend) + video = auto_close(VideoStreamConcat([test_fades_clip] * 2, backend=backend)) # Seek into the second source. target = FADES_DURATION + 5.0 video.seek(target) @@ -79,10 +105,10 @@ def test_seek(test_fades_clip, backend): assert abs(video.position.seconds - 5.0) < 0.25 -def test_seek_backward_then_cross_seam(test_fades_clip): +def test_seek_backward_then_cross_seam(test_fades_clip, auto_close): """Crossing the seam a second time after a backward seek must not shift the timeline again (offset correction must be idempotent).""" - video = VideoStreamConcat([test_fades_clip] * 2) + video = auto_close(VideoStreamConcat([test_fades_clip] * 2)) # Read across the seam once. video.seek(FADES_DURATION - 0.5) while video.position.seconds < FADES_DURATION + 0.5: @@ -98,10 +124,10 @@ def test_seek_backward_then_cross_seam(test_fades_clip): assert abs(video.position.seconds - first_pass) < 0.25 -def test_seam_monotonicity_vfr(test_vfr_drop3_video): +def test_seam_monotonicity_vfr(test_vfr_drop3_video, auto_close): """Position must also be strictly increasing across the seam between variable framerate inputs, whose declared duration is less exact than CFR.""" - video = VideoStreamConcat([test_vfr_drop3_video] * 2) + video = auto_close(VideoStreamConcat([test_vfr_drop3_video] * 2)) last_seconds = -1.0 while video.read(decode=False) is not False: seconds = video.position.seconds @@ -109,9 +135,9 @@ def test_seam_monotonicity_vfr(test_vfr_drop3_video): last_seconds = seconds -def test_map_span(test_fades_clip): +def test_map_span(test_fades_clip, auto_close): """A span crossing the seam between two inputs must map to two local spans.""" - video = VideoStreamConcat([test_fades_clip] * 2) + video = auto_close(VideoStreamConcat([test_fades_clip] * 2)) duration = FADES_DURATION start = video.base_timecode + (duration - 3.0) end = video.base_timecode + (duration + 3.0) @@ -133,31 +159,31 @@ def test_mismatched_resolution(test_fades_clip, test_video_file): VideoStreamConcat([test_fades_clip, test_video_file]) -def test_unknown_backend_falls_back(test_fades_clip): +def test_unknown_backend_falls_back(test_fades_clip, auto_close): """An unknown backend name falls back to OpenCV instead of failing.""" - video = VideoStreamConcat([test_fades_clip], backend="not_a_backend") + video = auto_close(VideoStreamConcat([test_fades_clip], backend="not_a_backend")) assert video.child_backend == "opencv" assert video.read(decode=False) is not False -def test_open_video_list(test_fades_clip): +def test_open_video_list(test_fades_clip, auto_close): """`open_video` accepts a list of paths and returns a concatenated stream.""" - video = open_video([test_fades_clip, test_fades_clip]) + video = auto_close(open_video([test_fades_clip, test_fades_clip])) assert isinstance(video, VideoStreamConcat) assert video.duration.seconds == pytest.approx(2 * FADES_DURATION, abs=0.1) # A single-element list also returns a concatenated stream. - video = open_video([test_fades_clip]) + video = auto_close(open_video([test_fades_clip])) assert isinstance(video, VideoStreamConcat) -def test_scene_manager_detect(test_fades_clip): +def test_scene_manager_detect(test_fades_clip, auto_close): """The concatenated stream must work end-to-end with SceneManager: detecting fades over two spliced copies must find twice as many scenes as a single copy.""" def detect_scenes(paths): scene_manager = SceneManager() scene_manager.add_detector(ThresholdDetector()) - video = open_video(paths) + video = auto_close(open_video(paths)) scene_manager.detect_scenes(video=video) return scene_manager.get_scene_list() @@ -168,10 +194,98 @@ def detect_scenes(paths): @pytest.mark.skipif("pyav" not in AVAILABLE_BACKENDS, reason="PyAV backend not available") -def test_corrupt_concat(corrupt_video_file): +def test_corrupt_concat(corrupt_video_file, auto_close): """The PyAV input path must tolerate corrupt frames and decode the full stream.""" - video = VideoStreamConcat([corrupt_video_file], backend="pyav") + video = auto_close(VideoStreamConcat([corrupt_video_file], backend="pyav")) num_frames = 0 while video.read(decode=False) is not False: num_frames += 1 assert num_frames >= 590 + + +def test_probed_sources_closed(test_fades_clip, tracked_streams, auto_close): + """Only the first input stays open after probing; the others are closed once probed.""" + auto_close(VideoStreamConcat([test_fades_clip] * 3, backend="tracked")) + assert [stream.is_closed for stream in tracked_streams] == [False, True, True] + + +def test_failed_open_closes_sources(test_fades_clip, test_video_file, tracked_streams): + """Every probed input must be closed if the inputs can't be concatenated.""" + with pytest.raises(VideoOpenFailure): + VideoStreamConcat([test_fades_clip, test_video_file], backend="tracked") + assert len(tracked_streams) == 2 + assert all(stream.is_closed for stream in tracked_streams) + + +def test_switching_sources_closes_outgoing(test_fades_clip, tracked_streams, auto_close): + """Switching sources via `read()`, `seek()`, or `reset()` must close the outgoing source, + so only the source currently being decoded is open.""" + video = auto_close(VideoStreamConcat([test_fades_clip] * 2, backend="tracked")) + + def assert_only_latest_open(num_opened: int): + assert len(tracked_streams) == num_opened + assert all(stream.is_closed for stream in tracked_streams[:-1]) + assert not tracked_streams[-1].is_closed + + # Read across the seam into the second source. + video.seek(FADES_DURATION - 0.5) + while video.position.seconds < FADES_DURATION + 0.5: + assert video.read(decode=False) is not False + assert_only_latest_open(3) + # Seek back into the first source, then forward into the second. + video.seek(5.0) + assert_only_latest_open(4) + video.seek(FADES_DURATION + 5.0) + assert_only_latest_open(5) + video.reset() + assert_only_latest_open(6) + + +def test_close(test_fades_clip, tracked_streams): + """`close()` closes the current source and is idempotent. Afterwards `read()`, `seek()`, + and `reset()` raise `ValueError` without opening any source.""" + video = VideoStreamConcat([test_fades_clip] * 2, backend="tracked") + assert video.read(decode=False) is not False + video.close() + video.close() + assert all(stream.is_closed for stream in tracked_streams) + num_opened = len(tracked_streams) + with pytest.raises(ValueError): + video.read() + with pytest.raises(ValueError): + video.seek(FADES_DURATION + 5.0) + with pytest.raises(ValueError): + video.reset() + assert len(tracked_streams) == num_opened + + +@pytest.mark.parametrize("backend", BACKENDS) +def test_context_manager(test_fades_clip, backend): + """`with open_video([...]) as video:` yields the concatenated stream and closes it.""" + with open_video([test_fades_clip] * 2, backend=backend) as video: + assert isinstance(video, VideoStreamConcat) + assert video.child_backend == backend + assert video.read(decode=False) is not False + with pytest.raises(ValueError): + video.read() + + +@pytest.mark.parametrize("backend", BACKENDS) +def test_properties_after_close(test_fades_clip, backend): + """Properties describing the video must remain available after `close()`.""" + video = VideoStreamConcat([test_fades_clip] * 2, backend=backend) + + def properties(): + return ( + video.path, + video.name, + video.frame_rate, + video.duration, + video.frame_size, + video.aspect_ratio, + video.child_backend, + ) + + expected = properties() + video.close() + assert properties() == expected diff --git a/tests/test_video_stream.py b/tests/test_video_stream.py index 856ad1e7..d1953a25 100644 --- a/tests/test_video_stream.py +++ b/tests/test_video_stream.py @@ -23,6 +23,7 @@ import numpy import pytest +from scenedetect import open_video from scenedetect.backends import VideoStreamAv, VideoStreamMoviePy from scenedetect.backends.opencv import VideoStreamCv2 from scenedetect.video_stream import SeekError, VideoStream @@ -412,3 +413,71 @@ def test_delayed_start_normalized( assert stream.read(decode=False) is not False assert stream.position.seconds < 0.1 assert stream.frame_number == 1 + + +# +# Tests for closing streams. +# + + +def _video_properties(stream: VideoStream) -> tuple: + """Properties describing the video itself, which must remain available after `close()`.""" + return ( + stream.path, + stream.name, + stream.frame_rate, + stream.duration, + stream.frame_size, + stream.aspect_ratio, + ) + + +def test_close_is_idempotent(vs_type: ty.Callable[..., VideoStream]): + """Calling `close()` more than once must have no effect.""" + stream = vs_type(get_absolute_path("resources/testvideo.mp4")) + assert stream.read() is not False + stream.close() + stream.close() + with pytest.raises(ValueError): + stream.read() + + +def test_use_after_close(vs_type: ty.Callable[..., VideoStream]): + """`read()`, `seek()`, and `reset()` must raise `ValueError` once a stream is closed.""" + stream = vs_type(get_absolute_path("resources/testvideo.mp4")) + stream.close() + with pytest.raises(ValueError): + stream.read() + with pytest.raises(ValueError): + stream.read(decode=False) + with pytest.raises(ValueError): + stream.seek(10) + with pytest.raises(ValueError): + stream.reset() + + +def test_properties_after_close(vs_type: ty.Callable[..., VideoStream]): + """Properties describing the video must remain available after `close()`.""" + stream = vs_type(get_absolute_path("resources/testvideo.mp4")) + expected = _video_properties(stream) + stream.close() + assert _video_properties(stream) == expected + + +def test_context_manager(vs_type: type[VideoStream]): + """`with open_video(path) as video:` must yield the stream and close it on exit.""" + path = get_absolute_path("resources/testvideo.mp4") + with open_video(path, backend=vs_type.BACKEND_NAME) as video: + assert isinstance(video, vs_type) + assert video.read() is not False + with pytest.raises(ValueError): + video.read() + + +def test_context_manager_closes_on_error(vs_type: ty.Callable[..., VideoStream]): + """The stream must be closed if the `with` block raises, without suppressing the error.""" + stream = vs_type(get_absolute_path("resources/testvideo.mp4")) + with pytest.raises(RuntimeError, match="raised in with block"), stream: + raise RuntimeError("raised in with block") + with pytest.raises(ValueError): + stream.read() diff --git a/website/pages/changelog.md b/website/pages/changelog.md index 368e2378..d1dddeee 100644 --- a/website/pages/changelog.md +++ b/website/pages/changelog.md @@ -8,6 +8,8 @@ - [bugfix] `$VIDEO_NAME` is now derived from the path component of URL inputs, ignoring the query string and fragment (e.g. `https://host/clip.mp4?token=1.2` -> `clip`). URLs with no path use the host. Previously the query string leaked into output filenames, which fails on Windows. [#197](https://github.com/Breakthrough/PySceneDetect/issues/197) - [bugfix] OTIO exports use the full source duration when provided, including from the CLI, instead of declaring a fixed 1,980-frame duration. The available range remains unspecified when the duration is unknown. Scene source ranges are unchanged. [#580](https://github.com/Breakthrough/PySceneDetect/pull/580) - [bugfix] The OpenCV backend keeps `position` on the last frame after reaching the end of a VFR video, so the last scene no longer ends early (or before it starts) [#561](https://github.com/Breakthrough/PySceneDetect/issues/561) + - [feature] `VideoStream` has a new `close()` method to release the file handles and decoders a video holds, and can be used as a context manager (`with open_video(path) as video:`) to close it automatically. Calling `close()` more than once has no effect. Once closed, `read()`, `seek()` and `reset()` raise `ValueError`, while properties describing the video (`path`, `name`, `frame_rate`, `duration`, `frame_size`, `aspect_ratio`) remain available. The default implementation does nothing, so existing `VideoStream` subclasses are unaffected [#568](https://github.com/Breakthrough/PySceneDetect/issues/568) + - [bugfix] `VideoStreamConcat` now closes each input video as soon as it is no longer needed (after probing it, and when switching to another input during `read()`, `seek()` or `reset()`), instead of leaving them open until garbage collected [#568](https://github.com/Breakthrough/PySceneDetect/issues/568) ### PySceneDetect 0.7.2 (October 11, 2026) From 03a5e0169c21bd801c1ce8dd0e93723336719f77 Mon Sep 17 00:00:00 2001 From: vishal-tambi Date: Wed, 21 Oct 2026 18:15:49 +0530 Subject: [PATCH 2/2] [backends] Address review feedback for VideoStream.close() #568 --- scenedetect/backends/__init__.py | 6 ++-- scenedetect/backends/concat.py | 9 +++--- scenedetect/backends/moviepy.py | 3 +- scenedetect/backends/opencv.py | 20 ++++++++----- scenedetect/backends/pyav.py | 18 ++++++----- scenedetect/video_stream.py | 51 ++++++++++++++++---------------- tests/test_backend_opencv.py | 9 ++++++ tests/test_backend_pyav.py | 10 ++++--- tests/test_concat.py | 26 +++++++++------- tests/test_video_stream.py | 21 +++++++------ website/pages/changelog.md | 4 +-- 11 files changed, 102 insertions(+), 75 deletions(-) diff --git a/scenedetect/backends/__init__.py b/scenedetect/backends/__init__.py index cdcbb734..dace89d0 100644 --- a/scenedetect/backends/__init__.py +++ b/scenedetect/backends/__init__.py @@ -50,9 +50,9 @@ In both examples above, the resulting ``video`` can be used with :meth:`SceneManager.detect_scenes() `. -Every backend can be closed with :meth:`close() ` -to release the file handles and decoders it holds, or used as a context manager to close it -automatically: +Backends which open a video themselves can be closed with +:meth:`close() ` to release the file handles and +decoders they hold, or used as a context manager to close them automatically: .. code:: python diff --git a/scenedetect/backends/concat.py b/scenedetect/backends/concat.py index f6aa73fa..386ed866 100644 --- a/scenedetect/backends/concat.py +++ b/scenedetect/backends/concat.py @@ -215,11 +215,10 @@ def _open_source(self, index: int) -> VideoStream: ) def _switch_source(self, index: int): - """Make the source at `index` the one being decoded, closing the outgoing source. The - new source is opened first, so the outgoing one is kept if that fails.""" + """Make the source at `index` the one being decoded, and close the outgoing source.""" cap = self._open_source(index) - self._cap.close() - self._cap = cap + outgoing, self._cap = self._cap, cap + outgoing.close() def _child_position_seconds(self) -> Fraction: """Position of the current source as exact rational seconds (local timeline).""" @@ -288,7 +287,7 @@ def reset(self): def close(self) -> None: """Close the stream, closing the input video currently open for decoding (only one is kept open at a time). Has no effect if the stream is already closed. See - :meth:`VideoStream.close` for details.""" + :meth:`VideoStream.close() ` for details.""" if self._closed: return self._closed = True diff --git a/scenedetect/backends/moviepy.py b/scenedetect/backends/moviepy.py index 5afa27ad..1718f6c2 100644 --- a/scenedetect/backends/moviepy.py +++ b/scenedetect/backends/moviepy.py @@ -286,7 +286,8 @@ def reset(self, print_infos=False): def close(self) -> None: """Close the video, terminating the ffmpeg process used to decode it. Has no effect if - the stream is already closed. See :meth:`VideoStream.close` for details.""" + the stream is already closed. See + :meth:`VideoStream.close() ` for details.""" if self._closed: return self._closed = True diff --git a/scenedetect/backends/opencv.py b/scenedetect/backends/opencv.py index 3b470c08..25b36044 100644 --- a/scenedetect/backends/opencv.py +++ b/scenedetect/backends/opencv.py @@ -171,7 +171,8 @@ def capture(self) -> cv2.VideoCapture: Prefer to use this property only to take ownership of the underlying cv2.VideoCapture object backing this object. Seeking or using the read/grab methods through this property are - unsupported and will leave this object in an inconsistent state. + unsupported and will leave this object in an inconsistent state. Closing this object (e.g. + by leaving a `with` block) releases the capture. """ return self._cap @@ -228,7 +229,7 @@ def duration(self) -> FrameTimecode | None: if self._is_device: return None if self._closed: - return self.base_timecode + self._closed_frame_count + return self.base_timecode + math.trunc(self._closed_frame_count) return self.base_timecode + math.trunc(self._cap.get(cv2.CAP_PROP_FRAME_COUNT)) @property @@ -320,9 +321,14 @@ def reset(self): def close(self) -> None: """Close the video, releasing the underlying `cv2.VideoCapture`. Has no effect if the - stream is already closed. See :meth:`VideoStream.close` for details.""" + stream is already closed. See + :meth:`VideoStream.close() ` for details.""" if self._closed: return + # A released capture no longer reports the video's properties, so record them first. + self._closed_frame_size = self.frame_size + self._closed_frame_count = self._cap.get(cv2.CAP_PROP_FRAME_COUNT) + self._closed_aspect_ratio = self.aspect_ratio self._closed = True self._cap.release() @@ -418,16 +424,14 @@ def _open_capture(self, frame_rate: FrameRate | None = None): self._has_grabbed = False self._eof_timecode: Timecode | None = None cap.set(cv2.CAP_PROP_ORIENTATION_AUTO, 1.0) # https://github.com/opencv/opencv/issues/26795 - # A released capture reports 0 for every property, so record those describing the video - # for after `close()`. While open, they are still read from the capture. - self._closed_frame_size = self.frame_size - self._closed_frame_count = math.trunc(cap.get(cv2.CAP_PROP_FRAME_COUNT)) - self._closed_aspect_ratio = self.aspect_ratio class VideoCaptureAdapter(VideoStream): """Adapter for existing VideoCapture objects. Unlike VideoStreamCv2, this class supports VideoCaptures which may not support seeking. + + The VideoCapture remains owned by the caller: closing the adapter (e.g. by leaving a `with` + block) does not release it. """ def __init__( diff --git a/scenedetect/backends/pyav.py b/scenedetect/backends/pyav.py index 70caeaaf..2e3b4195 100644 --- a/scenedetect/backends/pyav.py +++ b/scenedetect/backends/pyav.py @@ -173,9 +173,6 @@ def __init__( # Calculate duration after we have set the framerate. self._duration_frames = self._get_duration() - # Properties read from the container are recorded so they remain available after `close()`. - self._closed_frame_size = self.frame_size - self._closed_aspect_ratio = self.aspect_ratio def __del__(self): # Finalizers must never raise - an exception here becomes an unraisable error. During @@ -186,12 +183,15 @@ def __del__(self): def close(self) -> None: """Close the video, releasing the decoder, the container, and the file handle opened if `path_or_io` is a path. File-like objects passed by the caller are left open. Has no - effect if the stream is already closed. See :meth:`VideoStream.close` for details.""" + effect if the stream is already closed. See + :meth:`VideoStream.close() ` for details.""" if self._closed: return - self._closed = True - self._frame = None - # Attributes below may be unset if `__init__` raised, since `__del__` calls this method. + # Record the properties read from the container while it is open. They can't be read if + # `__init__` raised (`__del__` also calls this method) or the video can't be decoded. + with contextlib.suppress(Exception): + self._closed_frame_size = self.frame_size + self._closed_aspect_ratio = self.aspect_ratio # Close the decode generator first to break its cycle with the container. decoder = getattr(self, "_decoder", None) if decoder is not None: @@ -207,6 +207,10 @@ def close(self) -> None: io = getattr(self, "_io", None) if io is not None: io.close() + # Only mark the stream as closed once everything is released, so a failed `close()` can be + # retried. The last decoded frame is dropped, since `position` is undefined once closed. + self._closed = True + self._frame = None # # VideoStream Methods/Properties diff --git a/scenedetect/video_stream.py b/scenedetect/video_stream.py index 8912817b..e87c9857 100644 --- a/scenedetect/video_stream.py +++ b/scenedetect/video_stream.py @@ -17,28 +17,19 @@ .. code:: python from scenedetect import open_video - video = open_video('video.mp4') - while True: - frame = video.read() - if frame is False: - break - print("Read %d frames" % video.frame_number) + with open_video('video.mp4') as video: + while True: + frame = video.read() + if frame is False: + break + print("Read %d frames" % video.frame_number) + +Leaving the ``with`` block closes the video, releasing resources such as file handles and decoders. +A stream can also be closed explicitly by calling :meth:`VideoStream.close`. You can also optionally specify a framerate and a specific backend library to use. Unless specified, OpenCV will be used as the video backend. See :mod:`scenedetect.backends` for a detailed example. -A :class:`VideoStream` holds resources such as file handles and decoders until it is closed. -Call :meth:`VideoStream.close` once done with it, or use it as a context manager to close it -automatically: - -.. code:: python - - from scenedetect import open_video - with open_video('video.mp4') as video: - while video.read() is not False: - pass - print("Read %d frames" % video.frame_number) - New :class:`VideoStream ` implementations can be tested by adding it to the test suite in `tests/test_video_stream.py`. """ @@ -120,10 +111,11 @@ def decode_failures(self) -> int: def close(self) -> None: # noqa: B027 - Not abstract, so existing subclasses remain valid. """Close the stream, releasing any resources it holds (e.g. file handles, decoders). - Calling `close()` more than once has no effect. Once closed, `read()`, `seek()`, and - `reset()` raise `ValueError`; the stream is never re-opened. Properties describing the - video itself (`path`, `name`, `frame_rate`, `duration`, `frame_size`, and `aspect_ratio`) - remain available, but the values of all others (e.g. `position`) are undefined. + Calling `close()` more than once has no effect. Backends which hold resources override + this method: once they are closed, `read()`, `seek()`, and `reset()` raise `ValueError`, + and the stream is never re-opened. Properties describing the video itself (`path`, `name`, + `frame_rate`, `duration`, `frame_size`, and `aspect_ratio`) remain available, but all + others (e.g. `position`) are undefined, and may return arbitrary values or raise. A stream can also be used as a context manager, which closes it on exit: @@ -132,7 +124,9 @@ def close(self) -> None: # noqa: B027 - Not abstract, so existing subclasses re with open_video("video.mp4") as video: scene_manager.detect_scenes(video=video) - The default implementation does nothing. Backends which hold resources override it. + The default implementation does nothing, so streams which hold no resources of their own + (e.g. a :class:`VideoCaptureAdapter `, + whose capture belongs to the caller) remain usable after closing. """ def __enter__(self: _VideoStreamT) -> _VideoStreamT: @@ -244,12 +238,18 @@ def read(self, decode: bool = True) -> np.ndarray | bool: Returns: If decode = True, the decoded frame (np.ndarray), or False (bool) if end of video. If decode = False, a bool indicating if advancing to the the next frame succeeded. + Raises: + ValueError: The stream is closed. """ ... @abstractmethod def reset(self) -> None: - """Close and re-open the VideoStream (equivalent to seeking back to beginning).""" + """Close and re-open the VideoStream (equivalent to seeking back to beginning). + + Raises: + ValueError: The stream is closed. + """ ... @abstractmethod @@ -270,6 +270,7 @@ def seek(self, target: TimecodeLike) -> None: If int, interpreted as frame number. Raises: SeekError: An error occurs while seeking, or seeking is not supported. - ValueError: `target` is not a valid value (i.e. it is negative). + ValueError: `target` is not a valid value (i.e. it is negative), or the stream is + closed. """ ... diff --git a/tests/test_backend_opencv.py b/tests/test_backend_opencv.py index 63cb5c37..d3c6e6e7 100644 --- a/tests/test_backend_opencv.py +++ b/tests/test_backend_opencv.py @@ -77,6 +77,15 @@ def test_capture_adapter(test_movie_clip: str): assert [start.frame_num for (start, _) in scenes] == GROUND_TRUTH_CAPTURE_ADAPTER_TEST +def test_close_releases_capture(test_movie_clip: str): + """`close()` must release the VideoCapture opened by the stream.""" + stream = VideoStreamCv2(test_movie_clip) + capture = stream.capture + assert capture.isOpened() + stream.close() + assert not capture.isOpened() + + def test_capture_adapter_close_leaves_capture_open(test_movie_clip: str): """The VideoCapture wrapped by a VideoCaptureAdapter belongs to the caller, so closing the adapter (here by leaving a `with` block) must not release it.""" diff --git a/tests/test_backend_pyav.py b/tests/test_backend_pyav.py index c8654c2c..b9d910e3 100644 --- a/tests/test_backend_pyav.py +++ b/tests/test_backend_pyav.py @@ -55,16 +55,18 @@ def test_video_stream_pyav_del_closes_owned_file_handle(test_video_file: str): assert handle.closed -def test_close_leaves_caller_file_open(test_video_file: str): +def test_close_leaves_caller_file_open(test_video_file: str, auto_close): """`close()` must not close a file-like object supplied by the caller (#568).""" with open(test_video_file, mode="rb") as video_file: - stream = VideoStreamAv(path_or_io=video_file, threading_mode=None) + stream = auto_close(VideoStreamAv(path_or_io=video_file, threading_mode=None)) assert stream.read() is not False stream.close() assert not video_file.closed -def test_close_releases_owned_file_handle(test_video_file: str, monkeypatch: pytest.MonkeyPatch): +def test_close_releases_owned_file_handle( + test_video_file: str, monkeypatch: pytest.MonkeyPatch, auto_close +): """`close()` must release the file handle the stream opened from a path (#568), without waiting for the stream to be finalized.""" opened = [] @@ -76,7 +78,7 @@ def recording_open(*args, **kwargs): return handle monkeypatch.setattr("scenedetect.backends.pyav.open", recording_open, raising=False) - stream = VideoStreamAv(test_video_file, threading_mode=None) + stream = auto_close(VideoStreamAv(test_video_file, threading_mode=None)) assert len(opened) == 1 assert not opened[0].closed stream.close() diff --git a/tests/test_concat.py b/tests/test_concat.py index b247d255..d2ef8424 100644 --- a/tests/test_concat.py +++ b/tests/test_concat.py @@ -30,18 +30,18 @@ class _TrackedStream(VideoStreamCv2): - """OpenCV backend which records every stream it opens, and whether it has been closed.""" + """OpenCV backend which records every stream it opens.""" instances: ty.ClassVar[list["_TrackedStream"]] = [] def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.is_closed = False _TrackedStream.instances.append(self) - def close(self) -> None: - super().close() - self.is_closed = True + @property + def is_closed(self) -> bool: + """True once the underlying capture has been released.""" + return not self.capture.isOpened() @pytest.fixture @@ -210,10 +210,14 @@ def test_probed_sources_closed(test_fades_clip, tracked_streams, auto_close): def test_failed_open_closes_sources(test_fades_clip, test_video_file, tracked_streams): - """Every probed input must be closed if the inputs can't be concatenated.""" + """Every input opened so far must be closed if the inputs can't be opened or concatenated.""" + # The second input has a different resolution. with pytest.raises(VideoOpenFailure): VideoStreamConcat([test_fades_clip, test_video_file], backend="tracked") - assert len(tracked_streams) == 2 + # The third input doesn't exist. + with pytest.raises(OSError): + VideoStreamConcat([test_fades_clip, test_fades_clip, "missing.mp4"], backend="tracked") + assert len(tracked_streams) == 4 assert all(stream.is_closed for stream in tracked_streams) @@ -241,10 +245,10 @@ def assert_only_latest_open(num_opened: int): assert_only_latest_open(6) -def test_close(test_fades_clip, tracked_streams): +def test_close(test_fades_clip, tracked_streams, auto_close): """`close()` closes the current source and is idempotent. Afterwards `read()`, `seek()`, and `reset()` raise `ValueError` without opening any source.""" - video = VideoStreamConcat([test_fades_clip] * 2, backend="tracked") + video = auto_close(VideoStreamConcat([test_fades_clip] * 2, backend="tracked")) assert video.read(decode=False) is not False video.close() video.close() @@ -271,9 +275,9 @@ def test_context_manager(test_fades_clip, backend): @pytest.mark.parametrize("backend", BACKENDS) -def test_properties_after_close(test_fades_clip, backend): +def test_properties_after_close(test_fades_clip, backend, auto_close): """Properties describing the video must remain available after `close()`.""" - video = VideoStreamConcat([test_fades_clip] * 2, backend=backend) + video = auto_close(VideoStreamConcat([test_fades_clip] * 2, backend=backend)) def properties(): return ( diff --git a/tests/test_video_stream.py b/tests/test_video_stream.py index d1953a25..fba7e966 100644 --- a/tests/test_video_stream.py +++ b/tests/test_video_stream.py @@ -432,9 +432,9 @@ def _video_properties(stream: VideoStream) -> tuple: ) -def test_close_is_idempotent(vs_type: ty.Callable[..., VideoStream]): +def test_close_is_idempotent(vs_type: ty.Callable[..., VideoStream], auto_close): """Calling `close()` more than once must have no effect.""" - stream = vs_type(get_absolute_path("resources/testvideo.mp4")) + stream = auto_close(vs_type(get_absolute_path("resources/testvideo.mp4"))) assert stream.read() is not False stream.close() stream.close() @@ -442,9 +442,9 @@ def test_close_is_idempotent(vs_type: ty.Callable[..., VideoStream]): stream.read() -def test_use_after_close(vs_type: ty.Callable[..., VideoStream]): +def test_use_after_close(vs_type: ty.Callable[..., VideoStream], auto_close): """`read()`, `seek()`, and `reset()` must raise `ValueError` once a stream is closed.""" - stream = vs_type(get_absolute_path("resources/testvideo.mp4")) + stream = auto_close(vs_type(get_absolute_path("resources/testvideo.mp4"))) stream.close() with pytest.raises(ValueError): stream.read() @@ -456,9 +456,12 @@ def test_use_after_close(vs_type: ty.Callable[..., VideoStream]): stream.reset() -def test_properties_after_close(vs_type: ty.Callable[..., VideoStream]): - """Properties describing the video must remain available after `close()`.""" - stream = vs_type(get_absolute_path("resources/testvideo.mp4")) +@pytest.mark.parametrize("test_video", get_test_video_params()) +def test_properties_after_close( + vs_type: ty.Callable[..., VideoStream], test_video: VideoParameters, auto_close +): + """Properties describing the video must remain available, and unchanged, after `close()`.""" + stream = auto_close(vs_type(test_video.path)) expected = _video_properties(stream) stream.close() assert _video_properties(stream) == expected @@ -474,9 +477,9 @@ def test_context_manager(vs_type: type[VideoStream]): video.read() -def test_context_manager_closes_on_error(vs_type: ty.Callable[..., VideoStream]): +def test_context_manager_closes_on_error(vs_type: ty.Callable[..., VideoStream], auto_close): """The stream must be closed if the `with` block raises, without suppressing the error.""" - stream = vs_type(get_absolute_path("resources/testvideo.mp4")) + stream = auto_close(vs_type(get_absolute_path("resources/testvideo.mp4"))) with pytest.raises(RuntimeError, match="raised in with block"), stream: raise RuntimeError("raised in with block") with pytest.raises(ValueError): diff --git a/website/pages/changelog.md b/website/pages/changelog.md index d1dddeee..cb929239 100644 --- a/website/pages/changelog.md +++ b/website/pages/changelog.md @@ -8,8 +8,8 @@ - [bugfix] `$VIDEO_NAME` is now derived from the path component of URL inputs, ignoring the query string and fragment (e.g. `https://host/clip.mp4?token=1.2` -> `clip`). URLs with no path use the host. Previously the query string leaked into output filenames, which fails on Windows. [#197](https://github.com/Breakthrough/PySceneDetect/issues/197) - [bugfix] OTIO exports use the full source duration when provided, including from the CLI, instead of declaring a fixed 1,980-frame duration. The available range remains unspecified when the duration is unknown. Scene source ranges are unchanged. [#580](https://github.com/Breakthrough/PySceneDetect/pull/580) - [bugfix] The OpenCV backend keeps `position` on the last frame after reaching the end of a VFR video, so the last scene no longer ends early (or before it starts) [#561](https://github.com/Breakthrough/PySceneDetect/issues/561) - - [feature] `VideoStream` has a new `close()` method to release the file handles and decoders a video holds, and can be used as a context manager (`with open_video(path) as video:`) to close it automatically. Calling `close()` more than once has no effect. Once closed, `read()`, `seek()` and `reset()` raise `ValueError`, while properties describing the video (`path`, `name`, `frame_rate`, `duration`, `frame_size`, `aspect_ratio`) remain available. The default implementation does nothing, so existing `VideoStream` subclasses are unaffected [#568](https://github.com/Breakthrough/PySceneDetect/issues/568) - - [bugfix] `VideoStreamConcat` now closes each input video as soon as it is no longer needed (after probing it, and when switching to another input during `read()`, `seek()` or `reset()`), instead of leaving them open until garbage collected [#568](https://github.com/Breakthrough/PySceneDetect/issues/568) + - [feature] `VideoStream` has a new `close()` method to release the file handles and decoders a video holds, and can be used as a context manager (`with open_video(path) as video:`) to close it automatically. Calling `close()` more than once has no effect. Once an OpenCV, PyAV, MoviePy or concatenated stream is closed, `read()`, `seek()` and `reset()` raise `ValueError`, while properties describing the video (`path`, `name`, `frame_rate`, `duration`, `frame_size`, `aspect_ratio`) remain available. The default implementation does nothing, so existing `VideoStream` subclasses are unaffected, and `VideoCaptureAdapter` leaves the caller's `VideoCapture` open [#568](https://github.com/Breakthrough/PySceneDetect/issues/568) + - [bugfix] `VideoStreamConcat` no longer keeps its input videos open when they can't be concatenated (e.g. mismatched resolutions), and now closes each input explicitly once it is no longer needed instead of relying on garbage collection [#568](https://github.com/Breakthrough/PySceneDetect/issues/568) ### PySceneDetect 0.7.2 (October 11, 2026)