Added automatic mode to FakeInputAudioStream to generate automatic beeps

Added automatic mode to FakeInputAudioStream to generate automatic beeps.

This patch is to allow writing audio only tests using the fake input stream, also it changes the name of the fake device from "Default" to "Fake Audio, which I hope it will make things less confusing.

NOTRY=true

BUG=358541
TEST=bots existing webrtc tests.

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

[no longer working on chromium, 2014-04-29 14:06:26]

Dale, could you please review the CL?

Patrik, FYI and feel free to take a look.

Thanks,
SX

[DaleCurtis, 2014-04-29 17:30:56]

lgtm % Q + nit.

https://codereview.chromium.org/260073013/diff/20001/media/audio/fake_audio_i...
File media/audio/fake_audio_input_stream.cc (right):

https://codereview.chromium.org/260073013/diff/20001/media/audio/fake_audio_i...
media/audio/fake_audio_input_stream.cc:29: BeepContext() : beep_once(false),
automatic(true) {}
Shouldn't this default to false? Isn't this used in production for cases w/o a
mic?

https://codereview.chromium.org/260073013/diff/20001/media/audio/fake_audio_i...
media/audio/fake_audio_input_stream.cc:77: thread_.Start();
Just an FYI, but this whole thing could be switched to use FakeAudioConsumer
which could be renamed FakeAudioScheduler or something.

https://codereview.chromium.org/260073013/diff/20001/media/audio/fake_audio_i...
media/audio/fake_audio_input_stream.cc:110: if (delta_ms > 0) {
just use > base::TimeDelta to avoid converting back and forth.

[DaleCurtis, 2014-04-29 17:33:31]

Make that a couple Qs.

https://codereview.chromium.org/260073013/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/260073013/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:39:
kFakeDeviceName,
FakeDeviceId?

https://codereview.chromium.org/260073013/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:147: if
(use_fake_device_ || devices->empty()) {
Won't devices() always be empty()? You're constructing it right here.

https://codereview.chromium.org/260073013/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:156: }
You're no longer injecting a fake device if no device is present.  Is this
intended?

[phoglund_chromium, 2014-04-30 08:03:49]

https://codereview.chromium.org/260073013/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/260073013/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:147: if
(use_fake_device_ || devices->empty()) {
On 2014/04/29 17:33:31, DaleCurtis wrote:
> Won't devices() always be empty()? You're constructing it right here.

Yeah, looks like the else branch will never be taken. You probably want to check
if device_names is empty.

[phoglund_chromium, 2014-05-02 11:27:47]

You get anywhere with this Shijing?

[no longer working on chromium, 2014-05-02 14:30:46]

Hi Dale and Patrik, sorry for the delay, PTAL.

Alpha, I added a automatic beep in fake_audio_input_stream.cc, which will be
disabled if the users explicitly call BeepOnce(), could you please take a look
to see if it is fine?

Thanks,
SX

https://codereview.chromium.org/260073013/diff/20001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/260073013/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:39:
kFakeDeviceName,
On 2014/04/29 17:33:31, DaleCurtis wrote:
> FakeDeviceId?

Done.

https://codereview.chromium.org/260073013/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:147: if
(use_fake_device_ || devices->empty()) {
On 2014/04/29 17:33:31, DaleCurtis wrote:
> Won't devices() always be empty()? You're constructing it right here.

Ah, right, I just missed that devices were different from device_names. That
might be why lots of unittests failed, I looked deeper at those tests, it seems
that they mocked out the AudioManager and injected fake devices in the
enumeration APIs.
So I just fall back to the previous behavior and only inject the fake device if
the use_fake_device is true and there is no enumerated device.

https://codereview.chromium.org/260073013/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:147: if
(use_fake_device_ || devices->empty()) {
On 2014/04/30 08:03:49, phoglund wrote:
> On 2014/04/29 17:33:31, DaleCurtis wrote:
> > Won't devices() always be empty()? You're constructing it right here.
> 
> Yeah, looks like the else branch will never be taken. You probably want to
check
> if device_names is empty.

That was what I was thinking but realized that it would inject a fake device in
production when the list is empty, which is un-wanted change. Also some
unittests mocked out the AudioManager and injected fake devices in the
enumeration APIs. There is no easy way to change them to use
AudioInputDeviceManager().
So I just fall back to the previous behavior and only inject the fake device if
the use_fake_device is true and there is no enumerated device.

https://codereview.chromium.org/260073013/diff/20001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:156: }
On 2014/04/29 17:33:31, DaleCurtis wrote:
> You're no longer injecting a fake device if no device is present.  Is this
> intended?

We don't inject the fake device in production. The previous code only inject the
fake device when use_fake_device_ is true (which is always false in production)
and devices->empty().

https://codereview.chromium.org/260073013/diff/20001/media/audio/fake_audio_i...
File media/audio/fake_audio_input_stream.cc (right):

https://codereview.chromium.org/260073013/diff/20001/media/audio/fake_audio_i...
media/audio/fake_audio_input_stream.cc:29: BeepContext() : beep_once(false),
automatic(true) {}
On 2014/04/29 17:30:56, DaleCurtis wrote:
> Shouldn't this default to false? Isn't this used in production for cases w/o a
> mic?

That is good point, I honestly have no idea how the fake input stream is used
for production.

My understanding is that, In production, when there is no mic, all the clients
won't pass the UI if they are requesting mic.

But I searched the code base and found out that in AudioManagerLinux, if no
cras, no pulse and no alsa, it will use FakeAudioManager:
#if defined(USE_ALSA)
  UMA_HISTOGRAM_ENUMERATION("Media.LinuxAudioIO", kAlsa, kAudioIOMax + 1);
  return new AudioManagerAlsa(audio_log_factory);
#else
  return new FakeAudioManager(audio_log_factory);
#endif
But I believe there is no way to make FakeAudioManager working in such case,
since it does not have any device:
bool FakeAudioManager::HasAudioOutputDevices() { return false; }
bool FakeAudioManager::HasAudioInputDevices() { return false; }

It is used in order to get things compiled.

I don't really want to add one more static method to enable the automatic beep,
so I added Alpha to double check if it OK to have the automatic as true by
default.

https://codereview.chromium.org/260073013/diff/20001/media/audio/fake_audio_i...
media/audio/fake_audio_input_stream.cc:77: thread_.Start();
On 2014/04/29 17:30:56, DaleCurtis wrote:
> Just an FYI, but this whole thing could be switched to use FakeAudioConsumer
> which could be renamed FakeAudioScheduler or something.

Sounds great.

https://codereview.chromium.org/260073013/diff/20001/media/audio/fake_audio_i...
media/audio/fake_audio_input_stream.cc:110: if (delta_ms > 0) {
On 2014/04/29 17:30:56, DaleCurtis wrote:
> just use > base::TimeDelta to avoid converting back and forth.

Done.

https://codereview.chromium.org/260073013/diff/80001/media/audio/fake_audio_i...
File media/audio/fake_audio_input_stream.cc (right):

https://codereview.chromium.org/260073013/diff/80001/media/audio/fake_audio_i...
media/audio/fake_audio_input_stream.cc:29: BeepContext() : beep_once(false),
automatic(true) {}
Alpha, could you please take a look at this code to see if it is fine to have
automatic beeping to be on by default?

[DaleCurtis, 2014-05-02 20:20:08]

https://codereview.chromium.org/260073013/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/260073013/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:149:
devices->push_back(StreamDeviceInfo(stream_type, kFakeDeviceName,
Won't this now put the fake device at the back of the devices list, so tests
would use a real device instead of the fake in most cases?

[no longer working on chromium, 2014-05-06 14:10:55]

https://codereview.chromium.org/260073013/diff/80001/content/browser/renderer...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/260073013/diff/80001/content/browser/renderer...
content/browser/renderer_host/media/audio_input_device_manager.cc:149:
devices->push_back(StreamDeviceInfo(stream_type, kFakeDeviceName,
On 2014/05/02 20:20:09, DaleCurtis wrote:
> Won't this now put the fake device at the back of the devices list, so tests
> would use a real device instead of the fake in most cases?

That has been the change before this CL, and the whole fake devices/streams code
is a bit messy.

I did some more clean-up and hope it is improved a bit now, ptal.

[no longer working on chromium, 2014-05-06 14:11:57]

On 2014/05/06 14:10:55, xians1 wrote:
>
https://codereview.chromium.org/260073013/diff/80001/content/browser/renderer...
> File content/browser/renderer_host/media/audio_input_device_manager.cc
(right):
> 
>
https://codereview.chromium.org/260073013/diff/80001/content/browser/renderer...
> content/browser/renderer_host/media/audio_input_device_manager.cc:149:
> devices->push_back(StreamDeviceInfo(stream_type, kFakeDeviceName,
> On 2014/05/02 20:20:09, DaleCurtis wrote:
> > Won't this now put the fake device at the back of the devices list, so tests
> > would use a real device instead of the fake in most cases?
> 
> That has been the change before this CL

Sorry for the typo, I meant, that had been the case before this CL.

[DaleCurtis, 2014-05-06 17:36:52]

lgtm

https://codereview.chromium.org/260073013/diff/140001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc
(right):

https://codereview.chromium.org/260073013/diff/140001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:71:
manager_->UseFakeDevice();
Do you want to set this only if no real devices exist?

https://codereview.chromium.org/260073013/diff/140001/media/audio/fake_audio_...
File media/audio/fake_audio_input_stream.cc (right):

https://codereview.chromium.org/260073013/diff/140001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:89: last_callback_time_ +
callback_interval_ * 2 - now;
Where does this *2 come from?

[no longer working on chromium, 2014-05-07 08:55:41]

Thanks Dale, I have double checked the code and fake input stream should be only
used for testing, so I am landing it now. I am expecting that the bots will
catch the problem if the automatic mode breaks anything.

https://codereview.chromium.org/260073013/diff/140001/content/browser/rendere...
File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc
(right):

https://codereview.chromium.org/260073013/diff/140001/content/browser/rendere...
content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:71:
manager_->UseFakeDevice();
On 2014/05/06 17:36:52, DaleCurtis wrote:
> Do you want to set this only if no real devices exist?

Not really, the tests are designed for testing the behavior of
AudioInputDeviceManager, using fake devices or real devices are the same.

https://codereview.chromium.org/260073013/diff/140001/media/audio/fake_audio_...
File media/audio/fake_audio_input_stream.cc (right):

https://codereview.chromium.org/260073013/diff/140001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:89: last_callback_time_ +
callback_interval_ * 2 - now;
On 2014/05/06 17:36:52, DaleCurtis wrote:
> Where does this *2 come from?

This is old code moved from line 121.

I guess the formula is:
now - last_callback_time_ - callback_interval_ = delta_now
next_callback_time - callback_interval_ + delta_now = 0
That becomes:
next_callback_time - 2 * callback_interval_ +now - last_callback_time_ = 0

[no longer working on chromium, 2014-05-07 08:55:46]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-05-07 08:58:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xians@chromium.org/260073013/140001

[phoglund_chromium, 2014-05-07 11:46:57]

On 2014/05/07 08:58:30, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/xians@chromium.org/260073013/140001

By the way, I tried out the patch locally and as far I can tell it works great.
Nice work Shijing!

[commit-bot: I haz the power, 2014-05-07 15:03:17]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
  win_chromium_rel on tryserver.chromium

[no longer working on chromium, 2014-05-07 15:09:36]

The CQ bit was unchecked by xians@chromium.org

[no longer working on chromium, 2014-05-07 15:09:55]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-05-07 15:13:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xians@chromium.org/260073013/140001

[commit-bot: I haz the power, 2014-05-07 17:49:46]

Change committed as 268848

[DaleCurtis, 2014-05-07 19:39:53]

Hey xians, this patch had a failing try: "MediaAccessAPIAllow_TestAllow" on OSX
and then you set NOTRY=true which == tree sadness :(

[no longer working on chromium, 2014-05-08 15:07:59]

On 2014/05/07 19:39:53, DaleCurtis wrote:
> Hey xians, this patch had a failing try: "MediaAccessAPIAllow_TestAllow" on
OSX
> and then you set NOTRY=true which == tree sadness :(

Hi Dale, it is a bit mysterious there. This CL is platform generic and should
not trigger failure on particular bot.
I tried running the browser_test in my local machine, and it failed both with
and without my patch, it was told that there was some problem with AVFoundation
that was un-related to my patch.
And I tested the patch by running the browser tests with --force-qtkit fixed all
the problem and it passed all the tests locally.

But I looked at the Mac bot this morning, it seems the bots became flaky with my
patch and got back to normal after the revert, so I am going to take a closer
look to double check if it somehow introduce the flakiness.

Thanks for the revert.

[phoglund_chromium, 2014-05-26 13:04:54]

> Thanks for the revert.

Ping - did Miguel land that patch that was required on mac so you can reland
this?