From 52f4c505c831ba83ebd07a23f57c8e7ff2ae4661 Mon Sep 17 00:00:00 2001 From: "xhwang@chromium.org" Date: Tue, 18 Feb 2014 21:00:56 +0000 Subject: Invalidate all weak pointers during Decrypting{Audio|Video}Decoder::Stop(). This is a clean up CL following r251277. During media pipeline shutdown, WebMediaPlayerImpl::Destroy() is blocking the main render thread to wait for the media filters to be stopped correctly. As a result, Decrypting{Audio|Video}Decoder::Stop() cannot wait for the Decryptor to finish all pending operations because some Decryptors rely on the main render thread to work. Instead, Decrypting{Audio|Video}Decoder::Stop() clears its internal data, fires all pending callbacks, sets it's state to kStopped and fires the stop callback immediately. However, after this process, there may still be callbacks outstanding on the Decrypting{Audio|Video}Decoder object. Previously we check the condition "state_ == kStopped" in all callback functions to turn those callbacks into a no-op. In this CL, we invalidate all weak pointers during the Stop() process so that no pending callbacks will ever be fired back, which is cleaner and safer. BUG=343748 TEST=Existing tests still pass. Review URL: https://codereview.chromium.org/167523004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@251837 0039d316-1c4b-4281-b951-d872f2087c98 --- media/filters/decrypting_audio_decoder.cc | 17 +++++------------ media/filters/decrypting_video_decoder.cc | 22 ++++++++-------------- 2 files changed, 13 insertions(+), 26 deletions(-) diff --git a/media/filters/decrypting_audio_decoder.cc b/media/filters/decrypting_audio_decoder.cc index 6a2dd3b..b311446 100644 --- a/media/filters/decrypting_audio_decoder.cc +++ b/media/filters/decrypting_audio_decoder.cc @@ -149,6 +149,10 @@ void DecryptingAudioDecoder::Stop(const base::Closure& closure) { DVLOG(2) << "Stop() - state: " << state_; DCHECK(task_runner_->BelongsToCurrentThread()); + // Invalidate all weak pointers so that pending callbacks won't be fired into + // this object. + weak_factory_.InvalidateWeakPtrs(); + if (decryptor_) { decryptor_->RegisterNewKeyCB(Decryptor::kAudio, Decryptor::NewKeyCB()); decryptor_->DeinitializeDecoder(Decryptor::kAudio); @@ -163,6 +167,7 @@ void DecryptingAudioDecoder::Stop(const base::Closure& closure) { base::ResetAndReturn(&read_cb_).Run(kAborted, NULL); if (!reset_cb_.is_null()) base::ResetAndReturn(&reset_cb_).Run(); + state_ = kStopped; task_runner_->PostTask(FROM_HERE, closure); } @@ -189,10 +194,6 @@ DecryptingAudioDecoder::~DecryptingAudioDecoder() { void DecryptingAudioDecoder::SetDecryptor(Decryptor* decryptor) { DVLOG(2) << "SetDecryptor()"; DCHECK(task_runner_->BelongsToCurrentThread()); - - if (state_ == kStopped) - return; - DCHECK_EQ(state_, kDecryptorRequested) << state_; DCHECK(!init_cb_.is_null()); DCHECK(!set_decryptor_ready_cb_.is_null()); @@ -232,10 +233,6 @@ void DecryptingAudioDecoder::SetDecryptor(Decryptor* decryptor) { void DecryptingAudioDecoder::FinishInitialization(bool success) { DVLOG(2) << "FinishInitialization()"; DCHECK(task_runner_->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. @@ -375,10 +372,6 @@ void DecryptingAudioDecoder::DeliverFrame( const Decryptor::AudioBuffers& frames) { DVLOG(3) << "DeliverFrame() - status: " << status; DCHECK(task_runner_->BelongsToCurrentThread()); - - if (state_ == kStopped) - return; - DCHECK_EQ(state_, kPendingDecode) << state_; DCHECK(!read_cb_.is_null()); DCHECK(pending_buffer_to_decode_.get()); diff --git a/media/filters/decrypting_video_decoder.cc b/media/filters/decrypting_video_decoder.cc index bae5ab1..35fb709 100644 --- a/media/filters/decrypting_video_decoder.cc +++ b/media/filters/decrypting_video_decoder.cc @@ -127,6 +127,10 @@ void DecryptingVideoDecoder::Stop(const base::Closure& closure) { DCHECK(task_runner_->BelongsToCurrentThread()); DVLOG(2) << "Stop() - state: " << state_; + // Invalidate all weak pointers so that pending callbacks won't be fired into + // this object. + 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 be processing messages to complete (such as PPAPI @@ -145,6 +149,7 @@ void DecryptingVideoDecoder::Stop(const base::Closure& closure) { base::ResetAndReturn(&decode_cb_).Run(kAborted, NULL); if (!reset_cb_.is_null()) base::ResetAndReturn(&reset_cb_).Run(); + state_ = kStopped; BindToCurrentLoop(closure).Run(); } @@ -156,10 +161,6 @@ DecryptingVideoDecoder::~DecryptingVideoDecoder() { void DecryptingVideoDecoder::SetDecryptor(Decryptor* decryptor) { DVLOG(2) << "SetDecryptor()"; DCHECK(task_runner_->BelongsToCurrentThread()); - - if (state_ == kStopped) - return; - DCHECK_EQ(state_, kDecryptorRequested) << state_; DCHECK(!init_cb_.is_null()); DCHECK(!set_decryptor_ready_cb_.is_null()); @@ -183,10 +184,6 @@ void DecryptingVideoDecoder::SetDecryptor(Decryptor* decryptor) { void DecryptingVideoDecoder::FinishInitialization(bool success) { DVLOG(2) << "FinishInitialization()"; DCHECK(task_runner_->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. @@ -229,16 +226,13 @@ void DecryptingVideoDecoder::DeliverFrame( const scoped_refptr& frame) { DVLOG(3) << "DeliverFrame() - status: " << status; DCHECK(task_runner_->BelongsToCurrentThread()); - TRACE_EVENT_ASYNC_END0( - "media", "DecryptingVideoDecoder::DecodePendingBuffer", trace_id_); - - if (state_ == kStopped) - return; - DCHECK_EQ(state_, kPendingDecode) << state_; DCHECK(!decode_cb_.is_null()); DCHECK(pending_buffer_to_decode_.get()); + TRACE_EVENT_ASYNC_END0( + "media", "DecryptingVideoDecoder::DecodePendingBuffer", trace_id_); + bool need_to_try_again_if_nokey_is_returned = key_added_while_decode_pending_; key_added_while_decode_pending_ = false; -- cgit v1.1