[needs-packaging] libjpeg-turbo

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.4) Gecko/20100611 Firefox/3.6.4
Build Identifier:

To quote the URL
"libjpeg-turbo is a high-speed version of libjpeg for x86 and x86-64 processors which uses SIMD instructions (MMX, SSE2, etc.) to accelerate baseline JPEG compression and decompression. libjpeg-turbo is generally 2-4x as fast as the unmodified version of libjpeg, all else being equal.

libjpeg-turbo was originally based on libjpeg/SIMD by Miyasaka Masaru, but the TigerVNC and VirtualGL projects made numerous enhancements to the codec, including improved support for Mac OS X, 64-bit support, support for 32-bit and big endian pixel formats (RGBA, ABGR, etc.), accelerated Huffman encoding/decoding, and various bug fixes. The goal was to produce a fully open source codec that could replace the partially closed source TurboJPEG/IPP codec used by VirtualGL and TurboVNC. libjpeg-turbo generally achieves 80-120% of the performance of TurboJPEG/IPP. It is faster in some areas but slower in others."

Fedora already working on incorporating it in their distribution.
http://fedoraproject.org/wiki/Features/libjpeg-turbo

License is wxWindows license for some parts and LGPL for the rest.

This would close a lot of bugs regarding libjpeg/SSE Optimization/64-bit Porting

Reproducible: Always

You'll probably need to consult with <email address hidden> about the license. See guidelines here: http://www.mozilla.org/MPL/license-policy.html

When looking at third party libraries for the video decoding and YCbCr conversion LGPL was an issue.

Using libjpeg-simd is probably an easier option here than libjpeg-turbo license wise.

According to
http://libjpeg-turbo.svn.sourceforge.net/viewvc/libjpeg-turbo/trunk/README-turbo.txt

"Some of the optimizations to the Huffman encoder/decoder were borrowed from
VirtualGL, and thus the libjpeg-turbo distribution, as a whole, falls under the
wxWindows Library Licence, Version 3.1. A copy of this license can be found in
this directory under LICENSE.txt. The wxWindows Library License is based on
the LGPL but includes provisions which allow the Library to be statically
linked into proprietary libraries and applications without requiring the
resulting binaries to be distributed under the terms of the LGPL.

The rest of the source code, apart from these modifications, falls under a less
restrictive, BSD-style license (see README.)"

So it seems that wxWindows license is a more liberal version of LGPL and the rest is BSD (I was mistaken in the initial posting)

