Do not stop audio physical stream immediately after logical one had stopped.

Wait some time.
We are still stopping/closing the stream, as (1) it is better for battery life,
and (2) some people can hear active device even when it is playing silence.

That increased audio startup latency, especially on Windows, because we are using 3
buffers on Windows. To fix that I changed the code to use 2 buffers on presumable
good Windows boxes -- i.e. running non-Vista and having more than single core.

Changed unit tests as well.

BUG=129190
TEST=Should not be noticeable difference in behavior. Run tests on Win7 and XP myself.

Review URL: https://chromiumcodereview.appspot.com/10540034

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@141476 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 70 insertions, 58 deletions

diff --git a/media/audio/audio_output_controller_unittest.cc b/media/audio/audio_output_controller_unittest.cc
index f40a9ae..6fe2499 100644
--- a/media/audio/audio_output_controller_unittest.cc
+++ b/media/audio/audio_output_controller_unittest.cc
@@ -196,7 +196,7 @@ TEST_F(AudioOutputControllerTest, PlayPausePlayClose) {
 
   MockAudioOutputControllerSyncReader sync_reader;
   EXPECT_CALL(sync_reader, UpdatePendingBytes(_))
-      .Times(AtLeast(2));
+      .Times(AtLeast(1));
   EXPECT_CALL(sync_reader, Read(_, kHardwareBufferSize))
       .WillRepeatedly(DoAll(SignalEvent(&event), Return(4)));
   EXPECT_CALL(sync_reader, DataReady())
diff --git a/media/audio/audio_output_mixer.cc b/media/audio/audio_output_mixer.cc
index edce4ea..542db78 100644
--- a/media/audio/audio_output_mixer.cc
+++ b/media/audio/audio_output_mixer.cc
@@ -47,6 +47,8 @@ bool AudioOutputMixer::OpenStream() {
   }
   pending_bytes_ = 0;  // Just in case.
   physical_stream_.reset(stream);
+  physical_stream_->SetVolume(1.0);
+  physical_stream_->Start(this);
   close_timer_.Reset();
   return true;
 }
@@ -64,46 +66,24 @@ bool AudioOutputMixer::StartStream(
 
   double volume = 0.0;
   stream_proxy->GetVolume(&volume);
-  bool should_start = proxies_.empty();
-  {
-    base::AutoLock lock(lock_);
-    ProxyData* proxy_data = &proxies_[stream_proxy];
-    proxy_data->audio_source_callback = callback;
-    proxy_data->volume = volume;
-    proxy_data->pending_bytes = 0;
-  }
-  // We cannot start physical stream under the lock,
-  // OnMoreData() would try acquiring it...
-  if (should_start) {
-    physical_stream_->SetVolume(1.0);
-    physical_stream_->Start(this);
-  }
+
+  base::AutoLock lock(lock_);
+  ProxyData* proxy_data = &proxies_[stream_proxy];
+  proxy_data->audio_source_callback = callback;
+  proxy_data->volume = volume;
+  proxy_data->pending_bytes = 0;
   return true;
 }
 
 void AudioOutputMixer::StopStream(AudioOutputProxy* stream_proxy) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
 
