Enable sync allocation of GpuMemoryBuffers from the browser process.

Enable sync allocation of GpuMemoryBuffers from the browser process.

This operation will be serviced on the IO thread in the GPU process and is expected to be very quick.

These sync allocations should be infrequent and happen mostly during startup to enable rendering via EGLImage.

BUG=380861
Committed: https://crrev.com/0269a5cd022535da97d21fee45162c24b211f9d3
Cr-Commit-Position: refs/heads/master@{#294109}

[alexst (slow to review), 2014-09-03 18:20:51]

Patchset #1 (id:1) has been deleted

[alexst (slow to review), 2014-09-04 13:11:54]

alexst@chromium.org changed reviewers:
+ piman@chromium.org, reveman@chromium.org

[alexst (slow to review), 2014-09-04 13:13:52]

Please take a look.

This code only does sync allocation for buffers originating from the browser,
those coming from the renderer remain async in the browser process.

[piman, 2014-09-04 17:21:33]

One thing, otherwise I think this is good. I can't help but think it would be a
bit simpler (and less latent) to have the renderer's request handled on the
browser's IO thread so we wouldn't need 2 types of CreateGpuMemoryBufferRequest.
But that can wait.

https://codereview.chromium.org/540443002/diff/60001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_channel_manager.cc (left):

https://codereview.chromium.org/540443002/diff/60001/content/common/gpu/gpu_c...
content/common/gpu/gpu_channel_manager.cc:209: } else {
Is this logic not needed any more? It used to be that we needed to ensure
ordering between earlier Destroy (that could be delayed because sync point) and
later Create, because (IIRC) ids might be reused.
Dave refactored some of this to use GpuMemoryBufferHandle instead of simple ids,
is that why this is not a problem any more (if handles aren't reused)?
If that's the case, we can remove the gpu_memory_buffer_operations_ altogether,
and simply queue the destruction via the sync point callback, because I don't
think we need ordering between destroys.

[alexst (slow to review), 2014-09-04 17:46:02]

https://codereview.chromium.org/540443002/diff/60001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_channel_manager.cc (left):

https://codereview.chromium.org/540443002/diff/60001/content/common/gpu/gpu_c...
content/common/gpu/gpu_channel_manager.cc:209: } else {
These are identified by an int pair, one of which is a client id, the other is a
monotonically increasing number.

Even if the client id is reused, the second number is not scoped by it, so it
just keeps going up. There won't be reuse, short of the monotonic sequence
wrapping itself.

I'll clean up the rest of the destroy code.

[alexst (slow to review), 2014-09-04 18:02:32]

PTAL

[piman, 2014-09-04 18:08:52]

https://codereview.chromium.org/540443002/diff/80001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/540443002/diff/80001/content/common/gpu/gpu_c...
content/common/gpu/gpu_channel_manager.cc:67: sender_->Send(new
GpuHostMsg_GpuMemoryBufferCreated(handle));
Lost the gpu_memory_buffer_factory_->CreateGpuMemoryBuffer(...) ?

[alexst (slow to review), 2014-09-04 18:45:12]

https://codereview.chromium.org/540443002/diff/80001/content/common/gpu/gpu_c...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/540443002/diff/80001/content/common/gpu/gpu_c...
content/common/gpu/gpu_channel_manager.cc:67: sender_->Send(new
GpuHostMsg_GpuMemoryBufferCreated(handle));
On 2014/09/04 18:08:52, piman (OOO) wrote:
> Lost the gpu_memory_buffer_factory_->CreateGpuMemoryBuffer(...) ?

Gaah! Thanks for catching it!

[piman, 2014-09-04 19:01:47]

lgtm

[reveman, 2014-09-04 19:43:31]

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_factory_host.h (right):

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_factory_host.h:38: unsigned usage) =
0;
Not a fan of this alternative function for doing this. Is there a way we can
just block on the browser UI thread and have everything on the IO thread work
exactly the same?

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:55: //
+1 ensures we always get non-zero IDs.
Except when it wraps around. Consider adding a GetNextBufferId utility function
that calls g_next_buffer_id.GetNext() multiple times if necessary.

Btw, why does this need to be a non-zero?

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h
(right):

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:33:
unsigned usage);
The current convention for doing this is to add an Initialize() member function
as the use of an gfx::GpuMemoryBufferHandle should not have to be exposed. See
GpuMemoryBufferImplSharedMemory.

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:28: class
GpuChannelManagerMessageFilter : public IPC::MessageFilter {
I would much prefer if this "allocation on IO thread" change was done in a
separate patch.

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:74: };
style nit: blank line after this

