Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23546014:
Plumb PPAPI PlatformVerification into CDM.h (Closed)
|
Created:
7 years, 3 months ago by DaleCurtis Modified:
7 years, 2 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, tinskip, Rintaro Kuroiwa, xhwang Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPlumb PPAPI PlatformVerification into CDM.h
BUG=270296
TEST=encrypted media browsertests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227436
R=ddorwin@chromium.org, dmichael@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227619
Patch Set 1 #
Total comments: 55
Patch Set 2 : Remove delayed requests. #
Total comments: 4
Patch Set 3 : Sanity checks. #
Total comments: 2
Patch Set 4 : Plumb HDCP, AudioFrames. Comments. #Patch Set 5 : Add DEPS roll. #
Total comments: 10
Patch Set 6 : Comments. #Patch Set 7 : Fix cast. #Patch Set 8 : Rebase. #
Messages
Total messages: 21 (0 generated)
ddorwin, xhwang: Please review w/ an eye for CDM mojo. dmichael: Please review PPAPI usage. Thanks!
Hopefully we can find an alternative to delaying GKR. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.cc File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:14: #include "media/cdm/ppapi/api/content_decryption_module.h" You need a DEPS roll in this CL. (Make sure it's backwards compatible.) https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:624: void CanChallengePlatformDone(int32_t result, bool can_challenge_platform); OnCan... ? https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:638: bool is_can_challenge_platform_set_; It's not clear from the name what this does. _initialized_? _pending_? In the latter case, it would be true in the constructor then cleared. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:641: // Since PPAPI doesn't provide handlers for CompleteCallbacks w/ more than one CompletionC... https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:642: // output we need to manage our own. These values are read only by nit: since this order could be ambiguous, swap read & only: only read https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:655: bool can_challenge_platform_; We don't really need this other than OS_CHROMEOS. It adds one more ifdef, but it's less confusing when grouped I think. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:673: platform_verification_.CanChallengePlatform( Re-entrancy from an Instance constructor seems like a bad idea. Maybe PostOnMain. That's also re-entrant, but at least I think that operates at a module level. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:714: delayed_gkr_ = callback_factory_.NewCallback( What if you get multiple GKRs at the same time? https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1242: &signed_data_signature_output_, &platform_key_certificate_output_, Why are these passed as pointers? Aren't all pp::Vars modifiable? https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1246: PlatformChallengeResponse pcr = {}; suggest: s/pcr/response/ https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1257: if (!delayed_gkr_.IsOptional()) ? Is the gkr ever optional? https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1263: cdm::PlatformChallengeResponse pcr = {}; same https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1270: std::string pkc_string = platform_key_certificate_output_.AsString(); nit: "pkc" is not descriptive https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1288: void CdmWrapper::DelayedGenerateKeyRequest(int32_t result, nit: The function name is a noun not an action https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1312: return static_cast<cdm::Host_1*>(static_cast<CdmWrapper*>(user_data)); tiny optional suggestion: It's an extra line but might be clearer to do the CdmWrapper cast on a separate line. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1316: return NULL; NOTREACHED()
https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.cc File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:624: void CanChallengePlatformDone(int32_t result, bool can_challenge_platform); On 2013/09/17 05:26:46, ddorwin wrote: > OnCan... ? Style elsewhere in the file is XxxDone; i.e., DecoderInitializeDone(), etc. I'm happy to change it if that's what you prefer though. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:673: platform_verification_.CanChallengePlatform( On 2013/09/17 05:26:46, ddorwin wrote: > Re-entrancy from an Instance constructor seems like a bad idea. Maybe > PostOnMain. That's also re-entrant, but at least I think that operates at a > module level. Hmm, it will end up as a post anyways when the IPC message is sent, but in an error state the completion callback might be called prior to returning. dmichael@ WDYT? https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:714: delayed_gkr_ = callback_factory_.NewCallback( On 2013/09/17 05:26:46, ddorwin wrote: > What if you get multiple GKRs at the same time? Ugh, I didn't realize that was a thing. To handle delay I'll need to use a list / deque of structs containing the initialization data. Note, this also unblocks the renderer thread when it returns, allowing other decoding tasks to start processing. In testing if the delay is too long the tests will fail since playback reaches > 1 second without a keyAdded event. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1242: &signed_data_signature_output_, &platform_key_certificate_output_, On 2013/09/17 05:26:46, ddorwin wrote: > Why are these passed as pointers? Aren't all pp::Vars modifiable? [out] type variables end up as Var* per the PPAPI generator, so I'm just going with the flow. dmichael@ can probably elaborate. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1257: if (!delayed_gkr_.IsOptional()) On 2013/09/17 05:26:46, ddorwin wrote: > ? Is the gkr ever optional? I'm just using it to check if the callback is uninitialized. In this case it's equivalent to is_null().
https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.cc File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:494: public cdm::Host_2 { Is this just temporary to deal with DEPS? If so, fine. Otherwise, I would rather have descriptive names :-) https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:645: // since the module could be destructed before callbacks complete... True? Can you point me to the comment you saw that indicates this? I don't *think* that can happen here. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:673: platform_verification_.CanChallengePlatform( On 2013/09/17 18:49:33, DaleCurtis wrote: > On 2013/09/17 05:26:46, ddorwin wrote: > > Re-entrancy from an Instance constructor seems like a bad idea. Maybe > > PostOnMain. That's also re-entrant, but at least I think that operates at a > > module level. > > Hmm, it will end up as a post anyways when the IPC message is sent, but in an > error state the completion callback might be called prior to returning. > dmichael@ WDYT? I'm not sure what re-entrancy you're worried about. The default completion callback is "required", meaning it will run asynchronously. (https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/pp_complet...) So CdmWrapper won't be re-entered. Re-entering in to Pepper is par for the course... we have to deal with it; it's just normal to call Pepper functions when Pepper calls you. So this looks fine to me. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1242: &signed_data_signature_output_, &platform_key_certificate_output_, On 2013/09/17 18:49:33, DaleCurtis wrote: > On 2013/09/17 05:26:46, ddorwin wrote: > > Why are these passed as pointers? Aren't all pp::Vars modifiable? > > [out] type variables end up as Var* per the PPAPI generator, so I'm just going > with the flow. dmichael@ can probably elaborate. ddorwin: I'm not sure what you mean... we try to follow Chromium/Google style as much as possible. So we use pointers for out-params, so we can modify the members. In this case, the Vars will be set to VarArrayBuffers. We have to be able to set the "type" and the "id" of the vars. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1269: pp::VarArrayBuffer signed_data_signature_var(signed_data_signature_output_); signed_data_output_ = pp::Var(); signed_data_signature_output_ = pp::Var(); ? https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1279: reinterpret_cast<const uint8_t*>(pkc_string.c_str()), static_cast? https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1291: pp::VarArrayBuffer init_data) { It's probably best to pass Var by const-reference, to avoid reference counting (which has to acquire a lock on both the AddRef and the Release).
(still just comments) https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.cc File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:494: public cdm::Host_2 { On 2013/09/17 20:10:46, dmichael wrote: > Is this just temporary to deal with DEPS? If so, fine. Otherwise, I would rather > have descriptive names :-) Temporary IIUC. Host_1 can be removed in favor of just Host once all clients are converted. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:645: // since the module could be destructed before callbacks complete... True? On 2013/09/17 20:10:46, dmichael wrote: > Can you point me to the comment you saw that indicates this? I don't *think* > that can happen here. https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/utility/comp... https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1279: reinterpret_cast<const uint8_t*>(pkc_string.c_str()), On 2013/09/17 20:10:46, dmichael wrote: > static_cast? Requires reinterpret_cast for char* -> uint8_t*
https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.cc File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:645: // since the module could be destructed before callbacks complete... True? On 2013/09/17 20:18:51, DaleCurtis wrote: > On 2013/09/17 20:10:46, dmichael wrote: > > Can you point me to the comment you saw that indicates this? I don't *think* > > that can happen here. > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/utility/comp... Ah, thanks. This stuff is confusing, and I still forget about that problem. Your case is a little different than what it's talking about. Your callbacks are scoped to platform_verification_, and when it gets destroyed, those will get aborted. Your Vars are scoped to the instance, so they get destroyed about the same time... and in the abort case, you don't write to the Vars. So you should be safe. It would be dangerous if you could get a "successful" callback invocation after the Vars are destroyed, but that doesn't seem possible here.
https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.cc File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:494: public cdm::Host_2 { On 2013/09/17 20:18:51, DaleCurtis wrote: > On 2013/09/17 20:10:46, dmichael wrote: > > Is this just temporary to deal with DEPS? If so, fine. Otherwise, I would > rather > > have descriptive names :-) > > Temporary IIUC. Host_1 can be removed in favor of just Host once all clients > are converted. Correct. Also, _2 is a version of the Host interface struct in our backwards-compatible API. It's not just type1 and type2. :) https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:673: platform_verification_.CanChallengePlatform( On 2013/09/17 20:10:46, dmichael wrote: > On 2013/09/17 18:49:33, DaleCurtis wrote: > > On 2013/09/17 05:26:46, ddorwin wrote: > > > Re-entrancy from an Instance constructor seems like a bad idea. Maybe > > > PostOnMain. That's also re-entrant, but at least I think that operates at a > > > module level. > > > > Hmm, it will end up as a post anyways when the IPC message is sent, but in an > > error state the completion callback might be called prior to returning. > > dmichael@ WDYT? > I'm not sure what re-entrancy you're worried about. > > The default completion callback is "required", meaning it will run > asynchronously. > (https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/pp_complet...) > > So CdmWrapper won't be re-entered. > > Re-entering in to Pepper is par for the course... we have to deal with it; it's > just normal to call Pepper functions when Pepper calls you. > > So this looks fine to me. I was just worried about re-entrancy when we're in the middle of creating a pp::Instance. CdmWrapper is a pp::Instance, and we're invoking a completely separate PPB interface from the constructor of the pp::Instance.
Changes I had to make to build. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.cc File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:653: cdm::ContentDecryptionModule* cdm_; When OS_CHROMEOS is defined, this will error at 1264. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:686: cdm_ = static_cast<cdm::ContentDecryptionModule*>( How are we going to make cdm_ be an instance of CDM_2? https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1279: reinterpret_cast<const uint8_t*>(pkc_string.c_str()), On 2013/09/17 20:18:51, DaleCurtis wrote: > On 2013/09/17 20:10:46, dmichael wrote: > > static_cast? > > Requires reinterpret_cast for char* -> uint8_t* Also need const_cast. The struct needs a non-const pointer.
https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.cc File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:638: bool is_can_challenge_platform_set_; On 2013/09/17 05:26:46, ddorwin wrote: > It's not clear from the name what this does. _initialized_? _pending_? In the > latter case, it would be true in the constructor then cleared. Done. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:641: // Since PPAPI doesn't provide handlers for CompleteCallbacks w/ more than one On 2013/09/17 05:26:46, ddorwin wrote: > CompletionC... Done. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:642: // output we need to manage our own. These values are read only by On 2013/09/17 05:26:46, ddorwin wrote: > nit: since this order could be ambiguous, swap read & only: only read Done. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:645: // since the module could be destructed before callbacks complete... True? On 2013/09/17 20:36:58, dmichael wrote: > On 2013/09/17 20:18:51, DaleCurtis wrote: > > On 2013/09/17 20:10:46, dmichael wrote: > > > Can you point me to the comment you saw that indicates this? I don't *think* > > > that can happen here. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/utility/comp... > Ah, thanks. This stuff is confusing, and I still forget about that problem. > > Your case is a little different than what it's talking about. Your callbacks are > scoped to platform_verification_, and when it gets destroyed, those will get > aborted. Your Vars are scoped to the instance, so they get destroyed about the > same time... and in the abort case, you don't write to the Vars. So you should > be safe. It would be dangerous if you could get a "successful" callback > invocation after the Vars are destroyed, but that doesn't seem possible here. Thanks for the explanation. Comment removed. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:653: cdm::ContentDecryptionModule* cdm_; On 2013/09/20 01:55:06, rkuroiwa wrote: > When OS_CHROMEOS is defined, this will error at 1264. Yeah I have another change to the CDM.h that switches over to v2 as the default. I'll get that uploaded separately. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:655: bool can_challenge_platform_; On 2013/09/17 05:26:46, ddorwin wrote: > We don't really need this other than OS_CHROMEOS. It adds one more ifdef, but > it's less confusing when grouped I think. Done. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:686: cdm_ = static_cast<cdm::ContentDecryptionModule*>( On 2013/09/20 01:55:06, rkuroiwa wrote: > How are we going to make cdm_ be an instance of CDM_2? It'll be the default. v1 CDM won't call into methods which require the v2 callbacks. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:714: delayed_gkr_ = callback_factory_.NewCallback( On 2013/09/17 18:49:33, DaleCurtis wrote: > On 2013/09/17 05:26:46, ddorwin wrote: > > What if you get multiple GKRs at the same time? > > Ugh, I didn't realize that was a thing. To handle delay I'll need to use a list > / deque of structs containing the initialization data. > > Note, this also unblocks the renderer thread when it returns, allowing other > decoding tasks to start processing. In testing if the delay is too long the > tests will fail since playback reaches > 1 second without a keyAdded event. Yeah, delay isn't going to work. I've instead changed it to err on the side of the most common case if the value isn't available for now... It's not great, but until we figure out whats happening with the Initialize() parameter, it's the best I can come up with. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1246: PlatformChallengeResponse pcr = {}; On 2013/09/17 05:26:46, ddorwin wrote: > suggest: s/pcr/response/ Done. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1257: if (!delayed_gkr_.IsOptional()) On 2013/09/17 18:49:33, DaleCurtis wrote: > On 2013/09/17 05:26:46, ddorwin wrote: > > ? Is the gkr ever optional? > > I'm just using it to check if the callback is uninitialized. In this case it's > equivalent to is_null(). Removed since now I can just store the key system. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1263: cdm::PlatformChallengeResponse pcr = {}; On 2013/09/17 05:26:46, ddorwin wrote: > same Done. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1269: pp::VarArrayBuffer signed_data_signature_var(signed_data_signature_output_); On 2013/09/17 20:10:46, dmichael wrote: > signed_data_output_ = pp::Var(); > signed_data_signature_output_ = pp::Var(); > ? Done. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1270: std::string pkc_string = platform_key_certificate_output_.AsString(); On 2013/09/17 05:26:46, ddorwin wrote: > nit: "pkc" is not descriptive Done. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1279: reinterpret_cast<const uint8_t*>(pkc_string.c_str()), On 2013/09/20 01:55:06, rkuroiwa wrote: > On 2013/09/17 20:18:51, DaleCurtis wrote: > > On 2013/09/17 20:10:46, dmichael wrote: > > > static_cast? > > > > Requires reinterpret_cast for char* -> uint8_t* > > Also need const_cast. The struct needs a non-const pointer. I have another patch which makes these fields const in CDM.h. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1288: void CdmWrapper::DelayedGenerateKeyRequest(int32_t result, On 2013/09/17 05:26:46, ddorwin wrote: > nit: The function name is a noun not an action Removed. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1291: pp::VarArrayBuffer init_data) { On 2013/09/17 20:10:46, dmichael wrote: > It's probably best to pass Var by const-reference, to avoid reference counting > (which has to acquire a lock on both the AddRef and the Release). Removed. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1312: return static_cast<cdm::Host_1*>(static_cast<CdmWrapper*>(user_data)); On 2013/09/17 05:26:46, ddorwin wrote: > tiny optional suggestion: It's an extra line but might be clearer to do the > CdmWrapper cast on a separate line. Done. https://codereview.chromium.org/23546014/diff/1/media/cdm/ppapi/cdm_wrapper.c... media/cdm/ppapi/cdm_wrapper.cc:1316: return NULL; On 2013/09/17 05:26:46, ddorwin wrote: > NOTREACHED() Done.
(lgtm from my standpoint)
LGTM % comments https://codereview.chromium.org/23546014/diff/9001/media/cdm/ppapi/cdm_wrappe... File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/9001/media/cdm/ppapi/cdm_wrappe... media/cdm/ppapi/cdm_wrapper.cc:1229: // Fall through on error and issue an empty OnPlatformChallengeResponse(). DCHECK != PP_OK? https://codereview.chromium.org/23546014/diff/9001/media/cdm/ppapi/cdm_wrappe... media/cdm/ppapi/cdm_wrapper.cc:1269: // Reset member variables for future ChallengePlatform() calls. Do we rely on the CDM to not call ChallengePlatform() when one is already pending? Should we DCHECK that?
https://codereview.chromium.org/23546014/diff/9001/media/cdm/ppapi/cdm_wrappe... File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/9001/media/cdm/ppapi/cdm_wrappe... media/cdm/ppapi/cdm_wrapper.cc:1229: // Fall through on error and issue an empty OnPlatformChallengeResponse(). On 2013/09/24 06:19:45, ddorwin wrote: > DCHECK != PP_OK? Done. https://codereview.chromium.org/23546014/diff/9001/media/cdm/ppapi/cdm_wrappe... media/cdm/ppapi/cdm_wrapper.cc:1269: // Reset member variables for future ChallengePlatform() calls. On 2013/09/24 06:19:45, ddorwin wrote: > Do we rely on the CDM to not call ChallengePlatform() when one is already > pending? Should we DCHECK that? Good idea. Done. I also realized its messy to account for all code paths unless this reset is done within ChallengePlatform(), so I've move it there.
lgtm https://codereview.chromium.org/23546014/diff/18001/media/cdm/ppapi/cdm_wrapp... File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/18001/media/cdm/ppapi/cdm_wrapp... media/cdm/ppapi/cdm_wrapper.cc:1220: // Ensure member variables are in a clean state. maybe (to be a bit clearer): ... variables set by the callback are...
ddorwin: PTAL for the DEPS roll, AudioFrames, and HDCP plumbing. https://codereview.chromium.org/23546014/diff/18001/media/cdm/ppapi/cdm_wrapp... File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/18001/media/cdm/ppapi/cdm_wrapp... media/cdm/ppapi/cdm_wrapper.cc:1220: // Ensure member variables are in a clean state. On 2013/09/24 23:53:51, ddorwin wrote: > maybe (to be a bit clearer): ... variables set by the callback are... Done.
https://codereview.chromium.org/23546014/diff/33001/media/cdm/ppapi/cdm_wrapp... File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/33001/media/cdm/ppapi/cdm_wrapp... media/cdm/ppapi/cdm_wrapper.cc:680: link_mask_(0), output_... ? https://codereview.chromium.org/23546014/diff/33001/media/cdm/ppapi/cdm_wrapp... media/cdm/ppapi/cdm_wrapper.cc:682: query_in_progress_(false), nit: query is not very specific. query_output_protection_...? https://codereview.chromium.org/23546014/diff/33001/media/cdm/ppapi/cdm_wrapp... media/cdm/ppapi/cdm_wrapper.cc:1242: // Ensure member variables set by the callback are in a clean state. Do we need to do the same for output protection? https://codereview.chromium.org/23546014/diff/33001/media/cdm/ppapi/cdm_wrapp... media/cdm/ppapi/cdm_wrapper.cc:1278: // TODO(dalecurtis): It'd be nice to log a message or non-fatal error here... Won't we always get PENDING since we're OOP? https://codereview.chromium.org/23546014/diff/33001/media/cdm/ppapi/cdm_wrapp... media/cdm/ppapi/cdm_wrapper.cc:1345: query_in_progress_ = false; Should we add DCHECK(query_in_progress_); ?
https://codereview.chromium.org/23546014/diff/33001/media/cdm/ppapi/cdm_wrapp... File media/cdm/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/23546014/diff/33001/media/cdm/ppapi/cdm_wrapp... media/cdm/ppapi/cdm_wrapper.cc:680: link_mask_(0), On 2013/10/05 21:39:36, ddorwin wrote: > output_... ? Done. https://codereview.chromium.org/23546014/diff/33001/media/cdm/ppapi/cdm_wrapp... media/cdm/ppapi/cdm_wrapper.cc:682: query_in_progress_(false), On 2013/10/05 21:39:36, ddorwin wrote: > nit: query is not very specific. query_output_protection_...? Done. https://codereview.chromium.org/23546014/diff/33001/media/cdm/ppapi/cdm_wrapp... media/cdm/ppapi/cdm_wrapper.cc:1242: // Ensure member variables set by the callback are in a clean state. On 2013/10/05 21:39:36, ddorwin wrote: > Do we need to do the same for output protection? No, not since the outputs are simple types. We needed it here since these are complex types that may leak. In either case, I do set them to zero prior to the call. https://codereview.chromium.org/23546014/diff/33001/media/cdm/ppapi/cdm_wrapp... media/cdm/ppapi/cdm_wrapper.cc:1278: // TODO(dalecurtis): It'd be nice to log a message or non-fatal error here... On 2013/10/05 21:39:36, ddorwin wrote: > Won't we always get PENDING since we're OOP? Not if there's an error that prevents the call from even occurring. Failing to reset the output variables to pp::Var() above would be an example of this. https://codereview.chromium.org/23546014/diff/33001/media/cdm/ppapi/cdm_wrapp... media/cdm/ppapi/cdm_wrapper.cc:1345: query_in_progress_ = false; On 2013/10/05 21:39:36, ddorwin wrote: > Should we add DCHECK(query_in_progress_); ? Done.
lgtm Thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/23546014/43001
Message was sent while issue was closed.
Change committed as 227436
Message was sent while issue was closed.
Committed patchset #8 manually as r227619 (presubmit successful).
Message was sent while issue was closed.
(new commit fixes cast issue temporarily per offline discussion with ddorwin, subsequent CL coming to fix incorrect usage of int32_t vs uint32_t where possible) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
