| Summary: | WebKit cannot decode jpegs that are in CMYK or YCCK format | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nico Weber <thakis> | ||||||||||||
| Component: | Images | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | abarth, ap, mitz | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Linux | ||||||||||||||
| URL: | http://picrevisions.s3.cdn.pmnw.net/pictures/4a59207b3cfd3/1/thumb_604.jpg | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Nico Weber
2009-07-31 19:14:46 PDT
Created attachment 33922 [details]
Fix.
This is http://code.google.com/p/chromium/issues/detail?id=15785 in chromium by the way. Comment on attachment 33922 [details]
Fix.
Test cases?
Hey, quick. Awesome. How are image decoders usually tested? Can you point me to an example test? 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 on attachment 33922 [details]
Fix.
Style issues (please see check-webkit-style script) and missing EMAIL_ADDRESS.
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 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? Created attachment 33957 [details]
Remove stupid comment
Heh. I thought I had removed that comment before creating the patch. My apologies.
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. Created attachment 34250 [details]
Fix style problems, extract rgb_convert_rgba() function
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.
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?).
+ // 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. 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.
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. 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 ;-) 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.
"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 on attachment 34282 [details]
Rename functions to match webkit style
LGTM. Thank you for your patience and iteration!
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 All reviewed patches have been landed. Closing bug. 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. |
