Bug 76804 - [chromium] PNG image with CMYK ICC color profile renders color-inverted and squashed
Summary: [chromium] PNG image with CMYK ICC color profile renders color-inverted and s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: noel gordon
URL:
Keywords:
Depends on: 48170 49110 74400
Blocks: 76968
  Show dependency treegraph
 
Reported: 2012-01-22 20:04 PST by noel gordon
Modified: 2012-01-24 18:12 PST (History)
5 users (show)

See Also:


Attachments
Example rendering on Chrome Mac 16.0.912.75 (198.80 KB, image/png)
2012-01-22 20:04 PST, noel gordon
no flags Details
Expected rendering of layout.png (237.27 KB, image/png)
2012-01-22 20:11 PST, noel gordon
no flags Details
ICC color profile of layout.png (544.11 KB, application/octet-stream)
2012-01-22 20:13 PST, noel gordon
no flags Details
Patch (952.94 KB, patch)
2012-01-22 20:57 PST, noel gordon
no flags Details | Formatted Diff | Diff
Patch (952.59 KB, patch)
2012-01-22 21:04 PST, noel gordon
no flags Details | Formatted Diff | Diff
Patch (954.79 KB, patch)
2012-01-23 20:33 PST, noel gordon
no flags Details | Formatted Diff | Diff
Patch (954.63 KB, patch)
2012-01-23 21:30 PST, noel gordon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description noel gordon 2012-01-22 20:04:54 PST
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.
Comment 1 noel gordon 2012-01-22 20:11:15 PST
Created attachment 123511 [details]
Expected rendering of layout.png
Comment 2 noel gordon 2012-01-22 20:13:52 PST
Created attachment 123512 [details]
ICC color profile of layout.png
Comment 3 noel gordon 2012-01-22 20:18:29 PST
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
Comment 4 noel gordon 2012-01-22 20:57:47 PST
Created attachment 123515 [details]
Patch
Comment 5 noel gordon 2012-01-22 21:02:54 PST
webkit-patch upload result looks wrong to me.
Comment 6 noel gordon 2012-01-22 21:04:15 PST
Created attachment 123517 [details]
Patch
Comment 7 noel gordon 2012-01-22 21:07:38 PST
(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 :)
Comment 8 Adam Barth 2012-01-23 01:30:55 PST
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.
Comment 9 noel gordon 2012-01-23 20:28:43 PST
> 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.
Comment 10 noel gordon 2012-01-23 20:33:26 PST
Created attachment 123693 [details]
Patch
Comment 11 Adam Barth 2012-01-23 20:46:45 PST
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.
Comment 12 noel gordon 2012-01-23 21:28:06 PST
(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()) ?
Comment 13 noel gordon 2012-01-23 21:30:17 PST
Created attachment 123696 [details]
Patch
Comment 14 Adam Barth 2012-01-23 21:33:51 PST
Comment on attachment 123696 [details]
Patch

Yeah, I meant the ASSERT, but this is fine too.
Comment 15 WebKit Review Bot 2012-01-24 01:57:18 PST
Comment on attachment 123696 [details]
Patch

Clearing flags on attachment: 123696

Committed r105709: <http://trac.webkit.org/changeset/105709>
Comment 16 WebKit Review Bot 2012-01-24 01:57:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 noel gordon 2012-01-24 18:12:20 PST
(In reply to comment #9)
> ...  Assuming that this is ok, I'll send a separate patch to update the JPEG decoder.

Filed bug 76968.