Add CDM allocator interface.

webkit/media/crypto/ppapi/content_decryption_module.h

Index: webkit/media/crypto/ppapi/content_decryption_module.h
diff --git a/webkit/media/crypto/ppapi/content_decryption_module.h b/webkit/media/crypto/ppapi/content_decryption_module.h
index 0f2244a5618b0c9e9c5e2399eb150a1f99f5ded7..13f85242c298013278eac962d4cbc243d34a0dd7 100644
--- a/webkit/media/crypto/ppapi/content_decryption_module.h
+++ b/webkit/media/crypto/ppapi/content_decryption_module.h
@@ -17,11 +17,13 @@ typedef __int64 int64_t;
 #include "webkit/media/crypto/ppapi/cdm_export.h"
 
 namespace cdm {
+class CdmAllocator;

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

"Cdm" is redundant in the "cdm" interface.

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

Done. Also renamed CdmBuffer to Buffer. For the same reason.

 class ContentDecryptionModule;
 }
 
 extern "C" {
-CDM_EXPORT cdm::ContentDecryptionModule* CreateCdmInstance();
+CDM_EXPORT cdm::ContentDecryptionModule* CreateCdmInstance(
+    cdm::CdmAllocator* allocator);
 CDM_EXPORT void DestroyCdmInstance(cdm::ContentDecryptionModule* instance);
 CDM_EXPORT const char* GetCdmVersion();
 }
@@ -37,8 +39,6 @@ enum Status {
 
 // Represents a key message sent by the CDM. It does not own any pointers in
 // this struct.
-// TODO(xhwang): Use int32_t instead of uint32_t for sizes here and below and
-// update checks to include <0.
 struct KeyMessage {
   KeyMessage()
       : session_id(NULL),
@@ -49,11 +49,14 @@ struct KeyMessage {
         default_url_size(0) {}
 
   char* session_id;
-  uint32_t session_id_size;
+  int32_t session_id_size;
+
+  // TODO(tomfinegan): Replace |message| and |message_size| with a CdmBuffer.
   uint8_t* message;
-  uint32_t message_size;
+  int32_t message_size;
+
   char* default_url;
-  uint32_t default_url_size;
+  int32_t default_url_size;
 };
 
 // An input buffer can be split into several continuous subsamples.
@@ -104,21 +107,21 @@ struct InputBuffer {
         timestamp(0) {}
 
   const uint8_t* data;  // Pointer to the beginning of the input data.
-  uint32_t data_size;  // Size (in bytes) of |data|.
+  int32_t data_size;  // Size (in bytes) of |data|.
 
-  uint32_t data_offset;  // Number of bytes to be discarded before decryption.
+  int32_t data_offset;  // Number of bytes to be discarded before decryption.
 
   const uint8_t* key_id;  // Key ID to identify the decryption key.
-  uint32_t key_id_size;  // Size (in bytes) of |key_id|.
+  int32_t key_id_size;  // Size (in bytes) of |key_id|.
 
   const uint8_t* iv;  // Initialization vector.
-  uint32_t iv_size;  // Size (in bytes) of |iv|.
+  int32_t iv_size;  // Size (in bytes) of |iv|.
 
   const uint8_t* checksum;
-  uint32_t checksum_size;  // Size (in bytes) of the |checksum|.
+  int32_t checksum_size;  // Size (in bytes) of the |checksum|.
 
   const struct SubsampleEntry* subsamples;
-  uint32_t num_subsamples;  // Number of subsamples in |subsamples|.
+  int32_t num_subsamples;  // Number of subsamples in |subsamples|.
 
   int64_t timestamp;  // Presentation timestamp in microseconds.
 };
@@ -128,12 +131,15 @@ struct OutputBuffer {
   OutputBuffer()
       : data(NULL),
         data_size(0),
-        timestamp(0) {}
+        timestamp(0),
+        buffer_id(0) {}
 
-  const uint8_t* data;  // Pointer to the beginning of the output data.
-  uint32_t data_size;  // Size (in bytes) of |data|.
+  uint8_t* data;  // Pointer to the beginning of the output data.
+  int32_t data_size;  // Size (in bytes) of |data|.
 
   int64_t timestamp;  // Presentation timestamp in microseconds.
+  int32_t buffer_id;  // Buffer identifier. Issued by CdmAllocator, and used by

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

Why is this not just a CdmBuffer?

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

Removed data, data_size, and buffer_id. Replaced with cdm::Buffer*.

+                      // the CDM wrapper.
 };
 
 // Surface formats based on FOURCC labels, see:
@@ -308,6 +314,53 @@ class ContentDecryptionModule {
   virtual ~ContentDecryptionModule() {}
 };
 

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

The comment should probably document the behavior. Will the buffer be freed when
this object is destroyed?
You might have similar questions on construction unless you prevent anything but
an empty buffer from being created in the base class.

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

Added note saying that the buffer is not freed.
Base class is now abstract, so moot, no?

+// Utility class that encapsulates all data required to interact with buffers

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

Remove "Utility class that ".

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

Done.

+// allocated by |CdmAllocator|s.

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

I don't think | is necessary around class names.

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

Done.

+class CdmBuffer {

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

This should be an abstract class that only exposes buffer()/data().
id and size are implementation details of the Impl. The former is specifically
related to the buffer.

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

I left the size method in the class since doing so allowed removal of the data
and data_size members from OutputBuffer.

+ public:
+  // Constructs an empty buffer.

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

Constructing a buffer sounds like allocation. But really it just represents it.
(Unless you change the Impl to hold the actual buffer.

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

Removed.

+  CdmBuffer() : buffer_(0), id_(0), size_(0) {}
+
+  // Constructs a buffer object containing all information necessary for a
+  // user writable buffer.

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

"user"?

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

Meant writable by user of the class, but I dropped the user.

+  CdmBuffer(uint8_t* buffer, int32_t id, int32_t size)
+      : buffer_(buffer), id_(id), size_(size) {}
+
+  // Constructs a buffer containing only a valid ID. Provided for the

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

This seems odd. Maybe it's not necessary if we store the CdmBuffer in
OutputBuffer.

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

You were right: storing the cdm::Buffer in OutputBuffer made this constructor
unnecessary.

+  // sake of convenience. For example, allows caller of
+  // CdmAllocator::ReleaseBuffer to pass CdmBuffer(value) when concerned only
+  // with releasing the buffer wrapped by this object.
+  explicit CdmBuffer(int32_t id) : buffer_(0), id_(id), size_(0) {}
+  ~CdmBuffer() {}

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

Be sure this is virtual after making this a base class.

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

Done.

+
+  uint8_t* buffer() const { return buffer_; }
+  int32_t id() const { return id_; }
+  int32_t size() const { return size_; }
+
+ private:
+  uint8_t* buffer_;

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

*const

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

cdm::Buffer no longer has data members.

+  const int32_t id_;
+  const int32_t size_;
+};
+
+// Interface class intended to hide cross object memory allocation details from

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

s/intended to/that/

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

Done.

+// CDMs. The CDM wrapper takes ownership of the allocated data when it is

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

This sentence sounds like it should be on those methods. What do we really need
to say here. Is my comment below sufficient?

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

Used your comment, but added a little bit...

+// returned to the wrapper by the CDM (i.e. via OutputBuffer in Decrypt()).

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

Allocated buffers should only be freed by the CDM wrapper.

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

Done.

+class CdmAllocator {
+ public:
+  CdmAllocator() {}
+  virtual ~CdmAllocator() {}
+
+  // Returns a CdmBuffer containing non-zero members upon success. Returns an
+  // empty CdmBuffer with all members equal to zero on failure.
+  virtual CdmBuffer Allocate(int32_t size) = 0;
+
+  // Used by the CDM to release a buffer that will not be delivered to the CDM
+  // wrapper. Returns true when |buffer| contains a valid buffer identifier,
+  // false otherwise.
+  virtual bool ReleaseBuffer(const CdmBuffer& buffer) = 0;
+};
+
 }  // namespace cdm
 
 #endif  // WEBKIT_MEDIA_CRYPTO_PPAPI_CONTENT_DECRYPTION_MODULE_H_