Bug 27909 - WebKit cannot decode jpegs that are in CMYK or YCCK format
Summary: WebKit cannot decode jpegs that are in CMYK or YCCK format
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL: http://picrevisions.s3.cdn.pmnw.net/p...
Depends on:
Reported: 2009-07-31 19:14 PDT by Nico Weber
Modified: 2012-08-29 11:47 PDT (History)
3 users (show)

See Also:

Fix. (4.24 KB, patch)
2009-07-31 19:29 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
with email address (4.17 KB, patch)
2009-07-31 22:17 PDT, Nico Weber
no flags Details | Formatted Diff | Diff
Remove stupid comment (4.15 KB, patch)
2009-08-02 18:35 PDT, Nico Weber
eric: review-
Details | Formatted Diff | Diff
Fix style problems, extract rgb_convert_rgba() function (4.45 KB, patch)
2009-08-06 22:16 PDT, Nico Weber
eric: review-
Details | Formatted Diff | Diff
Rename functions to match webkit style (4.45 KB, patch)
2009-08-07 09:11 PDT, Nico Weber
eric: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nico Weber 2009-07-31 19:14:46 PDT
Opening http://picrevisions.s3.cdn.pmnw.net/pictures/4a59207b3cfd3/1/thumb_604.jpg in webkit (e.g. in chrome/linux) displays a broken image.
Comment 1 Nico Weber 2009-07-31 19:29:37 PDT
Created attachment 33922 [details]
Comment 2 Nico Weber 2009-07-31 19:30:26 PDT
This is http://code.google.com/p/chromium/issues/detail?id=15785 in chromium by the way.
Comment 3 Darin Adler 2009-07-31 19:30:36 PDT
Comment on attachment 33922 [details]

Test cases?
Comment 4 Nico Weber 2009-07-31 19:32:08 PDT
Hey, quick. Awesome.

How are image decoders usually tested? Can you point me to an example test?
Comment 5 Nico Weber 2009-07-31 19:36:50 PDT
As Catfish_Man just informed me in #webkit, WebKit/mac uses CoreGraphics for jpeg decoding, so if I add a test, it would fail on OS X (CoreGraphics decodes the jpeg to a black image -- and I can't fix CoreGraphics due to lack of source code).

If you still want a test, I'll add one, just wanted to point that out (and as I said, I'd appreciate an example test).
Comment 6 Eric Seidel (no email) 2009-07-31 19:40:07 PDT
Comment on attachment 33922 [details]

Style issues (please see check-webkit-style script) and missing EMAIL_ADDRESS.
Comment 7 Nico Weber 2009-07-31 22:17:42 PDT
Created attachment 33929 [details]
with email address

Sorry about the email address.

check-webkit-style doesn't find anything, though:

Done processing WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
Total errors found: 0
Comment 8 Mark Rowe (bdash) 2009-08-02 18:08:45 PDT
Comment on attachment 33929 [details]
with email address

> +                buffer.setRGBA(x, info->output_scanline - 1, r, g, b, 0xFF);  // WTF???!??!?!?!

Do you honestly expect that a patch containing this comment will be landed?
Comment 9 Nico Weber 2009-08-02 18:35:48 PDT
Created attachment 33957 [details]
Remove stupid comment

Heh. I thought I had removed that comment before creating the patch. My apologies.
Comment 10 Eric Seidel (no email) 2009-08-06 17:59:58 PDT
Comment on attachment 33957 [details]
Remove stupid comment

This does not follow WebKit style.  Maybe it's not supposed to?

 481         dest.setRGBA(x, info->output_scanline - 1, c*k/255, m*k/255, y*k/255, 0xFF);

 513             JSAMPLE *j1 = samples[0];