(In reply to comment #2)
> Using libjpeg-simd is probably an easier option here than libjpeg-turbo license
> wise.

Sorry for the bug spam

Regarding libjpeg-simd

From the Google translation of http://cetus.sakura.ne.jp/softlab/jpeg-x86simd/jpegsimd.html

"In addition, AMD64 (EM64T/Intel64) in 64bit mode environment (currently) not supported."

"AMD64 compatible version is under construction...(October 26, 2006: The production process is currently stopped. Seems slow release schedule. Sorry.)"

Jpeg-simd would have to be ported to 64-bit which from what I have read is a cumbersome task.

(In reply to comment #4)
> Jpeg-simd would have to be ported to 64-bit which from what I have read is a
> cumbersome task.

From the diffs between the 64-bit version and the 32-bit version it seems that the process would be mostly mechanical. In fact, it should be possible to use the same source for 64-bit as 32-bit like libvpx does. Further, the 64-bit versions in libjpeg-turbo should be under the same license as jpeg-simd so we should be able to just use them directly.

Hi, all. I'm the maintainer and one of the creators of libjpeg-turbo, so I thought I'd throw in some comments. For starters, libjpeg-turbo is not licensed under pure LGPL. It uses the wxWindows Library License, which eliminates some of the viral provisions of the LGPL. My understanding of the differences between the two is spelled out here:

http://www.virtualgl.org/About/License

I have confirmed with the wxWindows developers who created the license that this matches their understanding as well. Essentially, it is our understanding that wxWindows nullifies Sections 6-7 of LGPL v2.1, which means you can statically link the Library with any application without requiring the application as a whole to fall under the provisions of the LGPL. You are still bound by other sections of the LGPL, which means you would still be required to distribute source for the Library along with the application.

That being said, however, I can understand the desire for a fully unencumbered version of the source. The only part of the code which is licensed under wxWindows is the Huffman encoding/decoding optimizations (portions of jchuff.c and jdhuff.c.) These were developed while I was a Sun employee, so unfortunately I don't have the right to dual license them, or else I certainly would do so.

What I could do, however, is develop a mechanism whereby you could easily build the code without the Huffman optimizations ('configure --without-huffman-opt' or something like that.) This would simply involve building libjpeg-turbo with the "stock" versions of jchuff.c and jdhuff.c from libjpeg-6b. I could even go one step farther and make it so that the wxWindows-licensed files are not included in the distribution tarball if you 'configure --without-huffman-opt' and then 'make dist'.

This would slow down the overall performance by 20-25%, but it will still be miles faster than regular libjpeg on x86/x86-64 systems.

What DRC suggests seems like the smart thing to do, to begin with. Let's take the parts which are unambiguously BSD and still provide a significant win.

We currently have a policy of only taking code which is compatible with all three of our licenses. We'd have to do some analysis on the wxWindows licence to see whether it fits our policy. (Please, people, stop rolling your own licences! :-) If someone could open another bug for that, that would be great.

But if it's not compatible in that way, I think it's highly unlikely we'd want to introduce a new licence into the Mozilla mix just for a 25% win in JPEG decoding (particularly if we'd already recently had a big win over what we'd been using for years).

Gerv

wxWindows is actually a very mature license at this point-- it just isn't well known. It was created to solve a specific problem -- allowing static linking of an open source library with proprietary code without forcing the proprietary application to fall under the terms of the LGPL, but also guaranteeing the open nature of the code (which BSD-style licenses do not do.) OpenSceneGraph uses it, and that's where I learned about it originally.

Let me know if there's anything I can do to facilitate this on my end. If you're just going to merge the code into your build tree, then you can just as easily replace jchuff.c and jdhuff.c with their original versions when you do that, but I'm still willing to add a configure switch to libjpeg-turbo as well.

(In reply to comment #8)
> Let me know if there's anything I can do to facilitate this on my end. If
> you're just going to merge the code into your build tree, then you can just as
> easily replace jchuff.c and jdhuff.c with their original versions when you do
> that, but I'm still willing to add a configure switch to libjpeg-turbo as well.

This seems like it might be best option.

Has any work been done on ARM or other platforms?

(In reply to comment #10)
> Has any work been done on ARM or other platforms?

One thing I've been meaning to do is switch mobile to use the IFAST dct instead of the ISLOW dct. Android uses IFAST and in fact have an ARM implementation of the IFAST dct. Unfortunately, it is licensed with the apache license. Perhaps we can convince Google to relicense it?

(In reply to comment #10)
> Has any work been done on ARM or other platforms?

We should write NEON SIMD code if we change to libjpeg-turbo.

(In reply to comment #11)
> (In reply to comment #10)
> > Has any work been done on ARM or other platforms?
>
> One thing I've been meaning to do is switch mobile to use the IFAST dct instead
> of the ISLOW dct. Android uses IFAST and in fact have an ARM implementation of
> the IFAST dct. Unfortunately, it is licensed with the apache license. Perhaps
> we can convince Google to relicense it?

Past year, Michael Moy tried to change to IFAST on some platforms by bug 416157. But many test cases became failed. If changing to IFAST, we have to modify many test cases, too.

DRC: the problem is not just incompatibility, but simplicity. A library under the straight LGPL would be compatible with the MPL, the LGPL and the GPL (our three licences) but would increase the licensing complexity of our codebase, something we have historically tried to avoid.

Gerv

So, just so I understand, inbound code to the Mozilla project has to either be licensed under the tri-license or something less restrictive that can be re-licensed under the tri-license? So if it were possible to re-license the offending code in libjpeg-turbo under your tri-license, then that would solve the problem?

Our goal is that people should be able to redistribute all the code that goes into our products under the terms of any one of the MPL, LGPL, or GPL. So:
-- code that is licensed with a license compatible with all of those (e.g., BSD) is fine
-- code that is licensed with multiple license options that cover the three above licenses is also fine (e.g., dual licensed MPL/LGPL, since the LGPL option is compatible with the GPL as well)

Note that we don't relicense other people's code. E.g., when we import BSD-licensed code into our tree, it keeps the original license.

Unless I'm missing something fundamental, I think wxWindows-licensed code could be legally redistributed under all three licenses. However, I fully understand the desire not to introduce new licenses into your code tree.

One question-- do you currently copy the libjpeg code into the Mozilla source or do you build it separately?

(In reply to comment #16)
> One question-- do you currently copy the libjpeg code into the Mozilla source
> or do you build it separately?

It's in our tree, but it's not by any means the original libjpeg, as it's been patched to pieces.

http://mxr.mozilla.org/mozilla-central/source/jpeg/

OK, then it seems as if merging in the "turbo" features (minus the Huffman coding) into your existing code tree would be the way to go.

(In reply to comment #13)
> DRC: the problem is not just incompatibility, but simplicity. A library under
> the straight LGPL would be compatible with the MPL, the LGPL and the GPL (our
> three licences) but would increase the licensing complexity of our codebase,
> something we have historically tried to avoid.

Original license of SIMD code is http://cetus.sakura.ne.jp/softlab/jpeg-x86simd/jpegsimd.html#conditions

In Japanese, "この SIMD 拡張版 IJG JPEG software の使用条件については、オリジナル版の IJG JPEG software の使用条件が適用されます。"

In English by Google Translate, "This SIMD extension for IJG JPEG software terms of use, the original version of the IJG JPEG software is subject to terms of use."

So, original author who is MIYASAKA-san says that this code is same license as libjpeg. I believe that license is no problem if we port MIYASAKA-san's code to our tree, not using libjpeg-turbo.

Gerv, how about?

Why does the idea of using the old libjpeg/SIMD code keep coming up in this discussion? This has already been covered multiple times-- libjpeg/SIMD does not have 64-bit support or many of the other bug fixes we made to libjpeg-turbo.

libjpeg-turbo also bears the same license as libjpeg, if you don't use our accelerated version of jchuff.c and jdhuff.c.

In short-- merge in libjpeg-turbo, minus jchuff.c and jdhuff.c, and there is no licensing problem.

There seems to be some confusion around what should happen here... the right thing, I think, is:

1. Look at our in-tree libjpeg and how it differs from stock libjpeg.

2. Based on that, either merge the delta from libjpeg -> libjpeg-turbo into our tree, OR merge delta of stock libjpeg and in-tree libjpeg into libjpeg-turbo, and then pull that in.

(minus j[cd]huff.c, of course).

I don't really know how extensive the changes in #1 are, and I don't know what, if any, conflicts there would be between the changes for -turbo and our tree version of libjpeg. But I think it's worth it to find out; the perf wins wouldn't hurt as we try to do decode on draw, and it sounds like there are some pretty significant ones here.

Forgot to ask -- who wants to make a patch? :-)

After a cursory look at the code, it seems like libjpeg-turbo may want to borrow Mozilla's enhancements, where applicable. In particular, the use of static tables in choice areas like RGB->YUV conversion is likely to improve our performance as well.

Also, there is some of the code which is going to conflict, because your version already has some SIMD acceleration. I think that most of it could be replaced with ours, but the areas in which you use SSE2 would probably need to be benchmarked against our version to see which is faster (integer IDCT, for instance.)

(In reply to comment #23)
> After a cursory look at the code, it seems like libjpeg-turbo may want to
> borrow Mozilla's enhancements, where applicable. In particular, the use of
> static tables in choice areas like RGB->YUV conversion is likely to improve our
> performance as well.
>
> Also, there is some of the code which is going to conflict, because your
> version already has some SIMD acceleration. I think that most of it could be
> replaced with ours, but the areas in which you use SSE2 would probably need to
> be benchmarked against our version to see which is faster (integer IDCT, for
> instance.)

Our integer SSE2 idct is buggy (bug 477728), so while it would be nice to see how the performance compares, I think I would still switch to the libjpeg-turbo one even if ours is faster.

(In reply to comment #22)
> Forgot to ask -- who wants to make a patch? :-)

I'll try.

(In reply to comment #6)
> What I could do, however, is develop a mechanism whereby you could easily build
> the code without the Huffman optimizations ('configure --without-huffman-opt'
> or something like that.)

My guess is that we'll not want to include j{c,d}huff.c in our source tree at all, if they're encumbered with an incompatible license. Is this right?

Oh, I see; that's a change we made, but which isn't in libjpeg-turbo.

Until it's fixed upstream, why don't we just guard the #define's in jmorecfg.h? We already keep that file out of sync with libjpeg-turbo's mainline.

(In reply to comment #155)
> Oh, I see; that's a change we made, but which isn't in libjpeg-turbo.
>
> Until it's fixed upstream, why don't we just guard the #define's in jmorecfg.h?
> We already keep that file out of sync with libjpeg-turbo's mainline.

That sounds fine.

Created attachment 522445
Patch v2.0. Part 1: Upgrade to libjpeg-turbo v1.1.0

Created attachment 522446
Patch v2.0. Part 2: Address review comments.

Created attachment 522449
Interdiff v1.3.1 -> v2.0 part 1

Created attachment 522459
Interdiff v1.3.1 -> v2.0 part 1 without deleted files

This interdiff is the same as attachment 522449, but it doesn't include the files I deleted, in an attempt to make it easier to read.

DRC, what would be a good medium for handling Jeff's remaining review comments?

I'm going to push the latest patches to try now.

Created attachment 522544
Proposed upstream patch for addressing concern about jround_up() argument types.

I'll address the review comments that are relevant to the upstream code here:

(1) jround_up() argument types: this was the result of a bad game of Celebrity Whack-a-Mole that I played while trying to port the code to Win64. Jeff's idea of creating a local, power-of-2-specific round-up function for jmemmgr.c is a better idea, and I've done so in a new patch (attachment 522544) (actually, I made it a macro instead of a function.) Please let me know if that looks like a reasonable solution.

(2) RGB_{RED|GREEN|BLUE} macro conflicts on OS/2 (OS/2? Really?): This seems like an innocuous enough patch, although I'd rather have it in jmorecfg.h instead of jpeglib.h. I'd also like to know more of the back story as to why OS/2 is defining those macros. Is this a generic problem that anyone building libjpeg-turbo on OS/2 would encounter? My lingering doubt here is that libjpeg supports OS/2, so I wonder why they haven't encountered this problem on that platform. Is Mozilla including some header file that causes those macros to be defined?

(3) bitscan instructions: I tested the proposed patch to make jcdctmgr.c use GCC 3.4+ compiler intrinsics to replace flss() with bitscan instructions, but I cannot observe any measurable speedup from this (tested several different image types and platforms, as well as 32-bit and 64-bit.) If it were even 2% faster, I would agree to include it, but a performance patch that causes no appreciable increase in performance does nothing but needlessly obfuscate the code.

(4) The tables in jccolor.c are part of the optimized RGB-to-grayscale conversion routines. These are pre-computed R-to-luminance, G-to-luminance, and B-to-luminance conversion values. The problem with this approach is that it loses a bit of accuracy due to round-off, so I recently checked in code to the SVN trunk (1.2 working version) that uses new SIMD routines to perform RGB-to-grayscale conversion. This is faster and eliminates the round-off issues, as well as the need for the tables in jccolor.c and the #ifdef in rgb_gray_convert().

(5) The rest of the concerns regarding jccolor.c have already been addressed in trunk as well (pulling rgb_red, rgb_green, rgb_blue out of the loop, etc.)

> (2) RGB_{RED|GREEN|BLUE} macro conflicts on OS/2 (OS/2? Really?): This seems
> like an innocuous enough patch, although I'd rather have it in jmorecfg.h
> instead of jpeglib.h.

Attachment 522446 puts the #undef's in jmorecfg.h, right above the #defines.

And just FYI, another thing that I'm currently being contracted to do is investigate rewriting the performance optimizations in j{c|d}huff*. This will hopefully allow me to re-license those files in libjpeg-turbo 1.2.

Comment on attachment 522544
Proposed upstream patch for addressing concern about jround_up() argument types.

>+#define PAD(size, align) (((size) + (align) - 1) & (~((align) - 1)))

I usually prefer functions over macros for the additional type safety and to avoid evaluating arguments twice (e.g. PAD(x, a++))

(In reply to comment #163)
> (2) RGB_{RED|GREEN|BLUE} macro conflicts on OS/2 (OS/2? Really?): This seems
> like an innocuous enough patch, although I'd rather have it in jmorecfg.h
> instead of jpeglib.h. I'd also like to know more of the back story as to why
> OS/2 is defining those macros. Is this a generic problem that anyone building
> libjpeg-turbo on OS/2 would encounter? My lingering doubt here is that libjpeg
> supports OS/2, so I wonder why they haven't encountered this problem on that
> platform. Is Mozilla including some header file that causes those macros to be
> defined?

I'd be fine with dropping this altogether. If OS/2 still needs it, we can always add it back.

>
> (3) bitscan instructions: I tested the proposed patch to make jcdctmgr.c use
> GCC 3.4+ compiler intrinsics to replace flss() with bitscan instructions, but I
> cannot observe any measurable speedup from this (tested several different image
> types and platforms, as well as 32-bit and 64-bit.) If it were even 2% faster,
> I would agree to include it, but a performance patch that causes no appreciable
> increase in performance does nothing but needlessly obfuscate the code.

It's probably worth adding a comment to flss saying that it's not performance critical enough to justify using compiler intrinsics.

> (4) The tables in jccolor.c are part of the optimized RGB-to-grayscale
> conversion routines. These are pre-computed R-to-luminance, G-to-luminance,
> and B-to-luminance conversion values. The problem with this approach is that
> it loses a bit of accuracy due to round-off, so I recently checked in code to
> the SVN trunk (1.2 working version) that uses new SIMD routines to perform
> RGB-to-grayscale conversion. This is faster and eliminates the round-off
> issues, as well as the need for the tables in jccolor.c and the #ifdef in
> rgb_gray_convert().

That sounds like a good solution to me.

(In reply to comment #165)
> And just FYI, another thing that I'm currently being contracted to do is
> investigate rewriting the performance optimizations in j{c|d}huff*. This will
> hopefully allow me to re-license those files in libjpeg-turbo 1.2.

This sounds great.

Created attachment 522583
New proposed upstream patch for addressing concern about jround_up() argument types.

(In reply to comment #166)
> >+#define PAD(size, align) (((size) + (align) - 1) & (~((align) - 1)))
>
> I usually prefer functions over macros for the additional type safety and to
> avoid evaluating arguments twice (e.g. PAD(x, a++))

My experience has been that the compiler does a good job of optimizing out that duplication of effort, and the lack of overhead of the additional function call usually makes up for it, but in the specific case we're referring to, it doesn't matter enough to worry about, and using a function call is more consistent with the way it worked before. Re-submitted as attachment 522583.

(In reply to comment #170)
> (In reply to comment #166)
> > >+#define PAD(size, align) (((size) + (align) - 1) & (~((align) - 1)))
> >
> > I usually prefer functions over macros for the additional type safety and to
> > avoid evaluating arguments twice (e.g. PAD(x, a++))
>
> My experience has been that the compiler does a good job of optimizing out that
> duplication of effort, and the lack of overhead of the additional function call
> usually makes up for it, but in the specific case we're referring to, it
> doesn't matter enough to worry about, and using a function call is more
> consistent with the way it worked before. Re-submitted as attachment 522583

I was referring to the fact that PAD(x, a++) does a++ twice which is not what the caller is likely to expect. Regardless, I like the new version better.

There are unfortunately reftest failures on Windows with this latest patch. Looking into it.

http://tbpl.mozilla.org/?tree=MozillaTry&rev=fceae635d617
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1301360568.1301363938.582.gz

Ah, the orange is due to unexpected success. That's not so bad. :)

I think we can just back out bug 582850.

http://hg.mozilla.org/mozilla-central/rev/4643426a1523

Yep, and this should close bug 477728

Created attachment 522674
Patch v2.0 Part 3: Re-enable reftests on Windows.

Why is support for arithmetic encoding/decoding disabled?

Don't we need to use the libjpeg v8 ABI in order to get this?

(In reply to comment #176)
> Why is support for arithmetic encoding/decoding disabled?

We haven't decided whether to enabled this yet. This is a separate issue and needs a separate bug as well as a lot more thought/research.

(In reply to comment #177)
> Don't we need to use the libjpeg v8 ABI in order to get this?

No, it works when emulating the v6b ABI as well. It just adds two additional functions without breaking compatibility with the rest of the ABI.

http://hg.mozilla.org/mozilla-central/rev/6a8baba56a65
http://hg.mozilla.org/mozilla-central/rev/3eb5a574319f

Please file follow-ups as needed.

Apparently this causes OOM in yasm 0.8.something we may need to up the min version

More specifically, with yasm 0.8.0.2194, which is the latest available by default with Ubuntu 10.04. yasm 1.1.0.2352 works fine.

Fedora 13 has old yasm too

(Though, honestly we'd really prefer to make it buildable with nasm, or gas. But that's another bug...)

NOTE: libjpeg-turbo trunk now contains a version of jdhuff.c and jdhuff.h (Huffman decoder) with the optimizations re-written and re-licensed under the same BSD-style license as the rest of libjpeg. Looking into jchuff* as well, but I figured you were probably more interested in the decoder than the encoder.

I've filed bug 650899 for updating libjpeg-turbo.

Jesse, why did you nominate this bug for tracking-firefox5?

Because bsmedberg told me to in http://groups.google.com/group/mozilla.dev.planning/msg/bfc2fabaecacdd97. Also because I think it should be listed on https://wiki.mozilla.org/Features/Release_Tracking to make sure it gets the right security reviews, license file additions, changelog entries, crash stats scrutiny, and whatever else people follow that page for.

NOTE: libjpeg-turbo trunk now contains a version of jchuff.c
(Huffman encoder) with the optimizations re-written and re-licensed under the
same BSD-style license as the rest of libjpeg.

... and the bug for tracking that is bug 650899.

That bug is for tracking the decoder. I'm talking about the encoder now.

FYI, the Chrome team has apparently done a security review of the library:
http://code.google.com/p/chromium/issues/detail?id=48789#c3

Security team finished fuzzing as planned, we are good here.