Premultiply alpha for transparent images to prevent dark edges #213

gasi · 2015-05-07T04:37:22Z

This change premultiplies the alpha channel into the RGB channels for RGBA images to prevent dark edges when transforming (scaling, blurring, etc.) images. I opened this against knife as it had significant changes to Composite as we want to avoid double premultiplication.

On a less serious note: I am happy to take superior naming suggestions for Unpremultiply, but it might be actually pretty standard: https://msdn.microsoft.com/en-us/library/windows/desktop/hh780397%28v=vs.85%29.aspx Oh, ’muhrica 🇺🇸

Supercedes #211.

I’ve done testing of this against ImageMagick with our own images output from Paper, but haven’t had a chance to do general regression testing with other kinds of inputs. This is a starting point and I’d be happy to polish it for inclusion in knife in the coming days. I placed the premultiplication logic before any image transformations and the unpremultiplication (sic!) before output. Please let me know if the placement can be optimized. The reference articles talk about having to watch out for zero alpha values (division by zero) and clamping, but I haven’t had a chance to explore that yet. I wonder if vips_divided, etc. take care of this internally, but maybe you know.

I did add the file-compare module as I needed really strict regression testing, although I realize it might fail due to different versions of vips creating the output. Happy to take suggestions for a better approach, although I think it needs to be closer to ImageMagick compare than the more loose perceptual hash. At some point, even with tolerance: 0 it couldn’t distinguish two reference images with similar shape but different colors. Added sharp.compare to compute mean squared error (though it doesn’t match the same numbers I get from compare -metric mse 😢). At least it lets us detect whether images are identical at a pixel level.

References:

/cc @julianwa

gasi · 2015-05-07T05:33:59Z

Argh, I see the file hashing based comparison doesn’t pass on CI. Will look into it on Friday.

lovell · 2015-05-07T09:30:50Z

Thanks Daniel, as always, for all your work on improving sharp.

To quote George Bernard Shaw, "England and America are two nations unpremultiplied by a common language."

I don't think Premultiply will have any effect on vips_shrink so it can occur after that, perhaps only when we know either an affine transform or composite will occur.

vips_divide guards against divide-by-zero.

As for the functional tests, perhaps you could use ...extract( ... ).raw().toBuffer( ... ) to inspect specific pixels, comparing the Euclidean distance between two RGB values (technically sRGB, but probably close enough) against a given threshold.

lovell · 2015-05-11T09:18:31Z

Thanks to John's sterling work, this improvement should now be able to take advantage of the new vips_premultiply and vips_unpremultiply operations available in libvips 8.1.0, wrapped in something like:

#if (VIPS_MAJOR_VERSION >= 9 || (VIPS_MAJOR_VERSION >= 8 && VIPS_MINOR_VERSION >= 1))
...
#endif

Are you able to make this update, which I guess should also apply to the Composite logic?

gasi · 2015-05-12T20:15:07Z

Now that the cat is out of the bag—we launched Think Kit—I will dedicate today to polishing up this addition to sharp. Thanks again for all your help, @lovell!

To quote George Bernard Shaw, "England and America are two nations unpremultiplied by a common language."

The team and I had a good laugh about that one 🇬🇧 🇺🇸 😜

I don't think Premultiply will have any effect on vips_shrink so it can occur after that, perhaps only when we know either an affine transform or composite will occur.

Is that because it doesn’t do any interpolation? Either way, I am happy to give it a shot and see what happens. Is your main motivation for avoiding premultiplication where possible for performance reasons? If so, what’s the best way to capture a condition that says we will require it, i.e. shouldPremultiplyImageAlpha? Is there a better way than the following?

