cc: Make occlusion a draw property.

cc: Make occlusion a draw property.

We were computing occlusion twice, once in UpdateDrawProperties
and once in CalculateRenderPasses. Now just do it once in
UpdateDrawProperties, which means we don't have to do that work
at all if we draw again without dirtying the render surface layer
list.

This moves the call to UpdateTiles out of the loop for occlusion,
instead calling it directly on each of the picture layers on the
tree, meaning it does not have to be virtual anymore.

This makes it possible for us to call UpdateTiles on picture layers
that were not part of the RenderSurfaceLayerList. Though for now
I left the behaviour as is with a TODO.

Also removed all the old LayerTreeHostOcclusionTests. Added a few
new ones to ensure occlusion gets saved correctly in layers and
surfaces. The test cases that would be missing, I moved to be
OcclusionTrackerTests.

R=enne, vmpstr
BUG=446751
Committed: https://crrev.com/4902c30c928ceb9f14366d9a6cad5ef85968a058
Cr-Commit-Position: refs/heads/master@{#316306}

[danakj, 2015-02-13 00:00:45]

Oops. Now with a description and less virtuals.

[vmpstr, 2015-02-13 00:24:54]

https://codereview.chromium.org/915083004/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/915083004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:751: // Grab this region here before iterating
layers. Taking copy requests from
Do you mean to say that UnoccludedScreenSpaceRegion might change it's value
during the iteration? If so and that's not acceptable, then in this case you
probably want a copy not a const ref.

If you do want the const ref and you expect that the value of it will change,
then I might be misunderstanding the comment.

https://codereview.chromium.org/915083004/diff/40001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/915083004/diff/40001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:612: it->render_surface()->draw_transform() *
This is probably lack of knowledge for my part, but why is it that for a layer
we use it->draw_transform() but for a mask layer we use
render_surface->draw_transform() * it->draw_transform()?

https://codereview.chromium.org/915083004/diff/40001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:647: layer->UpdateTiles(occlusion,
resourceless_software_draw);
mask and replica mask also need update tiles, right?

[danakj, 2015-02-13 00:27:07]

https://codereview.chromium.org/915083004/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/915083004/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:751: // Grab this region here before iterating
layers. Taking copy requests from
On 2015/02/13 00:24:54, vmpstr wrote:
> Do you mean to say that UnoccludedScreenSpaceRegion might change it's value
> during the iteration? If so and that's not acceptable, then in this case you
> probably want a copy not a const ref.
> 
> If you do want the const ref and you expect that the value of it will change,
> then I might be misunderstanding the comment.

No, i mean that the dirty bit is set to true so asking for it DCHECKs. I'll
clarify.

https://codereview.chromium.org/915083004/diff/40001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/915083004/diff/40001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:612: it->render_surface()->draw_transform() *
On 2015/02/13 00:24:54, vmpstr wrote:
> This is probably lack of knowledge for my part, but why is it that for a layer
> we use it->draw_transform() but for a mask layer we use
> render_surface->draw_transform() * it->draw_transform()?

The mask is used when drawing the surface, so I think it should have the same
occlusion as the surface, ie it should not be occluded by things inside the
surface.

[enne (OOO), 2015-02-13 00:30:24]

https://codereview.chromium.org/915083004/diff/40001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/915083004/diff/40001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:612: it->render_surface()->draw_transform() *
On 2015/02/13 00:27:07, danakj wrote:
> On 2015/02/13 00:24:54, vmpstr wrote:
> > This is probably lack of knowledge for my part, but why is it that for a
layer
> > we use it->draw_transform() but for a mask layer we use
> > render_surface->draw_transform() * it->draw_transform()?
> 
> The mask is used when drawing the surface, so I think it should have the same
> occlusion as the surface, ie it should not be occluded by things inside the
> surface.

Occlusion on masks is not used anywhere, as it's redundant with the surface. 
(If you did use it, what would you do if the mask and surface disagreed?)

I'd say to just leave the mask occlusion unset / totally unoccluded.

[danakj, 2015-02-13 00:34:12]

On Thu, Feb 12, 2015 at 4:30 PM, <enne@chromium.org> wrote:

