Implement AudioOutputProxy.

Implemented AudioOutputProxy.

AudioOutputProxy implements AudioOutputStream interface, but opens 
audio device only when audio is playing.

BUG=39825
TEST=Unittests.

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

[Sergey Ulanov, 2010-11-17 23:04:10]

I'm still working on unittests, but would appreciate comments on the current
design.

[Sergey Ulanov, 2010-11-18 01:11:19]

Added unittests now.

[Paweł Hajdan Jr., 2010-11-18 10:28:43]

Drive-by with a test comment. Could you let me take another look before
committing?

http://codereview.chromium.org/5158003/diff/5001/media/audio/audio_output_pro...
File media/audio/audio_output_proxy_unittest.cc (right):

http://codereview.chromium.org/5158003/diff/5001/media/audio/audio_output_pro...
media/audio/audio_output_proxy_unittest.cc:121:
PlatformThread::Sleep(kTestCloseDelayMs);
1. Why is this a Sleep and not a real wait?
2. If it turns out the Sleep is necessary, could you use
base/test/test_timeouts?

[Sergey Ulanov, 2010-11-19 00:56:36]

http://codereview.chromium.org/5158003/diff/5001/media/audio/audio_output_pro...
File media/audio/audio_output_proxy_unittest.cc (right):

http://codereview.chromium.org/5158003/diff/5001/media/audio/audio_output_pro...
media/audio/audio_output_proxy_unittest.cc:121:
PlatformThread::Sleep(kTestCloseDelayMs);
On 2010/11/18 10:28:43, Paweł Hajdan Jr. wrote:
> 1. Why is this a Sleep and not a real wait?
Not sure what you mean by "real wait" Are you suggesting creating an event and
waiting for that event here? In this test everything happens on the same thread:
the stream is closed from the task that is executed by RunAllTasks(). Changing
this Sleep to TimedWait would not change anything.

> 2. If it turns out the Sleep is necessary, could you use
> base/test/test_timeouts?
That is not necessary here. kTestCloseDelayMs is the delay used for the close
task that must be executed from RunAllPending().

[Paweł Hajdan Jr., 2010-11-19 18:51:38]

On 2010/11/19 00:56:36, sergeyu wrote:
>
http://codereview.chromium.org/5158003/diff/5001/media/audio/audio_output_pro...
> File media/audio/audio_output_proxy_unittest.cc (right):
> 
>
http://codereview.chromium.org/5158003/diff/5001/media/audio/audio_output_pro...
> media/audio/audio_output_proxy_unittest.cc:121:
> PlatformThread::Sleep(kTestCloseDelayMs);
> On 2010/11/18 10:28:43, Paweł Hajdan Jr. wrote:
> > 1. Why is this a Sleep and not a real wait?
> Not sure what you mean by "real wait" Are you suggesting creating an event and
> waiting for that event here? In this test everything happens on the same
thread:
> the stream is closed from the task that is executed by RunAllTasks(). Changing
> this Sleep to TimedWait would not change anything.

I see. Could you then change the comment from "wait" to something more like
"simulate a delay"?

> > 2. If it turns out the Sleep is necessary, could you use
> > base/test/test_timeouts?
> That is not necessary here. kTestCloseDelayMs is the delay used for the close
> task that must be executed from RunAllPending().

Still, could you use test_timeouts? You'd probably want to introduce a
few-hundred-milliseconds kind of timeout, but that's fine. The exact value of
this timeout seems arbitrary. I'd prefer that instead of wondering whether it
should be 100ms, 150ms or 75ms, we can just use some function from
test_timeouts.

[Sergey Ulanov, 2010-11-19 20:34:57]

On 2010/11/19 18:51:38, Paweł Hajdan Jr. wrote:
> On 2010/11/19 00:56:36, sergeyu wrote:
> >
>
http://codereview.chromium.org/5158003/diff/5001/media/audio/audio_output_pro...
> > File media/audio/audio_output_proxy_unittest.cc (right):
> > 
> >
>
http://codereview.chromium.org/5158003/diff/5001/media/audio/audio_output_pro...
> > media/audio/audio_output_proxy_unittest.cc:121:
> > PlatformThread::Sleep(kTestCloseDelayMs);
> > On 2010/11/18 10:28:43, Paweł Hajdan Jr. wrote:
> > > 1. Why is this a Sleep and not a real wait?
> > Not sure what you mean by "real wait" Are you suggesting creating an event
and
> > waiting for that event here? In this test everything happens on the same
> thread:
> > the stream is closed from the task that is executed by RunAllTasks().
Changing
> > this Sleep to TimedWait would not change anything.
> 
> I see. Could you then change the comment from "wait" to something more like
> "simulate a delay"?
Done.

