Bug 76804 – [chromium] PNG image with CMYK ICC color profile renders color-inverted and squashed

• Component: New Bugs
• Status: RESOLVED FIXED

[noel gordon]

Created attachment 123510 [details]
Example rendering on Chrome Mac 16.0.912.75

As reported in http://crbug.com/110708, image at http://fista.iscte-iul.pt/layout.png rendered incorrectly.

[noel gordon]

Created attachment 123511 [details]
Expected rendering of layout.png

[noel gordon]

Created attachment 123512 [details]
ICC color profile of layout.png

[noel gordon]

Same problem to bug 74400, but for PNG images; color profile is for a CMYK output class device (a printer).

% od -c color.profile.layout.png.icc | head -3

                     0000000   \0  \b 200   p   A   D   B   E 002 020  \0  \0   p   r   t   r <-- *printer*
*CMYK* -->  0000020    C   M   Y   K   L   a   b      \a 320  \0  \a  \0 032  \0 005
                     0000040   \0   )  \0   5   a   c   s   p   A   P   P   L  \0  \0  \0  \0

[noel gordon]

Created attachment 123515 [details]
Patch

[noel gordon]

webkit-patch upload result looks wrong to me.

[noel gordon]

Created attachment 123517 [details]
Patch

[noel gordon]

(In reply to comment #5)
> webkit-patch upload result looks wrong to me.

Ah nevermind, I should look at the patch in a browser that does _not_ exhibit this bug :)

[Adam Barth]

Comment on attachment 123517 [details]
Patch

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

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:233
> +// FIXME: color profile classification code was added in https://bugs.webkit.org/show_bug.cgi?id=74400
> +// for JPEG images. Use that code here for now, but factor the code out into some common place.

Can we do that in this patch?  It seems bad to have this copy/pasted code.

[noel gordon]

> Can we do that in this patch?  It seems bad to have this copy/pasted code.

OK, I'll add the profile helpers to ImageDecoder.h and make the PNGDecoder use them.  Assuming that this is ok, I'll send a separate patch to update the JPEG decoder.

[noel gordon]

Created attachment 123693 [details]
Patch

[Adam Barth]

Comment on attachment 123693 [details]
Patch

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

> Source/WebCore/platform/image-decoders/ImageDecoder.h:304
> +        enum Dimension { iccColorProfileHeaderLength = 128 };

You can also make this an anonymous enum, if you're not going to use the type anywhere.

> Source/WebCore/platform/image-decoders/ImageDecoder.h:312
> +            if (!memcmp(&profileData[16], "RGB ", 4))
> +                return true;
> +            return false;

Why not just:

return !memcmp(&profileData[16], "RGB ", 4);

?

> Source/WebCore/platform/image-decoders/ImageDecoder.h:323
> +            if (!memcmp(&profileData[12], "mntr", 4))
> +                return true;
> +            if (!memcmp(&profileData[12], "scnr", 4))
> +                return true;
> +            return false;

Similarly:

return !memcmp(&profileData[12], "mntr", 4) || !memcmp(&profileData[12], "scnr", 4);

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:256
> +    if (profileLength < ImageDecoder::iccColorProfileHeaderLength)
> +        ignoreProfile = true;
> +    else if (!ImageDecoder::rgbColorProfile(profileData, profileLength))
> +        ignoreProfile = true;
> +    else if (!ImageDecoder::inputDeviceColorProfile(profileData, profileLength))
> +        ignoreProfile = true;
> +
> +    if (!ignoreProfile)

I would have just combined this all into one if statement rather than using a temporary, but i'm not sure it's a big deal.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:297
> -        m_colorProfile = readColorProfile(png, info);
> +        readColorProfile(png, info, m_colorProfile);

Should we assert that m_colorProfile is empty?  readColorProfile doesn't zero it out when the profile isn't legit.

[noel gordon]

(In reply to comment #11)
> (From update of attachment 123693 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123693&action=review
> 
> > Source/WebCore/platform/image-decoders/ImageDecoder.h:304
> > +        enum Dimension { iccColorProfileHeaderLength = 128 };
> 
> You can also make this an anonymous enum, if you're not going to use the type anywhere.

Done.

> > Source/WebCore/platform/image-decoders/ImageDecoder.h:312
> > +            if (!memcmp(&profileData[16], "RGB ", 4))
> > +                return true;
> > +            return false;
> 
> Why not just:
> 
> return !memcmp(&profileData[16], "RGB ", 4);
> 
> ?
> 
> > Source/WebCore/platform/image-decoders/ImageDecoder.h:323
> > +            if (!memcmp(&profileData[12], "mntr", 4))
> > +                return true;
> > +            if (!memcmp(&profileData[12], "scnr", 4))
> > +                return true;
> > +            return false;
> 
> Similarly:
> 
> return !memcmp(&profileData[12], "mntr", 4) || !memcmp(&profileData[12], "scnr", 4);

A bit harder to read for me, but done.

 
> > Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:256
> > +    if (profileLength < ImageDecoder::iccColorProfileHeaderLength)
> > +        ignoreProfile = true;
> > +    else if (!ImageDecoder::rgbColorProfile(profileData, profileLength))
> > +        ignoreProfile = true;
> > +    else if (!ImageDecoder::inputDeviceColorProfile(profileData, profileLength))
> > +        ignoreProfile = true;
> > +
> > +    if (!ignoreProfile)
> 
> I would have just combined this all into one if statement rather than using a temporary, but i'm not sure it's a big deal.

Could just early return too, but written this way to match the JPEGDecoder.


> > Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:297
> > -        m_colorProfile = readColorProfile(png, info);
> > +        readColorProfile(png, info, m_colorProfile);
> 
> Should we assert that m_colorProfile is empty?  readColorProfile doesn't zero it out when the profile isn't legit.

PNGImageDecoder.cpp@232, maybe you missed the ASSERT(colorProfile.isEmpty()) ?

[noel gordon]

Created attachment 123696 [details]
Patch

[Adam Barth]

Comment on attachment 123696 [details]
Patch

Yeah, I meant the ASSERT, but this is fine too.

[WebKit Review Bot]

Comment on attachment 123696 [details]
Patch

Clearing flags on attachment: 123696

Committed r105709: <http://trac.webkit.org/changeset/105709>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(In reply to comment #9)
> ...  Assuming that this is ok, I'll send a separate patch to update the JPEG decoder.

Filed bug 76968.