Encrypted Media: Fix PpbBuffer::Destroy().

media/cdm/ppapi/cdm_helpers.h

Index: media/cdm/ppapi/cdm_helpers.h
diff --git a/media/cdm/ppapi/cdm_helpers.h b/media/cdm/ppapi/cdm_helpers.h
index e033dd79bf81b586327efc85e544250ef65fa73a..78223fb2b20a0319fcd8968efe4789901e61ff8a 100644
--- a/media/cdm/ppapi/cdm_helpers.h
+++ b/media/cdm/ppapi/cdm_helpers.h
@@ -20,6 +20,8 @@
 
 namespace media {
 
+class PpbBufferAllocator;
+
 // cdm::Buffer implementation that provides access to memory owned by a
 // pp::Buffer_Dev.
 // This class holds a reference to the Buffer_Dev throughout its lifetime.
@@ -27,48 +29,33 @@ namespace media {
 // pp::Buffer_Dev and PPB_Buffer_Dev.
 class PpbBuffer : public cdm::Buffer {
  public:
-  static PpbBuffer* Create(const pp::Buffer_Dev& buffer, uint32_t buffer_id) {
-    PP_DCHECK(buffer.data());
-    PP_DCHECK(buffer.size());
-    PP_DCHECK(buffer_id);
-    return new PpbBuffer(buffer, buffer_id);
-  }
+  static PpbBuffer* Create(const pp::Buffer_Dev& buffer, uint32_t buffer_id,
+                           PpbBufferAllocator* allocator);

[ddorwin, 2014/07/10 21:56:14]

Minor issue, but it seems odd that the buffer (wrapper) takes (and keeps) a
pointer to the allocator, especially when it is being provided with an allocated
buffer already.

This would probably make more sense to be a release callback.

[xhwang, 2014/07/10 22:37:33]

We can't use base::Callback here and to pass a function pointer, we need to pass
a member-function pointer type, which still requires the allocator_ to call.

 
   // cdm::Buffer implementation.
-  virtual void Destroy() OVERRIDE { delete this; }
-
-  virtual uint32_t Capacity() const OVERRIDE { return buffer_.size(); }
-
-  virtual uint8_t* Data() OVERRIDE {
-    return static_cast<uint8_t*>(buffer_.data());
-  }
-
-  virtual void SetSize(uint32_t size) OVERRIDE {
-    PP_DCHECK(size <= Capacity());
-    if (size > Capacity()) {
-      size_ = 0;
-      return;
-    }
-
-    size_ = size;
-  }
-
+  virtual void Destroy() OVERRIDE;
+  virtual uint32_t Capacity() const OVERRIDE;

[ddorwin, 2014/07/10 21:56:14]

unrelated nit: These should be GetCapacity() and GetData().

[xhwang, 2014/07/10 22:37:33]

This is in the CDM interface and we can update the interface when we have CDM6.

+  virtual uint8_t* Data() OVERRIDE;
+  virtual void SetSize(uint32_t size) OVERRIDE;
   virtual uint32_t Size() const OVERRIDE { return size_; }
 
-  pp::Buffer_Dev buffer_dev() const { return buffer_; }
+  // Takes the ownership of |buffer_| and returns it. The caller makes sure

[ddorwin, 2014/07/10 21:56:14]

The caller must ensure... ?

[xhwang, 2014/07/10 22:37:33]

Done.

+  // |allocator->Release()| is called later so that the buffer can be reused in
+  // the allocator.

[ddorwin, 2014/07/10 21:56:14]

As discussed offline, it's really taking ownership of a single PPB_Buffer_Dev
reference, similar to PassRefPtr.

Add a comment about this really taking ownership of the one reference and that
other references, including the allocator, may still exist. That's perhaps
implied above, but I think it would be helpful to be more explicit.

[xhwang, 2014/07/10 22:37:33]

Done.

+  pp::Buffer_Dev TakeBufferDev();
 
   uint32_t buffer_id() const { return buffer_id_; }
 
  private:
-  PpbBuffer(pp::Buffer_Dev buffer, uint32_t buffer_id)
-      : buffer_(buffer),
-        buffer_id_(buffer_id),
-        size_(0) {}
-  virtual ~PpbBuffer() {}
+  PpbBuffer(pp::Buffer_Dev buffer,
+            uint32_t buffer_id,
+            PpbBufferAllocator* allocator);
+  virtual ~PpbBuffer();
 
   pp::Buffer_Dev buffer_;
   uint32_t buffer_id_;
   uint32_t size_;
+  PpbBufferAllocator* allocator_;
 
   DISALLOW_COPY_AND_ASSIGN(PpbBuffer);
 };