VideoCaptureBufferPool: Refactor to allow support of non-ShMem backed buffers

VideoCaptureBufferPool: Refactor to allow support of non-ShMem backed buffers

This CL refactors VideoCaptureBufferPool, and in
particular its inner class Buffer, to allow pooling
GpuMemoryBuffers. For that, Buffer is renamed to
Tracker, and turned into a base class, where SharedMemory
specifics are implemented in SharedMemoryTracker.

In ToT, VideoCaptureDeviceClient calculates the
linear size in bytes needed for holding the captured
VideoFrame. This responsability is shifted into the
VCBPool, so in the inmediate future, GpuMemoryBuffers
can be allocated. To support this, VCBP::ReserveForProducer()
gets the pixel format and dimensions.

VideoCaptureDevice::Client::ReserveOutputBuffer()
goes through the same parameter upgrade.

(In the next CL, a GpuMemoryBufferTracker class
is introduced, allowing VCBP::ReserveForProducer()
will allocate GMBs if so instructed via the pixel
format.)

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

[mcasas, 2015-04-06 21:44:46]

Patchset #1 (id:1) has been deleted

[mcasas, 2015-04-06 21:56:01]

Patchset #1 (id:20001) has been deleted

[mcasas, 2015-04-06 21:58:12]

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

[mcasas, 2015-04-06 21:58:13]

miu@ PTAL.

First CL in the series.

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

Patchset #1 (id:40001) has been deleted

[mcasas, 2015-04-07 19:00:21]

mcasas@chromium.org changed reviewers:
+ magjed@chromium.org, perkj@chromium.org

[mcasas, 2015-04-07 19:00:21]

miu@ PING -- or perkj@/magjed@ PTAL

[mcasas, 2015-04-07 19:00:42]

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

[mcasas, 2015-04-07 19:00:42]

emircan@ PTAL/FYI

[miu, 2015-04-08 01:20:03]

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:162:
VideoFrame::AllocationSize(VideoFrame::I420, format.frame_size);
The call to AllocationSize() should use the format provided, rather than
hard-coded to I420.  For TEXTURE formats, the function will correctly return
zero.

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_buffer_pool.h (right):

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool.h:133: Tracker*
GetBuffer(int buffer_id);
naming: Should be GetTracker() now.  Also, there seems to be a lot of variables
in the .cc file named "buffer" that should be renamed.

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool.h:145: typedef
std::map<int, Tracker*> BufferMap;
Consider the newer, more readable C++11 style:

using BufferMap = std::map<int, Tracker*>;

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_device_client.cc:307: if
(format.pixel_format == media::PIXEL_FORMAT_TEXTURE) {
IMO, you should just delete lines 307-313 and replace with:

DCHECK_GT(format.frame_size.width(), 0);
DCHECK_GT(format.frame_size.height(), 0);

...since VCBufferPool will need to know the frame size in order to reserve a
buffer, regardless of whether it's SharedMerory or a GPUMemoryBuffer.

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_texture_wrapper.cc
(right):

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_texture_wrapper.cc:240: const
media::VideoCaptureFormat format2(gfx::Size(),
Here, an empty size is being passed.  If the texture format is specified, won't
VCBufferPool need to know the size to allocate a GpuMemoryBuffer?

https://codereview.chromium.org/1064703002/diff/60001/media/video/capture/vid...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1064703002/diff/60001/media/video/capture/vid...
media/video/capture/video_capture_device.h:248: const VideoCaptureFormat&
format) = 0;
Seems like this interface change is unnecsessary and complicates the calling
code.  Meaning, most of the callers of this method must now create a
VideoCaptureFormat temporary before calling ReserveOutputBuffer().  Is there a
reason you wanted to change it?

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

Patchset #2 (id:80001) has been deleted

[mcasas, 2015-04-08 22:07:06]

miu@ PTAL

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_buffer_pool.cc (right):

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:162:
VideoFrame::AllocationSize(VideoFrame::I420, format.frame_size);
On 2015/04/08 01:20:00, miu wrote:
> The call to AllocationSize() should use the format provided, rather than
> hard-coded to I420.  For TEXTURE formats, the function will correctly return
> zero.

Done.

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_buffer_pool.h (right):

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool.h:133: Tracker*
GetBuffer(int buffer_id);
On 2015/04/08 01:20:00, miu wrote:
> naming: Should be GetTracker() now.  Also, there seems to be a lot of
variables
> in the .cc file named "buffer" that should be renamed.

Done.

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_buffer_pool.h:145: typedef
std::map<int, Tracker*> BufferMap;
On 2015/04/08 01:20:00, miu wrote:
> Consider the newer, more readable C++11 style:
> 
> using BufferMap = std::map<int, Tracker*>;

Done.

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_device_client.cc (right):

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_device_client.cc:307: if
(format.pixel_format == media::PIXEL_FORMAT_TEXTURE) {
On 2015/04/08 01:20:00, miu wrote:
> IMO, you should just delete lines 307-313 and replace with:
> 
> DCHECK_GT(format.frame_size.width(), 0);
> DCHECK_GT(format.frame_size.height(), 0);
> 
> ...since VCBufferPool will need to know the frame size in order to reserve a
> buffer, regardless of whether it's SharedMerory or a GPUMemoryBuffer.

Done.

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_texture_wrapper.cc
(right):

https://codereview.chromium.org/1064703002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_texture_wrapper.cc:240: const
media::VideoCaptureFormat format2(gfx::Size(),
On 2015/04/08 01:20:00, miu wrote:
> Here, an empty size is being passed.  If the texture format is specified,
won't
> VCBufferPool need to know the size to allocate a GpuMemoryBuffer?

Yes, down the line it will. 

Changed this line to using |frame_format.frame_size|.

Please read the comments about the change of 
ReserveOutputBuffer() signature below.

https://codereview.chromium.org/1064703002/diff/60001/media/video/capture/vid...
File media/video/capture/video_capture_device.h (right):

https://codereview.chromium.org/1064703002/diff/60001/media/video/capture/vid...
media/video/capture/video_capture_device.h:248: const VideoCaptureFormat&
format) = 0;
On 2015/04/08 01:20:03, miu wrote:
> Seems like this interface change is unnecsessary and complicates the calling
> code.  Meaning, most of the callers of this method must now create a
> VideoCaptureFormat temporary before calling ReserveOutputBuffer().  Is there a
> reason you wanted to change it?

No reason to bundle them but I changed
the first param from media::VideoFrame to
media::VideoPixelFormat, which is the enum
used by capture devices.

[miu, 2015-04-08 22:26:14]

lgtm % some tweaks to consider:

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

https://codereview.chromium.org/1064703002/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:17:
VideoFrame::Format VideoPixelFormatToVideoFrameFormat(
I seem to recall seeing this enum-mapping function in other places (maybe in
video_capture_device_client.cc?).  So, consider laying the foundation to prevent
future code duplication by making this an exported static function declared in
media/base/video_capture_types.h:

  MEDIA_EXPORT VideoFrame::Format VideoPixelFormatToVideoFrameFormat(...);

And then put this function in media/base/video_capture_types.cc.

https://codereview.chromium.org/1064703002/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:28: for (const
auto& format_pair : kVideoFrameFormatToVideoPixelFormat) {
Instead of iterating a static array, consider using a switch/case statement so
the compiler will ensure we have all the mappings defined.

https://codereview.chromium.org/1064703002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.h (right):

https://codereview.chromium.org/1064703002/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.h:60: // Reserve
or allocate a buffer to support a frame of |dimensions| of pixel
nit: s/to support a frame/to support a packed frame/

"packed" meaning there is no padding at the end of the rows.

https://codereview.chromium.org/1064703002/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.h:70: // On
occasions, this call will decide to free an old buffer to make room for
nit: English is weird, but this should be "occasion" and not "occasions" (the
way it was).

[mcasas, 2015-04-09 02:03:23]

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

https://codereview.chromium.org/1064703002/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:17:
VideoFrame::Format VideoPixelFormatToVideoFrameFormat(
On 2015/04/08 22:26:14, miu wrote:
> I seem to recall seeing this enum-mapping function in other places (maybe in
> video_capture_device_client.cc?).  So, consider laying the foundation to
prevent
> future code duplication by making this an exported static function declared in
> media/base/video_capture_types.h:
> 
>   MEDIA_EXPORT VideoFrame::Format VideoPixelFormatToVideoFrameFormat(...);
> 
> And then put this function in media/base/video_capture_types.cc.

Perhaps you might be counting transforms
between FourCC or any other OS-enum and 
VideoPixelFormat/VideoFrame::Format, of which
there are plenty...

I'd rather not put it in video_capture_types.h
because that will link it with video_frame.h
and now they are separated entities, as I 
think they should be...

https://codereview.chromium.org/1064703002/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.cc:28: for (const
auto& format_pair : kVideoFrameFormatToVideoPixelFormat) {
On 2015/04/08 22:26:14, miu wrote:
> Instead of iterating a static array, consider using a switch/case statement so
> the compiler will ensure we have all the mappings defined.

Note that not all cases are contemplated here,
only those that we support allocation for, IOW
we don't want to reply NV21 if input format is
such one.

https://codereview.chromium.org/1064703002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/video_capture_buffer_pool.h (right):

https://codereview.chromium.org/1064703002/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.h:60: // Reserve
or allocate a buffer to support a frame of |dimensions| of pixel
On 2015/04/08 22:26:14, miu wrote:
> nit: s/to support a frame/to support a packed frame/
> 
> "packed" meaning there is no padding at the end of the rows.

Done.

https://codereview.chromium.org/1064703002/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_buffer_pool.h:70: // On
occasions, this call will decide to free an old buffer to make room for
On 2015/04/08 22:26:14, miu wrote:
> nit: English is weird, but this should be "occasion" and not "occasions" (the
> way it was).

Argh! Spanish got in the way ("En ocasiones").
Reverted.

[miu, 2015-04-09 02:17:56]

Responses sgtm, and this change still lgtm.

[mcasas, 2015-04-09 02:24:43]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2015-04-09 02:26:25]

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

[commit-bot: I haz the power, 2015-04-09 05:01:11]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2015-04-09 05:01:58]

Patchset 3 (id:??) landed as
https://crrev.com/0549561d5a5331a44a7db5fd4a441a9d6b6c92ad
Cr-Commit-Position: refs/heads/master@{#324380}