Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10545036:
Add PPAPI decryptor interfaces. (Closed)
|
Created:
8 years, 6 months ago by Tom Finegan Modified:
8 years, 4 months ago CC:
strobe_, alcatraz-eng_google.com Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd PPAPI decryptor interfaces.
BUG=138139
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151571
Patch Set 1 #Patch Set 2 : Add Decrypt/DecryptAndDecode #
Total comments: 12
Patch Set 3 : Fixed arg names in comments #
Total comments: 3
Patch Set 4 : Update interface, add CPP wrapper and add stub plugin. #Patch Set 5 : Updated interface order to match spec #Patch Set 6 : #Patch Set 7 : -t #Patch Set 8 : Add initial PPB interface for feedback #Patch Set 9 : Just because it builds... #Patch Set 10 : Add missing PPB callback args/Demo plugin iteration/allow empty init_data in GenerateKeyRequest #Patch Set 11 : Rebased on origin/master and fixed a minor conflict #
Total comments: 4
Patch Set 12 : Rebase on origin/master and fix PP_Var usage in wrapper #Patch Set 13 : Rebased again #Patch Set 14 : It lives/Needs some hacking to see if the plugin can respond #Patch Set 15 : Rebased (again) #Patch Set 16 : Implement enough thunk stuff to call into PluginInstance CDM stubs #
Total comments: 31
Patch Set 17 : s/ContentDecryptionModule/ContentDecryptor/g #Patch Set 18 : Updates resulting from David's comments #
Total comments: 8
Patch Set 19 : First attempt at CallOnMainThread usage #
Total comments: 10
Patch Set 20 : CallOnMain/scoper stuff. Sorry, rebased again before reading comment. Last time! #Patch Set 21 : More plumbing #
Total comments: 18
Patch Set 22 : Perhaps I've gone too far :) #
Total comments: 33
Patch Set 23 : More plumbing-- not rebased yet/removal of C interface usage still in progress. #Patch Set 24 : CPP interface migration complete #
Total comments: 12
Patch Set 25 : Sorry for the rebase noise-- unavoidable due to move from work to home PC. #
Total comments: 10
Patch Set 26 : Move decryptor from ppapi/examples to webkit/media/crypto. #Patch Set 27 : Revert change to ppapi_tests.gypi. #
Total comments: 68
Patch Set 28 : Changes in response to dmichael's comments/xhwang's testing. #
Total comments: 6
Patch Set 29 : Addressed remaining comments and added more TODOs #Patch Set 30 : Remove callbacks from API and start proxy/out of process support. #Patch Set 31 : Fix comments in content decryptor IDL and the generated C includes. #Patch Set 32 : Add PPB Content Decryptor proxy interface methods. #
Total comments: 12
Patch Set 33 : Add PPP proxy. #
Total comments: 8
Patch Set 34 : Fix PP_Resource/HostResource usage (hopefully), and update comments. #
Total comments: 39
Patch Set 35 : Moved decryptor interfaces from dev to private. #Patch Set 36 : Most comments addressed/breaking interface and implementation into two CLs before next update. #Patch Set 37 : Removed all but the interfaces from the CL. #Patch Set 38 : Cleaned up the comments a bit. #
Total comments: 32
Patch Set 39 : Comment responses... #
Total comments: 41
Patch Set 40 : Addressed ddorwin's comments. #
Total comments: 2
Patch Set 41 : Fixed typo. #
Total comments: 29
Patch Set 42 : Update comments in response to comments on comments... #
Total comments: 6
Patch Set 43 : Fixed nits. #
Total comments: 18
Patch Set 44 : Removed EME flow stuff in favor of slightly more generic mentions of web applications. #
Total comments: 2
Patch Set 45 : Fixed last nit. #
Messages
Total messages: 61 (0 generated)
LG overall. We may need to refine some of the APIs. https://chromiumcodereview.appspot.com/10545036/diff/2001/ppapi/api/dev/ppp_c... File ppapi/api/dev/ppp_content_decryption_module_dev.idl (right): https://chromiumcodereview.appspot.com/10545036/diff/2001/ppapi/api/dev/ppp_c... ppapi/api/dev/ppp_content_decryption_module_dev.idl:23: */ We'll need GenerateKeyRequest() relatively soon and eventually something like CancelKeyRequest(). We'll probably want to provide the key system name in case a single CDM implements multiple key system variations. https://chromiumcodereview.appspot.com/10545036/diff/2001/ppapi/api/dev/ppp_c... ppapi/api/dev/ppp_content_decryption_module_dev.idl:31: * decryptedBlock is valid only after the callback is notified of success. Argument names in comments do not match the arguments. https://chromiumcodereview.appspot.com/10545036/diff/2001/ppapi/api/dev/ppp_c... ppapi/api/dev/ppp_content_decryption_module_dev.idl:37: [in] PP_CompletionCallback callback); I wonder how we can report API-specific errors, such as those in MediaKeyError (or systemCode). Maybe user_data in the callback will need to be a structure with this data. We can add this later if necessary, I guess. https://chromiumcodereview.appspot.com/10545036/diff/2001/ppapi/api/dev/ppp_c... ppapi/api/dev/ppp_content_decryption_module_dev.idl:44: PP_Bool DecryptAndDecode( We're going to need to somehow provide codec information, etc. I guess we can figure that out.
https://chromiumcodereview.appspot.com/10545036/diff/2001/webkit/plugins/ppap... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): https://chromiumcodereview.appspot.com/10545036/diff/2001/webkit/plugins/ppap... webkit/plugins/ppapi/ppapi_plugin_instance.h:241: bool AddKey(PP_Resource key); I think these parameters should be standard Chromium/WebKit types, not PP_*. We should try to keep the PPAPI stuff hidden behind these functions. It's probably TBD what that means for the callback and we may need to see how the caller evolves.
lgtm
https://chromiumcodereview.appspot.com/10545036/diff/2001/ppapi/api/dev/ppp_c... File ppapi/api/dev/ppp_content_decryption_module_dev.idl (right): https://chromiumcodereview.appspot.com/10545036/diff/2001/ppapi/api/dev/ppp_c... ppapi/api/dev/ppp_content_decryption_module_dev.idl:23: */ On 2012/06/06 21:52:52, ddorwin wrote: > We'll need GenerateKeyRequest() relatively soon and eventually something like > CancelKeyRequest(). > > We'll probably want to provide the key system name in case a single CDM > implements multiple key system variations. How about: PP_Bool GenerateKeyRequest( [in] PP_Instance instance, [in] PP_CompletionCallback callback); PP_Bool CancelKeyRequest( [in] PP_Instance instance); https://chromiumcodereview.appspot.com/10545036/diff/2001/ppapi/api/dev/ppp_c... ppapi/api/dev/ppp_content_decryption_module_dev.idl:31: * decryptedBlock is valid only after the callback is notified of success. On 2012/06/06 21:52:52, ddorwin wrote: > Argument names in comments do not match the arguments. Done. https://chromiumcodereview.appspot.com/10545036/diff/2001/ppapi/api/dev/ppp_c... ppapi/api/dev/ppp_content_decryption_module_dev.idl:37: [in] PP_CompletionCallback callback); On 2012/06/06 21:52:52, ddorwin wrote: > I wonder how we can report API-specific errors, such as those in MediaKeyError > (or systemCode). Maybe user_data in the callback will need to be a structure > with this data. We can add this later if necessary, I guess. Yeah, I think that's what we'll have to do if we want to use PP_CompletionCallback as-is. Where would the struct end up getting defined-- is there something similar I can look at to get a head start on adding the error reporting? It should probably be in there from the start. https://chromiumcodereview.appspot.com/10545036/diff/2001/webkit/plugins/ppap... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): https://chromiumcodereview.appspot.com/10545036/diff/2001/webkit/plugins/ppap... webkit/plugins/ppapi/ppapi_plugin_instance.h:241: bool AddKey(PP_Resource key); On 2012/06/06 22:16:20, ddorwin wrote: > I think these parameters should be standard Chromium/WebKit types, not PP_*. We > should try to keep the PPAPI stuff hidden behind these functions. > > It's probably TBD what that means for the callback and we may need to see how > the caller evolves. I assumed PP_Resource was fine-- there are PluginInstance methods using it already. Is there a buffer type that should be used instead?
lgtm
Replies in both revisions. For now, I suggest filling out the IDL then getting feedback from the team. https://chromiumcodereview.appspot.com/10545036/diff/2001/ppapi/api/dev/ppp_c... File ppapi/api/dev/ppp_content_decryption_module_dev.idl (right): https://chromiumcodereview.appspot.com/10545036/diff/2001/ppapi/api/dev/ppp_c... ppapi/api/dev/ppp_content_decryption_module_dev.idl:23: */ On 2012/06/07 00:50:50, tomf wrote: > On 2012/06/06 21:52:52, ddorwin wrote: > > We'll need GenerateKeyRequest() relatively soon and eventually something like > > CancelKeyRequest(). > > > > We'll probably want to provide the key system name in case a single CDM > > implements multiple key system variations. > > How about: > > PP_Bool GenerateKeyRequest( > [in] PP_Instance instance, > [in] PP_CompletionCallback callback); Need key_system and init_data. > > PP_Bool CancelKeyRequest( > [in] PP_Instance instance); Need session_id https://chromiumcodereview.appspot.com/10545036/diff/2001/ppapi/api/dev/ppp_c... ppapi/api/dev/ppp_content_decryption_module_dev.idl:37: [in] PP_CompletionCallback callback); On 2012/06/07 00:50:50, tomf wrote: > On 2012/06/06 21:52:52, ddorwin wrote: > > I wonder how we can report API-specific errors, such as those in MediaKeyError > > (or systemCode). Maybe user_data in the callback will need to be a structure > > with this data. We can add this later if necessary, I guess. > > Yeah, I think that's what we'll have to do if we want to use > PP_CompletionCallback as-is. Where would the struct end up getting defined-- is > there something similar I can look at to get a head start on adding the error > reporting? It should probably be in there from the start. I'm not sure if anyone actually does this. I think the general usage is to pass data to yourself (the same side of the API). Thus, the browser could have the plugin pass data back to it or vice versa. This use case would require both sides to modify/read the data structure. I noticed that some PPP APIs have corresponding PPB APIs that provide callbacks. That's another option, though it's more complex. It might actually be necessary unless the decoded_frame PPB_Buffer can be resized by the plugin. https://chromiumcodereview.appspot.com/10545036/diff/2001/webkit/plugins/ppap... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): https://chromiumcodereview.appspot.com/10545036/diff/2001/webkit/plugins/ppap... webkit/plugins/ppapi/ppapi_plugin_instance.h:241: bool AddKey(PP_Resource key); On 2012/06/07 00:50:50, tomf wrote: > On 2012/06/06 22:16:20, ddorwin wrote: > > I think these parameters should be standard Chromium/WebKit types, not PP_*. > We > > should try to keep the PPAPI stuff hidden behind these functions. > > > > It's probably TBD what that means for the callback and we may need to see how > > the caller evolves. > > I assumed PP_Resource was fine-- there are PluginInstance methods using it > already. Those are all for the PPB_Instance implementation. They aren't the code that src/webkit/ calls. Specifically, we won't want to include src/ppapi/ code in src/webkit/media/. > Is there a buffer type that should be used instead? Not sure. The frame buffers come from src/media/, but I don't think those types are known in src/webkit/media/ and certainly not in src/webkit/plugins/. I think src/media is going to use std::string to hold buffers. We may need to revisit for decrypted_frame, but that's probably a good place to start for now. https://chromiumcodereview.appspot.com/10545036/diff/6001/ppapi/api/dev/ppp_c... File ppapi/api/dev/ppp_content_decryption_module_dev.idl (right): https://chromiumcodereview.appspot.com/10545036/diff/6001/ppapi/api/dev/ppp_c... ppapi/api/dev/ppp_content_decryption_module_dev.idl:26: [in] PP_Resource key); need session_id. (initData is also missing but will probably be dropped so, we can leave that off.) https://chromiumcodereview.appspot.com/10545036/diff/6001/ppapi/api/dev/ppp_c... ppapi/api/dev/ppp_content_decryption_module_dev.idl:35: [in] PP_Resource encrypted_block, Do other APIs provide comments as to what type of resource a parameter is? For example, in this case, it's expected to be a PPB_Buffer_Dev.
https://chromiumcodereview.appspot.com/10545036/diff/6001/ppapi/api/dev/ppp_c... File ppapi/api/dev/ppp_content_decryption_module_dev.idl (right): https://chromiumcodereview.appspot.com/10545036/diff/6001/ppapi/api/dev/ppp_c... ppapi/api/dev/ppp_content_decryption_module_dev.idl:26: [in] PP_Resource key); On 2012/06/07 18:51:26, ddorwin wrote: > need session_id. (initData is also missing but will probably be dropped so, we > can leave that off.) Done/added in new patchset.
Added the first pass at the PPB interface. The three Deliver* methods might be a bit excessive.
Was not really reviewing this CL but trying to learn from it :) Only one question and one nit. http://codereview.chromium.org/10545036/diff/17001/ppapi/c/dev/ppb_content_de... File ppapi/c/dev/ppb_content_decryption_module_dev.h (right): http://codereview.chromium.org/10545036/diff/17001/ppapi/c/dev/ppb_content_de... ppapi/c/dev/ppb_content_decryption_module_dev.h:105: void (*DeliverSamples)(PP_Instance instance, I am a little confused about the difference between "frame" and "samples". Also why sample is in plural but frame is in singular? http://codereview.chromium.org/10545036/diff/17001/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/17001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:303: memcpy(mapper.data(), data.c_str(), data.size()); Not a big difference here, but I feel data.data() (string::data()) reflects the intention better than data.c_str() since we are copying binary data.
The switch from c_str() to data() will get pushed when my build finishes, but I have to run out for a little bit-- I'll upload when I get back. http://codereview.chromium.org/10545036/diff/17001/ppapi/c/dev/ppb_content_de... File ppapi/c/dev/ppb_content_decryption_module_dev.h (right): http://codereview.chromium.org/10545036/diff/17001/ppapi/c/dev/ppb_content_de... ppapi/c/dev/ppb_content_decryption_module_dev.h:105: void (*DeliverSamples)(PP_Instance instance, On 2012/06/18 20:20:41, xhwang wrote: > I am a little confused about the difference between "frame" and "samples". Also > why sample is in plural but frame is in singular? I do this out of habit, but I also thinks it makes the interface easier to grok at first glance. When an interface has methods that deal with audio and video separately I tend to name the video methods DoSomethingWithFrame(), and the audio methods DoSomethingWithSamples(). The former because most video related code I've written deals with single frames, and the latter because when dealing with audio buffers they generally consist of many samples. I would be fine with having DeliverBlock() and a generalized DeliverSampleBuffer() that handles audio and video. This is still in RFC state as far as I'm concerned. Would everyone prefer a single method? http://codereview.chromium.org/10545036/diff/17001/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/17001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:303: memcpy(mapper.data(), data.c_str(), data.size()); On 2012/06/18 20:20:41, xhwang wrote: > Not a big difference here, but I feel data.data() (string::data()) reflects the > intention better than data.c_str() since we are copying binary data. Done.
Apologies for gclient/checkout drama: this version of the patch actually does something (with boring hard-coded strings and a hack version of WebMediaPlayerImpl that has an overloaded generateKeyRequest() that I call from play() :)
lgtm
Added some thunk stuff. Calling into PluginInstance works, but I need to figure out how to make the call into the media stack from PluginInstance (assuming that's what I want to do, anyway). I think I'm doing this wrong at this point-- calls into the thunk code come directly from the CDM wrapper, not via CallOnMainThread. I'm going to look into that next, and also add some temporary clean up code in the PluginInstance methods to fix the leaks I've introduced in the current CL.
Looks fine so far. Looking forward to the next step. I didn't review the c/, cpp/, or thunk code since I'm sure that will change over time. http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppb_content_... File ppapi/api/dev/ppb_content_decryption_module_dev.idl (right): http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryption_module_dev.idl:7: * This file defines the <code>PPP_ContentDecryptionModule_Dev</code> We should probably call this ContentDecryptor or something similar to be more consistent with the other API names. http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryption_module_dev.idl:16: * pointers the browser may implement to support plugins implementing the Why may and not MUST? http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryption_module_dev.idl:44: * must be sent in response to an <code>AddKey()</code> call. Messages may also be sent at other times during a session (not necessarily in response to AddKey()). http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryption_module_dev.idl:50: [in] PP_Resource message, We should define the types of all PP_Resource variables. I can find an example offline. http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryption_module_dev.idl:54: * An error occured in a <code>PPP_ContentDecryptionModule_Dev</code> method, nit: remove ',' http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppp_content_... File ppapi/api/dev/ppp_content_decryption_module_dev.idl (right): http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryption_module_dev.idl:21: * Generates a key request. key_system is an optional param that is used I think KS should be required (at least for the caller to send one) http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryption_module_dev.idl:24: * buffer containing initialization data for the key system(s) implemented s/for..../from the media data./ http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryption_module_dev.idl:25: * by the plugin. response is a data buffer filled only when callback is remove response sentence. http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryption_module_dev.idl:60: * unencrypted frame. Move unencrypted before raw. http://codereview.chromium.org/10545036/diff/32002/ppapi/ppapi_tests.gypi File ppapi/ppapi_tests.gypi (right): http://codereview.chromium.org/10545036/diff/32002/ppapi/ppapi_tests.gypi#new... ppapi/ppapi_tests.gypi:409: 'target_name': 'ppapi_example_content_decryption_module', The "wrapper" will not be a test, so we'll eventually need to move it somewhere. This is fine for now. http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:297: // Creates a PP_Resource containing a PPB_Buffer_Impl, copies |data| into the What did you base this on? It seems we should be able to re-use some code, at least for creation. http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1318: if (key_system_var.value().size() < key_system.size()) What is the purpose of this check, and why is it "<" instead of "!="? I ask because I don't see other similar examples. Is this copied from another file? http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1321: PP_Resource init_resource = 0; Would prefer this to have the full name ("init_data_resource"). You could add "local" to the other one I guess. http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1378: // TODO(tomfinegan): wrap callback in PP_CompletionCallback and pass it to As discussed offline, we probably aren't doing this anymore. http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.h:242: // Implementation of PPP_ContentDecryptionModule_Dev. Not really an implementation of. Provides access to. http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.h:250: const CDMStatusCB& callback); Hopefully this is some type of Base::Bind coming from PpapiDecryptor. http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.h:426: virtual void KeyAdded(PP_Instance instance, As we move to an OOD for the API, I wonder if these 3 events could/should be on the session object. I'm not exactly sure whether that would fit the pattern of PPB Impls. Nothing to do now, but something to keep in mind.
On 2012/07/13 19:19:48, ddorwin wrote: > I didn't review the c/, cpp/, or thunk code since I'm sure that will change over > time. I didn't review the example in detail either for the same reason.
http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppb_content_... File ppapi/api/dev/ppb_content_decryption_module_dev.idl (right): http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryption_module_dev.idl:7: * This file defines the <code>PPP_ContentDecryptionModule_Dev</code> On 2012/07/13 19:19:48, ddorwin wrote: > We should probably call this ContentDecryptor or something similar to be more > consistent with the other API names. Went with ContentDecryptor/Done. http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryption_module_dev.idl:16: * pointers the browser may implement to support plugins implementing the On 2012/07/13 19:19:48, ddorwin wrote: > Why may and not MUST? Lack of certainty at time of writing. Done. :) http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryption_module_dev.idl:44: * must be sent in response to an <code>AddKey()</code> call. On 2012/07/13 19:19:48, ddorwin wrote: > Messages may also be sent at other times during a session (not necessarily in > response to AddKey()). Added another sentence explaining that AddKey is not the only way to receive a KeyMessage. http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryption_module_dev.idl:50: [in] PP_Resource message, On 2012/07/13 19:19:48, ddorwin wrote: > We should define the types of all PP_Resource variables. I can find an example > offline. Done. I followed the doc, but aren't we going to use PPB_Buffer for all binary buffers? In my new patch I'm using PPB_Buffer instead of the "TBD binary buffer" noted in the doc. Should I update the doc, or should I change the patch? :) http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryption_module_dev.idl:54: * An error occured in a <code>PPP_ContentDecryptionModule_Dev</code> method, On 2012/07/13 19:19:48, ddorwin wrote: > nit: remove ',' Done. http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppp_content_... File ppapi/api/dev/ppp_content_decryption_module_dev.idl (right): http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryption_module_dev.idl:21: * Generates a key request. key_system is an optional param that is used On 2012/07/13 19:19:48, ddorwin wrote: > I think KS should be required (at least for the caller to send one) Dropped the bit about it being optional. http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryption_module_dev.idl:24: * buffer containing initialization data for the key system(s) implemented On 2012/07/13 19:19:48, ddorwin wrote: > s/for..../from the media data./ Done. http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryption_module_dev.idl:25: * by the plugin. response is a data buffer filled only when callback is On 2012/07/13 19:19:48, ddorwin wrote: > remove response sentence. Done. http://codereview.chromium.org/10545036/diff/32002/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryption_module_dev.idl:60: * unencrypted frame. On 2012/07/13 19:19:48, ddorwin wrote: > Move unencrypted before raw. Done. http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:297: // Creates a PP_Resource containing a PPB_Buffer_Impl, copies |data| into the On 2012/07/13 19:19:48, ddorwin wrote: > What did you base this on? It seems we should be able to re-use some code, at > least for creation. It's actually a wild guess based on a lot of time with cs.chromium.org and looking at video capture and decoder examples. It's entirely possible I reinvented a wheel here. http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1318: if (key_system_var.value().size() < key_system.size()) On 2012/07/13 19:19:48, ddorwin wrote: > What is the purpose of this check, and why is it "<" instead of "!="? I ask > because I don't see other similar examples. Is this copied from another file? Above is gone now-- I wasn't using StringVar correctly; that's what was causing the crash I mentioned during the standup today. http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1321: PP_Resource init_resource = 0; On 2012/07/13 19:19:48, ddorwin wrote: > Would prefer this to have the full name ("init_data_resource"). You could add > "local" to the other one I guess. Done. http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.h:242: // Implementation of PPP_ContentDecryptionModule_Dev. On 2012/07/13 19:19:48, ddorwin wrote: > Not really an implementation of. Provides access to. Done. http://codereview.chromium.org/10545036/diff/32002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.h:250: const CDMStatusCB& callback); On 2012/07/13 19:19:48, ddorwin wrote: > Hopefully this is some type of Base::Bind coming from PpapiDecryptor. Ok, the typedef is really just there for the sake of being a placeholder.
http://codereview.chromium.org/10545036/diff/51001/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/51001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2004: StringVar* key_system_string = StringVar::FromPPVar(key_system_var); Just checking, are these non-owning pointers? http://codereview.chromium.org/10545036/diff/51001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2008: if (!key_system_string || !session_id_string || !default_url_string) { Does this mean they are NULL or what? Maybe DCHECK on the first two. The last is optional. Or just pass the values back to the callback to handle there. http://codereview.chromium.org/10545036/diff/51001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2033: What about the other values? And where would we actually call a callback? http://codereview.chromium.org/10545036/diff/51001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2034: // Relase the PP_Vars. Release
http://codereview.chromium.org/10545036/diff/51001/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/51001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2004: StringVar* key_system_string = StringVar::FromPPVar(key_system_var); On 2012/07/17 21:47:56, ddorwin wrote: > Just checking, are these non-owning pointers? Correct-- this just gives access to the data. It does not take ownership. http://codereview.chromium.org/10545036/diff/51001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2008: if (!key_system_string || !session_id_string || !default_url_string) { On 2012/07/17 21:47:56, ddorwin wrote: > Does this mean they are NULL or what? > Maybe DCHECK on the first two. The last is optional. Or just pass the values > back to the callback to handle there. It means they're NULL. I'll DCHECK The first two and just pass things through to the callback. http://codereview.chromium.org/10545036/diff/51001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2033: On 2012/07/17 21:47:56, ddorwin wrote: > What about the other values? And where would we actually call a callback? The callback would happen right about where this comment is-- I was planning on passing the StringVar values using the StringVar::value(), but that's not going to work (assuming an async callback, the callback will not be pleased when it gets vars with already freed memory because they've been Release()'d from the tracker). If the callback is synchronous it probably doesn't matter, and cleaning up here would be fine. http://codereview.chromium.org/10545036/diff/51001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2034: // Relase the PP_Vars. On 2012/07/17 21:47:56, ddorwin wrote: > Release Done.
PTAL at the CallOnMainThread usage in content_decryptor.cc-- and while you're in there you might as well peek at DecryptorMessage. Note that I'm misbehaving and not using scopers, but that's mainly because I'm not sure how to setup the dependencies correctly so I can link things in base. I thought adding ppapi_internal.gyp:ppapi_shared to ppapi_example_content_decryptor's dependencies would do it, but I end up w/link errors. Do I need to use base.gyp:base? (the ppapi_shared target depends on ../base/base.gyp:base) Or should I just ignore scopers for now and live w/using delete in my Dispatch method?
Please try to avoid rebasing within a CL unless absolutely necessary or do so outside of a review round. http://codereview.chromium.org/10545036/diff/59001/ppapi/examples/content_dec... File ppapi/examples/content_decryptor/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/59001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:169: if (!pp::Module::Get()->core()->IsMainThread()) { As this function is written, this statement will never be true. Also, we always want the response to be async (I'm pretty sure), so we should just always do this. Long term, it will come from the wrapped CDM, which should have the same behavior. http://codereview.chromium.org/10545036/diff/59001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:170: DecryptorMessage* dm = new (std::nothrow) DecryptorMessage; s/dm/message/ http://codereview.chromium.org/10545036/diff/59001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:170: DecryptorMessage* dm = new (std::nothrow) DecryptorMessage; Why nothrow? We're not checking the pointer. I think we just crash on OOM. http://codereview.chromium.org/10545036/diff/59001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:178: 0, factory_.NewCallback(&CDMWrapper::DispatchKeyMessage, data), 0); data will be leaked if the callback is not called (i.e. this object is destroyed). xhwang pointed out that the examples pass by value/reference: http://code.google.com/searchframe#OAMlx_jo-ck/src/ppapi/utility/completion_c... http://codereview.chromium.org/10545036/diff/59001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:217: DecryptorMessage* message = reinterpret_cast<DecryptorMessage*>(data); This should be moot based on above, but I would just put this in a scoped_ptr and eliminate 224.
KeyMessage is working, but the code could certainly use a look. Thanks! http://codereview.chromium.org/10545036/diff/59001/ppapi/examples/content_dec... File ppapi/examples/content_decryptor/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/59001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:169: if (!pp::Module::Get()->core()->IsMainThread()) { On 2012/07/18 21:48:36, ddorwin wrote: > As this function is written, this statement will never be true. Also, we always > want the response to be async (I'm pretty sure), so we should just always do > this. Long term, it will come from the wrapped CDM, which should have the same > behavior. Done. http://codereview.chromium.org/10545036/diff/59001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:170: DecryptorMessage* dm = new (std::nothrow) DecryptorMessage; On 2012/07/18 21:48:36, ddorwin wrote: > Why nothrow? We're not checking the pointer. I think we just crash on OOM. Removed the nothrow. http://codereview.chromium.org/10545036/diff/59001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:170: DecryptorMessage* dm = new (std::nothrow) DecryptorMessage; On 2012/07/18 21:48:36, ddorwin wrote: > s/dm/message/ Renamed to decryptor_message (because I already have a message). http://codereview.chromium.org/10545036/diff/59001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:178: 0, factory_.NewCallback(&CDMWrapper::DispatchKeyMessage, data), 0); On 2012/07/18 21:48:36, ddorwin wrote: > data will be leaked if the callback is not called (i.e. this object is > destroyed). > xhwang pointed out that the examples pass by value/reference: > http://code.google.com/searchframe#OAMlx_jo-ck/src/ppapi/utility/completion_c... Ah-- I'll update to pass by value in the next pass. Forgot to double check comments before the upload just now. http://codereview.chromium.org/10545036/diff/59001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:217: DecryptorMessage* message = reinterpret_cast<DecryptorMessage*>(data); On 2012/07/18 21:48:36, ddorwin wrote: > This should be moot based on above, but I would just put this in a scoped_ptr > and eliminate 224. Done.
LG so far. Just some additional nits. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... File ppapi/examples/content_decryptor/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:11: #include "ppapi/c/pp_var.h" Shouldn't need anything from c/ except. cpp/ should include everything you need now. (And if you do, maybe you're not using the C++ wrapper. :) ) http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:91: std::string key_; I assume this is temporary for testing? Probably: TODO(tomfinegan): Remove after testing. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:110: decryptor_if_ = static_cast<const PPB_ContentDecryptor_Dev*>( TODO(tomfinegan): Replace with use of C++ interface when ready. ? http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:122: pp::Buffer_Dev init_buffer(init_data); You'll probably need this variable below, meaning it should be outside 121. Fine for now I guess. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:123: if (!buffer_if_->IsBuffer(init_buffer.pp_resource()) || .pp_resource() appears to be unnecessary (line 151) http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:129: using ppapi::StringVar; using should be at the top of the file. Same for 147. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:130: StringVar* key_system_var = ppapi::StringVar::FromPPVar(key_system_arg); no need for ppapi:: here. Update to match 148, which seems better. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:194: if (decryptor_message.get()) { assuming else is an error, fail early and de-indent the rest of the code.
More patchset spam for everyone! I might have gone a little bit too far here. PTAL at my TODOs-- some of them are really reminders for me to ask questions. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... File ppapi/examples/content_decryptor/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:11: #include "ppapi/c/pp_var.h" On 2012/07/23 23:53:36, ddorwin wrote: > Shouldn't need anything from c/ except. cpp/ should include everything you need > now. (And if you do, maybe you're not using the C++ wrapper. :) ) Removed some of the c/ stuff. I'll look into this some more. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:91: std::string key_; On 2012/07/23 23:53:36, ddorwin wrote: > I assume this is temporary for testing? Probably: > TODO(tomfinegan): Remove after testing. I have a TODO there as a result of today's work-- I might be confused. :) Included note about removal/might have made it worse. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:110: decryptor_if_ = static_cast<const PPB_ContentDecryptor_Dev*>( On 2012/07/23 23:53:36, ddorwin wrote: > TODO(tomfinegan): Replace with use of C++ interface when ready. > ? Done. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:122: pp::Buffer_Dev init_buffer(init_data); On 2012/07/23 23:53:36, ddorwin wrote: > You'll probably need this variable below, meaning it should be outside 121. Fine > for now I guess. Yeah-- I'll move it once I'm actually doing something with init_data. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:123: if (!buffer_if_->IsBuffer(init_buffer.pp_resource()) || On 2012/07/23 23:53:36, ddorwin wrote: > .pp_resource() appears to be unnecessary (line 151) Doesn't compile. key is a PP_Resource, but key_buffer is a pp::Buffer_Dev. Maybe I'm making this more difficult because I'm not sticking to cpp/ interfaces. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:129: using ppapi::StringVar; On 2012/07/23 23:53:36, ddorwin wrote: > using should be at the top of the file. > Same for 147. Done. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:130: StringVar* key_system_var = ppapi::StringVar::FromPPVar(key_system_arg); On 2012/07/23 23:53:36, ddorwin wrote: > no need for ppapi:: here. > Update to match 148, which seems better. Done. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:194: if (decryptor_message.get()) { On 2012/07/23 23:53:36, ddorwin wrote: > assuming else is an error, fail early and de-indent the rest of the code. Passing const DecryptorMessage& instead of a void* now, which seems weird since the local goes out of scope before the callback executes.
http://codereview.chromium.org/10545036/diff/74003/ppapi/api/dev/ppb_content_... File ppapi/api/dev/ppb_content_decryptor_dev.idl (right): http://codereview.chromium.org/10545036/diff/74003/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:83: [in] PP_Resource decrypted_frame, /* PPB_Buffer. */ Would decoded_frame be a better name? http://codereview.chromium.org/10545036/diff/74003/ppapi/cpp/dev/content_decr... File ppapi/cpp/dev/content_decryptor_dev.cc (right): http://codereview.chromium.org/10545036/diff/74003/ppapi/cpp/dev/content_decr... ppapi/cpp/dev/content_decryptor_dev.cc:38: kPPPContentDecryptorInterface); nit: put param on line 37 like functions. http://codereview.chromium.org/10545036/diff/74003/ppapi/cpp/dev/content_decr... ppapi/cpp/dev/content_decryptor_dev.cc:43: key)); nit: line up key with session_id http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... File ppapi/examples/content_decryptor/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:58: explicit CDMWrapper(PP_Instance instance, pp::Module* module); Don't need explicit. http://codereview.chromium.org/10545036/diff/74003/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/74003/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:302: if (!data.size()) nit: data.empty()
http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... File ppapi/examples/content_decryptor/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:123: if (!buffer_if_->IsBuffer(init_buffer.pp_resource()) || On 2012/07/24 00:26:21, tomf wrote: > On 2012/07/23 23:53:36, ddorwin wrote: > > .pp_resource() appears to be unnecessary (line 151) > > Doesn't compile. key is a PP_Resource, but key_buffer is a pp::Buffer_Dev. Maybe > I'm making this more difficult because I'm not sticking to cpp/ interfaces. Ahh, the difference with 151 was subtle. See new comment in next revision. http://codereview.chromium.org/10545036/diff/60024/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:194: if (decryptor_message.get()) { On 2012/07/24 00:26:21, tomf wrote: > On 2012/07/23 23:53:36, ddorwin wrote: > > assuming else is an error, fail early and de-indent the rest of the code. > > Passing const DecryptorMessage& instead of a void* now, which seems weird since > the local goes out of scope before the callback executes. I'm not sure of the mechanism, but pass by value, at least, seems to be used elsewhere. I'm not sure whether pass by reference will work. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... File ppapi/examples/content_decryptor/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:11: #include "ppapi/c/dev/ppb_buffer_dev.h" Shouldn't need. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:37: DecryptorMessage DM_Empty() { I think this is discouraged because of the static initialization of complex types (std::string members). Maybe someone else can chime in. The name needs to be changed. You shouldn't need it, though, if you just add a constructor for the struct. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:93: pp::CompletionCallbackFactory<CDMWrapper> factory_; Unless you see this named used elsewhere, you should prepend callback_ or cb_. factory_ is ambiguous. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:96: // testing (I think!). The EME API will eventually be based on object sessions. Whether we represent that in this API or even internally is TBD. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:99: // TODO(tomfinegan): Should these be multimaps of session_id:key_system and There is only one key system per CDM instance. (If they don't match the first call, you can error out. See AesDecryptor.) You'll need to map key IDs to keys. See AesDecryptor. Keep in mind that session ID, keys, etc. are only relevant for clearkeyexternal and will eventually need to be in a different .so. So you may want to at least indicate which need to be moved, if not create a separate (i.e. pimpl) object now. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:114: buffer_if_ = static_cast<const PPB_Buffer_Dev*>( Just use the C++ interface. Should simplify things, including 124. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:134: DecryptorMessage decryptor_message = DM_Empty(); You're just using an assignment operator here. Instead, create a proper constructor for DecryptorMessage. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:150: if (!buffer_if_->IsBuffer(key) || !key_buffer.data()) Shouldn't you be checking IsBuffer(key_buffer.pp_resource)? Sure, it _should_ be the same, but better to check what you're going to use. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:155: DecryptorMessage decryptor_message = DM_Empty(); same http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:158: decryptor_message.key_system = "look up in multimap maybe?"; just use the member - it can't change. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:166: // instance ever have multiple sessions? Yes. This is more for teardown. Not sure we need to stop sending events. Maybe WebKit/Chrome should handle ignoring in-flight callbacks. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:171: DecryptorMessage decryptor_message = DM_Empty(); There is no message to send for cancelKeyRequest. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:247: decryptor_message.media_error, You could do all of the params in these calls like this. Just indent four from the current indentation (two) on the line after KeyError( and it should fit.
http://codereview.chromium.org/10545036/diff/74003/ppapi/api/dev/ppb_content_... File ppapi/api/dev/ppb_content_decryptor_dev.idl (right): http://codereview.chromium.org/10545036/diff/74003/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:83: [in] PP_Resource decrypted_frame, /* PPB_Buffer. */ On 2012/07/24 04:33:41, fgalligan1 wrote: > Would decoded_frame be a better name? IIRC ddorwin specifically asked for decrypted_frame. I think I was using raw_frame initially (or maybe uncompressed). http://codereview.chromium.org/10545036/diff/74003/ppapi/cpp/dev/content_decr... File ppapi/cpp/dev/content_decryptor_dev.cc (right): http://codereview.chromium.org/10545036/diff/74003/ppapi/cpp/dev/content_decr... ppapi/cpp/dev/content_decryptor_dev.cc:38: kPPPContentDecryptorInterface); On 2012/07/24 04:33:41, fgalligan1 wrote: > nit: put param on line 37 like functions. Done. http://codereview.chromium.org/10545036/diff/74003/ppapi/cpp/dev/content_decr... ppapi/cpp/dev/content_decryptor_dev.cc:43: key)); On 2012/07/24 04:33:41, fgalligan1 wrote: > nit: line up key with session_id Done. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... File ppapi/examples/content_decryptor/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:11: #include "ppapi/c/dev/ppb_buffer_dev.h" On 2012/07/24 18:57:26, ddorwin wrote: > Shouldn't need. Done. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:37: DecryptorMessage DM_Empty() { On 2012/07/24 18:57:26, ddorwin wrote: > I think this is discouraged because of the static initialization of complex > types (std::string members). Maybe someone else can chime in. > The name needs to be changed. > You shouldn't need it, though, if you just add a constructor for the struct. Done/added ctor. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:58: explicit CDMWrapper(PP_Instance instance, pp::Module* module); On 2012/07/24 04:33:41, fgalligan1 wrote: > Don't need explicit. Done/that's what I get for copy/pasting. :) http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:93: pp::CompletionCallbackFactory<CDMWrapper> factory_; On 2012/07/24 18:57:26, ddorwin wrote: > Unless you see this named used elsewhere, you should prepend callback_ or cb_. > factory_ is ambiguous. Stole it from: http://code.google.com/p/chromium/source/search?q=CompletionCallbackFactory+f... But I agree w/your comment. Changed to callback_factory_. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:114: buffer_if_ = static_cast<const PPB_Buffer_Dev*>( On 2012/07/24 18:57:26, ddorwin wrote: > Just use the C++ interface. Should simplify things, including 124. Done. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:134: DecryptorMessage decryptor_message = DM_Empty(); On 2012/07/24 18:57:26, ddorwin wrote: > You're just using an assignment operator here. Instead, create a proper > constructor for DecryptorMessage. Done. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:150: if (!buffer_if_->IsBuffer(key) || !key_buffer.data()) On 2012/07/24 18:57:26, ddorwin wrote: > Shouldn't you be checking IsBuffer(key_buffer.pp_resource)? Sure, it _should_ be > the same, but better to check what you're going to use. Removed in favor of using the cpp wrapper. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:155: DecryptorMessage decryptor_message = DM_Empty(); On 2012/07/24 18:57:26, ddorwin wrote: > same Ditto. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:158: decryptor_message.key_system = "look up in multimap maybe?"; On 2012/07/24 18:57:26, ddorwin wrote: > just use the member - it can't change. Done. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:166: // instance ever have multiple sessions? On 2012/07/24 18:57:26, ddorwin wrote: > Yes. > This is more for teardown. Not sure we need to stop sending events. Maybe > WebKit/Chrome should handle ignoring in-flight callbacks. Removed. http://codereview.chromium.org/10545036/diff/74003/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:171: DecryptorMessage decryptor_message = DM_Empty(); On 2012/07/24 18:57:26, ddorwin wrote: > There is no message to send for cancelKeyRequest. Ah, ok. Removed. I'll read over the EME stuff some more-- I probably should have known that already. http://codereview.chromium.org/10545036/diff/74003/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/74003/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:302: if (!data.size()) On 2012/07/24 04:33:41, fgalligan1 wrote: > nit: data.empty() Done.
Note that DecryptAndDecode from the PPP interface, and DeliverFrame/DeliverSamples from the PPB interface have not been implemented. I *think* we're leaving those out for v0.1. Reviewers: the next patchset will be a rebase-- use patch set 24 if 25 exists to avoid rebase noise.
As before, I didn't review the cpp wrappers. http://codereview.chromium.org/10545036/diff/79001/ppapi/examples/content_dec... File ppapi/examples/content_decryptor/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/79001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:30: callback(PP_MakeCompletionCallback(NULL, NULL)) {} Prefer (but I don't believe required) init list ordering to match declaration. http://codereview.chromium.org/10545036/diff/79001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:164: If invalid callback, return false. No sense in decrypting something we can't return. http://codereview.chromium.org/10545036/diff/79001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:166: decryptor_message.message_data = "Pretend I'm decrypted data!"; Need to set the callback. http://codereview.chromium.org/10545036/diff/79001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:167: CallOnMain(callback_factory_.NewCallback(&CDMWrapper::DeliverBlock, This should not be using DecryptorMessage. This is data delivery, which is different from the events. (You may want a different struct for that.) http://codereview.chromium.org/10545036/diff/79001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:225: const DecryptorMessage& decryptor_message) { Wrong type as mentioned above. http://codereview.chromium.org/10545036/diff/79001/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:226: if (decryptor_message.callback.func) { Should this ever be false?
Sorry for the rebase. Moved to my home PC so I could get some more done after mishaps with gerrit[1] and chrome dev channel today[2]. [1] http://goo.gl/Wn81k Mainly just made me look like an idiot in code reviews :), but I had to make a patch. [2] And this (background/what happened to me, then the real problem): http://code.google.com/p/gerrit/issues/detail?id=1485 http://code.google.com/p/chromium/issues/detail?id=138488 https://chromiumcodereview.appspot.com/10545036/diff/79001/ppapi/examples/con... File ppapi/examples/content_decryptor/content_decryptor.cc (right): https://chromiumcodereview.appspot.com/10545036/diff/79001/ppapi/examples/con... ppapi/examples/content_decryptor/content_decryptor.cc:30: callback(PP_MakeCompletionCallback(NULL, NULL)) {} On 2012/07/26 22:20:17, ddorwin wrote: > Prefer (but I don't believe required) init list ordering to match declaration. Removed per later comment. Created DecryptedData struct. https://chromiumcodereview.appspot.com/10545036/diff/79001/ppapi/examples/con... ppapi/examples/content_decryptor/content_decryptor.cc:164: On 2012/07/26 22:20:17, ddorwin wrote: > If invalid callback, return false. No sense in decrypting something we can't > return. Done. https://chromiumcodereview.appspot.com/10545036/diff/79001/ppapi/examples/con... ppapi/examples/content_decryptor/content_decryptor.cc:166: decryptor_message.message_data = "Pretend I'm decrypted data!"; On 2012/07/26 22:20:17, ddorwin wrote: > Need to set the callback. Done. https://chromiumcodereview.appspot.com/10545036/diff/79001/ppapi/examples/con... ppapi/examples/content_decryptor/content_decryptor.cc:167: CallOnMain(callback_factory_.NewCallback(&CDMWrapper::DeliverBlock, On 2012/07/26 22:20:17, ddorwin wrote: > This should not be using DecryptorMessage. This is data delivery, which is > different from the events. (You may want a different struct for that.) As above-- DecryptedData. https://chromiumcodereview.appspot.com/10545036/diff/79001/ppapi/examples/con... ppapi/examples/content_decryptor/content_decryptor.cc:225: const DecryptorMessage& decryptor_message) { On 2012/07/26 22:20:17, ddorwin wrote: > Wrong type as mentioned above. Done. https://chromiumcodereview.appspot.com/10545036/diff/79001/ppapi/examples/con... ppapi/examples/content_decryptor/content_decryptor.cc:226: if (decryptor_message.callback.func) { On 2012/07/26 22:20:17, ddorwin wrote: > Should this ever be false? Nope.
http://codereview.chromium.org/10545036/diff/78002/ppapi/examples/content_dec... File ppapi/examples/content_decryptor/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/78002/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:38: struct DecryptedData { This will be different for each type of data returned. DecryptedBlock or DecryptedBuffer are probably better names. http://codereview.chromium.org/10545036/diff/78002/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:60: const static size_t kSessionIdLength = 8; Bits? Bytes? 8 seems odd for either. I would just use an int. See below. http://codereview.chromium.org/10545036/diff/78002/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:144: session_id_ = base::RandBytesAsString(kSessionIdLength); For Clear Key, I think we should just use an incrementing integer. See http://code.google.com/searchframe#OAMlx_jo-ck/src/media/crypto/aes_decryptor... http://codereview.chromium.org/10545036/diff/78002/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:191: decrypted_data)); This is probably a copy, which means we're copying the data. We should investigate how this works to make sure everything is cleaned up on errors. Maybe we can make the data member a scoped_ptr or something so the buffer will be deleted but does not need to be copied. http://codereview.chromium.org/10545036/diff/78002/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/78002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1375: const CDMStatusCB& callback) { Isn't this class going to need to take ownership of the callback? If so, this can't be const ref. I think it's just going away, though. http://codereview.chromium.org/10545036/diff/78002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1388: pp_callback)); I think we need to eliminate the PP_CompletionCallback in favor of a queue of callbacks or something. You and xhwang can work this out. http://codereview.chromium.org/10545036/diff/78002/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2090: HostGlobals::Get()->GetVarTracker()->ReleaseVar(key_system_var); Are these releases necessary? Should it happen automatically after this function returns?
Is it possible to add a test? http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppb_content_... File ppapi/api/dev/ppb_content_decryptor_dev.idl (right): http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:7: * This file defines the <code>PPP_ContentDecryptor_Dev</code> nit: PPB http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:16: * pointers the browser may implement to support plugins implementing the why "may" implement? Can the browser choose to not implement some of these? Are you implying that some of them may be NULL? (For the record, if what you mean is that the browser may not actually do anything in some of these calls, then it's probably worth being explicit that all the function pointers will be valid) http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:17: * <code>PPP_ContentDecryptor_Dev</code> interface. Can you describe more about what this API is for? Do plugins actually use this, or is it really just a surface for a content decryptor to use to interact with the Browser? Can you point to a spec or anything? If it's not an actual API for a plugin to use, it should be pointed out that it's special and why. http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:22: * A key or license is needed to decrypt media data. Can you rephrase this to say what the method actually does? http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:32: * method on the <code>PPP_ContentDecryptor_Dev</code> interface. If Chrome is calling AddKey first, why would the content decryptor thingy call back to Chrome? Shouldn't the browser already know? http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:40: * A message or request has been generated by or for key_system. For example, Again, I'd rather an active voice statement of what the Browser does when you call this. http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:64: [in] uint16_t system_error); We tend to use int32_t for error codes... are these from a spec? Even if they are 16 bits in the spec, it might be a good idea to use int32_t and maybe define an enum for each if you can enumerate all possible errors. http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppp_content_... File ppapi/api/dev/ppp_content_decryptor_dev.idl (right): http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryptor_dev.idl:19: interface PPP_ContentDecryptor_Dev { We're trying to get away from defining new PPP interfaces. brettw or viettrungluu might have more information, but I think the intent would be that the plugin passes a struct of function pointers in to a PPB call. Or, if it makes sense, even better is just to use CompletionCallbacks as the only way for the browser to call the plugin. I might be more helpful after I read your design document. http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryptor_dev.idl:23: * systems. init_data is a data buffer containing initialization data from Maybe it's just that I'm ignorant of content decryption, but I don't know what sort of string to expect here. Could you provide an example? http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryptor_dev.idl:29: [in] PP_Resource init_data); /* PPB_Buffer. */ FYI: Another option is an ArrayBuffer var. ArrayBuffers have some advantages: 1) Not in dev (if you think this will ever come out of dev, that might matter) 2) Can be posted to/from JavaScript as a JavaScript ArrayBuffer (also might not matter to you.) ArrayBuffer also currently doesn't use shared memory, though, so if this is large, PPB_Buffer might be better. http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryptor_dev.idl:53: [in] PP_CompletionCallback callback); We've never used a PP_CompletionCallback in the plugin side before, and it's not clear what the semantics are. Can you document and explain this? Maybe once I know what you're trying to do, I can help you come up with an alternative (or maybe I'll be convinced that PP_CompletionCallback is fine here). But it definitely needs to be explained. http://codereview.chromium.org/10545036/diff/91001/ppapi/cpp/dev/content_decr... File ppapi/cpp/dev/content_decryptor_dev.h (right): http://codereview.chromium.org/10545036/diff/91001/ppapi/cpp/dev/content_decr... ppapi/cpp/dev/content_decryptor_dev.h:23: virtual bool GenerateKeyRequest(PP_Var key_system, // String. Why not pp::Var instead of PP_Var? http://codereview.chromium.org/10545036/diff/91001/ppapi/cpp/dev/content_decr... ppapi/cpp/dev/content_decryptor_dev.h:24: PP_Resource init_data) = 0; // PPB_Buffer. pp::Buffer_Dev? (unless you go with ArrayBuffer, which would be pp::VarArrayBuffer) http://codereview.chromium.org/10545036/diff/91001/ppapi/cpp/dev/content_decr... ppapi/cpp/dev/content_decryptor_dev.h:32: PP_CompletionCallback callback) = 0; Maybe pp::CompletionCallback, depending on what the intent is... but I'm hoping we can come up with something else anyway for the "plugin" side. http://codereview.chromium.org/10545036/diff/91001/ppapi/proxy/ppb_instance_p... File ppapi/proxy/ppb_instance_proxy.cc (right): http://codereview.chromium.org/10545036/diff/91001/ppapi/proxy/ppb_instance_p... ppapi/proxy/ppb_instance_proxy.cc:537: } Are these intentionally blank? Assuming so, please add TODOs http://codereview.chromium.org/10545036/diff/91001/ppapi/thunk/ppb_content_de... File ppapi/thunk/ppb_content_decryptor_thunk.cc (right): http://codereview.chromium.org/10545036/diff/91001/ppapi/thunk/ppb_content_de... ppapi/thunk/ppb_content_decryptor_thunk.cc:20: if (enter.succeeded()) { nit: no curly braces for 1-liners http://codereview.chromium.org/10545036/diff/91001/webkit/media/crypto/conten... File webkit/media/crypto/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/91001/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:262: class MyModule : public pp::Module { I'm confused about what this file is... is this the "plugin"? (Admitting that IIRC, you guys are doing something a little different than a regular plugin...) If so, you shouldn't be using anything from ppapi/shared_impl (like StringVar::StringToPPVar above). That's for Chrome's internal use only, and using it here is likely to break stuff. http://codereview.chromium.org/10545036/diff/91001/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10545036/diff/91001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:87: '<(DEPTH)/ppapi/../base/base.gyp:base', As noted elsewhere, if this is more like a "plugin", you should leave out ppapi_shared (and probably base as well). I'm also not sure this belongs in webkit...? What's its relationship to WebKit? You might check with somebody like darin or brettw to make sure this is the right place. http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:305: ScopedPPResource resource(PPB_Buffer_Impl::Create(instance, data.size())); I think that using this constructor, ScopedPPResource takes an additional ref, so now you have 2. It looks like you're coding assuming you have one reference below. Maybe you mean the PassRef constructor? http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:315: return 0; I don't think this check is necessary http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:320: PpapiGlobals::Get()->GetResourceTracker()->AddRefResource(resource.get()); ...and Release below should return the ScopedPPResource's reference out, so I think this AddRefResource is unnecessary (even after changing to use the PassRef constructor above). http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:995: module_->GetPluginInterface( nit: I think there should be 2 more spaces here http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1330: init_data)); Why is init_data a string, if you're using it for binary data? I would think something like vector<unsigned char> or scoped_array might be more appropriate? Then again, I'm not clear on what GenerateKeyRequest is for, so maybe there is a goo reason ;-) http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1350: return false; I don't think this check should be necessary http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1368: return false; unnecessary check http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2028: HostGlobals::Get()->GetVarTracker()->ReleaseVar(key_system_var); I may be wrong, but I think that our usual policy for Vars in this situation is that the browser won't release them. The reference count is the same after the method returns as when it's called. Our C++ wrappers, e.g., won't know about the method call here, so you probably would get a double delete.. http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2035: } TODO? http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2054: NOTREACHED(); You shouldn't do NOTREACHED or other crash-inducing things just because a plugin provided bad args (maybe this is just leftover from debugging?) http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2061: NOTREACHED(); Also probably not appropriate to crash here (resource exhaustion?), though I'm less sure http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2065: std::string message(reinterpret_cast<char*>(mapper.data()), mapper.size()); Why are you using a string here? You don't seem to use it, so it's not clear to me what's going on. http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2075: HostGlobals::Get()->GetVarTracker()->ReleaseVar(default_url_var); Like above, you probably shouldn't release the vars here (here and elsewhere) http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2079: HostGlobals::Get()->GetResourceTracker()->ReleaseResource(message_resource); As with Vars, I don't think you want to release the plugin's reference to the Resource (here and elsewhere) http://codereview.chromium.org/10545036/diff/91001/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2107: // Not implemented in v0.1 of the EME/CDM work. TODO might still be good, so curious people know whom to ask about it
Forgot to publish one set of comment responses... http://codereview.chromium.org/10545036/diff/78002/ppapi/examples/content_dec... File ppapi/examples/content_decryptor/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/78002/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:38: struct DecryptedData { On 2012/07/28 01:43:26, ddorwin wrote: > This will be different for each type of data returned. DecryptedBlock or > DecryptedBuffer are probably better names. Renamed to DecryptedBlock. http://codereview.chromium.org/10545036/diff/78002/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:60: const static size_t kSessionIdLength = 8; On 2012/07/28 01:43:26, ddorwin wrote: > Bits? Bytes? > 8 seems odd for either. I would just use an int. See below. Just pulled a number out of the air-- removed, doing something similar to AesDecryptor now. http://codereview.chromium.org/10545036/diff/78002/ppapi/examples/content_dec... ppapi/examples/content_decryptor/content_decryptor.cc:144: session_id_ = base::RandBytesAsString(kSessionIdLength); On 2012/07/28 01:43:26, ddorwin wrote: > For Clear Key, I think we should just use an incrementing integer. See > http://code.google.com/searchframe#OAMlx_jo-ck/src/media/crypto/aes_decryptor... Done.
I finished my first pass through comments, and I made some changes. I have not finished going through all the feedback-- it's mostly nits and TODOs remaining, but there are a couple substantive things that still require attention. Please take a look at the updated stuff-- I'll have another updated patch some time tomorrow that addresses the remaining comments (or asks for more information, at least :) Thanks for the review and for comments-- it's a huge help! https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppb_... File ppapi/api/dev/ppb_content_decryptor_dev.idl (right): https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppb_... ppapi/api/dev/ppb_content_decryptor_dev.idl:7: * This file defines the <code>PPP_ContentDecryptor_Dev</code> On 2012/07/31 03:36:31, dmichael wrote: > nit: PPB Done. https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppb_... ppapi/api/dev/ppb_content_decryptor_dev.idl:16: * pointers the browser may implement to support plugins implementing the On 2012/07/31 03:36:31, dmichael wrote: > why "may" implement? Can the browser choose to not implement some of these? Are > you implying that some of them may be NULL? (For the record, if what you mean is > that the browser may not actually do anything in some of these calls, then it's > probably worth being explicit that all the function pointers will be valid) That should have said MUST-- I missed an earlier comment/or possibly rebased the change out. Sorry about that. https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppb_... ppapi/api/dev/ppb_content_decryptor_dev.idl:17: * <code>PPP_ContentDecryptor_Dev</code> interface. On 2012/07/31 03:36:31, dmichael wrote: > Can you describe more about what this API is for? Do plugins actually use this, > or is it really just a surface for a content decryptor to use to interact with > the Browser? Can you point to a spec or anything? If it's not an actual API for > a plugin to use, it should be pointed out that it's special and why. EME and PPAPI CDM design links sent offline. https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppb_... ppapi/api/dev/ppb_content_decryptor_dev.idl:64: [in] uint16_t system_error); On 2012/07/31 03:36:31, dmichael wrote: > We tend to use int32_t for error codes... are these from a spec? Even if they > are 16 bits in the spec, it might be a good idea to use int32_t and maybe define > an enum for each if you can enumerate all possible errors. Done. https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppp_... File ppapi/api/dev/ppp_content_decryptor_dev.idl (right): https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppp_... ppapi/api/dev/ppp_content_decryptor_dev.idl:19: interface PPP_ContentDecryptor_Dev { On 2012/07/31 03:36:31, dmichael wrote: > We're trying to get away from defining new PPP interfaces. brettw or > viettrungluu might have more information, but I think the intent would be that > the plugin passes a struct of function pointers in to a PPB call. Or, if it > makes sense, even better is just to use CompletionCallbacks as the only way for > the browser to call the plugin. > > I might be more helpful after I read your design document. So the PPB interface would have an OnCreate method (or something), and the plugin would pass the function pointers in at that point? https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppp_... ppapi/api/dev/ppp_content_decryptor_dev.idl:23: * systems. init_data is a data buffer containing initialization data from On 2012/07/31 03:36:31, dmichael wrote: > Maybe it's just that I'm ignorant of content decryption, but I don't know what > sort of string to expect here. Could you provide an example? Sure, here are two examples from webkit/media/crypto/key_systems.cc: webkit-org.w3.clearkey org.chromium.externalclearkey https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppp_... ppapi/api/dev/ppp_content_decryptor_dev.idl:29: [in] PP_Resource init_data); /* PPB_Buffer. */ On 2012/07/31 03:36:31, dmichael wrote: > FYI: Another option is an ArrayBuffer var. ArrayBuffers have some advantages: > 1) Not in dev (if you think this will ever come out of dev, that might matter) > 2) Can be posted to/from JavaScript as a JavaScript ArrayBuffer (also might not > matter to you.) > > ArrayBuffer also currently doesn't use shared memory, though, so if this is > large, PPB_Buffer might be better. We're not really sure what the maximum size of init_data will be, but currently it's capped at 2KB. That doesn't seem to be that large-- I'll try changing this over to an ArrayBuffer. Update: seems to work; will test this some more. https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppp_... ppapi/api/dev/ppp_content_decryptor_dev.idl:53: [in] PP_CompletionCallback callback); On 2012/07/31 03:36:31, dmichael wrote: > We've never used a PP_CompletionCallback in the plugin side before, and it's not > clear what the semantics are. Can you document and explain this? Maybe once I > know what you're trying to do, I can help you come up with an alternative (or > maybe I'll be convinced that PP_CompletionCallback is fine here). But it > definitely needs to be explained. In brief: 1) Media stack calls Decrypt on webkit::ppapi::PluginInstance 2) webkit::ppapi::PluginInstance passes things on the the CDM wrapper 3) CDMWrapper decrypts asynchronously using the CDM 4) Upon completion in the CDM DeliverBlock is called on the main thread 5) webkit::ppapi::PluginInstance invokes the callback to hand data off to the media stack for decode/render - I'm over simplifying things between steps three and four; the comms between the wrapper an the CDM aren't worked out yet. - The exact details of steps four and five are still being worked out as well. https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/cpp/dev/cont... File ppapi/cpp/dev/content_decryptor_dev.h (right): https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/cpp/dev/cont... ppapi/cpp/dev/content_decryptor_dev.h:23: virtual bool GenerateKeyRequest(PP_Var key_system, // String. On 2012/07/31 03:36:31, dmichael wrote: > Why not pp::Var instead of PP_Var? For the PPP methods I thought I had to use the C interface types here because of the setup in the CPP wrapper implementation: const PPP_ContentDecryptor_Dev ppp_content_decryptor = { &GenerateKeyRequest, &AddKey, &CancelKeyRequest, &Decrypt, &DecryptAndDecode }; The PPB stuff uses the C interfaces to avoid doing things in two different ways within the wrapper. Is there an example of another way to go about doing this? I'm doing the same things done by other PPP wrappers. https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/cpp/dev/cont... ppapi/cpp/dev/content_decryptor_dev.h:24: PP_Resource init_data) = 0; // PPB_Buffer. On 2012/07/31 03:36:31, dmichael wrote: > pp::Buffer_Dev? > (unless you go with ArrayBuffer, which would be pp::VarArrayBuffer) Using PP_Var now. https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/cpp/dev/cont... ppapi/cpp/dev/content_decryptor_dev.h:32: PP_CompletionCallback callback) = 0; On 2012/07/31 03:36:31, dmichael wrote: > Maybe pp::CompletionCallback, depending on what the intent is... but I'm hoping > we can come up with something else anyway for the "plugin" side. Same as first comment response in this file. https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/media/crypt... File webkit/media/crypto/content_decryptor.cc (right): https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/media/crypt... webkit/media/crypto/content_decryptor.cc:262: class MyModule : public pp::Module { On 2012/07/31 03:36:31, dmichael wrote: > I'm confused about what this file is... is this the "plugin"? (Admitting that > IIRC, you guys are doing something a little different than a regular plugin...) > Don't worry-- it _is_ confusing. :) The pepper plugin is a wrapper that's going to be used to load CDMs (Content Decryption Modules) and be responsible for things like: - handling the movement of data between chrome and the CDM - ensure calls into the CDM are asynchronous (to avoid having each author/vendor deal with threading etc) > If so, you shouldn't be using anything from ppapi/shared_impl (like > StringVar::StringToPPVar above). That's for Chrome's internal use only, and > using it here is likely to break stuff. Removed the shared_impl stuff (from the source code and the gyp file). Is my usage correct? https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/media/webki... File webkit/media/webkit_media.gypi (right): https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/media/webki... webkit/media/webkit_media.gypi:87: '<(DEPTH)/ppapi/../base/base.gyp:base', On 2012/07/31 03:36:31, dmichael wrote: > As noted elsewhere, if this is more like a "plugin", you should leave out > ppapi_shared (and probably base as well). > > I'm also not sure this belongs in webkit...? What's its relationship to WebKit? > You might check with somebody like darin or brettw to make sure this is the > right place. Noted-- we'll loop in more reviewers as needed. I think we've already got an OK on this one, but I could be mistaken. I *thought* stuff in base was safe to use. I'm not touching much, so it wouldn't be the end of the world if it had to be removed. I'll keep an eye on this and figure things out as we move forward... https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1330: init_data)); On 2012/07/31 03:36:31, dmichael wrote: > Why is init_data a string, if you're using it for binary data? I would think > something like vector<unsigned char> or scoped_array might be more appropriate? > > Then again, I'm not clear on what GenerateKeyRequest is for, so maybe there is a > goo reason ;-) Our design uses std::string for data whether its binary or not-- it could just as well be vector<uint8>. Using std::string is consistent with the media/crypto stuff-- it uses std::string for binary data. It seems weird, but the container can handle it. E.g. http://code.google.com/searchframe#OAMlx_jo-ck/src/media/crypto/aes_decryptor... https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2065: std::string message(reinterpret_cast<char*>(mapper.data()), mapper.size()); On 2012/07/31 03:36:31, dmichael wrote: > Why are you using a string here? You don't seem to use it, so it's not clear to > me what's going on. As above.
Just some comments as I try to make PPP calls from the PpapiDecryptor. http://codereview.chromium.org/10545036/diff/84021/ppapi/api/dev/ppp_content_... File ppapi/api/dev/ppp_content_decryptor_dev.idl (right): http://codereview.chromium.org/10545036/diff/84021/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryptor_dev.idl:50: PP_Bool Decrypt( We have both sync return (PP_Bool) and async callback. I am not sure how things are supposed to work, e.g. if this function returns false, should the callback be fired or not? We'll need to work out these details later. http://codereview.chromium.org/10545036/diff/84021/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/84021/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1377: bool PluginInstance::Decrypt(const std::string& encrypted_block, Can we use base::StringPiece here and below, just to avoid an extra copy of the whole encrypted buffer. http://codereview.chromium.org/10545036/diff/84021/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): http://codereview.chromium.org/10545036/diff/84021/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.h:243: typedef base::Callback<void(void*, int)> CDMStatusCB; nit: CdmStatusCB?
I've addressed most comments, and added follow up questions to the ones that haven't been taken care of-- PTAL and let me know if I missed anything. Re tests: I'll look into this some more tomorrow, I'm running out of time-- heading to Kirkland, and less than an hour before I leave for the airport. https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppb_... File ppapi/api/dev/ppb_content_decryptor_dev.idl (right): https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppb_... ppapi/api/dev/ppb_content_decryptor_dev.idl:22: * A key or license is needed to decrypt media data. On 2012/07/31 03:36:31, dmichael wrote: > Can you rephrase this to say what the method actually does? Done. https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppb_... ppapi/api/dev/ppb_content_decryptor_dev.idl:32: * method on the <code>PPP_ContentDecryptor_Dev</code> interface. On 2012/07/31 03:36:31, dmichael wrote: > If Chrome is calling AddKey first, why would the content decryptor thingy call > back to Chrome? Shouldn't the browser already know? AddKey is asynchronous, so all the browser will know is that the attempt is in progress when AddKey reports success. https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/api/dev/ppb_... ppapi/api/dev/ppb_content_decryptor_dev.idl:40: * A message or request has been generated by or for key_system. For example, On 2012/07/31 03:36:31, dmichael wrote: > Again, I'd rather an active voice statement of what the Browser does when you > call this. I tweaked the comment to make this a little bit more clear, but per the Encrypted Media Extensions specification KeyMessage is somewhat general purpose. Providing an exhaustive list of the potential uses would result in a _very_ lengthy comment. If the tweaked version isn't enough, just let me know and I'll come up with some more information. https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/proxy/ppb_in... File ppapi/proxy/ppb_instance_proxy.cc (right): https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/proxy/ppb_in... ppapi/proxy/ppb_instance_proxy.cc:537: } On 2012/07/31 03:36:31, dmichael wrote: > Are these intentionally blank? Assuming so, please add TODOs Done; added NOTIMPLEMENTED() to each, and TODO about confirming the need for these. The proxy methods are for out-of-process plugins, right? I don't think they're needed since the CDM wrapper will be loaded in the renderer. https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/thunk/ppb_co... File ppapi/thunk/ppb_content_decryptor_thunk.cc (right): https://chromiumcodereview.appspot.com/10545036/diff/91001/ppapi/thunk/ppb_co... ppapi/thunk/ppb_content_decryptor_thunk.cc:20: if (enter.succeeded()) { On 2012/07/31 03:36:31, dmichael wrote: > nit: no curly braces for 1-liners Done. https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:305: ScopedPPResource resource(PPB_Buffer_Impl::Create(instance, data.size())); On 2012/07/31 03:36:31, dmichael wrote: > I think that using this constructor, ScopedPPResource takes an additional ref, > so now you have 2. It looks like you're coding assuming you have one reference > below. > > Maybe you mean the PassRef constructor? Fixed this-- I want the returned PP_Resource to have ref count of 1. https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:315: return 0; On 2012/07/31 03:36:31, dmichael wrote: > I don't think this check is necessary Done. https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1350: return false; On 2012/07/31 03:36:31, dmichael wrote: > I don't think this check should be necessary Removed. https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1368: return false; On 2012/07/31 03:36:31, dmichael wrote: > unnecessary check Removed. https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2028: HostGlobals::Get()->GetVarTracker()->ReleaseVar(key_system_var); On 2012/07/31 03:36:31, dmichael wrote: > I may be wrong, but I think that our usual policy for Vars in this situation is > that the browser won't release them. The reference count is the same after the > method returns as when it's called. Our C++ wrappers, e.g., won't know about the > method call here, so you probably would get a double delete.. So the caller of this method would be responsible for cleaning up any vars created, and I could use member pp::Vars to help minimize data copying and reallocation? https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2035: } On 2012/07/31 03:36:31, dmichael wrote: > TODO? Added a some code here, and a TODO. https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2054: NOTREACHED(); On 2012/07/31 03:36:31, dmichael wrote: > You shouldn't do NOTREACHED or other crash-inducing things just because a plugin > provided bad args (maybe this is just leftover from debugging?) It was, removed. https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2061: NOTREACHED(); On 2012/07/31 03:36:31, dmichael wrote: > Also probably not appropriate to crash here (resource exhaustion?), though I'm > less sure Removed; my understanding is that we'll crash on out of memory anyway. Left over debug code, anyway. :) https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2075: HostGlobals::Get()->GetVarTracker()->ReleaseVar(default_url_var); On 2012/07/31 03:36:31, dmichael wrote: > Like above, you probably shouldn't release the vars here (here and elsewhere) Done. https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2079: HostGlobals::Get()->GetResourceTracker()->ReleaseResource(message_resource); On 2012/07/31 03:36:31, dmichael wrote: > As with Vars, I don't think you want to release the plugin's reference to the > Resource (here and elsewhere) Done. https://chromiumcodereview.appspot.com/10545036/diff/91001/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2107: // Not implemented in v0.1 of the EME/CDM work. On 2012/07/31 03:36:31, dmichael wrote: > TODO might still be good, so curious people know whom to ask about it Done. https://chromiumcodereview.appspot.com/10545036/diff/84021/ppapi/api/dev/ppp_... File ppapi/api/dev/ppp_content_decryptor_dev.idl (right): https://chromiumcodereview.appspot.com/10545036/diff/84021/ppapi/api/dev/ppp_... ppapi/api/dev/ppp_content_decryptor_dev.idl:50: PP_Bool Decrypt( On 2012/08/01 18:55:38, xhwang wrote: > We have both sync return (PP_Bool) and async callback. I am not sure how things > are supposed to work, e.g. if this function returns false, should the callback > be fired or not? We'll need to work out these details later. At present a return of false would mean that arguments were invalid in some way, and that no callback would happen. Agreed that we need to work out the details. https://chromiumcodereview.appspot.com/10545036/diff/84021/webkit/plugins/ppa... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): https://chromiumcodereview.appspot.com/10545036/diff/84021/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1377: bool PluginInstance::Decrypt(const std::string& encrypted_block, On 2012/08/01 18:55:38, xhwang wrote: > Can we use base::StringPiece here and below, just to avoid an extra copy of the > whole encrypted buffer. Done. https://chromiumcodereview.appspot.com/10545036/diff/84021/webkit/plugins/ppa... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): https://chromiumcodereview.appspot.com/10545036/diff/84021/webkit/plugins/ppa... webkit/plugins/ppapi/ppapi_plugin_instance.h:243: typedef base::Callback<void(void*, int)> CDMStatusCB; On 2012/08/01 18:55:38, xhwang wrote: > nit: CdmStatusCB? Yeah-- this is just a placeholder typedef to be replaced once we nail down the callback type. It should probably be renamed anyway (DecryptedDataCallback?).
First pass at the PPB proxy stuff added. Not sure if it's correct, but hey, at least it compiles! Starting PPP proxy stuff tomorrow (or maybe tonight, but my brain is melting-- probably a bad idea). PTAL/comments welcome/hoping I haven't wandered too far down an incorrect path. And sorry for the rebase noise in $PATCH_SET - 1.
First attempt at PPP proxy; not sure if it works yet.
Still looking, but here are some comments. http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppb_content_... File ppapi/api/dev/ppb_content_decryptor_dev.idl (right): http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:17: * <code>PPP_ContentDecryptor_Dev</code> interface. On 2012/08/01 04:19:22, tomf wrote: > On 2012/07/31 03:36:31, dmichael wrote: > > Can you describe more about what this API is for? Do plugins actually use > this, > > or is it really just a surface for a content decryptor to use to interact with > > the Browser? Can you point to a spec or anything? If it's not an actual API > for > > a plugin to use, it should be pointed out that it's special and why. > > EME and PPAPI CDM design links sent offline. I mean in the documentation :-) I don't want other people to come across this and think they can use it in their plugin. In fact... make sure you consider that case so that nothing weird happens. http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppp_content_... File ppapi/api/dev/ppp_content_decryptor_dev.idl (right): http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryptor_dev.idl:19: interface PPP_ContentDecryptor_Dev { On 2012/08/01 04:19:22, tomf wrote: > On 2012/07/31 03:36:31, dmichael wrote: > > We're trying to get away from defining new PPP interfaces. brettw or > > viettrungluu might have more information, but I think the intent would be that > > the plugin passes a struct of function pointers in to a PPB call. Or, if it > > makes sense, even better is just to use CompletionCallbacks as the only way > for > > the browser to call the plugin. > > > > I might be more helpful after I read your design document. > > So the PPB interface would have an OnCreate method (or something), and the > plugin would pass the function pointers in at that point? Yes. You can use the same struct and typename, it just won't hook in to the GetInterface string-based stuff. There are a few reasons for this: - It's easier to support backwards-compatibility, because we know that a particular PPP interface version is always associated one-to-one with a particular PPB version. - We can support calling PPP functions on a background thread (whereas the "old" way, it really only ever makes sense to invoke them on the main thread). The idea is that whatever thread you use to register your PPP struct is the one we call you on. That may not matter here, but it might. http://codereview.chromium.org/10545036/diff/91001/ppapi/api/dev/ppp_content_... ppapi/api/dev/ppp_content_decryptor_dev.idl:23: * systems. init_data is a data buffer containing initialization data from On 2012/08/01 04:19:22, tomf wrote: > On 2012/07/31 03:36:31, dmichael wrote: > > Maybe it's just that I'm ignorant of content decryption, but I don't know what > > sort of string to expect here. Could you provide an example? > > Sure, here are two examples from webkit/media/crypto/key_systems.cc: > > webkit-org.w3.clearkey > org.chromium.externalclearkey I meant in the documentation :-) http://codereview.chromium.org/10545036/diff/91001/ppapi/cpp/dev/content_decr... File ppapi/cpp/dev/content_decryptor_dev.h (right): http://codereview.chromium.org/10545036/diff/91001/ppapi/cpp/dev/content_decr... ppapi/cpp/dev/content_decryptor_dev.h:23: virtual bool GenerateKeyRequest(PP_Var key_system, // String. On 2012/08/01 04:19:22, tomf wrote: > On 2012/07/31 03:36:31, dmichael wrote: > > Why not pp::Var instead of PP_Var? > > For the PPP methods I thought I had to use the C interface types here because of > the setup in the CPP wrapper implementation: > > const PPP_ContentDecryptor_Dev ppp_content_decryptor = { > &GenerateKeyRequest, > &AddKey, > &CancelKeyRequest, > &Decrypt, > &DecryptAndDecode > }; > > The PPB stuff uses the C interfaces to avoid doing things in two different ways > within the wrapper. > > Is there an example of another way to go about doing this? I'm doing the same > things done by other PPP wrappers. We convert the params to appropriate C++ types whenever possible. See ppapi/cpp/mouse_lock.cc/.h for an example. ppapi/cpp/instance.cc/.h needs more sophisticated type conversions, so it's a good example, too. But Instance is really converted in module.h/.cc... and you should try to lay your code out more like mouse_lock. That said... since you're going to be the guinea pigs for the new way of doing PPP interfaces, you should be able to implement all the conversion stuff within content_decryptor.cc/.h. http://codereview.chromium.org/10545036/diff/91026/DEPS File DEPS (right): http://codereview.chromium.org/10545036/diff/91026/DEPS#newcode564 DEPS:564: "webkit/media/crypto", This is really heavy-handed. You should instead tweak a DEPS file closer to webkit/media/crypto (maybe webkit/media/DEPS, or consider adding a DEPS file to webkit/media/crypto). http://codereview.chromium.org/10545036/diff/91026/ppapi/api/dev/ppb_content_... File ppapi/api/dev/ppb_content_decryptor_dev.idl (right): http://codereview.chromium.org/10545036/diff/91026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:17: * <code>PPP_ContentDecryptor_Dev</code> interface. can you reference the spec? http://codereview.chromium.org/10545036/diff/91026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:26: * key already in use has expired. PPB functions are telling the browser to do something. I know this case is a little weird, in that it's kind of more like a callback, but I'd still like this to say what the plugin expects the browser to do in this case. Best I can tell, it expects the browser to inform the client application that a key is needed via a DOM event. Same comment for other PPB methods... what does the browser do when you make the call? http://codereview.chromium.org/10545036/diff/91026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:32: * <code>PP_VARTYPE_STRING</code> containing the session ID. Is this the point at which the session_id is determined (if the CDM supports session ids)? If so, that seems worth noting somewhere. http://codereview.chromium.org/10545036/diff/91026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:61: * a key request has been generated as the result of call to the nit: of +a+ call http://codereview.chromium.org/10545036/diff/91026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:117: * @param[in] request_id A value used in <code>ppapi::PluginInstance</code> to I would rather we avoid talking about Chrome-specific implementation details. How about something more like "A unique value the user agent can use to associate @a decrypted_block with a prior call to Decrypt()." or something. http://codereview.chromium.org/10545036/diff/86027/ppapi/proxy/ppapi_messages.h File ppapi/proxy/ppapi_messages.h (right): http://codereview.chromium.org/10545036/diff/86027/ppapi/proxy/ppapi_messages... ppapi/proxy/ppapi_messages.h:488: // ppapi::HostResource and PP_Resource reversed. Confirm/fix if necessary. Yes, these being Host->Plugin messages, you should use HostResource here. http://codereview.chromium.org/10545036/diff/86027/ppapi/proxy/ppapi_messages... ppapi/proxy/ppapi_messages.h:1069: ppapi::HostResource /* message */, And these PPB messages should use PP_Resource. It's probably worth mentioning that this is a PPB_Buffer resource /* message, PPB_Buffer */ http://codereview.chromium.org/10545036/diff/86027/ppapi/proxy/ppb_instance_p... File ppapi/proxy/ppb_instance_proxy.h (right): http://codereview.chromium.org/10545036/diff/86027/ppapi/proxy/ppb_instance_p... ppapi/proxy/ppb_instance_proxy.h:131: #if !defined(OS_NACL) please put your stuff inside of the #if !defined(OS_NACL) (unless you're certain you want to support NaCl CDMs) http://codereview.chromium.org/10545036/diff/86027/ppapi/proxy/ppb_instance_p... ppapi/proxy/ppb_instance_proxy.h:231: #if !defined(OS_NACL) please put your message handlers inside the: #if !defined(OS_NACL)
http://codereview.chromium.org/10545036/diff/91026/DEPS File DEPS (right): http://codereview.chromium.org/10545036/diff/91026/DEPS#newcode564 DEPS:564: "webkit/media/crypto", On 2012/08/08 03:38:10, dmichael wrote: > This is really heavy-handed. You should instead tweak a DEPS file closer to > webkit/media/crypto (maybe webkit/media/DEPS, or consider adding a DEPS file to > webkit/media/crypto). Done; created DEPS in webkit/media/crypto http://codereview.chromium.org/10545036/diff/91026/ppapi/api/dev/ppb_content_... File ppapi/api/dev/ppb_content_decryptor_dev.idl (right): http://codereview.chromium.org/10545036/diff/91026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:17: * <code>PPP_ContentDecryptor_Dev</code> interface. On 2012/08/08 03:38:10, dmichael wrote: > can you reference the spec? Done; referenced in the PPB IDL as well. http://codereview.chromium.org/10545036/diff/91026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:26: * key already in use has expired. On 2012/08/08 03:38:10, dmichael wrote: > PPB functions are telling the browser to do something. I know this case is a > little weird, in that it's kind of more like a callback, but I'd still like this > to say what the plugin expects the browser to do in this case. Best I can tell, > it expects the browser to inform the client application that a key is needed via > a DOM event. > > Same comment for other PPB methods... what does the browser do when you make > the call? Done. http://codereview.chromium.org/10545036/diff/91026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:32: * <code>PP_VARTYPE_STRING</code> containing the session ID. On 2012/08/08 03:38:10, dmichael wrote: > Is this the point at which the session_id is determined (if the CDM supports > session ids)? If so, that seems worth noting somewhere. GenerateKeyRequest on the PPP interface creates session IDs. http://codereview.chromium.org/10545036/diff/91026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:61: * a key request has been generated as the result of call to the On 2012/08/08 03:38:10, dmichael wrote: > nit: of +a+ call Done. http://codereview.chromium.org/10545036/diff/91026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:117: * @param[in] request_id A value used in <code>ppapi::PluginInstance</code> to On 2012/08/08 03:38:10, dmichael wrote: > I would rather we avoid talking about Chrome-specific implementation details. > How about something more like "A unique value the user agent can use to > associate @a decrypted_block with a prior call to Decrypt()." > or something. Done; basically stole your wording. :) http://codereview.chromium.org/10545036/diff/86027/ppapi/proxy/ppapi_messages.h File ppapi/proxy/ppapi_messages.h (right): http://codereview.chromium.org/10545036/diff/86027/ppapi/proxy/ppapi_messages... ppapi/proxy/ppapi_messages.h:488: // ppapi::HostResource and PP_Resource reversed. Confirm/fix if necessary. On 2012/08/08 03:38:10, dmichael wrote: > Yes, these being Host->Plugin messages, you should use HostResource here. Done. http://codereview.chromium.org/10545036/diff/86027/ppapi/proxy/ppapi_messages... ppapi/proxy/ppapi_messages.h:1069: ppapi::HostResource /* message */, On 2012/08/08 03:38:10, dmichael wrote: > And these PPB messages should use PP_Resource. It's probably worth mentioning > that this is a PPB_Buffer resource > /* message, PPB_Buffer */ Done, also added type comments to the vars. http://codereview.chromium.org/10545036/diff/86027/ppapi/proxy/ppb_instance_p... File ppapi/proxy/ppb_instance_proxy.h (right): http://codereview.chromium.org/10545036/diff/86027/ppapi/proxy/ppb_instance_p... ppapi/proxy/ppb_instance_proxy.h:131: #if !defined(OS_NACL) On 2012/08/08 03:38:10, dmichael wrote: > please put your stuff inside of the #if !defined(OS_NACL) > > (unless you're certain you want to support NaCl CDMs) Done. http://codereview.chromium.org/10545036/diff/86027/ppapi/proxy/ppb_instance_p... ppapi/proxy/ppb_instance_proxy.h:231: #if !defined(OS_NACL) On 2012/08/08 03:38:10, dmichael wrote: > please put your message handlers inside the: > #if !defined(OS_NACL) Done.
http://codereview.chromium.org/10545036/diff/88026/ppapi/api/dev/ppb_content_... File ppapi/api/dev/ppb_content_decryptor_dev.idl (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:7: * This file defines the <code>PPB_ContentDecryptor_Dev</code> I talked to Brett about it, and we agreed that this stuff should go straight in to private (instead of dev). (Sorry.) http://codereview.chromium.org/10545036/diff/88026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:19: * http://goo.gl/rbdnR Also please note that this is a special interface, only to be used for Content Decryption Modules, not normal plugins. http://codereview.chromium.org/10545036/diff/88026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:30: * must provide a key to the decryptor by calling <code>AddKey()</code> on How does it do that? The way I was reading the spec, the app might need to talk to a server to get the key. So I don't think we can promise that the browser will call AddKey if it's the app's responsibility. http://codereview.chromium.org/10545036/diff/88026/ppapi/cpp/dev/content_decr... File ppapi/cpp/dev/content_decryptor_dev.h (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/cpp/dev/content_decr... ppapi/cpp/dev/content_decryptor_dev.h:25: virtual bool GenerateKeyRequest(PP_Var key_system, // String. As noted before, you should use a C++ version of a type wherever possible. In this particular case, it's sensible to just use const std::string&. PP_Var can go to pp::VarArrayBuffer when you know it's supposed to be an ArrayBuffer. etc. http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppb_instance_p... File ppapi/proxy/ppb_instance_proxy.cc (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppb_instance_p... ppapi/proxy/ppb_instance_proxy.cc:575: PP_Resource message, In this direction, I think you need to look the PP_Resource params up in the resource tracker and send the host_resource. There should be lots of examples. The idea is that the local, plugin-side resource ID is distinct from the ID the host uses for this resource, so when you go to the host side, you need to translate to the ID the host understands. (same for other resources below) http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_proxy.cc (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_proxy.cc:29: // TODO(tomfinegan): NOTREACHED safe here/throughout? Should be fine. If the dispatcher's not there, we're hosed. It could mean that the PP_Instance is invalid, but we should just make sure the renderer only calls GenerateKeyRequest with a valid PP_Instance. http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_proxy.cc:127: dispatcher->local_get_interface()(PPP_CONTENTDECRYPTOR_DEV_INTERFACE)); As mentioned elsewhere, you should instead be getting ppp_decryptor_impl_ via a new PPB function where it gets passed. http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_proxy.cc:171: // 1) Is CallWhileUnlocked necessary here/throughout? Yes, you must do CallWhileUnlocked every time you call in to the plugin. The whole plugin side of the proxy is basically a critical section. This is to support PPB calls coming from background threads. This is not actually turned on yet by default, but it will be. (you can turn it on locally by setting an enable_pepper_threading=1 gyp define, if you want to use the feature). http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_proxy.cc:172: // 2) What about return values? Log/DCHECK? I'm not sure what you mean? You definitely don't want to DCHECK (and probably don't want to LOG) based on plugin returns. I know your case is a little special, but in general, we can't trust the plugin to do anything at all reasonable. http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_proxy.h (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_proxy.h:30: // TODO(tomfinegan): Not sure if I want to use OnPluginMsg or OnMsg. OnMsg is fine in this case, since they're *all* host->plugin messages. If you end up moving these in to a place that also has host->plugin messages, you should use OnPluginMsg to make them distinct. (That might happen when you move to passing the PPP struct to some InitializeCDM-like method on PPB_ContentDecryptor, as we've discussed). http://codereview.chromium.org/10545036/diff/88026/ppapi/thunk/interfaces_ppb... File ppapi/thunk/interfaces_ppb_public_dev.h (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/thunk/interfaces_ppb... ppapi/thunk/interfaces_ppb_public_dev.h:78: UNPROXIED_IFACE(PPB_ContentDecryptor_Dev, I think you want to use PPB_Instance here, since that's where this gets proxied. http://codereview.chromium.org/10545036/diff/88026/ppapi/thunk/ppb_content_de... File ppapi/thunk/ppb_content_decryptor_thunk.cc (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/thunk/ppb_content_de... ppapi/thunk/ppb_content_decryptor_thunk.cc:40: default_url); style nit: when you have to go multi-line, you should add curly braces (even for a single statement) http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... File webkit/media/crypto/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:5: #include <cassert> I don't think you're using this? http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:10: #include "base/memory/scoped_ptr.h" you aren't using this. I'd suggest trying to not use base if you can help it. You might want a DEPS rule for this to make sure you only rely on ppapi/c and ppapi/cpp. http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:96: uint32 next_session_id_; I would suggest using ppapi/c/pp_stdint.h and uint32_t, rather than including base/basictypes.h http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:105: // session_id:key? Or am I completely confused? :) Or map to some delegate class that is set up to handle 1 session_id, so that this top-level class can just be responsible for dispatching by session_id? http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:132: session_id_ = base::UintToString(next_session_id_++); I think it would be better to use a stringstream instead, than to rely on base. http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:137: CallOnMain(callback_factory_.NewCallback(&CDMWrapper::KeyMessage, I don't know whether you want to rely on this... but if you're always running out-of-process (and you should :-) ), this stuff will all be asynchronous (since you use asynchronous messages in the proxy). So you don't really need to post a task. http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:148: if (!key.is_array_buffer()) you should always check is_array_buffer _before_ using the Var->VarArrayBuffer constructor (the constructor asserts that it's an array buffer). But... this check should probably be happening in the C++ wrapper, in any case. http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:187: PP_Resource CDMWrapper::StringToBufferResource(const std::string& str) { I don't think a string is the best way to represent your binary data. I know it's convenient right now since you're using c-strings to make easy dummy data, but it's probably worth changing now to something more appropriate so you don't get stuck down a hole. http://codereview.chromium.org/10545036/diff/88026/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/88026/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1353: bool PluginInstance::Decrypt(const base::StringPiece& encrypted_block, Why StringPiece? Isn't this going to come in to you as a WebKit ArrayBuffer? It seems like you're guaranteeing that you will have to copy, and StringPiece doesn't feel like the right type for binary stuff anyway.
http://codereview.chromium.org/10545036/diff/88026/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/88026/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1353: bool PluginInstance::Decrypt(const base::StringPiece& encrypted_block, On 2012/08/08 22:24:02, dmichael wrote: > Why StringPiece? Isn't this going to come in to you as a WebKit ArrayBuffer? It > seems like you're guaranteeing that you will have to copy, and StringPiece > doesn't feel like the right type for binary stuff anyway. This was my idea :/ I proposed that to Tom to avoid one extra data copy. To clarify, this Decrypt() is not directly called by the application or WebKit. It is called by the media stack (e.g. http://goo.gl/whIO8). Actually using StringPiece was my idea a week ago. After that I realized that I also need to pass in decryption configurations along with the encrypted data, so I just pass in media::DecoderBuffer now: http://goo.gl/e3Jdb Does this sound good to you? When you have time, please also take a look at: http://goo.gl/hLwlc ?
http://codereview.chromium.org/10545036/diff/88026/webkit/plugins/ppapi/ppapi... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10545036/diff/88026/webkit/plugins/ppapi/ppapi... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1353: bool PluginInstance::Decrypt(const base::StringPiece& encrypted_block, On 2012/08/09 00:49:26, xhwang wrote: > On 2012/08/08 22:24:02, dmichael wrote: > > Why StringPiece? Isn't this going to come in to you as a WebKit ArrayBuffer? > It > > seems like you're guaranteeing that you will have to copy, and StringPiece > > doesn't feel like the right type for binary stuff anyway. > > This was my idea :/ I proposed that to Tom to avoid one extra data copy. > > To clarify, this Decrypt() is not directly called by the application or WebKit. > It is called by the media stack (e.g. http://goo.gl/whIO8). > > Actually using StringPiece was my idea a week ago. After that I realized that I > also need to pass in decryption configurations along with the encrypted data, so > I just pass in media::DecoderBuffer now: http://goo.gl/e3Jdb > > Does this sound good to you? When you have time, please also take a look at: > http://goo.gl/hLwlc ? Yes, I think that makes sense. I also was misunderstanding StringPiece. I'm used to just using a raw pointer or something for this kind of thing, but StringPiece would've been fine for referencing a buffer with a length. I think long term, we'll probably want the buffer part to point to the ArrayBuffer from JavaScript, which in the future might be mappable to shared memory so we can save another copy. But that can probably be wrapped behind DecoderBuffer at this level anyway.
Finally getting around to do a full reading of the EME spec, then breaking this CL into two pieces (interfaces and implementation/wrapper). Comments welcome from all receiving this... PTAL/thanks! http://codereview.chromium.org/10545036/diff/88026/ppapi/api/dev/ppb_content_... File ppapi/api/dev/ppb_content_decryptor_dev.idl (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:7: * This file defines the <code>PPB_ContentDecryptor_Dev</code> On 2012/08/08 22:24:02, dmichael wrote: > I talked to Brett about it, and we agreed that this stuff should go straight in > to private (instead of dev). (Sorry.) Done. http://codereview.chromium.org/10545036/diff/88026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:19: * http://goo.gl/rbdnR On 2012/08/08 22:24:02, dmichael wrote: > Also please note that this is a special interface, only to be used for Content > Decryption Modules, not normal plugins. Done. http://codereview.chromium.org/10545036/diff/88026/ppapi/api/dev/ppb_content_... ppapi/api/dev/ppb_content_decryptor_dev.idl:30: * must provide a key to the decryptor by calling <code>AddKey()</code> on On 2012/08/08 22:24:02, dmichael wrote: > How does it do that? The way I was reading the spec, the app might need to talk > to a server to get the key. So I don't think we can promise that the browser > will call AddKey if it's the app's responsibility. Changed browser to application. http://codereview.chromium.org/10545036/diff/88026/ppapi/cpp/dev/content_decr... File ppapi/cpp/dev/content_decryptor_dev.h (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/cpp/dev/content_decr... ppapi/cpp/dev/content_decryptor_dev.h:25: virtual bool GenerateKeyRequest(PP_Var key_system, // String. On 2012/08/08 22:24:02, dmichael wrote: > As noted before, you should use a C++ version of a type wherever possible. > > In this particular case, it's sensible to just use const std::string&. PP_Var > can go to pp::VarArrayBuffer when you know it's supposed to be an ArrayBuffer. > etc. Done. http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppb_instance_p... File ppapi/proxy/ppb_instance_proxy.cc (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppb_instance_p... ppapi/proxy/ppb_instance_proxy.cc:575: PP_Resource message, On 2012/08/08 22:24:02, dmichael wrote: > In this direction, I think you need to look the PP_Resource params up in the > resource tracker and send the host_resource. There should be lots of examples. > The idea is that the local, plugin-side resource ID is distinct from the ID the > host uses for this resource, so when you go to the host side, you need to > translate to the ID the host understands. > > (same for other resources below) Done. http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_proxy.cc (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_proxy.cc:171: // 1) Is CallWhileUnlocked necessary here/throughout? On 2012/08/08 22:24:02, dmichael wrote: > Yes, you must do CallWhileUnlocked every time you call in to the plugin. The > whole plugin side of the proxy is basically a critical section. This is to > support PPB calls coming from background threads. This is not actually turned on > yet by default, but it will be. (you can turn it on locally by setting an > enable_pepper_threading=1 gyp define, if you want to use the feature). Done. http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_proxy.cc:172: // 2) What about return values? Log/DCHECK? On 2012/08/08 22:24:02, dmichael wrote: > I'm not sure what you mean? You definitely don't want to DCHECK (and probably > don't want to LOG) based on plugin returns. I know your case is a little > special, but in general, we can't trust the plugin to do anything at all > reasonable. You grokked what I was getting at; CallWhileUnlocked wraps the calls now. http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppp_content_de... File ppapi/proxy/ppp_content_decryptor_proxy.h (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/proxy/ppp_content_de... ppapi/proxy/ppp_content_decryptor_proxy.h:30: // TODO(tomfinegan): Not sure if I want to use OnPluginMsg or OnMsg. On 2012/08/08 22:24:02, dmichael wrote: > OnMsg is fine in this case, since they're *all* host->plugin messages. If you > end up moving these in to a place that also has host->plugin messages, you > should use OnPluginMsg to make them distinct. (That might happen when you move > to passing the PPP struct to some InitializeCDM-like method on > PPB_ContentDecryptor, as we've discussed). Done. http://codereview.chromium.org/10545036/diff/88026/ppapi/thunk/interfaces_ppb... File ppapi/thunk/interfaces_ppb_public_dev.h (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/thunk/interfaces_ppb... ppapi/thunk/interfaces_ppb_public_dev.h:78: UNPROXIED_IFACE(PPB_ContentDecryptor_Dev, On 2012/08/08 22:24:02, dmichael wrote: > I think you want to use PPB_Instance here, since that's where this gets proxied. Changed to PPB_Instance, moved into the private version of this file, and using PROXIED_IFACE instead. http://codereview.chromium.org/10545036/diff/88026/ppapi/thunk/ppb_content_de... File ppapi/thunk/ppb_content_decryptor_thunk.cc (right): http://codereview.chromium.org/10545036/diff/88026/ppapi/thunk/ppb_content_de... ppapi/thunk/ppb_content_decryptor_thunk.cc:40: default_url); On 2012/08/08 22:24:02, dmichael wrote: > style nit: when you have to go multi-line, you should add curly braces (even for > a single statement) Done. http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... File webkit/media/crypto/content_decryptor.cc (right): http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:5: #include <cassert> On 2012/08/08 22:24:02, dmichael wrote: > I don't think you're using this? Removed. http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:10: #include "base/memory/scoped_ptr.h" On 2012/08/08 22:24:02, dmichael wrote: > you aren't using this. I'd suggest trying to not use base if you can help it. > You might want a DEPS rule for this to make sure you only rely on ppapi/c and > ppapi/cpp. Done. http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:96: uint32 next_session_id_; On 2012/08/08 22:24:02, dmichael wrote: > I would suggest using ppapi/c/pp_stdint.h and uint32_t, rather than including > base/basictypes.h Done. http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:132: session_id_ = base::UintToString(next_session_id_++); On 2012/08/08 22:24:02, dmichael wrote: > I think it would be better to use a stringstream instead, than to rely on base. Done. http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:148: if (!key.is_array_buffer()) On 2012/08/08 22:24:02, dmichael wrote: > you should always check is_array_buffer _before_ using the Var->VarArrayBuffer > constructor (the constructor asserts that it's an array buffer). > > But... this check should probably be happening in the C++ wrapper, in any case. Done/checked in c++ wrapper. http://codereview.chromium.org/10545036/diff/88026/webkit/media/crypto/conten... webkit/media/crypto/content_decryptor.cc:187: PP_Resource CDMWrapper::StringToBufferResource(const std::string& str) { On 2012/08/08 22:24:02, dmichael wrote: > I don't think a string is the best way to represent your binary data. I know > it's convenient right now since you're using c-strings to make easy dummy data, > but it's probably worth changing now to something more appropriate so you don't > get stuck down a hole. Discussed offline.
Removed all but the IDL files and C interface code. PTAL-- it would be awesome if this piece could land today!
http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... File ppapi/api/private/ppb_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:19: * http://goo.gl/rbdnR I am not sure if we should use short URL in this file. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:28: * response to a call to a decrypt method on the s/decrypt/<code>Decrypt()<code> http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:31: * <code>AddKey()</code> on the <code>PPP_ContentDecryptor_Private<code> The application cannot call the PPP interface directly, it calls addkey method on the media element (http://goo.gl/2RWNI). It's the browser that calls the PPP interface's AddKey(), usually as a result of the addkey call from the application, but in theory nothing prevents the browser from grabbing a key magically somewhere and calling AddKey() without involving the application. How about something like "In response, the browser should provide a key ...." http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:53: * decryption of content can proceed. Remove "and decryption of content can proceed"? This message just tells the browser that the key has been added. But there's no guarantee that this key applies to the content being blocked/decrypted. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:59: * and the decryptor plugin is ready to begin decryption of data. ditto about the last sentence. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:96: * @param[in] session_id A <code>PP_Var</code> of type s/session_id/default_url http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:132: * @param[in] resource A <code>PP_Resource</code> corresponding to a s/resource/decrypted_block http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:145: * Called after the <code>DecryptAndDecode</code> method on the s/DecryptAndDecode/DecryptAndDecode() http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:149: * @param[in] resource A <code>PP_Resource</code> corresponding to a s/resource/decrypted_frame http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:164: * deliver decrypted_samples to the browser. Should we mention this is for audio and DeliverFrame is for video? Otherwise it's not obvious what's the difference b/w DeliverFrame and DeliverSamples. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:166: * @param[in] resource A <code>PP_Resource</code> corresponding to a s/resource/decrypted_samples http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:17: * the media stack. This interface provides the plugin side support for v0.1 of "media stack" seems implementation detail. Maybe just needed by "the browser"? http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:18: * the proposed Encrypted Media Extensions: http://goo.gl/rbdnR ditto: not sure about if short URL is appropriate. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:32: * provided to the user agent via <code>KeyMessage()</code> on the "user agent" is the term used in the spec. Should we just use "the browser" here? http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:93: * Decrypts the block, decodes it, and returns the unencrypted raw (decoded) s/raw/uncompressed http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:95: * interface. Should we mention to return the data via DeliverFrame() if it's video and via DeliverSamples() if it's audio?
Tom: added a few comments. Some of them (w/ question marks) I am not sure about either. You may want to wait for ddorwin@'s comment to start revising on those. On 2012/08/10 16:41:30, xhwang wrote: > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... > File ppapi/api/private/ppb_content_decryptor_private.idl (right): > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... > ppapi/api/private/ppb_content_decryptor_private.idl:19: * http://goo.gl/rbdnR > I am not sure if we should use short URL in this file. > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... > ppapi/api/private/ppb_content_decryptor_private.idl:28: * response to a call to > a decrypt method on the > s/decrypt/<code>Decrypt()<code> > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... > ppapi/api/private/ppb_content_decryptor_private.idl:31: * <code>AddKey()</code> > on the <code>PPP_ContentDecryptor_Private<code> > The application cannot call the PPP interface directly, it calls addkey method > on the media element (http://goo.gl/2RWNI). It's the browser that calls the PPP > interface's AddKey(), usually as a result of the addkey call from the > application, but in theory nothing prevents the browser from grabbing a key > magically somewhere and calling AddKey() without involving the application. > > How about something like "In response, the browser should provide a key ...." > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... > ppapi/api/private/ppb_content_decryptor_private.idl:53: * decryption of content > can proceed. > Remove "and decryption of content can proceed"? This message just tells the > browser that the key has been added. But there's no guarantee that this key > applies to the content being blocked/decrypted. > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... > ppapi/api/private/ppb_content_decryptor_private.idl:59: * and the decryptor > plugin is ready to begin decryption of data. > ditto about the last sentence. > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... > ppapi/api/private/ppb_content_decryptor_private.idl:96: * @param[in] session_id > A <code>PP_Var</code> of type > s/session_id/default_url > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... > ppapi/api/private/ppb_content_decryptor_private.idl:132: * @param[in] resource A > <code>PP_Resource</code> corresponding to a > s/resource/decrypted_block > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... > ppapi/api/private/ppb_content_decryptor_private.idl:145: * Called after the > <code>DecryptAndDecode</code> method on the > s/DecryptAndDecode/DecryptAndDecode() > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... > ppapi/api/private/ppb_content_decryptor_private.idl:149: * @param[in] resource A > <code>PP_Resource</code> corresponding to a > s/resource/decrypted_frame > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... > ppapi/api/private/ppb_content_decryptor_private.idl:164: * deliver > decrypted_samples to the browser. > Should we mention this is for audio and DeliverFrame is for video? Otherwise > it's not obvious what's the difference b/w DeliverFrame and DeliverSamples. > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... > ppapi/api/private/ppb_content_decryptor_private.idl:166: * @param[in] resource A > <code>PP_Resource</code> corresponding to a > s/resource/decrypted_samples > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... > File ppapi/api/private/ppp_content_decryptor_private.idl (right): > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... > ppapi/api/private/ppp_content_decryptor_private.idl:17: * the media stack. This > interface provides the plugin side support for v0.1 of > "media stack" seems implementation detail. Maybe just needed by "the browser"? > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... > ppapi/api/private/ppp_content_decryptor_private.idl:18: * the proposed Encrypted > Media Extensions: http://goo.gl/rbdnR > ditto: not sure about if short URL is appropriate. > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... > ppapi/api/private/ppp_content_decryptor_private.idl:32: * provided to the user > agent via <code>KeyMessage()</code> on the > "user agent" is the term used in the spec. Should we just use "the browser" > here? > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... > ppapi/api/private/ppp_content_decryptor_private.idl:93: * Decrypts the block, > decodes it, and returns the unencrypted raw (decoded) > s/raw/uncompressed > > http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... > ppapi/api/private/ppp_content_decryptor_private.idl:95: * interface. > Should we mention to return the data via DeliverFrame() if it's video and via > DeliverSamples() if it's audio?
http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... File ppapi/api/private/ppb_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:19: * http://goo.gl/rbdnR On 2012/08/10 16:41:30, xhwang wrote: > I am not sure if we should use short URL in this file. Originally I was using the full URL, but generators.py mangles the comment because it exceeds 80 columns. You end up with a C interface include that looks like this /** * Normal comment text blah blah blah * * http://dvcs.w3.org/hg/html-media/raw-file/tip/encrypted-media/encrypted-media... */ Note that contrary to the display in this page the '*' and the link are on the same line. Hand editing it would work, but to do that I have to bypass the presubmit. No problems doing that if the full link is preferred. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:28: * response to a call to a decrypt method on the On 2012/08/10 16:41:30, xhwang wrote: > s/decrypt/<code>Decrypt()<code> Done. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:31: * <code>AddKey()</code> on the <code>PPP_ContentDecryptor_Private<code> On 2012/08/10 16:41:30, xhwang wrote: > The application cannot call the PPP interface directly, it calls addkey method > on the media element (http://goo.gl/2RWNI). It's the browser that calls the PPP > interface's AddKey(), usually as a result of the addkey call from the > application, but in theory nothing prevents the browser from grabbing a key > magically somewhere and calling AddKey() without involving the application. > > How about something like "In response, the browser should provide a key ...." Done. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:53: * decryption of content can proceed. On 2012/08/10 16:41:30, xhwang wrote: > Remove "and decryption of content can proceed"? This message just tells the > browser that the key has been added. But there's no guarantee that this key > applies to the content being blocked/decrypted. Done. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:59: * and the decryptor plugin is ready to begin decryption of data. On 2012/08/10 16:41:30, xhwang wrote: > ditto about the last sentence. Done. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:96: * @param[in] session_id A <code>PP_Var</code> of type On 2012/08/10 16:41:30, xhwang wrote: > s/session_id/default_url Done. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:132: * @param[in] resource A <code>PP_Resource</code> corresponding to a On 2012/08/10 16:41:30, xhwang wrote: > s/resource/decrypted_block Done. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:145: * Called after the <code>DecryptAndDecode</code> method on the On 2012/08/10 16:41:30, xhwang wrote: > s/DecryptAndDecode/DecryptAndDecode() Done. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:149: * @param[in] resource A <code>PP_Resource</code> corresponding to a On 2012/08/10 16:41:30, xhwang wrote: > s/resource/decrypted_frame Done. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:164: * deliver decrypted_samples to the browser. On 2012/08/10 16:41:30, xhwang wrote: > Should we mention this is for audio and DeliverFrame is for video? Otherwise > it's not obvious what's the difference b/w DeliverFrame and DeliverSamples. Done. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:166: * @param[in] resource A <code>PP_Resource</code> corresponding to a On 2012/08/10 16:41:30, xhwang wrote: > s/resource/decrypted_samples Done. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:17: * the media stack. This interface provides the plugin side support for v0.1 of On 2012/08/10 16:41:30, xhwang wrote: > "media stack" seems implementation detail. Maybe just needed by "the browser"? s/media stack/user agent/. I can change it to browser if that's preferred. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:18: * the proposed Encrypted Media Extensions: http://goo.gl/rbdnR On 2012/08/10 16:41:30, xhwang wrote: > ditto: not sure about if short URL is appropriate. See comment in PPB IDL. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:32: * provided to the user agent via <code>KeyMessage()</code> on the On 2012/08/10 16:41:30, xhwang wrote: > "user agent" is the term used in the spec. Should we just use "the browser" > here? Not sure, comments welcome! :) http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:93: * Decrypts the block, decodes it, and returns the unencrypted raw (decoded) On 2012/08/10 16:41:30, xhwang wrote: > s/raw/uncompressed Done. http://codereview.chromium.org/10545036/diff/83034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:95: * interface. On 2012/08/10 16:41:30, xhwang wrote: > Should we mention to return the data via DeliverFrame() if it's video and via > DeliverSamples() if it's audio? Done.
Reviewed for content but not Pepper style or consistency. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... File ppapi/api/private/ppb_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:18: * browser side support for v0.1 of the proposed Encrypted Media Extensions: ... support for the CDM for... http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:29: * <code>PPP_ContentDecryptor_Private<code> interface, In response, the s/,/./ http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:32: * interface. The key could be provided by the browser, or from the I think this last sentence is unnecessary. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:42: * <code>PP_VARTYPE_ARRAYBUFFER</code> containing content specific s/content/container/ ? http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:55: * Note: the above describes the most simple case. Depending on the key s/the/The/ http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:96: * <code>PP_VARTYPE_STRING</code> containing the default URL for key system. s/key system/the message/ http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:115: * @param[in] media_error A media stack error code. error_code A MediaKeyError. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:117: * @param[in] system_error A system error code. system_code http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:141: [in] uint64_t request_id); We usually prefer signed ints. Is there a reason it must be unsigned (and 64 bits)? Are there other examples of this in Pepper? http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:154: void DeliverFrame( Thoughts on whether to include "Video" in the name since it is only for video? Same for the next method. Actually, if the answer to my next question is no, do we need separate functions to deliver decoded blocks? The request_id could be used to determine audio vs. video. Maybe it's simpler to be explicit since the plugin knows, though. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:156: [in] PP_Resource decrypted_frame, Are we going to meed more information than this? This is fine for now, but I wonder if we should just leave these out until we have a better idea. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:17: * the user agent. This interface provides the plugin side support for v0.1 of ... support for the CDM for... http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:17: * the user agent. This interface provides the plugin side support for v0.1 of Throughout this file: s/user agent/browser/? "user agent" is used in the spec, but for Pepper, it's the "browser". Unless this term is used in other IDL files. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:25: * Generates a key request. key_system specifies the key or licensing system key_system specifies the key or licensing system to use to generate the request. Or just ...to use. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:27: * systems. init_data is a data buffer containing initialization data from ...containing data to use in generating the request. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:39: * <code>PP_VARTYPE_ARRAYBUFFER</code> containing content specific s/content/container/ http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:48: * Provides a key or license to the decryptor for decrypting media data in the delete everything after "data." http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:55: * <code>PP_VARTYPE_ARRAYBUFFER</code> containing the decryption key for the key, license, or other message http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:94: * (decoded) media to the user agent via the remove "to the user agent " or replace user agent with browser and update the previous method too. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:97: * Note that decrypted and decoded video frames are sent to Remove "Note that ".
http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... File ppapi/api/private/ppb_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:18: * browser side support for v0.1 of the proposed Encrypted Media Extensions: On 2012/08/11 22:08:57, ddorwin wrote: > ... support for the CDM for... Done. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:29: * <code>PPP_ContentDecryptor_Private<code> interface, In response, the On 2012/08/11 22:08:57, ddorwin wrote: > s/,/./ Done. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:32: * interface. The key could be provided by the browser, or from the On 2012/08/11 22:08:57, ddorwin wrote: > I think this last sentence is unnecessary. Removed. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:42: * <code>PP_VARTYPE_ARRAYBUFFER</code> containing content specific On 2012/08/11 22:08:57, ddorwin wrote: > s/content/container/ ? Done. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:55: * Note: the above describes the most simple case. Depending on the key On 2012/08/11 22:08:57, ddorwin wrote: > s/the/The/ Done. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:96: * <code>PP_VARTYPE_STRING</code> containing the default URL for key system. On 2012/08/11 22:08:57, ddorwin wrote: > s/key system/the message/ Done. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:115: * @param[in] media_error A media stack error code. On 2012/08/11 22:08:57, ddorwin wrote: > error_code A MediaKeyError. Done. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:117: * @param[in] system_error A system error code. On 2012/08/11 22:08:57, ddorwin wrote: > system_code Done. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:141: [in] uint64_t request_id); On 2012/08/11 22:08:57, ddorwin wrote: > We usually prefer signed ints. Is there a reason it must be unsigned (and 64 > bits)? Are there other examples of this in Pepper? Taken directly from a suggestion in an offline disccusion-- changed to signed 32. Even at 60 frames/second we still have 414 days before rollover. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:154: void DeliverFrame( On 2012/08/11 22:08:57, ddorwin wrote: > Thoughts on whether to include "Video" in the name since it is only for video? > Same for the next method. > > Actually, if the answer to my next question is no, do we need separate functions > to deliver decoded blocks? The request_id could be used to determine audio vs. > video. Maybe it's simpler to be explicit since the plugin knows, though. I would be fine w/DeliverVideoFrame or just DeliverVideo (in case we ever want to send more than one frame at a time). http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:156: [in] PP_Resource decrypted_frame, On 2012/08/11 22:08:57, ddorwin wrote: > Are we going to meed more information than this? > This is fine for now, but I wonder if we should just leave these out until we > have a better idea. Probably not when returning the uncompressed data-- I think the request_id would be enough to get the data to where it needs to go from PI. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:17: * the user agent. This interface provides the plugin side support for v0.1 of On 2012/08/11 22:08:57, ddorwin wrote: > Throughout this file: > s/user agent/browser/? > "user agent" is used in the spec, but for Pepper, it's the "browser". Unless > this term is used in other IDL files. Done, because 'idl$ "user agent"' comes up almost empty on codesearch, and 'idl$ browser' yields several pages of results. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:17: * the user agent. This interface provides the plugin side support for v0.1 of On 2012/08/11 22:08:57, ddorwin wrote: > ... support for the CDM for... Done. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:25: * Generates a key request. key_system specifies the key or licensing system On 2012/08/11 22:08:57, ddorwin wrote: > key_system specifies the key or licensing system to use to generate the request. > > Or just ...to use. Done. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:27: * systems. init_data is a data buffer containing initialization data from On 2012/08/11 22:08:57, ddorwin wrote: > ...containing data to use in generating the request. Done. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:39: * <code>PP_VARTYPE_ARRAYBUFFER</code> containing content specific On 2012/08/11 22:08:57, ddorwin wrote: > s/content/container/ Done. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:48: * Provides a key or license to the decryptor for decrypting media data in the On 2012/08/11 22:08:57, ddorwin wrote: > delete everything after "data." Done. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:55: * <code>PP_VARTYPE_ARRAYBUFFER</code> containing the decryption key for the On 2012/08/11 22:08:57, ddorwin wrote: > key, license, or other message Done. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:94: * (decoded) media to the user agent via the On 2012/08/11 22:08:57, ddorwin wrote: > remove "to the user agent " or replace user agent with browser and update the > previous method too. All instances of user agent have been replaced w/browser. http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:97: * Note that decrypted and decoded video frames are sent to On 2012/08/11 22:08:57, ddorwin wrote: > Remove "Note that ". Done.
lgtm with one typo to fix http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... File ppapi/api/private/ppb_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/86029/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:154: void DeliverFrame( On 2012/08/13 16:27:17, tomf wrote: > On 2012/08/11 22:08:57, ddorwin wrote: > > Thoughts on whether to include "Video" in the name since it is only for video? > > Same for the next method. > > > > Actually, if the answer to my next question is no, do we need separate > functions > > to deliver decoded blocks? The request_id could be used to determine audio vs. > > video. Maybe it's simpler to be explicit since the plugin knows, though. > > I would be fine w/DeliverVideoFrame or just DeliverVideo (in case we ever want > to send more than one frame at a time). I think the signature would be different for more than one frame. http://codereview.chromium.org/10545036/diff/81027/ppapi/api/private/ppp_cont... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/81027/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:26: * to use. init_data is a data buffer containing to use in generating the containing _data_...
http://codereview.chromium.org/10545036/diff/81027/ppapi/api/private/ppp_cont... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/81027/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:26: * to use. init_data is a data buffer containing to use in generating the On 2012/08/13 17:43:50, ddorwin wrote: > containing _data_... Done.
mostly comments about comments... otherwise lgtm. Please address making the PPP interface use the new pattern we've discussed in a future CL, before this feature ships. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... File ppapi/api/private/ppb_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:19: * Extensions: http://goo.gl/rbdnR Please note (preferrably early in the comment) that this is *not* for use by conventional plugins, and is only for Content Decryption Modules (make sure you spell that out at least once). http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:32: * interface. I still think it's important that the browser might *not* call AddKey, since it's the responsibility of the application in some cases. Doesn't that potentially affect how you code the CDM? That's why I think it's worth pointing out. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:41: * <code>PP_VARTYPE_ARRAYBUFFER</code> containing container specific nit: "container specific"->"container-specific" http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:44: void NeedKey( I actually think RequestKey would be more appropriate here, but if this matches the spec better, that's OK for a private interface. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:56: * providing more data via additional calls to <code>AddKey()</code> could be I find this wording a bit confusing... can you make sure you say which party is responsible for which calls? The browser calls KeyMessage? Is that always because the app calls the browser, or does the browser do it without app intervention? And the CDM invokes AddKey? (in general: please prefer active voice and add subjects for these phrases; it's unclear who does what right now). http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:57: * required. <code>KeyAdded()</code> is sent once the sequence is completed. as above, "The CDM invokes <code>KeyAdded()</code> once the sequence is completed." (or similar) http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:76: * <code>PPP_ContentDecryptor_Private</code> interface, the decryptor will Maybe it would be clearer to say something like: "For example, if the browser invokes PPP_ContentDecryptor_Private::GenerateKeyRequest and the CDM successfully generates a key, the decryptor ..." http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:80: * responses to <code>GenerateKeyRequest()</code> calls. Of note is the text "Of note is" -> "See also" http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:97: void KeyMessage( This is another method name that's a bit opaque to me. "DidGenerateKeyMessage"? (Feel free to ignore for now, especially if this matches the spec better; but I really would prefer clearer names down the road if possible) http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:128: * deliver decrypted_block to the browser. As I mentioned before, we usually say what the browser does in response to calls. In this case, is it something like "The browser delivers the decrypted block to the application."? As I think I mentioned before, for any of these that map to a DOM event in the spec that the browser must deliver, you should say that the browser dispatches a DOM event (and specify which DOM event). http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:145: * deliver a decrypted and decoded video frame to the browser. same as above; what does the browser do? http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:161: * deliver a buffer of decrypted and decoded audio samples to the browser. ditto http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppp_cont... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:30: * session ID used in other methods on this interface. The session ID will be active voice... "The CDM [should|must] provide the session ID to the browser..." http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:47: * Provides a key or license to the decryptor for decrypting media data. Does the CDM call back to the browser at all for this one? If so, please note it (same for any methods for which the browser expects a response).
I hope this is ready to land... PTAL at updated comments. There's a lot more text now. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... File ppapi/api/private/ppb_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:19: * Extensions: http://goo.gl/rbdnR On 2012/08/14 03:31:37, dmichael wrote: > Please note (preferrably early in the comment) that this is *not* for use by > conventional plugins, and is only for Content Decryption Modules (make sure you > spell that out at least once). Done; added missing note to this file, and put it up above with the "This file defines..." http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:32: * interface. On 2012/08/14 03:31:37, dmichael wrote: > I still think it's important that the browser might *not* call AddKey, since > it's the responsibility of the application in some cases. Doesn't that > potentially affect how you code the CDM? That's why I think it's worth pointing > out. Done. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:41: * <code>PP_VARTYPE_ARRAYBUFFER</code> containing container specific On 2012/08/14 03:31:37, dmichael wrote: > nit: "container specific"->"container-specific" Done. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:44: void NeedKey( On 2012/08/14 03:31:37, dmichael wrote: > I actually think RequestKey would be more appropriate here, but if this matches > the spec better, that's OK for a private interface. NeedKey is from the spec. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:56: * providing more data via additional calls to <code>AddKey()</code> could be On 2012/08/14 03:31:37, dmichael wrote: > I find this wording a bit confusing... can you make sure you say which party is > responsible for which calls? The browser calls KeyMessage? Is that always > because the app calls the browser, or does the browser do it without app > intervention? And the CDM invokes AddKey? (in general: please prefer active > voice and add subjects for these phrases; it's unclear who does what right now). Done. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:57: * required. <code>KeyAdded()</code> is sent once the sequence is completed. On 2012/08/14 03:31:37, dmichael wrote: > as above, "The CDM invokes <code>KeyAdded()</code> once the sequence is > completed." (or similar) Done. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:76: * <code>PPP_ContentDecryptor_Private</code> interface, the decryptor will On 2012/08/14 03:31:37, dmichael wrote: > Maybe it would be clearer to say something like: > "For example, if the browser invokes > PPP_ContentDecryptor_Private::GenerateKeyRequest and the CDM successfully > generates a key, the decryptor ..." Done. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:80: * responses to <code>GenerateKeyRequest()</code> calls. Of note is the text On 2012/08/14 03:31:37, dmichael wrote: > "Of note is" -> "See also" Done. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:97: void KeyMessage( On 2012/08/14 03:31:37, dmichael wrote: > This is another method name that's a bit opaque to me. "DidGenerateKeyMessage"? > (Feel free to ignore for now, especially if this matches the spec better; but I > really would prefer clearer names down the road if possible) From spec. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:128: * deliver decrypted_block to the browser. On 2012/08/14 03:31:37, dmichael wrote: > As I mentioned before, we usually say what the browser does in response to > calls. In this case, is it something like "The browser delivers the decrypted > block to the application."? As I think I mentioned before, for any of these that > map to a DOM event in the spec that the browser must deliver, you should say > that the browser dispatches a DOM event (and specify which DOM event). All I can really add here is that the block is sent back for decode/render. That's done. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:145: * deliver a decrypted and decoded video frame to the browser. On 2012/08/14 03:31:37, dmichael wrote: > same as above; what does the browser do? Done. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:161: * deliver a buffer of decrypted and decoded audio samples to the browser. On 2012/08/14 03:31:37, dmichael wrote: > ditto Done. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppp_cont... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:30: * session ID used in other methods on this interface. The session ID will be On 2012/08/14 03:31:37, dmichael wrote: > active voice... "The CDM [should|must] provide the session ID to the > browser..." Done. http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:47: * Provides a key or license to the decryptor for decrypting media data. On 2012/08/14 03:31:37, dmichael wrote: > Does the CDM call back to the browser at all for this one? If so, please note it > (same for any methods for which the browser expects a response). Done.
just nits, lgtm http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppp_cont... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/84034/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:21: * Modules, not normal plugins. I think it would still be useful to have a note like this here; conventional plugins should not implement this interface. http://codereview.chromium.org/10545036/diff/85026/ppapi/api/private/ppb_cont... File ppapi/api/private/ppb_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/85026/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:73: * <code>MediaKeyCompleteEvent</code> at the <code>HTMlMediaElement</code>. nit: HTMl->HTML http://codereview.chromium.org/10545036/diff/85026/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:91: * on the<code>PPP_ContentDecryptor_Private</code> interface, the decryptor nit: space after first "the" http://codereview.chromium.org/10545036/diff/85026/ppapi/api/private/ppp_cont... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/85026/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:29: * be provided to the browser by the CDM via <code>KeyMessage()</code> on the I still think active voice is better than passive here, but this is better than it was.
http://codereview.chromium.org/10545036/diff/85026/ppapi/api/private/ppb_cont... File ppapi/api/private/ppb_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/85026/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:73: * <code>MediaKeyCompleteEvent</code> at the <code>HTMlMediaElement</code>. On 2012/08/14 16:54:12, dmichael wrote: > nit: HTMl->HTML Done. http://codereview.chromium.org/10545036/diff/85026/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:91: * on the<code>PPP_ContentDecryptor_Private</code> interface, the decryptor On 2012/08/14 16:54:12, dmichael wrote: > nit: space after first "the" Done. http://codereview.chromium.org/10545036/diff/85026/ppapi/api/private/ppp_cont... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/85026/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:29: * be provided to the browser by the CDM via <code>KeyMessage()</code> on the On 2012/08/14 16:54:12, dmichael wrote: > I still think active voice is better than passive here, but this is better than > it was. Done.
http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... File ppapi/api/private/ppb_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:9: * Decryption Modules (CDM), not normal plugins. singular acronym with plural expansion bugs me, but I don't have an easy fix for this sentence. http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:31: * For an internally implemented key system, the browser must provide a key to What is "internally implemented key system"? http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:32: * the decryptor plugin by calling <code>AddKey()</code> on the I would replace these two paragraphs with something that just says: The browser, likely at the direction of the Web application, should provide a key using AddKey(). We don't need to explain the whole EME flow here. http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:65: * <code>MediaKeyMessageEvent</code> at the <code>HTMLMediaElement</code>. Same, I don't think we need to go into the details of the EME calls. http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:88: * when the key system is external, must be sent to the application. Remove ", when the key system is external," http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:91: * on the <code>PPP_ContentDecryptor_Private</code> interface, the decryptor s/decryptor/plugin/ ? http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:92: * must send a key message with the session ID. must send a key message containing the key request. http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppp_cont... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:50: * passes to the application by firing a <code>MediaKeyMessageEvent</code> at Remove everything after "application" from this sentence. http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:54: * passes to the application by firing a <code>MediaKeyCompleteEvent</code> same
I can haz commit? http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... File ppapi/api/private/ppb_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:9: * Decryption Modules (CDM), not normal plugins. On 2012/08/14 17:39:58, ddorwin wrote: > singular acronym with plural expansion bugs me, but I don't have an easy fix for > this sentence. Moved down to the comment block above the interface definition so it can be "Content Decryption Module (CDM)" (in both files). http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:31: * For an internally implemented key system, the browser must provide a key to On 2012/08/14 17:39:58, ddorwin wrote: > What is "internally implemented key system"? Internally/externally stuff removed... http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:32: * the decryptor plugin by calling <code>AddKey()</code> on the On 2012/08/14 17:39:58, ddorwin wrote: > I would replace these two paragraphs with something that just says: > The browser, likely at the direction of the Web application, should provide a > key using AddKey(). > > We don't need to explain the whole EME flow here. Removed EME flow stuff-- spec is still being updated, and the information doesn't really belong here at this level. http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:65: * <code>MediaKeyMessageEvent</code> at the <code>HTMLMediaElement</code>. On 2012/08/14 17:39:58, ddorwin wrote: > Same, I don't think we need to go into the details of the EME calls. Done. http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:88: * when the key system is external, must be sent to the application. On 2012/08/14 17:39:58, ddorwin wrote: > Remove ", when the key system is external," Done. http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:91: * on the <code>PPP_ContentDecryptor_Private</code> interface, the decryptor On 2012/08/14 17:39:58, ddorwin wrote: > s/decryptor/plugin/ ? Done. http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:92: * must send a key message with the session ID. On 2012/08/14 17:39:58, ddorwin wrote: > must send a key message containing the key request. Done. http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppp_cont... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:50: * passes to the application by firing a <code>MediaKeyMessageEvent</code> at On 2012/08/14 17:39:58, ddorwin wrote: > Remove everything after "application" from this sentence. Done. http://codereview.chromium.org/10545036/diff/85027/ppapi/api/private/ppp_cont... ppapi/api/private/ppp_content_decryptor_private.idl:54: * passes to the application by firing a <code>MediaKeyCompleteEvent</code> On 2012/08/14 17:39:58, ddorwin wrote: > same Done plus minor tweak to wording.
LGTM. Just one nit. http://codereview.chromium.org/10545036/diff/92026/ppapi/api/private/ppb_cont... File ppapi/api/private/ppb_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/92026/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:60: * to pass additional data to the CDM via calls to <code>AddKey()</code> on nit: s/additional/the/
http://codereview.chromium.org/10545036/diff/92026/ppapi/api/private/ppb_cont... File ppapi/api/private/ppb_content_decryptor_private.idl (right): http://codereview.chromium.org/10545036/diff/92026/ppapi/api/private/ppb_cont... ppapi/api/private/ppb_content_decryptor_private.idl:60: * to pass additional data to the CDM via calls to <code>AddKey()</code> on On 2012/08/14 18:49:22, ddorwin wrote: > nit: s/additional/the/ Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/10545036/80024
Try job failure for 10545036-80024 (retry) on linux_rel for step "interactive_ui_tests". It's a second try, previously, step "interactive_ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
