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..2e0c51b341317c1fdf14c862a2ce4feb2df3eed2 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);
 
     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) &&
+        !copy_pass->cache_render_surface &&

[danakj, 2017/07/25 18:55:02]

I feel that each of these 3 conditions needs a comment explaining why it is
here. I tried to understand why this one is here below.. lmk.

[wutao, 2017/07/26 07:48:52]

Do not need this change anymore.

But I try to explain why it is here IIUC.
copy_pass's damage_rect by default is the source.output_rect. See line 265:
copy_pass->SetAll(...source.output_rect...).

When there is no copy_request or moved pixel_pass (I am guessing
moved-pixel-pass also needs full output damage because it can read pixel outside
the render pass?), we try to get the clipped damage rect on the render pass
space by line 295:
copy_pass->damage_rect.Intersect(damage_rect_in_render_pass_space);

This will be used in
DirectRenderer::DrawingFrame::ComputeScissorRectForRenderPass. If there are
partial_swap, then we allow non-root-render-pass damage_rect to be smaller than
the output_rect.

[wutao, 2017/07/27 02:30:13]

Added comments.

         !moved_pixel_passes_.count(remapped_pass_id)) {
       gfx::Transform inverse_transform(gfx::Transform::kSkipInitialization);
       if (copy_pass->transform_to_root_target.GetInverse(&inverse_transform)) {
@@ -301,6 +304,17 @@ 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. This is different than the damage_rect calculated in

[danakj, 2017/07/25 18:55:03]

I'm not clear why this is talking about Prewalk tree, maybe there's some
indirection here which isn't clear. We're computing damage_rect in this function
too, on line 295. So I was wondering what's different from here to there.

IIUC the difference is:
- Above we are reducing (setting?) the damage on the copy_pass to be the
computed damage for the root aggregated frame being swapped. Since offscreen
copy requests are not being swapped, they are not part of the root aggregated
damage, so in that case we don't know what is damaged there and leave it fully
damaged.

You added cache_render_surface there also for unrelated reasons, which are that
you want to ensure the cached render surface is fully drawn even if not all of
it is damaged? I don't follow the logic for why that's happening - it seems to
imply that we have a render surface already that is now partially damaged, but
we want to redraw the pixels that are already present.. Or can you have a render
surface appear the first time and not all of it is damaged? If so, how would
that happen?

- Below here, we are determining (recursively) if any aggregated frames for
SurfaceDrawQuads have damage. RenderPassDrawQuads are not checked because we
expect the client submitting the RenderPass to do that check.

If this is write can you turn more of this into comments?

[wutao, 2017/07/26 07:48:52]

I should mention Prewalk here. I did just because try to address your comments
in previous patch.
Make sense to me.
You could be totally right here. Thanks! I will test on removing this. Sounds
like the damage_rect only affect drawing/swap, not affect caching.
Yes, we are expecting the client submitting the RenderPass to do that check. But
thinking more with jbauman@'s corner case example below, I think we still need
this code in previous patch:
https://codereview.chromium.org/2873593002/diff/260001/components/viz/service...

+  // PrewalkTree, which also includes all the damage in the top level surface,
+  // containing damage from damage_tracker.

[danakj, 2017/07/25 18:55:03]

"containing damage specified with the CompositorFrame" or something. damage
tracker is part of layer compositor which this code does not know about and
could change arbitrarily.

[wutao, 2017/07/26 07:48:52]

Deleted comments related to PrewalkTree.

+  if (dest_pass->cache_render_surface) {
+    dest_pass->has_damage_from_contributing_content |=

[jbauman, 2017/07/25 20:40:34]

Suppose we have a
Surface 1 with damage, and a surface 2:
Pass A) SurfaceDrawQuad referencing 1
Pass B) RenderPassDrawQuad referencing A

If B has cache_render_surface, shouldn't has_damage_from_contributing_content be
set on it?

Also, what do we do with has_damage_from_contributing_content? I don't see it
used anywhere

[wutao, 2017/07/26 07:48:53]

Yes. we should set the flag. You example makes me think I do need this code in
previous patch:
https://codereview.chromium.org/2873593002/diff/280001/cc/output/direct_rende...
Explained as this:
When we process Pass A, we insert to dest_pass_list_ a render_pass_1 with damage
and flag (has_damage_from_contributing_content) set on it.
When we process Pass B, we have the remapped_pass_id in the
id_to_render_pass_map_, and it is referring to render_pass_1, which has the flag
set, then we will set the render_pass_2 as damaged as well.

I am adding a new test for this.
If has_damage_from_contributing_content is set, then in
DirectRenderer::UseRenderPass, we will not reuse the texture of the render pass:
https://codereview.chromium.org/2873593002/diff/280001/cc/output/direct_rende...

+        !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 +424,11 @@ 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, because we want full content.

[danakj, 2017/07/25 18:55:02]

I think this comment is saying what we want as the outcome but not why we need
to do it. For copy requests, it's because of how we aggregate damage for only
surfaces that will be swapped, right? For cached render surfaces, is it possible
to have a surface not fully damaged when it first appears, or is it something
else? DamageRectForSurface() seems to make a surface fully damaged the first
time it appears... do you have a unit test showing why we need
has_cached_render_surfaces_ here, and that we get an incorrect cache result
without it?

Put another way, for each if-condition added in this patch like this, if you
remove it, does some test fail?

I wanted to just mention also that it's unfortunate IMO that this patch is
phrasing things in terms of render surfaces (which are a mostly obsolete term
from layer compositors, they don't even exist anymore except or
RenderSurfaceImpl), instead of render targets and render passes. Especially for
types and variables that are used from SurfaceAggregator for which "Surface" has
a completely different meaning causing confusion, and "RenderSurface" is not
something it could every know about.

[jbauman, 2017/07/25 20:40:34]

I think if the renderpass is partially offscreen initially then it might not
necessarily be completely damaged then, because we start with global
screen-space damage rect and work from there to find the damage rect for the
renderpass.

[wutao, 2017/07/26 07:48:53]

We do not need this change any more for cache render pass.
I renamed the cache render surface to cache render pass in render_pass.cc,
surface aggregator.cc and direct_renderer.cc.

[wutao, 2017/07/27 02:30:13]

Yes, if the render pass is off screen, then it is not fully damaged. We should
avoid this happening.
Added some tests, please see below.
Added several tests to cover all the changes in this patch:
1. HasDamageByChangingChildSurface
for why we need DamageRectForSurface in HandleSurfaceQuad to get child surface
changes.
2. HasDamageByChangingGrandChildSurface
for why we need DamageRectForSurface in HandleSurfaceQuad to get grand child
surface changes.

3. HasDamageFromRenderPassQuads
for the change in if block (from line 499) if (quad->material ==
cc::DrawQuad::RENDER_PASS) in CopyQuadsToPass.
To get damage from RenderPassQuads.

4. DamageRectOfCachedRenderPass
for why we need the change (line 585) !copy_pass->cache_render_pass && in
CopyPasses to make the off screen render pass fully damaged.

5. DamageRectOfCachedRenderPassInChildSurface
for why we need the change (line 293) !copy_pass->cache_render_pass && in
HandleSurfaceQuad to make the off screen render pass fully damaged.

6. NotIgnoreOutsideForCachedRenderPass:
for why we need ignore_undamaged = ... && !has_cached_render_surfaces && ..., so
that we will not ignore outside render pass.

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

Thanks!

+  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 +469,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_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 +489,6 @@ 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);
-
         dest_quad = dest_pass->CopyFromAndAppendRenderPassDrawQuad(
             pass_quad, dest_shared_quad_state, remapped_pass_id);
       } else if (quad->material == cc::DrawQuad::TEXTURE_CONTENT) {
@@ -542,12 +556,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);
 
     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) &&
+        !copy_pass->cache_render_surface &&

[danakj, 2017/07/25 18:55:03]

I also think each of these needs the same comments explaining why the condition
is here, matching HandleSurfaceQuad() comments if that's the case.

[wutao, 2017/07/26 07:48:53]

Do not need this any more.

[wutao, 2017/07/27 02:30:13]

Added comments.

         !moved_pixel_passes_.count(remapped_pass_id)) {
       gfx::Transform inverse_transform(gfx::Transform::kSkipInitialization);
       if (copy_pass->transform_to_root_target.GetInverse(&inverse_transform)) {
@@ -761,11 +778,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()));
@@ -857,6 +875,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_surfaces_ = !cached_render_surface_passes_.empty();
   frame.metadata.may_contain_video = prewalk_result.may_contain_video;
 
   CopyUndrawnSurfaces(&prewalk_result);
@@ -868,6 +887,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.