Moved the fake input stream's processing onto the audio worker thread.

Moved the fake input stream's processing onto the audio worker thread.

The reason for this is that the fake input stream would not work on Mac
where the audio non-worker code runs on the UI thread. Therefore, the
fake input stream would get starved for instance in the WebRTC audio
quality tests and not play any audio (those tests block the UI thread
while recording).

This patch introduces a worker precisely like the fake audio consumer
used by the fake output stream.

BUG=453907, 446859
Committed: https://crrev.com/9e0a7c10163c9c22adad3135a1d9fc5c3682a592
Cr-Commit-Position: refs/heads/master@{#317557}

[phoglund_chromium, 2015-02-12 13:51:05]

phoglund@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2015-02-12 19:28:23]

Please add a unittest similar to FakeAudioConsumerUnittest, this is tricky code
to get right, so we should have a unit test for it.

[phoglund_chromium, 2015-02-13 15:41:56]

On 2015/02/12 19:28:23, DaleCurtis wrote:
> Please add a unittest similar to FakeAudioConsumerUnittest, this is tricky
code
> to get right, so we should have a unit test for it.

Yeah, I reckoned I might as well refactor out a fake audio input provider
similar to the consumer. Then I won't have to figure out how to get my task
runners in a faked audio manager. I need to make some small adjustments to the
browser test itself and write the unit test; I'll do that on Monday.

[phoglund_chromium, 2015-02-17 09:50:06]

All right, PTAL

https://codereview.chromium.org/922663002/diff/80001/media/audio/fake_audio_p...
File media/audio/fake_audio_provider_unittest.cc (right):

