Add YUV_420_BIPLANAR to gfx::BufferFormat.

Add YUV_420_BIPLANAR to gfx::BufferFormat.
Enable it as a supported configuration on Mac.

BUG=510260
TEST=Covered by GpuMemoryBufferImpl::Map and ::PersistentMap.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/f7a3b9416464e0c7384a6387caaeed230bc77a39
Cr-Commit-Position: refs/heads/master@{#343093}

[Andre, 2015-08-10 19:30:25]

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

[Andre, 2015-08-10 19:30:26]

PTAL

[ccameron, 2015-08-10 19:35:08]

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

[ccameron, 2015-08-10 19:35:08]

I think this is the right way to go -- haven't verified all of the details
though.

David, wdyt?

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

https://codereview.chromium.org/1282313002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:125:
AddIntegerValue(properties, kIOSurfacePixelFormat, PixelFormat(format));
I think that, for biplanar, this part needs a dictionary of plane info ...
something like

    NSArray* planes =
        [NSArray arrayWithObjects:
            [NSDictionary dictionaryWithObjectsAndKeys:
                [NSNumber numberWithInt:width],(NSString *)kIOSurfacePlaneWidth,
                [NSNumber numberWithInt:height],(NSString
*)kIOSurfacePlaneHeight,
                [NSNumber numberWithInt:1],(NSString
*)kIOSurfacePlaneBytesPerElement,
                nil],
            [NSDictionary dictionaryWithObjectsAndKeys:
                [NSNumber numberWithInt:width/2],(NSString
*)kIOSurfacePlaneWidth,
                [NSNumber numberWithInt:height/2],(NSString
*)kIOSurfacePlaneHeight,
                [NSNumber numberWithInt:2],(NSString
*)kIOSurfacePlaneBytesPerElement,
                nil],
            nil];
    NSDictionary *options = @{
        (id)kIOSurfaceWidth: @(width),
        (id)kIOSurfaceHeight: @(height),
        (id)kIOSurfacePixelFormat: @(pixelFormat),
        (id)kIOSurfacePlaneInfo: planes,
    };

[Andre, 2015-08-10 20:15:26]

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

https://codereview.chromium.org/1282313002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:125:
AddIntegerValue(properties, kIOSurfacePixelFormat, PixelFormat(format));
Yep, I'm working on that on a follow-up patch, which will also add
YUV_420_BIPLANAR to kSupportedConfigurations.

[reveman, 2015-08-10 20:28:16]

Does GpuMemoryBufferImplTests work properly with this format?

https://codereview.chromium.org/1282313002/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):

https://codereview.chromium.org/1282313002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:154: *size_in_bytes = width;
hm, is this really what we want here? seems like BufferSizeInBytes is not going
to return the correct value when this returns |width| for plane 1 and 2.

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

https://codereview.chromium.org/1282313002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:125:
AddIntegerValue(properties, kIOSurfacePixelFormat, PixelFormat(format));
On 2015/08/10 at 20:15:25, Andre wrote:
> Yep, I'm working on that on a follow-up patch, which will also add
YUV_420_BIPLANAR to kSupportedConfigurations.

It's fine to save that for a follow up patch but I think it's confusing to
implement PixelFormat, etc. until the format is properly supported.

[Andre, 2015-08-10 21:09:48]

https://codereview.chromium.org/1282313002/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):

https://codereview.chromium.org/1282313002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:154: *size_in_bytes = width;
Hmm, I'm not seeing why it's wrong.
Let's say size is 10x10, so U and V are both 5x5.
For the Y plane: rowsize=10, subsampling=1, buffersize=100.
For the interleaved UV plane: rowsize=10, subsampling=2, buffersize=50 (rowsize
* height / subsampling = 10 * 10 / 2).

Am I understanding this correctly?

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

https://codereview.chromium.org/1282313002/diff/20001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:125:
AddIntegerValue(properties, kIOSurfacePixelFormat, PixelFormat(format));
The follow up patch is https://codereview.chromium.org/1282933002/.
I think I will just combine that patch into this one, see patchset 3.

[reveman, 2015-08-10 22:40:55]

https://codereview.chromium.org/1282313002/diff/20001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):

https://codereview.chromium.org/1282313002/diff/20001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:154: *size_in_bytes = width;
On 2015/08/10 at 21:09:48, Andre wrote:
> Hmm, I'm not seeing why it's wrong.
> Let's say size is 10x10, so U and V are both 5x5.
> For the Y plane: rowsize=10, subsampling=1, buffersize=100.
> For the interleaved UV plane: rowsize=10, subsampling=2, buffersize=50
(rowsize * height / subsampling = 10 * 10 / 2).
> 
> Am I understanding this correctly?

Yes, that looks correct. Sorry my bad. Still thinking in terms of 3 planes for
some reason..

https://codereview.chromium.org/1282313002/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc (right):

https://codereview.chromium.org/1282313002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:65: const size_t
planes = IOSurfaceGetPlaneCount(io_surface_);
nit: no need for 'const' and maybe plane_count or num_planes instead of 'planes'

https://codereview.chromium.org/1282313002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:66: if (planes ==
0) {
Does IOSurfaceGetPlaneCount actually return this? Please DCHECK_GT and remove
the if clause if possible.

https://codereview.chromium.org/1282313002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:81: void
GpuMemoryBufferImplIOSurface::GetStride(int* strides) const {
same comments here as above

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

https://codereview.chromium.org/1282313002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:83:
{gfx::BufferFormat::YUV_420_BIPLANAR, gfx::BufferUsage::SCANOUT},
SCANOUT is supported by some other formats too, correct? How about we introduce
all these SCANOUT formats in a separate patch?

https://codereview.chromium.org/1282313002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:151:
CFDictionaryAddValue(properties, kIOSurfacePlaneInfo, planes);
Could we refactor this code to set kIOSurfacePlaneInfo for all formats instead
of having to special case this format? Basically generalize this code so it
works with N planes.

https://codereview.chromium.org/1282313002/diff/40001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1282313002/diff/40001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:38: case BufferFormat::YUV_420_BIPLANAR:
GLImage support for this format doesn't exist yet. I think this function should
return false for now.

https://codereview.chromium.org/1282313002/diff/40001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1282313002/diff/40001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:66: case BufferFormat::YUV_420_BIPLANAR:
hm, I don't think we should return true here for this format or YUV_420 for that
matter. This is used to determine when CompressedTexImage2D should be used so I
think it would be more correct to return false for these formats.

[Andre, 2015-08-10 23:30:25]

Patchset #4 (id:60001) has been deleted

[Andre, 2015-08-11 00:00:36]

Patchset #4 (id:80001) has been deleted

[Andre, 2015-08-11 00:12:31]

Patchset #4 (id:100001) has been deleted

[Andre, 2015-08-11 00:50:54]

Patchset #4 (id:120001) has been deleted

[Andre, 2015-08-11 01:41:10]

Patchset #5 (id:160001) has been deleted

[Andre, 2015-08-11 01:42:45]

Patchset #4 (id:140001) has been deleted

[Andre, 2015-08-11 01:42:50]

Patchset #4 (id:140001) has been deleted

[Andre, 2015-08-11 03:50:46]

PTAL

https://codereview.chromium.org/1282313002/diff/40001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc (right):

https://codereview.chromium.org/1282313002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:65: const size_t
planes = IOSurfaceGetPlaneCount(io_surface_);
On 2015/08/10 22:40:54, reveman (OOO Aug 10 - Aug 11) wrote:
> nit: no need for 'const' and maybe plane_count or num_planes instead of
'planes'

Done.

https://codereview.chromium.org/1282313002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:66: if (planes ==
0) {
Yes, IOSurface actually returns 0 in the non-planar case.
But I can replace it with a call to NumberOfPlanesForGpuMemoryBufferFormat() and
remove the if clause.

https://codereview.chromium.org/1282313002/diff/40001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_io_surface.cc:81: void
GpuMemoryBufferImplIOSurface::GetStride(int* strides) const {
On 2015/08/10 22:40:55, reveman (OOO Aug 10 - Aug 11) wrote:
> same comments here as above

Done.

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

https://codereview.chromium.org/1282313002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:83:
{gfx::BufferFormat::YUV_420_BIPLANAR, gfx::BufferUsage::SCANOUT},
Done, removed the SCANOUT usage from this patch.
I don't know the answer to that question, ccameron would.

https://codereview.chromium.org/1282313002/diff/40001/content/common/gpu/gpu_...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:151:
CFDictionaryAddValue(properties, kIOSurfacePlaneInfo, planes);
On 2015/08/10 22:40:55, reveman (OOO Aug 10 - Aug 11) wrote:
> Could we refactor this code to set kIOSurfacePlaneInfo for all formats instead
> of having to special case this format? Basically generalize this code so it
> works with N planes.

Done.

https://codereview.chromium.org/1282313002/diff/40001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1282313002/diff/40001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.mm:38: case BufferFormat::YUV_420_BIPLANAR:
On 2015/08/10 22:40:55, reveman (OOO Aug 10 - Aug 11) wrote:
> GLImage support for this format doesn't exist yet. I think this function
should
> return false for now.

Done.

https://codereview.chromium.org/1282313002/diff/40001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1282313002/diff/40001/ui/gl/gl_image_memory.c...
ui/gl/gl_image_memory.cc:66: case BufferFormat::YUV_420_BIPLANAR:
On 2015/08/10 22:40:55, reveman (OOO Aug 10 - Aug 11) wrote:
> hm, I don't think we should return true here for this format or YUV_420 for
that
> matter. This is used to determine when CompressedTexImage2D should be used so
I
> think it would be more correct to return false for these formats.

Done.

[reveman, 2015-08-11 08:20:08]

https://codereview.chromium.org/1282313002/diff/180001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1282313002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:132: if (num_planes
== 1) {
This is unfortunate. What happens if we remove this if statement? Single plane
formats don't work unless kIOSurfaceBytesPerElement is set?

In that case, I'd prefer if we handled this explicitly using a large "switch
(format)" statement to make it very clear what formats need the plane info and
what formats don't.

https://codereview.chromium.org/1282313002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:154: row_size /
plane_size.width());
Instead of using RowSizeInBytes here, can we replace the BytesPerPixel function
with a BytesPerElement function that takes the plane index as a parameter?

https://codereview.chromium.org/1282313002/diff/180001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.cc (right):

https://codereview.chromium.org/1282313002/diff/180001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.cc:45: }
bad rebase?

https://codereview.chromium.org/1282313002/diff/180001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1282313002/diff/180001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:59: case BufferFormat::YUV_420_BIPLANAR:
nit: move below ::YUV_420 as this it not yet valid

https://codereview.chromium.org/1282313002/diff/180001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:83: case BufferFormat::YUV_420_BIPLANAR:
nit: move below ::YUV_420 as this it not yet valid

https://codereview.chromium.org/1282313002/diff/180001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:107: case BufferFormat::YUV_420_BIPLANAR:
nit: move below ::YUV_420 as this it not yet valid

https://codereview.chromium.org/1282313002/diff/180001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1282313002/diff/180001/ui/gl/gl_image_memory....
ui/gl/gl_image_memory.cc:72: case BufferFormat::YUV_420_BIPLANAR:
Can you move BufferFormat::YUV_420 here too while touching this code?

[Andre, 2015-08-11 18:15:44]

PTAL

https://codereview.chromium.org/1282313002/diff/180001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1282313002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:132: if (num_planes
== 1) {
Done.
I removed the if statement, and it seems to work just fine.

https://codereview.chromium.org/1282313002/diff/180001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:154: row_size /
plane_size.width());
Great idea, done.

https://codereview.chromium.org/1282313002/diff/180001/ui/gfx/gpu_memory_buff...
File ui/gfx/gpu_memory_buffer.cc (right):

https://codereview.chromium.org/1282313002/diff/180001/ui/gfx/gpu_memory_buff...
ui/gfx/gpu_memory_buffer.cc:45: }
Oops, fixed.

