Add PPAPI CDM video decoder initialization.

Add PPAPI CDM video decoder initialization.

Updates PPB and PPP sides of PPAPI Content Decryptor interfaces
to support video decoder intialization. Adds support for decoder
initialization in the PPAPI proxy, plugin instance, and
the CDM wrapper.

Adds new PPAPI types PP_VideoCodecProfile and PP_VideoDecoderConfig.
Adds method InitializeVideoDecoder to PPP_ContentDecryptor_Private.
Adds method DecoderInitialized to PPB_ContentDecryptor_Private.

BUG=141780
TEST=


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

[Tom Finegan, 2012-10-06 23:56:35]

PTAL, thanks!

[xhwang, 2012-10-08 17:16:13]

Nice CL! I reviewed most of the CL but I am not an expert in ppapi
implementation so other people need to review that part.

Please see comments!

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

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:279: * status response.
Add a comment here that the extra data is sent separately as a resource.

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppb_conte...
File ppapi/api/private/ppb_content_decryptor_private.idl (right):

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:163: void
DecoderInitializeStatus(
This name is a little bit strange. How about DecoderInitialized() or
OnDecoderInitialized()?

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

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppp_conte...
ppapi/api/private/ppp_content_decryptor_private.idl:104: * Initializes the video
decoder using codec and settings in
indentation is off by 1

http://codereview.chromium.org/11013052/diff/2001/ppapi/cpp/private/content_d...
File ppapi/cpp/private/content_decryptor_private.h (right):

http://codereview.chromium.org/11013052/diff/2001/ppapi/cpp/private/content_d...
ppapi/cpp/private/content_decryptor_private.h:41: pp::Buffer_Dev
encrypted_buffer) = 0;
s/encrypted_buffer/extra_data_resource/ (or extra_data_buffer?)

http://codereview.chromium.org/11013052/diff/2001/ppapi/native_client/src/unt...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c (right):

http://codereview.chromium.org/11013052/diff/2001/ppapi/native_client/src/unt...
ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:6: /* Last
generated from IDL: Sat Oct  6 16:36:46 2012. */
Oh my, I am overwhelmed by the change in this file :P

http://codereview.chromium.org/11013052/diff/2001/ppapi/proxy/ppp_content_dec...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

http://codereview.chromium.org/11013052/diff/2001/ppapi/proxy/ppp_content_dec...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:117: return false;
wrap this w/ braces since the "if" spans multiple lines?

http://codereview.chromium.org/11013052/diff/2001/ppapi/proxy/ppp_content_dec...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:176: PP_Resource
encrypted_block,
indentation off

http://codereview.chromium.org/11013052/diff/2001/ppapi/proxy/ppp_content_dec...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:398: 
remove extra empty lines

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:547: PP_DCHECK(false);
We need to make clear assumptions about what codec can be passed here. See
comments in PluginInstance.

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:560: PP_DCHECK(false);
ditto

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:577: PP_DCHECK(false);
ditto

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

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:395: case media::kCodecTheora:
Why do we need to list them all? Is it because if we don't we'll get a compiler
warning? Can we just do:

if (codec == media::kCodecVP8)
  return PP_VIDEOCODEC_VP8;

return PP_VIDEOCODEC_UNKNOWN;

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:397: NOTREACHED();
By having NOTREACHED() here, we are assuming that the media pipeline never
passes in unsupported codec for initialization. But how can the media pipeline
know which codec is supported? It seems to me that's the responsibility of the
PluginInstance or the CDM. So here, can we just return PP_VIDEOCODEC_UNKNOWN and
remove the NOTREACHED()?

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:419: case
media::H264PROFILE_MULTIVIEWHIGH:
ditto

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:439: case
media::VideoFrame::NATIVE_TEXTURE:
ditto

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1617:
MediaVideoCodecToPpVideoCodec(decoder_config.codec());
So here if the codec is invalid (unknown), we are still passing them down to the
plugin? But in cdm wrapper, we have a PP_CHECK that the code cannot be unknown.

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1626:
DCHECK(!ContainsKey(pending_decryption_cbs_, pp_decoder_config.request_id));
We'll have at most two pending decoder initialization here, video/audio. Instead
of using a map, how about just use two pending cbs and request id:
pending_video_decoder_init_cb, pending_video_decoder_init_request_id? Later on,
I need to add canceling logic in PluginInstance, so we need to be able to
differentiate video/audio callbacks.

