Rebased to tip and made some small changes now the context creation matches that of the GLX provider in all three cases: basic, opengl and webrender (works now for webgl 1 in all cases, which previously didn't)

Unfortunately with webrender there's a blue/red swap which I have to figure out. Otherwise it seems to work reasonably well. It also seems to work around bug 1492774, although we should fix that one for GLES.

(In reply to robert.mader from comment #19)

Created attachment 9075220 [details] [diff] [review]
use_desktop_gl_in_egl.patch

Rebased to tip

This does not differentiate between Android (GLES) and desktop (LOCAL_EGL_OPENGL_BIT) like comment 18 does. Also, current master looks different.

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

(In reply to robert.mader from comment #19)

Created attachment 9075220 [details] [diff] [review]
use_desktop_gl_in_egl.patch

Rebased to tip

This does not differentiate between Android (GLES) and desktop (LOCAL_EGL_OPENGL_BIT) like comment 18 does. Also, current master looks different.

Yes I left that out again as it currently has no visible effect on my mashine and the patch does not yet implement the fallback to GLES as required in comment 15. Sorry, I should maybe outline this is WIP.
As soon as I have webrender working correctly, I'll try to add a reasonable fallback code, which will also take care of that bits.
As of current master: well it's gecko-dev master from today :/

@greg: just tested multiple windows, works here

FTR: I think I found the issue around the colors: GLES has GL_EXT_texture_format_BGRA8888, but at least on my intel card I don't get the equivalent GL_EXT_bgra in a core context (but in a compatibility context, which again doesn't work with webrender).
So around the lines in https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/gfx/wr/webrender/src/device/gl.rs#1246 webrender falls back to RGBA now, which matches exactly the observed behaviour. Even more, WebGL 1, which uses the compatibility profile, does not have the issue, while WebGL 2, which uses the core profile, does.

I suspect in the GLX-backend that is handled somewhere, but the Wayland code probably expects BRGA. Now I have to figure out what needs fixing here :) Maybe even mesa not exposing GL_EXT_bgra in core contexts? Apparently NVIDIA does.

Turns out that Webrender fails to properly detect the context as GL instead of GLES. If we force the GL behaviour (which should match what happens with GLX), colors are right again. So here's a little hack until for that until a proper fix lands in WR.

Patch was in the wrong direction :)

This one, too

These lines seem to assume EGL means GLES:
https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/gfx/webrender_bindings/src/bindings.rs#1113
https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/gfx/webrender_bindings/src/bindings.rs#1178

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

These lines seem to assume EGL means GLES:
https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/gfx/webrender_bindings/src/bindings.rs#1113
https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/gfx/webrender_bindings/src/bindings.rs#1178

Thanks for that!

So to sum up what we need to do for this:

In order to just work

• fix WR context detection, so GL on EGL is properly detected and GL on EGL takes the same path as GL on GLX

For best performance, which I'd assume does not involve any texture swizzling therefore would also benefit the GLX path:

• make WR use GL_EXT_bgra additionally to GL_EXT_texture_format_BGRA8888, which is GLES only
• make mesa export GL_EXT_bgra in the core profile

Is that right?

Hi Robert,

