gpu: Refactor GpuMemoryBuffer framework for multi-process support.

gpu: Refactor GpuMemoryBuffer framework for multi-process support.
    
This removes the ImageFactory interface and adjusts the buffer
allocation system for future multi-process support.

Also includes proper plumbing of internalformat to GLImage implementation
and makes sure the compositor is using the correct format.
    
TEST=gpu_unittests --gtest_filter=MockGpuMemoryBufferTest.Lifecycle
BUG=261649
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218034

[reveman, 2013-07-24 00:45:24]

For more context see: https://codereview.chromium.org/19762004/

This is the refactoring part of that patch without any of the multi-process
support, which I'm planning to hold off on until we have a fully working
implementation.

sievers, this is still limiting the in-process context to one image manager per
context. It think it might be better to register the gpu memory buffer with
multiple contexts instead of sharing the image manager. Either way, I prefer to
avoid that until we have a need for it. Normal out-of-process contexts supports
sharing of images between contexts just fine. Wdyt?

[kaanb, 2013-07-24 03:31:23]

Generally looks okay, if you use tools/git/move_source_file.py it would be
easier to review.

https://codereview.chromium.org/20017005/diff/1/android_webview/browser/gpu_m...
File android_webview/browser/gpu_memory_buffer_factory_impl.cc (right):

https://codereview.chromium.org/20017005/diff/1/android_webview/browser/gpu_m...
android_webview/browser/gpu_memory_buffer_factory_impl.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
I think joth@ already pointed this out. I think if you use
tools/git/move_source_file.py to move files around, this patch will be easier to
review.

https://codereview.chromium.org/20017005/diff/1/gpu/command_buffer/service/co...
File gpu/command_buffer/service/command_buffer_service.h (right):

https://codereview.chromium.org/20017005/diff/1/gpu/command_buffer/service/co...
gpu/command_buffer/service/command_buffer_service.h:85: // Register an existing
gpu memory buffer and get an ID that can be used
nit: s/Registers/Register/

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

https://codereview.chromium.org/20017005/diff/1/ui/gfx/gpu_memory_buffer.h#ne...
ui/gfx/gpu_memory_buffer.h:13: #include <third_party/khronos/EGL/egl.h>
I had added a define for EGL_NATIVE_BUFFER_ANDROID in gl_bindings.h, it may be
possible to remove it now that you're adding this include, can you check?

[reveman, 2013-07-24 05:47:12]

https://codereview.chromium.org/20017005/diff/1/android_webview/browser/gpu_m...
File android_webview/browser/gpu_memory_buffer_factory_impl.cc (right):

https://codereview.chromium.org/20017005/diff/1/android_webview/browser/gpu_m...
android_webview/browser/gpu_memory_buffer_factory_impl.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
On 2013/07/24 03:31:23, kaanb wrote:
> I think joth@ already pointed this out. I think if you use
> tools/git/move_source_file.py to move files around, this patch will be easier
to
> review.

I used "git mv" but the changes are too great for it to be recognized as a move.
The big difference is that the GpuMemoryBufferImpl is now private and lives in
an anonymous namespace. The implementation hasn't change much except for the
addition of GetHandle, which replaces GetNativeBuffer and some cleanup. Please
verify that things look good though.

https://codereview.chromium.org/20017005/diff/1/gpu/command_buffer/service/co...
File gpu/command_buffer/service/command_buffer_service.h (right):

https://codereview.chromium.org/20017005/diff/1/gpu/command_buffer/service/co...
gpu/command_buffer/service/command_buffer_service.h:85: // Register an existing
gpu memory buffer and get an ID that can be used
On 2013/07/24 03:31:23, kaanb wrote:
> nit: s/Registers/Register/

It describes what the function is used for and not what it does. "Registers"
would required that second part of the sentence to be rewritten. I think it's
more important to be consistent with RegisterTransferBuffer above so I'll keep
this as is.

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

https://codereview.chromium.org/20017005/diff/1/ui/gfx/gpu_memory_buffer.h#ne...
ui/gfx/gpu_memory_buffer.h:13: #include <third_party/khronos/EGL/egl.h>
On 2013/07/24 03:31:23, kaanb wrote:
> I had added a define for EGL_NATIVE_BUFFER_ANDROID in gl_bindings.h, it may be
> possible to remove it now that you're adding this include, can you check?

Oh, would it be more appropriate to include gl_bindings.h instead? Why was it
added to that file instead of including third_party/khronos/EGL/egl.h?

[reveman, 2013-07-25 19:50:25]

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

https://codereview.chromium.org/20017005/diff/1/ui/gfx/gpu_memory_buffer.h#ne...
ui/gfx/gpu_memory_buffer.h:13: #include <third_party/khronos/EGL/egl.h>
On 2013/07/24 03:31:23, kaanb wrote:
> I had added a define for EGL_NATIVE_BUFFER_ANDROID in gl_bindings.h, it may be
> possible to remove it now that you're adding this include, can you check?

Removed your EGL_NATIVE_BUFFER_ANDROID define from latest patch.

[reveman, 2013-07-25 21:43:57]

I'll try to reduce some of the code duplication between shared memory and gpu
memory buffers before landing this but want to make sure we agree on the general
approach before I go ahead with that.

[kaanb, 2013-07-25 21:50:32]

On 2013/07/25 21:43:57, David Reveman wrote:
> I'll try to reduce some of the code duplication between shared memory and gpu
> memory buffers before landing this but want to make sure we agree on the
general
> approach before I go ahead with that.

I'll take a look at this patch later today, but would you consider breaking it
to smaller patches so it's easier to review?

[reveman, 2013-07-25 21:57:08]

On 2013/07/25 21:50:32, kaanb wrote:
> On 2013/07/25 21:43:57, David Reveman wrote:
> > I'll try to reduce some of the code duplication between shared memory and
gpu
> > memory buffers before landing this but want to make sure we agree on the
> general
> > approach before I go ahead with that.
> 
> I'll take a look at this patch later today, but would you consider breaking it
> to smaller patches so it's easier to review?

Sure, I'll try to move most of the android_webview changes to a separate patch.
And maybe the gpu memory buffer type can be added in a separate patch too,
leaving only the CommandBuffer API change to this patch.

[kaanb, 2013-07-25 21:57:57]

On 2013/07/25 21:57:08, David Reveman wrote:
> On 2013/07/25 21:50:32, kaanb wrote:
> > On 2013/07/25 21:43:57, David Reveman wrote:
> > > I'll try to reduce some of the code duplication between shared memory and
> gpu
> > > memory buffers before landing this but want to make sure we agree on the
> > general
> > > approach before I go ahead with that.
> > 
> > I'll take a look at this patch later today, but would you consider breaking
it
> > to smaller patches so it's easier to review?
> 
> Sure, I'll try to move most of the android_webview changes to a separate
patch.
> And maybe the gpu memory buffer type can be added in a separate patch too,
> leaving only the CommandBuffer API change to this patch.

Sounds like a good plan.

[reveman, 2013-07-31 20:18:21]

This is now just a simple refactor that removes the ImageManager interface and
instead uses the CommandBuffer interface to register GLImages, which is what we
want for multi-process support. It doesn't include any content/ changes or new
ipc. That will be done in follow-up patches.

PTAL.

