Fix sequence points when there is no size/scale change.

This code currently requests LayerTreeHostImpl to allocate a new
LocalSurfaceId, but this is incorrect. Currently, all callers of
InsertSequencePoint do so after doing nothing (no synchronization
needed), or changing the size or scale. If the size or scale is changed,
UpdateCompositorScaleAndSize is called which increments the parent ID of
WindowTreeHost's window, so this is working by coincidence.

There seems to be a parent/child relationship between WindowTreeHost's
window and LayerTreeHostImpl. Currently, we request LayerTreeHostImpl to
allocate a new child ID, which does nothing since the parent ID is set
as the sequence ID.

This change forces a reallocation of the parent ID for WindowTreeHost's
window. LayerTreeHostImpl needs to sync its LocalSurfaceId with the
parent LocalSurfaceId, updating the parent ID which is later sent in the
CompositorFrame. This should ensure proper synchronisation.

It also renames InsertSequencePoint to OnStateUpdate and passes the
state that has changed into it. OnStateUpdate is responsible
for applying the state and inserting a sequence point if necessary.

Further, this updates the sequence IDs used to be the new (inserted)
LocalSurfaceId parent ID, rather than the previous one. This is
because we now have access to the new parent ID immediately.
The current (incorrect) code requests the impl thread to update
its child ID, which wasn't immediately available.

Bug: 1278651
Test: Manually using raster scale changes
Change-Id: Ia38f7ff65805dd6fdf9c25da81f9a1dc1d094882
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4194913
Reviewed-by: ccameron chromium <ccameron@chromium.org>
Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Eliot Courtney <edcourtney@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1100946}

ui/aura/window_tree_host_platform.cc

@@ -300,11 +300,34 @@ void WindowTreeHostPlatform::OnOcclusionStateChanged(
   SetNativeWindowOcclusionState(aura_occlusion_state, {});
 }
 
