Update PPP side of Pepper CDM API to support video decoding.

Update PPP side of Pepper CDM API to support video decoding.

This adds the PPP half of the video decoding support changes.

Adds the following pepper types:
- PP_VideoCodec
- PP_DecryptedFrameFormat
- PP_EncryptedVideoFrameInfo

Replaces PPP_ContentDecryptor_Private::DecryptAndDecode with
DecryptAndDecodeFrame.

BUG=141780
TEST=

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

[Tom Finegan, 2012-09-30 01:59:34]

Here's the first pass at the PPP changes, PTAL!

Thanks.

[brettw, 2012-09-30 03:59:16]

pepper integration lgtm, I don't check the details.

[ddorwin, 2012-10-01 00:48:10]

Thanks.
BUG should be 141780 or a sub-bug.
Added fischman@ for feedback on the PP* APIs.

http://codereview.chromium.org/11023004/diff/2001/ppapi/api/private/pp_conten...
File ppapi/api/private/pp_content_decryptor.idl (right):

http://codereview.chromium.org/11023004/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:165: PP_EncryptedBlockInfo
encrypted_frame_info;
This member name is very close to the class name. Should it be encryption_info
or something?

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

http://codereview.chromium.org/11023004/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1569:
DCHECK(encrypted_frame->GetDecryptConfig());
There must be a config, but the frame doesn't need to be encrypted, right? Might
be worth a comment somewhere that this function (and the PP* functions) take
unencrypted frames.

http://codereview.chromium.org/11023004/diff/2001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (left):

http://codereview.chromium.org/11023004/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:264: // TODO(xhwang): Update this
when we need to support decrypt and decode.
Is this TODO really done now?
Does the new TODO in the .cc address this?

[Ami GONE FROM CHROMIUM, 2012-10-01 03:21:40]

I'm late to this party, so this may be an inapplicable concern, but:

Feeding metadata to the decode call puts a non-trivial bitstream-parsing burden
on the caller.  In effect even though decode is happening in the PPP (so
theoretically a caller wouldn't need to know what bitstream parsing is), the
browser must have enough knowledge of the encoding to pull out frame metadata
(SPS/PPS for h264) and parse out single frame bitstream chunks (NALUs, for
h264).

While implementing the PPB_VideoDecoder_Dev interface it became clear pretty
fast that it's better to make the API client have to know much less about the
bitstream format; there the contract is that encoded bitstream is fed to the
decoder and pictures are sent back when they're ready (async'ly).

What you have now feels like libva, where we have to do an ungodly amount of
bitstream-parsing/decoding before the HW can be engaged, compared with a much
more agnostic API like OMX, where we can let the libs/drivers/HW take over. 
(basically all the code in content/common/gpu/media/*h264* is there to support
use of libva, and none of it has an OMX counterpart).

http://codereview.chromium.org/11023004/diff/2001/ppapi/api/private/ppp_conte...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

http://codereview.chromium.org/11023004/diff/2001/ppapi/api/private/ppp_conte...
ppapi/api/private/ppp_content_decryptor_private.idl:114: * needed to decrypt and
decode of the
s/of //

[xhwang, 2012-10-01 06:19:52]

Did review the whole CL; just a general question.

http://codereview.chromium.org/11023004/diff/2001/ppapi/api/private/pp_conten...
File ppapi/api/private/pp_content_decryptor.idl (right):

http://codereview.chromium.org/11023004/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:141: struct
PP_EncryptedVideoFrameInfo {
Since the decoder has been initialized with something like
InitializeVideoDecoder(config), shouldn't the decode already know the format,
codec and sizes? Do we need to pass these info in every encrypted block/frame?
Can we just use PP_EncryptedBlockInfo for the encrypted video frame?

[Tom Finegan, 2012-10-02 02:42:26]

Re Ami's comments:
From my perspective those are some fairly scary observations, but I think that
they need to be revisited later (v2 PPP CDM API, maybe?).

Re xhwang's question:
I think this needs discussion. Beyond some hand waving about adaptive streaming
(see my comment), I'm not sure we need to pass frame format information to every
D&D call. I don't have an immediate answer about how we init the video decoder.
I don't like the idea of adding InitializeVideoDecoder to the pepper interface,
but maybe we need that. 

One thing to note is that as things are right now InitializeVideoDecoder isn't
necessary. If DecryptAndDecodeFrame always calls DecryptAndDecodeVideo w/format
information the CDM can be responsible for initialization (although it will
probably lead to D&DV implementations always starting with an 'is initialized'
check).

https://codereview.chromium.org/11023004/diff/2001/ppapi/api/private/pp_conte...
File ppapi/api/private/pp_content_decryptor.idl (right):

https://codereview.chromium.org/11023004/diff/2001/ppapi/api/private/pp_conte...
ppapi/api/private/pp_content_decryptor.idl:141: struct
PP_EncryptedVideoFrameInfo {
On 2012/10/01 06:19:52, xhwang wrote:
> Since the decoder has been initialized with something like
> InitializeVideoDecoder(config), shouldn't the decode already know the format,
> codec and sizes? Do we need to pass these info in every encrypted block/frame?
> Can we just use PP_EncryptedBlockInfo for the encrypted video frame?

For playing back adaptive content, won't we need the format information to
determine if the decoder needs to be reinitialized?

https://codereview.chromium.org/11023004/diff/2001/ppapi/api/private/pp_conte...
ppapi/api/private/pp_content_decryptor.idl:165: PP_EncryptedBlockInfo
encrypted_frame_info;
On 2012/10/01 00:48:10, ddorwin wrote:
> This member name is very close to the class name. Should it be encryption_info
> or something?

Done.

https://codereview.chromium.org/11023004/diff/2001/ppapi/api/private/ppp_cont...
File ppapi/api/private/ppp_content_decryptor_private.idl (right):

https://codereview.chromium.org/11023004/diff/2001/ppapi/api/private/ppp_cont...
ppapi/api/private/ppp_content_decryptor_private.idl:114: * needed to decrypt and
decode of the
On 2012/10/01 03:21:41, Ami Fischman wrote:
> s/of //

Done, but reworded because the comment was incorrect.

https://codereview.chromium.org/11023004/diff/2001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

https://codereview.chromium.org/11023004/diff/2001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1569:
DCHECK(encrypted_frame->GetDecryptConfig());
On 2012/10/01 00:48:10, ddorwin wrote:
> There must be a config, but the frame doesn't need to be encrypted, right?
Might
> be worth a comment somewhere that this function (and the PP* functions) take
> unencrypted frames.

Added short notes above PPP_ methods in ppapi_plugin_instance.h, and just above
MakeEncryptedBlock.

https://codereview.chromium.org/11023004/diff/2001/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (left):

https://codereview.chromium.org/11023004/diff/2001/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.h:264: // TODO(xhwang): Update this
when we need to support decrypt and decode.
On 2012/10/01 00:48:10, ddorwin wrote:
> Is this TODO really done now?
> Does the new TODO in the .cc address this?

Not really... replaced TODO.

[ddorwin, 2012-10-02 05:36:50]

LGTM % comment.

@fischman: This is intended to be called by src/media, which already has parsed
the stream. Does that cancel out any concern about the burden? I understand that
the HW decode API had unknown clients and would not want to add this burden. We
can discuss Wednesday.

https://codereview.chromium.org/11023004/diff/16004/webkit/plugins/ppapi/ppap...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

https://codereview.chromium.org/11023004/diff/16004/webkit/plugins/ppapi/ppap...
webkit/plugins/ppapi/ppapi_plugin_instance.h:254: // Note: These methods can be
used with unecrypted data.
Really, it's jus the D&D method(s) that this makes sense for. And it should be
that unencrypted data may be _passed_ to them.

[xhwang, 2012-10-03 22:58:21]

lgtm with comment.

http://codereview.chromium.org/11023004/diff/2001/ppapi/api/private/pp_conten...
File ppapi/api/private/pp_content_decryptor.idl (right):

http://codereview.chromium.org/11023004/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:141: struct
PP_EncryptedVideoFrameInfo {
On 2012/10/02 02:42:26, tomf wrote:
> On 2012/10/01 06:19:52, xhwang wrote:
> > Since the decoder has been initialized with something like
> > InitializeVideoDecoder(config), shouldn't the decode already know the
format,
> > codec and sizes? Do we need to pass these info in every encrypted
block/frame?
> > Can we just use PP_EncryptedBlockInfo for the encrypted video frame?
> 
> For playing back adaptive content, won't we need the format information to
> determine if the decoder needs to be reinitialized? 

Re this comment and the general comment.

It's an option to hide initialization in DecryptAndDecodeFrame() calls. But in
that case we need all information needed to initialize a video codec in this
structure. I think we are still missing some, e.g. the extra data. From media
pipeline's perspective, we always know when the config changed, so that is not a
problem.

Since this CL is getting large, I agree we can land this CL now with a TODO that
we'll revisit decoder initialization later.

[Tom Finegan, 2012-10-04 02:22:11]

Addressed comment, renamed cdm::DecryptAndDecodeVideo to DecryptAndDecodeFrame
(to be consistent pepper interface), added TODO about decoder init, and rebased.

CQ'ing.

http://codereview.chromium.org/11023004/diff/16004/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/11023004/diff/16004/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.h:254: // Note: These methods can be
used with unecrypted data.
On 2012/10/02 05:36:50, ddorwin wrote:
> Really, it's jus the D&D method(s) that this makes sense for. And it should be
> that unencrypted data may be _passed_ to them.

Done, I think. Moved comment above PluginInstance::DecryptAndDecode in the cc
file. If that needs to change, we can fix it when we do the audio side.

[commit-bot: I haz the power, 2012-10-04 02:22:51]

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

[commit-bot: I haz the power, 2012-10-04 04:36:16]

Change committed as 160078

[ddorwin, 2012-10-04 16:37:09]

http://codereview.chromium.org/11023004/diff/16004/webkit/plugins/ppapi/ppapi...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/11023004/diff/16004/webkit/plugins/ppapi/ppapi...
webkit/plugins/ppapi/ppapi_plugin_instance.h:254: // Note: These methods can be
used with unecrypted data.
On 2012/10/04 02:22:12, tomf wrote:
> On 2012/10/02 05:36:50, ddorwin wrote:
> > Really, it's jus the D&D method(s) that this makes sense for. And it should
be
> > that unencrypted data may be _passed_ to them.
> 
> Done, I think. Moved comment above PluginInstance::DecryptAndDecode in the cc
> file. If that needs to change, we can fix it when we do the audio side.

The cc comment is fine, but I meant this comment should be above
DecryptAndDecodeFrame (and DecryptAndDecodeSample). Not a big deal; we can fix
it later.