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>

[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"
 #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"
@@ skipping 27 matching lines @@
   if (IsMainThread())
     cb.Run(PP_OK);
   else
     pp::Module::Get()->core()->CallOnMainThread(0, cb, PP_OK);
 }
 
 }  // namespace
 
 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,
                    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 bool GenerateKeyRequest(const std::string& key_system,
@@ skipping 20 matching lines @@
   // <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 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);
 }
 
 CdmWrapper::~CdmWrapper() {
   if (cdm_)
     DestroyCdmInstance(cdm_);
 }
 
 bool CdmWrapper::GenerateKeyRequest(const std::string& key_system,
                                     pp::VarArrayBuffer init_data) {
   PP_DCHECK(!key_system.empty());
 
   if (!cdm_) {
-    cdm_ = CreateCdmInstance();
+    cdm_ = CreateCdmInstance(&cdm_allocator_);
     if (!cdm_)
       return false;
   }
 
   cdm::KeyMessage key_request;
   cdm::Status status = cdm_->GenerateKeyRequest(
       reinterpret_cast<const uint8_t*>(init_data.Map()),
       init_data.ByteLength(),
       &key_request);
 
@@ skipping 146 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::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,
+  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;
 }
 
 // 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