WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
197971
GLContextEGL should check the supported EGL version at compile time
https://bugs.webkit.org/show_bug.cgi?id=197971
Summary
GLContextEGL should check the supported EGL version at compile time
Mads
Reported
2019-05-16 16:38:12 PDT
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
Attachments
Fix-patch
(2.05 KB, application/mbox)
2019-05-16 16:38 PDT
,
Mads
no flags
Details
Fix to check the EGL version at compile time
(2.22 KB, patch)
2019-05-17 15:27 PDT
,
Mads
mcatanzaro
: review+
mcatanzaro
: commit-queue-
Details
Formatted Diff
Diff
Error_log
(5.13 KB, text/plain)
2019-05-21 01:18 PDT
,
Mads
no flags
Details
Use a generic macro for EGL context version
(3.20 KB, patch)
2019-05-21 14:39 PDT
,
Mads
no flags
Details
Formatted Diff
Diff
Use a generic macro for EGL context version v2
(3.16 KB, patch)
2019-05-29 13:40 PDT
,
Mads
madhurkiran.h
: review?
mcatanzaro
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Michael Catanzaro
Comment 1
2019-05-16 19:55:55 PDT
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
Comment 2
2019-05-16 19:57:13 PDT
See also:
https://webkit.org/contributing-code/
Mads
Comment 3
2019-05-16 21:06:00 PDT
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
Comment 4
2019-05-17 09:42:03 PDT
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
Comment 5
2019-05-17 11:06:57 PDT
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
Comment 6
2019-05-17 12:04:04 PDT
Oh, you've *intentionally* left the final case as just #else if #endif Don't do that, please, it's not readable.
Mads
Comment 7
2019-05-17 15:24:01 PDT
Ok. Fixed it.
Mads
Comment 8
2019-05-17 15:27:21 PDT
Created
attachment 370161
[details]
Fix to check the EGL version at compile time
Michael Catanzaro
Comment 9
2019-05-18 08:04:17 PDT
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 10
2019-05-18 08:13:12 PDT
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
Comment 11
2019-05-18 10:08:05 PDT
Mads, can you copy/paste the build error you are seeing please?
Adrian Perez
Comment 12
2019-05-20 05:17:24 PDT
(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
Comment 13
2019-05-21 01:18:08 PDT
Created
attachment 370304
[details]
Error_log
Mads
Comment 14
2019-05-21 14:39:59 PDT
Created
attachment 370343
[details]
Use a generic macro for EGL context version
Adrian Perez
Comment 15
2019-05-27 16:12:43 PDT
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 16
2019-05-27 16:45:25 PDT
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
Comment 17
2019-05-29 13:40:16 PDT
Created
attachment 370881
[details]
Use a generic macro for EGL context version v2
Michael Catanzaro
Comment 18
2019-05-29 14:05:09 PDT
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
Comment 19
2019-05-29 15:41:36 PDT
I think this looks fine.
Michael Catanzaro
Comment 20
2019-05-30 08:23:46 PDT
Well it's not, because we require four-space indentation....
Mads
Comment 21
2019-06-07 13:52:25 PDT
If the "if" condition started at 8 space indentation why would it need 4 space to end?
Michael Catanzaro
Comment 22
2019-06-07 14:30:14 PDT
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
Comment 23
2019-06-07 15:02:25 PDT
Why not , as this conditional statement is within another conditional statement?
Michael Catanzaro
Comment 24
2019-06-08 11:05:04 PDT
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); }
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug