Expose GpuMemoryBufferId through glGetImageivCHROMIUM.

Expose GpuMemoryBufferId through glGetImageivCHROMIUM.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM Image extension to allow the GpuMemoryBuffer id to be
obtained by a new function glGetImageivCHROMIUM.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/11c5d208606dbce8660ad7f5e41e0aa770655c82
Cr-Commit-Position: refs/heads/master@{#396510}

[erikchen, 2016-05-13 21:09:10]

Description was changed from

==========
Expose gpu_memory_buffer_id through CreateGpuMemoryBufferImageCHROMIUM

BUG=
==========

to

==========
Expose gpu_memory_buffer_id through CreateGpuMemoryBufferImageCHROMIUM

BUG=
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
==========

[erikchen, 2016-05-13 21:09:11]

Description was changed from

==========
Expose gpu_memory_buffer_id through CreateGpuMemoryBufferImageCHROMIUM

BUG=
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

==========
Expose gpu_memory_buffer_id through CreateGpuMemoryBufferImageCHROMIUM

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-05-13 21:11:00]

Description was changed from

==========
Expose gpu_memory_buffer_id through CreateGpuMemoryBufferImageCHROMIUM

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Expose GpuMemoryBufferId through CreateGpuMemoryBufferImageCHROMIUM.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM gl extension CreateGpuMemoryBufferImageCHROMIUM to also
return the GpuMemoryBufferId.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-05-13 21:11:24]

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

[commit-bot: I haz the power, 2016-05-13 21:11:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974163003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974163003/1

[erikchen, 2016-05-13 21:11:59]

Description was changed from

==========
Expose GpuMemoryBufferId through CreateGpuMemoryBufferImageCHROMIUM.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM gl extension CreateGpuMemoryBufferImageCHROMIUM to also
return the GpuMemoryBufferId.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Expose GpuMemoryBufferId through CreateGpuMemoryBufferImageCHROMIUM.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM gl extension CreateGpuMemoryBufferImageCHROMIUM to also
return the GpuMemoryBufferId.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-05-13 21:15:19]

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

[erikchen, 2016-05-13 21:15:20]

reveman: Please review.

I'm not super pleased about modifying CreateGpuMemoryBufferImageCHROMIUM, but
the alternative would be to make/modify 2 new calls: 1 to return a
GpuMemoryBufferId, and another to make an image from a GpuMemoryBufferId.

On Mac, we want every non-test call site of CreateGpuMemoryBufferImageCHROMIUM
to eventually be passing a GpuMemoryBufferId to the compositor, so this change
didn't seem too unreasonable.

[commit-bot: I haz the power, 2016-05-13 21:30:44]

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

[commit-bot: I haz the power, 2016-05-13 21:30:45]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[erikchen, 2016-05-16 23:58:58]

On 2016/05/13 21:30:45, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

reveman: Ping?

[reveman, 2016-05-17 13:32:16]

hm, I think it might be time to remove CHROMIUM_gpu_memory_buffer_image and
instead expose the GpuMemoryBuffer interface to blink. ie. add a
WebGpuMemoryBuffer class. That was the original plan and
CHROMIUM_gpu_memory_buffer_image and was just to avoid all that plumbing until
the GMB API was relatively stable.

[erikchen, 2016-05-17 16:42:04]

On 2016/05/17 13:32:16, reveman wrote:
> hm, I think it might be time to remove CHROMIUM_gpu_memory_buffer_image and
> instead expose the GpuMemoryBuffer interface to blink. ie. add a
> WebGpuMemoryBuffer class. That was the original plan and
> CHROMIUM_gpu_memory_buffer_image and was just to avoid all that plumbing until
> the GMB API was relatively stable.

1. Can you send me this original plan? 
2. Did you talk with WebKit public/ OWNERs?
3. What benefit do you get by exposing a WebGpuMemoryBuffer class? [Is it just
going to be a wrapper around a uint32 "id"?]
4. How are you planning on exposing the new interface to the relevant WebKit
drawing classes? Add another virtual method to WebGraphicsContext3DProvider?

[erikchen, 2016-05-20 23:08:56]

reveman: Ping?

[reveman, 2016-05-23 17:39:25]

I think it would be a bit more GL-ish with an enum value for the GMB_ID and
something like a GetIntegerv to get the ID from an image but I'm fine with this
too if it's what you prefer. lgtm.

[erikchen, 2016-05-23 17:43:02]

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

