libvpx fails to link when building x86 Android

RESOLVED FIXED

Status

()

P5
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: thomas.g.eaton, Assigned: thomas.g.eaton)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

7 years ago
Created attachment 580095 [details]
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
(Assignee)

Updated

7 years ago
OS: Linux → Android
Hardware: x86_64 → x86
Assignee: nobody → thomas.g.eaton
Status: UNCONFIRMED → NEW
Ever confirmed: true
just to note, we're happy to review and take the patches but its not a priority for the upcoming release
Priority: -- → P5
(Assignee)

Comment 2

7 years ago
Created attachment 592175 [details] [diff] [review]
Proposed Patch
Blocks: 723713
(Assignee)

Updated

7 years ago
Attachment #592175 - Flags: review?(roc)
Attachment #592175 - Flags: review?(roc) → review?(tterribe)
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.
Attachment #592175 - Flags: review?(tterribe) → review-
(Assignee)

Comment 4

7 years ago
The upstream solution looks fine.  When will the upstream changes be pulled into the mozilla repository?
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, can some of that testing be off loaded?
(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?
(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.
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).
Depends on: 730907
This seems to be fixed because I can build for Android/x86 locally.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.