Browser side OZONE_NATIVE_BUFFER allocation

Browser side OZONE_NATIVE_BUFFER allocation.

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

[reveman, 2014-08-11 19:43:06]

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.cc:307: case
gfx::OZONE_NATIVE_BUFFER:
ifdef OZONE missing?

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

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:34: // be used to talk to the
GPU process.
I'm not sure we should mention GPU process here. From this interface's pov I
prefer to simply think of it as out-of-process allocation of buffers.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:40: int client_id,
does child_id make more sense here? this is the first mention of "client" here
otherwise..

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:41:
GpuMemoryBufferFactoryHost* gpu_factory_host,
The implementation of this interface should not be aware of the existence of a
GPU process. Just |factory| or |factory_host| would be better imo.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_linux.cc (right):

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_linux.cc:11: #if
defined(USE_OZONE)
No idefs in these files. Please add a platform specific file for ozone instead
of using _linux.cc. It should be named gpu_memory_buffer_impl_ozone.cc.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_linux.cc:32: #endif  //
defined(USE_OZONE)
do we need browser compositor support for this?

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:7: #include
"base/numerics/safe_math.h"
I don't think you need this.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:39: handle, size,
internalformat, GL_IMAGE_SCANOUT_CHROMIUM, callback);
You should pass usage as a parameter to this function and
DCHECK(IsUsageSupported(usage)) instead as you have a IsUsageSupported function.
That way adding new supported usage types is more straight forward.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:45: return true;
IsFormatSupported instead, if any size is supported. And please list each
supported format explicitly in a switch statement. That way we can add a new
format without having to update this code.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:66:
handle_.global_id.primary_id = GetNextBufferId();
I don't understand how this would work. How is the buffer actually allocated?

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.h (right):

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.h:1: // Copyright 2013
The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.h:12: // Implementation
of GPU memory buffer based on shared memory.
comment is incorrect.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.h:13: class
GpuMemoryBufferImplOzone : public GpuMemoryBufferImpl {
I don't think this is the correct name for this class.
GpuMemoryBufferImplOzoneNativeBuffer is better as this is only one supported
type on ozone platform.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.h:17: int32 client_id);
client_id? I think you should init this from InitializeFromHandle instead.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.h:48: static uint32_t
GetNextBufferId();
can you move this and next_buffer_id_ to anonymous namespace in the .cc file
instead?

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.h:50:
gfx::GpuMemoryBufferHandle handle_;
I think gfx::GpuMemoryBufferId is more appropriate.

[achaulk, 2014-08-12 19:03:58]

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_channel_host.cc:307: case
gfx::OZONE_NATIVE_BUFFER:
On 2014/08/11 19:43:05, reveman wrote:
> ifdef OZONE missing?

Done.

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

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:34: // be used to talk to the
GPU process.
On 2014/08/11 19:43:05, reveman wrote:
> I'm not sure we should mention GPU process here. From this interface's pov I
> prefer to simply think of it as out-of-process allocation of buffers.

Acknowledged.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:40: int client_id,
On 2014/08/11 19:43:05, reveman wrote:
> does child_id make more sense here? this is the first mention of "client" here
> otherwise..

Probably yeah

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:41:
GpuMemoryBufferFactoryHost* gpu_factory_host,
On 2014/08/11 19:43:05, reveman wrote:
> The implementation of this interface should not be aware of the existence of a
> GPU process. Just |factory| or |factory_host| would be better imo. 

Done.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_linux.cc (right):

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_linux.cc:11: #if
defined(USE_OZONE)
On 2014/08/11 19:43:05, reveman wrote:
> No idefs in these files. Please add a platform specific file for ozone instead
> of using _linux.cc. It should be named gpu_memory_buffer_impl_ozone.cc.

Ok

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_linux.cc:32: #endif  //
defined(USE_OZONE)
On 2014/08/11 19:43:05, reveman wrote:
> do we need browser compositor support for this?

Not for using them. We will eventually to render the main scene into an overlay.
They get allocated via CreateImageChromium and there's already support for that

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:7: #include
"base/numerics/safe_math.h"
On 2014/08/11 19:43:05, reveman wrote:
> I don't think you need this.

Done.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:39: handle, size,
internalformat, GL_IMAGE_SCANOUT_CHROMIUM, callback);
On 2014/08/11 19:43:05, reveman wrote:
> You should pass usage as a parameter to this function and
> DCHECK(IsUsageSupported(usage)) instead as you have a IsUsageSupported
function.
> That way adding new supported usage types is more straight forward.

Isn't that what IsConfigurationSupported is for?

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:45: return true;
On 2014/08/11 19:43:05, reveman wrote:
> IsFormatSupported instead, if any size is supported. And please list each
> supported format explicitly in a switch statement. That way we can add a new
> format without having to update this code.

Done.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:66:
handle_.global_id.primary_id = GetNextBufferId();
On 2014/08/11 19:43:06, reveman wrote:
> I don't understand how this would work. How is the buffer actually allocated?

This is for the browser path which we haven't finalized yet

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.h (right):

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.h:12: // Implementation
of GPU memory buffer based on shared memory.
On 2014/08/11 19:43:06, reveman wrote:
> comment is incorrect.

Done.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.h:13: class
GpuMemoryBufferImplOzone : public GpuMemoryBufferImpl {
On 2014/08/11 19:43:06, reveman wrote:
> I don't think this is the correct name for this class.
> GpuMemoryBufferImplOzoneNativeBuffer is better as this is only one supported
> type on ozone platform.

Done.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.h:17: int32 client_id);
On 2014/08/11 19:43:06, reveman wrote:
> client_id? I think you should init this from InitializeFromHandle instead.

Done.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.h:48: static uint32_t
GetNextBufferId();
On 2014/08/11 19:43:06, reveman wrote:
> can you move this and next_buffer_id_ to anonymous namespace in the .cc file
> instead?

Done.

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.h:50:
gfx::GpuMemoryBufferHandle handle_;
On 2014/08/11 19:43:06, reveman wrote:
> I think gfx::GpuMemoryBufferId is more appropriate.

Done.

[reveman, 2014-08-12 20:19:12]

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_linux.cc (right):

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_linux.cc:32: #endif  //
defined(USE_OZONE)
On 2014/08/12 19:03:57, achaulk wrote:
> On 2014/08/11 19:43:05, reveman wrote:
> > do we need browser compositor support for this?
> 
> Not for using them. We will eventually to render the main scene into an
overlay.
> They get allocated via CreateImageChromium and there's already support for
that

I'm failing to see how this works. Latest patch is just generating an id by
assuming that the client id for the browser is 1 but not telling the GPU process
to allocate any storage. Can you explain a bit more how this is supposed to
work?

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_linux.cc (right):

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_linux.cc:14: #endif  //
defined(USE_OZONE)
remove all this

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc (right):

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
2014

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:14:
uint32_t GetNextBufferId() {
is this function worth having?

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:42:
handle, size, internalformat, usage, callback);
Hm, it's a bit nasty to use a GpuMemoryBufferImpl::AllocationCallback as a
GpuMemoryBufferFactoryHost::CreateGpuMemoryBufferCallback. I'd prefer if we had
an intermediate callback function as part of this class that ran the
AllocationCallback.

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:50: case
GL_RGB8_OES:
GL_RGB8_OES is not supported by the GpuMemoryBufferImpl base class. If we need
to support this format then let's add it in a separate patch instead.

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:69:
const gfx::Size& size,
please remove |size| parameter if unused.

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h
(right):

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:45:
DISALLOW_COPY_AND_ASSIGN(GpuMemoryBufferImplOzoneNativeBuffer);
nit: move this to the bottom for consistency

[achaulk, 2014-08-12 21:38:03]

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_linux.cc (right):

https://codereview.chromium.org/458313002/diff/40001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_linux.cc:32: #endif  //
defined(USE_OZONE)
On 2014/08/12 20:19:11, reveman wrote:
> On 2014/08/12 19:03:57, achaulk wrote:
> > On 2014/08/11 19:43:05, reveman wrote:
> > > do we need browser compositor support for this?
> > 
> > Not for using them. We will eventually to render the main scene into an
> overlay.
> > They get allocated via CreateImageChromium and there's already support for
> that
> 
> I'm failing to see how this works. Latest patch is just generating an id by
> assuming that the client id for the browser is 1 but not telling the GPU
process
> to allocate any storage. Can you explain a bit more how this is supposed to
> work? 

That is something that we will need to figure out later. I'll remove it for now
though since it can be included in another patch where it will be used.

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_linux.cc (right):

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_linux.cc:14: #endif  //
defined(USE_OZONE)
On 2014/08/12 20:19:11, reveman wrote:
> remove all this

