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

Fix thumbnail geometry when DCT scaling is used #4231

Merged
merged 16 commits into from
Dec 25, 2019
Merged
6 changes: 5 additions & 1 deletion Tests/test_image_draft.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ def draft_roundtrip(self, in_mode, in_size, req_mode, req_size):
im = Image.new(in_mode, in_size)
data = tostring(im, "JPEG")
im = fromstring(data)
im.draft(req_mode, req_size)
mode, box = im.draft(req_mode, req_size)
scale, _ = im.decoderconfig
self.assertEqual(box[:2], (0, 0))
self.assertTrue((im.width - scale) < box[2] <= im.width)
self.assertTrue((im.height - scale) < box[3] <= im.height)
return im

def test_size(self):
Expand Down
13 changes: 12 additions & 1 deletion Tests/test_image_thumbnail.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from PIL import Image

from .helper import PillowTestCase, hopper
from .helper import PillowTestCase, fromstring, hopper, tostring


class TestImageThumbnail(PillowTestCase):
Expand Down Expand Up @@ -47,3 +47,14 @@ def test_no_resize(self):
with Image.open("Tests/images/hopper.jpg") as im:
im.thumbnail((64, 64))
self.assert_image(im, im.mode, (64, 64))

def test_DCT_scaling_edges(self):
# Make an image with red borders and size (N * 8) + 1 to cross DCT grid
im = Image.new("RGB", (257, 257), "red")
im.paste(Image.new("RGB", (255, 255)), (1, 1))

thumb = fromstring(tostring(im, "JPEG", quality=99, subsampling=0))
thumb.thumbnail((32, 32), Image.BICUBIC)

ref = im.resize((32, 32), Image.BICUBIC)
self.assert_image_similar(thumb, ref, 2)
14 changes: 14 additions & 0 deletions docs/releasenotes/7.0.0.rst
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,20 @@ Setting the size of TIFF images
Setting the size of a TIFF image directly (eg. ``im.size = (256, 256)``) throws
an error. Use ``Image.resize`` instead.

Image.draft() return value
Copy link
Member Author

Choose a reason for hiding this comment

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

@radarhere Does this description look good for you?

Copy link
Member

Choose a reason for hiding this comment

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

I've created uploadcare#60 with my suggestion.

^^^^^^^^^^^^^^^^^^^^^^^^^^

The ``Image.draft()`` method used to return ``None`` or the image itself.
Unlike other `chain methods`_, ``draft()`` modifies the image in-place
rather than returning a modified version.

In the new version, ``draft()`` returns ``None`` if it has no effect or
a tuple of new image mode and the box of original coordinates in the
bounds of resulting image otherwise
(the box could be useful in subsequent ``resize()`` call).

.. _chain methods: https://en.wikipedia.org/wiki/Method_chaining


API Changes
===========
Expand Down
14 changes: 10 additions & 4 deletions src/PIL/Image.py
Original file line number Diff line number Diff line change
Expand Up @@ -1126,12 +1126,15 @@ def draft(self, mode, size):
"""
Configures the image file loader so it returns a version of the
image that as closely as possible matches the given mode and
size. For example, you can use this method to convert a color
size. For example, you can use this method to convert a color
JPEG to greyscale while loading it, or to extract a 128x192
version from a PCD file.

If any changes are made, returns a tuple with the chosen ``mode`` and
``box`` with coordinates of the original image within the altered one.

Note that this method modifies the :py:class:`~PIL.Image.Image` object
in place. If the image has already been loaded, this method has no
in place. If the image has already been loaded, this method has no
effect.

Note: This method is not implemented for most images. It is
Expand Down Expand Up @@ -2143,14 +2146,17 @@ def thumbnail(self, size, resample=BICUBIC):
x = int(max(x * size[1] / y, 1))
y = int(size[1])
size = x, y
box = None

if size == self.size:
return

self.draft(None, size)
res = self.draft(None, size)
if res is not None:
box = res[1]

if self.size != size:
im = self.resize(size, resample)
im = self.resize(size, resample, box=box)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
im = self.resize(size, resample, box=box)
im = self.resize(size, resample, box)

I suggest not specifying the argument name - it's not necessary, and the variable name already clearly indicates the purpose.

Copy link
Member Author

Choose a reason for hiding this comment

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

In truth, I think this is a case where we could use Keyword-Only Arguments.

It seems obvious that we can omit the argument name when you watch on it and so you know that it exists and what exactly it is doing. But once you omit it, you can't know exactly where your variable goes.


self.im = im.im
self._size = size
Expand Down
5 changes: 0 additions & 5 deletions src/PIL/ImageFile.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,6 @@ def __init__(self, fp=None, filename=None):
self.fp.close()
raise

def draft(self, mode, size):
"""Set draft mode"""

pass

def get_format_mimetype(self):
if self.custom_mimetype:
return self.custom_mimetype
Expand Down
6 changes: 4 additions & 2 deletions src/PIL/JpegImagePlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,8 @@ def draft(self, mode, size):
return

d, e, o, a = self.tile[0]
scale = 0
scale = 1
Copy link
Member Author

Choose a reason for hiding this comment

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

Both values are not handled: https://github.com/python-pillow/Pillow/blob/master/src/libImaging/JpegDecode.c#L238

While 1 is more correct.

original_size = self.size

if a[0] == "RGB" and mode in ["L", "YCbCr"]:
self.mode = mode
Expand All @@ -432,7 +433,8 @@ def draft(self, mode, size):
self.tile = [(d, e, o, a)]
self.decoderconfig = (scale, 0)

return self
Copy link
Member Author

Choose a reason for hiding this comment

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

This is breaking change. Previously, draft() could return None or self. This is very strange behavior, I don't think anyone actually used it. Now draft() returns the results of its work or None.

I'll add this in docs.

box = (0, 0, original_size[0] / float(scale), original_size[1] / float(scale))
return (self.mode, box)

def load_djpeg(self):

Expand Down