Don't hang in Stop(): because the render thread is paused while Pipel…

…ine::Stop() runs, none of its components may depend on the render thread processing messages. In particular, the Stop() methods in DecryptingVideoDecoder and PpapiDecryptor no longer wait for their dependencies to complete. BUG=155412 Review URL: https://chromiumcodereview.appspot.com/11192011 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@162687 0039d316-1c4b-4281-b951-d872f2087c98
marcosholgado · Oct 18, 2012 · 9cea909 · 9cea909
1 parent 1194546
commit 9cea909
Show file tree

Hide file tree

Showing 8 changed files with 70 additions and 132 deletions.
diff --git a/media/filters/decrypting_video_decoder.cc b/media/filters/decrypting_video_decoder.cc
@@ -63,7 +63,6 @@ void DecryptingVideoDecoder::Reset(const base::Closure& closure) {
          state_ == kWaitingForKey ||
          state_ == kDecodeFinished) << state_;
   DCHECK(init_cb_.is_null());  // No Reset() during pending initialization.
-  DCHECK(stop_cb_.is_null());  // No Reset() during pending Stop().
   DCHECK(reset_cb_.is_null());
 
   reset_cb_ = closure;
@@ -97,55 +96,26 @@ void DecryptingVideoDecoder::Stop(const base::Closure& closure) {
   }
 
   DVLOG(2) << "Stop() - state: " << state_;
