Add Stop() to AudioDecoder.

media/filters/ffmpeg_audio_decoder_unittest.cc

Index: media/filters/ffmpeg_audio_decoder_unittest.cc
diff --git a/media/filters/ffmpeg_audio_decoder_unittest.cc b/media/filters/ffmpeg_audio_decoder_unittest.cc
index c19bbabf580e4e7cc99cf7b7cd20c02d73e1b279..f3120e47f661a4e1ab35e0fcec821850ba31d1c7 100644
--- a/media/filters/ffmpeg_audio_decoder_unittest.cc
+++ b/media/filters/ffmpeg_audio_decoder_unittest.cc
@@ -6,8 +6,10 @@
 
 #include "base/bind.h"
 #include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "media/base/audio_buffer.h"
+#include "media/base/bind_to_loop.h"
 #include "media/base/decoder_buffer.h"
 #include "media/base/mock_filters.h"
 #include "media/base/test_data_util.h"
@@ -26,11 +28,18 @@ ACTION_P(InvokeReadPacket, test) {
   test->ReadPacket(arg0);
 }
 
+ACTION_P(InvokeReadPacketNoCallback, test) {
+  test->ReadPacketNoCallback(arg0);
+}
+
 class FFmpegAudioDecoderTest : public testing::Test {
  public:
   FFmpegAudioDecoderTest()
       : decoder_(new FFmpegAudioDecoder(message_loop_.message_loop_proxy())),
-        demuxer_(new StrictMock<MockDemuxerStream>(DemuxerStream::AUDIO)) {
+        demuxer_(new StrictMock<MockDemuxerStream>(DemuxerStream::AUDIO)),
+        pending_read_(false),
+        pending_reset_(false),
+        pending_stop_(false) {
     FFmpegGlue::InitializeFFmpeg();
 
     vorbis_extradata_ = ReadTestDataFile("vorbis-extradata");
@@ -83,17 +92,79 @@ class FFmpegAudioDecoderTest : public testing::Test {
     read_cb.Run(status, buffer);
   }
 
+  void ReadPacketNoCallback(const DemuxerStream::ReadCB& read_cb) {
+    // We just take note of it and ignore the callback, we'll expect it to be
+    // fired as a result of a Stop/Reset/etc.
+    pending_read_ = true;
+    pending_read_cb_ = read_cb;
+  }
+
+  void PendingRead() {

[xhwang, 2014/01/14 22:25:02]

We should be able to merge this function into Read() to simplify things. To do
this we also need to merge the callbacks (DecodeFinished and PendingReadAborted)
to something like ReadFinished. Then we can check that if reset or stop is
pending, the read should be aborted, etc. WDYT?

[rileya (GONE FROM CHROMIUM), 2014/01/14 23:02:40]

Sounds good, I did this.

+    decoder_->Read(base::Bind(
+        &FFmpegAudioDecoderTest::PendingReadAborted, base::Unretained(this)));
+    message_loop_.RunUntilIdle();
+  }
+
+  void SatisfyPendingRead() {
+    EXPECT_TRUE(pending_read_ && !pending_read_cb_.is_null());
+    ReadPacket(pending_read_cb_);
+    base::RunLoop run_loop;

[DaleCurtis, 2014/01/14 22:31:45]

I'm wary of having mixed local global variables here.  As well as the mixed
usage of RunLoop.RunUntilIdle() and MessageLoop.RunUntilIdle(). 

It'd be nicer to have only RunLoop usage and have the RunLoop quit closure bound
into the respective callbacks and owned by each test.  Or if there's only one
instance of RunLoop.Run per test it can be owned at a global scope.  Otherwise,
maybe Reset() and Stop() take the quit closure as a parameter?

[rileya (GONE FROM CHROMIUM), 2014/01/14 23:02:40]

RunLoop::RunUntilIdle seems to behave properly (unlike the deprecated
MessageLoop version...) in the tests I was having issues with. I removed all
instances of MessageLoop::RunUntilIdle and replaced with the RunLoop version and
things seem to work fine. So I don't think the QuitClosure stuff is even
necessary at all.

+    run_loop_quit_ = run_loop.QuitClosure();
+    run_loop.Run();
+  }
+
   void Read() {

[xhwang, 2014/01/14 22:25:02]

Set pending_read_ = true here to be consistent with Reset() and Stop().

[rileya (GONE FROM CHROMIUM), 2014/01/14 23:02:40]

Done.

     decoder_->Read(base::Bind(
         &FFmpegAudioDecoderTest::DecodeFinished, base::Unretained(this)));
     message_loop_.RunUntilIdle();
   }
 
+  void Stop() {
+    pending_stop_ = true;
+    decoder_->Stop(base::Bind(
+        &FFmpegAudioDecoderTest::StopFinished, base::Unretained(this)));
+  }
+
+  void Reset() {
+    pending_reset_ = true;
+    decoder_->Reset(base::Bind(
+        &FFmpegAudioDecoderTest::ResetFinished, base::Unretained(this)));
+  }

[xhwang, 2014/01/14 22:25:02]

nit: change the order of Reset() and Stop().

[rileya (GONE FROM CHROMIUM), 2014/01/14 23:02:40]

Fixed.

+
   void DecodeFinished(AudioDecoder::Status status,
                       const scoped_refptr<AudioBuffer>& buffer) {
     decoded_audio_.push_back(buffer);
   }
 
+  void PendingReadAborted(AudioDecoder::Status status,
+                          const scoped_refptr<AudioBuffer>& buffer) {
+    EXPECT_EQ(status, AudioDecoder::kAborted);
+    EXPECT_TRUE(pending_read_);
+    pending_read_ = false;
+  }
+
+  void StopFinished() {
+    EXPECT_TRUE(pending_stop_);
+    pending_stop_ = false;
+
+    // Stop should always finish after Read and Reset.
+    EXPECT_FALSE(pending_read_);
+    EXPECT_FALSE(pending_reset_);
+    run_loop_quit_.Run();

[xhwang, 2014/01/14 22:25:02]

This seems too much hassle to exit the RunLoop. I saw RunLoop::RunUntilIdle()
used in a lot of places. We need to find out why RunUntilIdle() isn't working
for us.

For example:
https://code.google.com/p/chromium/codesearch#chromium/src/dbus/bus_unittest....

[rileya (GONE FROM CHROMIUM), 2014/01/14 23:02:40]

Ah, didn't know RunLoop had a RunUntilIdle, MessageLoop::RunUntilIdle seemed not
to work but RunLoop's version does, fixed.

+  }
+
+  void ResetFinished() {
+    EXPECT_TRUE(pending_reset_);
+    pending_reset_ = false;
+
+    // Reset should always finish after Read.
+    EXPECT_FALSE(pending_read_);
+
+    // if there's a stop pending, we expect StopFinished to end the RunLoop
+    if (!pending_stop_)
+      run_loop_quit_.Run();
+  }
+
   void ExpectDecodedAudio(size_t i, int64 timestamp, int64 duration) {
     EXPECT_LT(i, decoded_audio_.size());
     EXPECT_EQ(timestamp, decoded_audio_[i]->timestamp().InMicroseconds());
@@ -110,6 +181,11 @@ class FFmpegAudioDecoderTest : public testing::Test {
   scoped_ptr<FFmpegAudioDecoder> decoder_;
   scoped_ptr<StrictMock<MockDemuxerStream> > demuxer_;
   MockStatisticsCB statistics_cb_;
+  DemuxerStream::ReadCB pending_read_cb_;

[xhwang, 2014/01/14 22:25:02]

pending_demuxer_read_cb_ just to be clear?

[rileya (GONE FROM CHROMIUM), 2014/01/14 23:02:40]

Sounds good.

+  base::Closure run_loop_quit_;
+  bool pending_read_;
+  bool pending_reset_;
+  bool pending_stop_;
 
   scoped_refptr<DecoderBuffer> vorbis_extradata_;
 
@@ -168,4 +244,72 @@ TEST_F(FFmpegAudioDecoderTest, ReadAbort) {
   EXPECT_TRUE(decoded_audio_[0].get() ==  NULL);
 }
 
+TEST_F(FFmpegAudioDecoderTest, PendingRead_Stop) {
+  Initialize();
+
+  encoded_audio_.clear();
+  encoded_audio_.push_back(NULL);
+
+  EXPECT_CALL(*demuxer_, Read(_))
+      .WillOnce(InvokeReadPacketNoCallback(this));

[xhwang, 2014/01/14 22:25:02]

This name is a bit confusing. How about HoldDemuxerRead() or
EnterPendingDemuxerReadState()?

[rileya (GONE FROM CHROMIUM), 2014/01/14 23:02:40]

I like EnterPendingDemuxerReadState, going with that.

+  PendingRead();
+  Stop();

[xhwang, 2014/01/14 22:25:02]

After Stop() you can check that read and stop are still pending.

[rileya (GONE FROM CHROMIUM), 2014/01/14 23:02:40]

Good catch, added checks like that to each of these tests.

+  SatisfyPendingRead();
+
+  EXPECT_FALSE(pending_read_);
+  EXPECT_FALSE(pending_stop_);
+}
+
+TEST_F(FFmpegAudioDecoderTest, PendingRead_Reset) {
+  Initialize();
+
+  encoded_audio_.clear();
+  encoded_audio_.push_back(NULL);
+
+  EXPECT_CALL(*demuxer_, Read(_))
+      .WillOnce(InvokeReadPacketNoCallback(this));
+  PendingRead();
+  Reset();
+  SatisfyPendingRead();
+
+  EXPECT_FALSE(pending_read_);
+  EXPECT_FALSE(pending_reset_);
+}
+
+TEST_F(FFmpegAudioDecoderTest, PendingRead_ResetStop) {
+  Initialize();
+
+  encoded_audio_.clear();
+  encoded_audio_.push_back(NULL);
+
+  EXPECT_CALL(*demuxer_, Read(_))
+      .WillOnce(InvokeReadPacketNoCallback(this));
+  PendingRead();
+  Reset();
+  Stop();
+  SatisfyPendingRead();
+
+  EXPECT_FALSE(pending_read_);
+  EXPECT_FALSE(pending_stop_);
+  EXPECT_FALSE(pending_reset_);
+}
+
+TEST_F(FFmpegAudioDecoderTest, PendingReset_Stop) {
+  Initialize();
+
+  encoded_audio_.clear();
+  encoded_audio_.push_back(NULL);
+
+  Reset();
+  Stop();
+
+  base::RunLoop run_loop;
+  run_loop_quit_ = run_loop.QuitClosure();
+  run_loop.Run();
+
+  EXPECT_FALSE(pending_read_);
+  EXPECT_FALSE(pending_stop_);
+  EXPECT_FALSE(pending_reset_);

[xhwang, 2014/01/14 22:25:02]

You can probably add these 3 checks in the dtor of FADTest so that we don't need
to explicitly check them for every test.

[rileya (GONE FROM CHROMIUM), 2014/01/14 23:02:40]

Makes sense, done.

+}
+
 }  // namespace media