1699985 - Implement basic native Wayland compositor backend

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Robert Mader [:rmader]]

Create and merge a minimalist working native compositor backend as a first step. To be enabled via gfx.webrender.compositor and gfx.webrender.compositor.force-enabled.

Comment 1

[Robert Mader [:rmader]]

Development happens in https://github.com/rmader/gecko-dev/tree/bug1699985

Comment 2

[Robert Mader [:rmader]]

Attachment: Bug 1699985 - Add wp_viewporter protocol, r=stransky

This protocol allows cropping and scaling of surfaces and is
required for native compositor integration.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb0decd41bb1
Add wp_viewporter protocol, r=stransky

Comment 4

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/eb0decd41bb1

Comment 5

[Robert Mader [:rmader]]

Attachment: First image of firefox rendering using subsurfaces for tiles

And we start to render things \o/

Comment 6

[Robert Mader [:rmader]]

Attachment: Bug 1699985 - Implement basic native Wayland compositor backend, r?gw,stransky

This implements a minimal working native backend for Wayland. It can
be enabled via gfx.webrender.compositor.force-enabled.

While far from being usable for daily browsing, it works
good enough for some initial compositor testing and to let users
turn off the setting again :)

The focus here was to get a basic structure in place while mini-
mising changes in shared code.

Note:

• when enabled, this will currently crash instantly on Sway and Kwin
  i.e. it will only work on Weston or recent versions of Gnome-Shell
• it will instantly crash in debug builds. Fixing this will most likely
  require changes in WR and was thus left out for now.

Comment 7

[Robert Mader [:rmader]]

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

MOZ_ENABLE_WAYLAND=1 mozregression --repo try --launch b45328e3d930aeb7ce5c4bcaeb7c846392949a79 --pref gfx.webrender.compositor.force-enabled:true

Comment 8

[Robert Mader [:rmader]]

Unfortunately that didn't work as well as expected - the GL error in WR crashes even the prod build, while ignoring it worked for the example compositor. The problem is the glInvalidateFramebuffer at [1] which tries to clear the GL_DEPTH_ATTACHMENT. However, according to mesa[2], this is not a legal option for window system FBOs, which we use here. We may switch to custom managed FBOs in the future, which would allow this behaviour, but for now we stick with the Wayland EGL platform - just as we do on non-comositor Wayland or X11/EGL.

So I think it's valid to change the WR behaviour here, but possibly in a independent patch that we can land first.

1: https://searchfox.org/mozilla-central/source/gfx/wr/webrender/src/device/gl.rs#2618
2: https://gitlab.freedesktop.org/mesa/mesa/-/blob/master/src/mesa/main/fbobject.c#L5090

Comment 9

[Robert Mader [:rmader]]

Glenn, can you maybe help me out here? I need a somewhat non-intrusive way to disable the call in https://searchfox.org/mozilla-central/source/gfx/wr/webrender/src/device/gl.rs#2618 on Wayland - it's apparently not meant to work there.

Comment 10

[Glenn Watson [:gw]]

If I understand the mesa source code and [1], we should technically be calling that function with GL_DEPTH instead of GL_DEPTH_ATTACHMENT in the case where it's a window system FBO. Does that sound right?

We have an enum that defines a draw target [2]. It seems like the NativeSurface type of that enum corresponds closely enough with a window system FBO. We could possibly store a field in the renderer when a draw target is bound, that knows what kind of FBO we're drawing to, based on the type of that draw target enum that was passed in, and use that to set the correct value when calling glInvalidateFramebuffer ? Or, we could add a field to the DrawTarget enum that is set based on what the compositor trait returns from binding an FBO, if we need more granularity per platform. Would that work?

[1] https://www.khronos.org/registry/OpenGL-Refpages/es3.0/html/glInvalidateFramebuffer.xhtml
[2] https://searchfox.org/mozilla-central/source/gfx/wr/webrender/src/device/gl.rs#1164

Comment 11

[Robert Mader [:rmader]]

(In reply to Glenn Watson [:gw] from comment #10)

If I understand the mesa source code and [1], we should technically be calling that function with GL_DEPTH instead of GL_DEPTH_ATTACHMENT in the case where it's a window system FBO. Does that sound right?

Yep, I understand it exactly the same.

We have an enum that defines a draw target [2]. It seems like the NativeSurface type of that enum corresponds closely enough with a window system FBO. We could possibly store a field in the renderer when a draw target is bound, that knows what kind of FBO we're drawing to, based on the type of that draw target enum that was passed in, and use that to set the correct value when calling glInvalidateFramebuffer ?

This would work for Wayland, I'm just not sure about other platforms - IIUC, CA/DC also use NativeSurface and don't hit any problems with it, right? Or did we find a bug here and those other platforms are just less strict?

Or, we could add a field to the DrawTarget enum that is set based on what the compositor trait returns from binding an FBO, if we need more granularity per platform. Would that work?

I guess, if the first solution is not feasible.

The code in question was implemented in bug 1666404 - Jamie, do you happen to know in how far that code affects non-android? Does it actually do something on MacOS and Win10? (see comment 8 and following for context)

Comment 12

[Jamie Nicol [:jnicol]]

we should technically be calling that function with GL_DEPTH instead of GL_DEPTH_ATTACHMENT in the case where it's a window system FBO.

What an annoying distinction to have to make.

The code in question was implemented in bug 1666404 - Jamie, do you happen to know in how far that code affects non-android? Does it actually do something on MacOS and Win10? (see comment 8 and following for context)

I wrote this code specifically for Mali, as on tiled GPUs it can save memory bandwidth. I guess one day in the future it could be helpful for Wayland on Panfrost etc. My hunch, without any knowledge, is that it won't do much on Macos. On windows/ANGLE I think it might, we definitely deliberately call glInvalidateFramebuffer for color + optional depth attachments elsewhere in the code.

We could possibly store a field in the renderer when a draw target is bound, that knows what kind of FBO we're drawing to, based on the type of that draw target enum that was passed in, and use that to set the correct value when calling glInvalidateFramebuffer

I think this sounds like a good approach, with the extra field on the draw target if we need to distinguish between Wayland and DC/CA. It might be cleaner to add the DrawTarget argument to invalidate_depth_target(). And we can assert that it is the currently bound draw target, as the function only makes sense to call if that is the case (for Mali at least) and the Device already knows the currently bound fbo ID.

What is the FBO ID in this case? Is it zero because it's a "window system FBO", or is it a special non-zero framebuffer? If zero, maybe we just check for that in invalidate_depth_target() to choose between GL_DEPTH and GL_DEPTH_ATTACHMENT. That would be the simplest fix.

Comment 13

[Robert Mader [:rmader]]

Attachment: Bug 1699985 - Use correct depth enum in glInvalidateFramebuffer for the default framebuffer, r?jnicol

According to the spec, when the default framebuffer is bound, GL_DEPTH
has to be used instead of GL_DEPTH_ATTACHMENT. This is relevant
on the EGL Wayland platform.

Comment 14

[Robert Mader [:rmader]]

(In reply to Jamie Nicol [:jnicol] from comment #12)

What is the FBO ID in this case? Is it zero because it's a "window system FBO", or is it a special non-zero framebuffer? If zero, maybe we just check for that in invalidate_depth_target() to choose between GL_DEPTH and GL_DEPTH_ATTACHMENT. That would be the simplest fix.

Indeed "window system FBO" seems to be equal to "default fbo" IIUC. Added a patch which goes for this simple solution.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a58584bb44ed
Use correct depth enum in glInvalidateFramebuffer for the default framebuffer, r=jnicol

Comment 16

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/a58584bb44ed

Comment 17

[Robert Mader [:rmader]]

With the latest revision, this becomes surprisingly usable already. Unfortunately there's still a bunch of glitches, but they appear to be mainly because of missing buffer clearing - similar to how things locked when we started with partial damage.

For some reason there's an issue when trying to run via mozregression - manually downloading the tarbar works much better.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac3dcf91080f
Implement basic native Wayland compositor backend, r=stransky,mstange,aosmond

Comment 19

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/ac3dcf91080f