[commit-bot: I haz the power, 2016-05-23 17:43:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974163003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974163003/40001

[commit-bot: I haz the power, 2016-05-23 17:45:31]

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

[commit-bot: I haz the power, 2016-05-23 17:45:32]

Dry run: Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[erikchen, 2016-05-23 17:46:31]

erikchen@chromium.org changed reviewers:
+ piman@chromium.org

[erikchen, 2016-05-23 17:46:32]

piman: Do you prefer the current interface, or reveman's suggestion in #15? I'm
happy to do either.

[piman, 2016-05-23 17:52:24]

On 2016/05/23 17:46:32, erikchen wrote:
> piman: Do you prefer the current interface, or reveman's suggestion in #15?
I'm
> happy to do either.

I think I prefer the reveman's suggestion indeed... If that's ok?

[erikchen, 2016-05-25 00:17:39]

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

[commit-bot: I haz the power, 2016-05-25 00:18:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974163003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974163003/60001

[commit-bot: I haz the power, 2016-05-25 01:30:58]

Dry run: None

[erikchen, 2016-05-25 01:37:44]

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

[commit-bot: I haz the power, 2016-05-25 01:38:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974163003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974163003/80001

[erikchen, 2016-05-25 01:38:31]

Description was changed from

==========
Expose GpuMemoryBufferId through CreateGpuMemoryBufferImageCHROMIUM.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM gl extension CreateGpuMemoryBufferImageCHROMIUM to also
return the GpuMemoryBufferId.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Expose GpuMemoryBufferId through GetIntegerv.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM Image extension to allow the GpuMemoryBuffer id to be
exposed by GetIntegerv.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-05-25 01:38:39]

Description was changed from

==========
Expose GpuMemoryBufferId through GetIntegerv.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM Image extension to allow the GpuMemoryBuffer id to be
exposed by GetIntegerv.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Expose GpuMemoryBufferId through GetIntegerv.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM Image extension to allow the GpuMemoryBuffer id to be
exposed by GetIntegerv.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-25 03:15:27]

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

[commit-bot: I haz the power, 2016-05-25 03:15:28]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-25 03:15:39]

Dry run: None

[erikchen, 2016-05-25 17:27:51]

piman: PTAL

[piman, 2016-05-25 18:35:57]

On 2016/05/25 17:27:51, erikchen wrote:
> piman: PTAL

Oh, hmm I don't think this is what I had in mind - this version forces a full
round-trip to the GPU process to get the ID (think client-side finish), which is
likely to cause large slowdowns. Also tying it to the bound state may be overly
constraining moving forward - I was thinking maybe something like
glGetImageivCHROMIUM(image_id, GL_GPU_MEMORY_BUFFER_ID, &id);

I realize now we're not caching that information about the image on the client
side. We could do that in CommandBufferProxyImpl with a simple map.

[erikchen, 2016-05-26 00:49:59]

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

[commit-bot: I haz the power, 2016-05-26 00:50:35]

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

[erikchen, 2016-05-26 00:50:55]

Description was changed from

==========
Expose GpuMemoryBufferId through GetIntegerv.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM Image extension to allow the GpuMemoryBuffer id to be
exposed by GetIntegerv.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Expose GpuMemoryBufferId through glGetImageivCHROMIUM.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM Image extension to allow the GpuMemoryBuffer id to be
obtained by a new function glGetImageivCHROMIUM.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[erikchen, 2016-05-26 00:51:14]

Description was changed from

==========
Expose GpuMemoryBufferId through glGetImageivCHROMIUM.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM Image extension to allow the GpuMemoryBuffer id to be
obtained by a new function glGetImageivCHROMIUM.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Expose GpuMemoryBufferId through glGetImageivCHROMIUM.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM Image extension to allow the GpuMemoryBuffer id to be
obtained by a new function glGetImageivCHROMIUM.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-26 01:12:54]

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

[commit-bot: I haz the power, 2016-05-26 01:12:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-05-26 01:27:33]

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

[commit-bot: I haz the power, 2016-05-26 01:28:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974163003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974163003/120001

[commit-bot: I haz the power, 2016-05-26 01:46:50]

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

[commit-bot: I haz the power, 2016-05-26 01:46:51]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2016-05-26 05:10:13]

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

