Generalize Pepper CDM API decrypt and decode.

Generalize Pepper CDM API decrypt and decode.

- Removes PP_EncryptedVideoFrameInfo.
- Replaces PPP_ContentDecryptor_Private::DecryptAndDecodeFrame()
  with DecryptAndDecode().

BUG=141780
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161551

[Tom Finegan, 2012-10-10 07:23:46]

This lands after the video decoder init, and the decoder de-init/reset CLs.

Omitted pnacl_shim.c: simpler to use generate.py after other CLs land, and then
update the CL.

PTAL, thanks.

[Ami GONE FROM CHROMIUM, 2012-10-10 07:30:26]

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
File ppapi/api/private/pp_content_decryptor.idl (right):

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
ppapi/api/private/pp_content_decryptor.idl:121: PP_STREAMTYPE_UNKNOWN = 0,
why does it make sense to have UNKNOWN?

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
ppapi/api/private/pp_content_decryptor.idl:127: *
<code>PP_EncryptedMediaInfo</code> contains the information required to
Does this type justify its existence, as opposed to just passing a StreamType
along w/ the EncryptedBlockInfo to DecryptAndDecode ?

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
ppapi/api/private/pp_content_decryptor.idl:140: * boundary. The value of this
field should not be used.
Why does alignment matter?
(also, what makes you think the base will be 8-byte aligned on a 4-byte-word
arch?)

http://codereview.chromium.org/11087044/diff/1/webkit/media/crypto/ppapi/cdm_...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11087044/diff/1/webkit/media/crypto/ppapi/cdm_...
webkit/media/crypto/ppapi/cdm_wrapper.cc:547: if
(encrypted_media_info.media_type == PP_STREAMTYPE_VIDEO) {
s/VIDEO/AUDIO/
but actually see below

http://codereview.chromium.org/11087044/diff/1/webkit/media/crypto/ppapi/cdm_...
webkit/media/crypto/ppapi/cdm_wrapper.cc:548: PP_DCHECK(false);
DCHECKs shouldn't be handled.
I'd replace l547-550 with:
PP_DCHECK(encrypted_media_info.media_type == PP_STREAMTYPE_VIDEO);

http://codereview.chromium.org/11087044/diff/1/webkit/plugins/ppapi/ppapi_plu...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11087044/diff/1/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1568: media_info.media_type =
PP_STREAMTYPE_UNKNOWN;
shouldn't this be VIDEO until AUDIO support is added?

[Tom Finegan, 2012-10-10 08:26:06]

Some comments responded to with explanation instead of change. PTAL, thanks!

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
File ppapi/api/private/pp_content_decryptor.idl (right):

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
ppapi/api/private/pp_content_decryptor.idl:121: PP_STREAMTYPE_UNKNOWN = 0,
On 2012/10/10 07:30:26, Ami Fischman wrote:
> why does it make sense to have UNKNOWN?

I was thinking of metadata and text streams, and that all our other enums have
UNKNOWN values. Remove?

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
ppapi/api/private/pp_content_decryptor.idl:127: *
<code>PP_EncryptedMediaInfo</code> contains the information required to
On 2012/10/10 07:30:26, Ami Fischman wrote:
> Does this type justify its existence, as opposed to just passing a StreamType
> along w/ the EncryptedBlockInfo to DecryptAndDecode ?

We have to pass a resource around with the info, and I thought it was cleaner to
wrap everything else up in a struct since passing PP_Resources around in a
struct is a bad idea.

Not sure if that justifies the type, or not. :)

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
ppapi/api/private/pp_content_decryptor.idl:140: * boundary. The value of this
field should not be used.
On 2012/10/10 07:30:26, Ami Fischman wrote:
> Why does alignment matter?
> (also, what makes you think the base will be 8-byte aligned on a 4-byte-word
> arch?)

Fair point on alignment. I copied the comment from PP_DecryptedBlockInfo. The
main thing is that size is a multiple of 8 bytes so that PP_EncryptedMediaInfo
is the same size on all targets.

Fixed comment in both places. The padding is in the middle because we end up
with a 256 byte struct in MSVC when it's at the end.

http://codereview.chromium.org/11087044/diff/1/webkit/media/crypto/ppapi/cdm_...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11087044/diff/1/webkit/media/crypto/ppapi/cdm_...
webkit/media/crypto/ppapi/cdm_wrapper.cc:547: if
(encrypted_media_info.media_type == PP_STREAMTYPE_VIDEO) {
On 2012/10/10 07:30:26, Ami Fischman wrote:
> s/VIDEO/AUDIO/
> but actually see below

Done.

http://codereview.chromium.org/11087044/diff/1/webkit/media/crypto/ppapi/cdm_...
webkit/media/crypto/ppapi/cdm_wrapper.cc:548: PP_DCHECK(false);
On 2012/10/10 07:30:26, Ami Fischman wrote:
> DCHECKs shouldn't be handled.
> I'd replace l547-550 with:
> PP_DCHECK(encrypted_media_info.media_type == PP_STREAMTYPE_VIDEO);

Done.

http://codereview.chromium.org/11087044/diff/1/webkit/plugins/ppapi/ppapi_plu...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11087044/diff/1/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1568: media_info.media_type =
PP_STREAMTYPE_UNKNOWN;
On 2012/10/10 07:30:26, Ami Fischman wrote:
> shouldn't this be VIDEO until AUDIO support is added?

Done.

[Ami GONE FROM CHROMIUM, 2012-10-10 18:06:45]

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
File ppapi/api/private/pp_content_decryptor.idl (right):

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
ppapi/api/private/pp_content_decryptor.idl:121: PP_STREAMTYPE_UNKNOWN = 0,
On 2012/10/10 08:26:06, Tom Finegan wrote:
> On 2012/10/10 07:30:26, Ami Fischman wrote:
> > why does it make sense to have UNKNOWN?
> 
> I was thinking of metadata and text streams, and that all our other enums have
> UNKNOWN values. Remove?

yes, remove until needed.

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
ppapi/api/private/pp_content_decryptor.idl:127: *
<code>PP_EncryptedMediaInfo</code> contains the information required to
On 2012/10/10 08:26:06, Tom Finegan wrote:
> On 2012/10/10 07:30:26, Ami Fischman wrote:
> > Does this type justify its existence, as opposed to just passing a
StreamType
> > along w/ the EncryptedBlockInfo to DecryptAndDecode ?
> 
> We have to pass a resource around with the info, and I thought it was cleaner
to
> wrap everything else up in a struct since passing PP_Resources around in a
> struct is a bad idea.
> 
> Not sure if that justifies the type, or not. :)

I don't understand; my proposal is that APIs that in your CL take a
PP_EncryptedMediaInfo should instead take both a PP_EncryptedBlockInfo and a
PP_StreamType.  Are you saying that doesn't work or is clunky for some reason?

http://codereview.chromium.org/11087044/diff/14/ppapi/api/private/pp_content_...
File ppapi/api/private/pp_content_decryptor.idl (right):

http://codereview.chromium.org/11087044/diff/14/ppapi/api/private/pp_content_...
ppapi/api/private/pp_content_decryptor.idl:198: * a multiple of 8 bytes, and
ensure consistent size on all targets.
you dropped the comment that clarifies the contents of the field are unused.
(also above in the other version of this comment)

[Tom Finegan, 2012-10-10 20:02:50]

Comments addressed. PTAL, thanks!

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
File ppapi/api/private/pp_content_decryptor.idl (right):

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
ppapi/api/private/pp_content_decryptor.idl:121: PP_STREAMTYPE_UNKNOWN = 0,
On 2012/10/10 18:06:45, Ami Fischman wrote:
> On 2012/10/10 08:26:06, Tom Finegan wrote:
> > On 2012/10/10 07:30:26, Ami Fischman wrote:
> > > why does it make sense to have UNKNOWN?
> > 
> > I was thinking of metadata and text streams, and that all our other enums
have
> > UNKNOWN values. Remove?
> 
> yes, remove until needed.

Done.

http://codereview.chromium.org/11087044/diff/1/ppapi/api/private/pp_content_d...
ppapi/api/private/pp_content_decryptor.idl:127: *
<code>PP_EncryptedMediaInfo</code> contains the information required to
On 2012/10/10 18:06:45, Ami Fischman wrote:
> On 2012/10/10 08:26:06, Tom Finegan wrote:
> > On 2012/10/10 07:30:26, Ami Fischman wrote:
> > > Does this type justify its existence, as opposed to just passing a
> StreamType
> > > along w/ the EncryptedBlockInfo to DecryptAndDecode ?
> > 
> > We have to pass a resource around with the info, and I thought it was
cleaner
> to
> > wrap everything else up in a struct since passing PP_Resources around in a
> > struct is a bad idea.
> > 
> > Not sure if that justifies the type, or not. :)
> 
> I don't understand; my proposal is that APIs that in your CL take a
> PP_EncryptedMediaInfo should instead take both a PP_EncryptedBlockInfo and a
> PP_StreamType.  Are you saying that doesn't work or is clunky for some reason?

I just thought it was nicer to keep the PP_StreamType and PP_EncryptedBlockInfo
together, but thinking about it some more I've changed my mind and removed
PP_EncryptedMediaInfo. Encryption and decoding are separate actions, and moving
decoder information in a unit with encryption information doesn't make as much
sense to me now as it did last night. I will drink more coffee tonight. :)

http://codereview.chromium.org/11087044/diff/14/ppapi/api/private/pp_content_...
File ppapi/api/private/pp_content_decryptor.idl (right):

http://codereview.chromium.org/11087044/diff/14/ppapi/api/private/pp_content_...
ppapi/api/private/pp_content_decryptor.idl:198: * a multiple of 8 bytes, and
ensure consistent size on all targets.
On 2012/10/10 18:06:45, Ami Fischman wrote:
> you dropped the comment that clarifies the contents of the field are unused.
> (also above in the other version of this comment)

Fixed.

[Ami GONE FROM CHROMIUM, 2012-10-10 20:23:11]

lgtm

[xhwang, 2012-10-10 23:52:33]

lgtm % nits

http://codereview.chromium.org/11087044/diff/9003/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/11087044/diff/9003/ppapi/proxy/ppapi_messages....
ppapi/proxy/ppapi_messages.h:633: std::string /* serialized_encrypted_media_info
*/)
why not serialized_block_info?

http://codereview.chromium.org/11087044/diff/9003/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/11087044/diff/9003/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1577: // TODO(tomfinegan): Need to
get stream type from media stack.
Yep, I'll also do the same (stream type) in Decryptor interface when I add
audio.

[Tom Finegan, 2012-10-11 00:11:33]

+brettw

PTAL. This CL is intended to land after the video decoder intialization and, the
de-init/reset CL. Omitted pnacl_shim.c for the same reasons as before.

http://codereview.chromium.org/11087044/diff/9003/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

http://codereview.chromium.org/11087044/diff/9003/ppapi/proxy/ppapi_messages....
ppapi/proxy/ppapi_messages.h:633: std::string /* serialized_encrypted_media_info
*/)
On 2012/10/10 23:52:33, xhwang wrote:
> why not serialized_block_info?

Missed it, thanks!

[brettw, 2012-10-11 20:47:16]

lgtm

[commit-bot: I haz the power, 2012-10-12 08:16:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/11087044/15001

[commit-bot: I haz the power, 2012-10-12 10:48:54]

Change committed as 161551