Share one thread between all AudioOutputControllers instead of creating one per stream.

Share one thread between all AudioOutputControllers instead of creating one per stream.
TEST=unittests
BUG=39825
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57392

[Sergey Ulanov, 2010-08-23 23:44:08]

Tried it with http://www/~sergeyu/test2.html - starts two sounds at the same
time. On Windows it works even better than with one thread per stream (i.e.
delay between two sounds is smaller).

[Alpha Left Google, 2010-08-24 01:23:00]

This is looking really good. It will be better if we can get rid of the wait
when we are closing. With the thread of AudioManager we can do it right this
time.

http://codereview.chromium.org/3185022/diff/1/10
File media/audio/audio_output_controller.cc (right):

http://codereview.chromium.org/3185022/diff/1/10#newcode136
media/audio/audio_output_controller.cc:136: base::WaitableEvent
closed_event(false, false);
This is a long time TODO. Maybe we can do it right this time, what about we
don't wait but unplug the event handler safely so the caller can assume Close()
is safe while this object is doing things in the background in the AudioManager
thread?

[awong, 2010-08-24 17:37:51]

Looks very good.  I like.  Go Sergey!

Added some comments.

http://codereview.chromium.org/3185022/diff/1/6
File media/audio/audio_manager.cc (right):

http://codereview.chromium.org/3185022/diff/1/6#newcode52
media/audio/audio_manager.cc:52: // By convention, the AudioManager is not
thread safe.
What does this comment mean?  I don't quite understand...

Do you mean the creation of the singleton isn't threadsafe?  And why is a
comment about the whole AudioManager on the GetAudioManager function
implementation?

http://codereview.chromium.org/3185022/diff/1/6#newcode55
media/audio/audio_manager.cc:55: LOG(WARNING) << "Creating";
Why is this LOG(WARNING)?  Seems like it should be info.

http://codereview.chromium.org/3185022/diff/1/9
File media/audio/audio_manager_base.h (right):

