content: Add command line flag for native GPU memory buffers.

content: Add command line flag for native GPU memory buffers.

This adds a --enable-native-gpu-memory-buffers flag that can be used
to enable native GPU memory buffer support for buffers that are
mapped into the compositor's address space.

BUG=

Committed: https://crrev.com/44b807f10ce0c1de305036932601b4715729d1d6
Cr-Commit-Position: refs/heads/master@{#305765}

[enne (OOO), 2014-11-17 21:02:48]

enne@chromium.org changed reviewers:
+ enne@chromium.org

[enne (OOO), 2014-11-17 21:02:49]

reveman: can you pick a reviewer and get this landed?

[reveman, 2014-11-17 21:19:39]

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

[reveman, 2014-11-17 21:19:40]

+piman

fyi, https://codereview.chromium.org/733053005/ and
https://codereview.chromium.org/731293002/ need to land before this.

[piman, 2014-11-19 02:39:41]

https://codereview.chromium.org/699073004/diff/20001/content/common/gpu/clien...
File content/common/gpu/client/gpu_memory_buffer_impl.cc (right):

https://codereview.chromium.org/699073004/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.cc:59: for (size_t i = 0; i <
arraysize(kTypeNamePairs); ++i) {
nit: range for?

https://codereview.chromium.org/699073004/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl.cc:69: for (size_t i = 0; i <
arraysize(kTypeNamePairs); ++i) {
nit: range for?

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

https://codereview.chromium.org/699073004/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_android.cc:18:
gfx::SURFACE_TEXTURE_BUFFER
I would prefer having the decision about the types of buffers that are allowed
to be in a single place.

Really, there's only 2 cases:
1- shm only
2- a single platform-specific type, with a fallback to shm

Which really simplifies to whether or not the platform-specific type (if any) is
allowed.

Maybe it's not worth it to have a list of supported types... We only ever pick
the first element, which may be overridden anyway (from the command line).

I think it would be simpler to have a gfx::GpuMemoryBufferType
GpuMemoryBufferImpl::GetPreferredType() on each platform, which returns a single
type (e.g. EMPTY_BUFFER if none), that may be controlled with the command-line
(on mac and android for a staged rollout).

Then no need for the lazy-initialization logic, or generic override logic.

[reveman, 2014-11-20 00:53:02]

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

https://codereview.chromium.org/699073004/diff/20001/content/common/gpu/clien...
content/common/gpu/client/gpu_memory_buffer_impl_android.cc:18:
gfx::SURFACE_TEXTURE_BUFFER
On 2014/11/19 02:39:40, piman (Very slow to review) wrote:
> I would prefer having the decision about the types of buffers that are allowed
> to be in a single place.
> 
> Really, there's only 2 cases:
> 1- shm only
> 2- a single platform-specific type, with a fallback to shm
> 
> Which really simplifies to whether or not the platform-specific type (if any)
is
> allowed.
> 
> Maybe it's not worth it to have a list of supported types... We only ever pick
> the first element, which may be overridden anyway (from the command line).
> 
> I think it would be simpler to have a gfx::GpuMemoryBufferType
> GpuMemoryBufferImpl::GetPreferredType() on each platform, which returns a
single
> type (e.g. EMPTY_BUFFER if none), that may be controlled with the command-line
> (on mac and android for a staged rollout).
> 
> Then no need for the lazy-initialization logic, or generic override logic.

I think my other refactor will result in what you're asking for here. Please
review that patch first and I'll rebase this onto that.

[reveman, 2014-11-25 17:58:53]

Rebased this patch on the refactor that was reverted. Please take a look. I'd
like to land this after re-landing the refactor.

[piman, 2014-11-25 18:12:21]

lgtm

https://codereview.chromium.org/699073004/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/699073004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1998:
DCHECK(context_provider->ContextCapabilities().gpu.texture_rectangle);
Would it make sense to ask the GpuMemoryBufferImpl which texture target it
should use?

[reveman, 2014-11-25 19:36:11]

https://codereview.chromium.org/699073004/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/699073004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1998:
DCHECK(context_provider->ContextCapabilities().gpu.texture_rectangle);
On 2014/11/25 18:12:21, piman (Very slow to review) wrote:
> Would it make sense to ask the GpuMemoryBufferImpl which texture target it
> should use?

Maybe. I'm not sure I want the concept of GL texture targets at the
gfx::GpuMemoryBuffer level. We could add something to the CHROMIUM_image API for
this but I'm not sure exactly what that would look like. And if we make it a
buffer specific setting in cc (where we're not currently assuming that all
buffers are of the same type) it breaks our current resource pool usage where we
assume that all buffers use the same target.

Another option is to introduce a new chromium only texture target that must be
used with CHROMIUM_image, call it EXTERNAL_CHROMIUM. The use of this target
would be limited much in the same way as EXTERNAL_OES. The GPU service would
then automatically use the correct GL target based on buffer type. I think this
would be cleanest solution but hard to implement as texture coordinates can't be
normalized in the RECTANGLE_ARB case...

I think it's easiest to handle this using ifdefs and a setting for now.

[reveman, 2014-11-25 23:31:52]

enne, I had to make some LTHI changes to avoid having the DCHECK to fail on mac
in a clean way. please take a look.

[enne (OOO), 2014-11-25 23:52:29]

lgtm

https://codereview.chromium.org/699073004/diff/60001/cc/test/layer_tree_pixel...
File cc/test/layer_tree_pixel_resource_test.cc (right):

https://codereview.chromium.org/699073004/diff/60001/cc/test/layer_tree_pixel...
cc/test/layer_tree_pixel_resource_test.cc:162:
EXPECT_TRUE(host_impl->GetRendererCapabilities().using_image);
I kind of liked the more well-named CanUseFooRasterizer functions that
abstracted the tests from this implementation detail, but this is fine too.

[reveman, 2014-11-26 00:39:18]

https://codereview.chromium.org/699073004/diff/60001/cc/test/layer_tree_pixel...
File cc/test/layer_tree_pixel_resource_test.cc (right):

https://codereview.chromium.org/699073004/diff/60001/cc/test/layer_tree_pixel...
cc/test/layer_tree_pixel_resource_test.cc:162:
EXPECT_TRUE(host_impl->GetRendererCapabilities().using_image);
On 2014/11/25 23:52:29, enne wrote:
> I kind of liked the more well-named CanUseFooRasterizer functions that
> abstracted the tests from this implementation detail, but this is fine too.

I'll move these checks into Zero/OneCopyRasterWorkerPool implementations instead
once they are just DCHECKs and not something we need to take into account when
deciding what RWP instanced to create.

[reveman, 2014-11-26 00:51:57]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-11-26 00:53:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699073004/60001

[commit-bot: I haz the power, 2014-11-26 02:11:43]

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

[commit-bot: I haz the power, 2014-11-26 02:11:44]

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

[reveman, 2014-11-26 02:18:27]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-11-26 02:19:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/699073004/60001

[commit-bot: I haz the power, 2014-11-26 02:45:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-26 02:45:46]

Patchset 4 (id:??) landed as
https://crrev.com/44b807f10ce0c1de305036932601b4715729d1d6
Cr-Commit-Position: refs/heads/master@{#305765}