Thanks Lukasz. ll look into the points brought up. I'm unsure on scenarios where GLX will work and EGL will not, but Gtk has GLX fallback. I will look into it (maybe ask on a Mesa development channel). The force flag is a nice idea.
It's not a formal JBS issue yet because I don't know if it fits the JavaFX roadmap, or if there are any other efforts on doing it. It's a "step 1" for wayland support. This is the initial feedback I was looking for. I'm willing to invest time on this, but not if it will never be accepted. -- Thiago. Em ter., 2 de abr. de 2024 às 09:45, Lukasz Kostyra < notificati...@github.com> escreveu: > *@lukostyra* commented on this pull request. > > I took a brief look at your changes and I shared some comments. > > Overall the changes look good. The EGL/GLX loading in GLFactory feels a > bit hacky, but other than making EGL a completely separate Prism backend > (which would duplicate/reorganize a lot of common EGL/GLX code - definitely > a no-go IMO) I don't have a better idea how to solve this. Maybe with an > ES2-specific property like prism.es2.forceglx=true? > > This brings me to a question - other than some form of debugging/fallback > for when EGL is still new to JFX, do we even need GLX implementation when > we have a working EGL implementation? I'm fairly sure that current distros > (especially ones officially supported by JFX) have EGL support available at > this point in time, and since it's an intermediary layer between > application and runtime it shouldn't depend on used GPU. > ------------------------------ > > In > modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c > <https://github.com/openjdk/jfx/pull/1381#discussion_r1547684895>: > > > + > + eglExtensions = (const char *) eglQueryString(eglDisplay, > EGL_EXTENSIONS); > + if (eglExtensions == NULL) { > + eglDestroyContext(eglDisplay, eglContext); > + fprintf(stderr, "eglExtensions == null\n"); > + return 0; > + } > + > + /* > + fprintf(stderr, "glExtensions: %s\n", glExtensions); > + fprintf(stderr, "glxExtensions: %s\n", glxExtensions); > + */ > + > + /* allocate the structure */ > + ctxInfo = (ContextInfo *) malloc(sizeof (ContextInfo)); > + if (ctxInfo == NULL) { > > Shouldn't this also eglDestroyContext? > ------------------------------ > > In > modules/javafx.graphics/src/main/native-prism-es2/linux/egl/LinuxGLContext.c > <https://github.com/openjdk/jfx/pull/1381#discussion_r1547688868>: > > > + } > + > + /* initialize the structure */ > + initializeCtxInfo(ctxInfo); > + ctxInfo->versionStr = strdup(glVersion); > + ctxInfo->vendorStr = strdup(glVendor); > + ctxInfo->rendererStr = strdup(glRenderer); > + ctxInfo->glExtensionStr = strdup(glExtensions); > + ctxInfo->eglExtensionStr = strdup(eglExtensions); > + ctxInfo->versionNumbers[0] = versionNumbers[0]; > + ctxInfo->versionNumbers[1] = versionNumbers[1]; > + ctxInfo->context = eglContext; > + > + /* set function pointers */ > + ctxInfo->glActiveTexture = (PFNGLACTIVETEXTUREPROC) > + dlsym(RTLD_DEFAULT, "glActiveTexture"); > > Since you're using EGL, it would be better to use EGL's own > eglGetProcAddress here and below. > ------------------------------ > > In > modules/javafx.graphics/src/main/java/com/sun/prism/es2/LinuxEGLContext.java > <https://github.com/openjdk/jfx/pull/1381#discussion_r1547718158>: > > > @@ -0,0 +1,36 @@ > +/* > + * Copyright (c) 2012, 2013, Oracle and/or its affiliates. All rights > reserved. > > Copyright dates should be updated (here and in other files) > > — > Reply to this email directly, view it on GitHub > <https://github.com/openjdk/jfx/pull/1381#pullrequestreview-1973486376>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AHKIFHRJWJBPAPXKZHIHLO3Y3KR6HAVCNFSM6AAAAABDYFPNMCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNZTGQ4DMMZXGY> > . > You are receiving this because you were mentioned.Message ID: > <openjdk/jfx/pull/1381/review/1973486...@github.com> >
