Delegate Image mode and size to ImagingCore #7271

Yay295 · 2023-07-09T07:57:33Z

This causes a lot of errors. The main issues are that Image.im isn't always set, so we still need to keep Python-only variables for these properties, and the Python values are writable, but the C values are not writable from Python.

Yay295 · 2023-07-09T08:11:06Z

There is a setmode method on ImagingCore that could perhaps be used as the setter for ImagingCore.mode. I'm not sure why it wasn't, so I didn't make that change.

src/PIL/Image.py

radarhere · 2023-07-09T11:58:53Z

Could you provide an explanation for why this change is helpful?

Yay295 · 2023-07-09T15:43:13Z

I think keeping the mode and size of Image and ImagingCore in sync is a good thing, and delegating to ImagingCore when possible keeps them in sync.

radarhere · 2023-08-02T13:36:59Z

As this PR knows, because self.im starts out as None, and stays that way until an image is loaded, mode and size can't always be delegated to C, and must exist sometimes in Python.

I think keeping the mode and size of Image and ImagingCore in sync is a good thing

I agree, and I would think that they are already in sync.

and delegating to ImagingCore when possible keeps them in sync.

While this PR isn't yet finalised, it would seem that the idea here is to make Python's _mode and the ImagingCore mode go out of sync. The public API's mode might stay correct, but I'm uncomfortable with the idea of Python's _mode sometimes disagreeing with the C mode. It would seem to be clearer code if they were both kept the same.

Yay295 · 2023-08-02T14:09:04Z

The original issue that I created this change for was discussed here: #7260 (comment)

The problem was that changing an image's mode in C does not change the mode seen in Python, because the C code doesn't have access to the Python value. So if an image's mode or size is ever changed directly in C, it will be out of sync with the value(s) in Python. Likewise, if one of these values is changed in Python, it will be out of sync with the value(s) in C (making Image.size and Image.mode read-only helps prevent this). By always using the C value if it exists we can be sure we are using the same value everywhere.

radarhere · 2023-08-03T04:09:47Z

The problem was that changing an image's mode in C does not change the mode seen in Python, because the C code doesn't have access to the Python value. So if an image's mode or size is ever changed directly in C, it will be out of sync with the value(s) in Python.

Pillow has a public Python API. Our intention is not for users to access the C API directly. If ImagingCore.rewrite_mode() is something that is added in the future, we can talk about that then, but for the moment, I'm not aware of any logic in C determines what the ImagingCore mode is - the logic to do so lives in Python.

Likewise, if one of these values is changed in Python, it will be out of sync with the value(s) in C (making Image.size and Image.mode read-only helps prevent this).

I think ignoring the Python mode would obscure the fact that in this scenario, setting the mode in Python is not having the intended effect. That doesn't sound helpful to me.

By always using the C value if it exists we can be sure we are using the same value everywhere.

It ensures that im.mode reflects the C mode, but you're also deliberately removing setting _mode in this PR, making the Python _mode sometimes incorrect. This is not part of the public API, so you may think it doesn't matter, but _mode is something that plugins still use, and so something that users writing plugins still need to understand. It seems to me like this makes the code less clear.

Yay295 · 2023-08-03T04:31:13Z

If ImagingCore.rewrite_mode() is something that is added in the future, we can talk about that then,

ImagingCore.rewrite_mode() has been proposed as part of #7260, which is why we are talking about this now.

I think ignoring the Python mode would obscure the fact that in this scenario, setting the mode in Python is not having the intended effect.

This is currently the case. Before #7307 (which is not yet part of a release), it was possible to set Image.mode. This would sometimes actually work if the C mode wasn't used, and sometimes it wouldn't if the C mode was used.

but you're also deliberately removing setting _mode in this PR

_mode did not exist before #7307. This change was made before that one, and I did add a setter for mode. Your changes are the ones that removed the setters.

radarhere · 2023-08-03T05:36:40Z

If you'd like to wait until #7260 is resolved, okay.

It ensures that im.mode reflects the C mode, but you're also deliberately removing setting _mode in this PR, making the Python _mode sometimes incorrect.

When I said this, I wasn't referring to setters for mode, I was referring to

Yay295 · 2023-08-04T00:39:40Z

Image.im is also set in all of those locations, so Image.mode would have gotten its data from Image.im.mode, making those lines redundant. I suppose Image.im could be set to None after that point though, so they wouldn't be redundant in that case.

…ilable When setting the values on Image also try to update the values on Image.im. There isn't currently a way to update values in the other direction.

Yay295 · 2023-08-05T20:05:12Z

New change based on main.

mode and size are read-only properties.
_mode and _size are read-write properties.
__mode and __size are the actual values.

If _mode or _size are changed, it will also attempt to set those values on Image.im.

and use double quotes instead of single quotes

homm · 2023-08-07T10:01:04Z

src/PIL/ImageFile.py

@@ -139,6 +139,9 @@ def get_format_mimetype(self):
        if self.format is not None:
            return Image.MIME.get(self.format.upper())

+    def _use_im_values(self):
+        return self.tile is None and self.im is not None


What is purpose of self.tile check?

@radarhere proposed that change for ImageFile in a pull request they made for the previous code. The issue is that when seeking to another frame, the Image values may not match the Image.im values. Using not self.tile like in their pull request wasn't working for me though, so I changed it to self.tile is None, which after looking into it more, probably isn't correct.

homm · 2023-08-07T10:17:31Z

src/_imaging.c

