Add recording capability to AudioManager, and implemented on windows using the WaveIn APIs.

Add recording capability to AudioManager, and implemented on windows using the WaveIn APIs.
Implementation for other platforms will follow in future patches.
Also includes a unit test.
BUG=none
TEST=no user visible change yet, just adding audio recording backend.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52292

[cpu_(ooo_6.6-7.5), 2010-07-13 01:04:03]

the code looks good, my comments are basically beefing up the testing.

http://codereview.chromium.org/2966005/diff/2001/3009
File media/audio/win/audio_input_win_unittest.cc (right):

http://codereview.chromium.org/2966005/diff/2001/3009#newcode6
media/audio/win/audio_input_win_unittest.cc:6: #include <mmsystem.h>
do we need mmsystem.h hee?

http://codereview.chromium.org/2966005/diff/2001/3009#newcode10
media/audio/win/audio_input_win_unittest.cc:10: #include
"testing/gmock/include/gmock/gmock.h"
do we need gmock here?

http://codereview.chromium.org/2966005/diff/2001/3009#newcode24
media/audio/win/audio_input_win_unittest.cc:24: virtual void
OnData(AudioInputStream* stream, const uint8* data, uint32 size) {
touch the first byte of the memory, check the size makes sense

http://codereview.chromium.org/2966005/diff/2001/3009#newcode38
media/audio/win/audio_input_win_unittest.cc:38: int had_error() const {
you should be calling this one on the tests

http://codereview.chromium.org/2966005/diff/2001/3009#newcode94
media/audio/win/audio_input_win_unittest.cc:94: 
other tests in the waveOut case seem relevant like PCMWaveStreamTripleBuffer,
but in this case is double buffer.

http://codereview.chromium.org/2966005/diff/2001/3009#newcode120
media/audio/win/audio_input_win_unittest.cc:120:
EXPECT_GE(test_callback.callback_count(), 9);
make sure the sleep is ample to allow the number of callbacks, at first sight
seems ok.

http://codereview.chromium.org/2966005/diff/2001/3009#newcode124
media/audio/win/audio_input_win_unittest.cc:124: }
also a test using the equivalent of TestSourceLaggy, with a sleep of 10ms or so
sounds interesting.

http://codereview.chromium.org/2966005/diff/2001/3010
File media/audio/win/audio_manager_win.cc (right):

http://codereview.chromium.org/2966005/diff/2001/3010#newcode1
media/audio/win/audio_manager_win.cc:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
update copyright year. Same for all files in the cl

[Satish, 2010-07-13 11:03:50]

http://codereview.chromium.org/2966005/diff/2001/3009
File media/audio/win/audio_input_win_unittest.cc (right):

http://codereview.chromium.org/2966005/diff/2001/3009#newcode6
media/audio/win/audio_input_win_unittest.cc:6: #include <mmsystem.h>
On 2010/07/13 01:04:04, cpu wrote:
> do we need mmsystem.h hee?

No, removed

http://codereview.chromium.org/2966005/diff/2001/3009#newcode10
media/audio/win/audio_input_win_unittest.cc:10: #include
"testing/gmock/include/gmock/gmock.h"
On 2010/07/13 01:04:04, cpu wrote:
> do we need gmock here?

No, removed

http://codereview.chromium.org/2966005/diff/2001/3009#newcode24
media/audio/win/audio_input_win_unittest.cc:24: virtual void
OnData(AudioInputStream* stream, const uint8* data, uint32 size) {
On 2010/07/13 01:04:04, cpu wrote:
> touch the first byte of the memory, check the size makes sense

Since this can be a read only buffer, i've added a check to read and check the
size

http://codereview.chromium.org/2966005/diff/2001/3009#newcode38
media/audio/win/audio_input_win_unittest.cc:38: int had_error() const {
On 2010/07/13 01:04:04, cpu wrote:
> you should be calling this one on the tests

Done.

http://codereview.chromium.org/2966005/diff/2001/3009#newcode94
media/audio/win/audio_input_win_unittest.cc:94: 
On 2010/07/13 01:04:04, cpu wrote:
> other tests in the waveOut case seem relevant like PCMWaveStreamTripleBuffer,
> but in this case is double buffer.

In playback we can expect the buffers to be played out in the same order that we
queue in, so the triple buffer test makes sense. However in recording, I don't
think there is a guarantee that the buffers are filled in the same order as they
were queued. Hence I did not add such a test here. Is that ok?

http://codereview.chromium.org/2966005/diff/2001/3009#newcode120
media/audio/win/audio_input_win_unittest.cc:120:
EXPECT_GE(test_callback.callback_count(), 9);
On 2010/07/13 01:04:04, cpu wrote:
> make sure the sleep is ample to allow the number of callbacks, at first sight
> seems ok.

I've increased it to 540ms so there would be at least 90ms extra time available
for all 9 callbacks to be issued

http://codereview.chromium.org/2966005/diff/2001/3009#newcode124
media/audio/win/audio_input_win_unittest.cc:124: }
On 2010/07/13 01:04:04, cpu wrote:
> also a test using the equivalent of TestSourceLaggy, with a sleep of 10ms or
so
> sounds interesting.

Done.

http://codereview.chromium.org/2966005/diff/2001/3010
File media/audio/win/audio_manager_win.cc (right):

http://codereview.chromium.org/2966005/diff/2001/3010#newcode1
media/audio/win/audio_manager_win.cc:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
On 2010/07/13 01:04:04, cpu wrote:
> update copyright year. Same for all files in the cl

Done.

[Paweł Hajdan Jr., 2010-07-13 14:52:23]

Drive-by with some test comments. Please let me take another look before
committing.

http://codereview.chromium.org/2966005/diff/8001/9009
File media/audio/win/audio_input_win_unittest.cc (right):

http://codereview.chromium.org/2966005/diff/8001/9009#newcode149
media/audio/win/audio_input_win_unittest.cc:149: Sleep(540);
Huh, why is this Sleep needed? Do you need to wait for something?

http://codereview.chromium.org/2966005/diff/8001/9009#newcode173
media/audio/win/audio_input_win_unittest.cc:173: Sleep(540);
Same here.

[Satish, 2010-07-13 15:01:20]

http://codereview.chromium.org/2966005/diff/8001/9009
File media/audio/win/audio_input_win_unittest.cc (right):

http://codereview.chromium.org/2966005/diff/8001/9009#newcode149
media/audio/win/audio_input_win_unittest.cc:149: Sleep(540);
On 2010/07/13 14:52:23, Paweł Hajdan Jr. wrote:
> Huh, why is this Sleep needed? Do you need to wait for something?

Yes, waiting for 500ms worth of audio to be recorded (recording starts with the
above AudioInputStream::Start call)

http://codereview.chromium.org/2966005/diff/8001/9009#newcode173
media/audio/win/audio_input_win_unittest.cc:173: Sleep(540);
On 2010/07/13 14:52:23, Paweł Hajdan Jr. wrote:
> Same here.

same as above :)

[Alpha Left Google, 2010-07-13 18:38:18]

in general looks good, I just have a few nits and some questions.

http://codereview.chromium.org/2966005/diff/8001/9003
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/2966005/diff/8001/9003#newcode1
media/audio/linux/audio_manager_linux.cc:1: // Copyright (c) 2006-2010 The
Chromium Authors. All rights reserved.
nit: Should be just 2010 instead of 2006-2010.

http://codereview.chromium.org/2966005/diff/8001/9009
File media/audio/win/audio_input_win_unittest.cc (right):

http://codereview.chromium.org/2966005/diff/8001/9009#newcode23
media/audio/win/audio_input_win_unittest.cc:23: virtual void
OnData(AudioInputStream* stream, const uint8* data, uint32 size) {
nit: 80 chars

http://codereview.chromium.org/2966005/diff/8001/9012
File media/audio/win/wavein_input_win.cc (right):

http://codereview.chromium.org/2966005/diff/8001/9012#newcode17
media/audio/win/wavein_input_win.cc:17: namespace {
static instead of anonymous namespace?

http://codereview.chromium.org/2966005/diff/8001/9012#newcode46
media/audio/win/wavein_input_win.cc:46: // If we don't have a packet size we use
100ms.
Should make sure 100ms is good enough. For waveout 100ms will certainly have
clicks / pops.

http://codereview.chromium.org/2966005/diff/8001/9012#newcode116
media/audio/win/wavein_input_win.cc:116: QueueNextPacket(buffer);
Just being cautious, for the waveout code we need to pause then queue then start
to avoid deadlocking, do we need to do similar things here?

http://codereview.chromium.org/2966005/diff/8001/9012#newcode142
media/audio/win/wavein_input_win.cc:142: MMRESULT res = ::waveInReset(wavein_);
I'm just curious, why are we not calling waevInStop before waveInReset? or
waveInStop is not necessary?

[Paweł Hajdan Jr., 2010-07-13 19:32:12]

I think it'd be a good idea to somehow document in comments the purpose of those
sleeps. Anyway, removing myself from reviewers.

[Satish, 2010-07-13 20:06:54]

http://codereview.chromium.org/2966005/diff/8001/9003
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/2966005/diff/8001/9003#newcode1
media/audio/linux/audio_manager_linux.cc:1: // Copyright (c) 2006-2010 The
Chromium Authors. All rights reserved.
On 2010/07/13 18:38:20, Alpha wrote:
> nit: Should be just 2010 instead of 2006-2010.

Done.

http://codereview.chromium.org/2966005/diff/8001/9009
File media/audio/win/audio_input_win_unittest.cc (right):

http://codereview.chromium.org/2966005/diff/8001/9009#newcode23
media/audio/win/audio_input_win_unittest.cc:23: virtual void
OnData(AudioInputStream* stream, const uint8* data, uint32 size) {
On 2010/07/13 18:38:20, Alpha wrote:
> nit: 80 chars

Done.

http://codereview.chromium.org/2966005/diff/8001/9012
File media/audio/win/wavein_input_win.cc (right):

http://codereview.chromium.org/2966005/diff/8001/9012#newcode17
media/audio/win/wavein_input_win.cc:17: namespace {
On 2010/07/13 18:38:20, Alpha wrote:
> static instead of anonymous namespace?

As discussed over chat, both seem to be allowed in the style guide and for
consistency with the waveout code i'll continue with namespace

http://codereview.chromium.org/2966005/diff/8001/9012#newcode46
media/audio/win/wavein_input_win.cc:46: // If we don't have a packet size we use
100ms.
On 2010/07/13 18:38:20, Alpha wrote:
> Should make sure 100ms is good enough. For waveout 100ms will certainly have
> clicks / pops.
> 

We have 3 buffers worth 100ms queued initially, so when one buffer is full and
given back to the app the second will be filling up and the third will be in
queue. Tested and works as expected.

http://codereview.chromium.org/2966005/diff/8001/9012#newcode116
media/audio/win/wavein_input_win.cc:116: QueueNextPacket(buffer);
On 2010/07/13 18:38:20, Alpha wrote:
> Just being cautious, for the waveout code we need to pause then queue then
start
> to avoid deadlocking, do we need to do similar things here?

Yes that is not required here, since recording starts only after waveInStart
which is called below.

http://codereview.chromium.org/2966005/diff/8001/9012#newcode142
media/audio/win/wavein_input_win.cc:142: MMRESULT res = ::waveInReset(wavein_);
On 2010/07/13 18:38:20, Alpha wrote:
> I'm just curious, why are we not calling waevInStop before waveInReset? or
> waveInStop is not necessary?

waveInStop marks the current buffer as done and returns to the app, while
keeping the other two in the queue. But waveInReset marks all of them as done
and returns to the app, which would be a real reset so we can destroy the
buffers and move on.

[cpu_(ooo_6.6-7.5), 2010-07-13 20:44:47]

http://codereview.chromium.org/2966005/diff/2001/3006
File media/audio/mac/audio_manager_mac.h (right):

http://codereview.chromium.org/2966005/diff/2001/3006#newcode1
media/audio/mac/audio_manager_mac.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
copyright year

http://codereview.chromium.org/2966005/diff/2001/3009
File media/audio/win/audio_input_win_unittest.cc (right):

http://codereview.chromium.org/2966005/diff/2001/3009#newcode94
media/audio/win/audio_input_win_unittest.cc:94: 
Maybe I misunderstood your code. It seems you have n buffers in a circular list,
given to ::waveInPrepareHeader
why won't they be used in order?

On 2010/07/13 11:03:51, Satish wrote:
> On 2010/07/13 01:04:04, cpu wrote:
> > other tests in the waveOut case seem relevant like
PCMWaveStreamTripleBuffer,
> > but in this case is double buffer.
> 
> In playback we can expect the buffers to be played out in the same order that
we
> queue in, so the triple buffer test makes sense. However in recording, I don't
> think there is a guarantee that the buffers are filled in the same order as
they
> were queued. Hence I did not add such a test here. Is that ok?

[cpu_(ooo_6.6-7.5), 2010-07-13 20:48:39]

http://codereview.chromium.org/2966005/diff/8001/9012
File media/audio/win/wavein_input_win.cc (right):

http://codereview.chromium.org/2966005/diff/8001/9012#newcode17
media/audio/win/wavein_input_win.cc:17: namespace {
anonymous namespace is preferred and more in tune with modern c++

On 2010/07/13 18:38:20, Alpha wrote:
> static instead of anonymous namespace?

[Satish, 2010-07-13 20:55:26]

http://codereview.chromium.org/2966005/diff/2001/3009
File media/audio/win/audio_input_win_unittest.cc (right):

http://codereview.chromium.org/2966005/diff/2001/3009#newcode94
media/audio/win/audio_input_win_unittest.cc:94: 
On 2010/07/13 20:44:48, cpu wrote:
> Maybe I misunderstood your code. It seems you have n buffers in a circular
list,
> given to ::waveInPrepareHeader
> why won't they be used in order?

When starting to record, we queue 3 buffers one after the other. But there is no
guarantee that the audio driver will use them in the same order (at least not
mentioned in the docs). It is quite possible that after using the first buffer,
the driver may take the third buffer (depending on how it maintains the array of
buffers in it's own internal data structure). When the recording callback is
received with a full buffer we copy the data and give it back to the audio
driver, but again no guarantee if the driver is placing this in the beginning of
it's own list or at the end. Hence I did not want to test that all buffers are
used.. And even if they aren't used in the same order it doesn't cause any issue
as long as the buffers come back in regular frequency with the recorded data.

[cpu_(ooo_6.6-7.5), 2010-07-13 23:29:02]

On 2010/07/13 20:55:26, Satish wrote:
> http://codereview.chromium.org/2966005/diff/2001/3009
> File media/audio/win/audio_input_win_unittest.cc (right):
> 
> http://codereview.chromium.org/2966005/diff/2001/3009#newcode94
> media/audio/win/audio_input_win_unittest.cc:94: 
> On 2010/07/13 20:44:48, cpu wrote:
> > Maybe I misunderstood your code. It seems you have n buffers in a circular
> list,
> > given to ::waveInPrepareHeader
> > why won't they be used in order?
> 
> When starting to record, we queue 3 buffers one after the other. But there is
no
> guarantee that the audio driver will use them in the same order (at least not
> mentioned in the docs). It is quite possible that after using the first
buffer,
> the driver may take the third buffer (depending on how it maintains the array
of
> buffers in it's own internal data structure). When the recording callback is
> received with a full buffer we copy the data and give it back to the audio
> driver, but again no guarantee if the driver is placing this in the beginning
of
> it's own list or at the end. Hence I did not want to test that all buffers are
> used.. And even if they aren't used in the same order it doesn't cause any
issue
> as long as the buffers come back in regular frequency with the recorded data.

ok, lgtm on my side. See what else Alpha finds and you're good to go.

[cpu_(ooo_6.6-7.5), 2010-07-13 23:30:18]

One more thing, please make a bug (and assign to you) about implementing the
mock stream.

On 2010/07/13 23:29:02, cpu wrote:
> On 2010/07/13 20:55:26, Satish wrote:
> > http://codereview.chromium.org/2966005/diff/2001/3009
> > File media/audio/win/audio_input_win_unittest.cc (right):
> > 
> > http://codereview.chromium.org/2966005/diff/2001/3009#newcode94
> > media/audio/win/audio_input_win_unittest.cc:94: 
> > On 2010/07/13 20:44:48, cpu wrote:
> > > Maybe I misunderstood your code. It seems you have n buffers in a circular
> > list,
> > > given to ::waveInPrepareHeader
> > > why won't they be used in order?
> > 
> > When starting to record, we queue 3 buffers one after the other. But there
is
> no
> > guarantee that the audio driver will use them in the same order (at least
not
> > mentioned in the docs). It is quite possible that after using the first
> buffer,
> > the driver may take the third buffer (depending on how it maintains the
array
> of
> > buffers in it's own internal data structure). When the recording callback is
> > received with a full buffer we copy the data and give it back to the audio
> > driver, but again no guarantee if the driver is placing this in the
beginning
> of
> > it's own list or at the end. Hence I did not want to test that all buffers
are
> > used.. And even if they aren't used in the same order it doesn't cause any
> issue
> > as long as the buffers come back in regular frequency with the recorded
data.
> 
> ok, lgtm on my side. See what else Alpha finds and you're good to go.

[Alpha Left Google, 2010-07-13 23:31:05]

LGTM from me too.