Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10837252:
Update CDM interface and add clear key CDM. (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. |
DescriptionUpdate CDM interface and add clear key CDM.
The ClearKeyCdm implements the ContentDecryptionModule. It's a wrapping of the media::AesDecryptor to do real decryption work. The wrapping layer only translates input/output parameters and convert the async calls in media::AesDecryptor to sync calls.
TBR=scherkus@chromium.org
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152672
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153566
Patch Set 1 #Patch Set 2 : #
Total comments: 34
Patch Set 3 : Resolve comments. #
Total comments: 16
Patch Set 4 : Resolve ddorwin's comments. #
Total comments: 4
Patch Set 5 : Update CDM interface and clear key CDM. #
Total comments: 12
Patch Set 6 : Removed "#if defined(COMPONENT_BUILD)" #Patch Set 7 : Change KeyRequest to KeyMessage to better reflect it's type. #Patch Set 8 : Move clear_key_cdm.* into ppapi/ #Patch Set 9 : Workaround xcode build failure on mac. #Patch Set 10 : Remove tabs from .gypi file. #Patch Set 11 : Merge "ppapi_cdm_wrapper" target into "clearkeycdmplugin". #
Messages
Total messages: 27 (0 generated)
Hello ddorwin & tomf, This CL implements the clear key CDM. It compiles but I haven't tested it yet. Will test it after I rebase http://codereview.chromium.org/10836038/. PTAL! xhwang
+scherkus for reviewing.
http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.cc (right): http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:140: ScopedResetter<Client> _(&client_); How about s/_(/resetter(/? :) http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:141: decryptor_.GenerateKeyRequest("", init_data, init_data_size); Don't we have a placeholder key system name we can use? The wrapper (or the underlying module) won't be happy w/this. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:153: nit: can probably remove this empty line http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:161: ScopedResetter<Client> _(&client_); Ditto on all of these. :) http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:207: void ClearKeyCdm::OnBufferDecrpted( s/Decrpted/Decrypted/ http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.h (right): http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:37: int* default_url_size) OVERRIDE; Do we want to wrap all of these up in a struct? http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:45: int session_id_size) OVERRIDE; Is the size param necessary? I think we can probably guarantee that the session ID is NUL terminated. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:48: int session_id_size, Ditto. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:100: void OnBufferDecrpted(media::Decryptor::Status status, s/Decrpted/Decrypted/
Partial review, so you can see it before I get to the rest. I'm also concerned about the Client management, but I didn't get a chance to get into the details. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.cc (right): http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:57: Type* object_; *const http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:127: void ClearKeyCdm::Reset() { This doesn't reset the whole class, so I think this is inappropriate. What you really need is OnScopeExit, which does what you want but for a random function. I'm not sure if Chrome has something like this. If not, maybe you can change your class to take a function pointer. However, if Chrome doesn't have something like this, there may be a reason for that. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.h (right): http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:26: // Reset the |decryption_status_| and |decrypted_buffer|. Why would we need to do this? Who is calling it if it's not part of the inherited interface? (I see now, but it's weird.) http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:49: const InputBuffer& encrypted_buffer, I now realize that InputBuffer is in the global namespace. Maybe we should call it CdmInputBuffer or something to avoid conflicts. Thoughts? http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:105: media::Decryptor::Status decryption_status_; I wonder why these two are members instead of transient. We should not have any state related to decryption or we will not be able to parallelize, for example, audio and video. http://codereview.chromium.org/10837252/diff/2001/webkit/media/webkit_media.gypi File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10837252/diff/2001/webkit/media/webkit_media.g... webkit/media/webkit_media.gypi:82: 'target_name': 'clear_key_cdm', should this use underscores or be concatenated? The chrome/ directory is inconsistent. On Linux, will this start with "lib". I think it should, and we should be consistent with other lib* .so's. http://codereview.chromium.org/10837252/diff/2001/webkit/media/webkit_media.g... webkit/media/webkit_media.gypi:85: '<(DEPTH)/base/base.gyp:base' + webkit_media and media?
http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.cc (right): http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:141: decryptor_.GenerateKeyRequest("", init_data, init_data_size); On 2012/08/15 20:02:42, tomf wrote: > Don't we have a placeholder key system name we can use? The wrapper (or the > underlying module) won't be happy w/this. Err- disregard this, meant to remove it.
I'm not sure how we can go async -> sync -> async without an ID for each call. Maybe we can't use AesDecryptor as is. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.cc (right): http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:121: : decryptor_(&client_), Re-using the client means we cannot parallelize any of these calls. I don't think Chrome is preventing this, so we probably can't do this. It seems fine, especially for a test plugin, to just create a new client per call. Unfortunately, AesDecryptor has Client is a member, so we can't do that. One option is to collect callbacks in the Client then handle them all when a synchronous call to AesDecryptor returns. That would work if this class was asynchronous and we could just call callbacks, but it's not. Without an additional identifier for each call, we can't pick the correct response (i.e. which frame was decoded or the status from this call).
Comments resolved. PTAL! http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.cc (right): http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:57: Type* object_; On 2012/08/15 20:15:00, ddorwin wrote: > *const Done. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:121: : decryptor_(&client_), On 2012/08/15 21:03:07, ddorwin wrote: > Re-using the client means we cannot parallelize any of these calls. I don't > think Chrome is preventing this, so we probably can't do this. It seems fine, > especially for a test plugin, to just create a new client per call. > Unfortunately, AesDecryptor has Client is a member, so we can't do that. > > One option is to collect callbacks in the Client then handle them all when a > synchronous call to AesDecryptor returns. That would work if this class was > asynchronous and we could just call callbacks, but it's not. Without an > additional identifier for each call, we can't pick the correct response (i.e. > which frame was decoded or the status from this call). Yes, we cannot parallelize these calls. Added comment before the declaration of ClearKeyCdm. Need to figure this out later. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:127: void ClearKeyCdm::Reset() { On 2012/08/15 20:15:00, ddorwin wrote: > This doesn't reset the whole class, so I think this is inappropriate. What you > really need is OnScopeExit, which does what you want but for a random function. > I'm not sure if Chrome has something like this. If not, maybe you can change > your class to take a function pointer. However, if Chrome doesn't have something > like this, there may be a reason for that. Removed this method and made decryption_status_ and decrypted_buffer_ transient. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:140: ScopedResetter<Client> _(&client_); On 2012/08/15 20:02:42, tomf wrote: > How about s/_(/resetter(/? :) Replaced with auto_resetter. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:153: On 2012/08/15 20:02:42, tomf wrote: > nit: can probably remove this empty line Done. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:161: ScopedResetter<Client> _(&client_); On 2012/08/15 20:02:42, tomf wrote: > Ditto on all of these. :) Done. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:207: void ClearKeyCdm::OnBufferDecrpted( On 2012/08/15 20:02:42, tomf wrote: > s/Decrpted/Decrypted/ This function is removed, but thanks for catching this :) http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.h (right): http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:37: int* default_url_size) OVERRIDE; On 2012/08/15 20:02:42, tomf wrote: > Do we want to wrap all of these up in a struct? This probably will change as the spec changes. I'd keep it as is for now. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:45: int session_id_size) OVERRIDE; On 2012/08/15 20:02:42, tomf wrote: > Is the size param necessary? I think we can probably guarantee that the session > ID is NUL terminated. Good point. I use size here to be consistent with other code. As mentioned earlier, the spec may change, so I'll just keep it as is. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:49: const InputBuffer& encrypted_buffer, On 2012/08/15 20:15:00, ddorwin wrote: > I now realize that InputBuffer is in the global namespace. Maybe we should call > it CdmInputBuffer or something to avoid conflicts. Thoughts? Put it in cdm namespace. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:100: void OnBufferDecrpted(media::Decryptor::Status status, On 2012/08/15 20:02:42, tomf wrote: > s/Decrpted/Decrypted/ Done. http://codereview.chromium.org/10837252/diff/2001/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:105: media::Decryptor::Status decryption_status_; On 2012/08/15 20:15:00, ddorwin wrote: > I wonder why these two are members instead of transient. We should not have any > state related to decryption or we will not be able to parallelize, for example, > audio and video. Done. http://codereview.chromium.org/10837252/diff/2001/webkit/media/webkit_media.gypi File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10837252/diff/2001/webkit/media/webkit_media.g... webkit/media/webkit_media.gypi:82: 'target_name': 'clear_key_cdm', On 2012/08/15 20:15:00, ddorwin wrote: > should this use underscores or be concatenated? The chrome/ directory is > inconsistent. On Linux, will this start with "lib". I think it should, and we > should be consistent with other lib* .so's. On line 8, the target name for webkit_media is 'webkit_media', so I assume underscore is okay. The output file name on linux is libclear_key_cdm.so, which is at least compatible with other lib* names in src/out/Debug/lib/. I checked /usr/lib64 and found there are many styles of names so I am not sure if there's a standard linux naming style for libs. http://codereview.chromium.org/10837252/diff/2001/webkit/media/webkit_media.g... webkit/media/webkit_media.gypi:85: '<(DEPTH)/base/base.gyp:base' On 2012/08/15 20:15:00, ddorwin wrote: > + webkit_media and media? Added media, this shouldn't depend on webkit_media.
http://codereview.chromium.org/10837252/diff/2001/webkit/media/webkit_media.gypi File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10837252/diff/2001/webkit/media/webkit_media.g... webkit/media/webkit_media.gypi:82: 'target_name': 'clear_key_cdm', On 2012/08/16 02:32:40, xhwang wrote: > On 2012/08/15 20:15:00, ddorwin wrote: > > should this use underscores or be concatenated? The chrome/ directory is > > inconsistent. On Linux, will this start with "lib". I think it should, and we > > should be consistent with other lib* .so's. > > On line 8, the target name for webkit_media is 'webkit_media', so I assume > underscore is okay. The output file name on linux is libclear_key_cdm.so, which > is at least compatible with other lib* names in src/out/Debug/lib/. I checked > /usr/lib64 and found there are many styles of names so I am not sure if there's > a standard linux naming style for libs. webkit_media is a static library that we happen to make dynamic in shared object builds, which we don't ship. IOW, I don't think that's a good example. Also, this name is used for all OSes, so Linux naming is not necessarily a good guide. I would follow other Pepper .so's, which are libfoobarthing.so. http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.cc (right): http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:22: for (unsigned int i = 0; i < input_buffer.num_subsamples; ++i) { unit32_t. No reason to mix type synonyms and risks problems later. http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:30: std::string(reinterpret_cast<const char*>(input_buffer.key_id), Can we avoid a copy with StringPiece? http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:123: NOTREACHED(); Comment why. TODO? http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:139: decryptor_.GenerateKeyRequest("", init_data, init_data_size); What happened to the key system? We should be passing it in to the CDM when we create the session. http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:159: decryptor_.AddKey("", key, key_size, NULL, 0, How are you associating the key with the key ID? (This will change in the future, but v0.1 Clear Key relies on this parameter I think.) http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:190: // Callback is called synchronously, so we can use variables on the stack. http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.h (right): http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:24: // This implementation assumes that all method calls are made subsequently. subsequently doesn't seem right. Were you thinking sequentially? That doesn't seem right either. "This class is not thread-safe."? http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:25: // Simultaneous method calls are not supported. Is this still accurate? Is it just the session management calls that are not thread safe? It looks like Decrypt is probably thread-safe now assuming AesDecryptor is (for key access).
http://codereview.chromium.org/10837252/diff/2001/webkit/media/webkit_media.gypi File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10837252/diff/2001/webkit/media/webkit_media.g... webkit/media/webkit_media.gypi:82: 'target_name': 'clear_key_cdm', On 2012/08/16 05:00:35, ddorwin wrote: > On 2012/08/16 02:32:40, xhwang wrote: > > On 2012/08/15 20:15:00, ddorwin wrote: > > > should this use underscores or be concatenated? The chrome/ directory is > > > inconsistent. On Linux, will this start with "lib". I think it should, and > we > > > should be consistent with other lib* .so's. > > > > On line 8, the target name for webkit_media is 'webkit_media', so I assume > > underscore is okay. The output file name on linux is libclear_key_cdm.so, > which > > is at least compatible with other lib* names in src/out/Debug/lib/. I checked > > /usr/lib64 and found there are many styles of names so I am not sure if > there's > > a standard linux naming style for libs. > > webkit_media is a static library that we happen to make dynamic in shared object > builds, which we don't ship. IOW, I don't think that's a good example. Also, > this name is used for all OSes, so Linux naming is not necessarily a good guide. > I would follow other Pepper .so's, which are libfoobarthing.so. Done. http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.cc (right): http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:22: for (unsigned int i = 0; i < input_buffer.num_subsamples; ++i) { On 2012/08/16 05:00:35, ddorwin wrote: > unit32_t. No reason to mix type synonyms and risks problems later. Done. http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:30: std::string(reinterpret_cast<const char*>(input_buffer.key_id), On 2012/08/16 05:00:35, ddorwin wrote: > Can we avoid a copy with StringPiece? DecryptConfig's ctor takes std::string. I think the original argument is that key_id, iv, etc are all small data, so we don't really care that much. http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:123: NOTREACHED(); On 2012/08/16 05:00:35, ddorwin wrote: > Comment why. TODO? Done. http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:139: decryptor_.GenerateKeyRequest("", init_data, init_data_size); On 2012/08/16 05:00:35, ddorwin wrote: > What happened to the key system? We should be passing it in to the CDM when we > create the session. Key system is needed here if one CDM needs to support multiple key system. But this is not required right now and details TBD. As per offline discussion, I'll leave this as is for now. http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:159: decryptor_.AddKey("", key, key_size, NULL, 0, On 2012/08/16 05:00:35, ddorwin wrote: > How are you associating the key with the key ID? (This will change in the > future, but v0.1 Clear Key relies on this parameter I think.) Done. http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:190: On 2012/08/16 05:00:35, ddorwin wrote: > // Callback is called synchronously, so we can use variables on the stack. Done. http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.h (right): http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:24: // This implementation assumes that all method calls are made subsequently. On 2012/08/16 05:00:35, ddorwin wrote: > subsequently doesn't seem right. Were you thinking sequentially? That doesn't > seem right either. > "This class is not thread-safe."? I believe it's thread-safe now. Remove this comment. But yes, I meant sequentially :) http://codereview.chromium.org/10837252/diff/3005/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:25: // Simultaneous method calls are not supported. On 2012/08/16 05:00:35, ddorwin wrote: > Is this still accurate? Is it just the session management calls that are not > thread safe? It looks like Decrypt is probably thread-safe now assuming > AesDecryptor is (for key access). It should be thread safe now.
lgtm Yay for thread-safe! http://codereview.chromium.org/10837252/diff/4004/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.cc (right): http://codereview.chromium.org/10837252/diff/4004/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:171: // Client methods are called synchronously, so we can use client variables Not sure what this is trying to say (that isn't already obvious from acquiring client_lock_. http://codereview.chromium.org/10837252/diff/4004/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.h (right): http://codereview.chromium.org/10837252/diff/4004/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:106: base::Lock lock_; client_lock_?
Updated the CDM interface a little bit more and updated the clear key CDM accordingly. Before landing, I'll move the clear_key_cdm.* into webkit/media/crypto/ppapi/ folder. PTAL! http://codereview.chromium.org/10837252/diff/4004/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.cc (right): http://codereview.chromium.org/10837252/diff/4004/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.cc:171: // Client methods are called synchronously, so we can use client variables On 2012/08/16 17:36:50, ddorwin wrote: > Not sure what this is trying to say (that isn't already obvious from acquiring > client_lock_. Done. http://codereview.chromium.org/10837252/diff/4004/webkit/media/crypto/clear_k... File webkit/media/crypto/clear_key_cdm.h (right): http://codereview.chromium.org/10837252/diff/4004/webkit/media/crypto/clear_k... webkit/media/crypto/clear_key_cdm.h:106: base::Lock lock_; On 2012/08/16 17:36:50, ddorwin wrote: > client_lock_? Done. http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:23: CDM_EXPORT cdm::ContentDecryptionModule* CreateCdmInstance(); This is to simulate what's in MEDIA_EXPORT, or CONTENT_EXPORT, the chromium way to make symbols visible. http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:30: enum Status { Move it here so that the caller (e.g. CdmWrapper) doesn't need to use the long name like cdm::ContentDecryptionModule::Status. http://codereview.chromium.org/10837252/diff/12002/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10837252/diff/12002/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:110: 'target_name': 'clearkeycdmplugin', How do you like this name? Doesn't seem perfect for me but I can't find a better one now.
Not sure about the component build thing. Otherwise, LG. http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_export.h (right): http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_export.h:11: #if defined(COMPONENT_BUILD) Not sure this should be here. See comment in CDM.h. http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_export.h:20: #else // defined(WIN32) !defined ? http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_export.h:28: #else // defined(COMPONENT_BUILD) same http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:23: CDM_EXPORT cdm::ContentDecryptionModule* CreateCdmInstance(); On 2012/08/20 21:09:45, xhwang wrote: > This is to simulate what's in MEDIA_EXPORT, or CONTENT_EXPORT, the chromium way > to make symbols visible. I don't think its definition should be based on defined(COMPONENT_BUILD) then. We always want to export these because this is always a .so/.dll. http://codereview.chromium.org/10837252/diff/12002/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10837252/diff/12002/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:110: 'target_name': 'clearkeycdmplugin', On 2012/08/20 21:09:45, xhwang wrote: > How do you like this name? Doesn't seem perfect for me but I can't find a better > one now. Fine.
Removed "#if defined(COMPONENT_BUILD)" and tested on both chromium static build and shared build. PTAL! http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_export.h (right): http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_export.h:11: #if defined(COMPONENT_BUILD) On 2012/08/20 21:27:07, ddorwin wrote: > Not sure this should be here. See comment in CDM.h. Done. http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_export.h:20: #else // defined(WIN32) On 2012/08/20 21:27:07, ddorwin wrote: > !defined ? This comment is saying "this '#else' corresponds to the above 'defined(WIN32)'". It's ambiguous but it seems to be the style in chromium: http://code.google.com/searchframe#OAMlx_jo-ck/src/content/common/content_exp... http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_export.h:28: #else // defined(COMPONENT_BUILD) On 2012/08/20 21:27:07, ddorwin wrote: > same Done. http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10837252/diff/12002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:23: CDM_EXPORT cdm::ContentDecryptionModule* CreateCdmInstance(); On 2012/08/20 21:27:07, ddorwin wrote: > On 2012/08/20 21:09:45, xhwang wrote: > > This is to simulate what's in MEDIA_EXPORT, or CONTENT_EXPORT, the chromium > way > > to make symbols visible. > > I don't think its definition should be based on defined(COMPONENT_BUILD) then. > We always want to export these because this is always a .so/.dll. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/10837252/2003
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/10837252/11003
Try job failure for 10837252-11003 (retry) on mac_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/10837252/11003
Change committed as 152672
thakis@: Please take a look at the .gypi file. Thanks! xhwang
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/10837252/19001
Try job failure for 10837252-19001 (retry) on mac for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
On 2012/08/24 18:26:52, xhwang wrote: > thakis@: > > Please take a look at the .gypi file. Thanks! From the parallel email thread (why have both?): """It looks like clearkeycdmplugin doesn't have any 'sources' section after your change, so I suppose xcode doesn't bother to create a binary file at all. (It's a bug that ninja/mac doesn't do the same, it should always match xcode's behavior.) build/common.gypi assumes that it's always save to run strip_from_xcode on loadable_modules. I think we have a similar module somewhere…yes, in chrome/app/policy/policy_templates.gypi (look for chrome_manifest_bundle). It looks the way they work around this is by setting 'mac_strip': 0, which looks like an acceptable workaround to prevent strip_from_xcode from running. I'm a bit surprised that you don't run into this with ninja though. Now that I look, we have a test that makes sure that ninja behaves like xcode here (tools/gyp/test/mac/sourceless-module/test.gyp) -- I think you'll see the same issue with ninja if you do a clobber build, and the old executable just happens to be still around. It's arguably a ninja bug that we don't delete that executable.""" It looks like this got landed without this discussion being resolved first :-/
On 2012/08/26 22:14:14, Nico wrote: > On 2012/08/24 18:26:52, xhwang wrote: > > thakis@: > > > > Please take a look at the .gypi file. Thanks! > > From the parallel email thread (why have both?): """It looks like > clearkeycdmplugin doesn't have any 'sources' section > after your change, so I suppose xcode doesn't bother to create a > binary file at all. (It's a bug that ninja/mac doesn't do the same, it > should always match xcode's behavior.) > > build/common.gypi assumes that it's always save to run > strip_from_xcode on loadable_modules. > > I think we have a similar module somewhere…yes, in > chrome/app/policy/policy_templates.gypi (look for > chrome_manifest_bundle). It looks the way they work around this is by > setting 'mac_strip': 0, which looks like an acceptable workaround to > prevent strip_from_xcode from running. > > I'm a bit surprised that you don't run into this with ninja though. > Now that I look, we have a test that makes sure that ninja behaves > like xcode here (tools/gyp/test/mac/sourceless-module/test.gyp) -- I > think you'll see the same issue with ninja if you do a clobber build, > and the old executable just happens to be still around. It's arguably > a ninja bug that we don't delete that executable.""" > > It looks like this got landed without this discussion being resolved first :-/ Hello Nico: Sorry for the confusion. My fault creating two threads (email and here) for discussion (lesson learnt). After discussion with ddorwin@, we agreed to merge the "ppapi_cdm_wrapper" target into "clearkeycdmplugin" and include source files in "clearkeycdmplugin" directly (as done in patch set 11). This way we won't have the tricky "no-source-file-target" at all and the gypi file actually gets simpler. So now we don't need the "'mac_strip': 0" trick, but thanks a lot for your insights! Best, xhwang
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xhwang@chromium.org/10837252/22004
On Mon, Aug 27, 2012 at 11:52 AM, <xhwang@chromium.org> wrote: > On 2012/08/26 22:14:14, Nico wrote: > >> On 2012/08/24 18:26:52, xhwang wrote: >> > thakis@: >> > >> > Please take a look at the .gypi file. Thanks! >> > > From the parallel email thread (why have both?): """It looks like >> clearkeycdmplugin doesn't have any 'sources' section >> after your change, so I suppose xcode doesn't bother to create a >> binary file at all. (It's a bug that ninja/mac doesn't do the same, it >> should always match xcode's behavior.) >> > > build/common.gypi assumes that it's always save to run >> strip_from_xcode on loadable_modules. >> > > I think we have a similar module somewhere…yes, in >> chrome/app/policy/policy_**templates.gypi (look for >> chrome_manifest_bundle). It looks the way they work around this is by >> setting 'mac_strip': 0, which looks like an acceptable workaround to >> prevent strip_from_xcode from running. >> > > I'm a bit surprised that you don't run into this with ninja though. >> Now that I look, we have a test that makes sure that ninja behaves >> like xcode here (tools/gyp/test/mac/**sourceless-module/test.gyp) -- I >> think you'll see the same issue with ninja if you do a clobber build, >> and the old executable just happens to be still around. It's arguably >> a ninja bug that we don't delete that executable.""" >> > > It looks like this got landed without this discussion being resolved >> first :-/ >> > > Hello Nico: > > Sorry for the confusion. My fault creating two threads (email and here) for > discussion (lesson learnt). > > After discussion with ddorwin@, we agreed to merge the "ppapi_cdm_wrapper" > target into "clearkeycdmplugin" and include source files in > "clearkeycdmplugin" > directly (as done in patch set 11). Ok, thanks for following up :-)
Change committed as 153566 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
