Add support for converting I420 software frames into NV12 hardware frames

Add support for converting I420 software frames into NV12 hardware frames

Enhance MaybeCreateHardwareFrame() to be able to create a NV12 hardware
frame backed by a YUV_420_BIPLANAR GpuMemoryBuffer. This format uses 
low power than UYVY_422.

This code path is not enabled yet.

BUG=510260
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[Andre, 2015-08-24 22:50:21]

andresantoso@chromium.org changed reviewers:
+ ccameron@chromium.org, dcastagna@chromium.org

[Andre, 2015-08-24 22:50:22]

PTAL, this is just missing the non-overlay/shader part. I may your to help get
that part working.

[Daniele Castagna, 2015-08-25 15:08:43]

On 2015/08/24 at 22:50:22, andresantoso wrote:
> PTAL, this is just missing the non-overlay/shader part. I may your to help get
that part working.

I'm a little confused about the texture/image/gmb/planes relationships.
If I'm not misunderstanding anything, if we set the output_format_ to
PIXEL_FORMAT_NV12
we end up with:
- 1 GMB with 2 planes (Y and UV)
- 1 image with R_8 internal format
- 1 texture with R_8 internal format

Am I understanding this correctly?

What's the plan for GL? Can we use that IOSurface directly as a single texture
and sample out of it as RGBA or do we have to convert it before sampling from
it?

CC'ing reveman for the GMB part.

https://codereview.chromium.org/1316493004/diff/40001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/1316493004/diff/40001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:427: if (mailbox_holder.mailbox.IsZero())
This is because |textures| == 2 while we created only one texture, right?

We tried to add a VideoFrame::TextureFormat and VideoFrame::NumTextures, but it
was removed.

It's pretty confusing to see int textures = NumPlanes(format), and then this to
correct the number of textures. :/

https://codereview.chromium.org/1316493004/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1316493004/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:95:
{gfx::BufferFormat::YUV_420_BIPLANAR, gfx::BufferUsage::PERSISTENT_MAP},
No scanout?

https://codereview.chromium.org/1316493004/diff/40001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1316493004/diff/40001/media/base/video_frame....
media/base/video_frame.cc:228: format != PIXEL_FORMAT_NV12) {
You need to add the same to the DCHECK in media/blink/skcanvas_video_renderer.cc
NewSkImageFromVideoFrameNative and deal in some way

https://codereview.chromium.org/1316493004/diff/40001/media/renderers/mock_gp...
File media/renderers/mock_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/1316493004/diff/40001/media/renderers/mock_gp...
media/renderers/mock_gpu_video_accelerator_factories.cc:38: return format_;
Is this reached now?
If it's not, I'd leave the NOTREACHED().

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
File media/video/gpu_memory_buffer_video_frame_pool.cc (right):

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:182: return GL_R8_EXT;
Why is the internal format R_8?

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:230: first_row *
source_frame->stride(VideoFrame::kUPlane),
Shouldn't this start at first_row/2?

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:236: dest_uv + first_row *
dest_stride_uv, dest_stride_uv,
Same here.

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:365: scoped_ptr<uint8*[]>
dest_buffers(new uint8*[buffer_planes]);
Can we avoid the allocation with an array of size kMaxPlanes?

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:366: scoped_ptr<int[]>
dest_strides(new int[buffer_planes]);
Same here.

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:392: std::min(rows_per_copy,
rows - row), bytes_per_row,
nit: we could move std::min(rows_per_copy, rows - row) and declare a variable
just before the switch statement.

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:395: ++i;  // Consume an extra
plane.
nit: I'd elaborate on the comment a little to explain that we have two planes in
one GpuMemoryBuffer.

[reveman, 2015-08-25 15:17:03]

reveman@chromium.org changed reviewers:
+ reveman@chromium.org

[reveman, 2015-08-25 15:17:03]

I'd like to see this split into 3 patches.

1. Add YUV_420_BIPLANAR GpuMemoryBuffer format.
2. Add GLImage support for YUV_420_BIPLANAR format.
3. media/ changes that make use of this new format.

[Andre, 2015-08-25 18:19:57]

https://codereview.chromium.org/1316493004/diff/40001/cc/resources/video_reso...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/1316493004/diff/40001/cc/resources/video_reso...
cc/resources/video_resource_updater.cc:427: if (mailbox_holder.mailbox.IsZero())
Yes, let me see if I can find a better way to do this.

https://codereview.chromium.org/1316493004/diff/40001/content/common/gpu/gpu_...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1316493004/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:95:
{gfx::BufferFormat::YUV_420_BIPLANAR, gfx::BufferUsage::PERSISTENT_MAP},
reveman suggested adding that with a separate patch.

https://codereview.chromium.org/1282313002/diff/40001/content/common/gpu/gpu_...

https://codereview.chromium.org/1316493004/diff/40001/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/1316493004/diff/40001/media/base/video_frame....
media/base/video_frame.cc:228: format != PIXEL_FORMAT_NV12) {
I'm not sure how to deal with it there, let me look into that when splitting
this patch.

https://codereview.chromium.org/1316493004/diff/40001/media/renderers/mock_gp...
File media/renderers/mock_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/1316493004/diff/40001/media/renderers/mock_gp...
media/renderers/mock_gpu_video_accelerator_factories.cc:38: return format_;
Yes, this is now called from CopyVideoFrameToGpuMemoryBuffers().

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
File media/video/gpu_memory_buffer_video_frame_pool.cc (right):

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:182: return GL_R8_EXT;
I'm not sure actually, what do you think the format should be?

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:365: scoped_ptr<uint8*[]>
dest_buffers(new uint8*[buffer_planes]);
Done.
I suppose it's ok to use VideoFrame::kMaxPlanes with a DCHECK.

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:366: scoped_ptr<int[]>
dest_strides(new int[buffer_planes]);
On 2015/08/25 15:08:43, Daniele Castagna wrote:
> Same here.

Done.

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:392: std::min(rows_per_copy,
rows - row), bytes_per_row,
On 2015/08/25 15:08:43, Daniele Castagna wrote:
> nit: we could move std::min(rows_per_copy, rows - row) and declare a variable
> just before the switch statement. 

Done.

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:395: ++i;  // Consume an extra
plane.
On 2015/08/25 15:08:42, Daniele Castagna wrote:
> nit: I'd elaborate on the comment a little to explain that we have two planes
in
> one GpuMemoryBuffer.

Done.

[Andre, 2015-08-25 18:20:28]

Sounds good.

[Andre, 2015-08-25 18:20:47]

On 2015/08/25 15:17:03, reveman wrote:
> I'd like to see this split into 3 patches.
> 
> 1. Add YUV_420_BIPLANAR GpuMemoryBuffer format.
> 2. Add GLImage support for YUV_420_BIPLANAR format.
> 3. media/ changes that make use of this new format.

Sounds good, I will make the split.

[Andre, 2015-08-25 21:04:33]

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
File media/video/gpu_memory_buffer_video_frame_pool.cc (right):

https://codereview.chromium.org/1316493004/diff/40001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool.cc:230: first_row *
source_frame->stride(VideoFrame::kUPlane),
You're right, fixed.
There's also still an issue where #copies is twice too big in the case of NV12,
I will need to fix that.

[Andre, 2015-08-27 22:34:23]

Patchset #7 (id:120001) has been deleted