content: Move type selection logic out of GpuMemoryBufferImpl.

content: Move type selection logic out of GpuMemoryBufferImpl.

This moves the logic to determine what type of buffer
to allocate out of the GpuMemoryBuffer implementation.

The GpuMemoryBufferImpl class instead provides a
mechanism to query supported types and check if a
format/usage configuration is supported by a specific
type.

This allows us create a generic set of unit tests
that exercise each supported buffer type and it
becomes trivial to add a command line flag for
selecting a preferred GPU memory buffer type.

BUG=431059
TEST=content_unittests --gtest_filter=GpuMemoryBufferImplTests/GpuMemoryBufferImplTest.*

Committed: https://crrev.com/bc2de2bc9f44493125df09842ce09883217cedaf
Cr-Commit-Position: refs/heads/master@{#304381}

[reveman, 2014-11-11 14:59:48]

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

[alexst (slow to review), 2014-11-11 18:49:23]

https://codereview.chromium.org/701033005/diff/80001/content/browser/gpu/brow...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/701033005/diff/80001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:263:
GetPreferredGpuMemoryBufferTypeForConfiguration(
Should this early out in BrowserGpuMemoryBufferManager::AllocateGpuMemoryBuffer
for consistency? 

If GetPreferredGpuMemoryBufferTypeForConfiguration ever returns EMPTY_BUFFER
here, we'll end up hitting a NOTREACHED in the GpuMemoryBufferImpl.

[reveman, 2014-11-11 19:07:45]

https://codereview.chromium.org/701033005/diff/80001/content/browser/gpu/brow...
File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right):

https://codereview.chromium.org/701033005/diff/80001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:261: 
hm, this blank line should not be here.

https://codereview.chromium.org/701033005/diff/80001/content/browser/gpu/brow...
content/browser/gpu/browser_gpu_memory_buffer_manager.cc:263:
GetPreferredGpuMemoryBufferTypeForConfiguration(
On 2014/11/11 18:49:23, alexst wrote:
> Should this early out in
BrowserGpuMemoryBufferManager::AllocateGpuMemoryBuffer
> for consistency? 
> 
> If GetPreferredGpuMemoryBufferTypeForConfiguration ever returns EMPTY_BUFFER
> here, we'll end up hitting a NOTREACHED in the GpuMemoryBufferImpl.

I think we should consider it a browser bug if this ever returns EMPTY_BUFFER.
And I'd rather NOTREACHED than early out for browser bugs.

[alexst (slow to review), 2014-11-11 21:05:49]

Looks good, I just want to sort out the unittests and capabilities.

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

https://codereview.chromium.org/701033005/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:134:
switch (usage) {
I've been looking at the unittests, since there are many platforms with varying
capabilities, we should change this to
SurfaceFactoryOzone::GetInstance()->CanCreateNativePixmap(BufferUsage usage)
instead of the hardcoded values here. We just recently added it for situations
like this.

[piman, 2014-11-11 22:38:30]

https://codereview.chromium.org/701033005/diff/100001/content/common/child_pr...
File content/common/child_process_host_impl.cc (left):

https://codereview.chromium.org/701033005/diff/100001/content/common/child_pr...
content/common/child_process_host_impl.cc:325: if (!size.IsValid()) {
Why removing this?

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

https://codereview.chromium.org/701033005/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:25:
base::LazyInstance<DefaultPreferredType>::Leaky g_default_preferred_type =
Why leaky?

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

https://codereview.chromium.org/701033005/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_android.cc:34:
GpuMemoryBufferImplSurfaceTexture::IsUsageSupported(usage);
So, there's a lot of duplication between all the platform-specific files.

I wonder if we could have it all in gpu_memory_buffer_impl.cc, with a single
switch on all the types (with ifdefs if needed).

Actually, we're really aching for a factory pattern, because the mapping from
the enum to the GpuMemoryBufferImpl type is always the same in each of the
GpuMemoryBufferImpl static calls. So it would be nice to have a static map from
the enum to a per-type factory that implements
IsFormatSupported/IsUsageSupported/Create/AllocateForChildProcess/DeletedByChildProcess/CreateFromHandle

Then all the platform-specific implementation has to do is to provide the
GetSupportedTypes. The per-type implementation wouldn't change much
(IsFormatSupported and friends would be member methods of
GpuMemoryBufferImpl*::Factory instead of static methods of
GpuMemoryBufferImpl*), and all the switch logic would be replaced by a single
map (array?) lookup in the common GpuMemoryBufferImpl implementation.

WDYT?

[reveman, 2014-11-12 05:09:20]

https://codereview.chromium.org/701033005/diff/100001/content/common/child_pr...
File content/common/child_process_host_impl.cc (left):

https://codereview.chromium.org/701033005/diff/100001/content/common/child_pr...
content/common/child_process_host_impl.cc:325: if (!size.IsValid()) {
On 2014/11/11 22:38:30, piman (Very slow to review) wrote:
> Why removing this?

This check is now done by
GpuMemoryBufferImplSharedMemory::AllocateForChildProcess. The reasoning is that
only ::AllocateForChildProcess implementations can know what values can cause
problems. In the SharedMemory case, bbp * width * height overflow is what we
need to check for but that could be different for other implementations.

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

https://codereview.chromium.org/701033005/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:25:
base::LazyInstance<DefaultPreferredType>::Leaky g_default_preferred_type =
On 2014/11/11 22:38:30, piman (Very slow to review) wrote:
> Why leaky?

In case someone would call GetPreferredType() from the dtor of some other lazy
instance. Maybe overkill to consider that use case. I kept it in latest patch
but don't mind removing it if you prefer that.

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

https://codereview.chromium.org/701033005/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_android.cc:34:
GpuMemoryBufferImplSurfaceTexture::IsUsageSupported(usage);
On 2014/11/11 22:38:30, piman (Very slow to review) wrote:
> So, there's a lot of duplication between all the platform-specific files.
> 
> I wonder if we could have it all in gpu_memory_buffer_impl.cc, with a single
> switch on all the types (with ifdefs if needed).
> 
> Actually, we're really aching for a factory pattern, because the mapping from
> the enum to the GpuMemoryBufferImpl type is always the same in each of the
> GpuMemoryBufferImpl static calls. So it would be nice to have a static map
from
> the enum to a per-type factory that implements
>
IsFormatSupported/IsUsageSupported/Create/AllocateForChildProcess/DeletedByChildProcess/CreateFromHandle
> 
> Then all the platform-specific implementation has to do is to provide the
> GetSupportedTypes. The per-type implementation wouldn't change much
> (IsFormatSupported and friends would be member methods of
> GpuMemoryBufferImpl*::Factory instead of static methods of
> GpuMemoryBufferImpl*), and all the switch logic would be replaced by a single
> map (array?) lookup in the common GpuMemoryBufferImpl implementation.
> 
> WDYT?

I agree that we need a factory here. I don't think we need an array or map. The
manager can simply create factory instances for the preferred and the shared
memory type and keep track of those.

However, instead of introducing a new factory interface (we already have too
many GpuMemoryBuffer related classes) I'd like to find a way to make our
existing GpuMemoryBufferFactory class serve this purpose. This factory interface
is currently used to allocate buffers on the GPU service side but I feel like it
makes some sense to use this on the browser side too. The functions used by the
browser would be different than what's currently used by the GPU service. We
currently use the GpuMemoryBufferImpl API in both the renderer and the browser.
I'd like to keep whatever is used by the renderer (client) in
GpuMemoryBufferImpl (only CreateFromHandle) but move the rest to the
GpuMemoryBufferFactory interface. I think it makes more sense to share the
GpuMemoryBufferFactory interface between the browser and the GPU service than
the current sharing of the GpuMemoryBufferImpl interface between renderer and
browser. The browser and the GPU service are both taking part in the process of
allocating buffers after all. The browser allocation code in some cases just
need to talk to the factory instance in the GPU service side...

I'm going to implement this as a follow up to this patch. I'll wait to land this
patch until that's done if you prefer but it would be nice to land this for now
so we can start testing 1-copy on MacOS and Android in the meantime. Let me know
what you prefer.

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

https://codereview.chromium.org/701033005/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl_ozone_native_buffer.cc:134:
switch (usage) {
On 2014/11/11 21:05:49, alexst wrote:
> I've been looking at the unittests, since there are many platforms with
varying
> capabilities, we should change this to
> SurfaceFactoryOzone::GetInstance()->CanCreateNativePixmap(BufferUsage usage)
> instead of the hardcoded values here. We just recently added it for situations
> like this.

I made it use CanCreateNativePixmap in the SCANOUT case. MAP case is still hard
coded to false as we need to change the ::Create/:AllocateForChildProcess
functions to adjust that and I'd rather do that in a follow up.

[alexst (slow to review), 2014-11-13 20:13:24]

I agree that we should take a pass at refactoring (or should I say
re-factory-ing) this code, it has grown to be quite complicated, but I would
prefer we did it in a separate patch that didn't also change functionality.

There is a lot of classes and factories dealing with GpuMemoryBuffers already.
Re-thinking them in isolation will make it a lot easier to keep track of what's
going on as well as to take a look at the whole thing from 30,000ft.

As far as added functionality here, lgtm, and looking forward to a global
simplification.

[enne (OOO), 2014-11-13 20:17:39]

(Reviewer ping.  This and the followup
https://codereview.chromium.org/699073004/ are blocking me turning on impl-side
painting for layout tests on Mac.)

[reveman, 2014-11-13 20:25:01]

On 2014/11/13 20:17:39, enne wrote:
> (Reviewer ping.  This and the followup
> https://codereview.chromium.org/699073004/ are blocking me turning on
impl-side
> painting for layout tests on Mac.)

I'm working on a refactor as a follow up. I probably need a few more days finish
it. I'm OK landing this for now but I'll leave it up to alexst@ and piman@ to
decide if this can land as it is or if we should wait for the refactor.

[piman, 2014-11-14 03:59:44]

LGTM (sorry I was out)

https://codereview.chromium.org/701033005/diff/100001/content/common/child_pr...
File content/common/child_process_host_impl.cc (left):

https://codereview.chromium.org/701033005/diff/100001/content/common/child_pr...
content/common/child_process_host_impl.cc:325: if (!size.IsValid()) {
On 2014/11/12 05:09:20, reveman wrote:
> On 2014/11/11 22:38:30, piman (Very slow to review) wrote:
> > Why removing this?
> 
> This check is now done by
> GpuMemoryBufferImplSharedMemory::AllocateForChildProcess. The reasoning is
that
> only ::AllocateForChildProcess implementations can know what values can cause
> problems. In the SharedMemory case, bbp * width * height overflow is what we
> need to check for but that could be different for other implementations.

Ok, can you add a comment? It's generally good to have checks closer to the IPC
handlers, because that's where the security decisions are usually taken. If it's
in other places it's easy to lose the checks during refactoring etc.

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

https://codereview.chromium.org/701033005/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:25:
base::LazyInstance<DefaultPreferredType>::Leaky g_default_preferred_type =
On 2014/11/12 05:09:20, reveman wrote:
> On 2014/11/11 22:38:30, piman (Very slow to review) wrote:
> > Why leaky?
> 
> In case someone would call GetPreferredType() from the dtor of some other lazy
> instance. Maybe overkill to consider that use case. I kept it in latest patch
> but don't mind removing it if you prefer that.

I prefer to avoid leaking if we can...

[reveman, 2014-11-17 02:26:53]

https://codereview.chromium.org/701033005/diff/100001/content/common/child_pr...
File content/common/child_process_host_impl.cc (left):

https://codereview.chromium.org/701033005/diff/100001/content/common/child_pr...
content/common/child_process_host_impl.cc:325: if (!size.IsValid()) {
On 2014/11/14 03:59:44, piman (Very slow to review) wrote:
> On 2014/11/12 05:09:20, reveman wrote:
> > On 2014/11/11 22:38:30, piman (Very slow to review) wrote:
> > > Why removing this?
> > 
> > This check is now done by
> > GpuMemoryBufferImplSharedMemory::AllocateForChildProcess. The reasoning is
> that
> > only ::AllocateForChildProcess implementations can know what values can
cause
> > problems. In the SharedMemory case, bbp * width * height overflow is what we
> > need to check for but that could be different for other implementations.
> 
> Ok, can you add a comment? It's generally good to have checks closer to the
IPC
> handlers, because that's where the security decisions are usually taken. If
it's
> in other places it's easy to lose the checks during refactoring etc.

Done.

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

https://codereview.chromium.org/701033005/diff/100001/content/common/gpu/clie...
content/common/gpu/client/gpu_memory_buffer_impl.cc:25:
base::LazyInstance<DefaultPreferredType>::Leaky g_default_preferred_type =
On 2014/11/14 03:59:44, piman (Very slow to review) wrote:
> On 2014/11/12 05:09:20, reveman wrote:
> > On 2014/11/11 22:38:30, piman (Very slow to review) wrote:
> > > Why leaky?
> > 
> > In case someone would call GetPreferredType() from the dtor of some other
lazy
> > instance. Maybe overkill to consider that use case. I kept it in latest
patch
> > but don't mind removing it if you prefer that.
> 
> I prefer to avoid leaking if we can...

Done.

[reveman, 2014-11-17 02:27:13]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-11-17 02:27:51]

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

[commit-bot: I haz the power, 2014-11-17 03:21:42]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-11-17 03:22:22]

Patchset 10 (id:??) landed as
https://crrev.com/bc2de2bc9f44493125df09842ce09883217cedaf
Cr-Commit-Position: refs/heads/master@{#304381}