Repository: python-pillow/Pillow

Problem statement:
Image.save() causes images created by Image.frombuffer() to stop reflecting changes in that buffer

Hi, recently messed with PIL to encounter what is probably a bug

### What did you do?

```python
import numpy as np
from PIL import Image


# Source of data for image
im_data: np.typing.NDArray = np.array([[255]], dtype=np.uint8)
assert im_data[0, 0] == 255  # OK

# Attach to buffer protocol of an array, should reflect changes in im_data
im: Image.Image = Image.frombuffer(mode="L", size=(1, 1), data=im_data)
assert im_data[0, 0] == 255  # OK
assert im.getdata()[0] == 255  # OK

im_data[:, :] = np.array([[128]], dtype=np.uint8)
assert im_data[0, 0] == 128  # OK
assert im.getdata()[0] == 128  # OK!

im.save("./test_image.bmp")
assert im_data[0, 0] == 128  # OK
assert im.getdata()[0] == 128  # OK

im_data[:, :] = np.array([[64]], dtype=np.uint8)
assert im_data[0, 0] == 64  # OK
assert im.getdata()[0] == 64  # Failed!!! It is still == 128
```

### What did you expect to happen?

Line with last assert should pass the assertion:
```
...
assert im.getdata()[0] == 64   # Should pass
```

### What actually happened?

Yes, so it seams that `Image.save()` messes with image buffer state. Moreover, this is actually explicitly stated that it does in source code:

https://github.com/python-pillow/Pillow/blob/af09976a5f57e8aebaedf332bc4025ea1b068af4/src/PIL/Image.py#L2513

```python
#     def save(...):
#         ...
           # may mutate self!
           self._ensure_mutable()

```

Once one saved the image, one can no longer work with buffer and expect image working properly.

I can *try* to work out some patches but really I am somewhat new to the pillow and not well aware of all internal connections with things. It would be nice to have some hints on what may going on - then I can try to fix this.

### What are your OS, Python and Pillow versions?

#### OS:
```shell
lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 22.04.5 LTS
Release:	22.04
Codename:	jammy
```

#### Python: 
- `Python 3.12.9`
- Prebuilt binary from deadsnakes ppa if I remember correctly

#### Pillow: 

```shell
poetry run python3 -m PIL --report
--------------------------------------------------------------------
Pillow 11.1.0
Python 3.12.9 (main, Feb  5 2025, 08:49:00) [GCC 11.4.0]
--------------------------------------------------------------------
Python executable is [...]/.venv/bin/python3
Environment Python files loaded from [...]/.venv
System Python files loaded from /usr
--------------------------------------------------------------------
Python Pillow modules loaded from [...]/.venv/lib/python3.12/site-packages/PIL
Binary Pillow modules loaded from [...]/.venv/lib/python3.12/site-packages/PIL
--------------------------------------------------------------------
--- PIL CORE support ok, compiled for 11.1.0
*** TKINTER support not installed
--- FREETYPE2 support ok, loaded 2.13.2
--- LITTLECMS2 support ok, loaded 2.16
--- WEBP support ok, loaded 1.5.0
--- JPEG support ok, compiled for libjpeg-turbo 3.1.0
--- OPENJPEG (JPEG2000) support ok, loaded 2.5.3
--- ZLIB (PNG/ZIP) support ok, loaded 1.2.11, compiled for zlib-ng 2.2.2
--- LIBTIFF support ok, loaded 4.6.0
--- RAQM (Bidirectional Text) support ok, loaded 0.10.1, fribidi 1.0.8, harfbuzz 10.1.0
*** LIBIMAGEQUANT (Quantization method) support not installed
--- XCB (X protocol) support ok
--------------------------------------------------------------------
```



Hints:
To explain slightly, you can see in `frombuffer()` that `readonly` is set to 1.

https://github.com/python-pillow/Pillow/blob/af09976a5f57e8aebaedf332bc4025ea1b068af4/src/PIL/Image.py#L3186

And when `_ensure_mutable` sees this,

https://github.com/python-pillow/Pillow/blob/af09976a5f57e8aebaedf332bc4025ea1b068af4/src/PIL/Image.py#L631-L640

`self.im = self.im.copy()` means 'clone the copy of the image that lives in C, and use the clone from now on'.

You may or may not consider this helpful - if you save a copy of the image instead, replacing `im.save("./test_image.bmp")` with `im.copy().save("./test_image.bmp")`, then the original image stays the same.

If you're looking for context as to why `save()` calls `_ensure_mutable`, see #3724.

