Low level windows audio support (part 1 of 2)...

Low level windows audio support (part 1 of 2)
- Windows specific changes to AudioManager Allows testing for mock streams

This this the first part of the code that adds support for audio using waveXXX API.
Since is relatively lot of code I will do two CLs, unfortunately the complexity
is not evenly split.



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

[scherkus (not reviewing), 2009-01-08 02:35:41]

Tiny nits, but I've got questions about the mock stream.  It seems to me like it
could be cross-platform, unless you have bigger plans for it!

http://codereview.chromium.org/17401/diff/1/2
File media/audio/audio_output.h (right):

http://codereview.chromium.org/17401/diff/1/2#newcode130
Line 130: virtual const void* GetLastMockBuffer() = 0;
I'm wondering if there's a better way to do this.  All I can come up with is
maybe moving the code to the mock stream class itself.

http://codereview.chromium.org/17401/diff/1/3
File media/audio/win/audio_output_win.cc (right):

http://codereview.chromium.org/17401/diff/1/3#newcode6
Line 6: #include <mmsystem.h>
Includes should be in alphabetical order, but I think <windows.h> is an
exception :)

http://codereview.chromium.org/17401/diff/1/3#newcode17
Line 17: class AudioOutputStreamMockWin : public AudioOutputStream {
Is there anything that makes the mock stream windows specific?  Otherwise it
could be a separate file compiled by all platforms.

http://codereview.chromium.org/17401/diff/1/3#newcode25
Line 25: right_volume_(0.0) {
Is there a contract for the initial volume level, or should callers check?  I'm
just thinking if most platforms create streams with volume at 1.0 and then we
have a mock which is using 0.0.

http://codereview.chromium.org/17401/diff/1/3#newcode63
Line 63: char* get_buffer() {
I think getters are just buffer() and not get_buffer(), however I've always been
really confused about this part of the style guide.

http://codereview.chromium.org/17401/diff/1/3#newcode92
Line 92: // - PCMDXSoundAudioOutputStream: Based on DirectSond or XAudio (future
work).
DirectSond typo

[cpu_(ooo_6.6-7.5), 2009-01-08 20:35:19]

CL updated. Please take another look.

http://codereview.chromium.org/17401/diff/1/3
File media/audio/win/audio_output_win.cc (right):

http://codereview.chromium.org/17401/diff/1/3#newcode6
Line 6: #include <mmsystem.h>
On 2009/01/08 02:35:42, scherkus wrote:
> Includes should be in alphabetical order, but I think <windows.h> is an
> exception :)

I think is ok because it belongs to the 'C' headers group. It could be
alphabetic within the group but I don't think is correct to include windows
after.

http://codereview.chromium.org/17401/diff/1/3#newcode17
Line 17: class AudioOutputStreamMockWin : public AudioOutputStream {
On 2009/01/08 02:35:42, scherkus wrote:
> Is there anything that makes the mock stream windows specific?  Otherwise it
> could be a separate file compiled by all platforms.

The thread part. The Start() and Stop() methods should spawn a thread that does
periodic callbacks. It could even be with a realtime priority.

http://codereview.chromium.org/17401/diff/1/3#newcode25
Line 25: right_volume_(0.0) {
On 2009/01/08 02:35:42, scherkus wrote:
> Is there a contract for the initial volume level, or should callers check? 
I'm
> just thinking if most platforms create streams with volume at 1.0 and then we

Yeah. my bad. This should be 1.0. Volume is something that is not implemented
even in the non-mock parts.

> have a mock which is using 0.0.

http://codereview.chromium.org/17401/diff/1/3#newcode63
Line 63: char* get_buffer() {
On 2009/01/08 02:35:42, scherkus wrote:
> I think getters are just buffer() and not get_buffer(), however I've always
been
> really confused about this part of the style guide.

True

[brettw, 2009-01-08 21:07:31]

http://codereview.chromium.org/17401/diff/7/207
File media/audio/audio_output.h (right):

http://codereview.chromium.org/17401/diff/7/207#newcode140
Line 140: // Some useful sampling rates.
I think all of these should be inside the AudioManager class. Actually, so
should GetAudioManager() (just make it a static in the class). We generally
avoid these globals.

http://codereview.chromium.org/17401/diff/7/209
File media/audio/win/audio_manager_win.h (right):

http://codereview.chromium.org/17401/diff/7/209#newcode34
Line 34: protected:
Is there any reason this needs to be protected instead of private? Google style
is that it should default to private.

You probably also want DISALLOW_COPY_AND_ASSIGN at the bottom.

I think the same applies to AudioManager.

http://codereview.chromium.org/17401/diff/7/208
File media/audio/win/audio_output_win.cc (right):

http://codereview.chromium.org/17401/diff/7/208#newcode17
Line 17: class AudioOutputStreamMockWin : public AudioOutputStream {
Can you put this class and the local function definitions in this file in an
anonymous namespace?

http://codereview.chromium.org/17401/diff/7/208#newcode67
Line 67: virtual ~AudioOutputStreamMockWin() {
Put this below the constructor.

http://codereview.chromium.org/17401/diff/7/208#newcode100
Line 100: return new PCMWaveOutAudioOutputStream(this, channels, sample_rate,
Chrome style is to always use {} for multi-line statements (even if it's only
one statement. And Google-style is to use {} for all arms of an if statement if
any of them have it, so you should also add it to the if.

[scherkus (not reviewing), 2009-01-08 21:34:16]

http://codereview.chromium.org/17401/diff/1/3
File media/audio/win/audio_output_win.cc (right):

http://codereview.chromium.org/17401/diff/1/3#newcode17
Line 17: class AudioOutputStreamMockWin : public AudioOutputStream {
On 2009/01/08 20:35:19, cpu wrote:
> On 2009/01/08 02:35:42, scherkus wrote:
> > Is there anything that makes the mock stream windows specific?  Otherwise it
> > could be a separate file compiled by all platforms.
> 
> The thread part. The Start() and Stop() methods should spawn a thread that
does
> periodic callbacks. It could even be with a realtime priority.
> 

Assuming you use base/thread.h and base/timer.h the cross-platform bits should
be taken care of, no?

[cpu_(ooo_6.6-7.5), 2009-01-08 23:44:37]

code updated, please take another look.

http://codereview.chromium.org/17401/diff/7/207
File media/audio/audio_output.h (right):

http://codereview.chromium.org/17401/diff/7/207#newcode140
Line 140: // Some useful sampling rates.
On 2009/01/08 21:07:32, brettw wrote:
> I think all of these should be inside the AudioManager class. Actually, so
> should GetAudioManager() (just make it a static in the class). We generally
> avoid these globals.

Done.

http://codereview.chromium.org/17401/diff/7/209
File media/audio/win/audio_manager_win.h (right):

http://codereview.chromium.org/17401/diff/7/209#newcode34
Line 34: protected:
On 2009/01/08 21:07:32, brettw wrote:
> Is there any reason this needs to be protected instead of private? Google
style
> is that it should default to private.
> 
> You probably also want DISALLOW_COPY_AND_ASSIGN at the bottom.
> 
> I think the same applies to AudioManager.

Done.

http://codereview.chromium.org/17401/diff/7/208
File media/audio/win/audio_output_win.cc (right):

http://codereview.chromium.org/17401/diff/7/208#newcode17
Line 17: class AudioOutputStreamMockWin : public AudioOutputStream {
On 2009/01/08 21:07:32, brettw wrote:
> Can you put this class and the local function definitions in this file in an
> anonymous namespace?

I cant move the class itself because of ReleaseStream(AudioOutputStreamMockWin
*stream)

but I've put the standalone functions.

http://codereview.chromium.org/17401/diff/7/208#newcode67
Line 67: virtual ~AudioOutputStreamMockWin() {
On 2009/01/08 21:07:32, brettw wrote:
> Put this below the constructor.

Done.

http://codereview.chromium.org/17401/diff/7/208#newcode100
Line 100: return new PCMWaveOutAudioOutputStream(this, channels, sample_rate,
On 2009/01/08 21:07:32, brettw wrote:
> Chrome style is to always use {} for multi-line statements (even if it's only
> one statement. And Google-style is to use {} for all arms of an if statement
if
> any of them have it, so you should also add it to the if.

Done.

[cpu_(ooo_6.6-7.5), 2009-01-08 23:45:18]

On 2009/01/08 21:34:16, scherkus wrote:
> http://codereview.chromium.org/17401/diff/1/3
> File media/audio/win/audio_output_win.cc (right):
> 
> http://codereview.chromium.org/17401/diff/1/3#newcode17
> Line 17: class AudioOutputStreamMockWin : public AudioOutputStream {
> On 2009/01/08 20:35:19, cpu wrote:
> > On 2009/01/08 02:35:42, scherkus wrote:
> > > Is there anything that makes the mock stream windows specific?  Otherwise
it
> > > could be a separate file compiled by all platforms.
> > 
> > The thread part. The Start() and Stop() methods should spawn a thread that
> does
> > periodic callbacks. It could even be with a realtime priority.
> > 
> 
> Assuming you use base/thread.h and base/timer.h the cross-platform bits should
> be taken care of, no?

On 2009/01/08 21:34:16, scherkus wrote:
> http://codereview.chromium.org/17401/diff/1/3
> File media/audio/win/audio_output_win.cc (right):
> 
> http://codereview.chromium.org/17401/diff/1/3#newcode17
> Line 17: class AudioOutputStreamMockWin : public AudioOutputStream {
> On 2009/01/08 20:35:19, cpu wrote:
> > On 2009/01/08 02:35:42, scherkus wrote:
> > > Is there anything that makes the mock stream windows specific?  Otherwise
it
> > > could be a separate file compiled by all platforms.
> > 
> > The thread part. The Start() and Stop() methods should spawn a thread that
> does
> > periodic callbacks. It could even be with a realtime priority.
> > 
> 
> Assuming you use base/thread.h and base/timer.h the cross-platform bits should
> be taken care of, no?

Yeah, sort of. Our Thread class is meant to be doing messageloop work, now that
I look at this I think the worker_pool.h might be usable if I forgo the realtime
issue. Well, I really need one thread. I really need a 'lite' version of our
thread class.

All things considered, I don't see much gain. Each platform audio stream might
have different quirks that I would like to try to mimic in the mock sans the
fact that it does not talk to hardware.

[brettw, 2009-01-08 23:53:37]

L
 G
  T
   M

[scherkus (not reviewing), 2009-01-08 23:57:50]

LGTM

Thanks for satisfying my curiosity about the mock cross platform stuff :)