Reland: Add Stop() to AudioDecoder. (https://codereview.chromium.org/126793002/)

media/filters/audio_renderer_impl_unittest.cc

Index: media/filters/audio_renderer_impl_unittest.cc
diff --git a/media/filters/audio_renderer_impl_unittest.cc b/media/filters/audio_renderer_impl_unittest.cc
index 5adfbc499f7491f8176e71d4c801104f0fa1cf26..37a334ae3ab48b8d6353dbf8426bda0d9c3d7f2c 100644
--- a/media/filters/audio_renderer_impl_unittest.cc
+++ b/media/filters/audio_renderer_impl_unittest.cc
@@ -7,6 +7,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "media/base/audio_buffer.h"
@@ -46,7 +47,8 @@ class AudioRendererImplTest : public ::testing::Test {
  public:
   // Give the decoder some non-garbage media properties.
   AudioRendererImplTest()
-      : demuxer_stream_(DemuxerStream::AUDIO),
+      : needs_stop_(true),
+        demuxer_stream_(DemuxerStream::AUDIO),
         decoder_(new MockAudioDecoder()) {
     AudioDecoderConfig audio_config(kCodec,
                                     kSampleFormat,
@@ -64,6 +66,9 @@ class AudioRendererImplTest : public ::testing::Test {
     EXPECT_CALL(*decoder_, Reset(_))
         .WillRepeatedly(Invoke(this, &AudioRendererImplTest::ResetDecoder));
 
+    EXPECT_CALL(*decoder_, Stop(_))
+        .WillRepeatedly(Invoke(this, &AudioRendererImplTest::StopDecoder));
+
     // Set up audio properties.
     EXPECT_CALL(*decoder_, bits_per_channel())
         .WillRepeatedly(Return(audio_config.bits_per_channel()));
@@ -88,9 +93,11 @@ class AudioRendererImplTest : public ::testing::Test {
 
   virtual ~AudioRendererImplTest() {
     SCOPED_TRACE("~AudioRendererImplTest()");
-    WaitableMessageLoopEvent event;
-    renderer_->Stop(event.GetClosure());
-    event.RunAndWait();
+    if (needs_stop_) {
+      WaitableMessageLoopEvent event;
+      renderer_->Stop(event.GetClosure());
+      event.RunAndWait();
+    }
   }
 
   void ExpectUnsupportedAudioDecoder() {
@@ -371,11 +378,25 @@ class AudioRendererImplTest : public ::testing::Test {
     time_ += time;
   }
 
+  void HoldStopDecoderCB() {
+    EXPECT_CALL(*decoder_, Stop(_)).WillRepeatedly(
+        Invoke(this, &AudioRendererImplTest::StopDecoderHoldCB));
+  }
+
+  void DispatchHeldStopDecoderCB() {
+    base::ResetAndReturn(&stop_decoder_cb_).Run();
+  }
+
+
   // Fixture members.
   base::MessageLoop message_loop_;
   scoped_ptr<AudioRendererImpl> renderer_;
   scoped_refptr<FakeAudioRendererSink> sink_;
 
+  // Whether or not the test needs the destructor to call Stop() on
+  // |renderer_| at destruction.
+  bool needs_stop_;
+
  private:
   TimeTicks GetTime() {
     base::AutoLock auto_lock(lock_);
@@ -383,6 +404,9 @@ class AudioRendererImplTest : public ::testing::Test {
   }
 
   void ReadDecoder(const AudioDecoder::ReadCB& read_cb) {
+    // We shouldn't ever call Read() after Stop():
+    EXPECT_TRUE(stop_decoder_cb_.is_null());
+
     // TODO(scherkus): Make this a DCHECK after threading semantics are fixed.
     if (base::MessageLoop::current() != &message_loop_) {
       message_loop_.PostTask(FROM_HERE, base::Bind(
@@ -406,6 +430,14 @@ class AudioRendererImplTest : public ::testing::Test {
     message_loop_.PostTask(FROM_HERE, reset_cb);
   }
 
+  void StopDecoder(const base::Closure& stop_cb) {
+    message_loop_.PostTask(FROM_HERE, stop_cb);
+  }
+
+  void StopDecoderHoldCB(const base::Closure& stop_cb) {
+    stop_decoder_cb_ = stop_cb;
+  }
+
   void DeliverBuffer(AudioDecoder::Status status,
                      const scoped_refptr<AudioBuffer>& buffer) {
     CHECK(!read_cb_.is_null());
@@ -428,6 +460,8 @@ class AudioRendererImplTest : public ::testing::Test {
   // Run during ReadDecoder() to unblock WaitForPendingRead().
   base::Closure wait_for_pending_read_cb_;
 
+  base::Closure stop_decoder_cb_;
+
   DISALLOW_COPY_AND_ASSIGN(AudioRendererImplTest);
 };
 
@@ -769,7 +803,6 @@ TEST_F(AudioRendererImplTest, PendingRead_Pause) {
   Preroll(1000, PIPELINE_OK);
 }
 
-
 TEST_F(AudioRendererImplTest, PendingRead_Flush) {
   Initialize();
 
@@ -798,7 +831,32 @@ TEST_F(AudioRendererImplTest, PendingRead_Flush) {
   Preroll(1000, PIPELINE_OK);
 }
 
-TEST_F(AudioRendererImplTest, StopDuringFlush) {
+TEST_F(AudioRendererImplTest, PendingRead_Stop) {
+  Initialize();
+
+  Preroll();
+  Play();
+
+  // Partially drain internal buffer so we get a pending read.
+  EXPECT_TRUE(ConsumeBufferedData(frames_buffered() / 2, NULL));
+  WaitForPendingRead();
+
+  Pause();
+
+  EXPECT_TRUE(IsReadPending());
+
+  WaitableMessageLoopEvent stop_event;
+  renderer_->Stop(stop_event.GetClosure());
+  needs_stop_ = false;
+
+  SatisfyPendingRead(kDataSize);
+
+  stop_event.RunAndWait();
+
+  EXPECT_FALSE(IsReadPending());
+}
+
+TEST_F(AudioRendererImplTest, PendingFlush_Stop) {
   Initialize();
 
   Preroll();
@@ -818,9 +876,38 @@ TEST_F(AudioRendererImplTest, StopDuringFlush) {
 
   SatisfyPendingRead(kDataSize);
 
-  // Request a Stop() before the flush completes.
+  WaitableMessageLoopEvent event;
+  renderer_->Stop(event.GetClosure());
+  event.RunAndWait();
+  needs_stop_ = false;
+}
+
+TEST_F(AudioRendererImplTest, PendingStop_Read) {
+
+  // This reproduces crbug.com/335181, basically an extra Read() call to the
+  // decoder can sneak into the gap between Stop() and StopDecoderDone().
+
+  Initialize();
+
+  Preroll();
+  Play();
+
+  HoldStopDecoderCB();
   WaitableMessageLoopEvent stop_event;
   renderer_->Stop(stop_event.GetClosure());
+  needs_stop_ = false;
+
+  // ARI::Stop has called Stop() on the decoder now, but we've stalled
+  // the call to ARI::StopDecoderDone()...
+
+  // Now, let's try to cause a read:
+  ConsumeAllBufferedData();

[xhwang, 2014/01/22 03:53:54]

Shouldn't we make sure that this never happens? If we do so, then we probably
don't need this test.

[rileya (GONE FROM CHROMIUM), 2014/01/22 17:35:28]

I guess this was an unrealistic repro case. I've adjusted it to a more feasible
set of events. It calls ConsumeAllBufferedData() before Stop()-ing, which queues
up some ARI::AttemptRead() calls, and then does a RunUntilIdle() after calling
Stop() which dispatches those queued calls.

It's still a somewhat contrived test, but I'd argue it has value in preventing
anyone from doing what I did in the future, since it was a pretty
subtle/annoying to track down race condition (that said, we'll soon move this
logic into the audio equivalent of VideoFrameStream anyway, so this test and any
reason for it existing will die then anyway...).

+
+  // Run until idle to let any pending calls to Read() to go through.
+  base::RunLoop().RunUntilIdle();
+
+  // And now run ARI::StopDecoderDone()
+  DispatchHeldStopDecoderCB();
   stop_event.RunAndWait();
 }