Force use of and cache render surface.

components/viz/service/display/surface_aggregator.cc

Index: components/viz/service/display/surface_aggregator.cc
diff --git a/components/viz/service/display/surface_aggregator.cc b/components/viz/service/display/surface_aggregator.cc
index c72ed3606ea78a949a584a5976eb8d251d277b8d..2a7ae8716913752299d63b1125ad7d6c04115c3a 100644
--- a/components/viz/service/display/surface_aggregator.cc
+++ b/components/viz/service/display/surface_aggregator.cc
@@ -265,7 +265,9 @@ void SurfaceAggregator::HandleSurfaceQuad(
     copy_pass->SetAll(remapped_pass_id, source.output_rect, source.output_rect,
                       source.transform_to_root_target, source.filters,
                       source.background_filters, blending_color_space_,
-                      source.has_transparent_background);
+                      source.has_transparent_background,
+                      source.cache_render_pass,
+                      source.has_damage_from_contributing_content);
 
     MoveMatchingRequests(source.id, &copy_requests, &copy_pass->copy_requests);
 
@@ -282,7 +284,13 @@ void SurfaceAggregator::HandleSurfaceQuad(
                     child_to_parent_map, gfx::Transform(), ClipData(),
                     copy_pass.get(), surface_id);
 
+    // If the render pass has copy requests, or should be cached, or has
+    // moving-pixel filters, or in a moving-pixel surface, we should damage the
+    // whole output rect so that we always drawn the full content. Otherwise, we
+    // might have incompleted copy request, or cached patially drawn render pass
+    // .

[danakj, 2017/07/27 21:24:33]

this period should be attached to a word :) so wrap the word instead

[wutao, 2017/07/28 17:05:16]

Done

     if (!copy_request_passes_.count(remapped_pass_id) &&
+        !copy_pass->cache_render_pass &&
         !moved_pixel_passes_.count(remapped_pass_id)) {
       gfx::Transform inverse_transform(gfx::Transform::kSkipInitialization);
       if (copy_pass->transform_to_root_target.GetInverse(&inverse_transform)) {
@@ -293,6 +301,7 @@ void SurfaceAggregator::HandleSurfaceQuad(
       }
     }
 
+    id_to_render_pass_map_[copy_pass->id] = copy_pass.get();

[danakj, 2017/07/27 21:24:33]

one thought here: it seems like this map is only used to check for the bool
has_damage_from_contributing_content. So instead you could make this a set of
RenderPassIds, and only insert if has_damage_from_contributing_content was true,
then code just checks to see if the id is present or not later? Or can that bool
change after it was inserted into the map here?

If we can, then that'd eliminate any map inserts here in the common case.

[wutao, 2017/07/28 17:05:16]

Very nice, thank you.
The bool should not change after it becomes true and be inserted.

     dest_pass_list_->push_back(std::move(copy_pass));
   }
 
