Move some generic functions to AudioManagerBase to be inherited by platform-specific AudioManager***

Moved the implementations of ReleaseOutputStream() and ReleaseInputStream() to AudioManagerBase and let all the AudioManagerPlatforms inherit the same implementations.

Also moved the MakeAudioOutputStream() and MakeAudioInputStream() to AudioManagerBase, separate the AUDIO_PCM_LINEAR mode and AUDIO_PCM_LOW_LATENCY mode into two different functions inside the AudioManagerPlatforms. So that the structure is clearer and also easier to deprecate the AUDIO_PCM_LINEAR for the future.

Made the destructor of the AudioManagerPlatforms protected so it can be called by only the AudioManagerBase.

BUG=116064
TEST=media_unittests

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

[no longer working on chromium, 2012-03-01 16:02:19]

This is a refactoring CL to improve the inheritance of AudioManager family. 

Some breakdown in terms of "owner approval".
Tommi: It will be great if you can be the main reviewer to this CL.
Henrik: Windows part.
Robert: I hope I won't break the BSD build, but please take a look at it.

Thanks,

/SX

http://codereview.chromium.org/9570014/diff/1/media/audio/linux/alsa_output_u...
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/9570014/diff/1/media/audio/linux/alsa_output_u...
media/audio/linux/alsa_output_unittest.cc:78: MOCK_METHOD0(Init, void());
Question to Tommi: We don't use these mock functions in this test, do we need
them?

[henrika (OOO until Aug 14), 2012-03-02 08:31:09]

Some generic questions first.

You state "Made the destructor of the AudioManagerPlatforms protected so it can
be called by only the AudioManagerBase." byt I am not able to find such
modifications.

Please remove usage of Linear in function names. It adds no information to
maintain it.

Is this CL really complete? I assume that many more files must be modified to
ensure that Chrome builds.

[henrika (OOO until Aug 14), 2012-03-02 08:31:16]

http://codereview.chromium.org/9570014/diff/1018/media/audio/android/audio_ma...
File media/audio/android/audio_manager_android.h (right):

