Add multi-process GpuMemoryBuffer framework.

Add multi-process GpuMemoryBuffer framework.

This adds a multi-process framework for reading/writing directly to
memory that the 3D graphics hardware can use for rendering without
any costly copying having to be done on the GPU process side.

A GpuMemoryBuffer is a type of shared memory that can be accessed
by the GPU. The high level procedure required to allocate this type
of memory is almost exactly the same as that for standard shared
memory. Only the browser process can allocated the memory and it
needs to be shared and registered with the GPU process before it can
be used.

This also adds a GpuMemoryBuffer type that is backed by standard
shared memory for testing purposes.

TEST=gpu_unittests --gtest_filter=MockGpuMemoryBufferTest.Lifecycle
BUG=261649
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230248

[joth, 2013-07-24 02:13:23]

https://codereview.chromium.org/19762004/diff/3001/android_webview/browser/gp...
File android_webview/browser/gpu_memory_buffer_factory_impl.cc (right):

https://codereview.chromium.org/19762004/diff/3001/android_webview/browser/gp...
android_webview/browser/gpu_memory_buffer_factory_impl.cc:5: #include
"android_webview/browser/gpu_memory_buffer_factory_impl.h"
other than renaming, did anything change in this file? Unfortunately the diff
didn't seem to work out that this was a move.

[joth, 2013-07-24 02:15:23]

On 2013/07/24 02:13:23, joth wrote:
>
https://codereview.chromium.org/19762004/diff/3001/android_webview/browser/gp...
> File android_webview/browser/gpu_memory_buffer_factory_impl.cc (right):
> 
>
https://codereview.chromium.org/19762004/diff/3001/android_webview/browser/gp...
> android_webview/browser/gpu_memory_buffer_factory_impl.cc:5: #include
> "android_webview/browser/gpu_memory_buffer_factory_impl.h"
> other than renaming, did anything change in this file? Unfortunately the diff
> didn't seem to work out that this was a move.

sorry I meant to make that comment on the other patch...
https://codereview.chromium.org/20017005/ but same comment applies :)

[reveman, 2013-10-19 20:47:01]

This is ready for review. I think it's pretty clear that we will be using this
in the near future, if not for 0-copy, for overlays and/or desktop capture.
Having this in place also makes it easy to test the framework on ChromeOS
devices so I'd like to land this even though we don't have a real working
multi-process 0-copy implementation at the moment. WDYT?

[piman, 2013-10-21 21:56:59]

LGTM + some nits.

https://codereview.chromium.org/19762004/diff/340001/content/browser/gpu/brow...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/19762004/diff/340001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:295: return
scoped_ptr<gfx::GpuMemoryBuffer>();
nit: either DCHECK or test, but not both.
If the contract is that all internalformats passed here are valid (and you've
audited the callers), then keep the DCHECK and remove the test.
If the contract is that this can fail gracefully if the internalformat is
invalid, then remove the DCHECK.

https://codereview.chromium.org/19762004/diff/340001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/19762004/diff/340001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1175: buffer_size,
PeerHandle(), handle);
nit: the intention here is to do more than this, otherwise there's no
justification for the new IPC. Add a TODO to explain the plan here and/or that
it's work in progress?

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/clien...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.cc:370: return true;
Won't this force impl-side painting to go through this path, which removes all
scheduling of texture uploads? Isn't that a bad thing?

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.cc:392: 
nit: can we DCHECK the handle type here?

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.cc:412: return
gpu_memory_buffer.release();
nit: these 2 lines would be a bit clearer if written something like:

gpu_memory_buffers_[new_id] = gpu_memory_buffer.release();
return gpu_memory_buffers_[new_id];

You're transferring ownership to the gpu_memory_buffers_ map. As you have it
written, it looks as if you're transferring ownership to the caller.

Have you considered using linked_ptr instead, that can be put it in a map?

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/clien...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/clien...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:1544: }
nit: DELEGATE_TO_GL_3R should work, right?

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/gpu_m...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/gpu_m...
content/common/gpu/gpu_messages.h:649: // used to identify the gpu memory buffer
from a command buffer.
nit: this doesn't return an id. fix doc?

