GLContextEGL should check the supported EGL version at compile time

[Mads]

Created attachment 370090 [details]
Fix-patch

Webkitgtk should check the supported version at compile time. The macro definitions for 1.5 may not be defined if the EGL only supports for 1.4

[Michael Catanzaro]

The #endif is misplaced!

- Be sure to add a ChangeLog entry using Tools/Scripts/prepare-ChangeLog
- Be sure to flag it as a patch, to enable review
- Then set the r? and cq? Bugzilla flags to request review and commit

[Michael Catanzaro]

See also: https://webkit.org/contributing-code/

[Mads]

Can you please explain why you think its misplaced. I am expecting the platform to support at least EGL 1.4 as default. I will add the changelog.

-Mads

[Michael Catanzaro]

It clearly won't compile:

@@ -384,7 +384,11 @@ EGLContext GLContextEGL::createContextForEGLVersion(PlatformDisplay& platformDis
 
             // Try to create a context with this configuration.
             context = eglCreateContext(platformDisplay.eglDisplay(), config, sharingContext, contextAttributes);
-        } else if (platformDisplay.eglCheckVersion(1, 4)) {
+        } else if
+#else
+	if
+#endif
+		(platformDisplay.eglCheckVersion(1, 4)) {
             const char* extensions = eglQueryString(platformDisplay.eglDisplay(), EGL_EXTENSIONS);
             if (GLContext::isExtensionSupported(extensions, "EGL_KHR_create_context")) {
                 contextAttributes[0] = EGL_CONTEXT_MAJOR_VERSION_KHR;
-- 
2.7.4

[Mads]

Sorry, but there is the ifdef in the patch, I think you missed that.

--- a/Source/WebCore/platform/graphics/egl/GLContextEGL.cpp
+++ b/Source/WebCore/platform/graphics/egl/GLContextEGL.cpp
@@ -372,7 +372,7 @@ EGLContext GLContextEGL::createContextForEGLVersion(PlatformDisplay& platformDis
         // we'll use whatever is available. In order to request a concrete version of OpenGL we need EGL version
         // 1.5 or EGL version 1.4 with the extension EGL_KHR_create_context.
         EGLContext context = EGL_NO_CONTEXT;
-
+#ifdef EGL_VERSION_1_5
         if (platformDisplay.eglCheckVersion(1, 5)) {
             contextAttributes[0] = EGL_CONTEXT_MAJOR_VERSION;
             contextAttributes[1] = 3;
@@ -384,7 +384,11 @@ EGLContext GLContextEGL::createContextForEGLVersion(PlatformDisplay& platformDis
 
             // Try to create a context with this configuration.
             context = eglCreateContext(platformDisplay.eglDisplay(), config, sharingContext, contextAttributes);
-        } else if (platformDisplay.eglCheckVersion(1, 4)) {
+        } else if
+#else
+	if
+#endif
+		(platformDisplay.eglCheckVersion(1, 4)) {
             const char* extensions = eglQueryString(platformDisplay.eglDisplay(), EGL_EXTENSIONS);
             if (GLContext::isExtensionSupported(extensions, "EGL_KHR_create_context")) {
                 contextAttributes[0] = EGL_CONTEXT_MAJOR_VERSION_KHR;
-- 
2.7.4

[Michael Catanzaro]

Oh, you've *intentionally* left the final case as just 

#else
    if
#endif

Don't do that, please, it's not readable.

[Mads]

Ok. Fixed it.

[Mads]

Created attachment 370161 [details]
Fix to check the EGL version at compile time

[Michael Catanzaro]

Comment on attachment 370161 [details]
Fix to check the EGL version at compile time

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

OK this looks better, thanks!

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:419
>          } else if (platformDisplay.eglCheckVersion(1, 4)) {
> +#else
> +        if (platformDisplay.eglCheckVersion(1, 4)) {
> +#endif

How about:

} else
#endif
    if (platformDisplay.eglCheckVersion(1, 4)) {

That way we don't need to repeat the condition at all.

[Adrian Perez]

Comment on attachment 370161 [details]
Fix to check the EGL version at compile time

I don't understand why it is needed to have guards for EGL
version 1.5: The call to “.eglCheckVersion()” can be done
regardless of the version, and if at runtime the check fails
then we are already falling back to check for 1.4 anyway. I
am failing to see how this can be useful and/or needed.

I can make a case for having *only* checks at runtime being
better. Imagine the case in which a distributor packages
WebKit using for building headers from a version of Mesa
which does NOT define EGL_VERSION_1_5; then Mesa is updated
to a new release that happens to support EGL version 1.5 and
new packages available. With a build-time check, the WebKit
package would need to be rebuilt to take advantage of new
features, with a runtime check things will just start working
automatically.

(Not to mention that usual “modus operandi” of OpenGL, EGL,
and related APIs is to provide a common base guaranteed to
work, and have applications check at run time by querying
extensions, versions, and so on.)

[Michael Catanzaro]

Mads, can you copy/paste the build error you are seeing please?

[Adrian Perez]

(In reply to Michael Catanzaro from comment #11)
> Mads, can you copy/paste the build error you are seeing please?

OK, I have been investigating a bit about this: in EGL version 1.5
the extension EGL_KHR_create_context is built-in, but headers for
*older* versions of EGL will not have defined the constants which
were added in 1.5 and that is likely the cause for the compilation
failure, checking the EGL 1.4 spec [1]:

 - EGL_CONTEXT_MAJOR_VERSION was EGL_CONTEXT_CLIENT_VERSION in EGL <1.5
 - EGL_CONTEXT_MINOR_VERSION was not defined prior to EGL 1.5

I would do something like the following instead, because the constant
values for the macros above are set on stone in the spec and official
headers:


  // The EGL headers being used may not include version 1.5 symbols,
  // so we define here the ones we need, which will be used *only*
  // if the EGL version detected at runtime is 1.5 or newer. Values
  // taken from the official headers as published by Khronos, see:
  //
  // https://www.khronos.org/registry/EGL/api/EGL/egl.h

  #ifndef EGL_CONTEXT_MAJOR_VERSION
  #define EGL_CONTEXT_MAJOR_VERSION 0x3098
  #endif
  #ifndef EGL_CONTEXT_MINOR_VERSION
  #define EGL_CONTEXT_MINOR_VERSION 0x30FB
  #endif

  // ...

  if (platformDisplay.eglCheckVersion(1, 5) {
      contextAttributes[0] = EGL_CONTEXT_MAJOR_VERSION;
      contextAttributes[1] = 3;
      contextAttributes[2] = EGL_CONTEXT_MINOR_VERSION;
      // ...and so on, no #ifdefs needed here.
  }

---
[1] https://www.khronos.org/registry/EGL/specs/eglspec.1.5.withchanges.pdf

[Mads]

Created attachment 370304 [details]
Error_log

[Mads]

Created attachment 370343 [details]
Use a generic macro for EGL context version

[Adrian Perez]

Comment on attachment 370343 [details]
Use a generic macro for EGL context version

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

I think this is looking pretty good for landing now. Could a reviewer
please take a look? (Thanks!)

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:407
> +            || (platformDisplay.eglCheckVersion(1, 4) && GLContext::isExtensionSupported(extensions, "EGL_KHR_create_context"))) {

I quite like unifying the two if-branches into a single one, good thinking! Probably
we still want to do the #ifndef-#define dance for the macros, because EGL 1.5 headers
might provide EGL_CONTEXT_MAJOR_VERSION, but not EGL_CONTEXT_MAJOR_VERSION_KHR as an
extension — though it is very likely that the _KHR symbols are provided also on EGL 1.5
headers, so it is probably fine to merge this patch as-is, and add the #ifdef-#define
bits if needed in the future... What do reviewers think about this?

[Michael Catanzaro]

Comment on attachment 370343 [details]
Use a generic macro for EGL context version

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

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:-430
>              }
> -        }

This indentation can't be right.

[Mads]

Created attachment 370881 [details]
Use a generic macro for EGL context version v2

[Michael Catanzaro]

Comment on attachment 370881 [details]
Use a generic macro for EGL context version v2

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

> Source/WebCore/platform/graphics/egl/GLContextEGL.cpp:418
>                  context = eglCreateContext(platformDisplay.eglDisplay(), config, sharingContext, contextAttributes);
> -            }
>          }

Again, this is eight-space indentation when it should be four-space.

[Mads]

I think this looks fine.

[Michael Catanzaro]

Well it's not, because we require four-space indentation....

[Mads]

If the "if" condition started at 8 space indentation why would it need 4 space to end?

[Michael Catanzaro]

It's not allowed to use eight-space indentation in WebKit. We use four-space indentation everywhere. Your patch changes this code from four-space to eight-space; that's why you have cq-.

[Mads]

Why not , as this conditional statement is within another conditional statement?

[Michael Catanzaro]

Maybe we have a misunderstanding.

Your code starts like this:

        if (platformDisplay.eglCheckVersion(1, 5)
            || (platformDisplay.eglCheckVersion(1, 4) && GLContext::isExtensionSupported(extensions, "EGL_KHR_create_context"))) {
                contextAttributes[0] = EGL_CONTEXT_MAJOR_VERSION_KHR;
                contextAttributes[1] = 3;
                contextAttributes[2] = EGL_CONTEXT_MINOR_VERSION_KHR;

with eight spaces of indentation between the if and the code within the condition. But to be accepted into WebKit, it needs to look like this:

        if (platformDisplay.eglCheckVersion(1, 5)
            || (platformDisplay.eglCheckVersion(1, 4) && GLContext::isExtensionSupported(extensions, "EGL_KHR_create_context"))) {
            contextAttributes[0] = EGL_CONTEXT_MAJOR_VERSION_KHR;
            contextAttributes[1] = 3;
            contextAttributes[2] = EGL_CONTEXT_MINOR_VERSION_KHR;

with four spaces.

Similarly, your code ends like this:
 
                // Try to create a context with this configuration.
                context = eglCreateContext(platformDisplay.eglDisplay(), config, sharingContext, contextAttributes);
        }

again, with eight spaces there. But it needs to use four spaces:

            // Try to create a context with this configuration.
            context = eglCreateContext(platformDisplay.eglDisplay(), config, sharingContext, contextAttributes);
        }