Bug 235182

Summary: [GTK][WPE] Share code between NicosiaGCGLLayer+ANGLE and NicosiaImageBufferPipe
Product: WebKit Reporter: Chris Lord <clord>
Component: ANGLEAssignee: Chris Lord <clord>
Status: RESOLVED FIXED    
Severity: Normal CC: alex, annulen, cdumez, cmarcelo, dino, ews-watchlist, gyuyoung.kim, kbr, kkinnunen, kondapallykalyan, luiz, pnormand, ryuan.choi, sergio, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Lord 2022-01-13 07:27:57 PST
Currently, these two classes are doing basically the same thing when ANGLE is enabled. I think we should remove the ANGLE-specific code from NicosiaGCGLLayer and that NicosiaGCGLANGLELayer should be a sub-class of NicosiaImageBufferPipe so that they can share the code and OffscreenCanvas can benefit when we get copy-less buffer transfers for WebGL+ANGLE.
Comment 1 Chris Lord 2022-01-13 07:45:07 PST
Created attachment 449058 [details]
Patch
Comment 2 Chris Lord 2022-01-13 07:47:58 PST
Perhaps it's coincidence, but this also seems to have fixed some flickering I was seeing prior to this patch - I don't believe this works in any significantly different way to before though, so maybe just a driver bug exposed by timing differences or something...
Comment 3 Alejandro G. Castro 2022-01-14 04:02:51 PST
Comment on attachment 449058 [details]
Patch

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

Great patch! Very helpful for the next refactorings.

> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.h:45
> -class GCGLANGLELayer;
> +class GCGLANGLEPipe;

Can we ifdef USE(ANGLE) like we do in the attribute definition?

> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLEPipe.h:51
> +class GCGLANGLEPipeSource;

Leftover?
Comment 4 Chris Lord 2022-01-14 04:16:50 PST
Thanks for the review!

(In reply to Alejandro G. Castro from comment #3)
> Comment on attachment 449058 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449058&action=review
> 
> Great patch! Very helpful for the next refactorings.
> 
> > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.h:45
> > -class GCGLANGLELayer;
> > +class GCGLANGLEPipe;
> 
> Can we ifdef USE(ANGLE) like we do in the attribute definition?

I think it's convention to just declare the class regardless (at least in a previous patch where I did what you're suggesting, a review comment was to remove the #ifdef :))

> > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLEPipe.h:51
> > +class GCGLANGLEPipeSource;
> 
> Leftover?

Nice catch, thanks :)
Comment 5 Chris Lord 2022-01-14 04:18:40 PST
Created attachment 449159 [details]
Patch
Comment 6 EWS 2022-01-14 04:56:15 PST
Committed r288014 (246040@main): <https://commits.webkit.org/246040@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449159 [details].
Comment 7 Radar WebKit Bug Importer 2022-01-14 04:57:19 PST
<rdar://problem/87596622>
Comment 8 Philippe Normand 2022-01-15 05:26:44 PST
This broke clean WPE builds, both clang and gcc. There's a bunch of errors, here's an extract:

