media: Make MediaKeys ref-counted.

media: Make MediaKeys ref-counted.

MediaKeys (CDM) is created and owned by the EME stack (e.g. CdmSessionAdapter)
but is dereferenced by the media pipeline (e.g. DecryptingVideoDecoder). During
teardown, we need to make sure the CDM is not dereferenced by the media pipeline
after it's destructed.

In the Render process, this is guaranteed by the order of destruction of
MediaKeys and Media elements in the Blink layer.

However, when the CDM and the media pipeline are running out-of-process, because
the order of IPC messages on different IPC channels is nondeterministic, we need
some extra mechanism to ensure that the media pipeline doesn't dereference a
destructed CDM. Currently on Android, this is achieved by using an extra
PlayerTracker interface. However, in some situations, e.g. when the media
pipeline and CDM are running on different threads, the destruction handling code
could be pretty complicated.

When a MojoMediaApplication is hosted in a non-Render process, we face the same
issue.

Starting with this CL, we'll make MediaKeys ref-counted, so that the media
pipeline that uses it will also hold a ref-count to it. This will greatly
simplify the lifetime management between the CDM and the media pipeline.

BUG=511040
TEST=All existing use cases still work.

Committed: https://crrev.com/b24facba10f213d4448bf108ba0d01821b2ba05d
Cr-Commit-Position: refs/heads/master@{#357514}

[xhwang, 2015-10-28 20:39:57]

xhwang@chromium.org changed reviewers:
+ ddorwin@chromium.org, halliwell@chromium.org, jrummell@chromium.org,
timav@chromium.org

[xhwang, 2015-10-28 20:39:58]

This CL is touching a lot of files since I am updating a base media/EME class.
But the changes are actually pretty straightforward.

I'll do more test on desktop and Android today.

jrummell@chromium.org: Please review everything except for Chromecast and
Android code.

halliwell@chromium.org: Please review chromecast changes. If needed, please also
test it on a chromecast build. I am not sure how to test it.

timav@chromium.org: Please review Android changes.

ddorwin@chromium.org: Please review from an interface/design perspective.

https://chromiumcodereview.appspot.com/1407933010/diff/1/chromecast/browser/m...
File chromecast/browser/media/cma_message_filter_host.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/1/chromecast/browser/m...
chromecast/browser/media/cma_message_filter_host.cc:125:
base::Unretained(browser_cdm_cast)));
Does it make sense to pass the scoped_refptr here and let the CMA pipeline hold
a ref-count to the CDM?

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

Drive-by review comments.

I like the idea in principle, though I'm mildly concerned about the possibility
of issues arising from not being able to trace the destruction path as clearly.

We've had issues in the past since, for example, only one low-level PlayReady
implementation expects to be running at a time.

That said, I think we dealt with a lot of those issues in the transition from
prefixed to unprefixed EME (since JS MediaKeys instances can't be
deterministically destroyed in the same way that changing the <video> src used
to accomplish).

The best way forward is probably just running the Cast DRM tests on a Chromecast
build with this patched in.

https://chromiumcodereview.appspot.com/1407933010/diff/1/chromecast/media/cdm...
File chromecast/media/cdm/browser_cdm_cast.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/1/chromecast/media/cdm...
chromecast/media/cdm/browser_cdm_cast.cc:163:
DCHECK(task_runner_->BelongsToCurrentThread());
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?

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

I talked with xhwang@ a bit offline. I think this is less big of a change for
Chromecast than I was concerned about, given that BrowserCdmCastUi remains the
only smart-ptr owner of BrowserCdmCast, so the destruction path should still be
just as deterministic as before, despite it becoming ref-counted.

My bigger concern is probably for long-term maintainability, since I think it
would be easy to create a circular reference loop keeping it alive. See comment
in BrowserCdmCast.

https://chromiumcodereview.appspot.com/1407933010/diff/1/chromecast/media/cdm...
File chromecast/media/cdm/browser_cdm_cast.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/1/chromecast/media/cdm...
chromecast/media/cdm/browser_cdm_cast.cc:81:
player_tracker_impl_->NotifyCdmUnset();
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]

