Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10876014:
Hook up CDM calls in CdmWrapper. (Closed)
|
Created:
8 years, 4 months ago by xhwang Modified:
7 years, 3 months ago CC:
chromium-reviews, piman+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, yzshen+watch_chromium.org, ihf+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHook up CDM calls in CdmWrapper.
Updated pp::ContentDecryptor_Private methods in CdmWrapper to call into the CDM. Also updated the event firing through pp::ContentDecryptor_Private interface in CdmWrapper.
BUG=138139
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153704
Patch Set 1 #
Total comments: 66
Patch Set 2 : Resolve comments. #
Total comments: 7
Patch Set 3 : Resolve ddorwin@'s comments. #
Total comments: 4
Patch Set 4 : Resolve comments and Rebase. #
Messages
Total messages: 13 (0 generated)
Hello, This CL connects the CdmWrapper and the CDM. PTAL! xhwang
http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:9: * Decryption Modules (CDM), not normal plugins. The acronym definition was moved down to the block below because a singular acronym looked weird next to the plural phrase. http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:41: * successfully. <code>PP_TRUE</code> otherwise. Once the call reaches the I think you meant PP_FALSE here. :) Also, sentences; Returns <code>PP_FALSE</code> otherwise. Still a fragment, but makes sense contextually. http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:42: * CDM, the call result/stuatus can always be reported asynchronously through s/stuatus/status/ s/can always be/is/ Ditto for repeated blocks. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:28: // This needs to be consistent with media::Decryptor::KeyError! s/needs to/must/ http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:135: std::string())); Seems kind of unfriendly to pass an empty string back up-- even something like "internal error" might be better. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:155: session_id.size(), I still think we should get rid of the size param for session ID's, since we can pretty much guarantee NUL termination. :) Add a TODO? http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:214: cdm::Status status = cdm_->Decrypt(input_buffer, &output_buffer); I think we should check this status here, and return false if we already know there was an error. It doesn't mesh with your current comments in the IDL, but I think it's better to notify the caller instead hiding the information. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:230: A comment here that this returns a resource with a ref count of one would be good-- you can copy/paste the one in ppapi_plugin_instance.cc. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:260: // another object. s/object/object that runs in a different thread/ ? Just a TODO, but maybe clearer this way...
http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:9: * Decryption Modules (CDM), not normal plugins. Tom removed this because of the plural - singular mismatch and just left it below at 19. You can try to fix it another way. http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:40: * @return <code>PP_TRUE</code> if this call can be forwarded to the CDM s/can be/was/ http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:41: * successfully. <code>PP_TRUE</code> otherwise. Once the call reaches the PP_FALSE http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:42: * CDM, the call result/stuatus can always be reported asynchronously through s/can always be/should be/ ? http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:87: void KeyAdded(int32_t result, const std::string& session_id); result is never used. Do we need it? http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:89: void KeyError(int32_t result, const std::string& session_id); Should have the two error values as parameters too. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:134: CallOnMain(callback_factory_.NewCallback(&CdmWrapper::KeyError, We need to pass cdm::Status to the error. Eventually, cdm::Status should have the same values as the EME API. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:135: std::string())); On 2012/08/22 16:54:31, tomf wrote: > Seems kind of unfriendly to pass an empty string back up-- even something like > "internal error" might be better. it's the session ID, which doesn't exist yet. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:155: session_id.size(), On 2012/08/22 16:54:31, tomf wrote: > I still think we should get rid of the size param for session ID's, since we can > pretty much guarantee NUL termination. :) > > Add a TODO? This is consistent with the other parameters, and I think it's better to be specific, especially with external code. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:183: // TODO(xhwang): Need to do anything here? Spec says TBD. I think this TBD belongs in the CDMs, not here. There is not an event to report. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:214: cdm::Status status = cdm_->Decrypt(input_buffer, &output_buffer); I've changed my mind (since our conversation) about error reporting after looking at this. We are hiding CDM errors (i.e. license expired, etc.) because we end up converting this return value to one of three decrypt results. Right now, the only way these can be reported in the steady state is via Decrypt calls. (That might be a problem we should solve with a permanent callback - i.e. in case playback is paused. Until then we should handle it here.) What do others think? http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:259: // TODO(xhwang): Fix this. This is not safe as the memory is allocated in "not _always_ safe" It works for now... http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:260: // another object. On 2012/08/22 16:54:31, tomf wrote: > s/object/object that runs in a different thread/ ? > > Just a TODO, but maybe clearer this way... It's really a different shared object. I'm sure there is a better name for the real issue. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:275: const cdm::OutputBuffer& output_buffer, can't be const if we're deleting a member. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:284: if (status == cdm::kSuccess) switch statement? esp. useful when we have more errors. Thought it's TBD whether this conversion should be here or at the caller (see earlier comment about reporting errors). http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:296: delete [] output_buffer.data; How do we guarantee output_buffer is not being used by another thread? .data = NULL since it's a parameter and could be accessed elsewhere. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cont... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:46: uint32_t session_id_size; should this really be unsigned? If you change this, need to change ==0 to <=0 for error checking.
http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:74: * CDM, the call result/stuatus can always be reported asynchronously through Here and elsewhere... can we omit asynchronously? It's not totally clear what that means to someone writing the PPP side. In reality, I think the PPB call will ensure that it happens asynchronously, even if it is called immediately from within the corresponding PPP call. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:28: // This needs to be consistent with media::Decryptor::KeyError! On 2012/08/22 16:54:31, tomf wrote: > s/needs to/must/ Should these be defined in the API? If they were, you could do a compile assert somewhere to make sure they stay in sync. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:29: enum KeyError { How about a different name, like KeyErrorCode, to distinguish from the function of the same name? http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:120: std::string())); You don't have to CallOnMainThread for this, do you? You know you're on the main thread, and all your PPB calls will behave asynchronously. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:134: CallOnMain(callback_factory_.NewCallback(&CdmWrapper::KeyError, CallOnMain isn't necessary here either (it's probably not necessary anywhere yet, since you're not calling from background threads yet). And... we're probably about a month away from you being able to make PPB calls off the main thread. Though you'll still have to worry about the thread safety of your own class, of course. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:150: PP_DCHECK(init_data.Map() && init_data.ByteLength() > 0); Please don't do Map() inside of a DCHECK. It can have non-trivial behavior, and you don't want it to work differently in debug vs release. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:155: session_id.size(), On 2012/08/22 18:25:54, ddorwin wrote: > On 2012/08/22 16:54:31, tomf wrote: > > I still think we should get rid of the size param for session ID's, since we > can > > pretty much guarantee NUL termination. :) > > > > Add a TODO? > > This is consistent with the other parameters, and I think it's better to be > specific, especially with external code. It also makes it easier to use stuff like memcpy or memset without having to measure first. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:175: session_id.size()); Could maybe be one line? http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:178: CallOnMain(callback_factory_.NewCallback(&CdmWrapper::KeyError, could be one line? (esp. since probably doesn't need to be CallOnMain) http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:232: uint32_t data_size) { nit: looks like data_size could fit on the previous line, maybe? http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:263: delete [] key_message.default_url; Can you make cdm::KeyMessage smarter or something? This looks ugly and easy to forget.
Comments mostly resolved, PTAL! http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:9: * Decryption Modules (CDM), not normal plugins. On 2012/08/22 16:54:31, tomf wrote: > The acronym definition was moved down to the block below because a singular > acronym looked weird next to the plural phrase. I didn't notice the one on line 19. No need to add one here then. http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:40: * @return <code>PP_TRUE</code> if this call can be forwarded to the CDM On 2012/08/22 18:25:54, ddorwin wrote: > s/can be/was/ Done. http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:41: * successfully. <code>PP_TRUE</code> otherwise. Once the call reaches the On 2012/08/22 18:25:54, ddorwin wrote: > PP_FALSE Done. http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:42: * CDM, the call result/stuatus can always be reported asynchronously through On 2012/08/22 18:25:54, ddorwin wrote: > s/can always be/should be/ ? Done. http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:42: * CDM, the call result/stuatus can always be reported asynchronously through On 2012/08/22 16:54:31, tomf wrote: > s/stuatus/status/ > s/can always be/is/ > > Ditto for repeated blocks. Done. http://codereview.chromium.org/10876014/diff/1/ppapi/api/private/ppp_content_... ppapi/api/private/ppp_content_decryptor_private.idl:74: * CDM, the call result/stuatus can always be reported asynchronously through On 2012/08/23 18:11:29, dmichael wrote: > Here and elsewhere... can we omit asynchronously? It's not totally clear what > that means to someone writing the PPP side. In reality, I think the PPB call > will ensure that it happens asynchronously, even if it is called immediately > from within the corresponding PPP call. Done. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:28: // This needs to be consistent with media::Decryptor::KeyError! On 2012/08/22 16:54:31, tomf wrote: > s/needs to/must/ Done. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:28: // This needs to be consistent with media::Decryptor::KeyError! On 2012/08/23 18:11:29, dmichael wrote: > On 2012/08/22 16:54:31, tomf wrote: > > s/needs to/must/ > Should these be defined in the API? If they were, you could do a compile assert > somewhere to make sure they stay in sync. Good point, added a TODO to add a PP_MediaKeyError enum to avoid the static_cast here: http://codereview.chromium.org/10871006/diff/9007/webkit/plugins/ppapi/ppapi_... http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:29: enum KeyError { On 2012/08/23 18:11:29, dmichael wrote: > How about a different name, like KeyErrorCode, to distinguish from the function > of the same name? Renamed to MediaKeyError to be consistent to the spec. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:87: void KeyAdded(int32_t result, const std::string& session_id); On 2012/08/22 18:25:54, ddorwin wrote: > result is never used. Do we need it? Even if we don't use the result, I think this is required if we want to use CallOnMainThread/PP_CompletionCallback. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:89: void KeyError(int32_t result, const std::string& session_id); On 2012/08/22 18:25:54, ddorwin wrote: > Should have the two error values as parameters too. For now we really don't need it. If we need it in the future, we can add it any time. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:120: std::string())); On 2012/08/23 18:11:29, dmichael wrote: > You don't have to CallOnMainThread for this, do you? You know you're on the main > thread, and all your PPB calls will behave asynchronously. See reply below. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:134: CallOnMain(callback_factory_.NewCallback(&CdmWrapper::KeyError, On 2012/08/23 18:11:29, dmichael wrote: > CallOnMain isn't necessary here either (it's probably not necessary anywhere > yet, since you're not calling from background threads yet). > > And... we're probably about a month away from you being able to make PPB calls > off the main thread. Though you'll still have to worry about the thread safety > of your own class, of course. As per offline discussion with dmichael@: If running out-of-process, PPB calls will always behave asynchronously since IPC is involved. In that case, if we are already on main thread, we don't need to use CallOnMain to help us call PPB call on main thread, or to help call PPB asynchronously. For now, we still want to keep in-process working (for easy debugging). So I'll keep the CallOnMain here and anywhere else to make sure we call PPB asynchronously. In the near future, we will support out-of-process only (either because we will start using SimpleThread to forward Decrypt() calls to another thread and in-process will stop working, or because pepper team stop supporting in-process). Then we can drop CallOnMain at least for GenerateKeyRequest/AddKey/CancelKeyQuest. Added TODO about the above. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:134: CallOnMain(callback_factory_.NewCallback(&CdmWrapper::KeyError, On 2012/08/22 18:25:54, ddorwin wrote: > We need to pass cdm::Status to the error. Eventually, cdm::Status should have > the same values as the EME API. Agreed, added a TODO. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:183: // TODO(xhwang): Need to do anything here? Spec says TBD. On 2012/08/22 18:25:54, ddorwin wrote: > I think this TBD belongs in the CDMs, not here. There is not an event to report. Done. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:214: cdm::Status status = cdm_->Decrypt(input_buffer, &output_buffer); On 2012/08/22 16:54:31, tomf wrote: > I think we should check this status here, and return false if we already know > there was an error. It doesn't mesh with your current comments in the IDL, but I > think it's better to notify the caller instead hiding the information. The reason I add the comments in the IDL is to avoid double firing of callbacks. For example, if I return false here, the caller would assume the call didn't reach the CDM at all and will fire the callback with error. But the DeliverBlock below will also trigger the callback to be fired. So for this particular case we can't return false. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:214: cdm::Status status = cdm_->Decrypt(input_buffer, &output_buffer); On 2012/08/22 18:25:54, ddorwin wrote: > I've changed my mind (since our conversation) about error reporting after > looking at this. We are hiding CDM errors (i.e. license expired, etc.) because > we end up converting this return value to one of three decrypt results. Right > now, the only way these can be reported in the steady state is via Decrypt > calls. (That might be a problem we should solve with a permanent callback - i.e. > in case playback is paused. Until then we should handle it here.) > > What do others think? As per the spec: 5.1. Encrypted Block Encountered: - KeyError is only fired "if handler fails to load or initialize". - For all other errors, we report "MEDIA_ERR_ENCRYPTED". So even if licence expired and decryption error happened, we should report MEDIA_ERR_ENCRYPTED by converting the cdm status to a decrypt error code. We can talk about this more offline. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:230: On 2012/08/22 16:54:31, tomf wrote: > A comment here that this returns a resource with a ref count of one would be > good-- you can copy/paste the one in ppapi_plugin_instance.cc. Done. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:232: uint32_t data_size) { On 2012/08/23 18:11:29, dmichael wrote: > nit: looks like data_size could fit on the previous line, maybe? Unfortunately it doesn't fit :( http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:259: // TODO(xhwang): Fix this. This is not safe as the memory is allocated in On 2012/08/22 18:25:54, ddorwin wrote: > "not _always_ safe" > It works for now... Done. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:260: // another object. On 2012/08/22 18:25:54, ddorwin wrote: > On 2012/08/22 16:54:31, tomf wrote: > > s/object/object that runs in a different thread/ ? > > > > Just a TODO, but maybe clearer this way... > > It's really a different shared object. I'm sure there is a better name for the > real issue. Done. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:263: delete [] key_message.default_url; On 2012/08/23 18:11:29, dmichael wrote: > Can you make cdm::KeyMessage smarter or something? This looks ugly and easy to > forget. We have a plan which is not trivial. The above TODO covers this task. Will fix this pretty soon. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:275: const cdm::OutputBuffer& output_buffer, On 2012/08/22 18:25:54, ddorwin wrote: > can't be const if we're deleting a member. Done. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:284: if (status == cdm::kSuccess) On 2012/08/22 18:25:54, ddorwin wrote: > switch statement? esp. useful when we have more errors. > Thought it's TBD whether this conversion should be here or at the caller (see > earlier comment about reporting errors). Done. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:296: delete [] output_buffer.data; On 2012/08/22 18:25:54, ddorwin wrote: > How do we guarantee output_buffer is not being used by another thread? I am not sure what's the problem? The output_buffer is created in the Decrypt() call and will only be accessed here afterwards. > .data = NULL since it's a parameter and could be accessed elsewhere. Done. http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cont... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cont... webkit/media/crypto/ppapi/content_decryption_module.h:46: uint32_t session_id_size; On 2012/08/22 18:25:54, ddorwin wrote: > should this really be unsigned? > If you change this, need to change ==0 to <=0 for error checking. Good catch, chromium/google uses signed int (int32_t to make sure it's 32bit in our case). I browsed around and found mixed use of signed and unsigned types in pepper code. It should be a good idea to unify sizes to be signed to comply to google/chromium guide. But I'd like to do that in another CL not to pollute this CL. Added a TODO here. Please let me know if this doesn't sound good to you.
http://codereview.chromium.org/10876014/diff/1006/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10876014/diff/1006/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:226: input_buffer.timestamp = encrypted_block_info.tracking_info.timestamp; Hello all, When I look at this code I hate myself :) The CdmWrapper need to do a lot of translation between PP_Foo and cdm::Foo types. But PP_Foo and cdm::Foo have so many in common. I wonder if we can increase code reuse and reduce code like above. I have some brainstorming to start the discussion: 1, make CDM interface use pepper types directly. pros: minimum struct translation. cons: introduce dependency on pepper to CMD. 2, have some python code (similar to generator.py) to convert PP_Foo to cdm::Foo automatically so that cdm::Foo is always in sync with PP_Foo and they always share the same memory layout. pros: We can just do memcpy() or reinterpret_cast. No conversion needed. cons: more work to do. 3, of course we can also keep the current way. Any thoughts?
Just a few things, including one case of inversed logic http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:230: On 2012/08/24 22:01:59, xhwang wrote: > On 2012/08/22 16:54:31, tomf wrote: > > A comment here that this returns a resource with a ref count of one would be > > good-- you can copy/paste the one in ppapi_plugin_instance.cc. > > Done. Where is it? http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:296: delete [] output_buffer.data; On 2012/08/24 22:01:59, xhwang wrote: > On 2012/08/22 18:25:54, ddorwin wrote: > > How do we guarantee output_buffer is not being used by another thread? > > I am not sure what's the problem? The output_buffer is created in the Decrypt() > call and will only be accessed here afterwards. That answers my question. Would be nice if we could make that clear, but I'm not sure how. > > .data = NULL since it's a parameter and could be accessed elsewhere. > > Done. http://codereview.chromium.org/10876014/diff/1006/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10876014/diff/1006/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:169: if (!key_ptr || key_size <= 0 || init_data_ptr || init_data_size <= 0) !init_data_ptr http://codereview.chromium.org/10876014/diff/1006/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:226: input_buffer.timestamp = encrypted_block_info.tracking_info.timestamp; On 2012/08/25 00:04:32, xhwang wrote: > Hello all, > > When I look at this code I hate myself :) > > The CdmWrapper need to do a lot of translation between PP_Foo and cdm::Foo > types. But PP_Foo and cdm::Foo have so many in common. I wonder if we can > increase code reuse and reduce code like above. > > I have some brainstorming to start the discussion: > > 1, make CDM interface use pepper types directly. > pros: minimum struct translation. > cons: introduce dependency on pepper to CMD. > 2, have some python code (similar to generator.py) to convert PP_Foo to cdm::Foo > automatically so that cdm::Foo is always in sync with PP_Foo and they always > share the same memory layout. > pros: We can just do memcpy() or reinterpret_cast. No conversion needed. > cons: more work to do. > 3, of course we can also keep the current way. > > Any thoughts? My thoughts: The point of CDM.h is to provide a (somewhat) stable API that is independent of other Chrome code. That API can roll at a different time from the associated Pepper APIs. If we were auto-generating the files, I don't think the above would be true. You can always move the above to a helper function and DCHECK the struct sizes to make sure the helpers are still complete. http://codereview.chromium.org/10876014/diff/1006/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10876014/diff/1006/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:36: // TODO(xhwang): Use int32_t instead of uint32_t for sizes here and below. and update checks to include <0.
http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10876014/diff/1/webkit/media/crypto/ppapi/cdm_... webkit/media/crypto/ppapi/cdm_wrapper.cc:230: On 2012/08/27 17:33:54, ddorwin wrote: > On 2012/08/24 22:01:59, xhwang wrote: > > On 2012/08/22 16:54:31, tomf wrote: > > > A comment here that this returns a resource with a ref count of one would be > > > good-- you can copy/paste the one in ppapi_plugin_instance.cc. > > > > Done. > > Where is it? http://codereview.chromium.org/10876014/diff/1006/webkit/media/crypto/ppapi/c... http://codereview.chromium.org/10876014/diff/1006/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10876014/diff/1006/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:169: if (!key_ptr || key_size <= 0 || init_data_ptr || init_data_size <= 0) On 2012/08/27 17:33:54, ddorwin wrote: > !init_data_ptr Done. http://codereview.chromium.org/10876014/diff/1006/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:226: input_buffer.timestamp = encrypted_block_info.tracking_info.timestamp; On 2012/08/27 17:33:54, ddorwin wrote: > On 2012/08/25 00:04:32, xhwang wrote: > > Hello all, > > > > When I look at this code I hate myself :) > > > > The CdmWrapper need to do a lot of translation between PP_Foo and cdm::Foo > > types. But PP_Foo and cdm::Foo have so many in common. I wonder if we can > > increase code reuse and reduce code like above. > > > > I have some brainstorming to start the discussion: > > > > 1, make CDM interface use pepper types directly. > > pros: minimum struct translation. > > cons: introduce dependency on pepper to CMD. > > 2, have some python code (similar to generator.py) to convert PP_Foo to > cdm::Foo > > automatically so that cdm::Foo is always in sync with PP_Foo and they always > > share the same memory layout. > > pros: We can just do memcpy() or reinterpret_cast. No conversion needed. > > cons: more work to do. > > 3, of course we can also keep the current way. > > > > Any thoughts? > > My thoughts: > The point of CDM.h is to provide a (somewhat) stable API that is independent of > other Chrome code. That API can roll at a different time from the associated > Pepper APIs. If we were auto-generating the files, I don't think the above would > be true. > > You can always move the above to a helper function and DCHECK the struct sizes > to make sure the helpers are still complete. Sounds good. I'll do the helper function later (tracked by the TODO on 207). http://codereview.chromium.org/10876014/diff/1006/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10876014/diff/1006/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:36: // TODO(xhwang): Use int32_t instead of uint32_t for sizes here and below. On 2012/08/27 17:33:54, ddorwin wrote: > and update checks to include <0. Done.
lgtm Thanks.
lgtm http://codereview.chromium.org/10876014/diff/7006/ppapi/api/private/ppp_conte... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10876014/diff/7006/ppapi/api/private/ppp_conte... ppapi/api/private/ppp_content_decryptor_private.idl:73: * successfully. <code>PP_FALSE</code> otherwise. Once the call reaches the nit: I would use a comma instead of a period after "successfully" throughout. http://codereview.chromium.org/10876014/diff/7006/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10876014/diff/7006/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:147: // TODO(xhwang): Remove unnecessary CallOnMain calls here and blow once we blow->below
http://codereview.chromium.org/10876014/diff/7006/ppapi/api/private/ppp_conte... File ppapi/api/private/ppp_content_decryptor_private.idl (right): http://codereview.chromium.org/10876014/diff/7006/ppapi/api/private/ppp_conte... ppapi/api/private/ppp_content_decryptor_private.idl:73: * successfully. <code>PP_FALSE</code> otherwise. Once the call reaches the On 2012/08/27 20:03:03, dmichael wrote: > nit: I would use a comma instead of a period after "successfully" throughout. Done. http://codereview.chromium.org/10876014/diff/7006/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10876014/diff/7006/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:147: // TODO(xhwang): Remove unnecessary CallOnMain calls here and blow once we On 2012/08/27 20:03:03, dmichael wrote: > blow->below Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/10876014/13001
Change committed as 153704 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
