Chromecast: mitigate threading issue between AvPipeline/MediaKeys.

chromecast/media/cma/pipeline/av_pipeline_impl.cc

Index: chromecast/media/cma/pipeline/av_pipeline_impl.cc
diff --git a/chromecast/media/cma/pipeline/av_pipeline_impl.cc b/chromecast/media/cma/pipeline/av_pipeline_impl.cc
index 2e2da54facea855610ce4925b4979c3927d82fbb..a90881cca58d9863dcbdc63cfb83acafd675cced 100644
--- a/chromecast/media/cma/pipeline/av_pipeline_impl.cc
+++ b/chromecast/media/cma/pipeline/av_pipeline_impl.cc
@@ -58,8 +58,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) {
@@ -183,15 +186,18 @@ 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, weak_this_)),
+        // CDM destruction requires immediate update; don't change threads.

[lcwu1, 2015/03/03 18:52:35]

A more detailed explanation of why the unset_cb should be called synchronously
instead of posting it to the media thread is probably warranted. (Especially the
problem that the original asynchronous call caused where the media_keys object
was destroyed before the unset_cb_ got the chance to run to set media_keys_ to
null.)

[gunsch, 2015/03/03 23:04:19]

Done.

+        base::Bind(&AvPipelineImpl::OnCdmDestroyed, weak_this_));
+  }
 }
 
 void AvPipelineImpl::OnEos() {
@@ -262,12 +268,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()
@@ -329,7 +338,10 @@ void AvPipelineImpl::OnCdmStateChanged() {
 
 void AvPipelineImpl::OnCdmDestroyed() {
   DCHECK(thread_checker_.CalledOnValidThread());

[lcwu1, 2015/03/03 18:52:35]

Now that this function can be called from the main thread, won't you hit a
dcheck failure if you still keep the thread checker here?

[gunsch, 2015/03/03 23:04:19]

Done.

-  media_keys_ = NULL;
+  {
+    base::AutoLock lock(media_keys_lock_);
+    media_keys_ = NULL;
+  }
 }
 
 void AvPipelineImpl::OnFrameBuffered(