[piman, 2014-09-04 19:59:20]

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_factory_host.h (right):

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_factory_host.h:38: unsigned usage) =
0;
On 2014/09/04 19:43:31, reveman wrote:
> Not a fan of this alternative function for doing this. Is there a way we can
> just block on the browser UI thread and have everything on the IO thread work
> exactly the same?

We should not block the UI thread except when we really need to. In particular,
we don't need to block in response to the renderer's messages, and so we
shouldn't.

[alexst (slow to review), 2014-09-04 21:14:48]

PTAL

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:55: //
+1 ensures we always get non-zero IDs.
On 2014/09/04 19:43:31, reveman wrote:
> Except when it wraps around. Consider adding a GetNextBufferId utility
function
> that calls g_next_buffer_id.GetNext() multiple times if necessary.
> 
> Btw, why does this need to be a non-zero?

This is copy/pasted from just above in this file. I don't think there is any
hard reason why it couldn't be zero other than by convention of zero being a
null object.

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h
(right):

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:33:
unsigned usage);
On 2014/09/04 19:43:31, reveman wrote:
> The current convention for doing this is to add an Initialize() member
function
> as the use of an gfx::GpuMemoryBufferHandle should not have to be exposed. See
> GpuMemoryBufferImplSharedMemory.

Done.

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:28: class
GpuChannelManagerMessageFilter : public IPC::MessageFilter {
On 2014/09/04 19:43:31, reveman wrote:
> I would much prefer if this "allocation on IO thread" change was done in a
> separate patch.

I originally started writing these as two separate patches, but decided to merge
them because one gives much needed context to the other.

Servicing this on IO in the GPU process is part of the design for the browser
side UI thread blocking for as little as possible. I felt there is benefit in
having the end-to-end interaction in one patch.

That said, if you feel strongly about the two parts being separate, I'll split
them up.

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:74: };
On 2014/09/04 19:43:31, reveman wrote:
> style nit: blank line after this

Done.

[reveman, 2014-09-04 22:37:24]

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_factory_host.h (right):

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_factory_host.h:38: unsigned usage) =
0;
On 2014/09/04 19:59:20, piman (OOO) wrote:
> On 2014/09/04 19:43:31, reveman wrote:
> > Not a fan of this alternative function for doing this. Is there a way we can
> > just block on the browser UI thread and have everything on the IO thread
work
> > exactly the same?
> 
> We should not block the UI thread except when we really need to. In
particular,
> we don't need to block in response to the renderer's messages, and so we
> shouldn't.

Agree. I guess my question was, can we make this interface be something that is
used on the IO thread instead?

We'd have the renderer usage be completely asynchronous on the browser side and
all work done on the IO thread. While the browser would use the exact same code
on the IO thread, just block on the reply on the UI thread. I think that would
be much cleaner if possible.

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:55: //
+1 ensures we always get non-zero IDs.
On 2014/09/04 21:14:47, alexst wrote:
> On 2014/09/04 19:43:31, reveman wrote:
> > Except when it wraps around. Consider adding a GetNextBufferId utility
> function
> > that calls g_next_buffer_id.GetNext() multiple times if necessary.
> > 
> > Btw, why does this need to be a non-zero?
> 
> This is copy/pasted from just above in this file. I don't think there is any
> hard reason why it couldn't be zero other than by convention of zero being a
> null object.

There's already an explicit NULL value for GpuMemoryBufferHandle. I think we
should clean this up by making 0 a valid id. You can do that in a separate patch
or in this patch. Whatever you prefer.

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:28: class
GpuChannelManagerMessageFilter : public IPC::MessageFilter {
On 2014/09/04 21:14:47, alexst wrote:
> On 2014/09/04 19:43:31, reveman wrote:
> > I would much prefer if this "allocation on IO thread" change was done in a
> > separate patch.
> 
> I originally started writing these as two separate patches, but decided to
merge
> them because one gives much needed context to the other.
> 
> Servicing this on IO in the GPU process is part of the design for the browser
> side UI thread blocking for as little as possible. I felt there is benefit in
> having the end-to-end interaction in one patch.
> 
> That said, if you feel strongly about the two parts being separate, I'll split
> them up.

Ok, makes sense. You can include it in this patch if you like.

https://codereview.chromium.org/540443002/diff/120001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):

https://codereview.chromium.org/540443002/diff/120001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:20:
AllocateOzoneNativeBufferForBrowser(size, internalformat, usage);
Please make it:

scoped_ptr<GpuMemoryBufferImplOzoneNativeBuffer> buffer(
    new GpuMemoryBufferImplOzoneNativeBuffer(size, internalformat));
if (!buffer->Initialize())
  return scoped_ptr<GpuMemoryBufferImpl>();

and adjust GpuMemoryBufferImplOzoneNativeBuffer accordingly. This code should
not really have to be aware of a browser process. All it knows is that it's
initializing an in-process GpuMemoryBufferImpl instance.

If there's a good reason to change this convention then we should change it for
all implementations.

https://codereview.chromium.org/540443002/diff/120001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/540443002/diff/120001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:76: }
nit: }  // namespace

fyi, you'll probably see cl format kill this blank line you added otherwise as
it thinks the namespace is an array or something.. :)

[piman, 2014-09-04 23:49:56]

On Thu, Sep 4, 2014 at 3:37 PM, <reveman@chromium.org> wrote:

>
> https://codereview.chromium.org/540443002/diff/100001/
> content/common/gpu/client/gpu_memory_buffer_factory_host.h
> File content/common/gpu/client/gpu_memory_buffer_factory_host.h (right):
>
> https://codereview.chromium.org/540443002/diff/100001/
> content/common/gpu/client/gpu_memory_buffer_factory_host.h#newcode38
> content/common/gpu/client/gpu_memory_buffer_factory_host.h:38: unsigned
> usage) = 0;
> On 2014/09/04 19:59:20, piman (OOO) wrote:
>
>> On 2014/09/04 19:43:31, reveman wrote:
>> > Not a fan of this alternative function for doing this. Is there a
>>
> way we can
>
>> > just block on the browser UI thread and have everything on the IO
>>
> thread work
>
>> > exactly the same?
>>
>
>  We should not block the UI thread except when we really need to. In
>>
> particular,
>
>> we don't need to block in response to the renderer's messages, and so
>>
> we
>
>> shouldn't.
>>
>
> Agree. I guess my question was, can we make this interface be something
> that is used on the IO thread instead?
>
> We'd have the renderer usage be completely asynchronous on the browser
> side and all work done on the IO thread. While the browser would use the
> exact same code on the IO thread, just block on the reply on the UI
> thread. I think that would be much cleaner if possible.


OK, +1 on that.


>
>
> https://codereview.chromium.org/540443002/diff/100001/
> content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
> File
> content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
> (right):
>
> https://codereview.chromium.org/540443002/diff/100001/
> content/common/gpu/client/gpu_memory_buffer_impl_ozone_
> native_buffer.cc#newcode55
> content/common/gpu/client/gpu_memory_buffer_impl_ozone_
> native_buffer.cc:55:
> // +1 ensures we always get non-zero IDs.
> On 2014/09/04 21:14:47, alexst wrote:
>
>> On 2014/09/04 19:43:31, reveman wrote:
>> > Except when it wraps around. Consider adding a GetNextBufferId
>>
> utility
>
>> function
>> > that calls g_next_buffer_id.GetNext() multiple times if necessary.
>> >
>> > Btw, why does this need to be a non-zero?
>>
>
>  This is copy/pasted from just above in this file. I don't think there
>>
> is any
>
>> hard reason why it couldn't be zero other than by convention of zero
>>
> being a
>
>> null object.
>>
>
> There's already an explicit NULL value for GpuMemoryBufferHandle. I
> think we should clean this up by making 0 a valid id. You can do that in
> a separate patch or in this patch. Whatever you prefer.
>
>
> https://codereview.chromium.org/540443002/diff/100001/
> content/common/gpu/gpu_channel_manager.cc
> File content/common/gpu/gpu_channel_manager.cc (right):
>
> https://codereview.chromium.org/540443002/diff/100001/
> content/common/gpu/gpu_channel_manager.cc#newcode28
> content/common/gpu/gpu_channel_manager.cc:28: class
> GpuChannelManagerMessageFilter : public IPC::MessageFilter {
> On 2014/09/04 21:14:47, alexst wrote:
>
>> On 2014/09/04 19:43:31, reveman wrote:
>> > I would much prefer if this "allocation on IO thread" change was
>>
> done in a
>
>> > separate patch.
>>
>
>  I originally started writing these as two separate patches, but
>>
> decided to merge
>
>> them because one gives much needed context to the other.
>>
>
>  Servicing this on IO in the GPU process is part of the design for the
>>
> browser
>
>> side UI thread blocking for as little as possible. I felt there is
>>
> benefit in
>
>> having the end-to-end interaction in one patch.
>>
>
>  That said, if you feel strongly about the two parts being separate,
>>
> I'll split
>
>> them up.
>>
>
> Ok, makes sense. You can include it in this patch if you like.
>
> https://codereview.chromium.org/540443002/diff/120001/
> content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc
> File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):
>
> https://codereview.chromium.org/540443002/diff/120001/
> content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc#newcode20
> content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:20:
> AllocateOzoneNativeBufferForBrowser(size, internalformat, usage);
> Please make it:
>
> scoped_ptr<GpuMemoryBufferImplOzoneNativeBuffer> buffer(
>     new GpuMemoryBufferImplOzoneNativeBuffer(size, internalformat));
> if (!buffer->Initialize())
>   return scoped_ptr<GpuMemoryBufferImpl>();
>
> and adjust GpuMemoryBufferImplOzoneNativeBuffer accordingly. This code
> should not really have to be aware of a browser process. All it knows is
> that it's initializing an in-process GpuMemoryBufferImpl instance.
>
> If there's a good reason to change this convention then we should change
> it for all implementations.
>
> https://codereview.chromium.org/540443002/diff/120001/
> content/common/gpu/gpu_channel_manager.cc
> File content/common/gpu/gpu_channel_manager.cc (right):
>
> https://codereview.chromium.org/540443002/diff/120001/
> content/common/gpu/gpu_channel_manager.cc#newcode76
> content/common/gpu/gpu_channel_manager.cc:76: }
> nit: }  // namespace
>
> fyi, you'll probably see cl format kill this blank line you added
> otherwise as it thinks the namespace is an array or something.. :)
>
> https://codereview.chromium.org/540443002/
>

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

