[cc] Allow resources and ui resources to specify wrap mode

[cc] Allow resources and ui resources to specify wrap mode

All textures are currently GL_CLAMP_TO_EDGE. This is good for tiles,
but not good for the background linen texture, which needs to be
repeated.

Add a mechanism to specify the texture wrap mode.

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

[ccameron, 2013-08-07 20:39:10]

This builds on the UI resource manager added in
https://chromiumcodereview.appspot.com/18191020

The idea is to use a UI resource to draw the gutter tiles as linen on Mac. A
hacky sketch of what that will look like is at
https://chromiumcodereview.appspot.com/22313011/

[enne (OOO), 2013-08-07 20:43:27]

+piman, powei, aelias FYI

[ccameron, 2013-08-07 20:50:08]

On 2013/08/07 20:43:27, enne wrote:
> +piman, powei, aelias FYI

While I have an audience for this, I should mention that I'm going to want UI
resources to be "managed" in the sense that I will want them to be freed when
tabs are backgrounded (as it is, the follow-on patch eats up 256k of GPU and CPU
memory for every backgrounded tab).

[jamesr, 2013-08-07 20:53:04]

On 2013/08/07 20:50:08, ccameron1 wrote:
> On 2013/08/07 20:43:27, enne wrote:
> > +piman, powei, aelias FYI
> 
> While I have an audience for this, I should mention that I'm going to want UI
> resources to be "managed" in the sense that I will want them to be freed when
> tabs are backgrounded (as it is, the follow-on patch eats up 256k of GPU and
CPU
> memory for every backgrounded tab).

Can't we instance this?  The linen texture is pretty small (something like
32x32x4 = 4k) and we only need one in the browser.

[piman, 2013-08-07 20:55:05]

https://codereview.chromium.org/22529002/diff/6001/cc/resources/ui_resource_b...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22529002/diff/6001/cc/resources/ui_resource_b...
cc/resources/ui_resource_bitmap.h:26: ClampToEdge,
nit: chrome style = uppercase for enum values.

[enne (OOO), 2013-08-07 20:57:52]

