Change glimage to accept a type.

Rearrange GLImage API to create them with a usage type
instead of using usage type while mapping the buffer.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268548

[alexst (slow to review), 2014-04-29 19:19:54]

David, please have a look at this.

The CL is surprisingly big, but most of it is just plumbing.

Ozone parts are subject to change based on where the buffer is actually
allocated, but this is to give you an idea what I see it looking like.

[reveman, 2014-04-29 20:04:43]

https://codereview.chromium.org/255713008/diff/60001/ui/gfx/gpu_memory_buffer.h
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/255713008/diff/60001/ui/gfx/gpu_memory_buffer...
ui/gfx/gpu_memory_buffer.h:88: virtual void* Map() = 0;
Can we start be making this change first and all the ozone stuff in a follow up?

[alexst (slow to review), 2014-04-29 20:29:48]

On 2014/04/29 20:04:43, reveman wrote:
>
https://codereview.chromium.org/255713008/diff/60001/ui/gfx/gpu_memory_buffer.h
> File ui/gfx/gpu_memory_buffer.h (right):
> 
>
https://codereview.chromium.org/255713008/diff/60001/ui/gfx/gpu_memory_buffer...
> ui/gfx/gpu_memory_buffer.h:88: virtual void* Map() = 0;
> Can we start be making this change first and all the ozone stuff in a follow
up?


Sure, new patch is up without ozone.

[reveman, 2014-04-30 11:31:27]

https://codereview.chromium.org/255713008/diff/80001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl.h (right):

https://codereview.chromium.org/255713008/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:39:
gfx::GpuMemoryBuffer::Usage usage_;
Doesn't seem to be used by this class. I don't think it should be here unless
it's actually used by this base class.

https://codereview.chromium.org/255713008/diff/80001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_android.cc (right):

https://codereview.chromium.org/255713008/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_android.cc:17: if (usage ==
gfx::GpuMemoryBuffer::SCANOUT)
I don't think this should be here. If the browser created some type of handle
for some usage you can assume that it knows what it's doing here. If some
specific implementation doesn't support Map or whatever we can just NOTREACHED
there.

Same comment in other platform specific files.

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt (right):

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:3: CHROMIUM_map_image
"map" is awkward naming now that that is not necessarily supported. I think we
can just rename this to CHROMIUM_image. Alternatively we can move the
create/destroy parts of this extension into a new CHROMIUM_image_base extension
and cpu read/write, scanout can be provided using layered extensions. Not sure
that's worth it though. Benefit would be that it matches what we probably need
on the EGL side.

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:21: support by providing
avility to create buffers capable of being scanned out
the ability

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:39: None
Can you add the enums to this section? including IMAGE_ROWBYTES_CHROMIUM

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:43: GLuint
CreateImageCHROMIUM (GLsizei width, GLsizei height,
while here, can you remove space between "CHROMIUM" and "("?

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:54: INVALID_ENUM is
generated if <usage> is not one of READ_WRITE and
CPU_READ_WRITE?

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:57: void
DestroyImageCHROMIUM (GLuint image_id)
please remove the space here too

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:63: void* MapImageCHROMIUM
(GLuint image_id)
remove space

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:66: the image. Returns null
is the image was not created with READ_WRITE usage.
s/is/if/

hm, I prefer if we just make it illegal to call this on an image that was not
created for this type of usage so we can crash in debug builds if this happens.

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:73: void UnmapImageCHROMIUM
(GLuint image_id)
and the space here, thanks :)

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/gl2extchromium.h
File gpu/GLES2/gl2extchromium.h (right):

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/gl2extchromium...
gpu/GLES2/gl2extchromium.h:126: GLsizei heiBht,
height

https://codereview.chromium.org/255713008/diff/80001/ui/gfx/gpu_memory_buffer.h
File ui/gfx/gpu_memory_buffer.h (right):

https://codereview.chromium.org/255713008/diff/80001/ui/gfx/gpu_memory_buffer...
ui/gfx/gpu_memory_buffer.h:68: enum Usage { READ_WRITE, SCANOUT, USAGE_LAST =
SCANOUT };
I don't think this needs to be part of this interface anymore. We can just use
the GLenums where needed just like we currently use internalformat GLenums.

