Enable audio capture on Android

Enable audio capture on Android

This CL JUST enables audio capture on Android, further tunning and
making audio work better might take extra time, I think it's better to
first enable it. A few TODOs are added and minor changes are added into
CL too.

BUG=161417
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181948

[wjia(left Chromium), 2013-02-06 18:17:05]

Please fix unittests.

https://codereview.chromium.org/12218036/diff/1/media/audio/android/audio_man...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/12218036/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:17: static const char
kAndroidInputDeviceId[] = "mic";
move these 2 into anonymous namespace and remove static.

https://codereview.chromium.org/12218036/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:46: void
AudioManagerAndroid::ShowAudioInputSettings() {
Do you need a no-op override here? It's same def as in AudioManagerBase.

[leozwang1, 2013-02-11 08:05:00]

https://codereview.chromium.org/12218036/diff/1/media/audio/android/audio_man...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/12218036/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:17: static const char
kAndroidInputDeviceId[] = "mic";
I removed these defines because unit test assume there must be a "default"
device.

https://codereview.chromium.org/12218036/diff/1/media/audio/android/audio_man...
media/audio/android/audio_manager_android.cc:46: void
AudioManagerAndroid::ShowAudioInputSettings() {
On 2013/02/06 18:17:05, wjia wrote:
> Do you need a no-op override here? It's same def as in AudioManagerBase.

Done.

[leozwang1, 2013-02-11 08:08:56]

Hi, Wei and Shijing, this cl is going to enable audio capture on Android, I will
address audio quality later. please review.

[leozwang1, 2013-02-11 08:14:19]

Add Tommi as the reviewer.

[tommi (sloooow) - chröme, 2013-02-11 09:52:58]

https://codereview.chromium.org/12218036/diff/2006/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://codereview.chromium.org/12218036/diff/2006/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:328: #if
!defined(OS_ANDROID)
Instead of this, can you use the MAYBE_ approach as we do elsewhere?  This means
that MAYBE_StartRecording will be defined as StartRecording on all platforms
except Android.  On Android it will be defined as DISABLED_StartRecording.

The test will continue to be compiled for all platforms and you can still run it
manually if you want by overriding the disabled check.

https://codereview.chromium.org/12218036/diff/2006/media/audio/android/audio_...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/12218036/diff/2006/media/audio/android/audio_...
media/audio/android/audio_manager_android.cc:48: }
This implementation is making a few assumptions that I'm not sure are safe. 
Would it make sense to add something like this?

} else {
  DCHECK_EQ(device_names->size(), 1u);
  DCHECK_EQ(device_names->at(0).unique_id,
            kDefaultDeviceId);
}

Alternatively, the implementation could be changed to either:

DCHECK(device_names->empty());
device_names->push_front(
    media::AudioDeviceName(kDefaultDeviceName, kDefaultDeviceId));

or

device_names->clear();
device_names->push_front(
    media::AudioDeviceName(kDefaultDeviceName, kDefaultDeviceId));


Personally, I'd prefer the one with DCHECK(device_names->empty());

https://codereview.chromium.org/12218036/diff/2006/media/audio/android/audio_...
media/audio/android/audio_manager_android.cc:64: input_params.sample_rate(), 16,
buffer_size);
Is audio always 16 bit?

[leozwang1, 2013-02-11 18:03:43]

Hi Tommi, thanks for reviewing it, please take a look at patch 5.

https://codereview.chromium.org/12218036/diff/2006/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://codereview.chromium.org/12218036/diff/2006/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:328: #if
!defined(OS_ANDROID)
On 2013/02/11 09:52:58, tommi wrote:
> Instead of this, can you use the MAYBE_ approach as we do elsewhere?  This
means
> that MAYBE_StartRecording will be defined as StartRecording on all platforms
> except Android.  On Android it will be defined as DISABLED_StartRecording.
> 
> The test will continue to be compiled for all platforms and you can still run
it
> manually if you want by overriding the disabled check.

Done.

