Do not clip inside OcclusionTracker.

cc/trees/occlusion_tracker.cc

Index: cc/trees/occlusion_tracker.cc
diff --git a/cc/trees/occlusion_tracker.cc b/cc/trees/occlusion_tracker.cc
index 9fc26da566700ce9b46cb88974730ba722a6b155..0f5cae930c9b9347db8bd3e4eb63bc58233003ed 100644
--- a/cc/trees/occlusion_tracker.cc
+++ b/cc/trees/occlusion_tracker.cc
@@ -19,9 +19,8 @@ namespace cc {
 
 template <typename LayerType, typename RenderSurfaceType>
 OcclusionTrackerBase<LayerType, RenderSurfaceType>::OcclusionTrackerBase(
-    gfx::Rect screen_space_clip_rect, bool record_metrics_for_frame)
-    : screen_space_clip_rect_(screen_space_clip_rect),
-      overdraw_metrics_(OverdrawMetrics::Create(record_metrics_for_frame)),
+    bool record_metrics_for_frame)
+    : overdraw_metrics_(OverdrawMetrics::Create(record_metrics_for_frame)),
       prevent_occlusion_(false),
       occluding_screen_space_rects_(NULL),
       non_occluding_screen_space_rects_(NULL) {}
@@ -59,22 +58,7 @@ void OcclusionTrackerBase<LayerType, RenderSurfaceType>::LeaveLayer(
 }
 
 template <typename RenderSurfaceType>
