Bug 235182 – [GTK][WPE] Share code between NicosiaGCGLLayer+ANGLE and NicosiaImageBufferPipe

• Status: RESOLVED
• Resolution: FIXED
• Component: ANGLE

[Chris Lord]

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.

[Chris Lord]

Created attachment 449058 [details]
Patch

[Chris Lord]

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

[Alejandro G. Castro]

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?

[Chris Lord]

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 :)

[Chris Lord]

Created attachment 449159 [details]
Patch

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/87596622>

[Philippe Normand]

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

[Philippe Normand]

Ah sorry, this is happening elsewhere in my build, something related with an empty llint offsets headers...