Add DecryptingDemuxerStream::Stop().

During the teardown process, media pipeline will be waiting on the
render main thread. If a Decryptor depends on the render main thread
(e.g. PpapiDecryptor), the pending DecryptCB will not be satisfied.

This CL adds a DecryptingDemuxerStream::Stop() method so that the
stopping process doesn't wait for any pending operation.

BUG=343748
TEST=Add unittests to cover this case.
R=ddorwin@chromium.org

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

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

media/filters/decoder_selector.cc

@@ -130,7 +130,7 @@ void DecoderSelector<StreamType>::Abort() {
   }
 
   if (decrypted_stream_) {
-    decrypted_stream_->Reset(
+    decrypted_stream_->Stop(
         base::Bind(&DecoderSelector<StreamType>::ReturnNullDecoder,
                    weak_ptr_factory_.GetWeakPtr()));
     return;

media/filters/decrypting_demuxer_stream.cc

@@ -75,10 +75,13 @@ void DecryptingDemuxerStream::Reset(const base::Closure& closure) {
   DVLOG(2) << __FUNCTION__ << " - state: " << state_;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(state_ != kUninitialized) << state_;
+  DCHECK(state_ != kStopped) << state_;
   DCHECK(reset_cb_.is_null());
 
   reset_cb_ = BindToCurrentLoop(closure);
 
+  // TODO(xhwang): This should not happen. Remove it, DCHECK against the
+  // condition and clean up related tests.
   if (state_ == kDecryptorRequested) {
     DCHECK(!init_cb_.is_null());
     set_decryptor_ready_cb_.Run(DecryptorReadyCB());
@@ -108,6 +111,38 @@ void DecryptingDemuxerStream::Reset(const base::Closure& closure) {
   DoReset();
 }
 
+void DecryptingDemuxerStream::Stop(const base::Closure& closure) {
+  DVLOG(2) << __FUNCTION__ << " - state: " << state_;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK(state_ != kUninitialized) << state_;
+
+  // Invalidate all weak pointers so that pending callbacks won't fire.
+  weak_factory_.InvalidateWeakPtrs();
+
+  // At this point the render thread is likely paused (in WebMediaPlayerImpl's
+  // Destroy()), so running |closure| can't wait for anything that requires the
+  // render thread to process messages to complete (such as PPAPI methods).
+  if (decryptor_) {
+    // Clear the callback.
+    decryptor_->RegisterNewKeyCB(GetDecryptorStreamType(),
+                                 Decryptor::NewKeyCB());
+    decryptor_->CancelDecrypt(GetDecryptorStreamType());
+    decryptor_ = NULL;
+  }
+  if (!set_decryptor_ready_cb_.is_null())
+    base::ResetAndReturn(&set_decryptor_ready_cb_).Run(DecryptorReadyCB());
+  if (!init_cb_.is_null())
+    base::ResetAndReturn(&init_cb_).Run(PIPELINE_ERROR_ABORT);
+  if (!read_cb_.is_null())
+    base::ResetAndReturn(&read_cb_).Run(kAborted, NULL);
+  if (!reset_cb_.is_null())
+    base::ResetAndReturn(&reset_cb_).Run();
+  pending_buffer_to_decrypt_ = NULL;
+
+  state_ = kStopped;
+  BindToCurrentLoop(closure).Run();
+}
+
 AudioDecoderConfig DecryptingDemuxerStream::audio_decoder_config() {
   DCHECK(state_ != kUninitialized && state_ != kDecryptorRequested) << state_;
   CHECK_EQ(demuxer_stream_->type(), AUDIO);

media/filters/decrypting_demuxer_stream.h

@@ -36,11 +36,19 @@ class MEDIA_EXPORT DecryptingDemuxerStream : public DemuxerStream {
   void Initialize(DemuxerStream* stream,
                   const PipelineStatusCB& status_cb);
 
-  // Cancels all pending operations and fires all pending callbacks.  Sets
-  // |this| to kUninitialized state if |this| hasn't been initialized, or to
-  // kIdle state otherwise.
+  // Cancels all pending operations and fires all pending callbacks. If in
+  // kPendingDemuxerRead or kPendingDecrypt state, waits for the pending
+  // operation to finish before satisfying |closure|. Sets the state to
+  // kUninitialized if |this| hasn't been initialized, or to kIdle otherwise.
   void Reset(const base::Closure& closure);
 
+  // Cancels all pending operations immediately and fires all pending callbacks
+  // and sets the state to kStopped. Does NOT wait for any pending operations.
+  // Note: During the teardown process, media pipeline will be waiting on the
+  // render main thread. If a Decryptor depends on the render main thread
+  // (e.g. PpapiDecryptor), the pending DecryptCB would not be satisfied.
+  void Stop(const base::Closure& closure);
+
   // DemuxerStream implementation.
   virtual void Read(const ReadCB& read_cb) OVERRIDE;
   virtual AudioDecoderConfig audio_decoder_config() OVERRIDE;
@@ -60,6 +68,7 @@ class MEDIA_EXPORT DecryptingDemuxerStream : public DemuxerStream {
     kPendingDemuxerRead,
     kPendingDecrypt,
     kWaitingForKey,
+    kStopped
   };
 
   // Callback for DecryptorHost::RequestDecryptor().

media/filters/decrypting_demuxer_stream_unittest.cc

@@ -50,9 +50,11 @@ ACTION_P(ReturnBuffer, buffer) {
   arg0.Run(buffer.get() ? DemuxerStream::kOk : DemuxerStream::kAborted, buffer);
 }
 
-ACTION_P(RunCallbackIfNotNull, param) {
+ACTION_P2(SetDecryptorIfNotNull, decryptor, is_decryptor_set) {
   if (!arg0.is_null())
-    arg0.Run(param);
+    arg0.Run(decryptor);
+
+  *is_decryptor_set = !arg0.is_null() && decryptor;
 }
 
 ACTION_P2(ResetAndRunCallback, callback, param) {
@@ -74,6 +76,7 @@ class DecryptingDemuxerStreamTest : public testing::Test {
                 &DecryptingDemuxerStreamTest::RequestDecryptorNotification,
                 base::Unretained(this)))),
         decryptor_(new StrictMock<MockDecryptor>()),
+        is_decryptor_set_(false),
         input_audio_stream_(
             new StrictMock<MockDemuxerStream>(DemuxerStream::AUDIO)),
         input_video_stream_(
@@ -106,9 +109,9 @@ class DecryptingDemuxerStreamTest : public testing::Test {
 
   void Initialize() {
     EXPECT_CALL(*this, RequestDecryptorNotification(_))
-        .WillOnce(RunCallbackIfNotNull(decryptor_.get()));
+        .WillOnce(SetDecryptorIfNotNull(decryptor_.get(), &is_decryptor_set_));
     EXPECT_CALL(*decryptor_, RegisterNewKeyCB(Decryptor::kAudio, _))
-        .WillOnce(SaveArg<1>(&key_added_cb_));
+        .WillRepeatedly(SaveArg<1>(&key_added_cb_));
 
     AudioDecoderConfig input_config(
         kCodecVorbis, kSampleFormatPlanarF32, CHANNEL_LAYOUT_STEREO, 44100,
@@ -221,14 +224,25 @@ class DecryptingDemuxerStreamTest : public testing::Test {
   }
 
   void Reset() {
-    EXPECT_CALL(*decryptor_, CancelDecrypt(Decryptor::kAudio))
-        .WillRepeatedly(InvokeWithoutArgs(
-            this, &DecryptingDemuxerStreamTest::AbortPendingDecryptCB));
+    if (is_decryptor_set_) {
+      EXPECT_CALL(*decryptor_, CancelDecrypt(Decryptor::kAudio))
+          .WillRepeatedly(InvokeWithoutArgs(
+              this, &DecryptingDemuxerStreamTest::AbortPendingDecryptCB));
+    }
 
     demuxer_stream_->Reset(NewExpectedClosure());
     message_loop_.RunUntilIdle();
   }
 
+  // Stops the |demuxer_stream_| without satisfying/aborting any pending
+  // operations.
+  void Stop() {
+    if (is_decryptor_set_)
+      EXPECT_CALL(*decryptor_, CancelDecrypt(Decryptor::kAudio));
+    demuxer_stream_->Stop(NewExpectedClosure());
+    message_loop_.RunUntilIdle();
+  }
+
   MOCK_METHOD1(RequestDecryptorNotification, void(const DecryptorReadyCB&));
 
   MOCK_METHOD2(BufferReady, void(DemuxerStream::Status,
@@ -237,6 +251,8 @@ class DecryptingDemuxerStreamTest : public testing::Test {
   base::MessageLoop message_loop_;
   scoped_ptr<DecryptingDemuxerStream> demuxer_stream_;
   scoped_ptr<StrictMock<MockDecryptor> > decryptor_;
+  // Whether a valid Decryptor is set to the |demuxer_stream_|.
+  bool is_decryptor_set_;
   scoped_ptr<StrictMock<MockDemuxerStream> > input_audio_stream_;
   scoped_ptr<StrictMock<MockDemuxerStream> > input_video_stream_;
 
@@ -260,9 +276,9 @@ TEST_F(DecryptingDemuxerStreamTest, Initialize_NormalAudio) {
 
 TEST_F(DecryptingDemuxerStreamTest, Initialize_NormalVideo) {
   EXPECT_CALL(*this, RequestDecryptorNotification(_))
-      .WillOnce(RunCallbackIfNotNull(decryptor_.get()));
+      .WillOnce(SetDecryptorIfNotNull(decryptor_.get(), &is_decryptor_set_));
   EXPECT_CALL(*decryptor_, RegisterNewKeyCB(Decryptor::kVideo, _))
-        .WillOnce(SaveArg<1>(&key_added_cb_));
+      .WillOnce(SaveArg<1>(&key_added_cb_));
 
   VideoDecoderConfig input_config = TestVideoConfig::NormalEncrypted();
   InitializeVideoAndExpectStatus(input_config, PIPELINE_OK);
@@ -287,7 +303,8 @@ TEST_F(DecryptingDemuxerStreamTest, Initialize_NormalVideo) {
 
 TEST_F(DecryptingDemuxerStreamTest, Initialize_NullDecryptor) {
   EXPECT_CALL(*this, RequestDecryptorNotification(_))
-      .WillRepeatedly(RunCallbackIfNotNull(static_cast<Decryptor*>(NULL)));
+      .WillRepeatedly(SetDecryptorIfNotNull(static_cast<Decryptor*>(NULL),
+                                            &is_decryptor_set_));
 
   AudioDecoderConfig input_config(kCodecVorbis, kSampleFormatPlanarF32,
                                   CHANNEL_LAYOUT_STEREO, 44100, NULL, 0, true);
@@ -467,7 +484,7 @@ TEST_F(DecryptingDemuxerStreamTest, DemuxerRead_ConfigChanged) {
   ReadAndExpectBufferReadyWith(DemuxerStream::kConfigChanged, NULL);
 }
 
-// Test resetting when DecryptingDemuxerStream is waiting for an config changed
+// Test resetting when DecryptingDemuxerStream is waiting for a config changed
 // read.
 TEST_F(DecryptingDemuxerStreamTest, Reset_DuringConfigChangedDemuxerRead) {
   Initialize();
@@ -481,4 +498,67 @@ TEST_F(DecryptingDemuxerStreamTest, Reset_DuringConfigChangedDemuxerRead) {
   message_loop_.RunUntilIdle();
 }
 
+// Test stopping when the DecryptingDemuxerStream is in kDecryptorRequested
+// state.
+TEST_F(DecryptingDemuxerStreamTest, Stop_DuringDecryptorRequested) {
+  // One for decryptor request, one for canceling request during Reset().
+  EXPECT_CALL(*this, RequestDecryptorNotification(_))
+      .Times(2);
+  AudioDecoderConfig input_config(
+      kCodecVorbis, kSampleFormatPlanarF32, CHANNEL_LAYOUT_STEREO, 44100,
+      NULL, 0, true);
+  InitializeAudioAndExpectStatus(input_config, PIPELINE_ERROR_ABORT);
+  Stop();
+}
+
+// Test stopping when the DecryptingDemuxerStream is in kIdle state but has
+// not returned any buffer.
+TEST_F(DecryptingDemuxerStreamTest, Stop_DuringIdleAfterInitialization) {
+  Initialize();
+  Stop();
+}
+
+// Test stopping when the DecryptingDemuxerStream is in kIdle state after it
+// has returned one buffer.
+TEST_F(DecryptingDemuxerStreamTest, Stop_DuringIdleAfterReadOneBuffer) {
+  Initialize();
+  EnterNormalReadingState();
+  Stop();
+}
+
+// Test stopping when DecryptingDemuxerStream is in kPendingDemuxerRead state.
+TEST_F(DecryptingDemuxerStreamTest, Stop_DuringPendingDemuxerRead) {
+  Initialize();
+  EnterPendingReadState();
+
+  EXPECT_CALL(*this, BufferReady(DemuxerStream::kAborted, IsNull()));
+  Stop();
+}
+
+// Test stopping when the DecryptingDemuxerStream is in kPendingDecrypt state.
+TEST_F(DecryptingDemuxerStreamTest, Stop_DuringPendingDecrypt) {
+  Initialize();
+  EnterPendingDecryptState();
+
+  EXPECT_CALL(*this, BufferReady(DemuxerStream::kAborted, IsNull()));
+  Stop();
+}
+
+// Test stopping when the DecryptingDemuxerStream is in kWaitingForKey state.
+TEST_F(DecryptingDemuxerStreamTest, Stop_DuringWaitingForKey) {
+  Initialize();
+  EnterWaitingForKeyState();
+
+  EXPECT_CALL(*this, BufferReady(DemuxerStream::kAborted, IsNull()));
+  Stop();
+}
+
+// Test stopping after the DecryptingDemuxerStream has been reset.
+TEST_F(DecryptingDemuxerStreamTest, Stop_AfterReset) {
+  Initialize();
+  EnterNormalReadingState();
+  Reset();
+  Stop();
+}
+
 }  // namespace media

media/filters/video_frame_stream.cc

@@ -145,7 +145,7 @@ void VideoFrameStream::Stop(const base::Closure& closure) {
     task_runner_->PostTask(FROM_HERE, base::ResetAndReturn(&reset_cb_));
 
   if (decrypting_demuxer_stream_) {
-    decrypting_demuxer_stream_->Reset(base::Bind(
+    decrypting_demuxer_stream_->Stop(base::Bind(
         &VideoFrameStream::StopDecoder, weak_factory_.GetWeakPtr()));
     return;
   }