Bug 74286

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

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

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

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

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

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

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

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

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

Detecting JCS_ALPHA_EXTENSIONS should be enough to test for a modern libjpeg-turbo I believe.

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

Created attachment 124255 [details] Patch

Comment on attachment 124255 [details] Patch Looks fine assuming it compiles and runs. r=me

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

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

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

Comment on attachment 124255 [details] Patch Clearing flags on attachment: 124255 Committed r106203: <http://trac.webkit.org/changeset/106203>

All reviewed patches have been landed. Closing bug.

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