Bug 59670

Summary: [chromium] Use data decoding swizzle for turbo JPEG image decoding
Product: WebKit Reporter: Hironori Bono <hbono>
Component: ImagesAssignee: noel gordon <noel.gordon>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, jbauman, kbr, noel.gordon, pkasting, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 72234, 72240, 72321, 73284, 74286, 75189, 75861, 79457    
Bug Blocks: 78323    
Attachments:
Description Flags
A quick fix v0
none
Patch
none
Regression: red pixel noise, white swathe errors
none
Patch - Don't swizzle grayscale JPEG images.
none
Patch - Don't swizzle grayscale JPEG images.
none
Patch
none
Patch
none
Patch none

Hironori Bono
Reported 2011-04-27 20:41:41 PDT
Greetings, JPEGImageDecoder::outputScanlines() copies row pixels returned by jpeg_read_scanlines() calls from a vector to a ImageFrame because libjpeg cannot output RGBA pixels used by WebKit. It improves the performance of decoding JPEG data to remove these copies when using libjpeg-turbo because it can output RGBA pixels used by WebKit. (It improves the rendering performance of JPEG images by >30%, especially huge ones as written in the following test results.) 1. Before removing memory copies Before removing memory copies, it takes ~50s for our page_cycler_tests to render test images in <http://www.imagecompression.info/test_images/>. Note: Google Test filter = PageCyclerTest.JPEGFile [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from PageCyclerTest [ RUN ] PageCyclerTest.JPEGFile *RESULT vm_size_final_b: vm_size_f_b= 417281 bytes *RESULT vm_rss_final_b: vm_rss_f_b= 15 bytes *RESULT vm_size_final_r: vm_size_f_r= 417281 bytes *RESULT vm_rss_final_r: vm_rss_f_r= 15 bytes *RESULT vm_size_final_t: vm_size_f_t= 834562 bytes *RESULT vm_rss_final_t: vm_rss_f_t= 30 bytes RESULT processes: proc_= 2 RESULT read_op_b: r_op_b= 0 RESULT write_op_b: w_op_b= 0 RESULT other_op_b: o_op_b= 0 *RESULT total_op_b: IO_op_b= 0 RESULT read_byte_b: r_b= 0 kb RESULT write_byte_b: w_b= 0 kb RESULT other_byte_b: o_b= 0 kb *RESULT total_byte_b: IO_b= 0 kb RESULT read_op_r: r_op_r= 0 RESULT write_op_r: w_op_r= 0 RESULT other_op_r: o_op_r= 0 *RESULT total_op_r: IO_op_r= 0 RESULT read_byte_r: r_r= 0 kb RESULT write_byte_r: w_r= 0 kb RESULT other_byte_r: o_r= 0 kb *RESULT total_byte_r: IO_r= 0 kb RESULT commit_charge: cc= 78680 kb Pages: [artificial,big_building,big_tree,bridge,cathedral,deer,fireworks,flower_foveon,hdr,leaves_iso_1600,leaves_iso_200,nightshot_iso_100,nightshot_iso_1600,spider_web] *RESULT times: t= [110,27,43,38,11,12,11,9,10,15,53,12,13,11,10,23,43,39,11,12,10,10,10,14,13,12,13,10,10,23,43,37,10,12,10,9,11,14,14,12,13,11,9,23,44,38,12,12,10,9,11,15,12,12,13,13,11,23,44,39,11,12,10,9,10,14,14,12,13,14,10,22,43,38,11,13,10,9,10,14,139,403,13,11,11,23,44,38,11,12,10,9,11,14,13,15,12,11,9,23,44,39,12,12,10,9,10,14,13,11,12,11,11,23,43,38,12,12,10,9,10,14,14,11,13,11,11,22,43,38,11,12,10,9,10,14,13,12,12,12] ms [ OK ] PageCyclerTest.JPEGFile (49039 ms) [----------] 1 test from PageCyclerTest (49040 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (49040 ms total) [ PASSED ] 1 test. YOU HAVE 15 FLAKY TESTS 2. After removing memory copies After removing memory copies, it takes ~30s for our page_cycler_tests to render the same test images. Note: Google Test filter = PageCyclerTest.JPEGFile [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from PageCyclerTest [ RUN ] PageCyclerTest.JPEGFile *RESULT vm_size_final_b: vm_size_f_b= 416961 bytes *RESULT vm_rss_final_b: vm_rss_f_b= 15 bytes *RESULT vm_size_final_r: vm_size_f_r= 416961 bytes *RESULT vm_rss_final_r: vm_rss_f_r= 15 bytes *RESULT vm_size_final_t: vm_size_f_t= 833922 bytes *RESULT vm_rss_final_t: vm_rss_f_t= 30 bytes RESULT processes: proc_= 2 RESULT read_op_b: r_op_b= 0 RESULT write_op_b: w_op_b= 0 RESULT other_op_b: o_op_b= 0 *RESULT total_op_b: IO_op_b= 0 RESULT read_byte_b: r_b= 0 kb RESULT write_byte_b: w_b= 0 kb RESULT other_byte_b: o_b= 0 kb *RESULT total_byte_b: IO_b= 0 kb RESULT read_op_r: r_op_r= 0 RESULT write_op_r: w_op_r= 0 RESULT other_op_r: o_op_r= 0 *RESULT total_op_r: IO_op_r= 0 RESULT read_byte_r: r_r= 0 kb RESULT write_byte_r: w_r= 0 kb RESULT other_byte_r: o_r= 0 kb *RESULT total_byte_r: IO_r= 0 kb RESULT commit_charge: cc= 78116 kb Pages: [artificial,big_building,big_tree,bridge,cathedral,deer,fireworks,flower_foveon,hdr,leaves_iso_1600,leaves_iso_200,nightshot_iso_100,nightshot_iso_1600,spider_web] *RESULT times: t= [55,26,19,13,11,12,10,9,10,14,13,12,12,11,10,23,19,13,11,12,12,13,142,14,12,12,12,12,10,23,47,13,11,12,10,9,10,14,41,16,12,10,10,24,45,14,11,12,10,9,10,14,12,12,12,11,10,24,47,14,11,12,10,9,10,14,13,12,13,11,10,23,44,13,11,13,11,9,10,14,12,12,13,11,10,23,45,14,11,12,10,9,11,14,13,12,13,11,10,24,44,14,11,13,10,10,11,14,13,11,12,18,52,23,44,14,12,14,10,10,11,14,12,12,17,11,11,22,45,14,12,13,10,9,10,14,13,11,13,11] ms [ OK ] PageCyclerTest.JPEGFile (30283 ms) [----------] 1 test from PageCyclerTest (30283 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (30283 ms total) [ PASSED ] 1 test. YOU HAVE 15 FLAKY TESTS Regards, Hironori Bono
Attachments
A quick fix v0 (16.61 KB, patch)
2011-04-27 21:03 PDT, Hironori Bono 		no flags
DetailsFormatted DiffDiff
Patch (3.12 KB, patch)
2011-11-16 06:51 PST, noel gordon 		no flags
DetailsFormatted DiffDiff
Regression: red pixel noise, white swathe errors (165.01 KB, image/png)
2011-11-17 04:12 PST, noel gordon 		no flags
Details
Patch - Don't swizzle grayscale JPEG images. (3.66 KB, patch)
2011-11-17 04:53 PST, noel gordon 		no flags
DetailsFormatted DiffDiff
Patch - Don't swizzle grayscale JPEG images. (3.66 KB, patch)
2011-11-17 04:58 PST, noel gordon 		no flags
DetailsFormatted DiffDiff
Patch (3.81 KB, patch)
2011-11-18 00:37 PST, noel gordon 		no flags
DetailsFormatted DiffDiff
Patch (3.81 KB, patch)
2011-11-18 03:00 PST, noel gordon 		no flags
DetailsFormatted DiffDiff
Patch (4.02 KB, patch)
2011-11-25 00:13 PST, noel gordon 		no flags
DetailsFormatted DiffDiff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Hironori Bono
Comment 1 2011-04-27 21:03:44 PDT
Created attachment 91421 [details] A quick fix v0 Greetings, This is the quick fix used for measuring the performance. Regards, Hironori Bono
Hironori Bono
Comment 2 2011-04-27 21:30:56 PDT
Tamura-san, Is it possible to tell me the good reviewer for this change? Regards, Hironori Bono
Kent Tamura
Comment 3 2011-04-30 23:29:01 PDT
> Is it possible to tell me the good reviewer for this change? Adam, Eric, could you review this please?
Adam Barth
Comment 4 2011-04-30 23:48:11 PDT
The friend declaration seems wrong. Also I'd like pasting to take a look first...
Eric Seidel (no email)
Comment 5 2011-05-01 09:41:36 PDT
Comment on attachment 91421 [details] A quick fix v0 Yup. If Peter likes it, I'm happy to r+ it.
Peter Kasting
Comment 6 2011-05-02 09:45:01 PDT
Comment on attachment 91421 [details] A quick fix v0 View in context: https://bugs.webkit.org/attachment.cgi?id=91421&action=review > Source/WebCore/ChangeLog:9 > + to copy row pixels from a vector to a ImageFrame because libjpeg-turbo Nit: "a ImageFrame" -> "an ImageFrame" > Source/WebCore/ChangeLog:10 > + can output RBA pixels used by WebKit. This change writes the output Nit: RBA -> RGBA > Source/WebCore/ChangeLog:12 > + this memory copies. Nit: "this memory copies" -> "the extra copy" > Source/WebCore/platform/image-decoders/ImageDecoder.h:56 > + friend class JPEGImageDecoder; This isn't the right way to do this. I'm concerned about blindly writing into the buffer. Do all platforms guarantee the image format is 32bpp ARGB and that the stride is not larger than the actual line width? If so, you can probably just make getAddr() public. If not, you'll need a function which returns a pointer to the image data only if the above are met, and NULL otherwise. > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:220 > + m_info.out_color_space = JCS_EXT_BGRX; I believe this is only correct for little-endian machines. For big-endian I think you need XRGB, because libjpeg-turbo writes using byte offsets rather than bit shifts, so it doesn't auto-swap with machine endianness. (I'm saying this based on trying to decipher http://sourceforge.net/mailarchive/forum.php?thread_name=4D850B59.1050201%40users.sourceforge.net&forum_name=tigervnc-users ).
noel gordon
Comment 7 2011-11-13 20:08:13 PST
I think the three tests would be better combined into one test. Filed bug 72234 about it and posted a patch. Tamura-san could you review please?
noel gordon
Comment 8 2011-11-14 15:43:39 PST
Examining the history of ImageFrame suggests that it is in fact an abstraction of an RGBA32 pixel buffer. I think it's fine to make getAddr() public, filed bug 72321 with a patch.
noel gordon
Comment 9 2011-11-16 02:04:18 PST
Related tests: fast/images/rgb-jpeg-endian-pixels.html fast/images/cmyk-jpeg-with-color-profile.html fast/images/gray-scale-jpeg-with-color-profile.html Looking green now on the bots: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=rgb-jpeg-endian-pixels.html%2Ccmyk-jpeg-with-color-profile.html%2Cgray-scale-jpeg-with-color-profile.html&showExpectations=true&group=%40ToT%20-%20webkit.org
noel gordon
Comment 10 2011-11-16 06:31:09 PST
JCS_EXTENSIONS is only defined if libjpeg-turbo is compiled in and Chromium is the only port using libjpeg-turbo. Next the endianness question ... Chromium is designed for little endian machines only, reading from src/build/build-config.h. http://www.google.com/codesearch#OAMlx_jo-ck/src/build/build_config.h&type=cs&l=86 Brett Wilson made the same point in his recent reply about Chromium endianness on chrome-dev@. http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thread/c6c4a23a33c155f4?fwc=1&pli=1 Chrome Skia code states: "All little-endian Chrome platforms agree: BGRA is the optimal pixel layout." http://www.google.com/codesearch#OAMlx_jo-ck/src/skia/config/SkUserConfig.h&type=cs&l=184 so a BGRX swizzle makes sense to me. Another concern I had - the decoders m_scaled member. Talked to Peter off-line, it's enabled by flag ENABLE(IMAGE_DECODER_DOWN_SAMPLING). Chromium does not enable that flag. Next step: code all the above in a WIP patch.
noel gordon
Comment 11 2011-11-16 06:51:28 PST
Created attachment 115373 [details] Patch
noel gordon
Comment 12 2011-11-16 07:00:36 PST
Above is a WIP Patch, baking it in the queues.
WebKit Review Bot
Comment 13 2011-11-16 07:31:20 PST
Comment on attachment 115373 [details] Patch Attachment 115373 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10484787 New failing tests: tables/mozilla/bugs/bug29314.html tables/mozilla/bugs/bug13169.html tables/mozilla/bugs/bug10565.html tables/mozilla/bugs/bug11026.html fast/repaint/backgroundSizeRepaint.html fast/repaint/block-layout-inline-children-replaced.html fast/repaint/clipped-relative.html fast/repaint/selected-replaced.html tables/mozilla/bugs/bug12908-1.html
noel gordon
Comment 14 2011-11-17 04:09:13 PST
These are regressions, and I reproduced on chrome-linux/win32 locally. The test image resources were created by XV Version 3.10a Rev: 12/29/94 http://trac.webkit.org/browser/trunk/LayoutTests/tables/mozilla/images/ant.jpg http://trac.webkit.org/browser/trunk/LayoutTests/fast/repaint/resources/apple.jpg They might be more than 10 years old, but they work fine with libjpeg. The ant.jpg image shows spot red/white pixel noise, or swathes of while pixel errors.
noel gordon
Comment 15 2011-11-17 04:12:20 PST
Created attachment 115556 [details] Regression: red pixel noise, white swathe errors tables/mozilla/bugs/bug12908-1.html regression.
noel gordon
Comment 16 2011-11-17 04:38:53 PST
Seems it's a libjpeg-turbo bug, a workaround is to exclude JSC_GRAYSCALE images.
noel gordon
Comment 17 2011-11-17 04:53:59 PST
Created attachment 115564 [details] Patch - Don't swizzle grayscale JPEG images.
noel gordon
Comment 18 2011-11-17 04:58:57 PST
Created attachment 115565 [details] Patch - Don't swizzle grayscale JPEG images.
Hironori Bono
Comment 19 2011-11-17 09:54:30 PST
Greetings, (In reply to comment #16) > Seems it's a libjpeg-turbo bug, a workaround is to exclude JSC_GRAYSCALE images. Thank you for taking over this issue and bug report. Even though I'm on a trip and cannot afford to investigate this libjpeg-turbo issue, I will investigate it when I'm back from the trip. Regards, Hironori Bono
noel gordon
Comment 20 2011-11-18 00:32:17 PST
Hironori-san, thank you for reporting the bug - the work you did here was important. Enjoy your trip. I look forward to talking with you again on your return.
noel gordon
Comment 21 2011-11-18 00:37:14 PST
Created attachment 115756 [details] Patch
noel gordon
Comment 22 2011-11-18 03:00:49 PST
Created attachment 115775 [details] Patch
Kenneth Russell
Comment 23 2011-11-21 16:24:21 PST
Comment on attachment 115775 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115775&action=review Thanks for pushing this through. A little more build infrastructure work is needed. > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:73 > +#if PLATFORM(CHROMIUM) && USE(SKIA) && ASSUME_LITTLE_ENDIAN This #if test is too fragile. It might be the case that Chromium stops using libjpeg-turbo. See in particular src/third_party/libjpeg_turbo/libjpeg.gyp; it looks like on Chrome OS the system JPEG library is used, which I think would result in the wrong code path being taken here. If that happened, would this code break? Alternatively, other ports might start using libjpeg-turbo. I think this should be written as "#if USE(LIBJPEG_TURBO) && ...". Conditionally define WTF_USE_LIBJPEG_TURBO in Source/WebKit/chromium/WebKit.gyp based on a gyp variable set in libjpeg.gyp.
noel gordon
Comment 24 2011-11-25 00:05:23 PST
(In reply to comment #23) > > Source/WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:73 > > +#if PLATFORM(CHROMIUM) && USE(SKIA) && ASSUME_LITTLE_ENDIAN > > This #if test is too fragile. It might be the case that Chromium stops using libjpeg-turbo. See in particular src/third_party/libjpeg_turbo/libjpeg.gyp; it looks like on Chrome OS the system JPEG library is used, which I think would result in the wrong code path being taken here. If that happened, would this code break? Alternatively, other ports might start using libjpeg-turbo. > Yes, too fragile and my try jobs broke on chrome-mac. I had assumed libjpeg-turbo was not enabled there, and yes, what if other ports or variants (chrome-os) want to use it? My #if test is too restrictive. > I think this should be written as "#if USE(LIBJPEG_TURBO) && ...". Conditionally define WTF_USE_LIBJPEG_TURBO in Source/WebKit/chromium/WebKit.gyp based on a gyp variable set in libjpeg.gyp. If any port switches to using libjepg-turbo, it'll be via build file or other means, but #include "jpeglib.h" will load libjepg-turbo's version of that file, which #defines JCS_EXTENSIONS. So I think JCS_EXTENSIONS can be used to detect libjepg-turbo? Anyway, I've gone that way for now to continue my testing with a much simpler #if test.
noel gordon
Comment 26 2011-11-25 00:13:59 PST
Created attachment 116577 [details] Patch
Hironori Bono
Comment 27 2011-11-25 01:09:30 PST
Greetings, Thank you for your bug report. This is a bug of gray->rgbx conversion in libjpeg-turbo. I will file a upstream bug and upload its fix. Regards, Hironori Bono (In reply to comment #20) > Hironori-san, thank you for reporting the bug - the work you did here was important. Enjoy your trip. I look forward to talking with you again on your return.
Kenneth Russell
Comment 28 2011-11-28 13:10:21 PST
Comment on attachment 116577 [details] Patch Looks good. Testing for JCS_EXTENSIONS does seem to be a reasonable discriminator between libjpeg-turbo and the original libjpeg. r=me
WebKit Review Bot
Comment 29 2011-11-28 14:55:48 PST
Comment on attachment 116577 [details] Patch Clearing flags on attachment: 116577 Committed r101286: <http://trac.webkit.org/changeset/101286>
WebKit Review Bot
Comment 30 2011-11-28 14:55:55 PST
All reviewed patches have been landed. Closing bug.
noel gordon
Comment 31 2011-12-24 05:21:21 PST
(In reply to comment #27) > Thank you for your bug report. > This is a bug of gray->rgbx conversion in libjpeg-turbo. I will file a upstream bug and upload its fix. Filed bug 75189 about the grayscale issue.
Note You need to log in before you can comment on or make changes to this bug.