859430 - Add Bluetooth headset functionality to WebRTC calls

Status: NEW

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

Description

[cary.bran]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20130404 Firefox/23.0
Build ID: 20130404030859

Steps to reproduce:

Created a webrtc call on an android phone that is paired and connected to a bluetooth headset


Actual results:

The call progresses as normal, however the headset is only able to receive audio, there is no transmission of audio through the headset's microphone


Expected results:

The headset should function as a headset (receive/transmit audio) when on a WebRTC call.

Comment 1

[Maxim Leykin]

Attachment: Logic to support switching between device and BT headset

Patch contains necessary logic to use AudioDeviceAndroidJni interface for audio device selection. Also AudioDeviceAndroidJni class was optimized to work with Bluetooth headsets. Since current implementation of OpenSL ES library for Android doesn’t support the feature of selecting audio device it was disabled.
Allows to switch sound between device and BT headset either before answering or during the call. There should be no delays. The playback frequencies of the record were changed from 44KHz to 16KHz. Buffer's size was changed accordingly.
Added changes allows to register Bluetooth receiver only once and Bluetooth initialization was performed through startBluetoothSCO().

Important! This patch is dependent on the patch created for Bug 834772 - Add support for Hands Free Profile to Firefox for Android

Comment 2

[Aaron Train [:aaronmt]]

Did you mean to obsolete that patch or were you looking for a review from a module peer?

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 735063 [details] [diff] [review]
Logic to support switching between device and BT headset

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

::: a/media/webrtc/trunk/src/voice_engine/Android.mk
@@ +54,1 @@
>  

