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.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282544

[xhwang, 2014-07-10 16:42:40]

PTAL

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_adapter.cc
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:860: buffer = ppb_buffer->TakeBufferDev();
This call also clears buffer_id and size. That's why I move it here. I thought
about some other names but still feel this is the best. Naming suggestions
welcome!

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers.cc
File media/cdm/ppapi/cdm_helpers.cc (right):

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers....
media/cdm/ppapi/cdm_helpers.cc:7: #include <algorithm>
This is for std::swap.

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers....
media/cdm/ppapi/cdm_helpers.cc:76: allocator_->Release(buffer_id_);
Sorry for moving things around. The changes in this class are:
1) Add allocator in ctor and in Create().
2) Call allocator_->Release() in dtor.
3) Change buffer_dev() to TakeBufferDev() to make ownership clearer.

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

LG. Minor stuff.

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_adapter.cc
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:860: buffer = ppb_buffer->TakeBufferDev();
On 2014/07/10 16:42:39, xhwang wrote:
> This call also clears buffer_id and size. That's why I move it here. I thought
> about some other names but still feel this is the best. Naming suggestions
> welcome!

We never fixed the original naming. PpbBuffer isn't actually a PPB_Buffer_Dev.
*sigh*

I have no suggestions for that or this at the moment. We could probably drop the
"Dev" part of this new function name. Maybe it is differentiating the two
"buffers", but that's a crutch and we should solve the real problem. Maybe we
don't need to differentiate them, though. This is basically a deep move, leaving
the wrapper "empty".

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:933:
pp::ContentDecryptor_Private::DeliverFrame(buffer, decrypted_frame_info);
nit: add an empty line for consistency (and because it's better IMO)

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers.cc
File media/cdm/ppapi/cdm_helpers.cc (right):

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers....
media/cdm/ppapi/cdm_helpers.cc:75: PP_DCHECK(!buffer_.is_null());
nit: Move up and make sure !id == !is_null()

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers.h
File media/cdm/ppapi/cdm_helpers.h (right):

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers....
media/cdm/ppapi/cdm_helpers.h:33: PpbBufferAllocator* allocator);
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.

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers....
media/cdm/ppapi/cdm_helpers.h:37: virtual uint32_t Capacity() const OVERRIDE;
unrelated nit: These should be GetCapacity() and GetData().

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers....
media/cdm/ppapi/cdm_helpers.h:42: // Takes the ownership of |buffer_| and
returns it. The caller makes sure
The caller must ensure... ?

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers....
media/cdm/ppapi/cdm_helpers.h:44: // the allocator.
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:36:33]

comments addressed

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

PTAL again.

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_adapter.cc
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:860: buffer = ppb_buffer->TakeBufferDev();
On 2014/07/10 21:56:14, ddorwin wrote:
> On 2014/07/10 16:42:39, xhwang wrote:
> > This call also clears buffer_id and size. That's why I move it here. I
thought
> > about some other names but still feel this is the best. Naming suggestions
> > welcome!
> 
> We never fixed the original naming. PpbBuffer isn't actually a PPB_Buffer_Dev.
> *sigh*

How about just BufferImpl, which is consistent with VideoFrameImpl.
 
> I have no suggestions for that or this at the moment. We could probably drop
the
> "Dev" part of this new function name. Maybe it is differentiating the two
> "buffers", but that's a crutch and we should solve the real problem. Maybe we
> don't need to differentiate them, though. This is basically a deep move,
leaving
> the wrapper "empty".

Ok, I'll use TakeBuffer().

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:933:
pp::ContentDecryptor_Private::DeliverFrame(buffer, decrypted_frame_info);
On 2014/07/10 21:56:14, ddorwin wrote:
> nit: add an empty line for consistency (and because it's better IMO)

Done.

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers.cc
File media/cdm/ppapi/cdm_helpers.cc (right):

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers....
media/cdm/ppapi/cdm_helpers.cc:75: PP_DCHECK(!buffer_.is_null());
On 2014/07/10 21:56:14, ddorwin wrote:
> nit: Move up and make sure !id == !is_null()

Done.

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers.h
File media/cdm/ppapi/cdm_helpers.h (right):

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers....
media/cdm/ppapi/cdm_helpers.h:33: PpbBufferAllocator* allocator);
On 2014/07/10 21:56:14, ddorwin wrote:
> 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.

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.

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers....
media/cdm/ppapi/cdm_helpers.h:37: virtual uint32_t Capacity() const OVERRIDE;
On 2014/07/10 21:56:14, ddorwin wrote:
> unrelated nit: These should be GetCapacity() and GetData().

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

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers....
media/cdm/ppapi/cdm_helpers.h:42: // Takes the ownership of |buffer_| and
returns it. The caller makes sure
On 2014/07/10 21:56:14, ddorwin wrote:
> The caller must ensure... ?