https://codereview.chromium.org/255713008/diff/80001/ui/gfx/gpu_memory_buffer...
ui/gfx/gpu_memory_buffer.h:74: // accessing the buffer or if CPU caches need to
be synchronized. |mode|
Please update this comment.

[alexst (slow to review), 2014-04-30 23:41:45]

Please have another look.

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt (right):

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:54: INVALID_ENUM is
generated if <usage> is not one of READ_WRITE and
On 2014/04/30 11:31:28, reveman wrote:
> CPU_READ_WRITE?

GL_IMAGE_CPU_READ_WRITE_CHROMIUM to match IMAGE_ROWBYTES_CHROMIUM?

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:66: the image. Returns null
is the image was not created with READ_WRITE usage.
On 2014/04/30 11:31:28, reveman wrote:
> s/is/if/
> 
> hm, I prefer if we just make it illegal to call this on an image that was not
> created for this type of usage so we can crash in debug builds if this
happens. 

Done.

[reveman, 2014-05-01 12:06:37]

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt (right):

https://codereview.chromium.org/255713008/diff/80001/gpu/GLES2/extensions/CHR...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_map_image.txt:54: INVALID_ENUM is
generated if <usage> is not one of READ_WRITE and
On 2014/04/30 23:41:46, alexst wrote:
> On 2014/04/30 11:31:28, reveman wrote:
> > CPU_READ_WRITE?
> 
> GL_IMAGE_CPU_READ_WRITE_CHROMIUM to match IMAGE_ROWBYTES_CHROMIUM?

Yes, maybe even IMAGE_MAP_READ_WRITE or simply IMAGE_MAP is better? Would make
it a bit more clear that this affects whether Map/UnmapImage functions can be
used with the image. You can also make this a bit field if you like
"IMAGE_MAP_BIT_CHROMIUM | IMAGE_SCANOUT_BIT_CHROMIUM". I don't feel that
strongly about it though. Latest patch is fine with me in this regard if that's
what you prefer.

https://codereview.chromium.org/255713008/diff/140001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt (right):

https://codereview.chromium.org/255713008/diff/140001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt:41: GL_IMAGE_ROWBYTES_CHROMIUM
0x78F0
nit: indent 4 spaces and remove GL_ prefix for consistency (not used when
referring to INVALID_VALUE below)

https://codereview.chromium.org/255713008/diff/140001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt:46: GL_IMAGE_SCANOUT_CHROMIUM
0x78F2
nit: indent 4 spaces and remove GL_ prefix

https://codereview.chromium.org/255713008/diff/140001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt:48: #ifndef
GL_IMAGE_SCANOUT_CHROMIUM
typo?

https://codereview.chromium.org/255713008/diff/140001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_control_service.h (right):

https://codereview.chromium.org/255713008/diff/140001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_control_service.h:59: unsigned usage);
Does the GPU service really need to know about usage? We can't rely on this to
determine the actual usage that is allowed for a buffer as a malicious renderer
can easily manufacture this.

https://codereview.chromium.org/255713008/diff/140001/webkit/common/gpu/webgr...
File webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h
(right):

https://codereview.chromium.org/255713008/diff/140001/webkit/common/gpu/webgr...
webkit/common/gpu/webgraphicscontext3d_in_process_command_buffer_impl.h:480: //
TODO: remove this old function after blink cleanup.
nit: TODO(user)

[alexst (slow to review), 2014-05-01 13:44:51]

https://codereview.chromium.org/255713008/diff/140001/gpu/GLES2/extensions/CH...
File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt (right):

https://codereview.chromium.org/255713008/diff/140001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt:41: GL_IMAGE_ROWBYTES_CHROMIUM
0x78F0
On 2014/05/01 12:06:38, reveman wrote:
> nit: indent 4 spaces and remove GL_ prefix for consistency (not used when
> referring to INVALID_VALUE below)

Done.

https://codereview.chromium.org/255713008/diff/140001/gpu/GLES2/extensions/CH...
gpu/GLES2/extensions/CHROMIUM/CHROMIUM_image.txt:48: #ifndef
GL_IMAGE_SCANOUT_CHROMIUM
On 2014/05/01 12:06:38, reveman wrote:
> typo?