You can also leave this part to me if you want as I am updating
DecryptAndDecodeFrame() in this file.

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1639: return false;
always return false?

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2377: DecryptionCBMap::iterator
found = pending_decryption_cbs_.find(request_id);
ditto, we'll not use map here and use individual variables for pending callback
and request id. Also, use DecoderInitCB instead of DecryptCB.

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

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:267: const
media::Decryptor::DecryptCB& decrypt_cb);
DecoderInitCB

[Ami GONE FROM CHROMIUM, 2012-10-08 18:35:16]

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

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:131: enum PP_VideoCodec {
can probably drop this (see comment below).

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:271: PP_VIDEOCODECPROFILE_UNKNOWN =
0,
Any reason not to use PP_VideoDecoder_Profile?

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:278: * that allows clients to
associate a decoder intialization request with a
typo: intialization
(here and a surprising number of other times ;))

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:309: * Client-specified identifier
for the associated video decoder intialization
I don't follow why this needs an extra id, over returning a resource ID and
making an async callback.
(like the way the reverse works in the PPB_VideoDecoder_Dev case)

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppb_conte...
File ppapi/api/private/ppb_content_decryptor_private.idl (right):

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:12: M23 = 0.3
is any of this really being merged back to 23?  
do you really need to care about versioning as long as nobody's using
internally, and it's in the private/ dir?

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:162: */
Doesn't versioning require marking the method as being new in 0.3 or something?

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

