ozone/wayland: pass scale GPU scale to browser. patch 3/* [chromium/src : main]

Maksim Sisov (Gerrit)

unread,

May 24, 2021, 9:37:51 AM5/24/21

to Kramer Ge, Robert Kroeger, ipc-securi...@chromium.org, ozone-...@chromium.org

Attention is currently required from: Kramer Ge, Robert Kroeger.

Maksim Sisov would like Kramer Ge and Robert Kroeger to review this change.

ozone/wayland: pass scale GPU scale to browser. patch 3/*

This change doesn't bring any functional changes except that
it passes the current scale factor used by GPU. My next CL
will do plumbing.

Problem: Ozone/Wayland listens to display(wl_output) leave/enter
events and update buffer scale immediately when one changes.
However, GPU continues to use the previous buffer scale, which
means the Wayland compositor gets wrong scale factor while the
same buffer size is maintained. Thus, it starts to treat the
size of the buffer incorrectly.

To fix the problem, pass the currently used scale factor of the
GPU resources back to browser so that it can later reuse the same
scale factor when buffers are commited.

However, it's worth noting that buffer scale and viewport scale
factors can be different. The buffer scale is determined by
the time the buffer is initialized and cannot be changed. The
viewport scale is determined by the current scale factor of the
GLSurface.

The example when these two scale factors are different is the
resizing operation. Due to resizing optimization work, buffers
can be created with a bigger size than needed and the size of
the buffers is maintained for additional 1 second if size
of the surface doesn't change. The same happens when the surface
is dragged from one display that has one scale factor to
another display with a different scale factor - the size
in pixels changes, a bigger buffer is allocated (or not if
the buffer has already been big enough like if scale changes
from 2 to 1), scale factor of GLSurface changes and an
overlay is committed. This overlay is going to result in committing
buffer that has had scale determined at construction time, but
viewport's size and scale will be determined by the size and the
scale of the GLSurface when an overlay is committed.

This doesn't apply to WaylandCanvasSurface as the canvas is
constantly resized on resize operation and viewport scale and
buffer's scale is the same (maybe optimization is needed).

Bug: 1198965
Change-Id: I19b8f1d644de434c1ab3b37e05587580f53e4e20
---
M ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.cc
M ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.h
M ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.cc
M ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.h
M ui/ozone/platform/wayland/gpu/gl_surface_egl_readback_wayland.cc
M ui/ozone/platform/wayland/gpu/gl_surface_egl_readback_wayland.h
M ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc
M ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.h
M ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc
M ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc
M ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h
M ui/ozone/platform/wayland/host/wayland_window_unittest.cc
M ui/ozone/platform/wayland/wayland_buffer_manager_unittest.cc
M ui/ozone/public/mojom/wayland/wayland_buffer_manager.mojom
M ui/ozone/public/mojom/wayland/wayland_overlay_config.mojom
15 files changed, 238 insertions(+), 107 deletions(-)

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Sisov (Gerrit)

unread,

May 24, 2021, 9:38:00 AM5/24/21

to ipc-securi...@chromium.org, ozone-...@chromium.org, Robert Kroeger, Kramer Ge, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Kramer Ge, Robert Kroeger.

Patch set 2:-Commit-Queue

View Change

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Sisov (Gerrit)

unread,

May 25, 2021, 3:40:02 AM5/25/21

to ipc-securi...@chromium.org, ozone-...@chromium.org

Attention is currently required from: Kramer Ge, Robert Kroeger.

Maksim Sisov uploaded patch set #5 to this change.

View Change

ozone/wayland: pass scale GPU scale to browser. patch 3/*

This change doesn't bring any functional changes except that
it passes the current scale factor used by GPU. My next CL
will do plumbing.

Problem: Ozone/Wayland listens to display(wl_output) leave/enter
events and update buffer scale immediately when one changes.
However, GPU continues to use the previous buffer scale, which
means the Wayland compositor gets wrong scale factor while the
same buffer size is maintained. Thus, it starts to treat the
size of the buffer incorrectly.

To fix the problem, pass the currently used scale factor of the
GPU resources back to browser so that it can later reuse the same

scale factor when buffers are committed.

M ui/ozone/platform/wayland/fuzzer/wayland_buffer_fuzzer.cc


M ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.cc
M ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.h
M ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.cc
M ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.h
M ui/ozone/platform/wayland/gpu/gl_surface_egl_readback_wayland.cc
M ui/ozone/platform/wayland/gpu/gl_surface_egl_readback_wayland.h
M ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc
M ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.h
M ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc
M ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc
M ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h
M ui/ozone/platform/wayland/host/wayland_window_unittest.cc
M ui/ozone/platform/wayland/wayland_buffer_manager_unittest.cc
M ui/ozone/public/mojom/wayland/wayland_buffer_manager.mojom
M ui/ozone/public/mojom/wayland/wayland_overlay_config.mojom

16 files changed, 240 insertions(+), 108 deletions(-)

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Robert Kroeger (Gerrit)

unread,

May 31, 2021, 2:58:12 PM5/31/21

to Maksim Sisov, ipc-securi...@chromium.org, ozone-...@chromium.org, Tricium, Kramer Ge, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Maksim Sisov, Kramer Ge.

View Change

18 comments:

Commit Message:
- Patch Set #5, Line 10: it passes the current scale factor used by GPU. My next CL
  GPU is "the GPU process"?
- Patch Set #5, Line 14: events and update buffer scale immediately when one changes.
  nit: and updates buffer
  nit: when it changes
- Patch Set #5, Line 16: means the Wayland compositor gets wrong scale factor while the
  nit: gets the wrong scale
- Patch Set #5, Line 24: However, it's worth noting that buffer scale and viewport scale
  nit: that the buffer
- Patch Set #5, Line 25: factors can be different. The buffer scale is determined by
  nit: is determined at
- Patch Set #5, Line 26: the time the buffer is initialized and cannot be changed. The
  nit: the time that the
- Patch Set #5, Line 33: the buffers is maintained for additional 1 second if size
  nit: for an additional
  nit: if the size
- Patch Set #5, Line 34: of the surface doesn't change. The same happens when the surface
  I'd not expected that we pixel stretch in the resize case? (Except many in video?)
- Patch Set #5, Line 39: from 2 to 1), scale factor of GLSurface changes and an
  nit: the scale factor of the GLSurface
- Patch Set #5, Line 41: buffer that has had scale determined at construction time, but
  nit: a buffer
  nit: that has had its scale
- Patch Set #5, Line 42: viewport's size and scale will be determined by the size and the
  nit: the viewport's
File ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.h:
- Patch Set #5, Line 88: // Scale factor of the buffer. Determined by the surface this buffer is
  My understanding of a scale factor is that it's a multiplicative transform between two coordinate systems. Am I understanding? Can you define the coordinate systems of the scale factor? i.e. scale w.r.t. to the pixel scale of the Display backing this GBM buffer?
File ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.cc:
- Patch Set #5, Line 176: GetGbmSurfacelessWaylandWidget(buffer_manager_, widget)
  This is getting hard to skim. While you're here... consider using some temporary variables to make it easier to read.
File ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc:
- Patch Set #5, Line 192: int32_t viewport_scale,
  What is the viewport_scale vs buffer_scale?
File ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h:
- Patch Set #5, Line 54: const int32_t buffer_scale;
  default value?
- Patch Set #5, Line 209: int32_t buffer_scale,
  Can you add a method comment?
File ui/ozone/public/mojom/wayland/wayland_buffer_manager.mojom:
- Patch Set #5, Line 42: int32 buffer_scale,
  Needs an appropriate comment.
- Patch Set #5, Line 61: int32 buffer_scale,
  ditto: comment.

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Kramer Ge (Gerrit)

unread,

May 31, 2021, 3:37:38 PM5/31/21

to Maksim Sisov, ipc-securi...@chromium.org, ozone-...@chromium.org, Tricium, Robert Kroeger, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Maksim Sisov.

View Change

1 comment:

Commit Message:
- Patch Set #5, Line 31: resizing operation. Due to resizing optimization work, buffers
  I'm not sure what it means that buffer_scale and viewport_scale are different during resize. Buffers should be cropped when resize happens but buffer_scale/viewport_scale don't change.
  I'm also not sure that buffer_scale and viewport_scale will differentiate when window moves to a different wl_output. I expect that once viewport_scale is changed, buffer is recreated to with buffer_scale=viewport_scale.

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Sisov (Gerrit)

unread,

Jun 1, 2021, 9:13:23 AM6/1/21

to ipc-securi...@chromium.org, ozone-...@chromium.org

Attention is currently required from: Maksim Sisov.

Maksim Sisov uploaded patch set #6 to this change.

View Change

ozone/wayland: pass scale GPU scale to browser. patch 3/*

This change doesn't bring any functional changes except that

it passes the current scale factor used by GPU. My next CL

will do plumbing.

Problem: Ozone/Wayland listens to display(wl_output) leave/enter

events and updates buffer scale immediately when it changes.


However, GPU continues to use the previous buffer scale, which

means the Wayland compositor gets the wrong scale factor while the


same buffer size is maintained. Thus, it starts to treat the
size of the buffer incorrectly.

To fix the problem, pass the currently used scale factor of the
GPU resources back to browser so that it can later reuse the same
scale factor when buffers are committed.

However, it's worth noting that the buffer scale and viewport scale
factors can be different. The buffer scale is determined at
the time that the the buffer is initialized and cannot be changed. The


viewport scale is determined by the current scale factor of the
GLSurface.

The example when these two scale factors are different is the

resizing operation. Due to resizing optimization work, buffers

can be created with a bigger size than needed and the size of

the buffers is maintained for an additional 1 second if the size


of the surface doesn't change. The same happens when the surface

is dragged from one display that has one scale factor to
another display with a different scale factor - the size
in pixels changes, a bigger buffer is allocated (or not if
the buffer has already been big enough like if scale changes

from 2 to 1), scale factor of the GLSurface changes and an


overlay is committed. This overlay is going to result in committing

a buffer that has had its scale determined at construction time, but
the viewport's size and scale will be determined by the size and the


scale of the GLSurface when an overlay is committed.

This doesn't apply to WaylandCanvasSurface as the canvas is
constantly resized on resize operation and viewport scale and
buffer's scale is the same (maybe optimization is needed).

Bug: 1198965
Change-Id: I19b8f1d644de434c1ab3b37e05587580f53e4e20
---
M ui/ozone/platform/wayland/fuzzer/wayland_buffer_fuzzer.cc
M ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.cc
M ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.h
M ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.cc
M ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.h
M ui/ozone/platform/wayland/gpu/gl_surface_egl_readback_wayland.cc
M ui/ozone/platform/wayland/gpu/gl_surface_egl_readback_wayland.h
M ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc
M ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.h
M ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc
M ui/ozone/platform/wayland/host/wayland_buffer_manager_host.cc
M ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h
M ui/ozone/platform/wayland/host/wayland_window_unittest.cc
M ui/ozone/platform/wayland/wayland_buffer_manager_unittest.cc
M ui/ozone/public/mojom/wayland/wayland_buffer_manager.mojom
M ui/ozone/public/mojom/wayland/wayland_overlay_config.mojom
16 files changed, 240 insertions(+), 108 deletions(-)

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Sisov (Gerrit)

unread,

Jun 1, 2021, 9:49:51 AM6/1/21

to ipc-securi...@chromium.org, ozone-...@chromium.org, Tricium, Robert Kroeger, Kramer Ge, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Kramer Ge, Robert Kroeger.

View Change

19 comments:

Commit Message:
- Patch Set #5, Line 10: it passes the current scale factor used by GPU. My next CL

- GPU is "the GPU process"?

- correct.

- Patch Set #5, Line 14: events and update buffer scale immediately when one changes.

- nit: and updates buffer […]
  Done

- Patch Set #5, Line 16: means the Wayland compositor gets wrong scale factor while the
  nit: gets the wrong scale

- Done
- Patch Set #5, Line 24: However, it's worth noting that buffer scale and viewport scale
  nit: that the buffer
  Done

- Patch Set #5, Line 25: factors can be different. The buffer scale is determined by
  nit: is determined at

- Done

- Patch Set #5, Line 26: the time the buffer is initialized and cannot be changed. The
  nit: the time that the

- Done

- Patch Set #5, Line 31: resizing operation. Due to resizing optimization work, buffers

- I'm not sure what it means that buffer_scale and viewport_scale are different during resize. […]
  They are different during scale changes (which changes when a surface is dragged between displays with different scale factors).
  It's because the way how DirectRenderer calculates the viewport size [1]. It assumes resize happened, it checks current surface size and requested size and will figure out that the current buffer size is way bigger than the requested on and will continue using the same buffers that were created when the surface was on a display with scale == 2 until last_viewport_resize_time_ is >= 1 second.
  However, GLSurface::Resize will be called with a new surface scale factor already.
  [1] https://source.chromium.org/chromium/chromium/src/+/main:components/viz/service/display/direct_renderer.cc;l=933?q=direct_renderer.cc&ss=chromium.

- Patch Set #5, Line 33: the buffers is maintained for additional 1 second if size

- nit: for an additional […]
  Done

- Patch Set #5, Line 34: of the surface doesn't change. The same happens when the surface
  I'd not expected that we pixel stretch in the resize case? (Except many in video?)

- we don't stretch, but scale and crop instead.

- Patch Set #5, Line 39: from 2 to 1), scale factor of GLSurface changes and an

- nit: the scale factor of the GLSurface
  Done
- Patch Set #5, Line 41: buffer that has had scale determined at construction time, but
  nit: a buffer […]
  Done
- Patch Set #5, Line 42: viewport's size and scale will be determined by the size and the
  nit: the viewport's
  Done
File ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.h:

- Patch Set #5, Line 88: // Scale factor of the buffer. Determined by the surface this buffer is

- My understanding of a scale factor is that it's a multiplicative transform between two coordinate sy […]
  precisely. it's value is determined by the display a surface is located at. eg - surface is located on a display with scale == 1, then the buffer scale will be 1 as well. if the surface is moved to a display 2 with scale == 3, then the buffer scale will be 3 as well as all the resources will be recreated when ui::Compositor sees the scale factor has been changed.
File ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.cc:

- Patch Set #5, Line 176: GetGbmSurfacelessWaylandWidget(buffer_manager_, widget)

- This is getting hard to skim. While you're here... […]
  Done
File ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc:

- Patch Set #5, Line 192: int32_t viewport_scale,
  What is the viewport_scale vs buffer_scale?

- viewport_scale is the current scale factor of the surface determined by a display it's located at. the buffer scale is the scale factor of the buffer that is determined by the scale factor of a surface for which the buffer is created at the creation time.
File ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h:

- Patch Set #5, Line 54: const int32_t buffer_scale;
  default value?

- Done

- Patch Set #5, Line 209: int32_t buffer_scale,
  Can you add a method comment?

- Done
File ui/ozone/public/mojom/wayland/wayland_buffer_manager.mojom:

- Patch Set #5, Line 42: int32 buffer_scale,
  Needs an appropriate comment.

- Done

- Patch Set #5, Line 61: int32 buffer_scale,
  ditto: comment.

- Done

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Kramer Ge (Gerrit)

unread,

Jun 1, 2021, 12:20:16 PM6/1/21

to Maksim Sisov, ipc-securi...@chromium.org, ozone-...@chromium.org, Tricium, Robert Kroeger, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Maksim Sisov, Robert Kroeger.

View Change

1 comment:

Commit Message:
- Patch Set #5, Line 31: resizing operation. Due to resizing optimization work, buffers
  They are different during scale changes (which changes when a surface is dragged between displays wi […]
  Can you take me through the below example?
  In my understanding, if a window of 100x100 dip moved from an output of scale=2 to an output of scale=1, so the current surface size is 200x200 px, and the requested size is 100x100 px. Since "requested" is smaller than "current", won't it draw 100x100 px region out of the 200x200 px "current"? And in that sense, the 100x100 px drawn region corresponds to the 100x100 dip that will be displayed so buffer scale have changed from 2 to 1 as well?

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Robert Kroeger (Gerrit)

unread,

Jun 1, 2021, 1:19:27 PM6/1/21

to Maksim Sisov, ipc-securi...@chromium.org, ozone-...@chromium.org, Tricium, Kramer Ge, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Maksim Sisov.

View Change

12 comments:

File ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.h:
- Patch Set #7, Line 90: // initialization and mustn't be changed later.
  Can you make buffer_scale_ be const?
File ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.h:

- Patch Set #5, Line 88: // Scale factor of the buffer. Determined by the surface this buffer is

- precisely. it's value is determined by the display a surface is located at. […]
  It would be great to add this to the comment in the code as it makes the buffer_scale_ completely clear.
File ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.h:
- Patch Set #7, Line 149: // Scale factor of the current surface.
  This is mutable unless the buffer scale right?
File ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc:

- Patch Set #5, Line 192: int32_t viewport_scale,

- viewport_scale is the current scale factor of the surface determined by a display it's located at. […]
  Can the viewport_scale change once the surface is created? It can right? As a surface is moved to a different display. buffer_scale is immutable right?
  So it's conceivable that the buffer here is not of "best" resolution for the display it's being used on?
File ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc:
- Patch Set #7, Line 60: bool Initialize(const gfx::Size& size, int32_t scale) {
  should be buffer_scale right?
File ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h:
- Patch Set #7, Line 213: // stores it into the |anonymous_buffers_|.
  Please add to the comment that the buffer_scale is immutable and that size (presumably?) is in pixels?
File ui/ozone/public/mojom/wayland/wayland_buffer_manager.mojom:
- Patch Set #7, Line 39: // reassign it to another AcceleratedWidget. |buffer_scale| is an additional
  nit: is additional
- Patch Set #7, Line 40: // information that must be passed to Wayland compositor so that it is aware
  nit: to the Wayland
- Patch Set #7, Line 42: // rendering the surface on an output with scale factor > 1.
  If buffer_scale doesn't match the display where the buffer is being displayed, what happens? Presumably pixel stretching/squashing?
- Patch Set #7, Line 60: // reassign it to another AcceleratedWidget. |buffer_scale| is an additional
  nit: is additional
- Patch Set #7, Line 61: // information that must be passed to Wayland compositor so that it is aware
  nit: to the Wayland
File ui/ozone/public/mojom/wayland/wayland_overlay_config.mojom:
- Patch Set #7, Line 25: // Scale factor of the viewport, which can be used to translate |bounds_rect|
  Can you note the relationship between bounds_rect, buffer_scale as found below and viewport_scale?

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Sisov (Gerrit)

unread,

Jun 2, 2021, 7:24:35 AM6/2/21

to ipc-securi...@chromium.org, ozone-...@chromium.org

Attention is currently required from: Maksim Sisov.

Maksim Sisov uploaded patch set #9 to this change.

View Change

ozone/wayland: pass scale GPU scale to browser. patch 3/*

This change doesn't bring any functional changes except that

it passes the current scale factor used by GPU. My next CL

will do plumbing.

Problem: Ozone/Wayland listens to display(wl_output) leave/enter
events and updates buffer scale immediately when it changes.
However, GPU continues to use the previous buffer scale, which
means the Wayland compositor gets the wrong scale factor while the
same buffer size is maintained. Thus, it starts to treat the
size of the buffer incorrectly.

To fix the problem, pass the currently used scale factor of the

GPU surface back to browser so that it can later reuse the same


scale factor when buffers are committed.

Bug: 1198965
Change-Id: I19b8f1d644de434c1ab3b37e05587580f53e4e20
---
M ui/ozone/platform/wayland/fuzzer/wayland_buffer_fuzzer.cc
M ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.cc

M ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.cc
M ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.h
M ui/ozone/platform/wayland/gpu/gl_surface_egl_readback_wayland.cc
M ui/ozone/platform/wayland/gpu/gl_surface_egl_readback_wayland.h
M ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc
M ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.h
M ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc

M ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h
M ui/ozone/platform/wayland/host/wayland_window_unittest.cc
M ui/ozone/platform/wayland/wayland_buffer_manager_unittest.cc
M ui/ozone/public/mojom/wayland/wayland_buffer_manager.mojom
M ui/ozone/public/mojom/wayland/wayland_overlay_config.mojom

14 files changed, 149 insertions(+), 70 deletions(-)

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Sisov (Gerrit)

unread,

Jun 2, 2021, 7:36:15 AM6/2/21

to ipc-securi...@chromium.org, ozone-...@chromium.org, Tricium, Robert Kroeger, Kramer Ge, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Kramer Ge, Robert Kroeger.

Patch set 11:Commit-Queue +1

View Change

13 comments:

Commit Message:

- Patch Set #5, Line 31: resizing operation. Due to resizing optimization work, buffers

- Can you take me through the below example? […]
  As we discussed, splitting the buffer scale is not required as viewport is used in any case.
File ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.h:

- Patch Set #7, Line 90: // initialization and mustn't be changed later.
  Can you make buffer_scale_ be const?

- change reverted
File ui/ozone/platform/wayland/gpu/gbm_pixmap_wayland.h:

- Patch Set #5, Line 88: // Scale factor of the buffer. Determined by the surface this buffer is

- It would be great to add this to the comment in the code as it makes the buffer_scale_ completely cl […]
  I reverted some changes. Please review again.
File ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.h:

- Patch Set #7, Line 149: // Scale factor of the current surface.
  This is mutable unless the buffer scale right?

- gbm_pixmap_wayland no longer stores a buffer scale.
  and yes, it's mutable.
File ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc:

- Patch Set #5, Line 192: int32_t viewport_scale,

- Can the viewport_scale change once the surface is created? It can right? As a surface is moved to a […]
  indeed, buffer scale can be not of the best one. However, as we discussed with Kramer, during resize/scale change, viewport scale/crop is used, which means setting buffer scale is not necessary (so it's reset instead). Thus, during each Commitbuffer/CommitOverlay call, the current GPU surface scale is passed. please see the latest patch set.
File ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc:

- Patch Set #7, Line 60: bool Initialize(const gfx::Size& size, int32_t scale) {
  should be buffer_scale right?

- change reverted.
File ui/ozone/platform/wayland/host/wayland_buffer_manager_host.h:

- Patch Set #7, Line 213: // stores it into the |anonymous_buffers_|.

- Please add to the comment that the buffer_scale is immutable and that size (presumably?) is in pixel […]
  change reverted
File ui/ozone/public/mojom/wayland/wayland_buffer_manager.mojom:

- Patch Set #7, Line 39: // reassign it to another AcceleratedWidget. |buffer_scale| is an additional
  nit: is additional

- change reverted

- Patch Set #7, Line 40: // information that must be passed to Wayland compositor so that it is aware
  nit: to the Wayland

- change reverted

- Patch Set #7, Line 42: // rendering the surface on an output with scale factor > 1.

- If buffer_scale doesn't match the display where the buffer is being displayed, what happens? Presuma […]
  change reverted

- Patch Set #7, Line 60: // reassign it to another AcceleratedWidget. |buffer_scale| is an additional
  nit: is additional

- change reverted

- Patch Set #7, Line 61: // information that must be passed to Wayland compositor so that it is aware
  nit: to the Wayland

- change reverted
File ui/ozone/public/mojom/wayland/wayland_overlay_config.mojom:

- Patch Set #7, Line 25: // Scale factor of the viewport, which can be used to translate |bounds_rect|
  Can you note the relationship between bounds_rect, buffer_scale as found below and viewport_scale?

- I changed it to surface_scale_factor instead.

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Robert Kroeger (Gerrit)

unread,

Jun 2, 2021, 3:04:19 PM6/2/21

to Maksim Sisov, ipc-securi...@chromium.org, ozone-...@chromium.org, Tricium, Kramer Ge, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Maksim Sisov, Kramer Ge.

Patch set 11:Code-Review +1

View Change

1 comment:

File ui/ozone/public/mojom/wayland/wayland_overlay_config.mojom:
- Patch Set #11, Line 25: // Scale factor of the GPU side surface with respect to a display where the
  That's clearer. Thanks.

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Kramer Ge (Gerrit)

unread,

Jun 2, 2021, 4:05:06 PM6/2/21

to Maksim Sisov, ipc-securi...@chromium.org, ozone-...@chromium.org, Robert Kroeger, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Maksim Sisov.

Patch set 11:Code-Review +1

View Change

2 comments:

Commit Message:

- Patch Set #5, Line 31: resizing operation. Due to resizing optimization work, buffers
  Can you take me through the below example? […]

- Thanks, we can try to group/synchronize state changes and make it simpler in the WBMH refactor.
Patchset:
- Patch Set #11:
  LGTM

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Sisov (Gerrit)

unread,

Jun 3, 2021, 1:45:18 AM6/3/21

to ipc-securi...@chromium.org, ozone-...@chromium.org, Kramer Ge, Robert Kroeger, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

View Change

1 comment:

Commit Message:

- Patch Set #5, Line 31: resizing operation. Due to resizing optimization work, buffers

- Thanks, we can try to group/synchronize state changes and make it simpler in the WBMH refactor.
  Indeed. We could probably make a pending state of a surface with pending buffer attach, pending damage, pending viewport/buffer_scale and then simply commit the state.

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Sisov (Gerrit)

unread,

Jun 3, 2021, 2:37:51 AM6/3/21

to ipc-securi...@chromium.org, ozone-...@chromium.org, Kramer Ge, Robert Kroeger, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Patch set 12:Commit-Queue +2

View Change

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Jun 3, 2021, 2:37:55 AM6/3/21

to Maksim Sisov, ipc-securi...@chromium.org, ozone-...@chromium.org, Kramer Ge, Robert Kroeger, Tricium, chromium...@chromium.org, Kalyan Kondapally

CL chromium-review.googlesource.com/2910337 must be approved before triggering CQ
Code Owners

View Change

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Sisov (Gerrit)

unread,

Jun 3, 2021, 2:59:50 AM6/3/21

to Tom Sepez, ipc-securi...@chromium.org, ozone-...@chromium.org, Kramer Ge, Robert Kroeger

Attention is currently required from: Tom Sepez.

Maksim Sisov would like Tom Sepez to review this change.

View Change

ozone/wayland: pass scale GPU scale to browser. patch 3/*

This change doesn't bring any functional changes except that
it passes the current scale factor used by GPU. My next CL
will do plumbing.

Problem: Ozone/Wayland listens to display(wl_output) leave/enter
events and updates buffer scale immediately when it changes.
However, GPU continues to use the previous buffer scale, which
means the Wayland compositor gets the wrong scale factor while the
same buffer size is maintained. Thus, it starts to treat the
size of the buffer incorrectly.

To fix the problem, pass the currently used scale factor of the
GPU surface back to browser so that it can later reuse the same
scale factor when buffers are committed.

Bug: 1198965
Change-Id: I19b8f1d644de434c1ab3b37e05587580f53e4e20
---

M ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.cc
M ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.h
M ui/ozone/platform/wayland/gpu/gl_surface_egl_readback_wayland.cc
M ui/ozone/platform/wayland/gpu/gl_surface_egl_readback_wayland.h
M ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc
M ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.h
M ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc

M ui/ozone/platform/wayland/wayland_buffer_manager_unittest.cc
M ui/ozone/public/mojom/wayland/wayland_overlay_config.mojom
9 files changed, 133 insertions(+), 62 deletions(-)

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Sisov (Gerrit)

unread,

Jun 3, 2021, 3:00:00 AM6/3/21

to ipc-securi...@chromium.org, ozone-...@chromium.org, Tom Sepez, Kramer Ge, Robert Kroeger, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Tom Sepez.

View Change

1 comment:

Patchset:
- Patch Set #12:
  Tom, please review the mojom file.

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Tom Sepez (Gerrit)

unread,

Jun 3, 2021, 1:05:30 PM6/3/21

to Maksim Sisov, ipc-securi...@chromium.org, ozone-...@chromium.org, Kramer Ge, Robert Kroeger, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Maksim Sisov.

View Change

2 comments:

Commit Message:
- Patch Set #12, Line 9:

his change doesn't bring any functional changes except that
it passes the current scale factor used by GPU. My next CL

- Generally, we don't like to sign off from a security perspective until we see all the places the data might flow. But this seems straightforward.
- Patch Set #12, Line 17:

 Thus, it starts to treat the
size of the buffer incorrectly.

- The question becomes, what if a compromised GPU process passes bad values, what's the worst that can happen? Out of bounds read/write in the other processes?

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Sisov (Gerrit)

unread,

Jun 4, 2021, 1:36:35 AM6/4/21

to ipc-securi...@chromium.org, ozone-...@chromium.org, Tom Sepez, Kramer Ge, Robert Kroeger, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Tom Sepez.

View Change

2 comments:

Commit Message:

Patch Set #12, Line 9:

his change doesn't bring any functional changes except that
it passes the current scale factor used by GPU. My next CL

- Generally, we don't like to sign off from a security perspective until we see all the places the dat […]
  Indeed. Sorry for that. I split the patches to make the review go smoother.
  As I explained in the other comment, the value is provided to [1]. And if something is wrong, the client is immediately disconnected.
  [1] https://source.chromium.org/chromium/chromium/src/+/main:third_party/wayland/include/protocol/wayland-client-protocol.h;l=3856?q=wl_surface_set_buffer_scale&ss=chromium%2Fchromium%2Fsrc

Patch Set #12, Line 17:

 Thus, it starts to treat the
size of the buffer incorrectly.

- The question becomes, what if a compromised GPU process passes bad values, what's the worst that can […]
  Let me explain :) when Wayland compositor receives a buffer attached to a surface, it also needs to know if the client is DPI aware. The size of the buffer is always in px - surface_size_dip * scale. The size of the surface is also determined by the size of the buffer which is buffer_size_px / scale (happens on the Wayland compositor side). If the scale passed by the client is arbitrary large, the size of the surface will be arbitrary small and vice versa. Passing negative or 0 values is a Wayland protocol error. And the client (chromium) will be disconnected by the compositor immediately and the browser will shutdown then [1].
  This code is not by any means new from the Ozone/Wayland point of view. We have already been providing Wayland with the scale factor used by the client, but it was sent back to the Wayland compositor too early (before GPU resources were allocated and new buffers with new size were attached. That resulted in a client surfaces becoming smaller than it supposed to be as surf_size = buf_size_px / scale.). So, this patch simply postpones setting the used scale factor and sets right after the GPU adopted it.
  [1] https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/host/wayland_event_watcher.cc;l=242

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Tom Sepez (Gerrit)

unread,

Jun 4, 2021, 4:23:01 PM6/4/21

to Maksim Sisov, ipc-securi...@chromium.org, ozone-...@chromium.org, Kramer Ge, Robert Kroeger, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Maksim Sisov.

Patch set 12:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #12:
  Thanks for the explanation.

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Maksim Sisov (Gerrit)

unread,

Jun 5, 2021, 1:47:49 AM6/5/21

to ipc-securi...@chromium.org, ozone-...@chromium.org, Tom Sepez, Kramer Ge, Robert Kroeger, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Kalyan Kondapally

Attention is currently required from: Maksim Sisov.

Patch set 12:Commit-Queue +2

View Change

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Jun 5, 2021, 3:05:02 AM6/5/21

to Maksim Sisov, ipc-securi...@chromium.org, ozone-...@chromium.org, Tom Sepez, Kramer Ge, Robert Kroeger, Tricium, chromium...@chromium.org, Kalyan Kondapally

Chromium LUCI CQ submitted this change.

View Change

Approvals:
  Robert Kroeger: Looks good to me
  Tom Sepez: Looks good to me
  Kramer Ge: Looks good to me
  Maksim Sisov: Commit

ozone/wayland: pass scale GPU scale to browser. patch 3/*

This change doesn't bring any functional changes except that


it passes the current scale factor used by GPU. My next CL

will do plumbing.

Problem: Ozone/Wayland listens to display(wl_output) leave/enter
events and updates buffer scale immediately when it changes.
However, GPU continues to use the previous buffer scale, which
means the Wayland compositor gets the wrong scale factor while the

same buffer size is maintained. Thus, it starts to treat the


size of the buffer incorrectly.

To fix the problem, pass the currently used scale factor of the
GPU surface back to browser so that it can later reuse the same
scale factor when buffers are committed.

Bug: 1198965
Change-Id: I19b8f1d644de434c1ab3b37e05587580f53e4e20

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2910337
Reviewed-by: Tom Sepez <tse...@chromium.org>
Reviewed-by: Robert Kroeger <rjkr...@chromium.org>
Reviewed-by: Kramer Ge <fang...@chromium.org>
Commit-Queue: Maksim Sisov <msi...@igalia.com>
Cr-Commit-Position: refs/heads/master@{#889552}


---
M ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.cc
M ui/ozone/platform/wayland/gpu/gbm_surfaceless_wayland.h
M ui/ozone/platform/wayland/gpu/gl_surface_egl_readback_wayland.cc
M ui/ozone/platform/wayland/gpu/gl_surface_egl_readback_wayland.h
M ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.cc
M ui/ozone/platform/wayland/gpu/wayland_buffer_manager_gpu.h
M ui/ozone/platform/wayland/gpu/wayland_canvas_surface.cc
M ui/ozone/platform/wayland/wayland_buffer_manager_unittest.cc
M ui/ozone/public/mojom/wayland/wayland_overlay_config.mojom
9 files changed, 133 insertions(+), 62 deletions(-)

To view, visit change 2910337. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward