[Wayland] Implement WaylandWpColorManagementSurface::OnPreferredChanged [chromium/src : main]
0 views
Skip to first unread message
Thomas Anderson (Gerrit)
Aug 21, 2025, 4:39:17 PMAug 21
to Kramer Ge, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge
New activity on the change![]( "Open in Gerrit")
Related details
Attention is currently required from:
- Kramer Ge
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Aug 22, 2025, 5:15:02 PMAug 22
to Kramer Ge, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge
Thomas Anderson added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Kramer Ge
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Aug 25, 2025, 2:49:43 PM (14 days ago) Aug 25
to Thomas Anderson, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Thomas Anderson
Kramer Ge added 3 comments![]( "Open in Gerrit")
File ui/aura/window_tree_host.cc
Line 671, Patchset 6 (Latest):
compositor_->SetDisplayColorSpaces(color_spaces->color_spaces());Kramer Ge . unresolved
Should this be set when `compositor_` is created also?
File ui/ozone/platform/wayland/host/wayland_wp_color_management_surface.cc
Line 86, Patchset 6 (Latest):
CHECK_EQ(image_description, image_description_);Kramer Ge . unresolved
Potentially we can be crashed by compositor sending multiple `preferred_changed` events?
Line 98, Patchset 6 (Latest):
uint32_t identity) {Kramer Ge . unresolved
we don't need `identity` here, is it because we only retain one description at a time?
Related details
Attention is currently required from:
- Thomas Anderson
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Aug 25, 2025, 4:36:30 PM (13 days ago) Aug 25
to Kramer Ge, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Kramer Ge
Thomas Anderson voted and added 3 comments![]( "Open in Gerrit")
Votes added by Thomas Anderson
| Commit-Queue | +1 |
3 comments
File ui/aura/window_tree_host.cc
Line 671, Patchset 6:
compositor_->SetDisplayColorSpaces(color_spaces->color_spaces());Kramer Ge . resolved
Should this be set when `compositor_` is created also?
Thomas Anderson
It is, on L574
File ui/ozone/platform/wayland/host/wayland_wp_color_management_surface.cc
Potentially we can be crashed by compositor sending multiple `preferred_changed` events?
Thomas Anderson
Yes, removed.
we don't need `identity` here, is it because we only retain one description at a time?
Thomas Anderson
We always recreate the WaylandWpImageDescription in this method, so `identity` isn't needed. Possibly it can be used to cache the description in a followup so a round-trip wouldn't be required.
Related details
Attention is currently required from:
- Kramer Ge
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Aug 25, 2025, 5:06:18 PM (13 days ago) Aug 25
to Thomas Anderson, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Thomas Anderson
Kramer Ge voted and added 1 comment![]( "Open in Gerrit")
Attention is currently required from:
- Thomas Anderson
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Aug 25, 2025, 5:23:11 PM (13 days ago) Aug 25
to Avi Drissman, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Avi Drissman
Thomas Anderson added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Attention is currently required from:
- Avi Drissman
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Avi Drissman (Gerrit)
Aug 25, 2025, 5:29:50 PM (13 days ago) Aug 25
to Thomas Anderson, Avi Drissman, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Thomas Anderson
Avi Drissman voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Thomas Anderson
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Aug 25, 2025, 5:33:17 PM (13 days ago) Aug 25
to Avi Drissman, Chromium LUCI CQ, chromium...@chromium.org, Sadrul Chowdhury, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Thomas Anderson voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Aug 25, 2025, 6:27:18 PM (13 days ago) Aug 25
to Thomas Anderson, Avi Drissman, Kramer Ge, chromium...@chromium.org, Sadrul Chowdhury, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[Wayland] Implement WaylandWpColorManagementSurface::OnPreferredChanged
* Move DisplayColorSpacesRef from Display private to a public class.
This is required Because DisplayColorSpaces is large and probably
shouldn't be copied.
* Add WindowTreeHost::display_color_spaces_ to hold the explicitly-set
DisplayColorSpaces.
* Move conversion code into
WaylandWpImageDescription::AsDisplayColorSpaces and reuse it for the
OnPreferredChanged callback.
Fixed: 375959958
Change-Id: I5885feefb7534de9a3a86fdd2f7f9752c6e559a3
Reviewed-by: Avi Drissman <a...@chromium.org>
Reviewed-by: Kramer Ge <fang...@chromium.org>
Commit-Queue: Thomas Anderson <thomasa...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1506174}
Files:
- M ui/aura/window_tree_host.cc
- M ui/aura/window_tree_host.h
- M ui/aura/window_tree_host_platform.cc
- M ui/aura/window_tree_host_platform.h
- M ui/display/display.cc
- M ui/display/display.h
- M ui/gfx/display_color_spaces.cc
- M ui/gfx/display_color_spaces.h
- M ui/ozone/platform/wayland/host/wayland_screen.cc
- M ui/ozone/platform/wayland/host/wayland_window.cc
- M ui/ozone/platform/wayland/host/wayland_window.h
- M ui/ozone/platform/wayland/host/wayland_wp_color_management_output.cc
- M ui/ozone/platform/wayland/host/wayland_wp_color_management_output.h
- M ui/ozone/platform/wayland/host/wayland_wp_color_management_surface.cc
- M ui/ozone/platform/wayland/host/wayland_wp_color_management_surface.h
- M ui/ozone/platform/wayland/host/wayland_wp_image_description.cc
- M ui/ozone/platform/wayland/host/wayland_wp_image_description.h
- M ui/platform_window/platform_window_delegate.cc
- M ui/platform_window/platform_window_delegate.h
Change size: M
Delta: 19 files changed, 157 insertions(+), 72 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Kramer Ge, +1 by Avi Drissman
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mitsuru Oshima (Gerrit)
Aug 26, 2025, 11:55:27 PM (12 days ago) Aug 26
to Chromium LUCI CQ, Thomas Anderson, Avi Drissman, chromium...@chromium.org, Sadrul Chowdhury, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Attention needed from Thomas Anderson
Mitsuru Oshima added 1 comment![]( "Open in Gerrit")
File ui/aura/window_tree_host.cc
Line 575, Patchset 9 (Latest):
: display.GetColorSpaces());Mitsuru Oshima . unresolved
Hi, can you explain why this change is needed (and can't be done via display)? This creates another source of the truth about color space, which can be error prone.
Related details
Attention is currently required from:
- Thomas Anderson
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Aug 27, 2025, 1:42:38 PM (12 days ago) Aug 27
to Chromium LUCI CQ, Mitsuru Oshima, Avi Drissman, chromium...@chromium.org, Sadrul Chowdhury, max+watc...@igalia.com, nickdiego+wa...@igalia.com, ozone-...@chromium.org
Thomas Anderson added 1 comment![]( "Open in Gerrit")
File ui/aura/window_tree_host.cc
Line 575, Patchset 9 (Latest):
: display.GetColorSpaces());Mitsuru Oshima . resolved
Hi, can you explain why this change is needed (and can't be done via display)? This creates another source of the truth about color space, which can be error prone.
Thomas Anderson
It must be per-surface and cannot go in display because on Wayland, clients don't get to know their global window coordinates, so the position is always treated as 0,0. So if you have 2 monitors side-by-side with the left HDR and the right SDR, no matter where the window is placed, it will think it's on the HDR monitor. So colors will be wrong if the window is on the SDR monitor. The solution is to handle a signal sent from the compositor, which is sent per-window, that indicates the desired color space.
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages