[GTK] Layout tests with ANGLE backend require to use GLX to make Xvfb and software rasterization work #4429

alexgcastro · 2022-09-16T19:09:27Z

`7edb066`

[GTK] Layout tests with ANGLE backend require to use GLX to make Xvfb and software rasterization work
https://bugs.webkit.org/show_bug.cgi?id=245288

Reviewed by NOBODY (OOPS!).

Gtk layout tests are using Xvfb and software rasterization, this is detected and uses the X11
backends and GLX in WebKit, we need to make sure we have the GLX backend compiled for ANGLE and
we use it in GTK for the GraphicsContextGLFallback class. That solves crashes in the Layout tests
for WebGL with ANGLE.

* Source/ThirdParty/ANGLE/CMakeLists.txt: Compile GLX backend when using
  gtk if x11 is enabled.
* Source/ThirdParty/ANGLE/PlatformGTK.cmake: Add compilation options for
  x11, use platform types in that case.
* Source/WebCore/platform/graphics/gbm/GraphicsContextGLFallback.cpp:
(WebCore::GraphicsContextGLFallback::platformInitializeContext): In case
of gtk and x11 enabled we configure the display to use the ANGLE GLX backend.

7edb066

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	❌ 🧪 api-gtk
	~~🛠 tv~~	✅ 🧪 mac-wk1
	~~🛠 tv-sim~~	✅ 🧪 mac-wk2
	~~🛠 watch~~	✅ 🧪 mac-AS-debug-wk2
	~~🛠 watch-sim~~	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-09-16T19:09:41Z

EWS run on previous version of this PR (hash f3403d3)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

mcatanzaro

Hm, it's OK for some things to be different in developer mode, but forcing GLX is pretty extreme. It means we'll have a different set of graphics bugs in developer mode than our users will encounter. And frankly, we have a lot of graphics bugs. This seems like too much: if developers get forced into a legacy graphics mode, we'll fail to notice bugs before users complain, and will have increased difficulty reproducing bugs that users report.

It's also moving backwards. GLX is a legacy technology, and I understand that nowadays using EGL instead of GLX is recommended even under X11, right?

Ideally, we'd stop using Xvfb for layout tests and use Weston instead, but I understand that would probably require a bunch of rebaselining.

alexgcastro · 2022-09-17T18:58:24Z

Hm, it's OK for some things to be different in developer mode, but forcing GLX is pretty extreme. It means we'll have a different set of graphics bugs in developer mode than our users will encounter. And frankly, we have a lot of graphics bugs. This seems like too much: if developers get forced into a legacy graphics mode, we'll fail to notice bugs before users complain, and will have increased difficulty reproducing bugs that users report.

I agree, but in this case we are choosing using GLX inside ANGLE because WebKit is already using GLX for rendering, that is the problem, we are using GLX already and ANGLE was forcing EGL backend, I mean we need to use GLX for ANGLE because tests were already using it. It is not that we are changing that.

It's also moving backwards. GLX is a legacy technology, and I understand that nowadays using EGL instead of GLX is recommended even under X11, right?

Ideally, we'd stop using Xvfb for layout tests and use Weston instead, but I understand that would probably require a bunch of rebaselining.

Exactly I would love we stop using Xvfb but that is a different task, I don't think we should block bugs waiting for that.

mcatanzaro · 2022-09-17T21:33:14Z

I agree, but in this case we are choosing using GLX inside ANGLE because WebKit is already using GLX for rendering, that is the problem, we are using GLX already and ANGLE was forcing EGL backend, I mean we need to use GLX for ANGLE because tests were already using it. It is not that we are changing that.

Are you sure that your changes are limited to the case where we are using GLX already? It doesn't look like they are?

DEVELOPER_MODE does not mean we're running layout tests, and it certainly doesn't mean we're running under X11.

mcatanzaro · 2022-09-17T21:44:35Z

