[CMake] Support USE_ANGLE_EGL on additional platforms

• Component: CMake
• Status: RESOLVED FIXED

[Don Olmstead]

...

[Don Olmstead]

Created attachment 426733 [details]
WIP Patch

[Don Olmstead]

Created attachment 426763 [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 426951 [details]
WIP Patch

[Radar WebKit Bug Importer]

<rdar://problem/77280211>

[Don Olmstead]

Created attachment 428411 [details]
Patch

[Kenneth Russell]

Comment on attachment 428411 [details]
Patch

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

Looks good to me overall - a few small comments. The real proof is whether things build with these changes. r+

> Source/ThirdParty/ANGLE/PlatformGTK.cmake:49
> +        list(APPEND ANGLE_DEFINITIONS WL_EGL_PLATFORM)

Understanding the comment above, still, won't some Wayland definitions be lost with this if/elseif construction if both X11 and Wayland are defined? Should that be fixed?

> Source/ThirdParty/ANGLE/PlatformGTK.cmake:50
> +    else ()

Wondering whether this should be something like if (!ENABLE_X11_TARGET && !ENABLE_WAYLAND_TARGET), and detached from this if/else chain.

> Source/WebKit/CMakeLists.txt:320
> +    # Prepend to make sure the ANGLE headers are found before system headers

This kind of search path manipulation seemed fragile, which is why the code elsewhere in WebKit that calls into ANGLE on macOS uses more explicitly-scoped include paths, and processing is/was done when publishing these headers to the outside world. Not 100% sure if this is exactly the same issue, but something to think about.

> Tools/Scripts/update-angle:169
> +./gni-to-cmake.py src/libANGLE/renderer/metal/BUILD.gn Metal.cmake --prepend 'src/libANGLE/renderer/metal/'

Great to see these updates.

[Michael Catanzaro]

Comment on attachment 428411 [details]
Patch

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

>> Source/WebKit/CMakeLists.txt:320
>> +    # Prepend to make sure the ANGLE headers are found before system headers
> 
> This kind of search path manipulation seemed fragile, which is why the code elsewhere in WebKit that calls into ANGLE on macOS uses more explicitly-scoped include paths, and processing is/was done when publishing these headers to the outside world. Not 100% sure if this is exactly the same issue, but something to think about.

Won't this cause the UI process to link to ANGLE? I think the UI process should not be allowed to link to ANGLE? This could cause weird issues in applications that don't expect to wind up using WebKit's special private OpenGL rather than mesa, right? I wonder what Adrian thinks.

[Don Olmstead]

Created attachment 429774 [details]
WIP Patch

Igalia has hikiko working on ANGLE. Updating the patch to hopefully get her closer to a full build.

[Don Olmstead]

Created attachment 429780 [details]
WIP Patch

[Don Olmstead]

Created attachment 429783 [details]
WIP Patch

[Don Olmstead]

Created attachment 429785 [details]
WIP Patch

[Don Olmstead]

Created attachment 429791 [details]
Patch

[Don Olmstead]

Comment on attachment 428411 [details]
Patch

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

>> Source/ThirdParty/ANGLE/PlatformGTK.cmake:50
>> +    else ()
> 
> Wondering whether this should be something like if (!ENABLE_X11_TARGET && !ENABLE_WAYLAND_TARGET), and detached from this if/else chain.

This should be fixed in the patch for landing

>>> Source/WebKit/CMakeLists.txt:320
>>> +    # Prepend to make sure the ANGLE headers are found before system headers
>> 
>> This kind of search path manipulation seemed fragile, which is why the code elsewhere in WebKit that calls into ANGLE on macOS uses more explicitly-scoped include paths, and processing is/was done when publishing these headers to the outside world. Not 100% sure if this is exactly the same issue, but something to think about.
> 
> Won't this cause the UI process to link to ANGLE? I think the UI process should not be allowed to link to ANGLE? This could cause weird issues in applications that don't expect to wind up using WebKit's special private OpenGL rather than mesa, right? I wonder what Adrian thinks.

This is potentially fragile BUT this shouldn't be a problem for GTK Michael.

USE_ANGLE_EGL means use ANGLE as the OpenGL ES implementation for everything. So GTK won't hit that because it compiles with USE_ANGLE_WEBGL which just means use ANGLE for WebGL support and that's it.

[Don Olmstead]

Ok so I believe I have addressed all concerns with this patch.

The GTK EWS for the last patch ran successfully with USE_ANGLE_WEBGL enabled. It had some local changes that aren't for upstream but were just to get a green build. I'm sure there's probably more changes for GTK but I feel this patch is good to go and anything else with getting GTK working should be done in a follow up.

[Don Olmstead]

Created attachment 429792 [details]
Patch for Igalia to get started

This contains the local changes to get EWS to complete successfully to continue investigation of turning on ANGLE for WebGL 2 on GTK.

[Kenneth Russell]

Comment on attachment 429791 [details]
Patch

Latest patch still looks good to me.

[Michael Catanzaro]

(In reply to Don Olmstead from comment #14)
> This is potentially fragile BUT this shouldn't be a problem for GTK Michael.
> 
> USE_ANGLE_EGL means use ANGLE as the OpenGL ES implementation for
> everything. So GTK won't hit that because it compiles with USE_ANGLE_WEBGL
> which just means use ANGLE for WebGL support and that's it.

OK, I agree this would only be a problem if USE_ANGLE_EGL were to be turned on.

[EWS]

Committed r278130 (238180@main): <https://commits.webkit.org/238180@main>

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