Skip to content

refactor: update image resizing implementation to use OpenCV and add … #268

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

Closed
wants to merge 3 commits into from

Conversation

r-bit-rry
Copy link

closing this issue:
#253

removing scipy dependency
chaging resize_image from scipy.ndimage.zoom to opencv resize
adding unittest to avoid regression

@maxlund
Copy link

maxlund commented Mar 23, 2025

This might be worth taking a look at
https://zuru.tech/blog/the-dangers-behind-image-resizing

image

@Blaizzy
Copy link
Owner

Blaizzy commented Mar 24, 2025

@r-bit-rry could you share some benchmark results with the old and new method?

@Blaizzy
Copy link
Owner

Blaizzy commented Mar 24, 2025

Also, why can't we use PIL for this?

@maxlund
Copy link

maxlund commented Mar 25, 2025

Also, why can't we use PIL for this?

From the link I posted above, PIL seems to be the best choice

@r-bit-rry
Copy link
Author

r-bit-rry commented Mar 25, 2025

I guess it doesn't really matter.
I chose opencv for the speed (at least the documented one)
I don't even know if the scipy method zoom that was used before was equivalent to the quality by PIL

One last note, the article is from 4 years ago. it might be worth retesting what he's done, or just take PIL whatever takes less time

@r-bit-rry
Copy link
Author

r-bit-rry commented Mar 25, 2025

@r-bit-rry could you share some benchmark results with the old and new method?

checked for regression and added a test, not for benchmark.
benchmarking can be seen here but obviously it was not made for this specific usecase:
https://github.com/libvips/libvips/wiki/Speed-and-memory-use

generally speaking I'm not sure that now is the time to optimize performance, especially when the bottleneck is most likely the mlx model itself.

looking over the updated benchmark it looks like PIL is the proper choice if using latest versions

@Blaizzy
Copy link
Owner

Blaizzy commented Mar 27, 2025

Awesome!

Lets migrate to PIL then, it's already widely used in the project.

Could you update the PR @r-bit-rry?

@Blaizzy
Copy link
Owner

Blaizzy commented Apr 17, 2025

Hey @r-bit-rry

I went ahead and tested this and it works, thank you very much!

However, there are more places where the change was needed and after testing different resampling techniques, I found that LANCZOS4 performs best overall with CV2 compared to CUBIC, INTER_AREA and others. It captures more details on images. #301

And for PIL bilinear was the the best.

However, they don't numerically match scipy.ndimage.zoom outputs.

PIL (Bilinear) Mean pixel difference between methods: 2.64
CV Mean pixel difference between methods: 49.28
Allclose with rtol and atol (1e-05): False

Here is an open issue that we can work towards closing that gap: #302

@Blaizzy Blaizzy closed this Apr 17, 2025
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