-  DCHECK(stop_cb_.is_null());
-  stop_cb_ = closure;
-
-  // We need to call Decryptor::StopVideoDecoder() if we ever called
-  // Decryptor::InitializeVideoDecoder() to cancel the pending initialization if
-  // the initialization is still pending, or to stop the video decoder if
-  // the initialization has completed.
-  // When the state is kUninitialized and kDecryptorRequested,
-  // InitializeVideoDecoder() has not been called, so we are okay.
-  // When the state is kStopped, the video decoder should have already been
-  // stopped, so no need to call StopVideoDecoder() either.
-  // In all other cases, we need to call StopVideoDecoder()!
-  switch (state_) {
-    case kUninitialized:
-    case kStopped:
-      DoStop();
-      break;
-    case kDecryptorRequested:
-      // Stop() cannot complete if the decryptor request is still pending.
-      // Defer the stopping process in this case. The |stop_cb_| will be fired
-      // after the request decryptor callback is fired - see SetDecryptor().
-      request_decryptor_notification_cb_.Run(DecryptorNotificationCB());
-      break;
-    case kIdle:
-    case kDecodeFinished:
-      decryptor_->StopVideoDecoder();
-      DoStop();
-      break;
-    case kWaitingForKey:
-      decryptor_->StopVideoDecoder();
-      DCHECK(!read_cb_.is_null());
-      pending_buffer_to_decode_ = NULL;
-      base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
-      DoStop();
-      break;
-    case kPendingDecoderInit:
-    case kPendingDemuxerRead:
-    case kPendingDecode:
-      // Stop() cannot complete if the init or read callback is still pending.
-      // Defer the stopping process in these cases. The |stop_cb_| will be
-      // fired after the init or read callback is fired - see
-      // FinishInitialization(), DoDecryptAndDecodeBuffer() and
-      // DoDeliverFrame(), respectively.
-      decryptor_->StopVideoDecoder();
-      DCHECK(!init_cb_.is_null() || !read_cb_.is_null());
-      break;
-    default:
-      NOTREACHED();
+
+  // 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 be processing messages to complete (such as PPAPI
+  // callbacks).
+  if (decryptor_)
+    decryptor_->StopVideoDecoder();
+  if (!request_decryptor_notification_cb_.is_null()) {
+    base::ResetAndReturn(&request_decryptor_notification_cb_).Run(
+        DecryptorNotificationCB());
   }
+  pending_buffer_to_decode_ = NULL;
+  if (!init_cb_.is_null())
+    base::ResetAndReturn(&init_cb_).Run(DECODER_ERROR_NOT_SUPPORTED);
+  if (!read_cb_.is_null())
+    base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
+  if (!reset_cb_.is_null())
+    base::ResetAndReturn(&reset_cb_).Run();
+  state_ = kStopped;
+  closure.Run();
 }
 
 DecryptingVideoDecoder::~DecryptingVideoDecoder() {
@@ -189,14 +159,12 @@ void DecryptingVideoDecoder::DoInitialize(
 void DecryptingVideoDecoder::SetDecryptor(Decryptor* decryptor) {
   DVLOG(2) << "SetDecryptor()";
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK_EQ(state_, kDecryptorRequested) << state_;
-  DCHECK(!init_cb_.is_null());
 
-  if (!stop_cb_.is_null()) {
-    base::ResetAndReturn(&init_cb_).Run(DECODER_ERROR_NOT_SUPPORTED);
-    DoStop();
+  if (state_ == kStopped)
     return;
-  }
+
+  DCHECK_EQ(state_, kDecryptorRequested) << state_;
+  DCHECK(!init_cb_.is_null());
 
   decryptor_ = decryptor;
 
@@ -213,17 +181,15 @@ void DecryptingVideoDecoder::SetDecryptor(Decryptor* decryptor) {
 void DecryptingVideoDecoder::FinishInitialization(bool success) {
   DVLOG(2) << "FinishInitialization()";
   DCHECK(message_loop_->BelongsToCurrentThread());
+
+  if (state_ == kStopped)
+    return;
+
   DCHECK_EQ(state_, kPendingDecoderInit) << state_;
   DCHECK(!init_cb_.is_null());
   DCHECK(reset_cb_.is_null());  // No Reset() before initialization finished.
   DCHECK(read_cb_.is_null());  // No Read() before initialization finished.
 
-  if (!stop_cb_.is_null()) {
-    base::ResetAndReturn(&init_cb_).Run(DECODER_ERROR_NOT_SUPPORTED);
-    DoStop();
-    return;
-  }
-
   if (!success) {
     base::ResetAndReturn(&init_cb_).Run(DECODER_ERROR_NOT_SUPPORTED);
     state_ = kStopped;
@@ -279,16 +245,18 @@ void DecryptingVideoDecoder::DoDecryptAndDecodeBuffer(
     const scoped_refptr<DecoderBuffer>& buffer) {
   DVLOG(3) << "DoDecryptAndDecodeBuffer()";
   DCHECK(message_loop_->BelongsToCurrentThread());
+
+  if (state_ == kStopped)
+    return;
+
   DCHECK_EQ(state_, kPendingDemuxerRead) << state_;
   DCHECK(!read_cb_.is_null());
   DCHECK_EQ(buffer != NULL, status == DemuxerStream::kOk) << status;
 
-  if (!reset_cb_.is_null() || !stop_cb_.is_null()) {
+  if (!reset_cb_.is_null()) {
     base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
     if (!reset_cb_.is_null())
       DoReset();
-    if (!stop_cb_.is_null())
-      DoStop();
     return;
   }
 
@@ -343,20 +311,22 @@ void DecryptingVideoDecoder::DoDeliverFrame(
     const scoped_refptr<VideoFrame>& frame) {
   DVLOG(3) << "DoDeliverFrame()";
   DCHECK(message_loop_->BelongsToCurrentThread());
+
+  if (state_ == kStopped)
+    return;
+
   DCHECK_EQ(state_, kPendingDecode) << state_;
   DCHECK(!read_cb_.is_null());
   DCHECK(pending_buffer_to_decode_);
 
   bool need_to_try_again_if_nokey_is_returned = key_added_while_pending_decode_;
   key_added_while_pending_decode_ = false;
 
-  if (!reset_cb_.is_null() || !stop_cb_.is_null()) {
+  if (!reset_cb_.is_null()) {
     pending_buffer_to_decode_ = NULL;
     base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
     if (!reset_cb_.is_null())
       DoReset();
-    if (!stop_cb_.is_null())
-      DoStop();
     return;
   }
 
@@ -420,11 +390,4 @@ void DecryptingVideoDecoder::DoReset() {
   base::ResetAndReturn(&reset_cb_).Run();
 }
 
-void DecryptingVideoDecoder::DoStop() {
-  DCHECK(init_cb_.is_null());
-  DCHECK(read_cb_.is_null());
-  state_ = kStopped;
-  base::ResetAndReturn(&stop_cb_).Run();
-}
-
 }  // namespace media
diff --git a/media/filters/decrypting_video_decoder.h b/media/filters/decrypting_video_decoder.h
@@ -136,7 +136,6 @@ class MEDIA_EXPORT DecryptingVideoDecoder : public VideoDecoder {
   StatisticsCB statistics_cb_;
   ReadCB read_cb_;
   base::Closure reset_cb_;
-  base::Closure stop_cb_;
 
   // Pointer to the demuxer stream that will feed us compressed buffers.
   scoped_refptr<DemuxerStream> demuxer_stream_;

diff --git a/media/filters/decrypting_video_decoder_unittest.cc b/media/filters/decrypting_video_decoder_unittest.cc
@@ -56,7 +56,8 @@ ACTION(ReturnConfigChanged) {
 }
 
 ACTION_P(RunCallback0, param) {
-  arg0.Run(param);
+  if (!arg0.is_null())
+    arg0.Run(param);
 }
 
 ACTION_P(RunCallback1, param) {

diff --git a/media/filters/ffmpeg_video_decoder.cc b/media/filters/ffmpeg_video_decoder.cc
@@ -208,29 +208,20 @@ void FFmpegVideoDecoder::Stop(const base::Closure& closure) {
     return;
   }
 
-  DCHECK(stop_cb_.is_null());
-
   if (state_ == kUninitialized) {
     closure.Run();
     return;
   }
 
-  stop_cb_ = closure;
-
   if (decryptor_)
     decryptor_->CancelDecrypt();
 
-  // Defer stopping if a read is pending.
   if (!read_cb_.is_null())
-    return;
-
-  DoStop();
-}
+    base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
 
-void FFmpegVideoDecoder::DoStop() {
   ReleaseFFmpegResources();
   state_ = kUninitialized;
-  base::ResetAndReturn(&stop_cb_).Run();
+  closure.Run();
 }
 
 FFmpegVideoDecoder::~FFmpegVideoDecoder() {
@@ -278,15 +269,12 @@ void FFmpegVideoDecoder::DoDecryptOrDecodeBuffer(
     DemuxerStream::Status status,
     const scoped_refptr<DecoderBuffer>& buffer) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK_NE(state_, kUninitialized);
   DCHECK_NE(state_, kDecodeFinished);
-  DCHECK(!read_cb_.is_null());
 
-  if (!stop_cb_.is_null()) {
-    base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
-    DoStop();
+  if (state_ == kUninitialized)
     return;
-  }
+
+  DCHECK(!read_cb_.is_null());
 
   if (!reset_cb_.is_null()) {
     base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
@@ -331,15 +319,12 @@ void FFmpegVideoDecoder::DoBufferDecrypted(
     Decryptor::Status decrypt_status,
     const scoped_refptr<DecoderBuffer>& buffer) {
   DCHECK(message_loop_->BelongsToCurrentThread());
-  DCHECK_NE(state_, kUninitialized);
   DCHECK_NE(state_, kDecodeFinished);
-  DCHECK(!read_cb_.is_null());
 
-  if (!stop_cb_.is_null()) {
-    base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
-    DoStop();
+  if (state_ == kUninitialized)
     return;
-  }
+
+  DCHECK(!read_cb_.is_null());
 
   if (!reset_cb_.is_null()) {
     base::ResetAndReturn(&read_cb_).Run(kOk, NULL);

diff --git a/media/filters/ffmpeg_video_decoder.h b/media/filters/ffmpeg_video_decoder.h
@@ -91,9 +91,6 @@ class MEDIA_EXPORT FFmpegVideoDecoder : public VideoDecoder {
   // Reset decoder and call |reset_cb_|.
   void DoReset();
 
-  // Free decoder resources and call |stop_cb_|.
-  void DoStop();
-
   // This is !is_null() iff Initialize() hasn't been called.
   MessageLoopFactoryCB message_loop_factory_cb_;
 
@@ -105,7 +102,6 @@ class MEDIA_EXPORT FFmpegVideoDecoder : public VideoDecoder {
 
   ReadCB read_cb_;
   base::Closure reset_cb_;
-  base::Closure stop_cb_;
 
   // FFmpeg structures owned by this object.
   AVCodecContext* codec_context_;

diff --git a/media/filters/ffmpeg_video_decoder_unittest.cc b/media/filters/ffmpeg_video_decoder_unittest.cc
@@ -648,10 +648,10 @@ TEST_F(FFmpegVideoDecoderTest, Stop_DuringPendingRead) {
   // Make sure the Read() on the decoder triggers a Read() on the demuxer.
   EXPECT_FALSE(read_cb.is_null());
 
-  Stop();
-
   EXPECT_CALL(*this, FrameReady(VideoDecoder::kOk, IsNull()));
 
+  Stop();
+
   read_cb.Run(DemuxerStream::kOk, i_frame_buffer_);
   message_loop_.RunAllPending();
 }

diff --git a/webkit/media/crypto/ppapi_decryptor.cc b/webkit/media/crypto/ppapi_decryptor.cc
@@ -26,7 +26,9 @@ PpapiDecryptor::PpapiDecryptor(
   const scoped_refptr<webkit::ppapi::PluginInstance>& plugin_instance)
     : client_(client),
       cdm_plugin_(plugin_instance),
-      render_loop_proxy_(base::MessageLoopProxy::current()) {
+      render_loop_proxy_(base::MessageLoopProxy::current()),
+      weak_ptr_factory_(this),
+      weak_this_(weak_ptr_factory_.GetWeakPtr()) {
   DCHECK(client_);
   DCHECK(cdm_plugin_);
   cdm_plugin_->set_decrypt_client(client);
@@ -87,16 +89,12 @@ void PpapiDecryptor::CancelKeyRequest(const std::string& key_system,
     ReportFailureToCallPlugin(key_system, session_id);
 }
 
-// TODO(xhwang): Remove Unretained in the following methods.
-
 void PpapiDecryptor::Decrypt(
     const scoped_refptr<media::DecoderBuffer>& encrypted,
     const DecryptCB& decrypt_cb) {
   if (!render_loop_proxy_->BelongsToCurrentThread()) {
-    render_loop_proxy_->PostTask(
-        FROM_HERE,
-        base::Bind(&PpapiDecryptor::Decrypt, base::Unretained(this),
-                   encrypted, decrypt_cb));
+    render_loop_proxy_->PostTask(FROM_HERE, base::Bind(
+        &PpapiDecryptor::Decrypt, weak_this_, encrypted, decrypt_cb));
     return;
   }
 
@@ -115,11 +113,9 @@ void PpapiDecryptor::InitializeVideoDecoder(
     const DecoderInitCB& init_cb,
     const KeyAddedCB& key_added_cb) {
   if (!render_loop_proxy_->BelongsToCurrentThread()) {
-    render_loop_proxy_->PostTask(
-        FROM_HERE,
-        base::Bind(&PpapiDecryptor::InitializeVideoDecoder,
-                   base::Unretained(this), base::Passed(&config),
-                   init_cb, key_added_cb));
+    render_loop_proxy_->PostTask(FROM_HERE, base::Bind(
+        &PpapiDecryptor::InitializeVideoDecoder, weak_this_,
+        base::Passed(&config), init_cb, key_added_cb));
     return;
   }
 
@@ -132,8 +128,7 @@ void PpapiDecryptor::InitializeVideoDecoder(
 
   if (!cdm_plugin_->InitializeVideoDecoder(
       *config,
-      base::Bind(&PpapiDecryptor::OnVideoDecoderInitialized,
-                 base::Unretained(this)))) {
+      base::Bind(&PpapiDecryptor::OnVideoDecoderInitialized, weak_this_))) {
     key_added_cb_.Reset();
     base::ResetAndReturn(&video_decoder_init_cb_).Run(false);
     return;
@@ -144,10 +139,9 @@ void PpapiDecryptor::DecryptAndDecodeVideo(
     const scoped_refptr<media::DecoderBuffer>& encrypted,
     const VideoDecodeCB& video_decode_cb) {
   if (!render_loop_proxy_->BelongsToCurrentThread()) {
-    render_loop_proxy_->PostTask(
-        FROM_HERE,
-        base::Bind(&PpapiDecryptor::DecryptAndDecodeVideo,
-                   base::Unretained(this), encrypted, video_decode_cb));
+    render_loop_proxy_->PostTask(FROM_HERE, base::Bind(
+        &PpapiDecryptor::DecryptAndDecodeVideo, weak_this_,
+        encrypted, video_decode_cb));
     return;
   }
 
@@ -158,10 +152,8 @@ void PpapiDecryptor::DecryptAndDecodeVideo(
 
 void PpapiDecryptor::CancelDecryptAndDecodeVideo() {
   if (!render_loop_proxy_->BelongsToCurrentThread()) {
-    render_loop_proxy_->PostTask(
-        FROM_HERE,
-        base::Bind(&PpapiDecryptor::CancelDecryptAndDecodeVideo,
-                   base::Unretained(this)));
+    render_loop_proxy_->PostTask(FROM_HERE, base::Bind(
+        &PpapiDecryptor::CancelDecryptAndDecodeVideo, weak_this_));
     return;
   }
 
@@ -171,12 +163,10 @@ void PpapiDecryptor::CancelDecryptAndDecodeVideo() {
 
 void PpapiDecryptor::StopVideoDecoder() {
   if (!render_loop_proxy_->BelongsToCurrentThread()) {
-    render_loop_proxy_->PostTask(
-        FROM_HERE,
-        base::Bind(&PpapiDecryptor::StopVideoDecoder, base::Unretained(this)));
+    render_loop_proxy_->PostTask(FROM_HERE, base::Bind(
+        &PpapiDecryptor::StopVideoDecoder, weak_this_));
     return;
   }
-
   DVLOG(2) << "StopVideoDecoder()";
   cdm_plugin_->DeinitializeDecoder();
 }