Bug 98878

Summary:

Fix JPEG turbo decoding failure when IMAGE_DECODER_DOWN_SAMPLING is enabled

Product:

WebKit

Reporter:

Jaehun Lim <ljaehun.lim>

Component:

Images

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

kbr, noel.gordon, webkit.review.bot, yong.li.webkit

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Bug Depends on:

Bug Blocks:

103749, 103856

Attachments:

Description	Flags
Patch	none
Patch	none

Description Jaehun Lim 2012-10-10 02:43:02 PDT

When using libjpeg-turbo and enabling IMAGE_DECODER_DOWN_SAMPLING,
JPEG decoding failed because of no support for JCS_EXT_RGBA, JCS_EXT_BGRA.
Set RGBA values when color space is JCS_EXT_RGBA or JCS_EXT_BGRA.

Comment 1 Jaehun Lim 2012-10-10 02:43:37 PDT

Created attachment 167962 [details]
Patch

Comment 2 WebKit Review Bot 2012-10-10 02:45:08 PDT

Attachment 167962 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 3 Jaehun Lim 2012-10-10 02:46:46 PDT

Created attachment 167963 [details]
Patch

Comment 4 Kenneth Russell 2012-10-10 10:59:07 PDT

This looks fine, but I'd like Noel to do an unofficial review before r+'ing it.

Comment 5 noel gordon 2012-10-11 07:21:06 PDT

Looks sane.  Sadly no tests exist for the IMAGE_DECODER_DOWN_SAMPLING enable.

Comment 6 Kenneth Russell 2012-10-11 09:33:36 PDT

Comment on attachment 167963 [details]
Patch

That's unfortunate, but I assume having this work is better than not. r=me

Comment 7 WebKit Review Bot 2012-10-11 09:45:33 PDT

Comment on attachment 167963 [details]
Patch

Clearing flags on attachment: 167963

Committed r131075: <http://trac.webkit.org/changeset/131075>

Comment 8 WebKit Review Bot 2012-10-11 09:45:36 PDT

All reviewed patches have been landed.  Closing bug.

Comment 9 noel gordon 2012-11-28 16:45:40 PST

Someone asked me a question about this.  @jaehun - should the #if test be as follows?

   #if ENABLE(IMAGE_DECODER_DOWN_SAMPLING) && defined(TURBO_JPEG_RGB_SWIZZLE)

Alternatively, we could remove all this and ensure that ports that use IMAGE_DECODER_DOWN_SAMPLING can use libjoeg-turbo correctly.   TURBO_JPEG_RGB_SWIZZLE should not be taken to mean libjpeg-turbo and I think we should fix that.  Thoughts?

Comment 10 Jaehun Lim 2012-11-28 17:50:37 PST

(In reply to comment #9)
> Someone asked me a question about this.  @jaehun - should the #if test be as follows?
> 
>    #if ENABLE(IMAGE_DECODER_DOWN_SAMPLING) && defined(TURBO_JPEG_RGB_SWIZZLE)
>
I considered the same #if statement at first. I have no objection to change #if statement.

> Alternatively, we could remove all this and ensure that ports that use IMAGE_DECODER_DOWN_SAMPLING can use libjoeg-turbo correctly.   TURBO_JPEG_RGB_SWIZZLE should not be taken to mean libjpeg-turbo and I think we should fix that.  Thoughts?

I perfectly agree with you.

Comment 11 Yong Li 2012-11-30 08:09:39 PST

Just noticed this fix. Nice job. For those images are not scaled when DOWNSAMPLE is enabled, it seems that we can still use the swizzling path?

Comment 12 Yong Li 2012-11-30 10:46:15 PST

(In reply to comment #11)
> Just noticed this fix. Nice job. For those images are not scaled when DOWNSAMPLE is enabled, it seems that we can still use the swizzling path?

Bug 103749 created for this, and patch is uploaded

Comment 13 noel gordon 2012-12-02 22:13:40 PST

I have reviewed that patch, looks good.

Second thing to do is to handle the scaled case better per comment 10.