cc: Ensure that large damage doesn't register as "frame has no damage"

cc/trees/damage_tracker.cc

Index: cc/trees/damage_tracker.cc
diff --git a/cc/trees/damage_tracker.cc b/cc/trees/damage_tracker.cc
index 8a2a3ff6f031e4126b5814dd2e1ca7d1266aba69..6225264d91732929f24aed8d5aa9558ffa2671fb 100644
--- a/cc/trees/damage_tracker.cc
+++ b/cc/trees/damage_tracker.cc
@@ -29,20 +29,6 @@ DamageTracker::DamageTracker()
 
 DamageTracker::~DamageTracker() {}
 
-static inline void ExpandDamageRectInsideRectWithFilters(
-    gfx::Rect* damage_rect,
-    const gfx::Rect& pre_filter_rect,
-    const FilterOperations& filters) {
-  // Compute the pixels in the background of the surface that could be affected
-  // by the damage in the content below.
-  gfx::Rect expanded_damage_rect = filters.MapRect(*damage_rect, SkMatrix::I());
-
-  // Restrict it to the rectangle in which the background filter is shown.
-  expanded_damage_rect.Intersect(pre_filter_rect);
-
-  damage_rect->Union(expanded_damage_rect);
-}
-
 void DamageTracker::UpdateDamageTrackingState(
     const LayerImplList& layer_list,
     const RenderSurfaceImpl* target_surface,
@@ -122,30 +108,47 @@ void DamageTracker::UpdateDamageTrackingState(
   // These functions cannot be bypassed with early-exits, even if we know what
   // the damage will be for this frame, because we need to update the damage
   // tracker state to correctly track the next frame.
-  gfx::Rect damage_from_active_layers =
+  DamageAccumulator damage_from_active_layers =
       TrackDamageFromActiveLayers(layer_list, target_surface);
-  gfx::Rect damage_from_surface_mask =
+  DamageAccumulator damage_from_surface_mask =
       TrackDamageFromSurfaceMask(target_surface_mask_layer);
-  gfx::Rect damage_from_leftover_rects = TrackDamageFromLeftoverRects();
+  DamageAccumulator damage_from_leftover_rects = TrackDamageFromLeftoverRects();
 
-  gfx::Rect damage_rect_for_this_update;
+  DamageAccumulator damage_for_this_update;
 
   if (target_surface_property_changed_only_from_descendant) {
-    damage_rect_for_this_update = target_surface_content_rect;
+    damage_for_this_update.Union(target_surface_content_rect);
   } else {
     // TODO(shawnsingh): can we clamp this damage to the surface's content rect?
     // (affects performance, but not correctness)
-    damage_rect_for_this_update = damage_from_active_layers;
-    damage_rect_for_this_update.Union(damage_from_surface_mask);
-    damage_rect_for_this_update.Union(damage_from_leftover_rects);
-    damage_rect_for_this_update =
-        filters.MapRect(damage_rect_for_this_update,
-                        target_surface->FiltersTransform().matrix());
+    damage_for_this_update.Union(damage_from_active_layers);
+    damage_for_this_update.Union(damage_from_surface_mask);
+    damage_for_this_update.Union(damage_from_leftover_rects);
+
+    gfx::Rect damage_rect;
+    bool is_rect_valid = damage_for_this_update.GetAsRect(&damage_rect);
+    if (is_rect_valid) {
+      damage_rect = filters.MapRect(
+          damage_rect, target_surface->FiltersTransform().matrix());
+      damage_for_this_update = DamageAccumulator();
+      damage_for_this_update.Union(damage_rect);
+    }
   }
 
   // Damage accumulates until we are notified that we actually did draw on that
   // frame.
-  current_damage_rect_.Union(damage_rect_for_this_update);
+  current_damage_.Union(damage_for_this_update);
+}
+
+bool DamageTracker::ShouldDamageEverything() {
+  gfx::Rect rect;
+  return !current_damage_.GetAsRect(&rect);

[danakj, 2017/01/13 23:17:58]

I thot it a bit odd this computes a rect to throw it away, and compute it again
below. I also noticed all callers of ShouldDamageEverything then call
CurrentDamageRect after and vice versa.. So maybe they should be one function
that returns bool and rect also?

[vmpstr, 2017/01/19 23:08:27]

Done. I with with bool GetRect(Rect*) type of function, but let me know if you
prefer something different like returning a pair or maybe returning a rect and
taking an optional bool*

+}
+
+gfx::Rect DamageTracker::CurrentDamageRect() {
+  gfx::Rect rect;
+  current_damage_.GetAsRect(&rect);
+  return rect;
 }
 
 DamageTracker::LayerRectMapData& DamageTracker::RectDataForLayer(
@@ -181,10 +184,10 @@ DamageTracker::SurfaceRectMapData& DamageTracker::RectDataForSurface(
   return *it;
 }
 
-gfx::Rect DamageTracker::TrackDamageFromActiveLayers(
+DamageTracker::DamageAccumulator DamageTracker::TrackDamageFromActiveLayers(
     const LayerImplList& layer_list,
     const RenderSurfaceImpl* target_surface) {
-  gfx::Rect damage_rect;
+  DamageAccumulator damage;
 
   for (size_t layer_index = 0; layer_index < layer_list.size(); ++layer_index) {
     // Visit layers in back-to-front order.
@@ -197,42 +200,42 @@ gfx::Rect DamageTracker::TrackDamageFromActiveLayers(
       continue;
 
     if (layer->render_surface() && layer->render_surface() != target_surface)
-      ExtendDamageForRenderSurface(layer->render_surface(), &damage_rect);
+      ExtendDamageForRenderSurface(layer->render_surface(), &damage);
     else
-      ExtendDamageForLayer(layer, &damage_rect);
+      ExtendDamageForLayer(layer, &damage);
   }
 
-  return damage_rect;
+  return damage;
 }
 
-gfx::Rect DamageTracker::TrackDamageFromSurfaceMask(
+DamageTracker::DamageAccumulator DamageTracker::TrackDamageFromSurfaceMask(
     LayerImpl* target_surface_mask_layer) {
-  gfx::Rect damage_rect;
+  DamageAccumulator damage;
 
   if (!target_surface_mask_layer)
-    return damage_rect;
+    return damage;
 
   // Currently, if there is any change to the mask, we choose to damage the
   // entire surface. This could potentially be optimized later, but it is not
   // expected to be a common case.
   if (target_surface_mask_layer->LayerPropertyChanged() ||
       !target_surface_mask_layer->update_rect().IsEmpty()) {
-    damage_rect = gfx::Rect(target_surface_mask_layer->bounds());
+    damage.Union(gfx::Rect(target_surface_mask_layer->bounds()));
   }
 
-  return damage_rect;
+  return damage;
 }
 
 void DamageTracker::PrepareRectHistoryForUpdate() {
   mailboxId_++;
 }
 
-gfx::Rect DamageTracker::TrackDamageFromLeftoverRects() {
+DamageTracker::DamageAccumulator DamageTracker::TrackDamageFromLeftoverRects() {
   // After computing damage for all active layers, any leftover items in the
   // current rect history correspond to layers/surfaces that no longer exist.
   // So, these regions are now exposed on the target surface.
 
-  gfx::Rect damage_rect;
+  DamageAccumulator damage;
   SortedRectMapForLayers::iterator layer_cur_pos =
       rect_history_for_layers_.begin();
   SortedRectMapForLayers::iterator layer_copy_pos = layer_cur_pos;
@@ -241,7 +244,7 @@ gfx::Rect DamageTracker::TrackDamageFromLeftoverRects() {
   SortedRectMapForSurfaces::iterator surface_copy_pos = surface_cur_pos;
 
   // Loop below basically implements std::remove_if loop with and extra
-  // processing (adding deleted rect to damage_rect) for deleted items.
+  // processing (adding deleted rect to damage) for deleted items.
   // cur_pos iterator runs through all elements of the vector, but copy_pos
   // always points to the element after the last not deleted element. If new
   // not deleted element found then it is copied to the *copy_pos and copy_pos
@@ -255,7 +258,7 @@ gfx::Rect DamageTracker::TrackDamageFromLeftoverRects() {
 
       ++layer_copy_pos;
     } else {
-      damage_rect.Union(layer_cur_pos->rect_);
+      damage.Union(layer_cur_pos->rect_);
     }
 
     ++layer_cur_pos;
@@ -268,7 +271,7 @@ gfx::Rect DamageTracker::TrackDamageFromLeftoverRects() {
 
       ++surface_copy_pos;
     } else {
-      damage_rect.Union(surface_cur_pos->rect_);
+      damage.Union(surface_cur_pos->rect_);
     }
 
     ++surface_cur_pos;
@@ -290,11 +293,32 @@ gfx::Rect DamageTracker::TrackDamageFromLeftoverRects() {
     SortedRectMapForSurfaces(rect_history_for_surfaces_)
         .swap(rect_history_for_surfaces_);
 
-  return damage_rect;
+  return damage;
+}
+
+void DamageTracker::ExpandDamageInsideRectWithFilters(
+    DamageAccumulator* damage,
+    const gfx::Rect& pre_filter_rect,
+    const FilterOperations& filters) {
+  gfx::Rect damage_rect;
+  bool is_valid_rect = damage->GetAsRect(&damage_rect);
+  // If the input isn't a valid rect, then there is no point in trying to make
+  // it bigger.
+  if (!is_valid_rect)
+    return;
+
+  // Compute the pixels in the background of the surface that could be affected
+  // by the damage in the content below.
+  gfx::Rect expanded_damage_rect = filters.MapRect(damage_rect, SkMatrix::I());
+
+  // Restrict it to the rectangle in which the background filter is shown.
+  expanded_damage_rect.Intersect(pre_filter_rect);
+
+  damage->Union(expanded_damage_rect);
 }
 
 void DamageTracker::ExtendDamageForLayer(LayerImpl* layer,
-                                         gfx::Rect* target_damage_rect) {
+                                         DamageAccumulator* target_damage) {
   // There are two ways that a layer can damage a region of the target surface:
   //   1. Property change (e.g. opacity, position, transforms):
   //        - the entire region of the layer itself damages the surface.
@@ -323,11 +347,11 @@ void DamageTracker::ExtendDamageForLayer(LayerImpl* layer,
   if (layer_is_new || layer->LayerPropertyChanged()) {
     // If a layer is new or has changed, then its entire layer rect affects the
     // target surface.
-    target_damage_rect->Union(rect_in_target_space);
+    target_damage->Union(rect_in_target_space);
 
     // The layer's old region is now exposed on the target surface, too.
     // Note old_rect_in_target_space is already in target space.
-    target_damage_rect->Union(old_rect_in_target_space);
+    target_damage->Union(old_rect_in_target_space);
     return;
   }
 
@@ -339,13 +363,13 @@ void DamageTracker::ExtendDamageForLayer(LayerImpl* layer,
   if (!damage_rect.IsEmpty()) {
     gfx::Rect damage_rect_in_target_space =
         MathUtil::MapEnclosingClippedRect(layer->DrawTransform(), damage_rect);
-    target_damage_rect->Union(damage_rect_in_target_space);
+    target_damage->Union(damage_rect_in_target_space);
   }
 }
 
 void DamageTracker::ExtendDamageForRenderSurface(
     RenderSurfaceImpl* render_surface,
-    gfx::Rect* target_damage_rect) {
+    DamageAccumulator* target_damage) {
   // There are two ways a "descendant surface" can damage regions of the "target
   // surface":
   //   1. Property change:
@@ -372,22 +396,27 @@ void DamageTracker::ExtendDamageForRenderSurface(
 
   if (surface_is_new || render_surface->SurfacePropertyChanged()) {
     // The entire surface contributes damage.
-    target_damage_rect->Union(surface_rect_in_target_space);
+    target_damage->Union(surface_rect_in_target_space);
 
     // The surface's old region is now exposed on the target surface, too.
-    target_damage_rect->Union(old_surface_rect);
+    target_damage->Union(old_surface_rect);
   } else {
     // Only the surface's damage_rect will damage the target surface.
-    gfx::Rect damage_rect_in_local_space =
-        render_surface->damage_tracker()->current_damage_rect();
-
-    // If there was damage, transform it to target space, and possibly
-    // contribute its reflection if needed.
-    if (!damage_rect_in_local_space.IsEmpty()) {
-      const gfx::Transform& draw_transform = render_surface->draw_transform();
-      gfx::Rect damage_rect_in_target_space = MathUtil::MapEnclosingClippedRect(
-          draw_transform, damage_rect_in_local_space);
-      target_damage_rect->Union(damage_rect_in_target_space);
+    if (render_surface->damage_tracker()->ShouldDamageEverything()) {
+      target_damage->SetInvalid();

[enne (OOO), 2017/01/13 22:21:38]

Should you use the render surface content rect as the damage rect in this case,
instead of propagating the invalid state?

[vmpstr, 2017/01/19 23:08:27]

Done.

+    } else {
+      gfx::Rect damage_rect_in_local_space =
+          render_surface->damage_tracker()->CurrentDamageRect();
+
+      // If there was damage, transform it to target space, and possibly
+      // contribute its reflection if needed.
+      if (!damage_rect_in_local_space.IsEmpty()) {
+        const gfx::Transform& draw_transform = render_surface->draw_transform();
+        gfx::Rect damage_rect_in_target_space =
+            MathUtil::MapEnclosingClippedRect(draw_transform,
+                                              damage_rect_in_local_space);
+        target_damage->Union(damage_rect_in_target_space);
+      }
     }
   }
 
@@ -400,9 +429,35 @@ void DamageTracker::ExtendDamageForRenderSurface(
   const FilterOperations& background_filters =
       render_surface->BackgroundFilters();
   if (background_filters.HasFilterThatMovesPixels()) {
-    ExpandDamageRectInsideRectWithFilters(
-        target_damage_rect, surface_rect_in_target_space, background_filters);
+    ExpandDamageInsideRectWithFilters(
+        target_damage, surface_rect_in_target_space, background_filters);
+  }
+}
+
+bool DamageTracker::DamageAccumulator::GetAsRect(gfx::Rect* rect) {
+  if (!is_valid_rect_) {
+    *rect = gfx::Rect();

[danakj, 2017/01/13 23:17:58]

think not needed? dont use the rect if return is false

[vmpstr, 2017/01/19 23:08:27]

Done.

+    return false;
   }
+
+  double width = right_ - static_cast<double>(x_);

[danakj, 2017/01/13 23:17:58]

does not base::CheckedNumeric do this stuff and you should use that?

[vmpstr, 2017/01/19 23:08:27]

Done.

+  if (width > static_cast<double>(std::numeric_limits<int>::max())) {

[danakj, 2017/01/13 23:17:58]

if not, can you store the intmax as a double in a constexr variable?

[vmpstr, 2017/01/19 23:08:27]

Removed this.

+    *rect = gfx::Rect();
+    is_valid_rect_ = false;
+    return false;
+  }
+  double height = bottom_ - static_cast<double>(bottom_);
+  if (height > static_cast<double>(std::numeric_limits<int>::max())) {
+    *rect = gfx::Rect();
+    is_valid_rect_ = false;
+    return false;
+  }
+
+  rect->set_x(x_);
+  rect->set_y(y_);
+  rect->set_width(right_ - x_);
+  rect->set_height(bottom_ - y_);
+  return true;
 }
 
 }  // namespace cc