(In reply to robert.mader from comment #28)

In order to just work

• fix WR context detection, so GL on EGL is properly detected and GL on EGL takes the same path as GL on GLX

Yes, this sounds like it will fix the red and blue inversion. I should probably have added a warning to that fallback code so it was immediately obvious why the inversion was happening.

For best performance, which I'd assume does not involve any texture swizzling therefore would also benefit the GLX path:

• make WR use GL_EXT_bgra additionally to GL_EXT_texture_format_BGRA8888, which is GLES only
• make mesa export GL_EXT_bgra in the core profile

Is that right?

I don't think this is required, nor correct. GL_EXT_texture_format_BGRA8888 is about allowing BGRA as an internal format. Whereas GL_EXT_BGRA allows BGRA as the external format (with automatic swizzling if required). EXT_BGRA is an ancient extension, which is included in GL for a long time, and we currently rely on that behaviour. As for performance, the driver is hopefully smart enough to use BGRA for the internal format too, but if it is not I don't believe there is any way for us to ask it to.

So just fixing the GL/GLES detection should be enough.

I filed bug 1562817 about switching away from BGRA, since it's an ongoing source of pain.

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

These lines seem to assume EGL means GLES:
https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/gfx/webrender_bindings/src/bindings.rs#1113
https://searchfox.org/mozilla-central/rev/8a990595ce6d5ed07ace2d4d5d86cc69aec90bde/gfx/webrender_bindings/src/bindings.rs#1178

Should they be changed to one of these?

if cfg!(target_os = "android") || unsafe { is_glcontext_angle(gl_context) } {
    gl = unsafe { gl::GlesFns::load_with(|symbol| get_proc_address(gl_context, symbol)) };
} else {
    gl = unsafe { gl::GlFns::load_with(|symbol| get_proc_address(gl_context, symbol)) };
}

#[cfg(target_os = "android")]
gl = unsafe { gl::GlesFns::load_with(|symbol| get_proc_address(gl_context, symbol)) };

#[cfg(not(target_os = "android"))]
if unsafe { is_glcontext_angle(gl_context) } {
    gl = unsafe { gl::GlesFns::load_with(|symbol| get_proc_address(gl_context, symbol)) };
} else {
    gl = unsafe { gl::GlFns::load_with(|symbol| get_proc_address(gl_context, symbol)) };
}

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

Well if we want the EGL backend to be able to use both, GLES and GL, and choose between at runtime (try GL, fall back to GLES for example), it would be nicer if WR would properly detect what kind of context it gets. At runtime.

Rebased on bug 1347442, which implements proper gl type detection. It still chooses the type during compile time, though, so WIP

Ah I meant bug 1563035 of course!

I tinkered a bit with this but before going forward I like again to hear an opinion how to implement this best. I see the following options how to choose which GL type to use:

1. Runtime fallback: where supported, first try GL, then GLES. In comment #15 :jgilbert says we must do that, but it's definitely the hardest and most error prone option. Requires quite a bit of restructuring if I get things right.
2. At compile time
3. Via an environment variable

I'd personally prefer a mixture of 2 and 3, because the GL variant targets only Wayland and X11 clients, both cases where can reasonably assume GL support to be present. The only reason for me to go for option 1 would be if we need to support the case where clients only support GLES and we don't know beforehand / at compile time. Is there such a scenario?

To make it clear, I'd like to:

• have a build variable which controls if GL support should be added to EGL, something like MOZ_EGL_GL. If not set, always use GLES
• if compiled with MOZ_EGL_GL, have an env variable like MOZ_EGL_USE_GLES which makes the EGL provider use GLES, otherwise default to GL

The main reason for the env variable would be to make it easy to compare the two backends, e.g. compare performance or work around bugs like bug 1492774

What do you think?

Any time we can get Desktop GL, that's what we want. (though it's nice to have a pref for experimentation)

Requiring a re-compile isn't something we want.
Env vars are really very similar to a pref, but prefs are documented and env-vars are more error-prone.

Do not make this a build-variable. It's such a pain to tie things tightly to our build system.

(In reply to Jeff Gilbert [:jgilbert] from comment #36)

Any time we can get Desktop GL, that's what we want. (though it's nice to have a pref for experimentation)

Requiring a re-compile isn't something we want.
Env vars are really very similar to a pref, but prefs are documented and env-vars are more error-prone.

Do not make this a build-variable. It's such a pain to tie things tightly to our build system.

Ok, but that does not yet answer the question about option 1 completely. According to https://wiki.mozilla.org/Platform/GFX/WebGL/Backends (which might be a little out-dated), on Windows we just fail if loading ANGLE fails, instead of falling back to native GL. Would the same be ok here, just trying GL and not falling back to GLES? AFAIK FF currently only supports GLX/GL on Linux, so we wouldn't regress.
I know it sounds a bit lazy, but things would be really much easier that way :)

I own this code and I'm saying I'd prefer to load desktop GL, and fall back to GLES.

(In reply to Jeff Gilbert [:jgilbert] from comment #38)

I own this code and I'm saying I'd prefer to load desktop GL, and fall back to GLES.

Alright, just wanted to be sure. Will try to figure it out.

Awesome, thanks!

First draft of a solution with fallback for interested parties. Now I'm go figure out how to use phabricator for a proper review etc.

In the past EGL only supported GLES, not OGL. This has not been true
for a very long time, so lets support OGL context creation in the EGL
backend.

This allows e.g. the Wayland backend to use OGL contexts, which brings
it on par with the X11/GLX backend.

Ok, finally got around to setup moz-phab and friends. Turned out to be so much more straight forward than I thought all the time when looking at text walls. Maybe I should write a blog post, 'how to setup moz-phab with git - for stupid people with low frustration tolerance' or so.

Jokes aside, this is still the same version as I posted earlier. It has a 'MOZ_PREFER_GLES' env variable which allows easy comparison of GL vs GLES with the Wayland backend. Any feedback is welcome.

I tried the patch while I was investigating shader Bug 1583731 and found out that the desktop GL does not provide GL_OES_EGL_image_external_essl3 extension. Not sure if EGL_image_external (provided by desktop GL) is enough here.

:stransky was it necessary to use GL_OES_EGL_image_external_essl3 for bug 1583731 ? Or should it work with OGL, too? :)

GL_OES_EGL_image_external_essl3 is recently needed for WebRender to work with dmabuf backend unless shaders are updated to avoid textureSize calls...so I'm fine with the patch if the desktop GL / GL ES can be switched run-time for webrender.

Ah you're referring to https://github.com/servo/webrender/issues/3310#issuecomment-452433759, right? To use GL_OES_EGL_image_external and avoid calls to textureSize? I'll have a look if I can get that to work.

Actually mesa exposes GL_OES_EGL_image in the core profile, so there's a good chance that it also works for GL_OES_EGL_image_external and GL_OES_EGL_image_external_essl3. Doing that would also help other projects like skia, which also relies on GL_OES_EGL_image_external_essl3 (https://chromium.googlesource.com/skia/+/chrome/m49/src/gpu/gl/GrGLCaps.cpp#219)

Quote from https://gitlab.freedesktop.org/mesa/mesa/issues/2119:

TEXTURE_EXTERNAL was introduced (in ES1.1 days) for GPUs which had strict limitations when external or YUV buffers were used: it is TEXTURE_2D with far more limitations. As time has moved on, these limitations have become unnecessary, and TEXTURE_EXTERNAL is more or less a relic of the past.

I see, TEXTURE_2D can be used in Firefox then, there's no need of TEXTURE_EXTERNAL there.

This needs to be updated to work with webrender after bug 1556301

This patch works for me now as WebRender/GL compositor does not use TEXTURE_EXTERNAL any more. With this patch the GL backend on Wayland works better than GLES for me, for instance I can debug gl frames by apitrace. Jeff, can we move here?
Thanks.

I have to say that I'm not super happy with structure of the patch at the moment. So if you have some suggestions how to better split up the code that would be greatly appreciated. I've tried it already but didn't come up with a great solution thus far :/

https://hg.mozilla.org/mozilla-central/rev/76ee1827c820

Martin, as I don't have commit access yet, could you do a try push for the revision? It looks like this version is so small and simple now that we could land it (IMHO).

(In reply to robert.mader from comment #57)

Martin, as I don't have commit access yet

Can you please apply for it? I can vouch you. How-to is here:

https://www.mozilla.org/en-US/about/governance/policies/commit/
https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/

could you do a try push for the revision? It looks like this version is so small and simple now that we could land it (IMHO).

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

Thanks.

(In reply to Martin Stránský [:stransky] from comment #58)

(In reply to robert.mader from comment #57)

Martin, as I don't have commit access yet

Can you please apply for it? I can vouch you. How-to is here:

Will do!

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

If I interpret the result right this is as expected - no effect, as none of the tests runs on Wayland (AFAIK). But as the EGL provider is used on Android and Windows (angle), too, we should include those platforms (that's where I suspect breakage). I can do so as soon as I get commit permissions.

try for all:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c73645027199ac3e092002452b436dde461bbe28

This workaround was added more than eight years ago and likely has
been fixed by now. More importantly, there is still hardware around
that supports OGL 1.4 / GLES 2.0 (intel gen3). With GLES fallback in
place, these now support WebGL 1 and OpenGL compositor.

Tested with MESA_GL_VERSION_OVERRIDE=1.4 MESA_GLES_VERSION_OVERRIDE=2.0

Depends on D48096

Jeff, mind another round of review? Should be quick (as there's not much code) and it would be great if we carried on to bug 788319

Updated, Thanks. When I use this patch on Wayland, apitrace works for me.

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

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

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

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

https://hg.mozilla.org/mozilla-central/rev/2a8c06104c40