http://codereview.chromium.org/11013052/diff/2001/ppapi/proxy/ppapi_messages....
ppapi/proxy/ppapi_messages.h:630: std::string /* serialized_decoder_config */,
Why serialized and not typed?

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:546: default:
why default if you're handling all enum values?
(here and below)

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:684: void
CdmWrapper::DecoderInitializeStatus(int32_t result,
I'm confused about this being "result" and not "instance", but that's probably
my fault for forgetting too much about how ppapi works.

[xhwang, 2012-10-08 19:00:35]

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

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:309: * Client-specified identifier
for the associated video decoder intialization
On 2012/10/08 18:35:16, Ami Fischman wrote:
> I don't follow why this needs an extra id, over returning a resource ID and
> making an async callback.
> (like the way the reverse works in the PPB_VideoDecoder_Dev case)

Originally request ID was added in Decrypt()/DeliverBlock() to support tracking
concurrent decrypt() calls (e.g. from video and audio streams). That reason
doesn't apply to this struct so I understand why you ask this question. By
acknowledging the fact that we only support at most one video and one audio
stream we can even remove the request ID for the Decrypt()/DeliverBlock() case.

However, I'd like to keep the request ID for all content decryption related
callbacks (including this one) if the operation needs to be cancelable.

In current media pipeline, when we reset/stop the video decoder and if there are
callbacks outstanding, the video decoder will try to cancel the operation and
wait for the callback to return before reset/stop can be finished. For example,
if the video decoder initialization is pending at the plugin, the video decoder
will call CancelDecryptAndDecodeVideo() trying to cancel the initialization.

I feel it'd be better to make the requests cancellable at the PluginInstance
than having to wait for the plugin or CDM to respond. It'll be unfortunate if
the mediapipeline tearing-down halts due to a malfunctioning/bad CDM
implementation (what if the plugin never returns our callback?). To make the
callbacks cancellable in PluginInstance, we need to keep track all callbacks we
sent out, and the request id serves this purpose.

[Tom Finegan, 2012-10-08 23:23:27]

PTAL-- comments mostly addressed, but there's probably some discussion required
for some of them.

Thanks!

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

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:131: enum PP_VideoCodec {
On 2012/10/08 18:35:16, Ami Fischman wrote:
> can probably drop this (see comment below).

We'll need both when we support H.264-- I think we should keep both structs. We
need both to initialize a H.264 decoder, right?

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:271: PP_VIDEOCODECPROFILE_UNKNOWN =
0,
On 2012/10/08 18:35:16, Ami Fischman wrote:
> Any reason not to use PP_VideoDecoder_Profile?

These types are based on those defined in media/video_decoder_config.h. I took
the names from their and stuck pepper prefixes on them. PP_VideoCodec and
PP_VideoCodecProfile seem to fit together, but if we only need a
PP_VideoDecoderProfile I would be fine w/that name and dropping the other type.

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:278: * that allows clients to
associate a decoder intialization request with a
On 2012/10/08 18:35:16, Ami Fischman wrote:
> typo: intialization
> (here and a surprising number of other times ;))

Thanks, fixed.

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:279: * status response.
On 2012/10/08 17:16:13, xhwang wrote:
> Add a comment here that the extra data is sent separately as a resource.

Done.

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppb_conte...
File ppapi/api/private/ppb_content_decryptor_private.idl (right):

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:12: M23 = 0.3
On 2012/10/08 18:35:16, Ami Fischman wrote:
> is any of this really being merged back to 23?

I don't think so. Changed to M24.

> do you really need to care about versioning as long as nobody's using
> internally, and it's in the private/ dir?

Brettw asked that I increase the version number for the last interface change. I
don't really know what the policy is on versioning of these interfaces.

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:162: */
On 2012/10/08 18:35:16, Ami Fischman wrote:
> Doesn't versioning require marking the method as being new in 0.3 or
something?

Agreed, but media is the only client using this interface (via PluginInstance).
I don't think it's necessary to add versioning information here until after
we've completed the whole interface (i.e. this CL + the audio version of the
same).

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:163: void
DecoderInitializeStatus(
On 2012/10/08 17:16:13, xhwang wrote:
> This name is a little bit strange. How about DecoderInitialized() or
> OnDecoderInitialized()?

Done.

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

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppp_conte...
ppapi/api/private/ppp_content_decryptor_private.idl:104: * Initializes the video
decoder using codec and settings in
On 2012/10/08 17:16:13, xhwang wrote:
> indentation is off by 1

Done.

http://codereview.chromium.org/11013052/diff/2001/ppapi/cpp/private/content_d...
File ppapi/cpp/private/content_decryptor_private.h (right):

http://codereview.chromium.org/11013052/diff/2001/ppapi/cpp/private/content_d...
ppapi/cpp/private/content_decryptor_private.h:41: pp::Buffer_Dev
encrypted_buffer) = 0;
On 2012/10/08 17:16:13, xhwang wrote:
> s/encrypted_buffer/extra_data_resource/ (or extra_data_buffer?)

Done.

http://codereview.chromium.org/11013052/diff/2001/ppapi/native_client/src/unt...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c (right):

http://codereview.chromium.org/11013052/diff/2001/ppapi/native_client/src/unt...
ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:6: /* Last
generated from IDL: Sat Oct  6 16:36:46 2012. */
On 2012/10/08 17:16:13, xhwang wrote:
> Oh my, I am overwhelmed by the change in this file :P

Merging and rebasing this file is even more fun than attempting to review it. ;)

And I made this even noisier with the change from M23 to M24.

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

http://codereview.chromium.org/11013052/diff/2001/ppapi/proxy/ppapi_messages....
ppapi/proxy/ppapi_messages.h:630: std::string /* serialized_decoder_config */,
On 2012/10/08 18:35:16, Ami Fischman wrote:
> Why serialized and not typed?

Convention, but I'm not sure I understand the question. Are you asking why
PP_VideoDecoderConfig is shoved into a std::string instead of passed directly?

http://codereview.chromium.org/11013052/diff/2001/ppapi/proxy/ppp_content_dec...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

http://codereview.chromium.org/11013052/diff/2001/ppapi/proxy/ppp_content_dec...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:117: return false;
On 2012/10/08 17:16:13, xhwang wrote:
> wrap this w/ braces since the "if" spans multiple lines?

dmichael@ said not to do that in here in a previous CL. :)

http://codereview.chromium.org/11013052/diff/2001/ppapi/proxy/ppp_content_dec...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:176: PP_Resource
encrypted_block,
On 2012/10/08 17:16:13, xhwang wrote:
> indentation off

