Bug 53250 – [chromium] JPEG corruption

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

[Tony Gentilcore]

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.

[Hironori Bono]

(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

[Tony Gentilcore]

Adam, is it possible this is a ICCJPEG bug?

http://trac.webkit.org/changeset/71311/

[Adam Barth]

Does this occur on platforms other than Mac?

[Tony Gentilcore]

(In reply to comment #3)
> Does this occur on platforms other than Mac?

I've only experienced it on mac.

[Adam Barth]

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.

[Hironori Bono]

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

[Adam Barth]

Yes.  That's probably the most expedient solution.

[Hironori Bono]

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

[Mihai Parparita]

Created attachment 81065 [details]
CMYK JPEG

[Mihai Parparita]

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

[Hironori Bono]

Created attachment 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

[Adam Barth]

Comment on attachment 81194 [details]
A copy-and-paste fix

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.

[Hironori Bono]

Created attachment 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

[Mihai Parparita]

Comment on attachment 81194 [details]
A copy-and-paste fix

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

[Adam Barth]

Comment on attachment 81196 [details]
A copy-and-paste fix 2

Looks good modulo Mihai's comments.

[Adam Barth]

Created attachment 81332 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 81332 [details]
Patch for landing

Clearing flags on attachment: 81332

Committed r77724: <http://trac.webkit.org/changeset/77724>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Hironori Bono]

Greetings Mihai and Adam,

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

Regards,

Hironori Bono