Bug 69622 – Enable color profiles in JPEG, unless profile is grayscale

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Cary Clark]

Enable color profiles, unless profile is grayscale

[Cary Clark]

Created attachment 110137 [details]
Patch

[Avi Drissman]

drive-by: typo in comment on line 105

[Cary Clark]

Created attachment 110147 [details]
Patch

[Cary Clark]

fixed commit. Thanks Avi!

[Adam Barth]

Comment on attachment 110147 [details]
Patch

Test?  Do you have a YCbCr image we can use as a pixel test?

[Cary Clark]

The image referenced by fast/images/gray-scale-jpeg-with-color-profile.html is a YCbCr image with a gray color profile. It triggers the new code path.

[Adam Barth]

> The image referenced by fast/images/gray-scale-jpeg-with-color-profile.html is a YCbCr image with a gray color profile. It triggers the new code path.

Does it's behavior change with this patch?  We'd like a test that shows how this patch changes our behavior.  In this case, I'd expect that we'd need a YCbCr image with a non-gray color profile.

[Cary Clark]

Created attachment 110184 [details]
Patch

[Cary Clark]

Adam: Let me know if the added test is what you had in mind. Thanks

[Adam Barth]

Comment on attachment 110184 [details]
Patch

Looks great.  Thanks.

[Adam Barth]

The commit-queue is going to reject this patch without an expected PNG.  We can land it manually if you can't generate the needed PNG yourself.

[Cary Clark]

I'm a newbie at layout tests so I didn't know. I was curious about this because the CL I copied this from (yours, https://bugs.webkit.org/show_bug.cgi?id=49950 ) didn't include PNG. How did that get landed?

[Adam Barth]

Manually.  :)

[Cary Clark]

Committed r96970: <http://trac.webkit.org/changeset/96970>

[Hironori Bono]

Greetings,

(In reply to comment #14)
> Committed r96970: <http://trac.webkit.org/changeset/96970>

It seems this change enables color profiles even when USE_ICCJPEG is not defined. (Win Chrome does not define USE_ICCJPEG.)
As written in Issue 99936 <http://crbug.com/99936>, we see crashes in decoding a JPEG file on Windows and I suspect this change has some connection with them. Is it OK to move 'case JCS_YCbCr:' line only when #if use(ICCJPEG) is true so Win Chrome disables color profiles?

Regards,

Hironori Bono

[Cary Clark]

You may be right: the crash may be related to the change, but it isn't obvious to me how this change causes the jpeg decoder to crash. If rearranging the lines avoid the crash, then that seems like the right thing to do, but I'd like to understand better what is causing the crash in the first place.

Eventually, we'd like to enable color profiles for Windows as well. IE9/10 already has this support.

[Hironori Bono]

(In reply to comment #16)
> You may be right: the crash may be related to the change, but it isn't obvious to me how this change causes the jpeg decoder to crash. If rearranging the lines avoid the crash, then that seems like the right thing to do, but I'd like to understand better what is causing the crash in the first place.
> Eventually, we'd like to enable color profiles for Windows as well. IE9/10 already has this support.

Sorry for my confusing description. I do not have any objections for enabling color profiles on Windows. I just would have liked to write this change should work with or without defining USE_ICCJPEG. If I understand this file correctly, readColorProfile() reads ICC profiles only when we set USE_ICCJPEG. When we do not define USE_ICCJPEG, JPEGDeImageReader sets an empty color profile returned from the function. Even though it should be safe to set an empty color profile, I would like to try moving the 'case YCbCr' line to the original position if we do not define USE_ICCJPEG as listed below to see it fixes this crash.

#if !USE(ICCJPEG)
   case JCS_YCbCR:
#endif
   m_decoder->setIgnoreGammaAndColorProfile(true);
#if USE(ICCJPEG)
   case JCS_YCbCr:
#endif

Regards,

Hironori Bono

[Cary Clark]

Hironori: Your proposed fix sounds fine.