Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

wl_subsurface_place_above ignores parent #1865

Closed
christianrauch opened this issue Oct 21, 2019 · 12 comments · Fixed by #2948
Closed

wl_subsurface_place_above ignores parent #1865

christianrauch opened this issue Oct 21, 2019 · 12 comments · Fixed by #2948

Comments

@christianrauch
Copy link

Using wl_subsurface_place_above fails with wl_subsurface@92: error 0: place_above: wl_surface@9 is not a parent or sibling on a parent surface that has previously been used in wl_subcompositor_get_subsurface.
This works on GNOME shell and weston. Therefore, I assume for the moment that this is a sway compositor issue. This might as well be a client issue, but sway should at least behave like the reference compositor weston and show the same window surfaces.

sway 1.2 debug log: sway.log

steps:

  1. decompress the example libdecoration_subsurf_above_bug.tar.xz.txt
  2. build via meson build && ninja -C build
  3. run demo via demo.sh

wayland log on sway 1.2: wl_sway.txt

wayland log on GNOME shell 3.28: wl_gnome.log

@ascent12 ascent12 transferred this issue from swaywm/sway Oct 21, 2019
@ascent12
Copy link
Member

Moved to wlroots, because it would be an issue with wlroots's subsurface code rather than sway's.

Yeah, I have a suspicion our implementation of subsurfaces is not 100% conformant.
Subsurfaces are still a feature not used by a lot of clients, but I have my own test client which fails with a protocol error on wlroots and not weston.

@emersion emersion added the bug label Oct 21, 2019
@emersion
Copy link
Member

emersion commented Jan 9, 2020

Weston achieves this with a dummy subsurface for the parent in the subsurface list (see weston_subsurface_create_for_parent). IIRC we had this at some point but then removed it for some reason.

@berylline
Copy link
Contributor

Hi there @christianrauch,

Do you mind trying out my PR(#2676) to see if the issue has been fixed for you, when you have the time?

Thanks!

@christianrauch
Copy link
Author

I am not using sway on a regular basis. I only test libdecoration against released versions from time-to-time.

If the example above works in your setup, I am happy to close this issue after the PR has been merged.

@rmader
Copy link

rmader commented Mar 19, 2021

I'm seeing a similar issue with a new experimental Firefox backend (https://bugzilla.mozilla.org/show_bug.cgi?id=1695500). Will try the MR in the coming days, in any case it would be great to see this fixed :)

@emersion
Copy link
Member

The root cause is that wlroots has a single list of subsurfaces, and these can only be placed above the parent. Instead, we should have two lists, one for subsurfaces under the parent, one for subsurfaces above the parent.

@berylline
Copy link
Contributor

@rmader If you're talking about #2676, then I'm glad to hear that you want to try out my PR, but FYI, it's still WIP, as in it's not finished yet and I haven't worked on it for 2 months, although I'm planning on working on it again.

@rmader
Copy link

rmader commented Mar 20, 2021

@berylline , yep, I meant your PR :)

Concerning the issue: while libdecoration is certainly already a great unit test for compositors, the new FF backend can also already get tested quite easily. It also uses viewports a lot and may uncover bugs:

git clone https://github.com/servo/webrender.git
cd webrender/example-compositor/compositor
cargo run native [large|small|scroll] swap 1300 520

@rmader
Copy link

rmader commented Mar 21, 2021

@berylline as @emersion already pointed out and from a quick look over the PR it does not yet fix the issue here. Hope you'll find time to get back to it - subsurfaces are IMHO one of the coolest and most powerful features of Wayland :)

emersion added a commit to emersion/wlroots that referenced this issue Jun 1, 2021
Prior to this commit, subsurfaces could only be placed above their
parent. Any place_{above,below} request involving the parent would
fail with a protocol error.

However the Wayland protocol allows using the parent surface in the
place_{above,below} requests, and allows subsurfaces to be placed
below their parent.

Weston's implementation adds a dummy wl_list node in the subsurface
list. However this is potentially dangerous: iterating the list
requires making sure the dummy wl_list node is checked for, otherwise
memory corruption will happen.

Instead, split the list in two: one for subsurfaces above the parent,
the other for subsurfaces below.

Tested with wleird's subsurfaces demo client.

Closes: swaywm#1865
@emersion
Copy link
Member

emersion commented Jun 1, 2021

Can you try #2948?

@rmader
Copy link

rmader commented Jun 1, 2021

Looks good, thanks! I tested with the new experimental Firefox backend which can be enabled in nightly via gfx.webrender.compositor.force-enabled (only in combination with the Wayland backend of course, MOZ_ENABLE_WAYLAND=1), and it works now. There are a few other issues, but they are likely unrelated to subsurface ordering. I'll file follow-up issues.

@emersion
Copy link
Member

emersion commented Jun 1, 2021

Thanks for testing! Yeah, feel free to file new issues.

emersion added a commit that referenced this issue Jun 3, 2021
Prior to this commit, subsurfaces could only be placed above their
parent. Any place_{above,below} request involving the parent would
fail with a protocol error.

However the Wayland protocol allows using the parent surface in the
place_{above,below} requests, and allows subsurfaces to be placed
below their parent.

Weston's implementation adds a dummy wl_list node in the subsurface
list. However this is potentially dangerous: iterating the list
requires making sure the dummy wl_list node is checked for, otherwise
memory corruption will happen.

Instead, split the list in two: one for subsurfaces above the parent,
the other for subsurfaces below.

Tested with wleird's subsurfaces demo client.

Closes: #1865
udfn pushed a commit to udfn/wlroots that referenced this issue Jun 7, 2021
Prior to this commit, subsurfaces could only be placed above their
parent. Any place_{above,below} request involving the parent would
fail with a protocol error.

However the Wayland protocol allows using the parent surface in the
place_{above,below} requests, and allows subsurfaces to be placed
below their parent.

Weston's implementation adds a dummy wl_list node in the subsurface
list. However this is potentially dangerous: iterating the list
requires making sure the dummy wl_list node is checked for, otherwise
memory corruption will happen.

Instead, split the list in two: one for subsurfaces above the parent,
the other for subsurfaces below.

Tested with wleird's subsurfaces demo client.

Closes: swaywm#1865
g7 pushed a commit to droidian/wlroots that referenced this issue Aug 14, 2022
Prior to this commit, subsurfaces could only be placed above their
parent. Any place_{above,below} request involving the parent would
fail with a protocol error.

However the Wayland protocol allows using the parent surface in the
place_{above,below} requests, and allows subsurfaces to be placed
below their parent.

Weston's implementation adds a dummy wl_list node in the subsurface
list. However this is potentially dangerous: iterating the list
requires making sure the dummy wl_list node is checked for, otherwise
memory corruption will happen.

Instead, split the list in two: one for subsurfaces above the parent,
the other for subsurfaces below.

Tested with wleird's subsurfaces demo client.

Closes: swaywm/wlroots#1865
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging a pull request may close this issue.

5 participants