Enforce Stop() is always called before dtor in FFmepgVideoDecoder.

Enforce this because Stop() needs to do some shutdown work asynchronously.
- Updated comments in VideoDecoder interface.
- Added DCHECK in dtor.
- Updated FFmpegVideoDecoderTest to call Stop() in dtor.

BUG=93932,152262
TEST=Updated media_unittest

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

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

4 files changed, 53 insertions, 35 deletions

diff --git a/media/base/video_decoder.h b/media/base/video_decoder.h
index b22b144..4c86217 100644
--- a/media/base/video_decoder.h
+++ b/media/base/video_decoder.h
@@ -26,9 +26,9 @@ class MEDIA_EXPORT VideoDecoder
     kDecryptError  // Decrypting error happened.
   };
 
-  // Initialize a VideoDecoder with the given DemuxerStream, executing the
-  // callback upon completion.
-  // statistics_cb is used to update global pipeline statistics.
+  // Initializes a VideoDecoder with the given DemuxerStream, executing the
+  // |status_cb| upon completion.
+  // |statistics_cb| is used to update global pipeline statistics.
   virtual void Initialize(const scoped_refptr<DemuxerStream>& stream,
                           const PipelineStatusCB& status_cb,
                           const StatisticsCB& statistics_cb) = 0;
@@ -50,13 +50,16 @@ class MEDIA_EXPORT VideoDecoder
   typedef base::Callback<void(Status, const scoped_refptr<VideoFrame>&)> ReadCB;
   virtual void Read(const ReadCB& read_cb) = 0;
 
-  // Reset decoder state, fulfilling all pending ReadCB and dropping extra
+  // Resets decoder state, fulfilling all pending ReadCB and dropping extra
   // queued decoded data. After this call, the decoder is back to an initialized
   // clean state.
   virtual void Reset(const base::Closure& closure) = 0;
 
-  // Stop decoder and set it to an uninitialized state. Note that a VideoDecoder
-  // should/could not be re-initialized after it has been stopped.
+  // Stops decoder, fires any pending callbacks and sets the decoder to an
+  // uninitialized state. A VideoDecoder cannot be re-initialized after it has
+  // been stopped.
+  // Note that if Initialize() has been called, Stop() must be called and
+  // complete before deleting the decoder.
   virtual void Stop(const base::Closure& closure) = 0;
 
   // Returns true if the output format has an alpha channel. Most formats do not
diff --git a/media/filters/ffmpeg_video_decoder.cc b/media/filters/ffmpeg_video_decoder.cc
index f05fa3f..ff44bf8 100644
--- a/media/filters/ffmpeg_video_decoder.cc
+++ b/media/filters/ffmpeg_video_decoder.cc
@@ -207,6 +207,11 @@ void FFmpegVideoDecoder::Stop(const base::Closure& closure) {
     return;
   }
 
+  if (state_ == kUninitialized) {
+    closure.Run();
+    return;
+  }
+
   if (decryptor_)
     decryptor_->CancelDecrypt();
 