bool shouldApplyAffineTransform = xresidual != 0.0 || yresidual != 0.0;
bool shouldComposite = !baton->overlayPath.empty();
// …
bool shouldPremultiplyImageAlpha = HasAlpha(image) && image->Bands == 4 &&
  (shouldApplyAffineTransform || shouldComposite /* || … */);`

vips_divide guards against divide-by-zero.

Thanks for confirming.

As for the functional tests, perhaps you could use ...extract( ... ).raw().toBuffer( ... ) to inspect specific pixels, comparing the Euclidean distance between two RGB values (technically sRGB, but probably close enough) against a given threshold.

Thanks, I’ll give this a shot. This sounds like *Magick’s compare with mean square error, right?

Are you able to make this update, which I guess should also apply to the Composite logic?

Yes, I can do that too. Thanks for the snippet with the preprocessing statement 👍

gasi · 2015-05-13T06:05:11Z

Status update: I made the changes you suggested in resize.cc. Please take a look. I haven’t had a chance yet to actually compile & test using VIPS 8.1.0 for the built-in vips_premultiply / vips_unpremultiply changes. Additionally, I had this hope I could build my own *Magick compare -metric mse command using VIPS and expose it as Sharp.compare, but I got stuck on the lack of examples for vips_stats and vips_getpoint. In my last commit, I was trying to compute the stats for the difference of two input images but I am only getting 0 for sum2, etc.: ca42ec7

Do you agree it’d be valuable to have a VIPS based Sharp.compare for fixtures.assertIdentical?
Do you know of any examples on how to use vips_stats and vips_getpoint?

gasi · 2015-05-13T06:06:15Z

src/compare.c

+  printf("xmin: %f\n", (double) stats->data[6]);
+  printf("ymin: %f\n", (double) stats->data[7]);
+  printf("xmax: %f\n", (double) stats->data[8]);
+  printf("ymax: %f\n", (double) stats->data[9]);


What’s the right way to read the values from vips_stats? If this is correct, why I am only getting 0 for sum2 when running it with two different images?

I didn’t cast stats to double * correctly.

gasi · 2015-05-13T22:47:30Z

@lovell Alright, this is ready for another review. Will squash the commits once you give me a 👍

coveralls · 2015-05-13T22:51:26Z

Coverage decreased (-55.54%) to 40.29% when pulling 8bb88a9 on gasi:premultiply-alpha into e553e92 on lovell:knife.

lovell · 2015-05-14T08:33:16Z

src/compare-internal.c

+
+  VipsImage *actualPremultiplied;
+  VipsImage *expectedPremultiplied;
+  if (Premultiply(context, actual, &actualPremultiplied) ||


Does the use of Premultiply here need wrapping in the shouldPremultiplyAlpha logic?

I made Premultiply a noop (strictly: a copy) if the image has no transparency. I found an example in John’s code.

lovell · 2015-05-14T09:10:38Z

Thanks again for all your work on this Daniel. I've added a few comments inline. Sorry I wasn't able to make time to review this yesterday.

gasi · 2015-05-15T08:46:09Z

Thanks again for all your work on this Daniel. I've added a few comments inline. Sorry I wasn't able to make time to review this yesterday.

Thanks for your feedback and no need to apologise. I pushed some more improvements, but I haven’t checked if that was all since we had a little party for our Think Kit launch.

A main thing I’d love your 👀 on is, if there are any obvious or not so obvious memory leaks. We are seeing some in production and it’s likely from my sharp changes but not 100% confirmed. I tried running your leak tests but frankly haven’t fully understood how to interpret the results. I’d appreciate your help with that 🇬🇧 ❤️ 🇺🇸 Thanks 😉

gasi · 2015-05-15T08:47:15Z

README.md

+
+* `err` contains an error message, if any.
+* `info` contains the info about the difference between the two images such as `isEqual` (Boolean), `meanSquaredError` (Number; present iff `status='success'`, otherwise `undefined`), and `status` (String; one of `success`, `mismatchedDimensions`, `mismatchedBands`, `mismatchedType`).
+


Added status to not report certain errors as fatal, e.g. mismatched dimensions, bands, etc.

gasi · 2015-05-15T08:50:59Z

test/unit/overlay.js

-// Main
+
+// Constants
+var MAX_ALLOWED_IMAGE_MAGICK_MEAN_SQUARED_ERROR = 0.3;


⬇️ Turns out sharp’s results are really close to ImageMagick, as one would expect 😉

Let me know if this fails again on other platforms or versions. We can adjust it upwards then.

lovell · 2015-05-15T15:36:09Z

Running npm test with this PR on Ubuntu 14.04 using libvips master and its new (un)premultiply methods generates test failures beyond the 0.0005 MSE threshold:

  1) Alpha transparency Enlargement with non-nearest neighbor interpolation shouldn't cause dark edges:
     Error: Expected images be equal. Mean squared error: 0.028674960136413574.
      at test/fixtures/index.js:151:25

  2) Alpha transparency Reduction with non-nearest neighbor interpolation shouldn't cause dark edges:
     Error: Expected images be equal. Mean squared error: 0.008033929392695427.
      at test/fixtures/index.js:151:25

  3) Overlays Composite two transparent PNGs into one:
     Error: Expected images be equal. Mean squared error: 0.08248437941074371.
      at test/fixtures/index.js:151:25

  4) Overlays Composite two low-alpha transparent PNGs into one:
     Error: Expected images be equal. Mean squared error: 0.011143956333398819.
      at test/fixtures/index.js:151:25

Here's output.alpha-premultiply-enlargement-2048x1536-paper.png generated by the first test, which doesn't appear (to my eyes) to suffer from the dark edges seen previously:

Rather than increase the threshold, perhaps either extracting a region known to suffer regression/problems or using smaller test images will help.

gasi · 2015-05-15T23:02:43Z

Thanks for running the test with the latest VIPS version. Agreed, it doesn’t show black fringing.

Rather than increase the threshold, perhaps either extracting a region known to suffer regression/problems or using smaller test images will help.

I thought about that, but wouldn’t we want to ensure output images have a low error against a visually verified reference image as a whole? We might not want to only catch black fringes but any kind of errors. In this case, we could allow an MSE of 0.1 which based on my understanding would be a 0.1 / 255 * 100 ~= 0.04% error for an RGB image. What do you think?

More importantly, it might be interesting to investigate what causes the discrepancy. I am sure John was much more finessed in his code for the native vips_*multiply and maybe there is something we can backport to sharp’s *multiply functions to minimize the error.

gasi · 2015-05-15T23:15:12Z

src/composite.c

    return -1;

+  vips_object_local(context, t2);
  vips_object_local(context, outRGBPremultiplied);


@lovell: Isn’t this another memory leak: When vips_multiply succeeds t2 is allocated, but then vips_add fails and the function returns without ever calling vips_object_local(context, t2)? I originally combined multiple operations into one—if (vips_* || vips_* || vips_*)—based on @jcupitt’s pattern using vips_object_local_array: https://gist.github.com/jcupitt/abacc012e2991f332e8b#file-composite2-c-L16-L22. However in his case, all t[…] seemed to be pre-hooked onto context right away. No more extra vips_object_local calls afterwards. Then we switched back to using individual variables for better readability but I kept the if (… || … || …) pattern. Your code uses individual variables but only single operations per if (…). Am I right in identifying this as a potential leak (in error cases) or did I miss something?

Yes, in the event of an error occurring within vips_add, t2 would never be unreferenced.

I still believe using explicit references is safer even when it means having to spread multiple operations over multiple conditionals, although I may be suffering from a bout of "once bitten, twice shy".

Thanks for confirming. I agree, I’d rather play it safe and will try to rewrite it with multiple conditionals. Can’t wait for #152 ❤️

BTW, in ’Murica 🇺🇸 they say ‘Fool me once…’: https://www.youtube.com/watch?v=eKgPY1adc0A 😜

libvips uses this style, as a possible alternative:

VipsImage **t = vips_object_local_array(..); VipsImage *in; in = ..->input; if( vips_thing(in, &t[0], NULL ) || vips_thing2(t[0], &t[1], NULL ) return -1; in = t[2]; if( vips_thing3(in, &t[2], NULL ) || vips_thing4(t[2], &t[3], NULL ) return -1; in = t[3]; if (vips_image_write(in, out)) return -1; return 0;

This has some nice properties:

you can use .. || .. to chain operations together, so it's concise

guaranteed leak-free

you only need to keep track of the annoying t[] variables with a .. || .. block -- the blocks are independent and joined with properly named variables

because blocks are independent, you can reorder them safely: you could swap these two over and it would all work

Thanks John, I'm hoping this is relatively short-lived as sharp's move to the new vips8 C++ bindings will remove all the reference counting, taking advantage of internal calls to vips_object_local_array :)

Converted all code to use individual error conditionals 👍

lovell · 2015-05-16T08:08:07Z

"I thought about that, but wouldn’t we want to ensure output images have a low error against a visually verified reference image as a whole? We might not want to only catch black fringes but any kind of errors."

Yes, how about we test the image as a whole using a "general" threshold and test the region known to cause problems at a specific, lower threshold?

lovell · 2015-05-16T08:12:24Z

"I am sure John was much more finessed in his code for the native vips_*multiply and maybe there is something we can backport to sharp’s *multiply functions to minimize the error."

I suspect we're probably looking at difference due to cumulative rounding errors. John's approach avoids the band splitting/joining and seems to be a bit faster as a result, although I've yet to benchmark this.

jcupitt · 2015-05-16T09:08:31Z

The vips test suite looks for (a - b).abs().max() < threshold, ie. maximum absolute difference, I don't know if that would work here. Testing average difference can miss things, and picking the right threshold is difficult.

Discussion: lovell#213 (comment)

Add `Sharp.compare(file1, file2, callback)` function for comparing images using mean squared error (MSE). This is useful for unit tests. See: - https://github.com/jcupitt/libvips/issues/291 - http://entropymine.com/imageworsener/resizealpha/

gasi · 2015-05-18T21:40:19Z

@lovell: Yes, how about we test the image as a whole using a "general" threshold and test the region known to cause problems at a specific, lower threshold?
@jcupitt: The vips test suite looks for (a - b).abs().max() < threshold, ie. maximum absolute difference, I don't know if that would work here. Testing average difference can miss things, and picking the right threshold is difficult.

Both might be worth pursuing as well and the second shouldn’t be hard to add to Compare. Unfortunately, I won’t be able to spend more time on this PR as I have to work on other areas. I changed the control flow to use the sharp default of single if per vips operation to certainly avoid memory leaks and squashed all commits into one for merging into knife 👍

Thanks again to both of you for your help 😄

gasi · 2015-05-18T23:36:53Z

  1) Normalization keeps an existing alpha channel:
     Error: timeout of 20000ms exceeded. Ensure the done() callback is being called in this test.

Not sure why this is failing. I didn’t make any changes to it.

lovell · 2015-05-19T08:10:09Z

Great job Daniel, I'm happy to take it from here and will use John's max recommendation to sort out the tests.

Thanks again for you for all your work on this improvement.

Premultiply alpha for transparent images to prevent dark edges

gasi · 2015-05-21T00:28:57Z

Great job Daniel, I'm happy to take it from here…

Thanks for merging this and my pleasure ❤️

gasi mentioned this pull request May 7, 2015

Debug dark fringing when resizing large images with transparency gasi/sharp#3

Closed

lovell force-pushed the knife branch from acc6059 to e553e92 Compare May 11, 2015 11:45

gasi force-pushed the premultiply-alpha branch from 18d434a to ca42ec7 Compare May 13, 2015 06:01

gasi reviewed May 13, 2015
View reviewed changes

lovell reviewed May 14, 2015
View reviewed changes

gasi reviewed May 15, 2015
View reviewed changes

gasi pushed a commit to gasi/sharp that referenced this pull request May 18, 2015

Prevent memory leaks for error cases

b78decb

Discussion: lovell#213 (comment)

gasi force-pushed the premultiply-alpha branch from da24175 to 167d665 Compare May 18, 2015 21:37

lovell added a commit that referenced this pull request May 19, 2015

Merge pull request #213 from gasi/premultiply-alpha

a45281f

Premultiply alpha for transparent images to prevent dark edges

lovell merged commit a45281f into lovell:knife May 19, 2015

JimBobSquarePants mentioned this pull request Jan 6, 2018

Incorrect color bleed when applying processors to images with alpha SixLabors/ImageSharp#428

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Premultiply alpha for transparent images to prevent dark edges #213

Premultiply alpha for transparent images to prevent dark edges #213

gasi commented May 7, 2015

gasi commented May 7, 2015

lovell commented May 7, 2015

lovell commented May 11, 2015

gasi commented May 12, 2015

gasi commented May 13, 2015

gasi May 13, 2015

gasi May 14, 2015

gasi commented May 13, 2015

coveralls commented May 13, 2015

lovell May 14, 2015

gasi May 15, 2015

lovell commented May 14, 2015

gasi commented May 15, 2015

gasi May 15, 2015

gasi May 15, 2015

gasi May 15, 2015

lovell commented May 15, 2015

gasi commented May 15, 2015

gasi May 15, 2015

lovell May 16, 2015

gasi May 16, 2015

gasi May 16, 2015

jcupitt May 16, 2015

lovell May 16, 2015

gasi May 18, 2015

lovell commented May 16, 2015

lovell commented May 16, 2015

jcupitt commented May 16, 2015

gasi commented May 18, 2015

gasi commented May 18, 2015

lovell commented May 19, 2015

gasi commented May 21, 2015


		* `err` contains an error message, if any.
		* `info` contains the info about the difference between the two images such as `isEqual` (Boolean), `meanSquaredError` (Number; present iff `status='success'`, otherwise `undefined`), and `status` (String; one of `success`, `mismatchedDimensions`, `mismatchedBands`, `mismatchedType`).

Premultiply alpha for transparent images to prevent dark edges #213

Premultiply alpha for transparent images to prevent dark edges #213

Conversation

gasi commented May 7, 2015

gasi commented May 7, 2015

lovell commented May 7, 2015

lovell commented May 11, 2015

gasi commented May 12, 2015

gasi commented May 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gasi commented May 13, 2015

coveralls commented May 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovell commented May 14, 2015

gasi commented May 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovell commented May 15, 2015

gasi commented May 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lovell commented May 16, 2015

lovell commented May 16, 2015

jcupitt commented May 16, 2015

gasi commented May 18, 2015

gasi commented May 18, 2015

lovell commented May 19, 2015

gasi commented May 21, 2015