https://chromiumcodereview.appspot.com/1407933010/diff/1/chromecast/media/cdm...
File chromecast/media/cdm/browser_cdm_cast.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/1/chromecast/media/cdm...
chromecast/media/cdm/browser_cdm_cast.cc:81:
player_tracker_impl_->NotifyCdmUnset();
On 2015/10/28 22:10:33, gunsch wrote:
> 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).

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...

[Tima Vaisburd, 2015-10-28 22:49:40]

So far just nits. May be some are not even relevant after your cleanup.

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
File media/base/android/media_codec_player.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
media/base/android/media_codec_player.cc:104:
drm_bridge_->UnregisterPlayer(cdm_registration_id_);
Can you use a cast from MediaKeys to MediaDemBridge? Then, I think, you can get
rid of |drm_bridge_| member variable.

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
media/base/android/media_codec_player.cc:435: drm_bridge_ =
static_cast<MediaDrmBridge*>(cdm_.get());
I think we can get rid of |drm_bridge_|. See below.

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
File media/base/android/media_drm_bridge.h (right):

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
media/base/android/media_drm_bridge.h:29: // Thread Safty:
s/Safty/Safety/

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
media/base/android/media_drm_bridge.h:259: // The |player_tracker_| must be
accessed by one thread only. It is accessed
nit: please delete this comment, I think it's no longer true.

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
File media/base/android/media_source_player.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
media/base/android/media_source_player.cc:74:
drm_bridge_->UnregisterPlayer(cdm_registration_id_);
Same consideration as with MediaCodecPlayer: I would use a cast and keep just
one member var that points to one object.

[Tima Vaisburd, 2015-10-28 23:42:17]

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
File media/base/android/media_drm_bridge.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
media/base/android/media_drm_bridge.cc:422: task_runner_->DeleteSoon(FROM_HERE,
this)) {
Why did you prefer potentially calling destructor on the wrong thread to
leaking? It seems DeleteSoon() will return false if it cannot PostTask(), which
in some cases happens during shutdown?

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

I addressed gunsch and timav's comments. PTAL again.

https://chromiumcodereview.appspot.com/1407933010/diff/1/chromecast/media/cdm...
File chromecast/media/cdm/browser_cdm_cast.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/1/chromecast/media/cdm...
chromecast/media/cdm/browser_cdm_cast.cc:81:
player_tracker_impl_->NotifyCdmUnset();
On 2015/10/28 22:33:22, xhwang wrote:
> On 2015/10/28 22:10:33, gunsch wrote:
> > 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).
> 
> 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...

cdm_unset_cb registration in MediaSourcePlayer and MediaCodecPlayer is removed
in PS2.

https://chromiumcodereview.appspot.com/1407933010/diff/1/chromecast/media/cdm...
chromecast/media/cdm/browser_cdm_cast.cc:163:
DCHECK(task_runner_->BelongsToCurrentThread());
On 2015/10/28 20:51:01, gunsch wrote:
> 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?

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

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
File media/base/android/media_codec_player.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
media/base/android/media_codec_player.cc:435: drm_bridge_ =
static_cast<MediaDrmBridge*>(cdm_.get());
On 2015/10/28 22:49:40, Tima Vaisburd wrote:
> I think we can get rid of |drm_bridge_|. See below.