Done.

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_helpers....
media/cdm/ppapi/cdm_helpers.h:44: // the allocator.
On 2014/07/10 21:56:14, ddorwin wrote:
> 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.

Done.

[ddorwin, 2014-07-10 22:43:25]

LGTM % comments. Thanks!

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_adapter.cc
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:860: buffer = ppb_buffer->TakeBufferDev();
On 2014/07/10 22:37:33, xhwang wrote:
> On 2014/07/10 21:56:14, ddorwin wrote:
> > On 2014/07/10 16:42:39, xhwang wrote:
> > > This call also clears buffer_id and size. That's why I move it here. I
> thought
> > > about some other names but still feel this is the best. Naming suggestions
> > > welcome!
> > 
> > We never fixed the original naming. PpbBuffer isn't actually a
PPB_Buffer_Dev.
> > *sigh*
> 
> How about just BufferImpl, which is consistent with VideoFrameImpl.
>  
> > I have no suggestions for that or this at the moment. We could probably drop
> the
> > "Dev" part of this new function name. Maybe it is differentiating the two
> > "buffers", but that's a crutch and we should solve the real problem. Maybe
we
> > don't need to differentiate them, though. This is basically a deep move,
> leaving
> > the wrapper "empty".
> 
> Ok, I'll use TakeBuffer().

BufferImpl sounds good. In a different CL, of course.

https://codereview.chromium.org/374353004/diff/20001/media/cdm/ppapi/cdm_help...
File media/cdm/ppapi/cdm_helpers.h (right):

https://codereview.chromium.org/374353004/diff/20001/media/cdm/ppapi/cdm_help...
media/cdm/ppapi/cdm_helpers.h:46: // buffers. The caller must ensure
|allocator->Release()| is called later so
nit: Move this last sentence after the first one as they are the most important
and tell the caller exactly what they need to do. The rest is additional
details.

[xhwang, 2014-07-10 22:47:50]

comments addressed

[xhwang, 2014-07-10 22:48:51]

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_adapter.cc
File media/cdm/ppapi/cdm_adapter.cc (right):

https://codereview.chromium.org/374353004/diff/1/media/cdm/ppapi/cdm_adapter....
media/cdm/ppapi/cdm_adapter.cc:860: buffer = ppb_buffer->TakeBufferDev();
On 2014/07/10 22:43:24, ddorwin wrote:
> On 2014/07/10 22:37:33, xhwang wrote:
> > On 2014/07/10 21:56:14, ddorwin wrote:
> > > On 2014/07/10 16:42:39, xhwang wrote:
> > > > This call also clears buffer_id and size. That's why I move it here. I
> > thought
> > > > about some other names but still feel this is the best. Naming
suggestions
> > > > welcome!
> > > 
> > > We never fixed the original naming. PpbBuffer isn't actually a
> PPB_Buffer_Dev.
> > > *sigh*
> > 
> > How about just BufferImpl, which is consistent with VideoFrameImpl.
> >  
> > > I have no suggestions for that or this at the moment. We could probably
drop
> > the
> > > "Dev" part of this new function name. Maybe it is differentiating the two
> > > "buffers", but that's a crutch and we should solve the real problem. Maybe
> we
> > > don't need to differentiate them, though. This is basically a deep move,
> > leaving
> > > the wrapper "empty".
> > 
> > Ok, I'll use TakeBuffer().
> 
> BufferImpl sounds good. In a different CL, of course.

Will do.

https://codereview.chromium.org/374353004/diff/20001/media/cdm/ppapi/cdm_help...
File media/cdm/ppapi/cdm_helpers.h (right):

https://codereview.chromium.org/374353004/diff/20001/media/cdm/ppapi/cdm_help...
media/cdm/ppapi/cdm_helpers.h:46: // buffers. The caller must ensure
|allocator->Release()| is called later so
On 2014/07/10 22:43:24, ddorwin wrote:
> nit: Move this last sentence after the first one as they are the most
important
> and tell the caller exactly what they need to do. The rest is additional
> details.

Done.

[xhwang, 2014-07-10 22:48:55]

The CQ bit was checked by xhwang@chromium.org

[commit-bot: I haz the power, 2014-07-10 22:49:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/374353004/40001

[commit-bot: I haz the power, 2014-07-11 04:47:23]

Change committed as 282544