Remove unused ImagingConvertInPlace - #9878
Conversation
akx
left a comment
akx
left a comment
There was a problem hiding this comment.
I had been looking at removing ImagingConvertInPlace (in a bid to see if the shuffler functions could be implemented in a faster way) but didn't dare go through with it.
Some thoughts within though :)
| static PyObject * | ||
| im_setmode(ImagingObject *self, PyObject *args) { | ||
| im_setalpha(ImagingObject *self, PyObject *args) { | ||
| /* attempt to modify the mode of an image in place */ |
There was a problem hiding this comment.
This comment is a little stale now?
There was a problem hiding this comment.
Is it? The method is still modifying the mode of an image in place. It's more specific now, is all.
There was a problem hiding this comment.
I suppose...
Should this function know how to do P-to-PA and L-to-LA too?
There was a problem hiding this comment.
The thought occurred to me. It is a bit different to RGB, since RGB already has the same pixelsize as RGBA.
Do you mind if that is a follow-up PR? I would rather tidy things up first.
| {"setmode", (PyCFunction)im_setmode, METH_VARARGS}, | ||
| {"setalpha", (PyCFunction)im_setalpha, METH_NOARGS}, |
There was a problem hiding this comment.
What's the deprecation policy for methods on the core image objects in general (is it documented)?
GitHub's Code Search isn't making it easy to find whether there are external users of im.im.setmode.
There was a problem hiding this comment.
There is no deprecation period.
Pillow makes a commitment to stable public interfaces, which are defined at the Python layer. The C interfaces are explicitly internal, and no effort is made to keep them stable even between minor releases, and no support or warning is given when they change.
There was a problem hiding this comment.
That's an useful comment, thanks! Should maybe codify it into development documentation if it isn't already (for instance, I feel much better about touching the palette code even harder in #9829).
And I'll understand this as "the Python-facing interfaces defined in C extension module(s) in Pillow are also malleable" (since this is exactly that). 👍
| im_setalpha(ImagingObject *self, PyObject *args) { | ||
| /* attempt to modify the mode of an image in place */ | ||
| Imaging im = self->image; | ||
| if (im->mode == IMAGING_MODE_RGB || im->mode == IMAGING_MODE_RGBX) { |
There was a problem hiding this comment.
Should this function raise a mode error instead of quietly doing nothing?
There was a problem hiding this comment.
I've pushed a commit for this.
2 participants
im.putalpha()is the only place where C'ssetmode()is called.Pillow/src/PIL/Image.py
Lines 2050 to 2052 in 3220aca
This is only trying to convert the image to LA, PA or RGBA.
However, the only operation
setmode()can actually succeed at is converting RGB or RGBX to RGBA.Pillow/src/_imaging.c
Lines 2079 to 2090 in 3220aca
Pillow/src/libImaging/Convert.c
Lines 1739 to 1748 in 3220aca
ImagingConvertInPlace()isn't able to convert anything to LA, PA or RGBA.So this PR
ImagingConvertInPlace().setmode()is the only place whereImagingConvertInPlace()is called.setmode()to more clearly only convert RGB or RGBX images to RGBA. I've also renamed it tosetalpha().