path: root/0003-ozone-wayland-Stop-using-wl_display_roundtrip.patch

From e58ed0f56e9ffbee6222b9b8b3f62be4acde72ac Mon Sep 17 00:00:00 2001
From: Maksim Sisov <msisov@igalia.com>
Date: Thu, 8 Aug 2019 11:32:57 +0000
Subject: [PATCH 3/6] [ozone/wayland] Stop using wl_display_roundtrip

According to the Wayland documentation, wl_display_roundtrip is a
blocking call that can block if the event queue is empty. That is,
to be able to process buffer swap requests synchronously,
that call blocks until a frame callback comes or a wl_buffer
is attached. This results in blocking the CrBrowserMain thread.

This change alters the logic so that the buffer manager processes
one pending buffer at a time asynchronously avoiding blocking
the thread if synchronous processing is not possible due to
two conditions:
1) a frame callback is not received,
2) a wl_frame buffer is not created.

That is, on a frame no 1, the manager does not have a
frame callback installed and it may not process the buffer
synchronously if a wl_buffer is not created.
(note that wl_buffers can be created synchronously or asynchronously
depending on the protocol version of the zwp_linux_dmabuf[1]).
If such condition satisfies (there is no wl_buffer created), that
buffer is stored as a pending one and is processed on a
AttachWlBuffer call. Otherwise, the manager just processes
the buffer swap for the frame no 1 synchronously.

On frames no 2 and onwards, the manager may not process
buffer swaps synchronously if a frame callback is set, but not
received. That is, whenever the manager attaches
buffers to surfaces and commits them, it sets frame callbacks,
which Wayland compositor runs whenever it is time to send next
frame. If such condition satisfies, the manager stores the buffer
and processes it on next OnFrameCallback call. Otherwise, the swap
request processed synchronously.

[1] https://cs.chromium.org/chromium/src/ui/ozone/platform/wayland/host/wayland_zwp_linux_dmabuf.cc?g=0&l=67

