Skip to content

Revise premultiply #2120

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

Merged
merged 15 commits into from
Mar 6, 2021
Merged

Revise premultiply #2120

merged 15 commits into from
Mar 6, 2021

Conversation

jcupitt
Copy link
Member

@jcupitt jcupitt commented Feb 27, 2021

This PR revises unpremultiply (again), and tries to simplify thumbnail a bit. It fixes resizing RGBA images in scRGB space, and hopefully doesn't regress an earlier RGBA resize issue. There's a test as well, to try to stop this happening again.

Background: #1941

Related issue: #1675

We were not detecting division by zero carefully enough, nor clipping
the alpha range sufficiently in unpremultiply.

see #1941

also see #1675 for another
difficult test case
we actually test vipsthumbnail --linear on an RGBA image, which should
catch everything, hopefully
for compat in thumbnail behaviour
@lovell lovell self-assigned this Mar 4, 2021
@lovell
Copy link
Member

lovell commented Mar 5, 2021

Profiling suggests the unpremultiply operation might be ~17% slower with this change.

Using callgrind to unpremultiply a 6MP RGBA image with code compiled using gcc 9.3.0 and -O3.

Before:

  175,523,571  ???:vips_unpremultiply_gen [/usr/local/lib/libvips.so.42.12.7]

After:

  212,753,893  ???:vips_unpremultiply_gen [/usr/local/lib/libvips.so.42.12.7]

Would the following diff be functionally equivalent?

--- a/libvips/include/vips/util.h
+++ b/libvips/include/vips/util.h
@@ -91,11 +91,7 @@ vips_recip( double x )
 {
        static const double epsilon = 1e-10;
 
-       int sign = x < 0.0 ? -1.0 : 1.0;
-
-       return( sign * x >= epsilon ?
-               1.0 / x :
-               sign / epsilon );
+       return 1 / VIPS_CLIP( -epsilon, x, epsilon );
 }

If so, that's only 8% slower:

  190,352,189  ???:vips_unpremultiply_gen [/usr/local/lib/libvips.so.42.12.7]

it wasn't really necessary, and it was rather slow
@jcupitt
Copy link
Member Author

jcupitt commented Mar 5, 2021

I removed the recip thing -- it wasn't really necessary, and it was slower than I expected. Thanks for the speed check!

@jcupitt jcupitt merged commit 81dffdd into 8.10 Mar 6, 2021
@jcupitt
Copy link
Member Author

jcupitt commented Mar 6, 2021

\o/ good stuff, thanks!

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