VideoCapture: add support for GpuMemoryBuffer allocation and lifetime mgmt in VideoCaptureBufferPool

VideoCapture: add support for GpuMemoryBuffer allocation and lifetime mgmt in VideoCaptureBufferPool & relatives

- This CL adds support for GpuMemoryBufferTracker in
addition to the existing SharedMemTracker.

- A GpuMemoryBufferTracker owns a GpuMemoryBuffer,
which can only be accessed byte-wise between Map() and
Unmap().

- For that reason, instead of making a VCBP::Tracker
directly accessible via |void* data| and |size|, a
BufferHandle abstraction is introduced.

- VideoCaptureDeviceClient then defines its own
AutoReleaseBuffer in terms of this new BufferHandle.
This BufferHandle is supposed to stay Map()ed and
ready to use for the duration of its lifetime.

Summing up:
 * VideoCaptureBufferPool has a pool of Trackers,
 * from the VCBP we can get a BufferHandle per |buffer_id|,
 * from a BufferHandle we can obtain data(), valid
 and Map()ed as long as the BufferHandle itself exists.

- VideoCaptureDevice::ReserveOutputBuffer() changes to
return a scoped_ptr<Buffer> ISO a scoped_refptr.

- Added VideoCaptureDevice::OnIncomingCapturedBuffer()
to pass directly a Buffer with the capture contents.

- video_capture_texture_wrapper.{cc,h} is removed and
its contents are merged into VideoCaptureDeviceClient.

- FakeVideoCaptureDevice learns how to produce
GpuMemoryBuffers. Also, PIXEL_FORMAT_GPUMEMORYBUFFER
is added.

Summing up, VCDClient accepts:
 * memory backed buffers (the usual)
 * texture-backed buffers
 * GMB-backed buffers (these are wrapped into a Texture
  VideoFrame indistinguishable from the previous case)

- VideoCaptureTextureWrapper files are gone and
its contents folded into VideoCaptureDeviceClient.

- Unittests are updated, and so is FakeVideoCaptureDevice.

BUG=440843
Committed: https://crrev.com/abeb15fa955c570cd481accb04c2a677eb2865a9
Cr-Commit-Position: refs/heads/master@{#326171}

[mcasas, 2015-04-09 01:59:37]

Patchset #2 (id:20001) has been deleted

[mcasas, 2015-04-09 01:59:42]

Patchset #1 (id:1) has been deleted

[mcasas, 2015-04-09 15:38:08]

Patchset #1 (id:40001) has been deleted

[mcasas, 2015-04-10 02:34:24]

Patchset #1 (id:60001) has been deleted

[mcasas, 2015-04-10 02:34:43]

Patchset #1 (id:80001) has been deleted

[mcasas, 2015-04-10 02:34:53]

Patchset #1 (id:100001) has been deleted

[mcasas, 2015-04-10 21:59:06]

Patchset #1 (id:120001) has been deleted

[mcasas, 2015-04-10 21:59:18]

Patchset #1 (id:140001) has been deleted

[mcasas, 2015-04-10 21:59:26]

Patchset #1 (id:160001) has been deleted

[mcasas, 2015-04-11 00:08:48]

Patchset #1 (id:180001) has been deleted

[mcasas, 2015-04-11 00:08:58]

Patchset #1 (id:200001) has been deleted

[mcasas, 2015-04-11 00:11:01]

mcasas@chromium.org changed reviewers:
+ emircan@chromium.org, miu@chromium.org

[mcasas, 2015-04-11 00:11:02]

miu@, emircan@ PTAL.

[mcasas, 2015-04-11 00:12:37]

mcasas@chromium.org changed reviewers:
+ avi@chromium.org

[mcasas, 2015-04-11 00:12:37]

avi@: content/public/common/content_switches.{cc,h},
removing the test/experimental flag as promised.

[mcasas, 2015-04-11 02:08:28]

Patchset #1 (id:220001) has been deleted

[Avi (use Gerrit), 2015-04-13 21:11:10]

Flag removal LGTM; thank you!

[emircan, 2015-04-13 21:31:30]

https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:55:
gmb_->Map(&data_);
Map() can fill multiple ptrs. Since you are creating
gfx::GpuMemoryBuffer::BGRA_8888 it works fine now, but if internals of
GpuMemoryBuffer changes, it would cause issues. If given gmb.format is
multiplanar, it will try to fill >1 values on &data_ and possible overwrite some
memory. 

We can add a num_planes variable to keep track of the planes.
size_t num_planes =
GpuMemoryBufferImpl::NumberOfPlanesForGpuMemoryBufferFormat(gmb->GetFormat());
And create a data array.
scoped_ptr<void*[]> mapped_data_(new void*[num_planes]);
Then you can use the first plane as your mapped value.

https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:184:
dimensions);
How is this guaranteed to correspond to the allocated size of GpuMemoryBuffer?
AFAIK, in different implementations of GpuMemoryBuffer, there are different
stride and size methods. Is there need to define some common methods?

[miu, 2015-04-15 19:12:40]

Initial design-level comments:

https://codereview.chromium.org/1064963002/diff/240001/content/browser/media/...
File content/browser/media/capture/content_video_capture_device_core.cc (right):