http://codereview.chromium.org/3185022/diff/1/9#newcode12
media/audio/audio_manager_base.h:12: class AudioManagerBase : public
AudioManager {
This class is pretty thin...is it worth abstracting out?  I'm kinda an
inheritance hater -- especially if all we're abstracting is a message loop and
an "initialized" flag, and we're sharing via protected.  We're not really
saving/centralizing any serious logic here, but adding another layer of
complexity...

http://codereview.chromium.org/3185022/diff/1/9#newcode33
media/audio/audio_manager_base.h:33: 
remove extra space.

http://codereview.chromium.org/3185022/diff/1/10
File media/audio/audio_output_controller.cc (right):

http://codereview.chromium.org/3185022/diff/1/10#newcode110
media/audio/audio_output_controller.cc:110: void AudioOutputController::Play() {
For another CL...but I think we should switch this to the reflexive dispatch
style that we're starting to do in Chromoting and get rid of this proliferation
of Do* methods.

http://codereview.chromium.org/3185022/diff/1/10#newcode141
media/audio/audio_output_controller.cc:141: closed_event.Wait();
huh...yeah, is just funky.  Stupid synchronous APIs...

http://codereview.chromium.org/3185022/diff/1/11
File media/audio/audio_output_controller.h (right):

http://codereview.chromium.org/3185022/diff/1/11#newcode183
media/audio/audio_output_controller.h:183: void DoClose(base::WaitableEvent*
closed_event);
Woohoo!

[scherkus (not reviewing), 2010-08-24 17:51:34]

http://codereview.chromium.org/3185022/diff/1/7
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/3185022/diff/1/7#newcode11
media/audio/audio_manager.h:11: class AudioInputStream;
alphabetize forward decls

http://codereview.chromium.org/3185022/diff/1/7#newcode99
media/audio/audio_manager.h:99: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/3185022/diff/1/9
File media/audio/audio_manager_base.h (right):

http://codereview.chromium.org/3185022/diff/1/9#newcode12
media/audio/audio_manager_base.h:12: class AudioManagerBase : public
AudioManager {
On 2010/08/24 17:37:52, awong wrote:
> This class is pretty thin...is it worth abstracting out?  I'm kinda an
> inheritance hater -- especially if all we're abstracting is a message loop and
> an "initialized" flag, and we're sharing via protected.  We're not really
> saving/centralizing any serious logic here, but adding another layer of
> complexity...

yeah I'd agree

I think the composition based way of tackling this would be:

class AudioManagerThread {
 public:
  void Init();
  bool initialized();
  MessageLoop* message_loop();

 private:
  base::Thread thread_;
  bool initialized_;
}

...and then make that a member variable or inject it into each
AudioManager(Windows|Linux|Mac|etc...)

Thoughts?

[awong, 2010-08-24 18:01:54]

On Tue, Aug 24, 2010 at 10:51 AM, <scherkus@chromium.org> wrote:

>
> http://codereview.chromium.org/3185022/diff/1/7
> File media/audio/audio_manager.h (right):
>
> http://codereview.chromium.org/3185022/diff/1/7#newcode11
> media/audio/audio_manager.h:11: class AudioInputStream;
> alphabetize forward decls
>
> http://codereview.chromium.org/3185022/diff/1/7#newcode99
> media/audio/audio_manager.h:99: };
> DISALLOW_COPY_AND_ASSIGN
>
>
> http://codereview.chromium.org/3185022/diff/1/9
> File media/audio/audio_manager_base.h (right):
>
> http://codereview.chromium.org/3185022/diff/1/9#newcode12
> media/audio/audio_manager_base.h:12: class AudioManagerBase : public
> AudioManager {
> On 2010/08/24 17:37:52, awong wrote:
>
>> This class is pretty thin...is it worth abstracting out?  I'm kinda an
>> inheritance hater -- especially if all we're abstracting is a message
>>
> loop and
>
>> an "initialized" flag, and we're sharing via protected.  We're not
>>
> really
>
>> saving/centralizing any serious logic here, but adding another layer
>>
> of
>
>> complexity...
>>
>
> yeah I'd agree
>
> I think the composition based way of tackling this would be:
>
> class AudioManagerThread {
>  public:
>  void Init();
>  bool initialized();
>  MessageLoop* message_loop();
>
>  private:
>  base::Thread thread_;
>  bool initialized_;
> }
>
> ...and then make that a member variable or inject it into each
> AudioManager(Windows|Linux|Mac|etc...)
>
> Thoughts?


I think it's overkill.

To generalize ad absurdum, one, could suppose the existence of a class
MessageLooped that provided a message loop + an initialized variable that
all classes should multiply inherit from.

This doesn't seem that bad, but it's started to edge closer to over
generalized that I'd like.

-Albert

[Sergey Ulanov, 2010-08-24 18:47:36]

http://codereview.chromium.org/3185022/diff/1/6
File media/audio/audio_manager.cc (right):

http://codereview.chromium.org/3185022/diff/1/6#newcode55
media/audio/audio_manager.cc:55: LOG(WARNING) << "Creating";
On 2010/08/24 17:37:52, awong wrote:
> Why is this LOG(WARNING)?  Seems like it should be info.
Removed. Added it just for debugging, forgot to remove.

http://codereview.chromium.org/3185022/diff/1/7
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/3185022/diff/1/7#newcode11
media/audio/audio_manager.h:11: class AudioInputStream;
On 2010/08/24 17:51:34, scherkus wrote:
> alphabetize forward decls

Done.

http://codereview.chromium.org/3185022/diff/1/7#newcode99
media/audio/audio_manager.h:99: };
On 2010/08/24 17:51:34, scherkus wrote:
> DISALLOW_COPY_AND_ASSIGN

This is an interface. Added DISALLOW_COPY_AND_ASSIGN for AudioManagerBase.

http://codereview.chromium.org/3185022/diff/1/9
File media/audio/audio_manager_base.h (right):

http://codereview.chromium.org/3185022/diff/1/9#newcode12
media/audio/audio_manager_base.h:12: class AudioManagerBase : public
AudioManager {
On 2010/08/24 17:37:52, awong wrote:
> This class is pretty thin...is it worth abstracting out?  I'm kinda an
> inheritance hater -- especially if all we're abstracting is a message loop and
> an "initialized" flag, and we're sharing via protected.  We're not really
> saving/centralizing any serious logic here, but adding another layer of
> complexity...
The intent is to add more code into this class later (to actually fix the bug).
The idea is that this class will take care of opening/closing streams as
necessary (i.e. close stream if it is paused). This logic will be platform
independent, so it makes sense to put it here.

http://codereview.chromium.org/3185022/diff/1/9#newcode33
media/audio/audio_manager_base.h:33: 
On 2010/08/24 17:37:52, awong wrote:
> remove extra space.

Done.

http://codereview.chromium.org/3185022/diff/1/10
File media/audio/audio_output_controller.cc (right):

http://codereview.chromium.org/3185022/diff/1/10#newcode136
media/audio/audio_output_controller.cc:136: base::WaitableEvent
closed_event(false, false);
On 2010/08/24 01:23:00, Alpha wrote:
> This is a long time TODO. Maybe we can do it right this time, what about we
> don't wait but unplug the event handler safely so the caller can assume
Close()
> is safe while this object is doing things in the background in the
AudioManager
> thread?

Done. Also moved all the code where we check/access state under lock.

[Alpha Left Google, 2010-08-24 21:32:49]

LGTM given that the comments are corrected.

http://codereview.chromium.org/3185022/diff/12001/13010
File media/audio/audio_output_controller.h (right):

http://codereview.chromium.org/3185022/diff/12001/13010#newcode143
media/audio/audio_output_controller.h:143: // Closes the audio output stream and
shutdown the audio controller thread.
Now the logic of Close is changed, can you correct this comment?

http://codereview.chromium.org/3185022/diff/12001/13011
File media/audio/audio_output_controller_unittest.cc (right):

http://codereview.chromium.org/3185022/diff/12001/13011#newcode136
media/audio/audio_output_controller_unittest.cc:136: // Now stop the controller.
This should shutdown the internal
Now the logic of Close is changed, can you correct this comment?

http://codereview.chromium.org/3185022/diff/12001/13011#newcode188
media/audio/audio_output_controller_unittest.cc:188: // Now stop the controller.
This should shutdown the internal
Now the logic of Close is changed, can you correct this comment?

http://codereview.chromium.org/3185022/diff/12001/13011#newcode253
media/audio/audio_output_controller_unittest.cc:253: // thread and we hold the
only reference to it.
Now the logic of Close is changed, can you correct this comment?

[Sergey Ulanov, 2010-08-24 21:57:09]

http://codereview.chromium.org/3185022/diff/12001/13010
File media/audio/audio_output_controller.h (right):

http://codereview.chromium.org/3185022/diff/12001/13010#newcode143
media/audio/audio_output_controller.h:143: // Closes the audio output stream and
shutdown the audio controller thread.
On 2010/08/24 21:32:49, Alpha wrote:
> Now the logic of Close is changed, can you correct this comment?

Done.

http://codereview.chromium.org/3185022/diff/12001/13011
File media/audio/audio_output_controller_unittest.cc (right):

http://codereview.chromium.org/3185022/diff/12001/13011#newcode136
media/audio/audio_output_controller_unittest.cc:136: // Now stop the controller.
This should shutdown the internal
On 2010/08/24 21:32:49, Alpha wrote:
> Now the logic of Close is changed, can you correct this comment?

Done.

http://codereview.chromium.org/3185022/diff/12001/13011#newcode188
media/audio/audio_output_controller_unittest.cc:188: // Now stop the controller.
This should shutdown the internal
On 2010/08/24 21:32:49, Alpha wrote:
> Now the logic of Close is changed, can you correct this comment?

Done.

http://codereview.chromium.org/3185022/diff/12001/13011#newcode253
media/audio/audio_output_controller_unittest.cc:253: // thread and we hold the
only reference to it.
On 2010/08/24 21:32:49, Alpha wrote:
> Now the logic of Close is changed, can you correct this comment?

Done.

[Sergey Ulanov, 2010-08-25 19:41:40]

Also added a fix in fake_audio_output_stream.cc - needed to avoid intermittent
failure in AudioRendererHostTest.SetVolume test due to races.