Bug 145408 – [CMake][GTK] Improve detection and usage of GL/GLES/EGL libraries.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Alberto Lopez Perez]

I have observed some problems when building WebKitGTK+ on some embedded devices:

1) When building WebKitGTK+ on a freescale board, the build fails because the EGL headers of Vivante expect some variables to be defined.

$ pkg-config --cflags  egl
-DLINUX -I/usr/include/libdrm 

Our current CMake logic don't passes "-DLINUX" to the build, so when we include the Vivante headers it aborts because that headers need -DLINUX to be defined.

2) When building WebKitGTK+ on any board with any GLESv2 library that don't defines an include path (because it uses the standard system one), we fail to detect the GLES libraries because the way we handle the logic in findgles.cmake (we expect pkg-config to define an include path (-I/somepath) to set gles_found to true instead of using find_path() to find the path for the include headers. pkg-config will not define an include path when the headers are on the default system include path)

$ pkg-config --cflags --libs glesv2
-lGLESv2 


3) Not a problem but an improvement: we should be able to automatically switch to building with GLES when we don't detect OpenGL, but we detect OpenGLESv2.

[Martin Robinson]

(In reply to comment #0)
> 3) Not a problem but an improvement: we should be able to automatically
> switch to building with GLES when we don't detect OpenGL, but we detect
> OpenGLESv2.

We have decided to make everything require explicit configuration arguments to prevent getting builds that silently lack features. Perhaps Michael can comment about whether or not he believes this is the same situation.

[Carlos Alberto Lopez Perez]

(In reply to comment #1)
> (In reply to comment #0)
> > 3) Not a problem but an improvement: we should be able to automatically
> > switch to building with GLES when we don't detect OpenGL, but we detect
> > OpenGLESv2.
> 
> We have decided to make everything require explicit configuration arguments
> to prevent getting builds that silently lack features. Perhaps Michael can
> comment about whether or not he believes this is the same situation.

We are already conditional enabling ACCELERATED_2D_CANVAS depending on finding cairo-gl or not.

Also the feature here will be: "Use accelerated compositing". This can be done either via OpenGL or via OpenGLES. But the feature is the same. Is not like we will be switching off the feature when we find OpenGLES but not OpenGL, we will simply using the another library we support for the feature.

[Carlos Alberto Lopez Perez]

Created attachment 253792 [details]
Patch

[WebKit Commit Bot]

Attachment 253792 [details] did not pass style-queue:


ERROR: Source/WebCore/PlatformEfl.cmake:396:  Alphabetical sorting problem. "crypto/gnutls/CryptoAlgorithmRSASSA_PKCS1_v1_5GnuTLS.cpp" should be before "crypto/gnutls/CryptoAlgorithmRSA_OAEPGnuTLS.cpp".  [list/order] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

(In reply to comment #2)
> We are already conditional enabling ACCELERATED_2D_CANVAS depending on
> finding cairo-gl or not.

I think this is not a very good example though. This one is detected automatically mostly as a stopgap, since Debian and Ubuntu do not have CarioGL and it's impossible to enable ACCELERATED_2D_CANVAS there even if we want to, but all other major distros have been using ACCELERATED_2D_CANVAS for a while and I'm not aware of any reason to turn that off for them.

> Also the feature here will be: "Use accelerated compositing". This can be
> done either via OpenGL or via OpenGLES. But the feature is the same. Is not
> like we will be switching off the feature when we find OpenGLES but not
> OpenGL, we will simply using the another library we support for the feature.

This seems like a good argument to me. It should be fine to automatically fall back to OpenGLES if OpenGL is not available, so long as there's no significant functionality regression when built with OpenGLES instead of OpenGL.

[Michael Catanzaro]

Regarding your patch: it passes my sanity-check, except CMakePushCheckState should be included from FindOpenGL.cmake, not OptionsGTK.cmake and OptionsEFL.cmake.

[Carlos Alberto Lopez Perez]

(In reply to comment #6)
> Regarding your patch: it passes my sanity-check, except CMakePushCheckState
> should be included from FindOpenGL.cmake, not OptionsGTK.cmake and
> OptionsEFL.cmake.

Good point.

[Carlos Alberto Lopez Perez]

Created attachment 253804 [details]
Patch

[WebKit Commit Bot]

Attachment 253804 [details] did not pass style-queue:


ERROR: Source/WebCore/PlatformEfl.cmake:396:  Alphabetical sorting problem. "crypto/gnutls/CryptoAlgorithmRSASSA_PKCS1_v1_5GnuTLS.cpp" should be before "crypto/gnutls/CryptoAlgorithmRSA_OAEPGnuTLS.cpp".  [list/order] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Alberto Lopez Perez]

(In reply to comment #9)
> Attachment 253804 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/PlatformEfl.cmake:396:  Alphabetical sorting problem.
> "crypto/gnutls/CryptoAlgorithmRSASSA_PKCS1_v1_5GnuTLS.cpp" should be before
> "crypto/gnutls/CryptoAlgorithmRSA_OAEPGnuTLS.cpp".  [list/order] [5]
> Total errors found: 1 in 9 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

It was already reported: https://bugs.webkit.org/show_bug.cgi?id=144959

[Carlos Alberto Lopez Perez]

Created attachment 253827 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 253827 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=253827&action=review

This looks great to me, I only have the doubts in FindFoo whether we realy need to still use find_path and find_library even when pkg-config file was found

> Source/cmake/FindEGL.cmake:43
> +endif ()
> +
> +find_path(EGL_INCLUDE_DIRS NAMES EGL/egl.h
> +    HINTS ${PC_EGL_INCLUDEDIR} ${PC_EGL_INCLUDE_DIRS}
> +)

Do we still need to do this even if the pkg-config file was found?

> Source/cmake/FindEGL.cmake:48
> +find_library(EGL_LIBRARIES NAMES ${EGL_NAMES}
> +    HINTS ${PC_EGL_LIBDIR} ${PC_EGL_LIBRARY_DIRS}
> +)

Ditto.

[Carlos Alberto Lopez Perez]

(In reply to comment #12)
> Comment on attachment 253827 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253827&action=review
> 
> This looks great to me, I only have the doubts in FindFoo whether we realy
> need to still use find_path and find_library even when pkg-config file was
> found
> 
> > Source/cmake/FindEGL.cmake:43
> > +endif ()
> > +
> > +find_path(EGL_INCLUDE_DIRS NAMES EGL/egl.h
> > +    HINTS ${PC_EGL_INCLUDEDIR} ${PC_EGL_INCLUDE_DIRS}
> > +)
> 
> Do we still need to do this even if the pkg-config file was found?
> 
> > Source/cmake/FindEGL.cmake:48
> > +find_library(EGL_LIBRARIES NAMES ${EGL_NAMES}
> > +    HINTS ${PC_EGL_LIBDIR} ${PC_EGL_LIBRARY_DIRS}
> > +)
> 
> Ditto.

Yes. The reason is that when you below on that files you define:

FIND_PACKAGE_HANDLE_STANDARD_ARGS(EGL DEFAULT_MSG EGL_INCLUDE_DIRS EGL_LIBRARIES)

You are saying that you only want to define the library as found (EGL_FOUND=TRUE) if both variables are valid/defined (EGL_INCLUDE_DIRS and EGL_LIBRARIES).

But pkg-config will not define a <LIBRARY>_INCLUDE_DIRS when the headers are on the default system path (typically /usr/include) because is redundant.

But, find_path will do in any case. It finds the path where the headers are (even if they are on the default system path).

The same applies to find_library. If pkg-config don't defines a '-lfoobar', at least find_library will find all the .so files and add them with '-L/path/to/foobar.so'

So to my knowledge so far, the best way to find a library in a reliably way is to not directly use the values from pkg-config even when they are available, but use this values instead as hints to find_path() and find_library()

[Carlos Alberto Lopez Perez]

Comment on attachment 253827 [details]
Patch

Clearing flags on attachment: 253827

Committed r184954: <http://trac.webkit.org/changeset/184954>

[Carlos Alberto Lopez Perez]

All reviewed patches have been landed.  Closing bug.