https://codereview.chromium.org/1282313002/diff/180001/ui/gl/gl_image_io_surf...
File ui/gl/gl_image_io_surface.mm (right):

https://codereview.chromium.org/1282313002/diff/180001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:59: case BufferFormat::YUV_420_BIPLANAR:
On 2015/08/11 08:20:08, reveman (OOO Aug 10 - Aug 11) wrote:
> nit: move below ::YUV_420 as this it not yet valid

Done.

https://codereview.chromium.org/1282313002/diff/180001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:83: case BufferFormat::YUV_420_BIPLANAR:
On 2015/08/11 08:20:08, reveman (OOO Aug 10 - Aug 11) wrote:
> nit: move below ::YUV_420 as this it not yet valid

Done.

https://codereview.chromium.org/1282313002/diff/180001/ui/gl/gl_image_io_surf...
ui/gl/gl_image_io_surface.mm:107: case BufferFormat::YUV_420_BIPLANAR:
On 2015/08/11 08:20:08, reveman (OOO Aug 10 - Aug 11) wrote:
> nit: move below ::YUV_420 as this it not yet valid

Done.

https://codereview.chromium.org/1282313002/diff/180001/ui/gl/gl_image_memory.cc
File ui/gl/gl_image_memory.cc (right):

https://codereview.chromium.org/1282313002/diff/180001/ui/gl/gl_image_memory....
ui/gl/gl_image_memory.cc:72: case BufferFormat::YUV_420_BIPLANAR:
On 2015/08/11 08:20:08, reveman (OOO Aug 10 - Aug 11) wrote:
> Can you move BufferFormat::YUV_420 here too while touching this code?

Done.

[reveman, 2015-08-11 19:10:10]

lgtm with nits

https://codereview.chromium.org/1282313002/diff/200001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1282313002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:28: int32
BytesPerElement(gfx::BufferFormat format, size_t plane) {
nit: s/size_t/int/

https://codereview.chromium.org/1282313002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:31: return 1;
nit: DCHECK_EQ(plane, 0)

https://codereview.chromium.org/1282313002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:33: return 4;
nit: DCHECK_EQ(plane, 0)

https://codereview.chromium.org/1282313002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:36: return plane == 0
? 1 : 2;
nit: for consistency with ::SubsamplingFactor:

static size_t bytes_per_element[] = {1, 2};
DCHECK_LT(static_cast<size_t>(plane), arraysize(bytes_per_element));
return bytes_per_element[plane];

https://codereview.chromium.org/1282313002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:125:
DCHECK_GT(num_planes, 0u);
nit: not sure this DCHECK is necessary. NumberOfPlanesFor... has a NOTREACHED()
for this already

[Andre, 2015-08-11 19:46:21]

andresantoso@chromium.org changed reviewers:
+ avi@chromium.org, jbauman@chromium.org, msw@chromium.org

[Andre, 2015-08-11 19:46:22]

+msw for components/view_manager.
+jbauman for gpu/command_buffer.
+avi for ui and content.

https://codereview.chromium.org/1282313002/diff/200001/content/common/gpu/gpu...
File content/common/gpu/gpu_memory_buffer_factory_io_surface.cc (right):

https://codereview.chromium.org/1282313002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:28: int32
BytesPerElement(gfx::BufferFormat format, size_t plane) {
On 2015/08/11 19:10:10, reveman (OOO Aug 10 - Aug 11) wrote:
> nit: s/size_t/int/

Done.

https://codereview.chromium.org/1282313002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:31: return 1;
On 2015/08/11 19:10:10, reveman (OOO Aug 10 - Aug 11) wrote:
> nit: DCHECK_EQ(plane, 0)

Done.

https://codereview.chromium.org/1282313002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:33: return 4;
On 2015/08/11 19:10:10, reveman (OOO Aug 10 - Aug 11) wrote:
> nit: DCHECK_EQ(plane, 0)

Done.

https://codereview.chromium.org/1282313002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:36: return plane == 0
? 1 : 2;
On 2015/08/11 19:10:10, reveman (OOO Aug 10 - Aug 11) wrote:
> nit: for consistency with ::SubsamplingFactor:
> 
> static size_t bytes_per_element[] = {1, 2};
> DCHECK_LT(static_cast<size_t>(plane), arraysize(bytes_per_element));
> return bytes_per_element[plane];

Done.

https://codereview.chromium.org/1282313002/diff/200001/content/common/gpu/gpu...
content/common/gpu/gpu_memory_buffer_factory_io_surface.cc:125:
DCHECK_GT(num_planes, 0u);
Removed.

[msw, 2015-08-11 19:52:02]

components/view_manager lgtm

[jbauman, 2015-08-12 00:05:05]

gpu/command_buffer lgtm

[Andre, 2015-08-12 16:58:58]

Rebase

[Andre, 2015-08-12 17:09:37]

Avi PTAL for ui and content.

[Avi (use Gerrit), 2015-08-12 19:58:04]

lgtm

stampity stamp

[Andre, 2015-08-12 20:00:25]

The CQ bit was checked by andresantoso@chromium.org

[Andre, 2015-08-12 20:00:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, msw@chromium.org, jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/1282313002/#ps230001
(title: "Rebase")

[commit-bot: I haz the power, 2015-08-12 20:01:12]

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

[commit-bot: I haz the power, 2015-08-12 21:08:28]

Committed patchset #7 (id:230001)

[commit-bot: I haz the power, 2015-08-12 21:09:43]

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