Don't aggregate quads outside of damage rect when using partial swap.

cc/surfaces/surface_aggregator.cc

Index: cc/surfaces/surface_aggregator.cc
diff --git a/cc/surfaces/surface_aggregator.cc b/cc/surfaces/surface_aggregator.cc
index ddfac7670b40d6f021eb0293438d0aedd329efad..8020978c325b6766448e7b559f9768f1cf376402 100644
--- a/cc/surfaces/surface_aggregator.cc
+++ b/cc/surfaces/surface_aggregator.cc
@@ -41,8 +41,12 @@ void MoveMatchingRequests(
 }  // namespace
 
 SurfaceAggregator::SurfaceAggregator(SurfaceManager* manager,
-                                     ResourceProvider* provider)
-    : manager_(manager), provider_(provider), next_render_pass_id_(1) {
+                                     ResourceProvider* provider,
+                                     bool aggregate_only_damaged)
+    : manager_(manager),
+      provider_(provider),
+      next_render_pass_id_(1),
+      aggregate_only_damaged_(aggregate_only_damaged) {
   DCHECK(manager_);
 }
 
@@ -154,11 +158,8 @@ void SurfaceAggregator::HandleSurfaceQuad(
   if (referenced_surfaces_.count(surface_id))
     return;
   Surface* surface = manager_->GetSurfaceForId(surface_id);
-  if (!surface) {
-    contained_surfaces_[surface_id] = 0;
+  if (!surface)
     return;
-  }
-  contained_surfaces_[surface_id] = surface->frame_index();
   const CompositorFrame* frame = surface->GetEligibleFrame();
   if (!frame)
     return;
@@ -251,11 +252,10 @@ void SurfaceAggregator::HandleSurfaceQuad(
   } else {
     RenderPassId remapped_pass_id = RemapPassId(last_pass.id, surface_id);
 
-    CopySharedQuadState(surface_quad->shared_quad_state, target_transform,
-                        clip_rect, dest_pass);
-
     SharedQuadState* shared_quad_state =
-        dest_pass->shared_quad_state_list.back();
+        CopySharedQuadState(surface_quad->shared_quad_state, target_transform,
+                            clip_rect, dest_pass);
+
     RenderPassDrawQuad* quad =
         dest_pass->CreateAndAppendDrawQuad<RenderPassDrawQuad>();
     quad->SetNew(shared_quad_state,
@@ -270,11 +270,10 @@ void SurfaceAggregator::HandleSurfaceQuad(
                  FilterOperations());
   }
 
-
   referenced_surfaces_.erase(it);
 }
 
-void SurfaceAggregator::CopySharedQuadState(
+SharedQuadState* SurfaceAggregator::CopySharedQuadState(
     const SharedQuadState* source_sqs,
     const gfx::Transform& target_transform,
     const ClipData& clip_rect,
@@ -296,6 +295,23 @@ void SurfaceAggregator::CopySharedQuadState(
       target_transform);
   copy_shared_quad_state->is_clipped = new_clip_rect.is_clipped;
   copy_shared_quad_state->clip_rect = new_clip_rect.rect;
+  return copy_shared_quad_state;
+}
+
+static bool CalculateQuadSpaceDamageRect(
+    const gfx::Transform& quad_to_target_transform,
+    const gfx::Transform& target_to_root_transform,
+    const gfx::Rect& root_damage_rect,
+    gfx::Rect* quad_damage_rect) {
+  gfx::Transform quad_to_root_transform(quad_to_target_transform,
+                                        target_to_root_transform);
+  gfx::Transform inverse_transform(gfx::Transform::kSkipInitialization);
+  if (!quad_to_root_transform.GetInverse(&inverse_transform))

[danakj, 2015/07/07 20:53:53]

i don't think compositor is supposed to produce non-invertible transforms in the
frame. so you could just DCHECK the result was true here

+    return false;
+
+  *quad_damage_rect = MathUtil::ProjectEnclosingClippedRect(inverse_transform,
+                                                            root_damage_rect);
+  return true;
 }
 
 void SurfaceAggregator::CopyQuadsToPass(
@@ -307,6 +323,13 @@ void SurfaceAggregator::CopyQuadsToPass(
     RenderPass* dest_pass,
     SurfaceId surface_id) {
   const SharedQuadState* last_copied_source_shared_quad_state = NULL;
+  const SharedQuadState* dest_shared_quad_state = nullptr;
+  // Damage rect in the quad space of the current shared quad state, if
+  // the transform from that is invertible.
+  // TODO(jbauman): This rect may contain unnecessary area if
+  // transform isn't axis-aligned.
+  gfx::Rect damage_rect_in_quad_space;
+  bool valid_damage_rect = false;
 
   SharedQuadStateList::ConstIterator sqs_iter =
       source_shared_quad_state_list.begin();
@@ -319,13 +342,39 @@ void SurfaceAggregator::CopyQuadsToPass(
 
     if (quad->material == DrawQuad::SURFACE_CONTENT) {
       const SurfaceDrawQuad* surface_quad = SurfaceDrawQuad::MaterialCast(quad);
+
+      if (aggregate_only_damaged_ && !has_copy_requests_) {

[danakj, 2015/07/07 20:53:53]

can you comment why you're checking has copy requests?

+        gfx::Transform quad_to_target_transform(
+            quad->shared_quad_state->quad_to_target_transform,
+            target_transform);
+        valid_damage_rect = CalculateQuadSpaceDamageRect(
+            quad_to_target_transform, dest_pass->transform_to_root_target,
+            root_damage_rect_, &damage_rect_in_quad_space);

[danakj, 2015/07/07 20:53:53]

you're overwriting the damage_rect_in_quad_space, but the
last_copied_source_shared_quad_state isn't changed, this looks like it would
cause us to use the wrong damage_rect_in_quad_space after a surface quad?

+        if (valid_damage_rect &&
+            !damage_rect_in_quad_space.Intersects(quad->visible_rect))
+          continue;
+      }
+
       HandleSurfaceQuad(surface_quad, target_transform, clip_rect, dest_pass);
     } else {
       if (quad->shared_quad_state != last_copied_source_shared_quad_state) {
-        CopySharedQuadState(quad->shared_quad_state, target_transform,
-                            clip_rect, dest_pass);
+        dest_shared_quad_state = CopySharedQuadState(
+            quad->shared_quad_state, target_transform, clip_rect, dest_pass);
         last_copied_source_shared_quad_state = quad->shared_quad_state;
+        if (aggregate_only_damaged_ && !has_copy_requests_) {
+          valid_damage_rect = CalculateQuadSpaceDamageRect(
+              dest_shared_quad_state->quad_to_target_transform,
+              dest_pass->transform_to_root_target, root_damage_rect_,
+              &damage_rect_in_quad_space);
+        }
       }
+
+      if (aggregate_only_damaged_ && !has_copy_requests_) {
+        if (valid_damage_rect &&
+            !damage_rect_in_quad_space.Intersects(quad->visible_rect))
+          continue;
+      }
+
       DrawQuad* dest_quad;
       if (quad->material == DrawQuad::RENDER_PASS) {
         const RenderPassDrawQuad* pass_quad =
@@ -335,11 +384,10 @@ void SurfaceAggregator::CopyQuadsToPass(
             RemapPassId(original_pass_id, surface_id);
 
         dest_quad = dest_pass->CopyFromAndAppendRenderPassDrawQuad(
-            pass_quad, dest_pass->shared_quad_state_list.back(),
-            remapped_pass_id);
+            pass_quad, dest_shared_quad_state, remapped_pass_id);
       } else {
-        dest_quad = dest_pass->CopyFromAndAppendDrawQuad(
-            quad, dest_pass->shared_quad_state_list.back());
+        dest_quad =
+            dest_pass->CopyFromAndAppendDrawQuad(quad, dest_shared_quad_state);
       }
       if (!child_to_parent_map.empty()) {
         for (ResourceId& resource_id : dest_quad->resources) {
@@ -422,8 +470,11 @@ gfx::Rect SurfaceAggregator::ValidateAndCalculateDamageRect(
   if (referenced_surfaces_.count(surface_id))
     return gfx::Rect();
   Surface* surface = manager_->GetSurfaceForId(surface_id);
-  if (!surface)
+  if (!surface) {
+    contained_surfaces_[surface_id] = 0;
     return gfx::Rect();
+  }
+  contained_surfaces_[surface_id] = surface->frame_index();
   const CompositorFrame* surface_frame = surface->GetEligibleFrame();
   if (!surface_frame)
     return gfx::Rect();
@@ -492,11 +543,14 @@ gfx::Rect SurfaceAggregator::ValidateAndCalculateDamageRect(
   if (provider_)
     provider_->DeclareUsedResourcesFromChild(child_id, referenced_resources);
 
+  for (const auto& render_pass : frame_data->render_pass_list)
+    has_copy_requests_ |= !render_pass->copy_requests.empty();
+
   gfx::Rect damage_rect;
   if (!frame_data->render_pass_list.empty()) {
+    RenderPass* last_pass = frame_data->render_pass_list.back();
     damage_rect =
-        DamageRectForSurface(surface, *frame_data->render_pass_list.back(),
-                             frame_data->render_pass_list.back()->output_rect);
+        DamageRectForSurface(surface, *last_pass, last_pass->output_rect);
   }
 
   // Avoid infinite recursion by adding current surface to
@@ -531,7 +585,8 @@ scoped_ptr<CompositorFrame> SurfaceAggregator::Aggregate(SurfaceId surface_id) {
   dest_pass_list_ = &frame->delegated_frame_data->render_pass_list;
 
   valid_surfaces_.clear();
-  gfx::Rect damage_rect = ValidateAndCalculateDamageRect(surface_id);
+  has_copy_requests_ = false;

[danakj, 2015/07/07 20:53:53]

Do these need to be member variables? Can they be kept on the stack instead?
Maybe a struct passed through the recursion?

+  root_damage_rect_ = ValidateAndCalculateDamageRect(surface_id);
 
   SurfaceSet::iterator it = referenced_surfaces_.insert(surface_id).first;
   CopyPasses(root_surface_frame->delegated_frame_data.get(), surface);
@@ -541,7 +596,7 @@ scoped_ptr<CompositorFrame> SurfaceAggregator::Aggregate(SurfaceId surface_id) {
 
   if (dest_pass_list_->empty())
     return nullptr;
-  dest_pass_list_->back()->damage_rect = damage_rect;
+  dest_pass_list_->back()->damage_rect = root_damage_rect_;
 
   dest_pass_list_ = NULL;
   RemoveUnreferencedChildren();