fix: make the data views writable (#536)

* Make the data views writable.

Previously (while working on 11.0), we changed the .bgra and .rgb
attributes to be read-only memoryviews.  We also removed the .raw
attribute.

The main reason we made the memoryviews read-only is so that we don't
have to worry about cached values of .pixels and .rgb being in-sync
with the underlying buffer, if users mutated the pixel data.

However, I now realize that this takes away a possible valuable use
case: ctypes.  ctypes can only create an array (or pointer) from a
buffer if it's writable; it doesn't support a read-only version.
Previously, users could make a ctypes pointer using the .raw
attribute.  The new API doesn't give them that ability, without
copying the data (or using ctypes' from_address, which is perilous).

Read-only buffers also trigger a warning from PyTorch, although it's
clearly-written and only is printed once.

This PR, as it stands, makes the `.bgra` and `.rgb` properties return
writable memoryviews.  (It also restores `.raw`, this time as a
deprecated alias of bgra, since there's no pressing need to remove the
name entirely.)

It also documents that, while these memoryviews are writable, actually
modifying the data may cause undefined behavior.

There's alternatives, of course.  Instead of what I've got here,
ChatGPT instead suggests that we leave .bgra and .rgb read-only (as
they were in 10.2, since they were `bytes` objects), and to add a
property like `.writable_bgra` or something.  Its argument is as
follows:

> The problem with making .bgra writable and saying “mutating is UB
> [undefined behavior]” is that Python users will absolutely see
> writable buffer and think “cool, in-place image editing.”  That’s
> not UB in the fun C sense; it’s more like “congratulations, you have
> a weird cache-invalidation footgun.”  The API shape would be lying a
> little.

I'm making this PR for discussion and consensus on the best way to go.

* Update release notes

In the release notes, one change was accidentally in PR #535 instead,
and one change was omitted entirely.  Fix.

* Apply suggestions from code review

Co-authored-by: Halldor Fannar <halldorfannar@users.noreply.github.com>

---------

Co-authored-by: Halldor Fannar <halldorfannar@users.noreply.github.com>

Author: jholveck

docs/source/release-history/v11.0.0.md

@@ -15,11 +15,13 @@ writing, but should be by the time we release 11.0!)
 
 #### ScreenShot attributes
 
-The {py:attr}`mss.ScreenShot.raw` attribute has been removed.  Use the {py:attr}`mss.ScreenShot.bgra` property instead.
+The {py:attr}`mss.ScreenShot.raw` attribute has been deprecated, and will soon be removed.  Use the
+{py:attr}`mss.ScreenShot.bgra` property instead.
 
-The {py:attr}`mss.ScreenShot.bgra` and {py:attr}`mss.ScreenShot.rgb` properties now will return read-only bytes-like
+The {py:attr}`mss.ScreenShot.bgra` and {py:attr}`mss.ScreenShot.rgb` properties now will return bytes-like
 {py:type}`memoryview` objects, not necessarily {py:type}`bytes` or {py:type}`bytearray` objects.  For practical use
-cases, this should not be noticible.  This change was allows faster access to screenshot data, with fewer memory copies.
+cases, this should not be noticible.  This change allows faster access to screenshot data, with fewer memory
+copies.
 
 ### Python 3.9 EOL
 

src/mss/screenshot.py

@@ -3,6 +3,7 @@
 
 from __future__ import annotations
 
+import warnings
 from typing import TYPE_CHECKING
 
 from mss.exception import ScreenShotError
@@ -26,7 +27,6 @@ class ScreenShot:
     __slots__ = {"__bgra", "__pixels", "__rgb", "_raw", "pos", "size"}
 
     def __init__(self, data: Buffer, monitor: Monitor, /, *, size: Size | None = None) -> None:
-        self.__bgra: memoryview | None = None
         self.__pixels: Pixels | None = None
         self.__rgb: memoryview | None = None
 
@@ -36,13 +36,10 @@ def __init__(self, data: Buffer, monitor: Monitor, /, *, size: Size | None = Non
         #: NamedTuple of the screenshot size.
         self.size: Size = Size(monitor["width"], monitor["height"]) if size is None else size
 
-        # Buffer of the raw BGRA pixels, retrieved by the
-        # platform-specific implementations.  This is kept read-write
-        # if it was originally so, in order for _merge to work.
-        # However, it should be made read-only before returning to the
-        # user (via bgra), so that the cached values for __pixels and
-        # __rgb aren't potentially inconsistent if the user changes
-        # data.
+        # Buffer of the raw BGRA pixels, retrieved by the platform-specific implementations.  This is kept read-write if
+        # it was originally so, in order for _merge to work, and so it can be used with ctypes.  However, it should not
+        # be modified once __pixels or __rgb have been accessed, so that the cached values for __pixels and __rgb aren't
+        # potentially inconsistent if the user changes data.
         self._raw: memoryview = memoryview(data)
         assert self._raw.nbytes == self.size.width * self.size.height * 4, (  # noqa: S101
             "Data size does not match screenshot dimensions."
@@ -57,14 +54,18 @@ def __array_interface__(self) -> dict[str, Any]:
         """NumPy array interface support.
 
         This is used by NumPy, many SciPy projects, CuPy, PyTorch (via
-        ``torch.from_numpy``), TensorFlow (via ``tf.convert_to_tensor``),
-        JAX (via ``jax.numpy.asarray``), Pandas, scikit-learn, Matplotlib,
+        :py:func:`torch.from_numpy`), TensorFlow (via
+        :py:func:`tf.convert_to_tensor`), JAX (via
+        :py:func:`jax.numpy.asarray`), Pandas, scikit-learn, Matplotlib,
         some OpenCV functions, and others.  This allows you to pass a
         :class:`ScreenShot` instance directly to these libraries without
         needing to convert it first.
 
         This is in HWC order, with 4 channels (BGRA).
 
+        The array is read-write, for maximum compatibility.  However,
+        actually modifying the data may cause undefined behavior.
+
         .. seealso::
 
             https://numpy.org/doc/stable/reference/arrays.interface.html
@@ -74,7 +75,7 @@ def __array_interface__(self) -> dict[str, Any]:
             "version": 3,
             "shape": (self.height, self.width, 4),
             "typestr": "|u1",
-            "data": self.bgra,
+            "data": self._raw,
         }
 
     @classmethod
@@ -91,21 +92,43 @@ def bgra(self) -> memoryview:
         BGRxBGRx... sequence.  A specific pixel can be accessed as
         ``bgra[(y * width + x) * 4:(y * width + x) * 4 + 4].``
 
-        The memoryview is read-only.  PyTorch will issue a warning
-        when given a read-only buffer, but will still work.  However,
-        actually modifying the data may cause undefined behavior.
+        The memoryview is read-write, for compatibility with ctypes'
+        :py:func:`from_buffer`.  However, actually modifying the data
+        may cause undefined behavior.
 
         .. note::
             While the name is ``bgra``, the alpha channel may or may
             not be valid.
+
+        .. version-changed:: 11.0.0
+            Prior to this version, this was a :py:class:`bytes` object.
+            It was changed to a memoryview for improved performance.
+            Most practical uses are unaffected by this change, as
+            ``memoryview`` supports most of the same operations as
+            ``bytes``.  If needed, you can use
+            :py:meth:`memoryview.tobytes` to get a ``bytes`` object.
         """
-        # Making a read-only copy of a memoryview is very cheap.  But
-        # we still always return the same memoryview: somebody using a
-        # property may expect it to be identical (under the `is`
-        # operator) every time.
-        if self.__bgra is None:
-            self.__bgra = self._raw.toreadonly()
-        return self.__bgra
+        return self._raw
+
+    @property
+    def raw(self) -> memoryview:
+        """Deprecated alias for :py:attr:`bgra`.
+
+        .. version-deprecated:: 10.2.0
+            Use :py:attr:`bgra` instead.  This alias will be removed in
+            a future version.
+
+        .. version-changed:: 11.0.0
+            Prior to this version, this was a :py:class:`bytearray`.
+            This :py:attr:`raw` alias is retained, although as a
+            :py:class:`memoryview`, for backwards compatibility: most
+            existing uses are not affected, as ``memoryview`` supports
+            most of the same operations as ``bytearray``.  If needed,
+            you can use ``bytearray(raw)`` to get a ``bytearray``
+            object.
+        """
+        warnings.warn("The raw property is deprecated.  Use bgra instead.", DeprecationWarning, stacklevel=2)
+        return self._raw
 
     @property
     def pixels(self) -> Pixels:
@@ -137,24 +160,35 @@ def rgb(self) -> memoryview:
 
         The format is a memoryview object of bytes.  These are in a
         RGBRGB... sequence.  A specific pixel can be accessed as
-        ``rgb[(y * width + x) * 4:(y * width + x) * 4 + 4].``
+        ``rgb[(y * width + x) * 3:(y * width + x) * 3 + 3].``
 
-        The memoryview is read-only.  PyTorch will issue a warning
-        when given a read-only buffer, but will still work.  However,
-        actually modifying the data may cause undefined behavior.
+        The memoryview is read-write, for compatibility with ctypes'
+        :py:func:`from_buffer`.  However, actually modifying the data
+        may cause undefined behavior.
 
-        :: note::
+        .. note::
             This is a computed property.  If possible, using the
-            :py:attr:`bgra` property directly is usually more
-            efficient.
+            :py:attr:`bgra` property directly is usually more efficient.
+
+        .. version-changed:: 11.0.0
+            Prior to this version, this was a :py:class:`bytes` object.
+            It was changed to a memoryview for improved performance.
+            Most practical uses are unaffected by this change, as
+            ``memoryview`` supports most of the same operations as
+            ``bytes``.  If needed, you can use
+            :py:meth:`memoryview.tobytes` to get a ``bytes`` object.
         """
         if self.__rgb is None:
             rgb = bytearray(self.height * self.width * 3)
             raw = self._raw
             rgb[::3] = raw[2::4]
             rgb[1::3] = raw[1::4]
             rgb[2::3] = raw[::4]
-            self.__rgb = memoryview(rgb).toreadonly()
+            # We could just return the bytearray directly.  However, we would rather give ourselves the flexibility to
+            # use other buffer types in the future.  Rather than declaring our return type as just generically Buffer
+            # (which doesn't guarantee the indexing behavior of memoryview), we always return a memoryview, which is
+            # cheap.
+            self.__rgb = memoryview(rgb)
 
         return self.__rgb
 

src/tests/test_bgra_to_rgb.py

@@ -2,15 +2,30 @@
 Source: https://github.com/BoboTiG/python-mss.
 """
 
+import ctypes
+
 from mss.base import ScreenShot
 
 
 def test_good_types(raw: bytes) -> None:
     image = ScreenShot.from_size(bytearray(raw), 1024, 768)
     assert isinstance(image.rgb, memoryview)
-    assert image.rgb.readonly
 
 
 def test_contents() -> None:
     image = ScreenShot.from_size(b"BGRA" * 1024 * 768, 1024, 768)
     assert bytes(image.rgb) == b"RGB" * 1024 * 768
+
+
+def test_ctypes_pointers_from_rgb() -> None:
+    image = ScreenShot.from_size(b"BGRA" * 4, 2, 2)
+    assert image.rgb.readonly is False
+
+    rgb_array = (ctypes.c_uint8 * len(image.rgb)).from_buffer(image.rgb)
+    void_ptr = ctypes.c_void_p(ctypes.addressof(rgb_array))
+    uint8_ptr = ctypes.cast(void_ptr, ctypes.POINTER(ctypes.c_uint8))
+
+    assert void_ptr.value is not None
+    assert uint8_ptr[0] == ord("R")
+    assert uint8_ptr[1] == ord("G")
+    assert uint8_ptr[2] == ord("B")

src/tests/test_get_pixels.py

@@ -2,6 +2,7 @@
 Source: https://github.com/BoboTiG/python-mss.
 """
 
+import ctypes
 import itertools
 from collections.abc import Callable
 
@@ -17,7 +18,6 @@ def test_grab_monitor(mss_impl: Callable[..., MSS]) -> None:
             image = sct.grab(mon)
             assert isinstance(image, ScreenShot)
             assert isinstance(image.bgra, memoryview)
-            assert image.bgra.readonly
 
 
 def test_grab_part_of_screen(mss_impl: Callable[..., MSS]) -> None:
@@ -45,3 +45,18 @@ def test_get_pixel(raw: bytes) -> None:
 
     with pytest.raises(ScreenShotError):
         image.pixel(image.width + 1, 12)
+
+
+def test_ctypes_pointers_from_bgra(raw: bytes) -> None:
+    image = ScreenShot.from_size(bytearray(raw), 1024, 768)
+    assert image.bgra.readonly is False
+
+    bgra_array = (ctypes.c_uint8 * len(image.bgra)).from_buffer(image.bgra)
+    void_ptr = ctypes.c_void_p(ctypes.addressof(bgra_array))
+    uint8_ptr = ctypes.cast(void_ptr, ctypes.POINTER(ctypes.c_uint8))
+
+    assert void_ptr.value is not None
+    assert uint8_ptr[0] == raw[0]
+    assert uint8_ptr[1] == raw[1]
+    assert uint8_ptr[2] == raw[2]
+    assert uint8_ptr[3] == raw[3]