Bug 102687 – [EFL] GLX detection is broken.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kalyan]

It seems HAVE_GLX is enabled automatically if WebGL is enabled (in OptionEfl.cmake which is wrong). If WebGL is not enabled than we never enable it.

We need to add configure test for GLX and set HAVE_GLX flag appropriately.

[Kalyan]

Created attachment 175076 [details]
WIP

Patch does the following:

Tries to compile some basic GLX code.
If the compilation succeeds enables HAVE_GLX flag

[Kalyan]

Comment on attachment 175076 [details]
WIP

No review flag set yet.

[Raphael Kubo da Costa (:rakuco)]

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.

[Thiago Marcos P. Santos]

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.

[Kalyan]

(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.

[Kalyan]

(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.

[Kalyan]

Created attachment 175240 [details]
WIP

[Raphael Kubo da Costa (:rakuco)]

(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.

[Raphael Kubo da Costa (:rakuco)]

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?

[Kalyan]

(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.

[Kalyan]

(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.

[Kalyan]

(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.

[Kalyan]

Created attachment 175446 [details]
proposed patch

[Laszlo Gombos]

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.

[Laszlo Gombos]

Comment on attachment 175446 [details]
proposed patch

r=me. A step in the right direction.

[WebKit Review Bot]

Comment on attachment 175446 [details]
proposed patch

Clearing flags on attachment: 175446

Committed r135463: <http://trac.webkit.org/changeset/135463>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryuan Choi]

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