cc: Stop taking in SharedQuadState* when copying DrawQuad

When copying DrawQuad to a RenderPass, the SharedQuadState should
already be on that RenderPass. Do not take SharedQuadState* as input
parameter for copying DrawQuad, use the last SharedQuadState on the
RenderPass instead. This not only avoids invalid SharedQuadState* being
passed in, but also reinforce that SharedQuadState and DrawQuad are
appended in order so it satisifies assumption for future implementation
of iterator after SharedQuadState id.

R=danakj@chromium.org

Bug: 739429
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I63fc2ca7a250fb8f6f9056473e0c6ab046652018
Reviewed-on: https://chromium-review.googlesource.com/597228Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: weiliangc <weiliangc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491509}

cc/ipc/cc_param_traits_unittest.cc

@@ -336,8 +336,7 @@ TEST_F(CCParamTraitsTest, AllQuads) {
                          arbitrary_rect1_inside_rect3,
                          arbitrary_rect2_inside_rect3, arbitrary_bool1,
                          arbitrary_color, arbitrary_int);
-  pass_cmp->CopyFromAndAppendDrawQuad(debugborder_in,
-                                      debugborder_in->shared_quad_state);
+  pass_cmp->CopyFromAndAppendDrawQuad(debugborder_in);
 
   SharedQuadState* shared_state2_in = pass_in->CreateAndAppendSharedQuadState();
   shared_state2_in->SetAll(arbitrary_matrix2, arbitrary_rect2, arbitrary_rect2,
@@ -354,9 +353,8 @@ TEST_F(CCParamTraitsTest, AllQuads) {
       arbitrary_rect1_inside_rect1, arbitrary_bool1, child_id,
       arbitrary_resourceid2, arbitrary_rectf1, arbitrary_size1,
       arbitrary_vector2df2, arbitrary_pointf2, arbitrary_rectf1);
-  pass_cmp->CopyFromAndAppendRenderPassDrawQuad(
-      renderpass_in, renderpass_in->shared_quad_state,
-      renderpass_in->render_pass_id);
+  pass_cmp->CopyFromAndAppendRenderPassDrawQuad(renderpass_in,
+                                                renderpass_in->render_pass_id);
 
   SharedQuadState* shared_state3_in = pass_in->CreateAndAppendSharedQuadState();
   shared_state3_in->SetAll(arbitrary_matrix1, arbitrary_rect3, arbitrary_rect3,
@@ -372,8 +370,7 @@ TEST_F(CCParamTraitsTest, AllQuads) {
                         arbitrary_rect1_inside_rect3,
                         arbitrary_rect2_inside_rect3, arbitrary_bool1,
                         arbitrary_color, arbitrary_bool2);
-  pass_cmp->CopyFromAndAppendDrawQuad(solidcolor_in,
-                                      solidcolor_in->shared_quad_state);
+  pass_cmp->CopyFromAndAppendDrawQuad(solidcolor_in);
 
   StreamVideoDrawQuad* streamvideo_in =
       pass_in->CreateAndAppendDrawQuad<StreamVideoDrawQuad>();
@@ -381,8 +378,7 @@ TEST_F(CCParamTraitsTest, AllQuads) {
       shared_state3_in, arbitrary_rect2, arbitrary_rect2_inside_rect2,
       arbitrary_rect1_inside_rect2, arbitrary_bool1, arbitrary_resourceid2,
       arbitrary_size1, arbitrary_matrix1);
-  pass_cmp->CopyFromAndAppendDrawQuad(streamvideo_in,
-                                      streamvideo_in->shared_quad_state);
+  pass_cmp->CopyFromAndAppendDrawQuad(streamvideo_in);
 
   viz::SurfaceId arbitrary_surface_id(
       kArbitraryFrameSinkId,
@@ -393,8 +389,7 @@ TEST_F(CCParamTraitsTest, AllQuads) {
                      arbitrary_rect2_inside_rect2, arbitrary_rect1_inside_rect2,
                      arbitrary_bool1, arbitrary_surface_id,
                      cc::SurfaceDrawQuadType::PRIMARY, nullptr);
-  pass_cmp->CopyFromAndAppendDrawQuad(surface_in,
-                                      surface_in->shared_quad_state);
+  pass_cmp->CopyFromAndAppendDrawQuad(surface_in);
 
   TextureDrawQuad* texture_in =
       pass_in->CreateAndAppendDrawQuad<TextureDrawQuad>();
@@ -404,15 +399,14 @@ TEST_F(CCParamTraitsTest, AllQuads) {
                      arbitrary_bool2, arbitrary_pointf1, arbitrary_pointf2,
                      arbitrary_color, arbitrary_float_array, arbitrary_bool4,
                      arbitrary_bool5, arbitrary_bool6);
-  pass_cmp->CopyFromAndAppendDrawQuad(texture_in,
-                                      texture_in->shared_quad_state);
+  pass_cmp->CopyFromAndAppendDrawQuad(texture_in);
 
   TileDrawQuad* tile_in = pass_in->CreateAndAppendDrawQuad<TileDrawQuad>();
   tile_in->SetAll(shared_state3_in, arbitrary_rect2,
                   arbitrary_rect2_inside_rect2, arbitrary_rect1_inside_rect2,
                   arbitrary_bool1, arbitrary_resourceid3, arbitrary_rectf1,
                   arbitrary_size1, arbitrary_bool2, arbitrary_bool3);
-  pass_cmp->CopyFromAndAppendDrawQuad(tile_in, tile_in->shared_quad_state);
+  pass_cmp->CopyFromAndAppendDrawQuad(tile_in);
 
   YUVVideoDrawQuad* yuvvideo_in =
       pass_in->CreateAndAppendDrawQuad<YUVVideoDrawQuad>();
@@ -423,8 +417,7 @@ TEST_F(CCParamTraitsTest, AllQuads) {
       arbitrary_resourceid2, arbitrary_resourceid3, arbitrary_resourceid4,
       arbitrary_video_color_space, arbitrary_color_space, arbitrary_float1,
       arbitrary_float2, arbitrary_int, arbitrary_bool2);
-  pass_cmp->CopyFromAndAppendDrawQuad(yuvvideo_in,
-                                      yuvvideo_in->shared_quad_state);
+  pass_cmp->CopyFromAndAppendDrawQuad(yuvvideo_in);
 
   // Make sure the in and cmp RenderPasses match.
   Compare(child_pass_cmp.get(), child_pass_in.get());

cc/quads/draw_quad_unittest.cc

@@ -73,15 +73,27 @@ SharedQuadState* CreateSharedQuadState(RenderPass* render_pass) {
   return state;
 }
 
-void CompareDrawQuad(DrawQuad* quad,
-                     DrawQuad* copy,
-                     SharedQuadState* copy_shared_state) {
+void CompareSharedQuadState(const SharedQuadState* source_sqs,
+                            const SharedQuadState* copy_sqs) {
+  EXPECT_EQ(source_sqs->quad_to_target_transform,
+            copy_sqs->quad_to_target_transform);
+  EXPECT_EQ(source_sqs->quad_layer_rect, copy_sqs->quad_layer_rect);
+  EXPECT_EQ(source_sqs->visible_quad_layer_rect,
+            copy_sqs->visible_quad_layer_rect);
+  EXPECT_EQ(source_sqs->clip_rect, copy_sqs->clip_rect);
+  EXPECT_EQ(source_sqs->is_clipped, copy_sqs->is_clipped);
+  EXPECT_EQ(source_sqs->opacity, copy_sqs->opacity);
+  EXPECT_EQ(source_sqs->blend_mode, copy_sqs->blend_mode);
+  EXPECT_EQ(source_sqs->sorting_context_id, copy_sqs->sorting_context_id);
+}
+
+void CompareDrawQuad(DrawQuad* quad, DrawQuad* copy) {
   EXPECT_EQ(quad->material, copy->material);
   EXPECT_EQ(quad->rect, copy->rect);
   EXPECT_EQ(quad->visible_rect, copy->visible_rect);
   EXPECT_EQ(quad->opaque_rect, copy->opaque_rect);
   EXPECT_EQ(quad->needs_blending, copy->needs_blending);
-  EXPECT_EQ(copy_shared_state, copy->shared_quad_state);
+  CompareSharedQuadState(quad->shared_quad_state, copy->shared_quad_state);
 }
 
 #define CREATE_SHARED_STATE()                                              \
@@ -99,30 +111,28 @@ void CompareDrawQuad(DrawQuad* quad,
   bool needs_blending = true;                  \
   ALLOW_UNUSED_LOCAL(needs_blending);
 
-#define SETUP_AND_COPY_QUAD_NEW(Type, quad)                                \
-  DrawQuad* copy_new =                                                     \
-      render_pass->CopyFromAndAppendDrawQuad(quad_new, copy_shared_state); \
-  CompareDrawQuad(quad_new, copy_new, copy_shared_state);                  \
-  const Type* copy_quad = Type::MaterialCast(copy_new);                    \
+#define SETUP_AND_COPY_QUAD_NEW(Type, quad)                              \
+  DrawQuad* copy_new = render_pass->CopyFromAndAppendDrawQuad(quad_new); \
+  CompareDrawQuad(quad_new, copy_new);                                   \
+  const Type* copy_quad = Type::MaterialCast(copy_new);                  \
   ALLOW_UNUSED_LOCAL(copy_quad);
 
-#define SETUP_AND_COPY_QUAD_ALL(Type, quad)                                \
-  DrawQuad* copy_all =                                                     \
-      render_pass->CopyFromAndAppendDrawQuad(quad_all, copy_shared_state); \
-  CompareDrawQuad(quad_all, copy_all, copy_shared_state);                  \
+#define SETUP_AND_COPY_QUAD_ALL(Type, quad)                              \
+  DrawQuad* copy_all = render_pass->CopyFromAndAppendDrawQuad(quad_all); \
+  CompareDrawQuad(quad_all, copy_all);                                   \
   copy_quad = Type::MaterialCast(copy_all);
 
-#define SETUP_AND_COPY_QUAD_NEW_RP(Type, quad, a)                        \
-  DrawQuad* copy_new = render_pass->CopyFromAndAppendRenderPassDrawQuad( \
-      quad_new, copy_shared_state, a);                                   \
-  CompareDrawQuad(quad_new, copy_new, copy_shared_state);                \
-  const Type* copy_quad = Type::MaterialCast(copy_new);                  \
+#define SETUP_AND_COPY_QUAD_NEW_RP(Type, quad, a)                    \
+  DrawQuad* copy_new =                                               \
+      render_pass->CopyFromAndAppendRenderPassDrawQuad(quad_new, a); \
+  CompareDrawQuad(quad_new, copy_new);                               \
+  const Type* copy_quad = Type::MaterialCast(copy_new);              \
   ALLOW_UNUSED_LOCAL(copy_quad);
 
-#define SETUP_AND_COPY_QUAD_ALL_RP(Type, quad, a)                        \
-  DrawQuad* copy_all = render_pass->CopyFromAndAppendRenderPassDrawQuad( \
-      quad_all, copy_shared_state, a);                                   \
-  CompareDrawQuad(quad_all, copy_all, copy_shared_state);                \
+#define SETUP_AND_COPY_QUAD_ALL_RP(Type, quad, a)                    \
+  DrawQuad* copy_all =                                               \
+      render_pass->CopyFromAndAppendRenderPassDrawQuad(quad_all, a); \
+  CompareDrawQuad(quad_all, copy_all);                               \
   copy_quad = Type::MaterialCast(copy_all);
 
 #define CREATE_QUAD_ALL(Type, ...)                                        \

cc/quads/render_pass.cc

@@ -132,31 +132,32 @@ std::unique_ptr<RenderPass> RenderPass::DeepCopy() const {
                     filters, background_filters, color_space,
                     has_transparent_background, cache_render_pass,
                     has_damage_from_contributing_content);
-  for (auto* shared_quad_state : shared_quad_state_list) {
-    SharedQuadState* copy_shared_quad_state =
-        copy_pass->CreateAndAppendSharedQuadState();
-    *copy_shared_quad_state = *shared_quad_state;
+
+  if (shared_quad_state_list.empty()) {
+    DCHECK(quad_list.empty());
+    return copy_pass;
   }
+
   SharedQuadStateList::ConstIterator sqs_iter = shared_quad_state_list.begin();
-  SharedQuadStateList::Iterator copy_sqs_iter =
-      copy_pass->shared_quad_state_list.begin();
+  SharedQuadState* copy_shared_quad_state =
+      copy_pass->CreateAndAppendSharedQuadState();
+  *copy_shared_quad_state = **sqs_iter;
   for (auto* quad : quad_list) {
     while (quad->shared_quad_state != *sqs_iter) {
       ++sqs_iter;
-      ++copy_sqs_iter;
       DCHECK(sqs_iter != shared_quad_state_list.end());
+      copy_shared_quad_state = copy_pass->CreateAndAppendSharedQuadState();
+      *copy_shared_quad_state = **sqs_iter;
     }
     DCHECK(quad->shared_quad_state == *sqs_iter);
 
-    SharedQuadState* copy_shared_quad_state = *copy_sqs_iter;
-
     if (quad->material == DrawQuad::RENDER_PASS) {
       const RenderPassDrawQuad* pass_quad =
           RenderPassDrawQuad::MaterialCast(quad);
-      copy_pass->CopyFromAndAppendRenderPassDrawQuad(
-          pass_quad, copy_shared_quad_state, pass_quad->render_pass_id);
+      copy_pass->CopyFromAndAppendRenderPassDrawQuad(pass_quad,
+                                                     pass_quad->render_pass_id);
     } else {
-      copy_pass->CopyFromAndAppendDrawQuad(quad, copy_shared_quad_state);
+      copy_pass->CopyFromAndAppendDrawQuad(quad);
     }
   }
   return copy_pass;
@@ -261,18 +262,17 @@ SharedQuadState* RenderPass::CreateAndAppendSharedQuadState() {
 
 RenderPassDrawQuad* RenderPass::CopyFromAndAppendRenderPassDrawQuad(
     const RenderPassDrawQuad* quad,
-    const SharedQuadState* shared_quad_state,
     RenderPassId render_pass_id) {
+  DCHECK(!shared_quad_state_list.empty());
   RenderPassDrawQuad* copy_quad =
       CopyFromAndAppendTypedDrawQuad<RenderPassDrawQuad>(quad);
-  copy_quad->shared_quad_state = shared_quad_state;
+  copy_quad->shared_quad_state = shared_quad_state_list.back();
   copy_quad->render_pass_id = render_pass_id;
   return copy_quad;
 }
 
-DrawQuad* RenderPass::CopyFromAndAppendDrawQuad(
-    const DrawQuad* quad,
-    const SharedQuadState* shared_quad_state) {
+DrawQuad* RenderPass::CopyFromAndAppendDrawQuad(const DrawQuad* quad) {
+  DCHECK(!shared_quad_state_list.empty());
   switch (quad->material) {
     case DrawQuad::DEBUG_BORDER:
       CopyFromAndAppendTypedDrawQuad<DebugBorderDrawQuad>(quad);
@@ -304,7 +304,7 @@ DrawQuad* RenderPass::CopyFromAndAppendDrawQuad(
       LOG(FATAL) << "Invalid DrawQuad material " << quad->material;
       break;
   }
-  quad_list.back()->shared_quad_state = shared_quad_state;
+  quad_list.back()->shared_quad_state = shared_quad_state_list.back();
   return quad_list.back();
 }
 

cc/quads/render_pass.h

@@ -109,10 +109,8 @@ class CC_EXPORT RenderPass {
 
   RenderPassDrawQuad* CopyFromAndAppendRenderPassDrawQuad(
       const RenderPassDrawQuad* quad,
-      const SharedQuadState* shared_quad_state,
       RenderPassId render_pass_id);
-  DrawQuad* CopyFromAndAppendDrawQuad(const DrawQuad* quad,
-                                      const SharedQuadState* shared_quad_state);
+  DrawQuad* CopyFromAndAppendDrawQuad(const DrawQuad* quad);
 
   // Uniquely identifies the render pass in the compositor's current frame.
   RenderPassId id = 0;

components/viz/service/display/surface_aggregator.cc

@@ -431,7 +431,6 @@ void SurfaceAggregator::CopyQuadsToPass(
     cc::RenderPass* dest_pass,
     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 or cached render passes, then
   // aggregate the entire thing, as otherwise parts of the copy requests may be
   // ignored and we could cache partially drawn render pass.
@@ -475,7 +474,7 @@ void SurfaceAggregator::CopyQuadsToPass(
                         &damage_rect_in_quad_space_valid);
     } else {
       if (quad->shared_quad_state != last_copied_source_shared_quad_state) {
-        dest_shared_quad_state = CopySharedQuadState(
+        const cc::SharedQuadState* 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_ &&
@@ -507,23 +506,22 @@ void SurfaceAggregator::CopyQuadsToPass(
           dest_pass->has_damage_from_contributing_content = true;
 
         dest_quad = dest_pass->CopyFromAndAppendRenderPassDrawQuad(
-            pass_quad, dest_shared_quad_state, remapped_pass_id);
+            pass_quad, remapped_pass_id);
       } else if (quad->material == cc::DrawQuad::TEXTURE_CONTENT) {
         const auto* texture_quad = cc::TextureDrawQuad::MaterialCast(quad);
         if (texture_quad->secure_output_only &&
             (!output_is_secure_ || copy_request_passes_.count(dest_pass->id))) {
           auto* solid_color_quad =
               dest_pass->CreateAndAppendDrawQuad<cc::SolidColorDrawQuad>();
-          solid_color_quad->SetNew(dest_shared_quad_state, quad->rect,
-                                   quad->visible_rect, SK_ColorBLACK, false);
+          solid_color_quad->SetNew(dest_pass->shared_quad_state_list.back(),
+                                   quad->rect, quad->visible_rect,
+                                   SK_ColorBLACK, false);
           dest_quad = solid_color_quad;
         } else {
-          dest_quad = dest_pass->CopyFromAndAppendDrawQuad(
-              quad, dest_shared_quad_state);
+          dest_quad = dest_pass->CopyFromAndAppendDrawQuad(quad);
         }
       } else {
-        dest_quad =
-            dest_pass->CopyFromAndAppendDrawQuad(quad, dest_shared_quad_state);
+        dest_quad = dest_pass->CopyFromAndAppendDrawQuad(quad);
       }
       if (!child_to_parent_map.empty()) {
         for (ResourceId& resource_id : dest_quad->resources) {