Bug 74400 - [chromium] JPEG image with CMYK ICC color profile renders color-inverted and squashed
Summary: [chromium] JPEG image with CMYK ICC color profile renders color-inverted and ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified OS X 10.6
: P2 Normal
Assignee: noel gordon
URL:
Keywords:
Depends on: 49950 53250 69622 75182
Blocks: 76804
  Show dependency treegraph
 
Reported: 2011-12-13 04:17 PST by noel gordon
Modified: 2012-06-06 06:38 PDT (History)
5 users (show)

See Also:


Attachments
Example rendering Chromium Mac/OSX 18.0.965.0 (104.48 KB, image/png)
2011-12-13 04:17 PST, noel gordon
no flags Details
KZGJX-no-color-profile.jpg (18.54 KB, image/jpeg)
2011-12-13 04:24 PST, noel gordon
no flags Details
Expected rendering of the KZGJX.jpg (90.25 KB, image/png)
2011-12-13 04:32 PST, noel gordon
no flags Details
Patch (704.19 KB, patch)
2011-12-13 05:22 PST, noel gordon
no flags Details | Formatted Diff | Diff
icc.org ICC color profileDump of KZGJX.jpg color profile (41.34 KB, image/png)
2011-12-14 06:05 PST, noel gordon
no flags Details
JPEG encoder phases (28.40 KB, image/png)
2011-12-16 03:42 PST, noel gordon
no flags Details
Patch (707.91 KB, patch)
2011-12-17 21:04 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 2011-12-13 04:17:52 PST
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
Comment 1 noel gordon 2011-12-13 04:24:34 PST
Created attachment 119003 [details]
KZGJX-no-color-profile.jpg
Comment 2 noel gordon 2011-12-13 04:26:46 PST
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
Comment 3 noel gordon 2011-12-13 04:32:32 PST
Created attachment 119005 [details]
Expected rendering of the KZGJX.jpg
Comment 4 noel gordon 2011-12-13 04:34:02 PST
The "Example rendering of the KZGJX.jpg" is a PNG image showing how the image should be drawn.
Comment 5 noel gordon 2011-12-13 04:38:38 PST
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.
Comment 6 noel gordon 2011-12-13 05:08:35 PST
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.
Comment 7 noel gordon 2011-12-13 05:22:03 PST
Created attachment 119008 [details]
Patch
Comment 8 WebKit Review Bot 2011-12-13 07:10:18 PST
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
Comment 9 Peter Kasting 2011-12-13 10:30:20 PST
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.
Comment 10 noel gordon 2011-12-14 05:43:36 PST
(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?
Comment 11 noel gordon 2011-12-14 06:00:10 PST
(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".
Comment 12 noel gordon 2011-12-14 06:03:12 PST
(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) ...
Comment 13 noel gordon 2011-12-14 06:05:55 PST
Created attachment 119212 [details]
icc.org ICC color profileDump of KZGJX.jpg color profile
Comment 14 noel gordon 2011-12-14 06:48:40 PST
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.
Comment 15 noel gordon 2011-12-14 07:17:03 PST
(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.
Comment 16 noel gordon 2011-12-14 07:38:37 PST
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.
Comment 17 Peter Kasting 2011-12-14 10:36:17 PST
(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.
Comment 18 noel gordon 2011-12-16 03:12:03 PST
(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.
Comment 19 noel gordon 2011-12-16 03:41:15 PST
(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 ...
Comment 20 noel gordon 2011-12-16 03:42:41 PST
Created attachment 119595 [details]
JPEG encoder phases
Comment 21 noel gordon 2011-12-16 04:33:18 PST
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.
Comment 22 noel gordon 2011-12-16 08:23:55 PST
(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.
Comment 23 Peter Kasting 2011-12-16 09:58:26 PST
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.
Comment 24 noel gordon 2011-12-17 20:41:41 PST
Since the decoder and CMS deal in RGBA, sanity prevails by only accepting RGB color space color profiles from input class devices.
Comment 25 noel gordon 2011-12-17 21:04:24 PST
Created attachment 119750 [details]
Patch
Comment 26 noel gordon 2011-12-17 21:28:19 PST
% 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
Comment 27 Peter Kasting 2011-12-19 11:27:27 PST
(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.
Comment 28 noel gordon 2011-12-21 18:09:46 PST
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.
Comment 29 Peter Kasting 2011-12-21 18:12:59 PST
(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.
Comment 31 noel gordon 2011-12-22 03:39:57 PST
So qcms doesn't support color correction for JCS_CMYK, JCS_YCCK, or JCS_YCbCr images.
Comment 32 noel gordon 2011-12-22 05:01:20 PST
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.
Comment 33 noel gordon 2011-12-23 18:21:35 PST
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?
Comment 34 Adam Barth 2011-12-23 18:31:20 PST
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.
Comment 35 noel gordon 2011-12-23 20:01:33 PST
Color inversion: R,G,B = 1 - C,M,Y
Comment 36 noel gordon 2011-12-23 20:05:48 PST
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.
Comment 37 WebKit Review Bot 2011-12-23 21:08:33 PST
Comment on attachment 119750 [details]
Patch

Clearing flags on attachment: 119750

Committed r103648: <http://trac.webkit.org/changeset/103648>
Comment 38 WebKit Review Bot 2011-12-23 21:08:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 39 noel gordon 2012-06-01 21:41:11 PDT
(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.
Comment 40 Peter Kasting 2012-06-02 12:07:14 PDT
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!
Comment 41 noel gordon 2012-06-06 06:38:48 PDT
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.