[alexst (slow to review), 2014-09-05 02:04:53]

> > Agree. I guess my question was, can we make this interface be something
> > that is used on the IO thread instead?
> >
> > We'd have the renderer usage be completely asynchronous on the browser
> > side and all work done on the IO thread. While the browser would use the
> > exact same code on the IO thread, just block on the reply on the UI
> > thread. I think that would be much cleaner if possible.
> 
> 
> OK, +1 on that.
> 

I agree with this too, but I would suggest that this is better left to a
separate CL since moving renderer allocations to IO is outside the scope of this
particular change.

We had renderer allocations on browser IO at one point, then moved them to UI. I
don't remember exactly why and would like to track it down before we move them
back. We will still need two separate functions for UI and IO and probably some
other stuff too, definitely a whole new CL's worth.

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:55: //
+1 ensures we always get non-zero IDs.
On 2014/09/04 22:37:24, reveman wrote:
> On 2014/09/04 21:14:47, alexst wrote:
> > On 2014/09/04 19:43:31, reveman wrote:
> > > Except when it wraps around. Consider adding a GetNextBufferId utility
> > function
> > > that calls g_next_buffer_id.GetNext() multiple times if necessary.
> > > 
> > > Btw, why does this need to be a non-zero?
> > 
> > This is copy/pasted from just above in this file. I don't think there is any
> > hard reason why it couldn't be zero other than by convention of zero being a
> > null object.
> 
> There's already an explicit NULL value for GpuMemoryBufferHandle. I think we
> should clean this up by making 0 a valid id. You can do that in a separate
patch
> or in this patch. Whatever you prefer.

Done.

https://codereview.chromium.org/540443002/diff/120001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):

https://codereview.chromium.org/540443002/diff/120001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:20:
AllocateOzoneNativeBufferForBrowser(size, internalformat, usage);
On 2014/09/04 22:37:24, reveman wrote:
> Please make it:
> 
> scoped_ptr<GpuMemoryBufferImplOzoneNativeBuffer> buffer(
>     new GpuMemoryBufferImplOzoneNativeBuffer(size, internalformat));
> if (!buffer->Initialize())
>   return scoped_ptr<GpuMemoryBufferImpl>();
> 
> and adjust GpuMemoryBufferImplOzoneNativeBuffer accordingly. This code should
> not really have to be aware of a browser process. All it knows is that it's
> initializing an in-process GpuMemoryBufferImpl instance.
> 
> If there's a good reason to change this convention then we should change it
for
> all implementations.

I see what you meant now, sounds good.

I added usage to Initialize() because it's not stored on GpuMemoryBuffer.
Alternatively we need to plumb it into the constructors for all the types if you
want to keep Initialize argument free.

https://codereview.chromium.org/540443002/diff/120001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/540443002/diff/120001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:76: }
On 2014/09/04 22:37:24, reveman wrote:
> nit: }  // namespace
> 
> fyi, you'll probably see cl format kill this blank line you added otherwise as
> it thinks the namespace is an array or something.. :)

