Remove the default format from ResourcePool.

Remove the default format from ResourcePool.

Let each client store the format locally instead.

BUG=434699
Committed: https://crrev.com/66304d3c5632199f46abb85b1acca5d87f03ce49
Cr-Commit-Position: refs/heads/master@{#313247}

[peterp, 2015-01-20 13:50:38]

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

[peterp, 2015-01-20 13:53:05]

This is the follow up from https://codereview.chromium.org/817133006 to remove
the default format in ResourcePool now that it can handle multiple formats.

[reveman, 2015-01-20 18:45:48]

https://codereview.chromium.org/860813002/diff/1/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/860813002/diff/1/cc/test/layer_tree_test.h#ne...
cc/test/layer_tree_test.h:39: virtual ResourceFormat
CreateResourceAndTileTaskWorkerPool(
Would it make sense to add a virtual GetResourceFormat() function to the
TileTaskWorkerPool class instead?

[peterp, 2015-01-22 14:29:59]

https://codereview.chromium.org/860813002/diff/1/cc/test/layer_tree_test.h
File cc/test/layer_tree_test.h (right):

https://codereview.chromium.org/860813002/diff/1/cc/test/layer_tree_test.h#ne...
cc/test/layer_tree_test.h:39: virtual ResourceFormat
CreateResourceAndTileTaskWorkerPool(
On 2015/01/20 18:45:48, reveman wrote:
> Would it make sense to add a virtual GetResourceFormat() function to the
> TileTaskWorkerPool class instead?

Yes, that might be cleaner. Patch updated.

[reveman, 2015-01-22 23:01:46]

https://codereview.chromium.org/860813002/diff/40001/cc/resources/gpu_tile_ta...
File cc/resources/gpu_tile_task_worker_pool.h (right):

https://codereview.chromium.org/860813002/diff/40001/cc/resources/gpu_tile_ta...
cc/resources/gpu_tile_task_worker_pool.h:23: ResourceFormat resource_format);
Can this just use resource_provider->best_texture_format() internally instead of
having to take a resource_format as parameter? I think that would be a bit more
consistent with other TileTaskWorkerPool implementations.

https://codereview.chromium.org/860813002/diff/40001/cc/resources/one_copy_ti...
File cc/resources/one_copy_tile_task_worker_pool.h (right):

https://codereview.chromium.org/860813002/diff/40001/cc/resources/one_copy_ti...
cc/resources/one_copy_tile_task_worker_pool.h:110: ResourceFormat
resource_format_;
is this member variable needed? just use
resource_provider->best_texture_format() without caching the result instead?

https://codereview.chromium.org/860813002/diff/40001/cc/resources/tile_task_w...
File cc/resources/tile_task_worker_pool.h (right):

https://codereview.chromium.org/860813002/diff/40001/cc/resources/tile_task_w...
cc/resources/tile_task_worker_pool.h:85: virtual ResourceFormat
GetResourceFormat() = 0;
How about we make this part of the TileTaskRunner interface instead so we can
avoid passing the format as a ctor parameter to the TileManager?

[peterp, 2015-01-23 13:19:06]

https://codereview.chromium.org/860813002/diff/40001/cc/resources/gpu_tile_ta...
File cc/resources/gpu_tile_task_worker_pool.h (right):

https://codereview.chromium.org/860813002/diff/40001/cc/resources/gpu_tile_ta...
cc/resources/gpu_tile_task_worker_pool.h:23: ResourceFormat resource_format);
On 2015/01/22 23:01:46, reveman wrote:
> Can this just use resource_provider->best_texture_format() internally instead
of
> having to take a resource_format as parameter? I think that would be a bit
more
> consistent with other TileTaskWorkerPool implementations.

Then we have to pass in and store the ResourceProvider instead. But I guess it
doesn't really matter if we store a pointer to that instead of an enum, and
you're right, it would make it more consistent. I'll do that.

https://codereview.chromium.org/860813002/diff/40001/cc/resources/one_copy_ti...
File cc/resources/one_copy_tile_task_worker_pool.h (right):

https://codereview.chromium.org/860813002/diff/40001/cc/resources/one_copy_ti...
cc/resources/one_copy_tile_task_worker_pool.h:110: ResourceFormat
resource_format_;
On 2015/01/22 23:01:46, reveman wrote:
> is this member variable needed? just use
> resource_provider->best_texture_format() without caching the result instead?

Alright. Done.

https://codereview.chromium.org/860813002/diff/40001/cc/resources/tile_task_w...
File cc/resources/tile_task_worker_pool.h (right):

https://codereview.chromium.org/860813002/diff/40001/cc/resources/tile_task_w...
cc/resources/tile_task_worker_pool.h:85: virtual ResourceFormat
GetResourceFormat() = 0;
On 2015/01/22 23:01:46, reveman wrote:
> How about we make this part of the TileTaskRunner interface instead so we can
> avoid passing the format as a ctor parameter to the TileManager?

Good idea. Done.

[reveman, 2015-01-23 16:41:38]

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

[reveman, 2015-01-23 16:41:42]

lgtm with nits but please have vmpstr take a look at this before you land it.
Things have changed a bit since last time I worked on this code so I'm not 100%
sure GetResourceFormat as part of the TileTaskRunner interface is correct. Maybe
it should be part of the Rasterizer interface..

https://codereview.chromium.org/860813002/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/860813002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:294: ResourceFormat resource_format_;
nit: Do we still need this member variable? Can we just use
tile_task_runner_->GetResourceFormat() everywhere instead?

https://codereview.chromium.org/860813002/diff/140001/cc/resources/tile_task_...
File cc/resources/tile_task_runner.h (right):

https://codereview.chromium.org/860813002/diff/140001/cc/resources/tile_task_...
cc/resources/tile_task_runner.h:163: // Returns the most appropriate format to
use.
nit: Sounds like it would be ok to use a different format and ignore the return
value of this function. That's not really the case, is it? Should we make it
clear that it's required to use the format returned by this function?

[vmpstr, 2015-01-24 00:22:05]

On 2015/01/23 16:41:42, reveman wrote:
> lgtm with nits but please have vmpstr take a look at this before you land it.
> Things have changed a bit since last time I worked on this code so I'm not
100%
> sure GetResourceFormat as part of the TileTaskRunner interface is correct.
Maybe
> it should be part of the Rasterizer interface..
> 

I think this is fine. The rasterizer doesn't currently have enough information
to determine the best resource format to use. The reason is that we really only
have software/hardware rasterizers, the fact that software might use async
uploads, zero, or one copy is only exposed to the tile manager via the tile task
runner (that is, the rasterizer is not in a good position to determine this).
This might change in the future if the tile task runner becomes owned by the
rasterizer instead of tile manager, but that's a separate refactor.

What I'm a bit confused about is how this is going to be used. From what is
currently here, tile manager gets a resource_format_ from tile task runner and
it uses it to create resources, but then why have this on tile task runner at
all? Can't we set the resource pool to have "best_texture_format" at the time of
creation and just use that throughout? Or am I missing something?

[peterp, 2015-01-26 17:05:11]

> What I'm a bit confused about is how this is going to be used. From what is
> currently here, tile manager gets a resource_format_ from tile task runner and
> it uses it to create resources, but then why have this on tile task runner at
> all? Can't we set the resource pool to have "best_texture_format" at the time
of
> creation and just use that throughout? Or am I missing something?

The resource pool now supports multiple formats and there won't always be one
best format. This is one of the support patches needed for the tile compression
feature and if it's enabled then it might use different formats for opaque and
translucent tiles for example. The idea is to have some logic in the TileManager
to determine which format to use per tile.

[peterp, 2015-01-26 17:05:28]

https://codereview.chromium.org/860813002/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/860813002/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.h:294: ResourceFormat resource_format_;
On 2015/01/23 16:41:42, reveman wrote:
> nit: Do we still need this member variable? Can we just use
> tile_task_runner_->GetResourceFormat() everywhere instead?

You're right. I've remove it.

https://codereview.chromium.org/860813002/diff/140001/cc/resources/tile_task_...
File cc/resources/tile_task_runner.h (right):

https://codereview.chromium.org/860813002/diff/140001/cc/resources/tile_task_...
cc/resources/tile_task_runner.h:163: // Returns the most appropriate format to
use.
On 2015/01/23 16:41:42, reveman wrote:
> nit: Sounds like it would be ok to use a different format and ignore the
return
> value of this function. That's not really the case, is it? Should we make it
> clear that it's required to use the format returned by this function?

In the current version of the code it needs to use this format.

Later on, that's only going to be true for the hardware rasterizer. With tile
compression and the software rasterizer enabled it may use different formats
depending on the tile.

I'll change the comment.

[vmpstr, 2015-01-26 18:02:17]

On 2015/01/26 17:05:11, peterp wrote:
> > What I'm a bit confused about is how this is going to be used. From what is
> > currently here, tile manager gets a resource_format_ from tile task runner
and
> > it uses it to create resources, but then why have this on tile task runner
at
> > all? Can't we set the resource pool to have "best_texture_format" at the
time
> of
> > creation and just use that throughout? Or am I missing something?
> 
> The resource pool now supports multiple formats and there won't always be one
> best format. This is one of the support patches needed for the tile
compression
> feature and if it's enabled then it might use different formats for opaque and
> translucent tiles for example. The idea is to have some logic in the
TileManager
> to determine which format to use per tile.

The only thing that makes it a bit iffy in this patch is the fact that gpu
rasterizer doesn't actually use tile task runner for anything except getting
this texture format. That's something to be mindful of when refactoring
(+hendrikw). Other than that, lgtm.

[vmpstr, 2015-01-26 18:02:31]

vmpstr@chromium.org changed reviewers:
+ hendrikw@chromium.org

[peterp, 2015-01-27 08:11:19]

The CQ bit was checked by peterp@opera.com

[commit-bot: I haz the power, 2015-01-27 08:11:43]

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

[commit-bot: I haz the power, 2015-01-27 09:34:36]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-01-27 09:35:38]

Patchset 9 (id:??) landed as
https://crrev.com/66304d3c5632199f46abb85b1acca5d87f03ce49
Cr-Commit-Position: refs/heads/master@{#313247}