http://codereview.chromium.org/9570014/diff/1018/media/audio/android/audio_ma...
media/audio/android/audio_manager_android.h:23: virtual AudioOutputStream*
MakeAudioLinearOutputStream(
Why not call this MakeAudioOutputStream() instead? Adding Linear does not make
sense to me. It is in fact the default method given that it is used bu
<audio>/<video>.

http://codereview.chromium.org/9570014/diff/1018/media/audio/android/audio_ma...
media/audio/android/audio_manager_android.h:27: virtual AudioInputStream*
MakeAudioLinearInputStream(
Remove Linear. Please.

http://codereview.chromium.org/9570014/diff/1018/media/audio/audio_input_volu...
File media/audio/audio_input_volume_unittest.cc (right):

http://codereview.chromium.org/9570014/diff/1018/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:98: if (device_names.empty())
Why added to this CL?

http://codereview.chromium.org/9570014/diff/1018/media/audio/win/audio_manage...
File media/audio/win/audio_manager_win.cc (right):

http://codereview.chromium.org/9570014/diff/1018/media/audio/win/audio_manage...
media/audio/win/audio_manager_win.cc:42: static const int kMaxOutputStreams =
50;
why changed  to int?

[no longer working on chromium, 2012-03-02 13:00:49]

http://codereview.chromium.org/9570014/diff/1018/media/audio/android/audio_ma...
File media/audio/android/audio_manager_android.h (right):

http://codereview.chromium.org/9570014/diff/1018/media/audio/android/audio_ma...
media/audio/android/audio_manager_android.h:23: virtual AudioOutputStream*
MakeAudioLinearOutputStream(
On 2012/03/02 08:31:16, henrika wrote:
> Why not call this MakeAudioOutputStream() instead? Adding Linear does not make
> sense to me. It is in fact the default method given that it is used bu
> <audio>/<video>.

As we discussed offline, the name is picked to match the AUDIO_PCM_LINEAR mode,
but I am open to another better name.

http://codereview.chromium.org/9570014/diff/1018/media/audio/audio_input_volu...
File media/audio/audio_input_volume_unittest.cc (right):

http://codereview.chromium.org/9570014/diff/1018/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:98: if (device_names.empty())
On 2012/03/02 08:31:16, henrika wrote:
> Why added to this CL?

If we enable the FullDuplexTest in audio_low_latency_input_output_unittest.cc,
the device_names can be a empty list in linux since the device can't be opened.
Maybe I should remove this and make it a separate CL, but since it is so
trivial, so I do the modification here. Let me know if you want me to do it in
another CL.

http://codereview.chromium.org/9570014/diff/1018/media/audio/win/audio_manage...
File media/audio/win/audio_manager_win.cc (right):

http://codereview.chromium.org/9570014/diff/1018/media/audio/win/audio_manage...
media/audio/win/audio_manager_win.cc:42: static const int kMaxOutputStreams =
50;
On 2012/03/02 08:31:16, henrika wrote:
> why changed  to int?

Either int or size_t works. I use int because I don't see the reason why we
should use an unsigned type size_t

[tommi (sloooow) - chröme, 2012-03-05 14:28:28]

http://codereview.chromium.org/9570014/diff/5002/media/audio/android/audio_ma...
File media/audio/android/audio_manager_android.cc (right):

http://codereview.chromium.org/9570014/diff/5002/media/audio/android/audio_ma...
media/audio/android/audio_manager_android.cc:54: return
AudioTrackOutputStream::MakeStream(params);;
fix indent and remove extra semicolon

http://codereview.chromium.org/9570014/diff/5002/media/audio/android/audio_ma...
File media/audio/android/audio_manager_android.h (right):

http://codereview.chromium.org/9570014/diff/5002/media/audio/android/audio_ma...
media/audio/android/audio_manager_android.h:22: virtual int
GetMaxAudioOutputStreamsAllowed() OVERRIDE;
return size_t?

http://codereview.chromium.org/9570014/diff/5002/media/audio/android/audio_ma...
media/audio/android/audio_manager_android.h:23: virtual AudioOutputStream*
MakeAudioLinearOutputStream(
I think the word "Audio" isn't necessary in these function names.

http://codereview.chromium.org/9570014/diff/5002/media/audio/audio_input_volu...
File media/audio/audio_input_volume_unittest.cc (right):

http://codereview.chromium.org/9570014/diff/5002/media/audio/audio_input_volu...
media/audio/audio_input_volume_unittest.cc:99: return;
LOG(WARNING)

http://codereview.chromium.org/9570014/diff/5002/media/audio/audio_manager_ba...
File media/audio/audio_manager_base.cc (right):

http://codereview.chromium.org/9570014/diff/5002/media/audio/audio_manager_ba...
media/audio/audio_manager_base.cc:50: return NULL;
NOTREACHED?

http://codereview.chromium.org/9570014/diff/5002/media/audio/audio_manager_ba...
media/audio/audio_manager_base.cc:62: stream =
FakeAudioOutputStream::MakeFakeStream(params);
missing increment?

http://codereview.chromium.org/9570014/diff/5002/media/audio/audio_manager_ba...
media/audio/audio_manager_base.cc:64: num_output_streams_++;
what about not incrementing inside every if/else and instead add an extra if()
just before return:

if (stream)
  ++num_output_streams_;

That way you also avoid erroneously incrementing if the return value was NULL.

http://codereview.chromium.org/9570014/diff/5002/media/audio/audio_manager_ba...
media/audio/audio_manager_base.cc:77: return NULL;
NOTREACHED?

http://codereview.chromium.org/9570014/diff/5002/media/audio/audio_manager_ba...
media/audio/audio_manager_base.cc:88: return stream;
just thinking out loud here... 

we're not counting the input streams here as we do for output streams.  Yet we
have special functions below for counting the input streams via
IncreaseActiveInputStreamCount etc.

Can't we simplify this and increment the input stream count here and decrement
in ReleaseInputStream, the same way as we do for output streams, and get rid of
num_active_input_streams_ and related functions?  Is there something I'm missing
perhaps?

http://codereview.chromium.org/9570014/diff/5002/media/audio/linux/alsa_input.cc
File media/audio/linux/alsa_input.cc (right):

http://codereview.chromium.org/9570014/diff/5002/media/audio/linux/alsa_input...
media/audio/linux/alsa_input.cc:256: if (mixer_handle_)
where is mixer_handle_ set to NULL?
... I guess it's not needed here since we're about to delete the object, but
since we do it for audio_packet_ and device_handle_, we might as well do it for
mixer_handle_, right?

http://codereview.chromium.org/9570014/diff/5002/media/audio/linux/alsa_outpu...
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/9570014/diff/5002/media/audio/linux/alsa_outpu...
media/audio/linux/alsa_output_unittest.cc:94: // the number of active streams.
nit: maybe a note on why we want to skip checking it?

http://codereview.chromium.org/9570014/diff/5002/media/audio/linux/alsa_outpu...
media/audio/linux/alsa_output_unittest.cc:180: scoped_ptr<AlsaPcmOutputStream>
test_stream_;
when we call test_stream_->Close(), won't that cause the test_stream_ object to
be deleted?  If so, then won't we run into double deletion when the scoped_ptr
tries to delete it?

Same for every time we call test_stream_.reset() right after calling
test_stream_->Close().

http://codereview.chromium.org/9570014/diff/5002/media/audio/linux/audio_mana...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/9570014/diff/5002/media/audio/linux/audio_mana...
media/audio/linux/audio_manager_linux.cc:277: switches::kAlsaOutputDevice)) {
indent feels off to me

Foo(arg1,
arg2)

compared to
Foo(arg1,
    arg2)

http://codereview.chromium.org/9570014/diff/5002/media/audio/win/audio_manage...
File media/audio/win/audio_manager_win.cc (right):

http://codereview.chromium.org/9570014/diff/5002/media/audio/win/audio_manage...
media/audio/win/audio_manager_win.cc:264: DLOG(INFO) << "Using WaveOut since
WASAPI requires at least Vista.";
DVLOG(1)
(same for any other DLOG(INFO))

[no longer working on chromium, 2012-03-06 15:27:07]

Changed the code based on the comments from Tommi and discussion with Henrik. 
Please take the second round.

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/android/...
File media/audio/android/audio_manager_android.cc (right):

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/android/...
media/audio/android/audio_manager_android.cc:54: return
AudioTrackOutputStream::MakeStream(params);;
On 2012/03/05 14:28:28, tommi wrote:
> fix indent and remove extra semicolon 

Done.

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/android/...
File media/audio/android/audio_manager_android.h (right):

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/android/...
media/audio/android/audio_manager_android.h:22: virtual int
GetMaxAudioOutputStreamsAllowed() OVERRIDE;
On 2012/03/05 14:28:28, tommi wrote:
> return size_t?

As we discussed, hope it is fine to use int.

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/audio_in...
File media/audio/audio_input_volume_unittest.cc (right):

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/audio_in...
media/audio/audio_input_volume_unittest.cc:99: return;
On 2012/03/05 14:28:28, tommi wrote:
> LOG(WARNING)

Done.

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/audio_ma...
File media/audio/audio_manager_base.cc (right):

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/audio_ma...
media/audio/audio_manager_base.cc:50: return NULL;
On 2012/03/05 14:28:28, tommi wrote:
> NOTREACHED?

We have some failed unittests which will try the invalid parameters, adding a
NOTREACHED will break those tests, so I added a DLOG(ERROR) instead.

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/audio_ma...
media/audio/audio_manager_base.cc:62: stream =
FakeAudioOutputStream::MakeFakeStream(params);
On 2012/03/05 14:28:28, tommi wrote:
> missing increment?

We do not need to limit the max number of the fake streams.

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/audio_ma...
media/audio/audio_manager_base.cc:64: num_output_streams_++;
On 2012/03/05 14:28:28, tommi wrote:
> what about not incrementing inside every if/else and instead add an extra if()
> just before return:
> 
> if (stream)
>   ++num_output_streams_;
> 
> That way you also avoid erroneously incrementing if the return value was NULL.

it may not be a good idea since fake stream needs to be out of the counting.

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/audio_ma...
media/audio/audio_manager_base.cc:77: return NULL;
On 2012/03/05 14:28:28, tommi wrote:
> NOTREACHED?

The same here, added a DLOG instead.

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/audio_ma...
media/audio/audio_manager_base.cc:88: return stream;
On 2012/03/05 14:28:28, tommi wrote:
> just thinking out loud here... 
> 
> we're not counting the input streams here as we do for output streams.  Yet we
> have special functions below for counting the input streams via
> IncreaseActiveInputStreamCount etc.
> 
> Can't we simplify this and increment the input stream count here and decrement
> in ReleaseInputStream, the same way as we do for output streams, and get rid
of
> num_active_input_streams_ and related functions?  Is there something I'm
missing
> perhaps?

These ActiveInputStreamCount APIs are added by speech recognition team to keep
track if there is any active recording stream, so they are called in Start/Stop,
which is completely different from our num_output_streams_.

We should keep it for now, and may refactor it in a follow up CL.

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/linux/al...
File media/audio/linux/alsa_input.cc (right):

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/linux/al...
media/audio/linux/alsa_input.cc:256: if (mixer_handle_)
On 2012/03/05 14:28:28, tommi wrote:
> where is mixer_handle_ set to NULL?
> ... I guess it's not needed here since we're about to delete the object, but
> since we do it for audio_packet_ and device_handle_, we might as well do it
for
> mixer_handle_, right?

Done.

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/linux/au...
File media/audio/linux/audio_manager_linux.cc (right):

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/linux/au...
media/audio/linux/audio_manager_linux.cc:277: switches::kAlsaOutputDevice)) {
On 2012/03/05 14:28:28, tommi wrote:
> indent feels off to me
> 
> Foo(arg1,
> arg2)
> 
> compared to
> Foo(arg1,
>     arg2)

Done.

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/win/audi...
File media/audio/win/audio_manager_win.cc (right):

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/win/audi...
media/audio/win/audio_manager_win.cc:264: DLOG(INFO) << "Using WaveOut since
WASAPI requires at least Vista.";
On 2012/03/05 14:28:28, tommi wrote:
> DVLOG(1)
> (same for any other DLOG(INFO))

Done.

[no longer working on chromium, 2012-03-06 15:27:27]

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/android/...
File media/audio/android/audio_manager_android.h (right):

https://chromiumcodereview.appspot.com/9570014/diff/5002/media/audio/android/...
media/audio/android/audio_manager_android.h:23: virtual AudioOutputStream*
MakeAudioLinearOutputStream(
On 2012/03/05 14:28:28, tommi wrote:
> I think the word "Audio" isn't necessary in these function names.

Done.

[tommi (sloooow) - chröme, 2012-03-06 16:16:04]

lgtm with a couple of little requests.

https://chromiumcodereview.appspot.com/9570014/diff/15001/media/audio/audio_m...
File media/audio/audio_manager_base.cc (right):

https://chromiumcodereview.appspot.com/9570014/diff/15001/media/audio/audio_m...
media/audio/audio_manager_base.cc:119: DCHECK(stream);
Can you add
DCHECK(GetMessageLoop()->BelongsToCurrentThread());

https://chromiumcodereview.appspot.com/9570014/diff/15001/media/audio/audio_m...
media/audio/audio_manager_base.cc:125: DCHECK(stream);
DCHECK(GetMessageLoop()->BelongsToCurrentThread());
here as well

https://chromiumcodereview.appspot.com/9570014/diff/15001/media/audio/audio_m...
File media/audio/audio_manager_base.h (right):

https://chromiumcodereview.appspot.com/9570014/diff/15001/media/audio/audio_m...
media/audio/audio_manager_base.h:69: virtual int GetMaxOutputStreamsAllowed() =
0;
nit: Instead of a pure virtual function + implementation everywhere, we could
just have an int member variable in this class and an inline setter that can be
called in the constructor of the derived classes.

 protected:
  void set_max_output_streams(int max) { max_output_streams_ = max; }

 private:
  int max_output_streams_;  // initialized to something reasonable.


Then in derived classes:

MySpecialAudioManager() {
  set_max_output_streams(10);
}

[no longer working on chromium, 2012-03-06 17:38:40]

Addressed Tommi's another comments.

http://codereview.chromium.org/9570014/diff/15001/media/audio/audio_manager_b...
File media/audio/audio_manager_base.cc (right):

http://codereview.chromium.org/9570014/diff/15001/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:119: DCHECK(stream);
On 2012/03/06 16:16:04, tommi wrote:
> Can you add
> DCHECK(GetMessageLoop()->BelongsToCurrentThread());

Doing this will break quite some unittests, since we have quite some tests which
are not running on the AudioManager thread. Is it necessary to change them?

http://codereview.chromium.org/9570014/diff/15001/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:125: DCHECK(stream);
On 2012/03/06 16:16:04, tommi wrote:
> DCHECK(GetMessageLoop()->BelongsToCurrentThread());
> here as well

same question.

http://codereview.chromium.org/9570014/diff/15001/media/audio/audio_manager_b...
File media/audio/audio_manager_base.h (right):

http://codereview.chromium.org/9570014/diff/15001/media/audio/audio_manager_b...
media/audio/audio_manager_base.h:69: virtual int GetMaxOutputStreamsAllowed() =
0;
On 2012/03/06 16:16:04, tommi wrote:
> nit: Instead of a pure virtual function + implementation everywhere, we could
> just have an int member variable in this class and an inline setter that can
be
> called in the constructor of the derived classes.
> 
>  protected:
>   void set_max_output_streams(int max) { max_output_streams_ = max; }
> 
>  private:
>   int max_output_streams_;  // initialized to something reasonable.
> 
> 
> Then in derived classes:
> 
> MySpecialAudioManager() {
>   set_max_output_streams(10);
> }

Done.

[tommi (sloooow) - chröme, 2012-03-06 17:44:12]

Ah, skip the dcheck then.
Lgtm++
On Mar 6, 2012 6:38 PM, <xians@chromium.org> wrote:

> Addressed Tommi's another comments.
>
>
> http://codereview.chromium.**org/9570014/diff/15001/media/**
>
audio/audio_manager_base.cc<http://codereview.chromium.org/9570014/diff/15001/media/audio/audio_manager_base.cc>
> File media/audio/audio_manager_**base.cc (right):
>
> http://codereview.chromium.**org/9570014/diff/15001/media/**
>
audio/audio_manager_base.cc#**newcode119<http://codereview.chromium.org/9570014/diff/15001/media/audio/audio_manager_base.cc#newcode119>
> media/audio/audio_manager_**base.cc:119: DCHECK(stream);
> On 2012/03/06 16:16:04, tommi wrote:
>
>> Can you add
>> DCHECK(GetMessageLoop()->**BelongsToCurrentThread());
>>
>
> Doing this will break quite some unittests, since we have quite some
> tests which are not running on the AudioManager thread. Is it necessary
> to change them?
>
> http://codereview.chromium.**org/9570014/diff/15001/media/**
>
audio/audio_manager_base.cc#**newcode125<http://codereview.chromium.org/9570014/diff/15001/media/audio/audio_manager_base.cc#newcode125>
> media/audio/audio_manager_**base.cc:125: DCHECK(stream);
> On 2012/03/06 16:16:04, tommi wrote:
>
>> DCHECK(GetMessageLoop()->**BelongsToCurrentThread());
>> here as well
>>
>
> same question.
>
> http://codereview.chromium.**org/9570014/diff/15001/media/**
>
audio/audio_manager_base.h<http://codereview.chromium.org/9570014/diff/15001/media/audio/audio_manager_base.h>
> File media/audio/audio_manager_**base.h (right):
>
> http://codereview.chromium.**org/9570014/diff/15001/media/**
>
audio/audio_manager_base.h#**newcode69<http://codereview.chromium.org/9570014/diff/15001/media/audio/audio_manager_base.h#newcode69>
> media/audio/audio_manager_**base.h:69: virtual int
> GetMaxOutputStreamsAllowed() = 0;
> On 2012/03/06 16:16:04, tommi wrote:
>
>> nit: Instead of a pure virtual function + implementation everywhere,
>>
> we could
>
>> just have an int member variable in this class and an inline setter
>>
> that can be
>
>> called in the constructor of the derived classes.
>>
>
>   protected:
>>   void set_max_output_streams(int max) { max_output_streams_ = max; }
>>
>
>   private:
>>   int max_output_streams_;  // initialized to something reasonable.
>>
>
>
>  Then in derived classes:
>>
>
>  MySpecialAudioManager() {
>>   set_max_output_streams(10);
>> }
>>
>
> Done.
>
>
http://codereview.chromium.**org/9570014/<http://codereview.chromium.org/9570...
>

[scherkus (not reviewing), 2012-03-06 22:31:51]

few drive-by things but not adding myself as a reviewer so don't wait for my
review before committing! :)

http://codereview.chromium.org/9570014/diff/16006/media/audio/android/audio_m...
File media/audio/android/audio_manager_android.cc (right):

http://codereview.chromium.org/9570014/diff/16006/media/audio/android/audio_m...
media/audio/android/audio_manager_android.cc:61: AudioInputStream*
AudioManagerAndroid::MakeALowLatencyInputStream(
typo?  "MakeA..." --> "Make..." ?

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
File media/audio/audio_manager_base.cc (right):

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:22: max_num_output_streams_(0),
so if implementors fail to set something all audio will fail?

is that intended?

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:60: return NULL;
should we LOG(WARNING) here?  otherwise the resulting behaviour is very strange
(no sound)

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:119: void
AudioManagerBase::ReleaseOutputStream(AudioOutputStream* stream) {
I'd really like us to see this re-done at some point

right now we have a very non-obvious destruction path via
AudioOutputStream::Close() which implementors must remember to call
AudioManagerBase::ReleaseOutputStream()

perhaps the proper way to "close" a stream is to actually pass ownership back to
AudioManager so it can close, delete, and decrement

you don't have to do this in this CL but it would be good to add a TODO+bug and
think about cleaning this up at some point

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
File media/audio/audio_manager_base.h (right):

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
media/audio/audio_manager_base.h:67: // Creates the output stream for the
|AUDIO_PCM_LINEAR format|. The legacy
nit: move format outside of ||

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
media/audio/audio_manager_base.h:76: // Creates the input stream for the
|AUDIO_PCM_LINEAR format|. The legacy
nit: move format outside of ||

http://codereview.chromium.org/9570014/diff/16006/media/audio/win/audio_manag...
File media/audio/win/audio_manager_win.h (right):

http://codereview.chromium.org/9570014/diff/16006/media/audio/win/audio_manag...
media/audio/win/audio_manager_win.h:41: const AudioParameters& params, const
std::string& device_id) OVERRIDE;
nit: indent

[tommi (sloooow) - chröme, 2012-03-07 07:44:55]

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
File media/audio/audio_manager_base.cc (right):

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:22: max_num_output_streams_(0),
On 2012/03/06 22:31:51, scherkus wrote:
> so if implementors fail to set something all audio will fail?
> 
> is that intended?

ugh, nope, that's not what I asked for.  Shijing, see my earlier comment about a
"reasonable default".  I don't think 0 is reasonable.

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:60: return NULL;
On 2012/03/06 22:31:51, scherkus wrote:
> should we LOG(WARNING) here?  otherwise the resulting behaviour is very
strange
> (no sound)
+1  even LOG(ERROR)

[no longer working on chromium, 2012-03-07 09:57:08]

http://codereview.chromium.org/9570014/diff/16006/media/audio/android/audio_m...
File media/audio/android/audio_manager_android.cc (right):

http://codereview.chromium.org/9570014/diff/16006/media/audio/android/audio_m...
media/audio/android/audio_manager_android.cc:61: AudioInputStream*
AudioManagerAndroid::MakeALowLatencyInputStream(
On 2012/03/06 22:31:51, scherkus wrote:
> typo?  "MakeA..." --> "Make..." ?

Thanks, some uncleaned code when doing the changes.

Fixed now.

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
File media/audio/audio_manager_base.cc (right):

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:22: max_num_output_streams_(0),
On 2012/03/06 22:31:51, scherkus wrote:
> so if implementors fail to set something all audio will fail?
> 
> is that intended?

Thanks, Done.

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:22: max_num_output_streams_(0),
On 2012/03/07 07:44:55, tommi wrote:
> On 2012/03/06 22:31:51, scherkus wrote:
> > so if implementors fail to set something all audio will fail?
> > 
> > is that intended?
> 
> ugh, nope, that's not what I asked for.  Shijing, see my earlier comment about
a
> "reasonable default".  I don't think 0 is reasonable.

Sorry, I missed the comments.

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:60: return NULL;
On 2012/03/06 22:31:51, scherkus wrote:
> should we LOG(WARNING) here?  otherwise the resulting behaviour is very
strange
> (no sound)

Good point. Done

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:60: return NULL;
On 2012/03/07 07:44:55, tommi wrote:
> On 2012/03/06 22:31:51, scherkus wrote:
> > should we LOG(WARNING) here?  otherwise the resulting behaviour is very
> strange
> > (no sound)
> +1  even LOG(ERROR)

Done.

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
File media/audio/audio_manager_base.h (right):

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
media/audio/audio_manager_base.h:67: // Creates the output stream for the
|AUDIO_PCM_LINEAR format|. The legacy
On 2012/03/06 22:31:51, scherkus wrote:
> nit: move format outside of ||

Done.

http://codereview.chromium.org/9570014/diff/16006/media/audio/audio_manager_b...
media/audio/audio_manager_base.h:76: // Creates the input stream for the
|AUDIO_PCM_LINEAR format|. The legacy
On 2012/03/06 22:31:51, scherkus wrote:
> nit: move format outside of ||

Done.

http://codereview.chromium.org/9570014/diff/16006/media/audio/win/audio_manag...
File media/audio/win/audio_manager_win.h (right):

http://codereview.chromium.org/9570014/diff/16006/media/audio/win/audio_manag...
media/audio/win/audio_manager_win.h:41: const AudioParameters& params, const
std::string& device_id) OVERRIDE;
On 2012/03/06 22:31:51, scherkus wrote:
> nit: indent

Done.

[henrika (OOO until Aug 14), 2012-03-07 13:40:46]

LGTM.

[commit-bot: I haz the power, 2012-03-07 13:52:17]

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

[commit-bot: I haz the power, 2012-03-07 15:25:54]

Change committed as 125389

[scherkus (not reviewing), 2012-03-07 18:06:03]

thanks xians!