From d246673a4c0563bda72857f47cdea9d94c6ea0d9 Mon Sep 17 00:00:00 2001 From: Maksim Sisov 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 Commit-Queue: Maksim Sisov 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>( + 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(buffer_params_resource); - - wl_resource* buffer_resource = - CreateResourceWithImpl<::testing::NiceMock>( - 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(resource)->CreateImmed( - client, resource, buffer_id, width, height, format, flags); + auto* buffer_params = + GetUserDataAs(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 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>( - client, &zwp_linux_buffer_params_v1_interface, - wl_resource_get_version(resource), &kMockZwpLinuxBufferParamsV1Impl, id); + wl_resource* params_resource = + CreateResourceWithImpl<::testing::NiceMock>( + client, &zwp_linux_buffer_params_v1_interface, + wl_resource_get_version(resource), &kMockZwpLinuxBufferParamsV1Impl, + id); - GetUserDataAs(resource)->CreateParams(client, resource, - id); + auto* zwp_linux_dmabuf = GetUserDataAs(resource); + auto* buffer_params = + GetUserDataAs(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& buffer_params() const { + return buffer_params_; + } + + void StoreBufferParams(MockZwpLinuxBufferParamsV1* params); + + void OnBufferParamsDestroyed(MockZwpLinuxBufferParamsV1* params); + private: + std::vector buffer_params_; + DISALLOW_COPY_AND_ASSIGN(MockZwpLinuxDmabufV1); }; -- 2.23.0