[kaanb, 2013-07-31 23:44:05]

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common...
gpu/command_buffer/common/command_buffer.h:149: // Create a gpu memory buffer of
the given dimensions and format. Returns
optional: I prefer using third-person in comments, i.e. s/Create/Creates/ which
is the Java standard. You start with "Create" in the first sentence and
"Returns" in the second. It would be good to at least be consistent
Create/Return or Creates/Returns.

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
File gpu/command_buffer/service/command_buffer_service.cc (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/command_buffer_service.cc:230: if (!buffer)
buffer.get()

https://codereview.chromium.org/20017005/diff/22001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/20017005/diff/22001/ui/gl/gl_image.h#newcode42
ui/gl/gl_image.h:42: const gfx::Size& size,
wasn't there a consensus on passing structs with sizeof() less than 16 bytes by
value?

[no sievers, 2013-08-01 00:38:04]

Yay, you are addressing my TODO ("ID allocation should go through
CommandBuffer") in
https://codereview.chromium.org/19522006/diff/75001/gpu/command_buffer/servic...
:)

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
File gpu/command_buffer/service/command_buffer_service.cc (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/command_buffer_service.cc:234: *id = next_id++;
Does it make sense to establish this 1:1 relationship between GLImage and
GpuMemoryBuffer, and basically saying this is the only way a GL client can use
GL images?

Should we then maybe rather have a GLImageClient interface with GpuMemoryBuffers
being an impl of that (one that allows you to map/unmap a GLImage into the cpu
address space)?

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_scheduler.h (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_scheduler.h:91: virtual gfx::GLImage*
GetImage(int32 image_id) OVERRIDE;
I find plumbing the ImageManager through the Scheduler a little odd.
Maybe the decoder should just keep its own pointer?

Would also make some changes to the unittests and other files unnecessary here.

[reveman, 2013-08-01 13:32:17]

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common...
gpu/command_buffer/common/command_buffer.h:149: // Create a gpu memory buffer of
the given dimensions and format. Returns
On 2013/07/31 23:44:05, kaanb wrote:
> optional: I prefer using third-person in comments, i.e. s/Create/Creates/
which
> is the Java standard. You start with "Create" in the first sentence and
> "Returns" in the second. It would be good to at least be consistent
> Create/Return or Creates/Returns.

I prefer to keep it consistent with CreateTransferBuffer for now. We can change
all the comments in this file as a follow-up if we think that's important.

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
File gpu/command_buffer/service/command_buffer_service.cc (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/command_buffer_service.cc:230: if (!buffer)
On 2013/07/31 23:44:05, kaanb wrote:
> buffer.get()

scoped_ptr implements operator Testable() and can be used in boolean
expressions.

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/command_buffer_service.cc:234: *id = next_id++;
On 2013/08/01 00:38:04, sievers wrote:
> Does it make sense to establish this 1:1 relationship between GLImage and
> GpuMemoryBuffer, and basically saying this is the only way a GL client can use
> GL images?

I don't necessarily think we are. We can add another set of Create/Register
functions for a different type of GLImage.

> 
> Should we then maybe rather have a GLImageClient interface with
GpuMemoryBuffers
> being an impl of that (one that allows you to map/unmap a GLImage into the cpu
> address space)?

This is what the GpuMemoryBuffers interface is at the moment. We could rename it
and even make it more generic in the future if that makes sense. I don't think
we need to do that yet.

If/when we add additional GLImage types that can be created from the renderer,
we should evaluate if they fit into the existing GpuMemoryBuffer category or
should use something new. I think it depends on a number of factors, such as can
the GLImage storage be mapped and how is it serialized.

There should always be 1:1 mapping between client interface and GLImage
instantiate function on the service side. ie.
GLImage::CreateGLImageFromGpuMemoryBuffer and GpuMemoryBuffer.

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_scheduler.h (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_scheduler.h:91: virtual gfx::GLImage*
GetImage(int32 image_id) OVERRIDE;
On 2013/08/01 00:38:04, sievers wrote:
> I find plumbing the ImageManager through the Scheduler a little odd.
> Maybe the decoder should just keep its own pointer?

I'd rather not add new state unless necessary. Current patch is consistent with
how transfer buffers and shared memory is handled.

> 
> Would also make some changes to the unittests and other files unnecessary
here.

https://codereview.chromium.org/20017005/diff/22001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/20017005/diff/22001/ui/gl/gl_image.h#newcode42
ui/gl/gl_image.h:42: const gfx::Size& size,
On 2013/07/31 23:44:05, kaanb wrote:
> wasn't there a consensus on passing structs with sizeof() less than 16 bytes
by
> value?

maybe, I'll change this in that case. not clear to me why we can't just leave
that decision to the compiler though.

[kaanb, 2013-08-01 17:00:48]

On 2013/08/01 13:32:17, David Reveman wrote:
>
https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common...
> File gpu/command_buffer/common/command_buffer.h (right):
> 
>
https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common...
> gpu/command_buffer/common/command_buffer.h:149: // Create a gpu memory buffer
of
> the given dimensions and format. Returns
> On 2013/07/31 23:44:05, kaanb wrote:
> > optional: I prefer using third-person in comments, i.e. s/Create/Creates/
> which
> > is the Java standard. You start with "Create" in the first sentence and
> > "Returns" in the second. It would be good to at least be consistent
> > Create/Return or Creates/Returns.
> 
> I prefer to keep it consistent with CreateTransferBuffer for now. We can
change
> all the comments in this file as a follow-up if we think that's important.
> 
>
https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
> File gpu/command_buffer/service/command_buffer_service.cc (right):
> 
>
https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
> gpu/command_buffer/service/command_buffer_service.cc:230: if (!buffer)
> On 2013/07/31 23:44:05, kaanb wrote:
> > buffer.get()
> 
> scoped_ptr implements operator Testable() and can be used in boolean
> expressions.
> 
>
https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
> gpu/command_buffer/service/command_buffer_service.cc:234: *id = next_id++;
> On 2013/08/01 00:38:04, sievers wrote:
> > Does it make sense to establish this 1:1 relationship between GLImage and
> > GpuMemoryBuffer, and basically saying this is the only way a GL client can
use
> > GL images?
> 
> I don't necessarily think we are. We can add another set of Create/Register
> functions for a different type of GLImage.
> 
> > 
> > Should we then maybe rather have a GLImageClient interface with
> GpuMemoryBuffers
> > being an impl of that (one that allows you to map/unmap a GLImage into the
cpu
> > address space)?
> 
> This is what the GpuMemoryBuffers interface is at the moment. We could rename
it
> and even make it more generic in the future if that makes sense. I don't think
> we need to do that yet.
> 
> If/when we add additional GLImage types that can be created from the renderer,
> we should evaluate if they fit into the existing GpuMemoryBuffer category or
> should use something new. I think it depends on a number of factors, such as
can
> the GLImage storage be mapped and how is it serialized.
> 
> There should always be 1:1 mapping between client interface and GLImage
> instantiate function on the service side. ie.
> GLImage::CreateGLImageFromGpuMemoryBuffer and GpuMemoryBuffer.
> 
>
https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
> File gpu/command_buffer/service/gpu_scheduler.h (right):
> 
>
https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
> gpu/command_buffer/service/gpu_scheduler.h:91: virtual gfx::GLImage*
> GetImage(int32 image_id) OVERRIDE;
> On 2013/08/01 00:38:04, sievers wrote:
> > I find plumbing the ImageManager through the Scheduler a little odd.
> > Maybe the decoder should just keep its own pointer?
> 
> I'd rather not add new state unless necessary. Current patch is consistent
with
> how transfer buffers and shared memory is handled.
> 
> > 
> > Would also make some changes to the unittests and other files unnecessary
> here.
> 
> https://codereview.chromium.org/20017005/diff/22001/ui/gl/gl_image.h
> File ui/gl/gl_image.h (right):
> 
> https://codereview.chromium.org/20017005/diff/22001/ui/gl/gl_image.h#newcode42
> ui/gl/gl_image.h:42: const gfx::Size& size,
> On 2013/07/31 23:44:05, kaanb wrote:
> > wasn't there a consensus on passing structs with sizeof() less than 16 bytes
> by
> > value?
> 
> maybe, I'll change this in that case. not clear to me why we can't just leave
> that decision to the compiler though.

lgtm

[piman, 2013-08-01 21:19:30]

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common...
gpu/command_buffer/common/command_buffer.h:151: virtual gfx::GpuMemoryBuffer*
CreateGpuMemoryBuffer(
I would prefer if we didn't load up CommandBuffer with GL-y concepts and keep it
to the command processing level.

We have been talking about adding a separate interface that GLES2Implementation
(and higher level classes) can use to deal with things that don't go through
command buffers (e.g. IPCs, or accessing singletons when in-process). Maybe now
is the time to do it?

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
File gpu/command_buffer/service/cmd_buffer_engine.h (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/cmd_buffer_engine.h:46: virtual gfx::GLImage*
GetImage(int32 image_id) = 0;
Same comment here - I think this is exposed at a too low level.

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
File gpu/command_buffer/service/command_buffer_service.cc (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/command_buffer_service.cc:41:
CommandBufferService::~CommandBufferService() {
I think if the client isn't well behaved (or crashes), you'll have leftover
buffers in gpu_memory_buffers_ that need deletion here.

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/command_buffer_service.cc:226:
scoped_ptr<gfx::GpuMemoryBuffer> buffer = make_scoped_ptr(
nit: you could use linked_ptr and put that linked_ptr in the map too so that you
could avoid potential leaks (see above).

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/command_buffer_service.cc:233: static int32 next_id =
1;
Does this ID need to be shared between contexts?

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/command_buffer_service.cc:286:
image_manager_->AddImage(gl_image.get(), id);
Mmh, this is the id generated locally for the GpuMemoryBuffer. How does that
interact with the GpuMsg_CreateImage path, where ids get generated client-side?
It seems like a high potential for conflicts.

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
File gpu/command_buffer/service/common_decoder.h (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/common_decoder.h:142: gfx::GLImage* GetImage(int32
image_id);
nit: I guess we try to keep CommonDecoder GL-agnostic. Can we move this to
GLES2DecoderImpl?

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_scheduler.h (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_scheduler.h:91: virtual gfx::GLImage*
GetImage(int32 image_id) OVERRIDE;
On 2013/08/01 13:32:18, David Reveman wrote:
> On 2013/08/01 00:38:04, sievers wrote:
> > I find plumbing the ImageManager through the Scheduler a little odd.
> > Maybe the decoder should just keep its own pointer?
> 
> I'd rather not add new state unless necessary. Current patch is consistent
with
> how transfer buffers and shared memory is handled.

Transfer buffers and shared memory are an essential part of command buffers.
This is GL-specific and I think it should be kept out of the command processing
layer.

> 
> > 
> > Would also make some changes to the unittests and other files unnecessary
> here.
>

https://codereview.chromium.org/20017005/diff/22001/ui/gl/gl_image_shm.cc
File ui/gl/gl_image_shm.cc (right):

https://codereview.chromium.org/20017005/diff/22001/ui/gl/gl_image_shm.cc#new...
ui/gl/gl_image_shm.cc:60: NOTREACHED();
nit: this comes from the (untrusted) client side. While we don't expose this
cross-process yet, I'm pretty sure we won't go back and audit everything. So I'm
not sure NOTREACHED() is really what we want (false sense of security etc.),
though we can certainly DVLOG.

[no sievers, 2013-08-01 21:30:03]

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common...
gpu/command_buffer/common/command_buffer.h:151: virtual gfx::GpuMemoryBuffer*
CreateGpuMemoryBuffer(
On 2013/08/01 21:19:30, piman wrote:
> I would prefer if we didn't load up CommandBuffer with GL-y concepts and keep
it
> to the command processing level.
> 
> We have been talking about adding a separate interface that
GLES2Implementation
> (and higher level classes) can use to deal with things that don't go through
> command buffers (e.g. IPCs, or accessing singletons when in-process). Maybe
now
> is the time to do it?

Or can it just go through GpuCommandBufferStub?
We have other GL-y things (InsertSyncPoint, mailbox names) that send IPCs from
CommandBufferProxyImpl but get handled by the stub or channel on the other end.

I think it comes from the fact that CommandBufferProxyImpl implements two
things: it's a CommandBuffer but also the opposite end of a GpuCommandBufferStub
(which *owns* the CommandBufferService but does not derive from it). Not sure we
need to change things just for this?

Also, where I think David is coming from is that he is comparing 0-copy+ashmen
with transfer buffer pixel transfers. But I agree that we otherwise treat
GLImages and GpuMemoryBuffers more as a GL concept. One noticeable difference:
transfer buffer ids are not exposed to the client while the resource IDs for the
images/buffers are.

[piman, 2013-08-01 21:41:59]

On Thu, Aug 1, 2013 at 2:30 PM, <sievers@chromium.org> wrote:

>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/common/command_**buffer.h<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common/command_buffer.h>
> File gpu/command_buffer/common/**command_buffer.h (right):
>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/common/command_**buffer.h#newcode151<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common/command_buffer.h#newcode151>
> gpu/command_buffer/common/**command_buffer.h:151: virtual
> gfx::GpuMemoryBuffer* CreateGpuMemoryBuffer(
> On 2013/08/01 21:19:30, piman wrote:
>
>> I would prefer if we didn't load up CommandBuffer with GL-y concepts
>>
> and keep it
>
>> to the command processing level.
>>
>
>  We have been talking about adding a separate interface that
>>
> GLES2Implementation
>
>> (and higher level classes) can use to deal with things that don't go
>>
> through
>
>> command buffers (e.g. IPCs, or accessing singletons when in-process).
>>
> Maybe now
>
>> is the time to do it?
>>
>
> Or can it just go through GpuCommandBufferStub?
>

Well, for this CL, it doesn't implement the specific functionality for
CommandBufferProxy/GpuCommandBufferStub, but for the in-process one.


> We have other GL-y things (InsertSyncPoint, mailbox names) that send
> IPCs from CommandBufferProxyImpl but get handled by the stub or channel
> on the other end.
>

InsertSyncPoint is arguable not GL-y, since it manages interactions between
command buffers (i.e. it'd make the exact same amount of sense if command
buffers carried DirectX or Skia, or anything really).

GenerateMailboxNames is a good example of a GL-y thing that
GLES2Implementation needs. You'll note that today it's half hacked because
the WGC3DCBI is the one going directly to the CommandBufferProxyImpl (it
can because it knows the concrete type), rather than GLES2Implementation
(which can't). It means that the fast path only works when we use WGC3D,
not when we use GLES2Interface - it is a problem we'll encounter as we try
to rip out WGC3D from cc, for example.


> I think it comes from the fact that CommandBufferProxyImpl implements
> two things: it's a CommandBuffer but also the opposite end of a
> GpuCommandBufferStub (which *owns* the CommandBufferService but does not
> derive from it). Not sure we need to change things just for this?
>

Well I'll argue, we need to change it for GenerateMailboxNames to be
properly wired, as well as for this. There's likely to be more things like
that moving forward.
I'd rather split the API now when it's still small, than later when it grew
arbitrarily and becomes a giant mess to untangle.

Also, where I think David is coming from is that he is comparing
> 0-copy+ashmen with transfer buffer pixel transfers. But I agree that we
> otherwise treat GLImages and GpuMemoryBuffers more as a GL concept. One
> noticeable difference: transfer buffer ids are not exposed to the client
> while the resource IDs for the images/buffers are.
>
>
https://codereview.chromium.**org/20017005/<https://codereview.chromium.org/2...
>

[reveman, 2013-08-08 23:19:00]

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common...
File gpu/command_buffer/common/command_buffer.h (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common...
gpu/command_buffer/common/command_buffer.h:151: virtual gfx::GpuMemoryBuffer*
CreateGpuMemoryBuffer(
On 2013/08/01 21:19:30, piman wrote:
> I would prefer if we didn't load up CommandBuffer with GL-y concepts and keep
it
> to the command processing level.
> 
> We have been talking about adding a separate interface that
GLES2Implementation
> (and higher level classes) can use to deal with things that don't go through
> command buffers (e.g. IPCs, or accessing singletons when in-process). Maybe
now
> is the time to do it?

I've limited the CommandBuffer changes to adding Create/DestroyGpuMemoryBuffer
which I don't think of as GL specific.  But it sounds like we might still want
to add a separate interface for this. Any suggestions for what the name of this
interface should be?

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
File gpu/command_buffer/service/cmd_buffer_engine.h (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/cmd_buffer_engine.h:46: virtual gfx::GLImage*
GetImage(int32 image_id) = 0;
On 2013/08/01 21:19:30, piman wrote:
> Same comment here - I think this is exposed at a too low level.

Removed it.

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
File gpu/command_buffer/service/command_buffer_service.cc (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/command_buffer_service.cc:41:
CommandBufferService::~CommandBufferService() {
On 2013/08/01 21:19:30, piman wrote:
> I think if the client isn't well behaved (or crashes), you'll have leftover
> buffers in gpu_memory_buffers_ that need deletion here.

Done.

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/command_buffer_service.cc:226:
scoped_ptr<gfx::GpuMemoryBuffer> buffer = make_scoped_ptr(
On 2013/08/01 21:19:30, piman wrote:
> nit: you could use linked_ptr and put that linked_ptr in the map too so that
you
> could avoid potential leaks (see above).

Done.

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/command_buffer_service.cc:233: static int32 next_id =
1;
On 2013/08/01 21:19:30, piman wrote:
> Does this ID need to be shared between contexts?

Not before but it does in latest patch. Is this a problem?

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/command_buffer_service.cc:286:
image_manager_->AddImage(gl_image.get(), id);
On 2013/08/01 21:19:30, piman wrote:
> Mmh, this is the id generated locally for the GpuMemoryBuffer. How does that
> interact with the GpuMsg_CreateImage path, where ids get generated
client-side?
> It seems like a high potential for conflicts.

Yes, these will conflict but shouldn't be a problem until we add real
multi-process support and use it together with GpuMsg_CreateImage. This should
be easy to fix by having GpuMsg_CreateImage return an id instead.

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
File gpu/command_buffer/service/common_decoder.h (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/common_decoder.h:142: gfx::GLImage* GetImage(int32
image_id);
On 2013/08/01 21:19:30, piman wrote:
> nit: I guess we try to keep CommonDecoder GL-agnostic. Can we move this to
> GLES2DecoderImpl?

Removed this.

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_scheduler.h (right):

https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_scheduler.h:91: virtual gfx::GLImage*
GetImage(int32 image_id) OVERRIDE;
On 2013/08/01 21:19:30, piman wrote:
> On 2013/08/01 13:32:18, David Reveman wrote:
> > On 2013/08/01 00:38:04, sievers wrote:
> > > I find plumbing the ImageManager through the Scheduler a little odd.
> > > Maybe the decoder should just keep its own pointer?
> > 
> > I'd rather not add new state unless necessary. Current patch is consistent
> with
> > how transfer buffers and shared memory is handled.
> 
> Transfer buffers and shared memory are an essential part of command buffers.
> This is GL-specific and I think it should be kept out of the command
processing
> layer.

Remove this.

> 
> > 
> > > 
> > > Would also make some changes to the unittests and other files unnecessary
> > here.
> > 
>

https://codereview.chromium.org/20017005/diff/22001/ui/gl/gl_image_shm.cc
File ui/gl/gl_image_shm.cc (right):

https://codereview.chromium.org/20017005/diff/22001/ui/gl/gl_image_shm.cc#new...
ui/gl/gl_image_shm.cc:60: NOTREACHED();
On 2013/08/01 21:19:30, piman wrote:
> nit: this comes from the (untrusted) client side. While we don't expose this
> cross-process yet, I'm pretty sure we won't go back and audit everything. So
I'm
> not sure NOTREACHED() is really what we want (false sense of security etc.),
> though we can certainly DVLOG.

Good point. Fixed in latest patch.

[piman, 2013-08-08 23:22:07]

On Thu, Aug 8, 2013 at 4:19 PM, <reveman@chromium.org> wrote:

>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/common/command_**buffer.h<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common/command_buffer.h>
> File gpu/command_buffer/common/**command_buffer.h (right):
>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/common/command_**buffer.h#newcode151<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/common/command_buffer.h#newcode151>
> gpu/command_buffer/common/**command_buffer.h:151: virtual
> gfx::GpuMemoryBuffer* CreateGpuMemoryBuffer(
> On 2013/08/01 21:19:30, piman wrote:
>
>> I would prefer if we didn't load up CommandBuffer with GL-y concepts
>>
> and keep it
>
>> to the command processing level.
>>
>
>  We have been talking about adding a separate interface that
>>
> GLES2Implementation
>
>> (and higher level classes) can use to deal with things that don't go
>>
> through
>
>> command buffers (e.g. IPCs, or accessing singletons when in-process).
>>
> Maybe now
>
>> is the time to do it?
>>
>
> I've limited the CommandBuffer changes to adding
> Create/DestroyGpuMemoryBuffer which I don't think of as GL specific.
>

I would prefer that CommandBuffer sticks to command processing, not GPU
stuff...

But it sounds like we might still want to add a separate interface for
> this. Any suggestions for what the name of this interface should be?
>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/service/cmd_**buffer_engine.h<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/service/cmd_buffer_engine.h>
> File gpu/command_buffer/service/**cmd_buffer_engine.h (right):
>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/service/cmd_**buffer_engine.h#newcode46<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/service/cmd_buffer_engine.h#newcode46>
> gpu/command_buffer/service/**cmd_buffer_engine.h:46: virtual gfx::GLImage*
> GetImage(int32 image_id) = 0;
> On 2013/08/01 21:19:30, piman wrote:
>
>> Same comment here - I think this is exposed at a too low level.
>>
>
> Removed it.
>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/service/**command_buffer_service.cc<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/service/command_buffer_service.cc>
> File gpu/command_buffer/service/**command_buffer_service.cc (right):
>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/service/**command_buffer_service.cc#**newcode41<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/service/command_buffer_service.cc#newcode41>
> gpu/command_buffer/service/**command_buffer_service.cc:41:
> CommandBufferService::~**CommandBufferService() {
> On 2013/08/01 21:19:30, piman wrote:
>
>> I think if the client isn't well behaved (or crashes), you'll have
>>
> leftover
>
>> buffers in gpu_memory_buffers_ that need deletion here.
>>
>
> Done.
>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/service/**command_buffer_service.cc#**newcode226<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/service/command_buffer_service.cc#newcode226>
> gpu/command_buffer/service/**command_buffer_service.cc:226:
> scoped_ptr<gfx::**GpuMemoryBuffer> buffer = make_scoped_ptr(
> On 2013/08/01 21:19:30, piman wrote:
>
>> nit: you could use linked_ptr and put that linked_ptr in the map too
>>
> so that you
>
>> could avoid potential leaks (see above).
>>
>
> Done.
>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/service/**command_buffer_service.cc#**newcode233<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/service/command_buffer_service.cc#newcode233>
> gpu/command_buffer/service/**command_buffer_service.cc:233: static int32
> next_id = 1;
> On 2013/08/01 21:19:30, piman wrote:
>
>> Does this ID need to be shared between contexts?
>>
>
> Not before but it does in latest patch. Is this a problem?
>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/service/**command_buffer_service.cc#**newcode286<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/service/command_buffer_service.cc#newcode286>
> gpu/command_buffer/service/**command_buffer_service.cc:286:
> image_manager_->AddImage(gl_**image.get(), id);
> On 2013/08/01 21:19:30, piman wrote:
>
>> Mmh, this is the id generated locally for the GpuMemoryBuffer. How
>>
> does that
>
>> interact with the GpuMsg_CreateImage path, where ids get generated
>>
> client-side?
>
>> It seems like a high potential for conflicts.
>>
>
> Yes, these will conflict but shouldn't be a problem until we add real
> multi-process support and use it together with GpuMsg_CreateImage. This
> should be easy to fix by having GpuMsg_CreateImage return an id instead.
>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/service/common_**decoder.h<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/service/common_decoder.h>
> File gpu/command_buffer/service/**common_decoder.h (right):
>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/service/common_**decoder.h#newcode142<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/service/common_decoder.h#newcode142>
> gpu/command_buffer/service/**common_decoder.h:142: gfx::GLImage*
> GetImage(int32 image_id);
> On 2013/08/01 21:19:30, piman wrote:
>
>> nit: I guess we try to keep CommonDecoder GL-agnostic. Can we move
>>
> this to
>
>> GLES2DecoderImpl?
>>
>
> Removed this.
>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/service/gpu_**scheduler.h<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/service/gpu_scheduler.h>
> File gpu/command_buffer/service/**gpu_scheduler.h (right):
>
> https://codereview.chromium.**org/20017005/diff/22001/gpu/**
>
command_buffer/service/gpu_**scheduler.h#newcode91<https://codereview.chromium.org/20017005/diff/22001/gpu/command_buffer/service/gpu_scheduler.h#newcode91>
> gpu/command_buffer/service/**gpu_scheduler.h:91: virtual gfx::GLImage*
> GetImage(int32 image_id) OVERRIDE;
> On 2013/08/01 21:19:30, piman wrote:
>
>> On 2013/08/01 13:32:18, David Reveman wrote:
>> > On 2013/08/01 00:38:04, sievers wrote:
>> > > I find plumbing the ImageManager through the Scheduler a little
>>
> odd.
>
>> > > Maybe the decoder should just keep its own pointer?
>> >
>> > I'd rather not add new state unless necessary. Current patch is
>>
> consistent
>
>> with
>> > how transfer buffers and shared memory is handled.
>>
>
>  Transfer buffers and shared memory are an essential part of command
>>
> buffers.
>
>> This is GL-specific and I think it should be kept out of the command
>>
> processing
>
>> layer.
>>
>
> Remove this.
>
>
>  >
>> > >
>> > > Would also make some changes to the unittests and other files
>>
> unnecessary
>
>> > here.
>> >
>>
>
>
> https://codereview.chromium.**org/20017005/diff/22001/ui/gl/**
>
gl_image_shm.cc<https://codereview.chromium.org/20017005/diff/22001/ui/gl/gl_image_shm.cc>
> File ui/gl/gl_image_shm.cc (right):
>
> https://codereview.chromium.**org/20017005/diff/22001/ui/gl/**
>
gl_image_shm.cc#newcode60<https://codereview.chromium.org/20017005/diff/22001/ui/gl/gl_image_shm.cc#newcode60>
> ui/gl/gl_image_shm.cc:60: NOTREACHED();
> On 2013/08/01 21:19:30, piman wrote:
>
>> nit: this comes from the (untrusted) client side. While we don't
>>
> expose this
>
>> cross-process yet, I'm pretty sure we won't go back and audit
>>
> everything. So I'm
>
>> not sure NOTREACHED() is really what we want (false sense of security
>>
> etc.),
>
>> though we can certainly DVLOG.
>>
>
> Good point. Fixed in latest patch.
>
>
https://codereview.chromium.**org/20017005/<https://codereview.chromium.org/2...
>

[reveman, 2013-08-12 23:12:26]

Added a new interface instead of using CommandBuffer to latest patch. Called it
"GpuControl" for lack of better name. PTAL.

[piman, 2013-08-13 00:38:23]

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:2089: "GL_CHROMIUM_map_image "
I think this will enable the map image codepath in the compositor, even though
the out-of-process implementation of GpuControl isn't functional. Is that
intended? Would it work (with impl-side on)?

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_control_service.cc (right):

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_control_service.cc:31:
scoped_ptr<gfx::GpuMemoryBuffer> buffer = make_scoped_ptr(
nit: if you make this a linked_ptr from the start, then there's no shenanigans
below with having (temporarily) the buffer owned by both a linked_ptr and a
scoped_ptr at the same time.

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/servic...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/servic...
gpu/command_buffer/service/in_process_command_buffer.h:165: base::Lock
gpu_control_lock_;
This needs to be the same lock as command_buffer_lock_, otherwise you have a
race on the ImageManager, accessed both from the CommandBuffer on the GPU thread
(under command_buffer_lock_) and the GpuControl from the client thread (under
gpu_control_lock_) in CreateGpuMemoryBuffer.

https://codereview.chromium.org/20017005/diff/51001/ui/gl/gl_image_shm.h
File ui/gl/gl_image_shm.h (right):

https://codereview.chromium.org/20017005/diff/51001/ui/gl/gl_image_shm.h#newc...
ui/gl/gl_image_shm.h:15: GLImageShm(const gfx::Size& size, unsigned
internalformat);
nit: I think the consensus is to pass structs with <=4 ints by value.

[no sievers, 2013-08-13 00:45:42]

https://codereview.chromium.org/20017005/diff/51001/content/common/gpu/client...
File content/common/gpu/client/command_buffer_proxy_impl.h (right):

https://codereview.chromium.org/20017005/diff/51001/content/common/gpu/client...
content/common/gpu/client/command_buffer_proxy_impl.h:45: public
gpu::GpuControl,
I *thought* the concern of routing everything through the CommandBuffer was more
relevant to the client/proxy side.

If we still route things through here, why don't we just implement the service
portion in GpuCommandBufferStub then, similar to other things we already do?

[kaanb, 2013-08-13 00:51:09]

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/common...
File gpu/command_buffer/common/gpu_control.h (right):

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/common...
gpu/command_buffer/common/gpu_control.h:17: class GPU_EXPORT GpuControl {
How about GpuMemoryControl?

[piman, 2013-08-13 00:54:56]

On Mon, Aug 12, 2013 at 5:51 PM, <kaanb@chromium.org> wrote:

>
> https://codereview.chromium.**org/20017005/diff/51001/gpu/**
>
command_buffer/common/gpu_**control.h<https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/common/gpu_control.h>
> File gpu/command_buffer/common/gpu_**control.h (right):
>
> https://codereview.chromium.**org/20017005/diff/51001/gpu/**
>
command_buffer/common/gpu_**control.h#newcode17<https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/common/gpu_control.h#newcode17>
> gpu/command_buffer/common/gpu_**control.h:17: class GPU_EXPORT GpuControl
> {
> How about GpuMemoryControl?
>

We may want to do other things beyond GpuMemoryBuffers through that, e.g.
mailbox names, etc.


>
>
https://codereview.chromium.**org/20017005/<https://codereview.chromium.org/2...
>

[piman, 2013-08-13 00:57:55]

On Mon, Aug 12, 2013 at 5:45 PM, <sievers@chromium.org> wrote:

>
> https://codereview.chromium.**org/20017005/diff/51001/**
>
content/common/gpu/client/**command_buffer_proxy_impl.h<https://codereview.chromium.org/20017005/diff/51001/content/common/gpu/client/command_buffer_proxy_impl.h>
> File content/common/gpu/client/**command_buffer_proxy_impl.h (right):
>
> https://codereview.chromium.**org/20017005/diff/51001/**
>
content/common/gpu/client/**command_buffer_proxy_impl.h#**newcode45<https://codereview.chromium.org/20017005/diff/51001/content/common/gpu/client/command_buffer_proxy_impl.h#newcode45>
> content/common/gpu/client/**command_buffer_proxy_impl.h:**45: public
> gpu::GpuControl,
> I *thought* the concern of routing everything through the CommandBuffer
> was more relevant to the client/proxy side.
>

To me it's more a concern of what GLES2Implementation/GLES2CmdHelper talks
to. CommandBuffer for command control, something else (GpuControl) for GPU
things that go out-of-band.


> If we still route things through here, why don't we just implement the
> service portion in GpuCommandBufferStub then, similar to other things we
> already do?
>

I'll let David comment on the tradeoffs, but I imagine we want to share the
service-side implementation between the in-process and the out-of-process
versions, so GpuControlService makes at least some sense.


>
>
https://codereview.chromium.**org/20017005/<https://codereview.chromium.org/2...
>

[reveman, 2013-08-13 01:48:25]

https://codereview.chromium.org/20017005/diff/51001/content/common/gpu/client...
File content/common/gpu/client/command_buffer_proxy_impl.h (right):

https://codereview.chromium.org/20017005/diff/51001/content/common/gpu/client...
content/common/gpu/client/command_buffer_proxy_impl.h:45: public
gpu::GpuControl,
On 2013/08/13 00:45:43, sievers wrote:
> I *thought* the concern of routing everything through the CommandBuffer was
more
> relevant to the client/proxy side.
> 
> If we still route things through here, why don't we just implement the service
> portion in GpuCommandBufferStub then, similar to other things we already do?

I like having a *Service class that can be shared between in-process and
out-of-process usage. Part for code-reuse but mostly because I think it forces
us to make code work in both modes, or at least makes us think about how
something would work in the alternative mode when adding something new.

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/client...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/client...
gpu/command_buffer/client/gles2_implementation.cc:2089: "GL_CHROMIUM_map_image "
On 2013/08/13 00:38:24, piman wrote:
> I think this will enable the map image codepath in the compositor, even though
> the out-of-process implementation of GpuControl isn't functional. Is that
> intended? Would it work (with impl-side on)?

it would fail at the moment but you would have to specify --use-map-image to
have the compositor use 0-copy and trigger this. I can save the removal of this
conditional to a follow up patch if that's preferred? Doesn't matter to me. I
just don't think it should be here long term.

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/common...
File gpu/command_buffer/common/gpu_control.h (right):

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/common...
gpu/command_buffer/common/gpu_control.h:17: class GPU_EXPORT GpuControl {
On 2013/08/13 00:51:09, kaanb wrote:
> How about GpuMemoryControl?

Based on previous discussion this interface might be used for more than
GpuMemoryBuffers. Anything that might need IPC but is not a critical part of the
command buffer would go here.

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_control_service.cc (right):

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_control_service.cc:31:
scoped_ptr<gfx::GpuMemoryBuffer> buffer = make_scoped_ptr(
On 2013/08/13 00:38:24, piman wrote:
> nit: if you make this a linked_ptr from the start, then there's no shenanigans
> below with having (temporarily) the buffer owned by both a linked_ptr and a
> scoped_ptr at the same time.

Done.

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/servic...
File gpu/command_buffer/service/in_process_command_buffer.h (right):

https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/servic...
gpu/command_buffer/service/in_process_command_buffer.h:165: base::Lock
gpu_control_lock_;
On 2013/08/13 00:38:24, piman wrote:
> This needs to be the same lock as command_buffer_lock_, otherwise you have a
> race on the ImageManager, accessed both from the CommandBuffer on the GPU
thread
> (under command_buffer_lock_) and the GpuControl from the client thread (under
> gpu_control_lock_) in CreateGpuMemoryBuffer.

Done.

https://codereview.chromium.org/20017005/diff/51001/ui/gl/gl_image_shm.h
File ui/gl/gl_image_shm.h (right):

https://codereview.chromium.org/20017005/diff/51001/ui/gl/gl_image_shm.h#newc...
ui/gl/gl_image_shm.h:15: GLImageShm(const gfx::Size& size, unsigned
internalformat);
On 2013/08/13 00:38:24, piman wrote:
> nit: I think the consensus is to pass structs with <=4 ints by value.

Latest patch pass |size| by value.

[piman, 2013-08-13 20:54:43]

On Mon, Aug 12, 2013 at 6:48 PM, <reveman@chromium.org> wrote:

>
> https://codereview.chromium.**org/20017005/diff/51001/**
>
content/common/gpu/client/**command_buffer_proxy_impl.h<https://codereview.chromium.org/20017005/diff/51001/content/common/gpu/client/command_buffer_proxy_impl.h>
> File content/common/gpu/client/**command_buffer_proxy_impl.h (right):
>
> https://codereview.chromium.**org/20017005/diff/51001/**
>
content/common/gpu/client/**command_buffer_proxy_impl.h#**newcode45<https://codereview.chromium.org/20017005/diff/51001/content/common/gpu/client/command_buffer_proxy_impl.h#newcode45>
> content/common/gpu/client/**command_buffer_proxy_impl.h:**45: public
> gpu::GpuControl,
> On 2013/08/13 00:45:43, sievers wrote:
>
>> I *thought* the concern of routing everything through the
>>
> CommandBuffer was more
>
>> relevant to the client/proxy side.
>>
>
>  If we still route things through here, why don't we just implement the
>>
> service
>
>> portion in GpuCommandBufferStub then, similar to other things we
>>
> already do?
>
> I like having a *Service class that can be shared between in-process and
> out-of-process usage. Part for code-reuse but mostly because I think it
> forces us to make code work in both modes, or at least makes us think
> about how something would work in the alternative mode when adding
> something new.
>
>
> https://codereview.chromium.**org/20017005/diff/51001/gpu/**
>
command_buffer/client/gles2_**implementation.cc<https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/client/gles2_implementation.cc>
> File gpu/command_buffer/client/**gles2_implementation.cc (right):
>
> https://codereview.chromium.**org/20017005/diff/51001/gpu/**
>
command_buffer/client/gles2_**implementation.cc#newcode2089<https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/client/gles2_implementation.cc#newcode2089>
> gpu/command_buffer/client/**gles2_implementation.cc:2089:
> "GL_CHROMIUM_map_image "
> On 2013/08/13 00:38:24, piman wrote:
>
>> I think this will enable the map image codepath in the compositor,
>>
> even though
>
>> the out-of-process implementation of GpuControl isn't functional. Is
>>
> that
>
>> intended? Would it work (with impl-side on)?
>>
>
> it would fail at the moment but you would have to specify
> --use-map-image to have the compositor use 0-copy and trigger this. I
> can save the removal of this conditional to a follow up patch if that's
> preferred? Doesn't matter to me. I just don't think it should be here
> long term.


Agreed on the long term, but I think in the short term we shouldn't expose
an extension when we don't support it - there lies the path to madness.

The rest LGTM.


>
>
> https://codereview.chromium.**org/20017005/diff/51001/gpu/**
>
command_buffer/common/gpu_**control.h<https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/common/gpu_control.h>
> File gpu/command_buffer/common/gpu_**control.h (right):
>
> https://codereview.chromium.**org/20017005/diff/51001/gpu/**
>
command_buffer/common/gpu_**control.h#newcode17<https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/common/gpu_control.h#newcode17>
> gpu/command_buffer/common/gpu_**control.h:17: class GPU_EXPORT GpuControl
> {
> On 2013/08/13 00:51:09, kaanb wrote:
>
>> How about GpuMemoryControl?
>>
>
> Based on previous discussion this interface might be used for more than
> GpuMemoryBuffers. Anything that might need IPC but is not a critical
> part of the command buffer would go here.
>
>
> https://codereview.chromium.**org/20017005/diff/51001/gpu/**
>
command_buffer/service/gpu_**control_service.cc<https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/service/gpu_control_service.cc>
> File gpu/command_buffer/service/**gpu_control_service.cc (right):
>
> https://codereview.chromium.**org/20017005/diff/51001/gpu/**
>
command_buffer/service/gpu_**control_service.cc#newcode31<https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/service/gpu_control_service.cc#newcode31>
> gpu/command_buffer/service/**gpu_control_service.cc:31:
> scoped_ptr<gfx::**GpuMemoryBuffer> buffer = make_scoped_ptr(
> On 2013/08/13 00:38:24, piman wrote:
>
>> nit: if you make this a linked_ptr from the start, then there's no
>>
> shenanigans
>
>> below with having (temporarily) the buffer owned by both a linked_ptr
>>
> and a
>
>> scoped_ptr at the same time.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/20017005/diff/51001/gpu/**
>
command_buffer/service/in_**process_command_buffer.h<https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/service/in_process_command_buffer.h>
> File gpu/command_buffer/service/in_**process_command_buffer.h (right):
>
> https://codereview.chromium.**org/20017005/diff/51001/gpu/**
>
command_buffer/service/in_**process_command_buffer.h#**newcode165<https://codereview.chromium.org/20017005/diff/51001/gpu/command_buffer/service/in_process_command_buffer.h#newcode165>
> gpu/command_buffer/service/in_**process_command_buffer.h:165: base::Lock
> gpu_control_lock_;
> On 2013/08/13 00:38:24, piman wrote:
>
>> This needs to be the same lock as command_buffer_lock_, otherwise you
>>
> have a
>
>> race on the ImageManager, accessed both from the CommandBuffer on the
>>
> GPU thread
>
>> (under command_buffer_lock_) and the GpuControl from the client thread
>>
> (under
>
>> gpu_control_lock_) in CreateGpuMemoryBuffer.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/20017005/diff/51001/ui/gl/**
>
gl_image_shm.h<https://codereview.chromium.org/20017005/diff/51001/ui/gl/gl_image_shm.h>
> File ui/gl/gl_image_shm.h (right):
>
> https://codereview.chromium.**org/20017005/diff/51001/ui/gl/**
>
gl_image_shm.h#newcode15<https://codereview.chromium.org/20017005/diff/51001/ui/gl/gl_image_shm.h#newcode15>
> ui/gl/gl_image_shm.h:15: GLImageShm(const gfx::Size& size, unsigned
> internalformat);
> On 2013/08/13 00:38:24, piman wrote:
>
>> nit: I think the consensus is to pass structs with <=4 ints by value.
>>
>
> Latest patch pass |size| by value.
>
>
https://codereview.chromium.**org/20017005/<https://codereview.chromium.org/2...
>

[no sievers, 2013-08-14 03:43:02]

https://codereview.chromium.org/20017005/diff/64001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_control_service.cc (right):

https://codereview.chromium.org/20017005/diff/64001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_control_service.cc:37: static int32 next_id = 1;
Do you think we could either let the client pass in the id (and maybe use an id
allocator for namespace::kImages there), or use the existing id allocator on the
service side?

As discussed in chat earlier, it conflicts with
https://codereview.chromium.org/22824009, and what I suggested then turned out
to be a bit ugly too.

[reveman, 2013-08-14 15:48:59]

Latest patch doesn't expose the CHROMIUM_map_image extension unless supported
and avoids adding support for additional internal formats by fixing the
compositor usage. It still includes the plumbing necessary to support other
internal formats but keeps it limited to GL_RGBA8 for now.

https://codereview.chromium.org/20017005/diff/64001/gpu/command_buffer/servic...
File gpu/command_buffer/service/gpu_control_service.cc (right):

https://codereview.chromium.org/20017005/diff/64001/gpu/command_buffer/servic...
gpu/command_buffer/service/gpu_control_service.cc:37: static int32 next_id = 1;
On 2013/08/14 03:43:02, sievers wrote:
> Do you think we could either let the client pass in the id (and maybe use an
id
> allocator for namespace::kImages there), or use the existing id allocator on
the
> service side?
> 
> As discussed in chat earlier, it conflicts with
> https://codereview.chromium.org/22824009, and what I suggested then turned out
> to be a bit ugly too.

I don't want register of a buffer with the gpu process to require synchronous
IPC and I'd like to remove the ImageManager from the share group and limit the
scope of image ids to one context asap. Generating ids here prevents us from
making assumptions that will make it hard to achieve these goals.

I was hoping we can use a separate id namespace for stream texture images until
the usage lines up with GLImage.

[no sievers, 2013-08-14 16:00:12]

On 2013/08/14 15:48:59, David Reveman wrote:
> Latest patch doesn't expose the CHROMIUM_map_image extension unless supported
> and avoids adding support for additional internal formats by fixing the
> compositor usage. It still includes the plumbing necessary to support other
> internal formats but keeps it limited to GL_RGBA8 for now.
> 
>
https://codereview.chromium.org/20017005/diff/64001/gpu/command_buffer/servic...
> File gpu/command_buffer/service/gpu_control_service.cc (right):
> 
>
https://codereview.chromium.org/20017005/diff/64001/gpu/command_buffer/servic...
> gpu/command_buffer/service/gpu_control_service.cc:37: static int32 next_id =
1;
> On 2013/08/14 03:43:02, sievers wrote:
> > Do you think we could either let the client pass in the id (and maybe use an
> id
> > allocator for namespace::kImages there), or use the existing id allocator on
> the
> > service side?
> > 
> > As discussed in chat earlier, it conflicts with
> > https://codereview.chromium.org/22824009, and what I suggested then turned
out
> > to be a bit ugly too.
> 
> I don't want register of a buffer with the gpu process to require synchronous
> IPC and I'd like to remove the ImageManager from the share group and limit the
> scope of image ids to one context asap. Generating ids here prevents us from
> making assumptions that will make it hard to achieve these goals.
> 

But it could happen even further up in the client right? What about an id
allocator
in GLES2Implementation and allocating the id in
GLES2Implementation::CreateImageCHROMIUMHelper()?

You can then put the allocator either in the ShareGroup or in the
GLES2Implementation directly if you want to limit the scope.

> I was hoping we can use a separate id namespace for stream texture images
until
> the usage lines up with GLImage.

[piman, 2013-08-14 20:43:25]

https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/...
File gpu/command_buffer/tests/gl_manager.cc (right):

https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/...
gpu/command_buffer/tests/gl_manager.cc:104: #ifndef EGL_ALPHA_SIZE
Why is this needed suddenly?

[reveman, 2013-08-15 15:00:54]

https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/...
File gpu/command_buffer/tests/gl_manager.cc (right):

https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/...
gpu/command_buffer/tests/gl_manager.cc:104: #ifndef EGL_ALPHA_SIZE
On 2013/08/14 20:43:26, piman wrote:
> Why is this needed suddenly?

this file now has to include gpu_control_service.h, which has to include
ui/gfx/gpu_memory_buffer.h for the definition of gfx::GpuMemoryBufferHandle,
which contains a EGLClientBuffer on android and that makes us pull in egl.h.
Result is of course that these enums are already defined.

DEPS doesn't allow egl.h to be included here directly, which I think makes
sense.

Created https://codereview.chromium.org/23244002/ that limits the need to define
these EGL constants to command_buffer/common/gles2_cmd_helper.cc. These
constants are no longer needed here after rebasing this patch on that cl.

[no sievers, 2013-08-15 16:45:12]

On 2013/08/15 15:00:54, David Reveman wrote:
>
https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/...
> File gpu/command_buffer/tests/gl_manager.cc (right):
> 
>
https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/...
> gpu/command_buffer/tests/gl_manager.cc:104: #ifndef EGL_ALPHA_SIZE
> On 2013/08/14 20:43:26, piman wrote:
> > Why is this needed suddenly?
> 
> this file now has to include gpu_control_service.h, which has to include
> ui/gfx/gpu_memory_buffer.h for the definition of gfx::GpuMemoryBufferHandle,
> which contains a EGLClientBuffer on android and that makes us pull in egl.h.
> Result is of course that these enums are already defined.
> 

If it helps to remove EGLClientBuffer there, we can follow up in
https://codereview.chromium.org/22486004/ to get rid of it.

There is an existing problem with that since the type is basically completely
anonymous and doesn't allow us to modify the refcount, and the way this is
currently allocated for WebView is not compatible with passing this handle
around asynchronously (like we do it with shared memory handles).



> DEPS doesn't allow egl.h to be included here directly, which I think makes
> sense.
> 
> Created https://codereview.chromium.org/23244002/ that limits the need to
define
> these EGL constants to command_buffer/common/gles2_cmd_helper.cc. These
> constants are no longer needed here after rebasing this patch on that cl.

[no sievers, 2013-08-15 16:50:44]

On 2013/08/15 16:45:12, sievers wrote:
> On 2013/08/15 15:00:54, David Reveman wrote:
> >
>
https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/...
> > File gpu/command_buffer/tests/gl_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/...
> > gpu/command_buffer/tests/gl_manager.cc:104: #ifndef EGL_ALPHA_SIZE
> > On 2013/08/14 20:43:26, piman wrote:
> > > Why is this needed suddenly?
> > 
> > this file now has to include gpu_control_service.h, which has to include
> > ui/gfx/gpu_memory_buffer.h for the definition of gfx::GpuMemoryBufferHandle,
> > which contains a EGLClientBuffer on android and that makes us pull in egl.h.
> > Result is of course that these enums are already defined.
> > 
> 
> If it helps to remove EGLClientBuffer there, we can follow up in
> https://codereview.chromium.org/22486004/ to get rid of it.
> 
> There is an existing problem with that since the type is basically completely
> anonymous and doesn't allow us to modify the refcount, and the way this is
> currently allocated for WebView is not compatible with passing this handle
> around asynchronously (like we do it with shared memory handles).
> 

Or just make it something else if it causes problems (or 'void *'). It needs to
be static_casted in at least one place anyways.
I don't think it matters much where that happens. And that handle is entirely
opaque anyways.


> 
> 
> > DEPS doesn't allow egl.h to be included here directly, which I think makes
> > sense.
> > 
> > Created https://codereview.chromium.org/23244002/ that limits the need to
> define
> > these EGL constants to command_buffer/common/gles2_cmd_helper.cc. These
> > constants are no longer needed here after rebasing this patch on that cl.

[reveman, 2013-08-15 17:08:00]

On 2013/08/15 16:50:44, sievers wrote:
> On 2013/08/15 16:45:12, sievers wrote:
> > On 2013/08/15 15:00:54, David Reveman wrote:
> > >
> >
>
https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/...
> > > File gpu/command_buffer/tests/gl_manager.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/...
> > > gpu/command_buffer/tests/gl_manager.cc:104: #ifndef EGL_ALPHA_SIZE
> > > On 2013/08/14 20:43:26, piman wrote:
> > > > Why is this needed suddenly?
> > > 
> > > this file now has to include gpu_control_service.h, which has to include
> > > ui/gfx/gpu_memory_buffer.h for the definition of
gfx::GpuMemoryBufferHandle,
> > > which contains a EGLClientBuffer on android and that makes us pull in
egl.h.
> > > Result is of course that these enums are already defined.
> > > 
> > 
> > If it helps to remove EGLClientBuffer there, we can follow up in
> > https://codereview.chromium.org/22486004/ to get rid of it.
> > 
> > There is an existing problem with that since the type is basically
completely
> > anonymous and doesn't allow us to modify the refcount, and the way this is
> > currently allocated for WebView is not compatible with passing this handle
> > around asynchronously (like we do it with shared memory handles).
> > 
> 
> Or just make it something else if it causes problems (or 'void *'). It needs
to
> be static_casted in at least one place anyways.
> I don't think it matters much where that happens. And that handle is entirely
> opaque anyways.

I don't think that solves the real problem. We should be able to use a
EGLClientBuffer here if we like, which at the moment is pretty clean as it
doesn't require any static_casts and could be defined as a struct on some
platform without breaking the chromium code. Or is there some static_cast
somewhere that I've missed?

We might need the ref counting you mentioned but that's a different issue.

> 
> 
> > 
> > 
> > > DEPS doesn't allow egl.h to be included here directly, which I think makes
> > > sense.
> > > 
> > > Created https://codereview.chromium.org/23244002/ that limits the need to
> > define
> > > these EGL constants to command_buffer/common/gles2_cmd_helper.cc. These
> > > constants are no longer needed here after rebasing this patch on that cl.

[piman, 2013-08-15 17:11:46]

On Thu, Aug 15, 2013 at 8:00 AM, <reveman@chromium.org> wrote:

>
> https://codereview.chromium.**org/20017005/diff/91001/gpu/**
>
command_buffer/tests/gl_**manager.cc<https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/gl_manager.cc>
> File gpu/command_buffer/tests/gl_**manager.cc (right):
>
> https://codereview.chromium.**org/20017005/diff/91001/gpu/**
>
command_buffer/tests/gl_**manager.cc#newcode104<https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/gl_manager.cc#newcode104>
> gpu/command_buffer/tests/gl_**manager.cc:104: #ifndef EGL_ALPHA_SIZE
> On 2013/08/14 20:43:26, piman wrote:
>
>> Why is this needed suddenly?
>>
>
> this file now has to include gpu_control_service.h, which has to include
> ui/gfx/gpu_memory_buffer.h for the definition of
> gfx::GpuMemoryBufferHandle, which contains a EGLClientBuffer on android
> and that makes us pull in egl.h. Result is of course that these enums
> are already defined.
>
> DEPS doesn't allow egl.h to be included here directly, which I think
> makes sense.
>

Since many tests include the service side, I think it's really ok to
include egl.h (or, alternatively, gl_bindings.h)

>
> Created
https://codereview.chromium.**org/23244002/<https://codereview.chromium.org/2...
limits the need
> to define these EGL constants to
> command_buffer/common/gles2_**cmd_helper.cc. These constants are no longer
> needed here after rebasing this patch on that cl.
>
>
https://codereview.chromium.**org/20017005/<https://codereview.chromium.org/2...
>

[kaanb, 2013-08-15 18:22:49]

Android glue-layer returns a void* for the NativeHandle, so that's
implicitly being cast into EGLClientBuffer in the GpuMemoryBufferFactory. I
believe this is the cast Daniel is talking about.


On Thu, Aug 15, 2013 at 10:08 AM, <reveman@chromium.org> wrote:

> On 2013/08/15 16:50:44, sievers wrote:
>
>> On 2013/08/15 16:45:12, sievers wrote:
>> > On 2013/08/15 15:00:54, David Reveman wrote:
>> > >
>> >
>>
>
> https://codereview.chromium.**org/20017005/diff/91001/gpu/**
>
command_buffer/tests/gl_**manager.cc<https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/gl_manager.cc>
>
>> > > File gpu/command_buffer/tests/gl_**manager.cc (right):
>> > >
>> > >
>> >
>>
>
> https://codereview.chromium.**org/20017005/diff/91001/gpu/**
>
command_buffer/tests/gl_**manager.cc#newcode104<https://codereview.chromium.org/20017005/diff/91001/gpu/command_buffer/tests/gl_manager.cc#newcode104>
>
>> > > gpu/command_buffer/tests/gl_**manager.cc:104: #ifndef EGL_ALPHA_SIZE
>> > > On 2013/08/14 20:43:26, piman wrote:
>> > > > Why is this needed suddenly?
>> > >
>> > > this file now has to include gpu_control_service.h, which has to
>> include
>> > > ui/gfx/gpu_memory_buffer.h for the definition of
>>
> gfx::GpuMemoryBufferHandle,
>
>> > > which contains a EGLClientBuffer on android and that makes us pull in
>>
> egl.h.
>
>> > > Result is of course that these enums are already defined.
>> > >
>> >
>> > If it helps to remove EGLClientBuffer there, we can follow up in
>> >
https://codereview.chromium.**org/22486004/<https://codereview.chromium.org/2...
get rid of it.
>> >
>> > There is an existing problem with that since the type is basically
>>
> completely
>
>> > anonymous and doesn't allow us to modify the refcount, and the way this
>> is
>> > currently allocated for WebView is not compatible with passing this
>> handle
>> > around asynchronously (like we do it with shared memory handles).
>> >
>>
>
>  Or just make it something else if it causes problems (or 'void *'). It
>> needs
>>
> to
>
>> be static_casted in at least one place anyways.
>> I don't think it matters much where that happens. And that handle is
>> entirely
>> opaque anyways.
>>
>
> I don't think that solves the real problem. We should be able to use a
> EGLClientBuffer here if we like, which at the moment is pretty clean as it
> doesn't require any static_casts and could be defined as a struct on some
> platform without breaking the chromium code. Or is there some static_cast
> somewhere that I've missed?
>
> We might need the ref counting you mentioned but that's a different issue.
>
>
>
>
>  >
>> >
>> > > DEPS doesn't allow egl.h to be included here directly, which I think
>> makes
>> > > sense.
>> > >
>> > > Created
https://codereview.chromium.**org/23244002/<https://codereview.chromium.org/2...
limits the need to
>> > define
>> > > these EGL constants to command_buffer/common/gles2_**cmd_helper.cc.
>> These
>> > > constants are no longer needed here after rebasing this patch on that
>> cl.
>>
>
>
>
>
https://codereview.chromium.**org/20017005/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kaanb, 2013-08-15 18:31:29]

lgtm pending the resolution of other comments

https://codereview.chromium.org/20017005/diff/99001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/20017005/diff/99001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.h:187: // Returns the format that need to be
used for raster task resources.
nit: s/need/needs/

https://codereview.chromium.org/20017005/diff/99001/gpu/command_buffer/client...
File gpu/command_buffer/client/client_test_helper.h (right):

https://codereview.chromium.org/20017005/diff/99001/gpu/command_buffer/client...
gpu/command_buffer/client/client_test_helper.h:95:
MOCK_METHOD1(DestroyGpuMemoryBuffer, void(int32 id));
nit: you don't need the parameter name, i.e. s/int32 id/int32/

https://codereview.chromium.org/20017005/diff/99001/gpu/command_buffer/tests/...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc (right):

https://codereview.chromium.org/20017005/diff/99001/gpu/command_buffer/tests/...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:61:
MOCK_METHOD3(CreateGpuMemoryBuffer,
nit: parameter names are optional

[piman, 2013-08-15 19:19:32]

lgtm

[reveman, 2013-08-16 01:34:56]

https://codereview.chromium.org/20017005/diff/99001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/20017005/diff/99001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.h:187: // Returns the format that need to be
used for raster task resources.
On 2013/08/15 18:31:29, kaanb wrote:
> nit: s/need/needs/

Done.

https://codereview.chromium.org/20017005/diff/99001/gpu/command_buffer/client...
File gpu/command_buffer/client/client_test_helper.h (right):

https://codereview.chromium.org/20017005/diff/99001/gpu/command_buffer/client...
gpu/command_buffer/client/client_test_helper.h:95:
MOCK_METHOD1(DestroyGpuMemoryBuffer, void(int32 id));
On 2013/08/15 18:31:29, kaanb wrote:
> nit: you don't need the parameter name, i.e. s/int32 id/int32/

prefer to keep parameter name as it's consistent with the above code.

https://codereview.chromium.org/20017005/diff/99001/gpu/command_buffer/tests/...
File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc (right):

https://codereview.chromium.org/20017005/diff/99001/gpu/command_buffer/tests/...
gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:61:
MOCK_METHOD3(CreateGpuMemoryBuffer,
On 2013/08/15 18:31:29, kaanb wrote:
> nit: parameter names are optional

Removed them as that is consistent with above code.

[joth, 2013-08-16 01:41:27]

AW/ lgtm rubber stamp

On Thursday, 15 August 2013,  <reveman@chromium.org> wrote:
>
>
https://codereview.chromium.org/20017005/diff/99001/cc/resources/raster_worke...
> File cc/resources/raster_worker_pool.h (right):
>
>
https://codereview.chromium.org/20017005/diff/99001/cc/resources/raster_worke...
> cc/resources/raster_worker_pool.h:187: // Returns the format that need
> to be used for raster task resources.
> On 2013/08/15 18:31:29, kaanb wrote:
>>
>> nit: s/need/needs/
>
> Done.
>
>
https://codereview.chromium.org/20017005/diff/99001/gpu/command_buffer/client...
> File gpu/command_buffer/client/client_test_helper.h (right):
>
>
https://codereview.chromium.org/20017005/diff/99001/gpu/command_buffer/client...
> gpu/command_buffer/client/client_test_helper.h:95:
> MOCK_METHOD1(DestroyGpuMemoryBuffer, void(int32 id));
> On 2013/08/15 18:31:29, kaanb wrote:
>>
>> nit: you don't need the parameter name, i.e. s/int32 id/int32/
>
> prefer to keep parameter name as it's consistent with the above code.
>
>
https://codereview.chromium.org/20017005/diff/99001/gpu/command_buffer/tests/...
> File gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc (right):
>
>
https://codereview.chromium.org/20017005/diff/99001/gpu/command_buffer/tests/...
> gpu/command_buffer/tests/gl_gpu_memory_buffer_unittests.cc:61:
> MOCK_METHOD3(CreateGpuMemoryBuffer,
> On 2013/08/15 18:31:29, kaanb wrote:
>>
>> nit: parameter names are optional
>
> Removed them as that is consistent with above code.
>
> https://codereview.chromium.org/20017005/
>

[commit-bot: I haz the power, 2013-08-16 03:04:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/20017005/174001

[commit-bot: I haz the power, 2013-08-16 07:32:32]

Failed to apply patch for cc/test/fake_tile_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/test/fake_tile_manager.cc
  Hunk #1 FAILED at 15.
  Hunk #2 succeeded at 47 (offset 21 lines).
  Hunk #3 succeeded at 55 (offset 21 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
cc/test/fake_tile_manager.cc.rej

Patch:       cc/test/fake_tile_manager.cc
Index: cc/test/fake_tile_manager.cc
diff --git a/cc/test/fake_tile_manager.cc b/cc/test/fake_tile_manager.cc
index
c1322548492250673b41bf5a4858067be1cefc2c..7749965c44bc3f05be2ae62a81fd670c893339bf
100644
--- a/cc/test/fake_tile_manager.cc
+++ b/cc/test/fake_tile_manager.cc
@@ -15,6 +15,7 @@ class FakeRasterWorkerPool : public RasterWorkerPool {
   FakeRasterWorkerPool() : RasterWorkerPool(NULL, 1) {}
 
   virtual void ScheduleTasks(RasterTask::Queue* queue) OVERRIDE {}
+  virtual GLenum GetResourceFormat() const OVERRIDE { return GL_RGBA; }
   virtual void OnRasterTasksFinished() OVERRIDE {}
   virtual void OnRasterTasksRequiredForActivationFinished() OVERRIDE {}
 };
@@ -26,8 +27,7 @@ FakeTileManager::FakeTileManager(TileManagerClient* client)
                   NULL,
                   make_scoped_ptr<RasterWorkerPool>(new FakeRasterWorkerPool),
                   1,
-                  NULL,
-                  GL_RGBA) {}
+                  NULL) {}
 
 FakeTileManager::FakeTileManager(TileManagerClient* client,
                                  ResourceProvider* resource_provider)
@@ -35,8 +35,7 @@ FakeTileManager::FakeTileManager(TileManagerClient* client,
                   resource_provider,
                   make_scoped_ptr<RasterWorkerPool>(new FakeRasterWorkerPool),
                   1,
-                  NULL,
-                  resource_provider->best_texture_format()) {}
+                  NULL) {}
 
 FakeTileManager::~FakeTileManager() {}

[commit-bot: I haz the power, 2013-08-16 14:49:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/20017005/187001

[commit-bot: I haz the power, 2013-08-16 17:31:06]

Change committed as 218034

[sfaddy1983, 2013-08-17 19:36:42]

lgtm