Bug 138477 – Don't premultiply and then unpremultiply PNGs on iOS.

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: WebGL

[Roger Fong]

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.

[Roger Fong]

rdar://problem/15486393

[Roger Fong]

Created attachment 241136 [details]
patch

[Brent Fulgham]

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.

[Roger Fong]

Created attachment 241141 [details]
patch

done

[Roger Fong]

Created attachment 241146 [details]
patch

[Brent Fulgham]

Comment on attachment 241146 [details]
patch

Looks great!  r=me.

[Simon Fraser (smfr)]

Comment on attachment 241146 [details]
patch

Let's not be hasty.

[Tim Horton]

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

[Roger Fong]

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.

[Mr F]

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.

[mrmaxm]

(In reply to Roger Fong from comment #9)
> 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.

Since 2014 nothing happened on this bug.
It is still an issue, and iOS still premiltiplies PNG, making packing techniques such as RGBM not viable due to rounding issues.

Please advice on status of this bug.

[Kimmo Kinnunen]

I'll check it out.
It appears we still fail premultiplyalpha-test.html

[Kimmo Kinnunen]

conformance/context/premultiplyalpha-test.html now works.