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 63078563da7af8d630fa46df8bb90e08ee8679c2..a97e59e2a98a8afe1d576183ad658f14e67a7cf6 100644
--- a/webkit/media/crypto/ppapi/cdm_wrapper.cc
+++ b/webkit/media/crypto/ppapi/cdm_wrapper.cc
@@ -3,11 +3,16 @@
 // found in the LICENSE file.
 
 #include <cstring>  // For memcpy.
+#include <queue>

[ddorwin, 2012/09/07 09:48:48]

not used

[Tom Finegan, 2012/09/08 01:02:47]

Done.

 #include <vector>
 
 #include "base/compiler_specific.h"  // For OVERRIDE.
+#include "base/hash_tables.h"
+#include "base/logging.h"
+#include "base/stl_util.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/c/pp_stdint.h"
+#include "ppapi/c/dev/ppb_buffer_dev.h"

[ddorwin, 2012/09/07 09:48:48]

Remove

[Tom Finegan, 2012/09/08 01:02:47]

Done.

 #include "ppapi/c/private/pp_content_decryptor.h"
 #include "ppapi/cpp/completion_callback.h"
 #include "ppapi/cpp/core.h"
@@ -55,6 +60,28 @@ void CallOnMain(pp::CompletionCallback cb) {
 
 namespace webkit_media {
 
+class CdmAllocatorImpl : public cdm::CdmAllocator {
+ public:
+  explicit CdmAllocatorImpl(pp::Instance* instance);
+  virtual ~CdmAllocatorImpl();
+
+  // CdmAllocator methods.
+  // Creates a pp::Buffer_Dev, stores it in |buffer_map_|, and returns the
+  // buffer identifier, pointer, and size wrapped in a CdmBuffer. Upon success,
+  // the returned CdmBuffer will contain non-zero buffer pointer, identifier,
+  // and size values. Otherwise, all values will be 0.
+  virtual cdm::CdmBuffer Allocate(int32_t size) OVERRIDE;
+
+  // Relinquishes buffer ownership by removing it from |buffer_map_|.
+  virtual bool ReleaseBuffer(const cdm::CdmBuffer& buffer) OVERRIDE;
+
+ private:
+  typedef base::hash_map<int32_t, pp::Buffer_Dev> BufferMap;

[ddorwin, 2012/09/07 09:48:48]

Why not use the PP_Resource type?
Inserting will copy the Buffer_Dev. Is this what we want?

[dmichael (off chromium), 2012/09/07 16:43:43]

pp::Buffer_Dev is only a handle to the buffer. Copying just copies the handle,
not the actual underlying buffer.

[Tom Finegan, 2012/09/08 01:02:47]

Changed int32_t in the typedef to PP_Resource for clarity.

+
+  pp::Instance* instance_;
+  BufferMap buffer_map_;
+};
+
 // A wrapper class for abstracting away PPAPI interaction and threading for a
 // Content Decryption Module (CDM).
 class CdmWrapper : public pp::Instance,
@@ -62,7 +89,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;
   }
@@ -103,14 +129,52 @@ class CdmWrapper : public pp::Instance,
                     cdm::OutputBuffer& output_buffer,
                     const PP_DecryptTrackingInfo& tracking_info);
 
+  CdmAllocatorImpl cdm_allocator_;
   pp::CompletionCallbackFactory<CdmWrapper> callback_factory_;
   cdm::ContentDecryptionModule* cdm_;
   std::string key_system_;
 };
 
