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 size calculation of Image.thumbnail() #4404

Merged
merged 7 commits into from Mar 5, 2020

Conversation

orlnub123
Copy link
Contributor

The current size calculation is fine if only one axis is smaller or if the image is wide, but it trips up when both axes are smaller and the image is tall. Here's an example:

from PIL import Image

im = Image.new("L", (145, 100))
im.thumbnail((50, 50))
assert im.size == (50, 34)  # This is fine, the image is wide.

im = Image.new("L", (100, 145))
im.thumbnail((100, 50))
assert im.size == (34, 50)  # Also fine, only one axis is smaller.

im = Image.new("L", (100, 145))
im.thumbnail((50, 50))
assert im.size == (34, 50)  # Oh no! It returned (35, 50)!

Someone made a pull request that fixed this in 2016, but it looks like he closed it shortly after due to a CI failure.

@python-pillow python-pillow deleted a comment from codecov bot Feb 5, 2020
@radarhere
Copy link
Member

I imagine that @homm, as the author of #4256, would like to review this?

Copy link
Member

@homm homm left a comment

Choose a reason for hiding this comment

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

Looks like this happened due to double rounding in both branches.

src/PIL/Image.py Show resolved Hide resolved
@orlnub123
Copy link
Contributor Author

These latest commits have a fix related to the issue previously discussed. Rounding up the size contradicts the documentation which states:

This method modifies the image to contain a thumbnail version of itself, no larger than the given size.

So now when you request a thumbnail with a maximum size of (99.9, 99.9), you get back an image with the size of (99, 99) instead of (100, 100).

As far as I can tell passing float values to size is deprecated, so that commit can be reverted when the switch is made.

@orlnub123 orlnub123 requested a review from homm February 9, 2020 01:32
@radarhere radarhere force-pushed the bugfix/thumbnail branch 2 times, most recently from 5f0d846 to 7d3c9b0 Compare February 16, 2020 10:22
@homm
Copy link
Member

homm commented Feb 20, 2020

I have an interesting question. Let's say we have an image 100×30 pixels and requested 75×75 thumbnail. The result will be 75×22.5 which will be rounded to 75×22. It seems that there should be no difference in which direction we're rounding, up or down, both options should be equal.

But the thing is, 100/30 = 3.3(3), 75/22 = 3.4(09) and 75/23 ≈ 3.261 and this is a bit closer to 100/30, but we still have an answer 75×22. Why?

@orlnub123
Copy link
Contributor Author

I'd like to point out that the current implementation had the same issue. It happens because the rounding is indiscriminate towards the aspect ratio. I've added a commit that fixes the issue, improvements are welcome.

src/PIL/Image.py Outdated Show resolved Hide resolved
@hugovk hugovk merged commit 3f9b615 into python-pillow:master Mar 5, 2020
@hugovk
Copy link
Member

hugovk commented Mar 5, 2020

Thanks!

This was referenced Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants