Use audio thread for the fake input audio stream.

Use audio thread for the fake input audio stream.

Previously the fake input stream is creating a new thread to pump the callbacks, this is unnecessary and FakeAudioInputStream::Stop() might take time to stop the thread. I suspect that this is one of the potential causes for the timeout in WebRtc content browser tests on Android bots.

This patch changes the code to use AudioManager audio thread instead, hopefully this can reduce the flakiness on Android tests.
Also it changes |beep_lock| from a global lock to a lock used only by BeepContext privately.

BUG=387895
TEST=bots

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

[no longer working on chromium, 2014-07-17 11:17:31]

Henrik, please review.

Thanks,
SX

[henrika (OOO until Aug 14), 2014-07-17 11:21:21]

https://codereview.chromium.org/399623008/diff/1/media/audio/fake_audio_input...
File media/audio/fake_audio_input_stream.cc (right):

https://codereview.chromium.org/399623008/diff/1/media/audio/fake_audio_input...
media/audio/fake_audio_input_stream.cc:107:
base::Bind(&FakeAudioInputStream::DoCallback, weak_factory_.GetWeakPtr()),
Why do you need a weak pointer here?

[henrika (OOO until Aug 14), 2014-07-17 11:21:59]

https://codereview.chromium.org/399623008/diff/1/media/audio/fake_audio_input...
File media/audio/fake_audio_input_stream.cc (right):

https://codereview.chromium.org/399623008/diff/1/media/audio/fake_audio_input...
media/audio/fake_audio_input_stream.cc:29: class BeepContext {
How are the changes related to this CL?

[no longer working on chromium, 2014-07-17 11:22:48]

https://codereview.chromium.org/399623008/diff/1/media/audio/fake_audio_input...
File media/audio/fake_audio_input_stream.cc (right):

https://codereview.chromium.org/399623008/diff/1/media/audio/fake_audio_input...
media/audio/fake_audio_input_stream.cc:107:
base::Bind(&FakeAudioInputStream::DoCallback, weak_factory_.GetWeakPtr()),
On 2014/07/17 11:21:21, henrika wrote:
> Why do you need a weak pointer here?

If Stop() is called before the task is executed, then the task will never be
executed, otherwise it will crash.

[no longer working on chromium, 2014-07-17 11:31:15]

https://codereview.chromium.org/399623008/diff/1/media/audio/fake_audio_input...
File media/audio/fake_audio_input_stream.cc (right):

https://codereview.chromium.org/399623008/diff/1/media/audio/fake_audio_input...
media/audio/fake_audio_input_stream.cc:29: class BeepContext {
On 2014/07/17 11:21:59, henrika wrote:
> How are the changes related to this CL?

Comment was addressed offline, this is a better way than using a global lock.

[no longer working on chromium, 2014-07-17 11:31:43]

Henrik, FYI, description has been updated.

[henrika (OOO until Aug 14), 2014-07-17 11:35:20]

LGTM. Did you also verify the performance locally to ensure that the Beep signal
works?

[no longer working on chromium, 2014-07-17 11:39:22]

On 2014/07/17 11:35:20, henrika wrote:
> LGTM. Did you also verify the performance locally to ensure that the Beep
signal
> works?

Yes, I have already run the content browser tests locally.

[no longer working on chromium, 2014-07-17 11:39:26]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-07-17 11:40:08]

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

[commit-bot: I haz the power, 2014-07-17 15:48:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[no longer working on chromium, 2014-07-17 16:15:39]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-07-17 16:16:28]

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

[commit-bot: I haz the power, 2014-07-17 18:30:30]

Change committed as 283822

[DaleCurtis, 2014-07-17 18:32:04]

This is okay for now since this isn't actually used in production (AFAIK), but
it should be using AudioManager::GetWorkerTaskRunner(), so it doesn't overload
the UI thread on OSX.  However I recommend just using the FakeAudioConsumer
interface and extending it for input.

[no longer working on chromium, 2014-07-17 18:46:35]

On 2014/07/17 18:32:04, DaleCurtis wrote:
> This is okay for now since this isn't actually used in production (AFAIK), but
> it should be using AudioManager::GetWorkerTaskRunner(), so it doesn't overload
> the UI thread on OSX.  However I recommend just using the FakeAudioConsumer
> interface and extending it for input.

Thanks Dale for the input.

The first upload of the CL was using GetWorkerTaskRunner(), but it failed on Mac
bot with the weak pointer, error code:
64:FATAL:weak_ptr.cc(20)] Check failed:
sequence_checker_.CalledOnValidSequencedThread() || HasOneRef(). WeakPtrs must
be invalidated on the same sequenced thread.
So I simply switched over to GetTaskRunner() to workaround the problem (I really
want to use weak pointer to make FakeAudioInputStream::Stop() quick and less
flaky). I was hoping this would be fine since GetWorkerTaskRunner() is the same
as GetTaskRunner() on all platforms except for Mac. And using GetTaskRunner()
makes it consistent on all platforms.

This fake stream is only used by tests, extending FakeAudioConsumer for input
seems a bit overkill for the use cases.

[DaleCurtis, 2014-07-17 18:50:05]

My worry is that we create these fake streams in the production AudioManager, so
it's not inconceivable that one day someone enables this in production without
realizing the consequences.  Is there a CHECK() you can put anywhere to ensure
this doesn't happen?

Your weakptr violation is probably because you're crossing threads, the
fakeaudioconsumer goes out of its way to be thread safe across multiple threads.
 I think it'd be pretty easy to swap it out and cleaner to boot.