Bug 102687 - [EFL] GLX detection is broken.
Summary: [EFL] GLX detection is broken.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 102990 (view as bug list)
Depends on:
Blocks: 101291 102991
  Show dependency treegraph
 
Reported: 2012-11-19 06:55 PST by Kalyan
Modified: 2012-11-21 22:26 PST (History)
10 users (show)

See Also:


Attachments
WIP (645 bytes, patch)
2012-11-19 16:46 PST, Kalyan
no flags Details | Formatted Diff | Diff
WIP (2.55 KB, patch)
2012-11-20 10:11 PST, Kalyan
no flags Details | Formatted Diff | Diff
proposed patch (1.61 KB, patch)
2012-11-21 07:54 PST, Kalyan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kalyan 2012-11-19 06:55:25 PST
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.
Comment 1 Kalyan 2012-11-19 16:46:32 PST
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 2 Kalyan 2012-11-19 16:56:03 PST
Comment on attachment 175076 [details]
WIP

No review flag set yet.
Comment 3 Raphael Kubo da Costa (:rakuco) 2012-11-20 02:14:31 PST
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 4 Thiago Marcos P. Santos 2012-11-20 05:10:14 PST
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.
Comment 5 Kalyan 2012-11-20 09:43:25 PST
(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.
Comment 6 Kalyan 2012-11-20 09:51:52 PST
(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.
Comment 7 Kalyan 2012-11-20 10:11:24 PST
Created attachment 175240 [details]
WIP
Comment 8 Raphael Kubo da Costa (:rakuco) 2012-11-20 13:13:23 PST
(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 9 Raphael Kubo da Costa (:rakuco) 2012-11-20 13:18:27 PST
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?
Comment 10 Kalyan 2012-11-20 18:04:28 PST
(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.
Comment 11 Kalyan 2012-11-20 18:14:06 PST
(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.
Comment 12 Kalyan 2012-11-20 18:30:04 PST
(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.
Comment 13 Kalyan 2012-11-21 07:54:11 PST
Created attachment 175446 [details]
proposed patch
Comment 14 Laszlo Gombos 2012-11-21 09:52:13 PST
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 15 Laszlo Gombos 2012-11-21 10:09:15 PST
Comment on attachment 175446 [details]
proposed patch

r=me. A step in the right direction.
Comment 16 WebKit Review Bot 2012-11-21 19:09:28 PST
Comment on attachment 175446 [details]
proposed patch

Clearing flags on attachment: 175446

Committed r135463: <http://trac.webkit.org/changeset/135463>
Comment 17 WebKit Review Bot 2012-11-21 19:09:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Ryuan Choi 2012-11-21 22:26:36 PST
*** Bug 102990 has been marked as a duplicate of this bug. ***