Bug: 987821
Test: WaylandBufferManagerTest.TestCommitBufferConditions
Change-Id: I153b2071a453856a40e23a7f0a15f61dbc01d27f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1726051
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Commit-Queue: Maksim Sisov <msisov@igalia.com>
Cr-Commit-Position: refs/heads/master@{#685143}
---
 .../host/wayland_buffer_manager_host.cc       | 123 ++++++++++++------
 .../test/mock_zwp_linux_buffer_params.cc      |  50 +++++--
 .../test/mock_zwp_linux_buffer_params.h       |  17 +++
 .../wayland/test/mock_zwp_linux_dmabuf.cc     |  34 ++++-
 .../wayland/test/mock_zwp_linux_dmabuf.h      |  12 ++
 5 files changed, 175 insertions(+), 61 deletions(-)

diff --git a/ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc b/ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc
index 1bcf85261b41..9355d47f2ec7 100644
--- a/ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc
+++ b/ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc
@@ -63,10 +63,22 @@ class WaylandBufferManagerHost::Surface {
   ~Surface() = default;
 
   bool CommitBuffer(uint32_t buffer_id, const gfx::Rect& damage_region) {
+    DCHECK(!pending_buffer_);
+
     WaylandBuffer* buffer = GetBuffer(buffer_id);
     if (!buffer)
       return false;
 
+    buffer->damage_region = damage_region;
+
+    // If the wl_buffer has been attached, but the wl_buffer still is null, it
+    // means the Wayland server failed to create the buffer and we have to fail
+    // here.
+    //
+    // TODO(msisov): should we ask to recreate buffers instead of failing?
+    if (buffer->attached && !buffer->wl_buffer)
+      return false;
+
     // This request may come earlier than the Wayland compositor has imported a
     // wl_buffer. Wait until the buffer is created. The wait takes place only
     // once. Though, the case when a request to attach a buffer comes earlier
@@ -78,46 +90,12 @@ class WaylandBufferManagerHost::Surface {
     // Another case, which always happen is waiting until the frame callback is
     // completed. Thus, wait here when the Wayland compositor fires the frame
     // callback.
-    while (!buffer->wl_buffer || !!wl_frame_callback_) {
-      // If the wl_buffer has been attached, but the wl_buffer still has been
-      // null, it means the Wayland server failed to create the buffer and we
-      // have to fail here.
-      if ((buffer->attached && !buffer->wl_buffer) ||
-          wl_display_roundtrip(connection_->display()) == -1)
-        return false;
+    if (!buffer->wl_buffer || wl_frame_callback_) {
+      pending_buffer_ = buffer;
+      return true;
     }
 
-    // Once the BufferRelease is called, the buffer will be released.
-    DCHECK(buffer->released);
-    buffer->released = false;
-
-    DCHECK(!submitted_buffer_);
-    submitted_buffer_ = buffer;
-
-    AttachAndDamageBuffer(buffer, damage_region);
-
-    SetupFrameCallback();
-    SetupPresentationFeedback(buffer_id);
-
-    CommitSurface();
-
-    connection_->ScheduleFlush();
-
-    // If the contents were reset, there is no buffer attached. It means we have
-    // to behave the same way as if it was the very first frame. Check the
-    // comment below where the |contents_reset_| is declared.
-    if (contents_reset_) {
-      prev_submitted_buffer_ = nullptr;
-      contents_reset_ = false;
-    }
-
-    // If it was the very first frame, the surface has not had a back buffer
-    // before, and Wayland won't release the front buffer until next buffer is
-    // attached. Thus, notify about successful submission immediately.
-    if (!prev_submitted_buffer_)
-      CompleteSubmission();
-
-    return true;
+    return CommitBufferInternal(buffer);
   }
 
   bool CreateBuffer(const gfx::Size& size, uint32_t buffer_id) {
@@ -150,6 +128,9 @@ class WaylandBufferManagerHost::Surface {
     if (prev_submitted_buffer_ == buffer)
       prev_submitted_buffer_ = nullptr;
 
+    if (pending_buffer_ == buffer)
+      pending_buffer_ = nullptr;
+
     auto result = buffers_.erase(buffer_id);
     return result;
   }
@@ -167,6 +148,9 @@ class WaylandBufferManagerHost::Surface {
 
     if (buffer->wl_buffer)
       SetupBufferReleaseListener(buffer);
+
+    if (pending_buffer_ == buffer)
+      ProcessPendingBuffer();
   }
 
   void ClearState() {
@@ -213,6 +197,10 @@ class WaylandBufferManagerHost::Surface {
     // Actual buffer size.
     const gfx::Size size;
 
+    // Damage region this buffer describes. Must be emptied once buffer is
+    // submitted.
+    gfx::Rect damage_region;
+
     // The id of this buffer.
     const uint32_t buffer_id;
 
@@ -239,9 +227,45 @@ class WaylandBufferManagerHost::Surface {
     DISALLOW_COPY_AND_ASSIGN(WaylandBuffer);
   };
 
-  void AttachAndDamageBuffer(WaylandBuffer* buffer,
-                             const gfx::Rect& damage_region) {
-    gfx::Rect pending_damage_region = damage_region;
+  bool CommitBufferInternal(WaylandBuffer* buffer) {
+    DCHECK(buffer);
+    DCHECK(!pending_buffer_);
+
+    // Once the BufferRelease is called, the buffer will be released.
+    DCHECK(buffer->released);
+    buffer->released = false;
+
+    DCHECK(!submitted_buffer_);
+    submitted_buffer_ = buffer;
+
+    AttachAndDamageBuffer(buffer);
+
+    SetupFrameCallback();
+    SetupPresentationFeedback(buffer->buffer_id);
+
+    CommitSurface();
+
+    connection_->ScheduleFlush();
+
+    // If the contents were reset, there is no buffer attached. It means we have
+    // to behave the same way as if it was the very first frame. Check the
+    // comment below where the |contents_reset_| is declared.
+    if (contents_reset_) {
+      prev_submitted_buffer_ = nullptr;
+      contents_reset_ = false;
+    }
+
+    // If it was the very first frame, the surface has not had a back buffer
+    // before, and Wayland won't release the front buffer until next buffer is
+    // attached. Thus, notify about successful submission immediately.
+    if (!prev_submitted_buffer_)
+      CompleteSubmission();
+
+    return true;
+  }
+
+  void AttachAndDamageBuffer(WaylandBuffer* buffer) {
+    gfx::Rect pending_damage_region = std::move(buffer->damage_region);
     // If the size of the damage region is empty, wl_surface_damage must be
     // supplied with the actual size of the buffer, which is going to be
     // committed.
@@ -299,6 +323,8 @@ class WaylandBufferManagerHost::Surface {
   void OnFrameCallback(struct wl_callback* callback) {
     DCHECK(wl_frame_callback_.get() == callback);
     wl_frame_callback_.reset();
+
+    ProcessPendingBuffer();
   }
 
   // wl_callback_listener
@@ -390,7 +416,8 @@ class WaylandBufferManagerHost::Surface {
                       const gfx::PresentationFeedback& feedback) {
     // The order of submission and presentation callbacks cannot be controlled.
     // Some Wayland compositors may fire presentation callbacks earlier than we
-    // are able to send submission callbacks and this is bad. Thus, handle it here.
+    // are able to send submission callbacks and this is bad. Thus, handle it
+    // here.
     if (submitted_buffer_ && submitted_buffer_->buffer_id == buffer_id) {
       submitted_buffer_->needs_send_feedback = true;
       submitted_buffer_->feedback = feedback;
@@ -441,6 +468,15 @@ class WaylandBufferManagerHost::Surface {
                          gfx::PresentationFeedback::Failure());
   }
 
+  void ProcessPendingBuffer() {
+    if (!pending_buffer_)
+      return;
+
+    auto* buffer = pending_buffer_;
+    pending_buffer_ = nullptr;
+    CommitBufferInternal(buffer);
+  }
+
   // Widget this helper surface backs and has 1:1 relationship with the
   // WaylandWindow.
 
@@ -465,6 +501,9 @@ class WaylandBufferManagerHost::Surface {
   // shown.
   PresentationFeedbackQueue presentation_feedbacks_;
 
+  // A buffer, which is pending to be submitted (look the comment in the
+  // CommitBuffer method).
+  WaylandBuffer* pending_buffer_ = nullptr;
   // Current submitted buffer.
   WaylandBuffer* submitted_buffer_ = nullptr;
   // Previous submitted buffer.
diff --git a/ui/ozone/platform/wayland/test/mock_zwp_linux_buffer_params.cc b/ui/ozone/platform/wayland/test/mock_zwp_linux_buffer_params.cc
index ca7fa6add5b9..8ebe4e6444db 100644
--- a/ui/ozone/platform/wayland/test/mock_zwp_linux_buffer_params.cc
+++ b/ui/ozone/platform/wayland/test/mock_zwp_linux_buffer_params.cc
@@ -5,6 +5,7 @@
 #include "ui/ozone/platform/wayland/test/mock_zwp_linux_buffer_params.h"
 
 #include "ui/ozone/platform/wayland/test/mock_buffer.h"
+#include "ui/ozone/platform/wayland/test/mock_zwp_linux_dmabuf.h"
 
 namespace wl {
 
@@ -25,6 +26,20 @@ void Add(wl_client* client,
                      modifier_hi, modifier_lo);
 }
 
+void CreateCommon(MockZwpLinuxBufferParamsV1* buffer_params,
+                  wl_client* client,
+                  int32_t width,
+                  int32_t height,
+                  uint32_t format,
+                  uint32_t flags) {
+  wl_resource* buffer_resource =
+      CreateResourceWithImpl<::testing::NiceMock<MockBuffer>>(
+          client, &wl_buffer_interface, 1, &kMockWlBufferImpl, 0,
+          std::move(buffer_params->fds_));
+
+  buffer_params->SetBufferResource(buffer_resource);
+}
+
 void Create(wl_client* client,
             wl_resource* buffer_params_resource,
             int32_t width,
@@ -33,28 +48,23 @@ void Create(wl_client* client,
             uint32_t flags) {
   auto* buffer_params =
       GetUserDataAs<MockZwpLinuxBufferParamsV1>(buffer_params_resource);
-
-  wl_resource* buffer_resource =
-      CreateResourceWithImpl<::testing::NiceMock<MockBuffer>>(
-          client, &wl_buffer_interface, 1, &kMockWlBufferImpl, 0,
-          std::move(buffer_params->fds_));
-
-  zwp_linux_buffer_params_v1_send_created(buffer_params_resource,
-                                          buffer_resource);
-
+  CreateCommon(buffer_params, client, width, height, format, flags);
   buffer_params->Create(client, buffer_params_resource, width, height, format,
                         flags);
 }
 
 void CreateImmed(wl_client* client,
-                 wl_resource* resource,
+                 wl_resource* buffer_params_resource,
                  uint32_t buffer_id,
                  int32_t width,
                  int32_t height,
                  uint32_t format,
                  uint32_t flags) {
-  GetUserDataAs<MockZwpLinuxBufferParamsV1>(resource)->CreateImmed(
-      client, resource, buffer_id, width, height, format, flags);
+  auto* buffer_params =
+      GetUserDataAs<MockZwpLinuxBufferParamsV1>(buffer_params_resource);
+  CreateCommon(buffer_params, client, width, height, format, flags);
+  buffer_params->CreateImmed(client, buffer_params_resource, buffer_id, width,
+                             height, format, flags);
 }
 
 }  // namespace
@@ -66,6 +76,20 @@ const struct zwp_linux_buffer_params_v1_interface
 MockZwpLinuxBufferParamsV1::MockZwpLinuxBufferParamsV1(wl_resource* resource)
     : ServerObject(resource) {}
 
-MockZwpLinuxBufferParamsV1::~MockZwpLinuxBufferParamsV1() {}
+MockZwpLinuxBufferParamsV1::~MockZwpLinuxBufferParamsV1() {
+  DCHECK(linux_dmabuf_);
+  linux_dmabuf_->OnBufferParamsDestroyed(this);
+}
+
+void MockZwpLinuxBufferParamsV1::SetZwpLinuxDmabuf(
+    MockZwpLinuxDmabufV1* linux_dmabuf) {
+  DCHECK(!linux_dmabuf_);
+  linux_dmabuf_ = linux_dmabuf;
+}
+
+void MockZwpLinuxBufferParamsV1::SetBufferResource(wl_resource* resource) {
+  DCHECK(!buffer_resource_);
+  buffer_resource_ = resource;
+}
 
 }  // namespace wl
diff --git a/ui/ozone/platform/wayland/test/mock_zwp_linux_buffer_params.h b/ui/ozone/platform/wayland/test/mock_zwp_linux_buffer_params.h
index 57b4b9299a31..7da794a92c69 100644
--- a/ui/ozone/platform/wayland/test/mock_zwp_linux_buffer_params.h
+++ b/ui/ozone/platform/wayland/test/mock_zwp_linux_buffer_params.h
@@ -20,6 +20,8 @@ namespace wl {
 extern const struct zwp_linux_buffer_params_v1_interface
     kMockZwpLinuxBufferParamsV1Impl;
 
+class MockZwpLinuxDmabufV1;
+
 // Manage zwp_linux_buffer_params_v1
 class MockZwpLinuxBufferParamsV1 : public ServerObject {
  public:
@@ -52,9 +54,24 @@ class MockZwpLinuxBufferParamsV1 : public ServerObject {
                     uint32_t format,
                     uint32_t flags));
 
+  wl_resource* buffer_resource() const { return buffer_resource_; }
+
+  void SetZwpLinuxDmabuf(MockZwpLinuxDmabufV1* linux_dmabuf);
+
+  void SetBufferResource(wl_resource* resource);
+
   std::vector<base::ScopedFD> fds_;
 
  private:
+  // Non-owned pointer to the linux dmabuf object, which created this params
+  // resource and holds a pointer to it. On destruction, must notify it about
+  // going out of scope.
+  MockZwpLinuxDmabufV1* linux_dmabuf_ = nullptr;
+
+  // A buffer resource, which is created on Create or CreateImmed call. Can be
+  // null if not created/failed to be created.
+  wl_resource* buffer_resource_ = nullptr;
+
   DISALLOW_COPY_AND_ASSIGN(MockZwpLinuxBufferParamsV1);
 };
 
diff --git a/ui/ozone/platform/wayland/test/mock_zwp_linux_dmabuf.cc b/ui/ozone/platform/wayland/test/mock_zwp_linux_dmabuf.cc
index c44d41a40bb7..006d55ff40f4 100644
--- a/ui/ozone/platform/wayland/test/mock_zwp_linux_dmabuf.cc
+++ b/ui/ozone/platform/wayland/test/mock_zwp_linux_dmabuf.cc
@@ -17,12 +17,20 @@ namespace {
 constexpr uint32_t kLinuxDmabufVersion = 1;
 
 void CreateParams(wl_client* client, wl_resource* resource, uint32_t id) {
-  CreateResourceWithImpl<::testing::NiceMock<MockZwpLinuxBufferParamsV1>>(
-      client, &zwp_linux_buffer_params_v1_interface,
-      wl_resource_get_version(resource), &kMockZwpLinuxBufferParamsV1Impl, id);
+  wl_resource* params_resource =
+      CreateResourceWithImpl<::testing::NiceMock<MockZwpLinuxBufferParamsV1>>(
+          client, &zwp_linux_buffer_params_v1_interface,
+          wl_resource_get_version(resource), &kMockZwpLinuxBufferParamsV1Impl,
+          id);
 
-  GetUserDataAs<MockZwpLinuxDmabufV1>(resource)->CreateParams(client, resource,
-                                                              id);
+  auto* zwp_linux_dmabuf = GetUserDataAs<MockZwpLinuxDmabufV1>(resource);
+  auto* buffer_params =
+      GetUserDataAs<MockZwpLinuxBufferParamsV1>(params_resource);
+
+  DCHECK(buffer_params);
+  zwp_linux_dmabuf->StoreBufferParams(buffer_params);
+  buffer_params->SetZwpLinuxDmabuf(zwp_linux_dmabuf);
+  zwp_linux_dmabuf->CreateParams(client, resource, id);
 }
 
 }  // namespace
@@ -37,6 +45,20 @@ MockZwpLinuxDmabufV1::MockZwpLinuxDmabufV1()
                    &kMockZwpLinuxDmabufV1Impl,
                    kLinuxDmabufVersion) {}
 
-MockZwpLinuxDmabufV1::~MockZwpLinuxDmabufV1() {}
+MockZwpLinuxDmabufV1::~MockZwpLinuxDmabufV1() {
+  DCHECK(buffer_params_.empty());
+}
+
+void MockZwpLinuxDmabufV1::StoreBufferParams(
+    MockZwpLinuxBufferParamsV1* params) {
+  buffer_params_.push_back(params);
+}
+
+void MockZwpLinuxDmabufV1::OnBufferParamsDestroyed(
+    MockZwpLinuxBufferParamsV1* params) {
+  auto it = std::find(buffer_params_.begin(), buffer_params_.end(), params);
+  DCHECK(it != buffer_params_.end());
+  buffer_params_.erase(it);
+}
 
 }  // namespace wl
diff --git a/ui/ozone/platform/wayland/test/mock_zwp_linux_dmabuf.h b/ui/ozone/platform/wayland/test/mock_zwp_linux_dmabuf.h
index f63b92d34a50..9d2e1077d79f 100644
--- a/ui/ozone/platform/wayland/test/mock_zwp_linux_dmabuf.h
+++ b/ui/ozone/platform/wayland/test/mock_zwp_linux_dmabuf.h
@@ -18,6 +18,8 @@ namespace wl {
 
 extern const struct zwp_linux_dmabuf_v1_interface kMockZwpLinuxDmabufV1Impl;
 
+class MockZwpLinuxBufferParamsV1;
+
 // Manage zwp_linux_dmabuf_v1 object.
 class MockZwpLinuxDmabufV1 : public GlobalObject {
  public:
@@ -30,7 +32,17 @@ class MockZwpLinuxDmabufV1 : public GlobalObject {
                     wl_resource* resource,
                     uint32_t params_id));
 
+  const std::vector<MockZwpLinuxBufferParamsV1*>& buffer_params() const {
+    return buffer_params_;
+  }
+
+  void StoreBufferParams(MockZwpLinuxBufferParamsV1* params);
+
+  void OnBufferParamsDestroyed(MockZwpLinuxBufferParamsV1* params);
+
  private:
+  std::vector<MockZwpLinuxBufferParamsV1*> buffer_params_;
+
   DISALLOW_COPY_AND_ASSIGN(MockZwpLinuxDmabufV1);
 };
 
-- 
2.23.0