-int64_t WindowTreeHostPlatform::InsertSequencePoint() {
-  int64_t seq =
-      compositor()->local_surface_id_from_parent().parent_sequence_number();
-  compositor()->RequestNewLocalSurfaceId();
-  return seq;
+int64_t WindowTreeHostPlatform::OnStateUpdate(
+    const PlatformWindowDelegate::State& old,
+    const PlatformWindowDelegate::State& latest) {
+  if (old.bounds_dip != latest.bounds_dip || old.size_px != latest.size_px ||
+      old.window_scale != latest.window_scale) {
+    bool origin_changed = old.bounds_dip.origin() != latest.bounds_dip.origin();
+    OnBoundsChanged({origin_changed});
+  }
+
+  // Only set the sequence ID if this change will produce a frame.
+  // If it won't, we may wait indefinitely for a frame that will never come.
+  bool produces_frame = old.bounds_dip.size() != latest.bounds_dip.size() ||
+                        old.size_px != latest.size_px ||
+                        old.window_scale != latest.window_scale ||
+                        old.raster_scale != latest.raster_scale;
+
+  if (produces_frame) {
+    // Update window()'s LocalSurfaceId. This will ensure that the parent ID is
+    // updated both here and for LayerTreeHostImpl. So, the CompositorFrame sent
+    // by LayerTreeHostImpl will include the updated parent ID for
+    // synchronization. Some operations may have already updated the
+    // LocalSurfaceId, but this only modifies pending commit state, so it's not
+    // expensive.
+    window()->AllocateLocalSurfaceId();
+    compositor()->SetLocalSurfaceIdFromParent(window()->GetLocalSurfaceId());
+  }
+
+  return window()->GetLocalSurfaceId().parent_sequence_number();
 }
 
 void WindowTreeHostPlatform::SetFrameRateThrottleEnabled(bool enabled) {

ui/aura/window_tree_host_platform.h

@@ -88,7 +88,8 @@ class AURA_EXPORT WindowTreeHostPlatform : public WindowTreeHost,
   void OnMouseEnter() override;
   void OnOcclusionStateChanged(
       ui::PlatformWindowOcclusionState occlusion_state) override;
-  int64_t InsertSequencePoint() override;
+  int64_t OnStateUpdate(const PlatformWindowDelegate::State& old,
+                        const PlatformWindowDelegate::State& latest) override;
   void SetFrameRateThrottleEnabled(bool enabled) override;
 
   // Overridden from aura::WindowTreeHost:

ui/compositor/compositor.h

@@ -268,6 +268,11 @@ class COMPOSITOR_EXPORT Compositor : public base::PowerSuspendObserver,
     return host_->local_surface_id_from_parent();
   }
 
+  void SetLocalSurfaceIdFromParent(
+      const viz::LocalSurfaceId& local_surface_id_from_parent) {
+    host_->SetLocalSurfaceIdFromParent(local_surface_id_from_parent);
+  }
+
   void RequestNewLocalSurfaceId() { host_->RequestNewLocalSurfaceId(); }
 
   // Returns the size of the widget that is being drawn to in pixel coordinates.

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

@@ -284,7 +284,7 @@ void WaylandPopup::OnCloseRequest() {
 }
 
 bool WaylandPopup::OnInitialize(PlatformWindowInitProperties properties,
-                                State* state) {
+                                PlatformWindowDelegate::State* state) {
   DCHECK(parent_window());
   state->window_scale = parent_window()->applied_state().window_scale;
   state->size_px =

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

@@ -40,7 +40,7 @@ class WaylandPopup : public WaylandWindow {
 
   void OnCloseRequest() override;
   bool OnInitialize(PlatformWindowInitProperties properties,
-                    State* state) override;
+                    PlatformWindowDelegate::State* state) override;
   WaylandPopup* AsWaylandPopup() override;
   void SetWindowGeometry(gfx::Size size_dip) override;
   void UpdateWindowMask() override;

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

@@ -543,7 +543,7 @@ void WaylandToplevelWindow::OnSequencePoint(int64_t seq) {
 
 bool WaylandToplevelWindow::OnInitialize(
     PlatformWindowInitProperties properties,
-    State* state) {
+    PlatformWindowDelegate::State* state) {
 #if BUILDFLAG(IS_CHROMEOS_LACROS)
   auto token = base::UnguessableToken::Create();
   window_unique_id_ =

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

@@ -79,7 +79,7 @@ class WaylandToplevelWindow : public WaylandWindow,
   void AckConfigure(uint32_t serial) override;
 
   bool OnInitialize(PlatformWindowInitProperties properties,
-                    State* state) override;
+                    PlatformWindowDelegate::State* state) override;
   bool IsActive() const override;
   void SetWindowGeometry(gfx::Size size_dip) override;
   bool IsScreenCoordinatesEnabled() const override;

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

@@ -654,7 +654,7 @@ bool WaylandWindow::Initialize(PlatformWindowInitProperties properties) {
     return false;
   }
 
-  State state;
+  PlatformWindowDelegate::State state;
   state.bounds_dip = properties.bounds;
   // Properties contain DIP bounds but the buffer scale is initially 1 so it's
   // OK to assign.  The bounds will be recalculated when the buffer scale
@@ -1034,17 +1034,21 @@ void WaylandWindow::ProcessPendingConfigureState(uint32_t serial) {
   pending_configure_state_ = PendingConfigureState();
 }
 
-void WaylandWindow::RequestStateFromServer(State state, int64_t serial) {
+void WaylandWindow::RequestStateFromServer(PlatformWindowDelegate::State state,
+                                           int64_t serial) {
   RequestState(state, serial, /*force=*/false);
 }
 
-void WaylandWindow::RequestStateFromClient(State state) {
+void WaylandWindow::RequestStateFromClient(
+    PlatformWindowDelegate::State state) {
   // In general, client requested changes should not be throttled so force
   // apply this.
   RequestState(state, /*serial=*/-1, /*force=*/true);
 }
 
-void WaylandWindow::RequestState(State state, int64_t serial, bool force) {
+void WaylandWindow::RequestState(PlatformWindowDelegate::State state,
+                                 int64_t serial,
+                                 bool force) {
   LOG_IF(WARNING, in_flight_requests_.size() > 100u)
       << "The queue of configures is longer than 100!";
 
@@ -1111,8 +1115,8 @@ void WaylandWindow::ProcessSequencePoint(int64_t viz_seq) {
     // increase, since each request needs to produce a new sequence point. Any
     // requests that don't have a sequence id (-1) will be treated as done if
     // they have been applied. To latch a request, our sequence number must
-    // be larger than the request's sequence number.
-    if (i->viz_seq >= viz_seq && i->viz_seq != -1) {
+    // be greater than or equal to the request's sequence number.
+    if (i->viz_seq > viz_seq && i->viz_seq != -1) {
       break;
     }
     if (i->applied) {
@@ -1232,21 +1236,7 @@ void WaylandWindow::MaybeApplyLatestStateRequest(bool force) {
   auto old = applied_state_;
   applied_state_ = latest.state;
 
-  // Only set the sequence ID if this change will produce a frame.
-  // If it won't, we may wait indefinitely for a frame that will never come.
-  if (old.bounds_dip.size() != latest.state.bounds_dip.size() ||
-      old.size_px != latest.state.size_px ||
-      old.window_scale != latest.state.window_scale) {
-    latest.viz_seq = delegate()->InsertSequencePoint();
-  }
-
-  if (old.bounds_dip != latest.state.bounds_dip ||
-      old.size_px != latest.state.size_px ||
-      old.window_scale != latest.state.window_scale) {
-    bool origin_changed =
-        old.bounds_dip.origin() != latest.state.bounds_dip.origin();
-    delegate_->OnBoundsChanged({origin_changed});
-  }
+  latest.viz_seq = delegate()->OnStateUpdate(old, latest.state);
 
   // Latch in tests immediately if the test config is set.
   // Otherwise, such tests as interactive_ui_tests fail.
@@ -1255,14 +1245,4 @@ void WaylandWindow::MaybeApplyLatestStateRequest(bool force) {
   }
 }
 
-std::string WaylandWindow::State::ToString() const {
-  std::stringstream result;
-  result << "State {";
-  result << "bounds_dip = " << bounds_dip.ToString();
-  result << ", size_px = " << size_px.ToString();
-  result << ", window_scale = " << window_scale;
-  result << "}";
-  return result.str();
-}
-
 }  // namespace ui

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

@@ -254,33 +254,15 @@ class WaylandWindow : public PlatformWindow,
   // frames. Updating state based on configure is handled separately to this.
   void OnSurfaceConfigureEvent();
 
-  // State describes important data about this window, for example data that
-  // needs to be synchronized and acked. We apply this state to the client
-  // (us) and wait for a frame to be produced matching this state. That frame
-  // is identified by the sequence id.
-  struct State {
-    bool operator==(const State& rhs) const {
-      return std::tie(bounds_dip, size_px, window_scale) ==
-             std::tie(rhs.bounds_dip, rhs.size_px, rhs.window_scale);
-    }
-
-    // Bounds in DIP.
-    gfx::Rect bounds_dip;
-    // Size in pixels. Note that it's required to keep information in both DIP
-    // and pixels since it is not always possible to convert between them.
-    gfx::Size size_px;
-    // Current scale factor of the output where the window is located at.
-    float window_scale = 1.0;
-    // TODO(crbug.com/1395267): Add window states here.
-
-    std::string ToString() const;
-  };
-
   // See comments on the member variable for an explanation of this.
-  const State& applied_state() const { return applied_state_; }
+  const PlatformWindowDelegate::State& applied_state() const {
+    return applied_state_;
+  }
 
   // See comments on the member variable for an explanation of this.
-  const State& latched_state() const { return latched_state_; }
+  const PlatformWindowDelegate::State& latched_state() const {
+    return latched_state_;
+  }
 
   // Tells if the surface has already been configured. This will be true after
   // the first set of configure event and ack request, meaning that wl_surface
@@ -418,16 +400,19 @@ class WaylandWindow : public PlatformWindow,
 
   // Requests the given state via RequestState, given that this was a server
   // initiated change (e.g. configure).
-  void RequestStateFromServer(State state, int64_t serial);
+  void RequestStateFromServer(PlatformWindowDelegate::State state,
+                              int64_t serial);
 
   // Requests the given state via RequestState, given that this was a client
   // initiated change.
-  void RequestStateFromClient(State state);
+  void RequestStateFromClient(PlatformWindowDelegate::State state);
 
   // Requests the given state. If this request originates from a configure from
   // the server, specify |serial|. If |force| is true, the state will always be
   // applied, even if requests are being throttled.
-  void RequestState(State state, int64_t serial, bool force);
+  void RequestState(PlatformWindowDelegate::State state,
+                    int64_t serial,
+                    bool force);
 
   // Processes the given sequence point number. It will also latch and ack
   // the latest fulfilled in-flight request if it exists.
@@ -473,7 +458,7 @@ class WaylandWindow : public PlatformWindow,
 
   // Additional initialization of derived classes.
   virtual bool OnInitialize(PlatformWindowInitProperties properties,
-                            State* state) = 0;
+                            PlatformWindowDelegate::State* state) = 0;
 
   // Determines which keyboard shortcuts inhibition mode to be used and perform
   // required initialization steps, if any.
@@ -495,7 +480,7 @@ class WaylandWindow : public PlatformWindow,
   // came from a configure), or the viz sequence number.
   struct StateRequest {
     // State that has been requested.
-    State state;
+    PlatformWindowDelegate::State state;
 
     // Wayland serial number for acking a configure. This is -1 if there is no
     // serial number (e.g. from client initiated change).
@@ -627,12 +612,12 @@ class WaylandWindow : public PlatformWindow,
   //    the frame to come back
   // 4. Latched - the frame corresponding to this state came back, we can ack
   //    the configure if there was one
-  State applied_state_;
+  PlatformWindowDelegate::State applied_state_;
 
   // The current configuration state of the window. This is initially set to
   // values provided by the client, until we get an actual configure from the
   // server. See the comments on applied_state_ for further explanation.
-  State latched_state_;
+  PlatformWindowDelegate::State latched_state_;
 
   // In-flight state requests. Once a frame comes from the GPU
   // process with the appropriate viz sequence number, ack_configure request

ui/ozone/platform/wayland/test/mock_wayland_platform_window_delegate.cc

@@ -32,8 +32,15 @@ MockWaylandPlatformWindowDelegate::CreateWaylandWindow(
   return window;
 }
 
-int64_t MockWaylandPlatformWindowDelegate::InsertSequencePoint() {
-  return viz_seq_++;
+int64_t MockWaylandPlatformWindowDelegate::OnStateUpdate(
+    const PlatformWindowDelegate::State& old,
+    const PlatformWindowDelegate::State& latest) {
+  if (old.bounds_dip != latest.bounds_dip || old.size_px != latest.size_px ||
+      old.window_scale != latest.window_scale) {
+    bool origin_changed = old.bounds_dip.origin() != latest.bounds_dip.origin();
+    OnBoundsChanged({origin_changed});
+  }
+  return ++viz_seq_;
 }
 
 }  // namespace ui

ui/ozone/platform/wayland/test/mock_wayland_platform_window_delegate.h

@@ -29,7 +29,8 @@ class MockWaylandPlatformWindowDelegate : public MockPlatformWindowDelegate {
   // MockPlatformWindowDelegate:
   gfx::Rect ConvertRectToPixels(const gfx::Rect& rect_in_dp) const override;
   gfx::Rect ConvertRectToDIP(const gfx::Rect& rect_in_pixels) const override;
-  int64_t InsertSequencePoint() override;
+  int64_t OnStateUpdate(const PlatformWindowDelegate::State& old,
+                        const PlatformWindowDelegate::State& latest) override;
 
   int64_t viz_seq() const { return viz_seq_; }
 

ui/platform_window/platform_window_delegate.cc

@@ -4,6 +4,8 @@
 
 #include "ui/platform_window/platform_window_delegate.h"
 
+#include <sstream>
+
 #include "base/notreached.h"
 #include "third_party/skia/include/core/SkPath.h"
 #include "ui/base/owned_window_anchor.h"
@@ -12,6 +14,17 @@
 
 namespace ui {
 
+std::string PlatformWindowDelegate::State::ToString() const {
+  std::stringstream result;
+  result << "State {";
+  result << "bounds_dip = " << bounds_dip.ToString();
+  result << ", size_px = " << size_px.ToString();
+  result << ", window_scale = " << window_scale;
+  result << ", raster_scale = " << raster_scale;
+  result << "}";
+  return result.str();
+}
+
 PlatformWindowDelegate::PlatformWindowDelegate() = default;
 
 PlatformWindowDelegate::~PlatformWindowDelegate() = default;
@@ -42,7 +55,8 @@ absl::optional<MenuType> PlatformWindowDelegate::GetMenuType() {
 void PlatformWindowDelegate::OnOcclusionStateChanged(
     PlatformWindowOcclusionState occlusion_state) {}
 
-int64_t PlatformWindowDelegate::InsertSequencePoint() {
+int64_t PlatformWindowDelegate::OnStateUpdate(const State& old,
+                                              const State& latest) {
   NOTREACHED();
   return -1;
 }

ui/platform_window/platform_window_delegate.h

@@ -86,6 +86,33 @@ class COMPONENT_EXPORT(PLATFORM_WINDOW) PlatformWindowDelegate {
 #endif  // BUILDFLAG(IS_FUCHSIA)
   };
 
+  // State describes important data about this window, for example data that
+  // needs to be synchronized and acked. We apply this state to the client
+  // (us) and wait for a frame to be produced matching this state. That frame
+  // is identified by the sequence id.
+  // This is used by OnStateChanged and currently only by ozone/wayland.
+  struct COMPONENT_EXPORT(PLATFORM_WINDOW) State {
+    bool operator==(const State& rhs) const {
+      return std::tie(bounds_dip, size_px, window_scale, raster_scale) ==
+             std::tie(rhs.bounds_dip, rhs.size_px, rhs.window_scale,
+                      rhs.raster_scale);
+    }
+
+    // Bounds in DIP.
+    gfx::Rect bounds_dip;
+    // Size in pixels. Note that it's required to keep information in both DIP
+    // and pixels since it is not always possible to convert between them.
+    gfx::Size size_px;
+    // Current scale factor of the output where the window is located at.
+    float window_scale = 1.0;
+    // TODO(crbug.com/1395267): Add window states here.
+
+    // Scale to raster the window at.
+    float raster_scale = 1.0;
+
+    std::string ToString() const;
+  };
+
   PlatformWindowDelegate();
   virtual ~PlatformWindowDelegate();
 
@@ -160,12 +187,13 @@ class COMPONENT_EXPORT(PLATFORM_WINDOW) PlatformWindowDelegate {
   virtual void OnOcclusionStateChanged(
       PlatformWindowOcclusionState occlusion_state);
 
-  // Requests a new LocalSurfaceId for the window tree of this platform window.
-  // Returns the currently set child id (not the new one, since that requires
-  // an asynchronous operation). Calling code can compare this value with
-  // the gfx::FrameData::seq value to see when viz has produced a frame at or
-  // after the (conceptually) inserted sequence point.
-  virtual int64_t InsertSequencePoint();
+  // Updates state for clients that need sequence point synchronized
+  // PlatformWindowDelegate::State operations. In particular, this requests a
+  // new LocalSurfaceId for the window tree of this platform window. It returns
+  // the new parent ID. Calling code can compare this value with the
+  // gfx::FrameData::seq value to see when viz has produced a frame at or after
+  // the (conceptually) inserted sequence point.
+  virtual int64_t OnStateUpdate(const State& old, const State& latest);
 
   // Returns optional information for owned windows that require anchor for
   // positioning. Useful for such backends as Wayland as it provides flexibility