Connect WebMediaPlayerImpl to WebContentDecryptionModuleImpl for EME WD (Chromium side)

Connect WebMediaPlayerImpl to WebContentDecryptionModuleImpl for EME WD (Chromium side).

WebMediaPlayerImpl needs to access the Decryptor object when a MediaKeys object is associated with a MediaElement (HTMLMediaElement.setMediaKeys()). These are the changes required on the Chromium side to WebMediaPlayerImpl to get a pointer to the Decryptor. Use of the Decryptor will come later.

BUG=329582
TEST=new layout test in the Blink side CL

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243701

[ddorwin, 2013-12-18 23:47:34]

https://codereview.chromium.org/105143010/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/105143010/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:172: // Grab a copy of the current
CDM. This may be updated at any time by calls
It's not really a copy.

https://codereview.chromium.org/105143010/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:174: cdm_ =
client_->getWebContentDecryptionModule();
A WebContentDecryptionModule is not that interesting - it has no useful
interfaces. We really need the Decryptor from the MediaKeys from the
WebContentDecryptionModuleImpl.

It seems WebContentDecryptionModuleImpl is going to require a static_cast. Is
there a better way to do that?

We may still want to save the WebContentDecryptionModule for detaching, but we
should demonstrate in this CL how we are getting to something interesting so we
can evaluate whether the Blink APIs make sense.

https://codereview.chromium.org/105143010/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:863: cdm_ = cdm;
We need to stop any current uses of the old one (unless the pointers are the
same for whatever reason - might be a DCHECK here and Blink should prevent
that).

That makes things interesting because that operation will be asynchronous.
Therefore:

We also need to ensure that the previous CDM is not destroyed while the pipeline
is still using them. We should probably maintain a reference count to the
objects (MediaKeys/Decryptor?) that the pipeline is using in the pipeline
(Decoders?) and in WebCDMImpl.

We also need to figure out a way to prevent applications from attaching the
still-in-use MediaKeys implementation to another HTMLMediaElement. I'm going to
pursue this in a spec bug. Regardless, we will probably need some way to
indicate this is complete.

[ddorwin, 2013-12-19 00:26:27]

One additional comment, and please update the TEST= line as mentioned in the
Blink CL.

https://codereview.chromium.org/105143010/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/105143010/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:174: cdm_ =
client_->getWebContentDecryptionModule();
On 2013/12/18 23:47:35, ddorwin wrote:
> A WebContentDecryptionModule is not that interesting - it has no useful
> interfaces. We really need the Decryptor from the MediaKeys from the
> WebContentDecryptionModuleImpl.
> 
> It seems WebContentDecryptionModuleImpl is going to require a static_cast. Is
> there a better way to do that?
> 
> We may still want to save the WebContentDecryptionModule for detaching, but we
> should demonstrate in this CL how we are getting to something interesting so
we
> can evaluate whether the Blink APIs make sense.

I don't think this is the right time to set cdm_ (other than to NULL in the
initializer list). We should get it when we need it, which is when initializing
the pipeline. It might make more sense to be getting these objects there too.

We may end up with code that needs to be shared with mediaKeysChanged, in which
case we should consider a helper function if things get complicated.

[ddorwin, 2013-12-20 05:26:14]

The summary isn't very helpful - it doesn't even say the Chromium side of the
Blink change. You could say WMPI to WCDMI.

FYI, with the WMPCI changes removed, I think you can land this first then land
the Blink side - with tests.

Other than that, LG. I'll add the TM after we finish the Blink side.

https://codereview.chromium.org/105143010/diff/20001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodule_impl.h (right):

https://codereview.chromium.org/105143010/diff/20001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:34: // after
WebContentDecryptionModule is freed. http://crbug.com/309235
It seems like this bug is probably different (and worth filing given our
conversations today).

https://codereview.chromium.org/105143010/diff/20001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/105143010/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.h:383:
WebContentDecryptionModuleImpl* cdm_;
Because we have multiple levels of CDM, it might be helpful to call this
web_cdm_. WDYT?

[jrummell, 2013-12-20 23:32:09]

Updated.

https://codereview.chromium.org/105143010/diff/20001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodule_impl.h (right):

https://codereview.chromium.org/105143010/diff/20001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:34: // after
WebContentDecryptionModule is freed. http://crbug.com/309235
On 2013/12/20 05:26:14, ddorwin wrote:
> It seems like this bug is probably different (and worth filing given our
> conversations today).

Opened http://crbug.com/330324 and updated link.

https://codereview.chromium.org/105143010/diff/20001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.h (right):

https://codereview.chromium.org/105143010/diff/20001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.h:383:
WebContentDecryptionModuleImpl* cdm_;
On 2013/12/20 05:26:14, ddorwin wrote:
> Because we have multiple levels of CDM, it might be helpful to call this
> web_cdm_. WDYT?

Done.

[ddorwin, 2013-12-21 00:25:50]

LGTM, but wait until the Bink CL is LGTM'd by an OWNER before committing in case
we need to change anything in the Blink API.

Also, the TEST= line can just reference the "new layout test in the Blink side
CL" or something like that.

[xhwang, 2013-12-30 19:50:49]

drive-by nits

https://codereview.chromium.org/105143010/diff/40001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodule_impl.h (right):

https://codereview.chromium.org/105143010/diff/40001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:32: // Return the
Decryptor associated with this CDM.
nit: Return_s_

https://codereview.chromium.org/105143010/diff/40001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:32: // Return the
Decryptor associated with this CDM.
What if the current CDM doesn't have a decryptor, e.g. gtv?

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

https://codereview.chromium.org/105143010/diff/40001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:56: inline
WebContentDecryptionModuleImpl* toWebContentDecryptionModuleImpl(
s/to/To?

https://codereview.chromium.org/105143010/diff/40001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/105143010/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:168: web_cdm_(0) {
s/0/NULL

[jrummell, 2014-01-08 19:12:30]

Updated nits (plus looks like a rebase over the holidays).

https://codereview.chromium.org/105143010/diff/40001/content/renderer/media/w...
File content/renderer/media/webcontentdecryptionmodule_impl.h (right):

https://codereview.chromium.org/105143010/diff/40001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:32: // Return the
Decryptor associated with this CDM.
On 2013/12/30 19:50:49, xhwang wrote:
> nit: Return_s_

Done.

https://codereview.chromium.org/105143010/diff/40001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:32: // Return the
Decryptor associated with this CDM.
On 2013/12/30 19:50:49, xhwang wrote:
> What if the current CDM doesn't have a decryptor, e.g. gtv?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

Comment added.

https://codereview.chromium.org/105143010/diff/40001/content/renderer/media/w...
content/renderer/media/webcontentdecryptionmodule_impl.h:56: inline
WebContentDecryptionModuleImpl* toWebContentDecryptionModuleImpl(
On 2013/12/30 19:50:49, xhwang wrote:
> s/to/To?

Done.

https://codereview.chromium.org/105143010/diff/40001/content/renderer/media/w...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/105143010/diff/40001/content/renderer/media/w...
content/renderer/media/webmediaplayer_impl.cc:168: web_cdm_(0) {
On 2013/12/30 19:50:49, xhwang wrote:
> s/0/NULL

Done. WebKit presubmit checks complain about NULL, so I changed them all. Not
needed here.

[xhwang, 2014-01-08 19:21:56]

lgtm, thanks

[commit-bot: I haz the power, 2014-01-08 19:44:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jrummell@chromium.org/105143010/130001

[commit-bot: I haz the power, 2014-01-09 00:01:24]

Change committed as 243701