you are right, formatter ate it.

[reveman, 2014-09-05 16:36:32]

On 2014/09/05 02:04:53, alexst wrote:
> > > Agree. I guess my question was, can we make this interface be something
> > > that is used on the IO thread instead?
> > >
> > > We'd have the renderer usage be completely asynchronous on the browser
> > > side and all work done on the IO thread. While the browser would use the
> > > exact same code on the IO thread, just block on the reply on the UI
> > > thread. I think that would be much cleaner if possible.
> > 
> > 
> > OK, +1 on that.
> > 
> 
> I agree with this too, but I would suggest that this is better left to a
> separate CL since moving renderer allocations to IO is outside the scope of
this
> particular change.
> 
> We had renderer allocations on browser IO at one point, then moved them to UI.
I
> don't remember exactly why and would like to track it down before we move them
> back. We will still need two separate functions for UI and IO and probably
some
> other stuff too, definitely a whole new CL's worth.

Yes, I did that so that all allocations would be done on the same thread. It was
either the IO or the UI thread but I failed to understand at that point that we
needed these GPU process side allocated buffers to be created synchronously for
the browser compositor. My bad so here's a patch moves all this over to the IO
thread instead:

https://codereview.chromium.org/542083002/

> 
>
https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
> File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
> (right):
> 
>
https://codereview.chromium.org/540443002/diff/100001/content/common/gpu/clie...
> content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:55: //
> +1 ensures we always get non-zero IDs.
> On 2014/09/04 22:37:24, reveman wrote:
> > On 2014/09/04 21:14:47, alexst wrote:
> > > On 2014/09/04 19:43:31, reveman wrote:
> > > > Except when it wraps around. Consider adding a GetNextBufferId utility
> > > function
> > > > that calls g_next_buffer_id.GetNext() multiple times if necessary.
> > > > 
> > > > Btw, why does this need to be a non-zero?
> > > 
> > > This is copy/pasted from just above in this file. I don't think there is
any
> > > hard reason why it couldn't be zero other than by convention of zero being
a
> > > null object.
> > 
> > There's already an explicit NULL value for GpuMemoryBufferHandle. I think we
> > should clean this up by making 0 a valid id. You can do that in a separate
> patch
> > or in this patch. Whatever you prefer.
> 
> Done.
> 
>
https://codereview.chromium.org/540443002/diff/120001/content/common/gpu/clie...
> File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):
> 
>
https://codereview.chromium.org/540443002/diff/120001/content/common/gpu/clie...
> content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:20:
> AllocateOzoneNativeBufferForBrowser(size, internalformat, usage);
> On 2014/09/04 22:37:24, reveman wrote:
> > Please make it:
> > 
> > scoped_ptr<GpuMemoryBufferImplOzoneNativeBuffer> buffer(
> >     new GpuMemoryBufferImplOzoneNativeBuffer(size, internalformat));
> > if (!buffer->Initialize())
> >   return scoped_ptr<GpuMemoryBufferImpl>();
> > 
> > and adjust GpuMemoryBufferImplOzoneNativeBuffer accordingly. This code
should
> > not really have to be aware of a browser process. All it knows is that it's
> > initializing an in-process GpuMemoryBufferImpl instance.
> > 
> > If there's a good reason to change this convention then we should change it
> for
> > all implementations.
> 
> I see what you meant now, sounds good.
> 
> I added usage to Initialize() because it's not stored on GpuMemoryBuffer.
> Alternatively we need to plumb it into the constructors for all the types if
you
> want to keep Initialize argument free.
> 
>
https://codereview.chromium.org/540443002/diff/120001/content/common/gpu/gpu_...
> File content/common/gpu/gpu_channel_manager.cc (right):
> 
>
https://codereview.chromium.org/540443002/diff/120001/content/common/gpu/gpu_...
> content/common/gpu/gpu_channel_manager.cc:76: }
> On 2014/09/04 22:37:24, reveman wrote:
> > nit: }  // namespace
> > 
> > fyi, you'll probably see cl format kill this blank line you added otherwise
as
> > it thinks the namespace is an array or something.. :)
> 
> you are right, formatter ate it.

[alexst (slow to review), 2014-09-09 15:41:17]

Re-factored based on Dave's patch.

GPU is the same, browser is different. Please have another look.

[reveman, 2014-09-09 17:18:26]

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

