WebRender-offload-mode subsurface compositing glitches

[valpackett]

Describe the bug

Interesting visual glitches are happening with Firefox WebRender subsurface compositing… Possibly related to #1206

Compositor bugs tracking issue

To Reproduce

1. Get Firefox Nightly
2. Enable gfx.webrender.compositor.force-enabled and restart
3. Do things with the browser — scroll, resize the window, etc.

Screenshots or stacktrace

https://dl.unrelenting.technology/wf-fx-compositor-1.webm

Wayfire version

git 28b3d39

[ammen99]

What we don't support currently is the reordering of subsurfaces, does Firefox do that?

[valpackett]

The tracking bug I linked to links to swaywm/wlroots#1865 so… maybe? Is there a quick way to check? Anything in particular to look for in WAYLAND_DEBUG?

[ammen99]

The tracking bug I linked to links to swaywm/wlroots#1865 so… maybe? Is there a quick way to check? Anything in particular to look for in WAYLAND_DEBUG?

place_below/above are the relevant requests iirc

[valpackett]

Yeah, there's a lot of these.

[1469531.620]  -> [email protected]_above(wl_surface@92)
[1469531.624]  -> [email protected]()
[1469531.627]  -> [email protected]_above(wl_surface@86)
[1469531.629]  -> [email protected]_destination(4, 477)
[1469531.634]  -> [email protected]_source(0.000000, 0.000000, 8.000000, 954.000000)
[1469531.639]  -> [email protected]_buffer(0, 0, 8, 954)
[1469531.644]  -> [email protected](wl_buffer@319, 0, 0)
[1469531.648]  -> [email protected]()
[1469531.650]  -> [email protected]_above(wl_surface@150)
[1469531.652]  -> [email protected]_destination(4, 397)
[1469531.656]  -> [email protected]_source(0.000000, 230.000000, 8.000000, 794.000000)
[1469531.661]  -> [email protected]_buffer(0, 230, 8, 794)
[1469531.667]  -> [email protected](wl_buffer@294, 0, 0)
[1469531.671]  -> [email protected]()
[1469531.672]  -> [email protected]_above(wl_surface@214)
[1469531.675]  -> [email protected]_position(1304, 512)
[1469531.677]  -> [email protected]_destination(8, 477)
[1469531.681]  -> [email protected]_source(16.000000, 0.000000, 16.000000, 954.000000)
[1469531.686]  -> [email protected]_buffer(16, 0, 16, 954)
[1469531.691]  -> [email protected](wl_buffer@223, 0, 0)
[1469531.694]  -> [email protected]()
[1469531.696]  -> [email protected]_above(wl_surface@302)
[1469531.698]  -> [email protected]_position(1304, 115)
[1469531.701]  -> [email protected]_destination(8, 397)
[1469531.704]  -> [email protected]_source(16.000000, 230.000000, 16.000000, 794.000000)
[1469531.710]  -> [email protected]_buffer(16, 230, 16, 794)
[1469531.715]  -> [email protected](wl_buffer@325, 0, 0)

[valpackett]

Interestingly, Sway does not keep separate below/above lists internally:
https://github.com/swaywm/sway/blob/8fa7b99859066b9098acb158d08f7a060c3bf78e/sway/tree/view.c#L1029-L1040

And subsurface order is just handled inside wlroots:
https://github.com/swaywm/wlroots/blob/cdaab820201d6aff7ed44da35595df65b9739bea/types/wlr_surface.c#L432-L455

Sway does not do anything special for this and doesn't have the glitches, so this might not be the relevant issue at all??

I've tried this

awful test patch

diff --git i/src/view/surface.cpp w/src/view/surface.cpp
index ecc4f6d2..a03484d9 100644
--- i/src/view/surface.cpp
+++ w/src/view/surface.cpp
@@ -270,6 +270,7 @@ wf::wlr_surface_base_t::wlr_surface_base_t(surface_interface_t *self)
 
         auto subsurface = std::make_unique<subsurface_implementation_t>(sub);
         nonstd::observer_ptr<subsurface_implementation_t> ptr{subsurface};
+        sub->data = ptr.get();
         _as_si->add_subsurface(std::move(subsurface), false);
         if (sub->mapped)
         {
@@ -340,7 +341,30 @@ void wf::wlr_surface_base_t::map(wlr_surface *surface)
     /* Handle subsurfaces which were created before this surface was mapped */
     wlr_subsurface *sub;
     wl_list_for_each(sub, &surface->current.subsurfaces_below, current.link)
-    handle_new_subsurface(sub);
+    // handle_new_subsurface(sub);
+    {
+        if (sub->data)
+        {
+            LOGE("Creating the same subsurface twice!");
+
+            return;
+        }
+
+        // parent isn't mapped yet
+        if (!sub->parent->data)
+        {
+            return;
+        }
+
+        auto subsurface = std::make_unique<subsurface_implementation_t>(sub);
+        nonstd::observer_ptr<subsurface_implementation_t> ptr{subsurface};
+        sub->data = ptr.get();
+        _as_si->add_subsurface(std::move(subsurface), true);
+        if (sub->mapped)
+        {
+            ptr->map(sub->surface);
+        }
+    };
     wl_list_for_each(sub, &surface->current.subsurfaces_above, current.link)
     handle_new_subsurface(sub);
 
@@ -406,6 +430,50 @@ void wf::wlr_surface_base_t::commit()
          * a frame callback */
         _as_si->get_output()->render->schedule_redraw();
     }
