Bug 27909 – WebKit cannot decode jpegs that are in CMYK or YCCK format

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Nico Weber]

Opening http://picrevisions.s3.cdn.pmnw.net/pictures/4a59207b3cfd3/1/thumb_604.jpg in webkit (e.g. in chrome/linux) displays a broken image.

[Nico Weber]

Created attachment 33922 [details]
Fix.

[Nico Weber]

This is http://code.google.com/p/chromium/issues/detail?id=15785 in chromium by the way.

[Darin Adler]

Comment on attachment 33922 [details]
Fix.

Test cases?

[Nico Weber]

Hey, quick. Awesome.

How are image decoders usually tested? Can you point me to an example test?

[Nico Weber]

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).

[Eric Seidel (no email)]

Comment on attachment 33922 [details]
Fix.

Style issues (please see check-webkit-style script) and missing EMAIL_ADDRESS.

[Nico Weber]

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

[Mark Rowe (bdash)]

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?

[Nico Weber]

Created attachment 33957 [details]
Remove stupid comment

Heh. I thought I had removed that comment before creating the patch. My apologies.

[Eric Seidel (no email)]

Comment on attachment 33957 [details]
Remove stupid comment

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

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

spacing:
 513             JSAMPLE *j1 = samples[0];

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

See:
http://webkit.org/coding/coding-style.html

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.

[Nico Weber]

Created attachment 34250 [details]
Fix style problems, extract rgb_convert_rgba() function

[Eric Seidel (no email)]

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:
convertRGBToRGBA
convertCMYKToRGBA
would be webkit style naming.

Otherwise looks great.  If you were a committer I would r+ this and you could fix it when landing.

[Nico Weber]

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?).

[Alexey Proskuryakov]

+        // 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.

[Nico Weber]

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.

[Alexey Proskuryakov]

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.

[Nico Weber]

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 ;-)

[Alexey Proskuryakov]

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.

[Nico Weber]

"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.

[Eric Seidel (no email)]

Comment on attachment 34282 [details]
Rename functions to match webkit style

LGTM.  Thank you for your patience and iteration!

[Adam Barth]

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.
http://trac.webkit.org/changeset/46931

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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.