I agree, but in this case we are choosing using GLX inside ANGLE because WebKit is already using GLX for rendering, that is the problem, we are using GLX already and ANGLE was forcing EGL backend, I mean we need to use GLX for ANGLE because tests were already using it. It is not that we are changing that.

So does this affect more than just layout tests?

If not, I think the next step is: find another way to do what you need for layout tests without looking at DEVELOPER_MODE. The test runner could configure WebKit using a special C API, for example.

zdobersek · 2022-09-19T07:10:23Z

Far-sighted solution would be to just remove GLX support throughout.

alexgcastro · 2022-09-19T19:07:23Z

I agree, but in this case we are choosing using GLX inside ANGLE because WebKit is already using GLX for rendering, that is the problem, we are using GLX already and ANGLE was forcing EGL backend, I mean we need to use GLX for ANGLE because tests were already using it. It is not that we are changing that.

Are you sure that your changes are limited to the case where we are using GLX already? It doesn't look like they are?

DEVELOPER_MODE does not mean we're running layout tests, and it certainly doesn't mean we're running under X11.

Yeah, you are right, but this is the fallback class used when we do not have dmabuf support in a platform, we have it here just for the testing platforms, it is not something we want anyone to use because it copies memory. Currently layout tests is the first thing we want to use it until we solve the mess with EGL and nvidia drivers.

Anyway, a good addition I can add is to add the same check we use to choose GLX in other classes in that case, it is not something we know it is there but it can be a good improvement, that way it can pick EGL or GLX because we know in developer mode we are going to compile it.

I'll do the modification, but remember this is the fallback class for dmabuf support that is even causing a big warning because it is used for testing and making sure everyone knows we don't want you to use it.

alexgcastro · 2022-09-19T19:15:23Z

I agree, but in this case we are choosing using GLX inside ANGLE because WebKit is already using GLX for rendering, that is the problem, we are using GLX already and ANGLE was forcing EGL backend, I mean we need to use GLX for ANGLE because tests were already using it. It is not that we are changing that.

So does this affect more than just layout tests?

If not, I think the next step is: find another way to do what you need for layout tests without looking at DEVELOPER_MODE. The test runner could configure WebKit using a special C API, for example.

It affects any platform that does not support dmabuf, wants to use ANGLE and WebKit decides GLX is the way to go for all the GL rendering, this is not something we want applications to use. We don't know if there is going to be anyone using it like that in the future for testing purposes, because the fallback is not for production, so we don't think it makes sense to spend more effort trying to support this situation. At least for the moment.

mcatanzaro · 2022-09-19T21:41:09Z

Since this is only a fallback that you don't want applications to ever wind up in, how about just making the changes unconditionally, instead of looking at DEVELOPER_MODE?

alexgcastro · 2022-09-21T12:27:57Z

Since this is only a fallback that you don't want applications to ever wind up in, how about just making the changes unconditionally, instead of looking at DEVELOPER_MODE?

We didn't want to compile GLX backend in the release versions for the gtk port because we just wanted to solve a problem with Xvfb testing, we would like to avoid that backend and remove GLX support at some point as Zan explained. I guess we can add it to GTK and avoid it in WPE, if that is something acceptable for everyone I'm ok with it.

mcatanzaro · 2022-09-21T14:50:41Z

I guess we can add it to GTK and avoid it in WPE, if that is something acceptable for everyone I'm ok with it.

That sounds much better than varying graphics behavior based on DEVELOPER_MODE.

… and software rasterization work https://bugs.webkit.org/show_bug.cgi?id=245288 Reviewed by NOBODY (OOPS!). Gtk layout tests are using Xvfb and software rasterization, this is detected and uses the X11 backends and GLX in WebKit, we need to make sure we have the GLX backend compiled for ANGLE and we use it in GTK for the GraphicsContextGLFallback class. That solves crashes in the Layout tests for WebGL with ANGLE. * Source/ThirdParty/ANGLE/CMakeLists.txt: Compile GLX backend when using gtk if x11 is enabled. * Source/ThirdParty/ANGLE/PlatformGTK.cmake: Add compilation options for x11, use platform types in that case. * Source/WebCore/platform/graphics/gbm/GraphicsContextGLFallback.cpp: (WebCore::GraphicsContextGLFallback::platformInitializeContext): In case of gtk and x11 enabled we configure the display to use the ANGLE GLX backend.

webkit-early-warning-system · 2022-09-21T18:19:12Z

EWS run on current version of this PR (hash 7edb066)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	❌ 🧪 api-gtk
	~~🛠 tv~~	✅ 🧪 mac-wk1
	~~🛠 tv-sim~~	✅ 🧪 mac-wk2
	~~🛠 watch~~	✅ 🧪 mac-AS-debug-wk2
	~~🛠 watch-sim~~	✅ 🧪 mac-wk2-stress

mcatanzaro

OK thanks, this looks better.

I'll let graphics reviewers handle final review.

alexgcastro · 2022-09-21T18:44:45Z

OK thanks, this looks better.

I'll let graphics reviewers handle final review.

Thanks for your comments when reviewing the patch.

zdobersek · 2022-09-22T07:33:19Z

I think this needs a solution in the form of removing GLX support. Until that can be done, I'm not the best to review such patches.

alexgcastro · 2022-09-22T12:24:13Z

After https://commits.webkit.org/254751@main this is not a problem anymore because we prefer EGL, so we go in the good directorion to remove GLX. I'm closing this PR.

alexgcastro self-assigned this Sep 16, 2022

alexgcastro added ANGLE Bugs related to the ANGLE project WebKit Nightly Build labels Sep 16, 2022

alexgcastro requested review from zdobersek and carlosgcampos September 16, 2022 19:10

mcatanzaro requested changes Sep 16, 2022

View reviewed changes

alexgcastro force-pushed the gtk_angle_glx_backend branch from f3403d3 to 7edb066 Compare September 21, 2022 18:18

alexgcastro added WebKitGTK Bugs related to the Gtk API layer. and removed ANGLE Bugs related to the ANGLE project labels Sep 21, 2022

mcatanzaro reviewed Sep 21, 2022

View reviewed changes

zdobersek removed their request for review September 22, 2022 07:33

alexgcastro closed this Sep 22, 2022

zdobersek mentioned this pull request Sep 27, 2022

WebGL has a lot of #if USE(ANGLE) blocks, making it hard to maintain #4374

Merged

[GTK] Layout tests with ANGLE backend require to use GLX to make Xvfb and software rasterization work #4429

[GTK] Layout tests with ANGLE backend require to use GLX to make Xvfb and software rasterization work #4429

Uh oh!

Conversation

alexgcastro commented Sep 16, 2022 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

7edb066

Uh oh!

webkit-early-warning-system commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcatanzaro left a comment

Choose a reason for hiding this comment

Uh oh!

alexgcastro commented Sep 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcatanzaro commented Sep 17, 2022

Uh oh!

mcatanzaro commented Sep 17, 2022

Uh oh!

zdobersek commented Sep 19, 2022

Uh oh!

alexgcastro commented Sep 19, 2022

Uh oh!

alexgcastro commented Sep 19, 2022

Uh oh!

mcatanzaro commented Sep 19, 2022

Uh oh!

alexgcastro commented Sep 21, 2022

Uh oh!

mcatanzaro commented Sep 21, 2022

Uh oh!

webkit-early-warning-system commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcatanzaro left a comment

Choose a reason for hiding this comment

Uh oh!

alexgcastro commented Sep 21, 2022

Uh oh!

zdobersek commented Sep 22, 2022

Uh oh!

alexgcastro commented Sep 22, 2022

Uh oh!

Uh oh!

alexgcastro commented Sep 16, 2022 •

edited by webkit-early-warning-system

Loading

`7edb066`

webkit-early-warning-system commented Sep 16, 2022 •

edited

Loading

alexgcastro commented Sep 17, 2022 •

edited

Loading

webkit-early-warning-system commented Sep 21, 2022 •

edited

Loading