Closed Bug 750869 Opened 8 years ago Closed 7 years ago

Support WebRTC for Android in our build system

Categories

(Core :: WebRTC, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: jesup, Assigned: gcp)

References

(Blocks 1 open bug)

Details

(Whiteboard: [WebRTC], [blocking-webrtc-], [qa-])

Attachments

(1 file, 9 obsolete files)

10.57 KB, patch
gcp
: review+
Details | Diff | Splinter Review
Need to disable a whole much of linker and config stuff for Android from the gyp files, and modify configure.in to invoke gyp with the right options for Android.
In particular, android-9 (2.3) needed for OpenSLES is not supported by the ELFLoader.  Probably more bugs past that...
Depends on: 750865
Attachment #620050 - Attachment is obsolete: true
FYI, this is the bug I told Joey about which captures partial implementation of Android support for WebRTC.

Without significant back-porting of OpenSLES, this would bump our minimum Android version to 2.3
same patch, un-bitrotted, applies against alder/default
Try output from that build.  Next steps are obvious (muck with gyp files).

https://tbpl.mozilla.org/php/getParsedLog.php?id=12976705&tree=Try
Now builds on try(!)  https://tbpl.mozilla.org/?tree=Try&rev=850fb110f2d3  Not sure why we're getting all orange on the tests; could be the dependency on OpenSLES which is only in Android 2.3 and above.
Attachment #636426 - Attachment is obsolete: true
(In reply to Randell Jesup [:jesup] from comment #7)
> Now builds on try(!)  https://tbpl.mozilla.org/?tree=Try&rev=850fb110f2d3 
> Not sure why we're getting all orange on the tests; could be the dependency
> on OpenSLES which is only in Android 2.3 and above.

Yes, our current Android test boxes are Tegras which run Android 2.2. We have Pandaboards coming online in the near future which run ICS.

As I see it we have two options here:
1) Bump our minimum supported version to 2.3. We might be able to get away with this if we make our separate armv6 builds continue to support 2.2, but it will wreak havoc with our automated testing, per above.
2) Dynamically load the OpenSLES lib so as to not introduce a hard dependency. WebRTC will still fail on < 2.3, but at least the builds will run.
QA Contact: jsmith
Whiteboard: [WebRTC], [blocking-webrtc-]
In last Wednesday's call, Ted said that the current plan is to go with option 2 here.
There were two patches here which were similar but not identical.  My guess is that the oldest one was just forgotten to be marked out-of-date?  jesup, do you recall?

In any case, I took the newer one, and attempted to apply it, and almost all the hunks failed.  So I hand-rebased it, with a couple of exceptions, and it now builds again and Fennec runs.  I've attached that patch as a checkpoint, and marked the version that used to be most recent as obsolete.  

My suspicion is that if one tried to actually use webrtc it would crash.  So, my next steps are:

* get something that runs in mochitest on desktop to try, possibly jsmith's getUserMedia tests that are currently under review

* work on making that pass

* fix up the bits that currently are Android only

Of note are that the current iteration of the patch leaves the Android sdk version at v5 and removed the explicit linkage to OpenSLES, so my expectation is that the first thing I'll need to do to on my way to making a test pass will be to figure out where to the dynamic loading of that library and add that in.
Attachment #636406 - Attachment is obsolete: true
Great!

It's a surprisingly small set of mods in the end; I was surprised originally how few things I had to touch to get it built.

I checked; there is active support for Android in media/webrtc/trunk for capturing audio and video (I actually checked video), so in theory getUserMedia (at least ignoring the UI) should work.  You might need to set the pref to disable UI permission requests.

The mochitests will be a first cut but don't exercise actual hardware cameras, as there are one on the test servers.
Interestingly, even with appropriate prefs set, mozSrcObject on the video element is coming up null, so getUserMedia isn't quite there yet.  Time to debug a bit...
After discussion with jesup, the next step here is to get a version of this patch landed on mozilla-inbound but defaulting to disabled, since it's going to add bloat and possibly other issues that we don't want to work on just yet.  Doing this will make collaboration among the WebRTC devs much easier, because getting an Android build that runs will be as simple adding --enable-webrtc to anyone's .mozconfig.

Next steps after that which will be required along the way to getting things working (probably spinoff bugs) will include getting netwerk/sctp building and dynamically linking with OpenSLES.
This patch seems to fix a regression in earlier versions of the patch that broke the build for x86 targets (i.e. lots of desktop machines).  It also removes the code that defaulted webrtc to on for variety of builds.

Next step is probably to throw this at a try server.
Assignee: rjesup → dmose
Attachment #679878 - Attachment is obsolete: true
rjesup suggested the following try runs:

For this, a try run (all platforms) would be good with the default state (not enabled on android); one enabled would be good too though the tests would fail on non-pandaboards I assume
Comment on attachment 681729 [details] [diff] [review]
Android patches for make system and gyp files (WIP, not working), #4

As mentioned previously in the bug, the intent here is to simply cause this to build a firefox that runs on Android, so that it's straightforward for people to work on the Android port without having to apply this patch.

