cc: Make occlusion tracker always work in target space.

cc: Make occlusion tracker always work in target space.

On desktop this improves LayerTreeHostPerfTestJsonReader.tenTenSingleThread from ~3702.97 runs/s to ~3786.95 runs/s for about 2%. On a phone we observed about 20% reduction in calcRenderPasses in tracing.

Covered by the occlusion tracker tests as well as LayerTreeHost and TiledLayer tests with occlusion.

BUG=163035
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173411

[jamesr, 2012-12-16 00:27:54]

I *think* I understand this well enough to review, but if you want enne to take
a look as well that wouldn't be a terrible idea.

lgtm

https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc
File cc/occlusion_tracker.cc (right):

https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc#ne...
cc/occlusion_tracker.cc:420: // Check if the unoccluded rect shank when applying
outside occlusion.
typo shank -> shrank

https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc#ne...
cc/occlusion_tracker.cc:479: // Check if the unoccluded rect shank when applying
outside occlusion.
same typo

[danakj, 2012-12-16 00:38:53]

Thanks. Would definitely appreciate an enne review as well if possible :)

On Saturday, December 15, 2012, wrote:

> I *think* I understand this well enough to review, but if you want enne to
> take
> a look as well that wouldn't be a terrible idea.
>
> lgtm
>
>
> https://codereview.chromium.**org/11583005/diff/3002/cc/**
>
occlusion_tracker.cc<https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc>
> File cc/occlusion_tracker.cc (right):
>
> https://codereview.chromium.**org/11583005/diff/3002/cc/**
>
occlusion_tracker.cc#**newcode420<https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc#newcode420>
> cc/occlusion_tracker.cc:420: // Check if the unoccluded rect shank when
> applying outside occlusion.
> typo shank -> shrank
>
> https://codereview.chromium.**org/11583005/diff/3002/cc/**
>
occlusion_tracker.cc#**newcode479<https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc#newcode479>
> cc/occlusion_tracker.cc:479: // Check if the unoccluded rect shank when
> applying outside occlusion.
> same typo
>
>
https://codereview.chromium.**org/11583005/<https://codereview.chromium.org/1...
>
> --
>
>
>

[enne (OOO), 2012-12-16 19:44:24]

I'm having a hard time following where the speedups are coming from.

Is the speedup because the root does less work here (no need to transform to
screenspace if the target is the root)? Or, is it because you transform regions
per-rect into a new target space rather than transforming per-occluder into
screen space?

https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc
File cc/occlusion_tracker.cc (right):

https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc#ne...
cc/occlusion_tracker.cc:57: static gfx::Rect
screenSpaceClipRectInTargetSurface(const RenderSurfaceType* targetSurface,
gfx::Rect screenSpaceClipRect)
Do we not make this projection anywhere in LayerTreeHostCommon? Or, is that just
for layers?

https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc#ne...
cc/occlusion_tracker.cc:81: // TODO(danakj): If the Region is too complex,
degrade gracefully here by skipping rects in it.
I think it'd be great if we had a Region-derived class with a fixed size that
could just be dropped in.  I mean, you'd have to pick whether you preferred
false negatives or false positives, but it'd be a good abstraction to have in
our toolkit.

https://codereview.chromium.org/11583005/diff/3002/cc/tiled_layer_unittest.cc
File cc/tiled_layer_unittest.cc (right):

https://codereview.chromium.org/11583005/diff/3002/cc/tiled_layer_unittest.cc...
cc/tiled_layer_unittest.cc:118: void calcDrawProps(const
scoped_refptr<FakeTiledLayer>& layer1)
Yay, calling calcDrawProperties.

Although, functions taking const scoped_refptr<>& is a little weird.  Is this
just to avoid typing .get()?

https://codereview.chromium.org/11583005/diff/3002/cc/tiled_layer_unittest.cc...
cc/tiled_layer_unittest.cc:1224: layer->drawProperties().target_space_transform
= drawTransform;
If you're calling calcDrawProperties, can all this manual setting of draw
properties get removed?

[danakj, 2012-12-16 20:16:52]

https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc
File cc/occlusion_tracker.cc (right):

https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc#ne...
cc/occlusion_tracker.cc:57: static gfx::Rect
screenSpaceClipRectInTargetSurface(const RenderSurfaceType* targetSurface,
gfx::Rect screenSpaceClipRect)
On 2012/12/16 19:44:25, enne wrote:
> Do we not make this projection anywhere in LayerTreeHostCommon? Or, is that
just
> for layers?