Done.

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
File media/base/android/media_drm_bridge.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
media/base/android/media_drm_bridge.cc:422: task_runner_->DeleteSoon(FROM_HERE,
this)) {
On 2015/10/28 23:42:17, Tima Vaisburd wrote:
> Why did you prefer potentially calling destructor on the wrong thread to
> leaking? It seems DeleteSoon() will return false if it cannot PostTask(),
which
> in some cases happens during shutdown?

Good point. That's what I originally did. Then I saw this pattern somewhere and
felt it makes some sense:

https://code.google.com/p/chromium/codesearch#search/&q=%22task_runner_-%3EDe...


Now thinking about it again. I think your suggestion is more reasonable.

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
File media/base/android/media_drm_bridge.h (right):

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
media/base/android/media_drm_bridge.h:29: // Thread Safty:
On 2015/10/28 22:49:40, Tima Vaisburd wrote:
> s/Safty/Safety/

Done.

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
media/base/android/media_drm_bridge.h:259: // The |player_tracker_| must be
accessed by one thread only. It is accessed
On 2015/10/28 22:49:40, Tima Vaisburd wrote:
> nit: please delete this comment, I think it's no longer true.

Done.

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
File media/base/android/media_source_player.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/1/media/base/android/m...
media/base/android/media_source_player.cc:74:
drm_bridge_->UnregisterPlayer(cdm_registration_id_);
On 2015/10/28 22:49:40, Tima Vaisburd wrote:
> Same consideration as with MediaCodecPlayer: I would use a cast and keep just
> one member var that points to one object.

Done.

[Tima Vaisburd, 2015-10-29 01:17:53]

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/andro...
File media/base/android/media_codec_player.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/andro...
media/base/android/media_codec_player.cc:441: // No need to set |cdm_unset_cb|
since |this| holds a reference to the |cdm_|.
Please explain that the second callback will never be called because it can only
fire when there are no players any more.

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/andro...
File media/base/android/media_drm_bridge.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/andro...
media/base/android/media_drm_bridge.cc:736: player_tracker_.NotifyCdmUnset();
There seem to be no players to notify, right? If you prefer to keep it here for
consistency, please explain it in the comment.

On the other hand, would it be possible to do the similar change in Cast and get
rid of NotifyCdmUnset() altogether?

[xhwang, 2015-10-29 03:12:31]

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/andro...
File media/base/android/media_codec_player.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/andro...
media/base/android/media_codec_player.cc:441: // No need to set |cdm_unset_cb|
since |this| holds a reference to the |cdm_|.
On 2015/10/29 01:17:53, Tima Vaisburd wrote:
> Please explain that the second callback will never be called because it can
only
> fire when there are no players any more.

I'll update this comment when I address other comments tomorrow.

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/andro...
File media/base/android/media_drm_bridge.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/andro...
media/base/android/media_drm_bridge.cc:736: player_tracker_.NotifyCdmUnset();
On 2015/10/29 01:17:53, Tima Vaisburd wrote:
> There seem to be no players to notify, right? If you prefer to keep it here
for
> consistency, please explain it in the comment.

The fact that the callback won't fire is an implementation detail of the
MediaSourcePlayer and MediaCodecPlayer, who hold a scoped_refptr to the CDM and
know that these callbacks won't fire. However, it's possible that some other
player could hold a raw pointer to the CDM and will rely on this callback to
work properly. We don't have such a player now, but I'd like to keep
MediaDrmBridge self-contained.

> On the other hand, would it be possible to do the similar change in Cast and
get
> rid of NotifyCdmUnset() altogether?

We should. But I don't plan to dive too deep into ChromeCast code. ChromeCast
people can choose what to do next. If they do similar changes, we can drop
NotifyCdmUnset() altogther later.

[gunsch, 2015-10-29 16:01:28]

gunsch@chromium.org changed reviewers:
+ gunsch@chromium.org

[gunsch, 2015-10-29 16:01:29]

I agree with xhwang's description. It seems like this hinges on not being able
to change the CDM mid-playback (i.e. for a given WMP{I,A} lifetime), but that
should be a safe assumption long-term.

I also agree that we could make similar changes on the BrowserCdmCast side --- I
don't think it'd be too hard at all --- and then delete OnCdmUnset altogether.

That said, I'll hold off approving, because Luke's the one who gets to maintain
this :)