We specifically use the OpenSL audio backend on Android because the JNI one has an additional latency of +-100ms (at least, that's what someone tested it to be, IIRC) caused by having to upcall from C++ through JNI to Java and back again. 100ms latency is bad enough that it makes the voice calling experience significantly worse. Perhaps it's not big of a difference right now because other parts of our stack are not optimized and the total latency is relatively greater, but being stuck with JNI would prevent an optimal experience as soon as those are addressed.

I don't think any patch that switches us back to JNI could get approved unless it's demonstrated that it doesn't regress on this area, and even then you're threading on a path Google explicitly says is not recommended and not guaranteed to work for this application.

::: a/media/webrtc/trunk/src/modules/audio_device/main/source/android/audio_device_android_jni.cc
@@ +1,1 @@
> +/*

This entire file is in the diff, but I don't see any obvious differences in most lines. Did you accidentally mess up the line endings or something? Please fix this as it makes reviewing any changes in this file problematic.

::: a/media/webrtc/trunk/src/modules/audio_device/main/source/android/audio_device_android_jni.h
@@ +1,1 @@
> +/*

Same remark as previous file: the entire file is shown in the diff even though the changes appear much more limited.

::: a/media/webrtc/trunk/src/modules/audio_device/main/source/audio_device_impl.h
@@ +199,4 @@
>      virtual WebRtc_Word32 ResetAudioDevice();
>      virtual WebRtc_Word32 SetLoudspeakerStatus(bool enable);
>      virtual WebRtc_Word32 GetLoudspeakerStatus(bool* enabled) const;
> +    

nit: Spurious whitespace.

@@ +202,5 @@
> +    
> +	// Android specific function
> +    static WebRtc_Word32 SetAndroidObjects(
> +        void* javaVM, void* env, void* context);
> +		

nit: Spurious whitespace.

::: a/mobile/android/base/AudioDeviceAndroid.java
@@ +1,3 @@
> +/*
> + *  Copyright (c) 2011 The WebRTC project authors. All Rights Reserved.
> + *

What's the origin of this file? We seem to have an AudioDeviceAndroid.java already in the tree, but with bogus content. If it's from the webrtc.org upstream it should almost certainly be put in the upstream place in the media/webrtc/trunk subtree, and not below mobile/android (for one, the coding convention is completely different from ours).

::: a/mobile/android/base/GeckoAppShell.java
@@ +539,5 @@
>          sGeckoHandler = new Handler();
>  
>          // run gecko -- it will spawn its own thread
> +        GeckoAppShell.nativeInit(GeckoApp.mAppContext);
> +        AudioDeviceAndroid.mContext = GeckoApp.mAppContext;

The current WebRTC code passes the Context at the moment the WebRTC code is actually used. Would rather have that than to add startup overhead as this patch does.

I think this would also want a setter in any case.

::: a/mobile/android/base/Makefile.in
@@ +55,4 @@
>    awesomebar/HistoryTab.java \
>    BackButton.java \
>    BluetoothControlsReceiver.java \
> +  AudioDeviceAndroid.java \

Right now the files here are alphabetical, IIRC.

::: a/mobile/android/base/AndroidManifest.xml.in
@@ +34,1 @@
>  

Can you elaborate why these are needed exactly?

::: a/widget/android/AndroidJNI.cpp
@@ +74,2 @@
>  NS_EXPORT void JNICALL
> +Java_org_mozilla_gecko_GeckoAppShell_saveJavaVM(JavaVM* vm)

Is there really no way to avoid this contamination inside AndroidJNI? 

The video code avoids this entirely, see for example
media/webrtc/trunk/webrtc/modules/video_capture/android/video_capture_android.cc

Isn't this a matter of calling the VoiceEngine::SetAndroidObjects (which you just added) in the right place, exactly as done for video?

Comment 4

[Gian-Carlo Pascutto [:gcp]]

>Since current implementation of OpenSL ES library for Android doesn’t support the feature of selecting audio device it was disabled.

That's a problem, since as indicated above switching away from the OpenSL backend would really be an absolutely last resort with the understanding that his condemns us to having crappy delay forever.

What functionality do you need exactly that is not present?

Is there no way to coax Android to do what you want through an extension like "OpenSLES_AndroidConfiguration.h" set to SL_ANDROID_RECORDING_PRESET_VOICE_COMMUNICATION + SL_ANDROID_STREAM_VOICE?

>The playback frequencies of the record were changed from 44KHz to 16KHz

Why was this done? Why would we want this in the generic case?

Comment 5

[igarin]

Attachment: patch_4_18_2013

Comment 6

[igarin]

Please find new patch_4_18_2013 attached (attachment 738969 [details] [diff] [review]).

The differences are the following:
Comment regarding entire files (audio_device_android_jni.cc and audio_device_android_jni.h) are in the diff was fixed.
Spurious whitespaces were fixed.
The alphabetical order in AndroidManifest.xml.in file was restored.

Work on the following comments is in progress:
I am working on updating build procedures in order to move AudioDeviceAndroid.java file into webrtc subtree.
We are working on avoiding contamination inside AndroidJNI. I have made changes which are similar to way in which video_capture_android.cc works.
But this approach does not allow to get access to java classes of gecko application. Investigation of this issue is in progress.
Also, optimisation of passing of the Contex will be done.

Additionally, here is description of change regarding frequencies:
Limitations for Bluetooth output stream when it is initiated using startBluetoothSCO() are described here:
http://developer.android.com/reference/android/media/AudioManager.html#startBluetoothSco%28%29
Per specifications we need to use 16kHz or 8kHz sampling rate:
Even if a SCO connection is established, the following restrictions apply on audio output streams so that they can be routed to SCO headset:
	• the stream type must be STREAM_VOICE_CALL
	• the format must be mono
	• the sampling must be 16kHz or 8kHz
16kHz sampling rate was selected to provide better quality

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 738969 [details] [diff] [review]
patch_4_18_2013

Most of the fundamental remarks of the previous review still apply, as you mentioned. Thanks for fixing the whitespace issues, it's much clearer what the patch touches now.

Regarding sampling rate: just turning the sampling rate (and hence quality) down for *everything* is not acceptable just because the Bluetooth stuff doesn't support it. If there's no way to handle this on the Bluetooth side, you'll have to add logic to detect you're in the Bluetooth case and change the sampling rate then. But just switching it down for all users doesn't look acceptable to me.

For what it's worth, the current Firefox tree will try OpenSLES first, and fall back to the JNI layer if that fails (for example if it's not available on Android 2.2). If there is absolutely no way to coax Android to use the right mics/speakers for your application when using OpenSLES, you might be able to deal with that in a similar manner. See bug 815905 for that logic. But as mentioned before it's worth trying everything to avoid this, as your audio will always have latency issues with JNI.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Hmm, actually it looks like the OpenSLES backend defaults to 16kHz instead of 44.1kHz for the JNI one, so that issue would disappear.

Comment 9

[igarin]

Attachment: patch_4_26_2013

Comment 10

[igarin]

Please find new patch_4_18_2013 attached (attachment 742388 [details] [diff] [review]).

Remaining of the fundamental remarks of the previous review were fixed.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

1) First of all, please (re)read this:

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Notably, the preferred format of the patch, the need to mark the attachment as a patch, and the need to request review. That makes it way easier to review (and notice there's a need to review in the first place).

2) Your patch is based on a significantly outdated copy of the mozilla-central sources. As a result, it doesn't apply to the current source, and some of the code you patched has changed significantly in the mean time. You will need to rebase it to the current source tree (and you should do it regularly, as the WebRTC code is in heavy development).

3) It was pointed out before that we would not take a patch that bluntly switches out the OpenSLES backend and replaces it by the JNI backend, unless there were extremely good reasons to do so. I pointed out approaches to solve or attempt to solve your device selection problem with OpenSLES. Have you tried them? Can you please comment what you tried and what didn't work? 

I also pointed out that if it is *really not possible* to fix this or work around it, then the JNI path can be used as a fallback, but this means you actually need to code that up, i.e. detect when a Bluetooth device is present and switch backends if so. (Unfortunately I doubt that will be easy, all the more reasons to look for a solution with OpenSLES)

4) I would prefer to solve the problem of passing the JNIEnv/Context in the same way as the video code does it, i.e. by only doing it the moment the audio stack gets activated, and avoiding the ugly contamination in mozglue/JNIOnload. See the patch nr 2 here: https://bugzilla.mozilla.org/show_bug.cgi?id=866093 for how that probably should work. (It's possible there are still some bugs in the patch in that bug, but it should give you an idea for a more clean solution.) That patch also shows how to use FindClass without having to worry what thread you're on so you can do it after startup (again, the video code already uses that).

5) There are a number of questions regarding to the contents of your patch that went unanswered. For example: can you elaborate what the added Manifest permissions are required for exactly? We need to document this clearly as our users are not keen to grant an application permissions that aren't strictly required.

Comment 12

[igarin]

Here is a comment regarding switching out the OpenSLES backend and replacing it by the JNI backend.

We faced with serious issues trying to implement HFP for Fennec using standard OpenSLES library (part of Android NDK).
The current implementation of OpenSL ES library for Android (http://mobilepearls.com/labs/native-android-api/opensles/index.html) doesn’t support the feature of selecting audio device from the list of several ones. Please find the information found on the topic. 

•	OpenSL ES for Android( http://mobilepearls.com/labs/native-android-api/opensles/index.html ):
Supported features from OpenSL ES 1.0.1( I/O device data locator):
The only supported use of an I/O device data locator is when it is specified as the data source for Engine::CreateAudioRecorder. It should be initialized using these values, as shown in the example: 
SLDataLocator_IODevice loc_dev = {SL_DATALOCATOR_IODEVICE, SL_IODEVICE_AUDIOINPUT, SL_DEFAULTDEVICEID_AUDIOINPUT, NULL};

•	Android NDK Beginner's Guide(page 247):
Although OpenSL ES specification allows enumerating available output (and also input) devices, NDK implementation is not mature enough to obtain or select proper one (SLAudioIODeviceCapabilitiesItf, the official interface to obtain such an information). So when dealing with output and input device selection (only input device for recorders needs to be specified currently), prefer sticking to default values: SL_DEFAULTDEVICEID_AUDIOINPUT and SL_DEFAULTDEVICEID_AUDIOOUTPUT defined in OpenSLES.h.

Comment 13

[Jason Smith [:jsmith]]

FYI - This might be worthwhile to file this bug over in the core webrtc code shared with google and fix the issue over there first.

Comment 14

[Gian-Carlo Pascutto [:gcp]]

From the explanation given it's a bug in the Android OS (OpenSLES implementation) itself, not the WebRTC code. 

I suspect there may be a way to convince Android to do the right thing with a workaround, as explained in the comments above. Nevertheless a bug should be filed upstream indeed.

Comment 15

[igarin]

Attachment: patch_5_16_2013

Comment 16

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 750387 [details] [diff] [review]
patch_5_16_2013

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

1) Doesn't apply to current mozilla-central sources at all.
2) Still throws out the OpenSLES backend and replaces it by the JNI backend. I thought it was clear by now we can only accept this as a fallback solution.
3) Full of spurious whitespace changes.

Comment 17

[igarin]

Regarding workaround above:
To make application be able to setup SL_ANDROID_RECORDING_PRESET_VOICE_COMMUNICATION we should update ndk toolchain to api level 14 since this property was introduced in api level 14 in opensl.
As long as currently we are using min Android SDK version 8 we unable to use SL_ANDROID_RECORDING_PRESET_VOICE_COMMUNICATION property.
In spite of this limitation we had made build based on Android api level 14 with recorder configuration set to SL_ANDROID_RECORDING_PRESET_VOICE_COMMUNICATION. But this approach did not allow to get sound from Bluetooth headset.

Comment 18

[Gian-Carlo Pascutto [:gcp]]

We can probably import just the updated OpenSLES header and use that particular constant after a version check. So that wouldn't be an issue. Of course if it doesn't solve the problem it's a moot point.

Comment 19

[Maire Reavy [:mreavy]]

Still relevant?

Comment 20

[Gian-Carlo Pascutto [:gcp]]

Still relevant, this never got fixed.

Comment 21

[cary.bran]

Agree - still relevant.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.