693057 - Update libvpx to v-0.9.7-p1

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Timothy B. Terriberry (:derf)]

This release allows us to drop a number of our in-tree patches and provides some modest decoder speed-ups. It also adds error-concealment support, which will be required for WebRTC.

I'm including patches adding both the encoder and error concealment, but keeping them disabled at compile time. This code has not been through security review, but having it in-tree will be helpful for WebRTC development, and I'd like to get the build system changes they require reviewed now, since configure patches rot faster than a bruised pumpkin in Texas.

Comment 1

[Timothy B. Terriberry (:derf)]

Attachment: Upgrade libvpx decoder to v0.9.7-p1

Comment 2

[Timothy B. Terriberry (:derf)]

Attachment: Add libvpx's error concealment support (disabled)

Comment 3

[Timothy B. Terriberry (:derf)]

Attachment: Add libvpx's encoder support (disabled)

Comment 4

[Timothy B. Terriberry (:derf)]

I ran versions of these patches with error concealment and encoder support hard-coded on, instead of off through try, and they came out greenish:

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e39db7884209

I haven't run the versions with those features disabled through try.

Comment 5

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

Comment on attachment 565733 [details] [diff] [review]
Upgrade libvpx decoder to v0.9.7-p1

Review of attachment 565733 [details] [diff] [review]:
-----------------------------------------------------------------

I'm assuming configure.in and the Makefile.in is all you wanted me to look at.  They look fine.

::: configure.in
@@ +5359,1 @@
>                       ([--with-system-libvpx requested but symbol vpx_codec_dec_init_ver not found]))

Shouldn't this be an AC_MSG_ERROR?

@@ +5362,5 @@
> +            dnl We need at least v0.9.7 to fix several crash bugs (for which we
> +            dnl had local patches prior to v0.9.7).
> +            dnl
> +            dnl This is a terrible test for the library version, but we don't
> +            dnl have a good one. There is no version number in a public header,

lame

@@ +5364,5 @@
> +            dnl
> +            dnl This is a terrible test for the library version, but we don't
> +            dnl have a good one. There is no version number in a public header,
> +            dnl and testing the headers still doesn't guarantee we link against
> +            dnl the right version. While we could call vpx_codec_version() at

double lame

Comment 6

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

Comment on attachment 565734 [details] [diff] [review]
Add libvpx's error concealment support (disabled)

Review of attachment 565734 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +4316,5 @@
>  MOZ_TREMOR=
>  MOZ_WAVE=1
>  MOZ_MEDIA=
>  MOZ_WEBM=1
> +MOZ_VP8_ERROR_CONCEALMENT=0

You don't want the 0 here, I think.

@@ +5338,5 @@
>  MOZ_LIBVPX_LIBS=
>  
>  if test -n "$MOZ_WEBM"; then
>      AC_DEFINE(MOZ_WEBM)
> +    if test -n "$MOZ_VP8_ERROR_CONCEALMENT" ; then

Cause that will trip this test, no?

Comment 7

[Timothy B. Terriberry (:derf)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> > +MOZ_VP8_ERROR_CONCEALMENT=0
> 
> You don't want the 0 here, I think.

Yes, dur. Let me fix that.

Comment 8

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

Comment on attachment 565735 [details] [diff] [review]
Add libvpx's encoder support (disabled)

Review of attachment 565735 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +4317,5 @@
>  MOZ_WAVE=1
>  MOZ_MEDIA=
>  MOZ_WEBM=1
>  MOZ_VP8_ERROR_CONCEALMENT=0
> +MOZ_VP8_ENCODER=0

Again, the same issue with the 0.

@@ +5342,5 @@
>      AC_DEFINE(MOZ_WEBM)
>      if test -n "$MOZ_VP8_ERROR_CONCEALMENT" ; then
>          AC_DEFINE(MOZ_VP8_ERROR_CONCEALMENT)
>      fi
> +    if test -n "$MOZ_VP8_ENCODER" ; then

And here.

Comment 9

[Timothy B. Terriberry (:derf)]

Attachment: Add libvpx's error concealment support (disabled)

Comment 10

[Timothy B. Terriberry (:derf)]

Attachment: Add libvpx's encoder support (disabled)

Comment 11

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

Comment on attachment 565733 [details] [diff] [review]
Upgrade libvpx decoder to v0.9.7-p1

Review of attachment 565733 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libvpx/update.sh
@@ +314,5 @@
>  # Patch to compile with Sun Studio on Solaris
>  patch -p3 < solaris.patch
>  
> +# Patch to fix errors including C headers in C++
> +patch -p3 < compile_errors.patch

Pretty lame that we need this. :(

Comment 12

[Timothy B. Terriberry (:derf)]

Attachment: Add libvpx's error concealment support (disabled) v3

So, proving once again that anything untested is broken, the stuff I had in vpx_config.h to override the libvpx configuration flags with our own flags was broken when the features were actually disabled. Uploading new versions of these patches that fix this problem.

Greenish on try: https://tbpl.mozilla.org/?tree=Try&rev=695b04558e7f (except for OS X64, which appears to have two sets of test results for some reason I can't fathom, but I'm pretty sure that's tbpl being broken, not the patches).

Carrying forward r=cpearce,khuey.

Comment 13

[Timothy B. Terriberry (:derf)]

Attachment: Add libvpx's encoder support (disabled) v3

Comment 14

[Timothy B. Terriberry (:derf)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c7542ce9069a

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/64603f6ddda1
https://hg.mozilla.org/mozilla-central/rev/f5eded31718a
https://hg.mozilla.org/mozilla-central/rev/c7542ce9069a