835973 - getUserMedia video capture fails because Android JNI classes not built and packaged

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, enhancement)

Description

[Dan Mosedale (:dmosedale, :dmose)]

Running with the patch in bug 830942 applied, the first roadblock that we hit is the logs reveal that the Java classes supplied by webrtc.org can't be found.  Not totally surprising, since they aren't yet built and packaged.  

In order to figure out all the various build flags, defines, etc are required, I pulled webrtc.org 3.12 using the info on <http://www.webrtc.org/reference/getting-started/prerequisite-sw> and at the very bottom of <http://www.webrtc.org/reference/getting-started>.  Note that instead of using <https://webrtc.googlecode.com/svn/trunk> as the argument to "gclient config", I used <http://webrtc.googlecode.com/svn/branches/3.12/>.

Because things have moved around some since then, I still had to tweak things a bit to make it all work.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: webrtc-3.12 branch changes to build with moz toolchain

I had to symlink 3.12 to trunk/ and make some tweaks to the code to make it build with the ndk and sdk versions that Mozilla uses; this diff has the code changes.

I got a demo app that worked (ie captured and displayed video) on my Samsung Galaxy SIII.  The code in the tree seems to want android NDK platform version 10, it'll be interesting to see if/how that effects our Fennec usage.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: build output from ant

The real point of this exercise was to see the various flags that are needed to build something that works.  Attached is the build output from ant.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: WIP java build patch, v1

This patch manages to build up the relevant classes into a jar, and might even be putting that jar into the dex.  Nonetheless, it's still crashing on the gUM test page video button press with:

D/*WEBRTCN*( 9293): SetAndroidObjects: could not find java capture class

gcp, if you're up for doing some debugging here, that'd be awesome...

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

FWIW, the relevant patches in my mq are:

activity-context-jni
video-jni-wiring
java-build

though the first one is probably entirely unnecessary.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Spent some quality time with this today.  Verified that the classes are in the dex file.  At snorp's suggestion, I tried moving the class-loading to AndroidBridge::Init, and that seemed to not blow-up.  

Testing an adaptation of the strategy & code pointed to from <https://groups.google.com/forum/#!msg/android-ndk/eYL9tYkvefs/ORZezJiLXFUJ> resulted in the exact same error.

Next simple test, I think, is to see if we can load the capture class from AndroidBridge::Init, and then create and cache a global reference, which we can then pull out of AndroidBridge in or near video_capture_android.cc instead of calling FindClass.  Doing this with all 6 classes may well be fine for our demo.

Since we won't want to take the startup perf hit and memory bloat for cases where users don't use WebRTC, to actually land this on trunk, I expect a better "fix" eventually be to write a FindClass-like method for AndroidBridge that proxies class-loading to the main thread .

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: WIP patch to initialize classes in the Android bridge, v1

Patch from gcp.  It creates a global refs to classes and then passes them around.  There's at least one class that needs to be added, current theory is to try to actually hold them in the AndroidBridge and just call into the bridge to get them rather than passing them around.  This will first require making AndroidBridge.h be usable from the WebRTC, which is hopefully just a matter of adding some missing includes.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

So the problem that AndroidBridge.h was hitting was that it has string-related dependencies that require MOZILLA_INTERNAL_API.  Just adding that to video_capture_android.cc causes the unit tests to blow up at link time.  

It might work to make a few bits of the webrtc.org code do what the signaling code does: build those files twice, once for linkage with the tests (without MOZILLA_INTERNAL_API), and once for linkage with the browser (with it).  Doing so is not as simple as copying the appropriate clause from signaling.gyp, as the webrtc.org code doesn't definite the gyp symbol that depends on (build_for_test).  So next steps for this route include investigating how hard that would be, and whether it would be a good idea and or even work at all.  No part of our copy of the webrtc.org code builds with MOZILLA_INTERNAL_API currently, from what I can see, and flipping that selectively could conceivably have weird consequences.  Or maybe not.  I suspect ekr can enlighten us a bit here. 

If we do get the above working, it might make sense to just use WrapRunnableRet from runnable_utils.h and friends to synchronously load the classes on the main thread (possibly, though not necessarily, through a method that we add to AndroidBridge).

But for now, my suspicion is that the short path to get something working to demo is to move forward with gcp's patch strategy, possibly with the tweak of actually creating a structure to hold references to those six classes (video capture has 3, video render has 3, IIRC).

A complication here is that gcp's patch doesn't seem to work as well on my phone as it does on his.  I'll post a stack here shortly.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

ekr: would love any thoughts on comment 7 you have to offer

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Here's the stack trace from looking at video on the gUM page I mentioned earlier:

#0  0x40947fde in dvmAbort () from /Users/dmose/r/tools/android-gdb/moz-gdb/lib/7d0c479f/system/lib/libdvm.so
#1  0x409294e8 in IndirectRefTable::get(void*) const () from /Users/dmose/r/tools/android-gdb/moz-gdb/lib/7d0c479f/system/lib/libdvm.so
#2  0x4094cca6 in dvmDecodeIndirectRef(Thread*, _jobject*) () from /Users/dmose/r/tools/android-gdb/moz-gdb/lib/7d0c479f/system/lib/libdvm.so
#3  0x40964338 in dvmCallMethodV(Thread*, Method const*, Object*, bool, JValue*, std::__va_list) () from /Users/dmose/r/tools/android-gdb/moz-gdb/lib/7d0c479f/system/lib/libdvm.so
#4  0x4094ca22 in ?? () from /Users/dmose/r/tools/android-gdb/moz-gdb/lib/7d0c479f/system/lib/libdvm.so
#5  0x6789c718 in _JNIEnv::CallStaticObjectMethod (this=0xd5d408, clazz=0x1d200282, methodID=0x57126c80) at /usr/local/android-ndk-r8c/platforms/android-9/arch-arm/usr/include/jni.h:779
#6  0x689afe3a in webrtc::videocapturemodule::VideoCaptureAndroid::SetAndroidObjects (javaVM=0x8ed8a8, javaContext=0x1d200001, jCmClass=0x1d20027e, jCmDevInfoClass=0x1d200282) at /Users/dmose/r/alder/src/media/webrtc/trunk/src/modules/video_capture/main/source/android/video_capture_android.cc:160
#7  0x689afb1a in webrtc::SetCaptureAndroidVM (javaVM=0x8ed8a8, javaContext=0x1d200001, jCmClass=0x1d20027e, jCmDevInfoClass=0x1d200282) at /Users/dmose/r/alder/src/media/webrtc/trunk/src/modules/video_capture/main/source/android/video_capture_android.cc:30
#8  0x68a25866 in webrtc::VideoEngine::SetAndroidObjects (javaVM=0x8ed8a8, javaContext=0x1d200001, jCmClass=0x1d20027e, jCmDevInfoClass=0x1d200282) at /Users/dmose/r/alder/src/media/webrtc/trunk/src/video_engine/vie_impl.cc:215
#9  0x678ec7cc in mozilla::MediaEngineWebRTC::EnumerateVideoDevices (this=0x6c8e1a00, aVSources=0x6c9ffdcc) at /Users/dmose/r/alder/src/content/media/webrtc/MediaEngineWebRTC.cpp:59
#10 0x67666d1c in mozilla::GetUserMediaDevicesRunnable::Run (this=0x61495920) at /Users/dmose/r/alder/src/dom/media/MediaManager.cpp:750
#11 0x682abb6e in nsThread::ProcessNextEvent (this=0x6c0fcb00, mayWait=true, result=0x6c9ffe8f) at /Users/dmose/r/alder/src/xpcom/threads/nsThread.cpp:627
#12 0x6824cefe in NS_ProcessNextEvent_P (thread=0x6c0fcb00, mayWait=true) at /Users/dmose/r/alder/objdir-droid/xpcom/build/nsThreadUtils.cpp:238
#13 0x682aac7c in nsThread::ThreadFunc (arg=0x6c0fcb00) at /Users/dmose/r/alder/src/xpcom/threads/nsThread.cpp:265
#14 0x623d2eb2 in _pt_root (arg=0x6c0fcb70) at /Users/dmose/r/alder/src/nsprpub/pr/src/pthreads/ptthread.c:156
#15 0x400a7dd0 in __thread_entry () from /Users/dmose/r/tools/android-gdb/moz-gdb/lib/7d0c479f/system/lib/libc.so
#16 0x400a7924 in pthread_create () from /Users/dmose/r/tools/android-gdb/moz-gdb/lib/7d0c479f/system/lib/libc.so
#17 0x00000000 in ?? ()

More interestingly, logcat reveals this:

D/*WEBRTCN*( 1083): SetAndroidObjects: Registered native functions
D/*WEBRTCN*( 1083): VideoCaptureDeviceInfoAndroid get method id
D/*WEBRTCN*( 1083): SetAndroidObjects: construct static java device object
E/dalvikvm( 1083): JNI ERROR (app bug): accessed stale local reference 0x1d200001 (index 0 in a table of size 0)

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

I have another theory of an approach that I'm hopeful could work and be cleaner too, particularly, if it's possible to use WrapRunnableRet, Dispatch, etc without entraining a MOZILLA_INTERNAL_API requirement.  The idea here is to split the stuff in AndroidBridge.h with external C linkage (at the very least, jsjni_FindClass) into its own header which is included both by AndroidBridge.h as well as other things that need it.

video_capture_android.cc, for example, could then use WrapRunnableRet to proxy that call over to the main thread to load classes as needed instead of the existing env->FindClass stuff that's being used.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: WIP video hackery patch, v1

OK, here's a patch that, while it has various issues, is a good replacement for the android bridge patch, so I'm marking that one obsolete (gcp, feel free to disagree and revert if you wish).

We now are able to get classes and call into them, and the logs suggest that the camera is now being probed, so we've made a bunch of progress.  Current thing to fix is seeing this crash:

W/dalvikvm(11558): Invalid indirect reference 0x41e4e118 in decodeIndirectRef
E/dalvikvm(11558): VM aborting

On the alder equivalent of this line:

https://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/src/modules/video_capture/main/source/android/device_info_android.cc#113

This fixes the problem I was seeing about stale local cache stuff, it turns out that GetContext needs to either return a global ref, or callers need to be expected to globalize the ref they get.  I'll need to pull these changes out into the GetContext patch at some point.

It also fixes the various linkage problems we had with previous approaches by making the class-loading code live with the rest of the AndroidJNIWrapper bits.

It introduces a dependency on that code on the runnable_utils stuff, which I expect will need to be excised before landing (or, better, move runnable_utils someplace more global, like xpcom or even xpcom glue).

Some interesting docs I've stumbled across along the way:

http://android-developers.blogspot.com/2011/11/jni-local-reference-changes-in-ics.html
http://stackoverflow.com/questions/11055609/invalid-indirect-reference-on-newobject-call

The next step I want to try, just in case we're seeing some weird platform-level incompatibility here, is to try changing the target platform version for Fennec from 9 to 10, since that's what the WebRTCDemo app targets.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: WIP video hackery patch, v2

Updated video hackery patch.  We're no longer crashing on the gUM test page video test at all, and the page claims success, but nothing's actually being rendered.  

After spending a bunch of quality time with the debugger, it's clear that the external renderer is being properly set, but that VideoRenderFrames::FrameToRender is never actually returning anything usable to Render to IncomingVideoStream::IncomingVideoStreamProcess(), so the MediaEngineWebRTCVideo version of DeliverFrame never gets called at all.

I'm beginning to suspect that the capturing isn't working right, but it's possible that there's something deeper in the rendering that's broken.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

OK, so I think part of what's happening is that VideoCapturedAndroid.java is never getting a surfaceChanged() callback, which means that tryStartCapture bails out, because isSurfaceReady is not set.  I suspect we need to hook the capture classes into our Java widget layer somehow.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Apparently Android's camera object wants to have an Android surface to render preview frames directly into, and supposedly won't work if it doesn't.  It's not clear to me how or if this is really compatible with the DOM WebRTC previewing model and/or the browser code; that'll be the next thing to figure out.  Though if anyone happens to know offhand, feel free to drop some science here.  :-)

Comment 15

[Timothy B. Terriberry (:derf)]

(In reply to Dan Mosedale (:dmose) from comment #14)
> doesn't.  It's not clear to me how or if this is really compatible with the
> DOM WebRTC previewing model and/or the browser code; that'll be the next

No, that is not compatible with the WebRTC APIs. There is no requirement that the captured stream be rendered locally at all. The only way it ever would be is if the JS in the page hooks it up to a <video> tag.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

It appears that it's possible to use dummy surfaces to achieve the "no preview" use case, though it seems that that may not work on all phones.  Some key links are:

http://stackoverflow.com/questions/9744790/android-possible-to-camera-capture-without-a-preview
http://stackoverflow.com/questions/7398897/how-to-use-camera-to-take-picture-in-a-background-service-on-android

I suspect that the the external rendering implementation wasn't really ever tested or used with the Android Java classes.  That said, if we're lucky, it won't be too tough graft in a dummy surface for preview to get use further along.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

You can use SurfaceTexture on ICS and higher as well. You'll be able to render that in OpenGL without any Android widget stuff.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Java build patch has been landed on Alder:

https://hg.mozilla.org/projects/alder/rev/7e391d58f073

Next steps:

* split other patches into their own bugs

* rebase java build patch to trunk
* split out the classes.dex dependency on webrtc.jar and conditionalize it on MOZ_WEBRTC
* request review

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: WIP java build patch, v2 (landed on alder)

Here's the Java build patch that actually landed on alder, and to which the comments above apply.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: java build bustage fix, v1 (landed on alder)

The above patch blew up on alder because of a reference to the Java rendering code that we don't actually used.  Removed here.  This will also need to be uplifted.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Build bustage fix landing URL:

https://hg.mozilla.org/projects/alder/rev/bedb07ac0a0f

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Split out the GetGlobalClassRef implementation into bug 839836.

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 711078 [details] [diff] [review]
WIP video hackery patch, v2

Remainder of video-hackery patch split out into bug 839841; marking this patch as obsolete.

Comment 24

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Build Java WebRTC classes

Rebased to m-c.

Comment 25

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 2. Remove unused code.

Comment 26

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 3. Fixup code for m-c getmainhander changes.

getMainhandler was changed in m-c, update code to new style.

Comment 27

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

Comment on attachment 731891 [details] [diff] [review]
Patch 1. Build Java WebRTC classes

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

Let's build webrtc.jar in media/webrtc/trunk/src/modules/video_capture/main/source/android/java

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 731891 [details] [diff] [review]
Patch 1. Build Java WebRTC classes

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

Considering how webrtc stuff is being built and updated, I think building from mobile/android/base is the least worse option.

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 731891 [details] [diff] [review]
Patch 1. Build Java WebRTC classes

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

::: mobile/android/base/Makefile.in
@@ +228,5 @@
> +  VideoCaptureDeviceInfoAndroid.java \
> +  $(NULL)
> +
> +WEBRTC_JAVA_FILES = \
> +  $(addprefix ../../../media/webrtc/trunk/src/modules/video_capture/main/source/android/java/org/webrtc/videoengine/, $(WEBRTC_VIDEO_CAPTURE_JAVA_FILES)) \

use $(DEPTH) here

Comment 30

[Gian-Carlo Pascutto [:gcp]]

To elaborate: inside media/webtrc Makefile.in processing is entirely disabled (though exceptions can be added, but that's not making things cleaner), which makes moving the Makefile(.in) inside that tree problematic. The other solution is to expand our gyp-to-Makefile translator to handle compiling Java and making jars.

Given those options...

Comment 31

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 4. Don't try to use the webrtc JAR is webrtc is disabled.

Comment 32

[Mike Hommey [:glandium]]

Comment on attachment 732421 [details] [diff] [review]
Patch 4. Don't try to use the webrtc JAR is webrtc is disabled.

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

::: mobile/android/base/Makefile.in
@@ +1085,5 @@
>    res/menu-large-v11            \
>    res/menu-xlarge-v11           \
>    $(NULL)
>  
> +ALL_JARS = \

ALL_JARS is not entirely true. There are more jars than these two. Which bears the question: shouldn't they all be here?

@@ +1090,5 @@
> +  jars/gecko-browser.jar \
> +  $(NULL)
> +
> +ifdef MOZ_WEBRTC
> +ALL_JARS += \

You can put this one on one line.

Comment 33

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/186883a36aa0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7a0f01292af
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb3e0f8f8bd

Comment 34

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6dfce2d61cd

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/186883a36aa0
https://hg.mozilla.org/mozilla-central/rev/f7a0f01292af
https://hg.mozilla.org/mozilla-central/rev/2cb3e0f8f8bd
https://hg.mozilla.org/mozilla-central/rev/b6dfce2d61cd