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 <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"
 #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"
 #include "ppapi/cpp/var.h"
 #include "ppapi/cpp/var_array_buffer.h"
 #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 {
 
 // This must be consistent with MediaKeyError defined in the spec:
 // http://goo.gl/rbdnR
 // TODO(xhwang): Add PP_MediaKeyError enum to avoid later static_cast in
 // PluginInstance.
 enum MediaKeyError {
   kUnknownError = 1,
@@ skipping 16 matching lines @@
   else
     pp::Module::Get()->core()->CallOnMainThread(0, cb, PP_OK);
 }
 
 }  // namespace
 
 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
   // 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 bool GenerateKeyRequest(const std::string& key_system,
                                   pp::VarArrayBuffer init_data) OVERRIDE;
   virtual bool AddKey(const std::string& session_id,
                       pp::VarArrayBuffer key,
                       pp::VarArrayBuffer init_data) OVERRIDE;
   virtual bool CancelKeyRequest(const std::string& session_id) OVERRIDE;
   virtual bool Decrypt(
       pp::Buffer_Dev encrypted_buffer,
       const PP_EncryptedBlockInfo& encrypted_block_info) OVERRIDE;
   virtual bool DecryptAndDecode(
       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
   // ID of 0 on failure. Upon success, the returned Buffer resource has a
   // reference count of 1.
   PP_Resource 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);
 
+  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_;
 };
 
 CdmWrapper::CdmWrapper(PP_Instance instance, pp::Module* module)
     : pp::Instance(instance),
       pp::ContentDecryptor_Private(this),
+      buffer_iface_(NULL),
       cdm_(NULL) {
   callback_factory_.Initialize(this);
 }
 
 CdmWrapper::~CdmWrapper() {
   if (cdm_)
     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;
   }
 
   cdm::KeyMessage key_request;
   cdm::Status status = cdm_->GenerateKeyRequest(
       reinterpret_cast<const uint8_t*>(init_data.Map()),
       init_data.ByteLength(),
       &key_request);
 
   if (status != cdm::kSuccess ||
@@ skipping 96 matching lines @@
 
   return true;
 }
 
 bool CdmWrapper::DecryptAndDecode(
     pp::Buffer_Dev encrypted_buffer,
     const PP_EncryptedBlockInfo& encrypted_block_info) {
   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)
     return 0;
 
   pp::Buffer_Dev buffer(this, data_size);
   if (!buffer.data())
     return 0;
 
   memcpy(buffer.data(), data, data_size);
@@ skipping 31 matching lines @@
   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::kErrorNoKey:
       decrypted_block_info.result = PP_DECRYPTRESULT_DECRYPT_NOKEY;
       break;
     default:
       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;
 }
 
 // This object is the global object representing this plugin library as long
 // as it is loaded.
 class MyModule : public pp::Module {
  public:
   MyModule() : pp::Module() {}
   virtual ~MyModule() {}
 
   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();
 }
 
 }  // namespace pp