Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>;
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/29292a32f6f429a790e8663ce09bf67d165f49e9
Committed: https://crrev.com/47661a7fc2314e3fb9a1478b3e8611208e216f63
Cr-Original-Commit-Position: refs/heads/master@{#402656}
Cr-Commit-Position: refs/heads/master@{#403418}

[vinceh, 2016-06-06 07:28:17]

vince.h@nvidia.com changed reviewers:
+ marcheu@google.com

[marcheu1, 2016-06-07 01:25:36]

Description was changed from

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
==========

to

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
==========

[marcheu1, 2016-06-07 01:25:36]

marcheu@google.com changed reviewers:
+ alexst@chromium.org, spang@chromium.org

[marcheu1, 2016-06-07 01:26:24]

marcheu@google.com changed reviewers:
+ piman@chromium.org, reveman@chromium.org

[marcheu1, 2016-06-07 01:26:44]

added a few OWNERS for all these, will also take a look

[marcheu1, 2016-06-07 01:29:07]

https://codereview.chromium.org/2039813002/diff/1/ui/ozone/platform/drm/gpu/g...
File ui/ozone/platform/drm/gpu/gbm_buffer.h (right):

https://codereview.chromium.org/2039813002/diff/1/ui/ozone/platform/drm/gpu/g...
ui/ozone/platform/drm/gpu/gbm_buffer.h:53: const std::vector<uint64_t>&
modifiers);
Is there a way we can have multiple modifiers? I didn't think so.

[reveman, 2016-06-07 01:33:10]

ui/gl/gl_image_* lgtm with nits

https://codereview.chromium.org/2039813002/diff/1/ui/gl/gl_image_ozone_native...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2039813002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:154: 
nit: avoid adding this blankline?

https://codereview.chromium.org/2039813002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:186: 
nit: avoid adding this blankline?

[piman, 2016-06-07 01:37:46]

https://codereview.chromium.org/2039813002/diff/1/ui/gl/gl_image_ozone_native...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2039813002/diff/1/ui/gl/gl_image_ozone_native...
ui/gl/gl_image_ozone_native_pixmap.cc:142: attrs.push_back((uint32_t)((modifier
>> 32) & 0xffffffff));
nit: use static_cast<uint32_t> instead of c-style cast.

[vinceh, 2016-06-07 01:56:31]

https://codereview.chromium.org/2039813002/diff/1/ui/ozone/platform/drm/gpu/g...
File ui/ozone/platform/drm/gpu/gbm_buffer.h (right):

https://codereview.chromium.org/2039813002/diff/1/ui/ozone/platform/drm/gpu/g...
ui/ozone/platform/drm/gpu/gbm_buffer.h:53: const std::vector<uint64_t>&
modifiers);
On 2016/06/07 01:29:07, marcheu1 wrote:
> Is there a way we can have multiple modifiers? I didn't think so.

Since the patch "Add support for multiple FDs to NativePixmap/GbmBuffer" we have
multiple strides, I think that also applies to modifiers?

[spang, 2016-06-07 15:24:36]

https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_buffer.h (right):

https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.h:61: std::vector<uint64_t> modifiers_;
Can you do something like

struct GbmBufferPlane {
  base::ScopedFD fd;
  int stride;
  uint64_t modifier;
};

std::vector<GbmBufferPlane> planes_;

It's non-obvious that these 3 vectors are all just indexed on plane.

[vinceh, 2016-06-08 08:41:12]

On 2016/06/07 15:24:36, spang wrote:
>
https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
> File ui/ozone/platform/drm/gpu/gbm_buffer.h (right):
> 
>
https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
> ui/ozone/platform/drm/gpu/gbm_buffer.h:61: std::vector<uint64_t> modifiers_;
> Can you do something like
> 
> struct GbmBufferPlane {
>   base::ScopedFD fd;
>   int stride;
>   uint64_t modifier;
> };
> 
> std::vector<GbmBufferPlane> planes_;
> 
> It's non-obvious that these 3 vectors are all just indexed on plane.

That way I might have to initialize the vector GbmBufferPlane in constructor
explicitly like below. Is that what you want?

for (int i = 0; i < fds.size(); i++) {
  GbmBufferPlane plane;
  plane.fd = std::move(fds[i]);
  plane.stride = strides[i];
  plane.modifier = modifiers[i];
  planes_.push_back(plane);
}

[vinceh, 2016-06-13 03:23:40]

On 2016/06/08 08:41:12, vince.h wrote:
> On 2016/06/07 15:24:36, spang wrote:
> >
>
https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
> > File ui/ozone/platform/drm/gpu/gbm_buffer.h (right):
> > 
> >
>
https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
> > ui/ozone/platform/drm/gpu/gbm_buffer.h:61: std::vector<uint64_t> modifiers_;
> > Can you do something like
> > 
> > struct GbmBufferPlane {
> >   base::ScopedFD fd;
> >   int stride;
> >   uint64_t modifier;
> > };
> > 
> > std::vector<GbmBufferPlane> planes_;
> > 
> > It's non-obvious that these 3 vectors are all just indexed on plane.
> 
> That way I might have to initialize the vector GbmBufferPlane in constructor
> explicitly like below. Is that what you want?
> 
> for (int i = 0; i < fds.size(); i++) {
>   GbmBufferPlane plane;
>   plane.fd = std::move(fds[i]);
>   plane.stride = strides[i];
>   plane.modifier = modifiers[i];
>   planes_.push_back(plane);
> }

Actually dcastagna@chromium.org added one more field "offset" recently. I would
like to know his thought about #11, but I couldn't add him as reviewer. Anyone
can help? Thanks!

[reveman, 2016-06-13 03:30:01]

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

[reveman, 2016-06-13 03:30:02]

+dcastagna

[Daniele Castagna, 2016-06-13 05:14:02]

On 2016/06/13 at 03:23:40, vince.h wrote:
> On 2016/06/08 08:41:12, vince.h wrote:
> > On 2016/06/07 15:24:36, spang wrote:
> > >
> >
https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
> > > File ui/ozone/platform/drm/gpu/gbm_buffer.h (right):
> > > 
> > >
> >
https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
> > > ui/ozone/platform/drm/gpu/gbm_buffer.h:61: std::vector<uint64_t>
modifiers_;
> > > Can you do something like
> > > 
> > > struct GbmBufferPlane {
> > >   base::ScopedFD fd;
> > >   int stride;
> > >   uint64_t modifier;
> > > };
> > > 
> > > std::vector<GbmBufferPlane> planes_;
> > > 
> > > It's non-obvious that these 3 vectors are all just indexed on plane.
> > 
> > That way I might have to initialize the vector GbmBufferPlane in constructor
> > explicitly like below. Is that what you want?
> > 
> > for (int i = 0; i < fds.size(); i++) {
> >   GbmBufferPlane plane;
> >   plane.fd = std::move(fds[i]);
> >   plane.stride = strides[i];
> >   plane.modifier = modifiers[i];
> >   planes_.push_back(plane);
> > }
> 
> Actually dcastagna@chromium.org added one more field "offset" recently. I
would like to know his thought about #11, but I couldn't add him as reviewer.
Anyone can help? Thanks!