-static gfx::Rect ScreenSpaceClipRectInTargetSurface(
-    const RenderSurfaceType* target_surface, gfx::Rect screen_space_clip_rect) {
-  gfx::Transform inverse_screen_space_transform(
-      gfx::Transform::kSkipInitialization);
-  if (!target_surface->screen_space_transform().GetInverse(
-          &inverse_screen_space_transform))
-    return target_surface->content_rect();
-
-  return gfx::ToEnclosingRect(MathUtil::ProjectClippedRect(
-      inverse_screen_space_transform, screen_space_clip_rect));
-}
-
-template <typename RenderSurfaceType>
 static Region TransformSurfaceOpaqueRegion(const Region& region,
-                                           bool have_clip_rect,
-                                           gfx::Rect clip_rect_in_new_target,
                                            const gfx::Transform& transform) {
   if (region.IsEmpty())
     return Region();
@@ -97,8 +81,6 @@ static Region TransformSurfaceOpaqueRegion(const Region& region,
     gfx::Rect transformed_rect =
         gfx::ToEnclosedRect(transformed_quad.BoundingBox());
     DCHECK(!clipped);  // We only map if the transform preserves axis alignment.
-    if (have_clip_rect)

[danakj, 2013/09/16 17:29:08]

I don't really understand this change. It's just an intersect, it's not costly.
Have you tried to construct a counter-example that would hit your DCHECK for
this?

-      transformed_rect.Intersect(clip_rect_in_new_target);
     transformed_region.Union(transformed_rect);
   }
   return transformed_region;
@@ -207,14 +189,10 @@ void OcclusionTrackerBase<LayerType, RenderSurfaceType>::EnterRenderTarget(
   stack_[last_index].occlusion_from_outside_target =
       TransformSurfaceOpaqueRegion<RenderSurfaceType>(
           stack_[last_index - 1].occlusion_from_outside_target,
-          false,
-          gfx::Rect(),
           old_target_to_new_target_transform);
   stack_[last_index].occlusion_from_outside_target.Union(
       TransformSurfaceOpaqueRegion<RenderSurfaceType>(
           stack_[last_index - 1].occlusion_from_inside_target,
-          false,
-          gfx::Rect(),
           old_target_to_new_target_transform));
 }
 
@@ -260,12 +238,6 @@ static void ReduceOcclusionBelowSurface(LayerType* contributing_layer,
 
   gfx::Rect affected_area_in_target = gfx::ToEnclosingRect(
       MathUtil::MapClippedRect(surface_transform, gfx::RectF(surface_rect)));
-  if (contributing_layer->render_surface()->is_clipped()) {
-    affected_area_in_target.Intersect(

[danakj, 2013/09/16 17:29:08]

Again, just an intersection. I think there's two ways clipping can come up in
here.

One is clipping the unoccluded region, and that makes sense to move out. The
other is clipping occlusion, and that is really important to not over-estimate
what's occluded. I don't think that can go away. This is the latter here. I
would need to be convinced that this will never lead to us considering something
occluded when it is not.

[alokp, 2013/09/17 00:31:41]

I have decided to break this into two patches - one for query function and
another for things that compute occlusion. Lets focus on the query function for
now. This is not slowing us down anyway.

-        contributing_layer->render_surface()->clip_rect());
-  }
-  if (affected_area_in_target.IsEmpty())
-    return;
 
   int outset_top, outset_right, outset_bottom, outset_left;
   contributing_layer->background_filters().GetOutsets(
@@ -327,23 +299,22 @@ void OcclusionTrackerBase<LayerType, RenderSurfaceType>::LeaveToRenderTarget(
   Region old_occlusion_from_inside_target_in_new_target =
       TransformSurfaceOpaqueRegion<RenderSurfaceType>(
           stack_[last_index].occlusion_from_inside_target,
-          old_surface->is_clipped(),
-          old_surface->clip_rect(),
           old_surface->draw_transform());
   if (old_target->has_replica() && !old_target->replica_has_mask()) {
     old_occlusion_from_inside_target_in_new_target.Union(
         TransformSurfaceOpaqueRegion<RenderSurfaceType>(
             stack_[last_index].occlusion_from_inside_target,
-            old_surface->is_clipped(),
-            old_surface->clip_rect(),
             old_surface->replica_draw_transform()));
   }
+  // Verify that the occluded region is contained inside the
+  // target surface clip-rect.
+  DCHECK(!old_surface->is_clipped() ||
+      old_surface->clip_rect().Contains(
+          old_occlusion_from_inside_target_in_new_target.bounds()));
 
   Region old_occlusion_from_outside_target_in_new_target =
       TransformSurfaceOpaqueRegion<RenderSurfaceType>(
           stack_[last_index].occlusion_from_outside_target,
-          false,
-          gfx::Rect(),
           old_surface->draw_transform());
 
   gfx::Rect unoccluded_surface_rect;
@@ -436,15 +407,6 @@ void OcclusionTrackerBase<LayerType, RenderSurfaceType>::
   if (!layer->draw_transform().Preserves2dAxisAlignment())
     return;
 
-  gfx::Rect clip_rect_in_target = ScreenSpaceClipRectInTargetSurface(
-      layer->render_target()->render_surface(), screen_space_clip_rect_);
-  if (layer->is_clipped()) {
-    clip_rect_in_target.Intersect(layer->clip_rect());
-  } else {
-    clip_rect_in_target.Intersect(
-        layer->render_target()->render_surface()->content_rect());
-  }
-
   for (Region::Iterator opaque_content_rects(opaque_contents);
        opaque_content_rects.has_rect();
        opaque_content_rects.next()) {
@@ -456,7 +418,16 @@ void OcclusionTrackerBase<LayerType, RenderSurfaceType>::
     gfx::Rect transformed_rect =
         gfx::ToEnclosedRect(transformed_quad.BoundingBox());
     DCHECK(!clipped);  // We only map if the transform preserves axis alignment.
-    transformed_rect.Intersect(clip_rect_in_target);
+#ifndef NDEBUG

[danakj, 2013/09/16 17:29:08]

if (DCHECK_IS_ON()) {

+  // Verify that the occluded region is contained inside the target surface
+  // clip-rect or bounds. We do not do any clipping here in occlusion tracker.
+  // We assume that visible-content-rect has already been clipped during
+  // CalculateDrawProperties.
+  gfx::Rect clip_rect_in_target = layer->is_clipped() ?
+      layer->clip_rect() :
+      layer->render_target()->render_surface()->content_rect();
+  DCHECK(clip_rect_in_target.Contains(transformed_rect));

[danakj, 2013/09/16 17:29:08]

Note that this DCHECK is verifying the clip rect in the target, but it is not
including the viewport in that. I think one could construct a series of render
surfaces such that the visible_content_rect of the layer would end up not
clipped to the viewport, but would be axis-aligned to its target.

Maybe this won't be a problem in the wild, but if we start seeing missing tiles
on non-impl painting around the edge of the viewport.. we'll know why.

+#endif  // NDEBUG
     if (transformed_rect.width() < minimum_tracking_size_.width() &&
         transformed_rect.height() < minimum_tracking_size_.height())
       continue;
@@ -476,6 +447,7 @@ void OcclusionTrackerBase<LayerType, RenderSurfaceType>::
     occluding_screen_space_rects_->push_back(screen_space_rect);
   }
 
+

[danakj, 2013/09/16 17:29:08]

extra whitespace

[alokp, 2013/09/17 00:31:41]

Done.

   if (!non_occluding_screen_space_rects_)
     return;
 
@@ -489,7 +461,6 @@ void OcclusionTrackerBase<LayerType, RenderSurfaceType>::
     gfx::Rect transformed_rect = gfx::ToEnclosedRect(
         MathUtil::MapClippedRect(layer->draw_transform(),
                                  gfx::RectF(non_opaque_content_rects.rect())));
-    transformed_rect.Intersect(clip_rect_in_target);
     if (transformed_rect.IsEmpty())
       continue;
 
@@ -512,8 +483,6 @@ bool OcclusionTrackerBase<LayerType, RenderSurfaceType>::Occluded(
     gfx::Rect content_rect,
     const gfx::Transform& draw_transform,
     bool impl_draw_transform_is_unknown,
-    bool is_clipped,
-    gfx::Rect clip_rect_in_target,
     bool* has_occlusion_from_outside_target_surface) const {
   if (has_occlusion_from_outside_target_surface)
     *has_occlusion_from_outside_target_surface = false;
@@ -536,6 +505,11 @@ bool OcclusionTrackerBase<LayerType, RenderSurfaceType>::Occluded(
   DCHECK(render_target->render_surface());
   DCHECK_EQ(render_target, stack_.back().target);
 
+  if (stack_.back().occlusion_from_inside_target.IsEmpty() &&
+      stack_.back().occlusion_from_outside_target.IsEmpty()) {
+    return false;
+  }
+
   gfx::Transform inverse_draw_transform(gfx::Transform::kSkipInitialization);
   if (!draw_transform.GetInverse(&inverse_draw_transform))
     return false;
@@ -544,9 +518,6 @@ bool OcclusionTrackerBase<LayerType, RenderSurfaceType>::Occluded(
   // partial pixels in the resulting Rect.
   Region unoccluded_region_in_target_surface = gfx::ToEnclosingRect(
       MathUtil::MapClippedRect(draw_transform, gfx::RectF(content_rect)));
-  // Layers can't clip across surfaces, so count this as internal occlusion.
-  if (is_clipped)
-    unoccluded_region_in_target_surface.Intersect(clip_rect_in_target);

[danakj, 2013/09/16 17:29:08]

I think I buy that this can go away (and not move elsewhere). I was worried
about partial culling or rotated layers, but we'll have a clip rect defined at
draw time for these quads so that should prevent wasted drawing.

   unoccluded_region_in_target_surface.Subtract(
       stack_.back().occlusion_from_inside_target);
   gfx::RectF unoccluded_rect_in_target_surface_without_outside_occlusion =
@@ -554,14 +525,6 @@ bool OcclusionTrackerBase<LayerType, RenderSurfaceType>::Occluded(
   unoccluded_region_in_target_surface.Subtract(
       stack_.back().occlusion_from_outside_target);
 
-  // Treat other clipping as occlusion from outside the surface.

[danakj, 2013/09/16 17:29:08]

I'm not sure why you say that the has_occlusion_from_outside_target_surface will
always produce the same results. In the early-out case sure.. but what about
these cases?

For instance, if the render surface is rotated so it's top-left corner is
outside the viewport, and half of this query is outside the viewport in the
top-left corner there.. what is going to set this bool to true so the surface is
known to be incomplete?

Same question for the 2 functions below.

[alokp, 2013/09/17 00:31:41]

You are right. I do not see how I can preserve this bit without clipping. Per
our offline discussion, I will see if I can remove this feature as it is not
being used right now.

-  // TODO(danakj): Clip to visibleContentRect?
-  unoccluded_region_in_target_surface.Intersect(
-      render_target->render_surface()->content_rect());
-  unoccluded_region_in_target_surface.Intersect(
-      ScreenSpaceClipRectInTargetSurface(render_target->render_surface(),
-                                         screen_space_clip_rect_));
-
   gfx::RectF unoccluded_rect_in_target_surface =
       unoccluded_region_in_target_surface.bounds();
 
@@ -582,8 +545,6 @@ gfx::Rect OcclusionTrackerBase<LayerType, RenderSurfaceType>::
         gfx::Rect content_rect,
         const gfx::Transform& draw_transform,
         bool impl_draw_transform_is_unknown,
-        bool is_clipped,
-        gfx::Rect clip_rect_in_target,
         bool* has_occlusion_from_outside_target_surface) const {
   if (has_occlusion_from_outside_target_surface)
     *has_occlusion_from_outside_target_surface = false;
@@ -606,6 +567,11 @@ gfx::Rect OcclusionTrackerBase<LayerType, RenderSurfaceType>::
   DCHECK(render_target->render_surface());
   DCHECK_EQ(render_target, stack_.back().target);
 
+  if (stack_.back().occlusion_from_inside_target.IsEmpty() &&
+      stack_.back().occlusion_from_outside_target.IsEmpty()) {
+    return content_rect;
+  }
+
   gfx::Transform inverse_draw_transform(gfx::Transform::kSkipInitialization);
   if (!draw_transform.GetInverse(&inverse_draw_transform))
     return content_rect;
@@ -614,9 +580,6 @@ gfx::Rect OcclusionTrackerBase<LayerType, RenderSurfaceType>::
   // partial pixels in the resulting Rect.
   Region unoccluded_region_in_target_surface = gfx::ToEnclosingRect(
       MathUtil::MapClippedRect(draw_transform, gfx::RectF(content_rect)));
-  // Layers can't clip across surfaces, so count this as internal occlusion.
-  if (is_clipped)
-    unoccluded_region_in_target_surface.Intersect(clip_rect_in_target);
   unoccluded_region_in_target_surface.Subtract(
       stack_.back().occlusion_from_inside_target);
   gfx::RectF unoccluded_rect_in_target_surface_without_outside_occlusion =
@@ -624,14 +587,6 @@ gfx::Rect OcclusionTrackerBase<LayerType, RenderSurfaceType>::
   unoccluded_region_in_target_surface.Subtract(
       stack_.back().occlusion_from_outside_target);
 
-  // Treat other clipping as occlusion from outside the surface.
-  // TODO(danakj): Clip to visibleContentRect?
-  unoccluded_region_in_target_surface.Intersect(
-      render_target->render_surface()->content_rect());
-  unoccluded_region_in_target_surface.Intersect(
-      ScreenSpaceClipRectInTargetSurface(render_target->render_surface(),
-                                         screen_space_clip_rect_));
-
   gfx::RectF unoccluded_rect_in_target_surface =
       unoccluded_region_in_target_surface.bounds();
   gfx::Rect unoccluded_rect = gfx::ToEnclosingRect(
@@ -675,10 +630,18 @@ gfx::Rect OcclusionTrackerBase<LayerType, RenderSurfaceType>::
   if (content_rect.IsEmpty())
     return content_rect;
 
-  const RenderSurfaceType* surface = layer->render_surface();
-  const LayerType* contributing_surface_render_target =
-      layer->parent()->render_target();
+  // A contributing surface doesn't get occluded by things inside its own
+  // surface, so only things outside the surface can occlude it. That occlusion
+  // is found just below the top of the stack (if it exists).
+  if (stack_.size() == 1)
+    return content_rect;
+  const StackObject& second_last = stack_[stack_.size() - 2];
+  if (second_last.occlusion_from_inside_target.IsEmpty() &&
+      second_last.occlusion_from_outside_target.IsEmpty()) {
+    return content_rect;
+  }
 
+  const RenderSurfaceType* surface = layer->render_surface();
   if (!SurfaceTransformsToTargetKnown(surface))
     return content_rect;
 
@@ -689,38 +652,16 @@ gfx::Rect OcclusionTrackerBase<LayerType, RenderSurfaceType>::
   if (!draw_transform.GetInverse(&inverse_draw_transform))
     return content_rect;
 
-  // A contributing surface doesn't get occluded by things inside its own
-  // surface, so only things outside the surface can occlude it. That occlusion
-  // is found just below the top of the stack (if it exists).
-  bool has_occlusion = stack_.size() > 1;
-
   // Take the ToEnclosingRect at each step, as we want to contain any unoccluded
   // partial pixels in the resulting Rect.
   Region unoccluded_region_in_target_surface = gfx::ToEnclosingRect(
       MathUtil::MapClippedRect(draw_transform, gfx::RectF(content_rect)));
-  // Layers can't clip across surfaces, so count this as internal occlusion.
-  if (surface->is_clipped())
-    unoccluded_region_in_target_surface.Intersect(surface->clip_rect());
-  if (has_occlusion) {
-    const StackObject& second_last = stack_[stack_.size() - 2];
-    unoccluded_region_in_target_surface.Subtract(
-        second_last.occlusion_from_inside_target);
-  }
+  unoccluded_region_in_target_surface.Subtract(
+      second_last.occlusion_from_inside_target);
   gfx::RectF unoccluded_rect_in_target_surface_without_outside_occlusion =
       unoccluded_region_in_target_surface.bounds();
-  if (has_occlusion) {
-    const StackObject& second_last = stack_[stack_.size() - 2];
-    unoccluded_region_in_target_surface.Subtract(
-        second_last.occlusion_from_outside_target);
-  }
-
-  // Treat other clipping as occlusion from outside the target surface.
-  unoccluded_region_in_target_surface.Intersect(
-      contributing_surface_render_target->render_surface()->content_rect());
-  unoccluded_region_in_target_surface.Intersect(
-      ScreenSpaceClipRectInTargetSurface(
-          contributing_surface_render_target->render_surface(),
-          screen_space_clip_rect_));
+  unoccluded_region_in_target_surface.Subtract(
+      second_last.occlusion_from_outside_target);
 
   gfx::RectF unoccluded_rect_in_target_surface =
       unoccluded_region_in_target_surface.bounds();