Closed Bug 646254 Opened 13 years ago Closed 13 years ago

libjpeg-turbo requires yasm >0.8.0

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: Gavin, Assigned: justin.lebar+bug)

References

Details

Attachments

(1 file, 1 obsolete file)

Patch v2
4.33 KB, patch
khuey
: review+
Details | Diff | Splinter Review 
See comments in bug 573948. Building libjpeg turbo with an older yasm fails, so we should add a configure check. yasm 1.0.1 apparently works fine, so I suggest checking for:

"$_YASM_MAJOR_VERSION" -gt "1" -o "$_YASM_MAJOR_VERSION" -eq "1"

(or whatever smarter way there is of doing that)
Assignee: nobody → justin.lebar+bug
I suppose perhaps we could alternately disable LIBJPEG_TURBO_AS if the version isn't sufficient? I thought we didn't like silent-behavior-change configure behavior, though...
We still don't like silent changes.
Should we also fix the current LIBJPEG_TURBO_AS behavior, then?
I have no idea what the current behavior is, and it doesn't look like any build peers reviewed the patch ...

If we're auto-disabling functionality though, that should be fixed.
I was thinking we'd throw an error if yasm <  X, unless --disable-libjpeg-turbo is passed, in which case we'll use the unoptimized C routines.

I think we need yasm 1.1.0, at least on Mac and Windows, because bug 573948 comment 66 was made after yasm 1.0.1 was released [1].  Do we want to require 1.0.1 on Linux, or 1.1.0 across the board?

[1] https://github.com/yasm/yasm/commits/v1.0.1
yasm 1.1.0 is available through macports [1], and on Ubuntu 10.10 [2].

[1] http://www.macports.org/ports.php?by=name&substr=yasm
[2] http://packages.ubuntu.com/maverick/yasm
Attached patch Patch v1 (obsolete) — — Splinter Review 
This really should have been done along with the original patch; I'm sorry
I dropped the ball here.

This patch seems to work, although I'm not at all confident in my configure-fu.
khuey, how does it look to you?
Attachment #522889 - Flags: review?(khuey)
Comment on attachment 522889 [details] [diff] [review]
Patch v1

>@@ -6444,24 +6445,37 @@ if test -z "$MOZ_CRASHREPORTER_ENABLE_PE
>    MOZ_CRASHREPORTER_ENABLE_PERCENT=100
> fi
> AC_DEFINE_UNQUOTED(MOZ_CRASHREPORTER_ENABLE_PERCENT, $MOZ_CRASHREPORTER_ENABLE_PERCENT)
> 
> dnl ========================================================
> dnl = libjpeg-turbo configuration
> dnl ========================================================
> 
>+MOZ_ARG_DISABLE_BOOL(libjpeg_turbo,
>+[ --disable-libjpeg-turbo  Disable optimized jpeg decoding routines],
>+  MOZ_LIBJPEG_TURBO=,
>+  MOZ_LIBJPEG_TURBO=1)
>+

Nit: indent 4 spaces

> dnl Detect if we can use yasm to compile libjpeg-turbo's optimized assembly
> dnl files.
>-AC_MSG_CHECKING([for YASM assembler])
>-AC_CHECK_PROGS(LIBJPEG_TURBO_AS, yasm, "")
>-
>-dnl XXX jlebar -- need a yasm version check here.
>-
>-if test -n "LIBJPEG_TURBO_AS"; then
>+
>+if test -n "$MOZ_LIBJPEG_TURBO"; then
>+
>+  AC_MSG_CHECKING([for YASM assembler])
>+  AC_CHECK_PROGS(LIBJPEG_TURBO_AS, yasm, "")
>+
>+  if test -z "$LIBJPEG_TURBO_AS" ; then
>+    AC_MSG_ERROR([yasm is required to build with libjpeg-turbo's optimized JPEG decoding routines, but you do not appear to have yasm installed.  Either install it or configure with --disable-libjpeg-turbo to use the pure C JPEG decoder.  See https://developer.mozilla.org/en/YASM for more details.])
>+  fi
>+
>+  dnl Check for yasm 1.1 or greater.
>+  if test "$_YASM_MAJOR_VERSION" -lt "1" -o \( "$_YASM_MAJOR_VERSION" -eq "1" -a "$_YASM_MINOR_VERSION" -lt "1" \) ; then
>+    AC_MSG_ERROR([yasm 1.1 or greater is required to build with libjpeg-turbo's optimized JPEG decoding routines, but you appear to have version $_YASM_MAJOR_VERSION.$_YASM_MINOR_VERSION.  Upgrade to the newest version or configure with --disable-libjpeg-turbo to use the pure C JPEG decoder.  See https://developer.mozilla.org/en/YASM for more details.])
>+  fi

Can you rely on _YASM_MAJOR_VERSION being defined?  It's not set in a block that's if MOZ_WEBM is it?  MXR is broken or I'd check.

>@@ -6497,17 +6511,19 @@ if test -n "LIBJPEG_TURBO_AS"; then
>   esac
> 
> fi # end have YASM
> 
> if test -n "$LIBJPEG_TURBO_X86_ASM"; then
>   AC_DEFINE(LIBJPEG_TURBO_X86_ASM)
> elif test -n "$LIBJPEG_TURBO_X64_ASM"; then
>   AC_DEFINE(LIBJPEG_TURBO_X64_ASM)
>-else
>+elif test -n "$MOZ_LIBJPEG_TURBO"; then
>+  dnl Warn if we're not building the optimized routines, even though the user
>+  dnl didn't specify --disable-libjpeg-turbo.
>   AC_MSG_WARN([No assembler or assembly support for libjpeg-turbo.  Using unoptimized C routines.])

File a followup on making this AC_MSG_ERROR, and mention --disable-libjpeg-turbo.

r=me assuming that you can rely on _YASM_[MAJOR|MINOR]_VERSION being defined.
Attachment #522889 - Flags: review?(khuey) → review+
>>+elif test -n "$MOZ_LIBJPEG_TURBO"; then
>>+  dnl Warn if we're not building the optimized routines, even though the user
>>+  dnl didn't specify --disable-libjpeg-turbo.
>>   AC_MSG_WARN([No assembler or assembly support for libjpeg-turbo.  Using 
>>                unoptimized C routines.])

> File a followup on making this AC_MSG_ERROR, and mention
> --disable-libjpeg-turbo.

We don't want it to be an error on ARM.  I could special-case it, but the thrust was that it should only be an error if you're building on x86 or x64 and you don't have a new yasm.
> Can you rely on _YASM_MAJOR_VERSION being defined?  It's not set in a block
> that's if MOZ_WEBM is it?

No, but it is set in:

> AC_CHECK_PROGS(YASM, yasm, "")
> if test -n "$YASM"; then
>   _YASM_MAJOR_VERSION=`...`

Is it OK that I'm not guarding on $YASM specifically?
Still want that follow-up?  You should only be able to get to the warning if you're on ARM or an architecture not listed in the case statement, afaict.
I suppose not ... I'm just worried we'll accidentally switch it off on tinderbox or something like we did to PGO :-/
(In reply to comment #9)
> >>+elif test -n "$MOZ_LIBJPEG_TURBO"; then
> >>+  dnl Warn if we're not building the optimized routines, even though the user
> >>+  dnl didn't specify --disable-libjpeg-turbo.
> >>   AC_MSG_WARN([No assembler or assembly support for libjpeg-turbo.  Using 
> >>                unoptimized C routines.])
> 
> > File a followup on making this AC_MSG_ERROR, and mention
> > --disable-libjpeg-turbo.
> 
> We don't want it to be an error on ARM.  I could special-case it, but the
> thrust was that it should only be an error if you're building on x86 or x64 and
> you don't have a new yasm.

Actually, unless I am mistaken, the patch you wrote makes you error on any platform without YASM, that doesn't --disable-*. (Line 6467 from patch)

Unless I misunderstood what your goal here is, specifically.
(In reply to comment #14)
> Actually, unless I am mistaken, the patch you wrote makes you error on any
> platform without YASM, that doesn't --disable-*. (Line 6467 from patch)
> 
> Unless I misunderstood what your goal here is, specifically.

Ah, yes.  Good point.
Hm, I just pushed.
Oh, actually, no!  The tree closure hook saved me.
Perhaps you should move the YASM block after the rest of the libjpeg-turbo stuff and then check for YASM if the platform ASM variables are set.
Hmm, that probably won't work either.  /me sits down to actually read this code.
OK, yeah.  We don't actually test YASM later in this section, so you ought to be able to move the YASM check to the end of libjpeg-turbo block and then guard on LIBJPEG_TURBO_[X86|X64]_ASM.
Attached patch Patch v2 — — Splinter Review 
Like so?
Attachment #522992 - Flags: review?(khuey)
Attachment #522889 - Attachment is obsolete: true
Comment on attachment 522992 [details] [diff] [review]
Patch v2

Yeah, this looks good.
Attachment #522992 - Flags: review?(khuey) → review+
http://hg.mozilla.org/mozilla-central/rev/76c3ea5e679a
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 647081
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: