Harden frame cropping

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,14 +805,23 @@ 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.
 
         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.

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:

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

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,

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:
@@ -1645,3 +1663,58 @@ 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)
+
+
+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)