Last Comment Bug 693057 - Update libvpx to v-0.9.7-p1
: Update libvpx to v-0.9.7-p1
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Timothy B. Terriberry (:derf)
:
Mentors:
Depends on: 696622 608066 CVE-2012-0823
Blocks: webrtc 688178 763495
  Show dependency treegraph
 
Reported: 2011-10-08 06:19 PDT by Timothy B. Terriberry (:derf)
Modified: 2016-04-07 12:06 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Upgrade libvpx decoder to v0.9.7-p1 (861.07 KB, patch)
2011-10-08 06:25 PDT, Timothy B. Terriberry (:derf)
cpearce: review+
khuey: review+
Details | Diff | Review
Add libvpx's error concealment support (disabled) (29.51 KB, patch)
2011-10-08 06:27 PDT, Timothy B. Terriberry (:derf)
no flags Details | Diff | Review
Add libvpx's encoder support (disabled) (1.48 MB, patch)
2011-10-08 06:28 PDT, Timothy B. Terriberry (:derf)
no flags Details | Diff | Review
Add libvpx's error concealment support (disabled) (29.50 KB, patch)
2011-10-10 11:04 PDT, Timothy B. Terriberry (:derf)
cpearce: review+
khuey: review+
Details | Diff | Review
Add libvpx's encoder support (disabled) (1.48 MB, patch)
2011-10-10 11:05 PDT, Timothy B. Terriberry (:derf)
cpearce: review+
khuey: review+
Details | Diff | Review
Add libvpx's error concealment support (disabled) v3 (29.54 KB, patch)
2011-10-13 17:29 PDT, Timothy B. Terriberry (:derf)
tterribe: review+
Details | Diff | Review
Add libvpx's encoder support (disabled) v3 (1.48 MB, patch)
2011-10-13 17:30 PDT, Timothy B. Terriberry (:derf)
tterribe: review+
Details | Diff | Review

Description Timothy B. Terriberry (:derf) 2011-10-08 06:19:13 PDT
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) 2011-10-08 06:25:51 PDT
Created attachment 565733 [details] [diff] [review]
Upgrade libvpx decoder to v0.9.7-p1
Comment 2 Timothy B. Terriberry (:derf) 2011-10-08 06:27:17 PDT
Created attachment 565734 [details] [diff] [review]
Add libvpx's error concealment support (disabled)
Comment 3 Timothy B. Terriberry (:derf) 2011-10-08 06:28:24 PDT
Created attachment 565735 [details] [diff] [review]
Add libvpx's encoder support (disabled)
Comment 4 Timothy B. Terriberry (:derf) 2011-10-08 06:30:34 PDT
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 [:khuey] (khuey@mozilla.com) 2011-10-10 10:50:45 PDT
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 [:khuey] (khuey@mozilla.com) 2011-10-10 10:53:17 PDT
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) 2011-10-10 10:59:24 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 [:khuey] (khuey@mozilla.com) 2011-10-10 10:59:59 PDT
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) 2011-10-10 11:04:00 PDT
Created attachment 565979 [details] [diff] [review]
Add libvpx's error concealment support (disabled)
Comment 10 Timothy B. Terriberry (:derf) 2011-10-10 11:05:30 PDT
Created attachment 565981 [details] [diff] [review]
Add libvpx's encoder support (disabled)
Comment 11 Chris Pearce (:cpearce) 2011-10-10 13:36:03 PDT
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) 2011-10-13 17:29:32 PDT
Created attachment 566984 [details] [diff] [review]
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) 2011-10-13 17:30:26 PDT
Created attachment 566985 [details] [diff] [review]
Add libvpx's encoder support (disabled) v3
Comment 14 Timothy B. Terriberry (:derf) 2011-10-13 17:42:20 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/c7542ce9069a

Note You need to log in before you can comment on or make changes to this bug.