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 88baa44062b74920ed0ee272db53ec8c29824e7b..c7e505d739b6ff5c2579b30dccb19e2e1079971c 100644
--- a/webkit/media/crypto/ppapi/cdm_wrapper.cc
+++ b/webkit/media/crypto/ppapi/cdm_wrapper.cc
@@ -3,11 +3,13 @@
 // found in the LICENSE file.
 
 #include <cstring>  // For memcpy.
+#include <queue>
 #include <vector>
 
 #include "base/compiler_specific.h"  // For OVERRIDE.
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/c/pp_stdint.h"
+#include "ppapi/c/dev/ppb_buffer_dev.h"

[ddorwin, 2012/09/04 09:53:15]

Why not use cpp?

[Tom Finegan, 2012/09/07 00:46:36]

Done, using it now.

 #include "ppapi/c/private/pp_content_decryptor.h"
 #include "ppapi/cpp/completion_callback.h"
 #include "ppapi/cpp/core.h"
@@ -21,6 +23,7 @@
 #include "ppapi/cpp/dev/buffer_dev.h"
 #include "ppapi/cpp/private/content_decryptor_private.h"
 #include "ppapi/utility/completion_callback_factory.h"
+#include "webkit/media/crypto/ppapi/cdm_allocator.h"
 #include "webkit/media/crypto/ppapi/content_decryption_module.h"
 
 namespace {
@@ -57,15 +60,13 @@ namespace webkit_media {
 
 // A wrapper class for abstracting away PPAPI interaction and threading for a
 // Content Decryption Module (CDM).
-class CdmWrapper : public pp::Instance,
+class CdmWrapper : public CdmAllocator,

[dmichael (off chromium), 2012/09/04 18:06:01]

Is there any reason to have CdmWrapper implement this? Seems easy and logical to
abstract it out to a CdmAllocatorImpl (or whatever).

[Tom Finegan, 2012/09/07 00:46:36]

Done.

+                   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;
-  }
+  virtual bool Init(uint32_t argc, const char* argn[], const char* argv[]);
 
   // PPP_ContentDecryptor_Private methods
   // Note: As per comments in PPP_ContentDecryptor_Private, these calls should
@@ -85,6 +86,9 @@ class CdmWrapper : public pp::Instance,
       pp::Buffer_Dev encrypted_buffer,
       const PP_EncryptedBlockInfo& encrypted_block_info) OVERRIDE;
 
+  // CdmAllocator methods.
+  virtual uint8_t* Allocate(int32_t size, int32_t* id) 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
@@ -103,6 +107,7 @@ class CdmWrapper : public pp::Instance,
                     cdm::OutputBuffer& output_buffer,
                     const PP_DecryptTrackingInfo& tracking_info);
 
+  const PPB_Buffer_Dev* buffer_iface_;

[ddorwin, 2012/09/04 09:53:15]

Might be nice to keep all the raw pointers together.

[Tom Finegan, 2012/09/07 00:46:36]

Removed this.

   pp::CompletionCallbackFactory<CdmWrapper> callback_factory_;
   cdm::ContentDecryptionModule* cdm_;
   std::string key_system_;
@@ -111,6 +116,7 @@ class CdmWrapper : public pp::Instance,
 CdmWrapper::CdmWrapper(PP_Instance instance, pp::Module* module)
     : pp::Instance(instance),
       pp::ContentDecryptor_Private(this),
+      buffer_iface_(NULL),
       cdm_(NULL) {
   callback_factory_.Initialize(this);
 }
@@ -120,13 +126,20 @@ CdmWrapper::~CdmWrapper() {
     DestroyCdmInstance(cdm_);
 }
 
+bool CdmWrapper::Init(uint32_t argc, const char* argn[], const char* argv[]) {
+  buffer_iface_ = static_cast<const PPB_Buffer_Dev*>(

[ddorwin, 2012/09/04 09:53:15]

Why not just use the C++ object?

[Tom Finegan, 2012/09/07 00:46:36]

Done.

+      pp::Module::Get()->GetBrowserInterface(PPB_BUFFER_DEV_INTERFACE));
+  PP_DCHECK(buffer_iface_);
+  return true;
+}
+
 bool CdmWrapper::GenerateKeyRequest(const std::string& key_system,
                                     pp::VarArrayBuffer init_data) {
   PP_DCHECK(!key_system.empty());
 
   if (!cdm_) {
     cdm_ = CreateCdmInstance();
-    if (!cdm_)
+    if (!cdm_ || !cdm_->Initialize(this))
       return false;
   }
 
@@ -243,6 +256,22 @@ bool CdmWrapper::DecryptAndDecode(
   return false;
 }
 
+// This method uses the C buffer interface (PPB_Buffer_Dev) because callers
+// require that the buffer is mapped.

[ddorwin, 2012/09/04 09:53:15]

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.

[Tom Finegan, 2012/09/07 00:46:36]

Done.

+uint8_t* CdmWrapper::Allocate(int32_t size, int32_t* id) {
+  PP_DCHECK(size > 0);
+  PP_DCHECK(id);
+
+  PP_Resource buffer_resource = buffer_iface_->Create(pp_instance(), size);

[dmichael (off chromium), 2012/09/04 18:06:01]

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.

[Tom Finegan, 2012/09/07 00:46:36]

Done.

+  PP_DCHECK(buffer_resource);
+  PP_DCHECK(id);

[ddorwin, 2012/09/04 09:53:15]

delete

[Tom Finegan, 2012/09/07 00:46:36]

Done.

+
+  void* buffer = buffer_iface_->Map(buffer_resource);

[ddorwin, 2012/09/04 09:53:15]

Can this not fail? Seems there should either be a DCHECK here or none at 266.

[Tom Finegan, 2012/09/07 00:46:36]

Done.

+  *id = buffer_resource;
+
+  return reinterpret_cast<uint8_t*>(buffer);
+}
+
 PP_Resource CdmWrapper::MakeBufferResource(const uint8_t* data,
                                            uint32_t data_size) {
   if (!data || !data_size)
@@ -294,9 +323,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;
@@ -312,12 +338,8 @@ 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/04 09:53:15]

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.

[Tom Finegan, 2012/09/07 00:46:36]

Done.

                                              decrypted_block_info);

[ddorwin, 2012/09/04 09:53:15]

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.

[Tom Finegan, 2012/09/07 00:46:36]

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.

-
-  // 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;
 }