Remove the AudioManager singleton.

media/audio/audio_manager_base.cc

Index: media/audio/audio_manager_base.cc
===================================================================
--- media/audio/audio_manager_base.cc	(revision 113173)
+++ media/audio/audio_manager_base.cc	(working copy)
@@ -3,6 +3,8 @@
 // found in the LICENSE file.
 
 #include "media/audio/audio_manager_base.h"
+
+#include "base/bind.h"
 #include "media/audio/audio_output_dispatcher.h"
 #include "media/audio/audio_output_proxy.h"
 
@@ -13,44 +15,50 @@
 
 AudioManagerBase::AudioManagerBase()
     : audio_thread_("AudioThread"),
-      initialized_(false),
       num_active_input_streams_(0) {
 }
 
 AudioManagerBase::~AudioManagerBase() {
-  DCHECK(!audio_thread_.IsRunning());
+  Shutdown();
 }
 
-void AudioManagerBase::Init() {
-  initialized_ = audio_thread_.Start();
+#ifndef NDEBUG

[henrika (OOO until Aug 14), 2011/12/07 10:08:34]

Please add some comments here to explain the purpose of your checks. Thanks.

[tommi (sloooow) - chröme, 2011/12/07 12:26:44]

There are comments in the header.  Please let me know if they're not clear
enough.

+void AudioManagerBase::AddRef() const {
+  const MessageLoop* loop = audio_thread_.message_loop();
+  DCHECK(loop == NULL || loop != MessageLoop::current());
+  AudioManager::AddRef();
 }
 
-void AudioManagerBase::Cleanup() {
-  if (initialized_) {
-    initialized_ = false;
-    audio_thread_.Stop();
-  }
+void AudioManagerBase::Release() const {
+  const MessageLoop* loop = audio_thread_.message_loop();
+  DCHECK(loop == NULL || loop != MessageLoop::current());
+  AudioManager::Release();
 }
+#endif
 
+void AudioManagerBase::Init() {
+  CHECK(audio_thread_.Start());

[henrika (OOO until Aug 14), 2011/12/07 10:08:34]

Any point in checking Initialized() here to ensure that the thread is running?

[tommi (sloooow) - chröme, 2011/12/07 12:26:44]

audio_thread_.Start() returns true iff the thread is running.

+}
+
 string16 AudioManagerBase::GetAudioInputDeviceModel() {
   return string16();
 }
 
 MessageLoop* AudioManagerBase::GetMessageLoop() {
-  DCHECK(initialized_);
   return audio_thread_.message_loop();
 }
 
 AudioOutputStream* AudioManagerBase::MakeAudioOutputStreamProxy(
     const AudioParameters& params) {
-  if (!initialized_)
+  DCHECK_EQ(MessageLoop::current(), GetMessageLoop());
+
+  if (!initialized())
     return NULL;
 
   scoped_refptr<AudioOutputDispatcher>& dispatcher =
       output_dispatchers_[params];
   if (!dispatcher)
     dispatcher = new AudioOutputDispatcher(this, params, kStreamCloseDelayMs);
-
   return new AudioOutputProxy(dispatcher);
 }
 
@@ -66,17 +74,50 @@
 }
 
 void AudioManagerBase::IncreaseActiveInputStreamCount() {
-  base::AutoLock auto_lock(active_input_streams_lock_);
-  ++num_active_input_streams_;
+  base::AtomicRefCountInc(&num_active_input_streams_);

[henrika (OOO until Aug 14), 2011/12/07 10:08:34]

Nice!

[tommi (sloooow) - chröme, 2011/12/07 12:26:44]

Thanks! :)

 }
 
 void AudioManagerBase::DecreaseActiveInputStreamCount() {
-  base::AutoLock auto_lock(active_input_streams_lock_);
-  DCHECK_GT(num_active_input_streams_, 0);
-  --num_active_input_streams_;
+  DCHECK(IsRecordingInProcess());
+  base::AtomicRefCountDec(&num_active_input_streams_);
 }
 
 bool AudioManagerBase::IsRecordingInProcess() {
-  base::AutoLock auto_lock(active_input_streams_lock_);
-  return num_active_input_streams_ > 0;
+  return !base::AtomicRefCountIsZero(&num_active_input_streams_);
 }
+
+void AudioManagerBase::Shutdown() {
+  if (!initialized())
+    return;
+
+  DCHECK_NE(MessageLoop::current(), GetMessageLoop());
+
+  // We must use base::Unretained since Shutdown might have been called from
+  // the destructor and we can't alter the refcount of the object at that point.
+  GetMessageLoop()->PostTask(FROM_HERE, base::Bind(
+      &AudioManagerBase::ShutdownOnAudioThread,
+      base::Unretained(this)));
+  // Stop() will wait for any posted messages to be processed first.

[henrika (OOO until Aug 14), 2011/12/07 10:08:34]

Can you add some more information here please. You post a task on the audio
thread and then closes the same thread directly after. Even if your design
works; is it a good design? To me it feels a bit advanced. Can't it be
simplified or can you use any alternative method? If not, why?

[tommi (sloooow) - chröme, 2011/12/07 12:26:44]

Well, this is how the Thread class works, fine or not :) You'll see this pattern
all over chrome where the thread class is used - including inside the
implementation of Thread::Stop().  As the comment explains, Stop() will not
return until all pending messages have been processed, including the one we just
posted.

Regarding an alternative method, I'm not sure if you mean 1) an alternative
method of waiting for ShutdownOnAudioThread to complete or if 2) we can somehow
differently uninitialize the dispatchers. So, I'll answer both.

1) There are other ways such as by using a WaitableEvent, but this is a simpler
way that doesn't require any additional synchronization objects to the thread
itself. Also, it's really just two lines of code (well, two semicolons), so it's
hard to beat with regards to simplicity. ;)

2) The dispatchers are single threaded objects that perform most of their work
on the audio thread (and correctly have a lot of DCHECKs to make sure it stays
that way).  As such, they really must be torn down from that same thread to keep
things robust.  Additionally, the map that holds onto the dispatchers does not
have any lock to guard against multithreaded access, so it too must only be
accessed from the audio thread, so the 

+  audio_thread_.Stop();
+}
+
+void AudioManagerBase::ShutdownOnAudioThread() {
+  DCHECK_EQ(MessageLoop::current(), GetMessageLoop());
+
+  AudioOutputDispatchersMap::iterator it = output_dispatchers_.begin();
+  for (; it != output_dispatchers_.end(); ++it) {
+    scoped_refptr<AudioOutputDispatcher>& dispatcher = (*it).second;
+    if (dispatcher) {
+      dispatcher->Shutdown();
+      // All AudioOutputProxies must have been freed before Shutdown is called.
+      // If they still exist, things will go bad.  They have direct pointers to
+      // both physical audio stream objects that belong to the dispatcher as
+      // well as the message loop of the audio thread that will soon go away.
+      // So, better crash now than later.
+      CHECK(dispatcher->HasOneRef()) << "AudioOutputProxies are still alive";
+      dispatcher = NULL;
+    }
+  }
+
+  output_dispatchers_.clear();
+}