Summary: | [chromium] PNG image with CMYK ICC color profile renders color-inverted and squashed | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | noel gordon <noel.gordon> | ||||||||||||||||
Component: | New Bugs | Assignee: | noel gordon <noel.gordon> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, eric, jasonadamses, jbauman, kbr, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 48170, 49110, 74400 | ||||||||||||||||||
Bug Blocks: | 76968 | ||||||||||||||||||
Attachments: |
|
Description
noel gordon
2012-01-22 20:04:54 PST
Created attachment 123511 [details]
Expected rendering of layout.png
Created attachment 123512 [details]
ICC color profile of layout.png
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 Created attachment 123515 [details]
Patch
webkit-patch upload result looks wrong to me. Created attachment 123517 [details]
Patch
(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 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. > 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.
Created attachment 123693 [details]
Patch
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. (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()) ? Created attachment 123696 [details]
Patch
Comment on attachment 123696 [details]
Patch
Yeah, I meant the ASSERT, but this is fine too.
Comment on attachment 123696 [details] Patch Clearing flags on attachment: 123696 Committed r105709: <http://trac.webkit.org/changeset/105709> All reviewed patches have been landed. Closing bug. (In reply to comment #9) > ... Assuming that this is ok, I'll send a separate patch to update the JPEG decoder. Filed bug 76968. |