Fix resampler flushing while OnMoreData() is in progress.

Locking when using |source_lock_| just to get |source_callback_|
isn't enough.  We actually want to gate all objects which work
on the source data; i.e. the entire OnMoreData() pathway.

Otherwise the resampler or FIFO might get flushed by another thread
in the middle of fulfilling a Resample() or Consume() call.

BUG=none
TEST=tsan unit tests, open and closing tabs quickly.


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

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

3 files changed, 149 insertions, 34 deletions

diff --git a/media/audio/audio_output_proxy_unittest.cc b/media/audio/audio_output_proxy_unittest.cc
index 37e3245..4403fa7 100644
--- a/media/audio/audio_output_proxy_unittest.cc
+++ b/media/audio/audio_output_proxy_unittest.cc
@@ -7,6 +7,7 @@
 #include "base/message_loop.h"
 #include "base/message_loop_proxy.h"
 #include "base/threading/platform_thread.h"
+#include "base/threading/simple_thread.h"
 #include "media/audio/audio_output_dispatcher_impl.h"
 #include "media/audio/audio_output_proxy.h"
 #include "media/audio/audio_output_resampler.h"
@@ -44,6 +45,12 @@ static const int kTestCloseDelayMs = 100;
 // Used in the test where we don't want a stream to be closed unexpectedly.
 static const int kTestBigCloseDelaySeconds = 1000;
 
