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 4e1698d47613c5f605bad661703a0f3dbd279538..41c95ad76ca46f512f0d1fcd2424b8bb3b7c39c9 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_surface,
+                      source.has_damage_from_contributing_content);

[danakj, 2017/07/19 22:52:55]

Are you expecting CompositorFrames from clients to report
has_damage_from_contributing_content on renderpasses, or is it only meant to be
computed here? Which side is responsible for what parts of the computation?

[wutao, 2017/07/21 05:59:24]

There are 3 contributing_contents:
1. contributing render surface,
2. contributing layer,
3. surface qaud.

1 and 2 are handled by damage_tracker. Clients should report
has_damage_from_contributing_content of RenderPasses.

Here HandleSurfaceQuad is to calculate damage from surface qaud.

 
     MoveMatchingRequests(source.id, &copy_requests, &copy_pass->copy_requests);
 
@@ -283,6 +285,7 @@ void SurfaceAggregator::HandleSurfaceQuad(
                     copy_pass.get(), surface_id);
 
     if (!copy_request_passes_.count(remapped_pass_id) &&

[danakj, 2017/07/19 22:52:55]

Can you help me understand why in CopyQuadsToPass we check if there exists /any/
(copy requests|cached output) to ignore damage, but here we compute damage
anyway unless /this/ renderpass is (being copied|being cached).

[wutao, 2017/07/21 05:59:24]

Here we also ignore damage and do not recalculate damage if there is a copy
request. It is if (!copy_request...)
jbauman@ might be the best person to answer this. IIRC, non-root-renderpass'
damage_rect can be used in partial-swap. However if there is a copy request, the
copy_pass' damage_rect will be the output_rect. As indicated by the comment in
line 425, "If the current frame has copy requests, then aggregate the entire
thing, as otherwise parts of the copy requests may be ignored."

+        !cached_render_surface_passes_.count(remapped_pass_id) &&

[danakj, 2017/07/19 22:52:54]

How is this different than checking source.cache_render_surface ?

[wutao, 2017/07/21 05:59:24]

I think you are right. So I do not need to modify the function "PropogatePass"
to collecting this info. That would remove many changes in this cl.

         !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 +296,8 @@ void SurfaceAggregator::HandleSurfaceQuad(
       }
     }
 
+    id_to_render_pass_map_.insert(
+        std::make_pair(copy_pass->id, copy_pass.get()));
     dest_pass_list_->push_back(std::move(copy_pass));
   }
 
@@ -301,6 +306,13 @@ void SurfaceAggregator::HandleSurfaceQuad(
   surface_transform.ConcatTransform(target_transform);
 
   const auto& last_pass = *render_pass_list.back();
+  // Check if the surface_quad has damage.

[danakj, 2017/07/19 22:52:54]

Can this comment explain why this damage check is different from the above
computation of the copy_pass->damage_rect?

[wutao, 2017/07/21 05:59:24]

Will change the comment. The damage here is only from surface quads. The
copy_pass->damage_rect has all the damages on the top level surface including
all the damage from damage_tracker.

+  if (dest_pass->cache_render_surface) {
+    dest_pass->has_damage_from_contributing_content |=
+        !DamageRectForSurface(surface, last_pass, last_pass.output_rect)

[danakj, 2017/07/19 22:52:54]

This is different from the use of this function in PrewalkTree, it also looks at
child surfaces. Is that intentional?

Why doesn't this use a damage result computed there so we don't have to do it
again?

[wutao, 2017/07/21 05:59:24]

This will check if all the surface_quads (including child surfaces) has
damage because HandleSurfaceQuad is a recursive call by calling
CopyQuadsToPass in it.

This is different than the damage_rect calculated in PrewalkTree, which also
includes all the damage in the top level surface, containing damage from
damage_tracker.

+             .IsEmpty();
+  }
+
   if (merge_pass) {
     // TODO(jamesr): Clean up last pass special casing.
     const cc::QuadList& quads = last_pass.quad_list;
@@ -410,11 +422,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 cache render surface, then
+  // aggregate the entire thing, as otherwise parts of the copy requests or
+  // render surface may be ignored.

[danakj, 2017/07/19 22:52:55]

I think you mean to say that you want what's in the cache to be the full content
here?

[wutao, 2017/07/21 05:59:24]

Yes. Changed accordingly.

+  const bool ignore_undamaged =
+      aggregate_only_damaged_ && !has_copy_requests_ &&
+      !has_cached_render_surfaces_ && !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 +468,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_ &&

[danakj, 2017/07/19 22:52:54]

Can you help me understand why this doesn't compute damage_rect_in_quad_space
when there is any cached renderpass, but we do when there's moved pixel passes,
even tho in both cases we're going to aggregate all the quads anyway?

[wutao, 2017/07/21 05:59:24]

Looks like the final result is same, but we might do extra calculate if there is
moved pixel in the pass in the current code. When there's moved pixel in the
pass, the ignore_undamaged if false, then the damage_rect_in_quad_space_valid is
not used. If there is no moved pixel in the pass, then we can ignore the
out-side quad damage.

+            !has_cached_render_surfaces_) {
           damage_rect_in_quad_space_valid = CalculateQuadSpaceDamageRect(
               dest_shared_quad_state->quad_to_target_transform,
               dest_pass->transform_to_root_target, root_damage_rect_,
@@ -474,7 +488,13 @@ void SurfaceAggregator::CopyQuadsToPass(
         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);
-
+        if (dest_pass->cache_render_surface) {
+          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;

[danakj, 2017/07/19 22:52:54]

Is this just what HandleSurfaceDrawQuad() computed above on L463? Should it just
get that from there? Or is this including other information, from where?

[wutao, 2017/07/21 05:59:24]

They are in different code path. Here is to handle cc::DrawQuad::RENDER_PASS. I
should have already checked this damage either in HandleSurfaceDrawQuad or
render surface in damage_tracker. Removed related code.

+          }
+        }
         dest_quad = dest_pass->CopyFromAndAppendRenderPassDrawQuad(
             pass_quad, dest_shared_quad_state, remapped_pass_id);
       } else if (quad->material == cc::DrawQuad::TEXTURE_CONTENT) {
@@ -542,12 +562,15 @@ 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_surface,
+                      source.has_damage_from_contributing_content);

[danakj, 2017/07/19 22:52:54]

Same questions here as in HandleSurfaceDrawQuad

[wutao, 2017/07/21 05:59:24]

Please see the reply to the other comment.

 
     CopyQuadsToPass(source.quad_list, source.shared_quad_state_list,
                     child_to_parent_map, gfx::Transform(), ClipData(),
                     copy_pass.get(), surface->surface_id());
     if (!copy_request_passes_.count(remapped_pass_id) &&
+        !cached_render_surface_passes_.count(remapped_pass_id) &&
         !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 +581,8 @@ void SurfaceAggregator::CopyPasses(const cc::CompositorFrame& frame,
       }
     }
 
+    id_to_render_pass_map_.insert(
+        std::make_pair(copy_pass->id, copy_pass.get()));
     dest_pass_list_->push_back(std::move(copy_pass));
   }