https://chromiumcodereview.appspot.com/1407933010/diff/40001/chromecast/brows...
File chromecast/browser/media/cma_message_filter_host.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/chromecast/brows...
chromecast/browser/media/cma_message_filter_host.cc:122:
base::Unretained(browser_cdm_cast)));
TODO(halliwell) to fix this and pass the scoped_refptr all the way down? :)

[Tima Vaisburd, 2015-10-29 17:10:01]

lgtm for android part % please add comments

[jrummell, 2015-10-29 21:05:17]

lgtm w/nits.

https://codereview.chromium.org/1407933010/diff/40001/media/base/android/medi...
File media/base/android/media_codec_player.cc (right):

https://codereview.chromium.org/1407933010/diff/40001/media/base/android/medi...
media/base/android/media_codec_player.cc:416: DCHECK(cdm);
Should this DCHECK() be first to fail the caller rather than possibly switching
to the media thread (and losing the caller)?

https://codereview.chromium.org/1407933010/diff/40001/media/base/browser_cdm_...
File media/base/browser_cdm_factory.h (right):

https://codereview.chromium.org/1407933010/diff/40001/media/base/browser_cdm_...
media/base/browser_cdm_factory.h:11: #include "base/memory/scoped_ptr.h"
Should be "base/memory/ref_counted.h" now?

https://codereview.chromium.org/1407933010/diff/40001/media/base/cdm_initiali...
File media/base/cdm_initialized_promise.h (right):

https://codereview.chromium.org/1407933010/diff/40001/media/base/cdm_initiali...
media/base/cdm_initialized_promise.h:8: #include "base/memory/scoped_ptr.h"
Should be base/memory/ref_counted.h now.

https://codereview.chromium.org/1407933010/diff/40001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.h (right):

https://codereview.chromium.org/1407933010/diff/40001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.h:14: #include "base/memory/scoped_vector.h"
#include "base/memory/ref_counted.h"?

https://codereview.chromium.org/1407933010/diff/40001/media/mojo/services/moj...
File media/mojo/services/mojo_cdm_service.h (right):

https://codereview.chromium.org/1407933010/diff/40001/media/mojo/services/moj...
media/mojo/services/mojo_cdm_service.h:10: #include "base/memory/scoped_ptr.h"
#include "base/memory/ref_counted.h"?

https://codereview.chromium.org/1407933010/diff/40001/media/test/pipeline_int...
File media/test/pipeline_integration_test.cc (right):

https://codereview.chromium.org/1407933010/diff/40001/media/test/pipeline_int...
media/test/pipeline_integration_test.cc:7: #include "base/memory/scoped_ptr.h"
#include "base/memory/ref_counted.h"?

[halliwell, 2015-10-30 01:50:51]

https://chromiumcodereview.appspot.com/1407933010/diff/40001/chromecast/brows...
File chromecast/browser/media/cma_message_filter_host.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/chromecast/brows...
chromecast/browser/media/cma_message_filter_host.cc:122:
base::Unretained(browser_cdm_cast)));
On 2015/10/29 16:01:29, gunsch wrote:
> TODO(halliwell) to fix this and pass the scoped_refptr all the way down? :)

I filed internal b/25378705 for this.

https://chromiumcodereview.appspot.com/1407933010/diff/40001/chromecast/media...
File chromecast/media/cdm/browser_cdm_cast.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/chromecast/media...
chromecast/media/cdm/browser_cdm_cast.cc:166:
task_runner_->ReleaseSoon(FROM_HERE, raw_cdm);
Seems that we should assert that it HasOneRef before we call ReleaseSoon (if I'm
understanding correctly that this is intended to be a deterministic destruction
path still).

[xhwang, 2015-10-30 04:50:16]

https://chromiumcodereview.appspot.com/1407933010/diff/40001/chromecast/media...
File chromecast/media/cdm/browser_cdm_cast.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/chromecast/media...
chromecast/media/cdm/browser_cdm_cast.cc:166:
task_runner_->ReleaseSoon(FROM_HERE, raw_cdm);
On 2015/10/30 01:50:51, halliwell wrote:
> Seems that we should assert that it HasOneRef before we call ReleaseSoon (if
I'm
> understanding correctly that this is intended to be a deterministic
destruction
> path still).

