Move occlusion info to TilePriority.

Move occlusion info to TilePriority.

This stores the occlusion information on the TilePriority so that the
same tile can have different occlusion on each tree.

BUG=178971

[danakj, 2014-06-17 22:29:41]

I think this LG but I'm not so familiar with how TilePriority works.

If we don't share a tile between pending/active tree, because of invalidation on
the pending tree, do we expect the priority of the other tree to be valid on the
Tiles still?

https://codereview.chromium.org/343463004/diff/1/cc/layers/picture_layer_impl...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/343463004/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3010: const Tile* tile = *iter;
can you show with EXPECT checks that these Tile* are not shared with the active
tree so that you can demonstrate these two loops are doing different things?

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

https://codereview.chromium.org/343463004/diff/1/cc/resources/tile_priority.h...
cc/resources/tile_priority.h:62: TilePriority(const TilePriority& active, const
TilePriority& pending) {
What is this constructor used for? I'm not familiar with this.

[vmpstr, 2014-06-17 23:41:58]

On 2014/06/17 22:29:41, danakj wrote:
> I think this LG but I'm not so familiar with how TilePriority works.
> 
> If we don't share a tile between pending/active tree, because of invalidation
on
> the pending tree, do we expect the priority of the other tree to be valid on
the
> Tiles still?
> 

It's valid, but default, which makes it the lowest possible priority. So in
cases like SAME_PRIORITY_FOR_BOTH_TREES the tiles end up effectively using the
priority that _was_ set. In other cases like SMOOTHNESS_TAKES_PRIORITY, if
active tree priority was not set, it will end up being in the NEVER_BIN (and in
iterators case, it won't be visited from the active tree, since it's not there)

[vmpstr, 2014-06-17 23:42:10]

https://codereview.chromium.org/343463004/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/343463004/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:498: tile->SetIsOccluded(tree,
is_occluded);
I don't really like the fact that this is very brittle with respect to order.
That is, if we SetIsOccluded(tree, is_occluded) before SetPriority, then
SetPriority will wipe that flag away again.

It might be worth it to include "is_occluded" in the constructor of tile
priority. Can you do a quick check to see if that would work?

As an aside, why do we only care about visible rect occlusion? Should the loops
below have similar things or is it something that can only be in the visible
bin?

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

https://codereview.chromium.org/343463004/diff/1/cc/resources/tile_priority.h...
cc/resources/tile_priority.h:62: TilePriority(const TilePriority& active, const
TilePriority& pending) {
On 2014/06/17 22:29:41, danakj wrote:
> What is this constructor used for? I'm not familiar with this.

It's computing "combined" priority used in SAME_PRIORITY_FOR_BOTH_TREES (ugh...)

[jbedley, 2014-06-18 23:00:39]

https://codereview.chromium.org/343463004/diff/1/cc/layers/picture_layer_impl...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/343463004/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3010: const Tile* tile = *iter;
On 2014/06/17 22:29:41, danakj wrote:
> can you show with EXPECT checks that these Tile* are not shared with the
active
> tree so that you can demonstrate these two loops are doing different things?

Actually both loops were doing the same thing because all the tiles were shared
before. I've modified the test so that we get both shared and unshared tiles
now. We also get various combinations of tiles being occluded on different
trees.

https://codereview.chromium.org/343463004/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/343463004/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling.cc:498: tile->SetIsOccluded(tree,
is_occluded);
On 2014/06/17 23:42:10, vmpstr wrote:
> I don't really like the fact that this is very brittle with respect to order.
> That is, if we SetIsOccluded(tree, is_occluded) before SetPriority, then
> SetPriority will wipe that flag away again.
> 
> It might be worth it to include "is_occluded" in the constructor of tile
> priority. Can you do a quick check to see if that would work?

Yes, is_occluded works in the constructor. I removed the SetIsOccluded()
function altogether, so that we just call SetPriority() once here, with either
true or false for the is_occluded bit.

> As an aside, why do we only care about visible rect occlusion? Should the
loops
> below have similar things or is it something that can only be in the visible
> bin?

I think we only care to identify occluded tiles from what's visible. If a tile
is not visible then the is_occluded bit is not applicable (although by default
it will be false).

[danakj, 2014-06-19 19:19:59]

few last comments, but LGTM.

-> vmpstr for final reviews :)

https://codereview.chromium.org/343463004/diff/20001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/343463004/diff/20001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:524: resolution_, false,
TilePriority::SOON, distance_to_visible);
can you use a temp var here to give the |false| literal a name?

https://codereview.chromium.org/343463004/diff/20001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:545: resolution_, false,
TilePriority::EVENTUALLY, distance_to_visible);
can you use a temp var here to give the |false| literal a name?

https://codereview.chromium.org/343463004/diff/20001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:563: false,
can you use a temp var here to give the |false| literal a name?

https://codereview.chromium.org/343463004/diff/20001/cc/resources/tile_priori...
File cc/resources/tile_priority_unittest.cc (right):

https://codereview.chromium.org/343463004/diff/20001/cc/resources/tile_priori...
cc/resources/tile_priority_unittest.cc:11: TilePriority now(HIGH_RESOLUTION,
false, TilePriority::NOW, 0);
Can you add a few cases here to show that occlusion doesn't change priority?

https://codereview.chromium.org/343463004/diff/20001/cc/test/test_tile_priori...
File cc/test/test_tile_priorities.cc (right):

https://codereview.chromium.org/343463004/diff/20001/cc/test/test_tile_priori...
cc/test/test_tile_priorities.cc:17:
TilePriorityForNowBin::TilePriorityForNowBin()
Can you add Unoccluded to these names? like TilePriorityForUnoccludedNowBin?

For non-now we don't need to worry about occlusion so I think it's okay to leave
those.

[jbedley, 2014-06-19 20:51:20]

https://codereview.chromium.org/343463004/diff/20001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/343463004/diff/20001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:524: resolution_, false,
TilePriority::SOON, distance_to_visible);
On 2014/06/19 19:19:59, danakj wrote:
> can you use a temp var here to give the |false| literal a name?

Done.

https://codereview.chromium.org/343463004/diff/20001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:545: resolution_, false,
TilePriority::EVENTUALLY, distance_to_visible);
On 2014/06/19 19:19:59, danakj wrote:
> can you use a temp var here to give the |false| literal a name?

Done.

https://codereview.chromium.org/343463004/diff/20001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:563: false,
On 2014/06/19 19:19:59, danakj wrote:
> can you use a temp var here to give the |false| literal a name?

Done.

https://codereview.chromium.org/343463004/diff/20001/cc/resources/tile_priori...
File cc/resources/tile_priority_unittest.cc (right):

https://codereview.chromium.org/343463004/diff/20001/cc/resources/tile_priori...
cc/resources/tile_priority_unittest.cc:11: TilePriority now(HIGH_RESOLUTION,
false, TilePriority::NOW, 0);
On 2014/06/19 19:19:59, danakj wrote:
> Can you add a few cases here to show that occlusion doesn't change priority?

Done.

https://codereview.chromium.org/343463004/diff/20001/cc/test/test_tile_priori...
File cc/test/test_tile_priorities.cc (right):

https://codereview.chromium.org/343463004/diff/20001/cc/test/test_tile_priori...
cc/test/test_tile_priorities.cc:17:
TilePriorityForNowBin::TilePriorityForNowBin()
On 2014/06/19 19:19:59, danakj wrote:
> Can you add Unoccluded to these names? like TilePriorityForUnoccludedNowBin?
> 
> For non-now we don't need to worry about occlusion so I think it's okay to
leave
> those.

Done.

[jbedley, 2014-06-24 15:27:25]

On 2014/06/19 20:51:20, jbedley wrote:
>
https://codereview.chromium.org/343463004/diff/20001/cc/resources/picture_lay...
> File cc/resources/picture_layer_tiling.cc (right):
> 
>
https://codereview.chromium.org/343463004/diff/20001/cc/resources/picture_lay...
> cc/resources/picture_layer_tiling.cc:524: resolution_, false,
> TilePriority::SOON, distance_to_visible);
> On 2014/06/19 19:19:59, danakj wrote:
> > can you use a temp var here to give the |false| literal a name?
> 
> Done.
> 
>
https://codereview.chromium.org/343463004/diff/20001/cc/resources/picture_lay...
> cc/resources/picture_layer_tiling.cc:545: resolution_, false,
> TilePriority::EVENTUALLY, distance_to_visible);
> On 2014/06/19 19:19:59, danakj wrote:
> > can you use a temp var here to give the |false| literal a name?
> 
> Done.
> 
>
https://codereview.chromium.org/343463004/diff/20001/cc/resources/picture_lay...
> cc/resources/picture_layer_tiling.cc:563: false,
> On 2014/06/19 19:19:59, danakj wrote:
> > can you use a temp var here to give the |false| literal a name?
> 
> Done.
> 
>
https://codereview.chromium.org/343463004/diff/20001/cc/resources/tile_priori...
> File cc/resources/tile_priority_unittest.cc (right):
> 
>
https://codereview.chromium.org/343463004/diff/20001/cc/resources/tile_priori...
> cc/resources/tile_priority_unittest.cc:11: TilePriority now(HIGH_RESOLUTION,
> false, TilePriority::NOW, 0);
> On 2014/06/19 19:19:59, danakj wrote:
> > Can you add a few cases here to show that occlusion doesn't change priority?
> 
> Done.
> 
>
https://codereview.chromium.org/343463004/diff/20001/cc/test/test_tile_priori...
> File cc/test/test_tile_priorities.cc (right):
> 
>
https://codereview.chromium.org/343463004/diff/20001/cc/test/test_tile_priori...
> cc/test/test_tile_priorities.cc:17:
> TilePriorityForNowBin::TilePriorityForNowBin()
> On 2014/06/19 19:19:59, danakj wrote:
> > Can you add Unoccluded to these names? like TilePriorityForUnoccludedNowBin?
> > 
> > For non-now we don't need to worry about occlusion so I think it's okay to
> leave
> > those.
> 
> Done.

vmpstr, how does it look to you?

[danakj, 2014-06-26 15:42:31]

ping @ vmpstr

[vmpstr, 2014-07-07 14:06:11]

Hey, can you take a look at https://codereview.chromium.org/367833003/

I'm sorry to go back on what I said, but perhaps it's better to keep occlusion
as a tile property. The reason for this is that in my patch, I first determine
if the tile will need to be rasterized based on whether it has resources and
whether it's not occluded before updating the tile priority. We can always
change that patch as well (I need to get some initial feedback to see how people
feel about it). 

If we keep this patch, I can always work around it somehow but it seems that if
we can avoid priority calculation in some cases, it would be better. I'm happy
with this patch though, so I just want to get your thoughts on how to move
forward here. Without this patch, your changes to eviction are still applicable
except that we need to not use IsHigherPriority and instead explicitly compare
things like bin, then occlusion, then distance.

[jbedley, 2014-07-31 20:27:35]

Closing this in favour of https://codereview.chromium.org/374653003/