Bug 103856 – ENABLE(IMAGE_DECODER_DOWN_SAMPLING): Don't swizzle decode down sampled images

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

[noel gordon]

Add a way to query the JPEG decoder down scaling member

[noel gordon]

Created attachment 177183 [details]
Patch

[Kent Tamura]

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?

[noel gordon]

The is preparation fot the thing we'll do next, once Jaehun and Young Li come online tomorrow.

[noel gordon]

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.

[noel gordon]

Adding jaehun and yoli, per https://bugs.webkit.org/show_bug.cgi?id=98878#c10

[noel gordon]

Created attachment 177256 [details]
Patch

[noel gordon]

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.

[Yong Li]

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?

[noel gordon]

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

[Yong Li]

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

[Yong Li]

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?

[Yong Li]

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.

[Jaehun Lim]

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

[noel gordon]

(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

[noel gordon]

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

[noel gordon]

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

[noel gordon]

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

[noel gordon]

Created attachment 177407 [details]
Patch

[Yong Li]

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;
....

[noel gordon]

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

[noel gordon]

> 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 :/

[noel gordon]

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

[noel gordon]

ahem ... the compiler doesn't really call it :)

[Yong Li]

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

[Yong Li]

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.

[WebKit Review Bot]

Comment on attachment 177407 [details]
Patch

Clearing flags on attachment: 177407

Committed r136693: <http://trac.webkit.org/changeset/136693>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(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