https://codereview.chromium.org/540443002/diff/180001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:509: void
BrowserGpuChannelHostFactory::GpuMemoryBufferImplCreatedOnIO(
I think this should be named "OnGpuMemoryBufferCreated" or
"OnGpuMemoryBufferAllocated". There's obviously a conflict if using "Created"
which I think we need to address no matter what. It's getting pretty confusing
to have this class implement both the GpuMemoryBuffer part of
GpuChannelHostFactory and GpuMemoryBufferFactoryHost. How about we move the
GpuMemoryBufferFactoryHost part out into a GpuMemoryBufferFactoryHostImpl class?

https://codereview.chromium.org/540443002/diff/180001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:512:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
nit: blank line after this DCHECK to be consistent in style with other
GpuMemoryBuffer related functions.

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

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.h:21: CreationCallback;
nit: sort these typedefs in the order they are used below. Creation before
Allocation.

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

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_android.cc:21: if
(buffer->Initialize()) {
I think we should replace the Initialize convention with a static ::Create
function (taking the same parameters as GpuMemoryBufferImpl::Create) now that
some implementations might be doing GPU process side allocation. This will allow
us to keep the usage of each implementation more consistent.

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

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:22: }
As mentioned in the other comment. I think this would be better implemented as a
GpuMemoryBufferImplOzoneNativeBuffer::Create function.

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

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:36:
handle.global_id.primary_id = g_next_buffer_id.GetNext();
thanks!

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:28: class
GpuChannelManagerMessageFilter : public IPC::MessageFilter {
Just to be be sure. If we ever wanted to handle some other non-gpumemorybuffer
related message on the IO thread then this same filter would be used, right?

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:92:
gpu_memory_buffer_factory_(GpuMemoryBufferFactory::Create()),
could the ownership of the factory move to GpuChannelManagerMessageFilter
instead?

[piman, 2014-09-09 20:00:10]

lgtm

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:28: class
GpuChannelManagerMessageFilter : public IPC::MessageFilter {
On 2014/09/09 17:18:26, reveman wrote:
> Just to be be sure. If we ever wanted to handle some other non-gpumemorybuffer
> related message on the IO thread then this same filter would be used, right?

I think so. Filters have a non-0 cost, we're seeing this in the browser process
on android - each message has to be tested against each filter, and that
interaction tends to grow in O(n^2) as the complexity of the system increases.
So in general, if practical, keeping a single filter seems reasonable.

[alexst (slow to review), 2014-09-09 21:53:57]

new patch.

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

https://codereview.chromium.org/540443002/diff/180001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:509: void
BrowserGpuChannelHostFactory::GpuMemoryBufferImplCreatedOnIO(
On 2014/09/09 17:18:26, reveman wrote:
> I think this should be named "OnGpuMemoryBufferCreated" or
> "OnGpuMemoryBufferAllocated". There's obviously a conflict if using "Created"
> which I think we need to address no matter what. It's getting pretty confusing
> to have this class implement both the GpuMemoryBuffer part of
> GpuChannelHostFactory and GpuMemoryBufferFactoryHost. How about we move the
> GpuMemoryBufferFactoryHost part out into a GpuMemoryBufferFactoryHostImpl
class?
> 
> 

Yeah, the namespace is getting crowded, I'll add GpuMemoryBufferFactoryHostImpl.

https://codereview.chromium.org/540443002/diff/180001/content/browser/gpu/bro...
content/browser/gpu/browser_gpu_channel_host_factory.cc:512:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2014/09/09 17:18:26, reveman wrote:
> nit: blank line after this DCHECK to be consistent in style with other
> GpuMemoryBuffer related functions.

Done.

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

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.h:21: CreationCallback;
On 2014/09/09 17:18:26, reveman wrote:
> nit: sort these typedefs in the order they are used below. Creation before
> Allocation.

Done.

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

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_android.cc:21: if
(buffer->Initialize()) {
On 2014/09/09 17:18:26, reveman wrote:
> I think we should replace the Initialize convention with a static ::Create
> function (taking the same parameters as GpuMemoryBufferImpl::Create) now that
> some implementations might be doing GPU process side allocation. This will
allow
> us to keep the usage of each implementation more consistent.

Done.

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:28: class
GpuChannelManagerMessageFilter : public IPC::MessageFilter {
On 2014/09/09 17:18:26, reveman wrote:
> Just to be be sure. If we ever wanted to handle some other non-gpumemorybuffer
> related message on the IO thread then this same filter would be used, right?

Yes, I don't see why not.

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:92:
gpu_memory_buffer_factory_(GpuMemoryBufferFactory::Create()),
On 2014/09/09 17:18:26, reveman wrote:
> could the ownership of the factory move to GpuChannelManagerMessageFilter
> instead?

I just tried moving ownership to the filter, but I think having it on the
manager makes the code cleaner. We need to call DestroyGpuMemoryBuffer on the
factory from the manager, which means it still needs a ref. Since both classes
need the factory, flipping the relationship of who owns it and who has the ref
just created a bunch of extra code.

[reveman, 2014-09-09 23:17:55]

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/gpu_...
File content/common/gpu/gpu_channel_manager.cc (right):

https://codereview.chromium.org/540443002/diff/180001/content/common/gpu/gpu_...
content/common/gpu/gpu_channel_manager.cc:92:
gpu_memory_buffer_factory_(GpuMemoryBufferFactory::Create()),
On 2014/09/09 21:53:57, alexst wrote:
> On 2014/09/09 17:18:26, reveman wrote:
> > could the ownership of the factory move to GpuChannelManagerMessageFilter
> > instead?
> 
> I just tried moving ownership to the filter, but I think having it on the
> manager makes the code cleaner. We need to call DestroyGpuMemoryBuffer on the
> factory from the manager, which means it still needs a ref. Since both classes
> need the factory, flipping the relationship of who owns it and who has the ref
> just created a bunch of extra code.

Acknowledged.

https://codereview.chromium.org/540443002/diff/240001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_memory_buffer_factory_host_impl.h (right):

https://codereview.chromium.org/540443002/diff/240001/content/browser/gpu/gpu...
content/browser/gpu/gpu_memory_buffer_factory_host_impl.h:21: //
GpuMemoryBufferFactoryHost implementation.
nit: I typically use "Overridden from GpuMemoryBufferFactoryHost:" in
GpuMemoryBuffer related code.

https://codereview.chromium.org/540443002/diff/240001/content/browser/gpu/gpu...
content/browser/gpu/gpu_memory_buffer_factory_host_impl.h:29: int32 sync_point)
OVERRIDE;
nit: please add a blank line here to separate set_gpu_host_id from the functions
that implement the GpuMemoryBufferFactoryHost interface.

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:7: #include
"base/bind.h"
nit: I don't think you need this.

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:57: void
GpuMemoryBufferImplOzoneNativeBuffer::Create(
nit: please order the functions as GpuMemoryBufferImpl. Create,
Allocate*ForChild...

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:62:
GpuMemoryBufferImplOzoneNativeBuffer::AllocateOzoneNativeBufferForChildId(
Not a fan of using a function called *ForChildId here. This is not allocating a
buffer for a child process. It's allocating a buffer for in-process usage, or
for usage by the same client id (note: not child id) as client making the
allocation request.

I think it's fine to use 0 as a unique value for secondary_id to indicate that
it should be owned by the process sending the request or to indicate that the
client with client_id=0 should own it. Not sure we should be making the
assumption that the current process has client_id=0 from this code though..

So I don't think we should be using AllocateOzoneNativeBufferForChildId here but
GpuMemoryBufferFactoryHost::CreateGpuMemoryBuffer directly instead. Maybe you
could have the GPU process fill in the correct client_id to avoid the assumption
that the process using this API has client_id=0.

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h
(right):

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:31: const
CreationCallback& callback);
nit: please order the functions as GpuMemoryBufferImpl. Create,
Allocate*ForChild...

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.cc (right):

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.cc:41: void
GpuMemoryBufferImplSharedMemory::Create(const gfx::Size& size,
nit: please order the functions as GpuMemoryBufferImpl. Create,
Allocate*ForChild...

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h (right):

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h:27: static void
Create(const gfx::Size& size,
nit: please order the functions as GpuMemoryBufferImpl. Create,
Allocate*ForChild...

[alexst (slow to review), 2014-09-10 01:57:50]

https://codereview.chromium.org/540443002/diff/240001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_memory_buffer_factory_host_impl.h (right):

https://codereview.chromium.org/540443002/diff/240001/content/browser/gpu/gpu...
content/browser/gpu/gpu_memory_buffer_factory_host_impl.h:21: //
GpuMemoryBufferFactoryHost implementation.
On 2014/09/09 23:17:54, reveman wrote:
> nit: I typically use "Overridden from GpuMemoryBufferFactoryHost:" in
> GpuMemoryBuffer related code.

Done.

https://codereview.chromium.org/540443002/diff/240001/content/browser/gpu/gpu...
content/browser/gpu/gpu_memory_buffer_factory_host_impl.h:29: int32 sync_point)
OVERRIDE;
On 2014/09/09 23:17:54, reveman wrote:
> nit: please add a blank line here to separate set_gpu_host_id from the
functions
> that implement the GpuMemoryBufferFactoryHost interface.

Done.

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:7: #include
"base/bind.h"
On 2014/09/09 23:17:54, reveman wrote:
> nit: I don't think you need this.

right, missed one, thanks.

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:57: void
GpuMemoryBufferImplOzoneNativeBuffer::Create(
On 2014/09/09 23:17:54, reveman wrote:
> nit: please order the functions as GpuMemoryBufferImpl. Create,
> Allocate*ForChild...

Done.

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:62:
GpuMemoryBufferImplOzoneNativeBuffer::AllocateOzoneNativeBufferForChildId(
On 2014/09/09 23:17:54, reveman wrote:
> Not a fan of using a function called *ForChildId here. This is not allocating
a
> buffer for a child process. It's allocating a buffer for in-process usage, or
> for usage by the same client id (note: not child id) as client making the
> allocation request.
> 
> I think it's fine to use 0 as a unique value for secondary_id to indicate that
> it should be owned by the process sending the request or to indicate that the
> client with client_id=0 should own it. Not sure we should be making the
> assumption that the current process has client_id=0 from this code though..
> 
> So I don't think we should be using AllocateOzoneNativeBufferForChildId here
but
> GpuMemoryBufferFactoryHost::CreateGpuMemoryBuffer directly instead. Maybe you
> could have the GPU process fill in the correct client_id to avoid the
assumption
> that the process using this API has client_id=0.

I just used it as a shortcut, refactored to call
GpuMemoryBufferFactoryHost::CreateGpuMemoryBuffer directly. I used
secondary_id=0 to indicate in-process use.

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h
(right):

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:31: const
CreationCallback& callback);
On 2014/09/09 23:17:54, reveman wrote:
> nit: please order the functions as GpuMemoryBufferImpl. Create,
> Allocate*ForChild...

Done.

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.cc (right):

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.cc:41: void
GpuMemoryBufferImplSharedMemory::Create(const gfx::Size& size,
On 2014/09/09 23:17:54, reveman wrote:
> nit: please order the functions as GpuMemoryBufferImpl. Create,
> Allocate*ForChild...

Done.

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h (right):

https://codereview.chromium.org/540443002/diff/240001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.h:27: static void
Create(const gfx::Size& size,
On 2014/09/09 23:17:54, reveman wrote:
> nit: please order the functions as GpuMemoryBufferImpl. Create,
> Allocate*ForChild...

Done.

[reveman, 2014-09-10 02:34:06]

thanks! lgtm after adding a comment

https://codereview.chromium.org/540443002/diff/260001/content/common/gpu/clie...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/540443002/diff/260001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:45:
handle.global_id.secondary_id = 0;
This is fine for now but please add a short comment that makes it clear that
this code is making an assumption that client_id for the GPU channel that would
be using this API is 0. This would not work if client_id was something else. ie.
if the browser process was using multiple GPU channels. Not the case right now
and maybe never but it does seem a bit inappropriate to make this assumption
here.

[alexst (slow to review), 2014-09-10 03:10:40]

The CQ bit was checked by alexst@chromium.org

[commit-bot: I haz the power, 2014-09-10 03:12:07]

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

[alexst (slow to review), 2014-09-10 03:24:04]

On 2014/09/10 02:34:06, reveman wrote:
> thanks! lgtm after adding a comment
> 
>
https://codereview.chromium.org/540443002/diff/260001/content/common/gpu/clie...
> File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
> (right):
> 
>
https://codereview.chromium.org/540443002/diff/260001/content/common/gpu/clie...
> content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:45:
> handle.global_id.secondary_id = 0;
> This is fine for now but please add a short comment that makes it clear that
> this code is making an assumption that client_id for the GPU channel that
would
> be using this API is 0. This would not work if client_id was something else.
ie.
> if the browser process was using multiple GPU channels. Not the case right now
> and maybe never but it does seem a bit inappropriate to make this assumption
> here.

Thanks!

I know, it's kinda fragile. I'll do some digging to see how we can get rid of
this assumption in a more robust way.

[commit-bot: I haz the power, 2014-09-10 05:18:07]

Committed patchset #14 (id:280001) as b5fe45963a55e00b2b6ebe5e0f6c198a5452ed5e

[commit-bot: I haz the power, 2014-09-10 05:27:08]

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