right, copy paste...

https://codereview.chromium.org/255713008/diff/140001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_control_service.h (right):

https://codereview.chromium.org/255713008/diff/140001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_control_service.h:59: unsigned usage);
On 2014/05/01 12:06:38, reveman wrote:
> Does the GPU service really need to know about usage? We can't rely on this to
> determine the actual usage that is allowed for a buffer as a malicious
renderer
> can easily manufacture this.

I suppose it's not strictly necessary,  I was going to use this to reject
buffers not meant for scanout but set as overlay early to produce a better error
message in command buffer. If the usage is malicious here, it'll get rejected
eventually anyway in the lower level ozone bits because they know what the
actual buffer is like. I can remove this if you prefer.

[reveman, 2014-05-01 13:48:45]

https://codereview.chromium.org/255713008/diff/140001/gpu/command_buffer/serv...
File gpu/command_buffer/service/gpu_control_service.h (right):

https://codereview.chromium.org/255713008/diff/140001/gpu/command_buffer/serv...
gpu/command_buffer/service/gpu_control_service.h:59: unsigned usage);
On 2014/05/01 13:44:51, alexst wrote:
> On 2014/05/01 12:06:38, reveman wrote:
> > Does the GPU service really need to know about usage? We can't rely on this
to
> > determine the actual usage that is allowed for a buffer as a malicious
> renderer
> > can easily manufacture this.
> 
> I suppose it's not strictly necessary,  I was going to use this to reject
> buffers not meant for scanout but set as overlay early to produce a better
error
> message in command buffer. If the usage is malicious here, it'll get rejected
> eventually anyway in the lower level ozone bits because they know what the
> actual buffer is like. I can remove this if you prefer.

Let's avoid it for now and we can add it later if it turns out to be useful.

[alexst (slow to review), 2014-05-01 14:45:29]

ptal

[reveman, 2014-05-01 15:15:54]

You'll need to update android_webview/browser/gpu_memory_buffer_factory_impl.cc
too.

https://codereview.chromium.org/255713008/diff/180001/content/browser/gpu/bro...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/255713008/diff/180001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:383: if
(!GpuMemoryBufferImpl::IsFormatValid(internalformat))
Please add a IsUsageValid check here too.

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

https://codereview.chromium.org/255713008/diff/180001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:33: case
GL_IMAGE_MAP_CHROMIUM:
Please include SCANOUT here.

Each specific implementation should have a static IsUsageSupported function that
prevents allocation of buffers with unsupported usage mode.

[alexst (slow to review), 2014-05-01 17:45:06]

New patch.

https://codereview.chromium.org/255713008/diff/210001/android_webview/browser...
File android_webview/browser/gpu_memory_buffer_factory_impl.cc (right):

https://codereview.chromium.org/255713008/diff/210001/android_webview/browser...
android_webview/browser/gpu_memory_buffer_factory_impl.cc:36: int err =
g_gl_draw_functions->map(buffer_id_, MAP_READ_WRITE, &vaddr);
This seems to be mapped into the function ptr table inside Android, isn't it?

[reveman, 2014-05-01 18:07:32]

FYI, all platform specific code in
RenderMessageFilter::OnAllocateGpuMemoryBuffer is ugly and probably a bit
confusing. I'm planning to move that into the GpuMemoryBufferImpl interface
asap. Here's a wip patch for that: https://codereview.chromium.org/263553009/

https://codereview.chromium.org/255713008/diff/210001/android_webview/browser...
File android_webview/browser/gpu_memory_buffer_factory_impl.cc (right):

https://codereview.chromium.org/255713008/diff/210001/android_webview/browser...
android_webview/browser/gpu_memory_buffer_factory_impl.cc:36: int err =
g_gl_draw_functions->map(buffer_id_, MAP_READ_WRITE, &vaddr);
On 2014/05/01 17:45:07, alexst wrote:
> This seems to be mapped into the function ptr table inside Android, isn't it?

Yes. FYI, we're planning to move this GpuMemoryBuffer implementation to 	
content/common/gpu/client where all other implementations live.

