Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delegate Image mode and size to ImagingCore #7271

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/PIL/EpsImagePlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ def check_required_header_comments():

check_required_header_comments()

if not self._size:
if not self.size:
msg = "cannot determine EPS bounding box"
raise OSError(msg)

Expand Down
4 changes: 2 additions & 2 deletions src/PIL/GifImagePlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ def _seek(self, frame, update_image=True):
x1, y1 = x0 + i16(s, 4), y0 + i16(s, 6)
if (x1 > self.size[0] or y1 > self.size[1]) and update_image:
self._size = max(x1, self.size[0]), max(y1, self.size[1])
Image._decompression_bomb_check(self._size)
Image._decompression_bomb_check(self.size)
frame_dispose_extent = x0, y0, x1, y1
flags = s[8]

Expand Down Expand Up @@ -328,8 +328,8 @@ def _seek(self, frame, update_image=True):
self._mode = "RGBA"
del self.info["transparency"]
else:
self._mode = "RGB"
self.im = self.im.convert("RGB", Image.Dither.FLOYDSTEINBERG)
self._mode = "RGB"

def _rgb(color):
if self._frame_palette:
Expand Down
13 changes: 6 additions & 7 deletions src/PIL/IcnsImagePlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -261,11 +261,7 @@ def _open(self):
self.best_size[1] * self.best_size[2],
)

@property
def size(self):
return self._size

@size.setter
@Image.Image.size.setter
def size(self, value):
info_size = value
if info_size not in self.info["sizes"] and len(info_size) == 2:
Expand All @@ -283,7 +279,10 @@ def size(self, value):
if info_size not in self.info["sizes"]:
msg = "This is not one of the allowed sizes of this image"
raise ValueError(msg)
self._size = value
if value != self.size:
self.im = None
self.pyaccess = None
self._size = value

def load(self):
if len(self.size) == 3:
Expand All @@ -306,7 +305,7 @@ def load(self):

self.im = im.im
self._mode = im.mode
self.size = im.size
self._size = im.size

return px

Expand Down
20 changes: 10 additions & 10 deletions src/PIL/IcoImagePlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -310,36 +310,36 @@ def _open(self):
self.size = self.ico.entry[0]["dim"]
self.load()

@property
def size(self):
return self._size

@size.setter
@Image.Image.size.setter
def size(self, value):
if value not in self.info["sizes"]:
msg = "This is not one of the allowed sizes of this image"
raise ValueError(msg)
self._size = value
if value != self.size:
self.im = None
self.pyaccess = None
self._size = value

def load(self):
if self.im is not None and self.im.size == self.size:
# Already loaded
return Image.Image.load(self)
im = self.ico.getimage(self.size)
size_to_load = self.size
im = self.ico.getimage(size_to_load)
# if tile is PNG, it won't really be loaded yet
im.load()
self.im = im.im
self.pyaccess = None
self._mode = im.mode
if im.size != self.size:
if im.size != size_to_load:
warnings.warn("Image was not the expected size")

index = self.ico.getentryindex(self.size)
index = self.ico.getentryindex(size_to_load)
sizes = list(self.info["sizes"])
sizes[index] = im.size
self.info["sizes"] = set(sizes)

self.size = im.size
self._size = im.size

def load_seek(self):
# Flag the ImageFile.Parser so that it
Expand Down
38 changes: 33 additions & 5 deletions src/PIL/Image.py
Original file line number Diff line number Diff line change
Expand Up @@ -480,16 +480,24 @@ class Image:

def __init__(self):
# FIXME: take "new" parameters / other image?
# FIXME: turn mode and size into delegating properties?
self.im = None
self._mode = ""
self._size = (0, 0)
# do not directly change __mode; use _mode instead
self.__mode = ""
# do not directly change __size; use _size instead
self.__size = (0, 0)
self.palette = None
self.info = {}
self.readonly = 0
self.pyaccess = None
self._exif = None

def _use_im_values(self):
"""
Whether or not to try using values from self.im
in addition to the values in this class.
"""
return self.im is not None

@property
def width(self):
return self.size[0]
Expand All @@ -500,11 +508,31 @@ def height(self):

@property
def size(self):
return self._size
if self._use_im_values():
return self.im.size
return self.__size
Comment on lines +511 to +513
Copy link
Member

Choose a reason for hiding this comment

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

Why this can't be just in size property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

It would save three lines

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


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)

@property
def mode(self):
return self._mode
if self._use_im_values():
return self.im.mode
return self.__mode

def _mode(self, value):
# set im.mode first in case it raises an exception
if self._use_im_values():
self.im.mode = value
self.__mode = value

