Last Comment Bug 750447 - Builds broken after libvpx update
: Builds broken after libvpx update
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All FreeBSD
: -- normal (vote)
: mozilla17
Assigned To: Timothy B. Terriberry (:derf)
:
Mentors:
Depends on:
Blocks: 352822 730907
  Show dependency treegraph
 
Reported: 2012-04-30 13:20 PDT by Marco Perez
Modified: 2012-07-18 05:57 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
pass null for variance on plaforms where CONFIG_RUNTIME_CPU_DETECT is 0 (3.16 KB, patch)
2012-05-01 09:11 PDT, Landry Breuil (:gaston)
tterribe: review-
Details | Diff | Splinter Review
Fix libvpx build on platforms without RTCD (7.64 KB, patch)
2012-05-01 10:04 PDT, Timothy B. Terriberry (:derf)
cpearce: review+
Details | Diff | Splinter Review
Wrap &rtcd->common->variance in IF_RTCD (3.56 KB, patch)
2012-07-09 07:28 PDT, Timothy B. Terriberry (:derf)
cpearce: review+
landry: feedback+
Details | Diff | Splinter Review

Description Marco Perez 2012-04-30 13:20:30 PDT
The landing of the patch for bug 730907 unfortunately broke the builds on FreeBSD
(and maybe other tier-2 and tier-3 platforms). The build log contains:


[...]
gmake[5]: Entering directory `/work/fox/obj-x86_64-unknown-freebsd9.0/media/libvpx'
[...]
postproc.c
/work/fox/media/libvpx/vp8/common/postproc.c:451:19: warning: explicitly
      assigning a variable of type 'int' to itself [-Wself-assign]
        for (next = next; next < 256; next++)
             ~~~~ ^ ~~~~
/work/fox/media/libvpx/vp8/common/postproc.c:869:67: error: no member named
      'rtcd' in 'struct VP8Common'
                                                             &cm->rtcd....
                                                              ~~  ^
/work/fox/media/libvpx/vp8/common/postproc.c:886:59: error: no member named
      'rtcd' in 'struct VP8Common'
                                                     &cm->rtcd.variance);
                                                      ~~  ^
1 warning and 2 errors generated.

In the directory  /work/fox/obj-x86_64-unknown-freebsd9.0/media/libvpx
The following command failed to execute properly:
clang -o postproc.o [...] /work/fox/media/libvpx/vp8/common/postproc.c
gmake[5]: *** [postproc.o] Error 1
gmake[5]: Leaving directory `/work/fox/obj-x86_64-unknown-freebsd9.0/media/libvpx'
[...]


I suspect it broke because CONFIG_RUNTIME_CPU_DETECT is set to 0 for generic platforms.
In /media/libvpx/vp8/common/onyxc_int.h rtcd is defined conditionally but in
media/libvpx/vp8/common/postproc.c it is used unconditionally. Maybe there are
other such occurrences throughout the code. Didn't find the time yet to dig deeper.
Comment 1 Landry Breuil (:gaston) 2012-05-01 04:33:21 PDT
Same issue on OpenBSD/ppc with gcc :


/home/landry/src/mozilla-central/media/libvpx/vp8/common/postproc.c: In function 'vp8_multiframe_quality_enhance':
/home/landry/src/mozilla-central/media/libvpx/vp8/common/postproc.c:869: error: 'VP8_COMMON' has no member named 'rtcd'
/home/landry/src/mozilla-central/media/libvpx/vp8/common/postproc.c:886: error: 'VP8_COMMON' has no member named 'rtcd'
Comment 2 Landry Breuil (:gaston) 2012-05-01 09:11:53 PDT
Created attachment 619949 [details] [diff] [review]
pass null for variance on plaforms where CONFIG_RUNTIME_CPU_DETECT is 0

Here's a tentative patch that seems to fix the issue here, at least build goes further than libvpx. Rationale is to pass NULL when CONFIG_RUNTIME_CPU_DETECT is 0, since VARIANCE_INVOKE() ignores first argument if CONFIG_RUNTIME_CPU_DETECT is 0 (see end of media/libvpx/vp8/common/variance.h)
Comment 3 Timothy B. Terriberry (:derf) 2012-05-01 10:02:26 PDT
Comment on attachment 619949 [details] [diff] [review]
pass null for variance on plaforms where CONFIG_RUNTIME_CPU_DETECT is 0

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

The idea here is good, but it needs to be slotted into update.sh so this patch can be applied with the others we apply to libvpx. I'll post a patch that does that shortly.

::: media/libvpx/vp8/common/postproc.c
@@ +805,5 @@
>  }
>  
>  #if CONFIG_RUNTIME_CPU_DETECT
>  #define RTCD_VTABLE(oci) (&(oci)->rtcd.postproc)
> +#define VARIANCE(oci) (&(oci)->rtcd.variance)

