Simplify VideoFrameStream stopping process.

When VideoFrameStream::Stop() is called, invalidate all existing weak pointers,
satisfy all pending callbacks and start to stop DecryptingDemuxerStream and/or
VideoDecoder right away.

This also fixes the issue where VideoFrameStream::Stop() is called when
DecryptingDemuxerStream is in kWaitingForKey state.

BUG=304260,304577,303835
TEST=Added a test where VFS::Stop() is called when DDS is waiting for a key.

Review URL: https://codereview.chromium.org/26177002

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

3 files changed, 82 insertions, 60 deletions

diff --git a/media/filters/video_frame_stream.cc b/media/filters/video_frame_stream.cc
index 80e5937..88f965b 100644
--- a/media/filters/video_frame_stream.cc
+++ b/media/filters/video_frame_stream.cc
@@ -44,8 +44,6 @@ void VideoFrameStream::Initialize(DemuxerStream* stream,
   DCHECK(init_cb_.is_null());
   DCHECK(!init_cb.is_null());
 
-  weak_this_ = weak_factory_.GetWeakPtr();
-
   statistics_cb_ = statistics_cb;
   init_cb_ = init_cb;
   stream_ = stream;
@@ -53,7 +51,9 @@ void VideoFrameStream::Initialize(DemuxerStream* stream,
   state_ = STATE_INITIALIZING;
   // TODO(xhwang): VideoDecoderSelector only needs a config to select a decoder.
   decoder_selector_->SelectVideoDecoder(
-      stream, base::Bind(&VideoFrameStream::OnDecoderSelected, weak_this_));
+      stream,
+      base::Bind(&VideoFrameStream::OnDecoderSelected,
+                 weak_factory_.GetWeakPtr()));
 }
 
 void VideoFrameStream::Read(const ReadCB& read_cb) {
@@ -109,7 +109,7 @@ void VideoFrameStream::Reset(const base::Closure& closure) {
   // the decoder reset is finished.
   if (decrypting_demuxer_stream_) {
     decrypting_demuxer_stream_->Reset(base::Bind(
-        &VideoFrameStream::ResetDecoder, weak_this_));
+        &VideoFrameStream::ResetDecoder, weak_factory_.GetWeakPtr()));
     return;
   }
 
@@ -129,18 +129,21 @@ void VideoFrameStream::Stop(const base::Closure& closure) {
     return;
   }
 
-  // The stopping process will continue after the pending operation is finished.
-  if (state_ == STATE_PENDING_DEMUXER_READ)
-    return;
+  DCHECK(init_cb_.is_null());
+
+  // All pending callbacks will be dropped.
+  weak_factory_.InvalidateWeakPtrs();
+
+  // Post callbacks to prevent reentrance into this object.
+  if (!read_cb_.is_null())
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        base::ResetAndReturn(&read_cb_), ABORTED, scoped_refptr<VideoFrame>()));
+  if (!reset_cb_.is_null())
+    message_loop_->PostTask(FROM_HERE, base::ResetAndReturn(&reset_cb_));
 
-  // VideoDecoder API guarantees that if VideoDecoder::Stop() is called during
-  // a pending reset or a pending decode, the callbacks are always fired in the
-  // decode -> reset -> stop order. Therefore, we can call VideoDecoder::Stop()
-  // regardless of if we have a pending decode or reset and always satisfy the
-  // stop callback when the decoder decode/reset is finished.
   if (decrypting_demuxer_stream_) {
     decrypting_demuxer_stream_->Reset(base::Bind(
-        &VideoFrameStream::StopDecoder, weak_this_));
+        &VideoFrameStream::StopDecoder, weak_factory_.GetWeakPtr()));
     return;
   }
 
@@ -204,6 +207,12 @@ void VideoFrameStream::SatisfyRead(Status status,
 }
 
 void VideoFrameStream::AbortRead() {
+  // Abort read during pending reset. It is safe to fire the |read_cb_| directly
+  // instead of posting it because VideoRenderBase won't call into this class
+  // again when it's in kFlushing state.
+  // TODO(xhwang): Improve the resetting process to avoid this dependency on the
+  // caller.
+  DCHECK(!reset_cb_.is_null());
   SatisfyRead(ABORTED, NULL);
 }
 
@@ -217,7 +226,7 @@ void VideoFrameStream::Decode(const scoped_refptr<DecoderBuffer>& buffer) {
 
   int buffer_size = buffer->end_of_stream() ? 0 : buffer->data_size();
   decoder_->Decode(buffer, base::Bind(&VideoFrameStream::OnFrameReady,
-                                      weak_this_, buffer_size));
+                                      weak_factory_.GetWeakPtr(), buffer_size));
 }
 
 void VideoFrameStream::FlushDecoder() {
@@ -230,6 +239,7 @@ void VideoFrameStream::OnFrameReady(int buffer_size,
   DVLOG(2) << __FUNCTION__;
   DCHECK(state_ == STATE_NORMAL || state_ == STATE_FLUSHING_DECODER) << state_;
   DCHECK(!read_cb_.is_null());
+  DCHECK(stop_cb_.is_null());
 
   if (status == VideoDecoder::kDecodeError) {
     DCHECK(!frame.get());
@@ -252,10 +262,9 @@ void VideoFrameStream::OnFrameReady(int buffer_size,
     statistics_cb_.Run(statistics);
   }
 
-  // Drop decoding result if Reset()/Stop() was called during decoding.
-  // The stopping/resetting process will be handled when the decoder is
-  // stopped/reset.
-  if (!stop_cb_.is_null() || !reset_cb_.is_null()) {
+  // Drop decoding result if Reset() was called during decoding.
+  // The resetting process will be handled when the decoder is reset.
+  if (!reset_cb_.is_null()) {
     AbortRead();
     return;
   }
@@ -286,7 +295,8 @@ void VideoFrameStream::ReadFromDemuxerStream() {
   DCHECK(stop_cb_.is_null());
 
   state_ = STATE_PENDING_DEMUXER_READ;
-  stream_->Read(base::Bind(&VideoFrameStream::OnBufferReady, weak_this_));
+  stream_->Read(
+      base::Bind(&VideoFrameStream::OnBufferReady, weak_factory_.GetWeakPtr()));
 }
 
 void VideoFrameStream::OnBufferReady(
@@ -297,20 +307,10 @@ void VideoFrameStream::OnBufferReady(
   DCHECK_EQ(state_, STATE_PENDING_DEMUXER_READ) << state_;
   DCHECK_EQ(buffer.get() != NULL, status == DemuxerStream::kOk) << status;
   DCHECK(!read_cb_.is_null());
+  DCHECK(stop_cb_.is_null());
 
   state_ = STATE_NORMAL;
 
-  // Reset()/Stop() was postponed during STATE_PENDING_DEMUXER_READ state.
-  // We need to handle them in this function.
-
-  if (!stop_cb_.is_null()) {
-    AbortRead();
-    if (!reset_cb_.is_null())
-      Reset(base::ResetAndReturn(&reset_cb_));
-    Stop(base::ResetAndReturn(&stop_cb_));
-    return;
-  }
-
   if (status == DemuxerStream::kConfigChanged) {
     state_ = STATE_FLUSHING_DECODER;
     if (!reset_cb_.is_null()) {
@@ -345,40 +345,34 @@ void VideoFrameStream::ReinitializeDecoder() {
 
   DCHECK(stream_->video_decoder_config().IsValidConfig());
   state_ = STATE_REINITIALIZING_DECODER;
-  decoder_->Initialize(
-      stream_->video_decoder_config(),
-      base::Bind(&VideoFrameStream::OnDecoderReinitialized, weak_this_));
+  decoder_->Initialize(stream_->video_decoder_config(),
+                       base::Bind(&VideoFrameStream::OnDecoderReinitialized,
+                                  weak_factory_.GetWeakPtr()));
 }
 
 void VideoFrameStream::OnDecoderReinitialized(PipelineStatus status) {
   DVLOG(2) << __FUNCTION__;
   DCHECK(message_loop_->BelongsToCurrentThread());
   DCHECK_EQ(state_, STATE_REINITIALIZING_DECODER) << state_;
+  DCHECK(stop_cb_.is_null());
 
   // ReinitializeDecoder() can be called in two cases:
   // 1, Flushing decoder finished (see OnFrameReady()).
   // 2, Reset() was called during flushing decoder (see OnDecoderReset()).
-  // Also, Reset()/Stop() can be called during pending ReinitializeDecoder().
+  // Also, Reset() can be called during pending ReinitializeDecoder().
   // This function needs to handle them all!
 
   state_ = (status == PIPELINE_OK) ? STATE_NORMAL : STATE_ERROR;
 
-  if (!read_cb_.is_null() && (!stop_cb_.is_null() || !reset_cb_.is_null()))
-    AbortRead();
-
-  if (!reset_cb_.is_null())
+  if (!reset_cb_.is_null()) {
+    if (!read_cb_.is_null())
+      AbortRead();
     base::ResetAndReturn(&reset_cb_).Run();
-
-  // If !stop_cb_.is_null(), it will be handled in OnDecoderStopped().
+  }
 
   if (read_cb_.is_null())
     return;
 
-  if (!stop_cb_.is_null()) {
-    base::ResetAndReturn(&read_cb_).Run(ABORTED, NULL);
-    return;
-  }
-
   if (state_ == STATE_ERROR) {
     SatisfyRead(DECODE_ERROR, NULL);
     return;
@@ -394,7 +388,8 @@ void VideoFrameStream::ResetDecoder() {
          state_ == STATE_ERROR) << state_;
   DCHECK(!reset_cb_.is_null());
 
-  decoder_->Reset(base::Bind(&VideoFrameStream::OnDecoderReset, weak_this_));
+  decoder_->Reset(base::Bind(&VideoFrameStream::OnDecoderReset,
+                             weak_factory_.GetWeakPtr()));
 }
 
 void VideoFrameStream::OnDecoderReset() {
@@ -406,8 +401,9 @@ void VideoFrameStream::OnDecoderReset() {
   // before the reset callback is fired.
   DCHECK(read_cb_.is_null());
   DCHECK(!reset_cb_.is_null());
+  DCHECK(stop_cb_.is_null());
 
-  if (state_ != STATE_FLUSHING_DECODER || !stop_cb_.is_null()) {
+  if (state_ != STATE_FLUSHING_DECODER) {
     base::ResetAndReturn(&reset_cb_).Run();
     return;
   }
@@ -422,7 +418,8 @@ void VideoFrameStream::StopDecoder() {
   DCHECK(state_ != STATE_UNINITIALIZED && state_ != STATE_STOPPED) << state_;
   DCHECK(!stop_cb_.is_null());
 
-  decoder_->Stop(base::Bind(&VideoFrameStream::OnDecoderStopped, weak_this_));
+  decoder_->Stop(base::Bind(&VideoFrameStream::OnDecoderStopped,
+                            weak_factory_.GetWeakPtr()));
 }
 
 void VideoFrameStream::OnDecoderStopped() {
diff --git a/media/filters/video_frame_stream.h b/media/filters/video_frame_stream.h
index 7933e62..9b7f138 100644
--- a/media/filters/video_frame_stream.h
+++ b/media/filters/video_frame_stream.h
@@ -136,7 +136,6 @@ class MEDIA_EXPORT VideoFrameStream {
 
   scoped_refptr<base::MessageLoopProxy> message_loop_;
   base::WeakPtrFactory<VideoFrameStream> weak_factory_;
-  base::WeakPtr<VideoFrameStream> weak_this_;
 
   State state_;
 
diff --git a/media/filters/video_frame_stream_unittest.cc b/media/filters/video_frame_stream_unittest.cc
index e575105..8723e6e 100644
--- a/media/filters/video_frame_stream_unittest.cc
+++ b/media/filters/video_frame_stream_unittest.cc
@@ -40,7 +40,8 @@ class VideoFrameStreamTest : public testing::TestWithParam<bool> {
         pending_read_(false),
         pending_reset_(false),
         pending_stop_(false),
-        total_bytes_decoded_(0) {
+        total_bytes_decoded_(0),
+        has_no_key_(false) {
     ScopedVector<VideoDecoder> decoders;
     decoders.push_back(decoder_);
 
@@ -101,6 +102,11 @@ class VideoFrameStreamTest : public testing::TestWithParam<bool> {
   void Decrypt(Decryptor::StreamType stream_type,
                const scoped_refptr<DecoderBuffer>& encrypted,
                const Decryptor::DecryptCB& decrypt_cb) {
+    if (has_no_key_) {
+      decrypt_cb.Run(Decryptor::kNoKey, NULL);
+      return;
+    }
+
     DCHECK_EQ(stream_type, Decryptor::kVideo);
     scoped_refptr<DecoderBuffer> decrypted = DecoderBuffer::CopyFrom(
         encrypted->data(), encrypted->data_size());
@@ -116,7 +122,7 @@ class VideoFrameStreamTest : public testing::TestWithParam<bool> {
     // TODO(xhwang): Add test cases where the fake decoder returns error or
     // the fake demuxer aborts demuxer read.
     ASSERT_TRUE(status == VideoFrameStream::OK ||
-                status == VideoFrameStream::ABORTED);
+                status == VideoFrameStream::ABORTED) << status;
     frame_read_ = frame;
     if (frame.get() && !frame->IsEndOfStream())
       num_decoded_frames_++;
@@ -138,13 +144,17 @@ class VideoFrameStreamTest : public testing::TestWithParam<bool> {
     decoder_ = NULL;
   }
 
+  void ReadOneFrame() {
+    frame_read_ = NULL;
+    pending_read_ = true;
+    video_frame_stream_->Read(base::Bind(
+        &VideoFrameStreamTest::FrameReady, base::Unretained(this)));
+    message_loop_.RunUntilIdle();
+  }
+
   void ReadUntilPending() {
     do {
-      frame_read_ = NULL;
-      pending_read_ = true;
-      video_frame_stream_->Read(base::Bind(
-          &VideoFrameStreamTest::FrameReady, base::Unretained(this)));
-      message_loop_.RunUntilIdle();
+      ReadOneFrame();
     } while (!pending_read_);
   }
 
@@ -153,6 +163,7 @@ class VideoFrameStreamTest : public testing::TestWithParam<bool> {
     DEMUXER_READ_NORMAL,
     DEMUXER_READ_CONFIG_CHANGE,
     SET_DECRYPTOR,
+    DECRYPTOR_NO_KEY,
     DECODER_INIT,
     DECODER_REINIT,
     DECODER_READ,
@@ -181,6 +192,13 @@ class VideoFrameStreamTest : public testing::TestWithParam<bool> {
         InitializeVideoFrameStream();
         break;
 
+      case DECRYPTOR_NO_KEY:
+        EXPECT_CALL(*this, SetDecryptorReadyCallback(_))
+            .WillRepeatedly(RunCallback<0>(decryptor_.get()));
+        has_no_key_ = true;
+        ReadOneFrame();
+        break;
+
       case DECODER_INIT:
         EXPECT_CALL(*this, SetDecryptorReadyCallback(_))
             .WillRepeatedly(RunCallback<0>(decryptor_.get()));
@@ -230,9 +248,10 @@ class VideoFrameStreamTest : public testing::TestWithParam<bool> {
         demuxer_stream_->SatisfyRead();
         break;
 
+      // These two cases are only interesting to test during
+      // VideoFrameStream::Stop().  There's no need to satisfy a callback.
       case SET_DECRYPTOR:
-        // VideoFrameStream::Stop() does not wait for pending DecryptorReadyCB.
-        // Therefore there's no need to satisfy a callback.
+      case DECRYPTOR_NO_KEY:
         NOTREACHED();
         break;
 
@@ -245,12 +264,10 @@ class VideoFrameStreamTest : public testing::TestWithParam<bool> {
         break;
 
       case DECODER_READ:
-        DCHECK(pending_read_);
         decoder_->SatisfyRead();
         break;
 
       case DECODER_RESET:
-        DCHECK(pending_reset_);
         decoder_->SatisfyReset();
         break;
 
@@ -305,6 +322,9 @@ class VideoFrameStreamTest : public testing::TestWithParam<bool> {
   int total_bytes_decoded_;
   scoped_refptr<VideoFrame> frame_read_;
 
+  // Decryptor has no key to decrypt a frame.
+  bool has_no_key_;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(VideoFrameStreamTest);
 };
@@ -492,6 +512,12 @@ TEST_P(VideoFrameStreamTest, Stop_AfterConfigChangeRead) {
   Stop();
 }
 
+TEST_P(VideoFrameStreamTest, Stop_DuringNoKeyRead) {
+  Initialize();
+  EnterPendingState(DECRYPTOR_NO_KEY);
+  Stop();
+}
+
 TEST_P(VideoFrameStreamTest, Stop_DuringReset) {
   Initialize();
   EnterPendingState(DECODER_RESET);