Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10871006:
Connect PpapiDecryptor and PluginInstance. (Closed)
|
Created:
8 years, 4 months ago by xhwang Modified:
8 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConnect PpapiDecryptor and PluginInstance.
- In PpapiDecrytor, enable calls into the PluginInstance.
- In PluginInstance, passing in all data needed by the PPP_ContentDecryptor interface.
- Hook up DecryptorClient and PPB_ContentDecryptor calls in PluginInstance.
BUG=138139
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153629
Patch Set 1 #
Total comments: 68
Patch Set 2 : Resolve comments. #
Total comments: 4
Patch Set 3 : Resolve comments. #
Total comments: 6
Patch Set 4 : Resolve dmichael's comments. #Patch Set 5 : Avoid dependency on content. #
Total comments: 2
Patch Set 6 : Resolve comments. #Patch Set 7 : Rebase and fix lint warnings. #Patch Set 8 : Fix a minor bug happened during code copying. #
Messages
Total messages: 16 (0 generated)
Hello, This CL is based on tomf@'s CL: http://codereview.chromium.org/10854209/. I'll rebase when his CL is landed. PTAL!
Initial comments, more tomorrow... http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:48: client_->KeyError(key_system, "", kUnknownError, 0); nit: one line statement in the if, not sure about media code view on whether curly braces are needed here or not. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:307: const uint8* data, int size) { nit: seems to be consistently one arg per line in here, but maybe type* and size of pointer on the same line is fine in this code. (I assume that's the case in media code, now that I've seen this CL :)) http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1381: This probably belongs in the anonymous namespace w/MakeBufferResource. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1391: void MakePPEncryptedBlockInfo( s/MakePPEncryptedBlockInfo/MakeEncryptedBlockInfo/ ? More consistent w/MakeBufferResource, but I'm not sure how great a name that was to begin with. :) http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1442: &block_info); Wish there was a way to avoid this copy...
Need to look over PI.cc some more, but here's some initial feedback. http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:10: #include "base/location.h" What is this used for? http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:40: DVLOG(1) << "GenerateKeyRequest()"; Probably worth providing context (PPAPI) since AesDecryptor or WMPI might have similar logging. http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:41: DCHECK(render_loop_proxy_->BelongsToCurrentThread()); It's not obvious why this needs to be the case in this class. It may be true, but do we need to check it? Are these checks redundant with other checks? http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:46: std::string(reinterpret_cast<const char*>(init_data), It's unfortunate we have to do this cast in multiple files. Should we just do it in WMPI? http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:48: client_->KeyError(key_system, "", kUnknownError, 0); You might move this to a helper function and log "failed calling plugin" since there is no other way to distinguish this failure. http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:63: session_id, nit: It seems this would look better if indented further. http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:67: client_->KeyError(key_system, session_id, kUnknownError, 0); Does client always post to ensure we never report events on the same callstack as the WebKit call? I don't remember where we do that. http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:82: const scoped_refptr<media::DecoderBuffer>& encrypted, Is it possible to avoid refptr for this? http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:93: if (cdm_plugin_->Decrypt(encrypted, decrypt_cb)) { Missing a '!' ? http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:387: next_decryption_request_id_(0) { Should we avoid using 0? http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1331: decryptor_client_ = decryptor_client; Does this class assume it's only called on the main thread? http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1381: On 2012/08/22 01:05:47, tomf wrote: > This probably belongs in the anonymous namespace w/MakeBufferResource. Right, anonymous namespaces should not be in the middle of a file. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.h:19: #include "media/base/decryptor.h" After 20
Finishes reviewing. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1328: void PluginInstance::SetDecryptClient( Why not set_decryptor_client()? http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1385: DCHECK_LE(str.size(), array_size); CHECK_LE? Crash rather than overrun. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1402: CopyStringToArray(decrypt_config.key_id(), Just looking at the parameters here, the implementation does not do anything like I would expect. IOW, it's a weird signature. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1409: CHECK(block_info->num_subsamples < arraysize(block_info->subsamples)); Do we check nicely somewhere else? We shouldn't crash due to user/page input. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1436: memset(&block_info, 0, sizeof(block_info)); TODO - fix initialization here and in Tom's CL. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2119: if (mapper.data() || mapper.size()) && ? http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2156: << block_info->tracking_info.request_id << " not found."; The ID isn't really useful to anyone not debugging this code. Will a DLOG do? Is there any reason to think this will happen? http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2173: DCHECK_EQ(block_info->result, PP_DECRYPTRESULT_SUCCESS); Is there a DECODE_ERROR yet or is that another CL? http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2181: if (mapper.data() == NULL || mapper.size() == 0) { Inconsistent use of == vs. 2119. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.h:740: uint64_t next_decryption_request_id_; Does this need to be unsigned? Does it really need to be 64-bits?
http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:46: std::string(reinterpret_cast<const char*>(init_data), On 2012/08/22 01:26:26, ddorwin wrote: > It's unfortunate we have to do this cast in multiple files. Should we just do it > in WMPI? It's not really clear to me why we pass a string in to PluginInstance anyway. Aren't you forcing an unnecessary copy? Why not just pass the same pointer+length? http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:63: session_id, On 2012/08/22 01:26:26, ddorwin wrote: > nit: It seems this would look better if indented further. Agreed... I think it should be 4 from the open paren on 62 http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:307: const uint8* data, int size) { On 2012/08/22 01:05:47, tomf wrote: > nit: seems to be consistently one arg per line in here, but maybe type* and size > of pointer on the same line is fine in this code. (I assume that's the case in > media code, now that I've seen this CL :)) We usually do 1-per-line, but since those two are so closely related, this makes sense to me as-is. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1331: decryptor_client_ = decryptor_client; On 2012/08/22 01:26:26, ddorwin wrote: > Does this class assume it's only called on the main thread? Yes http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1422: const media::Decryptor::DecryptCB& decrypt_cb) { Does it still make sense to have a callback instead of just talking to the decrypt_client_? http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2107: StringVar* default_url_string = StringVar::FromPPVar(default_url_var); You have to check these pointers before calling value(). You shouldn't trust here that the PP_Var is a valid string var. (same elsewhere) http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2172: nit: It seems like there are a few excess lines here (2159, 2167, 2172 at least)
Most comments resolved, PTAL! http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:10: #include "base/location.h" On 2012/08/22 01:26:26, ddorwin wrote: > What is this used for? For FROM_HERE in PostTask. http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:40: DVLOG(1) << "GenerateKeyRequest()"; On 2012/08/22 01:26:26, ddorwin wrote: > Probably worth providing context (PPAPI) since AesDecryptor or WMPI might have > similar logging. In the logging, it will always show the file name so I thought function name here would be clear enough. An example in media code: https://cs.corp.google.com/#chrome/src/webkit/media/webmediaplayer_impl.cc&q=... But I can change for sure if the convention is otherwise. http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:41: DCHECK(render_loop_proxy_->BelongsToCurrentThread()); On 2012/08/22 01:26:26, ddorwin wrote: > It's not obvious why this needs to be the case in this class. It may be true, > but do we need to check it? Are these checks redundant with other checks? I guess this is just another (better) way to comment that this call should always be called on the renderer thread. If we don't DCHECK here, we'd need to comment it. http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:46: std::string(reinterpret_cast<const char*>(init_data), On 2012/08/22 01:26:26, ddorwin wrote: > It's unfortunate we have to do this cast in multiple files. Should we just do it > in WMPI? We discussed this before, and decided to use: - string for key, iv, key_id etc since they are short (and copying a short string doesn't hurt performance). - pointer/size for initdata, key message, or any other large binary data. This is why we have mixed type, e.g. in KeyMessage and NeedKey: https://cs.corp.google.com/#chrome/src/webkit/media/webmediaplayer_proxy.cc&t... But really, how big could initdata or key message be? http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:46: std::string(reinterpret_cast<const char*>(init_data), On 2012/08/22 20:30:15, dmichael wrote: > On 2012/08/22 01:26:26, ddorwin wrote: > > It's unfortunate we have to do this cast in multiple files. Should we just do > it > > in WMPI? > It's not really clear to me why we pass a string in to PluginInstance anyway. > Aren't you forcing an unnecessary copy? Why not just pass the same > pointer+length? Yes, if we decided to use pointer/size for init_data, we might just pass pointer/size to PluginInstance (PI) also. There're some inconsistency here. But we need to decide on what data to use in media pipeline first. http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:48: client_->KeyError(key_system, "", kUnknownError, 0); On 2012/08/22 01:26:26, ddorwin wrote: > You might move this to a helper function and log "failed calling plugin" since > there is no other way to distinguish this failure. Done. http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:48: client_->KeyError(key_system, "", kUnknownError, 0); On 2012/08/22 01:05:47, tomf wrote: > nit: one line statement in the if, not sure about media code view on whether > curly braces are needed here or not. I remember we should use curly braces when the if() statement takes more than one line... but not 100% sure either. http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:63: session_id, On 2012/08/22 20:30:15, dmichael wrote: > On 2012/08/22 01:26:26, ddorwin wrote: > > nit: It seems this would look better if indented further. > Agreed... I think it should be 4 from the open paren on 62 Done. But not sure if this is what you meant. http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:67: client_->KeyError(key_system, session_id, kUnknownError, 0); On 2012/08/22 01:26:26, ddorwin wrote: > Does client always post to ensure we never report events on the same callstack > as the WebKit call? I don't remember where we do that. Yes, the client is the webmediaplayer_proxy, which will always post: https://cs.corp.google.com/#chrome/src/webkit/media/webmediaplayer_proxy.cc&t... http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:82: const scoped_refptr<media::DecoderBuffer>& encrypted, On 2012/08/22 01:26:26, ddorwin wrote: > Is it possible to avoid refptr for this? I think the rule is if |encrypted| is a scoped_refptr, it should always be passed as a scoped_refptr, not, e.g., a raw pointer. http://codereview.chromium.org/10871006/diff/1/webkit/media/crypto/ppapi_decr... webkit/media/crypto/ppapi_decryptor.cc:93: if (cdm_plugin_->Decrypt(encrypted, decrypt_cb)) { On 2012/08/22 01:26:26, ddorwin wrote: > Missing a '!' ? Oops, thanks for catching this. Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:307: const uint8* data, int size) { On 2012/08/22 20:30:15, dmichael wrote: > On 2012/08/22 01:05:47, tomf wrote: > > nit: seems to be consistently one arg per line in here, but maybe type* and > size > > of pointer on the same line is fine in this code. (I assume that's the case in > > media code, now that I've seen this CL :)) > We usually do 1-per-line, but since those two are so closely related, this makes > sense to me as-is. Yeah, this is actually in chromium code style guide: // Only legal when |second_argument| and |third_argument| are intimately // related, e.g. (x, y) coords. void SpecialCase(TypeA first_argument, TypeB second_argument, TypeC third_argument, TypeD fourth_argument); http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:387: next_decryption_request_id_(0) { On 2012/08/22 01:26:26, ddorwin wrote: > Should we avoid using 0? Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1328: void PluginInstance::SetDecryptClient( On 2012/08/22 04:09:21, ddorwin wrote: > Why not set_decryptor_client()? Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1381: On 2012/08/22 01:05:47, tomf wrote: > This probably belongs in the anonymous namespace w/MakeBufferResource. Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1381: On 2012/08/22 01:26:26, ddorwin wrote: > On 2012/08/22 01:05:47, tomf wrote: > > This probably belongs in the anonymous namespace w/MakeBufferResource. > > Right, anonymous namespaces should not be in the middle of a file. Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1385: DCHECK_LE(str.size(), array_size); On 2012/08/22 04:09:21, ddorwin wrote: > CHECK_LE? Crash rather than overrun. Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1391: void MakePPEncryptedBlockInfo( On 2012/08/22 01:05:47, tomf wrote: > s/MakePPEncryptedBlockInfo/MakeEncryptedBlockInfo/ ? > > More consistent w/MakeBufferResource, but I'm not sure how great a name that was > to begin with. :) Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1402: CopyStringToArray(decrypt_config.key_id(), On 2012/08/22 04:09:21, ddorwin wrote: > Just looking at the parameters here, the implementation does not do anything > like I would expect. IOW, it's a weird signature. Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1409: CHECK(block_info->num_subsamples < arraysize(block_info->subsamples)); On 2012/08/22 04:09:21, ddorwin wrote: > Do we check nicely somewhere else? We shouldn't crash due to user/page input. Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1422: const media::Decryptor::DecryptCB& decrypt_cb) { On 2012/08/22 20:30:15, dmichael wrote: > Does it still make sense to have a callback instead of just talking to the > decrypt_client_? Unfortunately, decrypt_client_ connects the decryptor to the WebKit, and this decrypt_cb connects the decryptor back to the media pipeline. So we still need a separate cb here. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1436: memset(&block_info, 0, sizeof(block_info)); On 2012/08/22 04:09:21, ddorwin wrote: > TODO - fix initialization here and in Tom's CL. Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1442: &block_info); On 2012/08/22 01:05:47, tomf wrote: > Wish there was a way to avoid this copy... Yeah, it's ugly, fortunately we are not actually copying too much data. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2107: StringVar* default_url_string = StringVar::FromPPVar(default_url_var); On 2012/08/22 20:30:15, dmichael wrote: > You have to check these pointers before calling value(). You shouldn't trust > here that the PP_Var is a valid string var. (same elsewhere) Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2119: if (mapper.data() || mapper.size()) On 2012/08/22 04:09:21, ddorwin wrote: > && ? Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2156: << block_info->tracking_info.request_id << " not found."; On 2012/08/22 04:09:21, ddorwin wrote: > The ID isn't really useful to anyone not debugging this code. Will a DLOG do? Is > there any reason to think this will happen? Suppose the CDM is implemented by a third party. They can just return us blocks with invalid request ID. Removed log. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2173: DCHECK_EQ(block_info->result, PP_DECRYPTRESULT_SUCCESS); On 2012/08/22 04:09:21, ddorwin wrote: > Is there a DECODE_ERROR yet or is that another CL? DeliverBlock should not handle decode_error. Changed the code a little to make the error handling better organized. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2181: if (mapper.data() == NULL || mapper.size() == 0) { On 2012/08/22 04:09:21, ddorwin wrote: > Inconsistent use of == vs. 2119. Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.h:19: #include "media/base/decryptor.h" On 2012/08/22 01:26:26, ddorwin wrote: > After 20 Done. http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.h:740: uint64_t next_decryption_request_id_; On 2012/08/22 04:09:21, ddorwin wrote: > Does this need to be unsigned? Does it really need to be 64-bits? Done.
lgtm % comments http://codereview.chromium.org/10871006/diff/8001/webkit/media/crypto/ppapi_d... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/10871006/diff/8001/webkit/media/crypto/ppapi_d... webkit/media/crypto/ppapi_decryptor.cc:102: client_->KeyError(key_system, session_id, kUnknownError, 0); The intent was to DLOG "failed calling plugin". This should also be named as such, since we'd want to have parameters for the two error values if it was used for anything else. http://codereview.chromium.org/10871006/diff/8001/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10871006/diff/8001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2187: EnterResourceNoLock<PPB_Buffer_API> enter(decrypted_block, true); add empty line - error handling is complete, and the next line is the normal flow.
http://codereview.chromium.org/10871006/diff/8001/webkit/media/crypto/ppapi_d... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/10871006/diff/8001/webkit/media/crypto/ppapi_d... webkit/media/crypto/ppapi_decryptor.cc:102: client_->KeyError(key_system, session_id, kUnknownError, 0); On 2012/08/24 05:54:43, ddorwin wrote: > The intent was to DLOG "failed calling plugin". This should also be named as > such, since we'd want to have parameters for the two error values if it was used > for anything else. Done. http://codereview.chromium.org/10871006/diff/8001/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10871006/diff/8001/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2187: EnterResourceNoLock<PPB_Buffer_API> enter(decrypted_block, true); On 2012/08/24 05:54:43, ddorwin wrote: > add empty line - error handling is complete, and the next line is the normal > flow. Done.
https://chromiumcodereview.appspot.com/10871006/diff/1/webkit/media/crypto/pp... File webkit/media/crypto/ppapi_decryptor.cc (right): https://chromiumcodereview.appspot.com/10871006/diff/1/webkit/media/crypto/pp... webkit/media/crypto/ppapi_decryptor.cc:46: std::string(reinterpret_cast<const char*>(init_data), On 2012/08/24 00:51:51, xhwang wrote: > On 2012/08/22 20:30:15, dmichael wrote: > > On 2012/08/22 01:26:26, ddorwin wrote: > > > It's unfortunate we have to do this cast in multiple files. Should we just > do > > it > > > in WMPI? > > It's not really clear to me why we pass a string in to PluginInstance anyway. > > Aren't you forcing an unnecessary copy? Why not just pass the same > > pointer+length? > > Yes, if we decided to use pointer/size for init_data, we might just pass > pointer/size to PluginInstance (PI) also. There're some inconsistency here. But > we need to decide on what data to use in media pipeline first. The performance wouldn't bother me if the code was cleaner this way, but it seems odd to convert from pointer+size to a string, then back to pointer+size, with associated reinterpret_casts, if there's no benefit to readability or performance. Even a vector of uint8_t would seem better to me, since it better conveys that we're talking about binary data. Ultimately, I suspect you'll want some kind of ArrayBuffer or ArrayBufferView all the way through the stack. You can leave it this way if you think it's appropriate, but maybe a TODO would be good, if you're still figuring out what type it should be. https://chromiumcodereview.appspot.com/10871006/diff/9007/webkit/plugins/ppap... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): https://chromiumcodereview.appspot.com/10871006/diff/9007/webkit/plugins/ppap... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1442: uint64_t request_id = next_decryption_request_id_++; Now that next_decryption_request_id_ is an int, overflow is implementation-defined. You should either use an unsigned, or check for overflow. Also, should request_id be the same type as next_decryption_request_id_? https://chromiumcodereview.appspot.com/10871006/diff/9007/webkit/plugins/ppap... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1447: memset(&block_info, 0, sizeof(block_info)); Aren't you going to want to do this any time you call MakeEncryptedBlockInfo? Maybe it would make sense to memset inside that function? https://chromiumcodereview.appspot.com/10871006/diff/9007/webkit/plugins/ppap... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2108: decryptor_client_->KeyError("", "", media::Decryptor::kUnknownError, 0); You also should return early, or you might dereference a NULL pointer. (same elsewhere)
http://codereview.chromium.org/10871006/diff/9007/webkit/plugins/ppapi/ppapi_... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10871006/diff/9007/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1442: uint64_t request_id = next_decryption_request_id_++; On 2012/08/24 17:18:34, dmichael wrote: > Now that next_decryption_request_id_ is an int, overflow is > implementation-defined. You should either use an unsigned, or check for > overflow. Also, should request_id be the same type as > next_decryption_request_id_? Right, overflow on signed integer is undefined behavior. Thanks for bringing this up. Change it to uint32_t, also make request_id in PP_DecryptTrackingInfo an uint32_t to avoid unnecessary data conversion. http://codereview.chromium.org/10871006/diff/9007/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:1447: memset(&block_info, 0, sizeof(block_info)); On 2012/08/24 17:18:34, dmichael wrote: > Aren't you going to want to do this any time you call MakeEncryptedBlockInfo? > Maybe it would make sense to memset inside that function? Done. http://codereview.chromium.org/10871006/diff/9007/webkit/plugins/ppapi/ppapi_... webkit/plugins/ppapi/ppapi_plugin_instance.cc:2108: decryptor_client_->KeyError("", "", media::Decryptor::kUnknownError, 0); On 2012/08/24 17:18:34, dmichael wrote: > You also should return early, or you might dereference a NULL pointer. (same > elsewhere) Good catch, thanks! Done.
LGTM for me.
lgtm http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:387: next_decryption_request_id_(0) { On 2012/08/24 00:51:51, xhwang wrote: > On 2012/08/22 01:26:26, ddorwin wrote: > > Should we avoid using 0? > > Done. Don't know if you care, but 0 is still possible on roll-over. Not sure how many bundles you expect per second, but I expect roll-over is going to be rare at best. http://codereview.chromium.org/10871006/diff/18/webkit/media/crypto/ppapi_dec... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/10871006/diff/18/webkit/media/crypto/ppapi_dec... webkit/media/crypto/ppapi_decryptor.cc:44: std::string(reinterpret_cast<const char*>(init_data), Could you add a TODO here about figuring out what type you want to use to replace string? I'd rather we didn't have array->string->array long-term.
http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10871006/diff/1/webkit/plugins/ppapi/ppapi_plu... webkit/plugins/ppapi/ppapi_plugin_instance.cc:387: next_decryption_request_id_(0) { On 2012/08/27 20:14:48, dmichael wrote: > On 2012/08/24 00:51:51, xhwang wrote: > > On 2012/08/22 01:26:26, ddorwin wrote: > > > Should we avoid using 0? > > > > Done. > Don't know if you care, but 0 is still possible on roll-over. Not sure how many > bundles you expect per second, but I expect roll-over is going to be rare at > best. Session ID is not likely to roll-over at all. Not using 0 here so that an uninitialized variable (likely to be zero) can't coincidentally matches a valid session ID. http://codereview.chromium.org/10871006/diff/18/webkit/media/crypto/ppapi_dec... File webkit/media/crypto/ppapi_decryptor.cc (right): http://codereview.chromium.org/10871006/diff/18/webkit/media/crypto/ppapi_dec... webkit/media/crypto/ppapi_decryptor.cc:44: std::string(reinterpret_cast<const char*>(init_data), On 2012/08/27 20:14:48, dmichael wrote: > Could you add a TODO here about figuring out what type you want to use to > replace string? I'd rather we didn't have array->string->array long-term. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/10871006/6007
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/10871006/15005
Change committed as 153629 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
