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

cc/CCLayerTreeHostCommon.cpp

Index: cc/CCLayerTreeHostCommon.cpp
diff --git a/cc/CCLayerTreeHostCommon.cpp b/cc/CCLayerTreeHostCommon.cpp
index 141b3ffd172a8912bafe758e604579bd71c1ca02..6f4b99df112787a98a0044ffb12097c09271b84a 100644
--- a/cc/CCLayerTreeHostCommon.cpp
+++ b/cc/CCLayerTreeHostCommon.cpp
@@ -114,16 +114,27 @@ static IntRect calculateVisibleContentRect(LayerType* layer)
 {
     ASSERT(layer->renderTarget());
 
-    IntRect targetSurfaceRect = layer->renderTarget()->renderSurface()->contentRect();
+    // Nothing is visible if the layer bounds are empty.
+    if (!layer->drawsContent() || layer->contentBounds().isEmpty() || layer->drawableContentRect().isEmpty())
+        return IntRect();
 
-    targetSurfaceRect.intersect(layer->drawableContentRect());
+    IntRect targetSurfaceClipRect;
+
+    // First, compute visible bounds in target surface space.
+    if (layer->renderTarget()->renderSurface()->clipRect().isEmpty())
+        targetSurfaceClipRect = layer->drawableContentRect();

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

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]

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:59]

Done.

[epenner, 2012/10/10 21:13:59]

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]

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?

+    else {
+        // In this case the target surface does clip layers that contribute to it. So, we
+        // have convert the current surface's clipRect from its ancestor surface space to
+        // the current surface space.
+        targetSurfaceClipRect = enclosingIntRect(CCMathUtil::projectClippedRect(layer->renderTarget()->renderSurface()->drawTransform().inverse(), layer->renderTarget()->renderSurface()->clipRect()));
+        targetSurfaceClipRect.intersect(layer->drawableContentRect());
+    }
 
-    if (targetSurfaceRect.isEmpty() || layer->contentBounds().isEmpty())
+    if (targetSurfaceClipRect.isEmpty())
         return IntRect();
 
-    const IntRect contentRect = IntRect(IntPoint(), layer->contentBounds());
-    IntRect visibleContentRect = CCLayerTreeHostCommon::calculateVisibleRect(targetSurfaceRect, contentRect, layer->drawTransform());
-    return visibleContentRect;
+    return CCLayerTreeHostCommon::calculateVisibleRect(targetSurfaceClipRect, IntRect(IntPoint(), layer->contentBounds()), layer->drawTransform());
 }
 
 static bool isScaleOrTranslation(const WebTransformationMatrix& m)
