Implement basic native Wayland compositor backend
Categories
(Core :: Graphics: WebRender, enhancement)
Tracking
()
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
.
Assignee | ||
Comment 1•4 years ago
|
||
Development happens in https://github.com/rmader/gecko-dev/tree/bug1699985
Assignee | ||
Comment 2•4 years ago
|
||
This protocol allows cropping and scaling of surfaces and is
required for native compositor integration.
Assignee | ||
Updated•4 years ago
|
Comment 4•4 years ago
|
||
bugherder |
Assignee | ||
Comment 5•4 years ago
|
||
And we start to render things \o/
Assignee | ||
Comment 6•4 years ago
|
||
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.
Assignee | ||
Comment 7•4 years ago
•
|
||
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
Assignee | ||
Comment 8•4 years ago
|
||
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
Updated•4 years ago
|
Assignee | ||
Comment 9•4 years ago
|
||
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•4 years ago
|
||
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
Assignee | ||
Comment 11•4 years ago
|
||
(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 ofGL_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 callingglInvalidateFramebuffer
?
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•4 years ago
|
||
we should technically be calling that function with
GL_DEPTH
instead ofGL_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.
Assignee | ||
Comment 13•4 years ago
|
||
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.
Assignee | ||
Comment 14•4 years ago
|
||
(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 betweenGL_DEPTH
andGL_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•4 years ago
|
||
Comment 16•4 years ago
|
||
bugherder |
Assignee | ||
Comment 17•4 years ago
|
||
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.
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Comment 18•4 years ago
|
||
Comment 19•4 years ago
|
||
bugherder |
Description
•