+CdmAllocatorImpl::CdmAllocatorImpl(pp::Instance* instance)
+    : instance_(instance) {
+}
+
+CdmAllocatorImpl::~CdmAllocatorImpl() {
+}
+
+cdm::CdmBuffer CdmAllocatorImpl::Allocate(int32_t size) {
+  PP_DCHECK(size > 0);
+
+  pp::Buffer_Dev buffer(instance_, size);
+  if (buffer.is_null())
+    return cdm::CdmBuffer();
+
+  if (ContainsKey(buffer_map_, buffer.pp_resource()))
+    return cdm::CdmBuffer();
+
+  typedef std::pair<int32_t, pp::Buffer_Dev> BufferPair;
+  BufferPair pair = std::make_pair(buffer.pp_resource(), buffer);

[ddorwin, 2012/09/07 09:48:48]

Creates a copy of the Buffer_Dev. Is that costly?

[dmichael (off chromium), 2012/09/07 16:43:43]

No, it's just a handle. There's just reference counting and copying an int32_t
id.

+  buffer_map_.insert(pair);

[dmichael (off chromium), 2012/09/07 16:43:43]

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

[Tom Finegan, 2012/09/08 01:02:47]

Done.

+
+  return cdm::CdmBuffer(reinterpret_cast<uint8_t*>(buffer.data()),

[ddorwin, 2012/09/07 09:48:48]

Create PpbCdmBuffer

[Tom Finegan, 2012/09/08 01:02:47]

Do you want me to add a CreateBuffer method to PpbCdmBuffer, or is returned new
PpbCdmBuffer(...) sufficient?

+                        buffer.pp_resource(),
+                        buffer.size());
+}
+
+bool CdmAllocatorImpl::ReleaseBuffer(const cdm::CdmBuffer& buffer) {
+  bool result = false;

[ddorwin, 2012/09/07 09:48:48]

Why do we need this here instead of just returning false at 171?

[Tom Finegan, 2012/09/08 01:02:47]

Done.

+  PP_DCHECK(buffer.id());
+  if (ContainsKey(buffer_map_, buffer.id())) {
+    buffer_map_.erase(buffer.id());
+    result = true;
+  }
+  return result;
+}
+
 CdmWrapper::CdmWrapper(PP_Instance instance, pp::Module* module)
     : pp::Instance(instance),
       pp::ContentDecryptor_Private(this),
+      cdm_allocator_(this),
       cdm_(NULL) {
   callback_factory_.Initialize(this);
 }
@@ -125,7 +189,7 @@ bool CdmWrapper::GenerateKeyRequest(const std::string& key_system,
   PP_DCHECK(!key_system.empty());
 
   if (!cdm_) {
-    cdm_ = CreateCdmInstance();
+    cdm_ = CreateCdmInstance(&cdm_allocator_);
     if (!cdm_)
       return false;
   }
@@ -292,8 +356,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;
@@ -309,12 +371,12 @@ void CdmWrapper::DeliverBlock(int32_t result,
       decrypted_block_info.result = PP_DECRYPTRESULT_DECRYPT_ERROR;
   }
 
-  pp::ContentDecryptor_Private::DeliverBlock(decrypted_buffer,
+  pp::ContentDecryptor_Private::DeliverBlock(output_buffer.buffer_id,

[ddorwin, 2012/09/07 09:48:48]

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

[Tom Finegan, 2012/09/08 01:02:47]

Went w/static_cast<PpbCdmbuffer>.

                                              decrypted_block_info);
-
-  // TODO(xhwang): Fix this. This is not always safe as the memory is allocated
-  // in another shared object.
-  delete [] output_buffer.data;
+  if (!cdm_allocator_.ReleaseBuffer(cdm::CdmBuffer(output_buffer.buffer_id))) {
+    LOG(ERROR) << "CdmWrapper::DeliverBlock: buffer_id="

[ddorwin, 2012/09/07 09:48:48]

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?

[dmichael (off chromium), 2012/09/07 16:43:43]

Does logging work here? (Maybe you want PP_DCHECK)

[Tom Finegan, 2012/09/08 01:02:47]

Logging works, but I've removed the LOG usage-- it was only there for debugging,
anyway.

+               << output_buffer.buffer_id << " not found!";
+  }
   output_buffer.data = NULL;
 }