GNOME Bugzilla – Bug 728902
wayland: hidpi support for xwayland clients
Last modified: 2021-07-05 13:44:28 UTC
See patch no scaling for legacy apps (not sure we even should do that) yet and not tested on multi monitor (do not have an adapter to for that).
Created attachment 275077 [details] [review] wayland: Add basic hidpi support Advertise the scale factor on the output and transform pointer and damage events for clients that scale up by themselves i.e use set_buffer_scale. We do not scale any 'legacy' apps yet.
Created attachment 275083 [details] [review] wayland: Add basic hidpi support Advertise the scale factor on the output and transform pointer and damage events as well as input and opaque regions for clients that scale up by themselves i.e use set_buffer_scale. We do not scale any 'legacy' apps yet.
Review of attachment 275083 [details] [review]: <Jasper> pq, is set_buffer_scale a buffer state? If I attach the same buffer to another surface, will the buffer scale be preserved? <Jasper> And if I attach a new buffer, will we keep the old scale? What about attach buffer A, attach buffer B, attach buffer A? <pq> Jasper, yes, no <Jasper> pq, and the other two? <pq> buffer_scale is buffer_state, that is stored in the wl_surface, and is kept as is in the wl_surface until client resets it <Jasper> drago01, ^ <drago01> pq: so gtk shouldn't have to set the state each time it attaches a new buffer? * FreezingCold hat die Verbindung getrennt (Ping timeout: 255 seconds) <pq> we cannot store any additional parameters with wl_buffer, because we can't change wl_buffer interface at all <Jasper> It doesn't particularly matter, I don't think <drago01> my code assumes that it has to <pq> drago01, no, but it does not hurt either. * vinny hat die Verbindung getrennt (Quit: Leaving) <Jasper> Right, drago01's code resets it to 1 in the double-buffered state every time. <Jasper> That's why I wanted to triple-check * pauldog (quassel@nat/intel/x-kmsrqzizvomkcqmd) hat #wayland betreten <pq> wait, resets when where? <Jasper> In drago01's patches to mutter <pq> the server cannot go changing it on its own :-o <Jasper> It's not clear in the protocol * mahler hat die Verbindung getrennt (Ping timeout: 252 seconds) <pq> clients could rely on the server keeping the state they set <pq> really? I thought it was. <drago01> well gtk sets it every time <drago01> so I implemented it as double buffered sate <drago01> *state even <Jasper> A newly created surface has its buffer scale set to 1. <Jasper> wl_surface.set_buffer_scale changes the pending buffer scale. <Jasper> wl_surface.commit copies the pending buffer scale to the current one. <Jasper> Otherwise, the pending and current values are never changed. <Jasper> hm <Jasper> I guess that's fairly clear <Jasper> drago01, so it should be a MetaWaylandSurface property, not a MetaWaylandBuffer property <drago01> Jasper: yeah <pq> clients *can* set buffer_scale every time, but the server must not rely on that <Jasper> drago01, and we should probably have it set to -1 or 0 and only change it if it's >= 1 <Jasper> pq, yep <drago01> pq, Jasper ok
Created attachment 275141 [details] [review] wayland: Add basic hidpi support Advertise the scale factor on the output and transform pointer and damage events as well as input and opaque regions for clients that scale up by themselves i.e use set_buffer_scale. We do not scale any 'legacy' apps yet. -- Move scale property to the surface and only change it when the client asks for it.
Created attachment 275194 [details] [review] wayland: Add basic hidpi support Advertise the scale factor on the output and transform pointer and damage events as well as input and opaque regions for clients that scale up by themselves i.e use set_buffer_scale. We do not scale any 'legacy' apps yet. --- Rebase to current HEAD and implement same "sanity" in compute_scale() to make ebassi happy ;)
Created attachment 275195 [details] [review] wayland: Add basic hidpi support Advertise the scale factor on the output and transform pointer and damage events as well as input and opaque regions for clients that scale up by themselves i.e use set_buffer_scale. We do not scale any 'legacy' apps yet. -- Fix bad rebase.
Created attachment 275198 [details] [review] wayland: Add high dpi support for legacy applications Scale legacy applications (those who do not use set_buffer_scale and scale by themselves) by the scale factor of the monitor there are mostly on. -- With this we have hidpi support that is pretty much on par with OS X. While the scaled apps are not as sharp as apps that draw on a high resolution it is a huge gain for some apps like java swing apps that go from useless to useable. I didn't test multi monitor yet (still need to get the damn adapter) but it should work.
Review of attachment 275198 [details] [review]: ::: src/backends/meta-monitor-manager.h @@ +74,3 @@ int height_mm; CoglSubpixelOrder subpixel_order; + int scale; I have a feeling this was supposed to be in the other patch. ::: src/wayland/meta-wayland-surface.c @@ +372,3 @@ + /* scale surface actor */ + output_scale = meta_surface_actor_get_scale (surface->surface_actor); + clutter_actor_set_scale (CLUTTER_ACTOR (surface->surface_actor), output_scale, output_scale); Scaling the surface actor will result in subsurfaces that are double the size that they're supposed to be. You can't do that.
Review of attachment 275195 [details] [review]: Where do you actually scale the surface? ::: src/wayland/meta-wayland-outputs.c @@ +41,3 @@ } MetaWaylandOutput; + Extra whitespace. @@ +62,3 @@ } +/* Based on code from gnome-settings-daemon */ Not anymore it isn't. (Did you file a patch upstream?) @@ +93,3 @@ +} + + Extra whitespace. ::: src/wayland/meta-wayland-surface.c @@ +292,3 @@ + cairo_region_t *scaled_region; + + if (scale <= 1) if (scale == 1) @@ +344,3 @@ } + if (pending->scale) if (pending->scale > 0) @@ +537,3 @@ { + MetaWaylandSurface *surface = wl_resource_get_user_data (resource); + surface->pending.scale = scale; Should probably warn about values that are < 1.
(In reply to comment #8) > Review of attachment 275198 [details] [review]: > > ::: src/backends/meta-monitor-manager.h > @@ +74,3 @@ > int height_mm; > CoglSubpixelOrder subpixel_order; > + int scale; > > I have a feeling this was supposed to be in the other patch. > > ::: src/wayland/meta-wayland-surface.c > @@ +372,3 @@ > + /* scale surface actor */ > + output_scale = meta_surface_actor_get_scale (surface->surface_actor); > + clutter_actor_set_scale (CLUTTER_ACTOR (surface->surface_actor), > output_scale, output_scale); > > Scaling the surface actor will result in subsurfaces that are double the size > that they're supposed to be. You can't do that. Hmm ok ... would doing that just for toplevel surfaces solve this?
(In reply to comment #9) > Review of attachment 275195 [details] [review]: > > Where do you actually scale the surface? I don't. That's what the other patch does. This is only for apps that do support set_buffer_scale i.e they are already scaled we only have to scale regions, events etc. > ::: src/wayland/meta-wayland-outputs.c > @@ +41,3 @@ > } MetaWaylandOutput; > > + > > Extra whitespace. > > @@ +62,3 @@ > } > > +/* Based on code from gnome-settings-daemon */ > > Not anymore it isn't. (Did you file a patch upstream?) It is. Owen and Bastien fixed that upstream already my older code was based on an older g-s-d checkout. > @@ +93,3 @@ > +} > + > + > > Extra whitespace. > > ::: src/wayland/meta-wayland-surface.c > @@ +292,3 @@ > + cairo_region_t *scaled_region; > + > + if (scale <= 1) > > if (scale == 1) > > @@ +344,3 @@ > } > > + if (pending->scale) > > if (pending->scale > 0) > > @@ +537,3 @@ > { > + MetaWaylandSurface *surface = wl_resource_get_user_data (resource); > + surface->pending.scale = scale; > > Should probably warn about values that are < 1. OK.
(In reply to comment #11) > I don't. That's what the other patch does. This is only for apps that do > support set_buffer_scale i.e they are already scaled we only have to scale > regions, events etc. If you have an app that set_buffer_scale(2) and it's on an output with a scale of 1, you need to downscale the surface to 0.5. But maybe this is an edge case not to care about. What you do need to make sure about, and another thing you missed, is that the window size needs always be the scale at 1x. So if I have an app that does set_buffer_scale(2) and attaches a 1000x1000 buffer, we need to treat that surface as if it was 500x500, at least at the protocol level. That means that any configure events we send to the client should be at the 500x500 size. I'm not sure how we should handle this internally. Should we scale window->rect and the monitor sizes, or simply do it at the outskirts?
(In reply to comment #12) > (In reply to comment #11) > > I don't. That's what the other patch does. This is only for apps that do > > support set_buffer_scale i.e they are already scaled we only have to scale > > regions, events etc. > > If you have an app that set_buffer_scale(2) and it's on an output with a scale > of 1, you need to downscale the surface to 0.5. But maybe this is an edge case > not to care about. Well the second patch should handle this because +meta_surface_actor_wayland_get_scale does return output_scale / priv->surface->scale; ... so if output scale is 1 and buffer_scale has been set to 2 that would 1 / 2 = 0.5 ... ... expect we currently return an integer so changing that to double or float would magically fix it. > What you do need to make sure about, and another thing you missed, is that the > window size needs always be the scale at 1x. So if I have an app that does > set_buffer_scale(2) and attaches a 1000x1000 buffer, we need to treat that > surface as if it was 500x500, at least at the protocol level. > That means that any configure events we send to the client should be at the > 500x500 size. I'm not sure how we should handle this internally. Should we > scale window->rect and the monitor sizes, or simply do it at the outskirts? I'd just do the latter messing with the window_rect and monitor sizes is a can of worms I don't want to open. So we just have to scale the values in meta_wayland_surface_configure_notify() right?
Created attachment 275228 [details] [review] wayland: Add basic hidpi support Advertise the scale factor on the output and transform pointer and damage events as well as input and opaque regions for clients that scale up by themselves i.e use set_buffer_scale. We do not scale any 'legacy' apps yet. -- Address comments raised in review.
Created attachment 275229 [details] [review] wayland: Add high dpi support for legacy applications Scale legacy applications (those who do not use set_buffer_scale and scale by themselves) by the scale factor of the monitor there are mostly on. -- Address comments raised in review. Now handles the obscure "0.5" case and only scales top level surfaces.
Review of attachment 275228 [details] [review]: ::: src/wayland/meta-wayland-surface.c @@ +285,3 @@ } +static cairo_region_t* Missing space. @@ +540,3 @@ + surface->pending.scale = scale; + else + g_warning("Trying to set invalid buffer_scale of %d\n", scale); Missing space. @@ +1725,1 @@ { Can you put: /* new_width and new_height comes from window->rect, * which is based on the buffer size, not the surface * size. The configure event requires surface size. */ new_width /= surface->scale; new_height /= surface->scale; Instead of doing it inline? This will also become a *lot* more fun when we arrive at full set_window_geometry, hm.
Review of attachment 275229 [details] [review]: ::: src/compositor/meta-surface-actor-wayland.c @@ +116,3 @@ + int output_scale = 1; + + if (window) Can you split this out into get_output_scale (int output_id); ? if (window) output_scale = get_scale_for_output_id (window->monitor->output_id); else output_scale = 1; (This is horrendously wrong for subsurfaces, but oh well. We'll fix that later.) ::: src/compositor/meta-surface-actor.h @@ +39,3 @@ MetaWindow *(* get_window) (MetaSurfaceActor *actor); + + double (*get_scale) (MetaSurfaceActor *actor); What's the point of this indirection vfunc here? We're never going to call get_scale on the X11 class anyway, and everything could simply be done in meta-wayland-surface if I understand correctly. ::: src/compositor/meta-window-actor.c @@ +552,3 @@ cairo_region_get_extents (priv->shape_region, bounds); + + if (scale > 0) Why would this ever return a scale <= 0? ::: src/wayland/meta-wayland-surface.c @@ +371,3 @@ + /* scale (toplevel) surface actor */ + if (surface->window) This is still wrong for subsurfaces. @@ +374,3 @@ + { + output_scale = meta_surface_actor_get_scale (surface->surface_actor); + clutter_actor_set_scale (CLUTTER_ACTOR (surface->surface_actor), output_scale, output_scale); This needs to be in the previous patch. Again, if I have a hi-DPI client, we need to downscale it when it's displayed on a low-resolution monitor. Right now, surface actors are scene graph elements on a stage, but imagine if we rendered to each monitor separately. If you had a window straddling in the middle, you'd render it at 1.0 on the internal hi-DPI panel, and 0.5 on the external display. When it left the hi-DPI panel, the app would re-render at low-res and call set_buffer_scale(1), and it would "snap into place". We also can't scale the surface actor, as this would, again, break subsurfaces. set_buffer_scale applies to the buffer attached directly to the surface, only, and not to any additional subsurfaces.
(In reply to comment #17) > Review of attachment 275229 [details] [review]: > > ::: src/compositor/meta-surface-actor-wayland.c > @@ +116,3 @@ > + int output_scale = 1; > + > + if (window) > > Can you split this out into get_output_scale (int output_id); ? Yes. > if (window) > output_scale = get_scale_for_output_id (window->monitor->output_id); > else > output_scale = 1; > > (This is horrendously wrong for subsurfaces, but oh well. We'll fix that > later.) See below. > ::: src/compositor/meta-surface-actor.h > @@ +39,3 @@ > MetaWindow *(* get_window) (MetaSurfaceActor *actor); > + > + double (*get_scale) (MetaSurfaceActor *actor); > > What's the point of this indirection vfunc here? We're never going to call > get_scale on the X11 class anyway, and everything could simply be done in > meta-wayland-surface if I understand correctly. Because it gets used in meta-window-actor and there we have that indirection breaking the abstraction here just does not feel right to me. > ::: src/compositor/meta-window-actor.c > @@ +552,3 @@ > cairo_region_get_extents (priv->shape_region, bounds); > + > + if (scale > 0) > > Why would this ever return a scale <= 0? Uh that should read "> 1" not sure why it ended up like that. > ::: src/wayland/meta-wayland-surface.c > @@ +371,3 @@ > > + /* scale (toplevel) surface actor */ > + if (surface->window) > > This is still wrong for subsurfaces. See below. > @@ +374,3 @@ > + { > + output_scale = meta_surface_actor_get_scale (surface->surface_actor); > + clutter_actor_set_scale (CLUTTER_ACTOR (surface->surface_actor), > output_scale, output_scale); > > This needs to be in the previous patch. If I do that there is no point anymore in having this as two separate patches. So are you suggesting merging them? I don't think a split that have "broken state" + "2nd patch that fixes it" makes sense. > Again, if I have a hi-DPI client, we > need to downscale it when it's displayed on a low-resolution monitor. And why do you think that the patch does not do that? > Right now, surface actors are scene graph elements on a stage, but imagine if > we rendered to each monitor separately. What does "we rendered to each monitor separately" mean? We don't do anything like that once we do that we can fix things up. How would this work? > If you had a window straddling in the > middle, you'd render it at 1.0 on the internal hi-DPI panel, and 0.5 on the > external display. We currently don't do that it the larger part of the window "wins" so if more then 50% of the window are on the high dpi screen you get hi dpi rendering if it is on the low res screen you get the low res rendering. We currently don't have that "split rendering". > When it left the hi-DPI panel, the app would re-render at > low-res and call set_buffer_scale(1), and it would "snap into place". "snap into place" ? > We also can't scale the surface actor, as this would, again, break subsurfaces. > set_buffer_scale applies to the buffer attached directly to the surface, only, > and not to any additional subsurfaces. OK so apparently sub surfaces have a window (why?) the other way we could deal with it is to unscale sub surfaces: output_scale = 2 top level buffer_scale = 2 -> surface_actor gets scaled by 2 / 2 i.e not at all. loop over all subsurfaces and set their scale to output_scale / sub.buffer_scale That way the top level scaling won't affect the scaling of the sub surfaces.
Subsurfaces don't have a window. That's one of the reasons it's horrendously broken. The actor tree for a normal window, with no subsurfaces, looks like: MetaWindowActor MetaSurfaceActor (toplevel surface) MetaShapedTexture (toplevel surface) With a subsurface, it looks like this: MetaWindowActor MetaSurfaceActor (toplevel surface) MetaShapedTexture (toplevel surface) MetaSurfaceActor (subsurface) MetaShapedTexture (subsurface) With more subsurfaces, we have the obvious nesting. set_buffer_scale should affect the buffer attached to the surface, *not* any subsurfaces. So we need to scale the stex, not the surface. Only the toplevel MetaSurfaceActor directly has a MetaWindow. That's because it was given toplevel status with wl_shell_surface or xdg_surface. With the others, like wl_subsurface, it has a different role. Detailed review to your review coming.
(In reply to comment #18) > Because it gets used in meta-window-actor and there we have that indirection > breaking the abstraction here just does not feel right to me. MetaWindowActor is a terrible abstraction-breaking thing in the first place. I don't feel the vfunc is helping anybody. > > ::: src/compositor/meta-window-actor.c > > @@ +552,3 @@ > > cairo_region_get_extents (priv->shape_region, bounds); > > + > > + if (scale > 0) > > > > Why would this ever return a scale <= 0? > > Uh that should read "> 1" not sure why it ended up like that. Why? For the case of 0.5, we should multiply as well. Not that we'll ever hit this path, since the X11 wayland windows which shadows are used for in this never specify a surface size > 1. > If I do that there is no point anymore in having this as two separate patches. > So are you suggesting merging them? I don't think a split that have "broken > state" + "2nd patch that fixes it" makes sense. No. I'm saying to either rename this from "legacy" or something like that, or put parts of this in the first patch. What this is fixing is the case where the surface and output scales don't match, *in either direction*. The legacy case is a 1x surface on a 2x output, but the other case is a 2x surface on a 1x output.
(In reply to comment #20) > (In reply to comment #18) > > Because it gets used in meta-window-actor and there we have that indirection > > breaking the abstraction here just does not feel right to me. > > MetaWindowActor is a terrible abstraction-breaking thing in the first place. I > don't feel the vfunc is helping anybody. > > > > ::: src/compositor/meta-window-actor.c > > > @@ +552,3 @@ > > > cairo_region_get_extents (priv->shape_region, bounds); > > > + > > > + if (scale > 0) > > > > > > Why would this ever return a scale <= 0? > > > > Uh that should read "> 1" not sure why it ended up like that. > > Why? For the case of 0.5, we should multiply as well. Not that we'll ever hit > this path, since the X11 wayland windows which shadows are used for in this > never specify a surface size > 1. OK so either != 1 or just remove it. > > If I do that there is no point anymore in having this as two separate patches. > > So are you suggesting merging them? I don't think a split that have "broken > > state" + "2nd patch that fixes it" makes sense. > > No. I'm saying to either rename this from "legacy" or something like that, or > put parts of this in the first patch. > > What this is fixing is the case where the surface and output scales don't > match, *in either direction*. The legacy case is a 1x surface on a 2x output, > but the other case is a 2x surface on a 1x output. Well the split is currently this patch 1: "Handle the case where no scaling is needed" patch 2: "Do the scaling when needed" So I prefer the renaming instead of mixing them.
Created attachment 275243 [details] [review] wayland: Scale surfaces for hidpi Scale surfaces based on output scale and the buffer scale set by them. We pick the scale factor of the monitor there are mostly on. -- OK this one fixes subsurfaces (after the long discussion yesterday I decided to go with having the surface actor report the scaled size), got cleaned up and renamed to remove the legacy wording.
Hmm maximize and half maximize is still broken for scaled windows i.e ends up being twice the size ... don't know why though.
(In reply to comment #23) > Hmm maximize and half maximize is still broken for scaled windows i.e ends up > being twice the size ... don't know why though. OK, that's because xwayland windows are still x11 windows have to scale the configure notify that we send for interactive resize and maximization here as well.
Review of attachment 275243 [details] [review]: See comment 24
*** Bug 728058 has been marked as a duplicate of this bug. ***
Created attachment 275573 [details] [review] wayland: Add basic hidpi support Advertise the scale factor on the output and transform pointer and damage events as well as input and opaque regions for clients that scale up by themselves i.e use set_buffer_scale. We do not scale any 'legacy' apps yet. --- Style fixes from review.
Created attachment 275576 [details] [review] wayland: Scale native surfaces for hidpi Scale surfaces based on output scale and the buffer scale set by them. We pick the scale factor of the monitor there are mostly on. We only handle native i.e non xwayland / legacy clients yet. --- OK, split the native part out. X11 is messy and needs a bit more work (in gtk and other places). This one at least makes non scaled up wayland clients work better. With those two patches everything works at least as good as in x today modulo gnome-shell fonts (there is no xft-dpi to scale them up, needs to get fixed).
Review of attachment 275576 [details] [review]: ::: src/compositor/meta-window-actor.c @@ +552,3 @@ cairo_region_get_extents (priv->shape_region, bounds); + + if (meta_is_wayland_compositor ()) I'd prefer this to be META_IS_SURFACE_ACTOR_WAYLAND (priv->surface) ::: src/wayland/meta-wayland-surface.c @@ +378,3 @@ + /* scale surface texture */ + output_scale = meta_surface_actor_wayland_get_scale (META_SURFACE_ACTOR_WAYLAND (surface->surface_actor)); + clutter_actor_set_scale (CLUTTER_ACTOR (meta_surface_actor_get_texture (surface->surface_actor)), Could we move this to meta-surface-actor-wayland? It feels a bit weird having it in here.
(In reply to comment #29) > Review of attachment 275576 [details] [review]: > > ::: src/compositor/meta-window-actor.c > @@ +552,3 @@ > cairo_region_get_extents (priv->shape_region, bounds); > + > + if (meta_is_wayland_compositor ()) > > I'd prefer this to be META_IS_SURFACE_ACTOR_WAYLAND (priv->surface) OK. > ::: src/wayland/meta-wayland-surface.c > @@ +378,3 @@ > + /* scale surface texture */ > + output_scale = meta_surface_actor_wayland_get_scale > (META_SURFACE_ACTOR_WAYLAND (surface->surface_actor)); > + clutter_actor_set_scale (CLUTTER_ACTOR (meta_surface_actor_get_texture > (surface->surface_actor)), > > Could we move this to meta-surface-actor-wayland? It feels a bit weird having > it in here. Yeah that makes sense.
Attachment 275573 [details] pushed as 31c925c - wayland: Add basic hidpi support Attachment 275576 [details] pushed as f9bffae - wayland: Scale native surfaces for hidpi Pushed with suggested changes, still open need to handle legacy xwayland apps.
There are still some pretty savage issues with XWayland and HiDPI-aware clients. For example, Chrome is DPI-aware: when I run it on my 3200x1800 (HiDPI) internal panel, it renders everything correctly with a scale factor. When I move it on to my 2560x1440 (non-HiDPI) external monitor, it's huge and unreadable.
(In reply to Daniel Stone from comment #32) > There are still some pretty savage issues with XWayland and HiDPI-aware > clients. For example, Chrome is DPI-aware: when I run it on my 3200x1800 > (HiDPI) internal panel, it renders everything correctly with a scale factor. > When I move it on to my 2560x1440 (non-HiDPI) external monitor, it's huge > and unreadable. My guess is that it's just blithely picking the highest (or primary) scale factor, and using that everywhere. Setting the scaling-factor gsetting to 1 and starting Chrome works just fine; luckily Chrome doesn't monitor the setting for changes, so you can then change it back to 0 to unbreak the rest of your desktop ...
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/mutter/-/issues/ Thank you for your understanding and your help.