Add allocator interface for use by cdm_adapter

Add allocator interface for use by cdm_adapter

Rather than allocating a buffer directly, add CdmAllocator interface
that will allocate buffers as needed. Initial implementation of
CdmAllocator (SimpleCdmAllocator) simply creates a new buffer every
time one is needed. There is no reuse, since it will only be used
for testing purposes.

CdmAllocator also has an interface to create a cdm::VideoFrame,
which now has the additional ability to create a media::VideoFrame
using the data returned by the CDM.

BUG=510088
TEST=Updated tests pass

Committed: https://crrev.com/550e1c056d4456000cb4be88676d6f5f01b9efdb
Cr-Commit-Position: refs/heads/master@{#375987}

[jrummell, 2016-02-08 22:08:56]

jrummell@chromium.org changed reviewers:
+ xhwang@chromium.org

[jrummell, 2016-02-08 22:08:56]

PTAL

[xhwang, 2016-02-09 18:27:33]

I skimmed the CL and have some questions about API design. We can chat offline
about this.

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

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_adapter.cc#ne...
media/cdm/cdm_adapter.cc:335: CdmBufferAllocator* allocator) {
The |allocator| seems an input, while the callbacks are kinda outputs. Move the
allocator up to be grouped with the inputs?

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_adapter.cc#ne...
media/cdm/cdm_adapter.cc:335: CdmBufferAllocator* allocator) {
who owns the allocator?

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_adapter.cc#ne...
media/cdm/cdm_adapter.cc:684:
reinterpret_cast<CdmBuffer*>(video_frame->FrameBuffer());
Can we do static cast?

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer.h
File media/cdm/cdm_buffer.h (right):

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer.h#newc...
media/cdm/cdm_buffer.h:27: gfx::Size natural_size) = 0;
It's odd to have this function in this class. See my comment below please.

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_alloca...
File media/cdm/cdm_buffer_allocator.h (right):

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_alloca...
media/cdm/cdm_buffer_allocator.h:24: virtual void Release(uint32_t buffer_id) =
0;
What is a |buffer_id|? How do we get it? It's not available from Allocate().

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_alloca...
File media/cdm/cdm_buffer_allocator_impl.h (right):

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_alloca...
media/cdm/cdm_buffer_allocator_impl.h:19: // on the returned buffer when it is
no longer needed.
How about Release()? Can Release() be called after Destroy()?

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_alloca...
media/cdm/cdm_buffer_allocator_impl.h:20: class MEDIA_EXPORT
CdmBufferAllocatorImpl : public CdmBufferAllocator {
The "Impl" part makes it look like it'll be the default implementation to be
used in production.

Will we also have SharedMemoryCdmBufferAllocator? How about
s/CdmBufferAllocatorImpl/SimpleCdmBufferAllocator/? 

And the CdmBufferImpl can be SimpleCdmBuffer.

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_impl.cc
File media/cdm/cdm_buffer_impl.cc (right):

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_impl.c...
media/cdm/cdm_buffer_impl.cc:45: VideoFrameImpl* video_frame,
Not related to this CL: VideoFrameImpl should be renamed to CdmVideoFrame.

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_impl.c...
media/cdm/cdm_buffer_impl.cc:56:
base::TimeDelta::FromMicroseconds(video_frame->Timestamp()));
It seems we can have the same implementation for normal memory and shared memory
based CdmBuffer implementation?

If so, can we move MakeVideoFrame() from CdmBuffer to VideoFrameImpl? It seems
more natural to convert a VideoFrameImpl to VideoFrame.

This function here seems a bit odd. For example, is it possible that
video_frame->FrameBuffer() is not equal to |this|?

[jrummell, 2016-02-11 01:36:10]

Description was changed from

==========
Add allocator interface for use when cdm_adapter needs memory

Rather than allocating a buffer directly, add CdmBufferAllocator
interface that will allocate the buffer as needed. Initial
implementation of CdmBufferAllocator simply creates a new buffer
every time one is needed. There is no reuse, since it will only
be used for testing purposes.