https://codereview.chromium.org/12218036/diff/2006/media/audio/android/audio_...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/12218036/diff/2006/media/audio/android/audio_...
media/audio/android/audio_manager_android.cc:48: }
On 2013/02/11 09:52:58, tommi wrote:
> This implementation is making a few assumptions that I'm not sure are safe. 
> Would it make sense to add something like this?
> 
> } else {
>   DCHECK_EQ(device_names->size(), 1u);
>   DCHECK_EQ(device_names->at(0).unique_id,
>             kDefaultDeviceId);
> }
> 
> Alternatively, the implementation could be changed to either:
> 
> DCHECK(device_names->empty());
> device_names->push_front(
>     media::AudioDeviceName(kDefaultDeviceName, kDefaultDeviceId));
> 
> or
> 
> device_names->clear();
> device_names->push_front(
>     media::AudioDeviceName(kDefaultDeviceName, kDefaultDeviceId));
> 
> 
> Personally, I'd prefer the one with DCHECK(device_names->empty());

Done.

https://codereview.chromium.org/12218036/diff/2006/media/audio/android/audio_...
media/audio/android/audio_manager_android.cc:64: input_params.sample_rate(), 16,
buffer_size);
opensl only supports 8bit or 16bit, webrtc uses 16bit as the default. Maybe fix
it to 16bit in this patch? (Linux uses 16bit too.)

[no longer working on chromium, 2013-02-11 18:20:36]

One question, and I am going to defer my part to Tommi, please don't wait for my
lgt*.

https://codereview.chromium.org/12218036/diff/11008/content/renderer/media/we...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/12218036/diff/11008/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:57: buffer_size = 2 *
sample_rate / 100;
Have you tested that 20ms buffer size is a good start for Android?

[leozwang1, 2013-02-11 21:00:48]

Something was wrong when I switched client, please take a look at patch 7.
Seems the trybot is flanky, I will do more tests.

https://codereview.chromium.org/12218036/diff/11008/content/renderer/media/we...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/12218036/diff/11008/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:57: buffer_size = 2 *
sample_rate / 100;
In my local loopback test with video, I can still hear some "clipping" noise.

[tommi (sloooow) - chröme, 2013-02-11 22:59:11]

lgtm

https://codereview.chromium.org/12218036/diff/6013/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://codereview.chromium.org/12218036/diff/6013/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:327: // disable this unit
test on Android for now.
thanks

[leozwang1, 2013-02-11 23:11:32]

Thanks Tommi, will commit it for M26.
Seems tests are still flanky, will address these problem in late CLs.

[wjia(left Chromium), 2013-02-12 02:34:24]

lgtm with nit.

https://codereview.chromium.org/12218036/diff/3023/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://codereview.chromium.org/12218036/diff/3023/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:477: //
FullDuplexAudioWithAGC is flanky on Android, disable it for now.
nit: flaky

[commit-bot: I haz the power, 2013-02-12 03:56:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leozwang@chromium.org/12218036/2023

[commit-bot: I haz the power, 2013-02-12 04:48:25]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 05:43:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leozwang@chromium.org/12218036/2023

[commit-bot: I haz the power, 2013-02-12 06:41:16]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 12:04:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leozwang@chromium.org/12218036/2023

[commit-bot: I haz the power, 2013-02-12 12:48:15]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 15:03:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leozwang@chromium.org/12218036/2023

[commit-bot: I haz the power, 2013-02-12 17:00:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/leozwang@chromium.org/12218036/10013

[no longer working on chromium, 2013-02-12 20:04:45]

https://codereview.chromium.org/12218036/diff/11008/content/renderer/media/we...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/12218036/diff/11008/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:57: buffer_size = 2 *
sample_rate / 100;
On 2013/02/11 21:00:48, leozwang1 wrote:
> In my local loopback test with video, I can still hear some "clipping" noise.
> 

Then why don't we increase it to a reasonable value? like 50ms, for example.

[leozwang1, 2013-02-12 20:14:17]

https://codereview.chromium.org/12218036/diff/11008/content/renderer/media/we...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/12218036/diff/11008/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.cc:57: buffer_size = 2 *
sample_rate / 100;
Most of tests I have done are video+audio. The next step is to try audio only to
see where the baseline is.