Bug 49110

Summary: Gray-scale PNGs with color profiles don't decode properly on Chromium Mac
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: amanda, commit-queue, eric, mark, pkasting
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 76804    
Attachments:
Description Flags
Patch none

Description Adam Barth 2010-11-05 16:19:47 PDT
Gray-scale PNGs with color profiles don't decode properly on Chromium Mac
Comment 1 Eric Seidel (no email) 2010-11-05 16:39:06 PDT
I suspect you're not correctly representing to CG the expected colorspace of the decoded bytes.  (Probably telling CG your'e hadning it RGBA, when you're really handing it some greyscale data.
Comment 2 Adam Barth 2010-11-05 16:51:31 PDT
Created attachment 73141 [details]
Patch
Comment 3 Adam Barth 2010-11-05 16:52:35 PDT
Nope, we're really handing it RGBA data.  I think that the issue is the color profile we give it is for gray scale (even though we've already converted to RGBA).  I added a comment explaining it in the patch.
Comment 4 Eric Seidel (no email) 2010-11-05 17:12:44 PDT
Comment on attachment 73141 [details]
Patch

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

Was this happening on both Snow Leopard and Leopard?  The Leopard code path is hard-coded for 3 components:

    RetainPtr<CGColorSpaceRef> deviceColorSpace(AdoptCF, CGColorSpaceCreateDeviceRGB());
    RetainPtr<CGDataProviderRef> profileDataProvider(AdoptCF, CGDataProviderCreateWithCFData(data.get()));
    CGFloat ranges[] = {0.0, 255.0, 0.0, 255.0, 0.0, 255.0};
    return CGColorSpaceCreateICCBased(3, ranges, profileDataProvider.get(), deviceColorSpace.get());

> WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:268
> +    if (colorType == PNG_COLOR_TYPE_RGB || colorType == PNG_COLOR_TYPE_RGB_ALPHA) {

I assume this is intentionally excluding all other formats?

   color_type     - describes which color/alpha channels
                         are present.
                     PNG_COLOR_TYPE_GRAY
                        (bit depths 1, 2, 4, 8, 16)
                     PNG_COLOR_TYPE_GRAY_ALPHA
                        (bit depths 8, 16)
                     PNG_COLOR_TYPE_PALETTE
                        (bit depths 1, 2, 4, 8)
                     PNG_COLOR_TYPE_RGB
                        (bit_depths 8, 16)
                     PNG_COLOR_TYPE_RGB_ALPHA
                        (bit_depths 8, 16)

                     PNG_COLOR_MASK_PALETTE
                     PNG_COLOR_MASK_COLOR
                     PNG_COLOR_MASK_ALPHA

I'm not sure we care about any of those.
Comment 5 Adam Barth 2010-11-05 17:25:15 PDT
> Was this happening on both Snow Leopard and Leopard?  The Leopard code path is hard-coded for 3 components:

Chromium always compiles with the Leopard branch.

>     RetainPtr<CGColorSpaceRef> deviceColorSpace(AdoptCF, CGColorSpaceCreateDeviceRGB());
>     RetainPtr<CGDataProviderRef> profileDataProvider(AdoptCF, CGDataProviderCreateWithCFData(data.get()));
>     CGFloat ranges[] = {0.0, 255.0, 0.0, 255.0, 0.0, 255.0};
>     return CGColorSpaceCreateICCBased(3, ranges, profileDataProvider.get(), deviceColorSpace.get());

I tried different variations on this code.  Any number besides 3 returns NULL.  In theory, this function is supposed to accept 1 and 4, but I haven't found any ICC profiles that actually work with those values.

> > WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:268
> > +    if (colorType == PNG_COLOR_TYPE_RGB || colorType == PNG_COLOR_TYPE_RGB_ALPHA) {
> 
> I assume this is intentionally excluding all other formats?
> 
>    color_type     - describes which color/alpha channels
>                          are present.
>                      PNG_COLOR_TYPE_GRAY
>                         (bit depths 1, 2, 4, 8, 16)
>                      PNG_COLOR_TYPE_GRAY_ALPHA
>                         (bit depths 8, 16)
>                      PNG_COLOR_TYPE_PALETTE
>                         (bit depths 1, 2, 4, 8)
>                      PNG_COLOR_TYPE_RGB
>                         (bit_depths 8, 16)
>                      PNG_COLOR_TYPE_RGB_ALPHA
>                         (bit_depths 8, 16)
> 
>                      PNG_COLOR_MASK_PALETTE
>                      PNG_COLOR_MASK_COLOR
>                      PNG_COLOR_MASK_ALPHA
> 
> I'm not sure we care about any of those.

The only one I'm not sure about is PNG_COLOR_MASK_PALETTE.  However, we do the pixel expansion for PNG_COLOR_MASK_PALETTE also, so it seemed safer to whitelist the ones I know work.
Comment 6 Eric Seidel (no email) 2010-11-05 17:32:34 PDT
Would be useful if I still had active contacts on the CG team...

I'm sure there is a nice way to do this, but I don't have good suggestions for you.  Removing the broken case seems better than the current behavior.
Comment 7 Eric Seidel (no email) 2010-11-05 17:33:03 PDT
Comment on attachment 73141 [details]
Patch

Semi-reluctant r+. :)
Comment 8 Eric Seidel (no email) 2010-11-05 17:36:16 PDT
Oh, if it's not necessarily happening in Snow Leopard, then restricting to RGB always seems folly.
Comment 9 Eric Seidel (no email) 2010-11-05 17:36:51 PDT
I guess it being broken for one cg code path might be an excuse to disable it for all.
Comment 10 Adam Barth 2010-11-05 17:41:18 PDT
Yeah, I don't feel great about this patch either.  Hard-coding the 3 seems wrong.  I wish I had a nice suite of test images, but I couldn't find ones for some of the cases.
Comment 11 WebKit Commit Bot 2010-11-05 18:01:30 PDT
Comment on attachment 73141 [details]
Patch

Clearing flags on attachment: 73141

Committed r71461: <http://trac.webkit.org/changeset/71461>
Comment 12 WebKit Commit Bot 2010-11-05 18:01:36 PDT
All reviewed patches have been landed.  Closing bug.