@@ -301,6 +310,13 @@ void SurfaceAggregator::HandleSurfaceQuad(
   surface_transform.ConcatTransform(target_transform);
 
   const auto& last_pass = *render_pass_list.back();
+  // This will check if all the surface_quads (including child surfaces) has
+  // damage because HandleSurfaceQuad is a recursive call by calling
+  // CopyQuadsToPass in it.
+  dest_pass->has_damage_from_contributing_content |=
+      !DamageRectForSurface(surface, last_pass, last_pass.output_rect)
+           .IsEmpty();
+
   if (merge_pass) {
     // TODO(jamesr): Clean up last pass special casing.
     const cc::QuadList& quads = last_pass.quad_list;
@@ -410,11 +426,12 @@ void SurfaceAggregator::CopyQuadsToPass(
     const SurfaceId& surface_id) {
   const cc::SharedQuadState* last_copied_source_shared_quad_state = nullptr;
   const cc::SharedQuadState* dest_shared_quad_state = nullptr;
-  // If the current frame has copy requests then aggregate the entire
-  // thing, as otherwise parts of the copy requests may be ignored.
-  const bool ignore_undamaged = aggregate_only_damaged_ &&
-                                !has_copy_requests_ &&
-                                !moved_pixel_passes_.count(dest_pass->id);
+  // If the current frame has copy requests or cahced render passes, then

[danakj, 2017/07/27 21:24:33]

cached

[wutao, 2017/07/28 17:05:16]

Done.

+  // aggregate the entire thing, as otherwise parts of the copy requests may be
+  // ignored and we could cache partially drawn render pass.
+  const bool ignore_undamaged =
+      aggregate_only_damaged_ && !has_copy_requests_ &&
+      !has_cached_render_passes_ && !moved_pixel_passes_.count(dest_pass->id);
   // Damage rect in the quad space of the current shared quad state.
   // TODO(jbauman): This rect may contain unnecessary area if
   // transform isn't axis-aligned.
@@ -455,7 +472,8 @@ void SurfaceAggregator::CopyQuadsToPass(
         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_) {
+        if (aggregate_only_damaged_ && !has_copy_requests_ &&
+            !has_cached_render_passes_) {
           damage_rect_in_quad_space_valid = CalculateQuadSpaceDamageRect(
               dest_shared_quad_state->quad_to_target_transform,
               dest_pass->transform_to_root_target, root_damage_rect_,
@@ -472,8 +490,18 @@ void SurfaceAggregator::CopyQuadsToPass(
       cc::DrawQuad* dest_quad;
       if (quad->material == cc::DrawQuad::RENDER_PASS) {
         const auto* pass_quad = cc::RenderPassDrawQuad::MaterialCast(quad);
-        int original_pass_id = pass_quad->render_pass_id;
-        int remapped_pass_id = RemapPassId(original_pass_id, surface_id);
+        cc::RenderPassId original_pass_id = pass_quad->render_pass_id;
+        cc::RenderPassId remapped_pass_id =
+            RemapPassId(original_pass_id, surface_id);
+
+        // If the RenderPassDrawQuad is referring to other render pass with the
+        // |has_damage_from_contributing_content| set on it, then the dest_pass
+        // should have the flag set on it as well.
+        auto iter = id_to_render_pass_map_.find(remapped_pass_id);
+        if (iter != id_to_render_pass_map_.end()) {
+          dest_pass->has_damage_from_contributing_content |=
+              iter->second->has_damage_from_contributing_content;
+        }
 
         dest_quad = dest_pass->CopyFromAndAppendRenderPassDrawQuad(
             pass_quad, dest_shared_quad_state, remapped_pass_id);
@@ -542,12 +570,21 @@ void SurfaceAggregator::CopyPasses(const cc::CompositorFrame& frame,
     copy_pass->SetAll(remapped_pass_id, source.output_rect, source.output_rect,
                       source.transform_to_root_target, source.filters,
                       source.background_filters, blending_color_space_,
-                      source.has_transparent_background);
+                      source.has_transparent_background,
+                      source.cache_render_pass,
+                      source.has_damage_from_contributing_content);
 
     CopyQuadsToPass(source.quad_list, source.shared_quad_state_list,
                     child_to_parent_map, gfx::Transform(), ClipData(),
                     copy_pass.get(), surface->surface_id());
+
+    // If the render pass has copy requests, or should be cached, or has
+    // moving-pixel filters, or in a moving-pixel surface, we should damage the
+    // whole output rect so that we always drawn the full content. Otherwise, we
+    // might have incompleted copy request, or cached patially drawn render pass
+    // .

[danakj, 2017/07/27 21:24:33]

this one also

[wutao, 2017/07/28 17:05:16]

Done.

     if (!copy_request_passes_.count(remapped_pass_id) &&
+        !copy_pass->cache_render_pass &&
         !moved_pixel_passes_.count(remapped_pass_id)) {
       gfx::Transform inverse_transform(gfx::Transform::kSkipInitialization);
       if (copy_pass->transform_to_root_target.GetInverse(&inverse_transform)) {
@@ -558,6 +595,7 @@ void SurfaceAggregator::CopyPasses(const cc::CompositorFrame& frame,
       }
     }
 
+    id_to_render_pass_map_[copy_pass->id] = copy_pass.get();
     dest_pass_list_->push_back(std::move(copy_pass));
   }
 }
@@ -768,11 +806,12 @@ gfx::Rect SurfaceAggregator::PrewalkTree(const SurfaceId& surface_id,
 
   CHECK(debug_weak_this.get());
   for (const auto& render_pass : frame.render_pass_list) {
-    if (!render_pass->copy_requests.empty()) {
-      cc::RenderPassId remapped_pass_id =
-          RemapPassId(render_pass->id, surface_id);
+    cc::RenderPassId remapped_pass_id =
+        RemapPassId(render_pass->id, surface_id);
+    if (!render_pass->copy_requests.empty())
       copy_request_passes_.insert(remapped_pass_id);
-    }
+    if (render_pass->cache_render_pass)
+      cached_render_passes_.insert(remapped_pass_id);

[danakj, 2017/07/27 21:24:33]

it seems like we're building this structure just to ask if it's empty, so we
could just use a bool?

[wutao, 2017/07/28 17:05:16]

Good point, done.

   }
 
   // TODO(jbauman): Remove when https://crbug.com/745684 fixed.
@@ -871,6 +910,7 @@ cc::CompositorFrame SurfaceAggregator::Aggregate(const SurfaceId& surface_id) {
   root_damage_rect_ = PrewalkTree(surface_id, false, 0, &prewalk_result);
   PropagateCopyRequestPasses();
   has_copy_requests_ = !copy_request_passes_.empty();
+  has_cached_render_passes_ = !cached_render_passes_.empty();
   frame.metadata.may_contain_video = prewalk_result.may_contain_video;
 
   CopyUndrawnSurfaces(&prewalk_result);
@@ -901,6 +941,7 @@ cc::CompositorFrame SurfaceAggregator::Aggregate(const SurfaceId& surface_id) {
     return {};
 
   dest_pass_list_ = NULL;
+  id_to_render_pass_map_.clear();
   ProcessAddedAndRemovedSurfaces();
   contained_surfaces_.swap(previous_contained_surfaces_);
   contained_surfaces_.clear();