Add CDM allocator interface.

webkit/media/crypto/ppapi/cdm_wrapper.cc

Index: webkit/media/crypto/ppapi/cdm_wrapper.cc
diff --git a/webkit/media/crypto/ppapi/cdm_wrapper.cc b/webkit/media/crypto/ppapi/cdm_wrapper.cc
index 8a8cb37d26fdb0b469246563086f29c8bc428b16..25daf8fdcc4875def4b73ccaac5f088b7b607469 100644
--- a/webkit/media/crypto/ppapi/cdm_wrapper.cc
+++ b/webkit/media/crypto/ppapi/cdm_wrapper.cc
@@ -2,10 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <cstring>  // For memcpy.
+#include <cstring>
 #include <vector>
 
-#include "base/compiler_specific.h"  // For OVERRIDE.
+#include "base/compiler_specific.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/c/pp_stdint.h"
 #include "ppapi/c/private/pp_content_decryptor.h"
@@ -55,6 +55,47 @@ void CallOnMain(pp::CompletionCallback cb) {
 
 namespace webkit_media {
 
+// Provides access to memory owned by a pp::Buffer_Dev handle by storing the

[ddorwin, 2012/09/13 19:47:00]

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.

[Tom Finegan, 2012/09/15 08:03:14]

Done.

+// buffer instance for callers of CdmAllocatorImpl::Allocate.
+class PpbCdmBuffer : public cdm::Buffer {
+ public:
+  explicit PpbCdmBuffer(pp::Buffer_Dev buffer) : buffer_(buffer) {}
+  virtual ~PpbCdmBuffer() {}

[ddorwin, 2012/09/13 19:47:00]

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.

[Tom Finegan, 2012/09/15 08:03:14]

Done.

+
+  uint8_t* buffer() const OVERRIDE {

[ddorwin, 2012/09/14 00:20:39]

I don't think this method should be const since the returned buffer is not.

[Tom Finegan, 2012/09/15 08:03:14]

Done.

+    return static_cast<uint8_t*>(buffer_.data());
+  }
+  int32_t size() const OVERRIDE { return buffer_.size(); }
+
+  PP_Resource id() const { return buffer_.pp_resource(); }
+
+ private:
+  pp::Buffer_Dev buffer_;
+
+  // DISALLOW_COPY_AND_ASSIGN.

[xhwang, 2012/09/13 14:35:45]

Make this a sentence? This comment will look strange w/o background in chromium
codebase.

[ddorwin, 2012/09/13 19:47:00]

This is Chromium (not CDM) code, though. Is there a reason we can't use the
macro? 

[Tom Finegan, 2012/09/15 08:03:14]

Done. I was avoiding bringing in base/basictypes.h just for
DISALLOW_COPY_AND_ASSIGN. Using DISALLOW_COPY_AND_ASSIGN now.

+  PpbCdmBuffer(const PpbCdmBuffer&);
+  void operator=(const PpbCdmBuffer&);
+};
+
+class CdmAllocatorImpl : public cdm::Allocator {

[xhwang, 2012/09/13 14:35:45]

Since this is a PpbCdmBuffer specific Allocator impl, can we make the name more
informative, like PpbCdmBufferAllocator?

[ddorwin, 2012/09/13 19:47:00]

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]).

[Tom Finegan, 2012/09/15 08:03:14]

I think I've got both of these comments taken care of: We now have PpbBuffer and
PpbBufferAllocator.

+ public:
+  explicit CdmAllocatorImpl(pp::Instance* instance);
+  virtual ~CdmAllocatorImpl();
+
+  // CdmAllocator methods.
+  // Creates a PpbCdmBuffer* and returns it as a cdm::Buffer*. Returns NULL on

[ddorwin, 2012/09/13 19:47:00]

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.

[Tom Finegan, 2012/09/15 08:03:14]

Done.

+  // failure. Caller owns the buffer upon success.
+  virtual cdm::Buffer* Allocate(int32_t size) OVERRIDE;
+
+  // Deletes the cdm::Buffer*.
+  virtual void ReleaseBuffer(const cdm::Buffer* buffer) OVERRIDE {

[ddorwin, 2012/09/13 19:47:00]

This and Allocate() should have consistent naming. Thus, we can probably remove
"Buffer" from the name.
no const. the buffer is being destroyed.

[Tom Finegan, 2012/09/15 08:03:14]

Done.

+    delete buffer;

[ddorwin, 2012/09/13 19:47:00]

Since no other functions are defined inline, we should probably just move this
definition below as well.

[Tom Finegan, 2012/09/15 08:03:14]

Done.

+  }
+
+ private:
+  pp::Instance* instance_;

[ddorwin, 2012/09/14 00:20:39]

*const

[Tom Finegan, 2012/09/15 08:03:14]

Done.

+};
+
 // A wrapper class for abstracting away PPAPI interaction and threading for a
 // Content Decryption Module (CDM).
 class CdmWrapper : public pp::Instance,