https://codereview.chromium.org/922663002/diff/80001/media/audio/fake_audio_p...
media/audio/fake_audio_provider_unittest.cc:20: class FakeAudioProviderTest :
public ::testing::TestWithParam<bool> {
As you can see, the unit test is very similar to that of the consumer. The code
itself is very similar though so that isn't unexpected. One difference is that
the provider can run in both file and beep mode, which is why I've parameterized
it.

I considered adding a test where I verify the data coming in (for instance by
computing the audio energy for the bus in the callback), but it seemed complex
to write (and interpret the results if the test failed). Also the test would
have to run at least a couple seconds since the beep generator beeps once or
twice per second.

[DaleCurtis, 2015-02-17 23:27:50]

Thinking more generically, I wonder if we should rename FakeAudioConsumer to
FakeAudioWorker or something similar and remove the provided AudioBus. It could
then be used by both input and output streams to provided a repeating callback
managed across the worker and audio threads.

FakeAudioInputStream can then continue to inject whatever data it needs, without
needing such vast modification.  It only needs a fake audio engine which drives
the OnData() callback.

https://codereview.chromium.org/922663002/diff/100001/media/audio/fake_audio_...
File media/audio/fake_audio_provider.cc (right):

https://codereview.chromium.org/922663002/diff/100001/media/audio/fake_audio_...
media/audio/fake_audio_provider.cc:130: void LoadWavFileIntoWorker(const
base::FilePath& wav_filename);
The worker is already a pretty complicated beast. Is there a reason you want to
stuff all of this code into the worker instead of leaving it in the
FakeAudioInputStream?

https://codereview.chromium.org/922663002/diff/100001/media/audio/fake_audio_...
File media/audio/fake_audio_provider.h (right):

https://codereview.chromium.org/922663002/diff/100001/media/audio/fake_audio_...
media/audio/fake_audio_provider.h:49: static void BeepOnce();
Why static?

[phoglund_chromium, 2015-02-18 10:21:58]

On 2015/02/17 23:27:50, DaleCurtis wrote:
> Thinking more generically, I wonder if we should rename FakeAudioConsumer to
> FakeAudioWorker or something similar and remove the provided AudioBus. It
could
> then be used by both input and output streams to provided a repeating callback
> managed across the worker and audio threads.
> 
> FakeAudioInputStream can then continue to inject whatever data it needs,
without
> needing such vast modification.  It only needs a fake audio engine which
drives
> the OnData() callback.

Interesting idea, so you mean that the streams will essentially be called back
by
the fake audio worker and be responsible themselves for actually processing
audio?
One problem here is that the file input stream needs to load the file on the
worker
thread, so we need to introduce some initialization-on-worker-thread hook
besides the
timed callback. Otherwise the file loading will happen on the UI thread on Mac,
which
I wanted to avoid. Should be doable, I'll give it a shot.

> 
>
https://codereview.chromium.org/922663002/diff/100001/media/audio/fake_audio_...
> File media/audio/fake_audio_provider.cc (right):
> 
>
https://codereview.chromium.org/922663002/diff/100001/media/audio/fake_audio_...
> media/audio/fake_audio_provider.cc:130: void LoadWavFileIntoWorker(const
> base::FilePath& wav_filename);
> The worker is already a pretty complicated beast. Is there a reason you want
to
> stuff all of this code into the worker instead of leaving it in the
> FakeAudioInputStream?
> 
>
https://codereview.chromium.org/922663002/diff/100001/media/audio/fake_audio_...
> File media/audio/fake_audio_provider.h (right):
> 
>
https://codereview.chromium.org/922663002/diff/100001/media/audio/fake_audio_...
> media/audio/fake_audio_provider.h:49: static void BeepOnce();
> Why static?

[phoglund_chromium, 2015-02-18 10:22:06]

https://codereview.chromium.org/922663002/diff/100001/media/audio/fake_audio_...
File media/audio/fake_audio_provider.cc (right):

https://codereview.chromium.org/922663002/diff/100001/media/audio/fake_audio_...
media/audio/fake_audio_provider.cc:130: void LoadWavFileIntoWorker(const
base::FilePath& wav_filename);
On 2015/02/17 23:27:49, DaleCurtis wrote:
> The worker is already a pretty complicated beast. Is there a reason you want
to
> stuff all of this code into the worker instead of leaving it in the
> FakeAudioInputStream?

All the data needs to live on the worker's thread, so having the worker own all
the data and code seemed natural to me. Also the file loading needs to happen on
the Worker's thread, otherwise we will be loading the file on the UI thread on
Mac.

https://codereview.chromium.org/922663002/diff/100001/media/audio/fake_audio_...
File media/audio/fake_audio_provider.h (right):

https://codereview.chromium.org/922663002/diff/100001/media/audio/fake_audio_...
media/audio/fake_audio_provider.h:49: static void BeepOnce();
On 2015/02/17 23:27:49, DaleCurtis wrote:
> Why static?

It's static because BeepOnce is called from the fake video input stream. The
beeps are synchronized to the spinning green ball generated by that device.
Since the video stream doesn't have a ref to the audio stream it cheats by
making a static call. Somebody should perhaps redesign that at some point :)

[phoglund_chromium, 2015-02-18 16:38:15]