I'd have preferred to use IF_RTCD-style macros for both postproc and variance, like the encoder does, but sadly they hard-code the variable name of the encoder struct (cpi).

It's probably not worth the pain to do something cleaner here, sine all this code is gone from upstream, but I think this should at least have an RTCD in the name.
Comment 4 Timothy B. Terriberry (:derf) 2012-05-01 10:04:01 PDT
Created attachment 619959 [details] [diff] [review]
Fix libvpx build on platforms without RTCD
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-05-03 04:00:18 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4373dd60ed
Comment 6 Ed Morley [:emorley] 2012-05-04 03:59:06 PDT
https://hg.mozilla.org/mozilla-central/rev/7a4373dd60ed
Comment 7 Landry Breuil (:gaston) 2012-07-09 02:09:17 PDT
This build breakage came back (likely due to a recent commit to libvpx, or webrtc landing/enabling, didnt find it yet) :

/home/landry/src/mozilla-central/media/libvpx/vp8/encoder/pickinter.c: In function 'pick_intra4x4block':
/home/landry/src/mozilla-central/media/libvpx/vp8/encoder/pickinter.c:135: error: 'VP8_COMMON_RTCD' has no member named 'variance'

Testing a quick fix

-        distortion = get_prediction_error(be, b, &rtcd->common->variance);
+        distortion = get_prediction_error(be, b, RTCD_VARIANCE(rtcd->common));

but i think variance-invoke.patch should be extended to also use RTCD_VARIANCE (or a variation of it) here..

Reopening this bug instead of creating a new one, but if preferred (and if it appears it's a different issue) i can create a separate one.
Comment 8 Landry Breuil (:gaston) 2012-07-09 02:12:39 PDT
Forgot to add this was on OpenBSD/amd64, so not on powerpc like the last time...
Comment 9 Landry Breuil (:gaston) 2012-07-09 02:32:50 PDT
And the new breakage is likely caused by bug #757637 / https://hg.mozilla.org/mozilla-central/rev/a9aeaa818e64 (wild guess)
Comment 10 Timothy B. Terriberry (:derf) 2012-07-09 07:28:18 PDT
Created attachment 640206 [details] [diff] [review]
Wrap &rtcd->common->variance in IF_RTCD

Initial patch. Let me know if this fixes the problem for you.

Try results still coming in: https://tbpl.mozilla.org/?tree=Try&rev=311c8352d807
But I don't expect any problems since this shouldn't change the post-preprocessing code on any platform on try.

Speaking of, if your OpenBSD/amd64 build isn't using the assembler, it probably should be. Feel free to open a bug on that (though I can't fix it myself as I don't have any way to test).
Comment 11 Landry Breuil (:gaston) 2012-07-15 12:33:31 PDT
webrtc was disabled on *bsd in #771981 so the failure temporarly disappeared, but i'll test your patch when trying to fix/enable webrtc on openbsd. I think amd64 uses the assembler (will have to check) , the original issue was on ppc (which doesnt use the assembler)
Comment 12 Landry Breuil (:gaston) 2012-07-16 09:54:46 PDT
(In reply to Timothy B. Terriberry (:derf) from comment #10)

> Speaking of, if your OpenBSD/amd64 build isn't using the assembler, it
> probably should be. Feel free to open a bug on that (though I can't fix it
> myself as I don't have any way to test).

After looking more carefully (see http://mxr.mozilla.org/mozilla-central/source/configure.in#5440) , it seems that the assembler is used in libvpx only on Linux/SunOS/Darwin/WINNT for x86 and x86_64.VPX_X86_ASM is _not_ set on OpenBSD/amd64. I'll file another bug for that, but first i'll try your diff to see if it fixes the last build issue on platforms where CONFIG_RUNTIME_CPU_DETECT is 0.
Comment 13 Landry Breuil (:gaston) 2012-07-16 10:34:04 PDT
Comment on attachment 640206 [details] [diff] [review]
Wrap &rtcd->common->variance in IF_RTCD

I can confirm that with this diff, libvpx builds fine when --enable-webrtc is passed to configure on OpenBSD/amd64 where CONFIG_RUNTIME_CPU_DETECT is 0. Builds fails later on in webrtc but that's bug 771981. (wondering why libvpx builds fine when webrtc is disabled...) Passing r? to cpearce..
Comment 14 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-07-16 12:48:53 PDT
Comment on attachment 640206 [details] [diff] [review]
Wrap &rtcd->common->variance in IF_RTCD

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

Thanks!
Comment 16 Ed Morley [:emorley] 2012-07-18 05:57:56 PDT
https://hg.mozilla.org/mozilla-central/rev/f90923727eee

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