Encrypted Media: Handle empty init_data in AesDecryptor::GenerateKeyRequest().

Encrypted Media: Handle empty init_data in AesDecryptor::GenerateKeyRequest().

Based on the EME spec, init_data is optional in GenerateKeyRequest(). Update AesDecryptor to support this.

BUG=162759
TEST=added test in AesDecryptorTest to test this case.


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

[xhwang, 2012-11-29 08:52:12]

PTAL!

[ddorwin, 2012-11-29 17:55:08]

LGTM % question and nit.

https://codereview.chromium.org/11434025/diff/1/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

https://codereview.chromium.org/11434025/diff/1/media/crypto/aes_decryptor.cc...
media/crypto/aes_decryptor.cc:143: DVLOG(1) << "Could not generate key request
with empty init_data.";
nit: How about "init_data required to generate a key request." This tells the
reader what they need to do rather than that the code could not do something.

https://codereview.chromium.org/11434025/diff/1/media/crypto/aes_decryptor.cc...
media/crypto/aes_decryptor.cc:145: return false;
Is false going to cause some error to be reported synchronously? What about in
external Clear Key?

[xhwang, 2012-11-29 20:34:36]

PTAL!

Now if init_data is NULL in GKR(), if we use the plugin, we get a keyerror. If
not, we get a MediaKeyException. Would that be acceptable for now?

https://codereview.chromium.org/11434025/diff/1/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

https://codereview.chromium.org/11434025/diff/1/media/crypto/aes_decryptor.cc...
media/crypto/aes_decryptor.cc:143: DVLOG(1) << "Could not generate key request
with empty init_data.";
On 2012/11/29 17:55:08, ddorwin wrote:
> nit: How about "init_data required to generate a key request." This tells the
> reader what they need to do rather than that the code could not do something.

Done.

https://codereview.chromium.org/11434025/diff/1/media/crypto/aes_decryptor.cc...
media/crypto/aes_decryptor.cc:145: return false;
On 2012/11/29 17:55:08, ddorwin wrote:
> Is false going to cause some error to be reported synchronously? What about in
> external Clear Key?

We discussed offline that decryptors other than ProxyDecryptor should not return
"false" on GKR(). I looked the code more closely and found returning "false" IS
desired in some cases. For example, in PpapiDecryptor, after it returns false,
it'll assume AddKey() will never be called:
http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/media/crypto/ppapi_...
If we return true here, we'll have to check |plugin_cdm_delegate_| in a lot of
places (e.g. in AddKey()).

So my gut feeling is that for now we should return MediaKeyException as possible
as we can, and fire KeyError only if we can't return MediaKeyException. (Yes, we
shouldn't have both of then returned in one call.) WDYT?

Ultimately we need to solve this nicely. How about making
PPP::GenerateKeyRequest() a sync message?

[ddorwin, 2012-11-29 22:19:01]

https://codereview.chromium.org/11434025/diff/1/media/crypto/aes_decryptor.cc
File media/crypto/aes_decryptor.cc (right):

https://codereview.chromium.org/11434025/diff/1/media/crypto/aes_decryptor.cc...
media/crypto/aes_decryptor.cc:145: return false;
On 2012/11/29 20:34:37, xhwang wrote:
> On 2012/11/29 17:55:08, ddorwin wrote:
> > Is false going to cause some error to be reported synchronously? What about
in
> > external Clear Key?
> 
> We discussed offline that decryptors other than ProxyDecryptor should not
return
> "false" on GKR(). I looked the code more closely and found returning "false"
IS
> desired in some cases. For example, in PpapiDecryptor, after it returns false,
> it'll assume AddKey() will never be called:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/media/crypto/ppapi_...
> If we return true here, we'll have to check |plugin_cdm_delegate_| in a lot of
> places (e.g. in AddKey()).
> 
> So my gut feeling is that for now we should return MediaKeyException as
possible
> as we can, and fire KeyError only if we can't return MediaKeyException. (Yes,
we
> shouldn't have both of then returned in one call.) WDYT?
> 
> Ultimately we need to solve this nicely. How about making
> PPP::GenerateKeyRequest() a sync message?

No, I don't think we should do that. Let's file a bug to ensure behavior is the
same in internal and external Clear Key and fix it in the OO API.

Does the user of AesDecryptor ensure that an error is reported (cdm_wrapper in
the external case)?

[xhwang, 2012-11-30 04:55:39]

On 2012/11/29 22:19:01, ddorwin wrote:
> https://codereview.chromium.org/11434025/diff/1/media/crypto/aes_decryptor.cc
> File media/crypto/aes_decryptor.cc (right):
> 
>
https://codereview.chromium.org/11434025/diff/1/media/crypto/aes_decryptor.cc...
> media/crypto/aes_decryptor.cc:145: return false;
> On 2012/11/29 20:34:37, xhwang wrote:
> > On 2012/11/29 17:55:08, ddorwin wrote:
> > > Is false going to cause some error to be reported synchronously? What
about
> in
> > > external Clear Key?
> > 
> > We discussed offline that decryptors other than ProxyDecryptor should not
> return
> > "false" on GKR(). I looked the code more closely and found returning "false"
> IS
> > desired in some cases. For example, in PpapiDecryptor, after it returns
false,
> > it'll assume AddKey() will never be called:
> >
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/media/crypto/ppapi_...
> > If we return true here, we'll have to check |plugin_cdm_delegate_| in a lot
of
> > places (e.g. in AddKey()).
> > 
> > So my gut feeling is that for now we should return MediaKeyException as
> possible
> > as we can, and fire KeyError only if we can't return MediaKeyException.
(Yes,
> we
> > shouldn't have both of then returned in one call.) WDYT?
> > 
> > Ultimately we need to solve this nicely. How about making
> > PPP::GenerateKeyRequest() a sync message?
> 
> No, I don't think we should do that. Let's file a bug to ensure behavior is
the
> same in internal and external Clear Key and fix it in the OO API.
> 
> Does the user of AesDecryptor ensure that an error is reported (cdm_wrapper in
> the external case)?

In CdmWrapper, we always fire key error if AesDecryptor returns false in GKR():
http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/media/crypto/ppapi/...

[ddorwin, 2012-11-30 05:08:58]

LGTM. Please file a low priority bug to ensure behavior is the same in internal
and external Clear Key.

[commit-bot: I haz the power, 2012-11-30 06:31:30]

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

[xhwang, 2012-11-30 06:38:48]

On 2012/11/30 05:08:58, ddorwin wrote:
> LGTM. Please file a low priority bug to ensure behavior is the same in
internal
> and external Clear Key.

Bug filed:
http://code.google.com/p/chromium/issues/detail?id=163549

[commit-bot: I haz the power, 2012-11-30 08:31:39]

Change committed as 170424