Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
393713003:
CdmAdapter: Allow parallel SendPlatformChallenge() calls. (Closed)
|
Created:
6 years, 5 months ago by xhwang Modified:
6 years, 5 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, yusukes+watch_chromium.org, tzik, binji+watch_chromium.org, raymes+watch_chromium.org, eme-reviews_chromium.org, teravest+watch_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, ihf+watch_chromium.org, ddorwin Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionCdmAdapter: Allow parallel SendPlatformChallenge() calls.
Previously CdmAdapter only allows one outstanding SendPlatformChallenge() call.
However, CDM actually needs to make parallel SendPlatformChallenge() calls. This
CL fixes CdmAdapter to allow this.
BUG=393722
TEST=Tested on ChromeOS build on Linux with hack in
PepperPlatformVerificationMessageFilter to return faked challenge platform reply.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283651
Patch Set 1 #
Total comments: 3
Patch Set 2 : Remove member variables. #
Total comments: 5
Patch Set 3 : rebase only #Patch Set 4 : comments addressed #Messages
Total messages: 23 (0 generated)
See bug for more background info. dalecurtis: PTAL dmichael: PTAL from pepper's perspective. I want you to help double check whether my understanding of the same API keeping using the same set of variables is correct. https://codereview.chromium.org/393713003/diff/1/media/cdm/ppapi/cdm_adapter.cc File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/393713003/diff/1/media/cdm/ppapi/cdm_adapter.... media/cdm/ppapi/cdm_adapter.cc:1171: platform_key_certificate_output_ = pp::Var(); Move this block here so that we keep these variable clean most of the time, except when we are in the stack of the challenge reply.
I think this would only work if sends and receives, specifically the point at which the variables are written into, are serialized onto the same thread. Assignment when a Map() is outstanding may be a race condition or undefined behavior otherwise. I thought I had something similar to this originally, but can't find any archived patch sets with comments on why I changed it to the current form.
On 2014/07/15 19:22:39, DaleCurtis wrote: > I think this would only work if sends and receives, specifically the point at > which the variables are written into, are serialized onto the same thread. > Assignment when a Map() is outstanding may be a race condition or undefined > behavior otherwise. > > I thought I had something similar to this originally, but can't find any > archived patch sets with comments on why I changed it to the current form. +yzshen Hmm, isn't SendPlatformChallengeDone() always called on the main thread? It should always be called in the stack of the OnChallengePlatformReply() which should be called on the main thread since we are not providing a |reply_thread_hint|. But I am no expect here and could be wrong. dmichael/yzshen could you please help check? https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/plugin... https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/platfo...
On 2014/07/15 20:18:42, xhwang wrote: > On 2014/07/15 19:22:39, DaleCurtis wrote: > > I think this would only work if sends and receives, specifically the point at > > which the variables are written into, are serialized onto the same thread. > > Assignment when a Map() is outstanding may be a race condition or undefined > > behavior otherwise. > > > > I thought I had something similar to this originally, but can't find any > > archived patch sets with comments on why I changed it to the current form. > > +yzshen > > Hmm, isn't SendPlatformChallengeDone() always called on the main thread? It > should always be called in the stack of the OnChallengePlatformReply() which > should be called on the main thread since we are not providing a > |reply_thread_hint|. But I am no expect here and could be wrong. > > dmichael/yzshen could you please help check? > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/plugin... > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/platfo... It depends on where you call ChallengePlatform. The completion callback (SendPlatformChallengeDone) will always be invoked on the same thread as where you called ChallengePlatform. But... I don't really see any thread synchronization stuff around SendPlatformChallengeDone, so it seems like you're already assuming it's always called on 1 thread? I don't know enough about your CDMAdapter stuff to know what you mean about calls on the stack and reply_thread_hint.
On 2014/07/15 20:58:47, dmichael wrote: > On 2014/07/15 20:18:42, xhwang wrote: > > On 2014/07/15 19:22:39, DaleCurtis wrote: > > > I think this would only work if sends and receives, specifically the point > at > > > which the variables are written into, are serialized onto the same thread. > > > Assignment when a Map() is outstanding may be a race condition or undefined > > > behavior otherwise. > > > > > > I thought I had something similar to this originally, but can't find any > > > archived patch sets with comments on why I changed it to the current form. > > > > +yzshen > > > > Hmm, isn't SendPlatformChallengeDone() always called on the main thread? It > > should always be called in the stack of the OnChallengePlatformReply() which > > should be called on the main thread since we are not providing a > > |reply_thread_hint|. But I am no expect here and could be wrong. > > > > dmichael/yzshen could you please help check? > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/plugin... > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/platfo... > It depends on where you call ChallengePlatform. The completion callback > (SendPlatformChallengeDone) will always be invoked on the same thread as where > you called ChallengePlatform. > > But... I don't really see any thread synchronization stuff around > SendPlatformChallengeDone, so it seems like you're already assuming it's always > called on 1 thread? I don't know enough about your CDMAdapter stuff to know what > you mean about calls on the stack and reply_thread_hint. The CdmAdapter and the CDM runs totally on one thread. That's why you don't see any thread synchronization code in CdmAdapter. So SendPlatformChallenge() will always be called on one thread and SendPlatformChallengeDone() will always be called on the same thread. Since SendPlatformChallengeDone() calls is serialized, I suppose this CL is still valid?
Alternate suggestion. But I'm happy to go with what you have if you and dalecurtis prefer it. https://codereview.chromium.org/393713003/diff/1/media/cdm/ppapi/cdm_adapter.cc File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/393713003/diff/1/media/cdm/ppapi/cdm_adapter.... media/cdm/ppapi/cdm_adapter.cc:1015: callback_factory_.NewCallback(&CdmAdapter::SendPlatformChallengeDone)); Do you know you can bind parameters with the callback factory? E.g., I think you could in an unnamed namespace define something like: struct PlatformChallengeResponse { pp::Var signed_data_output; pp::Var signed_data_signature_output; pp::Var key_signature_output; }; Then say: PlatformChallengeResponse* response = new PlatformChallengeResponse(); callback_factory_.NewCallback(&CdmAdapter::SendPlatformChallengeDone, response); Then you might have: void CdmAdapter::SendPlatformChallengeDone( int32_t result, PlatformChallengeResponse* response) { // Do stuff with response delete response; } You could eliminate those member variables, and I think it would just look more obviously correct and more similar to how we would do this in Chromium. WDYT?
https://codereview.chromium.org/393713003/diff/1/media/cdm/ppapi/cdm_adapter.cc File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/393713003/diff/1/media/cdm/ppapi/cdm_adapter.... media/cdm/ppapi/cdm_adapter.cc:1015: callback_factory_.NewCallback(&CdmAdapter::SendPlatformChallengeDone)); On 2014/07/15 21:59:52, dmichael wrote: > Do you know you can bind parameters with the callback factory? > > E.g., I think you could in an unnamed namespace define something like: > struct PlatformChallengeResponse { > pp::Var signed_data_output; > pp::Var signed_data_signature_output; > pp::Var key_signature_output; > }; > Then say: > PlatformChallengeResponse* response = new PlatformChallengeResponse(); > callback_factory_.NewCallback(&CdmAdapter::SendPlatformChallengeDone, > response); > > Then you might have: > void CdmAdapter::SendPlatformChallengeDone( > int32_t result, PlatformChallengeResponse* response) { > // Do stuff with response > delete response; > } > > You could eliminate those member variables, and I think it would just look more > obviously correct and more similar to how we would do this in Chromium. > > WDYT? That would be awesome if it works now. IIRC, at the time of writing I wasn't able to do this for some reason that I can't remember.
On 2014/07/15 21:59:52, dmichael wrote: > Alternate suggestion. But I'm happy to go with what you have if you and > dalecurtis prefer it. > > https://codereview.chromium.org/393713003/diff/1/media/cdm/ppapi/cdm_adapter.cc > File media/cdm/ppapi/cdm_adapter.cc (right): > > https://codereview.chromium.org/393713003/diff/1/media/cdm/ppapi/cdm_adapter.... > media/cdm/ppapi/cdm_adapter.cc:1015: > callback_factory_.NewCallback(&CdmAdapter::SendPlatformChallengeDone)); > Do you know you can bind parameters with the callback factory? > > E.g., I think you could in an unnamed namespace define something like: > struct PlatformChallengeResponse { > pp::Var signed_data_output; > pp::Var signed_data_signature_output; > pp::Var key_signature_output; > }; > Then say: > PlatformChallengeResponse* response = new PlatformChallengeResponse(); > callback_factory_.NewCallback(&CdmAdapter::SendPlatformChallengeDone, > response); > > Then you might have: > void CdmAdapter::SendPlatformChallengeDone( > int32_t result, PlatformChallengeResponse* response) { > // Do stuff with response > delete response; > } > > You could eliminate those member variables, and I think it would just look more > obviously correct and more similar to how we would do this in Chromium. > > WDYT? Your suggestions make sense to me. I don't like the member variables either. But I thought that's the only way to make it work based on some existing comments. I'll definitely experiment with what you suggested to see how it works. Thanks!
dmichael's trick is working! Also I used link_ptr so that I don't have to manually delete the response. PTAL again! https://codereview.chromium.org/393713003/diff/20001/media/cdm/ppapi/cdm_adap... File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/393713003/diff/20001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.cc:1032: cdm::PlatformChallengeResponse platform_challenge_response = {}; rename because we have a different "response" on l.1013.
https://codereview.chromium.org/393713003/diff/20001/media/cdm/ppapi/cdm_adap... File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/393713003/diff/20001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.cc:1173: cdm_->OnPlatformChallengeResponse(platform_challenge_response); One thing I just noticed... OnPlatformChallengeResponse here has to make use of those buffers immediately, because they will be deleted when this function (SendPlatformChallengeDone) goes out of scope. That seems correct & appropriate from an API standpoint, but I wanted to point it out, because it looks like a behavior change from prior to this CL, so I want to make sure it works with your CDM code. https://codereview.chromium.org/393713003/diff/20001/media/cdm/ppapi/cdm_adap... File media/cdm/ppapi/cdm_adapter.h (right): https://codereview.chromium.org/393713003/diff/20001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:268: pp::Var platform_key_certificate_output_; Can you remove these now?
rebase only
comments addressed
PTAL again https://codereview.chromium.org/393713003/diff/20001/media/cdm/ppapi/cdm_adap... File media/cdm/ppapi/cdm_adapter.cc (right): https://codereview.chromium.org/393713003/diff/20001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.cc:1173: cdm_->OnPlatformChallengeResponse(platform_challenge_response); On 2014/07/16 15:21:32, dmichael wrote: > One thing I just noticed... OnPlatformChallengeResponse here has to make use of > those buffers immediately, because they will be deleted when this function > (SendPlatformChallengeDone) goes out of scope. That seems correct & appropriate > from an API standpoint, but I wanted to point it out, because it looks like a > behavior change from prior to this CL, so I want to make sure it works with your > CDM code. Thanks for pointing this out. In all CDM interface calls, the pointer is only guaranteed to be valid in the scope of that call. CDM has to use it immediately or copy it for future use. So this should be fine. https://codereview.chromium.org/393713003/diff/20001/media/cdm/ppapi/cdm_adap... File media/cdm/ppapi/cdm_adapter.h (right): https://codereview.chromium.org/393713003/diff/20001/media/cdm/ppapi/cdm_adap... media/cdm/ppapi/cdm_adapter.h:268: pp::Var platform_key_certificate_output_; On 2014/07/16 15:21:33, dmichael wrote: > Can you remove these now? oops, I thought I did. Done now.
lgtm
lgtm
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/393713003/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/393713003/60001
Message was sent while issue was closed.
Change committed as 283651 |
