Support LoadSession() in MediaKeys and PPP_ContentDecryptor_Private interface.

Support LoadSession() in MediaKeys and PPP_ContentDecryptor_Private interface.

- Add LoadSession() to all MediaKeys implementations.
- Add LoadSession() support in PPP_ContentDecryptor_Private stack.
- Move "LOAD_SESSION|" parsing code from CdmWrapper to ProxyDecryptor.
- Also update "type" parameter in CreateSession() to "content_type".

BUG=338831
TEST=LoadSession tests in browser_tests still pass.
R=dcheng@chromium.org, ddorwin@chromium.org, dmichael@chromium.org

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

[xhwang, 2014-02-08 01:05:25]

PTAL

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:302: cdm_->GetCdmVersion() <
cdm::ContentDecryptionModule_4::kVersion) {
Currently CdmWrapper doesn't have access to SendSessionError(), so we check the
version here so that if the CDM doesn't support LoadSession, we can fire
SessionError.

[ddorwin, 2014-02-10 19:05:22]

A lot of this is discussion unrelated to LoadSession() and can probably be
addressed in another CL.

https://codereview.chromium.org/131653003/diff/30001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/131653003/diff/30001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:622: //
https://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-medi...
s/tip/default/ wherever you see it.

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/a...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.cc:54: if (content_type ==
"audio/mp4" || content_type == "video/mp4") {
This should be checked elsewhere, which means we could DCHECK at line 59. Why
don't we have this in other implementations?

(See related comments in media_keys.h.)

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.cc:61:
OnSessionError(session_id, media::MediaKeys::kUnknownError, 0);
I don't think we should be throwing this error.

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/c...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:121:
DCHECK(plugin_cdm_delegate_);
Why can this never happen here or in CreateSession()?
It's allowed in all other functions.

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:140:
ReportFailureToCallPlugin(session_id);
Why don't we return here?

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:156:
ReportFailureToCallPlugin(session_id);
and here for consistency

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/c...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:1: // Copyright 2013 T/Chromium
Authors. All rights reserved.
oops

https://codereview.chromium.org/131653003/diff/30001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/131653003/diff/30001/media/base/media_keys.h#...
media/base/media_keys.h:49: // TODO(jrummell): Remove return value when prefixed
API is removed.
Looking at the code, I'm not sure we actually need this return value.

PpapiDecryptor::CreateSession() is the only one that can return false, but this
is based on ContentDecryptorDelegate::CreateSession(), which only ever returns
true. In fact, I think we can remove the return value from all the EME functions
in ContentDecryptorDelegate.
PpapiDecryptor::CreateSession() also checks whether the ContentDecryptorDelegate
member is null. Is a keyerror sufficient in that case?

Android's ProxyMediaKeys::CreateSession() also returns false, but only in
response to the |type|, and it also reports a keyerror. I think we can remove
the return value there. We don't have such checks elsewhere.
Actually, where do we check those. In WD, these types of checks should be done
at a higher level before the task:
https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-....
See below regarding piping things like this through.

Not for this CL, but something we should probably clean up.

https://codereview.chromium.org/131653003/diff/30001/media/base/media_keys.h#...
media/base/media_keys.h:59: virtual bool LoadSession(uint32 session_id,
Similar TODO about removing the return value.

Currently, this return value serves to say that CK does not support
loadSession(). However, this is not sufficient for the WD because this call is
in the task, but the algorithm specifies a synchronous exception in step 1:
https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-...

We may need to pipe through some feature detection from Blink to
media::MediaKeys. Once that's done, we could remove this bool.

So maybe the TODO is different - and even a bug (or covered by the loadSession()
bug).

https://codereview.chromium.org/131653003/diff/30001/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/131653003/diff/30001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:256: session_error_cb_.Run(session_id,
MediaKeys::kUnknownError, 0);
I don't think we need this error. For prefixed, we have the return. For WD, we
need to throw an exception earlier, which should make this a NOTREACHED().

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:253: // creation (CreateSession()) or session
loading (LoadSession()).
Technically, the spec says errors are to be saved then reported when those
functions are called. This implementation may be sufficient, but maybe not if we
need to be more specific than "Unknown". Maybe we should add a TODO or bug to
check this later.

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:301: if (!cdm_ ||
These two reasons for returning an error are distinctly different (and this one
may need to change - see above), so I think it's worth separating the checks and
returns. WDYT?

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:302: cdm_->GetCdmVersion() <
cdm::ContentDecryptionModule_4::kVersion) {
On 2014/02/08 01:05:25, xhwang wrote:
> Currently CdmWrapper doesn't have access to SendSessionError(), so we check
the
> version here so that if the CDM doesn't support LoadSession, we can fire
> SessionError.

Would it be better to temporarily add a bool return to CdmWrapper::LoadSession()
and let the wrapper decide whether it's supported?

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_wrap...
File media/cdm/ppapi/cdm_wrapper.h (right):

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_wrap...
media/cdm/ppapi/cdm_wrapper.h:55: // The CDM interface version this CDM
implements.
This probably shouldn't be exposed. I think a temporary bool (or even
IsLoadSessionSupported() would be better.

[xhwang, 2014-02-10 22:30:42]

comments addressed

[xhwang, 2014-02-10 22:30:54]

PTAL again

https://codereview.chromium.org/131653003/diff/30001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/131653003/diff/30001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:622: //
https://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-medi...
On 2014/02/10 19:05:25, ddorwin wrote:
> s/tip/default/ wherever you see it.

Done.

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/a...
File content/renderer/media/android/proxy_media_keys.cc (right):

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.cc:54: if (content_type ==
"audio/mp4" || content_type == "video/mp4") {
On 2014/02/10 19:05:25, ddorwin wrote:
> This should be checked elsewhere, which means we could DCHECK at line 59. Why
> don't we have this in other implementations?
> 
> (See related comments in media_keys.h.)

Added TODO.

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/a...
content/renderer/media/android/proxy_media_keys.cc:61:
OnSessionError(session_id, media::MediaKeys::kUnknownError, 0);
On 2014/02/10 19:05:25, ddorwin wrote:
> I don't think we should be throwing this error.

Done.

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/c...
File content/renderer/media/crypto/ppapi_decryptor.cc (right):

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:121:
DCHECK(plugin_cdm_delegate_);
On 2014/02/10 19:05:25, ddorwin wrote:
> Why can this never happen here or in CreateSession()?
> It's allowed in all other functions.

Removed.

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:140:
ReportFailureToCallPlugin(session_id);
On 2014/02/10 19:05:25, ddorwin wrote:
> Why don't we return here?

Done.

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/c...
content/renderer/media/crypto/ppapi_decryptor.cc:156:
ReportFailureToCallPlugin(session_id);
On 2014/02/10 19:05:25, ddorwin wrote:
> and here for consistency

Done.

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/c...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/131653003/diff/30001/content/renderer/media/c...
content/renderer/media/crypto/proxy_decryptor.cc:1: // Copyright 2013 T/Chromium
Authors. All rights reserved.
On 2014/02/10 19:05:25, ddorwin wrote:
> oops

Done.

https://codereview.chromium.org/131653003/diff/30001/media/base/media_keys.h
File media/base/media_keys.h (right):

https://codereview.chromium.org/131653003/diff/30001/media/base/media_keys.h#...
media/base/media_keys.h:49: // TODO(jrummell): Remove return value when prefixed
API is removed.
On 2014/02/10 19:05:25, ddorwin wrote:
> Looking at the code, I'm not sure we actually need this return value.
> 
> PpapiDecryptor::CreateSession() is the only one that can return false, but
this
> is based on ContentDecryptorDelegate::CreateSession(), which only ever returns
> true. In fact, I think we can remove the return value from all the EME
functions
> in ContentDecryptorDelegate.
> PpapiDecryptor::CreateSession() also checks whether the
ContentDecryptorDelegate
> member is null. Is a keyerror sufficient in that case?
> 
> Android's ProxyMediaKeys::CreateSession() also returns false, but only in
> response to the |type|, and it also reports a keyerror. I think we can remove
> the return value there. We don't have such checks elsewhere.
> Actually, where do we check those. In WD, these types of checks should be done
> at a higher level before the task:
>
https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-....
> See below regarding piping things like this through.
> 
> Not for this CL, but something we should probably clean up.

Created bug 342510 to track this.

https://codereview.chromium.org/131653003/diff/30001/media/base/media_keys.h#...
media/base/media_keys.h:59: virtual bool LoadSession(uint32 session_id,
On 2014/02/10 19:05:25, ddorwin wrote:
> Similar TODO about removing the return value.
> 
> Currently, this return value serves to say that CK does not support
> loadSession(). However, this is not sufficient for the WD because this call is
> in the task, but the algorithm specifies a synchronous exception in step 1:
>
https://dvcs.w3.org/hg/html-media/raw-file/default/encrypted-media/encrypted-...
> 
> We may need to pipe through some feature detection from Blink to
> media::MediaKeys. Once that's done, we could remove this bool.
> 
> So maybe the TODO is different - and even a bug (or covered by the
loadSession()
> bug).

Removed "bool" return value in this CL for LoadSession(), and fire
SessionError() if AesDecryptor::LoadSession() is called. Also add a TODO to
clean this later.

https://codereview.chromium.org/131653003/diff/30001/media/cdm/aes_decryptor.cc
File media/cdm/aes_decryptor.cc (right):

https://codereview.chromium.org/131653003/diff/30001/media/cdm/aes_decryptor....
media/cdm/aes_decryptor.cc:256: session_error_cb_.Run(session_id,
MediaKeys::kUnknownError, 0);
On 2014/02/10 19:05:25, ddorwin wrote:
> I don't think we need this error. For prefixed, we have the return. For WD, we
> need to throw an exception earlier, which should make this a NOTREACHED().

Done.

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_adap...
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:253: // creation (CreateSession()) or session
loading (LoadSession()).
On 2014/02/10 19:05:25, ddorwin wrote:
> Technically, the spec says errors are to be saved then reported when those
> functions are called. This implementation may be sufficient, but maybe not if
we
> need to be more specific than "Unknown". Maybe we should add a TODO or bug to
> check this later.

Done.

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:301: if (!cdm_ ||
On 2014/02/10 19:05:25, ddorwin wrote:
> These two reasons for returning an error are distinctly different (and this
one
> may need to change - see above), so I think it's worth separating the checks
and
> returns. WDYT?

Done.

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_adap...
media/cdm/ppapi/cdm_adapter.cc:302: cdm_->GetCdmVersion() <
cdm::ContentDecryptionModule_4::kVersion) {
On 2014/02/10 19:05:25, ddorwin wrote:
> On 2014/02/08 01:05:25, xhwang wrote:
> > Currently CdmWrapper doesn't have access to SendSessionError(), so we check
> the
> > version here so that if the CDM doesn't support LoadSession, we can fire
> > SessionError.
> 
> Would it be better to temporarily add a bool return to
CdmWrapper::LoadSession()
> and let the wrapper decide whether it's supported?

Done.

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_wrap...
File media/cdm/ppapi/cdm_wrapper.h (right):

https://codereview.chromium.org/131653003/diff/30001/media/cdm/ppapi/cdm_wrap...
media/cdm/ppapi/cdm_wrapper.h:55: // The CDM interface version this CDM
implements.
On 2014/02/10 19:05:25, ddorwin wrote:
> This probably shouldn't be exposed. I think a temporary bool (or even
> IsLoadSessionSupported() would be better.

Done.

[ddorwin, 2014-02-10 23:26:24]

lgtm % comment

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

https://codereview.chromium.org/131653003/diff/130001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:257: // support more delicate error reporting
(other than "Unknown").
delicate? Maybe specific?

[xhwang, 2014-02-10 23:41:54]

comments addressed

[xhwang, 2014-02-10 23:42:33]

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

https://codereview.chromium.org/131653003/diff/130001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:257: // support more delicate error reporting
(other than "Unknown").
On 2014/02/10 23:26:24, ddorwin wrote:
> delicate? Maybe specific?

Done.

https://codereview.chromium.org/131653003/diff/130001/media/cdm/ppapi/cdm_ada...
media/cdm/ppapi/cdm_adapter.cc:257: // support more delicate error reporting
(other than "Unknown").
On 2014/02/10 23:26:24, ddorwin wrote:
> delicate? Maybe specific?

Done.

[xhwang, 2014-02-10 23:43:50]

dmichael@: Please do an OWNERS review at 
- content/renderer/pepper/content_decryptor_delegate.*
- ppapi/*

[xhwang, 2014-02-10 23:45:48]

dcheng@: Please do an OWNERS review at ppapi/proxy/ppapi_messages.h

[xhwang, 2014-02-10 23:46:23]

dcheng@: Please do an OWNERS review at ppapi/proxy/ppapi_messages.h

[dmichael (off chromium), 2014-02-10 23:56:49]

https://codereview.chromium.org/131653003/diff/480001/ppapi/cpp/private/conte...
File ppapi/cpp/private/content_decryptor_private.h (right):

https://codereview.chromium.org/131653003/diff/480001/ppapi/cpp/private/conte...
ppapi/cpp/private/content_decryptor_private.h:39: const std::string&
web_session_id) = 0;
Don't you need a corresponding change to the body? Without it, I think when the
browser calls LoadSession, it will be dereferencing garbage memory.

[xhwang, 2014-02-11 00:08:29]

comments addressed

[xhwang, 2014-02-11 00:09:54]

https://codereview.chromium.org/131653003/diff/480001/ppapi/cpp/private/conte...
File ppapi/cpp/private/content_decryptor_private.h (right):

https://codereview.chromium.org/131653003/diff/480001/ppapi/cpp/private/conte...
ppapi/cpp/private/content_decryptor_private.h:39: const std::string&
web_session_id) = 0;
On 2014/02/10 23:56:50, dmichael wrote:
> Don't you need a corresponding change to the body? Without it, I think when
the
> browser calls LoadSession, it will be dereferencing garbage memory.

Done. I accidentally reverted the change in the implementations. Thanks for
catching that.

[xhwang, 2014-02-11 23:06:33]

kindly ping? This needs to be landed before the branch cut.

[dcheng, 2014-02-11 23:12:24]

ppapi/proxy/ppapi_messages.h changes LGTM.

Some random nits below that might be nice to fix.

https://codereview.chromium.org/131653003/diff/740001/content/renderer/media/...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/131653003/diff/740001/content/renderer/media/...
content/renderer/media/crypto/proxy_decryptor.cc:94:
sizeof(kPrefixedApiLoadSessionHeader) - 1;
Nit: I'd prefer strlen here. It has the benefit of clarity, and it should be
evaluated at compile time by the compiler and not result in additional overhead
at runtime.

https://codereview.chromium.org/131653003/diff/740001/content/renderer/media/...
content/renderer/media/crypto/proxy_decryptor.cc:232: uint32
ProxyDecryptor::LookupSessionId(const std::string& session_id) {
Minor unrelated nit: I think both of these functions can be const if iterator is
changed to const_iterator below.

[dmichael (off chromium), 2014-02-11 23:30:45]

pepper stuff lgtm

[xhwang, 2014-02-12 02:22:38]

comments addressed

[xhwang, 2014-02-12 02:22:44]

https://codereview.chromium.org/131653003/diff/740001/content/renderer/media/...
File content/renderer/media/crypto/proxy_decryptor.cc (right):

https://codereview.chromium.org/131653003/diff/740001/content/renderer/media/...
content/renderer/media/crypto/proxy_decryptor.cc:94:
sizeof(kPrefixedApiLoadSessionHeader) - 1;
On 2014/02/11 23:12:25, dcheng wrote:
> Nit: I'd prefer strlen here. It has the benefit of clarity, and it should be
> evaluated at compile time by the compiler and not result in additional
overhead
> at runtime.

kPrefixedApiLoadSessionHeader is uint8 array. So to use strlen I have to do a
cast. This code will be changed in a later CL. So I'll just keep it as is in
this CL.

https://codereview.chromium.org/131653003/diff/740001/content/renderer/media/...
content/renderer/media/crypto/proxy_decryptor.cc:232: uint32
ProxyDecryptor::LookupSessionId(const std::string& session_id) {
On 2014/02/11 23:12:25, dcheng wrote:
> Minor unrelated nit: I think both of these functions can be const if iterator
is
> changed to const_iterator below.

Done.

[xhwang, 2014-02-12 02:22:52]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-02-12 02:25:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/131653003/890002

[Paweł Hajdan Jr., 2014-02-12 16:12:20]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-12 16:14:21]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-12 16:16:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/131653003/890002

[xhwang, 2014-02-12 19:06:02]

rebase only

[xhwang, 2014-02-12 19:09:45]

Committed patchset #8 manually as r250769 (presubmit successful).