From 92a0638e714e00de5563a6cf21d3b1fe143cefe0 Mon Sep 17 00:00:00 2001 From: Breakthrough Date: Sun, 25 Oct 2026 18:57:34 -0400 Subject: [PATCH] [backends] Remove genpts option from fflags in PyAV backend #590 PyAV sets `AVFMT_FLAG_GENPTS` on every input (see PyAV-Org/PyAV#2477 / PyAV-Org/PyAV#2478). On containers that store DTS only (AVI, MPEG-TS) and contain B-frames, or streams that have no timestamps at all, the generated PTS can come out in the incorrect decode order. This was causing the PyAV backend to report non-monotonic timestamps in these cases, resulting in incorrect splits (#590). This PR now ensures we set `fflags=-genpts` to override the `+genpts` flag from PyAV. We also fallback from PTS to DTS, and count frames when there are no timestamps entirely. We also add support for when PyAV exposes `Frame.best_effort_timestamp` in a future release (tested locally with the development changes linked above). Lastly, a one-time warning is logged if timestamps are detected going backwards. Users hitting this issue should update PyAV to 19.1+ to resolve it. We cannot pin to PyAV 19.1+ because it requires Python 3.12, but `scenedetect` supports 3.10+ and 3.11 is supported until October 2027. Follow-ups kept as TODOs: - AVI tick rate and frame counts in `_get_duration` - MPEG-TS seeks that land after the target position --- scenedetect/backends/pyav.py | 320 +++++++++++++++++++++---- scenedetect/video_stream.py | 3 +- tests/conftest.py | 25 ++ tests/test_backend_pyav.py | 448 ++++++++++++++++++++++++++++++++++- website/pages/changelog.md | 10 +- 5 files changed, 755 insertions(+), 51 deletions(-) diff --git a/scenedetect/backends/pyav.py b/scenedetect/backends/pyav.py index 2e3b4195..449ed502 100644 --- a/scenedetect/backends/pyav.py +++ b/scenedetect/backends/pyav.py @@ -30,11 +30,34 @@ framerate_to_fraction, ) from scenedetect.platform import StrPath, get_file_name -from scenedetect.video_stream import FrameRateUnavailable, VideoOpenFailure, VideoStream +from scenedetect.video_stream import ( + FrameRateUnavailable, + SeekError, + VideoOpenFailure, + VideoStream, +) logger = getLogger("pyscenedetect") VALID_THREAD_MODES = ["NONE", "SLICE", "FRAME", "AUTO"] +# FFmpeg options passed to `av.open()`. By default, PyAV enables `AVFMT_FLAG_GENPTS` (`+genpts`) on +# every input container. For containers that only store decode timestamps (e.g. AVI), `+genpts` +# synthesizes missing presentation timestamps (PTS) from packet decode timestamps (DTS), which are +# in decode order rather than presentation order. When the video contains B-frames, the decoder +# reorders frames into presentation order while keeping those decode-order PTS values attached, +# causing `frame.pts` (and `best_effort_timestamp` on the initial frames) to jump backwards and +# forwards. Disabling `genpts` leaves `frame.pts` as `None` on DTS-only streams so we can fall back +# to `frame.dts` cleanly. See https://scenedetect.com/issues/590 and +# https://github.com/PyAV-Org/PyAV/pull/2478. +_CONTAINER_OPTIONS = {"fflags": "-genpts"} + + +MAX_SEEK_PROBE_FRAMES = 32 +"""Number of frames `VideoStreamAv.seek()` decodes looking for a timestamp before concluding the +stream has none and cannot be seeked (e.g. raw Annex-B H.264). This also caps how many leading +frames without timestamps a file may have when `seek()` is the first call on the stream: a file +whose first timestamp is on frame 40 can be read sequentially, but not seeked before reading.""" + MAX_CONSECUTIVE_DECODE_FAILURES = 8 """Number of consecutive frame decode failures after which `VideoStreamAv.read()` gives up. Isolated corrupt frames are skipped; this bound ensures a truncated file still terminates.""" @@ -117,6 +140,19 @@ def __init__( self._reopened = True self._decode_failures = 0 self._warning_displayed = False + # Presentation time of `_frame` relative to the start of the stream. None until the first + # `read()` (and after `close()`/`reset()`). + self._timecode: Timecode | None = None + # Timestamp (and timebase) subtracted from every frame timestamp such that that frame 1 is + # at time 0. Decided based on the the first decoded frame with a valid timestamp. Only set + # once, even after `reset()` or `seek()`. + self._offset: int | None = None + self._offset_time_base: Fraction | None = None + # Ensures we only log a warning once per pass (until `reset()`) if positions go backwards. + self._warned_non_monotonic = False + # 1-based count of frames decoded in the current stream pass, used to estimate the position + # of frames without timestamps and for diagnostic logging. + self._frames_decoded = 0 # 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 @@ -128,6 +164,7 @@ def __init__( raise ValueError( f"Invalid threading mode! Must be one of: {VALID_THREAD_MODES}" ) from None + self._threading_mode = threading_mode if not suppress_output: logger.debug("Restoring default ffmpeg log callbacks.") @@ -144,11 +181,11 @@ def __init__( else: self._io = path_or_io - self._container: av.container.InputContainer = av.open(self._io) # type: ignore[attr-defined] - if threading_mode is not None: - self._video_stream.thread_type = threading_mode + self._container: av.container.InputContainer = self._open_container() # type: ignore[attr-defined] + if self._threading_mode is not None: + self._video_stream.thread_type = self._threading_mode self._reopened = False - logger.debug("Threading mode set: %s", threading_mode) + logger.debug("Threading mode set: %s", self._threading_mode) except OSError: raise except Exception as ex: @@ -211,6 +248,7 @@ def close(self) -> None: # retried. The last decoded frame is dropped, since `position` is undefined once closed. self._closed = True self._frame = None + self._timecode = None # # VideoStream Methods/Properties @@ -257,18 +295,17 @@ def position(self) -> FrameTimecode: This can be interpreted as presentation time stamp, thus frame 1 corresponds to the presentation time 0. Returns 0 even if `frame_number` is 1.""" - if self._frame is None or self._frame.pts is None or not self._frame.time_base: + if self._timecode is None: return self.base_timecode - timecode = Timecode(pts=self._normalized_pts(), time_base=self._frame_time_base) - return FrameTimecode(timecode=timecode, fps=self.frame_rate) + return FrameTimecode(timecode=self._timecode, fps=self.frame_rate) @property def position_ms(self) -> float: """Current position within stream as a float of the presentation time in milliseconds. The first frame has a PTS of 0.""" - if self._frame is None or self._frame.pts is None or not self._frame.time_base: + if self._timecode is None: return 0.0 - return float(self._normalized_pts() * self._frame_time_base) * 1000.0 + return float(self._timecode.seconds) * 1000.0 @property def frame_number(self) -> int: @@ -276,10 +313,9 @@ def frame_number(self) -> int: Will return 0 until the first frame is `read`. For VFR video this is an approximation derived from PTS * framerate; use `position` for accurate PTS-based timing.""" - if self._frame is None or self._frame.pts is None or not self._frame.time_base: + if self._timecode is None: return 0 - seconds = float(self._normalized_pts() * self._frame_time_base) - return round(seconds * float(self.frame_rate)) + 1 + return round(float(self._timecode.seconds) * float(self.frame_rate)) + 1 @property def rate(self) -> Fraction | None: @@ -316,13 +352,17 @@ def seek(self, target: TimecodeLike) -> None: seek(0) followed by read(). If we want to seek to the 5th frame, we call seek(4) followed by read(), at which point frame_number will be 5. - May not be supported on all input codecs (see `is_seekable`). + May not be supported on all input codecs (see `is_seekable`). Elementary streams usually + cannot be seeked, and may raise `SeekError`, after which the stream position is undefined + until `reset()` is called. Arguments: target: Target position in video stream to seek to. If float, interpreted as time in seconds. If int, interpreted as frame number. Raises: + SeekError: Stream does not support seeking, target cannot be seeked to, or the video + could not be re-opened to read forward from the start. ValueError: `target` is not a valid value (i.e. it is negative), or the stream is closed. """ @@ -331,22 +371,86 @@ def seek(self, target: TimecodeLike) -> None: target = FrameTimecode(target, self.frame_rate) if target < 0: raise ValueError("Target cannot be negative!") + if not self.is_seekable: + raise SeekError("Stream is not seekable.") beginning = target == 0 target = self.base_timecode + target if target >= 1: target = target - 1 - target_pts = (self._video_stream.start_time or 0) + int( - (self.base_timecode + target).seconds / _to_fraction(self._video_stream.time_base) - ) + # The timestamp offset is anchored at the start of the stream (see `_update_timecode()`), + # so it must only ever be decided from frames decoded in order from the start: a seek can + # land on a later keyframe. While it is unknown, seeking to the start re-opens the stream, + # and seeking elsewhere first decodes frames until one carries a timestamp. Streams without + # any timestamps (e.g. raw H.264) cannot be seeked, so give up after + # `MAX_SEEK_PROBE_FRAMES` frames. As with any failed seek, the stream position is then + # undefined until `reset()` is called. + if beginning and self._offset is None: + self._reset_for_seek() + return + stream_time_base = self._video_stream.time_base + if not stream_time_base: + raise SeekError("Unable to seek in stream: stream time_base is unset.") + if self._offset is None: + while self._offset is None and self._frames_decoded < MAX_SEEK_PROBE_FRAMES: + if self.read(decode=False) is False: + break + if self._offset is None: + raise SeekError("Unable to seek in stream without timestamps.") + # Positions are relative to `_offset`, so the seek target must be too. For streams whose + # first frame has a PTS, this is the stream's `start_time`. + stream_time_base = _to_fraction(stream_time_base) + assert self._offset is not None and self._offset_time_base is not None + offset = self._offset + if offset and self._offset_time_base != stream_time_base: + offset = int(offset * self._offset_time_base / stream_time_base) + target_pts = offset + int((self.base_timecode + target).seconds / stream_time_base) + # TODO(https://scenedetect.com/issues/590): For some containers (e.g. MPEG-TS) whose + # keyframes lack timestamps, the demuxer can land past the target, or somewhere the + # position cannot be established. Both fall back to reading from the start of the stream + # (see below), which is correct but slow for distant targets; we could seek to an earlier + # keyframe instead. + try: + self._container.seek(target_pts, stream=self._video_stream) + except av.error.FFmpegError as ex: # type: ignore[attr-defined] + raise SeekError(f"Unable to seek in stream: {ex}") from ex self._frame = None self._decoder = None - self._container.seek(target_pts, stream=self._video_stream) + self._timecode = None + # Seed `_frames_decoded` with the target frame before walking forward from the keyframe, + # then sync it to `self.frame_number` once the walk completes. + self._frames_decoded = target.frame_num if not beginning: - self.read(decode=False) + # The demuxer normally lands on a keyframe at or before `target_pts`, but the frames + # decoded from there may lack timestamps, in which case their position is unknown. + # Read until a frame carries one to establish where we landed, leaving the position + # unset (rather than estimated) until then so the walk below cannot mistake an + # estimate for having reached the target. Frames consumed while probing cannot be + # given back, so if no timestamp is found within `MAX_SEEK_PROBE_FRAMES` frames, or the + # first one found is already past the target, read forward from the start instead. + # That gives the same result as a sequential read, and only happens on malformed + # inputs. If the end of the stream is reached before any frame is decoded, there is + # nothing to establish. + probe_limit = self._frames_decoded + MAX_SEEK_PROBE_FRAMES + landed = None + while self.read(decode=False) is not False: + if self._frame_has_timestamp(): + landed = self.position + break + if self._frames_decoded >= probe_limit: + break + self._timecode = None + if self._frame is not None and (landed is None or landed > target): + logger.debug( + "Seek landed at %s for target %s, reading from the start instead.", + "unknown position" if landed is None else landed, + target, + ) + self._reset_for_seek() while self.position < target: if self.read(decode=False) is False: break + self._frames_decoded = self.frame_number def reset(self): """Close and re-open the VideoStream (should be equivalent to calling `seek(0)`).""" @@ -354,8 +458,14 @@ def reset(self): self._container.close() self._frame = None self._decoder = None + self._timecode = None + self._frames_decoded = 0 + self._warned_non_monotonic = False try: - self._container = av.open(self._path if self._path else self._io) + self._container = self._open_container(rewind=True) + if self._threading_mode is not None: + self._video_stream.thread_type = self._threading_mode + self._reopened = False except Exception as ex: raise VideoOpenFailure() from ex @@ -363,16 +473,16 @@ 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 - # B-frame reordering) is never flushed prematurely. Creating a new generator each call - # caused the last buffered frame to be lost at EOF. + # Reuse the same decode generator for the lifetime of the container. Creating a new + # generator on each call would flush the codec's internal buffer at EOF and drop the + # last buffered frame. if self._decoder is None: self._decoder = self._container.decode(video=0) try: last_frame = self._frame assert self._decoder is not None self._frame = next(self._decoder) - # NOTE: EOFError subclasses FFmpegError, so this clause must come first. + # EOFError subclasses FFmpegError, so this clause must come first. except av.error.EOFError: # type: ignore[attr-defined] self._frame = last_frame if self._handle_eof(): @@ -381,12 +491,12 @@ def read(self, decode: bool = True) -> np.ndarray | bool: except StopIteration: return False except av.error.FFmpegError as ex: # type: ignore[attr-defined] - # `next()` raised before assignment, so `self._frame` is still the last good - # frame and position/frame_number are unaffected by the skipped packet. + # `next()` raised before assignment, so `self._frame` and `self._timecode` still + # reflect the last valid frame. self._decode_failures += 1 consecutive_failures += 1 - # The decoder generator is closed once an exception propagates through it; - # recreating it (next loop iteration) resumes demuxing after the bad packet. + # A generator cannot be resumed once an exception propagates out of it; recreating + # it on the next iteration continues demuxing after the corrupt packet. self._decoder = None if consecutive_failures >= MAX_CONSECUTIVE_DECODE_FAILURES: logger.error( @@ -401,6 +511,8 @@ def read(self, decode: bool = True) -> np.ndarray | bool: logger.warning("Failed to decode some frames, results may be inaccurate.") continue assert self._frame is not None + self._frames_decoded += 1 + self._update_timecode() return self._frame.to_ndarray(format="bgr24") if decode else True # @@ -423,24 +535,131 @@ def _frame_time_base(self) -> Fraction: assert self._frame is not None and self._frame.time_base return _to_fraction(self._frame.time_base) - def _normalized_pts(self) -> int: - """PTS of the current frame relative to the start of the stream. Some files have a - nonzero stream start_time (e.g. from edit lists); other backends report the first - frame's presentation time as 0, so we must do the same.""" - assert self._frame is not None and self._frame.pts is not None - start_time = self._video_stream.start_time or 0 - if start_time and self._video_stream.time_base != self._frame.time_base: - start_time = int( - start_time * _to_fraction(self._video_stream.time_base) / self._frame_time_base + def _open_container(self, rewind: bool = False) -> av.container.InputContainer: # type: ignore[attr-defined] + """Open `_io` as a PyAV container with `_CONTAINER_OPTIONS`. + + All container opens (initial open, `reset()`, and `_handle_eof()`) must go through this + helper so container flags like `-genpts` stay applied after re-opening. + + Arguments: + rewind: Seek `_io` back to byte 0 before opening (used when re-opening a stream). + """ + if rewind and self._io.seekable(): + self._io.seek(0) + return av.open(self._io, container_options=_CONTAINER_OPTIONS) # type: ignore[attr-defined] + + def _reset_for_seek(self): + """Call `reset()` on behalf of `seek()`, raising `SeekError` instead of `VideoOpenFailure` + if the video cannot be re-opened, so `seek()` only raises the exceptions it documents.""" + try: + self.reset() + except VideoOpenFailure as ex: + raise SeekError("Unable to seek in stream: failed to re-open video.") from ex + + def _frame_has_timestamp(self) -> bool: + """True if `_frame` carries a usable timestamp (a PTS or DTS, and a time base).""" + frame = self._frame + return ( + frame is not None + and bool(frame.time_base) + and (frame.pts is not None or frame.dts is not None) + ) + + def _update_timecode(self): + """Update `_timecode` with the presentation time of `_frame` relative to the first frame. + + Each frame is placed at its PTS, falling back to its DTS when the PTS is missing. This is + the same rule as OpenCV's FFmpeg backend. With `genpts` off, FFmpeg's + `best_effort_timestamp` gives the same result on streams whose PTS are monotonic; it + additionally switches to DTS once PTS has gone backwards more often than DTS. A frame with + neither is placed one nominal frame period after the previous frame (see + `_estimated_timecode()`). A warning is logged (once per pass) if a frame is placed before + the previous frame's position, estimated or not. + + `_offset` is decided once, from the first frame that carries a timestamp. When that is + frame 1 and it has a PTS, the stream `start_time` is used so files with edit lists start + at time 0 like the other backends. Otherwise the offset anchors frame 1 at time 0 assuming + the frames before it were spaced at the nominal frame rate. This covers DTS-only streams + (e.g. AVI), whose first DTS is already past the stream start by the decoder's B-frame + reordering delay, and streams whose first frames carry no timestamps at all. + """ + frame = self._frame + assert frame is not None + timestamp = frame.pts if frame.pts is not None else frame.dts + if timestamp is None or not frame.time_base: + self._timecode = self._estimated_timecode() + return + time_base = self._frame_time_base + if self._offset is None: + stream_time_base = self._video_stream.time_base + if self._frames_decoded == 1 and frame.pts is not None and stream_time_base: + self._offset = self._video_stream.start_time or 0 + self._offset_time_base = _to_fraction(stream_time_base) + else: + self._offset = timestamp - round( + (self._frames_decoded - 1) / (self.frame_rate * time_base) + ) + self._offset_time_base = time_base + logger.debug( + "First timestamp found on frame %d, using offset %d (time base %s).", + self._frames_decoded, + self._offset, + time_base, + ) + offset = self._offset + if offset and self._offset_time_base != time_base: + assert self._offset_time_base is not None + offset = int(offset * self._offset_time_base / time_base) + timecode = Timecode(pts=timestamp - offset, time_base=time_base) + # Compare against the last reported position, including estimated ones, since that is + # what callers see go backwards. + previous = self._timecode + if ( + previous is not None + and timecode.seconds < previous.seconds + and not self._warned_non_monotonic + ): + # With `-genpts` set on the container, non-monotonic timestamps indicate either a + # genuinely malformed input or unexpected demuxer/decoder behavior. + self._warned_non_monotonic = True + logger.warning( + "Timestamps went backwards at frame %d (%.3fs -> %.3fs, PyAV %s); results " + "may be inaccurate. See https://scenedetect.com/issues/590", + self._frames_decoded, + float(previous.seconds), + float(timecode.seconds), + av.__version__, ) - return self._frame.pts - start_time + self._timecode = timecode + + def _estimated_timecode(self) -> Timecode: + """The position we think the next frame should be at. Use only if we get a frame without a + timestamp. Calculated as one (nominal) frame period from the previous frame's position, or + as the frame count in nominal frame periods if there is no previous position.""" + previous = self._timecode + if previous is None: + return Timecode(pts=max(self._frames_decoded - 1, 0), time_base=1 / self.frame_rate) + return Timecode( + pts=previous.pts + self._frame_period(previous.time_base), time_base=previous.time_base + ) + + def _frame_period(self, time_base: Fraction) -> int: + """Duration of one frame at the nominal frame rate in `time_base` ticks (at least 1).""" + return max(round(1 / (self.frame_rate * time_base)), 1) def _get_duration(self) -> int: - """Get video duration as number of frames based on the video and set framerate.""" - # See https://pyav.org/docs/develop/api/time.html for details on how ffmpeg/PyAV - # handle time calculations internally and which time base to use. + """Get video duration as number of frames based on the video and set framerate. + + See https://pyav.org/docs/develop/api/time.html for details on how ffmpeg/PyAV + handle time calculations internally and which time base to use. + """ assert self.frame_rate is not None, "Frame rate must be set before calling _get_duration!" # See if we can obtain the number of frames directly from the stream itself. + # TODO(https://scenedetect.com/issues/590): Some AVI files remuxed by ffmpeg set the stream + # rate to a high tick rate (e.g. 600/1), causing `stream.frames` to report tick count + # rather than actual frame count (e.g. 12012 instead of 480). OpenCV reports the same value + # via CAP_PROP_FRAME_COUNT, but `duration_sec * frame_rate` would be more accurate when + # `stream.frames` disagrees with the stream duration and codec frame rate. if self._video_stream.frames > 0: return self._video_stream.frames # Calculate based on the reported container duration. @@ -473,18 +692,27 @@ def _handle_eof(self): return False self._reopened = True # Don't re-open the video if we can't seek or aren't in AUTO/FRAME thread_type mode. - if not self.is_seekable or self._video_stream.thread_type not in ("AUTO", "FRAME"): + thread_type = getattr( + self._video_stream.thread_type, "name", self._video_stream.thread_type + ) + if not self.is_seekable or self._offset is None or thread_type not in ("AUTO", "FRAME"): return False - last_pos_secs = self.position.seconds + last_pos = self.position if self._timecode is not None else None orig_pos = self._io.tell() try: - self._io.seek(0) - container = av.open(self._io) + container = self._open_container(rewind=True) except: self._io.seek(orig_pos) raise self._container.close() self._container = container self._decoder = None - self.seek(last_pos_secs) + if last_pos is not None: + try: + # `seek(target)` positions the stream one frame before `target` so the next `read()` + # returns `target`. Pass `last_pos + 1` so the reopened stream lands on `last_pos` + # and the caller's subsequent `read()` advances to the next frame. + self.seek(last_pos + 1) + except SeekError: + return False return True diff --git a/scenedetect/video_stream.py b/scenedetect/video_stream.py index e87c9857..1d55cd4b 100644 --- a/scenedetect/video_stream.py +++ b/scenedetect/video_stream.py @@ -262,7 +262,8 @@ def seek(self, target: TimecodeLike) -> None: seek(0) followed by read(). If we want to seek to the 5th frame, we call seek(4) followed by read(), at which point frame_number will be 5. - May not be supported on all backend types or inputs (e.g. cameras). + May not be supported on all backend types or inputs (e.g. cameras). If `SeekError` is + raised, the stream position is undefined. `reset()` can be called to restore the stream. Arguments: target: Target position in video stream to seek to. diff --git a/tests/conftest.py b/tests/conftest.py index dee805a4..8b6c9da2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -130,6 +130,31 @@ def test_vfr_drop3_video() -> str: return check_exists("tests/resources/goldeneye-vfr-drop3.mp4") +@pytest.fixture +def test_no_pts_avi() -> str: + """First 480 frames of goldeneye.mp4 re-encoded with B-frames (libx264, -bf 3 -g 48) and + remuxed into an AVI, so its packets only carry decode timestamps (no PTS). See issue #590.""" + return check_exists("tests/resources/issue-590-bframes.avi") + + +@pytest.fixture +def test_raw_h264() -> str: + """Same bitstream as `test_no_pts_avi` as a raw Annex-B H.264 elementary stream, which carries + no timestamps at all. See issue #590.""" + return check_exists("tests/resources/issue-590-bframes.h264") + + +@pytest.fixture +def test_missing_pts_ts() -> str: + """`goldeneye-vfr-drop3.mp4` remuxed into MPEG-TS (`ffmpeg -c:v copy -f mpegts`), then the + PTS/DTS fields of the first six video PES packets, and of two in every seven packets after the + tenth, overwritten with PES header stuffing bytes (PTS_DTS_flags cleared, packet sizes + unchanged). After decoding, frames 1-4 carry no timestamps, frame 5 only a DTS, and later + frames sporadically lack their PTS, DTS or both. Frame times otherwise match the mp4 it was + remuxed from. See PR #592.""" + return check_exists("tests/resources/issue-590-missing-pts.ts") + + @pytest.fixture def corrupt_video_file() -> str: """Video containing a corrupted frame causing a decode failure.""" diff --git a/tests/test_backend_pyav.py b/tests/test_backend_pyav.py index b9d910e3..44bd6ff7 100644 --- a/tests/test_backend_pyav.py +++ b/tests/test_backend_pyav.py @@ -18,13 +18,22 @@ """ import gc +import itertools from fractions import Fraction from types import SimpleNamespace import av import pytest -from scenedetect.backends.pyav import MAX_CONSECUTIVE_DECODE_FAILURES, VideoStreamAv, _to_fraction +from scenedetect import SceneManager +from scenedetect.backends.pyav import ( + MAX_CONSECUTIVE_DECODE_FAILURES, + MAX_SEEK_PROBE_FRAMES, + VideoStreamAv, + _to_fraction, +) +from scenedetect.detectors import ContentDetector +from scenedetect.video_stream import SeekError def test_video_stream_pyav_bytesio(test_video_file: str): @@ -56,12 +65,20 @@ def test_video_stream_pyav_del_closes_owned_file_handle(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).""" + """`close()` must not close a file-like object supplied by the caller (#568), and must drop + the last decoded frame and position.""" with open(test_video_file, mode="rb") as video_file: stream = auto_close(VideoStreamAv(path_or_io=video_file, threading_mode=None)) - assert stream.read() is not False + assert stream.read(decode=False) is not False + assert stream.read(decode=False) is not False + assert stream.frame_number == 2 + assert stream.position > stream.base_timecode + assert stream.position_ms > 0.0 stream.close() assert not video_file.closed + assert stream.frame_number == 0 + assert stream.position == stream.base_timecode + assert stream.position_ms == 0.0 def test_close_releases_owned_file_handle( @@ -250,3 +267,428 @@ def test_duration_fallback_unset_time_base( monkeypatch.setattr(VideoStreamAv, "_video_stream", property(lambda self: fake_stream)) assert stream._get_duration() == 0 assert any("time_base" in record.message for record in caplog.records) + with pytest.raises(SeekError): + stream.seek(10) + assert stream.frame_number == 0 + + +# +# Streams without presentation timestamps +# +# By default, PyAV enables AVFMT_FLAG_GENPTS on every input container. For containers that only +# store decode timestamps (e.g. AVI), GENPTS synthesizes PTS from decode-order packet DTS, which +# produces out-of-order frame PTS once B-frames are reordered by the decoder. Raw elementary +# streams have no timestamps at all. Both fixtures below contain the first 480 frames of +# `goldeneye.mp4` re-encoded with B-frames, so the cuts detected on `goldeneye.mp4` over that +# range serve as the reference. +# + +_BFRAMES_NUM_FRAMES = 480 + + +def _detect_cuts(path: str, num_frames: int | None = None) -> list: + scene_manager = SceneManager() + scene_manager.add_detector(ContentDetector()) + with VideoStreamAv(path) as video: + end_time = None if num_frames is None else video.base_timecode + num_frames + scene_manager.detect_scenes(video, end_time=end_time) + return [scene[0] for scene in scene_manager.get_scene_list()[1:]] + + +def test_no_pts_avi_positions_monotonic(test_no_pts_avi: str, auto_close): + """With GENPTS disabled the AVI falls back to DTS; positions must start at 0 and increase by + one frame per read instead of following the decoder's reordering.""" + stream = auto_close(VideoStreamAv(test_no_pts_avi)) + assert stream.read() is not False + assert stream.position == stream.base_timecode + assert stream.position_ms == pytest.approx(0.0) + assert stream.frame_number == 1 + previous = stream.position + for i in range(2, 100): + assert stream.read() is not False + assert stream.position > previous + assert stream.frame_number == i + previous = stream.position + + +def test_no_pts_avi_matches_mp4(test_no_pts_avi: str, test_movie_clip: str): + """Cuts detected on the AVI must land on the same frames as on the mp4 it was encoded from. + Before the fix the AVI cuts were one frame late and frame numbers were permuted.""" + expected = _detect_cuts(test_movie_clip, _BFRAMES_NUM_FRAMES) + actual = _detect_cuts(test_no_pts_avi) + assert len(expected) > 0 + assert [cut.frame_num for cut in actual] == [cut.frame_num for cut in expected] + for cut, oracle in zip(actual, expected, strict=True): + # The AVI time base (1/600) cannot represent 24000/1001 exactly, so allow a few ms. + assert cut.seconds == pytest.approx(oracle.seconds, abs=0.005) + + +def test_no_pts_avi_seek(test_no_pts_avi: str, auto_close): + """Seeking must give the same positions whether or not a frame was read first, since the DTS + offset has to be probed separately when `seek()` is the first call.""" + read_first = auto_close(VideoStreamAv(test_no_pts_avi)) + assert read_first.read() is not False + read_first.seek(50) + seek_first = auto_close(VideoStreamAv(test_no_pts_avi)) + seek_first.seek(50) + for stream in (read_first, seek_first): + assert stream.frame_number == 50 + assert stream._frames_decoded == 50 + frame_ms = 1000.0 / float(stream.frame_rate) + assert stream.position_ms == pytest.approx(49 * frame_ms, abs=frame_ms / 2) + assert stream.read() is not False + assert stream.frame_number == 51 + assert stream._frames_decoded == 51 + assert read_first.position == seek_first.position + + +def test_no_pts_avi_reopen(test_no_pts_avi: str, auto_close): + """Re-opening the container (`reset()`, and the AUTO threading EOF re-open) must keep GENPTS + disabled, otherwise positions go out of order again partway through the video.""" + stream = auto_close(VideoStreamAv(test_no_pts_avi, threading_mode="AUTO")) + for _ in range(10): + assert stream.read() is not False + stream.reset() + assert stream.frame_number == 0 + for _ in range(10): + assert stream.read() is not False + assert stream.frame_number == 10 + previous, frames = stream.position, stream.frame_number + assert stream._handle_eof() is True + assert stream.frame_number == 10 + assert stream.position == previous + while stream.read() is not False: + frames += 1 + assert stream.position > previous + previous = stream.position + # Not compared to `stream.duration`: this AVI declares a 600 Hz stream rate, so ffmpeg (and + # OpenCV) report its frame count in ticks. See the TODO in `VideoStreamAv._get_duration`. + assert frames == _BFRAMES_NUM_FRAMES + + +def test_raw_stream_without_timestamps_counts_frames( + test_raw_h264: str, test_movie_clip: str, auto_close +): + """A raw elementary stream has neither PTS nor DTS. Positions are then derived from the number + of decoded frames (like the OpenCV backend) instead of every frame reporting time 0. Seeking + raises `SeekError`, except to the start, which re-opens the stream.""" + stream = auto_close(VideoStreamAv(test_raw_h264)) + for i in range(1, 11): + assert stream.read() is not False + assert stream.frame_number == i + assert stream.position == stream.base_timecode + (i - 1) + assert stream._offset is None + with pytest.raises(SeekError): + stream.seek(50) + stream.seek(0) + assert stream.frame_number == 0 + assert stream.read() is not False + assert stream.frame_number == 1 + expected = _detect_cuts(test_movie_clip, _BFRAMES_NUM_FRAMES) + actual = _detect_cuts(test_raw_h264) + assert [cut.frame_num for cut in actual] == [cut.frame_num for cut in expected] + + +# +# Missing timestamps and monotonicity +# + + +def _positions(stream: VideoStreamAv, frames) -> list[int]: + """Feed synthetic `(pts, dts)` frames (one nominal frame period per tick) through the + timestamp policy and return the resulting positions in ticks.""" + stream._offset = None + stream._timecode = None + stream._frames_decoded = 0 + stream._warned_non_monotonic = False + positions = [] + for pts, dts in frames: + frame = SimpleNamespace(pts=pts, dts=dts, time_base=1 / stream.frame_rate) + stream._frame = frame # type: ignore[assignment] + stream._frames_decoded += 1 + stream._update_timecode() + assert stream._timecode is not None + positions.append(stream._timecode.pts) + return positions + + +def test_missing_timestamps_policy( + test_video_file: str, monkeypatch: pytest.MonkeyPatch, caplog, auto_close +): + """Each frame is placed at its PTS, else its DTS, else one frame after the previous one. The + first two cases are the examples raised in PR #592 against choosing a single source for the + whole stream from its first frame.""" + stream = auto_close(VideoStreamAv(test_video_file)) + fake_stream = _AttrOverridingProxy(stream._video_stream, start_time=0) + monkeypatch.setattr(VideoStreamAv, "_video_stream", property(lambda self: fake_stream)) + + # Frames before the first timestamp are spaced at the nominal frame rate, and later + # timestamps are followed (relative to that estimate) rather than being ignored. + assert _positions(stream, ((None, None), (2, None), (5, None), (7, None))) == [0, 1, 4, 6] + # Frames without a PTS fall back to their DTS, and frames with neither advance one frame + # period instead of extrapolating the previous (possibly large) step. + assert _positions(stream, ((1, 0), (4, 3), (None, None), (None, None), (7, 6))) == [ + 1, + 4, + 5, + 6, + 7, + ] + assert _positions(stream, ((0, 0), (None, 1), (None, None), (3, 3))) == [0, 1, 2, 3] + # DTS-only streams start at time 0 regardless of the initial reorder delay. + assert _positions(stream, ((None, 2), (None, 3), (None, 4), (None, 5))) == [0, 1, 2, 3] + assert not [record for record in caplog.records if "backwards" in record.message] + + # A frame with a timestamp but no time base is placed like one without a timestamp. + stream._offset = None + stream._timecode = None + stream._frames_decoded = 1 + stream._frame = SimpleNamespace(pts=None, dts=2, time_base=None) + stream._update_timecode() + assert stream._offset is None + assert stream.frame_number == 1 + + +def test_timestamp_behind_estimate_warns( + test_video_file: str, monkeypatch: pytest.MonkeyPatch, caplog, auto_close +): + """A timestamp placed before the previous *estimated* position must warn too, since the + reported positions go backwards either way (raised in PR #592).""" + stream = auto_close(VideoStreamAv(test_video_file)) + fake_stream = _AttrOverridingProxy(stream._video_stream, start_time=0) + monkeypatch.setattr(VideoStreamAv, "_video_stream", property(lambda self: fake_stream)) + frames = ((1, 0), (2, 1), (None, None), (None, None), (None, None), (4, 3)) + assert _positions(stream, frames) == [1, 2, 3, 4, 5, 4] + warnings = [record for record in caplog.records if "backwards" in record.message] + assert len(warnings) == 1 + assert "frame 6" in warnings[0].message + + +def test_missing_pts_ts_follows_timestamps( + test_missing_pts_ts: str, test_vfr_drop3_video: str, caplog, auto_close +): + """A VFR MPEG-TS whose first frames carry no timestamps, and later frames sporadically lack + PTS, DTS or both, must follow the timestamps that are present rather than counting frames + (which drifts over 3 seconds behind by the last frame of this file). Frames without any + timestamp are placed one nominal frame period after the previous frame.""" + oracle = auto_close(VideoStreamAv(test_vfr_drop3_video)) + expected = [] + while oracle.read(decode=False) is not False: + expected.append(oracle.position.seconds) + stream = auto_close(VideoStreamAv(test_missing_pts_ts)) + frame_period = 1.0 / float(stream.frame_rate) + actual, has_timestamp = [], [] + while stream.read(decode=False) is not False: + actual.append(stream.position.seconds) + has_timestamp.append(stream._frame.pts is not None or stream._frame.dts is not None) + assert len(actual) == len(expected) == 160 + assert has_timestamp[:4] == [False] * 4 and not all(has_timestamp[4:]) + assert actual[0] == 0.0 + assert all(later > earlier for earlier, later in itertools.pairwise(actual)) + # The leading frames without timestamps are spaced at the nominal frame rate, so positions + # are only expected to match the source up to a constant offset (two dropped frames here). + anchor = has_timestamp.index(True) + shift = float(expected[anchor] - actual[anchor]) + assert 0 <= shift <= anchor * frame_period + for i in range(anchor, len(actual)): + if has_timestamp[i]: + assert float(actual[i]) + shift == pytest.approx(float(expected[i]), abs=1e-3), i + else: + assert float(actual[i] - actual[i - 1]) == pytest.approx(frame_period, abs=1e-3), i + assert not [record for record in caplog.records if "backwards" in record.message] + + +def test_seek_probe_without_timestamps(test_raw_h264: str, auto_close): + """A stream without timestamps cannot be seeked: the probe gives up after + `MAX_SEEK_PROBE_FRAMES` frames, and later seeks fail immediately without probing again or + re-opening the stream. The stream can still be read from the start after `reset()`.""" + stream = auto_close(VideoStreamAv(test_raw_h264)) + for _ in range(3): + assert stream.read(decode=False) is not False + with pytest.raises(SeekError): + stream.seek(100) + assert stream.frame_number == MAX_SEEK_PROBE_FRAMES + container = stream._container + with pytest.raises(SeekError): + stream.seek(100) + assert stream._container is container + assert stream.frame_number == MAX_SEEK_PROBE_FRAMES + stream.reset() + assert stream.read(decode=False) is not False + assert stream.frame_number == 1 + + +def test_seek_to_start_before_offset_known(test_missing_pts_ts: str, auto_close): + """`seek(0)` before the offset is known must re-open the stream rather than seeking the + container, which can land on a later keyframe (frame 61 here, whose PTS is also missing) + and would anchor the offset there, shifting every position for the life of the stream.""" + sequential = auto_close(VideoStreamAv(test_missing_pts_ts)) + expected = [] + for _ in range(10): + assert sequential.read(decode=False) is not False + expected.append(sequential.position) + for first_reads in (0, 2): + stream = auto_close(VideoStreamAv(test_missing_pts_ts)) + for _ in range(first_reads): + assert stream.read(decode=False) is not False + stream.seek(0) + assert stream.frame_number == 0 + actual = [] + for _ in range(10): + assert stream.read(decode=False) is not False + actual.append(stream.position) + assert actual == expected, first_reads + assert stream._offset == sequential._offset + + +def test_seek_walks_past_frames_without_timestamps( + test_video_file: str, monkeypatch: pytest.MonkeyPatch, auto_close +): + """If the keyframe a seek lands on carries no timestamp, its position is unknown, so the + seek must keep reading until a frame with a timestamp is found before deciding it has reached + the target. Otherwise the next `read()` after `seek(200)` would return frame 152.""" + stream = auto_close(VideoStreamAv(test_video_file)) + assert stream.read(decode=False) is not False + real_container = stream._container + strip_next = False + + def stripping_decode(*args, **kwargs): + nonlocal strip_next + for frame in real_container.decode(*args, **kwargs): + if strip_next: + strip_next = False + frame.pts = None + frame.dts = None + yield frame + + def stripping_seek(*args, **kwargs): + nonlocal strip_next + strip_next = True + return real_container.seek(*args, **kwargs) + + stream._container = _AttrOverridingProxy( + real_container, decode=stripping_decode, seek=stripping_seek + ) + stream.seek(200) + assert stream.frame_number == 200 + assert stream.read(decode=False) is not False + assert stream.frame_number == 201 + + +def _true_frame_number(stream: VideoStreamAv, pts: int) -> int: + """1-based number of the frame with presentation timestamp `pts` in a CFR stream.""" + video_stream = stream._video_stream + elapsed = (pts - (video_stream.start_time or 0)) * _to_fraction(video_stream.time_base) + return round(elapsed * stream.frame_rate) + 1 + + +def test_seek_reads_from_start_if_probe_passes_target( + test_video_file: str, monkeypatch: pytest.MonkeyPatch, caplog, auto_close +): + """Frames read while looking for a timestamp after a seek cannot be given back, so if the + first timestamp found is already past the target, the seek must read forward from the start + instead. Otherwise `seek(2)` on a stream whose frames 1-4 have no timestamps would leave the + next `read()` on frame 6 (raised in PR #592).""" + stream = auto_close(VideoStreamAv(test_video_file)) + true_frame = None + + def stripping_decode(container, *args, **kwargs): + nonlocal true_frame + for frame in container.decode(*args, **kwargs): + true_frame = _true_frame_number(stream, frame.pts) + if true_frame <= 4: + frame.pts = None + frame.dts = None + yield frame + + # Strip the same frames from containers opened by `reset()`. + open_container = stream._open_container + monkeypatch.setattr( + stream, + "_open_container", + lambda rewind=False: _FaultInjectingContainer(open_container(rewind), stripping_decode), + ) + stream._container = _FaultInjectingContainer(stream._container, stripping_decode) + stream.seek(2) + assert stream.frame_number == 2 + assert stream.read(decode=False) is not False + assert true_frame == 3 + assert stream.frame_number == 3 + assert not [record for record in caplog.records if "backwards" in record.message] + + +def test_seek_reads_from_start_if_probe_finds_no_timestamp(test_video_file: str, auto_close): + """If no frame carries a timestamp within `MAX_SEEK_PROBE_FRAMES` frames after a seek, where + the demuxer landed is unknown, so the seek must read forward from the start instead. Otherwise + the frames would be estimated forward from the target and `seek(265)` would leave the next + `read()` on frame 292 (raised in PR #592).""" + stream = auto_close(VideoStreamAv(test_video_file)) + assert stream.read(decode=False) is not False + real_container = stream._container + strip_remaining = 0 + + def stripping_decode(*args, **kwargs): + nonlocal strip_remaining + for frame in real_container.decode(*args, **kwargs): + if strip_remaining > 0: + strip_remaining -= 1 + frame.pts = None + frame.dts = None + yield frame + + def stripping_seek(*args, **kwargs): + nonlocal strip_remaining + strip_remaining = MAX_SEEK_PROBE_FRAMES + return real_container.seek(*args, **kwargs) + + stream._container = _AttrOverridingProxy( + real_container, decode=stripping_decode, seek=stripping_seek + ) + stream.seek(265) + assert stream.frame_number == 265 + assert stream.read(decode=False) is not False + assert stream._frame is not None and stream._frame.pts is not None + assert _true_frame_number(stream, stream._frame.pts) == 266 + assert stream.frame_number == 266 + + +def test_missing_pts_ts_seek_matches_sequential(test_missing_pts_ts: str, caplog, auto_close): + """Seeking must land where a sequential read would. The first keyframe of this file has no + timestamps, so the demuxer lands on the next one (frame 61) for any target before it, which + used to leave the stream there. The file is VFR, so the expected frame is found by time: + `seek(t)` followed by `read()` must return the frame after the first one at or after `(t - 1)` + nominal frame periods.""" + stream = auto_close(VideoStreamAv(test_missing_pts_ts)) + sequential = [] + while stream.read(decode=False) is not False: + sequential.append(stream.position.seconds) + frame_period = 1.0 / float(stream.frame_rate) + # Positions are in 90 kHz ticks, so allow one tick when comparing against frame boundaries. + tick = 1.0 / 90000 + for target in (2, 10, 50, 100, 150): + stream.seek(target) + assert stream.read(decode=False) is not False + target_seconds = (target - 1) * frame_period + landed = next(i for i, seconds in enumerate(sequential) if seconds >= target_seconds - tick) + assert stream.position.seconds == sequential[landed + 1], target + assert not [record for record in caplog.records if "backwards" in record.message] + + +def test_non_monotonic_timestamps_warn_once( + test_video_file: str, monkeypatch: pytest.MonkeyPatch, caplog, auto_close +): + """A timestamp going backwards is reported once per stream, and only when it actually goes + backwards: repeated timestamps (as in VFR video) stay silent.""" + stream = auto_close(VideoStreamAv(test_video_file)) + fake_stream = _AttrOverridingProxy(stream._video_stream, start_time=0) + monkeypatch.setattr(VideoStreamAv, "_video_stream", property(lambda self: fake_stream)) + time_base = Fraction(1, 1000) + for pts in (0, 40, 40, 0, 80, 20): + stream._frame = SimpleNamespace(pts=pts, dts=None, time_base=time_base) + stream._frames_decoded += 1 + stream._update_timecode() + warnings = [record for record in caplog.records if "backwards" in record.message] + assert len(warnings) == 1 + assert "frame 4" in warnings[0].message + # Positions keep following the input; the warning does not alter them. + assert stream.position_ms == pytest.approx(20.0) diff --git a/website/pages/changelog.md b/website/pages/changelog.md index f674489a..03f5f4c3 100644 --- a/website/pages/changelog.md +++ b/website/pages/changelog.md @@ -826,4 +826,12 @@ Development - [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) - [bugfix] `split-video` with `-o` now logs the output folder instead of repeating the filename template [#595](https://github.com/Breakthrough/PySceneDetect/pull/595) - [bugfix] `write_scene_list()` now starts the cut list row with `Timecode List:` when no `cut_list` is passed, as it does when one is passed and as `write_scene_list_html()` does - - [bugfix] Allow `-d`/`--downscale` on the CLI and `downscale` in the config file to be set to `0` for automatic downscaling \ No newline at end of file + - [bugfix] Allow `-d`/`--downscale` on the CLI and `downscale` in the config file to be set to `0` for automatic downscaling + - [bugfix] Fix `pyav` backend timing for videos that lack presentation timestamps (e.g. AVI files, elementary H.264 streams), which could lead to frame positions being out of order or stuck at zero and shifting cuts by one or more frames ([#590](https://github.com/Breakthrough/PySceneDetect/issues/590), [PyAV#933](https://github.com/PyAV-Org/PyAV/discussions/933), [PyAV#2478](https://github.com/PyAV-Org/PyAV/pull/2478)) + - Videos are now opened with FFmpeg's `genpts` flag disabled. + - Each frame will use the exact presentation timestamp, but if it is unavailable we now fall back to the decode timestamp to be consistent with the `opencv` backend. + - A frame with neither timestamp is placed one frame period after the previous frame (as the `moviepy` backend does for every frame). + - A one-time warning is emitted if timestamps are still detected going backwards. + - `VideoStreamAv.seek()` now raises `SeekError` on streams that cannot be seeked (e.g. elementary streams without timestamps) instead of a backend-specific exception. + - `VideoStreamAv.seek()` now falls back to reading from the start when the demuxer lands on a frame without a timestamp past the target, fixing wrong positions after seeking in some MPEG-TS files. + - [api] `VideoStream.seek()` now documents that the stream position is undefined after a `SeekError` is raised, and `reset()` must be called before reading further