As far as I know the number of file descriptors doesn't need to match the number
of planes. The code you suggested would break YVU_420, that might have only one
fd for three planes.
We tried to dup the fds to have one fd per plane but that didn't work for
specific implementations of mesa.

Having a struct for offset, stride and modifier seems good to me.

Btw, in this patch I don't see where you actually pass any modifier. How did you
test this?
I'm guessing you also want to add modifier to NativePixmapHandle if we care
about passing those between processes.

[vinceh, 2016-06-13 05:45:09]

On 2016/06/13 05:14:02, Daniele Castagna wrote:
> On 2016/06/13 at 03:23:40, vince.h wrote:
> > On 2016/06/08 08:41:12, vince.h wrote:
> > > On 2016/06/07 15:24:36, spang wrote:
> > > >
> > >
>
https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
> > > > File ui/ozone/platform/drm/gpu/gbm_buffer.h (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
> > > > ui/ozone/platform/drm/gpu/gbm_buffer.h:61: std::vector<uint64_t>
> modifiers_;
> > > > Can you do something like
> > > > 
> > > > struct GbmBufferPlane {
> > > >   base::ScopedFD fd;
> > > >   int stride;
> > > >   uint64_t modifier;
> > > > };
> > > > 
> > > > std::vector<GbmBufferPlane> planes_;
> > > > 
> > > > It's non-obvious that these 3 vectors are all just indexed on plane.
> > > 
> > > That way I might have to initialize the vector GbmBufferPlane in
constructor
> > > explicitly like below. Is that what you want?
> > > 
> > > for (int i = 0; i < fds.size(); i++) {
> > >   GbmBufferPlane plane;
> > >   plane.fd = std::move(fds[i]);
> > >   plane.stride = strides[i];
> > >   plane.modifier = modifiers[i];
> > >   planes_.push_back(plane);
> > > }
> > 
> > Actually mailto:dcastagna@chromium.org added one more field "offset"
recently. I
> would like to know his thought about #11, but I couldn't add him as reviewer.
> Anyone can help? Thanks!
> 
> As far as I know the number of file descriptors doesn't need to match the
number
> of planes. The code you suggested would break YVU_420, that might have only
one
> fd for three planes.
> We tried to dup the fds to have one fd per plane but that didn't work for
> specific implementations of mesa.
> 
> Having a struct for offset, stride and modifier seems good to me.
> 
> Btw, in this patch I don't see where you actually pass any modifier. How did
you
> test this?
> I'm guessing you also want to add modifier to NativePixmapHandle if we care
> about passing those between processes.

Thanks for your suggestion. The modifier is passed from minigbm
https://chromium-review.googlesource.com/#/c/346366/ for Tegra platform.

Yes, I did consider adding modifier to NativePixmapHandle, although without it I
haven't found any problem during my testing. I will add it.

[reveman, 2016-06-13 16:43:42]

On 2016/06/13 at 05:45:09, vince.h wrote:
> On 2016/06/13 05:14:02, Daniele Castagna wrote:
> > On 2016/06/13 at 03:23:40, vince.h wrote:
> > > On 2016/06/08 08:41:12, vince.h wrote:
> > > > On 2016/06/07 15:24:36, spang wrote:
> > > > >
> > > >
> >
https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
> > > > > File ui/ozone/platform/drm/gpu/gbm_buffer.h (right):
> > > > > 
> > > > >
> > > >
> >
https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
> > > > > ui/ozone/platform/drm/gpu/gbm_buffer.h:61: std::vector<uint64_t>
> > modifiers_;
> > > > > Can you do something like
> > > > > 
> > > > > struct GbmBufferPlane {
> > > > >   base::ScopedFD fd;
> > > > >   int stride;
> > > > >   uint64_t modifier;
> > > > > };
> > > > > 
> > > > > std::vector<GbmBufferPlane> planes_;
> > > > > 
> > > > > It's non-obvious that these 3 vectors are all just indexed on plane.
> > > > 
> > > > That way I might have to initialize the vector GbmBufferPlane in
constructor
> > > > explicitly like below. Is that what you want?
> > > > 
> > > > for (int i = 0; i < fds.size(); i++) {
> > > >   GbmBufferPlane plane;
> > > >   plane.fd = std::move(fds[i]);
> > > >   plane.stride = strides[i];
> > > >   plane.modifier = modifiers[i];
> > > >   planes_.push_back(plane);
> > > > }
> > > 
> > > Actually mailto:dcastagna@chromium.org added one more field "offset"
recently. I
> > would like to know his thought about #11, but I couldn't add him as
reviewer.
> > Anyone can help? Thanks!
> > 
> > As far as I know the number of file descriptors doesn't need to match the
number
> > of planes. The code you suggested would break YVU_420, that might have only
one
> > fd for three planes.
> > We tried to dup the fds to have one fd per plane but that didn't work for
> > specific implementations of mesa.
> > 
> > Having a struct for offset, stride and modifier seems good to me.
> > 
> > Btw, in this patch I don't see where you actually pass any modifier. How did
you
> > test this?
> > I'm guessing you also want to add modifier to NativePixmapHandle if we care
> > about passing those between processes.
> 
> Thanks for your suggestion. The modifier is passed from minigbm
https://chromium-review.googlesource.com/#/c/346366/ for Tegra platform.
> 
> Yes, I did consider adding modifier to NativePixmapHandle, although without it
I haven't found any problem during my testing. I will add it.

Please add this as otherwise you'll lose this information when we fix
crbug.com/597932.

[vinceh, 2016-06-15 03:30:56]

On 2016/06/13 16:43:42, reveman wrote:
> On 2016/06/13 at 05:45:09, vince.h wrote:
> > On 2016/06/13 05:14:02, Daniele Castagna wrote:
> > > On 2016/06/13 at 03:23:40, vince.h wrote:
> > > > On 2016/06/08 08:41:12, vince.h wrote:
> > > > > On 2016/06/07 15:24:36, spang wrote:
> > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
> > > > > > File ui/ozone/platform/drm/gpu/gbm_buffer.h (right):
> > > > > > 
> > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/2039813002/diff/20001/ui/ozone/platform/drm/g...
> > > > > > ui/ozone/platform/drm/gpu/gbm_buffer.h:61: std::vector<uint64_t>
> > > modifiers_;
> > > > > > Can you do something like
> > > > > > 
> > > > > > struct GbmBufferPlane {
> > > > > >   base::ScopedFD fd;
> > > > > >   int stride;
> > > > > >   uint64_t modifier;
> > > > > > };
> > > > > > 
> > > > > > std::vector<GbmBufferPlane> planes_;
> > > > > > 
> > > > > > It's non-obvious that these 3 vectors are all just indexed on plane.
> > > > > 
> > > > > That way I might have to initialize the vector GbmBufferPlane in
> constructor
> > > > > explicitly like below. Is that what you want?
> > > > > 
> > > > > for (int i = 0; i < fds.size(); i++) {
> > > > >   GbmBufferPlane plane;
> > > > >   plane.fd = std::move(fds[i]);
> > > > >   plane.stride = strides[i];
> > > > >   plane.modifier = modifiers[i];
> > > > >   planes_.push_back(plane);
> > > > > }
> > > > 
> > > > Actually mailto:dcastagna@chromium.org added one more field "offset"
> recently. I
> > > would like to know his thought about #11, but I couldn't add him as
> reviewer.
> > > Anyone can help? Thanks!
> > > 
> > > As far as I know the number of file descriptors doesn't need to match the
> number
> > > of planes. The code you suggested would break YVU_420, that might have
only
> one
> > > fd for three planes.
> > > We tried to dup the fds to have one fd per plane but that didn't work for
> > > specific implementations of mesa.
> > > 
> > > Having a struct for offset, stride and modifier seems good to me.
> > > 
> > > Btw, in this patch I don't see where you actually pass any modifier. How
did
> you
> > > test this?
> > > I'm guessing you also want to add modifier to NativePixmapHandle if we
care
> > > about passing those between processes.
> > 
> > Thanks for your suggestion. The modifier is passed from minigbm
> https://chromium-review.googlesource.com/#/c/346366/ for Tegra platform.
> > 
> > Yes, I did consider adding modifier to NativePixmapHandle, although without
it
> I haven't found any problem during my testing. I will add it.
> 
> Please add this as otherwise you'll lose this information when we fix
> crbug.com/597932.

I've updated the v3. Could you guys review again? :) Thanks!

[Daniele Castagna, 2016-06-15 03:50:20]

Thank you!

If you don't mind updating also
ui/gl/gl_image_ozone_native_pixmap_drm_unittest.cc (it gets built only on intel
cros devices). If it's not easy to test that I can take care of it.

https://codereview.chromium.org/2039813002/diff/40001/components/exo/display.cc
File components/exo/display.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/components/exo/display....
components/exo/display.cc:66: const std::vector<gfx::GbmBufferPlane>&& planes,
No need for an rvalue reference here, just a normal const ref is ok. It's fine
to copy the planes.

https://codereview.chromium.org/2039813002/diff/40001/components/exo/wayland/...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/components/exo/wayland/...
components/exo/wayland/server.cc:531: std::move(planes), std::move(fds));
Again, no need to move.

