Bug 224786 – [CMake] Add OpenGLES2 targets

• Status: RESOLVED
• Resolution: FIXED
• Component: CMake

[Don Olmstead]

...

[Don Olmstead]

Created attachment 426485 [details]
Patch

[Don Olmstead]

Created attachment 426486 [details]
Patch

[Don Olmstead]

Created attachment 426489 [details]
Patch

[Don Olmstead]

Created attachment 426491 [details]
Patch

[Don Olmstead]

Created attachment 426558 [details]
WIP Patch

[EWS Watchlist]

Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE

[Don Olmstead]

Created attachment 426570 [details]
Patch

[Michael Catanzaro]

Comment on attachment 426570 [details]
Patch

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

> Source/ThirdParty/ANGLE/ChangeLog:9
> +        Add a ALIAS target mapping ANGLE's GLESv2 target to OpenGL::GLES for platforms that set
> +        USE_ANGLE_EGL.

Is this right though? If so, maybe USE_ANGLE_EGL should be renamed? I don't see any reason why using ANGLE for EGL should imply that it should also be used for GLES?

> Source/cmake/OptionsWPE.cmake:18
> +find_package(OpenGLES2 REQUIRED)

Let me CC Adrian. He will know whether this change is OK.

[Michael Catanzaro]

Comment on attachment 426570 [details]
Patch

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

> Source/cmake/FindOpenGLES2.cmake:62
> +    HINTS ${PC_OPENGLES2_INCLUDEDIR} ${PC_OPENGLES2_INCLUDE_DIRS} /usr/X11/include

/usr/X11/include?

> Source/cmake/FindOpenGLES2.cmake:71
> +# Look for OpenGL ES 3 headers existing to try and determine version
> +if (OpenGLES2_INCLUDE_DIR AND NOT OpenGLES2_VERSION)

Nit: your comment could say: "If version is unknown, look for..."

Because I got confused and was going to tell you to use pkg-config for this instead, until I realized that pkg_check_modules() will set OpenGLES2_VERSION.

[Don Olmstead]

Comment on attachment 426570 [details]
Patch

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

>> Source/ThirdParty/ANGLE/ChangeLog:9
>> +        USE_ANGLE_EGL.
> 
> Is this right though? If so, maybe USE_ANGLE_EGL should be renamed? I don't see any reason why using ANGLE for EGL should imply that it should also be used for GLES?

This is a weird one so I think I need to explain it better.

Basically if there is no OpenGL ES implementation, which is the case for Windows, then it should create that OpenGL::GLES target for use.

I do think the USE_ANGLE_EGL is bad and I really feel like everything should go through ANGLE or the system's OpenGL ES implementation rather than supporting desktop GL at all.

>> Source/cmake/FindOpenGLES2.cmake:71
>> +if (OpenGLES2_INCLUDE_DIR AND NOT OpenGLES2_VERSION)
> 
> Nit: your comment could say: "If version is unknown, look for..."
> 
> Because I got confused and was going to tell you to use pkg-config for this instead, until I realized that pkg_check_modules() will set OpenGLES2_VERSION.

The pkg-config stuff is weird. I saw it was reporting the version of whatever the GL implementation library was not the version of OpenGL ES. Not sure how we would want to resolve that since pkg-config isn't reporting what we'd want.

>> Source/cmake/OptionsWPE.cmake:18
>> +find_package(OpenGLES2 REQUIRED)
> 
> Let me CC Adrian. He will know whether this change is OK.

Below it sets HAVE_OPENGL_ES_3 and other things. Just for whatever reason it never did a find_package. Guessing the libWPE stuff ended up indirectly linking OpenGL ES.

[Michael Catanzaro]