The other change is that CdmBuffer now has a method to create a
VideoFrame. This allows the CdmBuffer implementation to decide
the best way to create the VideoFrame, which will get exercised
when the code starts using shared memory as the storage mechanism.

BUG=510088
TEST=Updated tests pass
==========

to

==========
Add allocator interface for use when cdm_adapter needs memory

Rather than allocating a buffer directly, add CdmBufferAllocator
interface that will allocate the buffer when requested. Initial
implementation of CdmBufferAllocator (SimpleCdmBufferAllocator)
simply creates a new buffer every time one is needed. There is
no reuse, since it will only be used for testing purposes.

CdmBufferAllocator also has an interface to create a CdmVideoFrame,
which is a cdm::VideoFrame with the additional ability to create
a media::VideoFrame using the data returned by the CDM.

BUG=510088
TEST=Updated tests pass
==========

[jrummell, 2016-02-11 01:39:39]

Updated based on offline comments as well as issues noted.

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

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_adapter.cc#ne...
media/cdm/cdm_adapter.cc:335: CdmBufferAllocator* allocator) {
On 2016/02/09 18:27:32, xhwang wrote:
> who owns the allocator?

Changed to scoped_ptr<> to be clear.

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_adapter.cc#ne...
media/cdm/cdm_adapter.cc:335: CdmBufferAllocator* allocator) {
On 2016/02/09 18:27:32, xhwang wrote:
> The |allocator| seems an input, while the callbacks are kinda outputs. Move
the
> allocator up to be grouped with the inputs?

Done.

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_adapter.cc#ne...
media/cdm/cdm_adapter.cc:684:
reinterpret_cast<CdmBuffer*>(video_frame->FrameBuffer());
On 2016/02/09 18:27:32, xhwang wrote:
> Can we do static cast?

Done (now moved elsewhere).

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer.h
File media/cdm/cdm_buffer.h (right):

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer.h#newc...
media/cdm/cdm_buffer.h:27: gfx::Size natural_size) = 0;
On 2016/02/09 18:27:32, xhwang wrote:
> It's odd to have this function in this class. See my comment below please.

Removed.

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_alloca...
File media/cdm/cdm_buffer_allocator.h (right):

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_alloca...
media/cdm/cdm_buffer_allocator.h:24: virtual void Release(uint32_t buffer_id) =
0;
On 2016/02/09 18:27:32, xhwang wrote:
> What is a |buffer_id|? How do we get it? It's not available from Allocate().

Fixed so that it uses the same object as Allocate() returns.

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_alloca...
File media/cdm/cdm_buffer_allocator_impl.h (right):

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_alloca...
media/cdm/cdm_buffer_allocator_impl.h:19: // on the returned buffer when it is
no longer needed.
On 2016/02/09 18:27:32, xhwang wrote:
> How about Release()? Can Release() be called after Destroy()?

Updated to mention Release() instead.

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_alloca...
media/cdm/cdm_buffer_allocator_impl.h:20: class MEDIA_EXPORT
CdmBufferAllocatorImpl : public CdmBufferAllocator {
On 2016/02/09 18:27:32, xhwang wrote:
> The "Impl" part makes it look like it'll be the default implementation to be
> used in production.
> 
> Will we also have SharedMemoryCdmBufferAllocator? How about
> s/CdmBufferAllocatorImpl/SimpleCdmBufferAllocator/? 
> 
> And the CdmBufferImpl can be SimpleCdmBuffer.
> 

Done.

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_impl.cc
File media/cdm/cdm_buffer_impl.cc (right):

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_impl.c...
media/cdm/cdm_buffer_impl.cc:45: VideoFrameImpl* video_frame,
On 2016/02/09 18:27:33, xhwang wrote:
> Not related to this CL: VideoFrameImpl should be renamed to CdmVideoFrame.

Acknowledged.

https://codereview.chromium.org/1673383002/diff/1/media/cdm/cdm_buffer_impl.c...
media/cdm/cdm_buffer_impl.cc:56:
base::TimeDelta::FromMicroseconds(video_frame->Timestamp()));
On 2016/02/09 18:27:33, xhwang wrote:
> It seems we can have the same implementation for normal memory and shared
memory
> based CdmBuffer implementation?
> 
> If so, can we move MakeVideoFrame() from CdmBuffer to VideoFrameImpl? It seems
> more natural to convert a VideoFrameImpl to VideoFrame.
> 
> This function here seems a bit odd. For example, is it possible that
> video_frame->FrameBuffer() is not equal to |this|?

