Skip to content

Make the data views writable#536

Merged
BoboTiG merged 5 commits into
BoboTiG:mainfrom
jholveck:memoryview-tweaks
Jun 26, 2026
Merged

Make the data views writable#536
BoboTiG merged 5 commits into
BoboTiG:mainfrom
jholveck:memoryview-tweaks

Conversation

@jholveck

@jholveck jholveck commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

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.

Changes proposed in this PR

  • Tests added/updated
  • Documentation updated
  • Changelog entry added
  • ./check.sh passed

jholveck added 3 commits June 1, 2026 20:35
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.
In the release notes, one change was accidentally in PR BoboTiG#535 instead,
and one change was omitted entirely.  Fix.
@jholveck

Copy link
Copy Markdown
Contributor Author

@BoboTiG , @halldorfannar , do you have any comments on this, or should we just go ahead and merge it?

@BoboTiG

BoboTiG commented Jun 25, 2026

Copy link
Copy Markdown
Owner

I think I prefer what you've done so far. I'm not really for the ChatGPT idea.

Documentation is clear enough, and I could live with UB :)

If @halldorfannar has no concern, OK to merge for me.

@halldorfannar halldorfannar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also fine with this approach but found two documentation issues that I suggested fixes for.

Comment thread docs/source/release-history/v11.0.0.md Outdated
Comment thread src/mss/screenshot.py Outdated
jholveck and others added 2 commits June 25, 2026 16:11
Co-authored-by: Halldor Fannar <halldorfannar@users.noreply.github.com>
@jholveck

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback, both of you!

The current test failure is because of a minor issue in main, fixed by #549. Once that gets committed to main, we can merge it into this branch, or we can just merge this into main directly (they don't conflict).

@jholveck jholveck marked this pull request as ready for review June 26, 2026 00:05
@BoboTiG BoboTiG merged commit cced209 into BoboTiG:main Jun 26, 2026
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants