Allow task pools to reason about transparency.

Allow task pools to reason about transparency.

This will allow task pools to select different formats depending on if a tile contains transparency or not.

BUG=434699
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[christiank, 2015-09-22 12:29:07]

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

[christiank, 2015-09-22 12:29:08]

Hi,

This CL adds support for selecting different resource formats depending on if a
tile is opaque or contains transparency. It's needed by the tile compression
feature to prevent the use of ETC1 when there is need for alpha.

This code is based on the code in:
https://codereview.chromium.org/1197423003/

David, you said it could be a separate CL which makes sense so here it is. Sorry
for the delay.

I plan on getting back to the review above once this has landed.

[reveman, 2015-09-22 14:18:49]

looks good. just some questions and minor requests.

https://codereview.chromium.org/1351283003/diff/1/cc/raster/tile_task_runner.h
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1351283003/diff/1/cc/raster/tile_task_runner....
cc/raster/tile_task_runner.h:152: virtual ResourceFormat GetResourceFormat(bool
must_support_alpha) const = 0;
This looks good for now. What do you think about adding something like this in
the future:

enum ResourceFormatDescription {
  COLOR,
  COLOR_AND_ALPHA,
  UNCOMPRESSED_COLOR_AND_ALPHA
};

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

https://codereview.chromium.org/1351283003/diff/1/cc/resources/resource_forma...
cc/resources/resource_format.h:40: CC_EXPORT ResourceFormat
BufferFormatToResourceFormat(gfx::BufferFormat format);
Can we add the resource format to the StagingBuffer struct instead?

https://codereview.chromium.org/1351283003/diff/1/cc/resources/resource_provi...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1351283003/diff/1/cc/resources/resource_provi...
cc/resources/resource_provider.h:97: ResourceFormat
memory_efficient_texture_format(bool must_support_alpha) const;
Can we remove this function from ResourceProvider and instead pass the
use_rgba_4444_texture_format_ and future compressed texture settings directly to
the TileTaskWorkerPool impls? That's the only place we use this function so it
doesn't really need to be here. It just adds additional plumbing and makes the
code harder to follow imo.

https://codereview.chromium.org/1351283003/diff/1/cc/test/fake_resource_provi...
File cc/test/fake_resource_provider.h (right):

https://codereview.chromium.org/1351283003/diff/1/cc/test/fake_resource_provi...
cc/test/fake_resource_provider.h:47: bool use_tile_compression,
is this used?

[christiank, 2015-09-23 12:29:12]

https://codereview.chromium.org/1351283003/diff/1/cc/raster/tile_task_runner.h
File cc/raster/tile_task_runner.h (right):

https://codereview.chromium.org/1351283003/diff/1/cc/raster/tile_task_runner....
cc/raster/tile_task_runner.h:152: virtual ResourceFormat GetResourceFormat(bool
must_support_alpha) const = 0;
On 2015/09/22 14:18:49, reveman wrote:
> This looks good for now. What do you think about adding something like this in
> the future:
> 
> enum ResourceFormatDescription {
>   COLOR,
>   COLOR_AND_ALPHA,
>   UNCOMPRESSED_COLOR_AND_ALPHA
> };

I like it! Much better than the old "two boolean parameters approach" that I
suggested a while back.

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

https://codereview.chromium.org/1351283003/diff/1/cc/resources/resource_forma...
cc/resources/resource_format.h:40: CC_EXPORT ResourceFormat
BufferFormatToResourceFormat(gfx::BufferFormat format);
On 2015/09/22 14:18:49, reveman wrote:
> Can we add the resource format to the StagingBuffer struct instead?

Done.

https://codereview.chromium.org/1351283003/diff/1/cc/resources/resource_provi...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/1351283003/diff/1/cc/resources/resource_provi...
cc/resources/resource_provider.h:97: ResourceFormat
memory_efficient_texture_format(bool must_support_alpha) const;
On 2015/09/22 14:18:49, reveman wrote:
> Can we remove this function from ResourceProvider and instead pass the
> use_rgba_4444_texture_format_ and future compressed texture settings directly
to
> the TileTaskWorkerPool impls? That's the only place we use this function so it
> doesn't really need to be here. It just adds additional plumbing and makes the
> code harder to follow imo.

Makes sense, fixed.

https://codereview.chromium.org/1351283003/diff/1/cc/test/fake_resource_provi...
File cc/test/fake_resource_provider.h (right):

https://codereview.chromium.org/1351283003/diff/1/cc/test/fake_resource_provi...
cc/test/fake_resource_provider.h:47: bool use_tile_compression,
On 2015/09/22 14:18:49, reveman wrote:
> is this used?

Oopps, sloppy copy from other patch. Now removed.

[reveman, 2015-09-23 13:42:46]

lgtm with nit

https://codereview.chromium.org/1351283003/diff/40001/cc/raster/one_copy_tile...
File cc/raster/one_copy_tile_task_worker_pool.h (right):

https://codereview.chromium.org/1351283003/diff/40001/cc/raster/one_copy_tile...
cc/raster/one_copy_tile_task_worker_pool.h:100: explicit StagingBuffer(const
gfx::Size& size, ResourceFormat format);
nit: can remove explicit

[christiank, 2015-09-23 14:02:55]

The CQ bit was checked by christiank@opera.com

[christiank, 2015-09-23 14:02:56]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/1351283003/#ps60001
(title: "Make StagingBuffer constructor non-explicit")

[commit-bot: I haz the power, 2015-09-23 14:04:53]

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

[commit-bot: I haz the power, 2015-09-23 14:10:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-23 14:10:46]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[christiank, 2015-09-24 06:10:10]

The CQ bit was checked by christiank@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-09-24 06:10:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1351283003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1351283003/60001

[commit-bot: I haz the power, 2015-09-24 07:28:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-24 07:28:23]

Dry run: This issue passed the CQ dry run.

[christiank, 2015-09-24 07:29:07]

The CQ bit was checked by christiank@opera.com

[commit-bot: I haz the power, 2015-09-24 07:29:25]

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

[commit-bot: I haz the power, 2015-09-24 07:48:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-24 07:49:02]

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