From 06dcd1eaf16004002aa8e090655c1036d5188478 Mon Sep 17 00:00:00 2001 From: WyattBlue Date: Wed, 28 Oct 2026 00:16:57 -0400 Subject: [PATCH 1/3] Keep VideoPlane geometry in sync with its frame VideoPlane cached width, height and buffer_size when created, but read the frame's data pointer live. A plane made before apply_cropping() then paired the shifted pointer with the old size, so bytes(plane), memoryview(plane), update() and DLPack export read or wrote past the buffer. Recompute the geometry whenever the frame's dimensions, data pointer or linesize change. Also document that for hardware frames and 1-bit formats FFmpeg applies only the right and bottom crop, leaving crop_left and crop_top set. --- av/video/frame.py | 4 ++ av/video/plane.pxd | 9 ++- av/video/plane.py | 125 ++++++++++++++++++++++++++------------- tests/test_videoframe.py | 35 +++++++++++ 4 files changed, 131 insertions(+), 42 deletions(-) diff --git a/av/video/frame.py b/av/video/frame.py index b1bdcf8e2..eff2eba91 100644 --- a/av/video/frame.py +++ b/av/video/frame.py @@ -780,6 +780,10 @@ def apply_cropping(self, unaligned=False): No pixels are copied; the planes are narrowed to the cropped region and the ``crop_*`` attributes are reset to 0. + For hardware frames and 1-bit formats (``monob``, ``monow``), FFmpeg only + applies :attr:`crop_right` and :attr:`crop_bottom`; :attr:`crop_left` and + :attr:`crop_top` are left unapplied and keep their values. + By default the left edge may be moved slightly to keep plane data aligned, leaving a few extra pixels. Pass ``unaligned=True`` to crop exactly. diff --git a/av/video/plane.pxd b/av/video/plane.pxd index 80ff79050..72112a771 100644 --- a/av/video/plane.pxd +++ b/av/video/plane.pxd @@ -5,8 +5,13 @@ from av.video.format cimport VideoFormatComponent cdef class VideoPlane(Plane): - cdef readonly size_t buffer_size - cdef readonly unsigned int width, height + # Geometry derived from the frame, recomputed by _refresh() whenever the + # frame's dimensions or data pointer change (e.g. after apply_cropping()). + cdef size_t _size + cdef unsigned int _width, _height + cdef int _seen_width, _seen_height, _seen_linesize + cdef void *_seen_data + cdef void _refresh(self) cdef enum DeviceType: diff --git a/av/video/plane.py b/av/video/plane.py index 2d056103f..0df7f643b 100644 --- a/av/video/plane.py +++ b/av/video/plane.py @@ -21,6 +21,23 @@ @cython.cclass class VideoPlane(Plane): def __cinit__(self, frame: VideoFrame, index: cython.int): + self._seen_data = cython.NULL + self._refresh() + + @cython.cfunc + def _refresh(self) -> cython.void: + frame: VideoFrame = self.frame + index: cython.int = self.index + linesize: cython.int = frame.ptr.linesize[index] + data: cython.p_void = frame.ptr.data[index] + if ( + self._seen_data == data + and self._seen_width == frame.ptr.width + and self._seen_height == frame.ptr.height + and self._seen_linesize == linesize + ): + return + # The palette plane has no associated component or linesize; set fields manually fmt = frame.format if frame.ptr.hw_frames_ctx: @@ -32,44 +49,70 @@ def __cinit__(self, frame: VideoFrame, index: cython.int): ) if index == 1 and fmt.name == "pal8": - self.width = 256 - self.height = 1 - self.buffer_size = 256 * 4 - return - - for i in range(fmt.ptr.nb_components): - if fmt.ptr.comp[i].plane == index: - component = VideoFormatComponent(fmt, i) - self.width = component.width - self.height = component.height - break + self._width = 256 + self._height = 1 + self._size = 256 * 4 else: - raise RuntimeError(f"could not find plane {index} of {fmt!r}") - - # Sometimes, linesize is negative (and that is meaningful). We are only - # insisting that the buffer size be based on the extent of linesize, and - # ignore it's direction. - self.buffer_size = abs(self.frame.ptr.linesize[self.index]) * self.height - - # Cropping moves the data pointer into the buffer, so the last row can end - # past the allocation. Clamp to what the plane's buffer actually holds. - if frame.ptr.hw_frames_ctx or self.frame.ptr.linesize[self.index] < 0: - return - buf: cython.pointer[lib.AVBufferRef] = lib.av_frame_get_plane_buffer( - frame.ptr, index - ) - if buf == cython.NULL: - return - start: cython.p_uchar = frame.ptr.data[index] - end: cython.p_uchar = buf.data + buf.size - if buf.data <= start < end and cython.cast(cython.size_t, end - start) < ( - self.buffer_size - ): - self.buffer_size = end - start + for i in range(fmt.ptr.nb_components): + if fmt.ptr.comp[i].plane == index: + component = VideoFormatComponent(fmt, i) + self._width = component.width + self._height = component.height + break + else: + raise RuntimeError(f"could not find plane {index} of {fmt!r}") + + # Sometimes, linesize is negative (and that is meaningful). We are only + # insisting that the buffer size be based on the extent of linesize, and + # ignore it's direction. + self._size = abs(linesize) * self._height + + # Cropping moves the data pointer into the buffer, so the last row can + # end past the allocation. Clamp to what the plane's buffer holds. + if not frame.ptr.hw_frames_ctx and linesize >= 0: + buf: cython.pointer[lib.AVBufferRef] = lib.av_frame_get_plane_buffer( + frame.ptr, index + ) + if buf != cython.NULL: + start: cython.p_uchar = frame.ptr.data[index] + end: cython.p_uchar = buf.data + buf.size + if ( + buf.data <= start < end + and cython.cast(cython.size_t, end - start) < self._size + ): + self._size = end - start + + self._seen_data = data + self._seen_width = frame.ptr.width + self._seen_height = frame.ptr.height + self._seen_linesize = linesize @cython.cfunc def _buffer_size(self) -> cython.size_t: - return self.buffer_size + self._refresh() + return self._size + + @property + def buffer_size(self): + return self._buffer_size() + + @property + def width(self): + """Width of this plane, in pixels. + + :type: int + """ + self._refresh() + return self._width + + @property + def height(self): + """Height of this plane, in pixels. + + :type: int + """ + self._refresh() + return self._height @property def line_size(self): @@ -95,13 +138,14 @@ def __getbuffer__(self, view: cython.pointer[Py_buffer], flags: cython.int): if flags & PyBUF_WRITABLE and not self._buffer_writable(): raise ValueError("buffer is not writable") + size: cython.size_t = self._buffer_size() ptr: cython.p_void = self._buffer_ptr() line_size: cython.int = self.frame.ptr.linesize[self.index] if line_size < 0: - height: cython.int = self.height + height: cython.int = self._height ptr = cython.cast(cython.p_char, ptr) + (height - 1) * line_size - PyBuffer_FillInfo(view, self, ptr, self._buffer_size(), 0, flags) + PyBuffer_FillInfo(view, self, ptr, size, 0, flags) def __dlpack_device__(self): if self.frame.ptr.hw_frames_ctx: @@ -148,6 +192,7 @@ def __dlpack__(self, *, stream: int | None = None): device_type = kCPU device_id = 0 + self._refresh() line_size = self.line_size if line_size < 0: raise NotImplementedError( @@ -260,14 +305,14 @@ def __dlpack__(self, *, stream: int | None = None): if ncomp == 1: ndim = 2 - s0 = self.height - s1 = self.width + s0 = self._height + s1 = self._width st0 = line_size // itemsize st1 = 1 else: ndim = 3 - s0 = self.height - s1 = self.width + s0 = self._height + s1 = self._width s2 = ncomp st0 = line_size // itemsize st1 = ncomp diff --git a/tests/test_videoframe.py b/tests/test_videoframe.py index 284ae32f0..ccdaffe56 100644 --- a/tests/test_videoframe.py +++ b/tests/test_videoframe.py @@ -1645,3 +1645,38 @@ def test_decode_error_flags() -> None: frames = _decode_m2v(data) assert any(DecodeError.concealment_active in f.decode_error_flags for f in frames) assert any(frame.is_corrupt for frame in frames) + + +def test_apply_cropping_updates_existing_planes() -> None: + array = numpy.arange(48 * 64, dtype=numpy.uint8).reshape(48, 64) + frame = VideoFrame.from_numpy_buffer(array, format="gray") + plane = frame.planes[0] + frame.crop_top = 40 + frame.crop_left = 8 + frame.apply_cropping(unaligned=True) + + # A plane made before cropping must not pair the shifted data pointer with + # its old size, or reading it runs past the end of the array. + assert (plane.width, plane.height) == (56, 8) + assert plane.buffer_size == 64 * 7 + 56 + assert bytes(plane) == array.tobytes()[40 * 64 + 8 :] + + +@pytest.mark.parametrize("format", ["monob", "monow"]) +def test_apply_cropping_bitstream(format: str) -> None: + frame = VideoFrame(64, 48, format) + plane = frame.planes[0] + line_size = plane.line_size + frame.crop_left = 8 + frame.crop_top = 4 + frame.crop_right = 3 + frame.crop_bottom = 5 + frame.apply_cropping() + + # FFmpeg cannot move a data pointer by a fraction of a byte, so only the + # right and bottom crops apply. + assert (frame.width, frame.height) == (61, 43) + assert (frame.crop_left, frame.crop_top) == (8, 4) + assert (frame.crop_right, frame.crop_bottom) == (0, 0) + assert plane.buffer_size == line_size * 43 + assert frame.to_ndarray(format="rgb24").shape == (43, 61, 3) From 23237b467aa046e8dc599f52d1698e45945e3f6d Mon Sep 17 00:00:00 2001 From: WyattBlue Date: Wed, 28 Oct 2026 00:19:44 -0400 Subject: [PATCH 2/3] Validate VideoFrame crop setters Reject a crop when the two crops on an axis would leave no pixels, the same rule av_frame_apply_cropping() uses. Without this, nonsense values reached hardware filters such as scale_cuda, scale_vt and scale_vulkan, which read the crop fields without checking them. Zero is always allowed so the fields can be cleared. --- av/video/frame.py | 46 ++++++++++++++++++++++++++++++++++++---- tests/test_videoframe.py | 26 +++++++++++++++++++---- 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/av/video/frame.py b/av/video/frame.py index eff2eba91..cbf541c62 100644 --- a/av/video/frame.py +++ b/av/video/frame.py @@ -456,6 +456,20 @@ def alloc_video_frame() -> VideoFrame: return VideoFrame(_cinit_bypass_sentinel) +def _check_crop(value, other, size, names): + # Mirror av_frame_apply_cropping(): the two crops on an axis must leave at + # least one pixel. Some hardware filters read these fields unchecked. + value = int(value) + if value < 0: + raise ValueError(f"{names[0]} must not be negative") + if value and value + other >= size: + raise ValueError( + f"{names[0]} + {names[1]} must be less than the frame " + f"{names[2]} ({size}), got {value} + {other}" + ) + return value + + class PictureType(IntEnum): NONE = lib.AV_PICTURE_TYPE_NONE # Undefined I = lib.AV_PICTURE_TYPE_I # Intra @@ -722,6 +736,10 @@ def crop_top(self): option is ``"0"``, in which case the crop is reported here and can be applied with :meth:`apply_cropping`. + Setting a value raises :class:`ValueError` if it would leave no rows, + i.e. if ``crop_top + crop_bottom`` would reach :attr:`height`. The same + goes for ``crop_left + crop_right`` and :attr:`width`. + Wraps :ffmpeg:`AVFrame.crop_top`. :type: int @@ -730,7 +748,12 @@ def crop_top(self): @crop_top.setter def crop_top(self, value): - self.ptr.crop_top = value + self.ptr.crop_top = _check_crop( + value, + self.ptr.crop_bottom, + self.ptr.height, + ("crop_top", "crop_bottom", "height"), + ) @property def crop_bottom(self): @@ -744,7 +767,12 @@ def crop_bottom(self): @crop_bottom.setter def crop_bottom(self, value): - self.ptr.crop_bottom = value + self.ptr.crop_bottom = _check_crop( + value, + self.ptr.crop_top, + self.ptr.height, + ("crop_bottom", "crop_top", "height"), + ) @property def crop_left(self): @@ -758,7 +786,12 @@ def crop_left(self): @crop_left.setter def crop_left(self, value): - self.ptr.crop_left = value + self.ptr.crop_left = _check_crop( + value, + self.ptr.crop_right, + self.ptr.width, + ("crop_left", "crop_right", "width"), + ) @property def crop_right(self): @@ -772,7 +805,12 @@ def crop_right(self): @crop_right.setter def crop_right(self, value): - self.ptr.crop_right = value + self.ptr.crop_right = _check_crop( + value, + self.ptr.crop_left, + self.ptr.width, + ("crop_right", "crop_left", "width"), + ) def apply_cropping(self, unaligned=False): """Crop the frame in place according to its ``crop_*`` attributes. diff --git a/tests/test_videoframe.py b/tests/test_videoframe.py index ccdaffe56..3acbd71a3 100644 --- a/tests/test_videoframe.py +++ b/tests/test_videoframe.py @@ -1582,11 +1582,29 @@ def test_apply_cropping_plane_stays_in_buffer() -> None: assertNdarraysEqual(frame.to_ndarray(), array[:, 10:]) -def test_apply_cropping_invalid() -> None: +def test_crop_setters_reject_invalid() -> None: frame = VideoFrame(64, 48, "yuv420p") - frame.crop_left = 100 - with pytest.raises(av.error.OverflowError): - frame.apply_cropping() + with pytest.raises(ValueError): + frame.crop_left = 64 + with pytest.raises(ValueError): + frame.crop_top = -1 + + frame.crop_left = 40 + with pytest.raises(ValueError, match="crop_right"): + frame.crop_right = 24 + frame.crop_right = 23 + + frame.crop_bottom = 47 + with pytest.raises(ValueError, match="crop_top"): + frame.crop_top = 1 + assert (frame.crop_left, frame.crop_right) == (40, 23) + assert (frame.crop_top, frame.crop_bottom) == (0, 47) + + # Zero is always allowed, even on a frame with no pixels. + empty = VideoFrame() + empty.crop_left = 0 + with pytest.raises(ValueError): + empty.crop_left = 1 def _encode_m2v() -> bytearray: From 9f62869465a7f67dc748620209609cb26135d316 Mon Sep 17 00:00:00 2001 From: WyattBlue Date: Wed, 28 Oct 2026 00:34:04 -0400 Subject: [PATCH 3/3] Drop crop fields when reformat() resizes a frame av_frame_copy_props() copies crop_* onto the reformatted frame. They are in source pixels, so they still hold after a pixel format change but not after a resize, where they can exceed the new dimensions. Clear them when the size changes. --- av/video/reformatter.py | 9 +++++++++ tests/test_videoframe.py | 20 ++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/av/video/reformatter.py b/av/video/reformatter.py index 7498bdcf1..030fc69fb 100644 --- a/av/video/reformatter.py +++ b/av/video/reformatter.py @@ -303,6 +303,15 @@ def _reformat( new_frame.ptr.width = width new_frame.ptr.height = height + # The copied crop is in source pixels. It still holds after a pixel + # format change, but not after a resize, where it could even exceed the + # new dimensions. + if width != frame.ptr.width or height != frame.ptr.height: + new_frame.ptr.crop_top = 0 + new_frame.ptr.crop_bottom = 0 + new_frame.ptr.crop_left = 0 + new_frame.ptr.crop_right = 0 + # A transfer-characteristic / primaries conversion is opt-in. Unlike the # pre-17.0 sws_scale, sws_scale_frame inspects color_trc/color_primaries # and rejects RESERVED (and other unsupported) values with EOPNOTSUPP, diff --git a/tests/test_videoframe.py b/tests/test_videoframe.py index 3acbd71a3..bc75d7a0d 100644 --- a/tests/test_videoframe.py +++ b/tests/test_videoframe.py @@ -1698,3 +1698,23 @@ def test_apply_cropping_bitstream(format: str) -> None: assert (frame.crop_right, frame.crop_bottom) == (0, 0) assert plane.buffer_size == line_size * 43 assert frame.to_ndarray(format="rgb24").shape == (43, 61, 3) + + +def test_reformat_crop() -> None: + frame = VideoFrame(64, 48, "yuv420p") + frame.crop_left = 40 + frame.crop_top = 30 + frame.crop_bottom = 2 + + # Same size: the crop still describes the same pixels. + rgb = frame.reformat(format="rgb24") + assert (rgb.crop_left, rgb.crop_top, rgb.crop_bottom) == (40, 30, 2) + rgb.apply_cropping(unaligned=True) + assert (rgb.width, rgb.height) == (24, 16) + + # Resized: the source crop no longer fits, so it is dropped. + small = frame.reformat(width=32, height=24, format="rgb24") + assert small.crop_left == small.crop_top == small.crop_bottom == 0 + assert small.crop_right == 0 + small.apply_cropping() + assert (small.width, small.height) == (32, 24)