diff mbox

[2/2] gnu: icecat: Use libjpeg-turbo instead of bundled libjpeg.

Message ID 20161127201707.3789-2-me@tobias.gr
State New
Headers show

Commit Message

Tobias Geerinckx-Rice Nov. 27, 2016, 8:17 p.m. UTC
* gnu/packages/gnuzilla.scm (icecat)[source]: Remove bundled libjpeg.
[inputs]: Add libjpeg-turbo.
[arguments]: Use it.
---
 gnu/packages/gnuzilla.scm | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

Comments

Leo Famulari Nov. 27, 2016, 8:56 p.m. UTC | #1
On Sun, Nov 27, 2016 at 09:17:07PM +0100, Tobias Geerinckx-Rice wrote:
> * gnu/packages/gnuzilla.scm (icecat)[source]: Remove bundled libjpeg.
> [inputs]: Add libjpeg-turbo.
> [arguments]: Use it.

> -                           ;; Fails with "libjpeg-turbo JCS_EXTENSIONS
> -                           ;; required".
> -                           ;; According to
> -                           ;; http://sourceforge.net/projects/libjpeg-turbo/ ,
> -                           ;; "libjpeg-turbo is a derivative of libjpeg that
> -                           ;; uses MMX, SSE, SSE2, and NEON SIMD instructions
> -                           ;; to accelerate baseline JPEG compression/
> -                           ;; decompression", so we had better not use it
> -                           ;; "--with-system-jpeg"

Will this work on nachines that don't support SSE? My understanding is
that we don't wish to require it.
Tobias Geerinckx-Rice Nov. 27, 2016, 9:02 p.m. UTC | #2
Leo,

On 27/11/16 21:56, Leo Famulari wrote:
> Will this work on nachines that don't support SSE? My understanding is
> that we don't wish to require it.

Why wouldn't it?

I don't often use Icecat, but when I do... I don't see why it would
require SSE by default. ;-) And it's not like we ever patched it to use
IJG's libjpeg to begin with.

Kind regards,

T G-R
Leo Famulari Nov. 27, 2016, 9:41 p.m. UTC | #3
On Sun, Nov 27, 2016 at 10:02:35PM +0100, Tobias Geerinckx-Rice wrote:
> Leo,
> 
> On 27/11/16 21:56, Leo Famulari wrote:
> > Will this work on nachines that don't support SSE? My understanding is
> > that we don't wish to require it.
> 
> Why wouldn't it?
> 
> I don't often use Icecat, but when I do... I don't see why it would
> require SSE by default. ;-) And it's not like we ever patched it to use
> IJG's libjpeg to begin with.

I'm not sure, but the comment seems to indicate that we don't want to
use it:

;; According to
;; http://sourceforge.net/projects/libjpeg-turbo/ ,
;; "libjpeg-turbo is a derivative of libjpeg that
;; uses MMX, SSE, SSE2, and NEON SIMD instructions
;; to accelerate baseline JPEG compression/
;; decompression", so we had better not use it

But I'm not sure how to interpret that, so I've copied Andreas on my
reply (he wrote that comment).
Marius Bakke Nov. 27, 2016, 9:50 p.m. UTC | #4
Leo Famulari <leo@famulari.name> writes:

> On Sun, Nov 27, 2016 at 10:02:35PM +0100, Tobias Geerinckx-Rice wrote:
>> Leo,
>> 
>> On 27/11/16 21:56, Leo Famulari wrote:
>> > Will this work on nachines that don't support SSE? My understanding is
>> > that we don't wish to require it.
>> 
>> Why wouldn't it?
>> 
>> I don't often use Icecat, but when I do... I don't see why it would
>> require SSE by default. ;-) And it's not like we ever patched it to use
>> IJG's libjpeg to begin with.
>
> I'm not sure, but the comment seems to indicate that we don't want to
> use it:
>
> ;; According to
> ;; http://sourceforge.net/projects/libjpeg-turbo/ ,
> ;; "libjpeg-turbo is a derivative of libjpeg that
> ;; uses MMX, SSE, SSE2, and NEON SIMD instructions
> ;; to accelerate baseline JPEG compression/
> ;; decompression", so we had better not use it

SSE is pentium III, and SSE2 was introduced in Pentium IV. MMX is even
older. Are we really committed to supporting 15+ year old hardware?

The NEON SIMD instruction seems to be ARM-specific and is available at
least on Cortex A8, which is an ARMv7 design.
Marius Bakke Nov. 27, 2016, 10:28 p.m. UTC | #5
Marius Bakke <mbakke@fastmail.com> writes:

> Leo Famulari <leo@famulari.name> writes:
>
>> On Sun, Nov 27, 2016 at 10:02:35PM +0100, Tobias Geerinckx-Rice wrote:
>>> Leo,
>>> 
>>> On 27/11/16 21:56, Leo Famulari wrote:
>>> > Will this work on nachines that don't support SSE? My understanding is
>>> > that we don't wish to require it.
>>> 
>>> Why wouldn't it?
>>> 
>>> I don't often use Icecat, but when I do... I don't see why it would
>>> require SSE by default. ;-) And it's not like we ever patched it to use
>>> IJG's libjpeg to begin with.
>>
>> I'm not sure, but the comment seems to indicate that we don't want to
>> use it:
>>
>> ;; According to
>> ;; http://sourceforge.net/projects/libjpeg-turbo/ ,
>> ;; "libjpeg-turbo is a derivative of libjpeg that
>> ;; uses MMX, SSE, SSE2, and NEON SIMD instructions
>> ;; to accelerate baseline JPEG compression/
>> ;; decompression", so we had better not use it
>
> SSE is pentium III, and SSE2 was introduced in Pentium IV. MMX is even
> older. Are we really committed to supporting 15+ year old hardware?

Digging a bit further, SSE2 is actually part of the AMD64
specification[0]. Perhaps we could make it conditional on architecture?

0: https://en.wikipedia.org/wiki/SSE2#CPU_support
Leo Famulari Nov. 27, 2016, 10:43 p.m. UTC | #6
On Sun, Nov 27, 2016 at 10:50:35PM +0100, Marius Bakke wrote:
> Leo Famulari <leo@famulari.name> writes:
> > I'm not sure, but the comment seems to indicate that we don't want to
> > use it:
> >
> > ;; According to
> > ;; http://sourceforge.net/projects/libjpeg-turbo/ ,
> > ;; "libjpeg-turbo is a derivative of libjpeg that
> > ;; uses MMX, SSE, SSE2, and NEON SIMD instructions
> > ;; to accelerate baseline JPEG compression/
> > ;; decompression", so we had better not use it
> 
> SSE is pentium III, and SSE2 was introduced in Pentium IV. MMX is even
> older. Are we really committed to supporting 15+ year old hardware?

See this earlier thread:

http://lists.gnu.org/archive/html/guix-devel/2016-07/msg01534.html

I think it's fine to use libjpeg-turbo if it can run on those older
processors, or if we decide to drop support for them.

> The NEON SIMD instruction seems to be ARM-specific and is available at
> least on Cortex A8, which is an ARMv7 design.

The extremely popular Cortex A7 [0] also has NEON SIMD.

[0] Allwinner A20
Leo Famulari Nov. 27, 2016, 10:44 p.m. UTC | #7
On Sun, Nov 27, 2016 at 11:28:45PM +0100, Marius Bakke wrote:
> Marius Bakke <mbakke@fastmail.com> writes:
> 
> > Leo Famulari <leo@famulari.name> writes:
> >
> >> On Sun, Nov 27, 2016 at 10:02:35PM +0100, Tobias Geerinckx-Rice wrote:
> >>> Leo,
> >>> 
> >>> On 27/11/16 21:56, Leo Famulari wrote:
> >>> > Will this work on nachines that don't support SSE? My understanding is
> >>> > that we don't wish to require it.
> >>> 
> >>> Why wouldn't it?
> >>> 
> >>> I don't often use Icecat, but when I do... I don't see why it would
> >>> require SSE by default. ;-) And it's not like we ever patched it to use
> >>> IJG's libjpeg to begin with.
> >>
> >> I'm not sure, but the comment seems to indicate that we don't want to
> >> use it:
> >>
> >> ;; According to
> >> ;; http://sourceforge.net/projects/libjpeg-turbo/ ,
> >> ;; "libjpeg-turbo is a derivative of libjpeg that
> >> ;; uses MMX, SSE, SSE2, and NEON SIMD instructions
> >> ;; to accelerate baseline JPEG compression/
> >> ;; decompression", so we had better not use it
> >
> > SSE is pentium III, and SSE2 was introduced in Pentium IV. MMX is even
> > older. Are we really committed to supporting 15+ year old hardware?
> 
> Digging a bit further, SSE2 is actually part of the AMD64
> specification[0]. Perhaps we could make it conditional on architecture?
> 
> 0: https://en.wikipedia.org/wiki/SSE2#CPU_support

Sure, if it requires SSE2 to run, and we keep the status quo regarding
i686, I don't see why not.
Tobias Geerinckx-Rice Nov. 27, 2016, 11:15 p.m. UTC | #8
Andreas, Leo,

Thanks for the feedback!

On 27/11/16 22:41, Leo Famulari wrote:
> I'm not sure, but the comment seems to indicate that we don't want to
> use it:
> 
> ;; According to
> ;; http://sourceforge.net/projects/libjpeg-turbo/ ,
> ;; "libjpeg-turbo is a derivative of libjpeg that
> ;; uses MMX, SSE, SSE2, and NEON SIMD instructions
> ;; to accelerate baseline JPEG compression/
> ;; decompression", so we had better not use it

There seems to be some misunderstanding here. Sorry if it wasn't clear:
this (libjpeg-turbo) was *already* bundled and used by icecat, as the
‘media/libjpeg’ (sic) that this patch removes. You'll find the same SIMD
code there. For the quote above is missing:

> ;; Fails with "libjpeg-turbo JCS_EXTENSIONS required".

Translation: icecat wouldn't build with Guix's ‘libjpeg’ even if we'd
want that.

And we don't. This isn't some dodgy go-faster fork of IJG's ‘real’
libjpeg. It's intended as a sane alternative to the IJG library, albeit
one that doesn't implement every recent extension. It detects CPU
features at runtime — although they're the status quo now, as Andreas
notes — and falls back to other algorithms that might still be faster.

Dang it. I thought I wrote good (description)s. :-(

Kind regards,

T G-R-turbo
Tobias Geerinckx-Rice Nov. 27, 2016, 11:20 p.m. UTC | #9
On 28/11/16 00:15, Tobias Geerinckx-Rice wrote:
> this (libjpeg-turbo) was *already* bundled and used by icecat, [...]

Some fortuitous grepping revealed that our spice packages also #warns
about *not* being built with libjpeg-turbo. Absent any known drawbacks,
I'd follow upstreams' recommendations in these matters.

Kind regards,

T G-R
Leo Famulari Nov. 27, 2016, 11:27 p.m. UTC | #10
On Mon, Nov 28, 2016 at 12:15:01AM +0100, Tobias Geerinckx-Rice wrote:
> There seems to be some misunderstanding here. Sorry if it wasn't clear:
> this (libjpeg-turbo) was *already* bundled and used by icecat, as the
> ‘media/libjpeg’ (sic) that this patch removes. You'll find the same SIMD
> code there. For the quote above is missing:

Ah, thanks for the clarification.

> > ;; Fails with "libjpeg-turbo JCS_EXTENSIONS required".
> 
> Translation: icecat wouldn't build with Guix's ‘libjpeg’ even if we'd
> want that.
> 
> And we don't. This isn't some dodgy go-faster fork of IJG's ‘real’
> libjpeg. It's intended as a sane alternative to the IJG library, albeit
> one that doesn't implement every recent extension. It detects CPU
> features at runtime — although they're the status quo now, as Andreas
> notes — and falls back to other algorithms that might still be faster.
> 
> Dang it. I thought I wrote good (description)s. :-(

The description is excellent. Thanks for putting up with my
misunderstanding :)
Leo Famulari Nov. 27, 2016, 11:42 p.m. UTC | #11
On Mon, Nov 28, 2016 at 12:20:53AM +0100, Tobias Geerinckx-Rice wrote:
> On 28/11/16 00:15, Tobias Geerinckx-Rice wrote:
> > this (libjpeg-turbo) was *already* bundled and used by icecat, [...]
> 
> Some fortuitous grepping revealed that our spice packages also #warns
> about *not* being built with libjpeg-turbo. Absent any known drawbacks,
> I'd follow upstreams' recommendations in these matters.

Now that we have the package, I think we should make spice use it.
diff mbox

Patch

diff --git a/gnu/packages/gnuzilla.scm b/gnu/packages/gnuzilla.scm
index 3b00d3c..f7a8c7d 100644
--- a/gnu/packages/gnuzilla.scm
+++ b/gnu/packages/gnuzilla.scm
@@ -403,6 +403,7 @@  standards.")
                       "modules/zlib"
                       "modules/libbz2"
                       "ipc/chromium/src/third_party/libevent"
+                      "media/libjpeg"
                       "media/libvpx"
                       "security/nss"
                       "gfx/cairo"
@@ -432,6 +433,7 @@  standards.")
        ("hunspell" ,hunspell)
        ("libcanberra" ,libcanberra)
        ("libgnome" ,libgnome)
+       ("libjpeg-turbo" ,libjpeg-turbo)
        ("libxft" ,libxft)
        ("libevent" ,libevent)
        ("libxinerama" ,libxinerama)
@@ -497,6 +499,7 @@  standards.")
                            ;; Avoid bundled libraries.
                            "--with-system-zlib"
                            "--with-system-bz2"
+                           "--with-system-jpeg"        ; must be libjpeg-turbo
                            "--with-system-libevent"
                            "--with-system-libvpx"
                            "--with-system-icu"
@@ -517,16 +520,6 @@  standards.")
                            ;; Network Graphics (PNG) format";
                            ;; we probably do not wish to support it.
                            ;; "--with-system-png"
-
-                           ;; Fails with "libjpeg-turbo JCS_EXTENSIONS
-                           ;; required".
-                           ;; According to
-                           ;; http://sourceforge.net/projects/libjpeg-turbo/ ,
-                           ;; "libjpeg-turbo is a derivative of libjpeg that
-                           ;; uses MMX, SSE, SSE2, and NEON SIMD instructions
-                           ;; to accelerate baseline JPEG compression/
-                           ;; decompression", so we had better not use it
-                           ;; "--with-system-jpeg"
                            )
 
        #:modules ((ice-9 ftw)