Chromecast: mitigate threading issue between AvPipeline/MediaKeys.

chromecast/media/cma/pipeline/av_pipeline_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromecast/media/cma/pipeline/av_pipeline_impl.h"
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 40 matching lines @@
   weak_this_ = weak_factory_.GetWeakPtr();
   thread_checker_.DetachFromThread();
 }
 
 AvPipelineImpl::~AvPipelineImpl() {
   // If there are weak pointers in the wild, they must be invalidated
   // on the right thread.
   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) {
   DCHECK(thread_checker_.CalledOnValidThread());
   state_ = state;
 }
 
 void AvPipelineImpl::SetCodedFrameProvider(
     scoped_ptr<CodedFrameProvider> frame_provider,
     size_t max_buffer_size,
@@ skipping 103 matching lines @@
   if (media_component_device_->GetState() == MediaComponentDevice::kStateIdle) {
     media_component_device_->SetState(
         MediaComponentDevice::kStateUninitialized);
   }
 }
 
 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_);
+  {
+    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_)),
-      ::media::BindToCurrentLoop(
-          base::Bind(&AvPipelineImpl::OnCdmDestroyed, weak_this_)));
+    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() {
   DCHECK(thread_checker_.CalledOnValidThread());
   CMALOG(kLogControl) << __FUNCTION__;
   if (state_ != kPlaying)
     return;
 
   if (!client_.eos_cb.is_null())
     client_.eos_cb.Run();
@@ skipping 50 matching lines @@
     CMALOG(kLogControl) << __FUNCTION__ << ": EOS reached, stopped feeding";
     enable_feeding_ = false;
   }
 
   scoped_refptr<DecryptContext> decrypt_context;
   if (!pending_buffer_->end_of_stream() &&
       pending_buffer_->decrypt_config()) {
     // 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()
                           << "): waiting for key id "
                           << base::HexEncode(&key_id[0], key_id.size());
       return;
     }
 
     // If we do have the clear key, decrypt the pending buffer
     // and reset the decryption context (not needed anymore).
@@ skipping 40 matching lines @@
 
   // Update the buffering state if needed.
   if (buffering_state_.get())
     UpdatePlayableFrames();
 
   // Process the pending buffer in case the CDM now has the frame key id.
   ProcessPendingBuffer();
 }
 
 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(
     const scoped_refptr<DecoderBufferBase>& buffer,
     bool is_at_max_capacity) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (!buffering_state_.get())
     return;
 
@@ skipping 39 matching lines @@
       }
     }
 
     // The frame is playable: remove it from the list of non playable frames.
     non_playable_frames_.pop_front();
   }
 }
 
 }  // namespace media
 }  // namespace chromecast