819856 - WebRTC voice engine code disabled on Android

Status: RESOLVED FIXED

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

Description

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: 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 (:dmosedale, :dmose)]

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 (:dmosedale, :dmose)]

Attachment: re-enable voice engine code for Android, v2

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

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

[Randell Jesup [:jesup] (needinfo me)]

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 (:dmosedale, :dmose)]

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.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/03759110bbd7