https://codereview.chromium.org/22529002/diff/6001/cc/resources/resource_prov...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/22529002/diff/6001/cc/resources/resource_prov...
cc/resources/resource_provider.cc:56: GLC(context3d, context3d->texParameteri(
This is just a drive-by nit, but it's a little weird to have this texture state
initialization code partially in CreateTextureId and partially in the calling
code.  This patch doesn't make it worse, since the split existed before, but
still.

Maybe this function should just not exist and be collapsed into LazyCreate?

https://codereview.chromium.org/22529002/diff/6001/cc/resources/resource_prov...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/22529002/diff/6001/cc/resources/resource_prov...
cc/resources/resource_provider_unittest.cc:1190:
Mock::VerifyAndClearExpectations(context);
Can you put this inside the loop?  I think these expectations here are
unordered.

[aelias_OOO_until_Jul13, 2013-08-07 21:20:07]

On 2013/08/07 20:50:08, ccameron1 wrote:
> On 2013/08/07 20:43:27, enne wrote:
> > +piman, powei, aelias FYI
> 
> While I have an audience for this, I should mention that I'm going to want UI
> resources to be "managed" in the sense that I will want them to be freed when
> tabs are backgrounded (as it is, the follow-on patch eats up 256k of GPU and
CPU
> memory for every backgrounded tab).

OK, but if you're implying CC should do this automatically in reaction to
visibility changes, I'm opposed to that.  The CC embedder (Blink, Aura, or
Mac/Android code) already has the tools it needs to manage memory usage using
LayerTreeHost::CreateUIResource/DeleteUIResource.

We have similar visibility needs on the Android platform and that's how we're
managing it.  All our resources and platform UI elements are managed in the
browser process which makes this approach natural.  Maybe because of weird Mac
constraints you want to do some things with the renderer compositor?  We should
have a design discussion about this if so -- I would favor Mac gaining a browser
compositor in any case, as it will be the last Chrome platform that doesn't have
one.

[ccameron, 2013-08-07 21:54:47]

I talked with jamesr about this, and we decided it's better to have the linen
texture shared across all renderers. The way we'll go about this is have a bunch
of read-only mailboxes provided to every command buffer at startup, so the
compositor will just say "give me the texture from the mailbox for the overhang
area".

So for the moment, this change is shelved (in particular, the thing about adding
a wrap mode wasn't added for mailboxes, which is now the one place that it's
needed).

[aelias_OOO_until_Jul13, 2013-08-08 02:06:50]

On 2013/08/07 21:54:47, ccameron1 wrote:
> I talked with jamesr about this, and we decided it's better to have the linen
> texture shared across all renderers. The way we'll go about this is have a
bunch
> of read-only mailboxes provided to every command buffer at startup, so the
> compositor will just say "give me the texture from the mailbox for the
overhang
> area".
> 
> So for the moment, this change is shelved (in particular, the thing about
adding
> a wrap mode wasn't added for mailboxes, which is now the one place that it's
> needed).

OK, sounds good.  Please cc me and jdduke@ on those patches since we may
eventually want to do the same thing for Android's overscroll glow textures.

[ccameron, 2013-08-31 00:37:57]

Thanks for the feedback!! Ptal.

I've resurrected this again because UI resources are going to be dropped on
backgrounding after all.

Also, sharing these resources across all renderers is a a large and
underconstrained undertaking. The way that we converged to in discussions was to
have a glTexImage2DSHARED sort of thing which would be told to look for
identical content, and share them in the GPU process. If this feature becomes a
priority (finds other applications), it can easily be shoehorned into the
ResourceProvider::SetPixels function. For now it's more complexity than it's
worth.

https://codereview.chromium.org/22529002/diff/6001/cc/resources/resource_prov...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/22529002/diff/6001/cc/resources/resource_prov...
cc/resources/resource_provider.cc:56: GLC(context3d, context3d->texParameteri(
On 2013/08/07 20:57:52, enne wrote:
> This is just a drive-by nit, but it's a little weird to have this texture
state
> initialization code partially in CreateTextureId and partially in the calling
> code.  This patch doesn't make it worse, since the split existed before, but
> still.
> 
> Maybe this function should just not exist and be collapsed into LazyCreate?

Done.

https://codereview.chromium.org/22529002/diff/6001/cc/resources/resource_prov...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/22529002/diff/6001/cc/resources/resource_prov...
cc/resources/resource_provider_unittest.cc:1190:
Mock::VerifyAndClearExpectations(context);
On 2013/08/07 20:57:52, enne wrote:
> Can you put this inside the loop?  I think these expectations here are
> unordered.

Done.

https://codereview.chromium.org/22529002/diff/6001/cc/resources/ui_resource_b...
File cc/resources/ui_resource_bitmap.h (right):

https://codereview.chromium.org/22529002/diff/6001/cc/resources/ui_resource_b...
cc/resources/ui_resource_bitmap.h:26: ClampToEdge,
On 2013/08/07 20:55:05, piman wrote:
> nit: chrome style = uppercase for enum values.

Done.

[ccameron, 2013-09-03 21:46:36]

Pinging this again in case it fell off the bottom of the the long weekend inbox.

[enne (OOO), 2013-09-03 22:18:52]

https://codereview.chromium.org/22529002/diff/18001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/22529002/diff/18001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:2608: id = resource_provider_->CreateGLTexture(
This seems like an unfortunate restriction to only let UI resources be backed by
hardware textures.  I'm no Skia expert, but it seems like maybe an SkShader
could be applied in the SoftwareRenderer with kRepeat_TileMode.

Could you change this call back to CreateResource (adding a wrap_mode there) and
filing a bug to support repeated textures in the software renderer? At least
that way UI resources (e.g. scrollbars) will still work for the software
compositor in the meantime.

[ccameron, 2013-09-03 22:52:08]

Thanks!!

https://codereview.chromium.org/22529002/diff/18001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/22529002/diff/18001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:2608: id = resource_provider_->CreateGLTexture(
On 2013/09/03 22:18:52, enne wrote:
> This seems like an unfortunate restriction to only let UI resources be backed
by
> hardware textures.  I'm no Skia expert, but it seems like maybe an SkShader
> could be applied in the SoftwareRenderer with kRepeat_TileMode.
> 
> Could you change this call back to CreateResource (adding a wrap_mode there)
and
> filing a bug to support repeated textures in the software renderer? At least
> that way UI resources (e.g. scrollbars) will still work for the software
> compositor in the meantime.

Good point -- done (crbug.com/284796).

[enne (OOO), 2013-09-04 17:36:56]

lgtm

https://codereview.chromium.org/22529002/diff/25001/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/22529002/diff/25001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:201: return CreateBitmap(size);
Can you leave a comment with the bug you filed here so that it's obvious that
wrap_mode not being handled is just a "not implemented yet" sort of bug?

https://codereview.chromium.org/22529002/diff/25001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/22529002/diff/25001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:2597: default:
Can you remove this default case so that the compiler can yell if somebody adds
an enum value and doesn't update this switch?

[ccameron, 2013-09-04 20:00:38]

Thanks!!

https://codereview.chromium.org/22529002/diff/25001/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/22529002/diff/25001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:201: return CreateBitmap(size);
On 2013/09/04 17:36:56, enne wrote:
> Can you leave a comment with the bug you filed here so that it's obvious that
> wrap_mode not being handled is just a "not implemented yet" sort of bug?

Done.

https://codereview.chromium.org/22529002/diff/25001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/22529002/diff/25001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_impl.cc:2597: default:
On 2013/09/04 17:36:56, enne wrote:
> Can you remove this default case so that the compiler can yell if somebody
adds
> an enum value and doesn't update this switch?

Done.

[commit-bot: I haz the power, 2013-09-05 00:10:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/22529002/36001

[commit-bot: I haz the power, 2013-09-05 01:41:16]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-09-05 08:26:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/22529002/22001

[commit-bot: I haz the power, 2013-09-05 08:35:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/22529002/66001

[commit-bot: I haz the power, 2013-09-05 09:02:58]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-05 09:17:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ccameron@chromium.org/22529002/83001

[commit-bot: I haz the power, 2013-09-05 15:30:20]

Change committed as 221433