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..f1a0cbf0a158c2dea7e67175230e109bd78a1649 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,19 +58,6 @@ 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,
@@ -436,15 +422,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 +433,6 @@ 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);

[danakj, 2013/09/10 14:36:37]

Not intersecting with the clip rect means you'll mark things as occluding even
tho they are being clipped. I don't think you can change this function and not
get wrong results. Unless you're planning to do the clipping in
VisibleContentOpaqueRegion()? Or somewhere else?

[alokp, 2013/09/10 18:15:46]

VisibleContentOpaqueRegion is already clipped. It is computed using
layer::draw_properties::visible_content_rect, which is clipped in
CalculateDrawProps. No?

We are computing enclosed-rect in multiple places, which made me suspicious, but
since we only consider axis-aligned occluding layers, I think this is correct.

Having said that, leaving the clipping here is fine. This is not what slows us
down. Clipping query-rects do because we cannot early out.

[danakj, 2013/09/10 18:21:10]

The visible content rect is in content space. So it is clipped to a bounding box
around the actual clip rect, which lives in target space. You can't clip things
in content space and expect them to be tight.

[alokp, 2013/09/10 20:51:27]

I do not understand. I am looking at this:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

The clipping happens in the target-space and projected back. If draw-transform
preserves axis alignment, why would it not be tight?

[danakj, 2013/09/10 20:54:24]

Yes it would, you're right! But if it is not axis-aligned.. then you need to do
clipping in the target space. The inputs to this function are in content space,
so you can't just use those, or you'll mark occlusion in the target where there
is actually none.

     if (transformed_rect.width() < minimum_tracking_size_.width() &&
         transformed_rect.height() < minimum_tracking_size_.height())
       continue;
@@ -489,7 +465,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);

[danakj, 2013/09/10 14:36:37]

same point here

     if (transformed_rect.IsEmpty())
       continue;
 
@@ -512,8 +487,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 +509,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 +522,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);
   unoccluded_region_in_target_surface.Subtract(
       stack_.back().occlusion_from_inside_target);
   gfx::RectF unoccluded_rect_in_target_surface_without_outside_occlusion =
@@ -554,14 +529,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.
-  // TODO(danakj): Clip to visibleContentRect?
-  unoccluded_region_in_target_surface.Intersect(
-      render_target->render_surface()->content_rect());

[enne (OOO), 2013/09/09 23:37:39]

I think you shouldn't remove this line.

[alokp, 2013/09/10 13:31:44]

I think we should. This is also a form of clipping that has already happened
during CalculateDrawProps. This function should return the same result in both
scenarios:

1. Occluded region is empty. We early-out in this case and do not bother
clipping with anything - clip-rect, target-surface-rect, or screen-rect.

2. Occluded region is non-empty, but does not occlude the given rect. Here if we
clip against the target-surface-rect, you might get a different result.

This was the motivation for not doing any kind of clipping here.

-  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 +549,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 +571,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 +584,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 +591,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());

[enne (OOO), 2013/09/09 23:37:39]

Same here.

-  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(
@@ -717,10 +676,6 @@ gfx::Rect OcclusionTrackerBase<LayerType, RenderSurfaceType>::
   // Treat other clipping as occlusion from outside the target surface.
   unoccluded_region_in_target_surface.Intersect(
       contributing_surface_render_target->render_surface()->content_rect());

[danakj, 2013/09/10 14:36:37]

Why not this?

[alokp, 2013/09/10 18:15:46]

I am planning to work on surface-rects in a separate patch.

-  unoccluded_region_in_target_surface.Intersect(
-      ScreenSpaceClipRectInTargetSurface(
-          contributing_surface_render_target->render_surface(),
-          screen_space_clip_rect_));
 
   gfx::RectF unoccluded_rect_in_target_surface =
       unoccluded_region_in_target_surface.bounds();