https://codereview.chromium.org/2039813002/diff/40001/components/exo/wayland/...
components/exo/wayland/server.cc:684: std::move(planes), std::move(fds));
Ditto.

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/ipc/gfx_param_tr...
File ui/gfx/ipc/gfx_param_traits_macros.h (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/ipc/gfx_param_tr...
ui/gfx/ipc/gfx_param_traits_macros.h:62: IPC_STRUCT_TRAITS_MEMBER(planes)
This change will require a security review, you can add tsepez@ that reviewed
stride and offset already.

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
File ui/gfx/native_pixmap_handle_ozone.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
ui/gfx/native_pixmap_handle_ozone.cc:9: GbmBufferPlane::GbmBufferPlane() {}
Is it ok not to initialize anything here?

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
File ui/gfx/native_pixmap_handle_ozone.h (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
ui/gfx/native_pixmap_handle_ozone.h:16: struct GFX_EXPORT GbmBufferPlane {
Comment before struct definition.

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
ui/gfx/native_pixmap_handle_ozone.h:26: uint64_t modifier;
nit: I think the modifier deserves a comment as well.

https://codereview.chromium.org/2039813002/diff/40001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:154:
attrs.push_back(static_cast<uint32_t>((modifier >> 32) & 0xffffffff));
Is the & 0xffffffff really needed after shifting? I'm not sure if even the cast
is needed.

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_thread.h (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.h:65: std::vector<gfx::GbmBufferPlane>&&
planes,
just const ref is fine.

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:63: int GbmBuffer::GetFd(size_t plane)
const {
I'd say |plane| here as parameter is confusing. Should we change this to index?

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:130:
planes.emplace_back(plane);
emplace_back here has the same effect of push_back, right?

[Daniele Castagna, 2016-06-15 03:50:20]

Thank you!

If you don't mind updating also
ui/gl/gl_image_ozone_native_pixmap_drm_unittest.cc (it gets built only on intel
cros devices). If it's not easy to test that I can take care of it.

https://codereview.chromium.org/2039813002/diff/40001/components/exo/display.cc
File components/exo/display.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/components/exo/display....
components/exo/display.cc:66: const std::vector<gfx::GbmBufferPlane>&& planes,
No need for an rvalue reference here, just a normal const ref is ok. It's fine
to copy the planes.

https://codereview.chromium.org/2039813002/diff/40001/components/exo/wayland/...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/components/exo/wayland/...
components/exo/wayland/server.cc:531: std::move(planes), std::move(fds));
Again, no need to move.

https://codereview.chromium.org/2039813002/diff/40001/components/exo/wayland/...
components/exo/wayland/server.cc:684: std::move(planes), std::move(fds));
Ditto.

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/ipc/gfx_param_tr...
File ui/gfx/ipc/gfx_param_traits_macros.h (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/ipc/gfx_param_tr...
ui/gfx/ipc/gfx_param_traits_macros.h:62: IPC_STRUCT_TRAITS_MEMBER(planes)
This change will require a security review, you can add tsepez@ that reviewed
stride and offset already.

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
File ui/gfx/native_pixmap_handle_ozone.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
ui/gfx/native_pixmap_handle_ozone.cc:9: GbmBufferPlane::GbmBufferPlane() {}
Is it ok not to initialize anything here?

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
File ui/gfx/native_pixmap_handle_ozone.h (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
ui/gfx/native_pixmap_handle_ozone.h:16: struct GFX_EXPORT GbmBufferPlane {
Comment before struct definition.

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
ui/gfx/native_pixmap_handle_ozone.h:26: uint64_t modifier;
nit: I think the modifier deserves a comment as well.

https://codereview.chromium.org/2039813002/diff/40001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:154:
attrs.push_back(static_cast<uint32_t>((modifier >> 32) & 0xffffffff));
Is the & 0xffffffff really needed after shifting? I'm not sure if even the cast
is needed.

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_thread.h (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.h:65: std::vector<gfx::GbmBufferPlane>&&
planes,
just const ref is fine.

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:63: int GbmBuffer::GetFd(size_t plane)
const {
I'd say |plane| here as parameter is confusing. Should we change this to index?

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:130:
planes.emplace_back(plane);
emplace_back here has the same effect of push_back, right?

[vinceh, 2016-06-15 04:14:25]

vince.h@nvidia.com changed reviewers:
+ tsepez@chromium.org

[vinceh, 2016-06-15 04:14:26]

Add tsepez for change
https://codereview.chromium.org/2039813002/patch/40001/50009 although the system
showed he is OOO to 6/20

[vinceh, 2016-06-15 04:35:18]

Somehow I missed ui/gl/gl_image_ozone_native_pixmap_drm_unittest.cc for the
strides_and_offsets change. Will update it. :)

https://codereview.chromium.org/2039813002/diff/40001/components/exo/display.cc
File components/exo/display.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/components/exo/display....
components/exo/display.cc:66: const std::vector<gfx::GbmBufferPlane>&& planes,
On 2016/06/15 03:50:19, Daniele Castagna wrote:
> No need for an rvalue reference here, just a normal const ref is ok. It's fine
> to copy the planes.

Will fix. Thanks.

https://codereview.chromium.org/2039813002/diff/40001/components/exo/wayland/...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/components/exo/wayland/...
components/exo/wayland/server.cc:531: std::move(planes), std::move(fds));
On 2016/06/15 03:50:19, Daniele Castagna wrote:
> Again, no need to move.

Will fix.

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/ipc/gfx_param_tr...
File ui/gfx/ipc/gfx_param_traits_macros.h (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/ipc/gfx_param_tr...
ui/gfx/ipc/gfx_param_traits_macros.h:62: IPC_STRUCT_TRAITS_MEMBER(planes)
On 2016/06/15 03:50:20, Daniele Castagna wrote:
> This change will require a security review, you can add tsepez@ that reviewed
> stride and offset already.

Added Tom. But he is OOO, do we have other reviewers for this?

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
File ui/gfx/native_pixmap_handle_ozone.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
ui/gfx/native_pixmap_handle_ozone.cc:9: GbmBufferPlane::GbmBufferPlane() {}
On 2016/06/15 03:50:20, Daniele Castagna wrote:
> Is it ok not to initialize anything here?

I will initialize all the members as zero.

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
File ui/gfx/native_pixmap_handle_ozone.h (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
ui/gfx/native_pixmap_handle_ozone.h:16: struct GFX_EXPORT GbmBufferPlane {
On 2016/06/15 03:50:20, Daniele Castagna wrote:
> Comment before struct definition.

Will fix.

https://codereview.chromium.org/2039813002/diff/40001/ui/gfx/native_pixmap_ha...
ui/gfx/native_pixmap_handle_ozone.h:26: uint64_t modifier;
On 2016/06/15 03:50:20, Daniele Castagna wrote:
> nit: I think the modifier deserves a comment as well.

Yes, will add.

https://codereview.chromium.org/2039813002/diff/40001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:154:
attrs.push_back(static_cast<uint32_t>((modifier >> 32) & 0xffffffff));
On 2016/06/15 03:50:20, Daniele Castagna wrote:
> Is the & 0xffffffff really needed after shifting? I'm not sure if even the
cast
> is needed.

Will remove & 0xffffffff and keep the cast.

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_thread.h (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_thread.h:65: std::vector<gfx::GbmBufferPlane>&&
planes,
On 2016/06/15 03:50:20, Daniele Castagna wrote:
> just const ref is fine.

OK. Will fix.

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_buffer.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_buffer.cc:63: int GbmBuffer::GetFd(size_t plane)
const {
On 2016/06/15 03:50:20, Daniele Castagna wrote:
> I'd say |plane| here as parameter is confusing. Should we change this to
index?

Yes, that's confusing. Will change it to index.

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:130:
planes.emplace_back(plane);
On 2016/06/15 03:50:20, Daniele Castagna wrote:
> emplace_back here has the same effect of push_back, right?

Yes. You prefer push_back here?

[vinceh, 2016-06-15 06:31:31]

Uploaded v4.

[Daniele Castagna, 2016-06-15 17:30:10]

https://codereview.chromium.org/2039813002/diff/60001/third_party/khronos/EGL...
File third_party/khronos/EGL/eglext.h (right):

https://codereview.chromium.org/2039813002/diff/60001/third_party/khronos/EGL...
third_party/khronos/EGL/eglext.h:564: #define
EGL_LINUX_DRM_PLANE0_MODIFIER0_EXT	0x3286
Shoudln't there be a space between the identifier and the replacement?

https://codereview.chromium.org/2039813002/diff/60001/ui/gfx/native_pixmap_ha...
File ui/gfx/native_pixmap_handle_ozone.h (right):

https://codereview.chromium.org/2039813002/diff/60001/ui/gfx/native_pixmap_ha...
ui/gfx/native_pixmap_handle_ozone.h:28: // The modifiers is retrieved from GBM
library and passed to EGL driver.
nit: s/modifiers/modifier or make the verb plural.

https://codereview.chromium.org/2039813002/diff/60001/ui/gfx/native_pixmap_ha...
ui/gfx/native_pixmap_handle_ozone.h:30: // browser code. Also one per plane per
entry.
nit: if you use the term browser in chromium, we probably think about the
browser process and not about the whole thing. Maybe you can refer to it as
Chromium code instead?

https://codereview.chromium.org/2039813002/diff/60001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2039813002/diff/60001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:151:
attrs.push_back(EGL_LINUX_DRM_PLANE0_MODIFIER0_EXT + plane * 3);
Is this working as intended?

for plane = 1 we pass EGL_LINUX_DRM_PLANE0_MODIFIER0_EXT + 3, that is
EGL_LINUX_DRM_PLANE1_MODIFIER1_EXT, while I'd expect it to be
EGL_LINUX_DRM_PLANE1_MODIFIER0_EXT.

What am I missing?

[Daniele Castagna, 2016-06-15 17:34:29]

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/gbm_surface_factory.cc (right):

https://codereview.chromium.org/2039813002/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/gbm_surface_factory.cc:130:
planes.emplace_back(plane);
On 2016/06/15 at 04:35:18, vince.h wrote:
> On 2016/06/15 03:50:20, Daniele Castagna wrote:
> > emplace_back here has the same effect of push_back, right?
> 
> Yes. You prefer push_back here?

I don't have any preference. It's just that I'm used to see push_back as the
default and if I see emplace_back I usually wonder why that is needed.
It's perfectly fine to use emplace_back though.

[vinceh, 2016-06-16 02:08:44]

https://codereview.chromium.org/2039813002/diff/60001/third_party/khronos/EGL...
File third_party/khronos/EGL/eglext.h (right):

https://codereview.chromium.org/2039813002/diff/60001/third_party/khronos/EGL...
third_party/khronos/EGL/eglext.h:564: #define
EGL_LINUX_DRM_PLANE0_MODIFIER0_EXT	0x3286
On 2016/06/15 17:30:09, Daniele Castagna wrote:
> Shoudln't there be a space between the identifier and the replacement?

It seems this header uses tab most of the time, so I followed the same.

https://codereview.chromium.org/2039813002/diff/60001/ui/gfx/native_pixmap_ha...
File ui/gfx/native_pixmap_handle_ozone.h (right):

https://codereview.chromium.org/2039813002/diff/60001/ui/gfx/native_pixmap_ha...
ui/gfx/native_pixmap_handle_ozone.h:28: // The modifiers is retrieved from GBM
library and passed to EGL driver.
On 2016/06/15 17:30:09, Daniele Castagna wrote:
> nit: s/modifiers/modifier or make the verb plural.

Oops. Will fix.

https://codereview.chromium.org/2039813002/diff/60001/ui/gfx/native_pixmap_ha...
ui/gfx/native_pixmap_handle_ozone.h:30: // browser code. Also one per plane per
entry.
On 2016/06/15 17:30:09, Daniele Castagna wrote:
> nit: if you use the term browser in chromium, we probably think about the
> browser process and not about the whole thing. Maybe you can refer to it as
> Chromium code instead?

Yes, will fix.

https://codereview.chromium.org/2039813002/diff/60001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2039813002/diff/60001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:151:
attrs.push_back(EGL_LINUX_DRM_PLANE0_MODIFIER0_EXT + plane * 3);
On 2016/06/15 17:30:10, Daniele Castagna wrote:
> Is this working as intended?
> 
> for plane = 1 we pass EGL_LINUX_DRM_PLANE0_MODIFIER0_EXT + 3, that is
> EGL_LINUX_DRM_PLANE1_MODIFIER1_EXT, while I'd expect it to be
> EGL_LINUX_DRM_PLANE1_MODIFIER0_EXT.
> 
> What am I missing?

You're right. That's my mistake. I should have it as "plane * 2" for
EGL_LINUX_DRM_PLANE0_MODIFIER0_EXT. I did not find any issue with it because we
only used one plane. Will fix it. Thanks for catching this.

[vinceh, 2016-06-16 03:10:05]

Uploaded v5. :)_

[Daniele Castagna, 2016-06-17 04:34:53]

Kicked off the trybots, let's see if everything is green.

Please also be aware of crrev.com/1837703002 since you might have to rebase on
that.

https://codereview.chromium.org/2039813002/diff/80001/ui/gl/gl_image_ozone_na...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2039813002/diff/80001/ui/gl/gl_image_ozone_na...
ui/gl/gl_image_ozone_native_pixmap.cc:151:
attrs.push_back(EGL_LINUX_DRM_PLANE0_MODIFIER0_EXT + plane * 2);
I'd prefer to have an array and index that by plane instead of relying on the
define values.
Something like:
const int kLinuxDrmModifier0[] = {EGL_LINUX_DRM_PLANE0_MODIFIER0_EXT,
EGL_LINUX_DRM_PLANE1_MODIFIER0_EXT, EGL_LINUX_DRM_PLANE2_MODIFIER0_EXT}

...

kLinuxDrmModifier0[plane]

WDYT?

[vinceh, 2016-06-17 04:40:30]

On 2016/06/17 04:34:53, Daniele Castagna wrote:
> Kicked off the trybots, let's see if everything is green.
> 
> Please also be aware of crrev.com/1837703002 since you might have to rebase on
> that.
> 
>
https://codereview.chromium.org/2039813002/diff/80001/ui/gl/gl_image_ozone_na...
> File ui/gl/gl_image_ozone_native_pixmap.cc (right):
> 
>
https://codereview.chromium.org/2039813002/diff/80001/ui/gl/gl_image_ozone_na...
> ui/gl/gl_image_ozone_native_pixmap.cc:151:
> attrs.push_back(EGL_LINUX_DRM_PLANE0_MODIFIER0_EXT + plane * 2);
> I'd prefer to have an array and index that by plane instead of relying on the
> define values.
> Something like:
> const int kLinuxDrmModifier0[] = {EGL_LINUX_DRM_PLANE0_MODIFIER0_EXT,
> EGL_LINUX_DRM_PLANE1_MODIFIER0_EXT, EGL_LINUX_DRM_PLANE2_MODIFIER0_EXT}
> 
> ...
> 
> kLinuxDrmModifier0[plane]
> 
> WDYT?

Sure. Will update accordingly. And it seems crrev.com/1837703002 hasn't been
merged. Should we wait for it?

[spang, 2016-06-17 19:41:27]

lgtm

[Daniele Castagna, 2016-06-17 19:47:08]

The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-17 19:47:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2039813002/100001

[commit-bot: I haz the power, 2016-06-17 19:47:28]

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

[commit-bot: I haz the power, 2016-06-17 19:47:29]

Dry run: The author vince.h@nvidia.com has not signed Google Contributor License
Agreement. Please visit https://cla.developers.google.com to sign and manage
CLA.

[vinceh, 2016-06-18 02:44:05]

Hi,

Sorry for some newbie questions. Daniele had helped me trigger the trybot with
v5, but it failed on some other platforms? How can I fix it? What should I do to
land this change?

Thanks,
Vince

[vinceh, 2016-06-22 01:27:03]

Description was changed from

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
==========

to

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[vinceh, 2016-06-22 02:31:28]

The CQ bit was checked by vince.h@nvidia.com

[vinceh, 2016-06-22 02:31:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, spang@chromium.org
Link to the patchset: https://codereview.chromium.org/2039813002/#ps120001
(title: "Add format modifier IDs for EGL_EXT_image_dma_buf_import extension")

[commit-bot: I haz the power, 2016-06-22 02:32:32]

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

[commit-bot: I haz the power, 2016-06-22 02:41:10]

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

[commit-bot: I haz the power, 2016-06-22 02:41:11]

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...)

[vinceh, 2016-06-23 11:02:30]

vince.h@nvidia.com changed reviewers:
+ dnicoara@chromium.org, jam@chromium.org, jln@chromium.org

[vinceh, 2016-06-23 11:02:31]

Add jln, dnicoara, and jam as reviewers.

[jam, 2016-06-23 15:58:27]

On 2016/06/23 11:02:31, vince.h wrote:
> Add jln, dnicoara, and jam as reviewers.

since you have multiple reviewers, please specify which files you want us to
look at

[vinceh, 2016-06-23 16:20:40]

On 2016/06/23 15:58:27, jam wrote:
> On 2016/06/23 11:02:31, vince.h wrote:
> > Add jln, dnicoara, and jam as reviewers.
> 
> since you have multiple reviewers, please specify which files you want us to
> look at

Hi jln, 

The presumit complained that I did not have module owners' approvals, and I got
you guys' names from "git cl owners". So I guess you guys can help on below?
Thanks!

jln:
  ui/gfx/ipc/gfx_param_traits_macros.h
dnicoara:
  ui/gl/gl_image_ozone_native_pixmap.cc
  ui/gl/gl_image_ozone_native_pixmap_drm_unittest.cc
  ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc
  ui/ozone/platform/drm/gpu/drm_thread.cc
  ui/ozone/platform/drm/gpu/drm_thread.h
  ui/ozone/platform/drm/gpu/drm_thread_proxy.cc
  ui/ozone/platform/drm/gpu/drm_thread_proxy.h
  ui/ozone/platform/drm/gpu/gbm_buffer.cc
  ui/ozone/platform/drm/gpu/gbm_buffer.h
  ui/ozone/platform/drm/gpu/gbm_surface_factory.cc
jam:
  chrome/gpu/arc_gpu_video_decode_accelerator.cc
  components/exo/display.cc
  components/exo/display.h
  components/exo/display_unittest.cc
  components/exo/wayland/server.cc
  gpu/ipc/client/gpu_channel_host.cc
  gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.cc
  gpu/ipc/client/gpu_memory_buffer_impl_ozone_native_pixmap.h
  media/gpu/video_decode_accelerator_unittest.cc
  third_party/khronos/EGL/eglext.h
  third_party/khronos/README.chromium
  ui/gfx/native_pixmap_handle_ozone.cc
  ui/gfx/native_pixmap_handle_ozone.h
  ui/gl/gl_image_ozone_native_pixmap.cc
  ui/gl/gl_image_ozone_native_pixmap_drm_unittest.cc
  ui/ozone/platform/cast/surface_factory_cast.cc
  ui/ozone/platform/drm/client_native_pixmap_factory_gbm.cc
  ui/ozone/platform/drm/gpu/drm_thread.cc
  ui/ozone/platform/drm/gpu/drm_thread.h
  ui/ozone/platform/drm/gpu/drm_thread_proxy.cc
  ui/ozone/platform/drm/gpu/drm_thread_proxy.h
  ui/ozone/platform/drm/gpu/gbm_buffer.cc
  ui/ozone/platform/drm/gpu/gbm_buffer.h
  ui/ozone/platform/drm/gpu/gbm_surface_factory.cc
  ui/ozone/platform/headless/headless_surface_factory.cc
  ui/ozone/public/native_pixmap.h

[dnicoara, 2016-06-23 16:40:50]

spang@ already approved ui/ozone/* and ui/gl/*ozone* and reveman@ approved
components/* and ui/gl/*

Remaining changes to review from owners:

jln@ for security review for the IPC changes: ui/gfx/ipc/*

jam@ for chrome/* gpu/* media/* third_party/* and ui/gfx/*

[jam, 2016-06-23 17:15:21]

On 2016/06/23 16:40:50, dnicoara wrote:
> spang@ already approved ui/ozone/* and ui/gl/*ozone* and reveman@ approved
> components/* and ui/gl/*
> 
> Remaining changes to review from owners:
> 
> jln@ for security review for the IPC changes: ui/gfx/ipc/*
> 
> jam@ for chrome/* gpu/* media/* third_party/* and ui/gfx/*

rubberstamp lgtm

[vinceh, 2016-06-24 04:51:28]

On 2016/06/23 17:15:21, jam wrote:
> On 2016/06/23 16:40:50, dnicoara wrote:
> > spang@ already approved ui/ozone/* and ui/gl/*ozone* and reveman@ approved
> > components/* and ui/gl/*
> > 
> > Remaining changes to review from owners:
> > 
> > jln@ for security review for the IPC changes: ui/gfx/ipc/*
> > 
> > jam@ for chrome/* gpu/* media/* third_party/* and ui/gfx/*
> 
> rubberstamp lgtm

Thanks, dnicoara & jam.

Hello jln, do you mind taking a look at the IPC changes? Thanks!

[vinceh, 2016-06-27 07:29:56]

Hi jln,

Could you please ack this patch? :)

Thanks,
Vince

[vinceh, 2016-06-28 01:30:47]

vince.h@nvidia.com changed reviewers:
+ dcheng@chromium.org, jschuh@chromium.org, nasko@chromium.org

[vinceh, 2016-06-28 01:30:48]

Adding dcheng/jschuh/nasko to review the ui/gfx/ipc/* change in case jln is OOO.

[marcheu1, 2016-06-28 01:36:27]

Description was changed from

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[marcheu1, 2016-06-28 01:36:27]

marcheu@google.com changed reviewers:
+ rickyz@chromium.org

[marcheu1, 2016-06-28 01:36:46]

On 2016/06/28 01:30:48, vince.h wrote:
> Adding dcheng/jschuh/nasko to review the ui/gfx/ipc/* change in case jln is
OOO.

Added rickyz@ who has been doing security reviews for cros.

[rickyz (no longer on Chrome), 2016-06-28 06:53:13]

lgtm

https://codereview.chromium.org/2039813002/diff/220001/components/exo/wayland...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/2039813002/diff/220001/components/exo/wayland...
components/exo/wayland/server.cc:530: int num_planes =
size_t

https://codereview.chromium.org/2039813002/diff/220001/ui/gl/gl_image_ozone_n...
File ui/gl/gl_image_ozone_native_pixmap.cc (right):

https://codereview.chromium.org/2039813002/diff/220001/ui/gl/gl_image_ozone_n...
ui/gl/gl_image_ozone_native_pixmap.cc:155:
attrs.push_back(kLinuxDrmModifiers[plane]);
Should we DCHECK that plane < arraysize(kLinuxDrmModifiers) here?

[reveman, 2016-06-28 07:16:17]

https://codereview.chromium.org/2039813002/diff/220001/ui/gfx/native_pixmap_h...
File ui/gfx/native_pixmap_handle_ozone.h (right):

https://codereview.chromium.org/2039813002/diff/220001/ui/gfx/native_pixmap_h...
ui/gfx/native_pixmap_handle_ozone.h:18: struct GFX_EXPORT GbmBufferPlane {
Should this be NativePixmapPlane? Not all ozone implementations are required to
use Gbm.

[dcheng, 2016-06-28 07:21:41]

On 2016/06/28 01:30:48, vince.h wrote:
> Adding dcheng/jschuh/nasko to review the ui/gfx/ipc/* change in case jln is
OOO.

Btw, please just pick one IPC reviewer: if they're not responsive, ping them via
chat / email. Picking a bunch of reviewers just means that no one will review
the CL, since everyone assumes someone else is going to look.

[vinceh, 2016-06-28 08:08:39]

On 2016/06/28 06:53:13, rickyz wrote:
> lgtm
> 
>
https://codereview.chromium.org/2039813002/diff/220001/components/exo/wayland...
> File components/exo/wayland/server.cc (right):
> 
>
https://codereview.chromium.org/2039813002/diff/220001/components/exo/wayland...
> components/exo/wayland/server.cc:530: int num_planes =
> size_t
> 
>
https://codereview.chromium.org/2039813002/diff/220001/ui/gl/gl_image_ozone_n...
> File ui/gl/gl_image_ozone_native_pixmap.cc (right):
> 
>
https://codereview.chromium.org/2039813002/diff/220001/ui/gl/gl_image_ozone_n...
> ui/gl/gl_image_ozone_native_pixmap.cc:155:
> attrs.push_back(kLinuxDrmModifiers[plane]);
> Should we DCHECK that plane < arraysize(kLinuxDrmModifiers) here?

Thanks! Updated.

[vinceh, 2016-06-28 08:10:54]

On 2016/06/28 07:16:17, reveman wrote:
>
https://codereview.chromium.org/2039813002/diff/220001/ui/gfx/native_pixmap_h...
> File ui/gfx/native_pixmap_handle_ozone.h (right):
> 
>
https://codereview.chromium.org/2039813002/diff/220001/ui/gfx/native_pixmap_h...
> ui/gfx/native_pixmap_handle_ozone.h:18: struct GFX_EXPORT GbmBufferPlane {
> Should this be NativePixmapPlane? Not all ozone implementations are required
to
> use Gbm.

Hmmm I don't know actually because I'm not familiar with ozone. I can rename the
struct if you think that's better. What's the implementation that is not using
Gbm btw?

Thanks,
Vince

[vinceh, 2016-06-28 08:14:04]

On 2016/06/28 07:21:41, dcheng wrote:
> On 2016/06/28 01:30:48, vince.h wrote:
> > Adding dcheng/jschuh/nasko to review the ui/gfx/ipc/* change in case jln is
> OOO.
> 
> Btw, please just pick one IPC reviewer: if they're not responsive, ping them
via
> chat / email. Picking a bunch of reviewers just means that no one will review
> the CL, since everyone assumes someone else is going to look.

Sorry about this. I don't know the review policy of Chromium. Obviously it's
unlike the kernel review. Will keep your suggestion in mind. :)

[reveman, 2016-06-28 08:32:33]

On 2016/06/28 at 08:10:54, vince.h wrote:
> On 2016/06/28 07:16:17, reveman wrote:
> >
https://codereview.chromium.org/2039813002/diff/220001/ui/gfx/native_pixmap_h...
> > File ui/gfx/native_pixmap_handle_ozone.h (right):
> > 
> >
https://codereview.chromium.org/2039813002/diff/220001/ui/gfx/native_pixmap_h...
> > ui/gfx/native_pixmap_handle_ozone.h:18: struct GFX_EXPORT GbmBufferPlane {
> > Should this be NativePixmapPlane? Not all ozone implementations are required
to
> > use Gbm.
> 
> Hmmm I don't know actually because I'm not familiar with ozone. I can rename
the struct if you think that's better. What's the implementation that is not
using Gbm btw?

wayland, x11, etc. See src/ui/ozone/platform/. I think renaming the struct will
avoid some confusion as those other platforms should also be able to use this
(especially wayland) even though they are not explicitly using gbm.

[vinceh, 2016-06-28 08:36:38]

On 2016/06/28 08:32:33, reveman wrote:
> On 2016/06/28 at 08:10:54, vince.h wrote:
> > On 2016/06/28 07:16:17, reveman wrote:
> > >
>
https://codereview.chromium.org/2039813002/diff/220001/ui/gfx/native_pixmap_h...
> > > File ui/gfx/native_pixmap_handle_ozone.h (right):
> > > 
> > >
>
https://codereview.chromium.org/2039813002/diff/220001/ui/gfx/native_pixmap_h...
> > > ui/gfx/native_pixmap_handle_ozone.h:18: struct GFX_EXPORT GbmBufferPlane {
> > > Should this be NativePixmapPlane? Not all ozone implementations are
required
> to
> > > use Gbm.
> > 
> > Hmmm I don't know actually because I'm not familiar with ozone. I can rename
> the struct if you think that's better. What's the implementation that is not
> using Gbm btw?
> 
> wayland, x11, etc. See src/ui/ozone/platform/. I think renaming the struct
will
> avoid some confusion as those other platforms should also be able to use this
> (especially wayland) even though they are not explicitly using gbm.

Thanks for your explanation. Will update. :)

[vinceh, 2016-06-28 13:21:52]

The CQ bit was checked by vince.h@nvidia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-28 13:22:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-28 16:18:59]

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

[commit-bot: I haz the power, 2016-06-28 16:19:01]

Dry run: This issue passed the CQ dry run.

[vinceh, 2016-06-29 00:58:12]

The CQ bit was checked by vince.h@nvidia.com

[vinceh, 2016-06-29 00:58:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, spang@chromium.org, jam@chromium.org, rickyz@chromium.org
Link to the patchset: https://codereview.chromium.org/2039813002/#ps260001
(title: "Add format modifier IDs for EGL_EXT_image_dma_buf_import extension")

[commit-bot: I haz the power, 2016-06-29 00:59:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 02:12:43]

Description was changed from

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2016-06-29 02:12:45]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-06-29 02:15:34]

Description was changed from

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/29292a32f6f429a790e8663ce09bf67d165f49e9
Cr-Commit-Position: refs/heads/master@{#402656}
==========

[commit-bot: I haz the power, 2016-06-29 02:15:36]

Patchset 14 (id:??) landed as
https://crrev.com/29292a32f6f429a790e8663ce09bf67d165f49e9
Cr-Commit-Position: refs/heads/master@{#402656}

[Josh Horwich, 2016-06-29 19:32:00]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/2110213002/ by jhorwich@chromium.org.

The reason for reverting is: crbug.com/624492

With this change we started getting Failed to create GLImage followed by a
flurry of GL_INVALID_OPERATION / GL_INVALID_FRAMEBUFFER_OPERATION on
veyron_minnie, see bug for details..

[vinceh, 2016-06-30 07:09:00]

On 2016/06/29 19:32:00, Josh Horwich wrote:
> A revert of this CL (patchset #14 id:260001) has been created in
> https://codereview.chromium.org/2110213002/ by mailto:jhorwich@chromium.org.
> 
> The reason for reverting is: crbug.com/624492
> 
> With this change we started getting Failed to create GLImage followed by a
> flurry of GL_INVALID_OPERATION / GL_INVALID_FRAMEBUFFER_OPERATION on
> veyron_minnie, see bug for details..

Sorry for the regression. I'm working on the fix based on this CL. :(

With Stephane's suggestion I will add a new extension
EGL_EXT_image_dma_buf_import_modifiers to indicate if the platform supports
modifiers for EGL_EXT_image_dma_buf_import extension. We can test the presence
of the new extension and then assign modifier for EGL_EXT_image_dma_buf_import
if need be. But then I have problem with knowing the presence of extension. We
don't have something like GLSurfaceEGL::HasEGLExtension in GLImageEGL, and it
seems weird to call eglQueryString in GLImageOzoneNativePixmap. So do you guys
have any suggestion about this?

[vinceh, 2016-06-30 08:27:58]

Description was changed from

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/29292a32f6f429a790e8663ce09bf67d165f49e9
Cr-Commit-Position: refs/heads/master@{#402656}
==========

to

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/29292a32f6f429a790e8663ce09bf67d165f49e9
Cr-Commit-Position: refs/heads/master@{#402656}
==========

[vinceh, 2016-06-30 09:04:30]

On 2016/06/30 07:09:00, vinceh wrote:
> On 2016/06/29 19:32:00, Josh Horwich wrote:
> > A revert of this CL (patchset #14 id:260001) has been created in
> > https://codereview.chromium.org/2110213002/ by mailto:jhorwich@chromium.org.
> > 
> > The reason for reverting is: crbug.com/624492
> > 
> > With this change we started getting Failed to create GLImage followed by a
> > flurry of GL_INVALID_OPERATION / GL_INVALID_FRAMEBUFFER_OPERATION on
> > veyron_minnie, see bug for details..
> 
> Sorry for the regression. I'm working on the fix based on this CL. :(
> 
> With Stephane's suggestion I will add a new extension
> EGL_EXT_image_dma_buf_import_modifiers to indicate if the platform supports
> modifiers for EGL_EXT_image_dma_buf_import extension. We can test the presence
> of the new extension and then assign modifier for EGL_EXT_image_dma_buf_import
> if need be. But then I have problem with knowing the presence of extension. We
> don't have something like GLSurfaceEGL::HasEGLExtension in GLImageEGL, and it
> seems weird to call eglQueryString in GLImageOzoneNativePixmap. So do you guys
> have any suggestion about this?

Oh, I did not notice the HasEGLExtension is a static function member. Are you
fine with the following check with extension?

--- a/ui/gl/gl_image_ozone_native_pixmap.cc
+++ b/ui/gl/gl_image_ozone_native_pixmap.cc
@@ -4,6 +4,7 @@
 
 #include "ui/gfx/buffer_format_util.h"
 #include "ui/gl/gl_image_ozone_native_pixmap.h"
+#include "ui/gl/gl_surface_egl.h"
 
 #define FOURCC(a, b, c, d)                                        \
   ((static_cast<uint32_t>(a)) | (static_cast<uint32_t>(b) << 8) | \
@@ -140,11 +141,12 @@ bool
GLImageOzoneNativePixmap::Initialize(ui::NativePixmap* pixmap,
     const EGLint kLinuxDrmModifiers[] = {EGL_LINUX_DRM_PLANE0_MODIFIER0_EXT,
                                          EGL_LINUX_DRM_PLANE1_MODIFIER0_EXT,
                                          EGL_LINUX_DRM_PLANE2_MODIFIER0_EXT};
+    bool has_dma_buf_import_modifier =
+       
GLSurfaceEGL::HasEGLExtension("EGL_EXT_image_dma_buf_import_modifiers");

@@ -152,11 +154,14 @@ bool
GLImageOzoneNativePixmap::Initialize(ui::NativePixmap* pixmap,
       attrs.push_back(pixmap->GetDmaBufOffset(plane));
       attrs.push_back(EGL_DMA_BUF_PLANE0_PITCH_EXT + plane * 3);
       attrs.push_back(pixmap->GetDmaBufPitch(plane));
+       
+      if (has_dma_buf_import_modifier) {
+        uint64_t modifier = pixmap->GetDmaBufModifier(plane);
+        DCHECK(plane < arraysize(kLinuxDrmModifiers));
+        attrs.push_back(kLinuxDrmModifiers[plane]);
+        attrs.push_back(modifier & 0xffffffff);
+        attrs.push_back(kLinuxDrmModifiers[plane] + 1);
+        attrs.push_back(static_cast<uint32_t>(modifier >> 32));
+      }
     }

[reveman, 2016-06-30 14:54:04]

On 2016/06/30 at 09:04:30, vince.h wrote:
> On 2016/06/30 07:09:00, vinceh wrote:
> > On 2016/06/29 19:32:00, Josh Horwich wrote:
> > > A revert of this CL (patchset #14 id:260001) has been created in
> > > https://codereview.chromium.org/2110213002/ by
mailto:jhorwich@chromium.org.
> > > 
> > > The reason for reverting is: crbug.com/624492
> > > 
> > > With this change we started getting Failed to create GLImage followed by a
> > > flurry of GL_INVALID_OPERATION / GL_INVALID_FRAMEBUFFER_OPERATION on
> > > veyron_minnie, see bug for details..
> > 
> > Sorry for the regression. I'm working on the fix based on this CL. :(
> > 
> > With Stephane's suggestion I will add a new extension
> > EGL_EXT_image_dma_buf_import_modifiers to indicate if the platform supports
> > modifiers for EGL_EXT_image_dma_buf_import extension. We can test the
presence
> > of the new extension and then assign modifier for
EGL_EXT_image_dma_buf_import
> > if need be. But then I have problem with knowing the presence of extension.
We
> > don't have something like GLSurfaceEGL::HasEGLExtension in GLImageEGL, and
it
> > seems weird to call eglQueryString in GLImageOzoneNativePixmap. So do you
guys
> > have any suggestion about this?
> 
> Oh, I did not notice the HasEGLExtension is a static function member. Are you
fine with the following check with extension?
> 
> --- a/ui/gl/gl_image_ozone_native_pixmap.cc
> +++ b/ui/gl/gl_image_ozone_native_pixmap.cc
> @@ -4,6 +4,7 @@
>  
>  #include "ui/gfx/buffer_format_util.h"
>  #include "ui/gl/gl_image_ozone_native_pixmap.h"
> +#include "ui/gl/gl_surface_egl.h"
>  
>  #define FOURCC(a, b, c, d)                                        \
>    ((static_cast<uint32_t>(a)) | (static_cast<uint32_t>(b) << 8) | \
> @@ -140,11 +141,12 @@ bool
GLImageOzoneNativePixmap::Initialize(ui::NativePixmap* pixmap,
>      const EGLint kLinuxDrmModifiers[] = {EGL_LINUX_DRM_PLANE0_MODIFIER0_EXT,
>                                           EGL_LINUX_DRM_PLANE1_MODIFIER0_EXT,
>                                           EGL_LINUX_DRM_PLANE2_MODIFIER0_EXT};
> +    bool has_dma_buf_import_modifier =
> +       
GLSurfaceEGL::HasEGLExtension("EGL_EXT_image_dma_buf_import_modifiers");
> 
> @@ -152,11 +154,14 @@ bool
GLImageOzoneNativePixmap::Initialize(ui::NativePixmap* pixmap,
>        attrs.push_back(pixmap->GetDmaBufOffset(plane));
>        attrs.push_back(EGL_DMA_BUF_PLANE0_PITCH_EXT + plane * 3);
>        attrs.push_back(pixmap->GetDmaBufPitch(plane));
> +       
> +      if (has_dma_buf_import_modifier) {
> +        uint64_t modifier = pixmap->GetDmaBufModifier(plane);
> +        DCHECK(plane < arraysize(kLinuxDrmModifiers));
> +        attrs.push_back(kLinuxDrmModifiers[plane]);
> +        attrs.push_back(modifier & 0xffffffff);
> +        attrs.push_back(kLinuxDrmModifiers[plane] + 1);
> +        attrs.push_back(static_cast<uint32_t>(modifier >> 32));
> +      }
>      }

sgtm

[vinceh, 2016-07-01 06:01:35]

The CQ bit was checked by vince.h@nvidia.com

[vinceh, 2016-07-01 06:01:36]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
rickyz@chromium.org, reveman@chromium.org, spang@chromium.org
Link to the patchset: https://codereview.chromium.org/2039813002/#ps300001
(title: "Add format modifier IDs for EGL_EXT_image_dma_buf_import extension")

[commit-bot: I haz the power, 2016-07-01 06:01:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marcheu1, 2016-07-01 06:02:05]

tested on veyron_jerry. LGTM.

[vinceh, 2016-07-01 06:03:30]

On 2016/07/01 06:02:05, marcheu1 wrote:
> tested on veyron_jerry. LGTM.

Thanks a lot! :)

[commit-bot: I haz the power, 2016-07-01 06:56:28]

Description was changed from

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/29292a32f6f429a790e8663ce09bf67d165f49e9
Cr-Commit-Position: refs/heads/master@{#402656}
==========

to

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/29292a32f6f429a790e8663ce09bf67d165f49e9
Cr-Commit-Position: refs/heads/master@{#402656}
==========

[commit-bot: I haz the power, 2016-07-01 06:56:30]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-07-01 06:56:37]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-01 06:58:16]

Description was changed from

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/29292a32f6f429a790e8663ce09bf67d165f49e9
Cr-Commit-Position: refs/heads/master@{#402656}
==========

to

==========
Add format modifier IDs for EGL_EXT_image_dma_buf_import extension

And use them when creating a eglImageKHR for a EGL_LINUX_DMA_BUF_EXT target.

BUG=chromium:478339
TEST=none

Signed-off-by: Vince Hsu <vince.h@nvidia.com>
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Committed: https://crrev.com/29292a32f6f429a790e8663ce09bf67d165f49e9
Committed: https://crrev.com/47661a7fc2314e3fb9a1478b3e8611208e216f63
Cr-Original-Commit-Position: refs/heads/master@{#402656}
Cr-Commit-Position: refs/heads/master@{#403418}
==========

[commit-bot: I haz the power, 2016-07-01 06:58:18]

Patchset 16 (id:??) landed as
https://crrev.com/47661a7fc2314e3fb9a1478b3e8611208e216f63
Cr-Commit-Position: refs/heads/master@{#403418}