https://codereview.chromium.org/1064963002/diff/240001/content/browser/media/...
content/browser/media/capture/content_video_capture_device_core.cc:121: const
scoped_ptr<media::DataHandle> scoped_data_handle(
Why is the DataHandle allowed to be destroyed while VideoFrame is still
referencing the data pointer?  Seems like the DataHandle should remain alive
(i.e., data is not yet unmapped) until all references to the data pointer are
gone.

https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
File content/browser/renderer_host/media/video_capture_texture_wrapper.cc
(left):

https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
content/browser/renderer_host/media/video_capture_texture_wrapper.cc:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
This file is marked "M" instead of "D" in the patch set.  Can you confirm the
file was actually deleted by the diff and there isn't still an empty file left
over?

https://codereview.chromium.org/1064963002/diff/240001/media/base/video_captu...
File media/base/video_capture_types.cc (right):

https://codereview.chromium.org/1064963002/diff/240001/media/base/video_captu...
media/base/video_capture_types.cc:99: case PIXEL_FORMAT_GPUMEMORYBUFFER:
This isn't a pixel format (and neither is TEXTURE), unless GPUMEMORYBUFFER is
guaranteed to always be ARGB format.  If that's the case, it should be
documented in a comment here.

BTW--Most graphics cards/drivers are optimized for BGRA, so consider leaving
open the option for using it.

https://codereview.chromium.org/1064963002/diff/240001/media/base/video_captu...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1064963002/diff/240001/media/base/video_captu...
media/base/video_capture_types.h:102: // A class to wrap access to an udnerlying
data pointer. Derived classes can
s/udnerlying/underlying/

https://codereview.chromium.org/1064963002/diff/240001/media/video/capture/vi...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1064963002/diff/240001/media/video/capture/vi...
media/video/capture/video_capture_device.h:203: virtual scoped_ptr<DataHandle>
GetDataHandle() = 0;
Having a second level of acquiring a buffer seems overly-complicated to me.  All
callers of ReserveOutputBuffer() will just immediately call GetDataHandle(), so
can we make a Buffer already provide a pointer to mapped memory?  The memory
could be unnmapped when the Buffer is passed back to the VCDClient.

I see that Buffers are ref-counted.  Would it be possible to get rid of the
ref-counting, and instead pass ownership to the caller of ReserveOutputBuffer(),
then pass ownership back to the VCDClient when calling OnIncomingCapturedXXX()? 
When ReserveOutputBuffer() is called, VCDClient can map the memory before
returning.  When OnIncomingCapturedXXX() is called, the memory can be unmapped.

Since Buffer pooling is a concern, you may want to make sure that Buffers
destroyed external to the VCDClient don't actually destroy their underlying
memory, but just return it to the pool.

WDYT of these ideas?

[mcasas, 2015-04-16 02:25:11]

Patchset #2 (id:260001) has been deleted

[mcasas, 2015-04-16 03:11:22]

miu@, emircan@ PTAL.

(there are a few comments on my own), please ignore.

https://codereview.chromium.org/1064963002/diff/240001/content/browser/media/...
File content/browser/media/capture/content_video_capture_device_core.cc (right):

https://codereview.chromium.org/1064963002/diff/240001/content/browser/media/...
content/browser/media/capture/content_video_capture_device_core.cc:121: const
scoped_ptr<media::DataHandle> scoped_data_handle(
On 2015/04/15 19:12:40, miu wrote:
> Why is the DataHandle allowed to be destroyed while VideoFrame is still
> referencing the data pointer?  Seems like the DataHandle should remain alive
> (i.e., data is not yet unmapped) until all references to the data pointer are
> gone.

Since I'm taking in your suggestion below, this
code is changed and this lifetime bug is no more.

https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:55:
gmb_->Map(&data_);
On 2015/04/13 21:31:30, emircan wrote:
> Map() can fill multiple ptrs. Since you are creating
> gfx::GpuMemoryBuffer::BGRA_8888 it works fine now, but if internals of
> GpuMemoryBuffer changes, it would cause issues. If given gmb.format is
> multiplanar, it will try to fill >1 values on &data_ and possible overwrite
some
> memory. 
> 
> We can add a num_planes variable to keep track of the planes.
> size_t num_planes =
> GpuMemoryBufferImpl::NumberOfPlanesForGpuMemoryBufferFormat(gmb->GetFormat());
> And create a data array.
> scoped_ptr<void*[]> mapped_data_(new void*[num_planes]);
> Then you can use the first plane as your mapped value.

Done.

https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:184:
dimensions);
On 2015/04/13 21:31:30, emircan wrote:
> How is this guaranteed to correspond to the allocated size of GpuMemoryBuffer?
> AFAIK, in different implementations of GpuMemoryBuffer, there are different
> stride and size methods. Is there need to define some common methods?

Changed to calculating using GpuMemoryBuffer methods.

https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
File content/browser/renderer_host/media/video_capture_texture_wrapper.cc
(left):

https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
content/browser/renderer_host/media/video_capture_texture_wrapper.cc:1: //
Copyright 2015 The Chromium Authors. All rights reserved.
On 2015/04/15 19:12:40, miu wrote:
> This file is marked "M" instead of "D" in the patch set.  Can you confirm the
> file was actually deleted by the diff and there isn't still an empty file left
> over?

It's deleted in my local repo, and the diff 
shows all lines removed. Strange...

https://codereview.chromium.org/1064963002/diff/240001/media/base/video_captu...
File media/base/video_capture_types.cc (right):

https://codereview.chromium.org/1064963002/diff/240001/media/base/video_captu...
media/base/video_capture_types.cc:99: case PIXEL_FORMAT_GPUMEMORYBUFFER:
On 2015/04/15 19:12:40, miu wrote:
> This isn't a pixel format (and neither is TEXTURE), unless GPUMEMORYBUFFER is
> guaranteed to always be ARGB format.  If that's the case, it should be
> documented in a comment here.
> 
> BTW--Most graphics cards/drivers are optimized for BGRA, so consider leaving
> open the option for using it.

Done.

https://codereview.chromium.org/1064963002/diff/240001/media/base/video_captu...
File media/base/video_capture_types.h (right):

https://codereview.chromium.org/1064963002/diff/240001/media/base/video_captu...
media/base/video_capture_types.h:102: // A class to wrap access to an udnerlying
data pointer. Derived classes can
On 2015/04/15 19:12:40, miu wrote:
> s/udnerlying/underlying/

Done.

https://codereview.chromium.org/1064963002/diff/240001/media/video/capture/vi...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1064963002/diff/240001/media/video/capture/vi...
media/video/capture/video_capture_device.h:203: virtual scoped_ptr<DataHandle>
GetDataHandle() = 0;
On 2015/04/15 19:12:40, miu wrote:
> Having a second level of acquiring a buffer seems overly-complicated to me. 
All
> callers of ReserveOutputBuffer() will just immediately call GetDataHandle(),
so
> can we make a Buffer already provide a pointer to mapped memory?  The memory
> could be unnmapped when the Buffer is passed back to the VCDClient.
> 
> I see that Buffers are ref-counted.  Would it be possible to get rid of the
> ref-counting, and instead pass ownership to the caller of
ReserveOutputBuffer(),
> then pass ownership back to the VCDClient when calling
OnIncomingCapturedXXX()? 
> When ReserveOutputBuffer() is called, VCDClient can map the memory before
> returning.  When OnIncomingCapturedXXX() is called, the memory can be
unmapped.
> 

SG, with that assumption in mind and after double checking
with GMB folks, it seems reasonable that the Buffer's 
underlying BufferHandle stays Map()ed for the duration of
the client loop ReserveOutputBuffer()-OnIncomingCapturedX().

> Since Buffer pooling is a concern, you may want to make sure that Buffers
> destroyed external to the VCDClient don't actually destroy their underlying
> memory, but just return it to the pool.
> 
> WDYT of these ideas?

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:41: // A simple
holder of a memory-backed buffer and a factory of SimpleDataHandles
Update comment.

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:138: DVLOG(1)
<< "allocating ShMem of " << dimensions.ToString() << "B";
Remove |<< "B"|

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:160: DVLOG(1)
<< "allocating GMB for " << dimensions.ToString() << "B";
Remove |<< "B"|

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:145:
capture_task_runner, format));
|format| is unused, remove.

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:135: // Do
not relinquish the underlying pool reservation if |kep_alive_|.
s/kep_alive/keep_alive/

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:568: this,
image_id, texture_id /* gpu_memory_buffer */ )),
remove /*...*/

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.h (right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.h:79: //
|capture_format| on constructor specifies PIXEL_FORMAT_GPUMEMORYBUFFER.
Update comment.

[mcasas, 2015-04-16 03:13:15]

On 2015/04/16 03:11:22, mcasas wrote:
> miu@, emircan@ PTAL.
> 
> (there are a few comments on my own), please ignore.
> 
>
https://codereview.chromium.org/1064963002/diff/240001/content/browser/media/...
> File content/browser/media/capture/content_video_capture_device_core.cc
(right):
> 
>
https://codereview.chromium.org/1064963002/diff/240001/content/browser/media/...
> content/browser/media/capture/content_video_capture_device_core.cc:121: const
> scoped_ptr<media::DataHandle> scoped_data_handle(
> On 2015/04/15 19:12:40, miu wrote:
> > Why is the DataHandle allowed to be destroyed while VideoFrame is still
> > referencing the data pointer?  Seems like the DataHandle should remain alive
> > (i.e., data is not yet unmapped) until all references to the data pointer
are
> > gone.
> 
> Since I'm taking in your suggestion below, this
> code is changed and this lifetime bug is no more.
> 
>
https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
> File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):
> 
>
https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
> content/browser/renderer_host/media/video_capture_buffer_pool.cc:55:
> gmb_->Map(&data_);
> On 2015/04/13 21:31:30, emircan wrote:
> > Map() can fill multiple ptrs. Since you are creating
> > gfx::GpuMemoryBuffer::BGRA_8888 it works fine now, but if internals of
> > GpuMemoryBuffer changes, it would cause issues. If given gmb.format is
> > multiplanar, it will try to fill >1 values on &data_ and possible overwrite
> some
> > memory. 
> > 
> > We can add a num_planes variable to keep track of the planes.
> > size_t num_planes =
> >
GpuMemoryBufferImpl::NumberOfPlanesForGpuMemoryBufferFormat(gmb->GetFormat());
> > And create a data array.
> > scoped_ptr<void*[]> mapped_data_(new void*[num_planes]);
> > Then you can use the first plane as your mapped value.
> 
> Done.
> 
>
https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
> content/browser/renderer_host/media/video_capture_buffer_pool.cc:184:
> dimensions);
> On 2015/04/13 21:31:30, emircan wrote:
> > How is this guaranteed to correspond to the allocated size of
GpuMemoryBuffer?
> > AFAIK, in different implementations of GpuMemoryBuffer, there are different
> > stride and size methods. Is there need to define some common methods?
> 
> Changed to calculating using GpuMemoryBuffer methods.
> 
>
https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
> File content/browser/renderer_host/media/video_capture_texture_wrapper.cc
> (left):
> 
>
https://codereview.chromium.org/1064963002/diff/240001/content/browser/render...
> content/browser/renderer_host/media/video_capture_texture_wrapper.cc:1: //
> Copyright 2015 The Chromium Authors. All rights reserved.
> On 2015/04/15 19:12:40, miu wrote:
> > This file is marked "M" instead of "D" in the patch set.  Can you confirm
the
> > file was actually deleted by the diff and there isn't still an empty file
left
> > over?
> 
> It's deleted in my local repo, and the diff 
> shows all lines removed. Strange...
> 
>
https://codereview.chromium.org/1064963002/diff/240001/media/base/video_captu...
> File media/base/video_capture_types.cc (right):
> 
>
https://codereview.chromium.org/1064963002/diff/240001/media/base/video_captu...
> media/base/video_capture_types.cc:99: case PIXEL_FORMAT_GPUMEMORYBUFFER:
> On 2015/04/15 19:12:40, miu wrote:
> > This isn't a pixel format (and neither is TEXTURE), unless GPUMEMORYBUFFER
is
> > guaranteed to always be ARGB format.  If that's the case, it should be
> > documented in a comment here.
> > 
> > BTW--Most graphics cards/drivers are optimized for BGRA, so consider leaving
> > open the option for using it.
> 
> Done.
> 
>
https://codereview.chromium.org/1064963002/diff/240001/media/base/video_captu...
> File media/base/video_capture_types.h (right):
> 
>
https://codereview.chromium.org/1064963002/diff/240001/media/base/video_captu...
> media/base/video_capture_types.h:102: // A class to wrap access to an
udnerlying
> data pointer. Derived classes can
> On 2015/04/15 19:12:40, miu wrote:
> > s/udnerlying/underlying/
> 
> Done.
> 
>
https://codereview.chromium.org/1064963002/diff/240001/media/video/capture/vi...
> File media/video/capture/video_capture_device.h (right):
> 
>
https://codereview.chromium.org/1064963002/diff/240001/media/video/capture/vi...
> media/video/capture/video_capture_device.h:203: virtual scoped_ptr<DataHandle>
> GetDataHandle() = 0;
> On 2015/04/15 19:12:40, miu wrote:
> > Having a second level of acquiring a buffer seems overly-complicated to me. 
> All
> > callers of ReserveOutputBuffer() will just immediately call GetDataHandle(),
> so
> > can we make a Buffer already provide a pointer to mapped memory?  The memory
> > could be unnmapped when the Buffer is passed back to the VCDClient.
> > 
> > I see that Buffers are ref-counted.  Would it be possible to get rid of the
> > ref-counting, and instead pass ownership to the caller of
> ReserveOutputBuffer(),
> > then pass ownership back to the VCDClient when calling
> OnIncomingCapturedXXX()? 
> > When ReserveOutputBuffer() is called, VCDClient can map the memory before
> > returning.  When OnIncomingCapturedXXX() is called, the memory can be
> unmapped.
> > 
> 
> SG, with that assumption in mind and after double checking
> with GMB folks, it seems reasonable that the Buffer's 
> underlying BufferHandle stays Map()ed for the duration of
> the client loop ReserveOutputBuffer()-OnIncomingCapturedX().
> 
> > Since Buffer pooling is a concern, you may want to make sure that Buffers
> > destroyed external to the VCDClient don't actually destroy their underlying
> > memory, but just return it to the pool.
> > 
> > WDYT of these ideas?
> 
>
https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
> File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):
> 
>
https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
> content/browser/renderer_host/media/video_capture_buffer_pool.cc:41: // A
simple
> holder of a memory-backed buffer and a factory of SimpleDataHandles
> Update comment.
> 
>
https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
> content/browser/renderer_host/media/video_capture_buffer_pool.cc:138: DVLOG(1)
> << "allocating ShMem of " << dimensions.ToString() << "B";
> Remove |<< "B"|
> 
>
https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
> content/browser/renderer_host/media/video_capture_buffer_pool.cc:160: DVLOG(1)
> << "allocating GMB for " << dimensions.ToString() << "B";
> Remove |<< "B"|
> 
>
https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
> File content/browser/renderer_host/media/video_capture_controller.cc (right):
> 
>
https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
> content/browser/renderer_host/media/video_capture_controller.cc:145:
> capture_task_runner, format));
> |format| is unused, remove.
> 
>
https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
> File content/browser/renderer_host/media/video_capture_device_client.cc
(right):
> 
>
https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
> content/browser/renderer_host/media/video_capture_device_client.cc:135: // Do
> not relinquish the underlying pool reservation if |kep_alive_|.
> s/kep_alive/keep_alive/
> 
>
https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
> content/browser/renderer_host/media/video_capture_device_client.cc:568: this,
> image_id, texture_id /* gpu_memory_buffer */ )),
> remove /*...*/
> 
>
https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
> File content/browser/renderer_host/media/video_capture_device_client.h
(right):
> 
>
https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
> content/browser/renderer_host/media/video_capture_device_client.h:79: //
> |capture_format| on constructor specifies PIXEL_FORMAT_GPUMEMORYBUFFER.
> Update comment.

I meant, please ignore the few added extra comments added by myself. Parenthesis
played tricks on me ;)

[miu, 2015-04-18 00:22:01]

Still reviewing, but looking good through to video_capture_controller.cc...

https://codereview.chromium.org/1064963002/diff/280001/content/browser/media/...
File content/browser/media/capture/desktop_capture_device_aura_unittest.cc
(right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/media/...
content/browser/media/capture/desktop_capture_device_aura_unittest.cc:61: const
gfx::Size& dimensions) override {
nit: indent

https://codereview.chromium.org/1064963002/diff/280001/content/browser/media/...
File content/browser/media/capture/desktop_capture_device_unittest.cc (right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/media/...
content/browser/media/capture/desktop_capture_device_unittest.cc:79: const
gfx::Size& dimensions) override {
nit: indent

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc
(right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc:299:
INSTANTIATE_TEST_CASE_P(,
Nice!

https://codereview.chromium.org/1064963002/diff/280001/media/video/capture/vi...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1064963002/diff/280001/media/video/capture/vi...
media/video/capture/video_capture_device.h:201: virtual ~Buffer() = 0;
Did you mean for this to be pure virtual?  You define the method in the .cc
file...

[mcasas, 2015-04-20 17:25:46]

mcasas@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[mcasas, 2015-04-20 17:25:48]

miu@ please continue review when ready.

dalecurtis@: media/base/video_capture_types.*

https://codereview.chromium.org/1064963002/diff/280001/content/browser/media/...
File content/browser/media/capture/desktop_capture_device_aura_unittest.cc
(right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/media/...
content/browser/media/capture/desktop_capture_device_aura_unittest.cc:61: const
gfx::Size& dimensions) override {
On 2015/04/18 00:22:01, miu wrote:
> nit: indent

Done.

https://codereview.chromium.org/1064963002/diff/280001/content/browser/media/...
File content/browser/media/capture/desktop_capture_device_unittest.cc (right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/media/...
content/browser/media/capture/desktop_capture_device_unittest.cc:79: const
gfx::Size& dimensions) override {
On 2015/04/18 00:22:01, miu wrote:
> nit: indent

Done.

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:41: // A simple
holder of a memory-backed buffer and a factory of SimpleDataHandles
On 2015/04/16 03:11:22, mcasas wrote:
> Update comment.

Done.

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:138: DVLOG(1)
<< "allocating ShMem of " << dimensions.ToString() << "B";
On 2015/04/16 03:11:22, mcasas wrote:
> Remove |<< "B"|

Done.

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:160: DVLOG(1)
<< "allocating GMB for " << dimensions.ToString() << "B";
On 2015/04/16 03:11:22, mcasas wrote:
> Remove |<< "B"|

Done.

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc
(right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool_unittest.cc:299:
INSTANTIATE_TEST_CASE_P(,
On 2015/04/18 00:22:01, miu wrote:
> Nice!

Acknowledged.

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_controller.cc:145:
capture_task_runner, format));
On 2015/04/16 03:11:22, mcasas wrote:
> |format| is unused, remove.

Done.

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:135: // Do
not relinquish the underlying pool reservation if |kep_alive_|.
On 2015/04/16 03:11:22, mcasas wrote:
> s/kep_alive/keep_alive/

Done.

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:568: this,
image_id, texture_id /* gpu_memory_buffer */ )),
On 2015/04/16 03:11:22, mcasas wrote:
> remove /*...*/

Done.

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.h (right):

https://codereview.chromium.org/1064963002/diff/280001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.h:79: //
|capture_format| on constructor specifies PIXEL_FORMAT_GPUMEMORYBUFFER.
On 2015/04/16 03:11:22, mcasas wrote:
> Update comment.

Done.

https://codereview.chromium.org/1064963002/diff/280001/media/video/capture/vi...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1064963002/diff/280001/media/video/capture/vi...
media/video/capture/video_capture_device.h:201: virtual ~Buffer() = 0;
On 2015/04/18 00:22:01, miu wrote:
> Did you mean for this to be pure virtual?  You define the method in the .cc
> file...

A pure virtual destructor still needs an implementation,
sadly enough :(

[mcasas, 2015-04-20 17:25:58]

Patchset #3 (id:300001) has been deleted

[DaleCurtis, 2015-04-20 18:22:46]

media/base lgtm

[mcasas, 2015-04-20 20:30:05]

Patchset #3 (id:320001) has been deleted

[miu, 2015-04-20 20:34:57]

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:168:
//linked_ptr<gfx::GpuMemoryBuffer> memory_buffer,
Please remove commented-out arg.

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:178: const
base::WeakPtr<VideoCaptureController> controller_;
nit: Please comment that controller_ should only be dereferenced on the IO
thread.

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:198: new
TextureWrapperDelegate(controller, capture_task_runner)),
TextureWrapperDelegate should be lazy-created when it is needed since it creates
a lot of heavy-weight objects.  How about instantiating it when
ReserveOutputBuffer() is called for the GPUMEMORYBUFFER format?

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:454:
timestamp));
Looks like frame_format.frame_rate is not going to be set in the VideoFrame
created downstream.  Can you pass that along and set the VideoFrameMetadata
field downstream?

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:530: if
(!gl_helper_)
Consider adding a comment here about the frame being dropped because
initialization is still in-progress on other threads (or the context has been
lost).

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.h (right):

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.h:29: //
incoming GpuMemoryBuffers into Texture backed Videoframes. This class creates
s/Videoframes/VideoFrames/

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.h:32: // needed
since the frame producer owns VideoCaptureDeviceClient, but this needs
What is "this" referring to?

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.h:78: class
TextureWrapperDelegate;
naming: The old name doesn't seem right now, given how the role and scope of the
class has changed a bit.  How about one of these:

  TextureUploadHelper
  TextureUploadWrapper
  GpuMemoryBufferImporter

https://codereview.chromium.org/1064963002/diff/340001/media/video/capture/vi...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1064963002/diff/340001/media/video/capture/vi...
media/video/capture/video_capture_device.h:201: virtual ~Buffer() {}
Makes sense for the destructor not to be pure virtual.  However, to minimize
potential linker problems on all platforms, I'd suggest you not inline the
destructor, and instead define it in the .cc file.  This will cause the vtable
for the Buffer class to be located in the correct object file, rather than play
"linker roulette" with the different rules on Win vs. Mac vs. Linux.

[mcasas, 2015-04-20 22:24:08]

miu@ PTAL

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:168:
//linked_ptr<gfx::GpuMemoryBuffer> memory_buffer,
On 2015/04/20 20:34:56, miu wrote:
> Please remove commented-out arg.

Done.

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:178: const
base::WeakPtr<VideoCaptureController> controller_;
On 2015/04/20 20:34:56, miu wrote:
> nit: Please comment that controller_ should only be dereferenced on the IO
> thread.

Done.

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:198: new
TextureWrapperDelegate(controller, capture_task_runner)),
On 2015/04/20 20:34:56, miu wrote:
> TextureWrapperDelegate should be lazy-created when it is needed since it
creates
> a lot of heavy-weight objects.  How about instantiating it when
> ReserveOutputBuffer() is called for the GPUMEMORYBUFFER format?

Well spotted. Done.

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:454:
timestamp));
On 2015/04/20 20:34:56, miu wrote:
> Looks like frame_format.frame_rate is not going to be set in the VideoFrame
> created downstream.  Can you pass that along and set the VideoFrameMetadata
> field downstream?

Done.

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.cc:530: if
(!gl_helper_)
On 2015/04/20 20:34:56, miu wrote:
> Consider adding a comment here about the frame being dropped because
> initialization is still in-progress on other threads (or the context has been
> lost).

Done.

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.h (right):

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.h:29: //
incoming GpuMemoryBuffers into Texture backed Videoframes. This class creates
On 2015/04/20 20:34:56, miu wrote:
> s/Videoframes/VideoFrames/

Done.

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.h:32: // needed
since the frame producer owns VideoCaptureDeviceClient, but this needs
On 2015/04/20 20:34:56, miu wrote:
> What is "this" referring to?

Done.

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.h:78: class
TextureWrapperDelegate;
On 2015/04/20 20:34:56, miu wrote:
> naming: The old name doesn't seem right now, given how the role and scope of
the
> class has changed a bit.  How about one of these:
> 
>   TextureUploadHelper
>   TextureUploadWrapper
>   GpuMemoryBufferImporter

What about TextureWrapper? I proceeded with
that naming unless you object.

https://codereview.chromium.org/1064963002/diff/340001/media/video/capture/vi...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1064963002/diff/340001/media/video/capture/vi...
media/video/capture/video_capture_device.h:201: virtual ~Buffer() {}
On 2015/04/20 20:34:56, miu wrote:
> Makes sense for the destructor not to be pure virtual.  However, to minimize
> potential linker problems on all platforms, I'd suggest you not inline the
> destructor, and instead define it in the .cc file.  This will cause the vtable
> for the Buffer class to be located in the correct object file, rather than
play
> "linker roulette" with the different rules on Win vs. Mac vs. Linux.