[commit-bot: I haz the power, 2016-05-26 05:10:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974163003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974163003/140001

[commit-bot: I haz the power, 2016-05-26 05:33:06]

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

[commit-bot: I haz the power, 2016-05-26 05:33:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-05-26 17:52:52]

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

[commit-bot: I haz the power, 2016-05-26 17:53:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974163003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974163003/160001

[commit-bot: I haz the power, 2016-05-26 17:58:58]

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

[commit-bot: I haz the power, 2016-05-26 17:58:59]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[erikchen, 2016-05-26 19:43:49]

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

[commit-bot: I haz the power, 2016-05-26 19:44:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974163003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974163003/180001

[erikchen, 2016-05-26 19:45:01]

piman: PTAL

[piman, 2016-05-26 21:16:07]

https://codereview.chromium.org/1974163003/diff/180001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1974163003/diff/180001/components/mus/gles2/c...
components/mus/gles2/command_buffer_local.cc:313: NOTIMPLEMENTED();
Can you add a TODO?

https://codereview.chromium.org/1974163003/diff/180001/components/mus/public/...
File components/mus/public/cpp/lib/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/1974163003/diff/180001/components/mus/public/...
components/mus/public/cpp/lib/command_buffer_client_impl.cc:275:
NOTIMPLEMENTED();
ditto?

https://codereview.chromium.org/1974163003/diff/180001/gpu/command_buffer/cmd...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/1974163003/diff/180001/gpu/command_buffer/cmd...
gpu/command_buffer/cmd_buffer_functions.txt:279: GL_APICALL GLint       
GL_APIENTRY glGetImageivCHROMIUM (GLuint image_id, GLenum param);
So, if we call it glGetImageivCHROMIUM, it should look like an "iv" function,
i.e. one that takes a GLint* (for the return value) - that's the most GL-y way,
and slightly more scalable because we can also have other variations for other
queries we might need (e.g. an "uiv" version to get a GLuint, or if we want to
return >1 values).

If we want to return a GLint instead (that's less consistent, only a couple of
API calls do that), it shouldn't be "iv", but then again glGetImageCHROMIUM
would be weird by itself. glGetImageInfoCHROMIUM maybe?

[commit-bot: I haz the power, 2016-05-26 21:40:33]

Dry run: None

[erikchen, 2016-05-26 21:41:27]

https://codereview.chromium.org/1974163003/diff/180001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1974163003/diff/180001/components/mus/gles2/c...
components/mus/gles2/command_buffer_local.cc:313: NOTIMPLEMENTED();
On 2016/05/26 21:16:07, piman wrote:
> Can you add a TODO?

Done.

https://codereview.chromium.org/1974163003/diff/180001/components/mus/public/...
File components/mus/public/cpp/lib/command_buffer_client_impl.cc (right):

https://codereview.chromium.org/1974163003/diff/180001/components/mus/public/...
components/mus/public/cpp/lib/command_buffer_client_impl.cc:275:
NOTIMPLEMENTED();
On 2016/05/26 21:16:07, piman wrote:
> ditto?

Done.

https://codereview.chromium.org/1974163003/diff/180001/gpu/command_buffer/cmd...
File gpu/command_buffer/cmd_buffer_functions.txt (right):

https://codereview.chromium.org/1974163003/diff/180001/gpu/command_buffer/cmd...
gpu/command_buffer/cmd_buffer_functions.txt:279: GL_APICALL GLint       
GL_APIENTRY glGetImageivCHROMIUM (GLuint image_id, GLenum param);
On 2016/05/26 21:16:07, piman wrote:
> So, if we call it glGetImageivCHROMIUM, it should look like an "iv" function,
> i.e. one that takes a GLint* (for the return value) - that's the most GL-y
way,
> and slightly more scalable because we can also have other variations for other
> queries we might need (e.g. an "uiv" version to get a GLuint, or if we want to
> return >1 values).
> 
> If we want to return a GLint instead (that's less consistent, only a couple of
> API calls do that), it shouldn't be "iv", but then again glGetImageCHROMIUM
> would be weird by itself. glGetImageInfoCHROMIUM maybe?

Changed the signature to be:
void glGetImageivCHROMIUM(GLuint image_id, GLenum param, GLint* data);

[erikchen, 2016-05-26 21:41:36]

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

[commit-bot: I haz the power, 2016-05-26 21:42:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974163003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974163003/200001

[piman, 2016-05-26 21:51:03]

lgtm

https://codereview.chromium.org/1974163003/diff/200001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1974163003/diff/200001/components/mus/gles2/c...
components/mus/gles2/command_buffer_local.cc:313: // TODO: Once this class
supports IOSurface GpuMemoryBuffer backed images, it
nit: please add your username to the TODO and/or link a bug. It's not
necessarily that you're on the hook to do it, but you are the one with the
knowledge about what to do.

[erikchen, 2016-05-26 22:00:17]

https://codereview.chromium.org/1974163003/diff/200001/components/mus/gles2/c...
File components/mus/gles2/command_buffer_local.cc (right):

https://codereview.chromium.org/1974163003/diff/200001/components/mus/gles2/c...
components/mus/gles2/command_buffer_local.cc:313: // TODO: Once this class
supports IOSurface GpuMemoryBuffer backed images, it
On 2016/05/26 21:51:03, piman wrote:
> nit: please add your username to the TODO and/or link a bug. It's not
> necessarily that you're on the hook to do it, but you are the one with the
> knowledge about what to do.

Done.

[commit-bot: I haz the power, 2016-05-26 22:00:44]

Dry run: None

[erikchen, 2016-05-26 22:01:32]

erikchen@chromium.org changed reviewers:
+ junov@chromium.org, sky@chromium.org

[erikchen, 2016-05-26 22:01:33]

sky: Please review components/mus
kbr: Please review third_party/WebKit/Source/platform/graphics

junov: Please review
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.*

[sky, 2016-05-26 22:38:27]

sky@chromium.org changed reviewers:
+ fsamuel@chromium.org

[sky, 2016-05-26 22:38:29]

LGTM

[erikchen, 2016-05-26 23:28:11]

erikchen@chromium.org changed reviewers:
+ kbr@chromium.org
- junov@chromium.org

[erikchen, 2016-05-26 23:28:12]

Not sure how kbr@ got lost in my first round of reviewers.

kbr: Please review third_party/WebKit/Source/platform/graphics/*

[erikchen, 2016-05-26 23:38:34]

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

[commit-bot: I haz the power, 2016-05-26 23:39:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974163003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974163003/220001

[commit-bot: I haz the power, 2016-05-27 00:27:26]

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

[commit-bot: I haz the power, 2016-05-27 00:27:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-05-27 00:46:16]

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

[commit-bot: I haz the power, 2016-05-27 00:46:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1974163003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1974163003/240001

[Ken Russell (switch to Gerrit), 2016-05-27 01:02:10]

lgtm given others have reviewed this; couple of questions about the behavior.

https://codereview.chromium.org/1974163003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/1974163003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:977:
m_gl->GetImageivCHROMIUM(imageId, GC3D_GPU_MEMORY_BUFFER_ID,
&gpuMemoryBufferId);
Questions:

1. Does glGetImageivCHROMIUM require a round-trip to the GPU process, or is it
handled entirely client-side?
2. It looks like when the compositor calls DrawingBuffer::mailboxReleased, the
MailboxInfo contains the previously-queried GPU_MEMORY_BUFFER_ID, so it doesn't
need to be queried again unless the canvas has changed size. Correct? So in the
steady state we are not making this call every frame?

[erikchen, 2016-05-27 01:13:37]

https://codereview.chromium.org/1974163003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/1974163003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:977:
m_gl->GetImageivCHROMIUM(imageId, GC3D_GPU_MEMORY_BUFFER_ID,
&gpuMemoryBufferId);
On 2016/05/27 01:02:10, Ken Russell wrote:
> Questions:
> 
> 1. Does glGetImageivCHROMIUM require a round-trip to the GPU process, or is it
> handled entirely client-side?
The GMB id is cached client side.

> 2. It looks like when the compositor calls DrawingBuffer::mailboxReleased, the
> MailboxInfo contains the previously-queried GPU_MEMORY_BUFFER_ID, so it
doesn't
> need to be queried again unless the canvas has changed size. Correct? So in
the
> steady state we are not making this call every frame?
That's correct. The only time we make this call is immediately after
CreateGpuMemoryBufferImageCHROMIUM.

[commit-bot: I haz the power, 2016-05-27 02:56:14]

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

[commit-bot: I haz the power, 2016-05-27 02:56:16]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-05-27 17:57:44]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-05-27 17:57:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, piman@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1974163003/#ps240001
(title: "Fix test.")

[commit-bot: I haz the power, 2016-05-27 17:58:10]

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

[commit-bot: I haz the power, 2016-05-27 18:04:35]

Description was changed from

==========
Expose GpuMemoryBufferId through glGetImageivCHROMIUM.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM Image extension to allow the GpuMemoryBuffer id to be
obtained by a new function glGetImageivCHROMIUM.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Expose GpuMemoryBufferId through glGetImageivCHROMIUM.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM Image extension to allow the GpuMemoryBuffer id to be
obtained by a new function glGetImageivCHROMIUM.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-27 18:04:40]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-05-27 18:06:21]

Description was changed from

==========
Expose GpuMemoryBufferId through glGetImageivCHROMIUM.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM Image extension to allow the GpuMemoryBuffer id to be
obtained by a new function glGetImageivCHROMIUM.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Expose GpuMemoryBufferId through glGetImageivCHROMIUM.

WebKit widgets that provide textures to TextureLayer need to also provide the
GpuMemoryBufferId if the texture is backed by a GpuMemoryBuffer. This CL
modifies the CHROMIUM Image extension to allow the GpuMemoryBuffer id to be
obtained by a new function glGetImageivCHROMIUM.

BUG=608026
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/11c5d208606dbce8660ad7f5e41e0aa770655c82
Cr-Commit-Position: refs/heads/master@{#396510}
==========

[commit-bot: I haz the power, 2016-05-27 18:06:24]

Patchset 13 (id:??) landed as
https://crrev.com/11c5d208606dbce8660ad7f5e41e0aa770655c82
Cr-Commit-Position: refs/heads/master@{#396510}