Created attachment 298604 [details] [review]
wayland: Fix subsurface positioning on HiDPI

Keep the active position state in its original coordinate space, and
synchronize the surface actor with it when it changes and when
synchronizing the rest of the surface state, in case the surface scale
had changed.

Created attachment 298605 [details] [review]
wayland: Fix subsurface place_above/below type cast error

A MetaWaylandSurface was casted into a ClutterActor, but it should have
been the MetaSurfaceActor.

Review of attachment 298604 [details] [review]:

::: src/compositor/meta-surface-actor-wayland.c
@@ +140,3 @@
+
+  texture_scale = meta_surface_actor_wayland_get_scale (self);
+  scale = (int)(surface->scale * texture_scale);

This is propagating a bug that we have in the current code. surface->scale is the scale factor specified by the application and says what the scale factor is between pixels and that buffer and logical pixels. But an app is perfectly free to provide a buffer with a scale factor of 2 on a non-hidpi screen. The scale factor between logical pixels and device pixels for a window is up to the compositor - for right now, it should be the scale factor of the "main monitor" for the window.

Review of attachment 298605 [details] [review]:

Patch looks correct. I think stylistically it would be better to pull surface_actor actor out of the loop as well if you are pulling parent_actor out, since having parent_actor separate from surface_actor implies that surface_actor is like sibling_actor and varies per-op.

Review of attachment 298604 [details] [review]:

::: src/compositor/meta-surface-actor-wayland.c
@@ +140,3 @@
+
+  texture_scale = meta_surface_actor_wayland_get_scale (self);
+  scale = (int)(surface->scale * texture_scale);

If a client provides a surface with a scale 2 buffer and its drawn on a scale 1 output, we should scale the buffer with output->scale / surface->scale = 1 / 2 = 0.5. That is what meta_surface_actor_wayland_get_scale does. Meaning:

 texture_scale = output->scale / surface->scale (where output = "main monitor" output)
 scale = surface->scale * texture_scale =
       = surface->scale * (output->scale / surface->scale) =
       = output->scale

except that meta_surface_actor_wayland_get_scale doesn't return "output->scale / surface->scale" for surfaces which top level parent is an Xwayland window since they are never scaled. So we don't actually hit that bug, but would still work for the hypothetical case where Xwayland starts using subsurfaces. That might never happen though so might be unnecessary confusion.