For now yes. In the future, if the CMA pipeline could also hods a ref-count to
the browser_cdm_cast, then the ref count could be >1. I am reluctant to add it
here since that really is the implementation detail of the player (pipeline),
and is not a part of the EME stack.

[halliwell, 2015-10-30 05:14:16]

On 2015/10/30 04:50:16, xhwang wrote:
>
https://chromiumcodereview.appspot.com/1407933010/diff/40001/chromecast/media...
> File chromecast/media/cdm/browser_cdm_cast.cc (right):
> 
>
https://chromiumcodereview.appspot.com/1407933010/diff/40001/chromecast/media...
> chromecast/media/cdm/browser_cdm_cast.cc:166:
> task_runner_->ReleaseSoon(FROM_HERE, raw_cdm);
> On 2015/10/30 01:50:51, halliwell wrote:
> > Seems that we should assert that it HasOneRef before we call ReleaseSoon (if
> I'm
> > understanding correctly that this is intended to be a deterministic
> destruction
> > path still).
> 
> For now yes. In the future, if the CMA pipeline could also hods a ref-count to
> the browser_cdm_cast, then the ref count could be >1. I am reluctant to add it
> here since that really is the implementation detail of the player (pipeline),
> and is not a part of the EME stack.

Yep, you're right.  BrowserCdmCast destructor already asserts that it's
destroyed on the correct thread, so an additional assert here is unnecessary. 
lgtm.

[xhwang, 2015-10-30 16:19:39]

comments addressed

[xhwang, 2015-10-30 16:20:59]

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/andro...
File media/base/android/media_codec_player.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/andro...
media/base/android/media_codec_player.cc:416: DCHECK(cdm);
On 2015/10/29 21:05:17, jrummell wrote:
> Should this DCHECK() be first to fail the caller rather than possibly
switching
> to the media thread (and losing the caller)?

Done.

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/andro...
media/base/android/media_codec_player.cc:441: // No need to set |cdm_unset_cb|
since |this| holds a reference to the |cdm_|.
On 2015/10/29 01:17:53, Tima Vaisburd wrote:
> Please explain that the second callback will never be called because it can
only
> fire when there are no players any more.

