Support different formats in the same resource pool.

Support different formats in the same resource pool.

This allows us to use different formats for tiles with opaque or translucent content for better memory savings or quality.

BUG=434699
Committed: https://crrev.com/8bde4e839b4c55733b730a71cd9beaff95e01fe4
Cr-Commit-Position: refs/heads/master@{#312090}

[peterp, 2015-01-12 14:03:49]

peterp@opera.com changed reviewers:
+ reveman@chromium.org

[peterp, 2015-01-12 14:06:58]

@reveman: This is another part of the tile compression patch. Does it look OK to
you?

[reveman, 2015-01-13 16:25:56]

Looks OK in general but I'm failing to see the need for adding
ResourceFormatUsage. Can you explain a bit more what the purpose of this enum
is?

https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_format.h
File cc/resources/resource_format.h (right):

https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_format...
cc/resources/resource_format.h:30: enum ResourceFormatUsage {
Do we need this? I found it a bit hard to follow how this maps to a format.

https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_pool.h
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_pool.h...
cc/resources/resource_pool.h:24: bool use_memory_efficient_format) {
does the resource pool need to know about use_memory_efficient_format? is it not
enough to remove format from here and add it to AcquireResource?

https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_pool.h...
cc/resources/resource_pool.h:79: typedef std::map<ResourceFormat, ResourceList>
UnusedMap;
do we need a map for this? can we instead just check that the format matches the
requested format in the same way we check that width/height matches the
requested dimensions when acquiring a new resource?

[peterp, 2015-01-14 12:33:14]

On 2015/01/13 16:25:56, reveman wrote:
> Looks OK in general but I'm failing to see the need for adding
> ResourceFormatUsage. Can you explain a bit more what the purpose of this enum
> is?
> 
>
https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_format.h
> File cc/resources/resource_format.h (right):
> 
>
https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_format...
> cc/resources/resource_format.h:30: enum ResourceFormatUsage {
> Do we need this? I found it a bit hard to follow how this maps to a format.
> 
> https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_pool.h
> File cc/resources/resource_pool.h (right):
> 
>
https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_pool.h...
> cc/resources/resource_pool.h:24: bool use_memory_efficient_format) {
> does the resource pool need to know about use_memory_efficient_format? is it
not
> enough to remove format from here and add it to AcquireResource?
> 
>
https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_pool.h...
> cc/resources/resource_pool.h:79: typedef std::map<ResourceFormat,
ResourceList>
> UnusedMap;
> do we need a map for this? can we instead just check that the format matches
the
> requested format in the same way we check that width/height matches the
> requested dimensions when acquiring a new resource?

It's to hide the actual formats being used inside the resource provider. That
way the tile manager only needs to tell it whether the tile is opaque or
transluscent and the gpu rasterizer just needs to ask for whatever format is
suitable for hardware rasterizing.

For this patch in isolation the Usage enum is indeed pointless, but in the
follow up patches ResourceProvider::memory_efficient_texture_format(usage) can
return any of 7 different format depending on GPU support, setting flags and the
usage parameter.

[peterp, 2015-01-14 12:33:36]

https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_format.h
File cc/resources/resource_format.h (right):

https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_format...
cc/resources/resource_format.h:30: enum ResourceFormatUsage {
On 2015/01/13 16:25:55, reveman wrote:
> Do we need this? I found it a bit hard to follow how this maps to a format. 

NATIVE will generally map to RGBA_4444, RGBA_8888 or BGRA_8888.
TRANSLUSCENT will generally map to ATC_IA, DXT5 or any of the NATIVE formats.
OPAQUE will generally map to ATC, DXT1, ETC1 or any of the NATIVE formats.

NATIVE is there because no GPU that I know of is capable of rendering directly
to a compressed format. Since there's no encoding it's also the fastest format
to use when generating tiles (but usually slower when OpenGL composits the final
fragments to the destination surface).

TRANSLUSCENT will generally achieve a 4:1 compression ratio and is the slowest
to encode but sometimes we need the alpha channel for blending in the
compositor.

OPAQUE will generally achive a 8:1 compression ratio and is the fastest to
encode. In practice we've found that this will be most of the tiles.

Maybe I should put all this in a comment for the enum?

https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_pool.h
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_pool.h...
cc/resources/resource_pool.h:24: bool use_memory_efficient_format) {
On 2015/01/13 16:25:55, reveman wrote:
> does the resource pool need to know about use_memory_efficient_format? is it
not
> enough to remove format from here and add it to AcquireResource?

The problem then is that when the tile manager asks the resource pool which
format to use the pool doesn't know if we have for example gpu rasterization
enabled.

https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_pool.h...
cc/resources/resource_pool.h:79: typedef std::map<ResourceFormat, ResourceList>
UnusedMap;
On 2015/01/13 16:25:55, reveman wrote:
> do we need a map for this? can we instead just check that the format matches
the
> requested format in the same way we check that width/height matches the
> requested dimensions when acquiring a new resource?

You're right, good point. Patch updated.

[reveman, 2015-01-14 13:46:03]

On 2015/01/14 12:33:14, peterp wrote:
> On 2015/01/13 16:25:56, reveman wrote:
> > Looks OK in general but I'm failing to see the need for adding
> > ResourceFormatUsage. Can you explain a bit more what the purpose of this
enum
> > is?
> > 
> >
>
https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_format.h
> > File cc/resources/resource_format.h (right):
> > 
> >
>
https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_format...
> > cc/resources/resource_format.h:30: enum ResourceFormatUsage {
> > Do we need this? I found it a bit hard to follow how this maps to a format.
> > 
> >
https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_pool.h
> > File cc/resources/resource_pool.h (right):
> > 
> >
>
https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_pool.h...
> > cc/resources/resource_pool.h:24: bool use_memory_efficient_format) {
> > does the resource pool need to know about use_memory_efficient_format? is it
> not
> > enough to remove format from here and add it to AcquireResource?
> > 
> >
>
https://codereview.chromium.org/817133006/diff/1/cc/resources/resource_pool.h...
> > cc/resources/resource_pool.h:79: typedef std::map<ResourceFormat,
> ResourceList>
> > UnusedMap;
> > do we need a map for this? can we instead just check that the format matches
> the
> > requested format in the same way we check that width/height matches the
> > requested dimensions when acquiring a new resource?
> 
> It's to hide the actual formats being used inside the resource provider. That
> way the tile manager only needs to tell it whether the tile is opaque or
> transluscent and the gpu rasterizer just needs to ask for whatever format is
> suitable for hardware rasterizing.

Could we just keep all this format selection logic in the tile manager and
rasterizer? I think that would be more consistent with existing code, which
typically avoids having logic in the ResourcePRovider class and instead expose
the extensions available..

> 
> For this patch in isolation the Usage enum is indeed pointless, but in the
> follow up patches ResourceProvider::memory_efficient_texture_format(usage) can
> return any of 7 different format depending on GPU support, setting flags and
the
> usage parameter.

[peterp, 2015-01-15 09:51:49]

On 2015/01/14 13:46:03, reveman wrote:
> On 2015/01/14 12:33:14, peterp wrote:
> > On 2015/01/13 16:25:56, reveman wrote:
> > > Looks OK in general but I'm failing to see the need for adding
> > > ResourceFormatUsage. Can you explain a bit more what the purpose of this
> enum
> > > is?
> > > 
> > >
> > 
> > It's to hide the actual formats being used inside the resource provider.
That
> > way the tile manager only needs to tell it whether the tile is opaque or
> > transluscent and the gpu rasterizer just needs to ask for whatever format is
> > suitable for hardware rasterizing.
> 
> Could we just keep all this format selection logic in the tile manager and
> rasterizer? I think that would be more consistent with existing code, which
> typically avoids having logic in the ResourcePRovider class and instead expose
> the extensions available..
> 
> > 
> > For this patch in isolation the Usage enum is indeed pointless, but in the
> > follow up patches ResourceProvider::memory_efficient_texture_format(usage)
can
> > return any of 7 different format depending on GPU support, setting flags and
> the
> > usage parameter.


Ok, I've removed ResourceFormatUsage and reduced the scope of this commit a bit.
Now this commit is only about letting the ResourcePool handle multiple formats.

We'll try to avoid adding logic to the ResourceProvider but I don't see how we
can completely avoid it though.
ResourceProvider::memory_efficient_texture_format() is already where it decides
if a 16-bit or 32-bit format should be used. I think it makes sense if that's
also where it would return which compressed format to use if that's enabled.

[reveman, 2015-01-15 16:33:11]

https://codereview.chromium.org/817133006/diff/40001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/817133006/diff/40001/cc/resources/resource_po...
cc/resources/resource_pool.cc:116: if (use_memory_efficient_format_) {
I would prefer if all format decisions are made by the client rather than some
here and some by the client.

https://codereview.chromium.org/817133006/diff/40001/cc/resources/resource_po...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/817133006/diff/40001/cc/resources/resource_po...
cc/resources/resource_pool.h:33: scoped_ptr<ScopedResource>
AcquireResource(const gfx::Size& size);
I'd rather not have 2 of these functions. I'd prefer if the client had to always
explicitly specify the format.

[peterp, 2015-01-16 12:10:11]

https://codereview.chromium.org/817133006/diff/40001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/817133006/diff/40001/cc/resources/resource_po...
cc/resources/resource_pool.cc:116: if (use_memory_efficient_format_) {
On 2015/01/15 16:33:11, reveman wrote:
> I would prefer if all format decisions are made by the client rather than some
> here and some by the client.

Ok, I've removed this logic and changed the pool to store the default format
given by the client instead.

https://codereview.chromium.org/817133006/diff/40001/cc/resources/resource_po...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/817133006/diff/40001/cc/resources/resource_po...
cc/resources/resource_pool.h:33: scoped_ptr<ScopedResource>
AcquireResource(const gfx::Size& size);
On 2015/01/15 16:33:11, reveman wrote:
> I'd rather not have 2 of these functions. I'd prefer if the client had to
always
> explicitly specify the format.

Fair enough, it's been removed.

[reveman, 2015-01-16 13:40:01]

https://codereview.chromium.org/817133006/diff/60001/cc/resources/resource_po...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/817133006/diff/60001/cc/resources/resource_po...
cc/resources/resource_pool.h:23: ResourceFormat default_format) {
I think it would be nice if we could just remove all format state from the
ResourcePool class. default_format is not really used by the ResourcePool class
but just some state that I think the client should instead keep track of.
Landing this first and removing default format in a follow up is fine with me if
you prefer that.

[peterp, 2015-01-16 14:31:52]

On 2015/01/16 13:40:01, reveman wrote:
>
https://codereview.chromium.org/817133006/diff/60001/cc/resources/resource_po...
> File cc/resources/resource_pool.h (right):
> 
>
https://codereview.chromium.org/817133006/diff/60001/cc/resources/resource_po...
> cc/resources/resource_pool.h:23: ResourceFormat default_format) {
> I think it would be nice if we could just remove all format state from the
> ResourcePool class. default_format is not really used by the ResourcePool
class
> but just some state that I think the client should instead keep track of.
> Landing this first and removing default format in a follow up is fine with me
if
> you prefer that.

Yes, that makes sense. I would prefer to do that in a follow up.

[reveman, 2015-01-16 15:02:53]

On 2015/01/16 14:31:52, peterp wrote:
> On 2015/01/16 13:40:01, reveman wrote:
> >
>
https://codereview.chromium.org/817133006/diff/60001/cc/resources/resource_po...
> > File cc/resources/resource_pool.h (right):
> > 
> >
>
https://codereview.chromium.org/817133006/diff/60001/cc/resources/resource_po...
> > cc/resources/resource_pool.h:23: ResourceFormat default_format) {
> > I think it would be nice if we could just remove all format state from the
> > ResourcePool class. default_format is not really used by the ResourcePool
> class
> > but just some state that I think the client should instead keep track of.
> > Landing this first and removing default format in a follow up is fine with
me
> if
> > you prefer that.
> 
> Yes, that makes sense. I would prefer to do that in a follow up.

OK, lgtm after adding a TODO next to default_format field.

[peterp, 2015-01-19 09:24:25]

The CQ bit was checked by peterp@opera.com

[commit-bot: I haz the power, 2015-01-19 09:24:36]

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

[commit-bot: I haz the power, 2015-01-19 10:21:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-19 10:22:39]

Patchset 5 (id:??) landed as
https://crrev.com/8bde4e839b4c55733b730a71cd9beaff95e01fe4
Cr-Commit-Position: refs/heads/master@{#312090}