Redone.

[xhwang, 2016-02-11 19:24:14]

This looks much better!

I didn't review all details. Mostly interface/design discussion.

https://chromiumcodereview.appspot.com/1673383002/diff/20001/media/cdm/cdm_bu...
File media/cdm/cdm_buffer_allocator.h (right):

https://chromiumcodereview.appspot.com/1673383002/diff/20001/media/cdm/cdm_bu...
media/cdm/cdm_buffer_allocator.h:25: // buffer, and must call Release() when it
is done with the memory.
The Allocate and Release calls exist on PpbBufferAllocator toady. But
PpbBufferAllocator runs in the plugin and we have very little room for smarter
programming :(

Now since we are using scoped_refptr, having to call Release() sounds a weird
requirement. Usually if we want something like this, Release() will be
automatically called in the destructor of CdmBuffer.

https://chromiumcodereview.appspot.com/1673383002/diff/20001/media/cdm/cdm_bu...
media/cdm/cdm_buffer_allocator.h:32: virtual scoped_ptr<CdmVideoFrame>
CreateCdmVideoFrame() = 0;
This class can create a VideoFrame now. Maybe we should rename this class to
just CdmAllocator.

https://chromiumcodereview.appspot.com/1673383002/diff/20001/media/cdm/cdm_vi...
File media/cdm/cdm_video_frame.cc (right):

https://chromiumcodereview.appspot.com/1673383002/diff/20001/media/cdm/cdm_vi...
media/cdm/cdm_video_frame.cc:12: CdmVideoFrame::CdmVideoFrame()
Are these implementations exactly the same as VideoFrameImpl?

Maybe we should just rename VideoFrameImpl to CdmVideoFrame and add
CreateVideoFrame(), which will only be used in the CdmAdapter path. Will that
work?

https://chromiumcodereview.appspot.com/1673383002/diff/20001/media/cdm/cdm_vi...
File media/cdm/cdm_video_frame.h (right):

https://chromiumcodereview.appspot.com/1673383002/diff/20001/media/cdm/cdm_vi...
media/cdm/cdm_video_frame.h:40: gfx::Size natural_size) = 0;
After this call, what's the state of |this|? Can we call this again?

https://chromiumcodereview.appspot.com/1673383002/diff/20001/media/cdm/simple...
File media/cdm/simple_cdm_buffer.cc (right):

https://chromiumcodereview.appspot.com/1673383002/diff/20001/media/cdm/simple...
media/cdm/simple_cdm_buffer.cc:32: allocator_->Release(this);
See comment above. Destroy() should call Release() (opposite of AddRef()) to
release the ref-count we manually added in CdmAdapter::Allocate().

This could trigger the destruction of SimpleCdmBuffer if no one else is holding
a reference.

Then in the destructor, we can notify the Allocator. The allocator can recycle
the internal buffer (e.g. shared memory) if necessary.

Note that we don't have to pass in the |allocator| explicitly. Instead  we can
have a destruction callback which will be called in the dtor of the CdmBuffer.
This is similar to the destruction callback in media::VdieoFrame.

[xhwang, 2016-02-11 19:50:19]

BTW, the CL title is not accurate now, since the allocator can also allocate
video frame.

[jrummell, 2016-02-11 22:00:18]

Description was changed from

==========
Add allocator interface for use when cdm_adapter needs memory

Rather than allocating a buffer directly, add CdmBufferAllocator
interface that will allocate the buffer when requested. Initial
implementation of CdmBufferAllocator (SimpleCdmBufferAllocator)
simply creates a new buffer every time one is needed. There is
no reuse, since it will only be used for testing purposes.

CdmBufferAllocator also has an interface to create a CdmVideoFrame,
which is a cdm::VideoFrame with the additional ability to create
a media::VideoFrame using the data returned by the CDM.