https://codereview.chromium.org/255713008/diff/210001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl.h (right):

https://codereview.chromium.org/255713008/diff/210001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.h:26: static bool
IsUsageSupportedForSharedMemory(unsigned usage);
Please move this to GpuMemoryBufferImplShm::IsUsageSupported instead.

[alexst (slow to review), 2014-05-01 21:03:20]

On 2014/05/01 18:07:32, reveman wrote:
> FYI, all platform specific code in
> RenderMessageFilter::OnAllocateGpuMemoryBuffer is ugly and probably a bit
> confusing. I'm planning to move that into the GpuMemoryBufferImpl interface
> asap. Here's a wip patch for that: https://codereview.chromium.org/263553009/

Yeah, that patch makes a lot of sense.
 
> Yes. FYI, we're planning to move this GpuMemoryBuffer implementation to 	
> content/common/gpu/client where all other implementations live.

We'll should clean up the function signature when it does.

>
https://codereview.chromium.org/255713008/diff/210001/content/common/gpu/clie...
> content/common/gpu/client/gpu_memory_buffer_impl.h:26: static bool
> IsUsageSupportedForSharedMemory(unsigned usage);
> Please move this to GpuMemoryBufferImplShm::IsUsageSupported instead.

Done.

[reveman, 2014-05-01 21:19:04]

lgtm

[alexst (slow to review), 2014-05-01 22:10:27]

+kenrb for IPC security review
+danakj for ui/gfx owner review
+brettw for pepper owner review
+ben for mojo owner review

[danakj, 2014-05-01 22:12:24]

On 2014/05/01 22:10:27, alexst wrote:
> +danakj for ui/gfx owner review

Looks like reveman is already an owner of that file. :)

[kenrb, 2014-05-02 15:44:29]

lgtm for IPC change

[piman, 2014-05-02 20:21:18]

https://codereview.chromium.org/255713008/diff/220001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_c_lib_autogen.h (right):

https://codereview.chromium.org/255713008/diff/220001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_c_lib_autogen.h:802: GLenum usage) {
These functions are autogenerated by
gpu/command_buffer/build_gles2_cmd_buffer.py, but I don't see changes to that
file.
(while some pick up argument lists from the header file, CreateImageCHROMIUM has
an explicit list of arguments, so I'm surprised we don't need to change it).

[alexst (slow to review), 2014-05-02 22:06:17]

https://codereview.chromium.org/255713008/diff/220001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_c_lib_autogen.h (right):

https://codereview.chromium.org/255713008/diff/220001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_c_lib_autogen.h:802: GLenum usage) {
On 2014/05/02 20:21:18, piman wrote:
> These functions are autogenerated by
> gpu/command_buffer/build_gles2_cmd_buffer.py, but I don't see changes to that
> file.
> (while some pick up argument lists from the header file, CreateImageCHROMIUM
has
> an explicit list of arguments, so I'm surprised we don't need to change it).

Interesting, I expected the definitions to come from cmd_buffer_functions.txt
and headers. Anyway, I added the arg to the .py file, thank you!

[piman, 2014-05-02 22:35:33]

lgtm

[Ben Goodger (Google), 2014-05-05 15:12:57]

lgtm

[brettw, 2014-05-05 16:34:54]

lgtm

[alexst (slow to review), 2014-05-05 17:23:25]

The CQ bit was checked by alexst@chromium.org

[commit-bot: I haz the power, 2014-05-05 17:23:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alexst@chromium.org/255713008/230001

[commit-bot: I haz the power, 2014-05-05 22:20:30]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium
  win_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-06 00:57:17]

FYI, CQ is re-trying this CL (attempt #2).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  mac_chromium_rel on tryserver.chromium

[commit-bot: I haz the power, 2014-05-06 03:04:16]

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

[commit-bot: I haz the power, 2014-05-06 03:04:16]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium

[alexst (slow to review), 2014-05-06 12:44:04]

The CQ bit was checked by alexst@chromium.org

[commit-bot: I haz the power, 2014-05-06 12:44:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alexst@chromium.org/255713008/250001

[commit-bot: I haz the power, 2014-05-06 17:16:57]

Change committed as 268548