Bug 49110 – Gray-scale PNGs with color profiles don't decode properly on Chromium Mac

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Gray-scale PNGs with color profiles don't decode properly on Chromium Mac

[Eric Seidel (no email)]

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.

[Adam Barth]

Created attachment 73141 [details]
Patch

[Adam Barth]

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.

[Eric Seidel (no email)]

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.

[Adam Barth]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 73141 [details]
Patch

Semi-reluctant r+. :)

[Eric Seidel (no email)]

Oh, if it's not necessarily happening in Snow Leopard, then restricting to RGB always seems folly.

[Eric Seidel (no email)]

I guess it being broken for one cg code path might be an excuse to disable it for all.

[Adam Barth]

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.

[WebKit Commit Bot]

Comment on attachment 73141 [details]
Patch

Clearing flags on attachment: 73141

Committed r71461: <http://trac.webkit.org/changeset/71461>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.