Bug 49110

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.

Created attachment 73141 [details] Patch

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

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

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 on attachment 73141 [details] Patch Semi-reluctant r+. :)

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

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

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 on attachment 73141 [details] Patch Clearing flags on attachment: 73141 Committed r71461: <http://trac.webkit.org/changeset/71461>

All reviewed patches have been landed. Closing bug.