> 
> > > 2. If it turns out the Sleep is necessary, could you use
> > > base/test/test_timeouts?
> > That is not necessary here. kTestCloseDelayMs is the delay used for the
close
> > task that must be executed from RunAllPending().
> 
> Still, could you use test_timeouts? You'd probably want to introduce a
> few-hundred-milliseconds kind of timeout, but that's fine. The exact value of
> this timeout seems arbitrary. I'd prefer that instead of wondering whether it
> should be 100ms, 150ms or 75ms, we can just use some function from
> test_timeouts.

I don't think that this particular value belongs to timeouts: it is not a
timeout. What we get from test_timeouts is that we can run tests with different
timeout values depending on the environment (e.g. higher timeouts when running
under valgrind), but this isn't needed here for this particular delay.

[Paweł Hajdan Jr., 2010-11-19 21:16:28]

Code I commented in the drive-by LGTM.

Thanks for your patience with my nits, and friendly explanations.

[scherkus (not reviewing), 2010-11-22 06:43:14]

bunch o' questions

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager.h
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager.h...
media/audio/audio_manager.h:74: virtual AudioOutputStream*
MakeAudioOutputStreamProxy(
comment?  for example how does this differ from MakeAudioOutputStream()?  In
fact this method definition should be moved underneath MakeAudioOutput() to make
the distinction more clear

const AudioParameters& params? (you seem to use const refs in other parts of
this patch and I think you said you were going to switch over in a future CL)

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

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:7: #include
"media/audio/audio_output_dispatcher.h"
dispatcher should come before proxy

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:55: output_dispatchers_[params];
I tend to avoid using operator[] with map since it'll create an element in the
map even if the key isn't found

also you don't seem to be inserting anything into output_dispatchers_ after
checking... are we supposed to be?

in any case, find() is your friend

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

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager_b...
media/audio/audio_manager_base.h:24: virtual AudioOutputStream*
MakeAudioOutputStreamProxy(
making sure are we using audio proxies on all platforms or just linux?

does it make sense to have it implemented in AudioManagerBase?

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager_b...
media/audio/audio_manager_base.h:25: AudioParameters params);
ditto here

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager_b...
media/audio/audio_manager_base.h:30: class CompareAudioParams {
does it makes sense to declare this on AudioParameters itself?

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:11: // Delay in milliseconds after since
the stream is paused untill the
untill -> until

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:33: paused_proxies_++;
I'm not really sure what this variable is tracking... also do we want to still
increase this even if we return false?

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:36: // Ensure that there is at least one
open stream.
out of curiosity do we need to have an open stream right away?

it's a bit confusing considering you attempt to schedule a close right away

what are the implications of waiting until a stream is started?

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:52: AudioOutputStream*
AudioOutputDispatcher::StreamStarted() {
so to help me understand... ownership is passed back and forth via
StreamStarted() and StreamStopped()

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:56: stream = CreateAndOpenStream();
you might be able to de-duplicate this + StreamOpened() logic if
CreateAndOpenStream() also adds it to streams_ and returns true if it was added,
false otherwise

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:139: int max_pending_streams = 
paused_proxies_;
nit: remove one space between = and paused_proxies_

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:159: if (close_task_scheduled_) {
nit: DCHECK for the message loop

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher.h (right):

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:29: ~AudioOutputDispatcher();
virtual

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:31: // Following 4 methods are called by
AudioOutput when a stream
Might be wrong here.. but do you meant AudioOutputProxy instead of AudioOutput
here?

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:35: void StreamStopped(AudioOutputStream*
stream);
I think we need some comments for these functions as reading through the code
they have some not-so-obvious logic + return values

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:40: void set_close_delay(base::TimeDelta
delay);
is this only used for tests?

consider using FRIEND_TEST() or passing this in via constructor

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:58: base::Time last_activity_time_;
I think you can replace a bunch of this last_activity_time_ + close_delay_ +
close_task_scheduled_ stuff with DelayTimer from base/timer.h

I'd encourage you to check that out + other helper classes to see if we can
avoid having complicated timing logic baked into this class

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
File media/audio/audio_output_proxy.cc (right):

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
media/audio/audio_output_proxy.cc:91: FROM_HERE, new
DeleteTask<AudioOutputProxy>(this));
you can use DeleteSoon(this) instead

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
File media/audio/audio_output_proxy.h (right):

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
media/audio/audio_output_proxy.h:4: 
could we get an explanation of this class somewhere?

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
media/audio/audio_output_proxy.h:5: #ifndef
MEDIA_AUDIO_AUDIO_OUTPUT_STREAM_PROXY_H
header guards should end with _

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
media/audio/audio_output_proxy.h:15: class AudioOutputProxy : public
AudioOutputStream {
hrmm I'm surprised more of the audio code isn't under namespace media

oh well don't change it for now we can do that in a follow up CL

http://codereview.chromium.org/5158003/diff/11001/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/5158003/diff/11001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:99: // Free output dispatchers first,
so that they closes all streams that
nit: so that they closes all streams that are still open -> closing all
remaining open streams

http://codereview.chromium.org/5158003/diff/11001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:101: output_dispatchers_.clear();
why only on linux?

http://codereview.chromium.org/5158003/diff/11001/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/5158003/diff/11001/media/media.gyp#newcode36
media/media.gyp:36: 'audio/audio_output_dispatcher.cc',
dispatcher should come before proxy

[scherkus (not reviewing), 2010-11-22 06:43:48]

one extra comment!

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
File media/audio/audio_output_proxy.cc (right):

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
media/audio/audio_output_proxy.cc:73: physical_stream_->SetVolume(volume);
I think I need an explanation of how everything ties together... the idea is to
*not* share physical streams between virtual streams, but rather still maintain
a 1:1 between physical:virtual but shut down the physical stream after a certain
amount of time has passed?

[Sergey Ulanov, 2010-11-23 19:51:46]

Thanks for suggesting DelayTimer! It simplified dispatcher code a lot.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager.h
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager.h...
media/audio/audio_manager.h:74: virtual AudioOutputStream*
MakeAudioOutputStreamProxy(
On 2010/11/22 06:43:15, scherkus wrote:
> comment?  for example how does this differ from MakeAudioOutputStream()?  In
> fact this method definition should be moved underneath MakeAudioOutput() to
make
> the distinction more clear
> 
Done.

> const AudioParameters& params? (you seem to use const refs in other parts of
> this patch and I think you said you were going to switch over in a future CL)
Changed it to const ref for this method. Will update MakeAudioOutputStream() in
a separate CL.

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

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:7: #include
"media/audio/audio_output_dispatcher.h"
On 2010/11/22 06:43:15, scherkus wrote:
> dispatcher should come before proxy

Done.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:55: output_dispatchers_[params];
On 2010/11/22 06:43:15, scherkus wrote:
> I tend to avoid using operator[] with map since it'll create an element in the
> map even if the key isn't found
Yes, and it is desired behavior here.

> 
> also you don't seem to be inserting anything into output_dispatchers_ after
> checking... are we supposed to be?
dispatcher is a reference to the map element. So the line below actually stores
new dispatcher in the map. Here is how this code would look like with
find()/insert()

  AudioOutputDispatcher* dispatcher;
  AudioOutputDispatchersMap::iterator it = output_dispatchers_.find(params);
  if (it == output_dispatchers_.end()) {
    dispatcher = new AudioOutputDispatcher(this, params);
    output_dispatchers_.insert(
        AudioOutputDispatchersMap::value_type(params, dispatcher));
  } else {
    dispatcher = it->second;
  }

It's almost twice as many lines, and it is less efficient :). Let me know if you
like this version better.

> 
> in any case, find() is your friend

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

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager_b...
media/audio/audio_manager_base.h:24: virtual AudioOutputStream*
MakeAudioOutputStreamProxy(
On 2010/11/22 06:43:15, scherkus wrote:
> making sure are we using audio proxies on all platforms or just linux?
> 
> does it make sense to have it implemented in AudioManagerBase?

It works on all platforms, not only linux.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager_b...
media/audio/audio_manager_base.h:25: AudioParameters params);
On 2010/11/22 06:43:15, scherkus wrote:
> ditto here

Done.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager_b...
media/audio/audio_manager_base.h:30: class CompareAudioParams {
On 2010/11/22 06:43:15, scherkus wrote:
> does it makes sense to declare this on AudioParameters itself?
Done

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:11: // Delay in milliseconds after since
the stream is paused untill the
On 2010/11/22 06:43:15, scherkus wrote:
> untill -> until

Done.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:33: paused_proxies_++;
On 2010/11/22 06:43:15, scherkus wrote:
> I'm not really sure what this variable is tracking... 
Number of streams that are currently paused.

> also do we want to still increase this even if we return false?
Yes: the proxy will call StreamClosed() later anyway.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:36: // Ensure that there is at least one
open stream.
On 2010/11/22 06:43:15, scherkus wrote:
> out of curiosity do we need to have an open stream right away?
It is just so that a page that created media element can be notified that the
stream is unplayable (e.g. invalid sample rate) before it actually tries to
starts playing. I discussed this design with Alpha a while ago, and he seemed to
agree. Do you think this isn't necessary?

> 
> it's a bit confusing considering you attempt to schedule a close right away
Yes, it is closed 5 seconds later if Start() isn't called before that.

> 
> what are the implications of waiting until a stream is started?

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:52: AudioOutputStream*
AudioOutputDispatcher::StreamStarted() {
On 2010/11/22 06:43:15, scherkus wrote:
> so to help me understand... ownership is passed back and forth via
> StreamStarted() and StreamStopped()
Yes. Added more comments about it in .h

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:56: stream = CreateAndOpenStream();
On 2010/11/22 06:43:15, scherkus wrote:
> you might be able to de-duplicate this + StreamOpened() logic if
> CreateAndOpenStream() also adds it to streams_ and returns true if it was
added,
> false otherwise
Good catch. Fixed!

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:139: int max_pending_streams = 
paused_proxies_;
On 2010/11/22 06:43:15, scherkus wrote:
> nit: remove one space between = and paused_proxies_

Done.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:159: if (close_task_scheduled_) {
On 2010/11/22 06:43:15, scherkus wrote:
> nit: DCHECK for the message loop

Done.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher.h (right):

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:29: ~AudioOutputDispatcher();
On 2010/11/22 06:43:15, scherkus wrote:
> virtual
Nobody inherits from this class, and there are no virtual methods, so destructor
doesn't need to be virtual.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:31: // Following 4 methods are called by
AudioOutput when a stream
On 2010/11/22 06:43:15, scherkus wrote:
> Might be wrong here.. but do you meant AudioOutputProxy instead of AudioOutput
> here?

Done.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:35: void StreamStopped(AudioOutputStream*
stream);
On 2010/11/22 06:43:15, scherkus wrote:
> I think we need some comments for these functions as reading through the code
> they have some not-so-obvious logic + return values

Done.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:40: void set_close_delay(base::TimeDelta
delay);
On 2010/11/22 06:43:15, scherkus wrote:
> is this only used for tests?
Yes.
> 
> consider using FRIEND_TEST() or passing this in via constructor
Move this to constructor

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.h:58: base::Time last_activity_time_;
On 2010/11/22 06:43:15, scherkus wrote:
> I think you can replace a bunch of this last_activity_time_ + close_delay_ +
> close_task_scheduled_ stuff with DelayTimer from base/timer.h
Wow, that is fantastic. I didn't know about it. Yes, DelayTimer works for this
scenario. Thanks for suggesting it!

> 
> I'd encourage you to check that out + other helper classes to see if we can
> avoid having complicated timing logic baked into this class

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
File media/audio/audio_output_proxy.cc (right):

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
media/audio/audio_output_proxy.cc:73: physical_stream_->SetVolume(volume);
On 2010/11/22 06:43:48, scherkus wrote:
> I think I need an explanation of how everything ties together... the idea is
to
> *not* share physical streams between virtual streams, but rather still
maintain
> a 1:1 between physical:virtual but shut down the physical stream after a
certain
> amount of time has passed?

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
media/audio/audio_output_proxy.cc:91: FROM_HERE, new
DeleteTask<AudioOutputProxy>(this));
On 2010/11/22 06:43:15, scherkus wrote:
> you can use DeleteSoon(this) instead

Done.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
File media/audio/audio_output_proxy.h (right):

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
media/audio/audio_output_proxy.h:4: 
On 2010/11/22 06:43:15, scherkus wrote:
> could we get an explanation of this class somewhere?

Done.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
media/audio/audio_output_proxy.h:5: #ifndef
MEDIA_AUDIO_AUDIO_OUTPUT_STREAM_PROXY_H
On 2010/11/22 06:43:15, scherkus wrote:
> header guards should end with _

Done.

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_output_pr...
media/audio/audio_output_proxy.h:15: class AudioOutputProxy : public
AudioOutputStream {
On 2010/11/22 06:43:15, scherkus wrote:
> hrmm I'm surprised more of the audio code isn't under namespace media
> 
> oh well don't change it for now we can do that in a follow up CL
Yes, There is a lot of stuff that isn't under media::. 
I will fix it for all classes at the same time in a separate CL.

http://codereview.chromium.org/5158003/diff/11001/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/5158003/diff/11001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:99: // Free output dispatchers first,
so that they closes all streams that
On 2010/11/22 06:43:15, scherkus wrote:
> nit: so that they closes all streams that are still open -> closing all
> remaining open streams

Done.

http://codereview.chromium.org/5158003/diff/11001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:101: output_dispatchers_.clear();
On 2010/11/22 06:43:15, scherkus wrote:
> why only on linux?
Because on Windows and on Mac AudioManager doesn't store list of audio streams.
I don't think we need this for Linux ether, but I'll leave this for a separate
CL.

http://codereview.chromium.org/5158003/diff/11001/media/media.gyp
File media/media.gyp (right):

http://codereview.chromium.org/5158003/diff/11001/media/media.gyp#newcode36
media/media.gyp:36: 'audio/audio_output_dispatcher.cc',
On 2010/11/22 06:43:15, scherkus wrote:
> dispatcher should come before proxy
Ah. That is because it was called ProxyDispatcher initially. Fixed.

[scherkus (not reviewing), 2010-11-24 02:01:59]

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

http://codereview.chromium.org/5158003/diff/11001/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:55: output_dispatchers_[params];
On 2010/11/23 19:51:46, sergeyu wrote:
> On 2010/11/22 06:43:15, scherkus wrote:
> > I tend to avoid using operator[] with map since it'll create an element in
the
> > map even if the key isn't found
> Yes, and it is desired behavior here.
> 
> > 
> > also you don't seem to be inserting anything into output_dispatchers_ after
> > checking... are we supposed to be?
> dispatcher is a reference to the map element. So the line below actually
stores
> new dispatcher in the map. Here is how this code would look like with
> find()/insert()
> 
>   AudioOutputDispatcher* dispatcher;
>   AudioOutputDispatchersMap::iterator it = output_dispatchers_.find(params);
>   if (it == output_dispatchers_.end()) {
>     dispatcher = new AudioOutputDispatcher(this, params);
>     output_dispatchers_.insert(
>         AudioOutputDispatchersMap::value_type(params, dispatcher));
>   } else {
>     dispatcher = it->second;
>   }
> 
> It's almost twice as many lines, and it is less efficient :). Let me know if
you
> like this version better.
> 
> > 
> > in any case, find() is your friend
> 

Thanks for explaining -- SGTM!

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_manager.h
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_manager.h...
media/audio/audio_manager.h:58: // AudioOutputStream inteface, but unlike
regular output stream
nit: inteface -> interface

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:31: if (streams_.size() == 0) {
combine these two conditions

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:45: if (streams_.size() == 0) {
combine these two conditions

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:79: while
(static_cast<int>(streams_.size()) > paused_proxies_) {
hmm maybe paused_proxies_ should be size_t ?

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:115: while
(static_cast<int>(streams_.size()) > 0) {
nit: instead of cast etc... what about
while (!streams_.empty()) { ... }

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_pr...
File media/audio/audio_output_proxy_unittest.cc (right):

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_pr...
media/audio/audio_output_proxy_unittest.cc:21: const int kTestBigCloseDelayMs =
1000 * 1000;
this worries me a lot w.r.t. flaky tests

any chance we can simply disable the auto-close behaviour when we don't want it
to interfere?

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_pr...
media/audio/audio_output_proxy_unittest.cc:124:
PlatformThread::Sleep(kTestCloseDelayMs * 2);
this could also be flaky (although rare)

it'd be best if we could loop until we're sure Close() has been closed.. gmock
might be able to help us out here

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_pr...
media/audio/audio_output_proxy_unittest.cc:184:
PlatformThread::Sleep(kTestCloseDelayMs * 10);
ditto

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_parameter...
File media/audio/audio_parameters.cc (right):

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_parameter...
media/audio/audio_parameters.cc:43: bool
AudioParameters::CompareAudioParams::operator()(
we should get a unit test slapped on this + GetPacketSize()

should be pretty easy to whip up a simple test case

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_parameters.h
File media/audio/audio_parameters.h (right):

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_parameter...
media/audio/audio_parameters.h:13: class CompareAudioParams {
nit: considering this is declared inside AudioParameters I'd simply name this
class "Compare"

[Sergey Ulanov, 2010-11-24 03:49:54]

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_manager.h
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_manager.h...
media/audio/audio_manager.h:58: // AudioOutputStream inteface, but unlike
regular output stream
On 2010/11/24 02:01:59, scherkus wrote:
> nit: inteface -> interface

Done.

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_di...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:31: if (streams_.size() == 0) {
On 2010/11/24 02:01:59, scherkus wrote:
> combine these two conditions

Done.

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:45: if (streams_.size() == 0) {
On 2010/11/24 02:01:59, scherkus wrote:
> combine these two conditions

Done.

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:79: while
(static_cast<int>(streams_.size()) > paused_proxies_) {
On 2010/11/24 02:01:59, scherkus wrote:
> hmm maybe paused_proxies_ should be size_t ?

Done.

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_di...
media/audio/audio_output_dispatcher.cc:115: while
(static_cast<int>(streams_.size()) > 0) {
On 2010/11/24 02:01:59, scherkus wrote:
> nit: instead of cast etc... what about
> while (!streams_.empty()) { ... }

Done.

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_pr...
File media/audio/audio_output_proxy_unittest.cc (right):

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_pr...
media/audio/audio_output_proxy_unittest.cc:21: const int kTestBigCloseDelayMs =
1000 * 1000;
On 2010/11/24 02:01:59, scherkus wrote:
> this worries me a lot w.r.t. flaky tests
> 
> any chance we can simply disable the auto-close behaviour when we don't want
it
> to interfere?

1000 seconds should be more than any reasonable timeout for this kind of tests
(even when running under TSan/valgrind), so setting delay to 1000 effectively
disables auto-close.

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_output_pr...
media/audio/audio_output_proxy_unittest.cc:124:
PlatformThread::Sleep(kTestCloseDelayMs * 2);
On 2010/11/24 02:01:59, scherkus wrote:
> this could also be flaky (although rare)
> 
> it'd be best if we could loop until we're sure Close() has been closed.. gmock
> might be able to help us out here
Sleep must guarantee that it sleeps the specified delay. MessageLoop must
guarantee that it runs delayed task after that sleep. Any flakiness here would
indicate a bug in ether Sleep, or in MessageLoop.

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_parameter...
File media/audio/audio_parameters.cc (right):

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_parameter...
media/audio/audio_parameters.cc:43: bool
AudioParameters::CompareAudioParams::operator()(
On 2010/11/24 02:01:59, scherkus wrote:
> we should get a unit test slapped on this + GetPacketSize()
> 
> should be pretty easy to whip up a simple test case

Done.

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_parameters.h
File media/audio/audio_parameters.h (right):

http://codereview.chromium.org/5158003/diff/29001/media/audio/audio_parameter...
media/audio/audio_parameters.h:13: class CompareAudioParams {
On 2010/11/24 02:01:59, scherkus wrote:
> nit: considering this is declared inside AudioParameters I'd simply name this
> class "Compare"

Done.

[scherkus (not reviewing), 2010-11-24 05:03:12]

LGTM w/ micronits

http://codereview.chromium.org/5158003/diff/46001/media/audio/audio_parameter...
File media/audio/audio_parameters.cc (right):

http://codereview.chromium.org/5158003/diff/46001/media/audio/audio_parameter...
media/audio/audio_parameters.cc:42: // map key.
nit: I'd delete this comment because it's duplicated from header file -- and it
still has old class name :P

http://codereview.chromium.org/5158003/diff/46001/media/audio/audio_parameters.h
File media/audio/audio_parameters.h (right):

http://codereview.chromium.org/5158003/diff/46001/media/audio/audio_parameter...
media/audio/audio_parameters.h:11: // Compare is useful when AudioParameters is
used as a key in
nit: this comment can fit on a single line

[Sergey Ulanov, 2010-11-24 19:13:51]

http://codereview.chromium.org/5158003/diff/46001/media/audio/audio_parameter...
File media/audio/audio_parameters.cc (right):

http://codereview.chromium.org/5158003/diff/46001/media/audio/audio_parameter...
media/audio/audio_parameters.cc:42: // map key.
On 2010/11/24 05:03:12, scherkus wrote:
> nit: I'd delete this comment because it's duplicated from header file -- and
it
> still has old class name :P

Done.

http://codereview.chromium.org/5158003/diff/46001/media/audio/audio_parameters.h
File media/audio/audio_parameters.h (right):

http://codereview.chromium.org/5158003/diff/46001/media/audio/audio_parameter...
media/audio/audio_parameters.h:11: // Compare is useful when AudioParameters is
used as a key in
On 2010/11/24 05:03:12, scherkus wrote:
> nit: this comment can fit on a single line

Done.