Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10914028:
Add CDM allocator interface. (Closed)
|
Created:
8 years, 3 months ago by Tom Finegan Modified:
8 years, 3 months ago Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd CDM allocator interface.
This allows CDMs to allocate shared memory while hiding the
ppapi implementation details from the CDM. It also removes
one extra memcpy from the Decrypt path.
BUG=138139
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157406
Patch Set 1 #Patch Set 2 : It works, and this removes a copy in Decrypt/DeliverBlock. #
Total comments: 33
Patch Set 3 : Some iteratative work plus work related to xhwang's comments. #
Total comments: 51
Patch Set 4 : Rebased. No other changes. #Patch Set 5 : Refactored... quite a bit. Sorry for rebase noise on top of everything else! #
Total comments: 64
Patch Set 6 : Iteration per comments #Patch Set 7 : Fixed compile errors, and changed ReleaseBuffer to return void. #Patch Set 8 : Fix gyp file (remove cdm_allocator.h). #
Total comments: 8
Patch Set 9 : Address comments. #
Total comments: 19
Patch Set 10 : Rebased (sorry! wfh tonight...) #Patch Set 11 : Address comments. #Patch Set 12 : Fix OutputBuffer. #Patch Set 13 : Fix double delete, and possible KeyMessage leak. #
Total comments: 63
Patch Set 14 : Rebase to pick up fix for crash in Decrypt on Windows #Patch Set 15 : Make KeyMessage and OutputBuffer interfaces... #
Total comments: 27
Patch Set 16 : Use linked_ptr from base instead of tr1::shared_ptr. #
Total comments: 4
Patch Set 17 : Replace Allocator::Release with Buffer::Destroy. #Patch Set 18 : Fix compile error... not sure what code MSVC was using... ;) #
Total comments: 8
Patch Set 19 : Fix compile error. #Patch Set 20 : Address comments. #Patch Set 21 : Rebase #
Messages
Total messages: 31 (0 generated)
ddorwin, dmichael, fgalligan1, xhwang: PTAL, this needs review. The code works, but I suspect I may be in for a reviewer pile. Please, be gentle. Also, obligatory: works on my machine, ship it! ;)
Took a quick look at things, and added comments for obvious stuff. Not sure about the allocator interface class name... maybe just drop the "Interface" suffix. I don't see that name used in Chromium, but maybe I'm just sucking at codesearch right now. http://codereview.chromium.org/10914028/diff/2001/ppapi/cpp/private/content_d... File ppapi/cpp/private/content_decryptor_private.h (right): http://codereview.chromium.org/10914028/diff/2001/ppapi/cpp/private/content_d... ppapi/cpp/private/content_decryptor_private.h:58: void DeliverBlock(PP_Resource decrypted_block, Changed this back to PP_Resource to avoid constructing a pp::Buffer_Dev using the PP_Resource ID from the CDM-- seems silly to pass the C++ version around when all we want to do is pass the resource ID back through the PPB proxy interface. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:18: #include "ppapi/cpp/module_impl.h" I might not need this-- was trying to use pp::get_interface<PPB_Buffer_Dev>() to get at the PPB_Buffer_Dev C interface, but that wasn't working. Maybe I should have been passing PPB_BUFFER_DEV_INTERFACE. Using pp::Module::Get()->GetBrowserInterface solved the problem, anyway. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:262: PP_DCHECK(buffer_iface); I'll move this to Init and make buffer_iface a member; doing this every time we allocate a buffer is unnecessary.
http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_allocator.h (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:17: class CdmAllocatorInterface { We don't usually use "Interface" suffix for interface base classes. Probably just use "CdmAllocator"? If I understand correctly (ddorwin: please correct me if I am wrong), this class should be a generic allocator class for the CDM interface. It should not care about how the allocator is implemented. The real allocator can use PPB_Buffer_Dev to leverage the shared memory, or it can be as simple as new/delete, malloc/release. So IMHO, this class should not depend on ppapi. If this sounds good to you, please remove PPAPI related contents in the class comment. Also, change the #include to use stdint.h (and add typedef for windows, see the header of CDM.h). http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:23: // Note: The memory returned is already mapped. ditto, remove implementation details, e.g. memoey is mapped.. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:24: virtual uint8_t* Allocate(int32_t size, int32_t* id) = 0; Return void* ? We may not need to do that since we always uses uint8_t in the CDM interface. In theory, |id| is just some opaque data, but |id| sounds ok here and I don't have a better name. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:25: }; We also need to add a void Deallocate(uint8_t*, int32_t id) function. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:63: class CdmWrapper : public CdmAllocatorInterface, Instead of making the CdmWrapper a CdmAllocator, how about making a new class in this file named PpbBufferCdmAllocator? In this CL, CdmWrapper::Allocate() doesn't use anything in CdmWrapper and does not need to be a class method of CdmWrapper. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:92: uint8_t* Allocate(int32_t size, int32_t* id) OVERRIDE; Add void Deallocate(uint8_t*, int32_t id) http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:137: if (!cdm_->Initialize(this)) Merge these two if's to: if (!cdm_ || !cdm_->Initialize(...)) http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:104: // Allocates shared memory buffers using the CDM wrapper. Again, the CDM doesn't care what the allocator implementation is used. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:131: uint8_t* data; // Pointer to the beginning of the output data. Does the "const" gave you any warning/error? http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:141: // Initialize the CDM by providing the allocator interface required for Initialize+s+. Remove double space here and on the next line. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:143: // true upon successful initialization, and false when |allocator| is NULL. Returns NULL when initialization fails, which includes the case where |allocator| is NULL. Need to rephrase somehow. http://codereview.chromium.org/10914028/diff/2001/webkit/media/webkit_media.gypi File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/webkit_media.g... webkit/media/webkit_media.gypi:107: 'crypto/ppapi/cdm_allocator.h', hmm, what's the rule to put interface header files in gyp files? Should we also add crypto/ppapi/content_decryption_module.h here?
ddorwin, dmichael: Updated per some of xhwang's comments, but some open questions remain. PTAL at the patchset 1 comments when you have a chance to review. Thanks! http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_allocator.h (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:17: class CdmAllocatorInterface { On 2012/09/01 13:49:09, xhwang wrote: > We don't usually use "Interface" suffix for interface base classes. Probably > just use "CdmAllocator"? > Done, using CdmAllocator. > If I understand correctly (ddorwin: please correct me if I am wrong), this class > should be a generic allocator class for the CDM interface. It should not care > about how the allocator is implemented. The real allocator can use > PPB_Buffer_Dev to leverage the shared memory, or it can be as simple as > new/delete, malloc/release. So IMHO, this class should not depend on ppapi. If > this sounds good to you, please remove PPAPI related contents in the class > comment. I think I get the underlying point here, but I think it's more complicated than that: 1) I think there has to be at least limited information provided re the fact that this allocator provides shared memory that is sent to the browser. I don't think that the implementation of this interface could ever use new/delete (or malloc/free) because the memory provided is owned by the ppapi resource tracker, and allocated using the PPB_Buffer_Dev methods, and... 2) If/when at some point we change to use ArrayBuffers, the same will remain true. This class will be using some PPB interface to allocate memory that belongs to the var tracker. Re my first point, maybe in a testing/mock situation it could use std memory routines. I don't think that potential situation is worth hiding this information from users/implementors of the interface. Anyway-- I'm not certain that my reasoning is correct. ddorwin, dmichael? > Also, change the #include to use stdint.h (and add typedef for windows, > see the header of CDM.h). Done. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:23: // Note: The memory returned is already mapped. On 2012/09/01 13:49:09, xhwang wrote: > ditto, remove implementation details, e.g. memoey is mapped.. Deferring this until open question above is resolved. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:25: }; On 2012/09/01 13:49:09, xhwang wrote: > We also need to add a void Deallocate(uint8_t*, int32_t id) function. I don't think this interface should have a public deallocation method. The ownership of the memory is only temporary, and we don't want CDMs trying to free it after it's been sent back to the browser. Instead, CdmWrapper could keep a list of resource IDs that have not yet been sent to the browser, and: 1) When sending an ID to the browser, remove it from the list. 2) In the dtor, release allocated resources that have not been sent to the browser. This would mean keeping some information around, and remembering to handle house keeping in the Deliver* methods, but it might not be necessary. My understanding is that if the plugin is destroyed, resources it owns will be cleaned up. dmichael can clarify for us. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:63: class CdmWrapper : public CdmAllocatorInterface, On 2012/09/01 13:49:09, xhwang wrote: > Instead of making the CdmWrapper a CdmAllocator, how about making a new class in > this file named PpbBufferCdmAllocator? > > In this CL, CdmWrapper::Allocate() doesn't use anything in CdmWrapper and does > not need to be a class method of CdmWrapper. It uses pp_instance(), and buffer_iface_ is now a CdmWrapper member. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:92: uint8_t* Allocate(int32_t size, int32_t* id) OVERRIDE; On 2012/09/01 13:49:09, xhwang wrote: > Add void Deallocate(uint8_t*, int32_t id) See comments in cdm_allocator.h http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:137: if (!cdm_->Initialize(this)) On 2012/09/01 13:49:09, xhwang wrote: > Merge these two if's to: if (!cdm_ || !cdm_->Initialize(...)) Done. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:262: PP_DCHECK(buffer_iface); On 2012/09/01 05:00:09, tomf wrote: > I'll move this to Init and make buffer_iface a member; doing this every time we > allocate a buffer is unnecessary. Done. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:104: // Allocates shared memory buffers using the CDM wrapper. On 2012/09/01 13:49:09, xhwang wrote: > Again, the CDM doesn't care what the allocator implementation is used. My reasoning for this comment is that allocation of shared memory carries implied alignment per https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/VzuISrCbq... http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:131: uint8_t* data; // Pointer to the beginning of the output data. On 2012/09/01 13:49:09, xhwang wrote: > Does the "const" gave you any warning/error? Yes, at the memcpy call site. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:141: // Initialize the CDM by providing the allocator interface required for On 2012/09/01 13:49:09, xhwang wrote: > Initialize+s+. Remove double space here and on the next line. Done. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:143: // true upon successful initialization, and false when |allocator| is NULL. On 2012/09/01 13:49:09, xhwang wrote: > Returns NULL when initialization fails, which includes the case where > |allocator| is NULL. Need to rephrase somehow. Not following-- method returns bool. http://codereview.chromium.org/10914028/diff/2001/webkit/media/webkit_media.gypi File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/webkit_media.g... webkit/media/webkit_media.gypi:107: 'crypto/ppapi/cdm_allocator.h', On 2012/09/01 13:49:09, xhwang wrote: > hmm, what's the rule to put interface header files in gyp files? Should we also > add crypto/ppapi/content_decryption_module.h here? I don't know the rule, but I like having the .h files referenced in the gyp file because it makes them accessible in MSVC's solution explorer. Added content_decryption_module.h.
ddorwin, dmichael: Err, PTAL at the patchset 2 comments. Sorry for the extra spam!
Replied to a few items in PS 2. Most comments are in PS 3. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_allocator.h (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:17: class CdmAllocatorInterface { On 2012/09/01 21:32:49, tomf wrote: > On 2012/09/01 13:49:09, xhwang wrote: > > We don't usually use "Interface" suffix for interface base classes. Probably > > just use "CdmAllocator"? > > > > Done, using CdmAllocator. > > > If I understand correctly (ddorwin: please correct me if I am wrong), this > class > > should be a generic allocator class for the CDM interface. It should not care > > about how the allocator is implemented. The real allocator can use > > PPB_Buffer_Dev to leverage the shared memory, or it can be as simple as > > new/delete, malloc/release. So IMHO, this class should not depend on ppapi. If > > this sounds good to you, please remove PPAPI related contents in the class > > comment. > > I think I get the underlying point here, but I think it's more complicated than > that: > > 1) I think there has to be at least limited information provided re the fact > that this allocator provides shared memory that is sent to the browser. I don't > think that the implementation of this interface could ever use new/delete (or > malloc/free) because the memory provided is owned by the ppapi resource tracker, > and allocated using the PPB_Buffer_Dev methods, and... > > 2) If/when at some point we change to use ArrayBuffers, the same will remain > true. This class will be using some PPB interface to allocate memory that > belongs to the var tracker. > > Re my first point, maybe in a testing/mock situation it could use std memory > routines. I don't think that potential situation is worth hiding this > information from users/implementors of the interface. > > Anyway-- I'm not certain that my reasoning is correct. ddorwin, dmichael? > > > Also, change the #include to use stdint.h (and add typedef for windows, > > see the header of CDM.h). > > Done. > This class should know nothing about PPAPi or how memory is allocated. This seems to be the case except for comments. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:23: // Note: The memory returned is already mapped. On 2012/09/01 21:32:49, tomf wrote: > On 2012/09/01 13:49:09, xhwang wrote: > > ditto, remove implementation details, e.g. memoey is mapped.. > > Deferring this until open question above is resolved. Agreed, the interface should not have implementation details. The only real requirement is that the buffer is valid when returned. See comment in Patch Set 3 - it may help with this. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:24: virtual uint8_t* Allocate(int32_t size, int32_t* id) = 0; On 2012/09/01 13:49:09, xhwang wrote: > Return void* ? We may not need to do that since we always uses uint8_t in the > CDM interface. > > In theory, |id| is just some opaque data, but |id| sounds ok here and I don't > have a better name. See comment in Patch Set 3 - it may help with this. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:25: }; On 2012/09/01 21:32:49, tomf wrote: > On 2012/09/01 13:49:09, xhwang wrote: > > We also need to add a void Deallocate(uint8_t*, int32_t id) function. > > I don't think this interface should have a public deallocation method. The > ownership of the memory is only temporary, and we don't want CDMs trying to free > it after it's been sent back to the browser. See comment in Patch Set 3 - If you have an object, you can encapsulate destruction there. > > Instead, CdmWrapper could keep a list of resource IDs that have not yet been > sent to the browser, and: > > 1) When sending an ID to the browser, remove it from the list. > 2) In the dtor, release allocated resources that have not been sent to the > browser. > > This would mean keeping some information around, and remembering to handle house > keeping in the Deliver* methods, but it might not be necessary. My understanding > is that if the plugin is destroyed, resources it owns will be cleaned up. > dmichael can clarify for us. I don't know whether we need to track. If you don't, you _could_ leak buffers, but that shouldn't happen. Maybe we need to delete them all on a flush or something. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:63: class CdmWrapper : public CdmAllocatorInterface, On 2012/09/01 21:32:49, tomf wrote: > On 2012/09/01 13:49:09, xhwang wrote: > > Instead of making the CdmWrapper a CdmAllocator, how about making a new class > in > > this file named PpbBufferCdmAllocator? > > > > In this CL, CdmWrapper::Allocate() doesn't use anything in CdmWrapper and does > > not need to be a class method of CdmWrapper. > > It uses pp_instance(), Pass it into the constructor. Also, see my comment in the CdmAllocator file. > and buffer_iface_ is now a CdmWrapper member. It could just as easily be a member of the new class. http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10914028/diff/2001/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:104: // Allocates shared memory buffers using the CDM wrapper. On 2012/09/01 21:32:49, tomf wrote: > On 2012/09/01 13:49:09, xhwang wrote: > > Again, the CDM doesn't care what the allocator implementation is used. > > My reasoning for this comment is that allocation of shared memory carries > implied alignment per > https://groups.google.com/a/chromium.org/forum/#%21topic/chromium-dev/VzuISrC... If alignment is the information you want to provide, just say so explicitly in the CdmAllocator (then DCHECK it everywhere). I would not have known that this implied alignment. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_allocator.h (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:17: // Interface class intended to hide the PPAPI memory allocation implementation Shouldn't mention PPAPI. It really addresses the cross-shared object allocation issues and PPAPI is an implementation detail. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:19: // interface handles allocation. The data will be freed when the last reference "The CDM wrapper takes ownership of the allocated data when it is returned to the wrapper by the CDM (i.e. by DeliverBlock)." ^ I think that summarizes what really happens without going into details. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:22: class CdmAllocator { Should this just be in CDM.h or is there a good reason to have a separate file? http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:29: virtual uint8_t* Allocate(int32_t size, int32_t* id) = 0; To further abstract things and ensure that things get deleted correctly: Does it make sense to return and pass around a CdmBuffer object that contains the pointer and ID values? Really, it's just: class CdmBuffer { uint8_t* getBuffer(); int32_t getBufferSize(); } The pointer and ID would be stored and only available within the PpbCdemBuffer subclass. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:12: #include "ppapi/c/dev/ppb_buffer_dev.h" Why not use cpp? http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:110: const PPB_Buffer_Dev* buffer_iface_; Might be nice to keep all the raw pointers together. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:130: buffer_iface_ = static_cast<const PPB_Buffer_Dev*>( Why not just use the C++ object? http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:260: // require that the buffer is mapped. This might be clearer wrt transferring ownership (see comment) if there was an allocated buffer object (see comment) and we extracted the ID to pass here. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:267: PP_DCHECK(id); delete http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:269: void* buffer = buffer_iface_->Map(buffer_resource); Can this not fail? Seems there should either be a DCHECK here or none at 266. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:341: pp::ContentDecryptor_Private::DeliverBlock(output_buffer.buffer_id, This might be clearer wrt transferring ownership (see comment) if there was an allocated buffer object (see comment) and we extracted the ID to pass here. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:342: decrypted_block_info); Who is dereferencing the buffer? There should be clear comments about transferring ownership. I think it's fair to say that the browser will AddRef the buffer if it needs to hang onto it. But then I wonder about the proxy, which is asynchronous, right? So, the plugin would need to leave at least one ref open so the browser can assume there is one when it gets the ID. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:132: bool ClearKeyCdm::Initialize(CdmAllocator* allocator) { Is there a reason we can't do this in the constructor? http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:133: if (!allocator) { Seems like a DCHECK. And can remove return type (or add to constructor per above). http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:152: key_request->session_id = AllocateAndCopy(client_.session_id().data(), These are still problematic. You can add a TODO for now if we want to get Decrypt* tested and landed. You may need to replace AllocateAndCopy() with the new code below and/or have separate allocate functions for big and small values. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:226: memcpy(reinterpret_cast<void*>(decrypted_buffer->data), Copy = boo! Maybe a TODO to make CopyDecoderBufferFrom take a target buffer so we can focus on that separately. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:105: CdmAllocator* allocator_iface_; generalize comment and remove _iface. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:135: int32_t buffer_id; // Buffer identifier. Issued by CdmAllocator, and used by A buffer object (see comment) makes comment this simpler or unnecessary. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:141: // Initializes the CDM by providing the allocator interface required for The real issue is related to shared object boundary. shared memory is an optimization. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:143: // true upon successful initialization, and false when |allocator| is NULL. No reason for a return value or supporting NULL. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:144: virtual bool Initialize(webkit_media::CdmAllocator* allocator) = 0; Does it make sense to pass this to CreateCdmInstance()? http://codereview.chromium.org/10914028/diff/1004/webkit/media/webkit_media.gypi File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/webkit_media.g... webkit/media/webkit_media.gypi:107: 'crypto/ppapi/content_decryption_module.h', alphabetical?
http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:63: class CdmWrapper : public CdmAllocator, Is there any reason to have CdmWrapper implement this? Seems easy and logical to abstract it out to a CdmAllocatorImpl (or whatever). http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:265: PP_Resource buffer_resource = buffer_iface_->Create(pp_instance(), size); You don't *have* to use the C interface. The alternative is to leave the pp::Buffer_Dev sitting around. Maybe in a hash map or something. You presumably need to keep track of them anyway. Speaking of... I don't see you clean up this PPB_Buffer_Dev resource anywhere.
http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:135: int32_t buffer_id; // Buffer identifier. Issued by CdmAllocator, and used by On 2012/09/04 09:53:15, ddorwin wrote: > A buffer object (see comment) makes comment this simpler or unnecessary. This comment is related to the comment at http://codereview.chromium.org/10900007/diff/14001/webkit/media/crypto/ppapi/... To clarify, replace data and buffer_id with the abstract buffer object. Depending on whether the buffer object owns the data and how you implement things, OutputBuffer may own the new object or have a copy or pointer to it. In either case, this should hopefully clarify ownership of the buffer (currently a pointer). The caller to Decrypt() could have already instantiated the buffer object (as a member of OutputBuffer) or the callee could assign the buffer object to a pointer in OutputBuffer.
PTAL, refactored and responded to comments. Lots of changes, but it doesn't leak and seems to work. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_allocator.h (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:17: // Interface class intended to hide the PPAPI memory allocation implementation On 2012/09/04 09:53:15, ddorwin wrote: > Shouldn't mention PPAPI. It really addresses the cross-shared object allocation > issues and PPAPI is an implementation detail. Done. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:19: // interface handles allocation. The data will be freed when the last reference On 2012/09/04 09:53:15, ddorwin wrote: > "The CDM wrapper takes ownership of the allocated data when it is returned to > the wrapper by the CDM (i.e. by DeliverBlock)." > > ^ I think that summarizes what really happens without going into details. Done. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:22: class CdmAllocator { On 2012/09/04 09:53:15, ddorwin wrote: > Should this just be in CDM.h or is there a good reason to have a separate file? Deleted cdm_allocator.h, and moved CdmAllocator to CDM.h. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_allocator.h:29: virtual uint8_t* Allocate(int32_t size, int32_t* id) = 0; On 2012/09/04 09:53:15, ddorwin wrote: > To further abstract things and ensure that things get deleted correctly: Does it > make sense to return and pass around a CdmBuffer object that contains the > pointer and ID values? Really, it's just: > class CdmBuffer { uint8_t* getBuffer(); int32_t getBufferSize(); } > The pointer and ID would be stored and only available within the PpbCdemBuffer > subclass. Added CdmBuffer to CDM.h. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:12: #include "ppapi/c/dev/ppb_buffer_dev.h" On 2012/09/04 09:53:15, ddorwin wrote: > Why not use cpp? Done, using it now. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:63: class CdmWrapper : public CdmAllocator, On 2012/09/04 18:06:01, dmichael wrote: > Is there any reason to have CdmWrapper implement this? Seems easy and logical to > abstract it out to a CdmAllocatorImpl (or whatever). Done. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:110: const PPB_Buffer_Dev* buffer_iface_; On 2012/09/04 09:53:15, ddorwin wrote: > Might be nice to keep all the raw pointers together. Removed this. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:130: buffer_iface_ = static_cast<const PPB_Buffer_Dev*>( On 2012/09/04 09:53:15, ddorwin wrote: > Why not just use the C++ object? Done. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:260: // require that the buffer is mapped. On 2012/09/04 09:53:15, ddorwin wrote: > This might be clearer wrt transferring ownership (see comment) if there was an > allocated buffer object (see comment) and we extracted the ID to pass here. Done. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:265: PP_Resource buffer_resource = buffer_iface_->Create(pp_instance(), size); On 2012/09/04 18:06:01, dmichael wrote: > You don't *have* to use the C interface. The alternative is to leave the > pp::Buffer_Dev sitting around. Maybe in a hash map or something. You presumably > need to keep track of them anyway. > > Speaking of... I don't see you clean up this PPB_Buffer_Dev resource anywhere. Done. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:267: PP_DCHECK(id); On 2012/09/04 09:53:15, ddorwin wrote: > delete Done. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:269: void* buffer = buffer_iface_->Map(buffer_resource); On 2012/09/04 09:53:15, ddorwin wrote: > Can this not fail? Seems there should either be a DCHECK here or none at 266. Done. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:341: pp::ContentDecryptor_Private::DeliverBlock(output_buffer.buffer_id, On 2012/09/04 09:53:15, ddorwin wrote: > This might be clearer wrt transferring ownership (see comment) if there was an > allocated buffer object (see comment) and we extracted the ID to pass here. Done. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:342: decrypted_block_info); On 2012/09/04 09:53:15, ddorwin wrote: > Who is dereferencing the buffer? There should be clear comments about > transferring ownership. > > I think it's fair to say that the browser will AddRef the buffer if it needs to > hang onto it. But then I wonder about the proxy, which is asynchronous, right? > So, the plugin would need to leave at least one ref open so the browser can > assume there is one when it gets the ID. I think we're OK here. The buffer has one ref in DeliverBlock on the plugin side of the proxy, and we now release the plugin ref after DeliverBlock returns. Things are working, for what that's worth, at least. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:132: bool ClearKeyCdm::Initialize(CdmAllocator* allocator) { On 2012/09/04 09:53:15, ddorwin wrote: > Is there a reason we can't do this in the constructor? Nope, done. Also, I made the default constructor private. Not sure if that's OK here, and, in the spirit of over engineering, added a NOTREACHED() to the default constructor implementation. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:133: if (!allocator) { On 2012/09/04 09:53:15, ddorwin wrote: > Seems like a DCHECK. And can remove return type (or add to constructor per > above). Moved to constructor, and I put a DCHECK there. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:152: key_request->session_id = AllocateAndCopy(client_.session_id().data(), On 2012/09/04 09:53:15, ddorwin wrote: > These are still problematic. You can add a TODO for now if we want to get > Decrypt* tested and landed. > You may need to replace AllocateAndCopy() with the new code below and/or have > separate allocate functions for big and small values. Added TODO for message. The other two are Vars-- maybe CdmAllocator needs another design iteration that adds a type field, and support for Vars. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.cc:226: memcpy(reinterpret_cast<void*>(decrypted_buffer->data), On 2012/09/04 09:53:15, ddorwin wrote: > Copy = boo! > Maybe a TODO to make CopyDecoderBufferFrom take a target buffer so we can focus > on that separately. TODO added. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:105: CdmAllocator* allocator_iface_; On 2012/09/04 09:53:15, ddorwin wrote: > generalize comment and remove _iface. Done. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:135: int32_t buffer_id; // Buffer identifier. Issued by CdmAllocator, and used by On 2012/09/04 09:53:15, ddorwin wrote: > A buffer object (see comment) makes comment this simpler or unnecessary. I've added CdmBuffer, but using a simple integer here seems simpler... at least, with a simple integer, CdmBuffer doesn't need an assignment operator or a copy contstructor (or a CopyFrom method, if that's preferred). Should I make cdm_buffer source files, and add the above? As it is the whole implementation is inline. Maybe I've approached this the wrong way. CdmBuffer is not abstract at present... http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:141: // Initializes the CDM by providing the allocator interface required for On 2012/09/04 09:53:15, ddorwin wrote: > The real issue is related to shared object boundary. shared memory is an > optimization. Method removed. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:143: // true upon successful initialization, and false when |allocator| is NULL. On 2012/09/04 09:53:15, ddorwin wrote: > No reason for a return value or supporting NULL. The constructor DCHECKs the pointer. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:144: virtual bool Initialize(webkit_media::CdmAllocator* allocator) = 0; On 2012/09/04 09:53:15, ddorwin wrote: > Does it make sense to pass this to CreateCdmInstance()? Yes, doing that now. http://codereview.chromium.org/10914028/diff/1004/webkit/media/webkit_media.gypi File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/webkit_media.g... webkit/media/webkit_media.gypi:107: 'crypto/ppapi/content_decryption_module.h', On 2012/09/04 09:53:15, ddorwin wrote: > alphabetical? Done.
One comment in patch set 3; he rest in 5. http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:135: int32_t buffer_id; // Buffer identifier. Issued by CdmAllocator, and used by On 2012/09/07 00:46:36, tomf wrote: > On 2012/09/04 09:53:15, ddorwin wrote: > > A buffer object (see comment) makes comment this simpler or unnecessary. > > I've added CdmBuffer, but using a simple integer here seems simpler... at least, > with a simple integer, CdmBuffer doesn't need an assignment operator or a copy > contstructor (or a CopyFrom method, if that's preferred). Yes, you'd copy 9 bytes, but that's fine, and the copy constructor and assignment operator are free since it only has basic types. Note that copying is fine because you're not (currently) storing the BufferDev in the CdmBuffer. > > Should I make cdm_buffer source files, and add the above? As it is the whole > implementation is inline. No, we just want one header file for the "public" interface. > > Maybe I've approached this the wrong way. CdmBuffer is not abstract at > present... Yes, please make it abstract. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:6: #include <queue> not used http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:15: #include "ppapi/c/dev/ppb_buffer_dev.h" Remove http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:79: typedef base::hash_map<int32_t, pp::Buffer_Dev> BufferMap; Why not use the PP_Resource type? Inserting will copy the Buffer_Dev. Is this what we want? http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:156: BufferPair pair = std::make_pair(buffer.pp_resource(), buffer); Creates a copy of the Buffer_Dev. Is that costly? http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:159: return cdm::CdmBuffer(reinterpret_cast<uint8_t*>(buffer.data()), Create PpbCdmBuffer http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:165: bool result = false; Why do we need this here instead of just returning false at 171? http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:374: pp::ContentDecryptor_Private::DeliverBlock(output_buffer.buffer_id, OutputBuffer should contain a CdmBuffer (pointer). Then you can pass its ID. You'd either need to leave id() exposed from CdmBuffer or static_cast back to a PpbCdmBuffer http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:377: LOG(ERROR) << "CdmWrapper::DeliverBlock: buffer_id=" DLOG. DCHECK? This should never happen, right? You could also move this loggin inside the ReleaseBuffer function so it is always called (if there was more than one call site). You might then consider a void return, especially if it should never happen. After all, what is the caller supposed to do? http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:19: media::DecoderBuffer::CopyFrom(input_buffer.data, input_buffer.data_size); We'll want to avoid this copy too. Should .data be a CdmBuffer? (I'm more worried about the interface and how it's used in the production CDM than I am about this specific code (which just needs a TODO). http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:128: NOTREACHED(); Why does this constructor exist? If you don't declare it, it won't exist because you have the constructor below. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:226: DCHECK(cdm_buffer.buffer()); This can fail, so you should check and return an error. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:228: decrypted_buffer->buffer_id = cdm_buffer.id(); After addressing comment in CDM.h, just copy the CdmBuffer here http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:231: // TODO(tomfinegan): Write a CopyDecoderBufferFrom that has a target buffer I don't understand this comment. What you really need is the ability for DecoderBuffer to wrap a CdmBuffer. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.h:99: ClearKeyCdm(); Remove http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:20: class CdmAllocator; "Cdm" is redundant in the "cdm" interface. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:141: int32_t buffer_id; // Buffer identifier. Issued by CdmAllocator, and used by Why is this not just a CdmBuffer? http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:316: The comment should probably document the behavior. Will the buffer be freed when this object is destroyed? You might have similar questions on construction unless you prevent anything but an empty buffer from being created in the base class. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:317: // Utility class that encapsulates all data required to interact with buffers Remove "Utility class that ". http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:318: // allocated by |CdmAllocator|s. I don't think | is necessary around class names. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:319: class CdmBuffer { This should be an abstract class that only exposes buffer()/data(). id and size are implementation details of the Impl. The former is specifically related to the buffer. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:321: // Constructs an empty buffer. Constructing a buffer sounds like allocation. But really it just represents it. (Unless you change the Impl to hold the actual buffer. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:325: // user writable buffer. "user"? http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:329: // Constructs a buffer containing only a valid ID. Provided for the This seems odd. Maybe it's not necessary if we store the CdmBuffer in OutputBuffer. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:334: ~CdmBuffer() {} Be sure this is virtual after making this a base class. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:341: uint8_t* buffer_; *const http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:346: // Interface class intended to hide cross object memory allocation details from s/intended to/that/ http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:347: // CDMs. The CDM wrapper takes ownership of the allocated data when it is This sentence sounds like it should be on those methods. What do we really need to say here. Is my comment below sufficient? http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:348: // returned to the wrapper by the CDM (i.e. via OutputBuffer in Decrypt()). Allocated buffers should only be freed by the CDM wrapper. http://codereview.chromium.org/10914028/diff/13001/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10914028/diff/13001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:108: 'crypto/ppapi/cdm_wrapper.cc', alphabetical
http://codereview.chromium.org/10914028/diff/13001/ppapi/cpp/private/content_... File ppapi/cpp/private/content_decryptor_private.h (right): http://codereview.chromium.org/10914028/diff/13001/ppapi/cpp/private/content_... ppapi/cpp/private/content_decryptor_private.h:58: void DeliverBlock(PP_Resource decrypted_block, What's the benefit to this part of the change? I'd rather see pp::Buffer_Dev for consistency, I guess, and to help keep the ref-counting sane. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:79: typedef base::hash_map<int32_t, pp::Buffer_Dev> BufferMap; On 2012/09/07 09:48:48, ddorwin wrote: > Why not use the PP_Resource type? > Inserting will copy the Buffer_Dev. Is this what we want? pp::Buffer_Dev is only a handle to the buffer. Copying just copies the handle, not the actual underlying buffer. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:156: BufferPair pair = std::make_pair(buffer.pp_resource(), buffer); On 2012/09/07 09:48:48, ddorwin wrote: > Creates a copy of the Buffer_Dev. Is that costly? No, it's just a handle. There's just reference counting and copying an int32_t id. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:157: buffer_map_.insert(pair); You could just insert unconditionally, then check the result to see if it failed. Teensy bit more efficient, and gets rid of a usage of base (ContainsKey). http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:377: LOG(ERROR) << "CdmWrapper::DeliverBlock: buffer_id=" On 2012/09/07 09:48:48, ddorwin wrote: > DLOG. DCHECK? This should never happen, right? > > You could also move this loggin inside the ReleaseBuffer function so it is > always called (if there was more than one call site). You might then consider a > void return, especially if it should never happen. After all, what is the caller > supposed to do? Does logging work here? (Maybe you want PP_DCHECK)
I think I've addressed all comment, but maybe not satisfactorily. PTAL/Thanks! http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/1004/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:135: int32_t buffer_id; // Buffer identifier. Issued by CdmAllocator, and used by On 2012/09/07 09:48:48, ddorwin wrote: > On 2012/09/07 00:46:36, tomf wrote: > > On 2012/09/04 09:53:15, ddorwin wrote: > > > A buffer object (see comment) makes comment this simpler or unnecessary. > > > > I've added CdmBuffer, but using a simple integer here seems simpler... at > least, > > with a simple integer, CdmBuffer doesn't need an assignment operator or a copy > > contstructor (or a CopyFrom method, if that's preferred). > > Yes, you'd copy 9 bytes, but that's fine, and the copy constructor and > assignment operator are free since it only has basic types. Note that copying is > fine because you're not (currently) storing the BufferDev in the CdmBuffer. > > > > Should I make cdm_buffer source files, and add the above? As it is the whole > > implementation is inline. > > No, we just want one header file for the "public" interface. > > > > Maybe I've approached this the wrong way. CdmBuffer is not abstract at > > present... > > Yes, please make it abstract. Done. Allocator::Allocate now returns a cdm::Buffer* that is owned by OutputBuffer. Should I make that an auto_ptr, or just leave it as a raw pointer w/a delete in ~OutputBuffer? http://codereview.chromium.org/10914028/diff/13001/ppapi/cpp/private/content_... File ppapi/cpp/private/content_decryptor_private.h (right): http://codereview.chromium.org/10914028/diff/13001/ppapi/cpp/private/content_... ppapi/cpp/private/content_decryptor_private.h:58: void DeliverBlock(PP_Resource decrypted_block, On 2012/09/07 16:43:43, dmichael wrote: > What's the benefit to this part of the change? I'd rather see pp::Buffer_Dev for > consistency, I guess, and to help keep the ref-counting sane. Changed it back; the above was done before I had my learning experience with the memory leak fix. ;) http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:6: #include <queue> On 2012/09/07 09:48:48, ddorwin wrote: > not used Done. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:15: #include "ppapi/c/dev/ppb_buffer_dev.h" On 2012/09/07 09:48:48, ddorwin wrote: > Remove Done. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:79: typedef base::hash_map<int32_t, pp::Buffer_Dev> BufferMap; On 2012/09/07 16:43:43, dmichael wrote: > On 2012/09/07 09:48:48, ddorwin wrote: > > Why not use the PP_Resource type? > > Inserting will copy the Buffer_Dev. Is this what we want? > pp::Buffer_Dev is only a handle to the buffer. Copying just copies the handle, > not the actual underlying buffer. Changed int32_t in the typedef to PP_Resource for clarity. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:157: buffer_map_.insert(pair); On 2012/09/07 16:43:43, dmichael wrote: > You could just insert unconditionally, then check the result to see if it > failed. Teensy bit more efficient, and gets rid of a usage of base > (ContainsKey). Done. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:159: return cdm::CdmBuffer(reinterpret_cast<uint8_t*>(buffer.data()), On 2012/09/07 09:48:48, ddorwin wrote: > Create PpbCdmBuffer Do you want me to add a CreateBuffer method to PpbCdmBuffer, or is returned new PpbCdmBuffer(...) sufficient? http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:165: bool result = false; On 2012/09/07 09:48:48, ddorwin wrote: > Why do we need this here instead of just returning false at 171? Done. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:374: pp::ContentDecryptor_Private::DeliverBlock(output_buffer.buffer_id, On 2012/09/07 09:48:48, ddorwin wrote: > OutputBuffer should contain a CdmBuffer (pointer). Then you can pass its ID. > You'd either need to leave id() exposed from CdmBuffer or static_cast back to a > PpbCdmBuffer Went w/static_cast<PpbCdmbuffer>. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:377: LOG(ERROR) << "CdmWrapper::DeliverBlock: buffer_id=" On 2012/09/07 16:43:43, dmichael wrote: > On 2012/09/07 09:48:48, ddorwin wrote: > > DLOG. DCHECK? This should never happen, right? > > > > You could also move this loggin inside the ReleaseBuffer function so it is > > always called (if there was more than one call site). You might then consider > a > > void return, especially if it should never happen. After all, what is the > caller > > supposed to do? > Does logging work here? (Maybe you want PP_DCHECK) Logging works, but I've removed the LOG usage-- it was only there for debugging, anyway. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:19: media::DecoderBuffer::CopyFrom(input_buffer.data, input_buffer.data_size); On 2012/09/07 09:48:48, ddorwin wrote: > We'll want to avoid this copy too. Should .data be a CdmBuffer? (I'm more > worried about the interface and how it's used in the production CDM than I am > about this specific code (which just needs a TODO). Done, added TODO. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:128: NOTREACHED(); On 2012/09/07 09:48:48, ddorwin wrote: > Why does this constructor exist? If you don't declare it, it won't exist because > you have the constructor below. Removed. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:226: DCHECK(cdm_buffer.buffer()); On 2012/09/07 09:48:48, ddorwin wrote: > This can fail, so you should check and return an error. Done-- also removed the DCHECKs, since they seem like overkill now that an error is returned. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:228: decrypted_buffer->buffer_id = cdm_buffer.id(); On 2012/09/07 09:48:48, ddorwin wrote: > After addressing comment in CDM.h, just copy the CdmBuffer here Done, but it's just a pointer assignment. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:231: // TODO(tomfinegan): Write a CopyDecoderBufferFrom that has a target buffer On 2012/09/07 09:48:48, ddorwin wrote: > I don't understand this comment. What you really need is the ability for > DecoderBuffer to wrap a CdmBuffer. Fixed TODO. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.h:99: ClearKeyCdm(); On 2012/09/07 09:48:48, ddorwin wrote: > Remove Done. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:20: class CdmAllocator; On 2012/09/07 09:48:48, ddorwin wrote: > "Cdm" is redundant in the "cdm" interface. Done. Also renamed CdmBuffer to Buffer. For the same reason. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:141: int32_t buffer_id; // Buffer identifier. Issued by CdmAllocator, and used by On 2012/09/07 09:48:48, ddorwin wrote: > Why is this not just a CdmBuffer? Removed data, data_size, and buffer_id. Replaced with cdm::Buffer*. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:316: On 2012/09/07 09:48:48, ddorwin wrote: > The comment should probably document the behavior. Will the buffer be freed when > this object is destroyed? Added note saying that the buffer is not freed. > You might have similar questions on construction unless you prevent anything but > an empty buffer from being created in the base class. Base class is now abstract, so moot, no? http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:317: // Utility class that encapsulates all data required to interact with buffers On 2012/09/07 09:48:48, ddorwin wrote: > Remove "Utility class that ". Done. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:318: // allocated by |CdmAllocator|s. On 2012/09/07 09:48:48, ddorwin wrote: > I don't think | is necessary around class names. Done. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:319: class CdmBuffer { On 2012/09/07 09:48:48, ddorwin wrote: > This should be an abstract class that only exposes buffer()/data(). > id and size are implementation details of the Impl. The former is specifically > related to the buffer. I left the size method in the class since doing so allowed removal of the data and data_size members from OutputBuffer. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:321: // Constructs an empty buffer. On 2012/09/07 09:48:48, ddorwin wrote: > Constructing a buffer sounds like allocation. But really it just represents it. > (Unless you change the Impl to hold the actual buffer. Removed. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:325: // user writable buffer. On 2012/09/07 09:48:48, ddorwin wrote: > "user"? Meant writable by user of the class, but I dropped the user. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:329: // Constructs a buffer containing only a valid ID. Provided for the On 2012/09/07 09:48:48, ddorwin wrote: > This seems odd. Maybe it's not necessary if we store the CdmBuffer in > OutputBuffer. You were right: storing the cdm::Buffer in OutputBuffer made this constructor unnecessary. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:334: ~CdmBuffer() {} On 2012/09/07 09:48:48, ddorwin wrote: > Be sure this is virtual after making this a base class. Done. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:341: uint8_t* buffer_; On 2012/09/07 09:48:48, ddorwin wrote: > *const cdm::Buffer no longer has data members. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:346: // Interface class intended to hide cross object memory allocation details from On 2012/09/07 09:48:48, ddorwin wrote: > s/intended to/that/ Done. http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:347: // CDMs. The CDM wrapper takes ownership of the allocated data when it is On 2012/09/07 09:48:48, ddorwin wrote: > This sentence sounds like it should be on those methods. What do we really need > to say here. Is my comment below sufficient? Used your comment, but added a little bit... http://codereview.chromium.org/10914028/diff/13001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:348: // returned to the wrapper by the CDM (i.e. via OutputBuffer in Decrypt()). On 2012/09/07 09:48:48, ddorwin wrote: > Allocated buffers should only be freed by the CDM wrapper. Done. http://codereview.chromium.org/10914028/diff/13001/webkit/media/webkit_media.... File webkit/media/webkit_media.gypi (right): http://codereview.chromium.org/10914028/diff/13001/webkit/media/webkit_media.... webkit/media/webkit_media.gypi:108: 'crypto/ppapi/cdm_wrapper.cc', On 2012/09/07 09:48:48, ddorwin wrote: > alphabetical Oops. Done.
http://codereview.chromium.org/10914028/diff/1014/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/1014/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:79: }; It kind of looks like you don't expect this class to be copied. Is that true? If so, you should add an unimplemented copy constructor and assignment operator. (Or use DISALLOW_COPY_AND_ASSIGN, if we're giving up on keeping base out of it). http://codereview.chromium.org/10914028/diff/1014/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:92: // Relinquishes buffer ownership by removing it from |buffer_map_|. Maybe point out that |buffer|'s internal data is probably invalid after this call? http://codereview.chromium.org/10914028/diff/1014/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:388: pp::Buffer_Dev(pp::PassRef(), cdm_buffer->id()), Is PassRef() the right thing here? Best I can tell, you do not have an extra reference to pass, and the buffer should go away when you ReleaseBuffer() below. I.e., before this temporary Buffer_Dev goes out of scope, it will become invalid. I think it will just silently/harmlessly fail in this case, since you don't end up trying to access the resource again after ReleaseBuffer. But the ref-counting needs to be right, or it will bite you later. So a couple of ways you could go: 1) Just use the normal Buffer_Dev constructor that takes a reference. 2) Actually store a pp::Resource in PpbCdmBuffer, and add a way to get it. Then, the PpbCdmBuffer will keep its own reference. You could just pass it directly here (cdm_buffer->pp_buffer_dev(), or something). You'd be less likely to mess up the reference counting, and I think you could also get rid of the hash map and ReleaseBuffer in your Allocator. http://codereview.chromium.org/10914028/diff/1014/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/1014/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:100: virtual int32_t size() const = 0; Consider disallowing copy and assign here, too.
http://codereview.chromium.org/10914028/diff/1014/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/1014/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:79: }; On 2012/09/10 16:10:35, dmichael wrote: > It kind of looks like you don't expect this class to be copied. Is that true? If > so, you should add an unimplemented copy constructor and assignment operator. > (Or use DISALLOW_COPY_AND_ASSIGN, if we're giving up on keeping base out of it). Done. http://codereview.chromium.org/10914028/diff/1014/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:92: // Relinquishes buffer ownership by removing it from |buffer_map_|. On 2012/09/10 16:10:35, dmichael wrote: > Maybe point out that |buffer|'s internal data is probably invalid after this > call? Done, added mention that the buffer memory will become invalid if the ref count is reduced to 0. http://codereview.chromium.org/10914028/diff/1014/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:388: pp::Buffer_Dev(pp::PassRef(), cdm_buffer->id()), On 2012/09/10 16:10:35, dmichael wrote: > Is PassRef() the right thing here? Best I can tell, you do not have an extra > reference to pass, and the buffer should go away when you ReleaseBuffer() below. > I.e., before this temporary Buffer_Dev goes out of scope, it will become > invalid. I think it will just silently/harmlessly fail in this case, since you > don't end up trying to access the resource again after ReleaseBuffer. But the > ref-counting needs to be right, or it will bite you later. > > So a couple of ways you could go: > 1) Just use the normal Buffer_Dev constructor that takes a reference. > 2) Actually store a pp::Resource in PpbCdmBuffer, and add a way to get it. Then, > the PpbCdmBuffer will keep its own reference. You could just pass it directly > here (cdm_buffer->pp_buffer_dev(), or something). You'd be less likely to mess > up the reference counting, and I think you could also get rid of the hash map > and ReleaseBuffer in your Allocator. Done, and thanks for catching this one! I took the easy way out and went with option 1. I'm not opposed to making the change, but I think I like the current implementation. ;) http://codereview.chromium.org/10914028/diff/1014/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/1014/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:100: virtual int32_t size() const = 0; On 2012/09/10 16:10:35, dmichael wrote: > Consider disallowing copy and assign here, too. Done, but did it manually (don't want to add the base/basictypes.h dependency in CDM.h).
lgtm
http://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:65: // Constructs a buffer object containing all information necessary for a This wording is a bit odd, especially such it doesn't allocate any buffer. Does it make sense for the atual PPAPI calls to occur in here? I guess that depends on whether we copy these buffers around. http://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:74: PP_Resource id() const { return id_; } nit: Space to separate overrides. http://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:92: // NULL on failure. Caller takes ownership? http://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:104: BufferMap buffer_map_; What do we need this map for? We only insert and remove from it but never look it up. If PpbCdmBuffer held the Buffer_Dev as a member, then I don't think we'd need it or the ReleaseBuffer() method. http://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:312: output_buffer, The .buffer member (PpbCdmBuffer) will be leaked if this callback is not called. If we made OutputBuffer own it, we wouldn't need to worry about it. We also have the ReleaseBuffer issue, but at least that memory would be released when the instance was destroyed. It would be a plus if we could manage ownership of that. http://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:97: Buffer() {} Do we need this? It can't be constructed because it's abstract, so it's already essentially protected. The copy constructor might be problematic and require this to be declared. Anyway, it should be protected I think. http://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:328: // exception noted by the |ReleaseBuffer()| comment. Is that really an exception? ReleaseBuffer() frees it in the wrapper. http://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:335: // failure. The caller does not own the Buffer*. The caller does own the Buffer* but not the underlying buffer. http://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:339: // wrapper. Returns true when |buffer| is valid, and false otherwise. No return value now
Couldn't sleep/not feeling well. Thought I'd hack on this some more. I don't really like the way I'm passing the allocator around, and I can't build anything right now anyway (last sync seems to have broken my build). I think this will work, though. It just seems ugly. https://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): https://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:65: // Constructs a buffer object containing all information necessary for a On 2012/09/12 23:53:01, ddorwin wrote: > This wording is a bit odd, especially such it doesn't allocate any buffer. > Does it make sense for the atual PPAPI calls to occur in here? I guess that > depends on whether we copy these buffers around. Removed. There's only one constructor now, and hopefully the class comment suffices. https://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:74: PP_Resource id() const { return id_; } On 2012/09/12 23:53:01, ddorwin wrote: > nit: Space to separate overrides. Done. https://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:92: // NULL on failure. On 2012/09/12 23:53:01, ddorwin wrote: > Caller takes ownership? Done. https://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:104: BufferMap buffer_map_; On 2012/09/12 23:53:01, ddorwin wrote: > What do we need this map for? We only insert and remove from it but never look > it up. If PpbCdmBuffer held the Buffer_Dev as a member, then I don't think we'd > need it or the ReleaseBuffer() method. Removed. https://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:312: output_buffer, On 2012/09/12 23:53:01, ddorwin wrote: > The .buffer member (PpbCdmBuffer) will be leaked if this callback is not called. > If we made OutputBuffer own it, we wouldn't need to worry about it. > I've moved things around a little so that OutputBuffer can own the Buffer*. > We also have the ReleaseBuffer issue, but at least that memory would be released > when the instance was destroyed. It would be a plus if we could manage ownership > of that. I think I still need ReleaseBuffer... can't delete the cdm::Buffer* without seeing its implementation. https://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): https://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:97: Buffer() {} On 2012/09/12 23:53:01, ddorwin wrote: > Do we need this? It can't be constructed because it's abstract, so it's already > essentially protected. The copy constructor might be problematic and require > this to be declared. Anyway, it should be protected I think. Need it to construct PpbCdmBuffer, but I made it protected. https://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:328: // exception noted by the |ReleaseBuffer()| comment. On 2012/09/12 23:53:01, ddorwin wrote: > Is that really an exception? ReleaseBuffer() frees it in the wrapper. Done. https://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:335: // failure. The caller does not own the Buffer*. On 2012/09/12 23:53:01, ddorwin wrote: > The caller does own the Buffer* but not the underlying buffer. Done. https://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:339: // wrapper. Returns true when |buffer| is valid, and false otherwise. On 2012/09/12 23:53:01, ddorwin wrote: > No return value now Done.
Didn't follow the whole thread. Just a few comments when looking at the last patch. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:75: // DISALLOW_COPY_AND_ASSIGN. Make this a sentence? This comment will look strange w/o background in chromium codebase. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:80: class CdmAllocatorImpl : public cdm::Allocator { Since this is a PpbCdmBuffer specific Allocator impl, can we make the name more informative, like PpbCdmBufferAllocator? http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:228: // TODO(tomfinegan): Change DecoderBuffer so it can wrap a cdm::Buffer. DecoderBuffer is used extensively in src/media. I am not sure if we want to change it to remove this extra memcpy here. The ClearKeyCdm is for testing purpose only so I assume some kind of hacking is okay. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.h:34: cdm::KeyMessage* key_request) OVERRIDE; why change the format here? http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.h:105: // Allocates buffers using the CDM wrapper. The cdm should know nothing about CDM wrapper? http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:91: int32_t data_size; // Size (in bytes) of |data|. Thanks for the change to from uint32_t to int32_t. Could you please also update it for SubsampleEntry? http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:314: virtual void ReleaseBuffer(const Buffer* buffer) = 0; I am a little confused here. Can ReleaseBuffer's implementation not be "delete buffer"? It looks like "delete buffer" will trigger the cdm::Buffer's overrided dtor, which should be sufficient to destroy the cdm::Buffer's implementation and release any resources associated with it. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:328: delete[] session_id; What's the plan for session_id and default_url? For now, there should be a space between delete and []. On line 332, should it also be "delete [] default_url"?
The allocation stuff looks good, with some minor changes needed. However, KeyMessage and OutputBuffer are a problem for which I don't have a great answer. http://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/3021/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:312: output_buffer, On 2012/09/13 10:33:40, tomf wrote: > On 2012/09/12 23:53:01, ddorwin wrote: > > The .buffer member (PpbCdmBuffer) will be leaked if this callback is not > called. > > If we made OutputBuffer own it, we wouldn't need to worry about it. > > > > I've moved things around a little so that OutputBuffer can own the Buffer*. > > > We also have the ReleaseBuffer issue, but at least that memory would be > released > > when the instance was destroyed. It would be a plus if we could manage > ownership > > of that. > > I think I still need ReleaseBuffer... can't delete the cdm::Buffer* without > seeing its implementation. Correct, you need it. It could be deleted via the abstract class, but then the memory free would be on the wrong side of the so/dll boundary. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:58: // Provides access to memory owned by a pp::Buffer_Dev handle by storing the How about: Provides access to memory owned by a pp::Buffer_Dev created by CdmAllocatorImpl::Allocate(). This class holds a reference to the Buffer_Dev throughout its lifetime. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:63: virtual ~PpbCdmBuffer() {} To force ReleaseBuffer() to be called instead of deleting the pointer directly, this should be made private. Then, friend CdmAllocatorImpl so ReleaseBuffer() can delete it. You can even make the constructor private then. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:75: // DISALLOW_COPY_AND_ASSIGN. On 2012/09/13 14:35:45, xhwang wrote: > Make this a sentence? This comment will look strange w/o background in chromium > codebase. This is Chromium (not CDM) code, though. Is there a reason we can't use the macro? http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:80: class CdmAllocatorImpl : public cdm::Allocator { On 2012/09/13 14:35:45, xhwang wrote: > Since this is a PpbCdmBuffer specific Allocator impl, can we make the name more > informative, like PpbCdmBufferAllocator? I wonder if we should just drop "Cdm" from both class names as well. It's implicit in this file and separates the parts of the actual interface name (PPB_Buffer[_Dev]). http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:86: // Creates a PpbCdmBuffer* and returns it as a cdm::Buffer*. Returns NULL on How about: Allocates a pp::Buffer_Dev of the specified size and wraps it in a PpbCdmBuffer, which it returns. The caller own the returned buffer and must free it by calling ReleaseBuffer(). Returns NULL on failure. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:91: virtual void ReleaseBuffer(const cdm::Buffer* buffer) OVERRIDE { This and Allocate() should have consistent naming. Thus, we can probably remove "Buffer" from the name. no const. the buffer is being destroyed. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:92: delete buffer; Since no other functions are defined inline, we should probably just move this definition below as well. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:190: &key_request); Any new buffers assigned to key_request by the CDM must also be freed by the CDM. :( Alternatively, we could pre-allocate arrays for session ID, URL, etc. See comments in CDM.h. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:297: pp::Buffer_Dev message_buffer(pp::Buffer_Dev(cdm_buffer->id())); Would it be better to just expose the Buffer_Dev since the ID is only useful to create one? Another option might be some type of copy constructor that converts the PpbCdmBuffer to a BufferDev. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:335: PP_DCHECK(cdm_buffer); Remove or DCHECK after 296 too. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:155: memcpy(reinterpret_cast<void*>(key_request->message->buffer()), Are the casts really necessary? http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.h:105: // Allocates buffers using the CDM wrapper. On 2012/09/13 14:35:45, xhwang wrote: > The cdm should know nothing about CDM wrapper? Probably self-describing anyway since the type has a very explicit purpose. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.h:106: cdm::Allocator* allocator_; *const http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:282: // Encapsulates all data required to interact with buffers allocated by How about: Represents a buffer created by Alocator implementations. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:283: // Allocator implementations. Note that the buffer described by this object is Isn't the second sentence an implementation detail? It doesn't really matter since this object can only be destroyed by calling ReleaseBuffer(). http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:287: virtual ~Buffer() {} Make this protected for the reasons described in cdm_wrpper.cc. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:312: // Releases a Buffer that will not be delivered to the CDM wrapper, and sets Release a Buffer returned by Allocate(). The |buffer| pointer is no longer valid after calling this method. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:317: // Represents a key message sent by the CDM. It does not own any pointers in It does own the pointers now. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:320: KeyMessage(Allocator* allocator_) need a different name than ending with an underscore. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:328: delete[] session_id; On 2012/09/13 14:35:45, xhwang wrote: > What's the plan for session_id and default_url? As noted in cdm_wrapper.cc, they need to be allocated and freed on the same side. Thus, having the deletion in the destructor is probably not a good idea. One option might be to make this entire class abstract and have set_session_id(), etc. so the wrapper can make copies of those things and manage them. I guess another option is for everything to be allocated using Allocator. For this CL, let's just move the TODO about this not being safe here. I think this will be easier and clearer to deal with after we resolve the large buffers and close out this CL. > > For now, there should be a space between delete and []. > On line 332, should it also be "delete [] default_url"? http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:346: // Represents an output decrypted buffer. It does not own |data|. |data| does not exist. It appears that |buffer| is owned by this class now. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:358: Buffer* buffer; I wonder if we should have a ScopedBuffer that contains the Buffer and calls ReleaseBuffer() on it. Then we don't have to worry about getting all the destructors right. However, I wonder whether we should even have concrete structs in this file. Currently, it _could_ be created and destroyed on different sides of the SO/DLL boundary and it's up to the individual functions to make sure this doesn't happen.
http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:65: uint8_t* buffer() const OVERRIDE { I don't think this method should be const since the returned buffer is not. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:96: pp::Instance* instance_; *const http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:347: struct OutputBuffer { If we keep this struct, we should change the name since it's a "buffer" that contains a buffer. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:356: int64_t timestamp; // Presentation timestamp in microseconds. Do we really need a timestamp for Decrypt()? Since there is no decoding, shouldn't it be known by the client? (I see we set it in ClearKey, but hat might be an artifact of the reuse of AesDecryptor.) If not, maybe we can eliminate this struct.
ddorwin, dmicheal, xhwang, PTAL, I think this has changed enough to warrant another look. pp::CompletionCallback copies the args passed to the callback that it invokes. DISALLOW_COPY_AND_ASSIGN can be used to demonstrate-- using it in a class you intend to pass to the callback by ref will not compile. Anyway, as the patchset title says, KeyMessage and OutputBuffer are now interfaces. In addition: 1) Their respective Impl classes have been added to cdm_wrapper.cc. 2) CdmWrapper now passes the Impl classes around wrapped in tr1::shared_ptrs unhappy results when passing data to functions invoked via pp::CompletionCallback. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:58: // Provides access to memory owned by a pp::Buffer_Dev handle by storing the On 2012/09/13 19:47:00, ddorwin wrote: > How about: > Provides access to memory owned by a pp::Buffer_Dev created by > CdmAllocatorImpl::Allocate(). This class holds a reference to the Buffer_Dev > throughout its lifetime. Done. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:63: virtual ~PpbCdmBuffer() {} On 2012/09/13 19:47:00, ddorwin wrote: > To force ReleaseBuffer() to be called instead of deleting the pointer directly, > this should be made private. Then, friend CdmAllocatorImpl so ReleaseBuffer() > can delete it. You can even make the constructor private then. Done. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:65: uint8_t* buffer() const OVERRIDE { On 2012/09/14 00:20:39, ddorwin wrote: > I don't think this method should be const since the returned buffer is not. Done. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:75: // DISALLOW_COPY_AND_ASSIGN. On 2012/09/13 19:47:00, ddorwin wrote: > On 2012/09/13 14:35:45, xhwang wrote: > > Make this a sentence? This comment will look strange w/o background in > chromium > > codebase. > > This is Chromium (not CDM) code, though. Is there a reason we can't use the > macro? Done. I was avoiding bringing in base/basictypes.h just for DISALLOW_COPY_AND_ASSIGN. Using DISALLOW_COPY_AND_ASSIGN now. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:80: class CdmAllocatorImpl : public cdm::Allocator { On 2012/09/13 19:47:00, ddorwin wrote: > On 2012/09/13 14:35:45, xhwang wrote: > > Since this is a PpbCdmBuffer specific Allocator impl, can we make the name > more > > informative, like PpbCdmBufferAllocator? > > I wonder if we should just drop "Cdm" from both class names as well. It's > implicit in this file and separates the parts of the actual interface name > (PPB_Buffer[_Dev]). I think I've got both of these comments taken care of: We now have PpbBuffer and PpbBufferAllocator. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:86: // Creates a PpbCdmBuffer* and returns it as a cdm::Buffer*. Returns NULL on On 2012/09/13 19:47:00, ddorwin wrote: > How about: > Allocates a pp::Buffer_Dev of the specified size and wraps it in a PpbCdmBuffer, > which it returns. The caller own the returned buffer and must free it by calling > ReleaseBuffer(). Returns NULL on failure. Done. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:91: virtual void ReleaseBuffer(const cdm::Buffer* buffer) OVERRIDE { On 2012/09/13 19:47:00, ddorwin wrote: > This and Allocate() should have consistent naming. Thus, we can probably remove > "Buffer" from the name. > no const. the buffer is being destroyed. Done. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:92: delete buffer; On 2012/09/13 19:47:00, ddorwin wrote: > Since no other functions are defined inline, we should probably just move this > definition below as well. Done. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:96: pp::Instance* instance_; On 2012/09/14 00:20:39, ddorwin wrote: > *const Done. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:190: &key_request); On 2012/09/13 19:47:00, ddorwin wrote: > Any new buffers assigned to key_request by the CDM must also be freed by the > CDM. :( Alternatively, we could pre-allocate arrays for session ID, URL, etc. > See comments in CDM.h. Had a bit of fun with this today/tonight. pp::CompletionCallback makes a copy-- you can verify by adding DISALLOW_COPY_AND_ASSIGN to KeyMessage or OutputBuffer. We now have tr1::shared_ptr abuse. Works, but I need to read over the offline conversation about the allocation code now that this works, and is relatively clean again. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:297: pp::Buffer_Dev message_buffer(pp::Buffer_Dev(cdm_buffer->id())); On 2012/09/13 19:47:00, ddorwin wrote: > Would it be better to just expose the Buffer_Dev since the ID is only useful to > create one? Another option might be some type of copy constructor that converts > the PpbCdmBuffer to a BufferDev. Done. I added buffer_dev() http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:335: PP_DCHECK(cdm_buffer); On 2012/09/13 19:47:00, ddorwin wrote: > Remove or DCHECK after 296 too. Done. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.cc (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:155: memcpy(reinterpret_cast<void*>(key_request->message->buffer()), On 2012/09/13 19:47:00, ddorwin wrote: > Are the casts really necessary? clang doesn't like implicitly casting from uint8* to void*. MSVC does. ;( http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.cc:228: // TODO(tomfinegan): Change DecoderBuffer so it can wrap a cdm::Buffer. On 2012/09/13 14:35:45, xhwang wrote: > DecoderBuffer is used extensively in src/media. I am not sure if we want to > change it to remove this extra memcpy here. The ClearKeyCdm is for testing > purpose only so I assume some kind of hacking is okay. TODO removed, one down, yay! ;) http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.h:34: cdm::KeyMessage* key_request) OVERRIDE; On 2012/09/13 14:35:45, xhwang wrote: > why change the format here? The ';' was in column 81 (unless my guide is set to 79 on one of my machines...) http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.h:105: // Allocates buffers using the CDM wrapper. On 2012/09/13 19:47:00, ddorwin wrote: > On 2012/09/13 14:35:45, xhwang wrote: > > The cdm should know nothing about CDM wrapper? > > Probably self-describing anyway since the type has a very explicit purpose. Removed comment. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/clear_key_cdm.h:106: cdm::Allocator* allocator_; On 2012/09/13 19:47:00, ddorwin wrote: > *const Done. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:91: int32_t data_size; // Size (in bytes) of |data|. On 2012/09/13 14:35:45, xhwang wrote: > Thanks for the change to from uint32_t to int32_t. Could you please also update > it for SubsampleEntry? Done. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:282: // Encapsulates all data required to interact with buffers allocated by On 2012/09/13 19:47:00, ddorwin wrote: > How about: > Represents a buffer created by Alocator implementations. Done. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:287: virtual ~Buffer() {} On 2012/09/13 19:47:00, ddorwin wrote: > Make this protected for the reasons described in cdm_wrpper.cc. Done. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:312: // Releases a Buffer that will not be delivered to the CDM wrapper, and sets On 2012/09/13 19:47:00, ddorwin wrote: > Release a Buffer returned by Allocate(). The |buffer| pointer is no longer valid > after calling this method. Done. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:314: virtual void ReleaseBuffer(const Buffer* buffer) = 0; On 2012/09/13 14:35:45, xhwang wrote: > I am a little confused here. Can ReleaseBuffer's implementation not be "delete > buffer"? It looks like "delete buffer" will trigger the cdm::Buffer's overrided > dtor, which should be sufficient to destroy the cdm::Buffer's implementation and > release any resources associated with it. Two things: 1) Deleting memory allocated in another SO probably isn't a good idea. 2) deleting a base class pointer when the derived class declaration isn't visible yields undefined behavior. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:317: // Represents a key message sent by the CDM. It does not own any pointers in On 2012/09/13 19:47:00, ddorwin wrote: > It does own the pointers now. This is now an interface. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:320: KeyMessage(Allocator* allocator_) On 2012/09/13 19:47:00, ddorwin wrote: > need a different name than ending with an underscore. Removed. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:346: // Represents an output decrypted buffer. It does not own |data|. On 2012/09/13 19:47:00, ddorwin wrote: > |data| does not exist. > It appears that |buffer| is owned by this class now. Ditto, now an interface. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:347: struct OutputBuffer { On 2012/09/14 00:20:39, ddorwin wrote: > If we keep this struct, we should change the name since it's a "buffer" that > contains a buffer. Haven't renamed it yet... maybe I'm overly fond of the word wrapper, but OutputBufferWrapper makes sense. Maybe I'll think of something better Saturday. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:356: int64_t timestamp; // Presentation timestamp in microseconds. On 2012/09/14 00:20:39, ddorwin wrote: > Do we really need a timestamp for Decrypt()? Since there is no decoding, > shouldn't it be known by the client? (I see we set it in ClearKey, but hat might > be an artifact of the reuse of AesDecryptor.) > > If not, maybe we can eliminate this struct. I'll look into this some time Saturday. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:358: Buffer* buffer; On 2012/09/13 19:47:00, ddorwin wrote: > I wonder if we should have a ScopedBuffer that contains the Buffer and calls > ReleaseBuffer() on it. Then we don't have to worry about getting all the > destructors right. > > However, I wonder whether we should even have concrete structs in this file. > Currently, it _could_ be created and destroyed on different sides of the SO/DLL > boundary and it's up to the individual functions to make sure this doesn't > happen. After changing KeyMessage and OutputBuffer to interfaces, and using tr1::shared_ptr in the wrapper I think we're doing an adequate job of managing this-- what do you think?
Thanks. I think we have something that will work. We can learn from use and revisit it later. There are a few things to address within the current design. http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/13009/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:314: virtual void ReleaseBuffer(const Buffer* buffer) = 0; On 2012/09/15 08:03:14, tomf wrote: > On 2012/09/13 14:35:45, xhwang wrote: > > I am a little confused here. Can ReleaseBuffer's implementation not be "delete > > buffer"? It looks like "delete buffer" will trigger the cdm::Buffer's > overrided > > dtor, which should be sufficient to destroy the cdm::Buffer's implementation > and > > release any resources associated with it. > > Two things: > 1) Deleting memory allocated in another SO probably isn't a good idea. > 2) deleting a base class pointer when the derived class declaration isn't > visible yields undefined behavior. Also, the PpbBuffer may not actually be deleted since it is ref counted. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:100: virtual void Release(const cdm::Buffer* buffer) OVERRIDE; I don't think the parameter should be const since it is being destroyed. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:165: bool KeyMessageImpl::set_session_id(const char* session_id, int32_t length) { The return type for simple setters, such as set_var(), is void. If we need more logic, it should be SetVar(). I suggest just DCHECKing. CDMs that pass invalid values will crash or have other problems in production. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:166: if (!session_id || length < 0) Is 0 okay? http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:174: return session_id_.data(); c_str() would probably be safer (null-terminated). http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:194: if (!default_url || length < 0) 0 okay? http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:202: return default_url_.data(); c_str() http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:431: PP_DCHECK(status == cdm::kSuccess); Handle errors including invalid buffer - similar to KeyMessage code. (DeliverBlock() assumes it's valid.) http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:455: std::string(key_message->session_id(), SharedKeyMessage can use the Impl type. In that case, you can use the actual string from the Impl instead of recreating here. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:107: cdm::Allocator* allocator_; *const http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:285: virtual ~Buffer() {} protected. Don't let the CDM delete Buffers. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:302: Allocator() {} protected for these two. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:310: // Release a Buffer returned by Allocate(). The |buffer| pointer is no longer Correcting myself: I think we should just say "|buffer| is no..." http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:341: virtual int64_t timestamp() const = 0; TODO: Figure out if we need timestamp() here. If not, just replace use with Buffer.
I think I've addressed all comments, but I have to run off and sign a lease. This works, but I'm concerned about comments regarding thread safety in linked_ptr.h. PTAL. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:100: virtual void Release(const cdm::Buffer* buffer) OVERRIDE; On 2012/09/17 21:19:23, ddorwin wrote: > I don't think the parameter should be const since it is being destroyed. Done. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:165: bool KeyMessageImpl::set_session_id(const char* session_id, int32_t length) { On 2012/09/17 21:19:23, ddorwin wrote: > The return type for simple setters, such as set_var(), is void. If we need more > logic, it should be SetVar(). > I suggest just DCHECKing. CDMs that pass invalid values will crash or have other > problems in production. Done, but did not add DCHECKs. If the CDM wants to clear a field, maybe it really needs that...? http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:166: if (!session_id || length < 0) On 2012/09/17 21:19:23, ddorwin wrote: > Is 0 okay? Experience and some quick tests of std::string::assign in combination w/c_str say yes. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:174: return session_id_.data(); On 2012/09/17 21:19:23, ddorwin wrote: > c_str() would probably be safer (null-terminated). Done. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:194: if (!default_url || length < 0) On 2012/09/17 21:19:23, ddorwin wrote: > 0 okay? Done. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:202: return default_url_.data(); On 2012/09/17 21:19:23, ddorwin wrote: > c_str() Done. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:431: PP_DCHECK(status == cdm::kSuccess); On 2012/09/17 21:19:23, ddorwin wrote: > Handle errors including invalid buffer - similar to KeyMessage code. > (DeliverBlock() assumes it's valid.) I added a PP_DCHECK(output_buffer->buffer()). Should I remove the two DCHECKs, and send a KeyError instead? http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:455: std::string(key_message->session_id(), On 2012/09/17 21:19:23, ddorwin wrote: > SharedKeyMessage can use the Impl type. In that case, you can use the actual > string from the Impl instead of recreating here. Done. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/clear_key_cdm.h (right): http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/clear_key_cdm.h:107: cdm::Allocator* allocator_; On 2012/09/17 21:19:23, ddorwin wrote: > *const Done. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:285: virtual ~Buffer() {} On 2012/09/17 21:19:23, ddorwin wrote: > protected. > Don't let the CDM delete Buffers. Done. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:302: Allocator() {} On 2012/09/17 21:19:23, ddorwin wrote: > protected for these two. Done. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:310: // Release a Buffer returned by Allocate(). The |buffer| pointer is no longer On 2012/09/17 21:19:23, ddorwin wrote: > Correcting myself: I think we should just say "|buffer| is no..." Done. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/content_decryption_module.h:341: virtual int64_t timestamp() const = 0; On 2012/09/17 21:19:23, ddorwin wrote: > TODO: Figure out if we need timestamp() here. If not, just replace use with > Buffer. Done.
Thanks. Almost there. http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/9017/webkit/media/crypto/ppapi/c... webkit/media/crypto/ppapi/cdm_wrapper.cc:431: PP_DCHECK(status == cdm::kSuccess); On 2012/09/18 01:08:02, tomf wrote: > On 2012/09/17 21:19:23, ddorwin wrote: > > Handle errors including invalid buffer - similar to KeyMessage code. > > (DeliverBlock() assumes it's valid.) > > I added a PP_DCHECK(output_buffer->buffer()). Should I remove the two DCHECKs, > and send a KeyError instead? Yes. http://codereview.chromium.org/10914028/diff/17001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/17001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:291: delete static_cast<PpbBuffer*>(buffer); Why is this cast necessary? The destructor is virtual. Is it because Buffer's destructor is now private? Hmm. How about a pure virtual Destroy()/Release() method on Buffer. That would move the deletion to the wrapper code. We probably don't even need this method then. http://codereview.chromium.org/10914028/diff/17001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:413: PP_DCHECK(status == cdm::kSuccess); We need to handle these errors. I think we should copy the code at 323.
http://codereview.chromium.org/10914028/diff/17001/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/17001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:291: delete static_cast<PpbBuffer*>(buffer); On 2012/09/18 01:26:08, ddorwin wrote: > Why is this cast necessary? The destructor is virtual. Is it because Buffer's > destructor is now private? > Hmm. How about a pure virtual Destroy()/Release() method on Buffer. That would > move the deletion to the wrapper code. We probably don't even need this method > then. Release is gone, now we have Buffer::Destroy. http://codereview.chromium.org/10914028/diff/17001/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:413: PP_DCHECK(status == cdm::kSuccess); On 2012/09/18 01:26:08, ddorwin wrote: > We need to handle these errors. I think we should copy the code at 323. Done.
lgtm assuming these changes are made. Thanks for persevering! http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:67: void Destroy() OVERRIDE { delete this; } These 3 should be virtual. http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:285: // Destroys a buffer that will not be passed to the CDM wrapper. "that..." probably isn't correct since this is used inside the wrapper too. How about "Destroys the buffer in the same context as it was created" or something like that? http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:301: // Allocated buffers should only be freed by the CDM wrapper. I think we can just eliminate this since we enforce it now. http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/linked_ptr.h (right): http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/linked_ptr.h:124: PP_DCHECK(last); adding (void)last; should fix the clang error for rel builds.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/10914028/24007
http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/cdm_wrapper.cc (right): http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/cdm_wrapper.cc:67: void Destroy() OVERRIDE { delete this; } On 2012/09/18 04:53:14, ddorwin wrote: > These 3 should be virtual. Done. Ugh.. can't believe I didn't notice that. http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/content_decryption_module.h (right): http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:285: // Destroys a buffer that will not be passed to the CDM wrapper. On 2012/09/18 04:53:14, ddorwin wrote: > "that..." probably isn't correct since this is used inside the wrapper too. > How about "Destroys the buffer in the same context as it was created" or > something like that? Done. http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/content_decryption_module.h:301: // Allocated buffers should only be freed by the CDM wrapper. On 2012/09/18 04:53:14, ddorwin wrote: > I think we can just eliminate this since we enforce it now. Done. http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... File webkit/media/crypto/ppapi/linked_ptr.h (right): http://codereview.chromium.org/10914028/diff/21002/webkit/media/crypto/ppapi/... webkit/media/crypto/ppapi/linked_ptr.h:124: PP_DCHECK(last); On 2012/09/18 04:53:14, ddorwin wrote: > adding > (void)last; > should fix the clang error for rel builds. Replaced the PP_DCHECK, and used your suggestion. My knee jerk reaction was to remove it. ;)
Failed to apply patch for webkit/media/crypto/ppapi/content_decryption_module.h: While running patch -p1 --forward --force; patching file webkit/media/crypto/ppapi/content_decryption_module.h Hunk #4 FAILED at 88. Hunk #5 succeeded at 287 (offset -5 lines). 1 out of 5 hunks FAILED -- saving rejects to file webkit/media/crypto/ppapi/content_decryption_module.h.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tomfinegan@chromium.org/10914028/21003
Change committed as 157406 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