BUG=510088
TEST=Updated tests pass
==========

to

==========
Add allocator interface for use by cdm_adapter

Rather than allocating a buffer directly, add CdmAllocator interface
that will allocate buffers as needed. Initial implementation of
CdmAllocator (SimpleCdmAllocator) simply creates a new buffer every
time one is needed. There is no reuse, since it will only be used
for testing purposes.

CdmAllocator also has an interface to create a cdm::VideoFrame,
which now has the additional ability to create a media::VideoFrame
using the data returned by the CDM.

BUG=510088
TEST=Updated tests pass
==========

[jrummell, 2016-02-11 22:08:16]

Updated (code and description).

https://codereview.chromium.org/1673383002/diff/20001/media/cdm/cdm_buffer_al...
File media/cdm/cdm_buffer_allocator.h (right):

https://codereview.chromium.org/1673383002/diff/20001/media/cdm/cdm_buffer_al...
media/cdm/cdm_buffer_allocator.h:25: // buffer, and must call Release() when it
is done with the memory.
On 2016/02/11 19:24:13, xhwang wrote:
> The Allocate and Release calls exist on PpbBufferAllocator toady. But
> PpbBufferAllocator runs in the plugin and we have very little room for smarter
> programming :(
> 
> Now since we are using scoped_refptr, having to call Release() sounds a weird
> requirement. Usually if we want something like this, Release() will be
> automatically called in the destructor of CdmBuffer.
> 

Changed to simply return cdm::Buffer. No more refptr.

https://codereview.chromium.org/1673383002/diff/20001/media/cdm/cdm_buffer_al...
media/cdm/cdm_buffer_allocator.h:32: virtual scoped_ptr<CdmVideoFrame>
CreateCdmVideoFrame() = 0;
On 2016/02/11 19:24:13, xhwang wrote:
> This class can create a VideoFrame now. Maybe we should rename this class to
> just CdmAllocator.

Done.

https://codereview.chromium.org/1673383002/diff/20001/media/cdm/cdm_video_fra...
File media/cdm/cdm_video_frame.cc (right):

https://codereview.chromium.org/1673383002/diff/20001/media/cdm/cdm_video_fra...
media/cdm/cdm_video_frame.cc:12: CdmVideoFrame::CdmVideoFrame()
On 2016/02/11 19:24:13, xhwang wrote:
> Are these implementations exactly the same as VideoFrameImpl?
> 
> Maybe we should just rename VideoFrameImpl to CdmVideoFrame and add
> CreateVideoFrame(), which will only be used in the CdmAdapter path. Will that
> work?

Done (conditionally).

https://codereview.chromium.org/1673383002/diff/20001/media/cdm/cdm_video_fra...
File media/cdm/cdm_video_frame.h (right):

https://codereview.chromium.org/1673383002/diff/20001/media/cdm/cdm_video_fra...
media/cdm/cdm_video_frame.h:40: gfx::Size natural_size) = 0;
On 2016/02/11 19:24:14, xhwang wrote:
> After this call, what's the state of |this|? Can we call this again?

Updated comment. FrameBuffer is transferred to become owned by the created
VideoFrame.

https://codereview.chromium.org/1673383002/diff/20001/media/cdm/simple_cdm_bu...
File media/cdm/simple_cdm_buffer.cc (right):

https://codereview.chromium.org/1673383002/diff/20001/media/cdm/simple_cdm_bu...
media/cdm/simple_cdm_buffer.cc:32: allocator_->Release(this);
On 2016/02/11 19:24:14, xhwang wrote:
> See comment above. Destroy() should call Release() (opposite of AddRef()) to
> release the ref-count we manually added in CdmAdapter::Allocate().
> 
> This could trigger the destruction of SimpleCdmBuffer if no one else is
holding
> a reference.
> 
> Then in the destructor, we can notify the Allocator. The allocator can recycle
> the internal buffer (e.g. shared memory) if necessary.
> 
> Note that we don't have to pass in the |allocator| explicitly. Instead  we can
> have a destruction callback which will be called in the dtor of the CdmBuffer.
> This is similar to the destruction callback in media::VdieoFrame.

