566247 - Merge libvpx to mozilla-central

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Need to make a unified patch to land on mozilla-central. I guess we won't be reviewing the VP8 code itself, but we'll need build-system review.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Import libvpx

Import libvpx, the VP8 decoding library. This is not ready for review yet. This builds on Mac x86 (probably Mac x64, though I've not tried that), Win32, Linux x86, and probably Linux x64. I've not tried other platforms, and PPC Macs definitely won't build yet. On Win32 you must have MASM installed (ships in Win7 SDK, or you can download directly from Microsoft.com). On Linux and Mac you need YASM 0.7 or higher.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Build/configure system changes

Build system changes required to build/link all WebM support patches. Without this patch none of the patches in bug 566246 or bug 566245 (or this bug) will actually be built.

These patches are not ready for review yet.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Build/configure system changes v2

Build system changes updated:
* Rename MOZ_VP8 to MOZ_WEBM
* Rename configure option --disable-vp8 --disable-webm
* Merge --disable-libnestegg into --disable-webm
* Change the libvpx's Makefile's to have LIBRARY_NAME vpx_* instead of libvpx_*

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Import libvpx v2

Rebased on build system changes.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 446668 [details] [diff] [review]
Build/configure system changes v2

>+if test -n "$MOZ_WEBM"; then
>+    AC_DEFINE(MOZ_WEBM)
>+    MOZ_SYDNEYAUDIO=1
>+    MOZ_MEDIA=1
>+    MOZ_VORBIS=1
>+fi

With things like this, it's sometimes better not to change options silently.  (Otherwise people get confused if they disable something and it's still being built.)  In other words, instead of:

>+    MOZ_SYDNEYAUDIO=1
>+    MOZ_MEDIA=1
>+    MOZ_VORBIS=1

