[ozone/wayland] Ensure root_surface is committed last in a frame

wl_surface.commits of a frame can happen asynchronously due to wl_buffer
import delay, frame callbacks, etc. This asynchrony can cause visual
glitches when the root_surface commits before its subsurfaces commit.

This CL ensures that a root_surface commit always happens after other
commits in a frame.

Change-Id: I7c83e122c4cdee42e6b8aca74875fa71b6c1ab5a
Bug: 1144179
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2547395
Commit-Queue: Kramer Ge <fangzhoug@chromium.org>
Reviewed-by: Robert Kroeger <rjkroege@chromium.org>
Reviewed-by: Maksim Sisov (GMT+2) <msisov@igalia.com>
Cr-Commit-Position: refs/heads/master@{#834327}

ui/ozone/common/features.cc

@@ -8,14 +8,8 @@
 
 namespace ui {
 
-const base::Feature kWaylandOverlayDelegation {
-  "WaylandOverlayDelegation",
-#if BUILDFLAG(IS_CHROMEOS_LACROS)
-      base::FEATURE_ENABLED_BY_DEFAULT
-#else
-      base::FEATURE_DISABLED_BY_DEFAULT
-#endif
-};
+const base::Feature kWaylandOverlayDelegation{"WaylandOverlayDelegation",
+                                              base::FEATURE_ENABLED_BY_DEFAULT};
 
 bool IsWaylandOverlayDelegationEnabled() {
   return base::FeatureList::IsEnabled(kWaylandOverlayDelegation);

ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc

@@ -7,7 +7,9 @@
 #include <presentation-time-client-protocol.h>
 #include <memory>
 
+#include "base/bind.h"
 #include "base/i18n/number_formatting.h"
+#include "base/stl_util.h"
 #include "base/strings/strcat.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/current_thread.h"
@@ -59,6 +61,34 @@ std::string NumberToString(uint32_t number) {
 
 }  // namespace
 
+struct WaylandBufferManagerHost::Frame {
+  explicit Frame(WaylandSurface* root_surface)
+      : root_surface(root_surface), weak_factory(this) {}
+  void IncrementPendingActions() { ++pending_actions; }
+  void PendingActionComplete() {
+    CHECK_GT(pending_actions, 0u);
+    if (!--pending_actions)
+      std::move(frame_commit_cb).Run();
+  }
+
+  // |root_surface| and |buffer_id| are saved so this Frame can be destroyed to
+  // prevent running |frame_commit_cb| in case the corresponding surface/buffer
+  // is removed.
+  WaylandSurface* root_surface;
+  uint32_t buffer_id = 0u;
+
+  // Number of actions to be completed before |frame_commit_cb| is run
+  // automatically. Such actions include:
+  //   1) End Frame;
+  //   2) Commit of a subsurface in this frame;
+  size_t pending_actions = 0u;
+
+  // This runs WaylandBufferManagerHost::Surface::CommitBuffer() of
+  // |root_surface| with |buffer_id|.
+  base::OnceCallback<bool()> frame_commit_cb;
+  base::WeakPtrFactory<WaylandBufferManagerHost::Frame> weak_factory;
+};
+
 class WaylandBufferManagerHost::Surface {
  public:
   Surface(WaylandSurface* wayland_surface,
@@ -73,7 +103,9 @@ class WaylandBufferManagerHost::Surface {
       uint32_t buffer_id,
       const gfx::Rect& damage_region,
       bool wait_for_frame_callback,
+      base::OnceClosure post_commit_cb,
       gfx::GpuFenceHandle access_fence_handle = gfx::GpuFenceHandle()) {
+    DCHECK(!post_commit_cb.is_null());
     // The window has already been destroyed.
     if (!wayland_surface_)
       return true;
@@ -81,7 +113,8 @@ class WaylandBufferManagerHost::Surface {
     // This is a buffer-less commit, do not lookup buffers.
     if (buffer_id == kInvalidBufferId) {
       DCHECK(access_fence_handle.is_null());
-      pending_commits_.push_back({nullptr, wait_for_frame_callback, nullptr});
+      pending_commits_.push_back({nullptr, wait_for_frame_callback, nullptr,
+                                  std::move(post_commit_cb)});
       MaybeProcessPendingBuffer();
       return true;
     }
@@ -113,7 +146,8 @@ class WaylandBufferManagerHost::Surface {
 
     pending_commits_.push_back(
         {buffer, wait_for_frame_callback,
-         std::make_unique<gfx::GpuFence>(std::move(access_fence_handle))});
+         std::make_unique<gfx::GpuFence>(std::move(access_fence_handle)),
+         std::move(post_commit_cb)});
     MaybeProcessPendingBuffer();
     return true;
   }
@@ -128,8 +162,10 @@ class WaylandBufferManagerHost::Surface {
       MaybeProcessSubmittedBuffers();
       for (auto it = pending_commits_.begin(); it != pending_commits_.end();
            ++it) {
-        if (it->buffer == buffer)
+        if (it->buffer == buffer) {
+          std::move(it->post_commit_cb).Run();
           pending_commits_.erase(it++);
+        }
       }
     }
 
@@ -161,6 +197,8 @@ class WaylandBufferManagerHost::Surface {
     ResetSurfaceContents();
 
     submitted_buffers_.clear();
+    for (auto& pending_commit : pending_commits_)
+      std::move(pending_commit.post_commit_cb).Run();
     pending_commits_.clear();
 
     connection_->ScheduleFlush();
@@ -236,6 +274,8 @@ class WaylandBufferManagerHost::Surface {
     // Fence to wait on before the |buffer| content is available to read by
     // Wayland host.
     std::unique_ptr<gfx::GpuFence> access_fence;
+    // Callback to run once this commit is applied.
+    base::OnceClosure post_commit_cb;
   };
 
   bool CommitBufferInternal(WaylandBuffer* buffer,
@@ -581,6 +621,7 @@ class WaylandBufferManagerHost::Surface {
       if (commit.wait_for_callback)
         SetupFrameCallback();
       CommitSurface();
+      std::move(commit.post_commit_cb).Run();
       connection_->ScheduleFlush();
       MaybeProcessSubmittedBuffers();
       return;
@@ -588,6 +629,7 @@ class WaylandBufferManagerHost::Surface {
 
     CommitBufferInternal(commit.buffer, commit.wait_for_callback,
                          commit.access_fence->GetGpuFenceHandle());
+    std::move(commit.post_commit_cb).Run();
   }
 
   // Widget this helper surface backs and has 1:1 relationship with the
@@ -660,6 +702,8 @@ void WaylandBufferManagerHost::OnWindowRemoved(WaylandWindow* window) {
     surface_graveyard_.emplace_back(std::move(it->second));
   }
   surfaces_.erase(it);
+
+  RemovePendingFrames(window->root_surface(), 0u);
 }
 
 void WaylandBufferManagerHost::OnWindowConfigured(WaylandWindow* window) {
@@ -690,6 +734,8 @@ void WaylandBufferManagerHost::OnSubsurfaceRemoved(
     surface_graveyard_.emplace_back(std::move(it->second));
   }
   surfaces_.erase(it);
+
+  RemovePendingFrames(subsurface->wayland_surface(), 0u);
 }
 
 void WaylandBufferManagerHost::SetSurfaceConfigured(WaylandSurface* surface) {
@@ -724,6 +770,7 @@ void WaylandBufferManagerHost::OnChannelDestroyed() {
     surface_pair.second->ClearState();
 
   anonymous_buffers_.clear();
+  pending_frames_.clear();
 }
 
 wl::BufferFormatsWithModifiersMap
@@ -824,11 +871,50 @@ void WaylandBufferManagerHost::CreateShmBasedBuffer(mojo::PlatformHandle shm_fd,
   connection_->ScheduleFlush();
 }
 
+void WaylandBufferManagerHost::StartFrame(WaylandSurface* root_surface) {
+  RemovePendingFrames(nullptr, 0u);
+  DCHECK_LE(pending_frames_.size(), 10u);
+  pending_frames_.push_back(
+      std::make_unique<WaylandBufferManagerHost::Frame>(root_surface));
+  pending_frames_.back()->IncrementPendingActions();
+}
+
+void WaylandBufferManagerHost::EndFrame(uint32_t buffer_id) {
+  DCHECK(base::CurrentUIThread::IsSet());
+
+  DCHECK(!pending_frames_.empty());
+  pending_frames_.back()->buffer_id = buffer_id;
+  Surface* surface = GetSurface(pending_frames_.back()->root_surface);
+  if (!surface) {
+    pending_frames_.erase(--pending_frames_.end());
+    return;
+  }
+
+  base::OnceClosure post_commit_cb = base::DoNothing();
+  pending_frames_.back()->frame_commit_cb =
+      base::BindOnce(&WaylandBufferManagerHost::Surface::CommitBuffer,
+                     base::Unretained(surface), buffer_id, gfx::Rect(), false,
+                     std::move(post_commit_cb), gfx::GpuFenceHandle());
+
+  pending_frames_.back()->PendingActionComplete();
+}
+
+void WaylandBufferManagerHost::RemovePendingFrames(WaylandSurface* root_surface,
+                                                   uint32_t buffer_id) {
+  base::EraseIf(pending_frames_,
+                [buffer_id, root_surface](const std::unique_ptr<Frame>& frame) {
+                  return !frame->pending_actions ||
+                         (frame->buffer_id == buffer_id && buffer_id) ||
+                         (frame->root_surface == root_surface && root_surface);
+                });
+}
+
 bool WaylandBufferManagerHost::CommitBufferInternal(
     WaylandSurface* wayland_surface,
     uint32_t buffer_id,
     const gfx::Rect& damage_region,
     bool wait_for_frame_callback,
+    bool commit_synced_subsurface,
     gfx::GpuFenceHandle access_fence_handle) {
   DCHECK(base::CurrentUIThread::IsSet());
 
@@ -836,7 +922,16 @@ bool WaylandBufferManagerHost::CommitBufferInternal(
   if (!surface || !ValidateBufferIdFromGpu(buffer_id))
     return false;
 
+  base::OnceClosure subsurface_committed_cb = base::DoNothing();
+  if (!pending_frames_.empty() && commit_synced_subsurface) {
+    pending_frames_.back()->IncrementPendingActions();
+    subsurface_committed_cb.Reset();
+    subsurface_committed_cb =
+        base::BindOnce(&WaylandBufferManagerHost::Frame::PendingActionComplete,
+                       pending_frames_.back()->weak_factory.GetWeakPtr());
+  }
   if (!surface->CommitBuffer(buffer_id, damage_region, wait_for_frame_callback,
+                             std::move(subsurface_committed_cb),
                              std::move(access_fence_handle))) {
     error_message_ =
         base::StrCat({"Buffer with ", NumberToString(buffer_id),
@@ -848,24 +943,6 @@ bool WaylandBufferManagerHost::CommitBufferInternal(
   return true;
 }
 
-bool WaylandBufferManagerHost::CommitWithoutBufferInternal(
-    WaylandSurface* wayland_surface,
-    bool wait_for_frame_callback) {
-  DCHECK(base::CurrentUIThread::IsSet());
-
-  Surface* surface = GetSurface(wayland_surface);
-  if (!surface)
-    return false;
-
-  bool result = surface->CommitBuffer(kInvalidBufferId, gfx::Rect(),
-                                      wait_for_frame_callback);
-  DCHECK(result);
-
-  if (!error_message_.empty())
-    TerminateGpuProcess();
-  return true;
-}
-
 void WaylandBufferManagerHost::CommitBuffer(gfx::AcceleratedWidget widget,
                                             uint32_t buffer_id,
                                             const gfx::Rect& damage_region) {
@@ -977,6 +1054,8 @@ void WaylandBufferManagerHost::DestroyBuffer(gfx::AcceleratedWidget widget,
     }
   }
 
+  RemovePendingFrames(nullptr, buffer_id);
+
   // Ensure that we can't destroy more than 1 buffer. This can be 0 as well
   // if no buffers are destroyed.
   DCHECK_LE(destroyed_count, 1u);

ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h

@@ -5,7 +5,6 @@
 #ifndef UI_OZONE_PLATFORM_WAYLAND_HOST_WAYLAND_BUFFER_MANAGER_HOST_H_
 #define UI_OZONE_PLATFORM_WAYLAND_HOST_WAYLAND_BUFFER_MANAGER_HOST_H_
 
-#include <map>
 #include <memory>
 #include <vector>
 
@@ -155,6 +154,14 @@ class WaylandBufferManagerHost : public ozone::mojom::WaylandBufferManagerHost,
       gfx::AcceleratedWidget widget,
       std::vector<ui::ozone::mojom::WaylandOverlayConfigPtr> overlays) override;
 
+  // Called by WaylandWindow to start recording a frame. This helps record the
+  // number of subsurface commits needed to finish for this frame before
+  // |root_surface| can be committed.
+  // This pairs with an EndCommitFrame(). Every CommitBufferInternal() in
+  // between increases the number of needed pending commits by 1.
+  void StartFrame(WaylandSurface* root_surface);
+  void EndFrame(uint32_t buffer_id = 0u);
+
   // Called by the WaylandWindow and asks to attach a wl_buffer with a
   // |buffer_id| to a WaylandSurface.
   // Calls OnSubmission and OnPresentation on successful swap and pixels
@@ -173,14 +180,9 @@ class WaylandBufferManagerHost : public ozone::mojom::WaylandBufferManagerHost,
       uint32_t buffer_id,
       const gfx::Rect& damage_region,
       bool wait_for_frame_callback = true,
+      bool commit_synced_subsurface = false,
       gfx::GpuFenceHandle access_fence_handle = gfx::GpuFenceHandle());
 
-  // Does a wl_surface commit without attaching any buffers. This commit will
-  // still wait for previous wl_frame_callback. Similar to above but for
-  // commits that do not change the root_surface.
-  bool CommitWithoutBufferInternal(WaylandSurface* wayland_surface,
-                                   bool wait_for_frame_callback = true);
-
   // When a surface is hidden, the client may want to detach the buffer attached
   // to the surface to ensure Wayland does not present those contents and do not
   // composite in a wrong way. Otherwise, users may see the contents of a hidden
@@ -196,10 +198,17 @@ class WaylandBufferManagerHost : public ozone::mojom::WaylandBufferManagerHost,
   // presentation callbacks for that surface.
   class Surface;
 
+  // This represents a frame that consists of state changes to multiple
+  // synchronized wl_surfaces that are in the same hierarchy. It defers
+  // committing the root surface until all child surfaces' states are ready.
+  struct Frame;
+
   bool CreateBuffer(const gfx::Size& size, uint32_t buffer_id);
 
   Surface* GetSurface(WaylandSurface* wayland_surface) const;
 
+  void RemovePendingFrames(WaylandSurface* root_surface, uint32_t buffer_id);
+
   // Validates data sent from GPU. If invalid, returns false and sets an error
   // message to |error_message_|.
   bool ValidateDataFromGpu(const base::ScopedFD& file,
@@ -237,6 +246,10 @@ class WaylandBufferManagerHost : public ozone::mojom::WaylandBufferManagerHost,
 
   base::flat_map<WaylandSurface*, std::unique_ptr<Surface>> surfaces_;
 
+  // When StartCommitFrame() is called, a Frame is pushed to
+  // |pending_frames_|. See StartCommitFrame().
+  std::vector<std::unique_ptr<Frame>> pending_frames_;
+
   // When a WaylandWindow/WaylandSubsurface is removed, its corresponding
   // Surface may still have an un-released buffer and un-acked presentation.
   // Thus, we keep removed surfaces in the graveyard. It's safe to delete them

ui/ozone/platform/wayland/host/wayland_window.cc

@@ -582,6 +582,9 @@ bool WaylandWindow::CommitOverlays(
   if (!ArrangeSubsurfaceStack(above, below))
     return false;
 
+  if (wayland_overlay_delegation_enabled_)
+    connection_->buffer_manager_host()->StartFrame(root_surface());
+
   {
     // Iterate through |subsurface_stack_below_|, setup subsurfaces and place
     // them in corresponding order. Commit wl_buffers once a subsurface is
@@ -606,7 +609,8 @@ bool WaylandWindow::CommitOverlays(
             nullptr, reference_above);
         connection_->buffer_manager_host()->CommitBufferInternal(
             (*iter)->wayland_surface(), (*overlay_iter)->buffer_id, gfx::Rect(),
-            /*wait_for_frame_callback=*/false,
+            /*wait_for_frame_callback=*/true,
+            /*commit_synced_subsurface=*/true,
             std::move((*overlay_iter)->access_fence_handle));
       } else {
         // If there're more subsurfaces requested that we don't need at the
@@ -636,7 +640,8 @@ bool WaylandWindow::CommitOverlays(
             reference_below, nullptr);
         connection_->buffer_manager_host()->CommitBufferInternal(
             (*iter)->wayland_surface(), (*overlay_iter)->buffer_id, gfx::Rect(),
-            /*wait_for_frame_callback=*/false,
+            /*wait_for_frame_callback=*/true,
+            /*commit_synced_subsurface=*/true,
             std::move((*overlay_iter)->access_fence_handle));
       } else {
         // If there're more subsurfaces requested that we don't need at the
@@ -660,7 +665,8 @@ bool WaylandWindow::CommitOverlays(
     connection_->buffer_manager_host()->CommitBufferInternal(
         primary_subsurface_->wayland_surface(), (*split)->buffer_id,
         (*split)->damage_region,
-        /*wait_for_frame_callback=*/false,
+        /*wait_for_frame_callback=*/true,
+        /*commit_synced_subsurface=*/true,
         std::move((*split)->access_fence_handle));
   }
 
@@ -672,15 +678,12 @@ bool WaylandWindow::CommitOverlays(
   }
 
   if (should_attach_background_buffer_) {
-    connection_->buffer_manager_host()->CommitBufferInternal(
-        root_surface(), background_buffer_id_, /*damage_region=*/gfx::Rect(),
-        /*wait_for_frame_callback=*/true);
+    connection_->buffer_manager_host()->EndFrame(background_buffer_id_);
     should_attach_background_buffer_ = false;
   } else {
     // Subsurfaces are set to sync, above surface configs will only take effect
     // when root_surface is committed.
-    connection_->buffer_manager_host()->CommitWithoutBufferInternal(
-        root_surface(), /*wait_for_frame_callback=*/true);
+    connection_->buffer_manager_host()->EndFrame();
   }
 
   return true;