+
+    wlr_subsurface *sub;
+    wl_list_for_each(sub, &surface->current.subsurfaces_below, current.link) {
+        // LOGI("below reordered ", sub->reordered, " ", sub, " data ", sub->data);
+        wf::surface_interface_t * si = static_cast<wf::surface_interface_t *>(sub->data);
+        auto it = std::find_if(_as_si->priv->surface_children_below.begin(), _as_si->priv->surface_children_below.end(),
+            [=] (const auto& ptr) { return ptr.get() == si; });
+        if (it != _as_si->priv->surface_children_below.end())
+        {
+            // LOGI("BELOW BELOW");
+        } else {
+            LOGE("BELOW NOT BELOW");
+        }
+    }
+    wl_list_for_each(sub, &surface->current.subsurfaces_above, current.link) {
+        // LOGI("above reordered ", sub->reordered, " ", sub, " data ", sub->data);
+        wf::surface_interface_t * si = static_cast<wf::surface_interface_t *>(sub->data);
+        auto it = std::find_if(_as_si->priv->surface_children_above.begin(), _as_si->priv->surface_children_above.end(),
+            [=] (const auto& ptr) { return ptr.get() == si; });
+        if (it != _as_si->priv->surface_children_above.end())
+        {
+            // LOGI("ABOVE ABOVE");
+        } else {
+            LOGE("ABOVE NOT ABOVE");
+        }
+    }
+    for (const auto &ptr : _as_si->priv->surface_children_above) {
+        bool found = false;
+        wl_list_for_each(sub, &surface->current.subsurfaces_above, current.link) {
+            if (sub->data == ptr.get())
+                found = true;
+        }
+        if (!found)
+            LOGE("above not above", ptr.get());
+    }
+    for (const auto &ptr : _as_si->priv->surface_children_below) {
+        bool found = false;
+        wl_list_for_each(sub, &surface->current.subsurfaces_below, current.link) {
+            if (sub->data == ptr.get())
+                found = true;
+        }
+        if (!found)
+            LOGE("below not below", ptr.get());
+    }
 }
 
 void wf::wlr_surface_base_t::update_output(wf::output_t *old_output,

to check if there's ever any mismatch between Wayfire's surface_children_below/above and wlroots's surface->current.subsurfaces_below/above. No, both sides seem to match perfectly.

[ammen99]

Sway does not do anything special for this and doesn't have the glitches, so this might not be the relevant issue at all??

Sway also does not support compositor-generated subsurfaces like we do, this is why they can use the wlroots list without problems. In fact, I have been considering that we can make wayfire do the same, and manually add the compositor subsurfaces (and keep only them in our lists). Shouldn't be too hard, we should already be storing the surface_interface_t* in wlr_subsurface->data, if not, we can easily add support for this.

The alternative would be to reorder the internal lists, which is ugly, and we'd be duplicating the logic from wlroots.

to check if there's ever any mismatch between Wayfire's surface_children_below/above and wlroots's surface->current.subsurfaces_below/above. No, both sides seem to match perfectly.

Yes, it is not surprising they match when the surfaces are created .. however they are reordered later, after mapping.

[valpackett]

it is not surprising they match when the surfaces are created

uhh in that patch above, I'm doing the check on every commit

we should already be storing the surface_interface_t* in wlr_subsurface->data

Actually wlr_subsurface->data was always nullptr (again, on commit). I've added sub->data = ptr.get(); in the patch to fix that.

The alternative would be to reorder the internal lists

Ohh right it's just a flat reordering, right, that's what I saw in wlroots, but when checking our lists I was expecting things to like, change nesting (e.g. become subsurface-of-subsurface) or change from below list to above list, for some reason :D

[valpackett]

Aha! So this quick patch fixes it (with a side effect of extra "subsurface-added" signals without matching "subsurface-removed" and without doing below initially correctly. only the above list is used by Firefox in any case):

diff --git i/src/view/surface.cpp w/src/view/surface.cpp
index ecc4f6d2..6798ad1e 100644
--- i/src/view/surface.cpp
+++ w/src/view/surface.cpp
@@ -270,6 +270,7 @@ wf::wlr_surface_base_t::wlr_surface_base_t(surface_interface_t *self)
 
         auto subsurface = std::make_unique<subsurface_implementation_t>(sub);
         nonstd::observer_ptr<subsurface_implementation_t> ptr{subsurface};
+        sub->data = ptr.get();
         _as_si->add_subsurface(std::move(subsurface), false);
         if (sub->mapped)
         {
@@ -406,6 +407,32 @@ void wf::wlr_surface_base_t::commit()
          * a frame callback */
         _as_si->get_output()->render->schedule_redraw();
     }
+    auto remove_from = [=] (auto& container, auto subsurface)
+    {
+        auto it = std::find_if(container.begin(), container.end(),
+            [=] (const auto& ptr) { return ptr.get() == subsurface.get(); });
+
+        std::unique_ptr<surface_interface_t> ret = nullptr;
+        if (it != container.end())
+        {
+            ret = std::move(*it);
+            container.erase(it);
+        }
+
+        return ret;
+    };
+
+    wlr_subsurface *sub;
+    wl_list_for_each(sub, &surface->current.subsurfaces_above, current.link) {
+        nonstd::observer_ptr<surface_interface_t> ptr{static_cast<surface_interface_t*>(sub->data)};
+        auto rm = remove_from(_as_si->priv->surface_children_above, ptr);
+        _as_si->add_subsurface(std::move(rm), false);
+    }
+    wl_list_for_each(sub, &surface->current.subsurfaces_below, current.link) {
+        nonstd::observer_ptr<surface_interface_t> ptr{static_cast<surface_interface_t*>(sub->data)};
+        auto rm = remove_from(_as_si->priv->surface_children_below, ptr);
+        _as_si->add_subsurface(std::move(rm), true);
+    }
 }
 
 void wf::wlr_surface_base_t::update_output(wf::output_t *old_output,

Not too ugly, but if you'd like to switch to the wlroots list please do :)

[valpackett]

In addition to the ordering thing, I have observed a little damage (?) related glitch (that also doesn't happen in Sway). e.g. with dropdowns like "View" on bugzilla.mozilla.org bug pages, or hover previews in the GitHub dashboard, when these page elements go away they can partially remain until scrolling away.

[KaryotypeB]

I apologize for asking in a somewhat old thread, but is a solution for this currently possible? The graphic artifacts are distressing to me, but I need WebRender for hardware-accelerated video decoding. If a solution isn't feasible right now, can I use the above patch without breaking things? If not, that's okay, I'll figure something out. :)

[valpackett]

@KaryotypeB WebRender itself has never had any problems, this is all about a specific experimental mode that offloads compositing to the system compositor (gfx.webrender.compositor.force-enabled)

[KaryotypeB]

Ah, I see! I must have misunderstood what that option did. Thanks for clearing that up! :D

[valpackett]

Just had a crash in the above patch: remove_from somehow returned nullptr, and add_subsurface does not like null subsurfaces. :D

[travankor]

Firefox webrender subsurface compositing will stay "experimental for the foreseeable future" according to this.

[valpackett]

Right. That doesn't mean we shouldn't properly implement all subsurface functionality though :)