it might be better to check that these are all defined already (presuming their sections are earlier in configure, which I'd think they would be), and if they aren't, give an error message.  See, for example, the bit of configure.in that says:

if test -n "$SYSTEM_LIBXUL" && test -z "$MOZ_ENABLE_LIBXUL"; then
    AC_MSG_ERROR([--with-system-libxul needs --with-libxul-sdk])
fi



Also, given the requirement for yasm (from media/libvpx/rules.mk), should configure.in be checking (when MOZ_WEBM is defined, and on the relevant platforms based on media/libvpx/platform-detect.mk) that yasm works?  (Or, if there are multiple options for the assembler, use AC_CHECK_PROGS and then use the result in that rules.mk?  Or maybe still use AC_CHECK_PROGS even if there's only one option?)  It's generally much nicer to people trying to build to give errors during configure than give the errors much later in the process.

Comment 6

[Dave Yeo]

(In reply to comment #5)
> (From update of attachment 446668 [details] [diff] [review])
[...] 
> 
> Also, given the requirement for yasm (from media/libvpx/rules.mk), should
> configure.in be checking (when MOZ_WEBM is defined, and on the relevant
> platforms based on media/libvpx/platform-detect.mk) that yasm works?  (Or, if
> there are multiple options for the assembler, use AC_CHECK_PROGS and then use
> the result in that rules.mk?  Or maybe still use AC_CHECK_PROGS even if there's
> only one option?)  It's generally much nicer to people trying to build to give
> errors during configure than give the errors much later in the process.

In theory nasm should also work (and if it doesn't bugs will be filed) so ideally the assembler should be able to be set, as yasm doesn't support all the tier 2 platforms. Also would be nice if there was an option to set the -f <obj-format> parameter to yasm/nasm.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The current plan is to use MASM on Windows, NASM on Mac, and YASM on Linux.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #5)
> (From update of attachment 446668 [details] [diff] [review])
> >+if test -n "$MOZ_WEBM"; then
> >+    AC_DEFINE(MOZ_WEBM)
> >+    MOZ_SYDNEYAUDIO=1
> >+    MOZ_MEDIA=1
> >+    MOZ_VORBIS=1
> >+fi
> 
> With things like this, it's sometimes better not to change options silently. 
> (Otherwise people get confused if they disable something and it's still being
> built.)  In other words, instead of:
> 
> >+    MOZ_SYDNEYAUDIO=1
> >+    MOZ_MEDIA=1
> >+    MOZ_VORBIS=1
> 
> it might be better to check that these are all defined already (presuming their
> sections are earlier in configure, which I'd think they would be), and if they
> aren't, give an error message.


We only support a specified/restricted set of codecs in the container formats we support. We don't actually want (at this stage) the ability to disable individual codecs, we want the supported containers that we've enabled to determine what codecs we support. So we don't have sections earlier in configure.in for --disable-vorbis or the like. We basically only want to enable libvorbis and libsyndeyaudio when we've got either ogg or webm support enabled.

Configuring with --disable-webm shouldn't require you to also configure with --enable-vorbis in order to get Vorbis-in-Ogg support, when building by default congfigures Vorbis-in-Ogg (without requiring --enable-vorbis).

How about I change the configure.in MOZ_ARG_DISABLE_BOOL ogg/vp8, lines so that the `configure --help` text explicitly says that Vorbis won't be disabled unless both Ogg and WebM are disabled?

> 
> Also, given the requirement for yasm (from media/libvpx/rules.mk), should
> configure.in be checking (when MOZ_WEBM is defined, and on the relevant
> platforms based on media/libvpx/platform-detect.mk) that yasm works?

Thanks for pointing that out! This is a good idea, I'll make this change.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Build system v3

* Detect when NASM/MASM isn't installed, and halt during configure with an error if so. Detection for YASM on Linux is there too, but disabled until we get YASM on Linux build machines.
* Add /media/libvpx/$(LIB_PREFIX)vpx.$(LIB_SUFFIX) to layout linkage.
* Make --disable-ogg and --disable-webm more explicit about what they're disabling.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Import libvpx v3

* Disable assembly on Linux and MacOSX x86_64. We can re-enable once we have YASM on the mozilla build Linux machines, and once someone who has a 64bit-capable mac can verify that we can do an x86_64 build with NASM.
* Don't make /media/libvpx/ a component, force it to be a static library like the other media libraries.
* Fix a few typos here and there.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 447269 [details] [diff] [review]
Build system v3

Ted: Can you review the build system changes for the VP8 video decoder backend please?

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 447270 [details] [diff] [review]
Import libvpx v3

Ted: Can you also review the build system changes required to import libvpx? There's no need to review the upstream files (basically all the c/h/asm files).

If you want to build the WebM decoder, you'd need the other patches from bug 566245 and bug 566246, and need to s/MOZ_VP8/MOZ_WEBM/g in the patches in bug 566245. Thanks very much!

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #10)
> once someone who has a 64bit-capable mac can verify that we can do an x86_64 build with NASM.

Matthew Gregan tells me we can't build with the system NASM on 64 bit mac, so we'll have to remove the assembly for 64 bit mac. I also realized I didn't disable the assembly for x86_64 MacOSX properly anyway, so I'll have to regenerate the "import libvpx" patch.

Comment 14

[Matthew Gregan [:kinetik]]

I think we may want to drop Tim's mv_bounds patch and possibly pick up (for the short term, at least) the newer version of his fix linked from bug 566987.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Build system v4

Updated build system to have configure/Make directives MOZ_HAVE_{YASM,MASM,NASM}, so that we can choose at build time which assembler to use from those installed on the user's computer, or fallback to C code if they don't have an assembler. This means that once YASM is installed on the Linux Mozilla build machines, we'll automatically start building using YASM, and won't have to change any of our code.

We *must* have MASM on Windows to build though, as we can't fallback to generic C code on Windows because it doesn't have a stdint.h (before VC Express 2010 anyway), and the generic C code assumes stdint.h exists.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Import libvpx v4

Updates libvpx build system to detect which assembler to use based on what's available on the user's computer.

The assemblers used on each platforms are:
* MASM on Win32 (required)
* NASM on Darwin x86 if available
* YASM on Linux if available
* YASM on Darwin x86_64 if available
* We fallback to generic C code an appropriate assembler isn't found on Mac/Linux.
* We can't compile on Win64 as per my previous comment re: stdint.h.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

YASM has been installed on the Linux mozilla build machines (CentOS and Fedora12 by the looks of it). We'll check here whether YASM 1.0.1 (the same version they installed on the Linux machines) correctly builds libvpx's assmebly on Mac64bit, and we'll file a bug to get that added to Mozilla build machines as well. Then I can rework the patch (again!) to not bother with the fallback. I'd like to get review feedback before reworking my patches again if possible please.

I need to also remember to include Tim Terriberry's new MV bounds patch from bug 566987 comment 6 in my "import libvpx" patch.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Note to self: we need to update the build requirements page when we land this patch, re: assemblers.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 447469 [details] [diff] [review]
Build system v4

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
> dnl ========================================================
> dnl = Disable Ogg Codecs
> dnl ========================================================
> MOZ_ARG_DISABLE_BOOL(ogg,
>-[  --disable-ogg           Disable Ogg Codec support],
>+[  --disable-ogg           Disable Ogg with Theora vidoeo and Vorbis audio support],

Spelling ("video"), also, these comments are a bit more confusing now. Perhaps:
"Disable support for OGG media (Theora video and Vorbis audio)"

> dnl ========================================================
>+dnl = Disable VP8 decoder support
>+dnl ========================================================
>+MOZ_ARG_DISABLE_BOOL(vp8,
>+[  --disable-webm         Disable WebM with VP8 video and Vorbis audio support],
>+    MOZ_WEBM=,
>+    MOZ_WEBM=1)
>+
>+AC_SUBST(MOZ_WEBM)

Generally most of the AC_SUBST lines are down near the bottom of the file. You should probably just put this one there as well.

>+if test -n "$MOZ_WEBM"; then
>+    AC_DEFINE(MOZ_WEBM)
>+    MOZ_SYDNEYAUDIO=1
>+    MOZ_MEDIA=1
>+    MOZ_VORBIS=1
>+    
>+    dnl For Darwin x86 we can use NASM for the assembly.
>+    AC_MSG_CHECKING([for NASM assembler])
>+    AC_CHECK_PROGS(MOZ_HAVE_NASM, nasm, "")

Generally we just do AC_CHECK_PROGS(NASM, $NASM nasm ""), so that:
a) you can pass in a different path to configure (NASM=/path/to/nasm ./configure)
b) The path that's found persists into the build system (call it with $(NASM))

Can you change that (+ the following two checks)?

Also, is it fatal to not have nasm on OS X or yasm on Linux? You don't error out here if they're missing.

>+    dnl For Win32 (MSVC) we must use MASM.
>+    if test "$HOST_OS_ARCH" = "WINNT" -a "$CPU_ARCH" = "x86" -a "$GCC" != "yes"; then
>+        AC_MSG_CHECKING([for MASM assembler])
>+        AC_CHECK_PROGS(MOZ_HAVE_MASM, ml, "")
>+        if test -n "$MOZ_HAVE_MASM"; then
>+            MOZ_HAVE_MASM=1
>+            AC_SUBST(MOZ_HAVE_MASM)
>+        else
>+            AC_MSG_ERROR([Need MASM (ml.exe) in order to assemble libvpx optimized assembly])

Your error message should mention the configure options that a user can pass to work around this (it's just --disable-webm here, right?) If there's a way to install the assembler (aside from installing VC++ Pro), you could list that here too. If it gets to be too much it's fine to put a link to a page on developer.mozilla.org in the error message.

>+AC_SUBST(MOZ_VORBIS)

Move this down as well.

>diff --git a/layout/build/Makefile.in b/layout/build/Makefile.in
>--- a/layout/build/Makefile.in
>+++ b/layout/build/Makefile.in
>+ifndef MOZ_GENERIC_VP8
>+SHARED_LIBRARY_LIBS 	+= \
>+	$(DEPTH)/media/libvpx/vp8/common/x86/$(LIB_PREFIX)vpx_vp8_common_x86.$(LIB_SUFFIX) \
>+	$(DEPTH)/media/libvpx/vp8/decoder/x86/$(LIB_PREFIX)vpx_vp8_decoder_x86.$(LIB_SUFFIX) \
>+	$(NULL)

This seems not-very-future-proofed if we gain non-x86 optimized assembly in the future. Can you wrap this in a CPU check as well? (Or hack platform-detect.mk to indicate which platform's optimized assembly we should be using?)

r=me with those changes.

Comment 20

[(not currently active) Ted Mielczarek]

Comment on attachment 447471 [details] [diff] [review]
Import libvpx v4

>diff --git a/media/libvpx/Makefile.in b/media/libvpx/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/media/libvpx/Makefile.in
>+include $(DEPTH)/config/autoconf.mk
>+include $(topsrcdir)/media/libvpx/platform-detect.mk
>+
>+MODULE = vpx
>+LIBRARY_NAME = vpx
>+FORCE_STATIC_LIB= 1
>+
>+PARALLEL_DIRS += \
>+		vpx_codec \
>+		vpx_mem \
>+		vpx_ports \
>+		vpx_scale \
>+		vp8 \
>+		$(NULL)

Prefer two-space indent for line continuations in makefiles when you're adding new stuff.

>+
>+ASM_SUFFIX=asm

This should probably go in libvpx-rules.mk.

>diff --git a/media/libvpx/nasm2masm-as.sh b/media/libvpx/nasm2masm-as.sh
>new file mode 100644
>--- /dev/null
>+++ b/media/libvpx/nasm2masm-as.sh
>+# NASM to MASM syntax translator plus assembler wrapper script.
>+# Takes standard gcc-style arguments, and calls nasm2masm.py to 
>+# translate the input source file, and then assembles the translated
>+# file using MASM.

I don't understand why you have a shell script that calls a Python script, but I understand that this is a bit of a hack around not having YASM everywhere, so I'm not really going to bother you about it. Presumably we can skip this all if we start shipping YASM with MozillaBuild, right?

>diff --git a/media/libvpx/nasm2masm.py b/media/libvpx/nasm2masm.py
>new file mode 100644
>--- /dev/null
>+++ b/media/libvpx/nasm2masm.py

I'm also not going to review this, for the same reason.

>diff --git a/media/libvpx/platform-detect.mk b/media/libvpx/platform-detect.mk
>new file mode 100644
>--- /dev/null
>+++ b/media/libvpx/platform-detect.mk
>@@ -0,0 +1,46 @@
>+# Detect the platform we're running on, and what assembler we can use, if any.

Can you add a comment here stating what the results of including this Makefile are? I gather it's, "If MOZ_GENERIC_VP8 is set, use generic C code, otherwise use x86-optimized assembly." Per my other review comment, it would be nice if this file also set a variable indicating which platform to use optimized assembly for (even though we only have x86 right now).

>diff --git a/media/libvpx/rules.mk b/media/libvpx/rules.mk
>new file mode 100644
>--- /dev/null
>+++ b/media/libvpx/rules.mk
>+
>+
>+DEFINES += -DHAVE_CONFIG_H=vpx_config.h
>+
>+INCLUDES += \
>+  -I. \
>+  -I$(topsrcdir)/media/libvpx \
>+  -I$(topsrcdir)/media/libvpx/vp8/ \
>+  -I$(topsrcdir)/media/libvpx/vp8/common \
>+  -I$(topsrcdir)/media/libvpx/vp8/common/x86 \
>+  -I$(topsrcdir)/media/libvpx/vp8/decoder \
>+  -I$(topsrcdir)/media/libvpx/vp8/decoder/x86 \
>+  -I$(topsrcdir)/media/libvpx/vpx_codec \
>+  -I$(topsrcdir)/media/libvpx/vpx_mem/ \
>+  -I$(topsrcdir)/media/libvpx/vpx_mem/include \
>+  -I$(topsrcdir)/media/libvpx/vpx_ports/ \
>+  -I$(topsrcdir)/media/libvpx/vpx_scale/ \
>+  $(NULL)
>+
>+# On x86, use optimized assembly routines.
>+ifeq (86,$(findstring 86, $(OS_TEST)))

Shouldn't you be using ifndef MOZ_GENERIC_VP8 (to avoid repeating logic)?

>+
>+  ifeq ($(OS_ARCH),WINNT)
>+    # Translate to MASM syntax and use MASM on Win32.
>+    AS="$(topsrcdir)/media/libvpx/nasm2masm-as.sh"
>+
>+  else ifeq ($(OS_ARCH),Darwin)
>+    AS_DASH_C_FLAG=
>+    ifneq (64,$(findstring 64,$(OS_TEST)))
>+      # Use NASM on x86 Darwin.
>+      AS=nasm
>+      ASFLAGS = -f macho -DPIC
>+    else
>+      # x86_64 Darwin, use YASM.
>+      AS=yasm
>+      ASFLAGS = -f macho64 -DPIC -rnasm -pnasm
>+    endif
>+
>+  else ifeq ($(OS_ARCH),Linux)
>+    # Use YASM on Linux.
>+    AS=yasm
>+    ASFLAGS = -f elf -rnasm -pnasm -DPIC
>+  endif # OS_ARCH

I think it would make more sense to do all of this detection in configure instead. Just set a variable like VPX_AS=$NASM (or whichever program is being used), and VPX_ASFLAGS=whatever, since you're already doing platform detection there. Then you can just set AS=$(VPX_AS) and ASFLAGS=$(VPX_ASFLAGS) in your makefile and be done.

>diff --git a/media/libvpx/update.sh b/media/libvpx/update.sh
>new file mode 100644
>--- /dev/null
>+++ b/media/libvpx/update.sh

This looks useful. Could you also write up a wiki document on how to update the VPX code in-tree? I wrote one for Breakpad updates, for example:
https://wiki.mozilla.org/Breakpad/Updating_to_latest_Breakpad_from_SVN

>diff --git a/media/libvpx/vp8/Makefile.in b/media/libvpx/vp8/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/media/libvpx/vp8/Makefile.in
>+EXPORTS_vpx = \
>+		vp8.h \
>+		vp8cx.h \
>+		vp8dx.h \
>+		vp8e.h \
>+		$(NULL)

Two-space indent please.

>+
>+CSRCS = \
>+		vp8_dx_iface.c \
>+		$(NULL)
>+
>+
>+PARALLEL_DIRS += \
>+		common \
>+		decoder \
>+		$(NULL)

Do you have these subdirs just because the VPX source has them? If all you want to do is compile source out of them, you can instead do:
VPATH += common decoder

and then just add files to CSRCS etc as if they were in the current directory. You can do this for all the subdirs too, like
VPATH += common common/generic decoder
etc.

>+
>+include $(topsrcdir)/config/rules.mk
>+include $(topsrcdir)/media/libvpx/rules.mk
>+
>diff --git a/media/libvpx/vp8/common/Makefile.in b/media/libvpx/vp8/common/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/media/libvpx/vp8/common/Makefile.in
>+
>+PARALLEL_DIRS = \
>+		generic \
>+		$(NULL)
>+

So we always build common/generic, even ifndef MOZ_GENERIC_VP8 ?

>diff --git a/media/libvpx/vp8/common/generic/Makefile.in b/media/libvpx/vp8/common/generic/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/media/libvpx/vp8/common/generic/Makefile.in
>+MODULE		= vpx
>+LIBRARY_NAME	= vpx_vp8_common_generic
>+FORCE_STATIC_LIB= 1
>+
>+CSRCS		= \
>+		systemdependent.c \
>+		$(NULL)

It definitely seems silly to have a separate Makefile for one source file. Just use VPATH as I mentioned above.

r- for moving that makefile goop into configure, and changing the makefiles to use VPATH.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks Ted!

(In reply to comment #19)
> Also, is it fatal to not have nasm on OS X or yasm on Linux? You don't error
> out here if they're missing.

It's not fatal on OS X or Linux if we don't have an assembler; we can fallback to use generic C code. The generic fallback doesn't work on Windows because it doesn't have a stdint.h. I probably could make it work on Windows (I think I'll need to do something for Win64 anyway) but MASM ships in the Windows 7 SDK, so it's likely the user will have it.


(In reply to comment #20)
> 
> I don't understand why you have a shell script that calls a Python script, but
> I understand that this is a bit of a hack around not having YASM everywhere, so
> I'm not really going to bother you about it. Presumably we can skip this all if
> we start shipping YASM with MozillaBuild, right?

Unfortunately YASM doesn't produce object files which can link with the /SAFESEH option (they have a bug filed for this), so we actually can't use YASM on Windows until they fix that. Eventually we should use YASM everywhere, once they fix that.

> >+PARALLEL_DIRS += \
> >+		common \
> >+		decoder \
> >+		$(NULL)
> 
> Do you have these subdirs just because the VPX source has them? If all you want
> to do is compile source out of them, you can instead do:
> VPATH += common decoder

Yeah, I'm just matching the upstream libvpx source tree. Thanks! Using VPATH looks like it will be much cleaner!

> >+PARALLEL_DIRS = \
> >+		generic \
> >+		$(NULL)
> >+
> 
> So we always build common/generic, even ifndef MOZ_GENERIC_VP8 ?

Yup.

Comment 22

[Dave Yeo]

(In reply to comment #21)
[...]
> 
> Unfortunately YASM doesn't produce object files which can link with the
> /SAFESEH option (they have a bug filed for this), so we actually can't use YASM
> on Windows until they fix that. Eventually we should use YASM everywhere, once
> they fix that.

Why not just use nasm on Windows then, it supports linking with /SAFESEH since version 2.03, the license is much the same as of version 2.07 (simplified BSD vs new BSD) and it supports all the architectures.
http://nasm.us/doc/nasmdoc7.html#section-7.5.2

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Because VP8 uses YASM syntax, not just NASM syntax.

Comment 24

[(no longer active)]

Dave in comment 22 accidentally reverted the value of the blocking1.9.3 flag...

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Build system v5

Build system updated W.R.T. Ted's comments. Notable changes:
* Moved the assembler detection into the configure step *in the "import libvpx" patch*.
* Assembly is now linked into media/libvpx/$(LIB_PREFIX)vpx.$(LIB_SUFFIX) directly in media/libvpx/Makefile. So we don't need to do a "ifndef MOZ_GENERIC_VP8 then link in the x86 assembly" in layout/build/Makefile.in.
* Moved all the media AC_SUBST down to the end of configure.in.

I'm carrying forward review. The assembly detection has been moved forward to the "import libvpx" patch, and will need to be reviewed there.

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Import libvpx v5

Changes from previous version:
* Added VPATH to media/libvpx/Makefile.in, and that now lists all C/asm files which we compile I removed all other makefiles in media/libvpx.
* Added assembly/assembler availability detection code to configure.in. We now define VPX_X86_ASM when we have an x86 assembler and we're building on an x86 platform which can build assembly.
* Ted: if you haven't already, it would be good if you can check that I'm using the right platform detection macros in media/libvpx/vpx_config.h/_c.c (around line 29560 of this patch).
* Removed media/libvpx/rules.mk and media/libvpx/platform-detect.mk.
* Removed usage if NASM assembler. We've just got YASM installed on the mac build machines, so now we use MASM on Win32, and YASM on Mac x86, Mac x86_64, and Linux x86. We fallback to C code otherwise.
* Removed the hacks required to get the assembly to build with NASM.
* Renamed NASMtoMASM.* to YASMtoMASM to reflect reality.
* Updated to include Tim Terriberry's latest MV-bound patch.

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #26)
> Created an attachment (id=449158) [details]
> Import libvpx v5

Damn... this is breaking the build on Maemo on TryServer. It looks like VPX_X86_ASM is defined somehow, but I can see from the warning about not having an assmebler in the build log that the configure-code-path which defines VPX_X86_ASM isn't being hit... Weird...

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Import libvpx v6

As "Import libvpx v5", but with an extra AC_SUBST(VPX_X86_ASM) in configure.in, now it builds on maemo...

Comment 29

[Matthew Gregan [:kinetik]]

Looks like the license clarification we've been waiting for has landed in the git repository: https://review.webmproject.org/#change,78

Comment 30

[(not currently active) Ted Mielczarek]

Comment on attachment 449231 [details] [diff] [review]
Import libvpx v6

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -4997,16 +4997,18 @@ MOZ_NO_ACTIVEX_SUPPORT=1
> MOZ_NO_INSPECTOR_APIS=
> MOZ_NO_FAST_LOAD=
> MOZ_OGG=1
> MOZ_SYDNEYAUDIO=
> MOZ_VORBIS=
> MOZ_WAVE=1
> MOZ_MEDIA=
> MOZ_WEBM=1
>+VPX_AS=
>+VPX_ASFLAGS=
> MOZ_PANGO=1
> MOZ_PERMISSIONS=1
> MOZ_PLACES=1
> MOZ_PLAINTEXT_EDITOR_ONLY=
> MOZ_PLUGINS=1
> MOZ_PREF_EXTENSIONS=1
> MOZ_PROFILELOCKING=1
> MOZ_PSM=1
>@@ -5957,16 +5959,60 @@ MOZ_ARG_DISABLE_BOOL(vp8,
>     MOZ_WEBM=,
>     MOZ_WEBM=1)
> 
> if test -n "$MOZ_WEBM"; then
>     AC_DEFINE(MOZ_WEBM)
>     MOZ_SYDNEYAUDIO=1
>     MOZ_MEDIA=1
>     MOZ_VORBIS=1
>+
>+    dnl Detect if we can use an assembler to compile optimized assembly for libvpx.
>+
>+    VPX_X86_ASM=

Any reason not to just stick this up with VPX_{AS,ASFLAGS} above?

>+    dnl For Win32 (MSVC) we must use MASM.
>+    if test "$OS_ARCH" = "WINNT" -a "$CPU_ARCH" = "x86" -a "$GCC" != "yes"; then

I think generally we test with -n "$GNU_CC".

>+    if test -n "$VPX_X86_ASM"; then
>+      AC_DEFINE(VPX_X86_ASM)
>+      VPX_ASFLAGS="$VPX_ASFLAGS"

This line has no effect.

Thanks for adding the verbose error message!



>diff --git a/media/libvpx/vpx_config.h b/media/libvpx/vpx_config.h
>new file mode 100644
>--- /dev/null
>+++ b/media/libvpx/vpx_config.h
>@@ -0,0 +1,27 @@
>+#if defined(VPX_X86_ASM)
>+
>+#if defined(WIN32) && !defined(__GNUC__) && !defined(_M_X64)
>+/* 32 bit Windows, MSVC. */

I think you want _WIN32 here to be correct, per:
http://msdn.microsoft.com/en-us/library/b0084kay.aspx

It also probably makes sense to instead explicitly check defined(_M_IX86).

The rest of the patch looks good, thanks! It's lot cleaner than the first version.

r=me with those nits fixed.

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Import libvpx v6

* Updated to address review comments.
* Updated libvpx to revision 0dd78af3e9b089eacc9af280adfb5549fc7ecdcd. This includes an update license, and a new PATENTS file.
* Added a README_MOZILLA file to media/libvpx to denote the revision.
* I had to add a new patch to subpixel_sse2.asm to make the new libvpx compile on Win32. I'll file a bug upstream for this.

Carrying forward r=ted.

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

> >diff --git a/media/libvpx/update.sh b/media/libvpx/update.sh
> >new file mode 100644
> >--- /dev/null
> >+++ b/media/libvpx/update.sh
> 
> This looks useful. Could you also write up a wiki document on how to update the
> VPX code in-tree? I wrote one for Breakpad updates, for example:
> https://wiki.mozilla.org/Breakpad/Updating_to_latest_Breakpad_from_SVN

I've added one here:
https://wiki.mozilla.org/WebM/Updating_libvpx
With a parent page here:
https://wiki.mozilla.org/WebM

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/1a95c30ec47b
http://hg.mozilla.org/mozilla-central/rev/b71fe75f19bd

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

I have updated https://developer.mozilla.org/En/Developer_Guide/Build_Instructions/Linux_Prerequisites and https://developer.mozilla.org/En/Developer_Guide/Build_Instructions/Mac_OS_X_Prerequisites to include YASM in the prerequisites. The Windows build prerequisites says you need the Windows 7 SDK, which includes MASM, so I've not updated the documentation for Windows.

Comment 35

[Walter Meinl]

Since we have to include some changes before we can build vp8 on OS/2 I tried to build with "--disable-webm" by adding the respective line to my .mozconfig.
However, disabling did not work in autoconf.mk it was still MOZ_WEBM = 1.
I think, there is a mistake in configure in.
+MOZ_ARG_DISABLE_BOOL(vp8,
>+[  --disable-webm         Disable WebM with VP8 video and Vorbis audio support],
When I substitute the first line by
+MOZ_ARG_DISABLE_BOOL(webm,
>+[  --disable-webm         Disable WebM with VP8 video and Vorbis audio 
disabling works perfectly (in autoconf.mk: MOZ_WEBM =)

Comment 36

[(not currently active) Ted Mielczarek]

Walter: thanks for pointing that out. I don't know how I missed that in review!

Comment 37

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks very much Walter, I've filed bug 571116 for your fix.

Comment 38

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This doesn't really matter because we're going to switch to YASM for windows soon, but in general configure error messages should point to mozilla.org pages, not to external links that will get moved by Microsoft or others.