(In reply to Jonas Ådahl from comment #5)
> Review of attachment 298604 [details] [review] [review]:
> 
> ::: src/compositor/meta-surface-actor-wayland.c
> @@ +140,3 @@
> +
> +  texture_scale = meta_surface_actor_wayland_get_scale (self);
> +  scale = (int)(surface->scale * texture_scale);
> 
> If a client provides a surface with a scale 2 buffer and its drawn on a
> scale 1 output, we should scale the buffer with output->scale /
> surface->scale = 1 / 2 = 0.5. That is what
> meta_surface_actor_wayland_get_scale does. Meaning:
> 
>  texture_scale = output->scale / surface->scale (where output = "main
> monitor" output)
>  scale = surface->scale * texture_scale =
>        = surface->scale * (output->scale / surface->scale) =
>        = output->scale

OK - true - sorry for not reading the code closely enough. 

But - but - the logical-to-device scale for a surface is something that's a fundamental property, not a derived property, so getting it by multiplying and dividing by surface->scale is really roundabout. There likely should be a MetaWaylandSurface getter for this - which can do the walking up the parent hierarchy. Then when the output scale is needed it can be computed from that. 

(surface->scale and meta_surface_actor_wayland_get_scale are both bad names, because there's no unique thing that is a "scale")

(In reply to Owen Taylor from comment #6)
> (In reply to Jonas Ådahl from comment #5)
> > Review of attachment 298604 [details] [review] [review] [review]:
> > 
> > ::: src/compositor/meta-surface-actor-wayland.c
> > @@ +140,3 @@
> > +
> > +  texture_scale = meta_surface_actor_wayland_get_scale (self);
> > +  scale = (int)(surface->scale * texture_scale);
> > 
> > If a client provides a surface with a scale 2 buffer and its drawn on a
> > scale 1 output, we should scale the buffer with output->scale /
> > surface->scale = 1 / 2 = 0.5. That is what
> > meta_surface_actor_wayland_get_scale does. Meaning:
> > 
> >  texture_scale = output->scale / surface->scale (where output = "main
> > monitor" output)
> >  scale = surface->scale * texture_scale =
> >        = surface->scale * (output->scale / surface->scale) =
> >        = output->scale
> 
> OK - true - sorry for not reading the code closely enough. 
> 
> But - but - the logical-to-device scale for a surface is something that's a
> fundamental property, not a derived property, so getting it by multiplying
> and dividing by surface->scale is really roundabout. There likely should be
> a MetaWaylandSurface getter for this - which can do the walking up the
> parent hierarchy. Then when the output scale is needed it can be computed
> from that.

Except that "output" here really is state of the window, not the Wayland surface. A Wayland surface doesn't and shouldn't have a single "output" parameter, as thats something that is purely a rendering and window management detail.

If we want to avoid the surface->scale * texture_scale thing, we should rather not allow subsurfaces on Xwayland windows and then just use the output->scale in that case, but I don't think it should be a MetaWaylandSurface getter.

> 
> (surface->scale and meta_surface_actor_wayland_get_scale are both bad names,
> because there's no unique thing that is a "scale")

Yes, It should rather be something like surface->buffer_scale and meta_surface_actor_calculate_texture_scale ().

Created attachment 299779 [details] [review]
wayland: Fix subsurface place_above/below type cast error

A MetaWaylandSurface was casted into a ClutterActor, but it should have
been the MetaSurfaceActor.

Move out parent_actor and surface_actor out of the loop while at it
since they won't change when iterating.

For now, a Wayland surface's output is the same as its parent's window, and a toplevel surface's output is its window's output.

Review of attachment 299779 [details] [review]:

OK.

Created attachment 300261 [details] [review]
wayland: Factor out some parts of meta_surface_actor_wayland_get_scale

Put a toplevel window getter in meta-wayland-surface.h and a main
monitor scale getter in window-wayland.h.

Created attachment 300262 [details] [review]
wayland: Fix subsurface positioning on HiDPI

Keep the active position state in its original coordinate space, and
synchronize the surface actor with it when it changes and when
synchronizing the rest of the surface state, in case the surface scale
had changed.

Created attachment 300424 [details] [review]
wayland: Fix subsurface positioning on HiDPI

Keep the active position state in its original coordinate space, and
synchronize the surface actor with it when it changes and when
synchronizing the rest of the surface state, in case the surface scale
had changed.

----

Changes since previous version:

 - Fixed incorrect g_warn_if_fail conditions

Review of attachment 300261 [details] [review]:

Looks fine. With https://bug744934.bugzilla-attachments.gnome.org/attachment.cgi?id=301180 does meta_window_wayland_get_main_monitor_scale() just become window->monitor->scale ?

Review of attachment 300424 [details] [review]:

Looks good to me.

::: src/compositor/meta-surface-actor-wayland.c
@@ +131,3 @@
+       * to happen. */
+      g_warn_if_fail (clutter_actor_get_x (CLUTTER_ACTOR (self)) == 0);
+      g_warn_if_fail (clutter_actor_get_y (CLUTTER_ACTOR (self)) == 0);

Are you worried that an application embedding Mutter will move the surface around within the window actor? If so, it seems like it should get what it deserves - there are all sorts of other things that *could* be done to the scene graph (like doing the same thing to a wayland window...) and this one isn't particularly worth a warning.

Review of attachment 300424 [details] [review]:

::: src/compositor/meta-surface-actor-wayland.c
@@ +131,3 @@
+       * to happen. */
+      g_warn_if_fail (clutter_actor_get_x (CLUTTER_ACTOR (self)) == 0);
+      g_warn_if_fail (clutter_actor_get_y (CLUTTER_ACTOR (self)) == 0);

These warnings were because normally the relative position should be (0,0) because Xwayland is not expected to deal with subsurfaces (which positions the surface actor relative to its parent actor), and if Xwayland changed that we'd get a warning. I suppose an application can move around surface actors, but that application should be aware that doing that will more or less break subsurfaces and other things.

(In reply to Jonas Ådahl from comment #16)
> Review of attachment 300424 [details] [review] [review]:
> 
> ::: src/compositor/meta-surface-actor-wayland.c
> @@ +131,3 @@
> +       * to happen. */
> +      g_warn_if_fail (clutter_actor_get_x (CLUTTER_ACTOR (self)) == 0);
> +      g_warn_if_fail (clutter_actor_get_y (CLUTTER_ACTOR (self)) == 0);
> 
> These warnings were because normally the relative position should be (0,0)
> because Xwayland is not expected to deal with subsurfaces (which positions
> the surface actor relative to its parent actor), and if Xwayland changed
> that we'd get a warning. I suppose an application can move around surface
> actors, but that application should be aware that doing that will more or
> less break subsurfaces and other things.

If you are worried about Xwayland setting offsets or using subsurfaces, shouldn't the assertions be on x/y rather than on the current position of the actor? If we return out at this point, we'll never change the actor's position and never hit the warnings. Right?

(In reply to Owen Taylor from comment #17)
> (In reply to Jonas Ådahl from comment #16)
> > Review of attachment 300424 [details] [review] [review] [review]:
> > 
> > ::: src/compositor/meta-surface-actor-wayland.c
> > @@ +131,3 @@
> > +       * to happen. */
> > +      g_warn_if_fail (clutter_actor_get_x (CLUTTER_ACTOR (self)) == 0);
> > +      g_warn_if_fail (clutter_actor_get_y (CLUTTER_ACTOR (self)) == 0);
> > 
> > These warnings were because normally the relative position should be (0,0)
> > because Xwayland is not expected to deal with subsurfaces (which positions
> > the surface actor relative to its parent actor), and if Xwayland changed
> > that we'd get a warning. I suppose an application can move around surface
> > actors, but that application should be aware that doing that will more or
> > less break subsurfaces and other things.
> 
> If you are worried about Xwayland setting offsets or using subsurfaces,
> shouldn't the assertions be on x/y rather than on the current position of
> the actor? If we return out at this point, we'll never change the actor's
> position and never hit the warnings. Right?

Eh, that is true.

Attachment 299779 [details] pushed as 208da23 - wayland: Fix subsurface place_above/below type cast error
Attachment 300261 [details] pushed as 117f57f - wayland: Factor out some parts of meta_surface_actor_wayland_get_scale
Attachment 300424 [details] pushed as 3b99313 - wayland: Fix subsurface positioning on HiDPI