Last Comment Bug 819856 - WebRTC voice engine code disabled on Android
: WebRTC voice engine code disabled on Android
Status: RESOLVED FIXED
[WebRTC][blocking-webrtc-][qa-]
:
Product: Core
Classification: Components
Component: WebRTC: Audio/Video (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla20
Assigned To: Dan Mosedale (:dmose)
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks: android-webrtc
  Show dependency treegraph
 
Reported: 2012-12-09 21:37 PST by Dan Mosedale (:dmose)
Modified: 2012-12-23 13:44 PST (History)
3 users (show)
dmose: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
explicitly un-disable voice engine code for android WIP, v1 (2.06 KB, patch)
2012-12-09 21:37 PST, Dan Mosedale (:dmose)
no flags Details | Diff | Review
re-enable voice engine code for Android, v2 (2.27 KB, patch)
2012-12-21 12:37 PST, Dan Mosedale (:dmose)
rjesup: review+
Details | Diff | Review

Description Dan Mosedale (:dmose) 2012-12-09 21:37:23 PST
Created attachment 690288 [details] [diff] [review]
explicitly un-disable voice engine code for android WIP, v1

Right now, the WebRTC voice engine code doesn't work right on Android because it is explicitly disabled for Android.  The attached patch that ekr and I whipped up on Friday re-enables that code.

It's not currently why the code was explicitly disabled, and whether just re-enabling it this way is at all a good idea; which is what needs some research.
Comment 1 Dan Mosedale (:dmose) 2012-12-21 12:36:51 PST
After discussion with jesup and ekr, the decision has been made to go with a version of this patch (re-enabled some stuff for Android), since it puts the code on a better trajectory: more unit tests pass on Android.  Separately, jesup will ping Niklas to try and understand more of the history about why this stuff was disabled, which will, if we're lucky, be helpful in deciding what to do going forward.
Comment 2 Dan Mosedale (:dmose) 2012-12-21 12:37:35 PST
Created attachment 694957 [details] [diff] [review]
re-enable voice engine code for Android, v2
Comment 3 Dan Mosedale (:dmose) 2012-12-21 13:06:26 PST
With this patch, the mediaconduit_unittests and mediapipeline_unittests pass.  When the signaling_unittests aren't failing for one of various as-yet-undiagnosed-but-presumably-unrelated reasons, all of them now pass!

Try server run, in progress:

https://tbpl.mozilla.org/?tree=Try&rev=39a5d1620124
Comment 4 [:jesup] on pto until 2016/7/5 Randell Jesup 2012-12-21 13:52:32 PST
Comment on attachment 694957 [details] [diff] [review]
re-enable voice engine code for Android, v2

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

::: media/webrtc/trunk/src/voice_engine/voice_engine_defines.h
@@ +411,5 @@
>  
> +  // This macro used to cause the calling function to set an error code and return
> +  // However, not doing that seems to cause the unit tests to pass / behave reasonably,
> +  // so it's disabled for now.  Jesup is going to ping Niklas about the original motivation for the 
> +  // macro, which may help us make a more informed choice about what to do about it.

Replace the "Jesup is going to..." with "See bug 819856"
Comment 5 Dan Mosedale (:dmose) 2012-12-21 14:05:18 PST
Pushed a version with the updated comment to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/03759110bbd7

This made existing broken tests pass, so in-testsuite+ set.

Note You need to log in before you can comment on or make changes to this bug.