-  // Because of possible deadlock we cannot stop physical stream under the lock
-  // (physical_stream_->Stop() can call OnError(), and it acquires the lock to
-  // iterate through proxies), so acquire the lock, update proxy list, release
-  // the lock, and only then stop physical stream if necessary.
-  bool stop_physical_stream = false;
-  {
-    base::AutoLock lock(lock_);
-    ProxyMap::iterator it = proxies_.find(stream_proxy);
-    if (it != proxies_.end()) {
-      proxies_.erase(it);
-      stop_physical_stream = proxies_.empty();
-    }
-  }
-  if (physical_stream_.get()) {
-    if (stop_physical_stream) {
-      physical_stream_->Stop();
-      pending_bytes_ = 0;  // Just in case.
-    }
+  base::AutoLock lock(lock_);
+  ProxyMap::iterator it = proxies_.find(stream_proxy);
+  if (it != proxies_.end())
+    proxies_.erase(it);
+  if (physical_stream_.get())
     close_timer_.Reset();
-  }
 }
 
 void AudioOutputMixer::StreamVolumeSet(AudioOutputProxy* stream_proxy,
@@ -144,8 +124,10 @@ void AudioOutputMixer::Shutdown() {
 void AudioOutputMixer::ClosePhysicalStream() {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
 
-  if (proxies_.empty() && physical_stream_.get() != NULL)
+  if (proxies_.empty() && physical_stream_.get() != NULL) {
+    physical_stream_->Stop();
     physical_stream_.release()->Close();
+  }
 }
 
 // AudioSourceCallback implementation.
diff --git a/media/audio/audio_util.cc b/media/audio/audio_util.cc
index 524b1e8..3768cd6 100644
--- a/media/audio/audio_util.cc
+++ b/media/audio/audio_util.cc
@@ -21,6 +21,7 @@
 #include "base/shared_memory.h"
 #include "base/time.h"
 #if defined(OS_WIN)
+#include "base/sys_info.h"
 #include "base/win/windows_version.h"
 #include "media/audio/audio_manager_base.h"
 #endif
@@ -519,6 +520,20 @@ bool IsWASAPISupported() {
   return base::win::GetVersion() >= base::win::VERSION_VISTA;
 }
 
+int NumberOfWaveOutBuffers() {
+  // Simple heuristic: use 3 buffers on single-core system or on Vista,
+  // 2 otherwise.
+  // Entire Windows audio stack was rewritten for Windows Vista, and wave out
+  // API is simulated on top of new API, so there is noticeable performance
+  // degradation compared to Windows XP. Part of regression was fixed in
+  // Windows 7. Maybe it is fixed in Vista Serice Pack, but let's be cautious.
+  if ((base::SysInfo::NumberOfProcessors() < 2) ||
+      (base::win::GetVersion() == base::win::VERSION_VISTA)) {
+    return 3;
+  }
+  return 2;
+}
+
 #endif
 
 }  // namespace media
diff --git a/media/audio/audio_util.h b/media/audio/audio_util.h
index df5683f..4ac0ef6 100644
--- a/media/audio/audio_util.h
+++ b/media/audio/audio_util.h
@@ -132,6 +132,9 @@ MEDIA_EXPORT bool IsUnknownDataSize(base::SharedMemory* shared_memory,
 // sometimes check was written incorrectly, so move into separate function.
 MEDIA_EXPORT bool IsWASAPISupported();
 
+// Returns number of buffers to be used by wave out.
+MEDIA_EXPORT int NumberOfWaveOutBuffers();
+
 #endif  // defined(OS_WIN)
 
 }  // namespace media
diff --git a/media/audio/win/audio_manager_win.cc b/media/audio/win/audio_manager_win.cc
index 93dcf2f..38c4615 100644
--- a/media/audio/win/audio_manager_win.cc
+++ b/media/audio/win/audio_manager_win.cc
@@ -244,7 +244,10 @@ AudioOutputStream* AudioManagerWin::MakeLinearOutputStream(
   if (params.channels() > kWinMaxChannels)
     return NULL;
 
-  return new PCMWaveOutAudioOutputStream(this, params, 3, WAVE_MAPPER);
+  return new PCMWaveOutAudioOutputStream(this,
+                                         params,
+                                         media::NumberOfWaveOutBuffers(),
+                                         WAVE_MAPPER);
 }
 
 // Factory for the implementations of AudioOutputStream for
diff --git a/media/audio/win/audio_output_win_unittest.cc b/media/audio/win/audio_output_win_unittest.cc
index 4066643..d954093 100644
--- a/media/audio/win/audio_output_win_unittest.cc
+++ b/media/audio/win/audio_output_win_unittest.cc
@@ -76,7 +76,7 @@ class TestSourceBasic : public AudioOutputStream::AudioSourceCallback {
   int had_error_;
 };
 
-const int kNumBuffers = 3;
+const int kMaxNumBuffers = 3;
 // Specializes TestSourceBasic to detect that the AudioStream is using
 // triple buffering correctly.
 class TestSourceTripleBuffer : public TestSourceBasic {
@@ -92,14 +92,14 @@ class TestSourceTripleBuffer : public TestSourceBasic {
                             AudioBuffersState buffers_state) {
     // Call the base, which increments the callback_count_.
     TestSourceBasic::OnMoreData(dest, max_size, buffers_state);
-    if (callback_count() % kNumBuffers == 2) {
+    if (callback_count() % NumberOfWaveOutBuffers() == 2) {
       set_error(!CompareExistingIfNotNULL(2, dest));
-    } else if (callback_count() % kNumBuffers == 1) {
+    } else if (callback_count() % NumberOfWaveOutBuffers() == 1) {
       set_error(!CompareExistingIfNotNULL(1, dest));
     } else {
       set_error(!CompareExistingIfNotNULL(0, dest));
     }
-    if (callback_count() > kNumBuffers) {
+    if (callback_count() > kMaxNumBuffers) {
       set_error(buffer_address_[0] == buffer_address_[1]);
       set_error(buffer_address_[1] == buffer_address_[2]);
     }
@@ -114,7 +114,7 @@ class TestSourceTripleBuffer : public TestSourceBasic {
     return (entry == address);
   }
 
-  void* buffer_address_[kNumBuffers];
+  void* buffer_address_[kMaxNumBuffers];
 };
 
 // Specializes TestSourceBasic to simulate a source that blocks for some time
@@ -129,7 +129,7 @@ class TestSourceLaggy : public TestSourceBasic {
                             AudioBuffersState buffers_state) {
     // Call the base, which increments the callback_count_.
     TestSourceBasic::OnMoreData(dest, max_size, buffers_state);
-    if (callback_count() > kNumBuffers) {
+    if (callback_count() > kMaxNumBuffers) {
       ::Sleep(lag_in_ms_);
     }
     return max_size;
@@ -312,7 +312,7 @@ TEST(WinAudioTest, PCMWaveStreamTripleBuffer) {
   EXPECT_TRUE(oas->Open());
   oas->Start(&test_triple_buffer);
   ::Sleep(300);
-  EXPECT_GT(test_triple_buffer.callback_count(), kNumBuffers);
+  EXPECT_GT(test_triple_buffer.callback_count(), kMaxNumBuffers);
   EXPECT_FALSE(test_triple_buffer.had_error());
   oas->Stop();
   ::Sleep(500);
@@ -600,28 +600,37 @@ TEST(WinAudioTest, PCMWaveStreamPendingBytes) {
 
   uint32 bytes_100_ms = samples_100_ms * 2;
 
-  // We expect the amount of pending bytes will reaching 2 times of
-  // |bytes_100_ms| because the audio output stream has a triple buffer scheme.
+  // Audio output stream has either a double or triple buffer scheme.
+  // We expect the amount of pending bytes will reaching up to 2 times of
+  // |bytes_100_ms| depending on number of buffers used.
   // From that it would decrease as we are playing the data but not providing
   // new one. And then we will try to provide zero data so the amount of
   // pending bytes will go down and eventually read zero.
   InSequence s;
+
   EXPECT_CALL(source, OnMoreData(NotNull(), bytes_100_ms,
                                  Field(&AudioBuffersState::pending_bytes, 0)))
       .WillOnce(Return(bytes_100_ms));
-  EXPECT_CALL(source, OnMoreData(NotNull(), bytes_100_ms,
-                                 Field(&AudioBuffersState::pending_bytes,
-                                       bytes_100_ms)))
-      .WillOnce(Return(bytes_100_ms));
-  EXPECT_CALL(source, OnMoreData(NotNull(), bytes_100_ms,
-                                 Field(&AudioBuffersState::pending_bytes,
-                                       2 * bytes_100_ms)))
-      .WillOnce(Return(bytes_100_ms));
-  EXPECT_CALL(source, OnMoreData(NotNull(), bytes_100_ms,
-                                 Field(&AudioBuffersState::pending_bytes,
-                                       2 * bytes_100_ms)))
-      .Times(AnyNumber())
-      .WillRepeatedly(Return(0));
+  switch (NumberOfWaveOutBuffers()) {
+    case 2:
+      break;  // Calls are the same as at end of 3-buffer scheme.
+    case 3:
+      EXPECT_CALL(source, OnMoreData(NotNull(), bytes_100_ms,
+                                     Field(&AudioBuffersState::pending_bytes,
+                                           bytes_100_ms)))
+          .WillOnce(Return(bytes_100_ms));
+      EXPECT_CALL(source, OnMoreData(NotNull(), bytes_100_ms,
+                                     Field(&AudioBuffersState::pending_bytes,
+                                           2 * bytes_100_ms)))
+          .WillOnce(Return(bytes_100_ms));
+      EXPECT_CALL(source, OnMoreData(NotNull(), bytes_100_ms,
+                                     Field(&AudioBuffersState::pending_bytes,
+                                           2 * bytes_100_ms)))
+          .Times(AnyNumber())
+          .WillRepeatedly(Return(0));
+    default:
+      ASSERT_TRUE(false) << "Unexpected number of buffers";
+  }
   EXPECT_CALL(source, OnMoreData(NotNull(), bytes_100_ms,
                                  Field(&AudioBuffersState::pending_bytes,
                                        bytes_100_ms)))