1228/7908] Building CXX object Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/common/PackedEGLEnums_autogen.cpp.o
FAILED: Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/common/PackedEGLEnums_autogen.cpp.o 
/usr/bin/sccache /usr/bin/c++ -DANGLE_ENABLE_ESSL -DANGLE_ENABLE_GLSL -DANGLE_PLATFORM_LINUX -DBUILDING_WITH_CMAKE=1 -DBUILDING_WPE__=1 -DBWRAP_EXECUTABLE=\"/usr/bin/bwrap\" -DDBUS_PROXY_EXECUTABLE=\"/usr/bin/xdg-dbus-proxy\" -DEGL_EGL_PROTOTYPES=0 -DGETTEXT_PACKAGE=\"WPE\" -DGL_GLES_PROTOTYPES=0 -DHAVE_CONFIG_H=1 -DJSC_GLIB_API_ENABLED -DLIBANGLE_IMPLEMENTATION -DPAS_BMALLOC=1 -I/app/webkit/Source/ThirdParty/ANGLE/include -I/app/webkit/Source/ThirdParty/ANGLE/include/KHR -I/app/webkit/Source/ThirdParty/ANGLE/src -I/app/webkit/Source/ThirdParty/ANGLE/src/common/third_party/base -I/app/webkit/Source/ThirdParty/ANGLE/third_party/zlib/google -I/app/webkit/WebKitBuild/Release/Source/ThirdParty/ANGLE/include -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-stringop-overread -Wno-nonnull -Wno-array-bounds -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-tautological-compare  -fno-strict-aliasing -fno-exceptions -fno-rtti -O3 -DNDEBUG -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -Wno-cast-align -Wno-extra -Wno-suggest-attribute=format -Wno-undef -Wno-unused-parameter -Wno-return-type -Wno-comment -std=c++20 -MD -MT Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/common/PackedEGLEnums_autogen.cpp.o -MF Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/common/PackedEGLEnums_autogen.cpp.o.d -o Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/common/PackedEGLEnums_autogen.cpp.o -c /app/webkit/Source/ThirdParty/ANGLE/src/common/PackedEGLEnums_autogen.cpp
[1230/7908] Building CXX object Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/common/PackedEnums.cpp.o
FAILED: Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/common/PackedEnums.cpp.o 
/usr/bin/sccache /usr/bin/c++ -DANGLE_ENABLE_ESSL -DANGLE_ENABLE_GLSL -DANGLE_PLATFORM_LINUX -DBUILDING_WITH_CMAKE=1 -DBUILDING_WPE__=1 -DBWRAP_EXECUTABLE=\"/usr/bin/bwrap\" -DDBUS_PROXY_EXECUTABLE=\"/usr/bin/xdg-dbus-proxy\" -DEGL_EGL_PROTOTYPES=0 -DGETTEXT_PACKAGE=\"WPE\" -DGL_GLES_PROTOTYPES=0 -DHAVE_CONFIG_H=1 -DJSC_GLIB_API_ENABLED -DLIBANGLE_IMPLEMENTATION -DPAS_BMALLOC=1 -I/app/webkit/Source/ThirdParty/ANGLE/include -I/app/webkit/Source/ThirdParty/ANGLE/include/KHR -I/app/webkit/Source/ThirdParty/ANGLE/src -I/app/webkit/Source/ThirdParty/ANGLE/src/common/third_party/base -I/app/webkit/Source/ThirdParty/ANGLE/third_party/zlib/google -I/app/webkit/WebKitBuild/Release/Source/ThirdParty/ANGLE/include -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-stringop-overread -Wno-nonnull -Wno-array-bounds -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-tautological-compare  -fno-strict-aliasing -fno-exceptions -fno-rtti -O3 -DNDEBUG -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -Wno-cast-align -Wno-extra -Wno-suggest-attribute=format -Wno-undef -Wno-unused-parameter -Wno-return-type -Wno-comment -std=c++20 -MD -MT Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/common/PackedEnums.cpp.o -MF Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/common/PackedEnums.cpp.o.d -o Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/common/PackedEnums.cpp.o -c /app/webkit/Source/ThirdParty/ANGLE/src/common/PackedEnums.cpp
[1231/7908] Building CXX object Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/compiler/translator/tree_ops/gl/UseInterfaceBlockFields.cpp.o
FAILED: Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/compiler/translator/tree_ops/gl/UseInterfaceBlockFields.cpp.o 
/usr/bin/sccache /usr/bin/c++ -DANGLE_ENABLE_ESSL -DANGLE_ENABLE_GLSL -DANGLE_PLATFORM_LINUX -DBUILDING_WITH_CMAKE=1 -DBUILDING_WPE__=1 -DBWRAP_EXECUTABLE=\"/usr/bin/bwrap\" -DDBUS_PROXY_EXECUTABLE=\"/usr/bin/xdg-dbus-proxy\" -DEGL_EGL_PROTOTYPES=0 -DGETTEXT_PACKAGE=\"WPE\" -DGL_GLES_PROTOTYPES=0 -DHAVE_CONFIG_H=1 -DJSC_GLIB_API_ENABLED -DLIBANGLE_IMPLEMENTATION -DPAS_BMALLOC=1 -I/app/webkit/Source/ThirdParty/ANGLE/include -I/app/webkit/Source/ThirdParty/ANGLE/include/KHR -I/app/webkit/Source/ThirdParty/ANGLE/src -I/app/webkit/Source/ThirdParty/ANGLE/src/common/third_party/base -I/app/webkit/Source/ThirdParty/ANGLE/third_party/zlib/google -I/app/webkit/WebKitBuild/Release/Source/ThirdParty/ANGLE/include -fdiagnostics-color=always -Wextra -Wall -Wno-expansion-to-defined -Wno-stringop-overread -Wno-nonnull -Wno-array-bounds -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-tautological-compare  -fno-strict-aliasing -fno-exceptions -fno-rtti -O3 -DNDEBUG -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -Wno-cast-align -Wno-extra -Wno-suggest-attribute=format -Wno-undef -Wno-unused-parameter -Wno-return-type -Wno-comment -std=c++20 -MD -MT Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/compiler/translator/tree_ops/gl/UseInterfaceBlockFields.cpp.o -MF Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/compiler/translator/tree_ops/gl/UseInterfaceBlockFields.cpp.o.d -o Source/ThirdParty/ANGLE/CMakeFiles/ANGLE.dir/src/compiler/translator/tree_ops/gl/UseInterfaceBlockFields.cpp.o -c /app/webkit/Source/ThirdParty/ANGLE/src/compiler/translator/tree_ops/gl/UseInterfaceBlockFields.cpp
Comment 9 Philippe Normand 2022-01-15 05:38:53 PST
Ah sorry, this is happening elsewhere in my build, something related with an empty llint offsets headers...