Bug 225563 - [GTK][WPE][WebGL2] compilation fixes
Summary: [GTK][WPE][WebGL2] compilation fixes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alejandro G. Castro
URL:
Keywords: InRadar
: 235120 (view as bug list)
Depends on:
Blocks: 235178
  Show dependency treegraph
 
Reported: 2021-05-08 14:22 PDT by Matt Turner
Modified: 2022-01-13 12:03 PST (History)
19 users (show)

See Also:


Attachments
patch (17.32 KB, patch)
2021-05-08 14:22 PDT, Matt Turner
no flags Details | Formatted Diff | Diff
Patch (5.06 KB, patch)
2022-01-12 10:53 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff
Patch (5.20 KB, patch)
2022-01-13 03:25 PST, Alejandro G. Castro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Turner 2021-05-08 14:22:19 PDT
Created attachment 428092 [details]
patch

When compiling WebKit GTK on Linux with -DENABLE_WEBGL2=ON, I get compilation errors about primitiveRestartIndex:

/var/tmp/portage/net-libs/webkit-gtk-2.32.0/work/webkitgtk-2.32.0/Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp: In member function ‘void WebCore::WebGLRenderingContextBase::drawElements(GCGLenum, GCG
Lsizei, GCGLenum, long long int)’:
/var/tmp/portage/net-libs/webkit-gtk-2.32.0/work/webkitgtk-2.32.0/Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:2647:20: error: ‘class WebCore::GraphicsContextGL’ has no member named ‘primitiveRestart
Index’
 2647 |         m_context->primitiveRestartIndex(getRestartIndex(type));
      |                    ^~~~~~~~~~~~~~~~~~~~~

Fixing that (in PATCH 2/4 in the attached patch series) lead to an undefined reference at linking:

lib/libWebCoreGTK.a(lib/../Source/WebCore/CMakeFiles/WebCore.dir/html/canvas/WebGLRenderingContextBase.cpp.o):WebGLRenderingContextBase.cpp:function WebCore::WebGLRenderingContextBase::drawElements(unsigned int, int, unsigned int, long long): error: undefined reference to 'WebCore::GraphicsContextGLOpenGL::primitiveRestartIndex(unsigned int)'
lib/libWebCoreGTK.a(lib/../Source/WebCore/CMakeFiles/WebCore.dir/html/canvas/WebGLRenderingContextBase.cpp.o):WebGLRenderingContextBase.cpp:function WebCore::WebGLRenderingContextBase::drawElementsInstanced(unsigned int, int, unsigned int, long long, int): error: undefined reference to 'WebCore::GraphicsContextGLOpenGL::primitiveRestartIndex(unsigned int)'
collect2: error: ld returned 1 exit status

which I fixed by adding the function to the OpenGL shim code.

I noticed that the OPENGL_4 macro is also never defined and speculatively changed it to USE(OPENGL), which necessitated adding some other functions to the shim.

Never contributed to WebKit before. Probably did some stuff wrong. The attached patch is a git formatted patch against the github repo.
Comment 1 Kenneth Russell 2021-05-10 16:59:51 PDT
Thanks for the patch and sorry about the build breakage.

WebGL2 in WebKit will only be supported on top of ANGLE. There is too much deep validation of OpenGL context and object states that ANGLE handles to replicate this into WebGLRenderingContextBase / WebGL2RenderingContext.

I suggest you pursue the route of building with USE_ANGLE=1 in your setup. ANGLE's already well supported on Linux, so this will hopefully not be too difficult. Then ENABLE_WEBGL2=ON should "just work".

Please feel free to join Chromium's Slack instance; there's an #angle-for-webkit channel there where developers working on other WebKit ports, as well as ANGLE developers, hang out and can help with any issues you run into.
Comment 2 Kenneth Russell 2021-05-10 17:00:36 PDT
CC'ing a couple of folks working on the WPE port.
Comment 3 Fujii Hironori 2021-05-10 17:38:04 PDT
GTK port has a CMake build option USE_ANGLE_WEBGL for it.
Zan summarized the problem of using ANGLE for WebGL in Coordinated Graphics the other day in WebKit slack. But, the log was expired due to Slack free plan. 
Zan, ChangSeok: Could you summarize USE_ANGLE_WEBGL current status?
Comment 4 Matt Turner 2021-05-11 12:04:58 PDT
Thanks for the comments, all!

I see the following in Source/cmake/OptionsGTK.cmake:

# Private options specific to the GTK port. Changing these options is
# completely unsupported. They are intended for use only by WebKit developers.
WEBKIT_OPTION_DEFINE(USE_ANGLE_WEBGL "Whether to use ANGLE as WebGL backend." PRIVATE OFF)

That seems to suggest that I should not ship WebKit with DUSE_ANGLE_WEBGL=ON to Gentoo Linux users? (I'm the Gentoo Linux packager) And by implication I shouldn't enable WebGL2 either?

Building with -DUSE_ANGLE_WEBGL=ON, I get undefined references when linking lib/libGLESv2.so:

> lib/libANGLE.a(lib/../Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/libANGLE/Display.cpp.o):Display.cpp:function egl::Display::GetDisplayFromNativeDisplay(_XDisplay*, egl::AttributeMap const&): error: undefined reference to 'rx::DisplayGLX::DisplayGLX(egl::DisplayState const&)'                                                                                                                                             
> lib/libANGLE.a(lib/../Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/libANGLE/Display.cpp.o):Display.cpp:function egl::Display::GetDisplayFromNativeDisplay(_XDisplay*, egl::AttributeMap const&): error: undefined reference to 'rx::DisplayEGL::DisplayEGL(egl::DisplayState const&)'                                                                                                                                             

and indeed I can't find DisplayEGL.cpp or DisplayGLX.cpp (which contain those definitions) listed in any CMakeLists.txt or *.cmake. So maybe this isn't wired up?

I'm happy to leave WebGL2 disabled; I just need guidance on what's expected to work.
Comment 5 Don Olmstead 2021-05-12 13:40:14 PDT
I have some build fixes for ANGLE on top of a system GL/GLES in https://bugs.webkit.org/show_bug.cgi?id=224888 which would be required for WebGL 2 to work on GTK.
Comment 6 Fujii Hironori 2021-06-07 19:42:55 PDT
Adoption of ANGLE in WPE/WebKitGTK - YouTube
https://youtu.be/nu3Zd20IntM
Comment 7 Alejandro G. Castro 2022-01-12 10:32:14 PST
*** Bug 235120 has been marked as a duplicate of this bug. ***
Comment 8 Matt Turner 2022-01-12 10:40:33 PST
I don't know what the typical webkit patch workflow is, and I don't know for sure if the attached patch still applies, but if it does, I think it should be accepted.
Comment 9 Alejandro G. Castro 2022-01-12 10:46:24 PST
(In reply to Matt Turner from comment #8)
> I don't know what the typical webkit patch workflow is, and I don't know for
> sure if the attached patch still applies, but if it does, I think it should
> be accepted.

Thanks for your report and the patch! Don't worry, the patch is different, we are adding support using ANGLE, I'll upload it in a few minutes. The architecture is still not final but we are allowing compiling WEBGL2 already for testing purposes.
Comment 10 Alejandro G. Castro 2022-01-12 10:53:39 PST
Created attachment 448965 [details]
Patch
Comment 11 Fujii Hironori 2022-01-12 12:18:06 PST
Comment on attachment 448965 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:8033
> +#if USE(OPENGL) && ENABLE(WEBGL2) && !USE(ANGLE)

Cocoa port no longer uses USE_OPENGL macro (Bug 217374)
WebKit has two WebGL implementations, USE(ANGLE) one and !USE(ANGLE) one.
These conditions should be #if ENABLE(WEBGL2) && !USE(ANGLE).
And, there is one more condition in this file. It should be replaced too.
Comment 12 Kenneth Russell 2022-01-12 14:22:50 PST
Comment on attachment 448965 [details]
Patch

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

>> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:8033
>> +#if USE(OPENGL) && ENABLE(WEBGL2) && !USE(ANGLE)
> 
> Cocoa port no longer uses USE_OPENGL macro (Bug 217374)
> WebKit has two WebGL implementations, USE(ANGLE) one and !USE(ANGLE) one.
> These conditions should be #if ENABLE(WEBGL2) && !USE(ANGLE).
> And, there is one more condition in this file. It should be replaced too.

Further: WebGL 2.0 is no longer supported in WebKit without ANGLE. These code blocks are historical and can be deleted - but that cleanup should probably be done in a different patch.
Comment 13 Alejandro G. Castro 2022-01-13 00:24:41 PST
(In reply to Kenneth Russell from comment #12)
> Comment on attachment 448965 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448965&action=review
> 
> >> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:8033
> >> +#if USE(OPENGL) && ENABLE(WEBGL2) && !USE(ANGLE)
> > 
> > Cocoa port no longer uses USE_OPENGL macro (Bug 217374)
> > WebKit has two WebGL implementations, USE(ANGLE) one and !USE(ANGLE) one.
> > These conditions should be #if ENABLE(WEBGL2) && !USE(ANGLE).
> > And, there is one more condition in this file. It should be replaced too.
> 
> Further: WebGL 2.0 is no longer supported in WebKit without ANGLE. These
> code blocks are historical and can be deleted - but that cleanup should
> probably be done in a different patch.

Thanks for the reviews, Fuiji and Kenneth! :-)

I agree, in my opinion it is a better idea to remove all the historical blocks in specific commits that explain the change instead or removing them here and there in differentn commits.

We are in the middle of the process of enabling it so in our case waiting until we have the final architecture patch in place it is a good idea to avoid rebasing issues.
Comment 14 Alejandro G. Castro 2022-01-13 03:25:26 PST
Created attachment 449040 [details]
Patch
Comment 15 Chris Lord 2022-01-13 03:29:14 PST
Comment on attachment 449040 [details]
Patch

LGTM - I suppose we need to file a bug about cleaning up USE(OPENGL) && ENABLE(WEBGL2) at some point.
Comment 16 Alejandro G. Castro 2022-01-13 04:13:46 PST
(In reply to Chris Lord from comment #15)
> Comment on attachment 449040 [details]
> Patch
> 
> LGTM - I suppose we need to file a bug about cleaning up USE(OPENGL) &&
> ENABLE(WEBGL2) at some point.

Thanks! Created bug 235178. I'm going to check if there are many places.
Comment 17 EWS 2022-01-13 11:04:37 PST
Committed r287983 (246012@main): <https://commits.webkit.org/246012@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449040 [details].
Comment 18 Radar WebKit Bug Importer 2022-01-13 11:05:17 PST
<rdar://problem/87560252>
Comment 19 Fujii Hironori 2022-01-13 12:00:20 PST
Comment on attachment 449040 [details]
Patch

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

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:-2823
> -        m_context->primitiveRestartIndex(getRestartIndex(type));

getRestartIndex becomes a unused function now. You should remove it.
Comment 20 Alejandro G. Castro 2022-01-13 12:03:34 PST
(In reply to Fujii Hironori from comment #19)
> Comment on attachment 449040 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449040&action=review
> 
> > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:-2823
> > -        m_context->primitiveRestartIndex(getRestartIndex(type));
> 
> getRestartIndex becomes a unused function now. You should remove it.

Thanks for pointing that out, it is removed in the patch 235178 that is focused in removing all the code inside USE(OPENGL) && ENABLE(WEBGL2) defines. I think it already landed.