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

API/resize: fix failure on greyscale images #97

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

dfdan
Copy link
Member

@dfdan dfdan commented Oct 19, 2023

❤️ Thank you for your contribution!

Description

As reported by @fenekku - resize operations were failing on greyscale images. This was caused by the method specified in IIIIF_RESIZE_RESAMPLE being applied to all image regardles of mode.

This setting appears to have been added to force Pillow to perform (higher quality) bicubic resampling on colour images.
Since Pillow 7.0, bicubic is used by default on colour images - thus we can simply bump the Pillow version requirement and remove the override / allow Pillow to select the most appropriate method.

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

Third-party code

If you've added third-party code (copy/pasted or new dependencies), please reach out to an architect.

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

  1. You license your contribution under the same terms as the current repository’s license.
  2. You agree that you have the right to license your contribution under the current repository’s license.

dfdan added 2 commits October 19, 2023 12:12
Bump Pillow to >=7.0 - now uses bicubic resize on colour images.
Remove code that forced this on all images as this caused greyscale images to fail resizing.
@dfdan dfdan requested a review from fenekku October 19, 2023 12:20
Copy link

@fenekku fenekku left a comment

Choose a reason for hiding this comment

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

Let's add a minor version bump since this does update some dependencies + deprecates some settings.

Thanks!

Comment on lines +204 to +205
if resample:
arguments["resample"] = resample
Copy link

Choose a reason for hiding this comment

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

This is not needed given the line above, right? arguments will already have a "resample" key set to the value passed as argument to the resize function now that the resample assignment from the setting has been removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, meant to take it out from the line above - I don't wnt to pass "resample=None" to resize.

Copy link

Choose a reason for hiding this comment

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

Ah I see so something like:

Suggested change
if resample:
arguments["resample"] = resample
if resample is None:
del arguments["resample"]

to remove resample completely.

Copy link

Choose a reason for hiding this comment

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

or better yet:

arguments = {...}  # without resample
if resample is not None:
    arguments["resample"] = resample

@fenekku
Copy link

fenekku commented Oct 19, 2023

I can't merge on this repo. Maybe someone else with merging powers?

@slint slint merged commit a02e34c into inveniosoftware:master Oct 26, 2023
3 checks passed
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.

3 participants