This looks a lot like clipRectFromAncestor for contributing surfaces to the
root. For other contributing surfaces (distance 2 from the root), this is more
aggressive than calcDrawProps, I think. calcDrawProps wants a clipRect only when
required for GL rendering correctness, whereas this wants the minimum clipRect
possible while maintaining correctness. I think trying to use something from
calcDrawProps here could introduce bugs, at this moment. I'd like to investigate
this though, how about I file a bug?

https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc#ne...
cc/occlusion_tracker.cc:81: // TODO(danakj): If the Region is too complex,
degrade gracefully here by skipping rects in it.
On 2012/12/16 19:44:25, enne wrote:
> I think it'd be great if we had a Region-derived class with a fixed size that
> could just be dropped in.  I mean, you'd have to pick whether you preferred
> false negatives or false positives, but it'd be a good abstraction to have in
> our toolkit.

I will make a bug for this too.

https://codereview.chromium.org/11583005/diff/3002/cc/tiled_layer_unittest.cc
File cc/tiled_layer_unittest.cc (right):

https://codereview.chromium.org/11583005/diff/3002/cc/tiled_layer_unittest.cc...
cc/tiled_layer_unittest.cc:118: void calcDrawProps(const
scoped_refptr<FakeTiledLayer>& layer1)
On 2012/12/16 19:44:25, enne wrote:
> Yay, calling calcDrawProperties.
> 
> Although, functions taking const scoped_refptr<>& is a little weird.  Is this
> just to avoid typing .get()?

It's roughly equivilent to passing by value. I don't feel strongly about either
way. Some of chromium does one, some does the other apparently, judging from the
chromium-dev thread about this.

Want me to switch it?

https://codereview.chromium.org/11583005/diff/3002/cc/tiled_layer_unittest.cc...
cc/tiled_layer_unittest.cc:1224: layer->drawProperties().target_space_transform
= drawTransform;
On 2012/12/16 19:44:25, enne wrote:
> If you're calling calcDrawProperties, can all this manual setting of draw
> properties get removed?

Yes, but then it needs to move to other properties. This might require a bit
more thought. I don't mind the "set draw props then override the ones i care
about testing" pattern here. It can be more clear than setting up elaborate
layer scenes just to get the right output from calcDrawProps, and requires
internal knowledge about how to generate a draw transform in calcDrawProps,
which maybe is not useful and breaks the encapsulation a bit, since this is not
testing calcDraw? (not that it would be elaborate here, just thinking about the
pattern)

[danakj, 2012-12-16 20:21:19]

Oh, the speed up is from the former. We reduce the number of map/projects that
are required. We also reduce the number of temp regions that are constructed.
Now we can subtract occlusion from inside and outside of the target from the
same temp region. Before we'd have to construct one for each space.

I think most of the win is:
- from fewer map/project (most of the time in profiling results before from
inside occlusion tracker was spent in map and project, which motivates this
change). 
- from having a clear place to drop complexity if we need to in the future
(where that TODO lies).

[enne (OOO), 2012-12-16 20:53:17]

lgtm.  Punting on the other things sounds fine.  I think this is a clear win
that we should land.

https://codereview.chromium.org/11583005/diff/3002/cc/tiled_layer_unittest.cc
File cc/tiled_layer_unittest.cc (right):

https://codereview.chromium.org/11583005/diff/3002/cc/tiled_layer_unittest.cc...
cc/tiled_layer_unittest.cc:118: void calcDrawProps(const
scoped_refptr<FakeTiledLayer>& layer1)
On 2012/12/16 20:16:52, danakj wrote:
> On 2012/12/16 19:44:25, enne wrote:
> > Yay, calling calcDrawProperties.
> > 
> > Although, functions taking const scoped_refptr<>& is a little weird.  Is
this
> > just to avoid typing .get()?
> 
> It's roughly equivilent to passing by value. I don't feel strongly about
either
> way. Some of chromium does one, some does the other apparently, judging from
the
> chromium-dev thread about this.
> 
> Want me to switch it?

Oh.  I didn't realize that this was an idiom for other folks.  That's fine,
then.

https://codereview.chromium.org/11583005/diff/3002/cc/tiled_layer_unittest.cc...
cc/tiled_layer_unittest.cc:1224: layer->drawProperties().target_space_transform
= drawTransform;
On 2012/12/16 20:16:52, danakj wrote:
> On 2012/12/16 19:44:25, enne wrote:
> > If you're calling calcDrawProperties, can all this manual setting of draw
> > properties get removed?
> 
> Yes, but then it needs to move to other properties. This might require a bit
> more thought. I don't mind the "set draw props then override the ones i care
> about testing" pattern here. It can be more clear than setting up elaborate
> layer scenes just to get the right output from calcDrawProps, and requires
> internal knowledge about how to generate a draw transform in calcDrawProps,
> which maybe is not useful and breaks the encapsulation a bit, since this is
not
> testing calcDraw? (not that it would be elaborate here, just thinking about
the
> pattern)

