Chromecast: mitigate threading issue between AvPipeline/MediaKeys.

R=lcwu@chromium.org,erickung@chromium.org
BUG=internal b/19550765

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

Cr-Commit-Position: refs/heads/master@{#318973}

6 files changed, 50 insertions, 40 deletions

diff --git a/chromecast/media/cma/pipeline/audio_pipeline_impl.cc b/chromecast/media/cma/pipeline/audio_pipeline_impl.cc
index 404031f..a524888 100644
--- a/chromecast/media/cma/pipeline/audio_pipeline_impl.cc
+++ b/chromecast/media/cma/pipeline/audio_pipeline_impl.cc
@@ -22,9 +22,9 @@ const size_t kMaxAudioFrameSize = 32 * 1024;
 AudioPipelineImpl::AudioPipelineImpl(AudioPipelineDevice* audio_device)
     : audio_device_(audio_device),
       weak_factory_(this) {
-  av_pipeline_impl_.reset(new AvPipelineImpl(
+  av_pipeline_impl_ = new AvPipelineImpl(
       audio_device_,
-      base::Bind(&AudioPipelineImpl::OnUpdateConfig, base::Unretained(this))));
+      base::Bind(&AudioPipelineImpl::OnUpdateConfig, base::Unretained(this)));
   weak_this_ = weak_factory_.GetWeakPtr();
 }
 
diff --git a/chromecast/media/cma/pipeline/audio_pipeline_impl.h b/chromecast/media/cma/pipeline/audio_pipeline_impl.h
index 7d2e63a..7451acf 100644
--- a/chromecast/media/cma/pipeline/audio_pipeline_impl.h
+++ b/chromecast/media/cma/pipeline/audio_pipeline_impl.h
@@ -62,7 +62,7 @@ class AudioPipelineImpl : public AudioPipeline {
 
   AudioPipelineDevice* audio_device_;
 
-  scoped_ptr<AvPipelineImpl> av_pipeline_impl_;
+  scoped_refptr<AvPipelineImpl> av_pipeline_impl_;
   AvPipelineClient audio_client_;
 
   ::media::PipelineStatistics previous_stats_;
diff --git a/chromecast/media/cma/pipeline/av_pipeline_impl.cc b/chromecast/media/cma/pipeline/av_pipeline_impl.cc
index 2e2da54..6c415c7 100644
--- a/chromecast/media/cma/pipeline/av_pipeline_impl.cc
+++ b/chromecast/media/cma/pipeline/av_pipeline_impl.cc
@@ -45,10 +45,8 @@ AvPipelineImpl::AvPipelineImpl(
       enable_time_update_(false),
       pending_time_update_task_(false),
       media_keys_(NULL),
-      media_keys_callback_id_(kNoCallbackId),
-      weak_factory_(this) {
+      media_keys_callback_id_(kNoCallbackId) {
   DCHECK(media_component_device);
-  weak_this_ = weak_factory_.GetWeakPtr();
   thread_checker_.DetachFromThread();
 }
 
@@ -58,8 +56,11 @@ AvPipelineImpl::~AvPipelineImpl() {
   DCHECK(thread_checker_.CalledOnValidThread());
   media_component_device_->SetClient(MediaComponentDevice::Client());
 
-  if (media_keys_ && media_keys_callback_id_ != kNoCallbackId)
-    media_keys_->UnregisterPlayer(media_keys_callback_id_);
+  {
+    base::AutoLock lock(media_keys_lock_);
+    if (media_keys_ && media_keys_callback_id_ != kNoCallbackId)
+      media_keys_->UnregisterPlayer(media_keys_callback_id_);
+  }
 }
 
 void AvPipelineImpl::TransitionToState(State state) {
@@ -80,7 +81,7 @@ void AvPipelineImpl::SetCodedFrameProvider(
           frame_provider.Pass(),
           max_buffer_size,
           max_frame_size,
-          base::Bind(&AvPipelineImpl::OnFrameBuffered, weak_this_)));
+          base::Bind(&AvPipelineImpl::OnFrameBuffered, this)));
 }
 
 void AvPipelineImpl::SetClient(const AvPipelineClient& client) {
@@ -94,7 +95,7 @@ bool AvPipelineImpl::Initialize() {
   DCHECK_EQ(state_, kUninitialized);
 
   MediaComponentDevice::Client client;
-  client.eos_cb = base::Bind(&AvPipelineImpl::OnEos, weak_this_);
+  client.eos_cb = base::Bind(&AvPipelineImpl::OnEos, this);
   media_component_device_->SetClient(client);
   if (!media_component_device_->SetState(MediaComponentDevice::kStateIdle))
     return false;
@@ -128,7 +129,7 @@ bool AvPipelineImpl::StartPlayingFrom(
   enable_feeding_ = true;
   base::MessageLoopProxy::current()->PostTask(
       FROM_HERE,
-      base::Bind(&AvPipelineImpl::FetchBufferIfNeeded, weak_this_));
+      base::Bind(&AvPipelineImpl::FetchBufferIfNeeded, this));
 
   return true;
 }
@@ -183,15 +184,21 @@ void AvPipelineImpl::SetCdm(BrowserCdmCast* media_keys) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(media_keys);
 
-  if (media_keys_ && media_keys_callback_id_ != kNoCallbackId)
-    media_keys_->UnregisterPlayer(media_keys_callback_id_);
-
-  media_keys_ = media_keys;
-  media_keys_callback_id_ = media_keys_->RegisterPlayer(
-      ::media::BindToCurrentLoop(
-          base::Bind(&AvPipelineImpl::OnCdmStateChanged, weak_this_)),
-      ::media::BindToCurrentLoop(
-          base::Bind(&AvPipelineImpl::OnCdmDestroyed, weak_this_)));
+  {
+    base::AutoLock lock(media_keys_lock_);
+    if (media_keys_ && media_keys_callback_id_ != kNoCallbackId)
+      media_keys_->UnregisterPlayer(media_keys_callback_id_);
+
+    media_keys_ = media_keys;
+    media_keys_callback_id_ = media_keys_->RegisterPlayer(
+        ::media::BindToCurrentLoop(
+            base::Bind(&AvPipelineImpl::OnCdmStateChanged, this)),
+        // Note: this callback gets invoked in ~BrowserCdmCast. Posting
+        // OnCdmDestroyed to the media thread results in a race condition
+        // with media_keys_ accesses. This callback must run synchronously,
+        // otherwise media_keys_ access might occur after it is deleted.
+        base::Bind(&AvPipelineImpl::OnCdmDestroyed, this));
+  }
 }
 
 void AvPipelineImpl::OnEos() {
@@ -214,7 +221,7 @@ void AvPipelineImpl::FetchBufferIfNeeded() {
 
   pending_read_ = true;
   frame_provider_->Read(
-      base::Bind(&AvPipelineImpl::OnNewFrame, weak_this_));
+      base::Bind(&AvPipelineImpl::OnNewFrame, this));
 }
 
 void AvPipelineImpl::OnNewFrame(
@@ -232,7 +239,7 @@ void AvPipelineImpl::OnNewFrame(
 
   base::MessageLoopProxy::current()->PostTask(
       FROM_HERE,
-      base::Bind(&AvPipelineImpl::FetchBufferIfNeeded, weak_this_));
+      base::Bind(&AvPipelineImpl::FetchBufferIfNeeded, this));
 }
 
 void AvPipelineImpl::ProcessPendingBuffer() {
@@ -243,7 +250,7 @@ void AvPipelineImpl::ProcessPendingBuffer() {
   if (!pending_buffer_.get() && !pending_read_) {
     base::MessageLoopProxy::current()->PostTask(
         FROM_HERE,
-        base::Bind(&AvPipelineImpl::FetchBufferIfNeeded, weak_this_));
+        base::Bind(&AvPipelineImpl::FetchBufferIfNeeded, this));
     return;
   }
 
@@ -262,12 +269,15 @@ void AvPipelineImpl::ProcessPendingBuffer() {
     // Verify that CDM has the key ID.
     // Should not send the frame if the key ID is not available yet.
     std::string key_id(pending_buffer_->decrypt_config()->key_id());
-    if (!media_keys_) {
-      CMALOG(kLogControl) << "No CDM for frame: pts="
-                          << pending_buffer_->timestamp().InMilliseconds();
-      return;
+    {
+      base::AutoLock lock(media_keys_lock_);
+      if (!media_keys_) {
+        CMALOG(kLogControl) << "No CDM for frame: pts="
+                            << pending_buffer_->timestamp().InMilliseconds();
+        return;
+      }
+      decrypt_context = media_keys_->GetDecryptContext(key_id);
     }
-    decrypt_context = media_keys_->GetDecryptContext(key_id);
     if (!decrypt_context.get()) {
       CMALOG(kLogControl) << "frame(pts="
                           << pending_buffer_->timestamp().InMilliseconds()
@@ -294,7 +304,7 @@ void AvPipelineImpl::ProcessPendingBuffer() {
   MediaComponentDevice::FrameStatus status = media_component_device_->PushFrame(
       decrypt_context,
       pending_buffer_,
-      base::Bind(&AvPipelineImpl::OnFramePushed, weak_this_));
+      base::Bind(&AvPipelineImpl::OnFramePushed, this));
   pending_buffer_ = scoped_refptr<DecoderBufferBase>();
 
   pending_push_ = (status == MediaComponentDevice::kFramePending);
@@ -313,7 +323,7 @@ void AvPipelineImpl::OnFramePushed(MediaComponentDevice::FrameStatus status) {
   }
   base::MessageLoopProxy::current()->PostTask(
       FROM_HERE,
-      base::Bind(&AvPipelineImpl::ProcessPendingBuffer, weak_this_));
+      base::Bind(&AvPipelineImpl::ProcessPendingBuffer, this));
 }
 
 void AvPipelineImpl::OnCdmStateChanged() {
@@ -328,7 +338,7 @@ void AvPipelineImpl::OnCdmStateChanged() {
 }
 
 void AvPipelineImpl::OnCdmDestroyed() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  base::AutoLock lock(media_keys_lock_);
   media_keys_ = NULL;
 }
 
diff --git a/chromecast/media/cma/pipeline/av_pipeline_impl.h b/chromecast/media/cma/pipeline/av_pipeline_impl.h
index 20b3fc4..19b8498 100644
--- a/chromecast/media/cma/pipeline/av_pipeline_impl.h
+++ b/chromecast/media/cma/pipeline/av_pipeline_impl.h
@@ -10,7 +10,7 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/memory/weak_ptr.h"
+#include "base/synchronization/lock.h"
 #include "base/threading/thread_checker.h"
 #include "chromecast/media/cma/backend/media_component_device.h"
 #include "chromecast/media/cma/pipeline/av_pipeline_client.h"
@@ -29,7 +29,7 @@ class CodedFrameProvider;
 class DecoderBufferBase;
 class MediaComponentDevice;
 
-class AvPipelineImpl {
+class AvPipelineImpl : public base::RefCountedThreadSafe<AvPipelineImpl> {
  public:
   // Pipeline states.
   enum State {
@@ -48,7 +48,6 @@ class AvPipelineImpl {
   AvPipelineImpl(
       MediaComponentDevice* media_component_device,
       const UpdateConfigCB& update_config_cb);
-  ~AvPipelineImpl();
 
   // Setting the frame provider or the client must be done in the
   // |kUninitialized| state.
@@ -78,6 +77,9 @@ class AvPipelineImpl {
   void SetCdm(BrowserCdmCast* media_keys);
 
  private:
+  friend class base::RefCountedThreadSafe<AvPipelineImpl>;
+  ~AvPipelineImpl();
+
   // Callback invoked when the CDM state has changed in a way that might
   // impact media playback.
   void OnCdmStateChange();
@@ -157,12 +159,10 @@ class AvPipelineImpl {
   bool pending_time_update_task_;
 
   // Decryption keys, if available.
+  base::Lock media_keys_lock_;
   BrowserCdmCast* media_keys_;
   int media_keys_callback_id_;
 
-  base::WeakPtr<AvPipelineImpl> weak_this_;
-  base::WeakPtrFactory<AvPipelineImpl> weak_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(AvPipelineImpl);
 };
 
diff --git a/chromecast/media/cma/pipeline/video_pipeline_impl.cc b/chromecast/media/cma/pipeline/video_pipeline_impl.cc
index d3ae9be..3a1c619 100644
--- a/chromecast/media/cma/pipeline/video_pipeline_impl.cc
+++ b/chromecast/media/cma/pipeline/video_pipeline_impl.cc
@@ -23,9 +23,9 @@ VideoPipelineImpl::VideoPipelineImpl(VideoPipelineDevice* video_device)
     : video_device_(video_device),
       weak_factory_(this) {
   weak_this_ = weak_factory_.GetWeakPtr();
-  av_pipeline_impl_.reset(new AvPipelineImpl(
+  av_pipeline_impl_ = new AvPipelineImpl(
       video_device_,
-      base::Bind(&VideoPipelineImpl::OnUpdateConfig, base::Unretained(this))));
+      base::Bind(&VideoPipelineImpl::OnUpdateConfig, base::Unretained(this)));
 }
 
 VideoPipelineImpl::~VideoPipelineImpl() {
diff --git a/chromecast/media/cma/pipeline/video_pipeline_impl.h b/chromecast/media/cma/pipeline/video_pipeline_impl.h
index e065ac1..9a57cd5 100644
--- a/chromecast/media/cma/pipeline/video_pipeline_impl.h
+++ b/chromecast/media/cma/pipeline/video_pipeline_impl.h
@@ -65,7 +65,7 @@ class VideoPipelineImpl : public VideoPipeline {
 
   VideoPipelineDevice* video_device_;
 
-  scoped_ptr<AvPipelineImpl> av_pipeline_impl_;
+  scoped_refptr<AvPipelineImpl> av_pipeline_impl_;
   VideoPipelineClient video_client_;
 
   ::media::PipelineStatistics previous_stats_;