You may also find #2973 and #2980 interesting.
Now I understand the context better. Ok, so can you tell my why there is still `readonly = 1` flag set in `frombuffer()` constructor? I can see that in original https://github.com/python-pillow/Pillow/pull/2980 PR this line was a subject of change:

https://github.com/python-pillow/Pillow/pull/2980/files#diff-4805c79264fea07df59058db82ed74bb2f5c5023e212ac678536a534c56e5be2R3185

This line was not touched since the original fork - this is not necessarily something bad - but maybe it can be revised, why `frombuffer()` constructor would want to have this set?

I can see that when no readonly flag is set, the `.load()` method is called - and it does some mem allocation (based on docs). Despite it being unclear to me (though I believe that there is a good reason for calling `.load()` here); I would argue that opening API for controlling this flag can be rewarding:

```python
def frombuffer(
    mode: str,
    size: tuple[int, int],
    data: bytes | SupportsArrayInterface,
    decoder_name: str = "raw",
    readonly: bool = True,
    *args: Any,
) -> Image:
    # ...
            # ...
            im.readonly = readonly
            return im

```
Then user controls it:
```python
im: Image.Image = Image.frombuffer(mode="L", size=(1, 1), data=im_data, readonly=False)
```

HOWEVER I do not know if this is a good solution at all. I can only confirm that in *my* specific case this solves the issue - I can freely call `.save()` and still have data reflected from my buffer.

I did not put any performance checks in this considerations also. 

It just *makes more sense* from the user point of view - that's all.


Ok, so #2980 would resolve your issue. If @wiredfool is available, he might be better placed to answer your question.
Apparently I've totally forgotten about that PR, since I've effectively implemented the majority of it again in the Arrow PR #8330 (at least the parts where we've got an internal read_only flag on the c image struct, but in this case it's enforced read only in the c layer where you can't change it to r/w).

I don't recall why #2980 never went anywhere, other than what I see in the PR there now, in that there are no docs or tests.  (actually, that was a couple weeks before a new job, so that was probably it)

Looking at this in the context of the Arrow PR, I think that at least one significant use of Pillow in that context will be to take a large array and save the data to an image format. It definitely defeats the point of zero copy if we can't save data without copying it into our own storage first, and then shuffling it out to the file.

The core issue with #3274 seems to be that we've opened a write connection to a file that's mmaped into our process, not that save is inherently something that requires mutability in the image data. I'd think that this should be solved at the `open()` stage, possibly with an exclusive flag or similar. 

As a somewhat side note -- once an arrow array is exported from a process, according to the arrow implementation guidelines, both ends are supposed to consider it read-only due to the likelihood of inconsistent data when there are writers and readers in separate threads. I assume similar issues could occur with the frombuffer call, though given the GIL and the python focus of the array interface, it's less likely than with arrow.



Would https://github.com/radarhere/Pillow/commit/600411a1e60274d154d015b446e521ad225f8498 be sufficient to resolve this? I've tested it, and it causes your original code to work.

Patch:
diff --git a/src/PIL/Image.py b/src/PIL/Image.py
--- a/src/PIL/Image.py
+++ b/src/PIL/Image.py
@@ -2540,8 +2540,13 @@ def save(
                 msg = f"unknown file extension: {ext}"
                 raise ValueError(msg) from e
 
+        from . import ImageFile
+
         # may mutate self!
-        self._ensure_mutable()
+        if isinstance(self, ImageFile.ImageFile) and filename == self.filename:
+            self._ensure_mutable()
+        else:
+            self.load()
 
         save_all = params.pop("save_all", None)
         self.encoderinfo = {**getattr(self, "encoderinfo", {}), **params}


Test patch:
diff --git a/Tests/test_image.py b/Tests/test_image.py
--- a/Tests/test_image.py
+++ b/Tests/test_image.py
@@ -258,6 +258,15 @@ def test_readonly_save(self, tmp_path: Path) -> None:
             assert im.readonly
             im.save(temp_file)
 
+    def test_save_without_changing_readonly(self, tmp_path: Path) -> None:
+        temp_file = tmp_path / "temp.bmp"
+
+        with Image.open("Tests/images/rgb32bf-rgba.bmp") as im:
+            assert im.readonly
+
+            im.save(temp_file)
+            assert im.readonly
+
     def test_dump(self, tmp_path: Path) -> None:
         im = Image.new("L", (10, 10))
         im._dump(str(tmp_path / "temp_L.ppm"))