Force use of and cache render surface.

components/viz/service/display/surface_aggregator_unittest.cc

Index: components/viz/service/display/surface_aggregator_unittest.cc
diff --git a/components/viz/service/display/surface_aggregator_unittest.cc b/components/viz/service/display/surface_aggregator_unittest.cc
index 9c1446127f158074cb91401952af1a2a3b08554a..7ae57eacf63b0630f4f3e715419f45d2e6be5cb4 100644
--- a/components/viz/service/display/surface_aggregator_unittest.cc
+++ b/components/viz/service/display/surface_aggregator_unittest.cc
@@ -2457,5 +2457,165 @@ TEST_F(SurfaceAggregatorValidSurfaceTest, ColorSpaceTest) {
   EXPECT_EQ(color_space3, aggregated_frame.render_pass_list[2]->color_space);
 }
 
+// Tests that has_damage_from_contributing_content are aggregated correctly from
+// surface quads.
+TEST_F(SurfaceAggregatorValidSurfaceTest, HasDamageFromSurfaces) {

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

Can you split this into smaller test cases?

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

I added a new test for jbauman@'s corner case.

I also separated this test into two tests: HasDamageByChangingChildSurface and
HasDamageByChangingGrandChildSurface.

The other test by adding new child_frame, new grand_child_frame and remove
grand_child_frame are redundant since child_support_->SubmitCompositorFrame will
always damage the surface, which are the same as the test of changing
child_frame.

+  auto grand_child_support = CompositorFrameSinkSupport::Create(
+      nullptr, &manager_, kArbitraryMiddleFrameSinkId, kChildIsRoot,
+      kHandlesFrameSinkIdInvalidation, kNeedsSyncPoints);
+
+  Quad child_surface_quads[] = {Quad::RenderPassQuad(1)};
+  Pass child_surface_passes[] = {
+      Pass(child_surface_quads, arraysize(child_surface_quads), 1)};
+
+  cc::CompositorFrame child_surface_frame =
+      cc::test::MakeEmptyCompositorFrame();
+  AddPasses(&child_surface_frame.render_pass_list, gfx::Rect(SurfaceSize()),
+            child_surface_passes, arraysize(child_surface_passes));
+
+  LocalSurfaceId child_local_surface_id = allocator_.GenerateId();
+  SurfaceId child_surface_id(child_support_->frame_sink_id(),
+                             child_local_surface_id);
+  child_support_->SubmitCompositorFrame(child_local_surface_id,
+                                        std::move(child_surface_frame));
+
+  Quad root_surface_quads[] = {
+      Quad::SurfaceQuad(child_surface_id, InvalidSurfaceId(), 1.f)};
+  Pass root_passes[] = {
+      Pass(root_surface_quads, arraysize(root_surface_quads), 1)};
+
+  cc::CompositorFrame root_frame = cc::test::MakeEmptyCompositorFrame();
+  AddPasses(&root_frame.render_pass_list, gfx::Rect(SurfaceSize()), root_passes,
+            arraysize(root_passes));
+  root_frame.render_pass_list[0]->cache_render_surface = true;
+
+  SurfaceId root_surface_id(support_->frame_sink_id(), root_local_surface_id_);
+  support_->SubmitCompositorFrame(root_local_surface_id_,
+                                  std::move(root_frame));
+
+  cc::CompositorFrame aggregated_frame = aggregator_.Aggregate(root_surface_id);
+  // First aggregation shold be true.
+  EXPECT_TRUE(aggregated_frame.render_pass_list[0]
+                  ->has_damage_from_contributing_content);

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

This is related to my questions in the aggregator, but why does this need to be
true on the first frame? Should the new object already be covered by the root
aggregated damage?

If this was false, what would end up in the cache? Is it possible to build
expectations around that?

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

DamageRectForSurface() makes a surface fully damaged the first time it appears.
Just like the root aggregated damage, "Damage rect for first aggregation should
contain entire root surface" [1] and then we start to track partial damage from
there?
[1]
https://cs.chromium.org/chromium/src/components/viz/service/display/surface_a...
Yes, it is, but has root aggregated damage does not mean we can or cannot cache
the render pass.
If this is false for the first frame, we do not have the texture of the render
pass, we need to always create new texture for the new surface, so we have
nothing to cache at this point?

+
+  // No Surface changed, so no damage should be given.
+  {
+    cc::CompositorFrame aggregated_frame =
+        aggregator_.Aggregate(root_surface_id);
+    EXPECT_FALSE(aggregated_frame.render_pass_list[0]
+                     ->has_damage_from_contributing_content);
+  }
+
+  // Change child_frame should cause damage.
+  {
+    cc::CompositorFrame child_surface_frame =
+        cc::test::MakeEmptyCompositorFrame();
+    AddPasses(&child_surface_frame.render_pass_list, gfx::Rect(SurfaceSize()),
+              child_surface_passes, arraysize(child_surface_passes));
+    child_support_->SubmitCompositorFrame(child_local_surface_id,
+                                          std::move(child_surface_frame));
+
+    cc::CompositorFrame aggregated_frame =
+        aggregator_.Aggregate(root_surface_id);
+    // True for new child_frame.
+    EXPECT_TRUE(aggregated_frame.render_pass_list[0]
+                    ->has_damage_from_contributing_content);
+  }
+
+  // No Surface changed, so no damage should be given.
+  {
+    cc::CompositorFrame aggregated_frame =
+        aggregator_.Aggregate(root_surface_id);
+    EXPECT_FALSE(aggregated_frame.render_pass_list[0]
+                     ->has_damage_from_contributing_content);
+  }
+
+  // Add a grand_child_frame should cause damage.
+  Quad grand_child_quads[] = {Quad::RenderPassQuad(1)};
+  Pass grand_child_passes[] = {
+      Pass(grand_child_quads, arraysize(grand_child_quads), 1)};
+  LocalSurfaceId grand_child_local_surface_id = allocator_.GenerateId();
+  SurfaceId grand_child_surface_id(grand_child_support->frame_sink_id(),
+                                   grand_child_local_surface_id);
+  {
+    cc::CompositorFrame grand_child_frame =
+        cc::test::MakeEmptyCompositorFrame();
+    AddPasses(&grand_child_frame.render_pass_list, gfx::Rect(SurfaceSize()),
+              grand_child_passes, arraysize(grand_child_passes));
+
+    grand_child_support->SubmitCompositorFrame(grand_child_local_surface_id,
+                                               std::move(grand_child_frame));
+
+    Quad new_child_surface_quads[] = {
+        child_surface_quads[0],
+        Quad::SurfaceQuad(grand_child_surface_id, InvalidSurfaceId(), 1.f)};
+    Pass new_child_surface_passes[] = {
+        Pass(new_child_surface_quads, arraysize(new_child_surface_quads), 1)};
+    child_surface_frame = cc::test::MakeEmptyCompositorFrame();
+    AddPasses(&child_surface_frame.render_pass_list, gfx::Rect(SurfaceSize()),
+              new_child_surface_passes, arraysize(new_child_surface_passes));
+    child_support_->SubmitCompositorFrame(child_local_surface_id,
+                                          std::move(child_surface_frame));
+
+    cc::CompositorFrame aggregated_frame =
+        aggregator_.Aggregate(root_surface_id);
+    // True for new grand_child_frame.
+    EXPECT_TRUE(aggregated_frame.render_pass_list[0]
+                    ->has_damage_from_contributing_content);

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

This is related to my questions in the aggregator, but why does this need to be
true on the first frame? Should the new object already be covered by the root
aggregated damage?

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

Similar to previous comments, root aggregated damage does not know where the
damage come from, so it cannot help to set has_damage_from_contributing_content.

+  }
+
+  // No Surface changed, so no damage should be given.
+  {
+    cc::CompositorFrame aggregated_frame =
+        aggregator_.Aggregate(root_surface_id);
+    EXPECT_FALSE(aggregated_frame.render_pass_list[0]
+                     ->has_damage_from_contributing_content);
+  }
+
+  // Change grand_child_frame should cause damage.
+  {
+    cc::CompositorFrame grand_child_frame =
+        cc::test::MakeEmptyCompositorFrame();
+    AddPasses(&grand_child_frame.render_pass_list, gfx::Rect(SurfaceSize()),
+              grand_child_passes, arraysize(grand_child_passes));
+    grand_child_support->SubmitCompositorFrame(grand_child_local_surface_id,
+                                               std::move(grand_child_frame));
+
+    cc::CompositorFrame aggregated_frame =
+        aggregator_.Aggregate(root_surface_id);
+    // True for new grand_child_frame.
+    EXPECT_TRUE(aggregated_frame.render_pass_list[0]
+                    ->has_damage_from_contributing_content);
+  }
+
+  // No Surface changed, so no damage should be given.
+  {
+    cc::CompositorFrame aggregated_frame =
+        aggregator_.Aggregate(root_surface_id);
+    EXPECT_FALSE(aggregated_frame.render_pass_list[0]
+                     ->has_damage_from_contributing_content);
+  }
+
+  // Remove a grand_child_frame should cause damage.
+  {
+    Quad new_child_surface_quads[] = {child_surface_quads[0]};
+    Pass new_child_surface_passes[] = {
+        Pass(new_child_surface_quads, arraysize(new_child_surface_quads), 1)};
+    child_surface_frame = cc::test::MakeEmptyCompositorFrame();
+    AddPasses(&child_surface_frame.render_pass_list, gfx::Rect(SurfaceSize()),
+              new_child_surface_passes, arraysize(new_child_surface_passes));
+    child_support_->SubmitCompositorFrame(child_local_surface_id,
+                                          std::move(child_surface_frame));
+
+    cc::CompositorFrame aggregated_frame =
+        aggregator_.Aggregate(root_surface_id);
+    // True for new child_frame.
+    EXPECT_TRUE(aggregated_frame.render_pass_list[0]
+                    ->has_damage_from_contributing_content);

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

This is related to my questions in the aggregator, but why does this need to be
true on the first frame? Should the removed object already be covered by the
root aggregated damage?

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

Actually, it seems no point for this test, just as previous test, when there is
a new frame for the child_frame (every time:
child_support_->SubmitCompositorFrame), this expectation is always true. Removed
this.

+  }
+
+  grand_child_support->EvictCurrentSurface();
+}
+
 }  // namespace
 }  // namespace viz