Bug 199060

Exciting that the GTK port will use ANGLE too!

I made a patch that renders webgl layers with ANGLE on top of GLX texture mapper and would start send it to upstream. The outline is here: https://github.com/shivamidow/webkit/tree/angle-webgl Sharing webgl textures beween GLX and ANGLE egl contexts seems not feasible so I chose a readpixel-writetexture approach for now. (i.e., webgl scene -- (read pixels) --> texture for texture mapper) Rather than trying to land the patch once, I will divide the work into multiple steps as follows. 1. Remove GraphicsContext3D dependencies from texture mapper and coordinate graphics code. It will be used by WebGL code only. 2. Land ANGLE support for WebGL 3. Make other rendering backends (e.g., opengl es) adopt ANGLE webgl. 4. Replace the readpixel part with texture sharing. 5. Bug fix and other improvement

Created attachment 379104 [details] Patch

Note that there are important steps to take when updating ANGLE. See http://trac.webkit.org/wiki/UpdatingANGLE

(In reply to ChangSeok Oh from comment #3) > Created attachment 379104 [details] > Patch This is a build test for EWS, not a final patch.

Created attachment 379114 [details] Patch No review required yet. EWS test purpose.

Created attachment 379115 [details] Patch

The Win-cairo bot keeps complaining as follow but it looks not related to this change. > patching file Source/ThirdParty/ANGLE/PlatformGTK.cmake > patch: **** Can't create file C:\Users\ContainerAdministrator\AppData\Local\Temp/ppz13552 : File exists > patching file Source/ThirdParty/ANGLE/include/CMakeLists.txt

This seems ok. Zan, do you have an opinion?

This approach of download-reupload is the only one we can take up now, but we'll have to modify ANGLE possibly a lot in order to get everything in place to work without this bypass but instead use textures between ANGLE and non-ANGLE GL paths. So this is a good first approach. I'll review it tomorrow.

Comment on attachment 379115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379115&action=review > Source/WebCore/platform/graphics/angle/GLContextANGLE.h:33 > +class GLContextANGLE final : public GLContext { I think this class can be avoided altogether. Instead of it, I would recommend packing all this logic into a new Nicosia::GC3DANGLELayer class. This would give us an easier time integrating with ANGLE, without having to work through the GLContext interface. > Source/WebCore/platform/graphics/texmap/GraphicsContext3DTextureMapper.cpp:266 > +#if !USE(ANGLE) I don't think this guard is needed -- this is already non-USE(ANGLE) code. > Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:86 > -#if USE(OPENGL_ES) > +#if USE(ANGLE) > + std::unique_ptr<Extensions3DANGLE> glExtensions = makeUnique<Extensions3DANGLE>(nullptr, GLContext::current()->version() >= 320); > +#elif USE(OPENGL_ES) This change is fine on its own, but this check should be done without any usage of the Extensions3D* classes. This can be fixed after the fact.

Created attachment 383236 [details] Patch

Comment on attachment 379115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=379115&action=review >> Source/WebCore/platform/graphics/angle/GLContextANGLE.h:33 >> +class GLContextANGLE final : public GLContext { > > I think this class can be avoided altogether. Instead of it, I would recommend packing all this logic into a new Nicosia::GC3DANGLELayer class. This would give us an easier time integrating with ANGLE, without having to work through the GLContext interface. I am not sure if you meant just peeling off this class into NicosiaGC3DANLGELayer. So, I just pack GLContextANGLE into NicosiaGC3DANLGELayer::ANGLEContext for now. >> Source/WebCore/platform/graphics/texmap/GraphicsContext3DTextureMapper.cpp:266 >> +#if !USE(ANGLE) > > I don't think this guard is needed -- this is already non-USE(ANGLE) code. You're right. Removed. >> Source/WebKit/UIProcess/gtk/AcceleratedBackingStoreWayland.cpp:86 >> +#elif USE(OPENGL_ES) > > This change is fine on its own, but this check should be done without any usage of the Extensions3D* classes. This can be fixed after the fact. If it is ok, I want to address this in the following changes since I already made a fix there. https://github.com/shivamidow/webkit/commit/c9981306834c745ed391608be30357794162883d

Created attachment 383240 [details] Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Comment on attachment 383240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383240&action=review > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:203 > +#if USE(COORDINATED_GRAPHICS) USE(COORDINATED_GRAPHICS) is always true for GTK port, so we can avoid it here > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGC3DANGLELayer.cpp:88 > + EGLDisplay display = EGL_GetDisplay(EGL_DEFAULT_DISPLAY); Why the default display? Can't you use the platformDisplay.eglDisplay() instead? > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGC3DANGLELayer.cpp:149 > + EGLSurface surface = EGL_CreatePbufferSurface(display, config, pbufferAttributes); Isn't it possible to use a surfaceless context? or do we really need a surface here? > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGC3DANGLELayer.h:54 > + class ANGLEContext : public WebCore::GLContext { It's weird to have this here, why not adding an angle directory in Source/WebCore/platform/graphics and add the new context there following EGL and GLX contexts? Shouldn't this inherit from EGLContext instead? > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGC3DANGLELayer.h:63 > + enum EGLSurfaceType { PbufferSurface, WindowSurface, PixmapSurface, Surfaceless }; Only PbufferSurface is supported, no? > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGC3DANGLELayer.h:70 > + bool canRenderToDefaultFramebuffer() override { return m_type == WindowSurface; }; Is this really possible? > Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGC3DLayer.h:46 > + GC3DLayer(WebCore::GraphicsContext3D&); explicit > Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:172 > +void BitmapTextureGL::setPendingContents(RefPtr<Image> image) RefPtr<Image>&&

Created attachment 383330 [details] Patch

Comment on attachment 383240 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383240&action=review >> Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:203 >> +#if USE(COORDINATED_GRAPHICS) > > USE(COORDINATED_GRAPHICS) is always true for GTK port, so we can avoid it here Removed. >> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGC3DANGLELayer.cpp:88 >> + EGLDisplay display = EGL_GetDisplay(EGL_DEFAULT_DISPLAY); > > Why the default display? Can't you use the platformDisplay.eglDisplay() instead? Because platformDisplay.eglDisplay() is not compatible to ANGLE. platformDisplay.eglDisplay returns EGLDisplay from Mesa so a crash happens in EGL_Initialize if we use it below. It looks same but Mesa and ANGLE implement different data structures for EGL/GL primitive data types. Basically, what we want to do is to isolate ANGLE use from Mesa GL calls. That is why we keep ANGLE gl headers in a different path to Mesa headers, and use internal ANGLE egl/gl calls instead of prototypes. Otherwise, many conflicts occurs in both compile time and runtime. >> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGC3DANGLELayer.cpp:149 >> + EGLSurface surface = EGL_CreatePbufferSurface(display, config, pbufferAttributes); > > Isn't it possible to use a surfaceless context? or do we really need a surface here? Creating pbuffer here is a little bit experimental. Yeah, I just confirmed surfaceless works here. >> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGC3DANGLELayer.h:54 >> + class ANGLEContext : public WebCore::GLContext { > > It's weird to have this here, why not adding an angle directory in Source/WebCore/platform/graphics and add the new context there following EGL and GLX contexts? Shouldn't this inherit from EGLContext instead? Main reason is to separate ANGLE calls from the rest of world calling gl calls (mesa and others). I thought GLContext::makeContextCurrent() should be called in ANGLEContext::makeContextCurrent but it seems not. We can remove the GLContext inheritance then. >> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGC3DANGLELayer.h:63 >> + enum EGLSurfaceType { PbufferSurface, WindowSurface, PixmapSurface, Surfaceless }; > > Only PbufferSurface is supported, no? Surfaceless only. We can simply remove EGLSurfaceType if ANGLEContext does not inherit GLContext. >> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGC3DANGLELayer.h:70 >> + bool canRenderToDefaultFramebuffer() override { return m_type == WindowSurface; }; > > Is this really possible? No. Removed. >> Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGC3DLayer.h:46 >> + GC3DLayer(WebCore::GraphicsContext3D&); > > explicit Fixed. >> Source/WebCore/platform/graphics/texmap/BitmapTextureGL.cpp:172 >> +void BitmapTextureGL::setPendingContents(RefPtr<Image> image) > > RefPtr<Image>&& Ditto.

Comment on attachment 383330 [details] Patch Looks OK. Thanks for addressing the feedback.

Anyone cq+, please?

The commit-queue encountered the following flaky tests while processing attachment 383330 [details]: imported/w3c/web-platform-tests/html/semantics/forms/form-submission-0/form-double-submit-3.html bug 203609 (author: cdumez@apple.com) The commit-queue is continuing to process your patch.

The commit-queue encountered the following flaky tests while processing attachment 383330 [details]: The commit-queue is continuing to process your patch.

Comment on attachment 383330 [details] Patch Clearing flags on attachment: 383330 Committed r252717: <https://trac.webkit.org/changeset/252717>

All reviewed patches have been landed. Closing bug.

<rdar://problem/57376341>