Add a way to query the JPEG decoder down scaling member
Created attachment 177183 [details] Patch
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?
The is preparation fot the thing we'll do next, once Jaehun and Young Li come online tomorrow.
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.
Adding jaehun and yoli, per https://bugs.webkit.org/show_bug.cgi?id=98878#c10
Created attachment 177256 [details] Patch
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 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?
(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.
(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 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?
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.
(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.
(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
(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.
(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.
(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.
Created attachment 177407 [details] Patch
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; ....
(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.
> 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 :/
(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 :)
ahem ... the compiler doesn't really call it :)
(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 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 on attachment 177407 [details] Patch Clearing flags on attachment: 177407 Committed r136693: <http://trac.webkit.org/changeset/136693>
All reviewed patches have been landed. Closing bug.
(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