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

• Component: New Bugs
• Status: RESOLVED FIXED

[noel gordon]

Created attachment 119002 [details]
Example rendering Chromium Mac/OSX 18.0.965.0

http://crbug.com/106465

Chromium Mac/OSX only, example broken image is http://i.imgur.com/KZGJX.jpg

[noel gordon]

Created attachment 119003 [details]
KZGJX-no-color-profile.jpg

[noel gordon]

I removed the color profile from the example image, KZGJX-no-color-profile.jpg attached, and it renders correctly on Chromium Mac/OSX 18.0.965.0

[noel gordon]

Created attachment 119005 [details]
Expected rendering of the KZGJX.jpg

[noel gordon]

The "Example rendering of the KZGJX.jpg" is a PNG image showing how the image should be drawn.

[noel gordon]

The issue has to do with ICC color profiles; these are enabled on Chromium Mac/OSX whether SKIA is on or not.

The broken JCS_YCbCr JPEG image KZGJX.jpeg has an ICC color profile.

The ICC color profile header states that the profile color space is CMYK.

[noel gordon]

Chromium Win/Linux ignore color profiles and http://i.imgur.com/KZGJX.jpg renders correctly therein, so maybe ignore the ICC color profile if we can detect that it is CMYK.

[noel gordon]

Created attachment 119008 [details]
Patch

[WebKit Review Bot]

Comment on attachment 119008 [details]
Patch

Attachment 119008 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10845373

New failing tests:
media/event-attributes.html

[Peter Kasting]

Wait a minute.

(1) Why does not ignoring the color profile also affect the aspect ratio?
(2) Why would we want to ignore CMYK profiles in general?  Is this particular image mis-tagged?  Are we hopelessly buggy with regard to properly converting the colorspaces?  What's going on?

Until these sorts of things are answered I worry that "renders correctly if we ignore this so let's just do that" is a bandaid, and a wrong one at that.

[noel gordon]