+    self->image->xsize = xsize;
+    self->image->ysize = ysize;


Why we want to do this? It will just break image

So that if Image.size is changed, Image.im.size will have the same value. Another option could be that if Image.size is changed, and Image.im.size doesn't match, it raises an exception.

homm · 2023-08-07T10:21:55Z

src/PIL/Image.py

+        if self._use_im_values():
+            return self.im.size
+        return self.__size


Why this can't be just in size property?

The _size getter is actually used in quite a few places, so that would have to be changed to use the size getter first. It would save three lines if the _size getter was removed though:

@property def size(self): if self._use_im_values(): return self.im.size return self.__size def _size(self, value): # set im.size first in case it raises an exception if self._use_im_values(): self.im.size = value self.__size = value _size = property(fset=_size)

It would save three lines

More important that this saves one indirect method call (width → size → _size) on each attribute getter.

radarhere · 2023-10-18T07:06:10Z

#7271 (comment)

ImagingCore.rewrite_mode() has been proposed as part of #7260, which is why we are talking about this now.

As far as I can see, Yay295#8 removed the need for ImagingCore.rewrite_mode() in that PR. Does that make this PR unnecessary?

I'm not in favour of this PR, for reasons stated earlier. @homm you've left some individual comments, but regarding the basic idea, do you see value here that I don't?

Part of the reason I mention this now is that I see pydicom/pydicom#1907 referencing this in trying to grapple with #7307, and that doesn't seem helpful. I know that this PR is unmerged and in draft mode, so it has all the indications that it is not ready for consumption, but nevertheless.

homm · 2023-10-18T11:18:34Z

regarding the basic idea, do you see value here that I don't?

I do see profit in wall-time and memory consumption, but I'm not sure about public interface. In general, I think we can close it for now.

Yay295 mentioned this pull request Jul 9, 2023

Add tp_richcompare handler for Imaging_Type/ImagingCore #7260

Open

homm reviewed Jul 9, 2023

View reviewed changes

src/PIL/Image.py Outdated Show resolved Hide resolved

This was referenced Jul 28, 2023

Changed Image mode property to be read-only by default #7307

Merged

Removed setters for read-only properties Yay295/Pillow#9

Closed

delegate Image.mode and Image.size to the values on Image.im when ava…

919dbbe

…ilable When setting the values on Image also try to update the values on Image.im. There isn't currently a way to update values in the other direction.

Yay295 force-pushed the image_mode_size_delegation branch from e311a46 to 919dbbe Compare August 5, 2023 20:02

split long comment across two lines

b879ada

and use double quotes instead of single quotes

Yay295 force-pushed the image_mode_size_delegation branch from def6e62 to b879ada Compare August 5, 2023 20:11

homm requested changes Aug 7, 2023

View reviewed changes

Yay295 and others added 3 commits August 7, 2023 10:35

fix typo in comment

0a88b5b

remove _size and _mode getters

555420b

Merge branch 'main' into image_mode_size_delegation

06aa04a

This was referenced Oct 15, 2023

Exception with Pillow 10.1+ Image.mode is read-only pydicom/pydicom#1907

Closed

[MRG] Fix Pillow raising AttributeError due to Image.mode being read-only pydicom/pydicom#1908

Merged

radarhere closed this Oct 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delegate Image mode and size to ImagingCore #7271

Delegate Image mode and size to ImagingCore #7271

Yay295 commented Jul 9, 2023

Yay295 commented Jul 9, 2023

radarhere commented Jul 9, 2023

Yay295 commented Jul 9, 2023

radarhere commented Aug 2, 2023

Yay295 commented Aug 2, 2023

radarhere commented Aug 3, 2023

Yay295 commented Aug 3, 2023 •

edited

Loading

radarhere commented Aug 3, 2023

Yay295 commented Aug 4, 2023

Yay295 commented Aug 5, 2023

homm Aug 7, 2023

Yay295 Aug 7, 2023

homm Aug 7, 2023

Yay295 Aug 7, 2023

homm Aug 7, 2023

Yay295 Aug 7, 2023

homm Aug 7, 2023

radarhere commented Oct 18, 2023 •

edited

Loading

homm commented Oct 18, 2023

Delegate Image mode and size to ImagingCore #7271

Delegate Image mode and size to ImagingCore #7271

Conversation

Yay295 commented Jul 9, 2023

Yay295 commented Jul 9, 2023

radarhere commented Jul 9, 2023

Yay295 commented Jul 9, 2023

radarhere commented Aug 2, 2023

Yay295 commented Aug 2, 2023

radarhere commented Aug 3, 2023

Yay295 commented Aug 3, 2023 • edited Loading

radarhere commented Aug 3, 2023

Yay295 commented Aug 4, 2023

Yay295 commented Aug 5, 2023

homm Aug 7, 2023

Choose a reason for hiding this comment

Yay295 Aug 7, 2023

Choose a reason for hiding this comment

homm Aug 7, 2023

Choose a reason for hiding this comment

Yay295 Aug 7, 2023

Choose a reason for hiding this comment

homm Aug 7, 2023

Choose a reason for hiding this comment

Yay295 Aug 7, 2023

Choose a reason for hiding this comment

homm Aug 7, 2023

Choose a reason for hiding this comment

radarhere commented Oct 18, 2023 • edited Loading

homm commented Oct 18, 2023

Yay295 commented Aug 3, 2023 •

edited

Loading

radarhere commented Oct 18, 2023 •

edited

Loading