Heh, I'd rather make the dtor pure virtual with a
separate implementation but Win builds are not happy,
hence the change. (You can see it in 
"win_chromium_compile_dbg_ng" in previous patchsets or
even in http://crrev.com/1005583003 that suffers from
the same problem). All in all, this version seem to
make all bots happy. Should I change it anyway?

[mcasas, 2015-04-20 22:25:54]

Patchset #4 (id:360001) has been deleted

[mcasas, 2015-04-20 22:33:19]

Patchset #4 (id:380001) has been deleted

[miu, 2015-04-21 04:41:24]

lgtm

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.h (right):

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.h:78: class
TextureWrapperDelegate;
On 2015/04/20 22:24:07, mcasas wrote:
> What about TextureWrapper? I proceeded with
> that naming unless you object.

My opinion, but your call: TextureWrapper is confusing because it sounds like an
object that wraps a texture (i.e., the adapter design pattern:
http://en.wikipedia.org/wiki/Adapter_pattern), where there would be a
TextureWrapper created for each buffer being wrapped.  Instead, this class is a
single instance (per VCDClient) that adds supplemental implementation to it. 
That's why I was suggesting names like XXXHelper or XXXImporter.

https://codereview.chromium.org/1064963002/diff/340001/media/video/capture/vi...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1064963002/diff/340001/media/video/capture/vi...
media/video/capture/video_capture_device.h:201: virtual ~Buffer() {}
On 2015/04/20 22:24:08, mcasas wrote:
> Heh, I'd rather make the dtor pure virtual with a
> separate implementation but Win builds are not happy,

Looks good, if this works.  Agree a pure virtual dtor with a default impl would
have been nice.

Just so we're on the same page, what I was proposing earlier was declaring it
as:

  class Buffer {
    public:
      virtual ~Buffer();
    ...
  };

And defining it in the .cc file as:

  VideoCaptureDevice::Client::Buffer::~Buffer() {}

...which I think would result in the vtable for the class being code-generated
exactly once (for video_capture_device.cc).  Because you have the dtor inlined
in the .h file, compiling any .cc file that includes this header may result in
duplicate code generation that the linker has to resolve.

[mcasas, 2015-04-21 21:21:08]

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
File content/browser/renderer_host/media/video_capture_device_client.h (right):

https://codereview.chromium.org/1064963002/diff/340001/content/browser/render...
content/browser/renderer_host/media/video_capture_device_client.h:78: class
TextureWrapperDelegate;
On 2015/04/21 04:41:24, miu wrote:
> On 2015/04/20 22:24:07, mcasas wrote:
> > What about TextureWrapper? I proceeded with
> > that naming unless you object.
> 
> My opinion, but your call: TextureWrapper is confusing because it sounds like
an
> object that wraps a texture (i.e., the adapter design pattern:
> http://en.wikipedia.org/wiki/Adapter_pattern), where there would be a
> TextureWrapper created for each buffer being wrapped.  Instead, this class is
a
> single instance (per VCDClient) that adds supplemental implementation to it. 
> That's why I was suggesting names like XXXHelper or XXXImporter.

I see and agree. Changing to TextureWrapHelper
(if it's still confusing I can change it in
subsequent and proximal CLs).

https://codereview.chromium.org/1064963002/diff/340001/media/video/capture/vi...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1064963002/diff/340001/media/video/capture/vi...
media/video/capture/video_capture_device.h:201: virtual ~Buffer() {}
On 2015/04/21 04:41:24, miu wrote:
> On 2015/04/20 22:24:08, mcasas wrote:
> > Heh, I'd rather make the dtor pure virtual with a
> > separate implementation but Win builds are not happy,
> 
> Looks good, if this works.  Agree a pure virtual dtor with a default impl
would
> have been nice.
> 
> Just so we're on the same page, what I was proposing earlier was declaring it
> as:
> 
>   class Buffer {
>     public:
>       virtual ~Buffer();
>     ...
>   };
> 
> And defining it in the .cc file as:
> 
>   VideoCaptureDevice::Client::Buffer::~Buffer() {}
> 
> ...which I think would result in the vtable for the class being code-generated
> exactly once (for video_capture_device.cc).  Because you have the dtor inlined
> in the .h file, compiling any .cc file that includes this header may result in
> duplicate code generation that the linker has to resolve.

Agree. Let's try the non-pure destructor and
cc-file definition.

https://codereview.chromium.org/1064963002/diff/440001/media/video/capture/vi...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1064963002/diff/440001/media/video/capture/vi...
media/video/capture/video_capture_device.h:201: virtual ~Buffer() {}
I tried:

1) .h
virtual ~Buffer() {}  // works

2) .h
virtual ~Buffer();
.cc
VideoCaptureDevice::Client::Buffer::~Buffer() {}

