media: Make MediaKeys ref-counted.

chromecast/media/cdm/browser_cdm_cast.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/cdm/browser_cdm_cast.h"
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
@@ skipping 60 matching lines @@
 
 }  // namespace
 
 BrowserCdmCast::BrowserCdmCast() {
   thread_checker_.DetachFromThread();
 }
 
 BrowserCdmCast::~BrowserCdmCast() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(player_tracker_impl_.get());
   player_tracker_impl_->NotifyCdmUnset();

[gunsch, 2015/10/28 22:10:33]

I'm a little concerned about ref-counting and lifetime using this path.

AvPipelineImpl holds a reference to BrowserCdmCast (right now, a raw reference).
It only releases its reference when it gets the notification from
PlayerTrackerImpl::NotifyCdmUnset.

See the other side here:

https://code.google.com/p/chromium/codesearch#chromium/src/chromecast/media/c...

This *should* be fine as-is, with ref-counted BrowserCdmCast + raw pointers
being roughly equivalent to what this was before. But it definitely feels a
little fragile (not that it wasn't already).

[xhwang, 2015/10/28 22:33:22]

Let me clarify a bit (code update will follow later):

I think this is the "circular reference" scenario you are talking about in the
new model where the player should hold a ref-count to the CDM:
- The player will only drop it's reference to the CDM when NotifyCdmUnset() is
called.
- The CDM will only call NotifyCdmUnset() when the CDM is destructed (e.g. [1])

However, this isn't the typical circular reference case because the CDM doesn't
own the player. The CDM will be destructed when:
1. the EME stack destroys the CDM, and
2. the media player is destroyed.

When the CDM is destructed, NotifyCdmUnset() will be called, and previously
registered CdmUnsetCB will be fired. Since those callbacks are typically bound
using a WeakPtr to the player, they will simply be dropped.

As you can see, in this new world, we actually don't need CdmUnsetCB and
NotifyCdmUnset. I am keeping it because:
- Chromecast code isn't fully migrated to the new model yet, and is still using
this callback.
- I was trying to make the diff in MediaDrmBridge/MediaCodecPlayer small because
this CL is already pretty large.

Now since this is causing confusion. I'll go ahead and cleanup
MediaDrmBridge/MediaCodecPlayer.

In the future, when ChromeCast switches to the new model, we should be able to
drop CdmUnsetCB and NotifyCdmUnset.

Note that in the future, if we want to support unsetting the CDM (e.g.
setMediaKeys(0)), we'll simply pass SetCdm(nullptr) along to the player. Then
the player can simply drop the reference to the CDM there.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...

[xhwang, 2015/10/29 00:17:28]

cdm_unset_cb registration in MediaSourcePlayer and MediaCodecPlayer is removed
in PS2.

 }
 
 void BrowserCdmCast::Initialize(
     const ::media::SessionMessageCB& session_message_cb,
     const ::media::SessionClosedCB& session_closed_cb,
     const ::media::LegacySessionErrorCB& legacy_session_error_cb,
     const ::media::SessionKeysChangeCB& session_keys_change_cb,
     const ::media::SessionExpirationUpdateCB& session_expiration_update_cb) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  player_tracker_impl_.reset(new ::media::PlayerTrackerImpl);
+  player_tracker_impl_.reset(new ::media::PlayerTrackerImpl());
 
   session_message_cb_ = session_message_cb;
   session_closed_cb_ = session_closed_cb;
   legacy_session_error_cb_ = legacy_session_error_cb;
   session_keys_change_cb_ = session_keys_change_cb;
   session_expiration_update_cb_ = session_expiration_update_cb;
 
   InitializeInternal();
 }
 
@@ skipping 44 matching lines @@
 }
 
 // A macro runs current member function on |task_runner_| thread.
 #define FORWARD_ON_CDM_THREAD(param_fn, ...) \
   task_runner_->PostTask(                    \
       FROM_HERE,                             \
       base::Bind(&BrowserCdmCast::param_fn,  \
                  base::Unretained(browser_cdm_cast_.get()), ##__VA_ARGS__))
 
 BrowserCdmCastUi::BrowserCdmCastUi(
-    scoped_ptr<BrowserCdmCast> browser_cdm_cast,
+    const scoped_refptr<BrowserCdmCast>& browser_cdm_cast,
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner)
-    : browser_cdm_cast_(browser_cdm_cast.Pass()), task_runner_(task_runner) {
-}
+    : browser_cdm_cast_(browser_cdm_cast), task_runner_(task_runner) {}
 
 BrowserCdmCastUi::~BrowserCdmCastUi() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  task_runner_->DeleteSoon(FROM_HERE, browser_cdm_cast_.release());
-}
-
-int BrowserCdmCastUi::RegisterPlayer(const base::Closure& new_key_cb,
-                                     const base::Closure& cdm_unset_cb) {
-  NOTREACHED() << "RegisterPlayer should be called on BrowserCdmCast";
-  return -1;
-}
-
-void BrowserCdmCastUi::UnregisterPlayer(int registration_id) {
-  NOTREACHED() << "UnregisterPlayer should be called on BrowserCdmCast";
+  // |this| should be destroyed on |task_runner_|.
+  DCHECK(task_runner_->BelongsToCurrentThread());

[gunsch, 2015/10/28 20:51:01]

It seems a little awkward to toss out thread_checker_ here and be okay with
destroying this class on another thread than all of its calls come in on. It
might not be an issue in practice (since BrowserCdmCastUi doesn't hold any
interesting members except for BrowserCdmCast).

Is there a reason to prefer this to simply releasing the browser_cdm_cast_
reference on the CDM thread, keeping closer behavior to before?

[xhwang, 2015/10/29 00:17:28]

As discussed offline (thanks!), using ReleaseSoon with the Addref/raw-pointer
magic.

 }
 
 BrowserCdmCast* BrowserCdmCastUi::browser_cdm_cast() const {
   DCHECK(thread_checker_.CalledOnValidThread());
   return browser_cdm_cast_.get();
 }
 
 void BrowserCdmCastUi::SetServerCertificate(
     const std::vector<uint8_t>& certificate,
     scoped_ptr<::media::SimpleCdmPromise> promise) {
@@ skipping 54 matching lines @@
   DCHECK(thread_checker_.CalledOnValidThread());
   FORWARD_ON_CDM_THREAD(RemoveSession, session_id,
       base::Passed(BindPromiseToCurrentLoop(promise.Pass())));
 }
 
 ::media::CdmContext* BrowserCdmCastUi::GetCdmContext() {
   NOTREACHED();
   return nullptr;
 }
 
+void BrowserCdmCastUi::DeleteOnCorrectThread() const {
+  // Always destroy |this| on |task_runner_|.
+  task_runner_->DeleteSoon(FROM_HERE, this);
+}
+
 // A default empty implementation for subclasses that don't need to provide
 // any key system specific initialization.
 void BrowserCdmCast::InitializeInternal() {
 }
 
 }  // namespace media
 }  // namespace chromecast