Done.

http://codereview.chromium.org/11013052/diff/2001/ppapi/proxy/ppp_content_dec...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:398: 
On 2012/10/08 17:16:13, xhwang wrote:
> remove extra empty lines

Done.

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:546: default:
On 2012/10/08 18:35:16, Ami Fischman wrote:
> why default if you're handling all enum values?
> (here and below)

Removed defaults.

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:547: PP_DCHECK(false);
On 2012/10/08 17:16:13, xhwang wrote:
> We need to make clear assumptions about what codec can be passed here. See
> comments in PluginInstance.

I've removed DCHECKs (and moved these up with the already existing converter
function in the anonymous namespace). Look better?

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:560: PP_DCHECK(false);
On 2012/10/08 17:16:13, xhwang wrote:
> ditto

Done.

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:577: PP_DCHECK(false);
On 2012/10/08 17:16:13, xhwang wrote:
> ditto

Done.

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:684: void
CdmWrapper::DecoderInitializeStatus(int32_t result,
On 2012/10/08 18:35:16, Ami Fischman wrote:
> I'm confused about this being "result" and not "instance", but that's probably
> my fault for forgetting too much about how ppapi works.

The pp::CompletionCallback templates all require a result parameter as the first
argument to the callback.

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

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:395: case media::kCodecTheora:
On 2012/10/08 17:16:13, xhwang wrote:
> Why do we need to list them all? Is it because if we don't we'll get a
compiler
> warning? Can we just do:
> 
> if (codec == media::kCodecVP8)
>   return PP_VIDEOCODEC_VP8;
> 
> return PP_VIDEOCODEC_UNKNOWN;

Done.

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:397: NOTREACHED();
On 2012/10/08 17:16:13, xhwang wrote:
> By having NOTREACHED() here, we are assuming that the media pipeline never
> passes in unsupported codec for initialization. But how can the media pipeline
> know which codec is supported? It seems to me that's the responsibility of the
> PluginInstance or the CDM. So here, can we just return PP_VIDEOCODEC_UNKNOWN
and
> remove the NOTREACHED()?

Removed the NOTREACHED (here and in other conversion functions), and removed the
PP_DCHECKs from conversion functions in the wrapper.

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:419: case
media::H264PROFILE_MULTIVIEWHIGH:
On 2012/10/08 17:16:13, xhwang wrote:
> ditto

Done.

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:439: case
media::VideoFrame::NATIVE_TEXTURE:
On 2012/10/08 17:16:13, xhwang wrote:
> ditto

Done.

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1617:
MediaVideoCodecToPpVideoCodec(decoder_config.codec());
On 2012/10/08 17:16:13, xhwang wrote:
> So here if the codec is invalid (unknown), we are still passing them down to
the
> plugin? But in cdm wrapper, we have a PP_CHECK that the code cannot be
unknown.

Done.

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1626:
DCHECK(!ContainsKey(pending_decryption_cbs_, pp_decoder_config.request_id));
On 2012/10/08 17:16:13, xhwang wrote:
> We'll have at most two pending decoder initialization here, video/audio.
Instead
> of using a map, how about just use two pending cbs and request id:
> pending_video_decoder_init_cb, pending_video_decoder_init_request_id? Later
on,
> I need to add canceling logic in PluginInstance, so we need to be able to
> differentiate video/audio callbacks.
> 
> You can also leave this part to me if you want as I am updating
> DecryptAndDecodeFrame() in this file.

Added TODO(xhwang).

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1639: return false;
On 2012/10/08 17:16:13, xhwang wrote:
> always return false?

Oops! Done.

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:2377: DecryptionCBMap::iterator
found = pending_decryption_cbs_.find(request_id);
On 2012/10/08 17:16:13, xhwang wrote:
> ditto, we'll not use map here and use individual variables for pending
callback
> and request id. Also, use DecoderInitCB instead of DecryptCB.

Added TODO(xhwang).

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

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:267: const
media::Decryptor::DecryptCB& decrypt_cb);
On 2012/10/08 17:16:13, xhwang wrote:
> DecoderInitCB

Added TODO(xhwang).