No longer ref-counted, so we can simply call the destructor.

[xhwang, 2016-02-12 08:45:30]

Looking really nice!

I just have some comments, mostly nits.

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/cdm_ad...
File media/cdm/cdm_adapter.h (right):

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/cdm_ad...
media/cdm/cdm_adapter.h:44: // |cdm_config|. |allocator| is to be used whenever
the CDM needs memory.
Also used to create VideoFrame?

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/cdm_al...
File media/cdm/cdm_allocator.h (right):

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/cdm_al...
media/cdm/cdm_allocator.h:28: virtual cdm::Buffer* Allocate(uint32_t capacity) =
0;
nit: "Allocate" is from existing code. But it seems this should really be
CreateCdmBuffer(), given we also have CreateCdmVideoFrame(). Feel free to ignore
this comment though since naming is hard ;)

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/cdm_he...
File media/cdm/cdm_helpers.h (right):

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/cdm_he...
media/cdm/cdm_helpers.h:61: // The returned object will own the memory in
FrameBuffer and will be
nit: s/the memory in FrameBuffer/|frame_buffer_| ?

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/cdm_he...
media/cdm/cdm_helpers.h:64: // will no longer have a reference to the memory.
nit: s/the VideoFrameImpl will no longer have a reference to the
memory/|frame_buffer_| will be cleared/.

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/cdm_he...
media/cdm/cdm_helpers.h:66: gfx::Size natural_size) = 0;
nit: Actually, this is not a typical "Create" call. It's more like "move" or
"convert" this into a media::VideoFrame. 
How about something like TransformToVideoFrame()?

Also in the comment we need to make the precondition and postcondition clear,
something like the precondition is that |this| needs to be initialized. The post
condition is that |this| will no longer own the buffer, etc.

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/simple...
File media/cdm/simple_cdm_allocator.cc (right):

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/simple...
media/cdm/simple_cdm_allocator.cc:15: class SimpleCdmVideoFrame : public
VideoFrameImpl {
Put this in an anonymous namespace.

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/simple...
media/cdm/simple_cdm_allocator.cc:22: gfx::Size natural_size) override {
s/override/final to be consistent

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/simple...
media/cdm/simple_cdm_allocator.cc:38: base::Bind(&SimpleCdmBuffer::Destroy,
base::Unretained(buffer)));
This is a little bit tricky. Can we add a unit test to cover it?

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/simple...
File media/cdm/simple_cdm_allocator.h (right):

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/simple...
media/cdm/simple_cdm_allocator.h:18: // of the memory allocated. The caller is
responsible for calling Release()
which Release()?

https://chromiumcodereview.appspot.com/1673383002/diff/40001/media/cdm/simple...
media/cdm/simple_cdm_allocator.h:23: ~SimpleCdmAllocator() override;
nit: s/override/final to be consistent.

[jrummell, 2016-02-12 23:14:20]

Updated.

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/cdm_adapter.h
File media/cdm/cdm_adapter.h (right):

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/cdm_adapter.h...
media/cdm/cdm_adapter.h:44: // |cdm_config|. |allocator| is to be used whenever
the CDM needs memory.
On 2016/02/12 08:45:29, xhwang wrote:
> Also used to create VideoFrame?

Done.

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/cdm_allocator.h
File media/cdm/cdm_allocator.h (right):

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/cdm_allocator...
media/cdm/cdm_allocator.h:28: virtual cdm::Buffer* Allocate(uint32_t capacity) =
0;
On 2016/02/12 08:45:30, xhwang wrote:
> nit: "Allocate" is from existing code. But it seems this should really be
> CreateCdmBuffer(), given we also have CreateCdmVideoFrame(). Feel free to
ignore
> this comment though since naming is hard ;)

I like the consistency. Done.

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/cdm_helpers.h
File media/cdm/cdm_helpers.h (right):

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/cdm_helpers.h...
media/cdm/cdm_helpers.h:61: // The returned object will own the memory in
FrameBuffer and will be
On 2016/02/12 08:45:30, xhwang wrote:
> nit: s/the memory in FrameBuffer/|frame_buffer_| ?

