Bug 103856

Summary: ENABLE(IMAGE_DECODER_DOWN_SAMPLING): Don't swizzle decode down sampled images
Product: WebKit Reporter: noel gordon <noel.gordon>
Component: New BugsAssignee: noel gordon <noel.gordon>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, ljaehun.lim, tkent, webkit.review.bot, yong.li.webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 98878    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description noel gordon 2012-12-02 22:44:21 PST
Add a way to query the JPEG decoder down scaling member
Comment 1 noel gordon 2012-12-02 22:49:25 PST
Created attachment 177183 [details]
Patch
Comment 2 Kent Tamura 2012-12-02 23:01:11 PST
Comment on attachment 177183 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177183&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests, no change in behavior.

Yeah, this patch looks to change no behavior.  So, why you need this change now?
I guess you will do something later.  Why don't you do this simple change when you really need it?
Comment 3 noel gordon 2012-12-02 23:21:55 PST
The is preparation fot the thing we'll do next, once Jaehun and Young Li come online tomorrow.
Comment 4 noel gordon 2012-12-03 07:39:21 PST
Tapping my fingers ...

(In reply to comment #2)

> I guess you will do something later.  Why don't you do this simple change when you really need it?

Here you go, the whole simple change.
Comment 5 noel gordon 2012-12-03 07:49:00 PST
Adding jaehun and yoli, per https://bugs.webkit.org/show_bug.cgi?id=98878#c10
Comment 6 noel gordon 2012-12-03 07:51:39 PST
Created attachment 177256 [details]
Patch
Comment 7 noel gordon 2012-12-03 07:55:02 PST
jaehun and yoli: your ports use ENABLE(IMAGE_DECODER_DOWN_SAMPLING) and it has no tests.  Could you please check this change works for you.
Comment 8 Yong Li 2012-12-03 07:56:36 PST
Comment on attachment 177256 [details]
Patch

Wow, I have a very similar patch. If this is done, then (!m_scaled) check in my previous patch isn't needed. But wouldn't copying from 4-byte to 4-byte still faster than copying from 3-byte to 4-byte?
Comment 9 noel gordon 2012-12-03 08:05:12 PST
(In reply to comment #8)
> (From update of attachment 177256 [details])
> If this is done, then (!m_scaled) check in my previous patch isn't needed.

It is still needed: unscaled images will/should still be turbo swizzled, right?

Here we are dealing with the scaled case only.
 
> But wouldn't copying from 4-byte to 4-byte still faster than copying from 3-byte to 4-byte?

Maybe, maybe not. One SSE instruction moves 8 bytes as fast as it does one byte, I believe.  I have nor real test harness for IMAGE_DECODER_DOWN_SAMPLING.  Not sure how you guys test it.
Comment 10 Yong Li 2012-12-03 08:20:07 PST
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 177256 [details] [details])
> > If this is done, then (!m_scaled) check in my previous patch isn't needed.
> 
> It is still needed: unscaled images will/should still be turbo swizzled, right?

Oops. you are right

> > But wouldn't copying from 4-byte to 4-byte still faster than copying from 3-byte to 4-byte?
> 
> Maybe, maybe not. One SSE instruction moves 8 bytes as fast as it does one byte, I believe.  I have nor real test harness for IMAGE_DECODER_DOWN_SAMPLING.  Not sure how you guys test it.

I'll try to get some time this week to test it. jaehun, are you going to test it?

As it doesn't fix any currently-known issue, I will r- the patch, until we get some performance numbers that show this patch helps.
Comment 11 Yong Li 2012-12-03 08:24:02 PST
Comment on attachment 177256 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177256&action=review

r- until we get perf numbers showing this helps.

> Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:397
> +#if ENABLE(IMAGE_DECODER_DOWN_SAMPLING) && defined(TURBO_JPEG_RGB_SWIZZLE)
> +            // There's no point swizzle decoding if image down sampling will
> +            // be applied. Revert to using JSC_RGB in that case.
> +            if (m_decoder->willDownSample() && turboSwizzled(m_info.out_color_space))
> +                m_info.out_color_space = JCS_RGB;
> +#endif

What will happen if jpeg_color_space is JCS_CMYK? I would move this code into:

359359 switch (m_info.jpeg_color_space) {
360360 case JCS_GRAYSCALE:
361361 case JCS_RGB:
362362 case JCS_YCbCr:
363363 // libjpeg can convert GRAYSCALE and YCbCr image pixels to RGB.
364364 m_info.out_color_space = rgbOutputColorSpace();
365365#if defined(TURBO_JPEG_RGB_SWIZZLE)
366366 if (m_info.saw_JFIF_marker)
367367 break;
368368 // FIXME: Swizzle decoding does not support Adobe transform=0
369369 // images (yet), so revert to using JSC_RGB in that case.
370370 if (m_info.saw_Adobe_marker && !m_info.Adobe_transform)
371371 m_info.out_color_space = JCS_RGB;
372372#endif
373373 break;

And move the switch block after setSize(). What do you think?
Comment 12 Yong Li 2012-12-03 11:11:12 PST
I did a quick test upon a random large jpeg. I turned on down sampling and set the limit to 128K pixels, and made the decoder decode only when all data arrives. I cannot tell any diff between the results with and w/o the patch. I'm not against of the change, though, if the JCS_CMYK thing is fixed.
Comment 13 Jaehun Lim 2012-12-03 16:10:40 PST
(In reply to comment #7)
> jaehun and yoli: your ports use ENABLE(IMAGE_DECODER_DOWN_SAMPLING) and it has no tests.  Could you please check this change works for you.

This patch works good for me. I checked with some test cases those had failed before my patch.
Comment 14 noel gordon 2012-12-03 20:04:45 PST
(In reply to comment #11)

> > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:397
> > +#if ENABLE(IMAGE_DECODER_DOWN_SAMPLING) && defined(TURBO_JPEG_RGB_SWIZZLE)
> > +            // There's no point swizzle decoding if image down sampling will
> > +            // be applied. Revert to using JSC_RGB in that case.
> > +            if (m_decoder->willDownSample() && turboSwizzled(m_info.out_color_space))
> > +                m_info.out_color_space = JCS_RGB;
> > +#endif
> 
> What will happen if jpeg_color_space is JCS_CMYK?

We don't (and can't) swizzle decode JCS_CMYK, see http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp?rev=136410#L68
Comment 15 noel gordon 2012-12-03 20:14:10 PST
(In reply to comment #11)

> And move the switch block after setSize(). What do you think?

I don't see why, and it would be the subject of another bug.
Comment 16 noel gordon 2012-12-03 20:18:44 PST
(In reply to comment #12)
> I did a quick test upon a random large jpeg. I turned on down sampling and set the limit to 128K pixels, and made the decoder decode only when all data arrives. I cannot tell any diff between the results with and w/o the patch.

Yes thank you, that is the expected result.  For scaled images, we must pass the data through buffer.setRGBA slow path.  Swizzle decoding is fast since it avoids that slow path.

> I'm not against of the change, though, if the JCS_CMYK thing is fixed.

The JCS_CMYK does not need fixing.
Comment 17 noel gordon 2012-12-03 20:19:56 PST
(In reply to comment #13)
> > jaehun and yoli: your ports use ENABLE(IMAGE_DECODER_DOWN_SAMPLING) and it has no tests.  Could you please check this change works for you.
> 
> This patch works good for me. I checked with some test cases those had failed before my patch.

Thank you for checking.
Comment 18 noel gordon 2012-12-03 21:01:54 PST
Created attachment 177407 [details]
Patch
Comment 19 Yong Li 2012-12-04 07:36:55 PST
I see JCS_CMYK isn't a problem with your patch, as you check turboSwizzled(m_info.out_color_space) before setting out_color_space to RGB.


(In reply to comment #15)
> (In reply to comment #11)
> 
> > And move the switch block after setSize(). What do you think?
> 
> I don't see why, and it would be the subject of another bug.

Wouldn't it be much neater to put all the changes to m_info.out_color_space together in one block?

In that way, it saves a call to turboSwizzled() and the code can be merged to existing #if defined(TURBO_JPEG_RGB_SWIZZLE) block.

switch (m_info.jpeg_color_space) {
case JCS_GRAYSCALE:
case JCS_RGB:
case JCS_YCbCr:
     // libjpeg can convert GRAYSCALE and YCbCr image pixels to RGB.
     m_info.out_color_space = rgbOutputColorSpace();
#if defined(TURBO_JPEG_RGB_SWIZZLE)
#if ENABLE(IMAGE_DECODER_DOWN_SAMPLING)
     // There's no point swizzle decoding if image down sampling will
     // be applied. Revert to using JSC_RGB in that case.
     if (m_decoder->willDownSample()) {
         m_info.out_color_space = JCS_RGB;
         break;
#endif
    if (m_info.saw_JFIF_marker)
       break;
....
Comment 20 noel gordon 2012-12-04 15:06:41 PST
(In reply to comment #19)
> I see JCS_CMYK isn't a problem with your patch, as you check turboSwizzled(m_info.out_color_space) before setting out_color_space to RGB.

Exactly.
Comment 21 noel gordon 2012-12-04 15:16:25 PST
> Wouldn't it be much neater to put all the changes to m_info.out_color_space together in one block?

Note there's also a block following the switch that deals with color profiles and m_info.out_color_space.  My goal was to keep these areas of the code together.  We could move the color profile handling into the switch statement too, but neatness as a virtue would be gone :/
Comment 22 noel gordon 2012-12-04 15:17:45 PST
(In reply to comment #19)
 
> In that way, it saves a call to turboSwizzled() and the code can be merged to existing #if defined(TURBO_JPEG_RGB_SWIZZLE) block.

turboSwizzled() is inline, the compile doesn't really call it :)
Comment 23 noel gordon 2012-12-04 15:19:26 PST
ahem ... the compiler doesn't really call it :)
Comment 24 Yong Li 2012-12-05 08:26:10 PST
(In reply to comment #22)
> (In reply to comment #19)
> 
> > In that way, it saves a call to turboSwizzled() and the code can be merged to existing #if defined(TURBO_JPEG_RGB_SWIZZLE) block.
> 
> turboSwizzled() is inline, the compile doesn't really call it :)

I know. but for neatness :)
Comment 25 Yong Li 2012-12-05 08:29:21 PST
Comment on attachment 177407 [details]
Patch

r+. I don't see it helps performance, but at least it cleans some code in outputScanline. Also Jaehun has seen this fixed some tests. See Comment #13 From Jaehun Lim.
Comment 26 WebKit Review Bot 2012-12-05 08:37:14 PST
Comment on attachment 177407 [details]
Patch

Clearing flags on attachment: 177407

Committed r136693: <http://trac.webkit.org/changeset/136693>
Comment 27 WebKit Review Bot 2012-12-05 08:37:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 noel gordon 2012-12-05 17:56:25 PST
(In reply to comment #25)
> r+. I don't see it helps performance, but at least it cleans some code in outputScanline. Also Jaehun has seen this fixed some tests. See Comment #13 From Jaehun Lim.

Thank you.  And yes, no change in performance, and some cleanup to avoid confusion about libjpeg-turbo use in outputScanlines() per bug 98878#c9