RFR: 8329820: [Linux] Prefer EGL over GLX [v3]

[Lukasz Kostyra]

On Fri, 19 Apr 2024 14:42:23 GMT, Thiago Milczarek Sayao <tsayao at openjdk.org> wrote:

>> Wayland implementation will require EGL. 
>> 
>> EGL works with Xorg as well. The idea is to be EGL first and if it fails, fallback to GLX. A force flag `prism.es2.forceGLX=true` is available.
>> 
>> 
>> See:
>> [Switching the Linux graphics stack from GLX to EGL](https://mozillagfx.wordpress.com/2021/10/30/switching-the-linux-graphics-stack-from-glx-to-egl/)
>> [Prefer EGL to GLX for the GL support on X11](https://gitlab.gnome.org/GNOME/gtk/-/merge_requests/3540)
>
> Thiago Milczarek Sayao has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Forgot debug message

I left a few comments, mostly for adjusting error-checking. Once those are fixed and others have their input I'll test those changes.

modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c line 252:

> 250:             eglGetProcAddress("glGetProgramInfoLog");
> 251:     ctxInfo->glTexImage2DMultisample = (PFNGLTEXIMAGE2DMULTISAMPLEPROC)
> 252:             dlsym(RTLD_DEFAULT,"glTexImage2DMultisample");

I think these three functions should also be available via `eglGetProcAddress`

modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c line 258:

> 256:             dlsym(RTLD_DEFAULT,"glBlitFramebuffer");
> 257: 
> 258:     eglSwapInterval(eglDisplay, 0);

`eglSwapInterval` can potentially fail, we should check for errors here

modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c line 267:

> 265: 
> 266:     // Release context once we are all done
> 267:     eglMakeCurrent(eglDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);

Similar as above, `eglMakeCurrent` can also fail

modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c line 301:

> 299: 
> 300:     if (!eglMakeCurrent(ctxInfo->eglDisplay, dInfo->eglSurface, dInfo->eglSurface, ctxInfo->context)) {
> 301:         fprintf(stderr, "Failed in eglMakeCurrent");

I think you might be missing a `return` statement here.

modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c line 310:

> 308:     interval = (vSyncNeeded) ? 1 : 0;
> 309:     ctxInfo->state.vSyncEnabled = vSyncNeeded;
> 310:     eglSwapInterval(ctxInfo->eglDisplay, interval);

Check for errors like above

modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLDrawable.c line 58:

> 56:     }
> 57: 
> 58:     EGLSurface eglSurface = eglCreateWindowSurface(pfInfo->eglDisplay, pfInfo->eglConfig,

I would add a check to make sure the surface was created successfully, as it can potentially return `EGL_NO_SURFACE`.

Some additional information from `eglGetError` could also come in handy if `eglCreateWindowSurface` does fail.

modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLDrawable.c line 119:

> 117: 
> 118:     eglSwapBuffers(eglGetCurrentDisplay(), dInfo->eglSurface);
> 119:     return JNI_TRUE;

eglSwapBuffers can fail, so I'd `return (eglSwapBuffers(...) == EGL_TRUE);` or something similar

-------------

PR Review: https://git.openjdk.org/jfx/pull/1381#pullrequestreview-2022336712
PR Review Comment: https://git.openjdk.org/jfx/pull/1381#discussion_r1579372523
PR Review Comment: https://git.openjdk.org/jfx/pull/1381#discussion_r1579386732
PR Review Comment: https://git.openjdk.org/jfx/pull/1381#discussion_r1579387499
PR Review Comment: https://git.openjdk.org/jfx/pull/1381#discussion_r1579431452
PR Review Comment: https://git.openjdk.org/jfx/pull/1381#discussion_r1579387989
PR Review Comment: https://git.openjdk.org/jfx/pull/1381#discussion_r1579376454
PR Review Comment: https://git.openjdk.org/jfx/pull/1381#discussion_r1579379362