Add CDM allocator interface.

webkit/media/crypto/ppapi/cdm_wrapper.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // 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"
 #include "ppapi/cpp/completion_callback.h"
 #include "ppapi/cpp/core.h"
 #include "ppapi/cpp/instance.h"
 #include "ppapi/cpp/logging.h"
 #include "ppapi/cpp/module.h"
 #include "ppapi/cpp/pass_ref.h"
 #include "ppapi/cpp/resource.h"
@@ skipping 29 matching lines @@
   if (IsMainThread())
     cb.Run(PP_OK);
   else
     pp::Module::Get()->core()->CallOnMainThread(0, cb, PP_OK);
 }
 
 }  // namespace
 
 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,
                    public pp::ContentDecryptor_Private {
  public:
   CdmWrapper(PP_Instance instance, pp::Module* module);
   virtual ~CdmWrapper();
-
   virtual bool Init(uint32_t argc, const char* argn[], const char* argv[]) {
     return true;
   }
 
   // PPP_ContentDecryptor_Private methods
   // Note: As per comments in PPP_ContentDecryptor_Private, these calls should
   // return false if the call was not forwarded to the CDM and should return
   // true otherwise. Once the call reaches the CDM, the call result/status
   // should be reported through the PPB_ContentDecryptor_Private interface.
   virtual void GenerateKeyRequest(const std::string& key_system,
                                   pp::VarArrayBuffer init_data) OVERRIDE;
   virtual void AddKey(const std::string& session_id,
                       pp::VarArrayBuffer key,
                       pp::VarArrayBuffer init_data) OVERRIDE;
   virtual void CancelKeyRequest(const std::string& session_id) OVERRIDE;
   virtual void Decrypt(
       pp::Buffer_Dev encrypted_buffer,
       const PP_EncryptedBlockInfo& encrypted_block_info) OVERRIDE;
   virtual void DecryptAndDecode(
       pp::Buffer_Dev encrypted_buffer,
       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.
   void KeyAdded(int32_t result, const std::string& session_id);
   void KeyMessage(int32_t result, cdm::KeyMessage& key_message);
   void KeyError(int32_t result, const std::string& session_id);
   void DeliverBlock(int32_t result,
                     const cdm::Status& status,
                     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);
 }
 
 CdmWrapper::~CdmWrapper() {
   if (cdm_)
     DestroyCdmInstance(cdm_);
 }
 
 void CdmWrapper::GenerateKeyRequest(const std::string& key_system,
                                     pp::VarArrayBuffer init_data) {
   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(),
       &key_request);

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

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.

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

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.

 
   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;
   }
 
   // TODO(xhwang): Remove unnecessary CallOnMain calls here and below once we
   // only support out-of-process.
   // If running out-of-process, PPB calls will always behave asynchronously
   // since IPC is involved. In that case, if we are already on main thread,
   // we don't need to use CallOnMain to help us call PPB call on main thread,
@@ skipping 58 matching lines @@
   input_buffer.num_subsamples = encrypted_block_info.num_subsamples;
   std::vector<cdm::SubsampleEntry> subsamples;
   for (uint32_t i = 0; i < encrypted_block_info.num_subsamples; ++i) {
     subsamples.push_back(cdm::SubsampleEntry(
         encrypted_block_info.subsamples[i].clear_bytes,
         encrypted_block_info.subsamples[i].cipher_bytes));
   }
   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,
       status,
       output_buffer,
       encrypted_block_info.tracking_info));
 }
 
 void CdmWrapper::DecryptAndDecode(
     pp::Buffer_Dev encrypted_buffer,
     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
 // interface and in this function.
 void CdmWrapper::KeyError(int32_t result, const std::string& session_id) {
   pp::ContentDecryptor_Private::KeyError(key_system_,
                                          session_id,
                                          kUnknownError,
                                          0);
 }
 
 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;
 
   switch (status) {
     case cdm::kSuccess:
       decrypted_block_info.result = PP_DECRYPTRESULT_SUCCESS;
       break;
     case cdm::kNoKey:
       decrypted_block_info.result = PP_DECRYPTRESULT_DECRYPT_NOKEY;
       break;
     default:
       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);
   }
 };
 
 }  // namespace webkit_media
 
 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