74286 2011-12-12 04:03:39 -0800 [Chromium] Use decoding swizzle only on libjpeg-turbo 1.1.90+ 2012-01-30 22:41:31 -0800 1 1 1 Unclassified WebKit Images 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 76881 59670 1 hbono noel.gordon dcommander floppymaster kbr noel.gordon phajdan.jr pkasting webkit.review.bot oldest_to_newest 519127 0 hbono 2011-12-12 04:03:39 -0800 (Copied from <http://crbug.com/106954>.) Chrome Version 17.0.963.0 and 17.0.963.2 (Dev Channel) URLs (if applicable) : all pages with contains images Other browsers tested: Add OK or FAIL after other browsers where you have tested this issue: Safari 5: Firefox 4.x: IE 7/8/9: What steps will reproduce the problem? 1. open chromium in start page (all thumbails fail render) 2. go to all webpages (whatever, whichever is) 3. view images What is the expected result? render JPEG images in any pages What happens instead? this http://wstaw.org/m/2011/12/09/plasma-desktopX20437.png http://wstaw.org/m/2011/12/09/plasma-desktopOG6636.png http://wstaw.org/m/2011/12/09/plasma-desktopgO6636.png http://wstaw.org/m/2011/12/09/plasma-desktopW20437.png http://wstaw.org/m/2011/12/09/plasma-desktopp20437.png Please provide any additional information below. Attach a screenshot if possible. system archlinux 64bits. build with: GYP_DEFINES=gcc_version=46 werror= no_strict_aliasing=1 linux_sandbox_path=/usr/lib/chromium-dev/chromium-sandbox linux_sandbox_chrome_path=/usr/lib/chromium-dev/chromium release_extra_cflags=-march=x86-64 -mtune=native -O2 -pipe -fstack-protector --param=ssp-buffer-size=4 -D_FORTIFY_SOURCE=2 disable_nacl=1 use_system_ffmpeg=0 build_ffmpegsumo=1 proprietary_codecs=1 use_system_vpx=1 use_system_speex=1 use_system_libxslt=1 use_system_libxml=1 use_system_bzip2=1 use_system_zlib=1 use_system_libjpeg=1 use_system_libpng=1 use_system_yasm=1 use_system_libevent=1 use_system_icu=0 use_system_ssl=0 use_pulseaudio=1 use_gconf=0 use_gnome_keyring=0 linux_link_gnome_keyring=0 target_arch=x64 linux_strip_binary=1 remove_webcore_debug_symbols=1 libjpeg-turbo 1.1.1-3 (latest update: 2011-06-26) (install by the distro [extra] repository) up to 17.0.963.0, work with external libjpeg libs when use internal (use_system_libjpeg=0) with latest version of chromium Dev-Channel the render JPEG images works without problem in compile time don't show any error greetings 519129 1 hbono 2011-12-12 04:06:25 -0800 Greetings Noel, Unfortunately, the decoding swizzle code (introduced by WebKit r101286 <http://trac.webkit.org/changeset/101286>) works only on libjpeg-turbo 1.1.90 or later. Is it possible to avoid using your swizzle code when we build Chromium with a USE_SYSTEM_JPEG macro, i.e. OS(CHROMIUM) && defined(USE_SYSTEM_JPEG)? Regards, Hironori Bono 519384 2 pkasting 2011-12-12 12:46:20 -0800 (In reply to comment #1) > Unfortunately, the decoding swizzle code (introduced by WebKit r101286 <http://trac.webkit.org/changeset/101286>) works only on libjpeg-turbo 1.1.90 or later. Is it possible to avoid using your swizzle code when we build Chromium with a USE_SYSTEM_JPEG macro, i.e. OS(CHROMIUM) && defined(USE_SYSTEM_JPEG)? Is there any way to query the libjpeg-turbo version, either via macros, or in some sort of package configure script that can then set the appropriate Chrome build options? I kind of dislike turning this off for all "use system libraries" cases because then even systems with more recent libraries miss out on the optimization. 519959 3 hbono 2011-12-13 01:43:31 -0800 Greetings Peter, Many thanks for your comments. (In reply to comment #2) > Is there any way to query the libjpeg-turbo version, either via macros, or in some sort of package configure script that can then set the appropriate Chrome build options? Unfortunately, I cannot find any macros representing the version in the (original) source code of libjpeg-turbo. I will file a feature request for it. > I kind of dislike turning this off for all "use system libraries" cases because then even systems with more recent libraries miss out on the optimization. It makes sense. I have focused on the "use_system_jpeg=1" option too much even though this bug name is "Use decoding swizzle only on libjpeg-turbo 1.1.90+". Until libjpeg-turbo adds its version number, should we enable this swizzle code for performance or disable it for compatibility with existing libjpeg-turbo? Regards, Hironori Bono 519966 4 phajdan.jr 2011-12-13 02:08:03 -0800 (In reply to comment #2) > Is there any way to query the libjpeg-turbo version, either via macros, > or in some sort of package configure script that can then set the appropriate Chrome build options? Note that it's not just about compile-time compatibility. It should be possible to compile against libjpeg-turbo and then use vanilla libjpeg at run-time without problems. Currently it's not the case, see https://bugs.gentoo.org/show_bug.cgi?id=393471#c3 and the next comment on that bug. The fix for this should allow the users to use any of libjpeg, libjpeg-turbo at run time and get correct rendering. > I kind of dislike turning this off for all "use system libraries" cases because then > even systems with more recent libraries miss out on the optimization. Arguably correctness is more important than performance. I don't find fast and broken rendering very useful. 519988 5 hbono 2011-12-13 02:45:09 -0800 Greetings, I should have written the background of this issue in details. (In reply to comment #4) > Arguably correctness is more important than performance. I don't find fast and broken rendering very useful. I assume we all think correctness is important. I think Peter just likes to find possible options that we can provide the performance gain provided by libjpeg-turbo 1.1.90+ on all platforms that have it installed. (JPEGImageDecoder.cpp is used not only for Chromium but also for all WebKit-based browsers.) Unfortunately, as written in my comment, we do not have good options to satisfy his request for now and we need to find good alternatives, such as: * Disable the swizzle code on all platforms: * Enable the swizzle code only when we compile WebKit with a USE() macro; * Enable the swizzle code only when we compile WebKit with use_system_jpeg=0. Since each option has both advantages and disadvantages, I would like to hear your thoughts. Regards, Hironori Bono 520183 6 floppymaster 2011-12-13 10:49:57 -0800 (In reply to comment #5) > * Enable the swizzle code only when we compile WebKit with a USE() macro; This seems like the most flexible option to me. Over in Gentoo land, we could introduce a portage use flag that would translate into a gyp definition and enable or disable the "swizzle" code explicitly. This would also allow us to add libjpeg-turbo as an optional dependency; if the user enables the use flag, we hard depend on libjpeg-turbo. If they disable it, we depend on either libjpeg or libjpeg-turbo. Other distros could make the decision when they build packages. Just my 2 cents as a user and a Gentoo dev. Hopefully Pawel agrees with me. 520765 7 phajdan.jr 2011-12-14 00:52:43 -0800 (In reply to comment #5) > JPEGImageDecoder.cpp is used not only for Chromium but also for all WebKit-based browsers. Just wait for all those WebKit-based browsers used in the Linux land to hit the same problem. They just update WebKit much more rarely than Chromium does. I'm pretty sure they'd be affected in the same way. > Unfortunately, as written in my comment, we do not have good > options to satisfy his request for now and we need to find good alternatives, such as: > * Disable the swizzle code on all platforms: > * Enable the swizzle code only when we compile WebKit with a USE() macro; > * Enable the swizzle code only when we compile WebKit with use_system_jpeg=0. Provided we get -DUSE_SYSTEM_LIBJPEG in that WebKit file when -Duse_system_jpeg=1 is passed (I haven't checked that, and it's obviously fixable by ensuring proper gyp dependencies are in place). For me, all of those options are fine. You probably want something that'd make it enabled for Google Chrome, so my best bet is a USE() macro, controlled by use_system_jpeg on the Chrome side. (In reply to comment #6) > Over in Gentoo land, we could introduce a portage use flag > that would translate into a gyp definition and enable or disable the "swizzle" code explicitly. Note: I'm not enthusiastic about such a flag. IMO it's an unnecessary complication. > Other distros could make the decision when they build packages. Sounds good. In Gentoo though I'd _probably_ prefer to have the swizzle code off, until the vanilla libjpeg also supports it. 529903 8 hbono 2012-01-04 17:36:50 -0800 Greetings, I'm a little out-of-sync due to vacation. (In reply to comment #7) > Provided we get -DUSE_SYSTEM_LIBJPEG in that WebKit file when -Duse_system_jpeg=1 is passed (I haven't checked that, and it's obviously fixable by ensuring proper gyp dependencies are in place). Oops, I forgot describing about the USE(SYSTEM_LIBJPEG) macro. (I assumed everyone had knowledge about WebKit macros.) As listed in its definition (copied from Platform.h), USE(SYSTEM_LIBJPEG) becomes 1 when we have a '-DWTF_USE_SYSTEM_LIBJPEG=1' flag. #define USE(WTF_FEATURE) (defined WTF_USE_##WTF_FEATURE && WTF_USE_##WTF_FEATURE) Regards, Hironori Bono 532865 9 hbono 2012-01-10 00:38:12 -0800 Greetings, I have finally caught up with the discussion in <https://bugs.gentoo.org/show_bug.cgi?id=393471>. As written in this discussion libjpeg-turbo now adds new colorspace extensions (JCS_EXT_RGBA, JCS_EXT_ARGB, etc.) so we can check if libjpeg-turbo can fill alpha values with 0xFF. So, can we use these extensions and use swizzling only when we can use these new extensions? Regards, Hironori Bono 543788 10 noel.gordon 2012-01-26 20:47:35 -0800 Detecting JCS_ALPHA_EXTENSIONS should be enough to test for a modern libjpeg-turbo I believe. 543818 11 noel.gordon 2012-01-26 21:35:01 -0800 (In reply to comment #10) > Detecting JCS_ALPHA_EXTENSIONS should be enough to test for a modern libjpeg-turbo I believe. And that was added at r732 http://libjpeg-turbo.svn.sourceforge.net/viewvc/libjpeg-turbo/trunk/jpeglib.h?r1=732&r2=731&pathrev=732 And bug 76881 updates libjpeg-turbo to r733. 543821 12 124255 noel.gordon 2012-01-26 21:44:25 -0800 Created attachment 124255 Patch 544156 13 124255 kbr 2012-01-27 10:16:42 -0800 Comment on attachment 124255 Patch Looks fine assuming it compiles and runs. r=me 544764 14 noel.gordon 2012-01-29 14:23:50 -0800 (In reply to comment #4) > (In reply to comment #2) > > Is there any way to query the libjpeg-turbo version, either via macros, > > or in some sort of package configure script that can then set the appropriate Chrome build options? > > Note that it's not just about compile-time compatibility. It should be possible to compile against libjpeg-turbo and then use vanilla libjpeg at run-time without problems. > > Currently it's not the case, see https://bugs.gentoo.org/show_bug.cgi?id=393471#c3 and the next comment on that bug. > > The fix for this should allow the users to use any of libjpeg, libjpeg-turbo at run time and get correct rendering. Simple rule: if you build against libjpeg-x for any x including turbo, then you should run against libjpeg-x. There is a cautionary note about this in the libjpeg.txt|.doc included in libjpeg distributions. https://bugs.gentoo.org/show_bug.cgi?id=393471#c3 shows what happens when you ignore that cautionary note (broken rendering, or no jpeg at all). The rule is not specific to libjpeg-turbo; it applies to all libjpeg. 544765 15 noel.gordon 2012-01-29 14:25:55 -0800 (In reply to comment #13) > (From update of attachment 124255 [details]) > Looks fine assuming it compiles and runs. r=me Compiles, runs, and the relevant tests look good to me. Submitting. 544768 16 noel.gordon 2012-01-29 15:19:36 -0800 (In reply to comment #11) > (In reply to comment #10) > > Detecting JCS_ALPHA_EXTENSIONS should be enough to test for a modern libjpeg-turbo I believe. > > And that was added at r732 > http://libjpeg-turbo.svn.sourceforge.net/viewvc/libjpeg-turbo/trunk/jpeglib.h?r1=732&r2=731&pathrev=732 > > And bug 76881 updates libjpeg-turbo to r733. I note that the libjpeg-turbo GenToo currently uses is buggy and contains a security flaw. Consider upgrading to r729+. 544791 17 124255 webkit.review.bot 2012-01-29 16:00:02 -0800 Comment on attachment 124255 Patch Clearing flags on attachment: 124255 Committed r106203: <http://trac.webkit.org/changeset/106203> 544792 18 webkit.review.bot 2012-01-29 16:00:07 -0800 All reviewed patches have been landed. Closing bug. 545796 19 dcommander 2012-01-30 22:41:31 -0800 (In reply to comment #14) > Simple rule: if you build against libjpeg-x for any x including turbo, then you should run against libjpeg-x. There is a cautionary note about this in the libjpeg.txt|.doc included in libjpeg distributions. > https://bugs.gentoo.org/show_bug.cgi?id=393471#c3 shows what happens when you ignore that cautionary note (broken rendering, or no jpeg at all). The rule is not specific to libjpeg-turbo; it applies to all libjpeg. Even though it looks like you guys have resolved this issue, I wanted to add a general comment for posterity regarding API/ABI compatibility, which is basically what this issue boils down to. The warning you parsed from libjpeg.txt is from the Thomas Lane days and has remained unchanged since at least jpeg-6b (1998.) What it's basically saying is: the libjpeg API/ABI exposes all of its configuration structures, and thus if the IJG has to add features to libjpeg, these new features may render the API/ABI backward-incompatible with previous releases. That is exactly what happened with jpeg-7, and again with jpeg-8. This is why libjpeg-turbo emulates all three of those API/ABIs. If you are building libjpeg-turbo with (for instance) jpeg-6b emulation, it should effectively work as a drop-in replacement for jpeg-6b. jpeg-7 and jpeg-8, similarly, except that a couple of features of jpeg-7 and jpeg-8 (DCT scaling and fancy downsampling in the compressor) are not supported, and those parameters are silently ignored if passed to libjpeg-turbo (irrelevant in your case, since you're using libjpeg-turbo in a web browser.) When building as a shared library on Linux, all of the symbols are versioned, which should prevent an application from using a shared library that exports a different version of the libjpeg API/ABI from the one against which the application was linked. In short, there should be no danger with interchanging libjpeg-turbo at run time with a version of libjpeg that exports the same API/ABI. The only potential problem involves the colorspace extensions, and jcstest.c in the libjpeg-turbo source shows how to detect their presence at run time. This allows applications that were built against libjpeg-turbo to safely fall back or to generate an error if the application dynamic links against libjpeg at run time. Anyhow, the above scenario may be rare enough to not warrant consideration, but I mainly wanted to document that it is technically feasible to swap libjpeg with libjpeg-turbo and vice versa at run time, if the need ever arises. That was one of the big reasons why we chose to build a SIMD-accelerated JPEG library around the libjpeg API instead of another one. I never imagined it would become so popular 2 years later. :) 124255 2012-01-26 21:44:25 -0800 2012-01-29 16:00:02 -0800 Patch bug-74286-20120127164423.patch text/plain 2383 noel.gordon U3VidmVyc2lvbiBSZXZpc2lvbjogMTA2MDg3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMTQ4NmQzZWMxYWM5MWFl MTY0NDViOWYwNDE1MjVlZGNiNzRhMTUyMi4uNzRiNjZiOTY0ZWI2YzJlODI0MDkwOWUxMWJjMDVm ZWY5NGEwYjllYiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI1IEBACisyMDEyLTAxLTI2ICBOb2Vs IEdvcmRvbiAgPG5vZWwuZ29yZG9uQGdtYWlsLmNvbT4KKworICAgICAgICBbY2hyb21pdW1dIFVz ZSBkZWNvZGluZyBzd2l6emxlIG9ubHkgb24gbGlianBlZy10dXJibyAxLjEuOTArCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD03NDI4NgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE5vIG5ldyB0ZXN0cy4gQ292 ZXJlZCBieSBtYW55IGV4aXN0aW5nIHRlc3RzOiBpbiBwYXJ0aWN1bGFyIAorICAgICAgICAgICAg ZmFzdC9pbWFnZXMvKiwgZmFzdC9jYW52YXMvKiwKKyAgICAgICAgICAgIHRhYmxlcy9tb3ppbGxh L2J1Z3MvYnVnMjkzMTQuaHRtbAorICAgICAgICAgICAgdGFibGVzL21vemlsbGEvYnVncy9idWcx MzE2OS5odG1sCisgICAgICAgICAgICB0YWJsZXMvbW96aWxsYS9idWdzL2J1ZzEwNTY1Lmh0bWwK KyAgICAgICAgICAgIHRhYmxlcy9tb3ppbGxhL2J1Z3MvYnVnMTEwMjYuaHRtbAorICAgICAgICAg ICAgZmFzdC9yZXBhaW50L2JhY2tncm91bmRTaXplUmVwYWludC5odG1sCisgICAgICAgICAgICBm YXN0L3JlcGFpbnQvYmxvY2stbGF5b3V0LWlubGluZS1jaGlsZHJlbi1yZXBsYWNlZC5odG1sCisg ICAgICAgICAgICBmYXN0L3JlcGFpbnQvY2xpcHBlZC1yZWxhdGl2ZS5odG1sCisgICAgICAgICAg ICBmYXN0L3JlcGFpbnQvc2VsZWN0ZWQtcmVwbGFjZWQuaHRtbAorICAgICAgICAgICAgdGFibGVz L21vemlsbGEvYnVncy9idWcxMjkwOC0xLmh0bWwKKworICAgICAgICAqIHBsYXRmb3JtL2ltYWdl LWRlY29kZXJzL2pwZWcvSlBFR0ltYWdlRGVjb2Rlci5jcHA6CisgICAgICAgIChyZ2JPdXRwdXRD b2xvclNwYWNlKTogU3dpenpsZSBkZWNvZGUgaWZmIGxpYmpwZWctdHVyYm8gaXMgcjczMiBvciBh Ym92ZS4KKwogMjAxMi0wMS0yNiAgU2hlcmlmZiBCb3QgIDx3ZWJraXQucmV2aWV3LmJvdEBnbWFp bC5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgcm9sbGluZyBvdXQgcjEwNTQ4Ni4KZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2ltYWdlLWRlY29kZXJzL2pwZWcvSlBFR0lt YWdlRGVjb2Rlci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9pbWFnZS1kZWNvZGVycy9q cGVnL0pQRUdJbWFnZURlY29kZXIuY3BwCmluZGV4IGU3MGIwYWY2ZWMzOTY2YWJmYjAxNDA3NTI4 MmI2MTcyYTYxYmEwYTQuLjViNDI0OTBhYmZhNTBiZDhhZjhkMDExOTgwODU0ZDg2YzRjMGU5MGEg MTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2ltYWdlLWRlY29kZXJzL2pwZWcv SlBFR0ltYWdlRGVjb2Rlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vaW1hZ2Ut ZGVjb2RlcnMvanBlZy9KUEVHSW1hZ2VEZWNvZGVyLmNwcApAQCAtNjksMTIgKzY5LDEyIEBAIGV4 dGVybiAiQyIgewogI2RlZmluZSBBU1NVTUVfTElUVExFX0VORElBTiAxCiAjZW5kaWYKIAotI2lm IGRlZmluZWQoSkNTX0VYVEVOU0lPTlMpICYmIEFTU1VNRV9MSVRUTEVfRU5ESUFOCisjaWYgZGVm aW5lZChKQ1NfQUxQSEFfRVhURU5TSU9OUykgJiYgQVNTVU1FX0xJVFRMRV9FTkRJQU4KICNkZWZp bmUgVFVSQk9fSlBFR19SR0JfU1dJWlpMRQogI2lmIFVTRShTS0lBKSAmJiAoIVNLX1IzMl9TSElG VCAmJiBTS19HMzJfU0hJRlQgPT0gOCAmJiBTS19CMzJfU0hJRlQgPT0gMTYpCi1pbmxpbmUgSl9D T0xPUl9TUEFDRSByZ2JPdXRwdXRDb2xvclNwYWNlKCkgeyByZXR1cm4gSkNTX0VYVF9SR0JYOyB9 CitpbmxpbmUgSl9DT0xPUl9TUEFDRSByZ2JPdXRwdXRDb2xvclNwYWNlKCkgeyByZXR1cm4gSkNT X0VYVF9SR0JBOyB9CiAjZWxzZQotaW5saW5lIEpfQ09MT1JfU1BBQ0UgcmdiT3V0cHV0Q29sb3JT cGFjZSgpIHsgcmV0dXJuIEpDU19FWFRfQkdSWDsgfQoraW5saW5lIEpfQ09MT1JfU1BBQ0Ugcmdi T3V0cHV0Q29sb3JTcGFjZSgpIHsgcmV0dXJuIEpDU19FWFRfQkdSQTsgfQogI2VuZGlmCiBpbmxp bmUgYm9vbCB0dXJib1N3aXp6bGVkKEpfQ09MT1JfU1BBQ0UgY29sb3JTcGFjZSkgeyByZXR1cm4g Y29sb3JTcGFjZSA9PSByZ2JPdXRwdXRDb2xvclNwYWNlKCk7IH0KICNlbHNlCg==