The current default of --disable-webrtc for Android will remain, and this patch will now generate a running Fennec for --enable-webrtc on Android as well.
Attachment #681729 - Flags: review?(ted)
Attachment #681729 - Flags: feedback?(rjesup)
Comment on attachment 681729 [details] [diff] [review]
Android patches for make system and gyp files (WIP, not working), #4

Try server finds that this has busted linux; removing review requests.  Thanks for the try server help, jesup.  If someone else doesn't get around to fixing the linux bustage by the morning, I'll start poking...
Attachment #681729 - Flags: review?(ted)
Attachment #681729 - Flags: feedback?(rjesup)
Attachment #681729 - Attachment is obsolete: true
Comment on attachment 682455 [details] [diff] [review]
Build system support for --enable-webrtc for Android (off by default)

https://tbpl.mozilla.org/?tree=Try&rev=a9da4605048f
that's with webrtc still disabled for android.  

With it enabled, see:
https://tbpl.mozilla.org/?tree=Try&rev=62229bd072ca
and after a small fix for gyp:
https://tbpl.mozilla.org/?tree=Try&rev=61627676c022
(the SCTP failures were expected)
Attachment #682455 - Flags: review?(ted)
Latest try (webrtc disabled on android) https://tbpl.mozilla.org/?tree=Try&rev=a9da4605048f is all green except for a B2G unagi that I retriggered (infra it appears).
Depends on: 813911
Depends on: 813913
Depends on: 813918
Put back the explicit linkage to OpenSLES to make more build errors go away.  Getting something building is currently a higher priority than making it maximally back-ported.  I've filed bug 815905 to track making this dynamically linked before we enable WebRTC by default on Android.
Attachment #682455 - Attachment is obsolete: true
Attachment #682455 - Flags: review?(ted)
Attachment #685912 - Flags: review?(ted)
Depends on: 816575
No longer depends on: 815883
Depends on: 815883
Comment on attachment 685912 [details] [diff] [review]
Build system support for --enable-webrtc for Android (off by default)

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

::: configure.in
@@ +5216,5 @@
>      MOZ_VP8_ENCODER=1
>      MOZ_VP8_ERROR_CONCEALMENT=1
> +
> +    if test "$OS_TARGET" = "Android"; then
> +       LDFLAGS="$LDFLAGS -lOpenSLES"

You should probably put a comment here mentioning that we want to remove the hard link dependency on OpenSLES.

@@ +8838,5 @@
> +*)
> +# unsupported arch for webrtc
> +    WEBRTC_TARGET_ARCH=unknown
> +# XXX we probably want to disable it entirely for unknown archs
> +#    MOZ_WEBRTC=

I'd rather you either do this or just leave this out entirely.

@@ +8853,5 @@
>     fi
>     EXTRA_GYP_DEFINES="-D MSVS_VERSION=${_MSVS_VERSION} -D MSVS_OS_BITS=${OS_BITS}"
> +
> +elif test "${OS_TARGET}" = "Android"; then
> +   # Not sure why this comes up as 'linux', perhaps due to cross-compile?

This comment isn't super helpful.

::: media/webrtc/trunk/build/common.gypi
@@ +2239,5 @@
> +#                      '-m32',
> +#                      '-mmmx',
> +#                    ],
> +#                  }],
> +#                ],

This doesn't seem like a good thing to disable globally. should this get put inside a "!android" block?
Attachment #685912 - Flags: review?(ted) → review+
Update patch; r+ carried forward.  Still testing in various configs.
Attachment #685912 - Attachment is obsolete: true
Attachment #687306 - Flags: review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #23)

> You should probably put a comment here mentioning that we want to remove the
> hard link dependency on OpenSLES.

Done.

> 
> @@ +8838,5 @@
> > +*)
> > +# unsupported arch for webrtc
> > +    WEBRTC_TARGET_ARCH=unknown
> > +# XXX we probably want to disable it entirely for unknown archs
> > +#    MOZ_WEBRTC=
> 
> I'd rather you either do this or just leave this out entirely.

Uncommented the MOZ_WEBRTC line and got rid of the XXX comment.
 
> > +   # Not sure why this comes up as 'linux', perhaps due to cross-compile?
> 
> This comment isn't super helpful.

Removed.

> ::: media/webrtc/trunk/build/common.gypi
> @@ +2239,5 @@
> > +#                      '-m32',
> > +#                      '-mmmx',
> > +#                    ],
> > +#                  }],
> > +#                ],
> 
> This doesn't seem like a good thing to disable globally. should this get put
> inside a "!android" block?

New patch has what I suspect to be an appropriate fix here.  I'm compiling locally now; if someone wants to eyeball my changes that'd rock.

Next step: throw this patch at the try server.
Attachment #687306 - Flags: feedback?(ted)
Just looking for feedback on common.gypi change.
Comment on attachment 687306 [details] [diff] [review]
685912: Build system support for --enable-webrtc for Android (off by default)

Looks good, thanks.
Attachment #687306 - Flags: feedback?(ted) → feedback+
https://hg.mozilla.org/mozilla-central/rev/0c5dad17fece
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Whiteboard: [WebRTC], [blocking-webrtc-] → [WebRTC], [blocking-webrtc-], [qa-]
Depends on: 857526
No longer depends on: 750865
Depends on: 901202
You need to log in before you can comment on or make changes to this bug.