Done.

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/cdm_helpers.h...
media/cdm/cdm_helpers.h:64: // will no longer have a reference to the memory.
On 2016/02/12 08:45:30, xhwang wrote:
> nit: s/the VideoFrameImpl will no longer have a reference to the
> memory/|frame_buffer_| will be cleared/.

Removed the whole sentence since it simply restates what the second sentence
says.

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/cdm_helpers.h...
media/cdm/cdm_helpers.h:66: gfx::Size natural_size) = 0;
On 2016/02/12 08:45:30, xhwang wrote:
> nit: Actually, this is not a typical "Create" call. It's more like "move" or
> "convert" this into a media::VideoFrame. 
> How about something like TransformToVideoFrame()?
> 
> Also in the comment we need to make the precondition and postcondition clear,
> something like the precondition is that |this| needs to be initialized. The
post
> condition is that |this| will no longer own the buffer, etc.

Done.

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/simple_cdm_al...
File media/cdm/simple_cdm_allocator.cc (right):

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/simple_cdm_al...
media/cdm/simple_cdm_allocator.cc:15: class SimpleCdmVideoFrame : public
VideoFrameImpl {
On 2016/02/12 08:45:30, xhwang wrote:
> Put this in an anonymous namespace.

Done.

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/simple_cdm_al...
media/cdm/simple_cdm_allocator.cc:22: gfx::Size natural_size) override {
On 2016/02/12 08:45:30, xhwang wrote:
> s/override/final to be consistent

Done.

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/simple_cdm_al...
media/cdm/simple_cdm_allocator.cc:38: base::Bind(&SimpleCdmBuffer::Destroy,
base::Unretained(buffer)));
On 2016/02/12 08:45:30, xhwang wrote:
> This is a little bit tricky. Can we add a unit test to cover it?

Done.

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/simple_cdm_al...
File media/cdm/simple_cdm_allocator.h (right):

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/simple_cdm_al...
media/cdm/simple_cdm_allocator.h:18: // of the memory allocated. The caller is
responsible for calling Release()
On 2016/02/12 08:45:30, xhwang wrote:
> which Release()?

s/b Destroy(). Done.

https://codereview.chromium.org/1673383002/diff/40001/media/cdm/simple_cdm_al...
media/cdm/simple_cdm_allocator.h:23: ~SimpleCdmAllocator() override;
On 2016/02/12 08:45:30, xhwang wrote:
> nit: s/override/final to be consistent.

Done.

[xhwang, 2016-02-12 23:26:08]

LGTM with nits. Thanks!

https://codereview.chromium.org/1673383002/diff/60001/media/cdm/cdm_allocator.h
File media/cdm/cdm_allocator.h (right):

https://codereview.chromium.org/1673383002/diff/60001/media/cdm/cdm_allocator...
media/cdm/cdm_allocator.h:26: // Allocates a buffer with at least |capacity|
bytes. Caller is required to
"Allocate" doesn't quite fit here.

https://codereview.chromium.org/1673383002/diff/60001/media/cdm/simple_cdm_al...
File media/cdm/simple_cdm_allocator.h (right):

https://codereview.chromium.org/1673383002/diff/60001/media/cdm/simple_cdm_al...
media/cdm/simple_cdm_allocator.h:25: // and takes over ownership of the
FrameBuffer.
These seem to be implementation details that belong to the .cc file.

[jrummell, 2016-02-13 00:02:43]

Thanks for the reviews.

https://codereview.chromium.org/1673383002/diff/60001/media/cdm/cdm_allocator.h
File media/cdm/cdm_allocator.h (right):

https://codereview.chromium.org/1673383002/diff/60001/media/cdm/cdm_allocator...
media/cdm/cdm_allocator.h:26: // Allocates a buffer with at least |capacity|
bytes. Caller is required to
On 2016/02/12 23:26:08, xhwang wrote:
> "Allocate" doesn't quite fit here.

Done.