+// Delay between callbacks to AudioSourceCallback::OnMoreData.
+static const int kOnMoreDataCallbackDelayMs = 10;
+
+// Let start run long enough for many OnMoreData callbacks to occur.
+static const int kStartRunTimeMs = kOnMoreDataCallbackDelayMs * 10;
+
 class MockAudioOutputStream : public AudioOutputStream {
  public:
   MockAudioOutputStream() {}
@@ -80,11 +87,53 @@ class MockAudioManager : public AudioManager {
 
 class MockAudioSourceCallback : public AudioOutputStream::AudioSourceCallback {
  public:
-  MOCK_METHOD2(OnMoreData, int(AudioBus* audio_bus,
-                               AudioBuffersState buffers_state));
+  int OnMoreData(AudioBus* audio_bus, AudioBuffersState buffers_state) {
+    audio_bus->Zero();
+    return audio_bus->frames();
+  }
   MOCK_METHOD2(OnError, void(AudioOutputStream* stream, int code));
 };
 
+// Simple class for repeatedly calling OnMoreData() to expose shutdown issues
+// with AudioSourceCallback users.
+class AudioThreadRunner : public base::DelegateSimpleThread::Delegate {
+ public:
+  AudioThreadRunner(AudioOutputStream::AudioSourceCallback* callback,
+                   base::TimeDelta delay, const AudioParameters& params)
+      : delay_(delay),
+        callback_(callback),
+        bus_(media::AudioBus::Create(params)),
+        running_(true) {
+    CHECK(delay_ > base::TimeDelta());
+  }
+
+  virtual void Run() {
+    while (true) {
+      {
+        base::AutoLock auto_lock(lock_);
+        if (!running_)
+          return;
+      }
+      base::PlatformThread::Sleep(delay_);
+      callback_->OnMoreData(bus_.get(), AudioBuffersState());
+    }
+  }
+
+  void Stop() {
+    base::AutoLock auto_lock(lock_);
+    running_ = false;
+  }
+
+ private:
+  base::TimeDelta delay_;
+  AudioOutputStream::AudioSourceCallback* callback_;
+  scoped_ptr<media::AudioBus> bus_;
+  bool running_;
+  base::Lock lock_;
+
+  DISALLOW_COPY_AND_ASSIGN(AudioThreadRunner);
+};
+
 }  // namespace
 
 namespace media {
@@ -106,26 +155,23 @@ class AudioOutputProxyTest : public testing::Test {
     message_loop_.RunAllPending();
   }
 
-  void InitDispatcher(base::TimeDelta close_delay) {
-    AudioParameters params(AudioParameters::AUDIO_PCM_LINEAR,
-                           CHANNEL_LAYOUT_STEREO, 44100, 16, 1024);
+  virtual void InitDispatcher(base::TimeDelta close_delay) {
+    params_ = AudioParameters(AudioParameters::AUDIO_PCM_LINEAR,
+                              CHANNEL_LAYOUT_STEREO, 44100, 16, 1024);
     dispatcher_impl_ = new AudioOutputDispatcherImpl(&manager(),
-                                                     params,
+                                                     params_,
                                                      close_delay);
 #if defined(ENABLE_AUDIO_MIXER)
-    mixer_ = new AudioOutputMixer(&manager(), params, close_delay);
+    mixer_ = new AudioOutputMixer(&manager(), params_, close_delay);
 #endif
 
-    AudioParameters out_params(AudioParameters::AUDIO_PCM_LINEAR,
-                               CHANNEL_LAYOUT_STEREO, 48000, 16, 128);
-    resampler_ = new AudioOutputResampler(
-        &manager(), params, out_params, close_delay);
-
     // Necessary to know how long the dispatcher will wait before posting
     // StopStreamTask.
     pause_delay_ = dispatcher_impl_->pause_delay_;
   }
 
+  virtual void OnStart() {}
+
   MockAudioManager& manager() {
     return manager_;
   }
@@ -176,6 +222,7 @@ class AudioOutputProxyTest : public testing::Test {
     EXPECT_TRUE(proxy->Open());
 
     proxy->Start(&callback_);
+    OnStart();
     proxy->Stop();
 
     proxy->Close();
@@ -203,6 +250,7 @@ class AudioOutputProxyTest : public testing::Test {
     EXPECT_TRUE(proxy->Open());
 
     proxy->Start(&callback_);
+    OnStart();
     proxy->Stop();
 
     // Wait for StopStream() to post StopStreamTask().
@@ -307,6 +355,7 @@ class AudioOutputProxyTest : public testing::Test {
 
     proxy1->Start(&callback_);
     message_loop_.RunAllPending();
+    OnStart();
     proxy1->Stop();
 
     proxy1->Close();
@@ -350,6 +399,7 @@ class AudioOutputProxyTest : public testing::Test {
 
     proxy1->Start(&callback_);
     proxy2->Start(&callback_);
+    OnStart();
     proxy1->Stop();
     proxy2->Stop();
 
@@ -397,10 +447,63 @@ class AudioOutputProxyTest : public testing::Test {
 #if defined(ENABLE_AUDIO_MIXER)
   scoped_refptr<AudioOutputMixer> mixer_;
 #endif
-  scoped_refptr<AudioOutputResampler> resampler_;
   base::TimeDelta pause_delay_;
   MockAudioManager manager_;
   MockAudioSourceCallback callback_;
+  AudioParameters params_;
+};
+
+class AudioOutputResamplerTest : public AudioOutputProxyTest {
+ public:
+  virtual void TearDown() {
+    AudioOutputProxyTest::TearDown();
+    ShutdownAudioThread();
+  }
+
+  void StartAudioThread() {
+    audio_thread_runner_.reset(new AudioThreadRunner(
+        resampler_,
+        base::TimeDelta::FromMilliseconds(kOnMoreDataCallbackDelayMs),
+        params_));
+    audio_thread_.reset(new base::DelegateSimpleThread(
+        audio_thread_runner_.get(), "AudioThreadRunner"));
+    audio_thread_->Start();
+  }
+
+  void ShutdownAudioThread() {
+    if (!audio_thread_.get()) {
+      ASSERT_FALSE(audio_thread_runner_.get());
+      return;
+    }
+    ASSERT_TRUE(audio_thread_runner_.get());
+    audio_thread_runner_->Stop();
+    audio_thread_->Join();
+  }
+
+  virtual void InitDispatcher(base::TimeDelta close_delay) {
+    AudioOutputProxyTest::InitDispatcher(close_delay);
+    // Attempt shutdown of audio thread in case InitDispatcher() was called
+    // previously.
+    ShutdownAudioThread();
+    resampler_params_ = AudioParameters(AudioParameters::AUDIO_PCM_LINEAR,
+                                        CHANNEL_LAYOUT_STEREO, 48000, 16, 128);
+    resampler_ = new AudioOutputResampler(
+        &manager(), params_, resampler_params_, close_delay);
+    StartAudioThread();
+  }
+
+  virtual void OnStart() {
+    // Let start run for a bit.
+    message_loop_.RunAllPending();
+    base::PlatformThread::Sleep(
+        base::TimeDelta::FromMilliseconds(kStartRunTimeMs));
+  }
+
+ protected:
+  AudioParameters resampler_params_;
+  scoped_refptr<AudioOutputResampler> resampler_;
+  scoped_ptr<AudioThreadRunner> audio_thread_runner_;
+  scoped_ptr<base::DelegateSimpleThread> audio_thread_;
 };
 
 TEST_F(AudioOutputProxyTest, CreateAndClose) {
@@ -415,7 +518,7 @@ TEST_F(AudioOutputProxyTest, CreateAndClose_Mixer) {
 }
 #endif
 
-TEST_F(AudioOutputProxyTest, CreateAndClose_Resampler) {
+TEST_F(AudioOutputResamplerTest, CreateAndClose) {
   AudioOutputProxy* proxy = new AudioOutputProxy(resampler_);
   proxy->Close();
 }
@@ -430,7 +533,7 @@ TEST_F(AudioOutputProxyTest, OpenAndClose_Mixer) {
 }
 #endif
 
-TEST_F(AudioOutputProxyTest, OpenAndClose_Resampler) {
+TEST_F(AudioOutputResamplerTest, OpenAndClose) {
   OpenAndClose(resampler_);
 }
 
@@ -442,7 +545,7 @@ TEST_F(AudioOutputProxyTest, CreateAndWait) {
 
 // Create a stream, and verify that it is closed after kTestCloseDelayMs.
 // if it doesn't start playing.
-TEST_F(AudioOutputProxyTest, CreateAndWait_Resampler) {
+TEST_F(AudioOutputResamplerTest, CreateAndWait) {
   CreateAndWait(resampler_);
 }
 
@@ -456,7 +559,7 @@ TEST_F(AudioOutputProxyTest, StartAndStop_Mixer) {
 }
 #endif
 
-TEST_F(AudioOutputProxyTest, StartAndStop_Resampler) {
+TEST_F(AudioOutputResamplerTest, StartAndStop) {
   StartAndStop(resampler_);
 }
 
@@ -470,7 +573,7 @@ TEST_F(AudioOutputProxyTest, CloseAfterStop_Mixer) {
 }
 #endif
 
-TEST_F(AudioOutputProxyTest, CloseAfterStop_Resampler) {
+TEST_F(AudioOutputResamplerTest, CloseAfterStop) {
   CloseAfterStop(resampler_);
 }
 
@@ -484,7 +587,7 @@ TEST_F(AudioOutputProxyTest, TwoStreams_Mixer) {
 }
 #endif
 
-TEST_F(AudioOutputProxyTest, TwoStreams_Resampler) {
+TEST_F(AudioOutputResamplerTest, TwoStreams) {
   TwoStreams(resampler_);
 }
 
@@ -530,7 +633,7 @@ TEST_F(AudioOutputProxyTest, TwoStreams_OnePlaying_Mixer) {
 }
 #endif
 
-TEST_F(AudioOutputProxyTest, TwoStreams_OnePlaying_Resampler) {
+TEST_F(AudioOutputResamplerTest, TwoStreams_OnePlaying) {
   InitDispatcher(base::TimeDelta::FromSeconds(kTestBigCloseDelaySeconds));
   TwoStreams_OnePlaying(resampler_);
 }
@@ -606,7 +709,7 @@ TEST_F(AudioOutputProxyTest, TwoStreams_BothPlaying_Mixer) {
 }
 #endif
 
-TEST_F(AudioOutputProxyTest, TwoStreams_BothPlaying_Resampler) {
+TEST_F(AudioOutputResamplerTest, TwoStreams_BothPlaying) {
   InitDispatcher(base::TimeDelta::FromSeconds(kTestBigCloseDelaySeconds));
   TwoStreams_BothPlaying(resampler_);
 }
@@ -621,7 +724,7 @@ TEST_F(AudioOutputProxyTest, OpenFailed_Mixer) {
 }
 #endif
 
-TEST_F(AudioOutputProxyTest, OpenFailed_Resampler) {
+TEST_F(AudioOutputResamplerTest, OpenFailed) {
   OpenFailed(resampler_);
 }
 
@@ -676,7 +779,7 @@ TEST_F(AudioOutputProxyTest, StartFailed_Mixer) {
 }
 #endif
 
-TEST_F(AudioOutputProxyTest, StartFailed_Resampler) {
+TEST_F(AudioOutputResamplerTest, StartFailed) {
   StartFailed(resampler_);
 }
 
diff --git a/media/audio/audio_output_resampler.cc b/media/audio/audio_output_resampler.cc
index 46296f8..e67e277 100644
--- a/media/audio/audio_output_resampler.cc
+++ b/media/audio/audio_output_resampler.cc
@@ -38,6 +38,8 @@ AudioOutputResampler::AudioOutputResampler(AudioManager* audio_manager,
       input_params.frames_per_buffer() != output_params.frames_per_buffer()) {
     // Only resample if necessary since it's expensive.
     if (input_params.sample_rate() != output_params.sample_rate()) {
+      DVLOG(1) << "Resampling from " << input_params.sample_rate() << " to "
+               << output_params.sample_rate();
       double io_sample_rate_ratio = input_params.sample_rate() /
           static_cast<double>(output_params.sample_rate());
       // Include the I/O resampling ratio in our global I/O ratio.
@@ -60,10 +62,15 @@ AudioOutputResampler::AudioOutputResampler(AudioManager* audio_manager,
     // read in chunk sizes they're configured for.
     if (input_params.sample_rate() != output_params.sample_rate() ||
         input_params.frames_per_buffer() != output_params.frames_per_buffer()) {
+      DVLOG(1) << "Rebuffering from " << input_params.frames_per_buffer()
+               << " to " << output_params.frames_per_buffer();
       audio_fifo_.reset(new AudioPullFifo(
           input_params.channels(), input_params.frames_per_buffer(), base::Bind(
-              &AudioOutputResampler::SourceCallback, base::Unretained(this))));
+              &AudioOutputResampler::SourceCallback_Locked,
+              base::Unretained(this))));
     }
+
+    DVLOG(1) << "I/O ratio is " << io_ratio_;
   }
 
   // TODO(dalecurtis): All this code should be merged into AudioOutputMixer once
@@ -122,12 +129,19 @@ void AudioOutputResampler::Shutdown() {
 
 int AudioOutputResampler::OnMoreData(AudioBus* audio_bus,
                                      AudioBuffersState buffers_state) {
+  base::AutoLock auto_lock(source_lock_);
+  // While we waited for |source_lock_| the callback might have been cleared.
+  if (!source_callback_) {
+    audio_bus->Zero();
+    return audio_bus->frames();
+  }
+
   current_buffers_state_ = buffers_state;
 
   if (!resampler_.get() && !audio_fifo_.get()) {
     // We have no internal buffers, so clear any outstanding audio data.
     outstanding_audio_bytes_ = 0;
-    SourceCallback(audio_bus);
+    SourceCallback_Locked(audio_bus);
     return audio_bus->frames();
   }
 
@@ -151,13 +165,8 @@ int AudioOutputResampler::OnMoreData(AudioBus* audio_bus,
   return audio_bus->frames();
 }
 
-void AudioOutputResampler::SourceCallback(AudioBus* audio_bus) {
-  base::AutoLock auto_lock(source_lock_);
-  // While we waited for |source_lock_| it might have been cleared.
-  if (!source_callback_) {
-    audio_bus->Zero();
-    return;
-  }
+void AudioOutputResampler::SourceCallback_Locked(AudioBus* audio_bus) {
+  source_lock_.AssertAcquired();
 
   // Adjust playback delay to include the state of the internal buffers used by
   // the resampler and/or the FIFO.  Since the sample rate and bits per channel
diff --git a/media/audio/audio_output_resampler.h b/media/audio/audio_output_resampler.h
index 6761499..eccab12 100644
--- a/media/audio/audio_output_resampler.h
+++ b/media/audio/audio_output_resampler.h
@@ -66,8 +66,9 @@ class MEDIA_EXPORT AudioOutputResampler
   // Called by MultiChannelResampler when more data is necessary.
   void ProvideInput(AudioBus* audio_bus);
 
-  // Called by AudioPullFifo when more data is necessary.
-  void SourceCallback(AudioBus* audio_bus);
+  // Called by AudioPullFifo when more data is necessary.  Requires
+  // |source_lock_| to have been acquired.
+  void SourceCallback_Locked(AudioBus* audio_bus);
 
   // Used by StopStream()/CloseStream()/Shutdown() to clear internal state.
   // TODO(dalecurtis): Probably only one of these methods needs to call this,
@@ -86,6 +87,8 @@ class MEDIA_EXPORT AudioOutputResampler
 
   // Used to buffer data between the client and the output device in cases where
   // the client buffer size is not the same as the output device buffer size.
+  // Bound to SourceCallback_Locked() so must only be used when |source_lock_|
+  // has already been acquired.
   scoped_ptr<AudioPullFifo> audio_fifo_;
 
   // Ratio of input bytes to output bytes used to correct playback delay with