extra { }:
2         } else if (info->out_color_space == JCS_CMYK) {
 523             cmyk_convert_rgb(buffer, *samples, info);
 524         } else {


check-webkit-style should also help you.

Seems cmyk_convert_rgb should ASSERT output colorspace is JCS_CMYK?

You should consider throwing the rgb stuff in a similar static inline.

r- for the style violiations (or lack of informations as to why we would not want to match WebKit style in this file).  please run check-webkit-style, there is a diff option I'm told.
Comment 11 Nico Weber 2009-08-06 22:16:42 PDT
Created attachment 34250 [details]
Fix style problems, extract rgb_convert_rgba() function
Comment 12 Eric Seidel (no email) 2009-08-07 08:54:44 PDT
Comment on attachment 34250 [details]
Fix style problems, extract rgb_convert_rgba() function

cmyk_convert_rgba and rgb_convert_rgba should follow WebKit style naming (unless we have good reason not do here?)

Something like:
would be webkit style naming.

Otherwise looks great.  If you were a committer I would r+ this and you could fix it when landing.
Comment 13 Nico Weber 2009-08-07 09:11:02 PDT
Created attachment 34282 [details]
Rename functions to match webkit style

The function name wasn't caught by check-webkit-style I believe.

Sadly, I'm not a committer (yet?).
Comment 14 Alexey Proskuryakov 2009-08-07 10:25:48 PDT
+        // R = 1 - C => 1 - (1 - iC*iK) => iC*iK
+        // G = 1 - M => 1 - (1 - iM*iK) => iM*iK
+        // B = 1 - Y => 1 - (1 - iY*iK) => iY*iK

These formulas give really poor fidelity - this may be better than a broken image icon, but may be not.

Per comments in Chromium bug, this doesn't work in IE or Safari either. Is there a need to rush a low-quality implementation?

We'll need a bug to track fixing this for Safari, as well.
Comment 15 Nico Weber 2009-08-07 10:30:50 PDT
ap: I don't see how this is "low quality". It looks decent enough. It might be that there's a "high-quality" mode that is slightly better, but having an image that looks decent is miles better than having a broken image. This is a pretty commonly reported chromium bug ("can see my pictures in firefox, but not in chrome. chrome FAIL"), so I'd like to get this fixed.

What do you think is wrong with the formula?

The Safari side of this fix needs a CoreGraphics patch, which I can't provide, sorry.
Comment 16 Alexey Proskuryakov 2009-08-07 10:45:35 PDT
Please see <http://en.wikipedia.org/wiki/CMYK_color_model#Conversion>, for example.

Please also note that the only sensible reason to use CMYK on the Web is to enable wide-gamut printing. So, besides a way to convert CMYK for display we'd need a way to convert it directly to printer color space, as CMYK->display->printer will lose a lot of quality regardless of how well it is performed, simply because printer gamut is different.

See <https://bugs.webkit.org/show_bug.cgi?id=12205#c6> for what Opera does with CMYK CSS colors - there's a good chance that they do the same with images.
Comment 17 Nico Weber 2009-08-07 11:31:38 PDT
CSS does not have CMYK support yet, so it can be designed in a vacuum. Jpegs that use cmyk on the other are being used on the web right now, Firefox supports them (using the exact same conversion that this patch adds), and users complain that they don't work in chrome. In my eyes, this patch makes the situation strictly better for these users. I guess most people won't care about color-correct conversions (cf. http://crbug.com/17354 )

As far as I can see, webkit does not support cmyk bitmaps, so fixing this the Right Way requires substantial work. Until someone feels like doing this work, this patch fixes the problem for probably at least 95% of our users.

Finally, the web is not intended to be printed, wide-gamut or not ;-)
Comment 18 Alexey Proskuryakov 2009-08-07 12:55:29 PDT
These images aren't even displayed by IE 8. Is there any urgency that would justify avoiding substantial work?

> Finally, the web is not intended to be printed, wide-gamut or not ;-)

I'm sure many people will disagree.
Comment 19 Nico Weber 2009-08-07 13:05:21 PDT
"But it doesn't work in IE8 either" does not sound like a strong argument.

Jpeg is one of the image formats supported on the web, so all of its subformats should be supported. It's hard to explain to users that a few jpegs don't work but most do. Firefox supports this format, and people complain that WebKit does not support it.

I don't think there's a need for "substantial work", i.e. adding full cmyk support to all of webkit (but if "many people" believe that printing the web might be a good idea, then I might be wrong).

In any case, this less-than-100-lines patch should go in in my opinion.
Comment 20 Eric Seidel (no email) 2009-08-07 14:06:17 PDT
Comment on attachment 34282 [details]
Rename functions to match webkit style

LGTM.  Thank you for your patience and iteration!
Comment 21 Adam Barth 2009-08-07 17:03:32 PDT
Comment on attachment 34282 [details]
Rename functions to match webkit style

Clearing review flag on attachment: 34282

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
Committed r46931
	M	LayoutTests/platform/win/Skipped
	M	LayoutTests/ChangeLog
r46929 = 0a5b5f5445f0bc5e2dee4ea00e4e9da2629c65a1 (trunk)
	M	LayoutTests/platform/gtk/Skipped
	M	LayoutTests/ChangeLog
r46930 = 25e753dc58a5fbc3508ecfe03c156db2172fa46e (trunk)
	M	WebCore/ChangeLog
	M	WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
r46931 = d484caf449660b8f8febb1049dd9ab9f6b67349b (trunk)
First, rewinding head to replay your work on top of it...
Nothing to do.
Comment 22 Adam Barth 2009-08-07 17:03:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Alexey Proskuryakov 2012-08-29 11:47:23 PDT
CoreGraphics issue has been fixed as of OS X Lion, so these images are displayed correctly in Safari.

AFAICT, Chrome is still using the naive CMYK conversion introduced by this patch though. And we still don't have a non-lossy printing path.