Restore CDM context on Resume().

media/base/pipeline_impl.cc

 // Copyright (c) 2012 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 "media/base/pipeline_impl.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 29 matching lines @@
       running_(false),
       did_loading_progress_(false),
       volume_(1.0f),
       playback_rate_(0.0),
       status_(PIPELINE_OK),
       state_(kCreated),
       suspend_timestamp_(kNoTimestamp()),
       renderer_ended_(false),
       text_renderer_ended_(false),
       demuxer_(NULL),
-      pending_cdm_context_(nullptr),
+      cdm_context_(nullptr),
       weak_factory_(this) {
   media_log_->AddEvent(media_log_->CreatePipelineStateChangedEvent(kCreated));
 }
 
 PipelineImpl::~PipelineImpl() {
   DCHECK(thread_checker_.CalledOnValidThread())
       << "Pipeline must be destroyed on same thread that created it";
   DCHECK(!running_) << "Stop() must complete before destroying object";
   DCHECK(stop_cb_.is_null());
   DCHECK(seek_cb_.is_null());
@@ skipping 465 matching lines @@
 
   CHECK_EQ(kCreated, state_)
       << "Media pipeline cannot be started more than once";
 
   text_renderer_ = CreateTextRenderer();
   if (text_renderer_) {
     text_renderer_->Initialize(base::Bind(&PipelineImpl::OnTextRendererEnded,
                                           weak_factory_.GetWeakPtr()));
   }
 
-  // Set CDM early to avoid unnecessary delay in Renderer::Initialize().
-  if (pending_cdm_context_) {
-    renderer_->SetCdm(pending_cdm_context_, base::Bind(&IgnoreCdmAttached));
-    pending_cdm_context_ = nullptr;
-  }
-
   StateTransitionTask(PIPELINE_OK);
 }
 
 void PipelineImpl::StopTask(const base::Closure& stop_cb) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(stop_cb_.is_null());
 
   if (state_ == kStopped) {
     // Invalid all weak pointers so it's safe to destroy |this| on the render
     // main thread.
@@ skipping 183 matching lines @@
   fns.Push(base::Bind(&PipelineImpl::InitializeRenderer, weak_this));
 
   pending_callbacks_ = SerialRunner::Run(
       fns, base::Bind(&PipelineImpl::StateTransitionTask, weak_this));
 }
 
 void PipelineImpl::SetCdmTask(CdmContext* cdm_context,
                               const CdmAttachedCB& cdm_attached_cb) {
   base::AutoLock auto_lock(lock_);
   if (!renderer_) {
-    pending_cdm_context_ = cdm_context;
+    cdm_context_ = cdm_context;
     cdm_attached_cb.Run(true);
     return;
   }
 
-  renderer_->SetCdm(cdm_context, cdm_attached_cb);
+  renderer_->SetCdm(cdm_context, base::Bind(&PipelineImpl::OnCdmAttached,
+                                            weak_factory_.GetWeakPtr(),

[xhwang, 2016/03/17 23:11:15]

It seems we are already calling GetWeakPtr() on two threads, this is unsafe
according to:

https://codereview.chromium.org/1464183002#msg22

Actually a WeakPtrFactory can only be bound to one task runner and we should
have DCHECK for it. So I am confused how things are working today?

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

[sandersd (OOO until July 31), 2016/03/17 23:25:03]

It's safe to call from two threads; the rule is that the weak pointers that
resulting pointers must be dereferenced from only one thread.

In this case I believe that Unretained(this) is safe because we own |renderer_|.
I'll just do that.

[xhwang, 2016/03/18 17:05:55]

+wez

GetWeakPtr() call is not thread safe according to previous discussion:
https://codereview.chromium.org/1464183002#msg22

wez: Could you please update the comments in weak_ptr.h to make this clear? It
has confused most people I think.

So here we probably should get a |weak_this| during construction time and add a
comment saying that it's bound to the media thread. Then we use |weak_this|
whenever we need a weak pointer.
I see. I think today we only dereference the weak pointer on the media thread,
so we are good here.
 
See my comments in the new code.

It seems using a weak pointer is actually safe because the callback should be
fired on the media thread.

[Wez, 2016/03/18 19:49:28]

GetWeakPtr is safe to call from different threads, but is NOT thread-safe - it
may be called on thread A and later on thread B, but not in parallel.
 
I have a CL in review w/ dcheng@ to clean that up, yes.
This class already uses weak_factory_.GetWeakPtr() from both the caller and
|task_runner_| threads, which is bad for two reasons:
1. GetWeakPtr(), as has been noted, is not thread-safe.
2. If a coding error (e.g. in the caller) leads to one of the
Bind(...GetWeakPtr()) paths being executed _after_
weak_factory_.InvalidateWeakPtrs() then the new task(s) _will_ get executed.

#1 doesn't break this code because GetWeakPtr is in practice thread-safe once
there is an active WeakPtr - this code cannot call GetWeakPtr() on
|task_runner_| without first having called it on the caller thread.

#2 should be addressed as you suggest, by taking a WeakPtr at construction time,
to pass to Bind.

However, the design of this class is also overcomplicated by having it run on
both threads; it should be split into an outer PipelineImpl that runs only on
the caller thread, and a subcomponent object that runs only on the
|task_runner_|, and which contains the WeakPtrFactory, for example.

[xhwang, 2016/03/18 20:05:34]

This is one of the oldest piece of our code base and I agree that it needs to be
improved :)
Agreed. We talked about this and it's in our plan to split PipelineImpl to two
parts.

+                                            cdm_attached_cb, cdm_context));
+}
+
+void PipelineImpl::OnCdmAttached(const CdmAttachedCB& cdm_attached_cb,
+                                 CdmContext* cdm_context,
+                                 bool success) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  if (success)
+    cdm_context_ = cdm_context;
+  cdm_attached_cb.Run(success);
 }
 
 void PipelineImpl::OnRendererEnded() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   media_log_->AddEvent(media_log_->CreateEvent(MediaLogEvent::ENDED));
 
   if (state_ != kPlaying)
     return;
 
   DCHECK(!renderer_ended_);
@@ skipping 72 matching lines @@
   if (!demuxer_->GetStream(DemuxerStream::AUDIO) &&
       !demuxer_->GetStream(DemuxerStream::VIDEO)) {
     {
       base::AutoLock auto_lock(lock_);
       renderer_.reset();
     }
     OnError(PIPELINE_ERROR_COULD_NOT_RENDER);
     return;
   }
 
+  if (cdm_context_)
+    renderer_->SetCdm(cdm_context_, base::Bind(&IgnoreCdmAttached));
+
   base::WeakPtr<PipelineImpl> weak_this = weak_factory_.GetWeakPtr();
   renderer_->Initialize(
       demuxer_, done_cb,
       base::Bind(&PipelineImpl::OnUpdateStatistics, weak_this),
       base::Bind(&PipelineImpl::BufferingStateChanged, weak_this),
       base::Bind(&PipelineImpl::OnRendererEnded, weak_this),
       base::Bind(&PipelineImpl::OnError, weak_this),
       waiting_for_decryption_key_cb_);
 }
 
@@ skipping 13 matching lines @@
   metadata_cb_.Run(metadata);
 }
 
 void PipelineImpl::BufferingStateChanged(BufferingState new_buffering_state) {
   DVLOG(1) << __FUNCTION__ << "(" << new_buffering_state << ") ";
   DCHECK(task_runner_->BelongsToCurrentThread());
   buffering_state_cb_.Run(new_buffering_state);
 }
 
 }  // namespace media