Refactor CCLayerTreeHostCommon - merge visible rect computation into calc draw transforms.

Refactor CCLayerTreeHostCommon -  merge visible rect computation into calc draw transforms.

To aid the design of intelligent prepainting, and to clean up the code anyway, it's appropriate to do this.
Unit tests are already in place that cover this refactoring.

BUG=154442
R=enne@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160676

[shawnsingh, 2012-10-06 02:23:27]

This is a reboot of the patch from a few weeks ago.  The old thread of
discussion was at https://bugs.webkit.org/show_bug.cgi?id=94541.   At that time,
we left off discussing that 3 early exit optimizations seemed
unnecessary/questionable.

It turned out condition 3 (exiting when surface doesn't clip) was not correct
anyway, since that would return the unclipped layer bounds rather than the
layer's drawableContentRect.  Specifically, the drawableContentRect could be
clipped by an intermediate layer, and not necessarily clipped by the surface
itself.

Also I removed the other two conditions that were related to animations as Dana
had suggested.

Finally, in this patch I also re-named calculateVisibleRect -->
calculateOverlapInContentSpace.   I wanted to make it a non-member static but
then realized we have unit tests for it.  I toyed with the idea of moving it to
CCMathUtil, actually, since it is just a mathematical helper for intersecting
two rects from different spaces.  Maybe if you like that idea we can do that in
a separate patch.

[enne (OOO), 2012-10-08 00:15:57]

I like this cleanup in general, other than the rename.

[shawnsingh, 2012-10-08 16:53:33]

On 2012/10/08 00:15:57, enne wrote:
> I like this cleanup in general, other than the rename.

This latest patch should be the same except without the renaming.

[enne (OOO), 2012-10-08 17:06:02]

lgtm

[commit-bot: I haz the power, 2012-10-08 17:07:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/11079012/11001

[commit-bot: I haz the power, 2012-10-08 19:11:59]

Change committed as 160676

[epenner, 2012-10-10 18:35:59]

Sorry for the lateness.

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon.cpp
File cc/CCLayerTreeHostCommon.cpp (left):

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon....
cc/CCLayerTreeHostCommon.cpp:677: if (ancestorClipsSubtree)
It looks like this code intends to only set an empty clipRect if
ancestorClipsSubtree is false. But couldn't it also set an empty clip rect if
ancestorClipsSubtree is true but the clipRect is now empty from two disjoint
clips?

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon.cpp
File cc/CCLayerTreeHostCommon.cpp (right):

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon....
cc/CCLayerTreeHostCommon.cpp:125: targetSurfaceClipRect =
layer->drawableContentRect();
I don't understand how this doesn't allow for possible out-of-memory cases.
Can't the drawableContentRect be an arbitrary size in this case?

[shawnsingh, 2012-10-10 19:54:56]

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon.cpp
File cc/CCLayerTreeHostCommon.cpp (left):

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon....
cc/CCLayerTreeHostCommon.cpp:677: if (ancestorClipsSubtree)
On 2012/10/10 18:35:59, epenner wrote:
> It looks like this code intends to only set an empty clipRect if
> ancestorClipsSubtree is false. But couldn't it also set an empty clip rect if
> ancestorClipsSubtree is true but the clipRect is now empty from two disjoint
> clips?

Yes... this is how we're encoding two different possibilities:
  (1) ancestorClipsSubtree == false implies that the clipRect is infinite and
its value should not be used
  (2) ancestorClipsSubtree == true implies that the clipRect value is finite,
meaningful, and should be used... even if it is empty.

I believe the same still holds even though this chunk of code was removed. 
technically it's not removed, but just moved up to inside the
subtreeShouldRenderToSeparateSurface() case.

Is there something specific that seems incorrect that you're concerned about?

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon.cpp
File cc/CCLayerTreeHostCommon.cpp (right):

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon....
cc/CCLayerTreeHostCommon.cpp:125: targetSurfaceClipRect =
layer->drawableContentRect();
On 2012/10/10 18:35:59, epenner wrote:
> I don't understand how this doesn't allow for possible out-of-memory cases.
> Can't the drawableContentRect be an arbitrary size in this case?

Aside from the max texture size safety, you're right this doesn't address that
issue.  I think the intent of this patch was to be a much needed cleanup anyway,
even though we didn't address this yet.  Let's definitely discuss with everyone
how we would want to address this. =)

[epenner, 2012-10-10 21:13:58]

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon.cpp
File cc/CCLayerTreeHostCommon.cpp (left):

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon....
cc/CCLayerTreeHostCommon.cpp:677: if (ancestorClipsSubtree)
It looks like line 124 checks clipRect().isEmpty() but there are two conditions
when it could be empty.

It seems to me like it should only ignore clipping if !ancestorClipsSubtree, but
return an empty visible rect if ancestorClipsSubtree.

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon.cpp
File cc/CCLayerTreeHostCommon.cpp (right):

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon....
cc/CCLayerTreeHostCommon.cpp:125: targetSurfaceClipRect =
layer->drawableContentRect();
On 2012/10/10 19:54:56, shawnsingh wrote:
> On 2012/10/10 18:35:59, epenner wrote:
> > I don't understand how this doesn't allow for possible out-of-memory cases.
> > Can't the drawableContentRect be an arbitrary size in this case?
> 
> Aside from the max texture size safety, you're right this doesn't address that
> issue.  I think the intent of this patch was to be a much needed cleanup
anyway,
> even though we didn't address this yet.  Let's definitely discuss with
everyone
> how we would want to address this. =)

Done.

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon....
cc/CCLayerTreeHostCommon.cpp:125: targetSurfaceClipRect =
layer->drawableContentRect();
IIUC The max texture size will limit the size of a render surface, but it will
not limit the size of content that renders into that surface. So I'm worried
that the visible content could be arbitrarily large.

[shawnsingh, 2012-10-10 22:35:42]

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon.cpp
File cc/CCLayerTreeHostCommon.cpp (left):

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon....
cc/CCLayerTreeHostCommon.cpp:677: if (ancestorClipsSubtree)
On 2012/10/10 21:13:59, epenner wrote:
> It looks like line 124 checks clipRect().isEmpty() but there are two
conditions
> when it could be empty.
> 
> It seems to me like it should only ignore clipping if !ancestorClipsSubtree,
but
> return an empty visible rect if ancestorClipsSubtree.

I see what you are saying now - thanks for catching this!  By fluke, this would
still be technically "correct" because an empty rendersurface would have been
seen and accounted for later in calcDrawTransforms, but clearly this is a kink
that needs to be ironed out somehow.  Created crbug.com/155151 to continue with
this issue.

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon.cpp
File cc/CCLayerTreeHostCommon.cpp (right):

https://codereview.chromium.org/11079012/diff/11001/cc/CCLayerTreeHostCommon....
cc/CCLayerTreeHostCommon.cpp:125: targetSurfaceClipRect =
layer->drawableContentRect();
On 2012/10/10 21:13:59, epenner wrote:
> IIUC The max texture size will limit the size of a render surface, but it will
> not limit the size of content that renders into that surface. So I'm worried
> that the visible content could be arbitrarily large.

Looks like the root of the problem is that we are not propagating clipRects
across surfaces.  However, originally Enne had a very strong reason not to do so
- because it will regularly change renderSurface sizes, causing things to be
less cacheable.  I'm guessing cache-ability (i.e. avoiding work entirely) takes
priority over intelligently being able to pre-paint (i.e. doing more work in
idle time)... so maybe we need to find another way to get the visibility
information here?

One option I can think of at this spontaneous moment - make another visibleRect
that does include propagating the clipRect across surfaces.  But I think you've
already considered that and we didn't go for that, right?