(In reply to comment #9)
> Wait a minute.
> 
> (1) Why does not ignoring the color profile also affect the aspect ratio?

I don't understand the question.  What aspect ratio are you referring to?

[noel gordon]

(In reply to comment #9)
> (2) Why would we want to ignore CMYK profiles in general?

Reading from http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp#L233
the only image types that get color profile management are m_info.jpeg_color_space JCS_RGB and JCS_YCbCr images.  Color management is ignored for all other image types. 

So if we are ignoring CMYK profiles, it is for JCS_RGB and JCS_YCbCr images only, not "in general".

[noel gordon]

(In reply to comment #9)
> Is this particular image mis-tagged?

I believe so.  I extracted the ICC color profile from KZGJX.jpg, and used the ICC Profile Data Validation tool available at icc.org to validate the profile data.  It's valid according to that tool, and reports a CMYK data color space ... (screen shot attached) ...

[noel gordon]

Created attachment 119212 [details]
icc.org ICC color profileDump of KZGJX.jpg color profile

[noel gordon]

I noted the http://mxr.mozilla.org/mozilla-aurora/source/gfx/qcms/qcms.h#44 list the valid ICC color space signatures, and CMKY is included (icSigCmykData).

I then noted https://bugzil.la/16769 that added color management to their JPEGDecoder, using libicc and littleCMS which were added at the same time, so a hefty patch:

  https://bug16769.bugzilla.mozilla.org/attachment.cgi?id=273012&action=diff&format=raw

Searching for [nsJPEGDecoder.cpp] to see the changes, I noted a careful matching the color profile space and the JPEG info->jpeg_color_space, the case in point being 

       case JCS_RGB:
+        if (profileSpace != icSigRgbData)
+          mismatch =  PR_TRUE;
+        break;
       case JCS_YCbCr:
+        if (profileSpace == icSigRgbData)
+          mInfo.out_color_space = JCS_RGB;
+        else if (profileSpace != icSigYCbCrData)
+          mismatch = PR_TRUE;
         break;

mismatch = PR_TRUE for a JCS_YCbCr and icSigCmykData image, such as KZGJX.jpg, so color management won't be applied in this case.

This lead me to believe that KZGJX.jpg is a mis-tagged image, we don't spot it in the webkit code, and apply color management to the image and attachment #1 results.

[noel gordon]

(In reply to comment #9)
> Are we hopelessly buggy with regard to properly converting the colorspaces?  What's going on?

I'm only just learning about ICC color management.  Buggy might one way to put it.  Lax in or color space comparisons might be another if the ... 
 
> Until these sorts of things are answered I worry that "renders correctly if we ignore this so let's just do that" is a bandaid, and a wrong one at that.

... moz code is correct.  That code suggests we should not color correct a JCS_YCbCr and icSigCmykData image.  That is the focus of this patch.  I don't consider it a "bandaid", but I'd agree it is only a mitigating patch.

It should be possible after digesting all the above to suggest a wider change as the subject of another bug.

[noel gordon]

This regression was introduced in r96970 for bug 69622, as a follow up to bug 49950.  r96970 allowed JCS_YCbCr images to be color corrected, except if their ICC color profile was GRAY.  CMYK color profiles came through the door.

[Peter Kasting]

(In reply to comment #10)
> (In reply to comment #9)
> > Wait a minute.
> > 
> > (1) Why does not ignoring the color profile also affect the aspect ratio?
> 
> I don't understand the question.  What aspect ratio are you referring to?

The horizontal squishing of the output image.

The likely explanation for this seems to be the "upsampling" problem detailed in the comments for JCS_GRAYSCALE in JPEGImageDecoder.cpp:Decode().

Reading of the rest of the comments suggests a couple things to me.  I hope these make sense; I could be confused enough that these suggestions are impossible.

(1) The existing code has a number of bits that are added seemingly only because of problems feeding the decoded data to CG for drawing.  I'm uncomfortable doing things like "ignore color profiles on all greyscale and CMYK images because it causes resampling problems for CG".  It seems like instead we should be doing something CG-specific; there are several choices including "add #ifdefs to JPEGImageDecoder.cpp to segregate the CG-mandated hacks" or "add code in places like ImageDecoderCG.cpp to detect problematic cases coming form the decoders and work around them, e.g. by disabling color conversion".  The ideal world is one where all images are color-corrected properly on all platforms; I'd like to at least get to the point where non-CG ports are there.

(2) Once we're color-managing grey and CMYK images (at least for non-CG), we'll want to avoid ignoring CMYK profiles as well.  It seems like if we need to write a patch to ignore these for now, it should be more in the vein of Mozilla's code that ignores mismatched color profiles than just a blanket "ignore them all".  This way things will naturally work correctly both now and in the future.

[noel gordon]

(In reply to comment #17)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Wait a minute.
> > > 
> > > (1) Why does not ignoring the color profile also affect the aspect ratio?
> > 
> > I don't understand the question.  What aspect ratio are you referring to?
> 
> The horizontal squishing of the output image.

We decode to RGBA data, and provide CG with a color profile for that data.  But the profile describes CMYK color space, not RGB color space.  We then ask CG to transform these "CMYK" pixels to the screen RGB color space.  That's all wrong, and in so many ways, that I'm amazed the result even resembles the expected image.

[noel gordon]

(In reply to comment #17)
>
> The horizontal squishing of the output image.
> 
> The likely explanation for this seems to be the "upsampling" problem detailed in the comments for JCS_GRAYSCALE in JPEGImageDecoder.cpp:Decode().

The comment is confused.  JPEG "upsamples" chroma signals only.  This is needed to undo the chroma down-sampling applied during encoding.  The encoder converts the input image data to YCbCr (luma and chroma signals) and down-samples the chroma only.  This because the human eye is relatively insensitive to rapid variations in chroma (color).  The luma (Y) and the down-sampled chroma are then encoded separately using a DCT (Discreet Cosine Transform).  The transform coefficients are quantized, entropy encoded, and packed into a JFIF encoded frame structure.  A picture might help ...

[noel gordon]

Created attachment 119595 [details]
JPEG encoder phases

[noel gordon]

So in reference to that picture, each 2x2 block of chroma pixels is down-sampled to one chroma pixel.  The decoder has to undo that by taking each decoded sub-sampled chroma pixel, S say, and "upsample" it to reproduce its 2x2 block.  There are two ways, 1) fill the 2x2 block with S, or 2) smooth S into the 2x2 block using "fancy-upsampling".  Our decoder uses fancy-upsampling. See http://assassinationscience.com/johncostella/magic for examples of its effect on decoded image quality.

[noel gordon]

(In reply to comment #17)
>
> The horizontal squishing of the output image.
> 
> The likely explanation for this seems to be the "upsampling" problem detailed in the comments for JCS_GRAYSCALE in JPEGImageDecoder.cpp:Decode().

So the "upsampling" problem.  We see that upsampling is an internal detail of JPEG decoding, it's required to undo the JPEG encoders pre-processing step (chroma sub-sampling), and it makes the decoded image have the width/height of the original image.  Moreover, it's only used for color images.  JCS_GRAYSCALE images have no chroma :) the encoder does not down-sample grayscale images, the decoder does not need to upsample.

In general, JPEG upsampling is not a problem for any image type, it is an integral part of the JPEG system.  The comment suggests it causes Color Management System (CMS) problems in CG.  I've not mentioned CMS in discussing JPEG because I don't need to.  JPEG provides decoded pixels, they might need color management, but JPEG is not a CMS.

Color profiles in JPEG are instead for CMS use.  The CMS uses the profile to map the decoded pixels colors to "the colors in the color space supported by the output device" (the output gamut).  No upsampling is involved, more often a complicated transform from the gamut of the input device to that of the output device - your screen, your printer.

Input device?  A color profile should define the gamut of the input device used to record the image (camera, scanner, screen).  Look closely at the profile dump in attachment #5.  What I see is an "output class" color profile.  Looks to me like the color profile of some CMYK printer.  And we use it draw decoded RGBA to the an RGB screen, comment #18 results: CMYK->RGB is well-defined, but the input pixels are RGBA.

[Peter Kasting]

You'll get no argument from me that we should not let anyone (ourselves, CG, whoever) try to apply a completely bogus profile :).

Hopefully the thrust of my comments was "eventually, I'd like to do correct decoding and color management for all types of JPEGs on all platforms", where "correct" involves, among other things, some sort of sanity check like Mozilla's code does to avoid applying transforms that are obviously wrong.

[noel gordon]

Since the decoder and CMS deal in RGBA, sanity prevails by only accepting RGB color space color profiles from input class devices.

[noel gordon]

Created attachment 119750 [details]
Patch

[noel gordon]

% od -c KZGJX.icc.color.profile | head -3

           0000000  \0  \b 200 204   A   D   B   E 002 020  \0  \0   p   r   t   r <-- *printer*
*CMYK* --> 0000020   C   M   Y   K   L   a   b      \a 327  \0  \b  \0 024  \0  \0
           0000040  \0  \0  \0  \0   a   c   s   p   A   P   P   L  \0  \0  \0  \0

[Peter Kasting]

(In reply to comment #24)
> Since the decoder and CMS deal in RGBA, sanity prevails by only accepting RGB color space color profiles from input class devices.

Sure, for now.  I guess I am trying to ask whether we couldn't have broader support someday.  Maybe I am confused about what Mozilla's support actually is.

[noel gordon]

Broader support someday is possible, but I am assuming the "broader" here means add support for window or linux or some such.  If you instead mean relative mozilla, their support is documented here http://muizelaar.blogspot.com/2009/06/qcms-color-management-for-web.html and you can read the "limitations" section, from which I read RGBA->RGBA color matching.

[Peter Kasting]

(In reply to comment #28)
> If you instead mean relative mozilla, their support is documented here http://muizelaar.blogspot.com/2009/06/qcms-color-management-for-web.html and you can read the "limitations" section, from which I read RGBA->RGBA color matching.

I mean relative to what JPEG is capable of, so CMYK included.

I thought Firefox and qcms had been extended in the 2+ years since that blog post to support CMYK but I could be wrong.

[noel gordon]

http://mxr.mozilla.org/mozilla-aurora/source/image/decoders/nsJPEGDecoder.cpp#300

[noel gordon]

So qcms doesn't support color correction for JCS_CMYK, JCS_YCCK, or JCS_YCbCr images.

[noel gordon]

After reading http://bholley.wordpress.com/2008/09/12/so-many-colors, a post that continues to get comments to this day, I wonder if broader support is even desired.  Seems designers get confused about whether they should add color profiles or not.  CSS works in sRGB space.  To work with it, camera images for example should be tagged sRGB and not have a color profile.  Designers might not understand that and make the wrong choice.  Yet I can understanding the need for color profiles for artistic works, maybe even medical imagery. http://www.imedicalapps.com/2011/12/review-vue-motion-medical-image-viewer suggests the FDA is already happy.

[noel gordon]

Running locally in chromium mac/win DRT, and webkit mac DRT, the tests:

  fast/images/ycbcr-with-cmyk-color-profile.html
  fast/images/gray-scale-jpeg-with-color-profile.html
  fast/images/cmyk-jpeg-with-color-profile.html
  fast/images/color-jpeg-with-color-profile.html

are passing for me.  Another more I need do here?

[Adam Barth]

Comment on attachment 119750 [details]
Patch

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

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:116
> +#define iccProfileHeaderSize 128

WebKit tends to use constants rather than #defines.

[noel gordon]

Color inversion: R,G,B = 1 - C,M,Y

[noel gordon]

Squashing: only 3/4 of the pixels are used.  The color management library (ColorSync on CG) is told to use 3 input color components, but is given a CMYK (4 component) color profile.

[WebKit Review Bot]

Comment on attachment 119750 [details]
Patch

Clearing flags on attachment: 119750

Committed r103648: <http://trac.webkit.org/changeset/103648>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(In reply to comment #23)
> You'll get no argument from me that we should not let anyone (ourselves, CG, whoever) try to apply a completely bogus profile :).
> 
> Hopefully the thrust of my comments was "eventually, I'd like to do correct decoding and color management for all types of JPEGs on all platforms", where "correct" involves, among other things, some sort of sanity check like Mozilla's code does to avoid applying transforms that are obviously wrong.

Peter, we have something for this now using qcms over on bug 81974.  Tom Payne has being doing this work, and I'm now satisfied that this work has reached the point where we could consider landing.  Before we do that, I'd like your thoughts on the patch also if you have time.  It's the second last patch on the bug, patch id 144838, the one with all green bubbles.

[Peter Kasting]

Thanks for the heads-up.  Darin (Fisher) actually mentioned this to me yesterday and I suggested that caryclark and senorblanco might be better reviewers; I don't really know anything about anything the patch is trying to do :).  But I do appreciate the note!

[noel gordon]

No worries, and thanks for the advice.  Mr White and Mr Clark have joined us on the qcms bug so we'll just continue there.