@@ -568,15 +579,25 @@ static void calculateDrawTransformsInternal(LayerType* layer, LayerType* rootLay
         // current target surface space that could cause more w < 0 headaches.
         subtreeShouldBeClipped = false;
 
-        if (layer->maskLayer())
+        if (layer->maskLayer()) {
             layer->maskLayer()->setRenderTarget(layer);
+            layer->maskLayer()->setVisibleContentRect(IntRect(IntPoint(), layer->contentBounds()));
+        }
 
-        if (layer->replicaLayer() && layer->replicaLayer()->maskLayer())
+        if (layer->replicaLayer() && layer->replicaLayer()->maskLayer()) {
             layer->replicaLayer()->maskLayer()->setRenderTarget(layer);
+            layer->replicaLayer()->maskLayer()->setVisibleContentRect(IntRect(IntPoint(), layer->contentBounds()));
+        }
 
         if (layer->filters().hasFilterThatMovesPixels())
             nearestAncestorThatMovesPixels = renderSurface;
 
+        // The render surface clipRect is expressed in the space where this surface draws, i.e. the same space as clipRectFromAncestor.
+        if (ancestorClipsSubtree)
+            renderSurface->setClipRect(clipRectFromAncestor);
+        else
+            renderSurface->setClipRect(IntRect());
+
         renderSurface->setNearestAncestorThatMovesPixels(nearestAncestorThatMovesPixels);
 
         renderSurfaceLayerList.append(layer);
@@ -668,17 +689,15 @@ static void calculateDrawTransformsInternal(LayerType* layer, LayerType* rootLay
         drawableContentRectOfLayer.intersect(clipRectForSubtree);
     layer->setDrawableContentRect(drawableContentRectOfLayer);
 
+    // Compute the layer's visible content rect (the rect is in content space)
+    IntRect visibleContentRectOfLayer = calculateVisibleContentRect(layer);
+    layer->setVisibleContentRect(visibleContentRectOfLayer);
+
     // Compute the remaining properties for the render surface, if the layer has one.
     if (layer->renderSurface() && layer != rootLayer) {
         RenderSurfaceType* renderSurface = layer->renderSurface();
         IntRect clippedContentRect = localDrawableContentRectOfSubtree;
 
-        // The render surface clipRect is expressed in the space where this surface draws, i.e. the same space as clipRectFromAncestor.
-        if (ancestorClipsSubtree)

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

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?

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

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?

[epenner, 2012/10/10 21:13:59]

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.

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

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.

-            renderSurface->setClipRect(clipRectFromAncestor);
-        else
-            renderSurface->setClipRect(IntRect());
-
         // Don't clip if the layer is reflected as the reflection shouldn't be
         // clipped. If the layer is animating, then the surface's transform to
         // its target is not known on the main thread, and we should not use it
@@ -757,31 +776,6 @@ static void calculateDrawTransformsInternal(LayerType* layer, LayerType* rootLay
         layer->renderTarget()->renderSurface()->addContributingDelegatedRenderPassLayer(layer);
 }
 
-// FIXME: Instead of using the following function to set visibility rects on a second
-// tree pass, revise calculateVisibleContentRect() so that this can be done in a single
-// pass inside calculateDrawTransformsInternal<>().
-template<typename LayerType, typename LayerList, typename RenderSurfaceType>
-static void calculateVisibleRectsInternal(const LayerList& renderSurfaceLayerList)
-{
-    // Use BackToFront since it's cheap and this isn't order-dependent.
-    typedef CCLayerIterator<LayerType, LayerList, RenderSurfaceType, CCLayerIteratorActions::BackToFront> CCLayerIteratorType;
-
-    CCLayerIteratorType end = CCLayerIteratorType::end(&renderSurfaceLayerList);
-    for (CCLayerIteratorType it = CCLayerIteratorType::begin(&renderSurfaceLayerList); it != end; ++it) {
-        if (it.representsTargetRenderSurface()) {
-            LayerType* maskLayer = it->maskLayer();
-            if (maskLayer)
-                maskLayer->setVisibleContentRect(IntRect(IntPoint(), it->contentBounds()));
-            LayerType* replicaMaskLayer = it->replicaLayer() ? it->replicaLayer()->maskLayer() : 0;
-            if (replicaMaskLayer)
-                replicaMaskLayer->setVisibleContentRect(IntRect(IntPoint(), it->contentBounds()));
-        } else if (it.representsItself()) {
-            IntRect visibleContentRect = calculateVisibleContentRect(*it);
-            it->setVisibleContentRect(visibleContentRect);
-        }
-    }
-}
-
 void CCLayerTreeHostCommon::calculateDrawTransforms(LayerChromium* rootLayer, const IntSize& deviceViewportSize, float deviceScaleFactor, int maxTextureSize, Vector<RefPtr<LayerChromium> >& renderSurfaceLayerList)
 {
     IntRect totalDrawableContentRect;
@@ -810,16 +804,6 @@ void CCLayerTreeHostCommon::calculateDrawTransforms(CCLayerImpl* rootLayer, cons
                                                                                                                 rootLayer->renderSurface()->layerList(), layerSorter, maxTextureSize, deviceScaleFactor, totalDrawableContentRect);
 }
 
-void CCLayerTreeHostCommon::calculateVisibleRects(Vector<RefPtr<LayerChromium> >& renderSurfaceLayerList)
-{
-    calculateVisibleRectsInternal<LayerChromium, Vector<RefPtr<LayerChromium> >, RenderSurfaceChromium>(renderSurfaceLayerList);
-}
-
-void CCLayerTreeHostCommon::calculateVisibleRects(Vector<CCLayerImpl*>& renderSurfaceLayerList)
-{
-    calculateVisibleRectsInternal<CCLayerImpl, Vector<CCLayerImpl*>, CCRenderSurface>(renderSurfaceLayerList);
-}
-
 static bool pointHitsRect(const IntPoint& viewportPoint, const WebTransformationMatrix& localSpaceToScreenSpaceTransform, FloatRect localSpaceRect)
 {
     // If the transform is not invertible, then assume that this point doesn't hit this rect.