Expand damage rect to include intersecting backdrop-filters

Prior to this CL, the root damage rect might have partially
intersected child render passes that had backdrop filters applied.
In that case, the backdrop filter code would attempt to read back
the backdrop image, and would find already-filtered content outside
of the damage rect, but inside the render pass quad bounds. It would
then filter this image, and if the backdrop filters moved pixels,
some of that double-filtered content would bleed back into the damage
rect and be used. This would lead to visual artifacts.

With this CL, any child render passes that contain pixel-moving
backdrop filters are checked for intersection with the root damage
rect, and if they intersect, the damage rect is expanded to include
the entire child render pass output rect.

Bug: 986206
Change-Id: I81a51e24548cae736172938ce190f6ba4c6d2b94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1847675
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Eric Karl <ericrk@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706531}

components/viz/service/display/direct_renderer.cc

@@ -22,6 +22,7 @@
 #include "components/viz/common/frame_sinks/copy_output_request.h"
 #include "components/viz/common/frame_sinks/copy_output_util.h"
 #include "components/viz/common/quads/draw_quad.h"
+#include "components/viz/common/quads/render_pass_draw_quad.h"
 #include "components/viz/service/display/bsp_tree.h"
 #include "components/viz/service/display/bsp_walk_action.h"
 #include "components/viz/service/display/output_surface.h"
@@ -308,6 +309,11 @@ void DirectRenderer::DrawFrame(RenderPassList* render_passes_in_draw_order,
       render_pass_backdrop_filters_[pass->id] = &pass->backdrop_filters;
       render_pass_backdrop_filter_bounds_[pass->id] =
           pass->backdrop_filter_bounds;
+      if (pass->backdrop_filters.HasFilterThatMovesPixels()) {
+        backdrop_filter_output_rects_[pass->id] =
+            cc::MathUtil::MapEnclosingClippedRect(
+                pass->transform_to_root_target, pass->output_rect);
+      }
     }
   }
 
@@ -403,6 +409,7 @@ void DirectRenderer::DrawFrame(RenderPassList* render_passes_in_draw_order,
   render_pass_backdrop_filters_.clear();
   render_pass_backdrop_filter_bounds_.clear();
   render_pass_bypass_quads_.clear();
+  backdrop_filter_output_rects_.clear();
 
   current_frame_valid_ = false;
 }
@@ -581,8 +588,9 @@ void DirectRenderer::DrawRenderPass(const RenderPass* render_pass) {
   const gfx::Rect surface_rect_in_draw_space = OutputSurfaceRectInDrawSpace();
   gfx::Rect render_pass_scissor_in_draw_space = surface_rect_in_draw_space;
 
-  if (current_frame()->current_render_pass ==
-      current_frame()->root_render_pass) {
+  bool is_root_render_pass =
+      current_frame()->current_render_pass == current_frame()->root_render_pass;
+  if (is_root_render_pass) {
     render_pass_scissor_in_draw_space.Intersect(
         DeviceViewportRectInDrawSpace());
   }
@@ -592,9 +600,6 @@ void DirectRenderer::DrawRenderPass(const RenderPass* render_pass) {
         ComputeScissorRectForRenderPass(current_frame()->current_render_pass));
   }
 
-  bool is_root_render_pass =
-      current_frame()->current_render_pass == current_frame()->root_render_pass;
-
   bool render_pass_is_clipped =
       !render_pass_scissor_in_draw_space.Contains(surface_rect_in_draw_space);
 
@@ -627,6 +632,9 @@ void DirectRenderer::DrawRenderPass(const RenderPass* render_pass) {
   PrepareSurfaceForPass(
       mode, MoveFromDrawToWindowSpace(render_pass_scissor_in_draw_space));
 
+  if (is_root_render_pass)
+    last_root_render_pass_scissor_rect_ = render_pass_scissor_in_draw_space;
+
   const QuadList& quad_list = render_pass->quad_list;
   base::circular_deque<std::unique_ptr<DrawPolygon>> poly_list;
 
@@ -743,6 +751,22 @@ gfx::Rect DirectRenderer::ComputeScissorRectForRenderPass(
 
   if (render_pass == root_render_pass) {
     root_damage_rect.Union(output_surface_->GetCurrentFramebufferDamage());
+    // If the root damage rect intersects any child render pass that has a
+    // pixel-moving backdrop-filter, expand the damage to include the entire
+    // child pass. See crbug.com/986206 for context.
+    if (!backdrop_filter_output_rects_.empty() && !root_damage_rect.IsEmpty()) {
+      for (auto* quad : render_pass->quad_list) {
+        if (quad->material == DrawQuad::Material::kRenderPass) {
+          auto iter = backdrop_filter_output_rects_.find(
+              RenderPassDrawQuad::MaterialCast(quad)->render_pass_id);
+          if (iter != backdrop_filter_output_rects_.end()) {
+            auto this_output_rect = iter->second;
+            if (root_damage_rect.Intersects(this_output_rect))
+              root_damage_rect.Union(this_output_rect);
+          }
+        }
+      }
+    }
     return root_damage_rect;
   }
 

components/viz/service/display/direct_renderer.h

@@ -116,6 +116,10 @@ class VIZ_SERVICE_EXPORT DirectRenderer {
   }
   bool OverlayNeedsSurfaceOccludingDamageRect() const;
 
+  gfx::Rect GetLastRootScissorRectForTesting() const {
+    return last_root_render_pass_scissor_rect_;
+  }
+
  protected:
   friend class BspWalkActionDrawPolygon;
 
@@ -258,6 +262,7 @@ class VIZ_SERVICE_EXPORT DirectRenderer {
       render_pass_backdrop_filters_;
   base::flat_map<RenderPassId, base::Optional<gfx::RRectF>>
       render_pass_backdrop_filter_bounds_;
+  base::flat_map<RenderPassId, gfx::Rect> backdrop_filter_output_rects_;
 
   bool visible_ = false;
   bool disable_color_checks_for_testing_ = false;
@@ -286,6 +291,7 @@ class VIZ_SERVICE_EXPORT DirectRenderer {
 #if DCHECK_IS_ON()
   bool overdraw_feedback_support_missing_logged_once_ = false;
 #endif
+  gfx::Rect last_root_render_pass_scissor_rect_;
   gfx::Size enlarge_pass_texture_amount_;
 
   // The current drawing frame is valid only during the duration of the

components/viz/service/display/display_unittest.cc

@@ -23,6 +23,7 @@
 #include "components/viz/common/quads/surface_draw_quad.h"
 #include "components/viz/common/surfaces/frame_sink_id.h"
 #include "components/viz/common/surfaces/parent_local_surface_id_allocator.h"
+#include "components/viz/service/display/direct_renderer.h"
 #include "components/viz/service/display/display_client.h"
 #include "components/viz/service/display/display_scheduler.h"
 #include "components/viz/service/display_embedder/server_shared_bitmap_manager.h"
@@ -46,6 +47,7 @@ namespace {
 
 static constexpr FrameSinkId kArbitraryFrameSinkId(3, 3);
 static constexpr FrameSinkId kAnotherFrameSinkId(4, 4);
+static constexpr FrameSinkId kAnotherFrameSinkId2(5, 5);
 
 class TestSoftwareOutputDevice : public SoftwareOutputDevice {
  public:
@@ -668,6 +670,157 @@ TEST_F(DisplayTest, DisableSwapUntilResize) {
   TearDownDisplay();
 }
 
+TEST_F(DisplayTest, BackdropFilterTest) {
+  RendererSettings settings;
+  settings.partial_swap_enabled = true;
+  id_allocator_.GenerateId();
+  const LocalSurfaceId local_surface_id(
+      id_allocator_.GetCurrentLocalSurfaceIdAllocation().local_surface_id());
+
+  // Set up first display.
+  SetUpSoftwareDisplay(settings);
+  StubDisplayClient client;
+  display_->Initialize(&client, manager_.surface_manager());
+  display_->SetLocalSurfaceId(local_surface_id, 1.f);
+
+  // Create frame sink for a sub surface.
+  const LocalSurfaceId sub_local_surface_id1(6,
+                                             base::UnguessableToken::Create());
+  const SurfaceId sub_surface_id1(kAnotherFrameSinkId, sub_local_surface_id1);
+  auto sub_support1 = std::make_unique<CompositorFrameSinkSupport>(
+      nullptr, &manager_, kAnotherFrameSinkId, /*is_root=*/false,
+      /*needs_sync_points=*/true);
+
+  // Create frame sink for another sub surface.
+  const LocalSurfaceId sub_local_surface_id2(7,
+                                             base::UnguessableToken::Create());
+  const SurfaceId sub_surface_id2(kAnotherFrameSinkId2, sub_local_surface_id2);
+  auto sub_support2 = std::make_unique<CompositorFrameSinkSupport>(
+      nullptr, &manager_, kAnotherFrameSinkId2, /*is_root=*/false,
+      /*needs_sync_points=*/true);
+
+  // Main surface M, damage D, sub-surface B with backdrop filter.
+  //   +-----------+
+  //   | +----+   M|
+  //   | |B +-|-+  |
+  //   | +--|-+ |  |
+  //   |    |  D|  |
+  //   |    +---+  |
+  //   +-----------+
+  const gfx::Size display_size(100, 100);
+  const gfx::Rect damage_rect(20, 20, 40, 40);
+  display_->Resize(display_size);
+  const gfx::Rect sub_surface_rect(5, 5, 25, 25);
+  const gfx::Rect no_damage;
+
+  uint64_t next_render_pass_id = 1;
+  for (size_t frame_num = 1; frame_num <= 2; ++frame_num) {
+    bool first_frame = frame_num == 1;
+    scheduler_->ResetDamageForTest();
+    {
+      // Sub-surface with backdrop-filter.
+      RenderPassList pass_list;
+      auto bd_pass = RenderPass::Create();
+      cc::FilterOperations backdrop_filters;
+      backdrop_filters.Append(cc::FilterOperation::CreateBlurFilter(5.0));
+      bd_pass->SetAll(
+          next_render_pass_id++, sub_surface_rect, no_damage, gfx::Transform(),
+          cc::FilterOperations(), backdrop_filters,
+          gfx::RRectF(gfx::RectF(sub_surface_rect), 0),
+          gfx::ColorSpace::CreateSRGB(), false, false, false, false);
+      pass_list.push_back(std::move(bd_pass));
+
+      CompositorFrame frame = CompositorFrameBuilder()
+                                  .SetRenderPassList(std::move(pass_list))
+                                  .Build();
+      sub_support1->SubmitCompositorFrame(sub_local_surface_id1,
+                                          std::move(frame));
+    }
+
+    {
+      // Sub-surface with damage.
+      RenderPassList pass_list;
+      auto other_pass = RenderPass::Create();
+      other_pass->output_rect = gfx::Rect(display_size);
+      other_pass->damage_rect = damage_rect;
+      other_pass->id = next_render_pass_id++;
+      pass_list.push_back(std::move(other_pass));
+
+      CompositorFrame frame = CompositorFrameBuilder()
+                                  .SetRenderPassList(std::move(pass_list))
+                                  .Build();
+      sub_support2->SubmitCompositorFrame(sub_local_surface_id2,
+                                          std::move(frame));
+    }
+
+    {
+      RenderPassList pass_list;
+      auto pass = RenderPass::Create();
+      pass->output_rect = gfx::Rect(display_size);
+      pass->damage_rect = damage_rect;
+      pass->id = next_render_pass_id++;
+
+      // Embed sub surface 1, with backdrop filter.
+      auto* shared_quad_state1 = pass->CreateAndAppendSharedQuadState();
+      shared_quad_state1->SetAll(
+          gfx::Transform(), /*quad_layer_rect=*/sub_surface_rect,
+          /*visible_quad_layer_rect=*/sub_surface_rect,
+          /*rounded_corner_bounds=*/gfx::RRectF(),
+          /*clip_rect=*/sub_surface_rect, /*is_clipped=*/false,
+          /*are_contents_opaque=*/true, /*opacity=*/1.0f, SkBlendMode::kSrcOver,
+          /*sorting_context_id=*/0);
+      auto* quad1 = pass->quad_list.AllocateAndConstruct<SurfaceDrawQuad>();
+      quad1->SetNew(shared_quad_state1, /*rect=*/sub_surface_rect,
+                    /*visible_rect=*/sub_surface_rect,
+                    SurfaceRange(base::nullopt, sub_surface_id1), SK_ColorBLACK,
+                    /*stretch_content_to_fill_bounds=*/false,
+                    /*has_pointer_events_none=*/false);
+      quad1->allow_merge = false;
+
+      // Embed sub surface 2, with damage.
+      auto* shared_quad_state2 = pass->CreateAndAppendSharedQuadState();
+      gfx::Rect rect1(display_size);
+      shared_quad_state2->SetAll(gfx::Transform(), /*quad_layer_rect=*/rect1,
+                                 /*visible_quad_layer_rect=*/rect1,
+                                 /*rounded_corner_bounds=*/gfx::RRectF(),
+                                 /*clip_rect=*/rect1, /*is_clipped=*/false,
+                                 /*are_contents_opaque=*/true, /*opacity=*/1.0f,
+                                 SkBlendMode::kSrcOver,
+                                 /*sorting_context_id=*/0);
+      auto* quad2 = pass->quad_list.AllocateAndConstruct<SurfaceDrawQuad>();
+      quad2->SetNew(shared_quad_state2, /*rect=*/rect1,
+                    /*visible_rect=*/rect1,
+                    SurfaceRange(base::nullopt, sub_surface_id2), SK_ColorBLACK,
+                    /*stretch_content_to_fill_bounds=*/false,
+                    /*has_pointer_events_none=*/false);
+      quad2->allow_merge = false;
+
+      pass_list.push_back(std::move(pass));
+      SubmitCompositorFrame(&pass_list, local_surface_id);
+
+      scheduler_->swapped = false;
+      display_->DrawAndSwap();
+      EXPECT_TRUE(scheduler_->swapped);
+      EXPECT_EQ(frame_num, output_surface_->num_sent_frames());
+      EXPECT_EQ(display_size, software_output_device_->viewport_pixel_size());
+      // The damage rect produced by surface_aggregator only includes the
+      // damaged surface rect, and is not expanded for the backdrop-filter
+      // surface.
+      auto expected_damage =
+          first_frame ? gfx::Rect(display_size) : gfx::Rect(20, 20, 40, 40);
+      EXPECT_EQ(expected_damage, software_output_device_->damage_rect());
+      // The scissor rect is expanded by direct_renderer to include the
+      // overlapping pixel-moving backdrop filter surface.
+      auto expected_scissor_rect =
+          first_frame ? gfx::Rect(display_size) : gfx::Rect(5, 5, 55, 55);
+      EXPECT_EQ(
+          expected_scissor_rect,
+          display_->renderer_for_testing()->GetLastRootScissorRectForTesting());
+    }
+  }
+  TearDownDisplay();
+}
+
 class CountLossDisplayClient : public StubDisplayClient {
  public:
   CountLossDisplayClient() = default;