Closed Bug 1699985 Opened 4 years ago Closed 4 years ago

Implement basic native Wayland compositor backend

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
firefox90 --- fixed

People

(Reporter: rmader, Assigned: rmader)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

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.

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

Keywords: leave-open

And we start to render things \o/

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.

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

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

Attachment #9215165 - Attachment description: Bug 1699985 - Implement basic native Wayland compositor backend, r?gw,stransky → Bug 1699985 - WIP: Implement basic native Wayland compositor backend, r?gw,stransky

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.

Flags: needinfo?(gwatson)

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

Flags: needinfo?(gwatson)

(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)

Flags: needinfo?(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.

Flags: needinfo?(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.

(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.

Pushed by robert.mader@posteo.de: https://hg.mozilla.org/integration/autoland/rev/a58584bb44ed Use correct depth enum in glInvalidateFramebuffer for the default framebuffer, r=jnicol

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.

Attachment #9215165 - Attachment description: Bug 1699985 - WIP: Implement basic native Wayland compositor backend, r?gw,stransky → Bug 1699985 - Implement basic native Wayland compositor backend, r?gw,stransky
Depends on: 1707943
See Also: → 1710120
Depends on: 1710180
Keywords: leave-open
Pushed by robert.mader@posteo.de: https://hg.mozilla.org/integration/autoland/rev/ac3dcf91080f Implement basic native Wayland compositor backend, r=stransky,mstange,aosmond
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: