Bug 102687

Created attachment 175076 [details] WIP Patch does the following: Tries to compile some basic GLX code. If the compilation succeeds enables HAVE_GLX flag

Comment on attachment 175076 [details] WIP No review flag set yet.

Comment on attachment 175076 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=175076&action=review > Source/cmake/OptionsEfl.cmake:190 > +include(CheckCSourceCompiles) > +include(CheckCXXSourceCompiles) Coding style: our CMake calls are all uppercased. > Source/cmake/OptionsEfl.cmake:200 > +CHECK_C_SOURCE_COMPILES( > +" > +#include <GL/glx.h> > +int main() > +{ > + GLXFBConfig c; > + return 0; > +} > +" > +GLXCOMPILATIONPASSED) Doesn't this boil down to just detecting whether mesa is present? I thought we already did that with FIND_PACKAGE(OpenGL). > Source/cmake/OptionsEfl.cmake:203 > + SET(HAVE_GLX 1) This does not seem to be used anywhere.

Comment on attachment 175076 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=175076&action=review >> Source/cmake/OptionsEfl.cmake:200 >> +GLXCOMPILATIONPASSED) > > Doesn't this boil down to just detecting whether mesa is present? I thought we already did that with FIND_PACKAGE(OpenGL). That's true, you could use FIND_PACKAGE(OpenGL) + check if OPENGL_XMESA_FOUND is set.