[xhwang, 2012-10-09 00:49:08]

Mostly looking good. Just a few more minor comments. Again, I am not sure if the
ppapi implementation is correct and will defer to bretts/fischman for this part.

Another question (not required for this CL), what's your plan to support
Reset/Stop the decoders?

http://codereview.chromium.org/11013052/diff/2001/ppapi/native_client/src/unt...
File ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c (right):

http://codereview.chromium.org/11013052/diff/2001/ppapi/native_client/src/unt...
ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c:6: /* Last
generated from IDL: Sat Oct  6 16:36:46 2012. */
On 2012/10/08 23:23:27, tomf wrote:
> On 2012/10/08 17:16:13, xhwang wrote:
> > Oh my, I am overwhelmed by the change in this file :P
> 
> Merging and rebasing this file is even more fun than attempting to review it.
;)
> 
> And I made this even noisier with the change from M23 to M24.

;) Good luck and thanks for the effort!

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

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:439: case
media::VideoFrame::NATIVE_TEXTURE:
On 2012/10/08 23:23:27, tomf wrote:
> On 2012/10/08 17:16:13, xhwang wrote:
> > ditto
> 
> Done.

This is not done?

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1626:
DCHECK(!ContainsKey(pending_decryption_cbs_, pp_decoder_config.request_id));
On 2012/10/08 23:23:27, tomf wrote:
> On 2012/10/08 17:16:13, xhwang wrote:
> > We'll have at most two pending decoder initialization here, video/audio.
> Instead
> > of using a map, how about just use two pending cbs and request id:
> > pending_video_decoder_init_cb, pending_video_decoder_init_request_id? Later
> on,
> > I need to add canceling logic in PluginInstance, so we need to be able to
> > differentiate video/audio callbacks.
> > 
> > You can also leave this part to me if you want as I am updating
> > DecryptAndDecodeFrame() in this file.
> 
> Added TODO(xhwang).

Thanks!

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:114:
PpVideoCodecProfileToCdmVideoCodecProfile(PP_VideoCodecProfile profile) {
I am not sure about if this indentation is correct... +fischman?

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:577: cdm_decoder_config.codec =
PpVideoCodecToCdmVideoCodec(decoder_config.codec);
This is okay. We can also set the status to kError right away if the code or
profile is unknown. But either way is okay for me.

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:675: bool success,
indentation is off

[Tom Finegan, 2012-10-09 01:12:42]

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

http://codereview.chromium.org/11013052/diff/2001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:439: case
media::VideoFrame::NATIVE_TEXTURE:
On 2012/10/09 00:49:08, xhwang wrote:
> On 2012/10/08 23:23:27, tomf wrote:
> > On 2012/10/08 17:16:13, xhwang wrote:
> > > ditto
> > 
> > Done.
> 
> This is not done?

Sorry. Missed it, done now.

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:114:
PpVideoCodecProfileToCdmVideoCodecProfile(PP_VideoCodecProfile profile) {
On 2012/10/09 00:49:08, xhwang wrote:
> I am not sure about if this indentation is correct... +fischman?

Yeah, me neither... incredibly long function name. :)

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:577: cdm_decoder_config.codec =
PpVideoCodecToCdmVideoCodec(decoder_config.codec);
On 2012/10/09 00:49:08, xhwang wrote:
> This is okay. We can also set the status to kError right away if the code or
> profile is unknown. But either way is okay for me.

My thinking was to allow the CDMs to decide which codecs and formats are
supported.

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:675: bool success,
On 2012/10/09 00:49:08, xhwang wrote:
> indentation is off

Done.

[Tom Finegan, 2012-10-09 04:34:11]

> Another question (not required for this CL), what's your plan to support
> Reset/Stop the decoders?

I'll add that in a separate CL.

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11013052/diff/2001/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:546: default:
On 2012/10/08 23:23:27, tomf wrote:
> On 2012/10/08 18:35:16, Ami Fischman wrote:
> > why default if you're handling all enum values?
> > (here and below)
> 
> Removed defaults.

Defaults replaced, because MSVC:
d:\src\chromium\src\webkit\media\crypto\ppapi\cdm_wrapper.cc(101): warning
C4715: '`anonymous namespace'::CdmVideoFormatToPpDecryptedFrameFormat' : not all
control paths return a value

It's either the default or a return.

[Ami GONE FROM CHROMIUM, 2012-10-09 07:21:12]

LGTM % nits

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

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/pp_conten...
ppapi/api/private/pp_content_decryptor.idl:271: PP_VIDEOCODECPROFILE_UNKNOWN =
0,
On 2012/10/08 23:23:27, Tom Finegan wrote:
> On 2012/10/08 18:35:16, Ami Fischman wrote:
> > Any reason not to use PP_VideoDecoder_Profile?
> 
> These types are based on those defined in media/video_decoder_config.h.

So is the enum I pointed at (well, they grew together, but they're in sync).

> I took
> the names from their and stuck pepper prefixes on them. PP_VideoCodec and
> PP_VideoCodecProfile seem to fit together, but if we only need a
> PP_VideoDecoderProfile I would be fine w/that name and dropping the other
type.

We don't need the codec as a separate enum here for the same reason we don't
need it in HW-decode: the profile implies the codec.

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppb_conte...
File ppapi/api/private/ppb_content_decryptor_private.idl (right):

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:12: M23 = 0.3
On 2012/10/08 23:23:27, Tom Finegan wrote:
> On 2012/10/08 18:35:16, Ami Fischman wrote:
> > is any of this really being merged back to 23?
> 
> I don't think so. Changed to M24.
> 
> > do you really need to care about versioning as long as nobody's using
> > internally, and it's in the private/ dir?
> 
> Brettw asked that I increase the version number for the last interface change.
I
> don't really know what the policy is on versioning of these interfaces.

Policy is "do what brettw says" ;)

http://codereview.chromium.org/11013052/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:162: */
On 2012/10/08 23:23:27, Tom Finegan wrote:
> On 2012/10/08 18:35:16, Ami Fischman wrote:
> > Doesn't versioning require marking the method as being new in 0.3 or
> something?
> 
> Agreed, but media is the only client using this interface (via
PluginInstance).
> I don't think it's necessary to add versioning information here until after
> we've completed the whole interface (i.e. this CL + the audio version of the
> same). 