[danakj, 2017/07/19 22:52:54]

It doesn't compute the damage from contributing things here, is that
intentional? Do copied currently-offscreen surfaces not need to compute that?
Why?

I think the root doesn't need to because we can't cache that one, is that right?

[wutao, 2017/07/21 05:59:24]

All damage from contributing contents should be handled:
1. copy_pass->SetAll(...)
2. HandleSurfaceQuad. Are offscreen surfaces handled here?

 }
@@ -761,11 +786,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_surface)
+      cached_render_surface_passes_.insert(remapped_pass_id);
   }
 
   referenced_surfaces_.erase(referenced_surfaces_.find(surface->surface_id()));
@@ -818,18 +844,19 @@ void SurfaceAggregator::CopyUndrawnSurfaces(PrewalkResult* prewalk_result) {
   }
 }
 
-void SurfaceAggregator::PropagateCopyRequestPasses() {
-  std::vector<cc::RenderPassId> copy_requests_to_iterate(
-      copy_request_passes_.begin(), copy_request_passes_.end());
-  while (!copy_requests_to_iterate.empty()) {
-    int first = copy_requests_to_iterate.back();
-    copy_requests_to_iterate.pop_back();
+void SurfaceAggregator::PropagatePasses(
+    base::flat_set<cc::RenderPassId>* passes) {
+  std::vector<cc::RenderPassId> passes_to_iterate(passes->begin(),
+                                                  passes->end());
+  while (!passes_to_iterate.empty()) {
+    cc::RenderPassId first = passes_to_iterate.back();
+    passes_to_iterate.pop_back();
     auto it = render_pass_dependencies_.find(first);
     if (it == render_pass_dependencies_.end())
       continue;
     for (auto pass : it->second) {
-      if (copy_request_passes_.insert(pass).second) {
-        copy_requests_to_iterate.push_back(pass);
+      if (passes->insert(pass).second) {
+        passes_to_iterate.push_back(pass);
       }
     }
   }
@@ -855,8 +882,10 @@ cc::CompositorFrame SurfaceAggregator::Aggregate(const SurfaceId& surface_id) {
   valid_surfaces_.clear();
   PrewalkResult prewalk_result;
   root_damage_rect_ = PrewalkTree(surface_id, false, 0, &prewalk_result);
-  PropagateCopyRequestPasses();
+  PropagatePasses(&copy_request_passes_);

[danakj, 2017/07/19 22:52:55]

granted the name comes from what was there before but what is this propogating?
I don't think it describes what it's doing at all.. it's more like collecting or
something. Can you improve this?

[wutao, 2017/07/21 05:59:24]

1. I do not need to modify this function.
2. I think the name could be fine. It is propagating all the dependent passes to
the /copy_request_passes_/.

+  PropagatePasses(&cached_render_surface_passes_);
   has_copy_requests_ = !copy_request_passes_.empty();
+  has_cached_render_surfaces_ = !cached_render_surface_passes_.empty();
   frame.metadata.may_contain_video = prewalk_result.may_contain_video;
 
   CopyUndrawnSurfaces(&prewalk_result);
@@ -868,6 +897,7 @@ cc::CompositorFrame SurfaceAggregator::Aggregate(const SurfaceId& surface_id) {
 
   moved_pixel_passes_.clear();
   copy_request_passes_.clear();
+  cached_render_surface_passes_.clear();
   render_pass_dependencies_.clear();
 
   // Remove all render pass mappings that weren't used in the current frame.
@@ -887,6 +917,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();