Encrypted Media: Fix PpbBuffer::Destroy().

media/cdm/ppapi/cdm_helpers.h

 // Copyright 2013 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.
 
 #ifndef MEDIA_CDM_PPAPI_CDM_HELPERS_H_
 #define MEDIA_CDM_PPAPI_CDM_HELPERS_H_
 
 #include <map>
 #include <utility>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "build/build_config.h"
 #include "media/cdm/ppapi/api/content_decryption_module.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/c/pp_stdint.h"
 #include "ppapi/cpp/dev/buffer_dev.h"
 #include "ppapi/cpp/instance.h"
 #include "ppapi/cpp/logging.h"
 
 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.
 // TODO(xhwang): Find a better name. It's confusing to have PpbBuffer,
 // 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);
 };
 
 class PpbBufferAllocator {
  public:
   explicit PpbBufferAllocator(pp::Instance* instance)
       : instance_(instance),
         next_buffer_id_(1) {}
   ~PpbBufferAllocator() {}
@@ skipping 138 matching lines @@
  private:
   PpbBuffer* buffer_;
   cdm::AudioFormat format_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioFramesImpl);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_CDM_PPAPI_CDM_HELPERS_H_