https://codereview.chromium.org/1673383002/diff/60001/media/cdm/simple_cdm_al...
File media/cdm/simple_cdm_allocator.h (right):

https://codereview.chromium.org/1673383002/diff/60001/media/cdm/simple_cdm_al...
media/cdm/simple_cdm_allocator.h:25: // and takes over ownership of the
FrameBuffer.
On 2016/02/12 23:26:08, xhwang wrote:
> These seem to be implementation details that belong to the .cc file.

Done.

[jrummell, 2016-02-13 00:02:47]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-02-13 00:02:48]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1673383002/#ps80001
(title: "nits")

[commit-bot: I haz the power, 2016-02-13 00:04:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673383002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673383002/80001

[commit-bot: I haz the power, 2016-02-13 01:15:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-13 01:15:55]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[jrummell, 2016-02-16 18:51:30]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-02-16 18:51:30]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1673383002/#ps100001
(title: "NON_EXPORTED_BASE()")

[commit-bot: I haz the power, 2016-02-16 18:56:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673383002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673383002/100001

[commit-bot: I haz the power, 2016-02-16 20:01:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-16 20:01:39]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[jrummell, 2016-02-17 20:16:50]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-02-17 20:16:51]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1673383002/#ps140001
(title: "add MEDIA_CDM_EXPORT for Windows")

[commit-bot: I haz the power, 2016-02-17 20:18:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1673383002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1673383002/140001

[commit-bot: I haz the power, 2016-02-17 21:38:02]

Description was changed from

==========
Add allocator interface for use by cdm_adapter

Rather than allocating a buffer directly, add CdmAllocator interface
that will allocate buffers as needed. Initial implementation of
CdmAllocator (SimpleCdmAllocator) simply creates a new buffer every
time one is needed. There is no reuse, since it will only be used
for testing purposes.

CdmAllocator also has an interface to create a cdm::VideoFrame,
which now has the additional ability to create a media::VideoFrame
using the data returned by the CDM.

BUG=510088
TEST=Updated tests pass
==========

to

==========
Add allocator interface for use by cdm_adapter

Rather than allocating a buffer directly, add CdmAllocator interface
that will allocate buffers as needed. Initial implementation of
CdmAllocator (SimpleCdmAllocator) simply creates a new buffer every
time one is needed. There is no reuse, since it will only be used
for testing purposes.

CdmAllocator also has an interface to create a cdm::VideoFrame,
which now has the additional ability to create a media::VideoFrame
using the data returned by the CDM.

BUG=510088
TEST=Updated tests pass
==========

[commit-bot: I haz the power, 2016-02-17 21:38:03]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-17 21:40:23]

Description was changed from

==========
Add allocator interface for use by cdm_adapter

Rather than allocating a buffer directly, add CdmAllocator interface
that will allocate buffers as needed. Initial implementation of
CdmAllocator (SimpleCdmAllocator) simply creates a new buffer every
time one is needed. There is no reuse, since it will only be used
for testing purposes.

CdmAllocator also has an interface to create a cdm::VideoFrame,
which now has the additional ability to create a media::VideoFrame
using the data returned by the CDM.

BUG=510088
TEST=Updated tests pass
==========

to

==========
Add allocator interface for use by cdm_adapter

Rather than allocating a buffer directly, add CdmAllocator interface
that will allocate buffers as needed. Initial implementation of
CdmAllocator (SimpleCdmAllocator) simply creates a new buffer every
time one is needed. There is no reuse, since it will only be used
for testing purposes.

CdmAllocator also has an interface to create a cdm::VideoFrame,
which now has the additional ability to create a media::VideoFrame
using the data returned by the CDM.

BUG=510088
TEST=Updated tests pass

Committed: https://crrev.com/550e1c056d4456000cb4be88676d6f5f01b9efdb
Cr-Commit-Position: refs/heads/master@{#375987}
==========

[commit-bot: I haz the power, 2016-02-17 21:40:24]

Patchset 8 (id:??) landed as
https://crrev.com/550e1c056d4456000cb4be88676d6f5f01b9efdb
Cr-Commit-Position: refs/heads/master@{#375987}