Bug 53250

Summary: [chromium] JPEG corruption
Product: WebKit Reporter: Tony Gentilcore <tonyg@chromium.org>
Component: ImagesAssignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth@webkit.org, commit-queue@webkit.org, hbono@chromium.org, mihaip@chromium.org, ojan@chromium.org
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Mac OS X 10.5   
URL: http://cdn.techi.com/wp-content/uploads/2011/01/Google-Censorship-Graphic-01.jpg
Bug Depends on:    
Bug Blocks: 74400    
Attachments:
Description Flags
CMYK JPEG
none
A copy-and-paste fix
abarth: review+, abarth: commit‑queue-
A copy-and-paste fix 2
none
Patch for landing none

Description From 2011-01-27 11:01:16 PST
1. Load http://cdn.techi.com/wp-content/uploads/2011/01/Google-Censorship-Graphic-01.jpg in Chrome 10.0.642.2 or above and notice the JPEG is corrupt.
2. Load http://cdn.techi.com/wp-content/uploads/2011/01/Google-Censorship-Graphic-01.jpg in Chrome 8.0.552.237 or below (or any other browser) and notice it displays properly.

http://trac.webkit.org/changeset/73736 seems like a likely candidate.
------- Comment #1 From 2011-01-27 21:10:08 PST -------
(In reply to comment #0)
> 1. Load http://cdn.techi.com/wp-content/uploads/2011/01/Google-Censorship-Graphic-01.jpg in Chrome 10.0.642.2 or above and notice the JPEG is corrupt.
> 2. Load http://cdn.techi.com/wp-content/uploads/2011/01/Google-Censorship-Graphic-01.jpg in Chrome 8.0.552.237 or below (or any other browser) and notice it displays properly.
> 
> http://trac.webkit.org/changeset/73736 seems like a likely candidate.

In brief, libjpeg-turbo is not enabled on Chromium or Chrome by default because it causes a build break on Mac. (We can use it only when we explicitly set the 'use_libjpeg_turbo' value to 1 in '~/.gyp/include.gypi'.)

By the way, to test the attached JPEG image with our continuous builds, r65032 (WebKit r71308) (*2) can show it without problems but r65179 (WebKit r71368) (*2) cannot. So, I assume a change from WebKit r71308 to WebKit r71368 caused this issue.

(*1) <http://build.chromium.org/f/chromium/continuous/mac/2010-11-03/65032/chrome-mac.zip>
(*2) <http://build.chromium.org/f/chromium/continuous/mac/2010-11-04/65179/chrome-mac.zip>

Regards,

Hironori Bono
------- Comment #2 From 2011-01-28 08:45:43 PST -------
Adam, is it possible this is a ICCJPEG bug?

http://trac.webkit.org/changeset/71311/
------- Comment #3 From 2011-01-28 11:37:19 PST -------
Does this occur on platforms other than Mac?
------- Comment #4 From 2011-01-28 11:38:35 PST -------
(In reply to comment #3)
> Does this occur on platforms other than Mac?

I've only experienced it on mac.
------- Comment #5 From 2011-01-28 11:47:11 PST -------
It looks similar to problems we had with PNGs.  In that case, we were trying to apply some image transformation (Grayscale => RGB) twice.  Should be easy to narrow down by commenting out code.
------- Comment #6 From 2011-02-02 00:34:23 PST -------
Greetings Adam,

(In reply to comment #5)
> It looks similar to problems we had with PNGs.  In that case, we were trying to apply some image transformation (Grayscale => RGB) twice.  Should be easy to narrow down by commenting out code.

Yes. This JPEG is a CMYK JPEG and JPEGDecoder applies CMYK->RGB conversion. (Unfortunately, I'm not sore who uses the ColorSpace object and applies another color conversion.) Is it OK to call 'ImageDecoder::setIgnoreGammaAndColorProfile(true)' also in this case as you did for Bug 49950 <http://webkit.org/b/49950>?

Regards,

Hironori Bono
------- Comment #7 From 2011-02-02 13:27:34 PST -------
Yes.  That's probably the most expedient solution.
------- Comment #8 From 2011-02-03 01:23:15 PST -------
Greetings,

(In reply to comment #7)
> Yes.  That's probably the most expedient solution.

Thanks you for your advice. Even though I have implemented a fix, I cannot upload it because this 'Google-Censorship-Graphic-01.jpg' (used as a layout test) is too huge. Even though I will try creating a small CMYK JPEG at home (where I can use Photoshop), it would be definitely helpful to give me small CMYK JPEGs.

Regards,

Hironori Bono
------- Comment #9 From 2011-02-03 08:35:39 PST -------
Created an attachment (id=81065) [details]
CMYK JPEG
------- Comment #10 From 2011-02-03 08:36:07 PST -------
(In reply to comment #8)
> it would be definitely helpful to give me small CMYK JPEGs.

I've attached a smaller image (it should be green).
------- Comment #11 From 2011-02-04 00:08:44 PST -------
Created an attachment (id=81194) [details]
A copy-and-paste fix

Greetings Mihai,

(In reply to comment #10)
> I've attached a smaller image (it should be green).

Thank you so much for your test file. I have integrated your JPEG to my layout test.

Regards,

Hironori Bono
------- Comment #12 From 2011-02-04 00:16:03 PST -------
(From update of attachment 81194 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=81194&action=review

Nice test!

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:226
> +                // Same as graysclale images, we convert CMYK images to RGBA

graysclale => grayscale.  Maybe "Same as with grayscale images" ?

> LayoutTests/fast/images/cmyk-jpeg-with-color-profile.html:19
>  Added: svn:eol-style
>    + LF

We probably don't need this eol-style.
------- Comment #13 From 2011-02-04 01:10:08 PST -------
Created an attachment (id=81196) [details]
A copy-and-paste fix 2

(In reply to comment #12)

Thank you for your quick review. I have updated this change to apply your comments and to clean up in my test script.

> > +                // Same as graysclale images, we convert CMYK images to RGBA
> graysclale => grayscale.  Maybe "Same as with grayscale images" ?

Yes, it seems I have too focused on writing a test script to check my English. :)

> > LayoutTests/fast/images/cmyk-jpeg-with-color-profile.html:19
> >  Added: svn:eol-style
> >    + LF
> 
> We probably don't need this eol-style.

Thank you for noticing it. I have removed these properties from the newly-added files.

Regards,

Hironori Bono
------- Comment #14 From 2011-02-04 07:49:48 PST -------
(From update of attachment 81194 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=81194&action=review

> LayoutTests/fast/images/cmyk-jpeg-with-color-profile.html:10
> +<canvas id="canvas1" width="64" height="64"></canvas>

If you create the canvas in the script, do you need to include it in the markup?

> LayoutTests/fast/images/script-tests/cmyk-jpeg-with-color-profile.js:12
> +canvas.id = 'canvas1';

Don't need the ID.

> LayoutTests/fast/images/script-tests/cmyk-jpeg-with-color-profile.js:24
> +    // Read the pixels in the canvas and calcualte their avarage values.

Typo ("calcualte").
------- Comment #15 From 2011-02-04 09:51:39 PST -------
(From update of attachment 81196 [details])
Looks good modulo Mihai's comments.
------- Comment #16 From 2011-02-04 18:09:18 PST -------
Created an attachment (id=81332) [details]
Patch for landing
------- Comment #17 From 2011-02-04 19:06:04 PST -------
(From update of attachment 81332 [details])
Clearing flags on attachment: 81332

Committed r77724: <http://trac.webkit.org/changeset/77724>
------- Comment #18 From 2011-02-04 19:06:14 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #19 From 2011-02-07 03:30:21 PST -------
Greetings Mihai and Adam,

Thank you so much for fixing my change and landing it.

Regards,

Hironori Bono