commit 4d3f57d9462d821e1b0caa9a7cbfa1b8867b6150
author Antonio Gomes <tonikitoo@igalia.com> Tue Apr 02 13:41:48 2024
committer Chromium LUCI CQ <chromium-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue Apr 02 13:41:48 2024
tree fe48ab932765925bf388c7486108a0bafee20e24
parent b3fad3aa312a86f21978ceb16e6740f6e586b108

[ozone/wayland] Pair up rendered content and requested window bounds

While investigating a black stripe that appears on the right border
of a lacros window while it is been resized horizontally from the
opposite border (left), it was figured that depending on the values
of the window `origin`, `size` and `scale factor`, the black line was
appearing or hidden.
A continuous interactive window resize is the perfect scenario to
replicate the bug intermittently, and give users the impression of
a flashing black line.

As an easy way to illustrate the issue, lets assume a lacros window
with the following bounds in DIPS, `330,0 596x664`, and a device scale
factor of  `1.62574` - these values can replicate the problem of the
vertical stripe appearing even without triggering a window resize.
For instance, this bounds in DIPS becomes `536,0 970x1080` in pixels
using DesktopWindowTreeHostPlatform::ConvertRectToPixels().

1.1) During the window creation, the `ui::Compositor` instance gets
its scale factor and size set in pixels.
It happens when `Compositor::SetScaleAndSize()` is called.
This is the stracktrace:
  ````
  #1 0x592718b9d713 base::debug::StackTrace::StackTrace()
  #2 0x5927205f4655 ui::Compositor::SetScaleAndSize()
  #3 0x5927205eac3e aura::WindowTreeHost::UpdateCompositorScaleAndSize()
  #4 0x5927205ec714 aura::WindowTreeHost::OnHostResizedInPixels()
  #5 0x592721d2dc2e aura::WindowTreeHostPlatform::OnBoundsChanged()
  #6 0x592721d2cc40 views::DesktopWindowTreeHostLacros::OnBoundsChanged()
  #7 0x592721d2e1b9 aura::WindowTreeHostPlatform::OnStateUpdate()
  #8 0x592719b83731 ui::WaylandWindow::MaybeApplyLatestStateRequest()
  #9 0x592719b83074 ui::WaylandWindow::RequestState()
  #10 0x592719b7e189 ui::WaylandWindow::SetWindowScale()
  #11 0x592719b7d985 ui::WaylandWindow::UpdateWindowScale()
  #12 0x592719b789ad ui::WaylandToplevelWindow::UpdateWindowScale()
  #13 0x592719b77f15 ui::WaylandToplevelWindow::Show()
  #14 0x592721d3139d views::DesktopWindowTreeHostPlatform::Show()
  #15 0x592721d18b24 views::DesktopNativeWidgetAura::Show()
  #16 0x592721cd0a7c views::Widget::Show()
  #17 0x592724d06ffb BrowserView::Show()
  (...)
  ````
In practice, `aura::WindowTreeHostPlatform::OnBoundsChanged()`
(frame #4) calls out to `WaylandWindow::GetBoundsInPixels()`,
that translates `330,0 596x664` in DIPS to `536,0 970x1080`
in pixels - see the method below.
Ultimately, only the size is set to the ui::Compositor instance,
ie `970x1080`.

  ````
  void WindowTreeHostPlatform::OnBoundsChanged(const BoundsChange& change) {
    (...)
    float current_scale = compositor()->device_scale_factor();
    float new_scale = ui::GetScaleFactorForNativeView(window());
    auto weak_ref = GetWeakPtr();
    auto new_size = GetBoundsInPixels().size(); <------
  `````

1.2) Meanwhile, `cc::Layer` instances for the window are also being
created. During the layer tree creation, the layers' bounds are all
set in DIPs.
For instance, the root layer is created with an origin of `0,0` and
bounds `596x664`, eg:

  ````
  2024-03-11T18:42:45.387311Z WARNING chrome[29500:29500]: [layer.cc(390)]
  #0 0x59271e998ce2 base::debug::CollectStackTrace()
  #1 0x592718b9d713 base::debug::StackTrace::StackTrace()
  #2 0x592725cc29d7 cc::Layer::SetBounds()
  #3 0x592725e2ee96 ui::Layer::SetBoundsFromAnimation()
  #4 0x5927265c0177 ui::LayerAnimator::SetBounds()
  #5 0x5927205d8fe0 aura::Window::SetBoundsInternal()
  #6 0x5927205d8f67 aura::Window::SetBounds()
  #7 0x5927205ea78e aura::WindowTreeHost::UpdateRootWindowSize()
  #8 0x5927205ea6df aura::WindowTreeHost::InitHost()
  #9 0x592721d30b36 views::DesktopWindowTreeHostPlatform::Init()
  #10 0x592721d16336 views::DesktopNativeWidgetAura::InitNativeWidget()
  #11 0x592725142c44 DesktopBrowserFrameAura::InitNativeWidget()
  #12 0x592721ccdd41 views::Widget::Init()
  #13 0x592724cf68af BrowserFrame::InitBrowserFrame()
  #14 0x592724db9f30 BrowserWindow::CreateBrowserWindow()
  #15 0x592724951ce0 Browser::Browser()
  #16 0x5927249511c6 Browser::Create()
  (...)
  ````
The layers' bounds get translated at places like
`draw_property_utils.cc` `ComputeLocalRectInTargetSpace()`, with
MathUtils::MapXXX() functions.
In practice, `596x664` translates to `968.942x1079.49`, and finally
gets rounded to `969x1080`.
Hence, we have a root ui::Compositor size of 970x1080 and a root
cc:Layer bounds of 969x1080.

Using the visual debugger tool attached to lacros, one can see that
all window-wide tiles's width are 969 [1].
OTOH, with the visual debugger tool attached to ash/chrome, we can
see that the width the the root surface (lacros) is 970 [2].
This 1px different creates a "punch role" effect in the lacros window,
and user sees whatever is underneath it rendered [3]. In the case of
this bug, what the user sees is a trailing 1px wide line part of
the so called "resize shadow".
The user sees it through this unintentionally "punch hole" explained
above.

This CL changes the way PlatformWindowDelegate::State::size_px
variable gets set in WaylandWindow::RequestState(), translating only
its size from DIPs to pixels. This way we match how this variable is
used in ui::Compositor level and cc::Layers et al (tiles, quads,
overlayers, etc).

As a way to illustrate the problem, see video [4]. It forcibly paints
the 1px lacros root layer transparent buffer red with viz_debugger,
so  one can clearly see it "flashing" due to the rounding error
being fixed here.

[1] https://issues.chromium.org/u/0/action/issues/40876438/attachments/54873934
[2] https://issues.chromium.org/u/0/action/issues/40876438/attachments/54878944
[3] https://issues.chromium.org/u/0/action/issues/40876438/attachments/54873933
[4] https://issues.chromium.org/action/issues/40876438/attachments/54936907

Bug: 40876438
Change-Id: Id36476d41e7a2c90f8a44337731a4cfad93e6a13
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5384752
Reviewed-by: Maksim Sisov <msisov@igalia.com>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1281135}