viz: Partial swap without readback

viz::BufferQueue currently implements partial swap reading back
from the previously swapped buffer contents that are outside the
current swapbuffer damage.

This introduces a dependency on a GPU API, that we want to get rid
of (crbug.com/958670).
This technique also assumes it is safe to read back from a buffer
that is used from another driver (crbug.com/457511).

This CL removes the readback and solves the same problem exposing
a rectangle that the compositor needs to redraw to make sure contents
damaged during all the swapbuffers before the current one are
recomposited.

Bug: 958670, 457511
Test: BufferQueueTest.PartialSwapCurrentBufferDamage

Change-Id: I2d4a70b0122f665b68e25e043d57de6044092fc2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1685909Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Andres Calderon Jaramillo <andrescj@chromium.org>
Commit-Queue: Daniele Castagna <dcastagna@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676467}

components/viz/service/display/direct_renderer.cc

@@ -789,10 +789,12 @@ void DirectRenderer::UseRenderPass(const RenderPass* render_pass) {
 gfx::Rect DirectRenderer::ComputeScissorRectForRenderPass(
     const RenderPass* render_pass) const {
   const RenderPass* root_render_pass = current_frame()->root_render_pass;
-  const gfx::Rect root_damage_rect = current_frame()->root_damage_rect;
+  gfx::Rect root_damage_rect = current_frame()->root_damage_rect;
 
-  if (render_pass == root_render_pass)
+  if (render_pass == root_render_pass) {
+    root_damage_rect.Union(output_surface_->GetCurrentFramebufferDamage());
     return root_damage_rect;
+  }
 
   // If the root damage rect has been expanded due to overlays, all the other
   // damage rect calculations are incorrect.

components/viz/service/display/output_surface.cc

@@ -16,6 +16,7 @@
 #include "gpu/GLES2/gl2extchromium.h"
 #include "gpu/command_buffer/client/context_support.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
+#include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/swap_result.h"
 
 namespace viz {
@@ -35,6 +36,10 @@ OutputSurface::OutputSurface(
 
 OutputSurface::~OutputSurface() = default;
 
+gfx::Rect OutputSurface::GetCurrentFramebufferDamage() const {
+  return gfx::Rect();
+}
+
 SkiaOutputSurface* OutputSurface::AsSkiaOutputSurface() {
   return nullptr;
 }

components/viz/service/display/output_surface.h

@@ -26,6 +26,7 @@
 
 namespace gfx {
 class ColorSpace;
+class Rect;
 class Size;
 struct SwapResponse;
 }  // namespace gfx
@@ -136,6 +137,12 @@ class VIZ_SERVICE_EXPORT OutputSurface {
   // after returning from this method in order to unblock the next frame.
   virtual void SwapBuffers(OutputSurfaceFrame frame) = 0;
 
+  // Returns a rectangle whose contents may have changed since the current
+  // buffer was last submitted and they need to be redrawn. For partial swap,
+  // the contents outside this rectangle can be considered valid and do not need
+  // to be redrawn.
+  virtual gfx::Rect GetCurrentFramebufferDamage() const;
+
   // Updates the GpuFence associated with this surface. The id of a newly
   // created GpuFence is returned, or if an error occurs, or fences are not
   // supported, the special id of 0 (meaning "no fence") is returned.  In all

components/viz/service/display_embedder/buffer_queue.cc

@@ -12,11 +12,8 @@
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "gpu/command_buffer/client/gpu_memory_buffer_manager.h"
 #include "gpu/command_buffer/common/gpu_memory_buffer_support.h"
-#include "third_party/skia/include/core/SkRect.h"
-#include "third_party/skia/include/core/SkRegion.h"
 #include "ui/display/types/display_snapshot.h"
 #include "ui/gfx/gpu_memory_buffer.h"
-#include "ui/gfx/skia_util.h"
 
 namespace viz {
 
@@ -52,21 +49,6 @@ unsigned BufferQueue::GetCurrentBuffer(unsigned* stencil) {
   return 0u;
 }
 
-void BufferQueue::CopyBufferDamage(unsigned texture,
-                                   unsigned source_texture,
-                                   const gfx::Rect& new_damage,
-                                   const gfx::Rect& old_damage) {
-  SkRegion region(gfx::RectToSkIRect(old_damage));
-  if (!region.op(gfx::RectToSkIRect(new_damage), SkRegion::kDifference_Op))
-    return;
-  for (SkRegion::Iterator it(region); !it.done(); it.next()) {
-    const SkIRect& rect = it.rect();
-    gl_->CopySubTextureCHROMIUM(
-        source_texture, 0, texture_target_, texture, 0, rect.x(), rect.y(),
-        rect.x(), rect.y(), rect.width(), rect.height(), false, false, false);
-  }
-}
-
 void BufferQueue::UpdateBufferDamage(const gfx::Rect& damage) {
   if (displayed_surface_)
     displayed_surface_->damage.Union(damage);
@@ -78,40 +60,15 @@ void BufferQueue::UpdateBufferDamage(const gfx::Rect& damage) {
   }
 }
 
-void BufferQueue::CopyDamageForCurrentSurface(const gfx::Rect& damage) {
-  if (!current_surface_)
-    return;
-
-  if (damage != gfx::Rect(size_)) {
-    // Copy damage from the most recently swapped buffer. In the event that
-    // the buffer was destroyed and failed to recreate, pick from the most
-    // recently available buffer.
-    unsigned texture_id = 0;
-    for (auto& surface : base::Reversed(in_flight_surfaces_)) {
-      if (surface) {
-        texture_id = surface->texture;
-        break;
-      }
-    }
-    if (!texture_id && displayed_surface_)
-      texture_id = displayed_surface_->texture;
-
-    if (texture_id) {
-      CopyBufferDamage(current_surface_->texture, texture_id, damage,
-                       current_surface_->damage);
-    }
-  }
-  current_surface_->damage = gfx::Rect();
+gfx::Rect BufferQueue::CurrentBufferDamage() const {
+  DCHECK(current_surface_);
+  return current_surface_->damage;
 }
 
 void BufferQueue::SwapBuffers(const gfx::Rect& damage) {
-  if (damage.IsEmpty()) {
-    in_flight_surfaces_.push_back(std::move(current_surface_));
-    return;
-  }
-
-  DCHECK(!current_surface_ || current_surface_->damage.IsEmpty());
   UpdateBufferDamage(damage);
+  if (current_surface_)
+    current_surface_->damage = gfx::Rect();
   in_flight_surfaces_.push_back(std::move(current_surface_));
 }
 

components/viz/service/display_embedder/buffer_queue.h

@@ -53,10 +53,16 @@ class VIZ_SERVICE_EXPORT BufferQueue {
   // current buffer and one could not be created.
   unsigned GetCurrentBuffer(unsigned* stencil);
 
+  // Returns a rectangle whose contents may have changed since the current
+  // buffer was last submitted and they need to be redrawn. For partial swap,
+  // only the contents outside this rectangle can be considered valid and do not
+  // need to be redrawn.
+  gfx::Rect CurrentBufferDamage() const;
+
   // Called by the user of this object to indicate that the buffer currently
-  // marked for drawing should be moved to the list of in-flight buffers. If
-  // |damage| is not empty, the damage rectangle for all buffers except the
-  // one currently marked for drawing is unioned with |damage|.
+  // marked for drawing should be moved to the list of in-flight buffers.
+  // |damage| represents the rectangle containing the damaged area since the
+  // last SwapBuffers.
   void SwapBuffers(const gfx::Rect& damage);
 
   // Called by the user of this object to indicate that a previous request to
@@ -78,10 +84,6 @@ class VIZ_SERVICE_EXPORT BufferQueue {
                const gfx::ColorSpace& color_space,
                bool use_stencil);
 
-  // Copies the damage from the most recently swapped available buffer into the
-  // current buffer.
-  void CopyDamageForCurrentSurface(const gfx::Rect& damage);
-
   uint32_t internal_format() const { return internal_format_; }
   gfx::BufferFormat buffer_format() const { return format_; }
   uint32_t texture_target() const { return texture_target_; }
@@ -108,13 +110,6 @@ class VIZ_SERVICE_EXPORT BufferQueue {
 
   void FreeSurfaceResources(AllocatedSurface* surface);
 
-  // Copy everything that is in |copy_rect|, except for what is in
-  // |exclude_rect| from |source_texture| to |texture|.
-  virtual void CopyBufferDamage(unsigned texture,
-                                unsigned source_texture,
-                                const gfx::Rect& new_damage,
-                                const gfx::Rect& old_damage);
-
   void UpdateBufferDamage(const gfx::Rect& damage);
 
   // Return a surface, available to be drawn into.

components/viz/service/display_embedder/buffer_queue_unittest.cc

@@ -104,20 +104,6 @@ const gpu::SurfaceHandle kFakeSurfaceHandle = 1;
 const unsigned int kBufferQueueInternalformat = GL_RGBA;
 const gfx::BufferFormat kBufferQueueFormat = gfx::BufferFormat::RGBA_8888;
 
-class MockBufferQueue : public BufferQueue {
- public:
-  MockBufferQueue(gpu::gles2::GLES2Interface* gl,
-                  gpu::GpuMemoryBufferManager* gpu_memory_buffer_manager,
-                  const gpu::Capabilities& capabilities)
-      : BufferQueue(gl,
-                    kBufferQueueFormat,
-                    gpu_memory_buffer_manager,
-                    kFakeSurfaceHandle,
-                    capabilities) {}
-  MOCK_METHOD4(CopyBufferDamage,
-               void(unsigned, unsigned, const gfx::Rect&, const gfx::Rect&));
-};
-
 class BufferQueueTest : public ::testing::Test {
  public:
   BufferQueueTest() {}
@@ -130,9 +116,10 @@ class BufferQueueTest : public ::testing::Test {
     context_provider_ = TestContextProvider::Create(std::move(context));
     context_provider_->BindToCurrentThread();
     gpu_memory_buffer_manager_.reset(new StubGpuMemoryBufferManager);
-    mock_output_surface_ = new MockBufferQueue(
-        context_provider_->ContextGL(), gpu_memory_buffer_manager_.get(),
-        context_provider_->ContextCapabilities());
+    mock_output_surface_ =
+        new BufferQueue(context_provider_->ContextGL(), kBufferQueueFormat,
+                        gpu_memory_buffer_manager_.get(), kFakeSurfaceHandle,
+                        context_provider_->ContextCapabilities());
     output_surface_.reset(mock_output_surface_);
   }
 
@@ -184,7 +171,6 @@ class BufferQueueTest : public ::testing::Test {
   }
 
   void SwapBuffers(const gfx::Rect& damage) {
-    output_surface_->CopyDamageForCurrentSurface(damage);
     output_surface_->SwapBuffers(damage);
   }
 
@@ -214,7 +200,7 @@ class BufferQueueTest : public ::testing::Test {
   scoped_refptr<TestContextProvider> context_provider_;
   std::unique_ptr<StubGpuMemoryBufferManager> gpu_memory_buffer_manager_;
   std::unique_ptr<BufferQueue> output_surface_;
-  MockBufferQueue* mock_output_surface_;
+  BufferQueue* mock_output_surface_;
 };
 
 namespace {
@@ -300,43 +286,9 @@ TEST(BufferQueueStandaloneTest, BufferCreation) {
   EXPECT_GT(output_surface->GetCurrentBuffer(&stencil), 0u);
 }
 
-TEST(BufferQueueStandaloneTest, CopyBufferDamage) {
-  scoped_refptr<TestContextProvider> context_provider =
-      TestContextProvider::Create();
-  context_provider->BindToCurrentThread();
-  std::unique_ptr<StubGpuMemoryBufferManager> gpu_memory_buffer_manager;
-  std::unique_ptr<BufferQueue> output_surface;
-  gpu_memory_buffer_manager.reset(new StubGpuMemoryBufferManager);
-
-  output_surface.reset(
-      new BufferQueue(context_provider->ContextGL(), kBufferQueueFormat,
-                      gpu_memory_buffer_manager.get(), kFakeSurfaceHandle,
-                      context_provider->ContextCapabilities()));
-  EXPECT_TRUE(
-      output_surface->Reshape(screen_size, 1.0f, gfx::ColorSpace(), false));
-  // Trigger a sub-buffer copy to exercise all paths.
-  unsigned stencil;
-  EXPECT_GT(output_surface->GetCurrentBuffer(&stencil), 0u);
-  output_surface->CopyDamageForCurrentSurface(screen_rect);
-  output_surface->SwapBuffers(screen_rect);
-  output_surface->PageFlipComplete();
-  EXPECT_GT(output_surface->GetCurrentBuffer(&stencil), 0u);
-  output_surface->CopyDamageForCurrentSurface(small_damage);
-  output_surface->SwapBuffers(small_damage);
-}
-
 TEST_F(BufferQueueTest, PartialSwapReuse) {
   EXPECT_TRUE(
       output_surface_->Reshape(screen_size, 1.0f, gfx::ColorSpace(), false));
-  EXPECT_CALL(*mock_output_surface_,
-              CopyBufferDamage(_, _, small_damage, screen_rect))
-      .Times(1);
-  EXPECT_CALL(*mock_output_surface_,
-              CopyBufferDamage(_, _, small_damage, small_damage))
-      .Times(1);
-  EXPECT_CALL(*mock_output_surface_,
-              CopyBufferDamage(_, _, large_damage, small_damage))
-      .Times(1);
   SendFullFrame();
   SendDamagedFrame(small_damage);
   SendDamagedFrame(small_damage);
@@ -348,9 +300,6 @@ TEST_F(BufferQueueTest, PartialSwapReuse) {
 TEST_F(BufferQueueTest, PartialSwapFullFrame) {
   EXPECT_TRUE(
       output_surface_->Reshape(screen_size, 1.0f, gfx::ColorSpace(), false));
-  EXPECT_CALL(*mock_output_surface_,
-              CopyBufferDamage(_, _, small_damage, screen_rect))
-      .Times(1);
   SendFullFrame();
   SendDamagedFrame(small_damage);
   SendFullFrame();
@@ -358,15 +307,37 @@ TEST_F(BufferQueueTest, PartialSwapFullFrame) {
   EXPECT_EQ(next_frame()->damage, screen_rect);
 }
 
+// Make sure that each time we swap buffers, the damage gets propagated to the
+// previously swapped buffers.
+TEST_F(BufferQueueTest, PartialSwapWithTripleBuffering) {
+  unsigned stencil = 0;
+  EXPECT_TRUE(
+      output_surface_->Reshape(screen_size, 1.0f, gfx::ColorSpace(), false));
+  SendFullFrame();
+  SendFullFrame();
+  // Let's triple buffer.
+  EXPECT_GT(output_surface_->GetCurrentBuffer(&stencil), 0u);
+  SwapBuffers(small_damage);
+  EXPECT_GT(output_surface_->GetCurrentBuffer(&stencil), 0u);
+  EXPECT_EQ(3, CountBuffers());
+  // The whole buffer needs to be redrawn since it's a newly allocated buffer
+  EXPECT_EQ(output_surface_->CurrentBufferDamage(), screen_rect);
+
+  SendDamagedFrame(overlapping_damage);
+  // The next buffer should include damage from |overlapping_damage| and
+  // |small_damage|.
+  EXPECT_GT(output_surface_->GetCurrentBuffer(&stencil), 0u);
+  const auto current_buffer_damage = output_surface_->CurrentBufferDamage();
+  EXPECT_TRUE(current_buffer_damage.Contains(overlapping_damage));
+  EXPECT_TRUE(current_buffer_damage.Contains(small_damage));
+
+  // Let's make sure the damage is not trivially the whole screen.
+  EXPECT_NE(current_buffer_damage, screen_rect);
+}
+
 TEST_F(BufferQueueTest, PartialSwapOverlapping) {
   EXPECT_TRUE(
       output_surface_->Reshape(screen_size, 1.0f, gfx::ColorSpace(), false));
-  EXPECT_CALL(*mock_output_surface_,
-              CopyBufferDamage(_, _, small_damage, screen_rect))
-      .Times(1);
-  EXPECT_CALL(*mock_output_surface_,
-              CopyBufferDamage(_, _, overlapping_damage, small_damage))
-      .Times(1);
 
   SendFullFrame();
   SendDamagedFrame(small_damage);
@@ -596,38 +567,20 @@ TEST_F(BufferQueueTest, AllocateFails) {
   SwapBuffers(screen_rect);
   EXPECT_FALSE(current_frame());
 
-  // Try another swap. It should copy the buffer damage from the back
-  // surface.
   gpu_memory_buffer_manager_->set_allocate_succeeds(true);
   EXPECT_GT(output_surface_->GetCurrentBuffer(&stencil), 0u);
-  unsigned int source_texture = in_flight_surfaces().front()->texture;
-  unsigned int target_texture = current_frame()->texture;
-  testing::Mock::VerifyAndClearExpectations(mock_output_surface_);
-  EXPECT_CALL(*mock_output_surface_,
-              CopyBufferDamage(target_texture, source_texture, small_damage, _))
-      .Times(1);
   SwapBuffers(small_damage);
-  testing::Mock::VerifyAndClearExpectations(mock_output_surface_);
 
-  // Destroy the just-created buffer, and try another swap. The copy should
-  // come from the displayed surface (because both in-flight surfaces are
-  // gone now).
+  // Destroy the just-created buffer, and try another swap.
   output_surface_->PageFlipComplete();
   in_flight_surfaces().back().reset();
   EXPECT_EQ(2u, in_flight_surfaces().size());
   for (auto& surface : in_flight_surfaces())
     EXPECT_FALSE(surface);
   EXPECT_GT(output_surface_->GetCurrentBuffer(&stencil), 0u);
-  source_texture = displayed_frame()->texture;
   EXPECT_TRUE(current_frame());
   EXPECT_TRUE(displayed_frame());
-  target_texture = current_frame()->texture;
-  testing::Mock::VerifyAndClearExpectations(mock_output_surface_);
-  EXPECT_CALL(*mock_output_surface_,
-              CopyBufferDamage(target_texture, source_texture, small_damage, _))
-      .Times(1);
   SwapBuffers(small_damage);
-  testing::Mock::VerifyAndClearExpectations(mock_output_surface_);
 }
 
 }  // namespace

components/viz/service/display_embedder/gl_output_surface_buffer_queue.cc

@@ -100,11 +100,6 @@ void GLOutputSurfaceBufferQueue::Reshape(const gfx::Size& size,
   }
 }
 
-void GLOutputSurfaceBufferQueue::SetDrawRectangle(const gfx::Rect& damage) {
-  GLOutputSurface::SetDrawRectangle(damage);
-  buffer_queue_->CopyDamageForCurrentSurface(damage);
-}
-
 void GLOutputSurfaceBufferQueue::SwapBuffers(OutputSurfaceFrame frame) {
   DCHECK(buffer_queue_);
 

components/viz/service/display_embedder/gl_output_surface_buffer_queue.h

@@ -61,7 +61,6 @@ class GLOutputSurfaceBufferQueue : public GLOutputSurface {
   bool IsDisplayedAsOverlayPlane() const override;
   unsigned GetOverlayTextureId() const override;
   gfx::BufferFormat GetOverlayBufferFormat() const override;
-  void SetDrawRectangle(const gfx::Rect& damage) override;
 
   // GLOutputSurface:
   void DidReceiveSwapBuffersAck(const gfx::SwapResponse& response) override;

content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.cc

@@ -121,6 +121,12 @@ void GpuSurfacelessBrowserCompositorOutputSurface::BindFramebuffer() {
   }
 }
 
+gfx::Rect
+GpuSurfacelessBrowserCompositorOutputSurface::GetCurrentFramebufferDamage()
+    const {
+  return buffer_queue_->CurrentBufferDamage();
+}
+
 GLenum GpuSurfacelessBrowserCompositorOutputSurface::
     GetFramebufferCopyTextureFormat() {
   return buffer_queue_->internal_format();
@@ -180,12 +186,6 @@ unsigned GpuSurfacelessBrowserCompositorOutputSurface::UpdateGpuFence() {
   return gpu_fence_id_;
 }
 
-void GpuSurfacelessBrowserCompositorOutputSurface::SetDrawRectangle(
-    const gfx::Rect& damage) {
-  GpuBrowserCompositorOutputSurface::SetDrawRectangle(damage);
-  buffer_queue_->CopyDamageForCurrentSurface(damage);
-}
-
 gpu::SurfaceHandle
 GpuSurfacelessBrowserCompositorOutputSurface::GetSurfaceHandle() const {
   return surface_handle_;

content/browser/compositor/gpu_surfaceless_browser_compositor_output_surface.h

@@ -36,6 +36,7 @@ class GpuSurfacelessBrowserCompositorOutputSurface
   // viz::OutputSurface implementation.
   void SwapBuffers(viz::OutputSurfaceFrame frame) override;
   void BindFramebuffer() override;
+  gfx::Rect GetCurrentFramebufferDamage() const override;
   uint32_t GetFramebufferCopyTextureFormat() override;
   void Reshape(const gfx::Size& size,
                float device_scale_factor,
@@ -46,7 +47,6 @@ class GpuSurfacelessBrowserCompositorOutputSurface
   unsigned GetOverlayTextureId() const override;
   gfx::BufferFormat GetOverlayBufferFormat() const override;
   unsigned UpdateGpuFence() override;
-  void SetDrawRectangle(const gfx::Rect& damage) override;
   gpu::SurfaceHandle GetSurfaceHandle() const override;
 
   // BrowserCompositorOutputSurface implementation.