-
Notifications
You must be signed in to change notification settings - Fork 105
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: add alpha premultiplication steps before and after resize #302
Fix: add alpha premultiplication steps before and after resize #302
Conversation
Thanks for the bug report and the PR! Unfortunately it looks like it breaks the tests, so it'll need a little more investigation.
I'll have to do some learning about this issue and see what I can come up with. |
We tried this change in production via a monkey-patch and it caused bugs when resizing images that don't have an alpha channel.
I updated the PR to only run the premultiplication / unpremultiplication steps when applicable. |
Thanks! Looks like that change fixed the test, likely because the test image doesn't have an alpha channel. That said, I'd like to add a test of an image which does have an alpha channel. Feel free to add it yourself, or I'll add one when I get the chance. If you do add a test image to the repo, please keep it small (I believe the existing images are 100px squares). |
I found an issue where the images with alpha channel would not get resized at all (files with proper widths in filenames would be generated, but all in the same original width). That would explain the failing test and why the tests started working again. I managed to fix it in my patch version by "chaining" the operations on image like this: def resize(image)
if image.has_alpha?
image = image.premultiply
image = image.resize(scale_value)
image.unpremultiply
else
image.resize(scale_value)
end
end I'm not well versed in ruby, but I suppose the issue was me assuming the image object will be mutated on each step, resulting in sequential actions. This version doesn't assume that and seems to finally work as expected on my project. I can try adding a test case with alpha transparency 👍 |
Heh, you've run into one of the reasons why I don't write much ruby anymore. Does the premultiply command modify the existing instance? Who knows! We have to run it and find out. |
Looks like it needs the assignment after each modification, otherwise it will only return the last one (thus skip the resize step). I managed to add a test case for images with alpha transparency (which fails if I remove the object assignment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Outstanding work, thank you.
I'll get a release out soon, but until then you can pull from the master branch to get them immediately. |
First of all, thank you for the great work!
I noticed an issue with resizing images that have an alpha channel. Since vips was introduced, white shapes on transparent background started having dark edges after resize. Project I'm working on contains many such images, and after the jekyll picture tag update most of them are displayed with unwanted dark borders.
Example (original source on left, resized image on right):
You can test it with this source image - resize it via the tag and view it on a light background:
The issue stems from premultiplication of the alpha channel. vips documentation advises to premultiply the alpha channel on images before resize, then resize, then unpremultiply alpha again - this PR does exactly that.
Disclaimer: I haven't checked the performance hit of this change, nor do I have the ruby chops to only run this when an alpha channel is actually present or similarly optimise it.