I don't understand your statement, but I'll let you hash it out w/ brettw.

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

http://codereview.chromium.org/11013052/diff/2001/ppapi/proxy/ppapi_messages....
ppapi/proxy/ppapi_messages.h:630: std::string /* serialized_decoder_config */,
On 2012/10/08 23:23:27, Tom Finegan wrote:
> On 2012/10/08 18:35:16, Ami Fischman wrote:
> > Why serialized and not typed?
> 
> Convention, but I'm not sure I understand the question. Are you asking why
> PP_VideoDecoderConfig is shoved into a std::string instead of passed directly?

Yes.

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:114:
PpVideoCodecProfileToCdmVideoCodecProfile(PP_VideoCodecProfile profile) {
On 2012/10/09 01:12:42, Tom Finegan wrote:
> On 2012/10/09 00:49:08, xhwang wrote:
> > I am not sure about if this indentation is correct... +fischman?
> 
> Yeah, me neither... incredibly long function name. :)

<MatrixEvade>
rename method to PpVCProfileToCdmVCProfile or even PpVCPToCdmVCP
</MatrixEvade>

http://codereview.chromium.org/11013052/diff/8007/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11013052/diff/8007/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:110: default:
The need for these default: cases when all enum values are handled explicilty is
madness.  Are you sure the compiler is seeing the right type for the enum?
(perhaps an IDL->.h conversion didn't happen or something?)

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

http://codereview.chromium.org/11013052/diff/8007/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:409: else
else is not necessary

[xhwang, 2012-10-09 15:50:49]

please address fischman's comments, otherwise lgtm.

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:577: cdm_decoder_config.codec =
PpVideoCodecToCdmVideoCodec(decoder_config.codec);
On 2012/10/09 01:12:42, Tom Finegan wrote:
> On 2012/10/09 00:49:08, xhwang wrote:
> > This is okay. We can also set the status to kError right away if the code or
> > profile is unknown. But either way is okay for me.
> 
> My thinking was to allow the CDMs to decide which codecs and formats are
> supported.

ok

[Tom Finegan, 2012-10-09 19:49:08]

brettw: I think this is ready for the pepper review, PTAL.

I think I've addressed all standing comments. Thanks for the reviews!

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11013052/diff/7003/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:114:
PpVideoCodecProfileToCdmVideoCodecProfile(PP_VideoCodecProfile profile) {
On 2012/10/09 07:21:12, Ami Fischman wrote:
> On 2012/10/09 01:12:42, Tom Finegan wrote:
> > On 2012/10/09 00:49:08, xhwang wrote:
> > > I am not sure about if this indentation is correct... +fischman?
> > 
> > Yeah, me neither... incredibly long function name. :)
> 
> <MatrixEvade>
> rename method to PpVCProfileToCdmVCProfile or even PpVCPToCdmVCP
> </MatrixEvade>

Done.

http://codereview.chromium.org/11013052/diff/8007/webkit/media/crypto/ppapi/c...
File webkit/media/crypto/ppapi/cdm_wrapper.cc (right):

http://codereview.chromium.org/11013052/diff/8007/webkit/media/crypto/ppapi/c...
webkit/media/crypto/ppapi/cdm_wrapper.cc:110: default:
On 2012/10/09 07:21:12, Ami Fischman wrote:
> The need for these default: cases when all enum values are handled explicilty
is
> madness.  Are you sure the compiler is seeing the right type for the enum?
> (perhaps an IDL->.h conversion didn't happen or something?)

MSVC has the correct type. I don't know why it's requiring the default. Changed
the switches to if/else if's.

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

http://codereview.chromium.org/11013052/diff/8007/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:409: else
On 2012/10/09 07:21:12, Ami Fischman wrote:
> else is not necessary

Done.

[Tom Finegan, 2012-10-09 21:59:17]

FYI re red bots: I think I need to rebase, but I'm going to wait on that until
the review is completed.

[brettw, 2012-10-11 20:45:58]

LGTM. Note: I don't really understand the big picture here and I don't have time
to do so. I just checked that nothing looked obviously wrong or that it worked
the same as the existing stuff (which I hope is correct!). This will also be the
case for the other patches.

http://codereview.chromium.org/11013052/diff/15010/ppapi/proxy/ppp_content_de...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

http://codereview.chromium.org/11013052/diff/15010/ppapi/proxy/ppp_content_de...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:404: const_cast<const
PP_VideoDecoderConfig*>(&decoder_config),
Are you sure you need this const cast? I thought that you could convert a foo*
to a const foo* implicitly.

[Tom Finegan, 2012-10-11 21:04:04]

Thanks for the review! Answered const_cast comment w/another comment. 

Rebase, rebuild, and land in progress... Fingers crossed!

http://codereview.chromium.org/11013052/diff/15010/ppapi/proxy/ppp_content_de...
File ppapi/proxy/ppp_content_decryptor_private_proxy.cc (right):

http://codereview.chromium.org/11013052/diff/15010/ppapi/proxy/ppp_content_de...
ppapi/proxy/ppp_content_decryptor_private_proxy.cc:404: const_cast<const
PP_VideoDecoderConfig*>(&decoder_config),
On 2012/10/11 20:45:58, brettw wrote:
> Are you sure you need this const cast? I thought that you could convert a foo*
> to a const foo* implicitly.

Can't construct the base::Closure because of non-const pointer, according to
clang:

../../ppapi/shared_impl/proxy_lock.h:103:12: note: candidate template ignored:
deduced conflicting types for parameter 'P2' ('const PP_VideoDecoderConfig *'
vs. 'PP_VideoDecoderConfig *')
ReturnType CallWhileUnlocked(ReturnType (*function)(P1, P2, P3),

[brettw, 2012-10-11 21:07:53]

Ah, closure stuff with implied types. Carry on then!

[commit-bot: I haz the power, 2012-10-11 21:32:50]

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

[commit-bot: I haz the power, 2012-10-11 23:36:59]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2012-10-11 23:42:21]

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

[commit-bot: I haz the power, 2012-10-11 23:43:57]

Change committed as 161448