vulkan: Add VulkanSwapChain::PostSubBufferAsync()

The bug is because X11 will present hided Window at 1Hz. So if there are
two windows on screen. One is hided, vkAcquireNextImageKHR() will be
blocked for 1 second, so the GPU main thread will be blocked with it.
Fix this problem by add PostSubBufferAsync(). It will call
vkAcquireNextImageKHR() off the GPU main thread.

Bug: 1097014
Change-Id: I2dc7988d5c6320d167b3280b3f92ac9979644887
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2256589
Commit-Queue: Peng Huang <penghuang@chromium.org>
Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#781654}

components/viz/service/display/output_surface.h

@@ -56,6 +56,10 @@ class VIZ_SERVICE_EXPORT OutputSurface {
     Capabilities(const Capabilities& capabilities);
 
     int max_frames_pending = 1;
+    // The number of buffers for the SkiaOutputDevice. If the
+    // |supports_post_sub_buffer| true, SkiaOutputSurfaceImpl will track target
+    // damaged area based on this number.
+    int number_of_buffers = 2;
     // Whether this output surface renders to the default OpenGL zero
     // framebuffer or to an offscreen framebuffer.
     bool uses_default_gl_framebuffer = true;

components/viz/service/display_embedder/skia_output_device_buffer_queue.cc

@@ -4,6 +4,10 @@
 
 #include "components/viz/service/display_embedder/skia_output_device_buffer_queue.h"
 
+#include <memory>
+#include <utility>
+#include <vector>
+
 #include "base/command_line.h"
 #include "components/viz/common/switches.h"
 #include "components/viz/service/display_embedder/skia_output_surface_dependency.h"
@@ -29,7 +33,7 @@ SkiaOutputDeviceBufferQueue::SkiaOutputDeviceBufferQueue(
   capabilities_.uses_default_gl_framebuffer = false;
   capabilities_.preserve_buffer_content = true;
   capabilities_.only_invalidates_damage_rect = false;
-  capabilities_.max_frames_pending = 2;
+  capabilities_.number_of_buffers = 3;
 
   // Force the number of max pending frames to one when the switch
   // "double-buffer-compositing" is passed.
@@ -37,7 +41,8 @@ SkiaOutputDeviceBufferQueue::SkiaOutputDeviceBufferQueue(
   // allocates at most one additional buffer.
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   if (command_line->HasSwitch(switches::kDoubleBufferCompositing))
-    capabilities_.max_frames_pending = 1;
+    capabilities_.number_of_buffers = 2;
+  capabilities_.max_frames_pending = capabilities_.number_of_buffers - 1;
 
   presenter_->InitializeCapabilities(&capabilities_);
 }
@@ -214,7 +219,7 @@ bool SkiaOutputDeviceBufferQueue::Reshape(const gfx::Size& size,
   FreeAllSurfaces();
 
   images_ = presenter_->AllocateImages(color_space_, image_size_,
-                                       capabilities_.max_frames_pending + 1);
+                                       capabilities_.number_of_buffers);
   if (images_.empty())
     return false;
 

components/viz/service/display_embedder/skia_output_device_vulkan.cc

@@ -102,23 +102,25 @@ void SkiaOutputDeviceVulkan::PostSubBuffer(
 #endif
 
   StartSwapBuffers(std::move(feedback));
-  auto image_size = vulkan_surface_->image_size();
-  gfx::SwapResult result = gfx::SwapResult::SWAP_ACK;
-  // If the swapchain is new created, but rect doesn't cover the whole buffer,
-  // we will still present it even it causes a artifact in this frame and
-  // recovered when the next frame is presented. We do that because the old
-  // swapchain's present thread is blocked on waiting a reply from xserver, and
-  // presenting a new image with the new create swapchain will somehow makes
-  // xserver send a reply to us, and then unblock the old swapchain's present
-  // thread. So the old swapchain can be destroyed properly.
-  if (!rect.IsEmpty())
-    result = vulkan_surface_->PostSubBuffer(rect);
-  if (is_new_swapchain_) {
-    is_new_swapchain_ = false;
-    result = gfx::SwapResult::SWAP_NAK_RECREATE_BUFFERS;
+
+  if (!rect.IsEmpty()) {
+    // If the swapchain is new created, but rect doesn't cover the whole buffer,
+    // we will still present it even it causes a artifact in this frame and
+    // recovered when the next frame is presented. We do that because the old
+    // swapchain's present thread is blocked on waiting a reply from xserver,
+    // and presenting a new image with the new create swapchain will somehow
+    // makes xserver send a reply to us, and then unblock the old swapchain's
+    // present thread. So the old swapchain can be destroyed properly.
+    vulkan_surface_->PostSubBufferAsync(
+        rect, base::BindOnce(&SkiaOutputDeviceVulkan::OnPostSubBufferFinished,
+                             weak_ptr_factory_.GetWeakPtr(),
+                             std::move(latency_info), is_new_swapchain_));
+  } else {
+    OnPostSubBufferFinished(std::move(latency_info), is_new_swapchain_,
+                            gfx::SwapResult::SWAP_ACK);
   }
-  FinishSwapBuffers(gfx::SwapCompletionResult(result), image_size,
-                    std::move(latency_info));
+
+  is_new_swapchain_ = false;
 }
 
 SkSurface* SkiaOutputDeviceVulkan::BeginPaint(
@@ -243,7 +245,8 @@ bool SkiaOutputDeviceVulkan::Initialize() {
   vulkan_surface_ = std::move(vulkan_surface);
 
   capabilities_.uses_default_gl_framebuffer = false;
-  capabilities_.max_frames_pending = vulkan_surface_->image_count() - 1;
+  capabilities_.max_frames_pending = 1;
+  capabilities_.number_of_buffers = vulkan_surface_->image_count();
   // Vulkan FIFO swap chain should return vk images in presenting order, so set
   // preserve_buffer_content & supports_post_sub_buffer to true to let
   // SkiaOutputBufferImpl to manager damages.
@@ -288,6 +291,16 @@ bool SkiaOutputDeviceVulkan::RecreateSwapChain(
   return true;
 }
 
+void SkiaOutputDeviceVulkan::OnPostSubBufferFinished(
+    std::vector<ui::LatencyInfo> latency_info,
+    bool is_new_swapchain,
+    gfx::SwapResult result) {
+  if (is_new_swapchain)
+    result = gfx::SwapResult::SWAP_NAK_RECREATE_BUFFERS;
+  FinishSwapBuffers(gfx::SwapCompletionResult(result),
+                    vulkan_surface_->image_size(), std::move(latency_info));
+}
+
 SkiaOutputDeviceVulkan::SkSurfaceSizePair::SkSurfaceSizePair() = default;
 SkiaOutputDeviceVulkan::SkSurfaceSizePair::SkSurfaceSizePair(
     const SkSurfaceSizePair& other) = default;

components/viz/service/display_embedder/skia_output_device_vulkan.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "base/util/type_safety/pass_key.h"
 #include "build/build_config.h"
@@ -72,6 +73,9 @@ class SkiaOutputDeviceVulkan final : public SkiaOutputDevice {
   bool RecreateSwapChain(const gfx::Size& size,
                          sk_sp<SkColorSpace> color_space,
                          gfx::OverlayTransform transform);
+  void OnPostSubBufferFinished(std::vector<ui::LatencyInfo> latency_info,
+                               bool is_new_swapchain,
+                               gfx::SwapResult result);
 
   VulkanContextProvider* const context_provider_;
 
@@ -90,6 +94,8 @@ class SkiaOutputDeviceVulkan final : public SkiaOutputDevice {
   sk_sp<SkColorSpace> color_space_;
   bool is_new_swapchain_ = true;
 
+  base::WeakPtrFactory<SkiaOutputDeviceVulkan> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(SkiaOutputDeviceVulkan);
 };
 

components/viz/service/display_embedder/skia_output_surface_impl.cc

@@ -695,7 +695,7 @@ bool SkiaOutputSurfaceImpl::Initialize() {
       capabilities_.supports_post_sub_buffer) {
     capabilities_.only_invalidates_damage_rect = false;
     capabilities_.supports_target_damage = true;
-    damage_of_buffers_.resize(capabilities_.max_frames_pending + 1);
+    damage_of_buffers_.resize(capabilities_.number_of_buffers);
   }
 
   return result;

gpu/vulkan/vulkan_surface.cc

@@ -11,6 +11,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/stl_util.h"
+#include "base/threading/scoped_blocking_call.h"
 #include "gpu/vulkan/vulkan_device_queue.h"
 #include "gpu/vulkan/vulkan_function_pointers.h"
 #include "gpu/vulkan/vulkan_swap_chain.h"
@@ -69,6 +70,9 @@ gfx::OverlayTransform FromVkSurfaceTransformFlag(
   }
 }
 
+// Minimum VkImages in a vulkan swap chain.
+uint32_t kMinImageCount = 3u;
+
 }  // namespace
 
 VulkanSurface::~VulkanSurface() {
@@ -166,7 +170,7 @@ bool VulkanSurface::Initialize(VulkanDeviceQueue* device_queue,
     return false;
   }
 
-  image_count_ = std::max(surface_caps.minImageCount, 3u);
+  image_count_ = std::max(surface_caps.minImageCount, kMinImageCount);
 
   return true;
 }
@@ -185,10 +189,18 @@ gfx::SwapResult VulkanSurface::SwapBuffers() {
 }
 
 gfx::SwapResult VulkanSurface::PostSubBuffer(const gfx::Rect& rect) {
-  return swap_chain_->PresentBuffer(rect);
+  return swap_chain_->PostSubBuffer(rect);
+}
+
+void VulkanSurface::PostSubBufferAsync(
+    const gfx::Rect& rect,
+    VulkanSwapChain::PostSubBufferCompletionCallback callback) {
+  swap_chain_->PostSubBufferAsync(rect, std::move(callback));
 }
 
 void VulkanSurface::Finish() {
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::WILL_BLOCK);
   vkQueueWaitIdle(device_queue_->GetVulkanQueue());
 }
 
@@ -262,7 +274,7 @@ bool VulkanSurface::CreateSwapChain(const gfx::Size& size,
   auto swap_chain = std::make_unique<VulkanSwapChain>();
 
   // Create swap chain.
-  DCHECK_EQ(image_count_, std::max(surface_caps.minImageCount, 3u));
+  DCHECK_EQ(image_count_, std::max(surface_caps.minImageCount, kMinImageCount));
   if (!swap_chain->Initialize(
           device_queue_, surface_, surface_format_, image_size_, image_count_,
           vk_transform, enforce_protected_memory_, std::move(swap_chain_))) {

gpu/vulkan/vulkan_surface.h

@@ -46,6 +46,9 @@ class COMPONENT_EXPORT(VULKAN) VulkanSurface {
 
   gfx::SwapResult SwapBuffers();
   gfx::SwapResult PostSubBuffer(const gfx::Rect& rect);
+  void PostSubBufferAsync(
+      const gfx::Rect& rect,
+      VulkanSwapChain::PostSubBufferCompletionCallback callback);
 
   void Finish();
 

gpu/vulkan/vulkan_swap_chain.h

@@ -10,13 +10,22 @@
 #include <memory>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/component_export.h"
 #include "base/containers/circular_deque.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/optional.h"
+#include "base/synchronization/condition_variable.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/thread_checker.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/swap_result.h"
 
+namespace base {
+class SingleThreadTaskRunner;
+}
+
 namespace gpu {
 
 class VulkanCommandBuffer;
@@ -72,12 +81,30 @@ class COMPONENT_EXPORT(VULKAN) VulkanSwapChain {
   void Destroy();
 
   // Present the current buffer.
-  gfx::SwapResult PresentBuffer(const gfx::Rect& rect);
-
-  uint32_t num_images() const { return static_cast<uint32_t>(images_.size()); }
-  const gfx::Size& size() const { return size_; }
-  bool use_protected_memory() const { return use_protected_memory_; }
-  VkResult state() const { return state_; }
+  gfx::SwapResult PostSubBuffer(const gfx::Rect& rect);
+  using PostSubBufferCompletionCallback =
+      base::OnceCallback<void(gfx::SwapResult)>;
+  void PostSubBufferAsync(const gfx::Rect& rect,
+                          PostSubBufferCompletionCallback callback);
+
+  uint32_t num_images() const {
+    // size of |images_| will not be changed after initializing, so it is safe
+    // to read it here.
+    return static_cast<uint32_t>(TS_UNCHECKED_READ(images_).size());
+  }
+  const gfx::Size& size() const {
+    // |size_| is never changed after initialization.
+    return size_;
+  }
+  bool use_protected_memory() const {
+    // |use_protected_memory_| is never changed after initialization.
+    return use_protected_memory_;
+  }
+
+  VkResult state() const {
+    base::AutoLock auto_lock(lock_);
+    return state_;
+  }
 
  private:
   bool InitializeSwapChain(VkSurfaceKHR surface,
@@ -86,26 +113,31 @@ class COMPONENT_EXPORT(VULKAN) VulkanSwapChain {
                            uint32_t min_image_count,
                            VkSurfaceTransformFlagBitsKHR pre_transform,
                            bool use_protected_memory,
-                           std::unique_ptr<VulkanSwapChain> old_swap_chain);
-  void DestroySwapChain();
+                           std::unique_ptr<VulkanSwapChain> old_swap_chain)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  void DestroySwapChain() EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
-  bool InitializeSwapImages(const VkSurfaceFormatKHR& surface_format);
-  void DestroySwapImages();
+  bool InitializeSwapImages(const VkSurfaceFormatKHR& surface_format)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  void DestroySwapImages() EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
   bool BeginWriteCurrentImage(VkImage* image,
                               uint32_t* image_index,
                               VkImageLayout* layout,
                               VkSemaphore* semaphore);
   void EndWriteCurrentImage(VkImageLayout layout, VkSemaphore semaphore);
-  bool AcquireNextImage();
+  bool PresentBuffer(const gfx::Rect& rect) EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  bool AcquireNextImage() EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  // Wait until PostSubBufferAsync() is finished on ThreadPool.
+  void WaitUntilPostSubBufferAsyncFinished() EXCLUSIVE_LOCKS_REQUIRED(lock_);
+
+  mutable base::Lock lock_;
 
   bool use_protected_memory_ = false;
   VulkanDeviceQueue* device_queue_ = nullptr;
   bool is_incremental_present_supported_ = false;
-  VkSwapchainKHR swap_chain_ = VK_NULL_HANDLE;
-
+  VkSwapchainKHR swap_chain_ GUARDED_BY(lock_) = VK_NULL_HANDLE;
   std::unique_ptr<VulkanCommandPool> command_pool_;
-
   gfx::Size size_;
 
   struct ImageData {
@@ -126,14 +158,33 @@ class COMPONENT_EXPORT(VULKAN) VulkanSwapChain {
     // to swapchain for presenting.
     bool is_acquired = false;
   };
-  std::vector<ImageData> images_;
-
-  // Acquired image index.
-  base::circular_deque<uint32_t> in_present_images_;
-  base::Optional<uint32_t> acquired_image_;
-  bool is_writing_ = false;
-  VkSemaphore end_write_semaphore_ = VK_NULL_HANDLE;
-  VkResult state_ = VK_SUCCESS;
+
+  // Images in the swap chain.
+  std::vector<ImageData> images_ GUARDED_BY(lock_);
+
+  base::circular_deque<uint32_t> in_present_images_ GUARDED_BY(lock_);
+  bool is_writing_ GUARDED_BY(lock_) = false;
+  VkSemaphore end_write_semaphore_ GUARDED_BY(lock_) = VK_NULL_HANDLE;
+
+  // Condition variable is signalled when a PostSubBufferAsync() is finished.
+  base::ConditionVariable condition_variable_{&lock_};
+
+  // Count of pending unfinished PostSubBufferAsync() calls.
+  uint32_t pending_post_sub_buffer_ GUARDED_BY(lock_) = 0;
+
+  // The current swapchain state_.
+  VkResult state_ GUARDED_BY(lock_) = VK_SUCCESS;
+
+  // Acquired images queue.
+  base::circular_deque<uint32_t> acquired_images_ GUARDED_BY(lock_);
+
+  // For executing task on GPU main thread.
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  // For executing PosSubBufferAsync tasks off the GPU main thread.
+  scoped_refptr<base::SequencedTaskRunner> post_sub_buffer_task_runner_;
+
+  THREAD_CHECKER(thread_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(VulkanSwapChain);
 };