(In reply to comment #3) > (From update of attachment 175076 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175076&action=review > > > Source/cmake/OptionsEfl.cmake:190 > > +include(CheckCSourceCompiles) > > +include(CheckCXXSourceCompiles) > > Coding style: our CMake calls are all uppercased. > > > Source/cmake/OptionsEfl.cmake:200 > > +CHECK_C_SOURCE_COMPILES( > > +" > > +#include <GL/glx.h> > > +int main() > > +{ > > + GLXFBConfig c; > > + return 0; > > +} > > +" > > +GLXCOMPILATIONPASSED) > > Doesn't this boil down to just detecting whether mesa is present? I thought we already did that with FIND_PACKAGE(OpenGL). This assumption would be wrong on some platforms i.e wayland. > > Source/cmake/OptionsEfl.cmake:203 > > + SET(HAVE_GLX 1) > > This does not seem to be used anywhere.

(In reply to comment #4) > (From update of attachment 175076 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175076&action=review > > >> Source/cmake/OptionsEfl.cmake:200 > >> +GLXCOMPILATIONPASSED) > > > > Doesn't this boil down to just detecting whether mesa is present? I thought we already did that with FIND_PACKAGE(OpenGL). > > That's true, you could use FIND_PACKAGE(OpenGL) + check if OPENGL_XMESA_FOUND is set. I was hoping we could do this. But the problem seems to be as follows: 1)In FindOpenGL.cmake OPENGL_XMESA_FOUND is set to True if OPENGL_xmesa_INCLUDE_DIR is True FIND_PATH(OPENGL_xmesa_INCLUDE_DIR GL/xmesa.h /usr/share/doc/NVIDIA_GLX-1.0/include /usr/openwin/share/include /opt/graphics/OpenGL/include /usr/X11R6/include ) Here GL/xmesa.h seems to be the problem. The "XMesa" interface is deprecated and GL/xmesa*.h files are no longer considered public.

Created attachment 175240 [details] WIP

(In reply to comment #5) > > Doesn't this boil down to just detecting whether mesa is present? I thought we already did that with FIND_PACKAGE(OpenGL). > > This assumption would be wrong on some platforms i.e wayland. (In reply to comment #6) > > That's true, you could use FIND_PACKAGE(OpenGL) + check if OPENGL_XMESA_FOUND is set. > > I was hoping we could do this. But the problem seems to be as follows: > > 1)In FindOpenGL.cmake OPENGL_XMESA_FOUND is set to True if OPENGL_xmesa_INCLUDE_DIR is True > > FIND_PATH(OPENGL_xmesa_INCLUDE_DIR GL/xmesa.h > /usr/share/doc/NVIDIA_GLX-1.0/include > /usr/openwin/share/include > /opt/graphics/OpenGL/include /usr/X11R6/include > ) > > Here GL/xmesa.h seems to be the problem. The "XMesa" interface is deprecated and GL/xmesa*.h files are no longer considered public. Could you elaborate on what the right solution that contemplated Wayland and didn't look for xmesa would be (ie. what libraries and headers would it look for)? As good citizens, it'd be nice to fix this upstream in CMake if possible.

Comment on attachment 175240 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=175240&action=review I need your input to my comment #8 to properly comment on your new Find-file, but a few things should be improved regardless of that: o It needs some documentation at the top like the other Find-files. o We use a 4-space indentation style instead of a 2-space one. o The `X11_FOUND' check implicitly assumes FindX11.cmake was already called before, which is not always the case. Since you do not include X11 headers or link against any X11 library in your CHECK_C_SOURCE_COMPILES_CALL(), it might be worth simply not checking for `X11_FOUND' at all. o "YES" (a string) is different from YES (a boolean). Either way, we normally use FindPackageHandleStandardArguments() to set that, since it also handles other Find-module arguments such as QUIET and the like. > Source/cmake/OptionsEfl.cmake:161 > + FIND_PACKAGE(OpenGLX) > + > + IF (OPENGLX_FOUND) > + ADD_DEFINITIONS(-DHAVE_GLX) > + ENDIF() Do the tiled backing store and WebGL still work if the GLX is not found?

(In reply to comment #8) > (In reply to comment #5) > > > Doesn't this boil down to just detecting whether mesa is present? I thought we already did that with FIND_PACKAGE(OpenGL). > > > > This assumption would be wrong on some platforms i.e wayland. > > (In reply to comment #6) > > > That's true, you could use FIND_PACKAGE(OpenGL) + check if OPENGL_XMESA_FOUND is set. > > > > I was hoping we could do this. But the problem seems to be as follows: > > > > 1)In FindOpenGL.cmake OPENGL_XMESA_FOUND is set to True if OPENGL_xmesa_INCLUDE_DIR is True > > > > FIND_PATH(OPENGL_xmesa_INCLUDE_DIR GL/xmesa.h > > /usr/share/doc/NVIDIA_GLX-1.0/include > > /usr/openwin/share/include > > /opt/graphics/OpenGL/include /usr/X11R6/include > > ) > > > > Here GL/xmesa.h seems to be the problem. The "XMesa" interface is deprecated and GL/xmesa*.h files are no longer considered public. > > Could you elaborate on what the right solution that contemplated Wayland and didn't look for xmesa would be (ie. what libraries and headers would it look for)? As good citizens, it'd be nice to fix this upstream in CMake if possible. Wayland Example: https://projects.kde.org/projects/kde/kde-workspace/repository/revisions/097771054ec91728efb08b883ae88e697df3a5c0/entry/cmake/modules/FindWayland.cmake We should look for GL/glx.h instead of GL/xmesa.h. I was bit pessimistic here thinking about the cases where we could have bogus Glx.h file. i.e. Header present without any proper support for the interface, or completely wrong contents with the name etc. So I ended up with this sol, trying to compile some basic code.

(In reply to comment #9) > (From update of attachment 175240 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175240&action=review > > I need your input to my comment #8 to properly comment on your new Find-file, but a few things should be improved regardless of that: > o It needs some documentation at the top like the other Find-files. > o We use a 4-space indentation style instead of a 2-space one. > o The `X11_FOUND' check implicitly assumes FindX11.cmake was already called before, which is not always the case. Since you do not include X11 headers or link against any X11 library in your CHECK_C_SOURCE_COMPILES_CALL(), it might be worth simply not checking for `X11_FOUND' at all. > o "YES" (a string) is different from YES (a boolean). Either way, we normally use FindPackageHandleStandardArguments() to set that, since it also handles other Find-module arguments such as QUIET and the like. > > > Source/cmake/OptionsEfl.cmake:161 > > + FIND_PACKAGE(OpenGLX) > > + > > + IF (OPENGLX_FOUND) > > + ADD_DEFINITIONS(-DHAVE_GLX) > > + ENDIF() > > Do the tiled backing store and WebGL still work if the GLX is not found? : Current Situation: No. Once these things are fixed for GLX we need to add support for EGL. Than we need some kind of mechanism to configure what back end we would like to use(GLX or EGL or something else ?). I see we already have support for configuration flags like --enable-egl , --enable--glx and so on. Probably we can investigate more on this front when we start adding support for EGL.

(In reply to comment #10) > (In reply to comment #8) > > (In reply to comment #5) > > > > Doesn't this boil down to just detecting whether mesa is present? I thought we already did that with FIND_PACKAGE(OpenGL). > > > > > > This assumption would be wrong on some platforms i.e wayland. > > > > (In reply to comment #6) > > > > That's true, you could use FIND_PACKAGE(OpenGL) + check if OPENGL_XMESA_FOUND is set. > > > > > > I was hoping we could do this. But the problem seems to be as follows: > > > > > > 1)In FindOpenGL.cmake OPENGL_XMESA_FOUND is set to True if OPENGL_xmesa_INCLUDE_DIR is True > > > > > > FIND_PATH(OPENGL_xmesa_INCLUDE_DIR GL/xmesa.h > > > /usr/share/doc/NVIDIA_GLX-1.0/include > > > /usr/openwin/share/include > > > /opt/graphics/OpenGL/include /usr/X11R6/include > > > ) > > > > > > Here GL/xmesa.h seems to be the problem. The "XMesa" interface is deprecated and GL/xmesa*.h files are no longer considered public. > > > > Could you elaborate on what the right solution that contemplated Wayland and didn't look for xmesa would be (ie. what libraries and headers would it look for)? As good citizens, it'd be nice to fix this upstream in CMake if possible. > > Wayland Example: https://projects.kde.org/projects/kde/kde-workspace/repository/revisions/097771054ec91728efb08b883ae88e697df3a5c0/entry/cmake/modules/FindWayland.cmake > > We should look for GL/glx.h instead of GL/xmesa.h. I was bit pessimistic here thinking about the cases where we could have bogus Glx.h file. i.e. Header present without any proper support for the interface, or completely wrong contents with the name etc. So I ended up with this sol, trying to compile some basic code. Something to add: Wayland, doesn't support GLX interface. It rather uses Mesa EGL/EGL-X11 interface.

Created attachment 175446 [details] proposed patch

Comment on attachment 175446 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=175446&action=review Overall looks good to me. > Source/cmake/OptionsEfl.cmake:161 > + IF (OPENGLX_FOUND) > + ADD_DEFINITIONS(-DHAVE_GLX) > + ENDIF() The rule of finding and defining GLX does not seems to be EFL specific, this could be shared (perhaps at later stage) with other CMake based ports.

Comment on attachment 175446 [details] proposed patch r=me. A step in the right direction.

Comment on attachment 175446 [details] proposed patch Clearing flags on attachment: 175446 Committed r135463: <http://trac.webkit.org/changeset/135463>

All reviewed patches have been landed. Closing bug.

*** Bug 102990 has been marked as a duplicate of this bug. ***