Yeah, I think it breaks the encapsulation a little bit.  I'm willing to punt,
but it seems like it'd be cleaner if drawable properties were only set by
calcDrawProperties.  I think if you're ever in a case where "calling
calcDrawProperties will break this carefully constructed scene" then maybe
you're doing something wrong and fragile.  I'm willing to punt on changing it in
this patch, though.

[danakj, 2012-12-16 21:26:20]

On Sun, Dec 16, 2012 at 3:53 PM,  <enne@chromium.org> wrote:
> lgtm.  Punting on the other things sounds fine.  I think this is a clear win
> that we should land.

kk, thanks for review.

>
> https://codereview.chromium.org/11583005/diff/3002/cc/tiled_layer_unittest.cc
> File cc/tiled_layer_unittest.cc (right):
>
>
https://codereview.chromium.org/11583005/diff/3002/cc/tiled_layer_unittest.cc...
> cc/tiled_layer_unittest.cc:118: void calcDrawProps(const
> scoped_refptr<FakeTiledLayer>& layer1)
> On 2012/12/16 20:16:52, danakj wrote:
>>
>> On 2012/12/16 19:44:25, enne wrote:
>> > Yay, calling calcDrawProperties.
>> >
>> > Although, functions taking const scoped_refptr<>& is a little weird.
>
>  Is this
>>
>> > just to avoid typing .get()?
>
>
>> It's roughly equivilent to passing by value. I don't feel strongly
>
> about either
>>
>> way. Some of chromium does one, some does the other apparently,
>
> judging from the
>>
>> chromium-dev thread about this.
>
>
>> Want me to switch it?
>
>
> Oh.  I didn't realize that this was an idiom for other folks.  That's
> fine, then.
>
>
>
https://codereview.chromium.org/11583005/diff/3002/cc/tiled_layer_unittest.cc...
> cc/tiled_layer_unittest.cc:1224:
> layer->drawProperties().target_space_transform = drawTransform;
> On 2012/12/16 20:16:52, danakj wrote:
>>
>> On 2012/12/16 19:44:25, enne wrote:
>> > If you're calling calcDrawProperties, can all this manual setting of
>
> draw
>>
>> > properties get removed?
>
>
>> Yes, but then it needs to move to other properties. This might require
>
> a bit
>>
>> more thought. I don't mind the "set draw props then override the ones
>
> i care
>>
>> about testing" pattern here. It can be more clear than setting up
>
> elaborate
>>
>> layer scenes just to get the right output from calcDrawProps, and
>
> requires
>>
>> internal knowledge about how to generate a draw transform in
>
> calcDrawProps,
>>
>> which maybe is not useful and breaks the encapsulation a bit, since
>
> this is not
>>
>> testing calcDraw? (not that it would be elaborate here, just thinking
>
> about the
>>
>> pattern)
>
>
> Yeah, I think it breaks the encapsulation a little bit.  I'm willing to
> punt, but it seems like it'd be cleaner if drawable properties were only
> set by calcDrawProperties.  I think if you're ever in a case where
> "calling calcDrawProperties will break this carefully constructed scene"
> then maybe you're doing something wrong and fragile.  I'm willing to
> punt on changing it in this patch, though.
>
> https://codereview.chromium.org/11583005/

[danakj, 2012-12-16 21:30:03]

https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc
File cc/occlusion_tracker.cc (right):

https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc#ne...
cc/occlusion_tracker.cc:420: // Check if the unoccluded rect shank when applying
outside occlusion.
On 2012/12/16 00:27:54, jamesr wrote:
> typo shank -> shrank

Done.

https://codereview.chromium.org/11583005/diff/3002/cc/occlusion_tracker.cc#ne...
cc/occlusion_tracker.cc:479: // Check if the unoccluded rect shank when applying
outside occlusion.
On 2012/12/16 00:27:54, jamesr wrote:
> same typo

Done.

[commit-bot: I haz the power, 2012-12-16 21:30:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11583005/12003

[commit-bot: I haz the power, 2012-12-16 23:41:54]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-17 00:09:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11583005/12003

[commit-bot: I haz the power, 2012-12-17 03:51:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/11583005/12003

[commit-bot: I haz the power, 2012-12-17 03:53:23]

Change committed as 173411