stopping the audio thread before destroying the AudioManager<Platform>

media/audio/audio_manager_base.cc

Index: media/audio/audio_manager_base.cc
diff --git a/media/audio/audio_manager_base.cc b/media/audio/audio_manager_base.cc
index 05ed044caef708b73ba09169f5e7c8ee8e44f8bd..16265ce25681831703764b71c26875920ee23b4a 100644
--- a/media/audio/audio_manager_base.cc
+++ b/media/audio/audio_manager_base.cc
@@ -18,6 +18,10 @@ static const int kStreamCloseDelaySeconds = 5;
 // for all platforms.
 static const int kDefaultMaxOutputStreams = 15;

[tommi (sloooow) - chröme, 2012/03/13 11:06:48]

16?

[no longer working on chromium, 2012/03/13 14:08:59]

Done.

 
+// Default maximum number of input streams that can be open simultaneously
+// for all platforms.
+static const int kDefaultMaxInputStreams = 10;

[tommi (sloooow) - chröme, 2012/03/13 11:06:48]

16?

[no longer working on chromium, 2012/03/13 14:08:59]

Done.

+
 static const int kMaxInputChannels = 2;
 
 const char AudioManagerBase::kDefaultDeviceName[] = "Default";
@@ -26,13 +30,22 @@ const char AudioManagerBase::kDefaultDeviceId[] = "default";
 AudioManagerBase::AudioManagerBase()
     : num_active_input_streams_(0),
       max_num_output_streams_(kDefaultMaxOutputStreams),
-      num_output_streams_(0) {
+      max_num_input_streams_(kDefaultMaxInputStreams),
+      num_output_streams_(0),
+      num_input_streams_(0) {
 }
 
 AudioManagerBase::~AudioManagerBase() {
-  Shutdown();
+  // The platform specific AudioManager implementation must have already
+  // stopped the audio thread. Otherwise, we may destroy audio streams before
+  // stopping the thread, resulting an unexpected behavior.
+  // This way we make sure activities of the audio streams are all stopped
+  // before we destroy them.
+  CHECK(!audio_thread_.get());
   // All the output streams should have been deleted.
   DCHECK_EQ(0, num_output_streams_);
+  // All the input streams should have been deleted.
+  DCHECK_EQ(0, num_input_streams_);
 }
 
 void AudioManagerBase::Init() {
@@ -63,8 +76,10 @@ AudioOutputStream* AudioManagerBase::MakeAudioOutputStream(
   // importantly it prevents instability on certain systems.
   // See bug: http://crbug.com/30242.
   if (num_output_streams_ >= max_num_output_streams_) {
-    DLOG(ERROR) << "Number of opened audio streams " << num_output_streams_
-                << " exceed the max allowed number " << max_num_output_streams_;
+    DLOG(ERROR) << "Number of opened output audio streams "
+                << num_output_streams_
+                << " exceed the max allowed number "
+                << max_num_output_streams_;
     return NULL;
   }
 
@@ -72,10 +87,10 @@ AudioOutputStream* AudioManagerBase::MakeAudioOutputStream(
   if (params.format == AudioParameters::AUDIO_MOCK) {
     stream = FakeAudioOutputStream::MakeFakeStream(params);
   } else if (params.format == AudioParameters::AUDIO_PCM_LINEAR) {
-    num_output_streams_++;
+    ++num_output_streams_;

[tommi (sloooow) - chröme, 2012/03/13 10:50:28]

I think I made a comment on this in an earlier review but apparently that
comment got ignored.
Instead of incrementing here, I'd prefer to have an if() after this if block:

if (params.format == ...) {
} else if (params.format == ...) {
} else if (params.format == ...) {
}

if (stream)
  ++num_output_streams_;

That way we only have the increment in one place and we also do not increment if
the platform class returned NULL.

Incidentally, why should we not increment for AUDIO_MOCK?

[no longer working on chromium, 2012/03/13 14:08:59]

Done.

     stream = MakeLinearOutputStream(params);
   } else if (params.format == AudioParameters::AUDIO_PCM_LOW_LATENCY) {
-    num_output_streams_++;
+    ++num_output_streams_;
     stream = MakeLowLatencyOutputStream(params);
   }
 
@@ -90,12 +105,21 @@ AudioInputStream* AudioManagerBase::MakeAudioInputStream(
     return NULL;
   }
 
+  if (num_input_streams_ >= max_num_input_streams_) {

[tommi (sloooow) - chröme, 2012/03/13 10:50:28]

this change seems unrelated to the bug. Do on a separate cl?

[tommi (sloooow) - chröme, 2012/03/13 11:06:48]

as discussed offline, this is OK with me.

[no longer working on chromium, 2012/03/13 14:08:59]

Done.

+    DLOG(ERROR) << "Number of opened input audio streams "
+                << num_input_streams_
+                << " exceed the max allowed number " << max_num_input_streams_;
+    return NULL;
+  }
+
   AudioInputStream* stream = NULL;
   if (params.format == AudioParameters::AUDIO_MOCK) {
     stream = FakeAudioInputStream::MakeFakeStream(params);
   } else if (params.format == AudioParameters::AUDIO_PCM_LINEAR) {
+    ++num_input_streams_;

[tommi (sloooow) - chröme, 2012/03/13 10:50:28]

same comment as for the output stream.

[no longer working on chromium, 2012/03/13 14:08:59]

Done.

     stream = MakeLinearInputStream(params, device_id);
   } else if (params.format == AudioParameters::AUDIO_PCM_LOW_LATENCY) {
+    ++num_input_streams_;
     stream = MakeLowLatencyInputStream(params, device_id);
   }
 
@@ -137,6 +161,7 @@ void AudioManagerBase::ReleaseOutputStream(AudioOutputStream* stream) {
 void AudioManagerBase::ReleaseInputStream(AudioInputStream* stream) {
   DCHECK(stream);
   // TODO(xians) : Have a clearer destruction path for the AudioInputStream.
+  num_input_streams_--;
   delete stream;
 }