Merge 282544 "Encrypted Media: Fix PpbBuffer::Destroy()."

> Encrypted Media: Fix PpbBuffer::Destroy().
> 
> This CL makes the ownership of the pp::Buffer_Dev clear:
> 
> 1) If the PpbBuffer owns the pp::Buffer_Dev, it always calls
> allocator_->Release() during the destructor so that the buffer is returned to
> the allocator_ and can be reused.
> 
> 2) If TakeBufferDev() is called. PpbBuffer doesn't own the pp::Buffer_Dev any
> more. The caller of TakeBufferDev() needs to make sure allocator->Release() is
> called sometime later to return the buffer to allocator_.
> 
> BUG=392497
> TEST=Tested with random stream switch video.
> 
> Review URL: https://codereview.chromium.org/374353004

TBR=xhwang@chromium.org

Review URL: https://codereview.chromium.org/396793002

git-svn-id: svn://svn.chromium.org/chrome/branches/2062/src@283262 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 86 insertions, 35 deletions

diff --git a/media/cdm/ppapi/cdm_adapter.cc b/media/cdm/ppapi/cdm_adapter.cc
index 8388f44..16ee830 100644
--- a/media/cdm/ppapi/cdm_adapter.cc
+++ b/media/cdm/ppapi/cdm_adapter.cc
@@ -855,9 +855,10 @@ void CdmAdapter::DeliverBlock(int32_t result,
     } else {
       PpbBuffer* ppb_buffer =
           static_cast<PpbBuffer*>(decrypted_block->DecryptedBuffer());
-      buffer = ppb_buffer->buffer_dev();
       decrypted_block_info.tracking_info.buffer_id = ppb_buffer->buffer_id();
       decrypted_block_info.data_size = ppb_buffer->Size();
+
+      buffer = ppb_buffer->TakeBuffer();
     }
   }
 
@@ -908,8 +909,6 @@ void CdmAdapter::DeliverFrame(
       PpbBuffer* ppb_buffer =
           static_cast<PpbBuffer*>(video_frame->FrameBuffer());
 
-      buffer = ppb_buffer->buffer_dev();
-
       decrypted_frame_info.tracking_info.timestamp = video_frame->Timestamp();
       decrypted_frame_info.tracking_info.buffer_id = ppb_buffer->buffer_id();
       decrypted_frame_info.format =
@@ -928,8 +927,11 @@ void CdmAdapter::DeliverFrame(
           video_frame->Stride(cdm::VideoFrame::kUPlane);
       decrypted_frame_info.strides[PP_DECRYPTEDFRAMEPLANES_V] =
           video_frame->Stride(cdm::VideoFrame::kVPlane);
+
+      buffer = ppb_buffer->TakeBuffer();
     }
   }
+
   pp::ContentDecryptor_Private::DeliverFrame(buffer, decrypted_frame_info);
 }
 
@@ -956,11 +958,13 @@ void CdmAdapter::DeliverSamples(int32_t result,
     } else {
       PpbBuffer* ppb_buffer =
           static_cast<PpbBuffer*>(audio_frames->FrameBuffer());
-      buffer = ppb_buffer->buffer_dev();
+
       decrypted_sample_info.tracking_info.buffer_id = ppb_buffer->buffer_id();
       decrypted_sample_info.data_size = ppb_buffer->Size();
       decrypted_sample_info.format =
           CdmAudioFormatToPpDecryptedSampleFormat(audio_frames->Format());
+
+      buffer = ppb_buffer->TakeBuffer();
     }
   }
 
diff --git a/media/cdm/ppapi/cdm_helpers.cc b/media/cdm/ppapi/cdm_helpers.cc
index 36b9502..62f93a0 100644
--- a/media/cdm/ppapi/cdm_helpers.cc
+++ b/media/cdm/ppapi/cdm_helpers.cc
@@ -4,6 +4,7 @@
 
 #include "media/cdm/ppapi/cdm_helpers.h"
 
+#include <algorithm>
 #include <utility>
 
 #include "base/basictypes.h"
@@ -20,6 +21,61 @@
 
 namespace media {
 
+// static
+PpbBuffer* PpbBuffer::Create(const pp::Buffer_Dev& buffer,
+                             uint32_t buffer_id,
+                             PpbBufferAllocator* allocator) {
+  PP_DCHECK(buffer.data());
+  PP_DCHECK(buffer.size());
+  PP_DCHECK(buffer_id);
+  PP_DCHECK(allocator);
+  return new PpbBuffer(buffer, buffer_id, allocator);
+}
+
+void PpbBuffer::Destroy() {
+  delete this;
+}
+
+uint32_t PpbBuffer::Capacity() const {
+  return buffer_.size();
+}
+
+uint8_t* PpbBuffer::Data() {
+  return static_cast<uint8_t*>(buffer_.data());
+}
+
+void PpbBuffer::SetSize(uint32_t size) {
+  PP_DCHECK(size <= Capacity());
+  if (size > Capacity()) {
+    size_ = 0;
+    return;
+  }
+
+  size_ = size;
+}
+
+pp::Buffer_Dev PpbBuffer::TakeBuffer() {
+  PP_DCHECK(!buffer_.is_null());
+  pp::Buffer_Dev buffer;
+  std::swap(buffer, buffer_);
+  buffer_id_ = 0;
+  size_ = 0;
+  return buffer;
+}
+
+PpbBuffer::PpbBuffer(pp::Buffer_Dev buffer,
+                     uint32_t buffer_id,
+                     PpbBufferAllocator* allocator)
+    : buffer_(buffer), buffer_id_(buffer_id), size_(0), allocator_(allocator) {
+}
+
+PpbBuffer::~PpbBuffer() {
+  PP_DCHECK(!buffer_id_ == buffer_.is_null());
+  // If still owning the |buffer_|, release it in the |allocator_|.
+  if (buffer_id_)
+    allocator_->Release(buffer_id_);
+}
+
 cdm::Buffer* PpbBufferAllocator::Allocate(uint32_t capacity) {
   PP_DCHECK(pp::Module::Get()->core()->IsMainThread());
 
@@ -46,7 +102,7 @@ cdm::Buffer* PpbBufferAllocator::Allocate(uint32_t capacity) {
 
   allocated_buffers_.insert(std::make_pair(buffer_id, buffer));
 
-  return PpbBuffer::Create(buffer, buffer_id);
+  return PpbBuffer::Create(buffer, buffer_id, this);
 }
 
 void PpbBufferAllocator::Release(uint32_t buffer_id) {
diff --git a/media/cdm/ppapi/cdm_helpers.h b/media/cdm/ppapi/cdm_helpers.h
index e033dd79b..1ee579b 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,37 @@ 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);
 
   // 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;
+  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 |buffer_| from this class and returns it.
+  // Note: The caller must ensure |allocator->Release()| is called later so that
+  // the buffer can be reused by the allocator.
+  // Since pp::Buffer_Dev is ref-counted, the caller now holds one reference to
+  // the buffer and this class holds no reference. Note that other references
+  // may still exist. For example, PpbBufferAllocator always holds a reference
+  // to all allocated buffers.
+  pp::Buffer_Dev TakeBuffer();
 
   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);
 };