708718 - libvpx fails to link when building x86 Android

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P5)

Description

[Thomas G Eaton]

Attachment: moz_x86_android

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:7.0.1) Gecko/20100101 Firefox/7.0.1
Build ID: 20110928224508

Steps to reproduce:

I tried to build Fennec for x86 Android


Actual results:

It failed to build with a linking error indicating that it couldn't link to rand.  


Expected results:

In the x86 Android NDK, rand is declared as an inline function.  Because of this, it can't be externed as is done in two libvpx .asm files.  Below is a patch that fixes the issue.  

diff -r aa6ad7ab9b43 media/libvpx/Makefile.in
--- a/media/libvpx/Makefile.in	Mon Nov 28 10:34:54 2011 +0000
+++ b/media/libvpx/Makefile.in	Tue Dec 06 08:33:09 2011 -0800
@@ -240,6 +240,7 @@
   vp8_asm_stubs.c \
   x86_systemdependent.c \
   x86_dsystemdependent.c \
+  fix_x86.c
   $(NULL)
 
 ASFILES += \
diff -r aa6ad7ab9b43 media/libvpx/vp8/common/fix_x86.c
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/media/libvpx/vp8/common/fix_x86.c	Tue Dec 06 08:33:09 2011 -0800
@@ -0,0 +1,8 @@
+#include <stdlib.h>
+
+/*extern long lrand48(void);*/
+
+int rand_not_inline(void)
+{
+    return rand();
+}
diff -r aa6ad7ab9b43 media/libvpx/vp8/common/x86/postproc_mmx.asm
--- a/media/libvpx/vp8/common/x86/postproc_mmx.asm	Mon Nov 28 10:34:54 2011 +0000
+++ b/media/libvpx/vp8/common/x86/postproc_mmx.asm	Tue Dec 06 08:33:09 2011 -0800
@@ -464,7 +464,7 @@
 ;                            unsigned char whiteclamp[16],
 ;                            unsigned char bothclamp[16],
 ;                            unsigned int Width, unsigned int Height, int Pitch)
-extern sym(rand)
+extern sym(rand_not_inline)
 global sym(vp8_plane_add_noise_mmx)
 sym(vp8_plane_add_noise_mmx):
     push        rbp
@@ -476,7 +476,7 @@
     ; end prolog
 
 addnoise_loop:
-    call sym(rand) WRT_PLT
+    call sym(rand_not_inline) WRT_PLT
     mov     rcx, arg(1) ;noise
     and     rax, 0xff
     add     rcx, rax
diff -r aa6ad7ab9b43 media/libvpx/vp8/common/x86/postproc_sse2.asm
--- a/media/libvpx/vp8/common/x86/postproc_sse2.asm	Mon Nov 28 10:34:54 2011 +0000
+++ b/media/libvpx/vp8/common/x86/postproc_sse2.asm	Tue Dec 06 08:33:09 2011 -0800
@@ -629,7 +629,7 @@
 ;                            unsigned char whiteclamp[16],
 ;                            unsigned char bothclamp[16],
 ;                            unsigned int Width, unsigned int Height, int Pitch)
-extern sym(rand)
+extern sym(rand_not_inline)
 global sym(vp8_plane_add_noise_wmt)
 sym(vp8_plane_add_noise_wmt):
     push        rbp
@@ -641,7 +641,7 @@
     ; end prolog
 
 addnoise_loop:
-    call sym(rand) WRT_PLT
+    call sym(rand_not_inline) WRT_PLT
     mov     rcx, arg(1) ;noise
     and     rax, 0xff
     add     rcx, rax

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

just to note, we're happy to review and take the patches but its not a priority for the upcoming release

Comment 2

[Thomas G Eaton]

Attachment: Proposed Patch

Comment 3

[Timothy B. Terriberry (:derf)]

Comment on attachment 592175 [details] [diff] [review]
Proposed Patch

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

Not that there is anything wrong with this approach per se, but this has already been fixed upstream using a slightly different method: https://gerrit.chromium.org/gerrit/15744

It'd be better to just use that approach, unless you have a specific reason not to. Also, patches to the 3rd-party code in media should be checked into the repository in a separate .patch that can be applied with the appropriate update script (media/libvpx/update.sh). See https://wiki.mozilla.org/WebM/Updating_libvpx for details.

Comment 4

[Thomas G Eaton]

The upstream solution looks fine.  When will the upstream changes be pulled into the mozilla repository?

Comment 5

[Timothy B. Terriberry (:derf)]

As soon as I get a chance to review and test them (sorry I don't have a better estimate, but probably weeks, not months).

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

derf, can some of that testing be off loaded?

Comment 7

[Makoto Kato [:m_kato]]

This is already fixed on upstream. 

http://git.chromium.org/gitweb/?p=webm/libvpx.git;a=commit;h=7989bb7fe74aa1a3e477b2228e32424376c2c5e9

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Timothy B. Terriberry (:derf) from comment #5)
> As soon as I get a chance to review and test them (sorry I don't have a
> better estimate, but probably weeks, not months).

Derf, any update?

Comment 9

[Timothy B. Terriberry (:derf)]

(In reply to Brad Lassey [:blassey] from comment #8)
> Derf, any update?

I can get a patch ready this week, if you want. I was holding off while m-c was approval-only.

Comment 10

[Timothy B. Terriberry (:derf)]

Patches are now posted in bug 730907. Sorry it didn't happen last week (I had basic patches ready last Saturday, but they triggered B2G failures which required toolchain changes and native Fennec crashes I had to track down and fix).

Comment 11

[(not currently active) Ted Mielczarek]

This seems to be fixed because I can build for Android/x86 locally.