(In reply to Don Olmstead from comment #10)
> >> Source/cmake/FindOpenGLES2.cmake:71
> >> +if (OpenGLES2_INCLUDE_DIR AND NOT OpenGLES2_VERSION)
> > 
> > Nit: your comment could say: "If version is unknown, look for..."
> > 
> > Because I got confused and was going to tell you to use pkg-config for this instead, until I realized that pkg_check_modules() will set OpenGLES2_VERSION.
> 
> The pkg-config stuff is weird. I saw it was reporting the version of
> whatever the GL implementation library was not the version of OpenGL ES. Not
> sure how we would want to resolve that since pkg-config isn't reporting what
> we'd want.

Yeah oops, my comment was totally wrong. I should have noticed that but I was looking at Fedora's glesv2.pc, which is provided by libglvnd. That just has version hardcoded to 3.2. But mesa's glesv2.pc has the mesa version there, which is how pkg-config files normally work.

The *proper* way to do this would be for mesa and libglvnd to install separate .pc files for each API separate version, but unfortunately they don't.

> Below it sets HAVE_OPENGL_ES_3 and other things. Just for whatever reason it
> never did a find_package. Guessing the libWPE stuff ended up indirectly
> linking OpenGL ES.

Ah, OK....

[Michael Catanzaro]

Comment on attachment 426570 [details]
Patch

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

> Source/cmake/FindOpenGLES2.cmake:44
> +``OpenGLES2_VERSION``
> +  the version of OpenGLES2.

"the version of OpenGLES2, which may be the version of the software implementing the OpenGL ES API, or may be the OpenGL ES API version."

Since there's no way to distinguish between the two, maybe it would be better to not export this variable at all. At least have a warning comment.

[Don Olmstead]

Created attachment 426587 [details]
Patch

[Michael Catanzaro]

Comment on attachment 426587 [details]
Patch

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

> Source/cmake/FindOpenGLES2.cmake:64
> +    HINTS ${PC_OPENGLES2_INCLUDEDIR} ${PC_OPENGLES2_INCLUDE_DIRS} /usr/X11/include

Why /usr/X11/include? That seems like a bad place for a GLESv2 header file?

> Source/cmake/FindOpenGLES2.cmake:88
> +# Look for OpenGL ES 3 headers existing to determine the API version
> +if (OpenGLES2_INCLUDE_DIR)
> +    if (EXISTS ${OpenGLES2_INCLUDE_DIR}/GLES3/gl32.h)
> +        set(OpenGLES2_API_VERSION "3.2")
> +    elseif (EXISTS ${OpenGLES2_INCLUDE_DIR}/GLES3/gl31.h)
> +        set(OpenGLES2_API_VERSION "3.1")
> +    elseif (EXISTS ${OpenGLES2_INCLUDE_DIR}/GLES3/gl3.h)
> +        set(OpenGLES2_API_VERSION "3.0")
> +    else ()
> +        set(OpenGLES2_API_VERSION "2.0")
> +    endif ()
> +
> +    # Set the version of the library to the API version if not present
> +    if (NOT OpenGLES2_VERSION)
> +        set(OpenGLES2_VERSION ${OpenGLES2_API_VERSION})
> +    endif ()
> +endif ()

LGTM, but we would need a graphics developer to say whether this really makes sense.

[Michael Catanzaro]

CMake changes seem fine.

[Don Olmstead]

Comment on attachment 426587 [details]
Patch

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

>> Source/cmake/FindOpenGLES2.cmake:64
>> +    HINTS ${PC_OPENGLES2_INCLUDEDIR} ${PC_OPENGLES2_INCLUDE_DIRS} /usr/X11/include
> 
> Why /usr/X11/include? That seems like a bad place for a GLESv2 header file?

That was from the prior version of the file. I'm fine with removing it if that makes sense.

[Michael Catanzaro]

(In reply to Don Olmstead from comment #16)
> That was from the prior version of the file. I'm fine with removing it if
> that makes sense.

Doesn't look like it. The current version of the file has:

find_path(OPENGLES2_INCLUDE_DIRS NAMES GLES2/gl2.h
    HINTS ${PC_OPENGLES2_INCLUDEDIR} ${PC_OPENGLES2_INCLUDE_DIRS}
)

I think you've copy/pasted it there somehow by mistake. ;)

[Don Olmstead]

Comment on attachment 426587 [details]
Patch

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

>>> Source/cmake/FindOpenGLES2.cmake:64
>>> +    HINTS ${PC_OPENGLES2_INCLUDEDIR} ${PC_OPENGLES2_INCLUDE_DIRS} /usr/X11/include
>> 
>> Why /usr/X11/include? That seems like a bad place for a GLESv2 header file?
> 
> That was from the prior version of the file. I'm fine with removing it if that makes sense.

You're right it snuck in from somewhere else. Will fix that.

[Adrian Perez]

Comment on attachment 426587 [details]
Patch

Patch LGTM as well. Please remember to remove the X11 include before landing :)


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

>> Source/cmake/FindOpenGLES2.cmake:88
>> +endif ()
> 
> LGTM, but we would need a graphics developer to say whether this really makes sense.

At least the switcheroo checking for headers is fine. The specs indicate which
headers must be available at different API levels, and as GLESv2 is the minimum
we support, making it fallback makes sense.

[Don Olmstead]

Created attachment 426687 [details]
Patch

[Don Olmstead]

Created attachment 426688 [details]
Patch

[Don Olmstead]

Created attachment 426689 [details]
Patch

[EWS]

Committed r276368 (236846@main): <https://commits.webkit.org/236846@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426689 [details].

[Alberto Garcia]

WebKitGTK 2.33.1 fails to build because of this patch

FAILED: Source/WebCore/CMakeFiles/WebCore.dir/platform/graphics/opengl/GraphicsContextGLOpenGLCommon.cpp.o 
In file included from ../Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLCommon.cpp:61:
../Source/WebCore/platform/graphics/OpenGLESShims.h:48: warning: "GL_COLOR_ATTACHMENT0_EXT" redefined
   48 | #define GL_COLOR_ATTACHMENT0_EXT GL_COLOR_ATTACHMENT0
      | 
In file included from ../Source/WebCore/platform/graphics/opengl/ExtensionsGLOpenGLES.h:37,
                 from ../Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLCommon.cpp:35:
/usr/include/GLES2/gl2ext.h:1336: note: this is the location of the previous definition
 1336 | #define GL_COLOR_ATTACHMENT0_EXT          0x8CE0
      | 
../Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLCommon.cpp: In member function ‘virtual void WebCore::GraphicsContextGLOpenGL::copyBufferSubData(GCGLenum, GCGLenum, GCGLintptr, GCGLintptr, GCGLsizeiptr)’:
../Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLCommon.cpp:2206:7: error: ‘::glCopyBufferSubData’ has not been declared; did you mean ‘copyBufferSubData’?
 2206 |     ::glCopyBufferSubData(readTarget, writeTarget, readOffset, writeOffset, size);
      |       ^~~~~~~~~~~~~~~~~~~
      |       copyBufferSubData
../Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLCommon.cpp: In member function ‘virtual void WebCore::GraphicsContextGLOpenGL::getBufferSubData(GCGLenum, GCGLintptr, GCGLSpan<void>)’:
../Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLCommon.cpp:2219:23: error: ‘::glMapBufferRange’ has not been declared; did you mean ‘bindBufferRange’?
 2219 |     GCGLvoid* ptr = ::glMapBufferRange(target, offset, data.bufSize, GraphicsContextGL::MAP_READ_BIT);
      |                       ^~~~~~~~~~~~~~~~
      |                       bindBufferRange

[Alberto Garcia]

(In reply to Alberto Garcia from comment #24)
> WebKitGTK 2.33.1 fails to build because of this patch

Bug 225867