_mode = property(fset=_mode)

def _new(self, im):
new = Image()
Expand Down
3 changes: 3 additions & 0 deletions src/PIL/ImageFile.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

What is purpose of self.tile check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.


def __setstate__(self, state):
self.tile = []
super().__setstate__(state)
Expand Down
2 changes: 1 addition & 1 deletion src/PIL/ImageOps.py
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ def exif_transpose(image, *, in_place=False):
if in_place:
image.im = transposed_image.im
image.pyaccess = None
image._size = transposed_image._size
image._size = transposed_image.size
exif_image = image if in_place else transposed_image

exif = exif_image.getexif()
Expand Down
2 changes: 1 addition & 1 deletion src/PIL/PcxImagePlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ def _open(self):
# Don't trust the passed in stride.
# Calculate the approximate position for ourselves.
# CVE-2020-35653
stride = (self._size[0] * bits + 7) // 8
stride = (self.size[0] * bits + 7) // 8

# While the specification states that this must be even,
# not all images follow this
Expand Down
2 changes: 1 addition & 1 deletion src/PIL/QoiImagePlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def _open(self):
self._mode = "RGB" if channels == 3 else "RGBA"

self.fp.seek(1, os.SEEK_CUR) # colorspace
self.tile = [("qoi", (0, 0) + self._size, self.fp.tell(), None)]
self.tile = [("qoi", (0, 0) + self.size, self.fp.tell(), None)]


class QoiDecoder(ImageFile.PyDecoder):
Expand Down
45 changes: 42 additions & 3 deletions src/_imaging.c
Original file line number Diff line number Diff line change
Expand Up @@ -3646,11 +3646,49 @@
return PyUnicode_FromString(self->image->mode);
}

static int
_setattr_mode(ImagingObject *self, PyObject *value, void *closure) {
if (value == NULL) {
self->image->mode[0] = '\0';
return 0;

Check warning on line 3653 in src/_imaging.c

View check run for this annotation

Codecov / codecov/patch

src/_imaging.c#L3652-L3653

Added lines #L3652 - L3653 were not covered by tests
}

const char *mode = PyUnicode_AsUTF8(value);
if (mode == NULL) {
return -1;

Check warning on line 3658 in src/_imaging.c

View check run for this annotation

Codecov / codecov/patch

src/_imaging.c#L3658

Added line #L3658 was not covered by tests
}
if (strlen(mode) >= IMAGING_MODE_LENGTH) {
PyErr_SetString(PyExc_ValueError, "given mode name is too long");
return -1;

Check warning on line 3662 in src/_imaging.c

View check run for this annotation

Codecov / codecov/patch

src/_imaging.c#L3661-L3662

Added lines #L3661 - L3662 were not covered by tests
}

strcpy(self->image->mode, mode);
return 0;
}

static PyObject *
_getattr_size(ImagingObject *self, void *closure) {
return Py_BuildValue("ii", self->image->xsize, self->image->ysize);
}

static int
_setattr_size(ImagingObject *self, PyObject *value, void *closure) {
if (value == NULL) {
self->image->xsize = 0;
self->image->ysize = 0;
return 0;

Check warning on line 3679 in src/_imaging.c

View check run for this annotation

Codecov / codecov/patch

src/_imaging.c#L3677-L3679

Added lines #L3677 - L3679 were not covered by tests
}

int xsize, ysize;
if (!PyArg_ParseTuple(value, "ii", &xsize, &ysize)) {
return -1;

Check warning on line 3684 in src/_imaging.c

View check run for this annotation

Codecov / codecov/patch

src/_imaging.c#L3684

Added line #L3684 was not covered by tests
}

self->image->xsize = xsize;
self->image->ysize = ysize;
Comment on lines +3687 to +3688
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return 0;
}

static PyObject *
_getattr_bands(ImagingObject *self, void *closure) {
return PyLong_FromLong(self->image->bands);
Expand Down Expand Up @@ -3679,13 +3717,14 @@
};

static struct PyGetSetDef getsetters[] = {
{"mode", (getter)_getattr_mode},
{"size", (getter)_getattr_size},
{"mode", (getter)_getattr_mode, (setter)_setattr_mode},
{"size", (getter)_getattr_size, (setter)_setattr_size},
{"bands", (getter)_getattr_bands},
{"id", (getter)_getattr_id},
{"ptr", (getter)_getattr_ptr},
{"unsafe_ptrs", (getter)_getattr_unsafe_ptrs},
{NULL}};
{NULL}
};

/* basic sequence semantics */

Expand Down
Loading