Revert 141770 - 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.

That CL finishes work on browser-side audio mixer. Not sure how important it is,
though -- hopefully it will provide some time while implementing renderer-side mixer.

Also moved code for crash Mac OS X into audio mixer, because problem can manifest
itself if Close() called immediately after Stop().

That CL also fixes bug 131720. Looks that it was caused by timing change, and starting
stream earlier causes less dropped frames. (I still cannot understand why on modern
system we should have even single dropped frame, and why slight timing change
caused us to drop frame, but that is different question...)

BUG=129190
BUG=131720
BUG=131720
BUG=102395

TEST=Should not be noticeable difference in behavior. Run tests on Win7 and XP myself.
TEST=Mac crashes on seeks should go away, too.

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

TBR=enal@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10532116

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

8 files changed, 87 insertions, 83 deletions

diff --git a/media/audio/audio_output_controller.cc b/media/audio/audio_output_controller.cc
index 7834ed7..70f7a1c 100644
--- a/media/audio/audio_output_controller.cc
+++ b/media/audio/audio_output_controller.cc
@@ -16,6 +16,10 @@ using base::Time;
 using base::TimeDelta;
 using base::WaitableEvent;
 
+// Workaround for crbug.com/128128.
+// Minimal delay between stream->Stop() and stream->Start().
+const int kMacWorkaroundInMilliseconds = 50;
+
 namespace media {
 
 // Signal a pause in low-latency mode.
@@ -34,7 +38,10 @@ AudioOutputController::AudioOutputController(EventHandler* handler,
       sync_reader_(sync_reader),
       message_loop_(NULL),
       number_polling_attempts_left_(0),
-      ALLOW_THIS_IN_INITIALIZER_LIST(weak_this_(this)) {
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_this_(this)),
+      previous_stop_time_(
+          Time::Now() -
+          TimeDelta::FromMilliseconds(kMacWorkaroundInMilliseconds + 1)) {
 }
 
 AudioOutputController::~AudioOutputController() {
@@ -203,6 +210,23 @@ void AudioOutputController::PollAndStartIfDataReady() {
 void AudioOutputController::StartStream() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
+#if defined(OS_MACOSX)
+  // HACK: workaround for crbug.com/128128.
+  // Mac OS crashes if we start playback too soon after previous ended.
+  // Audio mixer contains better fix, it keeps physical stream opened for
+  // some time after logical one is closed, so sequence of play / pause / play /
+  // pause / ... would reuse the same stream, but we need fix for M20.
+  // TODO(enal): Remove after turning on mixer by default.
+  int milliseconds_since_stop =
+      (Time::Now() - previous_stop_time_).InMilliseconds();
+  if ((milliseconds_since_stop >= 0) &&
+      (milliseconds_since_stop < kMacWorkaroundInMilliseconds)) {
+    base::PlatformThread::Sleep(
+        TimeDelta::FromMilliseconds(kMacWorkaroundInMilliseconds -
+                                    milliseconds_since_stop));
+  }
+#endif
+
   state_ = kPlaying;
 
   // We start the AudioOutputStream lazily.
@@ -220,6 +244,7 @@ void AudioOutputController::DoPause() {
     // because it discards all the internal buffer in the audio device.
     // TODO(hclam): Actually pause the audio device.
     stream_->Stop();
+    previous_stop_time_ = Time::Now();
   }
 
   switch (state_) {
diff --git a/media/audio/audio_output_controller.h b/media/audio/audio_output_controller.h
index 33bf217..2077c9b 100644
--- a/media/audio/audio_output_controller.h
+++ b/media/audio/audio_output_controller.h
@@ -225,6 +225,9 @@ class MEDIA_EXPORT AudioOutputController
   // Also, if we're shutting down, we do not want to poll for more data.
   base::WeakPtrFactory<AudioOutputController> weak_this_;
 
+  // Workaround for Mac OS X bug, see crbug.com/128128.
+  base::Time previous_stop_time_;
+
   DISALLOW_COPY_AND_ASSIGN(AudioOutputController);
 };
 
diff --git a/media/audio/audio_output_controller_unittest.cc b/media/audio/audio_output_controller_unittest.cc
index 6fe2499..f40a9ae 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(1));
+      .Times(AtLeast(2));
   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 5a5438c..edce4ea 100644
--- a/media/audio/audio_output_mixer.cc
+++ b/media/audio/audio_output_mixer.cc
@@ -9,7 +9,6 @@
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/message_loop.h"
-#include "base/threading/platform_thread.h"
 #include "base/time.h"
 #include "media/audio/audio_io.h"
 #include "media/audio/audio_output_proxy.h"
@@ -17,10 +16,6 @@
 
 namespace media {
 
-// Workaround for crbug.com/128128.
-// Minimal delay between stream->Stop() and stream->Close().
-const int kMacWorkaroundInMilliseconds = 50;
-
 AudioOutputMixer::AudioOutputMixer(AudioManager* audio_manager,
                                    const AudioParameters& params,
                                    const base::TimeDelta& close_delay)
@@ -52,8 +47,6 @@ 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;
 }
@@ -71,24 +64,46 @@ bool AudioOutputMixer::StartStream(
 
   double volume = 0.0;
   stream_proxy->GetVolume(&volume);
-
-  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;
+  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);
+  }
   return true;
 }
 
 void AudioOutputMixer::StopStream(AudioOutputProxy* stream_proxy) {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
 
-  base::AutoLock lock(lock_);
-  ProxyMap::iterator it = proxies_.find(stream_proxy);
-  if (it != proxies_.end())
-    proxies_.erase(it);
-  if (physical_stream_.get())
+  // 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.
+    }
     close_timer_.Reset();
+  }
 }
 
 void AudioOutputMixer::StreamVolumeSet(AudioOutputProxy* stream_proxy,
@@ -129,17 +144,8 @@ void AudioOutputMixer::Shutdown() {
 void AudioOutputMixer::ClosePhysicalStream() {
   DCHECK_EQ(MessageLoop::current(), message_loop_);
 
-  if (proxies_.empty() && physical_stream_.get() != NULL) {
-    physical_stream_->Stop();
-#if defined(OS_MACOSX)
-  // HACK: workaround for crbug.com/128128.
-  // Mac OS stops the stream asynchronously, even if we asked stop to be
-  // synchronous.
-  base::PlatformThread::Sleep(
-      base::TimeDelta::FromMilliseconds(kMacWorkaroundInMilliseconds));
-#endif
+  if (proxies_.empty() && physical_stream_.get() != NULL)
     physical_stream_.release()->Close();
-  }
 }
 
 // AudioSourceCallback implementation.
diff --git a/media/audio/audio_util.cc b/media/audio/audio_util.cc
index 3768cd6..524b1e8 100644
--- a/media/audio/audio_util.cc
+++ b/media/audio/audio_util.cc
@@ -21,7 +21,6 @@
 #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
@@ -520,20 +519,6 @@ 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 4ac0ef6..df5683f 100644
--- a/media/audio/audio_util.h
+++ b/media/audio/audio_util.h
@@ -132,9 +132,6 @@ 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 38c4615..93dcf2f 100644
--- a/media/audio/win/audio_manager_win.cc
+++ b/media/audio/win/audio_manager_win.cc
@@ -244,10 +244,7 @@ AudioOutputStream* AudioManagerWin::MakeLinearOutputStream(
   if (params.channels() > kWinMaxChannels)
     return NULL;
 
-  return new PCMWaveOutAudioOutputStream(this,
-                                         params,
-                                         media::NumberOfWaveOutBuffers(),
-                                         WAVE_MAPPER);
+  return new PCMWaveOutAudioOutputStream(this, params, 3, 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 d954093..4066643 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 kMaxNumBuffers = 3;
+const int kNumBuffers = 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() % NumberOfWaveOutBuffers() == 2) {
+    if (callback_count() % kNumBuffers == 2) {
       set_error(!CompareExistingIfNotNULL(2, dest));
-    } else if (callback_count() % NumberOfWaveOutBuffers() == 1) {
+    } else if (callback_count() % kNumBuffers == 1) {
       set_error(!CompareExistingIfNotNULL(1, dest));
     } else {
       set_error(!CompareExistingIfNotNULL(0, dest));
     }
-    if (callback_count() > kMaxNumBuffers) {
+    if (callback_count() > kNumBuffers) {
       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_[kMaxNumBuffers];
+  void* buffer_address_[kNumBuffers];
 };
 
 // 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() > kMaxNumBuffers) {
+    if (callback_count() > kNumBuffers) {
       ::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(), kMaxNumBuffers);
+  EXPECT_GT(test_triple_buffer.callback_count(), kNumBuffers);
   EXPECT_FALSE(test_triple_buffer.had_error());
   oas->Stop();
   ::Sleep(500);
@@ -600,37 +600,28 @@ TEST(WinAudioTest, PCMWaveStreamPendingBytes) {
 
   uint32 bytes_100_ms = samples_100_ms * 2;
 
-  // 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.
+  // 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.
   // 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));
-  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)))
+      .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));
   EXPECT_CALL(source, OnMoreData(NotNull(), bytes_100_ms,
                                  Field(&AudioBuffersState::pending_bytes,
                                        bytes_100_ms)))