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..aa3e613cc394d5d7d777373cef819a440f27ef0d 100644
--- a/webkit/media/crypto/ppapi/content_decryption_module.h
+++ b/webkit/media/crypto/ppapi/content_decryption_module.h
@@ -17,11 +17,16 @@ typedef __int64 int64_t;
 #include "webkit/media/crypto/ppapi/cdm_export.h"
 
 namespace cdm {
+class Allocator;
+class Buffer;
 class ContentDecryptionModule;
+struct KeyMessage;
+struct OutputBuffer;
 }
 
 extern "C" {
-CDM_EXPORT cdm::ContentDecryptionModule* CreateCdmInstance();
+CDM_EXPORT cdm::ContentDecryptionModule* CreateCdmInstance(
+    cdm::Allocator* allocator);
 CDM_EXPORT void DestroyCdmInstance(cdm::ContentDecryptionModule* instance);
 CDM_EXPORT const char* GetCdmVersion();
 }
@@ -35,27 +40,6 @@ enum Status {
   kError
 };
 
-// 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),
-        session_id_size(0),
-        message(NULL),
-        message_size(0),
-        default_url(NULL),
-        default_url_size(0) {}
-
-  char* session_id;
-  uint32_t session_id_size;
-  uint8_t* message;
-  uint32_t message_size;
-  char* default_url;
-  uint32_t default_url_size;
-};
-
 // An input buffer can be split into several continuous subsamples.
 // A SubsampleEntry specifies the number of clear and cipher bytes in each
 // subsample. For example, the following buffer has three subsamples:
@@ -104,34 +88,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|.

[xhwang, 2012/09/13 14:35:45]

Thanks for the change to from uint32_t to int32_t. Could you please also update
it for SubsampleEntry?

[Tom Finegan, 2012/09/15 08:03:14]

Done.

 
-  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|.
-
-  int64_t timestamp;  // Presentation timestamp in microseconds.
-};
-
-// Represents an output decrypted buffer. It does not own |data|.
-struct OutputBuffer {
-  OutputBuffer()
-      : data(NULL),
-        data_size(0),
-        timestamp(0) {}
-
-  const uint8_t* data;  // Pointer to the beginning of the output data.
-  uint32_t data_size;  // Size (in bytes) of |data|.
+  int32_t num_subsamples;  // Number of subsamples in |subsamples|.
 
   int64_t timestamp;  // Presentation timestamp in microseconds.
 };
@@ -308,6 +279,85 @@ class ContentDecryptionModule {
   virtual ~ContentDecryptionModule() {}
 };
 
+// Encapsulates all data required to interact with buffers allocated by

[ddorwin, 2012/09/13 19:47:00]

How about:
Represents a buffer created by Alocator implementations.

[Tom Finegan, 2012/09/15 08:03:14]

Done.

+// Allocator implementations. Note that the buffer described by this object is

[ddorwin, 2012/09/13 19:47:00]

Isn't the second sentence an implementation detail? It doesn't really matter
since this object can only be destroyed by calling ReleaseBuffer().

+// not freed when the object is destroyed.
+class Buffer {
+ public:
+  virtual ~Buffer() {}

[ddorwin, 2012/09/13 19:47:00]

Make this protected for the reasons described in cdm_wrpper.cc.

[Tom Finegan, 2012/09/15 08:03:14]

Done.

+  virtual uint8_t* buffer() const = 0;
+  virtual int32_t size() const = 0;
+
+ protected:
+  Buffer() {}
+
+ private:
+  Buffer(const Buffer&);
+  void operator=(const Buffer&);
+};
+
+// Interface class that hides cross object memory allocation details from CDMs.
+// Allocated buffers should only be freed by the CDM wrapper with the single
+// exception noted by the |ReleaseBuffer()| comment.
+class Allocator {
+ public:
+  Allocator() {}
+  virtual ~Allocator() {}
+
+  // Returns a Buffer* containing non-zero members upon success, or NULL on
+  // failure. The caller owns the Buffer* until it is passed back to the CDM
+  // wrapper.
+  virtual Buffer* Allocate(int32_t size) = 0;
+
+  // Releases a Buffer that will not be delivered to the CDM wrapper, and sets

[ddorwin, 2012/09/13 19:47:00]

Release a Buffer returned by Allocate(). The |buffer| pointer is no longer valid
after calling this method.

[Tom Finegan, 2012/09/15 08:03:14]

Done.

+  // your Buffer* to NULL.
+  virtual void ReleaseBuffer(const Buffer* buffer) = 0;

[xhwang, 2012/09/13 14:35:45]

I am a little confused here. Can ReleaseBuffer's implementation not be "delete
buffer"? It looks like "delete buffer" will trigger the cdm::Buffer's overrided
dtor, which should be sufficient to destroy the cdm::Buffer's implementation and
release any resources associated with it. 

[Tom Finegan, 2012/09/15 08:03:14]

Two things:
1) Deleting memory allocated in another SO probably isn't a good idea.
2) deleting a base class pointer when the derived class declaration isn't
visible yields undefined behavior.

[ddorwin, 2012/09/17 21:19:23]

Also, the PpbBuffer may not actually be deleted since it is ref counted.

+};
+
+// Represents a key message sent by the CDM. It does not own any pointers in

[ddorwin, 2012/09/13 19:47:00]

It does own the pointers now.

[Tom Finegan, 2012/09/15 08:03:14]

This is now an interface.

+// this struct.
+struct KeyMessage {
+  KeyMessage(Allocator* allocator_)

[ddorwin, 2012/09/13 19:47:00]

need a different name than ending with an underscore.

[Tom Finegan, 2012/09/15 08:03:14]

Removed.

+      : session_id(NULL),
+        session_id_size(0),
+        allocator(allocator_),
+        message(NULL),
+        default_url(NULL),
+        default_url_size(0) {}
+  ~KeyMessage() {
+    delete[] session_id;

[xhwang, 2012/09/13 14:35:45]

What's the plan for session_id and default_url? 

For now, there should be a space between delete and [].
On line 332, should it also be "delete [] default_url"?

[ddorwin, 2012/09/13 19:47:00]

As noted in cdm_wrapper.cc, they need to be allocated and freed on the same
side. Thus, having the deletion in the destructor is probably not a good idea.

One option might be to make this entire class abstract and have
set_session_id(), etc. so the wrapper can make copies of those things and manage
them.

I guess another option is for everything to be allocated using Allocator.

For this CL, let's just move the TODO about this not being safe here. I think
this will be easier and clearer to deal with after we resolve the large buffers
and close out this CL.

+    session_id = NULL;
+    allocator->ReleaseBuffer(message);
+    message = NULL;
+    delete default_url;
+    default_url = NULL;
+  }
+
+  char* session_id;
+  int32_t session_id_size;
+
+  Allocator* const allocator;
+  Buffer* message;
+
+  char* default_url;
+  int32_t default_url_size;
+};
+
+// Represents an output decrypted buffer. It does not own |data|.

[ddorwin, 2012/09/13 19:47:00]

|data| does not exist.
It appears that |buffer| is owned by this class now.

[Tom Finegan, 2012/09/15 08:03:14]

Ditto, now an interface.

+struct OutputBuffer {

[ddorwin, 2012/09/14 00:20:39]

If we keep this struct, we should change the name since it's a "buffer" that
contains a buffer.

[Tom Finegan, 2012/09/15 08:03:14]

Haven't renamed it yet... maybe I'm overly fond of the word wrapper, but
OutputBufferWrapper makes sense. Maybe I'll think of something better Saturday.

+  explicit OutputBuffer(Allocator* allocator_)
+      : timestamp(0),
+        allocator(allocator_),
+        buffer(NULL) {}
+  ~OutputBuffer() {
+    allocator->ReleaseBuffer(buffer);
+  }
+
+  int64_t timestamp;  // Presentation timestamp in microseconds.

[ddorwin, 2012/09/14 00:20:39]

Do we really need a timestamp for Decrypt()? Since there is no decoding,
shouldn't it be known by the client? (I see we set it in ClearKey, but hat might
be an artifact of the reuse of AesDecryptor.)

If not, maybe we can eliminate this struct.

[Tom Finegan, 2012/09/15 08:03:14]

I'll look into this some time Saturday.

+  Allocator* const allocator;
+  Buffer* buffer;

[ddorwin, 2012/09/13 19:47:00]

I wonder if we should have a ScopedBuffer that contains the Buffer and calls
ReleaseBuffer() on it. Then we don't have to worry about getting all the
destructors right.

However, I wonder whether we should even have concrete structs in this file.
Currently, it _could_ be created and destroyed on different sides of the SO/DLL
boundary and it's up to the individual functions to make sure this doesn't
happen.

[Tom Finegan, 2012/09/15 08:03:14]

After changing KeyMessage and OutputBuffer to interfaces, and using
tr1::shared_ptr in the wrapper I think we're doing an adequate job of managing
this-- what do you think?

+};
+
 }  // namespace cdm
 
 #endif  // WEBKIT_MEDIA_CRYPTO_PPAPI_CONTENT_DECRYPTION_MODULE_H_