Add decoder de-initialize and reset to the Pepper CDM API.

Add decoder de-initialization and reset to the Pepper CDM API.

Adds pepper type PP_StreamType for identifying type of decoder.

BUG=141780
TEST=


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

[Tom Finegan, 2012-10-09 06:17:23]

This adds video decoder stop and reset to the PPAPI CDM interface. It's intended
to land after the video decoder init CL. 

Note: I've intentionally omitted pnacl_shim.c-- merging it is painful. :)

PTAL, thanks!

[Ami GONE FROM CHROMIUM, 2012-10-09 07:25:54]

LGTM % naming nits

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

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:157: void DecoderReset(
s/Reset/ResetDone/ ?  (to clarify observation vs. request)

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:170: void DecoderStopped(
s/Stopped/StopDone/

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

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppp_conte...
ppapi/api/private/ppp_content_decryptor_private.idl:126: void StopVideoDecoder(
Is there a reason this is "Stop" and not "Destroy" other than consistency with
the poor naming in media/ ?

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

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

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:149: * Called after the
<code>ResetAudioDecoder()</code> or
This CL talks about Audio-related methods that don't exist yet.
Not sure how ppapi/OWNERS want to handle that; you might ask explicitly.

[xhwang, 2012-10-09 16:27:27]

Didn't review all files. Just some general questions about this CL.

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

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:157: void DecoderReset(
These two methods are not needed for now, at least in the current plan (using
request id to track callbacks and cancel them immediately upon reset/stop in
PluginInstance instead of waiting for reset/stop to finish).

All pending callbacks (e.g. decrypt_cb in Decrypt(), init_cb in
InitializeVideoDecoder()) are tracked with request ids. So when we cancel, we
can just fire them in the PluginInstance and don't need to wait for the
plugin/cdm to return.

WDYT? +ddorwin/fischman for comments.

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

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppp_conte...
ppapi/api/private/ppp_content_decryptor_private.idl:109: *
<code>DecoderReset()</code> method on the
It looks like the PPB interfaces are sharing APIs between video and audio, e.g.
DecoderReset is used for both video and audio. Why don't also have common APIs
for video/audio in PPP calls? If we do that, we can have something like:
void ResetDecoder(instance, stream_type, request_id), where the stream_type can
be enum/int/bool so that we can differentiate b/w video and auido.

Actually we can also do this for DecryptAndDecodeFrame. If we do that this
function should be renamed to DecryptAndDecode. Also, since we've moved video
initialization out of DecryptAndDecodeFrame, is there any reason that
DecryptAndDecodeFrame takes PP_EncryptedVideoFrameInfo instead of just
PP_EncryptedBlockInfo?

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppp_conte...
ppapi/api/private/ppp_content_decryptor_private.idl:126: void StopVideoDecoder(
On 2012/10/09 07:25:54, Ami Fischman wrote:
> Is there a reason this is "Stop" and not "Destroy" other than consistency with
> the poor naming in media/ ?

In the CDM world, a video decoder can be reinitialized after it's stopped. This
is the plan for handling config change in media source (not implemented yet
though). In this sense, "stop" sounds better then "destroy".

[Ami GONE FROM CHROMIUM, 2012-10-09 16:41:05]

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

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:157: void DecoderReset(
On 2012/10/09 16:27:27, xhwang wrote:
> These two methods are not needed for now, at least in the current plan (using
> request id to track callbacks and cancel them immediately upon reset/stop in
> PluginInstance instead of waiting for reset/stop to finish).
> 
> All pending callbacks (e.g. decrypt_cb in Decrypt(), init_cb in
> InitializeVideoDecoder()) are tracked with request ids. So when we cancel, we
> can just fire them in the PluginInstance and don't need to wait for the
> plugin/cdm to return.
> 
> WDYT? +ddorwin/fischman for comments.

I'm not sure what you're asking.
I thought this was Tom's partial impl of what we discussed, replacing request_id
with callbacks.

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

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppp_conte...
ppapi/api/private/ppp_content_decryptor_private.idl:126: void StopVideoDecoder(
On 2012/10/09 16:27:27, xhwang wrote:
> On 2012/10/09 07:25:54, Ami Fischman wrote:
> > Is there a reason this is "Stop" and not "Destroy" other than consistency
with
> > the poor naming in media/ ?
> 
> In the CDM world, a video decoder can be reinitialized after it's stopped.
This
> is the plan for handling config change in media source (not implemented yet
> though). In this sense, "stop" sounds better then "destroy".

Then how about DeinitializeVideoDecoder()?

Stop implies Start...

[xhwang, 2012-10-09 18:41:20]

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

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:157: void DecoderReset(
On 2012/10/09 16:41:05, Ami Fischman wrote:
> On 2012/10/09 16:27:27, xhwang wrote:
> > These two methods are not needed for now, at least in the current plan
(using
> > request id to track callbacks and cancel them immediately upon reset/stop in
> > PluginInstance instead of waiting for reset/stop to finish).
> > 
> > All pending callbacks (e.g. decrypt_cb in Decrypt(), init_cb in
> > InitializeVideoDecoder()) are tracked with request ids. So when we cancel,
we
> > can just fire them in the PluginInstance and don't need to wait for the
> > plugin/cdm to return.
> > 
> > WDYT? +ddorwin/fischman for comments.
> 
> I'm not sure what you're asking.
> I thought this was Tom's partial impl of what we discussed, replacing
request_id
> with callbacks.  

Chatted with fischman offline again. We'll need reset/stop completion callback
in the future. So please keep these functions.

About request ID, if we drop them, we need a way to distinguish between auido
and video in these shared PPB calls about which decode is reset/stopped. Same
for DeliverBlock.

Let's keep the request IDs for now and update them later.

[Tom Finegan, 2012-10-10 00:39:32]

PTAL at discussion in ppb_content_decryptor_private.idl.

Thanks!

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

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:149: * Called after the
<code>ResetAudioDecoder()</code> or
On 2012/10/09 07:26:38, Ami Fischman wrote:
> This CL talks about Audio-related methods that don't exist yet.
> Not sure how ppapi/OWNERS want to handle that; you might ask explicitly.

Good point. I'll ask Brett once we finish reviewing among ourselves.

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:157: void DecoderReset(
On 2012/10/09 18:41:20, xhwang wrote:
> On 2012/10/09 16:41:05, Ami Fischman wrote:
> > On 2012/10/09 16:27:27, xhwang wrote:
> > > These two methods are not needed for now, at least in the current plan
> (using
> > > request id to track callbacks and cancel them immediately upon reset/stop
in
> > > PluginInstance instead of waiting for reset/stop to finish).
> > > 
> > > All pending callbacks (e.g. decrypt_cb in Decrypt(), init_cb in
> > > InitializeVideoDecoder()) are tracked with request ids. So when we cancel,
> we
> > > can just fire them in the PluginInstance and don't need to wait for the
> > > plugin/cdm to return.
> > > 
> > > WDYT? +ddorwin/fischman for comments.
> > 
> > I'm not sure what you're asking.
> > I thought this was Tom's partial impl of what we discussed, replacing
> request_id
> > with callbacks.  
> 
> Chatted with fischman offline again. We'll need reset/stop completion callback
> in the future. So please keep these functions.
> 
> About request ID, if we drop them, we need a way to distinguish between auido
> and video in these shared PPB calls about which decode is reset/stopped. Same
> for DeliverBlock.
> 
> Let's keep the request IDs for now and update them later.

The request IDs remain, and this method has been renamed to DecoderResetDone.

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:170: void DecoderStopped(
On 2012/10/09 07:25:54, Ami Fischman wrote:
> s/Stopped/StopDone/

Done.

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

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppp_conte...
ppapi/api/private/ppp_content_decryptor_private.idl:109: *
<code>DecoderReset()</code> method on the
On 2012/10/09 16:27:27, xhwang wrote:
> It looks like the PPB interfaces are sharing APIs between video and audio,
e.g.
> DecoderReset is used for both video and audio. Why don't also have common APIs
> for video/audio in PPP calls? If we do that, we can have something like:
> void ResetDecoder(instance, stream_type, request_id), where the stream_type
can
> be enum/int/bool so that we can differentiate b/w video and auido.
> 

+ddorwin,fischman:
We still need audio and video specific delivery methods on the PPB side, but
generalizing DecryptAndDecode, ResetDecoder, and StopDecoder seems like a good
idea with the exception being Ami's noting that if we have an InitializeDecoder
we should have a DeinitializeDecoder (instead of StopDecoder).

> Actually we can also do this for DecryptAndDecodeFrame. If we do that this
> function should be renamed to DecryptAndDecode. Also, since we've moved video
> initialization out of DecryptAndDecodeFrame, is there any reason that
> DecryptAndDecodeFrame takes PP_EncryptedVideoFrameInfo instead of just
> PP_EncryptedBlockInfo?

No, I think we can get rid of PP_EncryptedVideoFrameInfo and do what you've
suggested. 

TODOs:

1) Add PP_DecryptedStreamType to pepper and DecryptedStreamType to CDM.h 
(I am not attached to these names; made them up just now for discussion)
2) Replace cdm::StopVideoDecoder with DeinitializeDecoder
3) As mentioned above, generalize the PPP decode methods (remove the words frame
and video, remove the type PP_EncryptedVideoFrameInfo).
4) Generalize the CDM.h decode methods.

I think 1 and 2 make sense as part of this CL, and 3 and 4 should be another CL.
What does everyone else think?

http://codereview.chromium.org/11028087/diff/2001/ppapi/api/private/ppp_conte...
ppapi/api/private/ppp_content_decryptor_private.idl:126: void StopVideoDecoder(
On 2012/10/09 16:41:05, Ami Fischman wrote:
> On 2012/10/09 16:27:27, xhwang wrote:
> > On 2012/10/09 07:25:54, Ami Fischman wrote:
> > > Is there a reason this is "Stop" and not "Destroy" other than consistency
> with
> > > the poor naming in media/ ?
> > 
> > In the CDM world, a video decoder can be reinitialized after it's stopped.
> This
> > is the plan for handling config change in media source (not implemented yet
> > though). In this sense, "stop" sounds better then "destroy".
> 
> Then how about DeinitializeVideoDecoder()?
> 
> Stop implies Start...

Agreed, discussing this in the above comment.

[Tom Finegan, 2012-10-10 05:10:33]

I finished the renaming and first pass at generalization... PTAL.

Thanks!

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

renamings LGTM

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

+brettw

Brett, PTAL. This is intended to land after the initialize video decoder CL, and
currently omits pnacl_shim.c to avoid the painful merge when it's much simpler
to run generate.py and update this CL after the init CL lands.

Thanks!

[xhwang, 2012-10-11 03:53:35]

lgtm % nits and a question about function ordering.

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

http://codereview.chromium.org/11028087/diff/9018/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:148: /**
Ordering of functions: Can we move these two new functions to after
DeliverSamples. Also, can we put DecoderDeinitializeDone after DecoderResetDone?

http://codereview.chromium.org/11028087/diff/9018/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:154:
*<code>DeinitializeDecoder()</code>.
space before <code>

http://codereview.chromium.org/11028087/diff/9018/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:170:
*<code>ResetDecoder()</code>.
ditto

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

http://codereview.chromium.org/11028087/diff/9018/ppapi/api/private/ppp_conte...
ppapi/api/private/ppp_content_decryptor_private.idl:102: /**
Ditto on order of functions.

[brettw, 2012-10-11 20:53:43]

Note: I put some interface comments in the .h, but you'll want to address them
in the .idl.

LGTM with these addressed.

http://codereview.chromium.org/11028087/diff/12001/ppapi/api/private/pp_conte...
File ppapi/api/private/pp_content_decryptor.idl (right):

http://codereview.chromium.org/11028087/diff/12001/ppapi/api/private/pp_conte...
ppapi/api/private/pp_content_decryptor.idl:268: enum PP_StreamType {
Can you give this struct (and the defines below) a more unique name?
PP_DecryptorStreamType/PP_DECRYPTORSTREAMTYPE_UNKNOWN or something.

http://codereview.chromium.org/11028087/diff/12001/ppapi/c/private/ppp_conten...
File ppapi/c/private/ppp_content_decryptor_private.h (right):

http://codereview.chromium.org/11028087/diff/12001/ppapi/c/private/ppp_conten...
ppapi/c/private/ppp_content_decryptor_private.h:130: void
(*DeinitializeDecoder)(PP_Instance instance,
Do you think CloseDecoder would work here? "Deinitialize" isn't something I
usually see.  I suggest "Close" since we use that in other places.

Can you also clarify in the comment that after deinitialization, will this
decoder ever be used again or is it "dead" at this point?

http://codereview.chromium.org/11028087/diff/12001/ppapi/c/private/ppp_conten...
ppapi/c/private/ppp_content_decryptor_private.h:147: void
(*ResetDecoder)(PP_Instance instance,
Can you also clarify in the comments what will happen after a reset? Like what
call will you get next if it's reused?

http://codereview.chromium.org/11028087/diff/12001/ppapi/thunk/ppb_content_de...
File ppapi/thunk/ppb_content_decryptor_private_thunk.cc (right):

http://codereview.chromium.org/11028087/diff/12001/ppapi/thunk/ppb_content_de...
ppapi/thunk/ppb_content_decryptor_private_thunk.cc:68: if (enter.succeeded())
Nit: Chrome style says to use {} any time the block is >1 line. See also
ppp_content_decryptor_private_proxy.cc

[Tom Finegan, 2012-10-12 05:01:56]

http://codereview.chromium.org/11028087/diff/12001/ppapi/api/private/pp_conte...
File ppapi/api/private/pp_content_decryptor.idl (right):

http://codereview.chromium.org/11028087/diff/12001/ppapi/api/private/pp_conte...
ppapi/api/private/pp_content_decryptor.idl:268: enum PP_StreamType {
On 2012/10/11 20:53:43, brettw wrote:
> Can you give this struct (and the defines below) a more unique name?
> PP_DecryptorStreamType/PP_DECRYPTORSTREAMTYPE_UNKNOWN or something.

Done.

http://codereview.chromium.org/11028087/diff/12001/ppapi/c/private/ppp_conten...
File ppapi/c/private/ppp_content_decryptor_private.h (right):

http://codereview.chromium.org/11028087/diff/12001/ppapi/c/private/ppp_conten...
ppapi/c/private/ppp_content_decryptor_private.h:130: void
(*DeinitializeDecoder)(PP_Instance instance,
On 2012/10/11 20:53:43, brettw wrote:
> Do you think CloseDecoder would work here? "Deinitialize" isn't something I
> usually see.  I suggest "Close" since we use that in other places.
> 

We're using this name for naming symmetry with InitializeVideo/AudioDecoder, and
because it matches what we use in content_decryption_module.h.

> Can you also clarify in the comment that after deinitialization, will this
> decoder ever be used again or is it "dead" at this point?

Clarified the comment.

http://codereview.chromium.org/11028087/diff/12001/ppapi/c/private/ppp_conten...
ppapi/c/private/ppp_content_decryptor_private.h:147: void
(*ResetDecoder)(PP_Instance instance,
On 2012/10/11 20:53:43, brettw wrote:
> Can you also clarify in the comments what will happen after a reset? Like what
> call will you get next if it's reused?

Done.

http://codereview.chromium.org/11028087/diff/12001/ppapi/thunk/ppb_content_de...
File ppapi/thunk/ppb_content_decryptor_private_thunk.cc (right):

http://codereview.chromium.org/11028087/diff/12001/ppapi/thunk/ppb_content_de...
ppapi/thunk/ppb_content_decryptor_private_thunk.cc:68: if (enter.succeeded())
On 2012/10/11 20:53:43, brettw wrote:
> Nit: Chrome style says to use {} any time the block is >1 line. See also
> ppp_content_decryptor_private_proxy.cc

Done.

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

http://codereview.chromium.org/11028087/diff/9018/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:148: /**
On 2012/10/11 03:53:35, xhwang wrote:
> Ordering of functions: Can we move these two new functions to after
> DeliverSamples. Also, can we put DecoderDeinitializeDone after
DecoderResetDone?

Talked about this offline: Keeping this order.

http://codereview.chromium.org/11028087/diff/9018/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:154:
*<code>DeinitializeDecoder()</code>.
On 2012/10/11 03:53:35, xhwang wrote:
> space before <code> 

Done.

http://codereview.chromium.org/11028087/diff/9018/ppapi/api/private/ppb_conte...
ppapi/api/private/ppb_content_decryptor_private.idl:170:
*<code>ResetDecoder()</code>.
On 2012/10/11 03:53:35, xhwang wrote:
> ditto

Done.

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

http://codereview.chromium.org/11028087/diff/9018/ppapi/api/private/ppp_conte...
ppapi/api/private/ppp_content_decryptor_private.idl:102: /**
On 2012/10/11 03:53:35, xhwang wrote:
> Ditto on order of functions.

See previous comment.

[commit-bot: I haz the power, 2012-10-12 05:02:55]

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

[commit-bot: I haz the power, 2012-10-12 07:03:11]

Change committed as 161522