3) 
.h
virtual ~Buffer();
.cc
VideoCaptureDevice::Client::Buffer::~Buffer() {}

2) and 3) have a linker problem _only in_
|win_chromium_compile_dbg_ng|. I think this is
a similar issue as the one in 
http://crrev.com/1005583003, and it affects only
that bot. Perhaps a .vcxproj problem? I'll
leave it like this and if I sort out the
other CL I'll come back here. Not happy with
this.

[mcasas, 2015-04-21 21:21:17]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2015-04-21 21:21:18]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
dalecurtis@chromium.org, miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1064963002/#ps440001
(title: "Reverted VCD::Client::~Buffer() implemented in .cc file")

[commit-bot: I haz the power, 2015-04-21 21:22:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1064963002/440001

[commit-bot: I haz the power, 2015-04-21 21:34:06]

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

[commit-bot: I haz the power, 2015-04-21 21:34:07]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mcasas, 2015-04-21 22:11:04]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2015-04-21 22:11:04]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
miu@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1064963002/#ps460001
(title: "VideoCaptureDevice::Client::Buffer::~Buffer() pure virtual. Avoid using
base::MessageLoopProxy::cur…")

[commit-bot: I haz the power, 2015-04-21 22:11:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1064963002/460001

[mcasas, 2015-04-21 22:31:18]

The CQ bit was unchecked by mcasas@chromium.org

[mcasas, 2015-04-21 22:32:00]

Patchset #7 (id:460001) has been deleted

[mcasas, 2015-04-21 22:32:07]

The CQ bit was checked by mcasas@chromium.org

[mcasas, 2015-04-21 22:32:08]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
miu@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1064963002/#ps480001
(title: "VideoCaptureDevice::Client::Buffer::~Buffer() pure virtual. Avoid using
base::MessageLoopProxy::cur…")

[commit-bot: I haz the power, 2015-04-21 22:33:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1064963002/480001

[commit-bot: I haz the power, 2015-04-21 23:37:07]

Committed patchset #7 (id:480001)

[commit-bot: I haz the power, 2015-04-21 23:40:01]

Patchset 7 (id:??) landed as
https://crrev.com/abeb15fa955c570cd481accb04c2a677eb2865a9
Cr-Commit-Position: refs/heads/master@{#326171}

[magjed_chromium, 2015-04-22 09:03:11]

A revert of this CL (patchset #7 id:480001) has been created in
https://codereview.chromium.org/1090273006/ by magjed@chromium.org.

The reason for reverting is: The reason for reverting is: Speculatively revert
to address Android Tests (dbg) content_unittests failure:

http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...

content_unittests content_unittests 
failures:
VideoCaptureBufferPoolTest.BufferPool/2
GpuMemoryBufferFactoryTests/GpuMemoryBufferFactoryTest.CreateAndDestroy/1
VideoCaptureBufferPoolTest.BufferPool/0
VideoCaptureBufferPoolTest.BufferPool/1

[ RUN      ] VideoCaptureBufferPoolTest.BufferPool/0
[FATAL:browser_gpu_memory_buffer_manager.cc(56)] Check failed:
!g_gpu_memory_buffer_manager. 
#00 0x764081d3
/data/app-lib/org.chromium.native_test-1/libcontent_unittests.so+0x00fdf1d3
#01 0x75cf95c3
/data/app-lib/org.chromium.native_test-1/libcontent_unittests.so+0x008d05c3
#02 0x75a3c697
/data/app-lib/org.chromium.native_test-1/libcontent_unittests.so+0x00613697
#03 0x75f8d1fb
/data/app-lib/org.chromium.native_test-1/libcontent_unittests.so+0x00b641fb
#04 0x75f8d35b
/data/app-lib/org.chromium.native_test-1/libcontent_unittests.so+0x00b6435b
#05 0x75f8d3e3
/data/app-lib/org.chromium.native_test-1/libcontent_unittests.so+0x00b643e3
#06 0x75f8d5e3
/data/app-lib/org.chromium.native_test-1/libcontent_unittests.so+0x00b645e3
#07 0x75f8c3c7
/data/app-lib/org.chromium.native_test-1/libcontent_unittests.so+0x00b633c7
#08 0x75f49bb7
/data/app-lib/org.chromium.native_test-1/libcontent_unittests.so+0x00b20bb7
#09 0x75f41c8b
/data/app-lib/org.chromium.native_test-1/libcontent_unittests.so+0x00b18c8b
#10 0x75736087
/data/app-lib/org.chromium.native_test-1/libcontent_unittests.so+0x0030d087
#11 0x763de291
/data/app-lib/org.chromium.native_test-1/libcontent_unittests.so+0x00fb5291
#12 0x41503bcf /system/lib/libdvm.so+0x0001dbcf
#13 0x41534125 /system/lib/libdvm.so+0x0004e125
#14 0x4150cfe3 /system/lib/libdvm.so+0x00026fe3
#15 0x41513fa3 /system/lib/libdvm.so+0x0002dfa3
#16 0x4151163b /system/lib/libdvm.so+0x0002b63b
#17 0x41546863 /system/lib/libdvm.so+0x00060863
#18 0x4154e7c5 /system/lib/libdvm.so+0x000687c5
#19 0x4150cfe3 /system/lib/libdvm.so+0x00026fe3
#20 0x41513fa3 /system/lib/libdvm.so+0x0002dfa3
#21 0x4151163b /system/lib/libdvm.so+0x0002b63b
#22 0x4154657f /system/lib/libdvm.so+0x0006057f
#23 0x4152fd0d /system/lib/libdvm.so+0x00049d0d
#24 0x401a82b3 /system/lib/libandroid_runtime.so+0x0004d2b3
#25 0x401a8fd9 /system/lib/libandroid_runtime.so+0x0004dfd9
#26 0x4006b05d /system/bin/app_process+0x0000105d
#27 0x400a234b /system/lib/libc.so+0x0000e34b

This CL is the most suspect in the blame-list because it touches the
VideoCaptureBufferPool..

[magjed_chromium, 2015-04-22 10:42:37]

Reverting this CL did fix the failing unittests so look into that before
relanding this CL.