Bug 138477 - Don't premultiply and then unpremultiply PNGs on iOS.
Summary: Don't premultiply and then unpremultiply PNGs on iOS.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-06 15:16 PST by Roger Fong
Modified: 2014-12-17 06:10 PST (History)
7 users (show)

See Also:


Attachments
patch (6.31 KB, patch)
2014-11-06 15:27 PST, Roger Fong
bfulgham: review-
Details | Formatted Diff | Diff
patch (6.31 KB, patch)
2014-11-06 15:55 PST, Roger Fong
no flags Details | Formatted Diff | Diff
patch (6.21 KB, patch)
2014-11-06 16:56 PST, Roger Fong
simon.fraser: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roger Fong 2014-11-06 15:16:10 PST
On iOS, when we specify png as not being premultiplied, CG will still premultiply the pngs for optimization purposes.
However, this causes rounding issues because if we want the raw PNG data back later and have to unmultiply, we lose data because the premultiplication ceil's the premultiplied value. 

This is an original issue if we have an alpha value of 1/255. We will round any color channel value to somewhere between 0-1 and then round to 1. When we unmultiply we get a final pixel value of 255,255,255,1.
While visually this is not a big issue since alpha is so low, we fail a number of conformance tests including
conformance/textures/gl-teximage.html
conformance/context/premultiplyalpha-test.html which explicitly check pixel values.

Thus in these special cases we should just retrieve the non premultiplied data in the first place.
Comment 1 Roger Fong 2014-11-06 15:21:18 PST
rdar://problem/15486393
Comment 2 Roger Fong 2014-11-06 15:27:32 PST
Created attachment 241136 [details]
patch
Comment 3 Brent Fulgham 2014-11-06 15:42:54 PST
Comment on attachment 241136 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241136&action=review

Looks good, but I think it could be better if we were using RetainPtrs so you didn't have to have a block of releases when these objects go out of scope.

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:364
> +        CFMutableDictionaryRef providerOptions = CFDictionaryCreateMutable(0, 0, &kCFCopyStringDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks);

Couldn't this be a RetainPtr<CFMutableDictionaryRef>?

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:367
> +        CGImageProviderRef provider = CGImageGetImageProvider(image.get());

Probably could use pngImage here.

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:387
> +        CFDataRef data = CFDataCreate(0, pngData, CGImageBlockGetBytesPerRow(block) * (size_t)CGRectGetHeight(blockRect));

Couldn't this be a RetainPtr<CFDataRef>?

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:390
> +        CGDataProviderRef imgDataProvider = CGDataProviderCreateWithCFData(data);

RetainPtr?

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:398
> +        CFRelease(providerOptions);

... these could go away if we were using RetainPtrs for the objects.
Comment 4 Roger Fong 2014-11-06 15:55:47 PST
Created attachment 241141 [details]
patch

done
Comment 5 Roger Fong 2014-11-06 16:56:28 PST
Created attachment 241146 [details]
patch
Comment 6 Brent Fulgham 2014-11-06 17:23:31 PST
Comment on attachment 241146 [details]
patch

Looks great!  r=me.
Comment 7 Simon Fraser (smfr) 2014-11-06 17:30:11 PST
Comment on attachment 241146 [details]
patch

Let's not be hasty.
Comment 8 Tim Horton 2014-11-06 17:33:29 PST
(In reply to comment #7)
> Comment on attachment 241146 [details]
> patch
> 
> Let's not be hasty.

What Simon said. Needs more review/performance testing/understanding/cleanup by everyone, I think. This is super critical code.
Comment 9 Roger Fong 2014-11-06 20:00:17 PST
I will run a test where I draw an image with an alpha gradient.
Because this only really effects low alpha values, i'll then re-render the image after removing the alpha channels. I'd expect to see a lot of banding.

I'll also write a performance test to continually redraw a (large) png (on a retina device?) and then run do some before and after profiling.

After I prove that this actually is an improvement (even if only slight) and if I determine that its not a noticeable performance hit then I think we'd be good to go.

If not, I can special case to only pngs used by WebGL (A call to texture2d).

It would be also nice if CG just had a flag for the decoder to return the raw png data instead of having to jump through all these hoops to get to it.
Comment 10 Mr F 2014-12-17 06:10:41 PST
Just stumbled across this nasty bug. Made a little test program: http://geom.io/pc25b/testPNG2.html
1st image: PNG with alpha as an Image element.
2nd image: same PNG without alpha channel as an Image element;
3rd image: canvas, first PNG with alpha, but loaded as WebGL texture and rendered on a quad.

On PC 2nd and 3rd images are the same.
On iOS 3rd image exhibits severe precision loss.

This is quite important to fix for WebGL apps, because we often store different encoded/packed data for textures and do not expect RGB to be broken.