Bug 225563

Summary:

[GTK][WPE][WebGL2] compilation fixes

Product:

WebKit

Reporter:

Matt Turner <mattst88>

Component:

WebGL

Assignee:

Alejandro G. Castro <alex>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

alex, annulen, cdumez, changseok, clord, dino, don.olmstead, esprehn+autocc, ews-watchlist, gyuyoung.kim, Hironori.Fujii, kbr, kkinnunen, kondapallykalyan, magomez, ryuan.choi, sergio, webkit-bug-importer, zdobersek

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

235178

Attachments:

Description	Flags
patch	none
Patch	none
Patch	none

Matt Turner

Reported 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.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Kenneth Russell

Comment 1 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.

Kenneth Russell

Comment 2 2021-05-10 17:00:36 PDT

CC'ing a couple of folks working on the WPE port.

Fujii Hironori

Comment 3 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?

Matt Turner

Comment 4 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.

Don Olmstead

Comment 5 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.

Fujii Hironori

Comment 6 2021-06-07 19:42:55 PDT

Adoption of ANGLE in WPE/WebKitGTK - YouTube https://youtu.be/nu3Zd20IntM

Alejandro G. Castro

Comment 7 2022-01-12 10:32:14 PST

*** Bug 235120 has been marked as a duplicate of this bug. ***

Matt Turner

Comment 8 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.

Alejandro G. Castro

Comment 9 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.

Alejandro G. Castro

Comment 10 2022-01-12 10:53:39 PST

Created attachment 448965 [details] Patch

Fujii Hironori

Comment 11 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.

Kenneth Russell

Comment 12 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.

Alejandro G. Castro

Comment 13 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.

Alejandro G. Castro

Comment 14 2022-01-13 03:25:26 PST

Created attachment 449040 [details] Patch

Chris Lord

Comment 15 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.

Alejandro G. Castro

Comment 16 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.

EWS

Comment 17 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].

Radar WebKit Bug Importer

Comment 18 2022-01-13 11:05:17 PST

<rdar://problem/87560252>

Fujii Hironori

Comment 19 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.

Alejandro G. Castro

Comment 20 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.

Note You need to log in before you can comment on or make changes to this bug.