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

Crop images within 3% of device aspect ratio #495

Merged
merged 6 commits into from
May 14, 2023
Merged

Conversation

axu2
Copy link
Collaborator

@axu2 axu2 commented Apr 1, 2023

Because filling the screen with double page spreads looks amazing on the new Kindle Scribe with its 4:3 aspect ratio. (even though the images are downscaled to 1920x1440 per

Also, replaces previous code that could potentially resize images twice, leading to quality loss, instead just using library functions to do it in one pass.

The problem becomes more serious the smaller the edge is.

Here is what the maximum crop looks like:

IMG_20230513_072645.jpg

IMG_20230513_072635.jpg

@axu2
Copy link
Collaborator Author

axu2 commented Apr 2, 2023

On further inspection I think this PR is big enough. I recommend squashing my commits.

@axu2 axu2 marked this pull request as ready for review April 2, 2023 15:40
@axu2 axu2 changed the title Crop images within 3.3% of device aspect ratio Crop images within 3% of device aspect ratio May 10, 2023
@darodi darodi added this to the 5.6.3 milestone May 13, 2023
@darodi
Copy link
Collaborator

darodi commented May 13, 2023

@axu2

forced pushed a rebase on master with squash and some refactoring

@axu2
Copy link
Collaborator Author

axu2 commented May 13, 2023

Ah, I meant squash the commits during merge, not in the PR itself. Anyways, just for convenience here are my test images:

fff (1)
fff

@axu2
Copy link
Collaborator Author

axu2 commented May 14, 2023

Since you already started refactoring, I may as well add the cbz case as well from #471

@axu2
Copy link
Collaborator Author

axu2 commented May 14, 2023

While we are at it, maybe we should remove the BICUBIC filter. Looks like in the past BICUBIC was better for upscaling but not anymore. This might be a separate PR/logical step though. I personally don't use upscale on the Kindle Scribe since the Scribe will fill the screen no matter what the original size was.

Filter Downscaling quality Upscaling quality Performance
Resampling.NEAREST     ⭐⭐⭐⭐⭐
Resampling.BOX   ⭐⭐⭐⭐
Resampling.BILINEAR ⭐⭐⭐
Resampling.HAMMING ⭐⭐   ⭐⭐⭐
Resampling.BICUBIC ⭐⭐⭐ ⭐⭐⭐ ⭐⭐
Resampling.LANCZOS ⭐⭐⭐⭐ ⭐⭐⭐⭐

https://pillow.readthedocs.io/en/stable/releasenotes/2.7.0.html
https://pillow.readthedocs.io/en/stable/handbook/concepts.html#filters-comparison-table

@darodi
Copy link
Collaborator

darodi commented May 14, 2023

Lanczos might produce slightly more ringing than bicubic.
We should try and check if the difference in quality is negligible and what is the impact on performance.
For a large image collection, if the processing time is multiplied, it changes a lot (see mozJpeg for example)

@axu2
Copy link
Collaborator Author

axu2 commented May 14, 2023

Yea definitely a separate PR then, if we decide to change. I personally think it's probably better to not change.

@darodi darodi merged commit 37200bd into ciromattia:master May 14, 2023
@axu2 axu2 deleted the crop branch May 14, 2023 17:27
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.

Add Page Fill Option
2 participants