1648872 - [Wayland] WebRender partial present never worked with official builds

Status: VERIFIED FIXED

(Firefox Build System :: General, defect)

Description

[Darkspirit]

(Jan Alexander Steffens [:heftig] from bug 1640858 comment 12)

Something's wrong with the official nightly, it seems. Enabling GNOME-Shell's damage region painting shows that the official build isn't posting any partial damage and just submitting the whole window, unlike my builds, which do show individual tiles getting updated.

---

You can toggle the damage region painting by entering the following into the looking glass console (run lg using Alt+F2):

• enable: Meta.add_clutter_debug_flags(0, Clutter.DrawDebugFlag.PAINT_DAMAGE_REGION, 0)
• disable: Meta.remove_clutter_debug_flags(0, Clutter.DrawDebugFlag.PAINT_DAMAGE_REGION, 0)

---

PS: Enabling damage region painting is also one way to disable the shell's partial redrawing optimization, so ironically the bug seems to disappear.

Build from bug 1620076 comment 17:
$ pip3 install --upgrade mozregression
$ MOZ_ENABLE_WAYLAND=1 mozregression --repo autoland --launch 90998e8e1bdd --pref gfx.webrender.all:true gfx.webrender.max-partial-present-rects:-1 -a https://tagesschau.de

Actual: The whole window is red when hovering a link.
Expected: Only the hovered region should be red. It correctly works with the context menu (Basic widget).

Comment 1

[Robert Mader [:rmader]]

Can confirm that, works fine with my own builds.

Comment 2

[Robert Mader [:rmader]]

This also applies to try builds like https://treeherder.mozilla.org/#/jobs?repo=try&revision=992d60ba9d8167a0f5de0421e4455decae405609&selectedTaskRun=O-_fUgOMTyOPwnx2WlVtpw.0

Comment 3

[Asif Youssuff]

That is really weird, given my experience in bug 1646202.

Comment 4

[Robert Mader [:rmader]]

Maybe I need to specify what I experience: when running gnome shell and enabling damage region painting (lg -> Meta.add_clutter_debug_flags(0, Clutter.DrawDebugFlag.PAINT_DAMAGE_REGION, 0)) Mutter draws damage regions as reported by applications. With the official build, these always show full damage, indicating that EGL_KHR_swap_buffers_with_damage is not used.

However, that does not necessarily mean that WR doesn't do partial damage internally somehow - maybe it even explains certain glitches.

Comment 5

[Johannes Pfrang [:johnp]]

This seems to only affect autoland and mozilla-central (and try); mozilla-beta and mozilla-release builds properly submit damage for me (as tested on sway with sway -Ddamage=highlight and the --repo option with mozregression).

edit: The Developer Edition build from DEVEDITION_79_0b1_BUILD1 does not work, while the beta build from the same revision does. Could this be caused by the Nightly/Aurora branding?

edit2: An artifact build with --enable-official-branding doesn't work and a local build with nightly branding does, so it doesn't look like a branding issue to me.

Comment 6

[Johannes Pfrang [:johnp]]

Disregard my previous comment. The beta/release builds fell back to Basic compositing instead of using WebRender and I didn't notice.

Comment 7

[Robert Mader [:rmader]]

Build with some debug logging from nical: https://treeherder.mozilla.org/#/jobs?repo=try&revision=939745bbf0ecdb4985b7a946545380e8b00d1e9d

Comment 8

[Robert Mader [:rmader]]

Attachment: local build damage log

Comment 9

[Robert Mader [:rmader]]

Attachment: try build damage log

Comment 10

[Robert Mader [:rmader]]

The logs look all fine in the try build - now I wonder if there's something else that could be triggering a full repaint. Maybe something about GTK or so - can I look up the build specs somewhere? Do we build with GTK 3.14, as required now?

Comment 11

[Robert Mader [:rmader]]

Important to note: on the distribution build of FF 78 on Fedora 32 things work fine. So this is apparently has to do with the build system.

Comment 12

[Darkspirit]

bug 1634204 upgraded Linux requirements. Let's move this bug over to the Firefox Build System component.

Comment 13

[Robert Mader [:rmader]]

Attachment: Bug 1648872 - Remove checks for unsupported GTK3 versions. r=stransky

This should not fix anything but cleans up the code a bit to make
actually used fallback code easier to spot.

Comment 14

[Robert Mader [:rmader]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=385d6de9fe926c9969f569306428de7a9d65ca85

Comment 15

[Robert Mader [:rmader]]

Hm try push is failing, but things work from a clean repo. I guess I need to schedule clobber or so - can someone point me to how to do that? Can't find it so far :/

Comment 16

[Johannes Pfrang [:johnp]]

Is the GDK_VERSION_3_24 symbol defined in the on-builder GTK version (which is it actually?)? AFAICS GTK doesn't forward-declare those symbols and the builders are on Debian 8 / GTK 3.14 per bug 1634204.

Comment 17

[Robert Mader [:rmader]]

(In reply to Johannes Pfrang [:johnp] from comment #16)

Is the GDK_VERSION_3_24 symbol defined in the on-builder GTK version (which is it actually?)? AFAICS GTK doesn't forward-declare those symbols and the builders are on Debian 8 / GTK 3.14 per bug 1634204.

Thanks, that was it :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4c6da910a8faa47a1001dde8c78542a36edd53c

Comment 18

[Darkspirit]

This patch doesn't fix the bug. Can it be moved to its own bug in case it causes regressions? Bugs are cheap. Just push an updated commit message with the new bug number.

Comment 19

[Robert Mader [:rmader]]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #18)

This patch doesn't fix the bug. Can it be moved to its own bug in case it causes regressions? Bugs are cheap. Just push an updated commit message with the new bug number.

Yep. It's a preparation patch that I'll most likely need here or in bug 1617002, will move it then.

Comment 20

[Phabricator Automation]

Comment on attachment 9162325 [details]
Bug 1648872 - Remove checks for unsupported GTK3 versions. r=stransky

Revision D82804 was moved to bug 1617002. Setting attachment 9162325 [details] to obsolete.

Comment 21

[Robert Mader [:rmader]]

Ok, getting closer to the root cause. It's not about GTK but rather something about mesa/wayland. The actual place where things go wrong / happen differently is: https://gitlab.freedesktop.org/mesa/mesa/-/blob/63cf8adb12440512226dddbe3e233bcba87c7c18/src/egl/drivers/dri2/platform_wayland.c#L1037-1039

if (wl_proxy_get_version((struct wl_proxy *) dri2_surf->wl_surface_wrapper)
       < WL_SURFACE_DAMAGE_BUFFER_SINCE_VERSION)
      return EGL_FALSE;

i.e. we hit https://gitlab.freedesktop.org/mesa/mesa/-/blob/63cf8adb12440512226dddbe3e233bcba87c7c18/src/egl/drivers/dri2/platform_wayland.c#L1117-1122:

   /* If the compositor doesn't support damage_buffer, we deliberately
    * ignore the damage region and post maximum damage, due to
    * https://bugs.freedesktop.org/78190 */
   if (!n_rects || !try_damage_buffer(dri2_surf, rects, n_rects))
      wl_surface_damage(dri2_surf->wl_surface_wrapper,
                        0, 0, INT32_MAX, INT32_MAX);

Comment 22

[Darkspirit]

https://gitlab.freedesktop.org/mesa/mesa/commit/d085a5dff5bf753b82228ef0827f2331aff7b35b
https://gitlab.freedesktop.org/mesa/mesa/commit/af4a298719e7b70cae6592c923ffbed9d4dc3678

Namely: the codepath used was determined solely during build time.

Comment 23

[Robert Mader [:rmader]]

So as far as I understand now, the problem is that wl_proxy_get_version returns 0, as wl_proxy is not versioned when compiled on try/official servers. Ignoring the check in mesa works just fine (successfully tested with a try-build), but that's not an option. Versions for these things were introduces in wayland-client 1.10 (feb 2016), while gtk-3.14 only requires 1.6. (~sept 2014).

Karl, would it be possible to bump Wayland requirements to 1.10 and ask older distributions to simply turn it off? That should fix the issue here I think. Something like adding wayland-client >= 1.10.0 to https://searchfox.org/mozilla-central/source/toolkit/moz.configure#279 and making sure it's available on the build servers.

Edit: maybe we can add a runtime check that disables the wayland backend if run on a system with older wayland-client before we enable it by default, so official builds can still run there?

See also: https://gitlab.freedesktop.org/wayland/wayland/-/blob/1.10/src/wayland-client.c (search for version)

Comment 24

[Robert Mader [:rmader]]

Martin, to me it looks like the implicit Wayland requirement is higher than 1.6 anyway, i.e. such systems can build but not run the Wayland backend. Is that right?

Comment 25

[Karl Tomlinson (:karlt)]

I don't have a feel for the consequences of a change in wayland-client requirements.
Martin is the best person to make a decision here.

Comment 26

[Martin Stránský [:stransky] (ni? me)]

Robert, IIRC the problem is inside Mesa where proxy is created by wl_proxy_create_wrapper(), right? If so I don't quite understand why wl_proxy_create_wrapper() creates the old proxy in Mesa when FF is built by Mozilla as we use system Mesa, right?

Comment 27

[Jan Alexander Steffens [:heftig]]

Martin,

The problem seems to be that wl_registry_bind is static inline. The code from Wayland 1.10 creates a versioned proxy using wl_proxy_marshal_constructor_versioned. This version is replicated to other proxies created from this proxy (e.g. by wl_proxy_marshal_constructor in wl_compositor_create_surface or wl_proxy_create_wrapper in Mesa).

But, the wl_registry_bind from Wayland 1.6 creates the proxy using wl_proxy_marshal_constructor, which in Wayland 1.10 then copies wl_registry's version instead of using the version from the arguments. This version is 0 and originally comes from wl_display:

	/* We set this version to 0 for backwards compatibility.
	 *
	 * If a client is using old versions of protocol headers,
	 * it will use unversioned API to create proxies.  Those
	 * proxies will inherit this 0.
	 *
	 * A client could be passing these proxies into library
	 * code newer than the headers that checks proxy
	 * versions.  When the proxy version is reported as 0
	 * the library will know that it can't reliably determine
	 * the proxy version, and should do whatever fallback is
	 * required.
	 *
	 * This trick forces wl_display to always report 0, but
	 * since it's a special object that we can't bind
	 * specific versions of anyway, this should be fine.
	 */
	display->proxy.version = 0;

Comment 28

[Jan Alexander Steffens [:heftig]]

Wouldn't replacing our uses of wl_registry_bind with something like this be sufficient? I'm not sure the comment is right.

template <class T>
static inline T* WaylandRegistryBind(struct wl_registry* wl_registry,
                                     uint32_t name,
                                     const struct wl_interface* interface,
                                     uint32_t version) {
  struct wl_proxy* id;

  // When libwayland-client does not provide this symbol, it will be
  // linked to the fallback in libmozwayland, which returns NULL.
  id = wl_proxy_marshal_constructor_versioned(
      (struct wl_proxy*)wl_registry, WL_REGISTRY_BIND, interface, version, name,
      interface->name, version, NULL);

  if (id == NULL) {
    id = wl_proxy_marshal_constructor((struct wl_proxy*)wl_registry,
                                      WL_REGISTRY_BIND, interface, name,
                                      interface->name, version, NULL);
  }

  return static_cast<T*>(id);
}

(It's a template because I think this makes it nicer to use, avoiding more casts.)

Comment 29

[Jan Alexander Steffens [:heftig]]

Attachment: Bug 1648872 - Add WaylandRegistryBind, a portable wl_registry_bind.

Comment 30

[Robert Mader [:rmader]]

This indeed looks promising! Unfortunately I won't be able to review and test for the next weeks (PTO) - so Martin, can you have a look (and maybe provide a try push so it can be tested?)

Comment 31

[Martin Stránský [:stransky] (ni? me)]

Great, I'll look at it.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a8ca967e963
Add WaylandRegistryBind, a portable wl_registry_bind. r=stransky

Comment 33

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 6a8ca967e963 (bug 1648872) for linux build bustages at nsWaylandDisplay.h.
https://hg.mozilla.org/integration/autoland/rev/572e4f9c2578fca700e0f16dfac53bd9bbb8b37e

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=6a8ca967e96398f26e8a4704b57fb29287d8d78f&selectedTaskRun=Fz9RcSmRR--SUF53ZoD9zw.0

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=314781459&repo=autoland&lineNumber=13456

Comment 34

[Martin Stránský [:stransky] (ni? me)]

I see. We need to add wl_proxy_marshal_constructor_versioned() to widget/gtk/mozwayland/mozwayland.h.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71d1cfda4d39
Add WaylandRegistryBind, a portable wl_registry_bind. r=stransky

Comment 36

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/71d1cfda4d39

Comment 37

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 38

[Darkspirit]

Verified fixed, thanks! :)
MOZ_ENABLE_WAYLAND=1 mozregression --launch 71d1cfda4d39 --pref gfx.webrender.all:true gfx.webrender.max-partial-present-rects:-1 -a https://www.youtube.com/watch?v=LXb3EKWsInQ