PTAL

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

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:109: class
FakeAudioInputStream::FileSource : public AudioConverter::InputCallback,
It felt wrong to toss back all the state into the input stream - most of this
state should be accessed only on the worker thread, so I took the opportunity to
abstract out the two kinds of sources. That way it's easier to maintain the
input stream in the future and not mix up the general state, like the audio bus,
and the worker-thread-only state like the wav file handler.

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:265: *buffer_size = buffer_size_;
Not sure about this: I do feel the sources know better than the input stream
what the buffer size is, but maybe the stream should compute the buffer size and
hand to the sources instead.

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:305:
audio_manager_->GetWorkerTaskRunner()->PostTask(
This is the file loading I talked about earlier. An alternative could be to hand
the worker a callback and make it schedule the run (like
worker->Open(callback_that_does_the_actual_opening) or something) but I didn't
see the benefit. Another alternative would be to make the file source load the
file on its first callback, but that seemed uglier. 

The above assumes that the Open is guaranteed to execute before the first
callback.

[DaleCurtis, 2015-02-18 19:04:50]

Looking good, don't forget to update the BUILD.gn files too.

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

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:109: class
FakeAudioInputStream::FileSource : public AudioConverter::InputCallback,
On 2015/02/18 16:38:15, phoglund wrote:
> It felt wrong to toss back all the state into the input stream - most of this
> state should be accessed only on the worker thread, so I took the opportunity
to
> abstract out the two kinds of sources. That way it's easier to maintain the
> input stream in the future and not mix up the general state, like the audio
bus,
> and the worker-thread-only state like the wav file handler.

Good idea. Take a look at media/audio/simple_sources.h I think maybe this code
(and BeepSource) should be added there and just use the
AudioOutputStream::AudioSourceCallback() interface for simplicity. This code
could then be used to drive an output stream too.

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:265: *buffer_size = buffer_size_;
On 2015/02/18 16:38:15, phoglund wrote:
> Not sure about this: I do feel the sources know better than the input stream
> what the buffer size is, but maybe the stream should compute the buffer size
and
> hand to the sources instead.

I don't think you need this, it seems like an incorrect remnant from when we
used a uint8[] instead of an AudioBus to transfer the data. I think it should be
removed.

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:305:
audio_manager_->GetWorkerTaskRunner()->PostTask(
On 2015/02/18 16:38:15, phoglund wrote:
> This is the file loading I talked about earlier. An alternative could be to
hand
> the worker a callback and make it schedule the run (like
> worker->Open(callback_that_does_the_actual_opening) or something) but I didn't
> see the benefit. Another alternative would be to make the file source load the
> file on its first callback, but that seemed uglier. 
> 
> The above assumes that the Open is guaranteed to execute before the first
> callback.

I think loading the file source on the first callback is fine and no worse than
the code here. I'd simply choose the source on first callback via something like
if (!audio_source_) audio_source_ = ChooseSource() or similar.

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:367: callback_->OnData(this,
audio_bus_.get(), buffer_size, 1.0);
Why are you returning the buffer_size like this? I don't see why you'd want to
use it as the hardware_delay. I think you can just use 0 here and drop the
buffer_size parameter on the source interface.

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
File media/audio/fake_audio_input_stream.h (right):

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.h:74: virtual void PlayInto(AudioBus*
audio_bus, int* buffer_size) = 0;
How about WriteInto? What is the int* buffer_size for?

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
File media/audio/fake_audio_worker.cc (right):

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_worker.cc:48: const scoped_ptr<AudioBus> audio_bus_;
Delete?

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
File media/audio/fake_audio_worker.h (right):

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_worker.h:35: typedef base::Callback<void()> WorkerCB;
Delete typedef in favor of base::Closure.

[phoglund_chromium, 2015-02-19 15:44:11]

Turns out the fake audio consumer was used in some other places I missed so I
had to rewrite them too to use the worker too. I think I addressed all the
comments.

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

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:109: class
FakeAudioInputStream::FileSource : public AudioConverter::InputCallback,
On 2015/02/18 19:04:49, DaleCurtis wrote:
> On 2015/02/18 16:38:15, phoglund wrote:
> > It felt wrong to toss back all the state into the input stream - most of
this
> > state should be accessed only on the worker thread, so I took the
opportunity
> to
> > abstract out the two kinds of sources. That way it's easier to maintain the
> > input stream in the future and not mix up the general state, like the audio
> bus,
> > and the worker-thread-only state like the wav file handler.
> 
> Good idea. Take a look at media/audio/simple_sources.h I think maybe this code
> (and BeepSource) should be added there and just use the
> AudioOutputStream::AudioSourceCallback() interface for simplicity. This code
> could then be used to drive an output stream too.

Sure, done.

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:265: *buffer_size = buffer_size_;
On 2015/02/18 19:04:49, DaleCurtis wrote:
> On 2015/02/18 16:38:15, phoglund wrote:
> > Not sure about this: I do feel the sources know better than the input stream
> > what the buffer size is, but maybe the stream should compute the buffer size
> and
> > hand to the sources instead.
> 
> I don't think you need this, it seems like an incorrect remnant from when we
> used a uint8[] instead of an AudioBus to transfer the data. I think it should
be
> removed.

Just passing 0 seems to work so I'll go with that.

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:305:
audio_manager_->GetWorkerTaskRunner()->PostTask(
On 2015/02/18 19:04:49, DaleCurtis wrote:
> On 2015/02/18 16:38:15, phoglund wrote:
> > This is the file loading I talked about earlier. An alternative could be to
> hand
> > the worker a callback and make it schedule the run (like
> > worker->Open(callback_that_does_the_actual_opening) or something) but I
didn't
> > see the benefit. Another alternative would be to make the file source load
the
> > file on its first callback, but that seemed uglier. 
> > 
> > The above assumes that the Open is guaranteed to execute before the first
> > callback.
> 
> I think loading the file source on the first callback is fine and no worse
than
> the code here. I'd simply choose the source on first callback via something
like
> if (!audio_source_) audio_source_ = ChooseSource() or similar.

Done.

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:367: callback_->OnData(this,
audio_bus_.get(), buffer_size, 1.0);
On 2015/02/18 19:04:49, DaleCurtis wrote:
> Why are you returning the buffer_size like this? I don't see why you'd want to
> use it as the hardware_delay. I think you can just use 0 here and drop the
> buffer_size parameter on the source interface. 

Done.

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
File media/audio/fake_audio_worker.cc (right):

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_worker.cc:48: const scoped_ptr<AudioBus> audio_bus_;
On 2015/02/18 19:04:49, DaleCurtis wrote:
> Delete?

Done.

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
File media/audio/fake_audio_worker.h (right):

https://codereview.chromium.org/922663002/diff/120001/media/audio/fake_audio_...
media/audio/fake_audio_worker.h:35: typedef base::Callback<void()> WorkerCB;
On 2015/02/18 19:04:49, DaleCurtis wrote:
> Delete typedef in favor of base::Closure.

Done.

[DaleCurtis, 2015-02-19 19:54:54]

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

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:97: if (audio_source_.get() == nullptr) {
just use if (!audio_source_)

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:120: } else {
No need for else {} since it includes a return.

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:121: return scoped_ptr<SimpleSource>(new
BeepingSource(params_));
You can just use make_scoped_ptr() if you want instead.

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
File media/audio/fake_audio_input_stream.h (right):

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.h:23: class SimpleSource;
No need for this now, just use AudioOutputStream::AudioSourceCallback

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
File media/audio/fake_audio_worker.h (right):

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
media/audio/fake_audio_worker.h:24: // |worker_task_runner| is the task runner
on which the WorkerCB provided to
s/WorkerCB/closure/ here and below.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
File media/audio/simple_sources.cc (right):

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:20: namespace {
media/ code uses static functions instead of anonymous namespaces.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:44: CHECK(false) << "Failed to read all bytes of "
<< wav_filename.value();
No need to return() after check; just CHECK_EQ() here.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:85: bool beep_once() const {
Add 1 line between each method.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:101: LAZY_INSTANCE_INITIALIZER;
Change this to a leaky lazy instance.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:160: buffer_size_(params.channels() *
params.bits_per_sample() *
params.GetBytesPerBuffer() ?

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:190: DCHECK(wav_audio_handler_.get() != nullptr);
Should be able to just DCHECK(wav_audio_handler_)

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:212: : buffer_size_(params.channels() *
params.bits_per_sample() *
Ditto.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:221: buffer_.reset(new uint8[buffer_size_]);
Could just be a constructor param if you want.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:222: memset(buffer_.get(), 0, buffer_size_);
Unnecessary since you clear it at the start of OnmoreData().

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:235: {
Extra {} seem unnecessary, no scoped lock.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:278: 
Extra space.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:281: BeepContext* beep_context =
g_beep_context.Pointer();
Combine these two lines?

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
File media/audio/simple_sources.h (right):

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.h:18: class SimpleSource : public
AudioOutputStream::AudioSourceCallback {
Why is this necessary? Open() is only used in the FileSource() and it's
definitely something you could just call on the first OnMoreData().

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.h:58: public AudioConverter::InputCallback{
Space after InputCallback

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.h:74: scoped_ptr<media::WavAudioHandler>
wav_audio_handler_;
No need for media:: prefix on anything in this .h or .cc

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.h:91: void OnError(AudioOutputStream* stream)
override {}
Inline methods aren't allowed for virtual functions, so you'll need to define
these in the .cc . Ditto for the rest in this class.

[phoglund_chromium, 2015-02-20 14:22:01]

PTAL

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

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:97: if (audio_source_.get() == nullptr) {
On 2015/02/19 19:54:52, DaleCurtis wrote:
> just use if (!audio_source_)

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:120: } else {
On 2015/02/19 19:54:52, DaleCurtis wrote:
> No need for else {} since it includes a return.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:121: return scoped_ptr<SimpleSource>(new
BeepingSource(params_));
On 2015/02/19 19:54:52, DaleCurtis wrote:
> You can just use make_scoped_ptr() if you want instead.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
File media/audio/fake_audio_input_stream.h (right):

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.h:23: class SimpleSource;
On 2015/02/19 19:54:52, DaleCurtis wrote:
> No need for this now, just use AudioOutputStream::AudioSourceCallback

I think I do need it; the problem is that the callback's destructor is
protected, so I can't keep a scoped_ptr of it. I assumed the practice of making
those destructors protected implied that one should not keep callbacks as
scoped_ptrs.

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
File media/audio/fake_audio_worker.h (right):

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
media/audio/fake_audio_worker.h:24: // |worker_task_runner| is the task runner
on which the WorkerCB provided to
On 2015/02/19 19:54:52, DaleCurtis wrote:
> s/WorkerCB/closure/ here and below.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
File media/audio/simple_sources.cc (right):

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:20: namespace {
On 2015/02/19 19:54:52, DaleCurtis wrote:
> media/ code uses static functions instead of anonymous namespaces.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:44: CHECK(false) << "Failed to read all bytes of "
<< wav_filename.value();
On 2015/02/19 19:54:53, DaleCurtis wrote:
> No need to return() after check; just CHECK_EQ() here.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:85: bool beep_once() const {
On 2015/02/19 19:54:53, DaleCurtis wrote:
> Add 1 line between each method.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:101: LAZY_INSTANCE_INITIALIZER;
On 2015/02/19 19:54:53, DaleCurtis wrote:
> Change this to a leaky lazy instance.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:160: buffer_size_(params.channels() *
params.bits_per_sample() *
On 2015/02/19 19:54:53, DaleCurtis wrote:
> params.GetBytesPerBuffer() ?

Yeah, I noticed I don't even need that anymore so I'll just remove it.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:190: DCHECK(wav_audio_handler_.get() != nullptr);
On 2015/02/19 19:54:53, DaleCurtis wrote:
> Should be able to just DCHECK(wav_audio_handler_)

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:212: : buffer_size_(params.channels() *
params.bits_per_sample() *
On 2015/02/19 19:54:53, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:212: : buffer_size_(params.channels() *
params.bits_per_sample() *
On 2015/02/19 19:54:53, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:221: buffer_.reset(new uint8[buffer_size_]);
On 2015/02/19 19:54:53, DaleCurtis wrote:
> Could just be a constructor param if you want.

What are you referring to, the buffer size?...

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:222: memset(buffer_.get(), 0, buffer_size_);
On 2015/02/19 19:54:53, DaleCurtis wrote:
> Unnecessary since you clear it at the start of OnmoreData().

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:235: {
On 2015/02/19 19:54:54, DaleCurtis wrote:
> Extra {} seem unnecessary, no scoped lock.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:235: {
On 2015/02/19 19:54:54, DaleCurtis wrote:
> Extra {} seem unnecessary, no scoped lock.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:278: 
On 2015/02/19 19:54:53, DaleCurtis wrote:
> Extra space.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:281: BeepContext* beep_context =
g_beep_context.Pointer();
On 2015/02/19 19:54:53, DaleCurtis wrote:
> Combine these two lines?

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
File media/audio/simple_sources.h (right):

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.h:18: class SimpleSource : public
AudioOutputStream::AudioSourceCallback {
On 2015/02/19 19:54:54, DaleCurtis wrote:
> Why is this necessary? Open() is only used in the FileSource() and it's
> definitely something you could just call on the first OnMoreData().

All right.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.h:58: public AudioConverter::InputCallback{
On 2015/02/19 19:54:54, DaleCurtis wrote:
> Space after InputCallback

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.h:74: scoped_ptr<media::WavAudioHandler>
wav_audio_handler_;
On 2015/02/19 19:54:54, DaleCurtis wrote:
> No need for media:: prefix on anything in this .h or .cc

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.h:91: void OnError(AudioOutputStream* stream)
override {}
On 2015/02/19 19:54:54, DaleCurtis wrote:
> Inline methods aren't allowed for virtual functions, so you'll need to define
> these in the .cc . Ditto for the rest in this class.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.h:91: void OnError(AudioOutputStream* stream)
override {}
On 2015/02/19 19:54:54, DaleCurtis wrote:
> Inline methods aren't allowed for virtual functions, so you'll need to define
> these in the .cc . Ditto for the rest in this class.

Done.

[DaleCurtis, 2015-02-20 18:45:06]

lgtm % removing SimpleSource and making the AudioSourceCallback destructor
public.

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
File media/audio/fake_audio_input_stream.h (right):

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.h:23: class SimpleSource;
On 2015/02/20 14:21:59, phoglund wrote:
> On 2015/02/19 19:54:52, DaleCurtis wrote:
> > No need for this now, just use AudioOutputStream::AudioSourceCallback
> 
> I think I do need it; the problem is that the callback's destructor is
> protected, so I can't keep a scoped_ptr of it. I assumed the practice of
making
> those destructors protected implied that one should not keep callbacks as
> scoped_ptrs.

Ah, it's fine to change the destructor to public for that case.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
File media/audio/simple_sources.cc (right):

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:221: buffer_.reset(new uint8[buffer_size_]);
On 2015/02/20 14:21:59, phoglund wrote:
> On 2015/02/19 19:54:53, DaleCurtis wrote:
> > Could just be a constructor param if you want.
> 
> What are you referring to, the buffer size?...

Yeah, just saying you can move buffer_() into constructor parameter list instead
of inside the {}.

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

https://codereview.chromium.org/922663002/diff/230001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:97: if (!audio_source_.get())
.get() shouldn't be necessary and is discouraged since it avoids the bool
expression evaluation template.

[phoglund_chromium, 2015-02-23 09:16:33]

New patchsets have been uploaded after l-g-t-m from dalecurtis@chromium.org

[phoglund_chromium, 2015-02-23 09:16:41]

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
File media/audio/fake_audio_input_stream.h (right):

https://codereview.chromium.org/922663002/diff/200001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.h:23: class SimpleSource;
On 2015/02/20 18:45:05, DaleCurtis wrote:
> On 2015/02/20 14:21:59, phoglund wrote:
> > On 2015/02/19 19:54:52, DaleCurtis wrote:
> > > No need for this now, just use AudioOutputStream::AudioSourceCallback
> > 
> > I think I do need it; the problem is that the callback's destructor is
> > protected, so I can't keep a scoped_ptr of it. I assumed the practice of
> making
> > those destructors protected implied that one should not keep callbacks as
> > scoped_ptrs.
> 
> Ah, it's fine to change the destructor to public for that case.

Done.

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
File media/audio/simple_sources.cc (right):

https://codereview.chromium.org/922663002/diff/200001/media/audio/simple_sour...
media/audio/simple_sources.cc:221: buffer_.reset(new uint8[buffer_size_]);
On 2015/02/20 18:45:05, DaleCurtis wrote:
> On 2015/02/20 14:21:59, phoglund wrote:
> > On 2015/02/19 19:54:53, DaleCurtis wrote:
> > > Could just be a constructor param if you want.
> > 
> > What are you referring to, the buffer size?...
> 
> Yeah, just saying you can move buffer_() into constructor parameter list
instead
> of inside the {}.

Done.

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

https://codereview.chromium.org/922663002/diff/230001/media/audio/fake_audio_...
media/audio/fake_audio_input_stream.cc:97: if (!audio_source_.get())
On 2015/02/20 18:45:06, DaleCurtis wrote:
> .get() shouldn't be necessary and is discouraged since it avoids the bool
> expression evaluation template.

Done.

[phoglund_chromium, 2015-02-23 09:26:36]

The CQ bit was checked by phoglund@chromium.org

[commit-bot: I haz the power, 2015-02-23 09:26:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/922663002/270001

[commit-bot: I haz the power, 2015-02-23 10:20:16]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2015-02-23 10:20:41]

Patchset 15 (id:??) landed as
https://crrev.com/9e0a7c10163c9c22adad3135a1d9fc5c3682a592
Cr-Commit-Position: refs/heads/master@{#317557}