>
> https://codereview.chromium.org/915083004/diff/40001/cc/
> trees/layer_tree_impl.cc
> File cc/trees/layer_tree_impl.cc (right):
>
> https://codereview.chromium.org/915083004/diff/40001/cc/
> trees/layer_tree_impl.cc#newcode612
> cc/trees/layer_tree_impl.cc:612: it->render_surface()->draw_transform()
> *
> On 2015/02/13 00:27:07, danakj wrote:
>
>> On 2015/02/13 00:24:54, vmpstr wrote:
>> > This is probably lack of knowledge for my part, but why is it that
>>
> for a layer
>
>> > we use it->draw_transform() but for a mask layer we use
>> > render_surface->draw_transform() * it->draw_transform()?
>>
>
>  The mask is used when drawing the surface, so I think it should have
>>
> the same
>
>> occlusion as the surface, ie it should not be occluded by things
>>
> inside the
>
>> surface.
>>
>
> Occlusion on masks is not used anywhere, as it's redundant with the
> surface.  (If you did use it, what would you do if the mask and surface
> disagreed?)
>

It would be used (incorrectly if there's a replica currently) for tile
occlusion culling for raster.


> I'd say to just leave the mask occlusion unset / totally unoccluded.
>
> https://codereview.chromium.org/915083004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmpstr, 2015-02-13 00:56:32]

lgtm % enne

[danakj, 2015-02-13 01:25:47]

PTAL I changed replica behaviour and added unit test to demonstrate it. Now
there's no occlusion on any layers in the subtree of a replica.

[danakj, 2015-02-13 01:26:37]

New patchsets have been uploaded after l-g-t-m from vmpstr@chromium.org

[enne (OOO), 2015-02-13 18:26:25]

https://codereview.chromium.org/915083004/diff/80001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/915083004/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:599: inside_replica = true;
And break? or while (layer && !inside_replica?)

https://codereview.chromium.org/915083004/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:635:
mask->draw_properties().occlusion_in_content_space = mask_occlusion;
Can you point me at tests verifying that masks are occluded correctly? I don't
think we set draw transforms on masks, so some of this is a little weird.

[danakj, 2015-02-13 18:33:27]

https://codereview.chromium.org/915083004/diff/80001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/915083004/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:599: inside_replica = true;
On 2015/02/13 18:26:24, enne wrote:
> And break? or while (layer && !inside_replica?)

oh ya. i had a forloop that broke on inside_replica before but i lost that.
thanks

https://codereview.chromium.org/915083004/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:635:
mask->draw_properties().occlusion_in_content_space = mask_occlusion;
On 2015/02/13 18:26:24, enne wrote:
> Can you point me at tests verifying that masks are occluded correctly? I don't
> think we set draw transforms on masks, so some of this is a little weird.

we dont. it uses the draw transform of the owning layer into the surface, and
the draw transform of the surface into its target, which is where the occlusion
is. i added a test in this CL that verifies this behavior. but to prove it's
correct.. other than to reason it out and then say the test guards it.. i dont
think we have something.

umm..hmm.. i could query is occluded on the occlusion object but thats no better
than verifying the occlusion object is what we expect.

i was thinking about if the layer and mask have different scale tilings but
thats not part of the draw transform, its all in layer space now and the tilings
themselves each deal with their own scale.

so i guess im not sure what test to add here. thoughts?

[enne (OOO), 2015-02-13 18:38:33]

lgtm

https://codereview.chromium.org/915083004/diff/80001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/915083004/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:635:
mask->draw_properties().occlusion_in_content_space = mask_occlusion;
Ok, I looked at the test and looked at this closely again, and it seems like
this covers it.

[danakj, 2015-02-13 20:59:39]

New patchsets have been uploaded after l-g-t-m from enne@chromium.org

[danakj, 2015-02-13 21:12:28]

https://codereview.chromium.org/915083004/diff/80001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/915083004/diff/80001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:599: inside_replica = true;
On 2015/02/13 18:33:27, danakj wrote:
> On 2015/02/13 18:26:24, enne wrote:
> > And break? or while (layer && !inside_replica?)
> 
> oh ya. i had a forloop that broke on inside_replica before but i lost that.
> thanks

Done.

[danakj, 2015-02-13 21:12:31]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-02-13 21:12:58]

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

[commit-bot: I haz the power, 2015-02-13 22:12:25]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-13 22:12:58]

Patchset 7 (id:??) landed as
https://crrev.com/4902c30c928ceb9f14366d9a6cad5ef85968a058
Cr-Commit-Position: refs/heads/master@{#316306}