Done.

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc
(right):

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:42:
handle, size, internalformat, usage, callback);
On 2014/08/12 20:19:11, reveman wrote:
> Hm, it's a bit nasty to use a GpuMemoryBufferImpl::AllocationCallback as a
> GpuMemoryBufferFactoryHost::CreateGpuMemoryBufferCallback. I'd prefer if we
had
> an intermediate callback function as part of this class that ran the
> AllocationCallback.

Done.

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:50: case
GL_RGB8_OES:
On 2014/08/12 20:19:11, reveman wrote:
> GL_RGB8_OES is not supported by the GpuMemoryBufferImpl base class. If we need
> to support this format then let's add it in a separate patch instead.

Okay, this is only used by the surfaceless code anyway

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:69:
const gfx::Size& size,
On 2014/08/12 20:19:11, reveman wrote:
> please remove |size| parameter if unused.

Done.

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h
(right):

https://codereview.chromium.org/458313002/diff/60001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:45:
DISALLOW_COPY_AND_ASSIGN(GpuMemoryBufferImplOzoneNativeBuffer);
On 2014/08/12 20:19:11, reveman wrote:
> nit: move this to the bottom for consistency

Done.

[reveman, 2014-08-13 08:03:11]

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

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:34: // be used to talk to the
allocator.
Can you merge the mention of |child_id| with the text above instead? ie. "for
|usage| by |child_process| with |child_id|"...

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:40:
GpuMemoryBufferFactoryHost* factory_host,
It's weird that we pass a factory to this function. Makes it seem like we might
need to be able to pass different factory instances to this function. The use of
the factory is supposed to be an implementation detail of this function. I would
prefer if we add a GpuMemoryBufferFactoryHost::GetInstance() function that can
be used when necessary instead. If we want to avoid global variables, then we'd
need to add a GpuMemoryBufferImplFactory and pass the FactoryHost to the ctor of
this new factory instance. Adding GpuMemoryBufferFactoryHost::GetInstance()
function is fine with me for now though.

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

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:12:
namespace {
nit: blank line between "namespace {" and first variable

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:13:
const int kBrowserId = 1;
I don't think this should be part of this patch.

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:16: void
GpuAllocationCallback(
I'd make this a private member of GpuMemoryBufferImplOzoneNativeBuffer to avoid
the need for the GpuMemoryBufferImpl:: prefix.

Can we also rename this to ::OnGpuMemoryBufferCreated. I think that's a bit more
consistent with similar callbacks in content/ and makes it clear that it's
called in response to CreateGpuMemoryBuffer.

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:20: }
nit: blank line between function and "}  // namespace"

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:37:
base::ProcessHandle child_process,
remove this parameter if unused.

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:85: }
remove this function completely as it doesn't work afaict

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h
(left):

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:12: //
Implementation of GPU memory buffer based on shared memory.
Please include a comment like this for consistency.

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

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:19: //
|internalformat| for use by |child_process|.
Please update comment.

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:20:
static void AllocateSharedMemoryForChildProcess(
This is not named properly. AllocateOzoneNativeBufferForChildId?

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:33: bool
Initialize();
remove this function if not supported.

[achaulk, 2014-08-13 15:53:20]

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

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:34: // be used to talk to the
allocator.
On 2014/08/13 08:03:11, reveman wrote:
> Can you merge the mention of |child_id| with the text above instead? ie. "for
> |usage| by |child_process| with |child_id|"...

Done.

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.h:40:
GpuMemoryBufferFactoryHost* factory_host,
On 2014/08/13 08:03:11, reveman wrote:
> It's weird that we pass a factory to this function. Makes it seem like we
might
> need to be able to pass different factory instances to this function. The use
of
> the factory is supposed to be an implementation detail of this function. I
would
> prefer if we add a GpuMemoryBufferFactoryHost::GetInstance() function that can
> be used when necessary instead. If we want to avoid global variables, then
we'd
> need to add a GpuMemoryBufferImplFactory and pass the FactoryHost to the ctor
of
> this new factory instance. Adding GpuMemoryBufferFactoryHost::GetInstance()
> function is fine with me for now though.

Sure, I can do that. The only implementation is a singleton anyway.

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

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:13:
const int kBrowserId = 1;
On 2014/08/13 08:03:11, reveman wrote:
> I don't think this should be part of this patch.

Done.

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:16: void
GpuAllocationCallback(
On 2014/08/13 08:03:11, reveman wrote:
> I'd make this a private member of GpuMemoryBufferImplOzoneNativeBuffer to
avoid
> the need for the GpuMemoryBufferImpl:: prefix.
> 
> Can we also rename this to ::OnGpuMemoryBufferCreated. I think that's a bit
more
> consistent with similar callbacks in content/ and makes it clear that it's
> called in response to CreateGpuMemoryBuffer.

Done.

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:37:
base::ProcessHandle child_process,
On 2014/08/13 08:03:11, reveman wrote:
> remove this parameter if unused.

Done.

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:85: }
On 2014/08/13 08:03:11, reveman wrote:
> remove this function completely as it doesn't work afaict

Done.

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h
(left):

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:12: //
Implementation of GPU memory buffer based on shared memory.
On 2014/08/13 08:03:11, reveman wrote:
> Please include a comment like this for consistency.

Done.

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

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:20:
static void AllocateSharedMemoryForChildProcess(
On 2014/08/13 08:03:11, reveman wrote:
> This is not named properly. AllocateOzoneNativeBufferForChildId?

Done.

https://codereview.chromium.org/458313002/diff/80001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:33: bool
Initialize();
On 2014/08/13 08:03:11, reveman wrote:
> remove this function if not supported.

Done.

[reveman, 2014-08-13 17:00:26]

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

https://codereview.chromium.org/458313002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_factory_host.h:32: static
GpuMemoryBufferFactoryHost* GetInstance();
nit: I'd move this static method above all virtual member functions.

https://codereview.chromium.org/458313002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_factory_host.h:32: static
GpuMemoryBufferFactoryHost* GetInstance();
Don't we need a InitInstance or SetInstance too? and where is the implementation
of this?

https://codereview.chromium.org/458313002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_factory_host.h:36: virtual
~GpuMemoryBufferFactoryHost();
hm, why do you need to change this?

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

https://codereview.chromium.org/458313002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:35:
handle.global_id.primary_id = g_next_buffer_id++;
Note: this assumes that this function will only be called from one thread as
usage of g_next_buffer_id is not thread safe. Fine but might be worth adding a
comment here.

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

https://codereview.chromium.org/458313002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.h:20: //
|internalformat| with usage |usage| for use by |child_id|.
nit: s/acked/backed/ and maybe "for |usage| by child process identified by
|child_id|"

[achaulk, 2014-08-13 17:10:13]

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

https://codereview.chromium.org/458313002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_factory_host.h:32: static
GpuMemoryBufferFactoryHost* GetInstance();
On 2014/08/13 17:00:25, reveman wrote:
> Don't we need a InitInstance or SetInstance too? and where is the
implementation
> of this?

Forgot to add the new file. I'm currently just doing the initialization in the
ctor, which matches how BrowserGpuChannelHostFactory does it

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

https://codereview.chromium.org/458313002/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:35:
handle.global_id.primary_id = g_next_buffer_id++;
On 2014/08/13 17:00:26, reveman wrote:
> Note: this assumes that this function will only be called from one thread as
> usage of g_next_buffer_id is not thread safe. Fine but might be worth adding a
> comment here.

Done.

[reveman, 2014-08-13 17:47:42]

lgtm

[achaulk, 2014-08-13 23:44:47]

+piman for owners review

[piman, 2014-08-15 18:07:38]

LGTM+nit

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

https://codereview.chromium.org/458313002/diff/120001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:15:
base::subtle::Atomic32 g_next_buffer_id = 1;
nit: use StaticAtomicSequenceNumber instead of rolling your own.

[achaulk, 2014-08-15 18:45:23]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-08-15 18:48:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/achaulk@chromium.org/458313002/140001

[commit-bot: I haz the power, 2014-08-15 22:17:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-15 22:28:10]

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

[commit-bot: I haz the power, 2014-08-15 22:28:11]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[achaulk, 2014-08-18 17:06:03]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-08-18 17:06:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/achaulk@chromium.org/458313002/140001

[commit-bot: I haz the power, 2014-08-18 18:19:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-18 18:30:42]

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

[commit-bot: I haz the power, 2014-08-18 18:30:43]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[achaulk, 2014-08-18 20:14:00]

The CQ bit was checked by achaulk@chromium.org

[commit-bot: I haz the power, 2014-08-18 20:15:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/achaulk@chromium.org/458313002/160001

[commit-bot: I haz the power, 2014-08-18 21:54:26]

Committed patchset #9 (160001) as 290368