cc: Add flag to specify to ResourceProvider to always use GMBs

cc: Add flag to specify to ResourceProvider to always use GMBs

Add a command line switch (similar to zero-copy) to specify that all
resources allocated by the cc::ResourceProvider should use GMBs and
have GLImages attached to them.

BUG=533677
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[ccameron, 2015-10-30 00:10:31]

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

[ccameron, 2015-10-30 00:10:31]

Mostly just plumbing, the only meat is in ResourceProvider, where we consolidate
the GMB-backing allocation into LazyAllocate.

Very open to better names for the flag.

[ccameron, 2015-10-30 21:07:35]

Updated the patch to use a name more consistent with
kEnableGpuMemoryBufferVideoFrames.

[reveman, 2015-11-02 17:28:53]

https://codereview.chromium.org/1425533006/diff/60001/cc/output/renderer_sett...
File cc/output/renderer_settings.h (right):

https://codereview.chromium.org/1425533006/diff/60001/cc/output/renderer_sett...
cc/output/renderer_settings.h:29: bool use_gpu_memory_buffers_by_default;
How about "use_gpu_memory_buffer_resources" to be consistent with the command
line switch?

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1501: BindImageForSampling(resource);
Why is this BindImageForSampling call needed?

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1518: void
ResourceProvider::SetGpuMemoryBuffer(
hm, this function is marking the image as dirty and lazy creating one if needed.
Can this be renamed to something like MarkImageAsDirty or LazyCreateImage and
not take a GMB as parameter but instead expect it to be set? I feel like that
would make it a bit easier to understand what this does.

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1521: DCHECK(!resource->gpu_memory_buffer);
can these 3 lines just become DCHECK(resource->gpu_memory_buffer)?

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1525: LazyCreate(resource);
I think this should be moved out of this function now that we use it from
LazyAllocate.

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1543: resource->allocated = true;
hm, this should not be set here as that means setting it twice in the
LAzyAllocate case.

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1545: resource->is_overlay_candidate = true;
might be better to set this by the code that allocates the storage too

[ccameron, 2015-11-02 22:07:34]

Thanks -- updated.

https://codereview.chromium.org/1425533006/diff/60001/cc/output/renderer_sett...
File cc/output/renderer_settings.h (right):

https://codereview.chromium.org/1425533006/diff/60001/cc/output/renderer_sett...
cc/output/renderer_settings.h:29: bool use_gpu_memory_buffers_by_default;
On 2015/11/02 17:28:53, reveman wrote:
> How about "use_gpu_memory_buffer_resources" to be consistent with the command
> line switch?

Good idea, done.

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1501: BindImageForSampling(resource);
On 2015/11/02 17:28:53, reveman wrote:
> Why is this BindImageForSampling call needed?

This can be moved to the ScopedWriteLockGL this to the lock for GL read/write.

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1518: void
ResourceProvider::SetGpuMemoryBuffer(
On 2015/11/02 17:28:53, reveman wrote:
> hm, this function is marking the image as dirty and lazy creating one if
needed.
> Can this be renamed to something like MarkImageAsDirty or LazyCreateImage and
> not take a GMB as parameter but instead expect it to be set? I feel like that
> would make it a bit easier to understand what this does.

Sounds better --done.

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1521: DCHECK(!resource->gpu_memory_buffer);
On 2015/11/02 17:28:53, reveman wrote:
> can these 3 lines just become DCHECK(resource->gpu_memory_buffer)?

Yeah, this becomes much simpler this way.

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1525: LazyCreate(resource);
On 2015/11/02 17:28:53, reveman wrote:
> I think this should be moved out of this function now that we use it from
> LazyAllocate.

Sg. Added a LazyCreate to the ~ScopedWriteLockGpuMemoryBuffer and a
DCHECK(gl_id) to ensure that it is hit in both paths.

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1543: resource->allocated = true;
On 2015/11/02 17:28:53, reveman wrote:
> hm, this should not be set here as that means setting it twice in the
> LAzyAllocate case.

Moved up to the caller and added a DCHECK for it.

https://codereview.chromium.org/1425533006/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1545: resource->is_overlay_candidate = true;
On 2015/11/02 17:28:53, reveman wrote:
> might be better to set this by the code that allocates the storage too

Done.

[reveman, 2015-11-02 23:10:25]

lgtm with a few nits

https://codereview.chromium.org/1425533006/diff/80001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1425533006/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1543: resource->dirty_image = true;
nit: it's unexpected that this function (with Lazy in the name) happens to be
used to mark the image as dirty. I'd expect that it does nothing if an image
already exists. can we move this to the caller?

https://codereview.chromium.org/1425533006/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1552: gl->BindTexture(resource->target,
resource->gl_id);
nit: as we didn't need this in the past, would it make more sense to add this
BindTexture call to the new caller of this function?

https://codereview.chromium.org/1425533006/diff/80001/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1425533006/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.h:548: void LazyAllocateImage(Resource*
resource);
nit: I think I prefer LazyCreateImage as this doesn't actually allocate anything

[ccameron, 2015-11-03 04:20:06]

ccameron@chromium.org changed reviewers:
+ avi@chromium.org

[ccameron, 2015-11-03 04:20:07]

Thanks! Adding avi@ to stamp content/ plumbing.

https://codereview.chromium.org/1425533006/diff/80001/cc/resources/resource_p...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/1425533006/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1543: resource->dirty_image = true;
On 2015/11/02 23:10:25, reveman wrote:
> nit: it's unexpected that this function (with Lazy in the name) happens to be
> used to mark the image as dirty. I'd expect that it does nothing if an image
> already exists. can we move this to the caller?

Done.

https://codereview.chromium.org/1425533006/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.cc:1552: gl->BindTexture(resource->target,
resource->gl_id);
On 2015/11/02 23:10:25, reveman wrote:
> nit: as we didn't need this in the past, would it make more sense to add this
> BindTexture call to the new caller of this function?

All of the callers used to do this independently, so it feels more appropriate
to do this here (the alternative is to add a comment saying that it needs to be
bound, and add binds at every callsite, which doesn't feel right to me).

https://codereview.chromium.org/1425533006/diff/80001/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1425533006/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.h:548: void LazyAllocateImage(Resource*
resource);
On 2015/11/02 23:10:25, reveman wrote:
> nit: I think I prefer LazyCreateImage as this doesn't actually allocate
anything

Done.

[Avi (use Gerrit), 2015-11-03 15:33:00]

lgtm

stampity stamp

[ccameron, 2015-11-03 17:28:53]

Thanks!

[ccameron, 2015-11-03 17:28:58]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-11-03 17:28:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1425533006/#ps120001
(title: "Fix test and build")

[commit-bot: I haz the power, 2015-11-03 17:31:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425533006/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425533006/120001

[ccameron, 2015-11-03 20:13:57]

The CQ bit was unchecked by ccameron@chromium.org

[ccameron, 2015-11-03 20:13:57]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2015-11-03 20:14:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1425533006/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1425533006/120001

[commit-bot: I haz the power, 2015-11-03 20:14:39]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-03 20:16:00]

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