Reset current_buffer_modified_ if swap skipped

This is a speculative fix for a DCHECK failure. If SkiaRenderer skips
swap then |current_buffer_modified_| isn't reset to false and looks like
it can violate a DCHECK condition. Add code to SwapBuffersSkipped() that
handles updating framebuffer damage rects and resetting
|current_buffer_modified_|.

Bug: 1161653
Change-Id: Ie811fca55cbf5c57ed5e4af50772e2e538f7b911
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2640895Reviewed-by: Peng Huang <penghuang@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845848}

components/viz/service/display/skia_output_surface.h

@@ -99,7 +99,7 @@ class VIZ_SERVICE_EXPORT SkiaOutputSurface : public OutputSurface,
       SkYUVAInfo::Subsampling subsampling) = 0;
 
   // Called if SwapBuffers() will be skipped.
-  virtual void SwapBuffersSkipped() = 0;
+  virtual void SwapBuffersSkipped(const gfx::Rect root_pass_damage_rect) = 0;
 
   // TODO(weiliangc): This API should move to OverlayProcessor.
   // Schedule |output_surface_plane| as an overlay plane to be displayed.

components/viz/service/display/skia_renderer.cc

@@ -782,7 +782,13 @@ void SkiaRenderer::SwapBuffers(SwapFrameData swap_frame_data) {
 }
 
 void SkiaRenderer::SwapBuffersSkipped() {
-  skia_output_surface_->SwapBuffersSkipped();
+  gfx::Rect root_pass_damage_rect = gfx::Rect(surface_size_for_swap_buffers());
+  if (use_partial_swap_)
+    root_pass_damage_rect.Intersect(swap_buffer_rect_);
+
+  skia_output_surface_->SwapBuffersSkipped(root_pass_damage_rect);
+  swap_buffer_rect_ = gfx::Rect();
+
   FlushOutputSurface();
 }
 

components/viz/service/display_embedder/skia_output_surface_impl.cc

@@ -226,7 +226,7 @@ void SkiaOutputSurfaceImpl::Reshape(const gfx::Size& size,
   } else {
     // Reshape will damage all buffers.
     current_buffer_ = 0u;
-    for (auto& damage : damage_of_buffers_)
+    for (auto& damage : accumulated_buffer_damage_)
       damage = gfx::Rect(size);
   }
 
@@ -426,19 +426,19 @@ void SkiaOutputSurfaceImpl::SwapBuffers(OutputSurfaceFrame frame) {
   // If current_buffer_modified_ is false, it means SkiaRenderer doesn't draw
   // anything for current frame. So this SwapBuffer() must be a empty swap, so
   // the previous buffer will be used for this frame.
-  if (!damage_of_buffers_.empty() && current_buffer_modified_) {
+  if (!accumulated_buffer_damage_.empty() && current_buffer_modified_) {
     gfx::Rect damage_rect =
         frame.sub_buffer_rect ? *frame.sub_buffer_rect : gfx::Rect(size_);
     // Calculate damage area for every buffer.
-    for (size_t i = 0u; i < damage_of_buffers_.size(); ++i) {
+    for (size_t i = 0u; i < accumulated_buffer_damage_.size(); ++i) {
       if (i == current_buffer_) {
-        damage_of_buffers_[i] = gfx::Rect();
+        accumulated_buffer_damage_[i] = gfx::Rect();
       } else {
-        damage_of_buffers_[i].Union(damage_rect);
+        accumulated_buffer_damage_[i].Union(damage_rect);
       }
     }
     // change the current buffer index to the next buffer in the queue.
-    if (++current_buffer_ == damage_of_buffers_.size())
+    if (++current_buffer_ == accumulated_buffer_damage_.size())
       current_buffer_ = 0u;
   }
   current_buffer_modified_ = false;
@@ -464,7 +464,19 @@ void SkiaOutputSurfaceImpl::SwapBuffers(OutputSurfaceFrame frame) {
   RecreateRootRecorder();
 }
 
-void SkiaOutputSurfaceImpl::SwapBuffersSkipped() {
+void SkiaOutputSurfaceImpl::SwapBuffersSkipped(
+    const gfx::Rect root_pass_damage_rect) {
+  if (current_buffer_modified_ && !accumulated_buffer_damage_.empty()) {
+    // If |current_buffer_modified_| is true but we skipped swap there is still
+    // damage to the current framebuffer to account for. Unlike SwapBuffers()
+    // don't reset current buffers rect, since that damage still need to be
+    // taken into account when the buffer is swapped later.
+    for (auto& frame_buffer_damage_rect : accumulated_buffer_damage_) {
+      frame_buffer_damage_rect.Union(root_pass_damage_rect);
+    }
+  }
+  current_buffer_modified_ = false;
+
   // PostTask to the GPU thread to deal with freeing resources and running
   // callbacks.
   auto task = base::BindOnce(&SkiaOutputSurfaceImplOnGpu::SwapBuffersSkipped,
@@ -773,7 +785,7 @@ bool SkiaOutputSurfaceImpl::Initialize() {
       use_damage_area_from_skia_output_device_ = true;
       damage_of_current_buffer_ = gfx::Rect();
     } else {
-      damage_of_buffers_.resize(capabilities_.number_of_buffers);
+      accumulated_buffer_damage_.resize(capabilities_.number_of_buffers);
     }
   }
   return result;
@@ -898,11 +910,11 @@ void SkiaOutputSurfaceImpl::DidSwapBuffersComplete(
   DCHECK(client_);
   last_swapped_mailbox_ = params.primary_plane_mailbox;
 
-  // Reset |damage_of_buffers_|, if buffers are new created.
+  // Reset |accumulated_buffer_damage_|, if buffers are new created.
   if (params.swap_response.result ==
       gfx::SwapResult::SWAP_NAK_RECREATE_BUFFERS) {
     client_->SetNeedsRedrawRect(gfx::Rect(size_));
-    for (auto& damage : damage_of_buffers_)
+    for (auto& damage : accumulated_buffer_damage_)
       damage = gfx::Rect(size_);
   }
 
@@ -1133,11 +1145,11 @@ gfx::Rect SkiaOutputSurfaceImpl::GetCurrentFramebufferDamage() const {
     return *damage_of_current_buffer_;
   }
 
-  if (damage_of_buffers_.empty())
+  if (accumulated_buffer_damage_.empty())
     return gfx::Rect();
 
-  DCHECK_LT(current_buffer_, damage_of_buffers_.size());
-  return damage_of_buffers_[current_buffer_];
+  DCHECK_LT(current_buffer_, accumulated_buffer_damage_.size());
+  return accumulated_buffer_damage_[current_buffer_];
 }
 
 void SkiaOutputSurfaceImpl::SetNeedsMeasureNextDrawLatency() {

components/viz/service/display_embedder/skia_output_surface_impl.h

@@ -97,7 +97,7 @@ class VIZ_SERVICE_EXPORT SkiaOutputSurfaceImpl : public SkiaOutputSurface {
       sk_sp<SkColorSpace> image_color_space,
       SkYUVAInfo::PlaneConfig plane_config,
       SkYUVAInfo::Subsampling subsampling) override;
-  void SwapBuffersSkipped() override;
+  void SwapBuffersSkipped(const gfx::Rect root_pass_damage_rect) override;
   void ScheduleOutputSurfaceAsOverlay(
       OverlayProcessorInterface::OutputSurfaceOverlayPlane output_surface_plane)
       override;
@@ -305,8 +305,9 @@ class VIZ_SERVICE_EXPORT SkiaOutputSurfaceImpl : public SkiaOutputSurface {
   base::Optional<gfx::Rect> damage_of_current_buffer_;
   // Current buffer index.
   size_t current_buffer_ = 0;
-  // Damage area of the buffer. Differ to the last submit buffer.
-  std::vector<gfx::Rect> damage_of_buffers_;
+  // Accumulates framebuffer damage since last drawing to a particular buffer.
+  // There is one gfx::Rect per framebuffer.
+  std::vector<gfx::Rect> accumulated_buffer_damage_;
   // Track if the current buffer content is changed.
   bool current_buffer_modified_ = false;
 

components/viz/service/display_embedder/skia_output_surface_impl_unittest.cc

@@ -163,7 +163,7 @@ TEST_F(SkiaOutputSurfaceImplTest, EndPaint) {
 
   output_surface_->CopyOutput(AggregatedRenderPassId{0}, geometry, color_space,
                               std::move(request));
-  output_surface_->SwapBuffersSkipped();
+  output_surface_->SwapBuffersSkipped(kSurfaceRect);
   output_surface_->Flush();
   BlockMainThread();
 
@@ -230,7 +230,7 @@ TEST_F(SkiaOutputSurfaceImplTest, CopyOutputBitmapSupportedColorSpace) {
   PaintRootRenderPass(kSurfaceRect, base::DoNothing::Once());
   output_surface_->CopyOutput(AggregatedRenderPassId{0}, geometry, color_space,
                               std::move(request));
-  output_surface_->SwapBuffersSkipped();
+  output_surface_->SwapBuffersSkipped(kSurfaceRect);
   output_surface_->Flush();
   run_loop.Run();
 
@@ -269,7 +269,7 @@ TEST_F(SkiaOutputSurfaceImplTest, CopyOutputBitmapUnsupportedColorSpace) {
   PaintRootRenderPass(kSurfaceRect, base::DoNothing::Once());
   output_surface_->CopyOutput(AggregatedRenderPassId{0}, geometry, color_space,
                               std::move(request));
-  output_surface_->SwapBuffersSkipped();
+  output_surface_->SwapBuffersSkipped(kSurfaceRect);
   output_surface_->Flush();
   run_loop.Run();
 

components/viz/test/fake_skia_output_surface.h

@@ -74,7 +74,7 @@ class FakeSkiaOutputSurface : public SkiaOutputSurface {
       sk_sp<SkColorSpace> image_color_space,
       SkYUVAInfo::PlaneConfig plane_config,
       SkYUVAInfo::Subsampling subsampling) override;
-  void SwapBuffersSkipped() override {}
+  void SwapBuffersSkipped(const gfx::Rect root_pass_damage_rect) override {}
   SkCanvas* BeginPaintRenderPass(const AggregatedRenderPassId& id,
                                  const gfx::Size& surface_size,
                                  ResourceFormat format,