@@ -226,7 +231,9 @@ void FFmpegVideoDecoder::DoStop() {
 }
 
 FFmpegVideoDecoder::~FFmpegVideoDecoder() {
-  ReleaseFFmpegResources();
+  DCHECK_EQ(kUninitialized, state_);
+  DCHECK(!codec_context_);
+  DCHECK(!av_frame_);
 }
 
 void FFmpegVideoDecoder::DoRead(const ReadCB& read_cb) {
@@ -528,11 +535,10 @@ bool FFmpegVideoDecoder::ConfigureDecoder() {
   codec_context_->release_buffer = ReleaseVideoBufferImpl;
 
   AVCodec* codec = avcodec_find_decoder(codec_context_->codec_id);
-  if (!codec)
-    return false;
-
-  if (avcodec_open2(codec_context_, codec, NULL) < 0)
+  if (!codec || avcodec_open2(codec_context_, codec, NULL) < 0) {
+    ReleaseFFmpegResources();
     return false;
+  }
 
   av_frame_ = avcodec_alloc_frame();
   return true;
diff --git a/media/filters/ffmpeg_video_decoder.h b/media/filters/ffmpeg_video_decoder.h
index 340864d..292adf1 100644
--- a/media/filters/ffmpeg_video_decoder.h
+++ b/media/filters/ffmpeg_video_decoder.h
@@ -50,7 +50,7 @@ class MEDIA_EXPORT FFmpegVideoDecoder : public VideoDecoder {
     kUninitialized,
     kNormal,
     kFlushCodec,
-    kDecodeFinished,
+    kDecodeFinished
   };
 
   // Carries out the reading operation scheduled by Read().
diff --git a/media/filters/ffmpeg_video_decoder_unittest.cc b/media/filters/ffmpeg_video_decoder_unittest.cc
index d97d7a0..00be225 100644
--- a/media/filters/ffmpeg_video_decoder_unittest.cc
+++ b/media/filters/ffmpeg_video_decoder_unittest.cc
@@ -6,6 +6,7 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/message_loop.h"
 #include "base/memory/singleton.h"
 #include "base/string_util.h"
@@ -25,6 +26,8 @@
 #include "testing/gmock/include/gmock/gmock.h"
 
 using ::testing::_;
+using ::testing::AtMost;
+using ::testing::Invoke;
 using ::testing::IsNull;
 using ::testing::Return;
 using ::testing::ReturnRef;
@@ -63,10 +66,6 @@ ACTION_P2(RunDecryptCB, status, buffer) {
   arg1.Run(status, buffer);
 }
 
-ACTION_P3(RunDecryptCB3, decrypt_cb, status, buffer) {
-  decrypt_cb.Run(status, buffer);
-}
-
 class FFmpegVideoDecoderTest : public testing::Test {
  public:
   FFmpegVideoDecoderTest()
@@ -90,7 +89,9 @@ class FFmpegVideoDecoderTest : public testing::Test {
     encrypted_i_frame_buffer_ = CreateFakeEncryptedBuffer();
   }
 
-  virtual ~FFmpegVideoDecoderTest() {}
+  virtual ~FFmpegVideoDecoderTest() {
+    Stop();
+  }
 
   void Initialize() {
     config_.Initialize(kCodecVP8, VIDEO_CODEC_PROFILE_UNKNOWN, kVideoFormat,
@@ -122,12 +123,27 @@ class FFmpegVideoDecoderTest : public testing::Test {
     InitializeWithConfigAndStatus(config, PIPELINE_OK);
   }
 
+  void CancelDecrypt() {
+    if (!decrypt_cb_.is_null()) {
+       base::ResetAndReturn(&decrypt_cb_).Run(
+           Decryptor::kError, scoped_refptr<DecoderBuffer>(NULL));
+    }
+  }
+
   void Reset() {
+    EXPECT_CALL(*decryptor_, CancelDecrypt())
+        .WillOnce(Invoke(this, &FFmpegVideoDecoderTest::CancelDecrypt));
     decoder_->Reset(NewExpectedClosure());
     message_loop_.RunAllPending();
   }
 
   void Stop() {
+    // Use AtMost(1) here because CancelDecrypt() will be called once if the
+    // decoder was initialized and has not been stopped, and will not be
+    // called otherwise.
+    EXPECT_CALL(*decryptor_, CancelDecrypt())
+        .Times(AtMost(1))
+        .WillRepeatedly(Invoke(this, &FFmpegVideoDecoderTest::CancelDecrypt));
     decoder_->Stop(NewExpectedClosure());
     message_loop_.RunAllPending();
   }
@@ -229,6 +245,7 @@ class FFmpegVideoDecoderTest : public testing::Test {
   VideoDecoderConfig config_;
 
   VideoDecoder::ReadCB read_cb_;
+  Decryptor::DecryptCB decrypt_cb_;
 
   // Various buffers for testing.
   scoped_array<uint8_t> frame_buffer_;
@@ -517,6 +534,9 @@ TEST_F(FFmpegVideoDecoderTest, DecodeEncryptedFrame_CorruptedBufferReturned) {
   EXPECT_CALL(*decryptor_, Decrypt(encrypted_i_frame_buffer_, _))
       .WillRepeatedly(RunDecryptCB(Decryptor::kSuccess,
                                    corrupt_i_frame_buffer_));
+  // The decoder only detects the error at the second decoding call. So
+  // |statistics_cb_| still gets called once.
+  EXPECT_CALL(statistics_cb_, OnStatistics(_));
 
   // Our read should still get satisfied with end of stream frame during an
   // error.
@@ -580,19 +600,14 @@ TEST_F(FFmpegVideoDecoderTest, Reset_DuringPendingDecrypt) {
 
   EXPECT_CALL(*demuxer_, Read(_))
       .WillRepeatedly(ReturnBuffer(encrypted_i_frame_buffer_));
-
-  Decryptor::DecryptCB decrypt_cb;
   EXPECT_CALL(*decryptor_, Decrypt(encrypted_i_frame_buffer_, _))
-      .WillOnce(SaveArg<1>(&decrypt_cb));
+      .WillOnce(SaveArg<1>(&decrypt_cb_));
 
   decoder_->Read(read_cb_);
   message_loop_.RunAllPending();
   // Make sure the Read() on the decoder triggers a Decrypt() on the decryptor.
-  EXPECT_FALSE(decrypt_cb.is_null());
+  EXPECT_FALSE(decrypt_cb_.is_null());
 
-  EXPECT_CALL(*decryptor_, CancelDecrypt())
-      .WillOnce(RunDecryptCB3(decrypt_cb, Decryptor::kError,
-                              scoped_refptr<DecoderBuffer>(NULL)));
   EXPECT_CALL(*this, FrameReady(VideoDecoder::kOk, IsNull()));
   Reset();
   message_loop_.RunAllPending();
@@ -647,19 +662,14 @@ TEST_F(FFmpegVideoDecoderTest, Stop_DuringPendingDecrypt) {
 
   EXPECT_CALL(*demuxer_, Read(_))
       .WillRepeatedly(ReturnBuffer(encrypted_i_frame_buffer_));
-
-  Decryptor::DecryptCB decrypt_cb;
   EXPECT_CALL(*decryptor_, Decrypt(encrypted_i_frame_buffer_, _))
-      .WillOnce(SaveArg<1>(&decrypt_cb));
+      .WillOnce(SaveArg<1>(&decrypt_cb_));
 
   decoder_->Read(read_cb_);
   message_loop_.RunAllPending();
   // Make sure the Read() on the decoder triggers a Decrypt() on the decryptor.
-  EXPECT_FALSE(decrypt_cb.is_null());
+  EXPECT_FALSE(decrypt_cb_.is_null());
 
-  EXPECT_CALL(*decryptor_, CancelDecrypt())
-      .WillOnce(RunDecryptCB3(decrypt_cb, Decryptor::kError,
-                              scoped_refptr<DecoderBuffer>(NULL)));
   EXPECT_CALL(*this, FrameReady(VideoDecoder::kOk, IsNull()));
   Stop();
   message_loop_.RunAllPending();
@@ -682,7 +692,7 @@ TEST_F(FFmpegVideoDecoderTest, AbortPendingRead) {
 }
 
 // Test aborted read on the demuxer stream.
-TEST_F(FFmpegVideoDecoderTest, AbortPendingReadDuringFlush) {
+TEST_F(FFmpegVideoDecoderTest, AbortPendingReadDuringReset) {
   Initialize();
 
   DemuxerStream::ReadCB read_cb;
@@ -695,9 +705,8 @@ TEST_F(FFmpegVideoDecoderTest, AbortPendingReadDuringFlush) {
   message_loop_.RunAllPending();
   ASSERT_FALSE(read_cb.is_null());
 
-  // Flush while there is still an outstanding read on the demuxer.
-  decoder_->Reset(NewExpectedClosure());
-  message_loop_.RunAllPending();
+  // Reset while there is still an outstanding read on the demuxer.
+  Reset();
 
   // Signal an aborted demuxer read.
   read_cb.Run(DemuxerStream::kAborted, NULL);