[reveman, 2013-10-22 16:26:11]

+cdn for review of IPC changes.

https://codereview.chromium.org/19762004/diff/340001/content/browser/gpu/brow...
File content/browser/gpu/browser_gpu_channel_host_factory.cc (right):

https://codereview.chromium.org/19762004/diff/340001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_channel_host_factory.cc:295: return
scoped_ptr<gfx::GpuMemoryBuffer>();
On 2013/10/21 21:57:00, piman wrote:
> nit: either DCHECK or test, but not both.
> If the contract is that all internalformats passed here are valid (and you've
> audited the callers), then keep the DCHECK and remove the test.
> If the contract is that this can fail gracefully if the internalformat is
> invalid, then remove the DCHECK.

Removed the DCHECK.

https://codereview.chromium.org/19762004/diff/340001/content/browser/renderer...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/19762004/diff/340001/content/browser/renderer...
content/browser/renderer_host/render_message_filter.cc:1175: buffer_size,
PeerHandle(), handle);
On 2013/10/21 21:57:00, piman wrote:
> nit: the intention here is to do more than this, otherwise there's no
> justification for the new IPC. Add a TODO to explain the plan here and/or that
> it's work in progress?

Done. I also changed it to return a gfx::GpuMemoryBufferHandle as that's what we
need in the future and I think it makes the purpose of this new IPC more clear.

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/clien...
File content/common/gpu/client/command_buffer_proxy_impl.cc (right):

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.cc:370: return true;
On 2013/10/21 21:57:00, piman wrote:
> Won't this force impl-side painting to go through this path, which removes all
> scheduling of texture uploads? Isn't that a bad thing?

It's disabled by default. You need to use --enable-map-image for this to be
used.

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.cc:392: 
On 2013/10/21 21:57:00, piman wrote:
> nit: can we DCHECK the handle type here?

Hm, I'd prefer to keep this code type agnostic. Can you explain a bit more why
you think this is necessary? ShareGpuMemoryBufferToGpuProcess already has a
NOTREACHED() for invalid types.

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/clien...
content/common/gpu/client/command_buffer_proxy_impl.cc:412: return
gpu_memory_buffer.release();
On 2013/10/21 21:57:00, piman wrote:
> nit: these 2 lines would be a bit clearer if written something like:
> 
> gpu_memory_buffers_[new_id] = gpu_memory_buffer.release();
> return gpu_memory_buffers_[new_id];

switched to this in latest patch.

> 
> You're transferring ownership to the gpu_memory_buffers_ map. As you have it
> written, it looks as if you're transferring ownership to the caller.
> 
> Have you considered using linked_ptr instead, that can be put it in a map?

I think that's a good idea but for consistency we should also change the
TransferBufferMap at the same time and as this will touch a few files I prefer
to do it as a follow up.

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/clien...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/clien...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:1544: }
On 2013/10/21 21:57:00, piman wrote:
> nit: DELEGATE_TO_GL_3R should work, right?

There was no DELEGATE_TO_GL_3R macro before this but I added it to latest patch
as we already have a DELEGATE_TO_GL_4R macro.

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/gpu_m...
File content/common/gpu/gpu_messages.h (right):

https://codereview.chromium.org/19762004/diff/340001/content/common/gpu/gpu_m...
content/common/gpu/gpu_messages.h:649: // used to identify the gpu memory buffer
from a command buffer.
On 2013/10/21 21:57:00, piman wrote:
> nit: this doesn't return an id. fix doc?

Done. Fixed the description of GpuCommandBufferMsg_RegisterTransferBuffer above
too.

[Cris Neckar, 2013-10-22 20:08:52]

IPC changes LGTM

[piman, 2013-10-22 20:29:43]

lgtm

[commit-bot: I haz the power, 2013-10-22 20:56:37]

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

[commit-bot: I haz the power, 2013-10-22 23:13:13]

Change committed as 230248