Done.

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/brows...
File media/base/browser_cdm_factory.h (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/brows...
media/base/browser_cdm_factory.h:11: #include "base/memory/scoped_ptr.h"
On 2015/10/29 21:05:17, jrummell wrote:
> Should be "base/memory/ref_counted.h" now?

Done.

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/cdm_i...
File media/base/cdm_initialized_promise.h (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/base/cdm_i...
media/base/cdm_initialized_promise.h:8: #include "base/memory/scoped_ptr.h"
On 2015/10/29 21:05:17, jrummell wrote:
> Should be base/memory/ref_counted.h now.

Done.

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/cdm/proxy_...
File media/cdm/proxy_decryptor.h (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/cdm/proxy_...
media/cdm/proxy_decryptor.h:14: #include "base/memory/scoped_vector.h"
On 2015/10/29 21:05:17, jrummell wrote:
> #include "base/memory/ref_counted.h"?

Done.

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/mojo/servi...
File media/mojo/services/mojo_cdm_service.h (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/mojo/servi...
media/mojo/services/mojo_cdm_service.h:10: #include "base/memory/scoped_ptr.h"
On 2015/10/29 21:05:17, jrummell wrote:
> #include "base/memory/ref_counted.h"?

Done.

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/test/pipel...
File media/test/pipeline_integration_test.cc (right):

https://chromiumcodereview.appspot.com/1407933010/diff/40001/media/test/pipel...
media/test/pipeline_integration_test.cc:7: #include "base/memory/scoped_ptr.h"
On 2015/10/29 21:05:17, jrummell wrote:
> #include "base/memory/ref_counted.h"?

Done.

[xhwang, 2015-10-30 16:25:19]

xhwang@chromium.org changed reviewers:
+ nasko@chromium.org

[xhwang, 2015-10-30 16:25:20]

All comments addressed.

nasko: Please OWNERS review trivial changes in:

	 content/browser/renderer_host/render_process_host_impl.*
	 content/public/browser/render_process_host.h
	 content/public/test/mock_render_process_host.*

ddorwin: Please review from api/design's perspective.

[xhwang, 2015-11-02 18:15:22]

nasko / ddorwin: kindly ping :)

[nasko, 2015-11-02 21:46:58]

*render_process_host* LGTM

[ddorwin, 2015-11-03 00:28:50]

I mainly looked at media_keys.h. LG. Some nits.

https://chromiumcodereview.appspot.com/1407933010/diff/60001/media/base/media...
File media/base/media_keys.h (right):

https://chromiumcodereview.appspot.com/1407933010/diff/60001/media/base/media...
media/base/media_keys.h:37: // An interface for the Content Decryption Module
(CDM) in the Encrypted Media
This makes it sound like _this_ represents the CDM in the spec. But that is
really WebCDM. Should we be more precise?

https://chromiumcodereview.appspot.com/1407933010/diff/60001/media/base/media...
media/base/media_keys.h:38: // Extensions (EME) spec. See
http://w3c.github.io/encrypted-media/#definitions
http://w3c.github.io/encrypted-media/#cdm

https://chromiumcodereview.appspot.com/1407933010/diff/60001/media/base/media...
media/base/media_keys.h:184: // See the spec for details:
http://w3c.github.io/encrypted-media/
https://w3c.github.io/encrypted-media/#mediakeysession-interface

But do we even need this link since each has a link?

[xhwang, 2015-11-03 00:50:07]

comments addressed

[xhwang, 2015-11-03 00:50:29]

https://chromiumcodereview.appspot.com/1407933010/diff/60001/media/base/media...
File media/base/media_keys.h (right):

https://chromiumcodereview.appspot.com/1407933010/diff/60001/media/base/media...
media/base/media_keys.h:37: // An interface for the Content Decryption Module
(CDM) in the Encrypted Media
On 2015/11/03 00:28:50, ddorwin wrote:
> This makes it sound like _this_ represents the CDM in the spec. But that is
> really WebCDM. Should we be more precise?

Added the "in chromium" part, since "WebCDM" is a Blink interface that
represents the CDM. WDYT?

https://chromiumcodereview.appspot.com/1407933010/diff/60001/media/base/media...
media/base/media_keys.h:38: // Extensions (EME) spec. See
http://w3c.github.io/encrypted-media/#definitions
On 2015/11/03 00:28:50, ddorwin wrote:
> http://w3c.github.io/encrypted-media/#cdm

Done.

https://chromiumcodereview.appspot.com/1407933010/diff/60001/media/base/media...
media/base/media_keys.h:184: // See the spec for details:
http://w3c.github.io/encrypted-media/
On 2015/11/03 00:28:50, ddorwin wrote:
> https://w3c.github.io/encrypted-media/#mediakeysession-interface
> 
> But do we even need this link since each has a link?

Removed.

[ddorwin, 2015-11-03 00:52:17]

LG. Thanks.

[xhwang, 2015-11-03 01:07:45]

rebase only

[xhwang, 2015-11-03 01:25:26]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-11-03 01:25:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
jrummell@chromium.org, timav@chromium.org, halliwell@chromium.org,
nasko@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1407933010/#ps100001 (title: "rebase
only")

[commit-bot: I haz the power, 2015-11-03 01:26:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1407933010/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1407933010/100001

[commit-bot: I haz the power, 2015-11-03 02:09:37]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-03 02:10:22]

Patchset 6 (id:??) landed as
https://crrev.com/b24facba10f213d4448bf108ba0d01821b2ba05d
Cr-Commit-Position: refs/heads/master@{#357514}