Initialize the CDM asynchronously

Initialize the CDM asynchronously

Currently initialization happens asynchronously, but as Initialize()
has no way to report success/failure, subsequent calls need to check
that it succeeded. This change passes a promise to Initialize() so
that the CDM can report success/failure properly.

BUG=407435, 469003
TEST=EME tests pass

Committed: https://crrev.com/87a2db5daf3fbada56114caa4bb61e596ad459a2
Cr-Commit-Position: refs/heads/master@{#328418}

[jrummell, 2015-04-27 19:58:17]

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

[jrummell, 2015-04-27 19:58:17]

PTAL.

[sandersd (OOO until July 31), 2015-04-27 20:17:48]

https://codereview.chromium.org/1102363005/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1102363005/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:700: void WebMediaPlayerImpl::SetCdm(const
CdmAttachedCB& cdm_attached_cb,
I'm not very familiar with how this happens, can you quickly describe how we get
here? Thanks!

[jrummell, 2015-04-27 21:24:38]

Comment only.

https://codereview.chromium.org/1102363005/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1102363005/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:700: void WebMediaPlayerImpl::SetCdm(const
CdmAttachedCB& cdm_attached_cb,
On 2015/04/27 20:17:48, sandersd wrote:
> I'm not very familiar with how this happens, can you quickly describe how we
get
> here? Thanks!

This is a CB passed down through ProxyDecryptor::CreateCdm(). When
initialization is complete, it is called. Previous CLs could provide NULL (see
ProxyDecryptor::OnCdmCreated(), line 96), but it looks like that path was never
executed (at least in the tests).

[sandersd (OOO until July 31), 2015-04-27 23:32:14]

I'm not sold on diverging the interfaces for the different CDM hosts more, but I
don't see an obvious interface that pulls them back together either, and at
least there is only once place in the code that needs to know the differences.

https://codereview.chromium.org/1102363005/diff/1/content/renderer/media/cryp...
File content/renderer/media/crypto/ppapi_decryptor.h (right):

https://codereview.chromium.org/1102363005/diff/1/content/renderer/media/cryp...
content/renderer/media/crypto/ppapi_decryptor.h:49: void InitializeCdm(const
std::string& key_system,
Perhaps this should be passed through the constructor so that we don't have to
pass it twice? (It saves a copy to do it this way, so it's fine either way.)

[ddorwin, 2015-04-27 23:52:18]

Start with the comments in the factory.

https://codereview.chromium.org/1102363005/diff/1/content/renderer/media/cryp...
File content/renderer/media/crypto/ppapi_decryptor.h (right):

https://codereview.chromium.org/1102363005/diff/1/content/renderer/media/cryp...
content/renderer/media/crypto/ppapi_decryptor.h:49: void InitializeCdm(const
std::string& key_system,
On 2015/04/27 23:32:14, sandersd wrote:
> Perhaps this should be passed through the constructor so that we don't have to
> pass it twice? (It saves a copy to do it this way, so it's fine either way.)

None of these values should be changeable AFTER creating the CDM instance. (From
an API perspective, we don't know that the CDM has not been instantiated until
InitializeCdm is called.) In theory, instantiating the process could require the
bools too.

If we have to store these values as members, so be it. At least they can be
const.

However, this may be moot based on my comments in the factory.

https://codereview.chromium.org/1102363005/diff/1/content/renderer/media/cryp...
content/renderer/media/crypto/ppapi_decryptor.h:52:
scoped_ptr<media::SimpleCdmPromise> promise);
If this is going to be a public method, it should be as consistent as possible
and probably part of the media::MediaKeys interface. However, see my comments in
the factory.

https://codereview.chromium.org/1102363005/diff/1/content/renderer/media/cryp...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/1102363005/diff/1/content/renderer/media/cryp...
content/renderer/media/crypto/render_cdm_factory.cc:120:
ppapi_decryptor->InitializeCdm(key_system, allow_distinctive_identifier,
If this is only called by us here, why not hide it in Create?

If we need asynchronicity, why not make Create return a promise and hide all the
initialization there?

https://codereview.chromium.org/1102363005/diff/1/content/renderer/media/cryp...
content/renderer/media/crypto/render_cdm_factory.cc:129: if (!cdm) {
Duplicating this logic seems wrong.

[jrummell, 2015-04-28 18:02:45]

Updated.

https://codereview.chromium.org/1102363005/diff/1/content/renderer/media/cryp...
File content/renderer/media/crypto/render_cdm_factory.cc (right):

https://codereview.chromium.org/1102363005/diff/1/content/renderer/media/cryp...
content/renderer/media/crypto/render_cdm_factory.cc:120:
ppapi_decryptor->InitializeCdm(key_system, allow_distinctive_identifier,
On 2015/04/27 23:52:18, ddorwin wrote:
> If this is only called by us here, why not hide it in Create?
> 
> If we need asynchronicity, why not make Create return a promise and hide all
the
> initialization there?

Changed to do all the work in ::Create(). Not returning anything as
|cdm_created_cb| should be called when the CDM is initialized.

[ddorwin, 2015-05-04 22:46:03]

LG overall. Thanks.

https://codereview.chromium.org/1102363005/diff/20001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/1102363005/diff/20001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:52: if (!ppapi_decryptor) {
No need to worry about constructor failure.

https://codereview.chromium.org/1102363005/diff/20001/content/renderer/media/...
File content/renderer/media/crypto/proxy_media_keys.cc (right):

https://codereview.chromium.org/1102363005/diff/20001/content/renderer/media/...
content/renderer/media/crypto/proxy_media_keys.cc:34: if (!proxy_media_keys) {
ditto

https://codereview.chromium.org/1102363005/diff/20001/content/renderer/media/...
content/renderer/media/crypto/proxy_media_keys.cc:213: // TODO(jrummell): |Pass
promise| on. http://crbug.com/407435.
This bug only refers to CdmAdapater. Did you mean 469003?

https://codereview.chromium.org/1102363005/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1102363005/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:702: // The CDM may fail to create, so no
need to tell the pipeline that a CDM
nit: // If CDM initialization failed, there is no need...

If you want the code to match the comment, you could have if (!context) return.

https://codereview.chromium.org/1102363005/diff/20001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/1102363005/diff/20001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:337: bool CdmAdapter::CreateCdmInstance(const
std::string& key_system) {
This can probably be simplified too. We can just return a pointer that the
caller can assign to cdm_.

https://codereview.chromium.org/1102363005/diff/20001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:361: RejectPromise(promise_id,
cdm::kInvalidAccessError, 0,
Not sure this is correct. Probably kUnknownError.

https://codereview.chromium.org/1102363005/diff/20001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:380: if (!cdm_ && !CreateCdmInstance(key_system))
{
I think cdm_ must be NULL. You can DCHECK at 356.

https://codereview.chromium.org/1102363005/diff/20001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/1102363005/diff/20001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:97: } else {
Now we complete pending GKRs whereas we didn't before. Is that right?

[jrummell, 2015-05-05 00:54:18]

Updated.

https://codereview.chromium.org/1102363005/diff/20001/content/renderer/media/...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/1102363005/diff/20001/content/renderer/media/...
content/renderer/media/crypto/ppapi_decryptor.cc:52: if (!ppapi_decryptor) {
On 2015/05/04 22:46:02, ddorwin wrote:
> No need to worry about constructor failure.

Done.

https://codereview.chromium.org/1102363005/diff/20001/content/renderer/media/...
File content/renderer/media/crypto/proxy_media_keys.cc (right):

https://codereview.chromium.org/1102363005/diff/20001/content/renderer/media/...
content/renderer/media/crypto/proxy_media_keys.cc:34: if (!proxy_media_keys) {
On 2015/05/04 22:46:02, ddorwin wrote:
> ditto

Done.

https://codereview.chromium.org/1102363005/diff/20001/content/renderer/media/...
content/renderer/media/crypto/proxy_media_keys.cc:213: // TODO(jrummell): |Pass
promise| on. http://crbug.com/407435.
On 2015/05/04 22:46:02, ddorwin wrote:
> This bug only refers to CdmAdapater. Did you mean 469003?

Done.

https://codereview.chromium.org/1102363005/diff/20001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1102363005/diff/20001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:702: // The CDM may fail to create, so no
need to tell the pipeline that a CDM
On 2015/05/04 22:46:02, ddorwin wrote:
> nit: // If CDM initialization failed, there is no need...
> 
> If you want the code to match the comment, you could have if (!context)
return.

Done.

https://codereview.chromium.org/1102363005/diff/20001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/1102363005/diff/20001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:337: bool CdmAdapter::CreateCdmInstance(const
std::string& key_system) {
On 2015/05/04 22:46:02, ddorwin wrote:
> This can probably be simplified too. We can just return a pointer that the
> caller can assign to cdm_.

Done.

https://codereview.chromium.org/1102363005/diff/20001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:361: RejectPromise(promise_id,
cdm::kInvalidAccessError, 0,
On 2015/05/04 22:46:02, ddorwin wrote:
> Not sure this is correct. Probably kUnknownError.

Done.

https://codereview.chromium.org/1102363005/diff/20001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:380: if (!cdm_ && !CreateCdmInstance(key_system))
{
On 2015/05/04 22:46:02, ddorwin wrote:
> I think cdm_ must be NULL. You can DCHECK at 356.

Done.

https://codereview.chromium.org/1102363005/diff/20001/media/cdm/proxy_decrypt...
File media/cdm/proxy_decryptor.cc (right):

https://codereview.chromium.org/1102363005/diff/20001/media/cdm/proxy_decrypt...
media/cdm/proxy_decryptor.cc:97: } else {
On 2015/05/04 22:46:02, ddorwin wrote:
> Now we complete pending GKRs whereas we didn't before. Is that right?

Correct. The ECKPrefixedEncryptedMediaTest.InitializeCDMFail test was failing,
due to the extra time through Pepper before the CDM is reporting failure.

[ddorwin, 2015-05-05 15:34:50]

LGTM with nit. Thanks.

https://codereview.chromium.org/1102363005/diff/40001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/1102363005/diff/40001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:342: (cdm != nullptr ? "" : " could not be") +
nit: Swap so we have positive logic. Also, just "cdm" should be sufficient.

[jrummell, 2015-05-05 17:12:18]

jrummell@chromium.org changed reviewers:
+ bbudge@chromium.org, dcheng@chromium.org

[jrummell, 2015-05-05 17:12:18]

+dcheng@ for ppapi/proxy/ppapi_messages.h
+bbudge@ for the rest of the Pepper code

https://codereview.chromium.org/1102363005/diff/40001/media/cdm/ppapi/cdm_ada...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/1102363005/diff/40001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:342: (cdm != nullptr ? "" : " could not be") +
On 2015/05/05 15:34:50, ddorwin wrote:
> nit: Swap so we have positive logic. Also, just "cdm" should be sufficient.

Done.

[bbudge, 2015-05-05 17:40:03]

Pepper LGTM w/formatting nit

https://codereview.chromium.org/1102363005/diff/60001/ppapi/cpp/private/conte...
File ppapi/cpp/private/content_decryptor_private.cc (right):

https://codereview.chromium.org/1102363005/diff/60001/ppapi/cpp/private/conte...
ppapi/cpp/private/content_decryptor_private.cc:42: ->Initialize(promise_id,
key_system_var.AsString(),
Formatting is strange. Is this from clang format?

[dcheng, 2015-05-05 18:26:52]

ipc changes lgtm

[jrummell, 2015-05-05 18:30:48]

Thanks for the reviews.

https://codereview.chromium.org/1102363005/diff/60001/ppapi/cpp/private/conte...
File ppapi/cpp/private/content_decryptor_private.cc (right):

https://codereview.chromium.org/1102363005/diff/60001/ppapi/cpp/private/conte...
ppapi/cpp/private/content_decryptor_private.cc:42: ->Initialize(promise_id,
key_system_var.AsString(),
On 2015/05/05 17:40:03, bbudge wrote:
> Formatting is strange. Is this from clang format?

Yup.

[jrummell, 2015-05-05 18:30:55]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2015-05-05 18:30:56]

The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1102363005/#ps60001
(title: "nit")

[commit-bot: I haz the power, 2015-05-05 18:34:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1102363005/60001

[commit-bot: I haz the power, 2015-05-05 19:04:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-05 19:05:00]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[jrummell, 2015-05-05 20:49:30]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2015-05-05 20:49:31]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
ddorwin@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1102363005/#ps80001
(title: "rebase + Android compile changes")

[commit-bot: I haz the power, 2015-05-05 20:51:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1102363005/80001

[commit-bot: I haz the power, 2015-05-05 22:27:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-05 22:28:36]

Patchset 5 (id:??) landed as
https://crrev.com/87a2db5daf3fbada56114caa4bb61e596ad459a2
Cr-Commit-Position: refs/heads/master@{#328418}