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: add alpha premultiplication steps before and after resize #302

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

KaarlisCaune
Copy link
Contributor

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):

edges

You can test it with this source image - resize it via the tag and view it on a light background:

source

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.

@rbuchberger
Copy link
Owner

rbuchberger commented Feb 2, 2024

Thanks for the bug report and the PR! Unfortunately it looks like it breaks the tests, so it'll need a little more investigation.

 .......................................................................................................................................................................F......................

Finished in 4.001440s, 47.4829 runs/s, 82.7202 assertions/s.

  1) Failure:
TestImageFile#test_resize [test/unit/images/test_image_file.rb:129]:
Expected: 10
  Actual: 100

190 runs, 331 assertions, 1 failures, 0 errors, 0 skips

I'll have to do some learning about this issue and see what I can come up with.

@KaarlisCaune
Copy link
Contributor Author

KaarlisCaune commented Feb 6, 2024

We tried this change in production via a monkey-patch and it caused bugs when resizing images that don't have an alpha channel.
For context:

  • libvips assumes the last band of an image to be the alpha channel when running premultiply
  • If the image is RGBA, it's the last one - alpha, and everything's alright
  • If the image is RGB, the blue channel will be used instead, causing the resulting image to be basically yellow - green 😬

I updated the PR to only run the premultiplication / unpremultiplication steps when applicable.
Not sure if the failing resizing test is related to this bug as well.

@rbuchberger
Copy link
Owner

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).

@KaarlisCaune
Copy link
Contributor Author

KaarlisCaune commented Feb 6, 2024

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.
Can I ask for a sanity check of this code?

I can try adding a test case with alpha transparency 👍

@rbuchberger
Copy link
Owner

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. require 'pry'; binding.pry will get you a debug console on that particular line where you can run ruby commands.

@KaarlisCaune
Copy link
Contributor Author

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).

Copy link
Owner

@rbuchberger rbuchberger left a 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.

@rbuchberger rbuchberger merged commit f393067 into rbuchberger:master Feb 19, 2024
1 check passed
@rbuchberger
Copy link
Owner

I'll get a release out soon, but until then you can pull from the master branch to get them immediately.

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.

2 participants