@@ -62,7 +103,6 @@ class CdmWrapper : public pp::Instance,
  public:
   CdmWrapper(PP_Instance instance, pp::Module* module);
   virtual ~CdmWrapper();
-
   virtual bool Init(uint32_t argc, const char* argn[], const char* argv[]) {
     return true;
   }
@@ -86,12 +126,6 @@ class CdmWrapper : public pp::Instance,
       const PP_EncryptedBlockInfo& encrypted_block_info) OVERRIDE;
 
  private:
-  // Creates a PP_Resource containing a PPB_Buffer_Impl, copies |data| into the
-  // buffer resource, and returns it. Returns a an invalid PP_Resource with an
-  // ID of 0 on failure. Upon success, the returned Buffer resource has a
-  // reference count of 1.
-  pp::Buffer_Dev MakeBufferResource(const uint8_t* data, uint32_t data_size);
-
   // <code>PPB_ContentDecryptor_Private</code> dispatchers. These are passed to
   // <code>callback_factory_</code> to ensure that calls into
   // <code>PPP_ContentDecryptor_Private</code> are asynchronous.
@@ -103,14 +137,33 @@ class CdmWrapper : public pp::Instance,
                     cdm::OutputBuffer& output_buffer,
                     const PP_DecryptTrackingInfo& tracking_info);
 
+  CdmAllocatorImpl allocator_;
   pp::CompletionCallbackFactory<CdmWrapper> callback_factory_;
   cdm::ContentDecryptionModule* cdm_;
   std::string key_system_;
 };
 
+CdmAllocatorImpl::CdmAllocatorImpl(pp::Instance* instance)
+    : instance_(instance) {
+}
+
+CdmAllocatorImpl::~CdmAllocatorImpl() {
+}
+
+cdm::Buffer* CdmAllocatorImpl::Allocate(int32_t size) {
+  PP_DCHECK(size > 0);
+
+  pp::Buffer_Dev buffer(instance_, size);
+  if (buffer.is_null())
+    return NULL;
+
+  return new PpbCdmBuffer(buffer);
+}
+
 CdmWrapper::CdmWrapper(PP_Instance instance, pp::Module* module)
     : pp::Instance(instance),
       pp::ContentDecryptor_Private(this),
+      allocator_(this),
       cdm_(NULL) {
   callback_factory_.Initialize(this);
 }
