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 | 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 | 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 | 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 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.