@@ -125,12 +178,12 @@ void CdmWrapper::GenerateKeyRequest(const std::string& key_system,
   PP_DCHECK(!key_system.empty());
 
   if (!cdm_) {
-    cdm_ = CreateCdmInstance();
+    cdm_ = CreateCdmInstance(&allocator_);
     if (!cdm_)
       return;
   }
 
-  cdm::KeyMessage key_request;
+  cdm::KeyMessage key_request(&allocator_);
   cdm::Status status = cdm_->GenerateKeyRequest(
       reinterpret_cast<const uint8_t*>(init_data.Map()),
       init_data.ByteLength(),
@@ -138,7 +191,7 @@ void CdmWrapper::GenerateKeyRequest(const std::string& key_system,
 
   if (status != cdm::kSuccess ||
       !key_request.message ||
-      key_request.message_size == 0) {
+      key_request.message->size() == 0) {
     CallOnMain(callback_factory_.NewCallback(&CdmWrapper::KeyError,
                                              std::string()));
     return;
@@ -217,8 +270,9 @@ void CdmWrapper::Decrypt(pp::Buffer_Dev encrypted_buffer,
   input_buffer.subsamples = &subsamples[0];
   input_buffer.timestamp = encrypted_block_info.tracking_info.timestamp;
 
-  cdm::OutputBuffer output_buffer;
+  cdm::OutputBuffer output_buffer(&allocator_);
   cdm::Status status = cdm_->Decrypt(input_buffer, &output_buffer);
+  PP_DCHECK(status == cdm::kSuccess);
 
   CallOnMain(callback_factory_.NewCallback(
       &CdmWrapper::DeliverBlock,
@@ -232,41 +286,20 @@ void CdmWrapper::DecryptAndDecode(
     const PP_EncryptedBlockInfo& encrypted_block_info) {
 }
 
-pp::Buffer_Dev CdmWrapper::MakeBufferResource(const uint8_t* data,
-                                              uint32_t data_size) {
-  if (!data || !data_size)
-    return pp::Buffer_Dev();
-
-  pp::Buffer_Dev buffer(this, data_size);
-  if (!buffer.data())
-    return pp::Buffer_Dev();
-
-  memcpy(buffer.data(), data, data_size);
-  return buffer;
-}
-
 void CdmWrapper::KeyAdded(int32_t result, const std::string& session_id) {
   pp::ContentDecryptor_Private::KeyAdded(key_system_, session_id);
 }
 
 void CdmWrapper::KeyMessage(int32_t result,
                             cdm::KeyMessage& key_message) {
-  pp::Buffer_Dev message_buffer(MakeBufferResource(key_message.message,
-                                                   key_message.message_size));
+  PpbCdmBuffer* cdm_buffer =
+      static_cast<PpbCdmBuffer*>(key_message.message);
+  pp::Buffer_Dev message_buffer(pp::Buffer_Dev(cdm_buffer->id()));

[ddorwin, 2012/09/13 19:47:00]

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.

[Tom Finegan, 2012/09/15 08:03:14]

Done. I added buffer_dev()

   pp::ContentDecryptor_Private::KeyMessage(
       key_system_,
       std::string(key_message.session_id, key_message.session_id_size),
       message_buffer,
       std::string(key_message.default_url, key_message.default_url_size));
-
-  // TODO(xhwang): Fix this. This is not always safe as the memory is allocated
-  // in another shared object.
-  delete [] key_message.session_id;
-  key_message.session_id = NULL;
-  delete [] key_message.message;
-  key_message.message = NULL;
-  delete [] key_message.default_url;
-  key_message.default_url = NULL;
 }
 
 // TODO(xhwang): Support MediaKeyError (see spec: http://goo.gl/rbdnR) in CDM
@@ -282,8 +315,6 @@ void CdmWrapper::DeliverBlock(int32_t result,
                               const cdm::Status& status,
                               cdm::OutputBuffer& output_buffer,
                               const PP_DecryptTrackingInfo& tracking_info) {
-  pp::Buffer_Dev decrypted_buffer(MakeBufferResource(output_buffer.data,
-                                                     output_buffer.data_size));
   PP_DecryptedBlockInfo decrypted_block_info;
   decrypted_block_info.tracking_info.request_id = tracking_info.request_id;
   decrypted_block_info.tracking_info.timestamp = output_buffer.timestamp;
@@ -299,21 +330,21 @@ void CdmWrapper::DeliverBlock(int32_t result,
       decrypted_block_info.result = PP_DECRYPTRESULT_DECRYPT_ERROR;
   }
 
-  pp::ContentDecryptor_Private::DeliverBlock(decrypted_buffer,
-                                             decrypted_block_info);
+  const PpbCdmBuffer* const cdm_buffer =
+      static_cast<PpbCdmBuffer*>(output_buffer.buffer);
+  PP_DCHECK(cdm_buffer);

[ddorwin, 2012/09/13 19:47:00]

Remove or DCHECK after 296 too.

[Tom Finegan, 2012/09/15 08:03:14]

Done.

 
-  // TODO(xhwang): Fix this. This is not always safe as the memory is allocated
-  // in another shared object.
-  delete [] output_buffer.data;
-  output_buffer.data = NULL;
+  pp::ContentDecryptor_Private::DeliverBlock(
+      pp::Buffer_Dev(cdm_buffer->id()),
+      decrypted_block_info);
 }
 
 // This object is the global object representing this plugin library as long
 // as it is loaded.
-class MyModule : public pp::Module {
+class CdmWrapperModule : public pp::Module {
  public:
-  MyModule() : pp::Module() {}
-  virtual ~MyModule() {}
+  CdmWrapperModule() : pp::Module() {}
+  virtual ~CdmWrapperModule() {}
 
   virtual pp::Instance* CreateInstance(PP_Instance instance) {
     return new CdmWrapper(instance, this);
@@ -326,7 +357,7 @@ namespace pp {
 
 // Factory function for your specialization of the Module object.
 Module* CreateModule() {
-  return new webkit_media::MyModule();
+  return new webkit_media::CdmWrapperModule();
 }
 
 }  // namespace pp