Bug 219421

Created attachment 415192 [details] WIP patch

Comment on attachment 415192 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=415192&action=review some ideas, but I'm not a reviewer > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:250 > +#elif PLATFORM(WIN) To clarify the code, you could consider to do following: - make the above elif an #else - change IOSurfaceTextureTarget to "drawingBufferTextureTarget()" so your code would be #else GLenum textureTarget = drawingBufferTexture(); gl::BindTexture(textureTarget, m_texture); gl::TexImage2D(textureTarget, 0, m_internalColorFormat, width, height, 0, colorFormat, GL_UNSIGNED_BYTE, 0); #If USE(COORDINATED_GRAPHICS) if (m_compositorTexture) { ... } #endif #endif > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1077 > +#endif GL_TEXTURE_RECTANGLE_ANGLE might still be supported on Windows, so this could be turned into a runtime check if you like. Granted, it'd only apply if ANGLE would run on top of GL for Windows. if (m_supportsTextureRectangle) gl::Disable(GL_TEXTURE_RECTANGLE_ANGLE); check for texture rectangle might be GL_ARB_texture_rectangle or EGL_ANGLE_iosurface_client_buffer (not sure if how ANGLE advertises the ARB_texture_rectangle). > Source/WebCore/platform/graphics/texmap/ANGLEContext.h:42 > +class ANGLEContext { There's quite a proliferation of "Context" classes already. If only possible, I'd suggest to make this similar to the GLContext, e.g a subclass or instance or something. > Source/WebCore/platform/graphics/texmap/TextureMapperGCGLPlatformLayer.cpp:107 > ::glBindFramebuffer(GraphicsContextGLOpenGL::FRAMEBUFFER, m_context.m_state.boundDrawFBO); This depicts some sort of mixup. This function is probably calling directly OpenGL ES or OpenGL, where as the context above is angle context. I'd suggest making the includes such that compiler catches this.. > Source/WebCore/platform/graphics/texmap/TextureMapperGCGLPlatformLayer.cpp:109 > + GLContext* ativeContext = GLContext::current(); If the GLContext is really useful and does something, such as makes real OpenGL or OpenGL ES context current, then it will confuse the angle context in cases where ANGLE is using OpenGL. In this light, it'd be maybe simpler if the ANGLE context would participate in the GLContext::current() tracking. One way to do this might be that case where ANGLEContext : public GLContext or something similar. There's a typo, ativeContext

Comment on attachment 415192 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=415192&action=review Thank you very much for the review. I will check and add my comments later. >> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:250 >> +#elif PLATFORM(WIN) > > To clarify the code, you could consider to do following: > - make the above elif an #else > - change IOSurfaceTextureTarget to "drawingBufferTextureTarget()" > > so your code would be > > #else > GLenum textureTarget = drawingBufferTexture(); > gl::BindTexture(textureTarget, m_texture); > gl::TexImage2D(textureTarget, 0, m_internalColorFormat, width, height, 0, colorFormat, GL_UNSIGNED_BYTE, 0); > #If USE(COORDINATED_GRAPHICS) > if (m_compositorTexture) { > ... > } > #endif > #endif Sounds nice. Filed: Bug 219475 – GraphicsContextGLOpenGL: Rename IOSurfaceTextureTarget to drawingBufferTextureTarget

Comment on attachment 415192 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=415192&action=review >> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:1077 >> +#endif > > GL_TEXTURE_RECTANGLE_ANGLE might still be supported on Windows, so this could be turned into a runtime check if you like. Granted, it'd only apply if ANGLE would run on top of GL for Windows. > > if (m_supportsTextureRectangle) > gl::Disable(GL_TEXTURE_RECTANGLE_ANGLE); > > check for texture rectangle might be > GL_ARB_texture_rectangle or EGL_ANGLE_iosurface_client_buffer (not sure if how ANGLE advertises the ARB_texture_rectangle). WebKit enables only ANGLE D3D backend on Windows at the moment. https://trac.webkit.org/browser/webkit/trunk/Source/ThirdParty/ANGLE/PlatformWin.cmake I tried to enable ANGLE OpenGL backend, and it turned out it's not simple task, and I gave up. >> Source/WebCore/platform/graphics/texmap/TextureMapperGCGLPlatformLayer.cpp:107 >> ::glBindFramebuffer(GraphicsContextGLOpenGL::FRAMEBUFFER, m_context.m_state.boundDrawFBO); > > This depicts some sort of mixup. This function is probably calling directly OpenGL ES or OpenGL, where as the context above is angle context. > I'd suggest making the includes such that compiler catches this.. Good catch. However, because WinCairo is using both ANGLE's EGL API and ANGLE's direct API (gl::*), I think I can't do so. >> Source/WebCore/platform/graphics/texmap/TextureMapperGCGLPlatformLayer.cpp:109 >> + GLContext* ativeContext = GLContext::current(); > > If the GLContext is really useful and does something, such as makes real OpenGL or OpenGL ES context current, then it will confuse the angle context in cases where ANGLE is using OpenGL. > In this light, it'd be maybe simpler if the ANGLE context would participate in the GLContext::current() tracking. One way to do this might be that case where ANGLEContext : public GLContext or something similar. > > There's a typo, ativeContext It should be possible. But, I don't like the idea tracking the current GL context in WebCore duplicately. I prefer the idea deprecating GLContext::current(). ANGLEContext is a copy of Nicosia::GCGLANGLELayer::ANGLEContext. https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/nicosia/texmap/NicosiaGCGLANGLELayer.cpp#L54 I don't want to copy the code. Unfortunately, GTK port's USE_ANGLE build is broken now. I will merge both ANGLEContext after someone restore GTK port's USE_ANGLE build.

Created attachment 415696 [details] Patch

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

Created attachment 415698 [details] Patch

OMG, I found severe regressions by browsing some WebGL pages. Nothing is shown in the following page with this patch. https://threejs.org/examples/ https://domenicobrz.github.io/webgl/projects/SSRefractionDepthPeeling/

(In reply to Fujii Hironori from comment #8) > OMG, I found severe regressions by browsing some WebGL pages. Nothing is > shown in the following page with this patch. > > https://threejs.org/examples/ It reports no message in inspector console. > https://domenicobrz.github.io/webgl/projects/SSRefractionDepthPeeling/ It reports two warning messages: [Warning] THREE.WebGLRenderer: WEBGL_depth_texture extension not supported. (three.module.js, line 16023) [Warning] THREE.WebGLRenderer: Texture has been resized from (1500x750) to (1024x512). (three.module.js, line 21359)

(In reply to Fujii Hironori from comment #8) * Should pass a sharing context to EGL_CreateContext to share a texture between compositor's EGL context and WebGL' one * GPUBasedCanvasRenderingContext::isAccelerated should return true

Created attachment 415833 [details] Patch

Created attachment 416037 [details] Patch

Could anyone review?

(In reply to Fujii Hironori from comment #13) > Could anyone review? Nice. Looks fine to me, but I'm not a reviewer.

Comment on attachment 416037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416037&action=review > Source/WebCore/PlatformWin.cmake:41 > + platform/graphics/angle/ExtensionsGLANGLE.cpp You add angle-related code here, but not the angle sources themselves. This seems to indicate that there's a place where ANGLE sources are added based on a ANGLE related flag. Some time maybe you could refactor the build system to add the angle support code at the same location as where the angle code itself is being added to the build. This way when somebody, for example I, adds a ANGLE related support file, I don't need to edit PlatformWin.cmake as well as PlatformGTK or so. So I'd edit something where the ANGLE sources are being declared. > Source/WebCore/PlatformWin.cmake:47 > platform/graphics/opengl/TemporaryOpenGLSetting.cpp This TemporaryOpenGLSetting seems to indicate either you have a missing #ifdef excluding its usage, or then you might be able to remove this one too. > Source/WebCore/platform/graphics/texmap/TextureMapperGCGLPlatformLayer.cpp:-108 > - ::glBindFramebuffer(GraphicsContextGLOpenGL::FRAMEBUFFER, m_context.m_state.boundDrawFBO); This seems to indicate that following never fires #If USE(TEXTURE_MAPPER) #If !USE(COORDINATED_GRAPHICS) #If !USE(ANGLE) #error this never fires #endif #endif #endif If this is the case, maybe it'd be possible to put this assertion in the CMake system, so it'd not come as surprise for somebody doing related work.

Comment on attachment 416037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416037&action=review Thank you for the review. >> Source/WebCore/PlatformWin.cmake:41 >> + platform/graphics/angle/ExtensionsGLANGLE.cpp > > You add angle-related code here, but not the angle sources themselves. > This seems to indicate that there's a place where ANGLE sources are added based on a ANGLE related flag. > Some time maybe you could refactor the build system to add the angle support code at the same location as where the angle code itself is being added to the build. > This way when somebody, for example I, adds a ANGLE related support file, I don't need to edit PlatformWin.cmake as well as PlatformGTK or so. So I'd edit something where the ANGLE sources are being declared. ANGLE sources are built in CMakeLists under Source/ThirdParty/ANGLE directory. They are in a different module of a different layer. I can't add those files into it. I can merge WinCairo and GTK's CMake code into the comon CMakeLists.txt. However, because WinCairo WebKit1 is using TemporaryOpenGLSetting.cpp, TemporaryOpenGLSetting.cpp will remain in PlatformGTK.cmake. if (NOT USE_ANGLE_WEBGL) list(APPEND WebCore_SOURCES platform/graphics/opengl/TemporaryOpenGLSetting.cpp ) endif () This is a unfortunate situation for GTK port because their OpenGL related sources are in both the common CMakeLists.txt and PlatformGTK.cmake. If all ports migrate to USE_ANGLE or WinCairo deprecates WebKit1, the code can be simplified. >> Source/WebCore/PlatformWin.cmake:47 >> platform/graphics/opengl/TemporaryOpenGLSetting.cpp > > This TemporaryOpenGLSetting seems to indicate either you have a missing #ifdef excluding its usage, or then you might be able to remove this one too. I can't remove it until WinCairo deprecate WebKit1. WinCairo is using ANGLE EGL and OpenGL ES public API for TextureMapper. And, WinCairo WebKit1 is using TemporaryOpenGLSetting. https://trac.webkit.org/browser/webkit/trunk/Source/WebKitLegacy/win/WebCoreSupport/AcceleratedCompositingContext.cpp?rev=270071#L179 However, WinCairo WebKit2's corresponding code doesn't use TemporaryOpenGLSetting. https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/LayerTreeHostTextureMapper.cpp#L65 I don't know why the code is needed for WinCairo WebKit1. >> Source/WebCore/platform/graphics/texmap/TextureMapperGCGLPlatformLayer.cpp:-108 >> - ::glBindFramebuffer(GraphicsContextGLOpenGL::FRAMEBUFFER, m_context.m_state.boundDrawFBO); > > This seems to indicate that following never fires > #If USE(TEXTURE_MAPPER) > #If !USE(COORDINATED_GRAPHICS) > #If !USE(ANGLE) > #error this never fires > #endif > #endif > #endif > > If this is the case, maybe it'd be possible to put this assertion in the CMake system, so it'd not come as surprise for somebody doing related work. I don't think this can cause a surprise. USE_ANGLE is a switch to use the new WebGL implementation. After all port will migrate to the new one, we can remove the old code and USE_ANGLE macro. All non-cocoa ports are using TextureMapper as the compositor if they enable Accelerated Compositing. And, only WinCairo port isn't using Coordinated Graphics. USE(TEXTURE_MAPPER) && !USE(COORDINATED_GRAPHICS) is used only by WinCairo port. After this patch will land, WinCairo won't swich back to !USE(ANGLE). WinCairo won't support !USE(ANGLE) build configuration.

Comment on attachment 416037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416037&action=review >>> Source/WebCore/PlatformWin.cmake:41 >>> + platform/graphics/angle/ExtensionsGLANGLE.cpp >> >> You add angle-related code here, but not the angle sources themselves. >> This seems to indicate that there's a place where ANGLE sources are added based on a ANGLE related flag. >> Some time maybe you could refactor the build system to add the angle support code at the same location as where the angle code itself is being added to the build. >> This way when somebody, for example I, adds a ANGLE related support file, I don't need to edit PlatformWin.cmake as well as PlatformGTK or so. So I'd edit something where the ANGLE sources are being declared. > > ANGLE sources are built in CMakeLists under Source/ThirdParty/ANGLE directory. > They are in a different module of a different layer. > I can't add those files into it. > > I can merge WinCairo and GTK's CMake code into the comon CMakeLists.txt. > However, because WinCairo WebKit1 is using TemporaryOpenGLSetting.cpp, TemporaryOpenGLSetting.cpp will remain in PlatformGTK.cmake. > > if (NOT USE_ANGLE_WEBGL) > list(APPEND WebCore_SOURCES > platform/graphics/opengl/TemporaryOpenGLSetting.cpp > ) > endif () > > This is a unfortunate situation for GTK port because their OpenGL related sources are in both the common CMakeLists.txt and PlatformGTK.cmake. > If all ports migrate to USE_ANGLE or WinCairo deprecates WebKit1, the code can be simplified. I was wrong. PlatformGTK.cmake doesn’t need to have the code snippet. I will fix it in a follow up patch.

Comment on attachment 416037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416037&action=review I'm fine with the changes just wondering what to do about the TextureMapperGCGLPlatformLayer files. >>> Source/WebCore/platform/graphics/texmap/TextureMapperGCGLPlatformLayer.cpp:-108 >>> - ::glBindFramebuffer(GraphicsContextGLOpenGL::FRAMEBUFFER, m_context.m_state.boundDrawFBO); >> >> This seems to indicate that following never fires >> #If USE(TEXTURE_MAPPER) >> #If !USE(COORDINATED_GRAPHICS) >> #If !USE(ANGLE) >> #error this never fires >> #endif >> #endif >> #endif >> >> If this is the case, maybe it'd be possible to put this assertion in the CMake system, so it'd not come as surprise for somebody doing related work. > > I don't think this can cause a surprise. > USE_ANGLE is a switch to use the new WebGL implementation. > After all port will migrate to the new one, we can remove the old code and USE_ANGLE macro. > > All non-cocoa ports are using TextureMapper as the compositor if they enable Accelerated Compositing. > And, only WinCairo port isn't using Coordinated Graphics. > USE(TEXTURE_MAPPER) && !USE(COORDINATED_GRAPHICS) is used only by WinCairo port. > After this patch will land, WinCairo won't swich back to !USE(ANGLE). > WinCairo won't support !USE(ANGLE) build configuration. If WinCairo is the only one who can potentially build this file would it be more appropriate to rename or rewrite it, to get rid of LGPL parts?

Comment on attachment 416037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416037&action=review >>>> Source/WebCore/platform/graphics/texmap/TextureMapperGCGLPlatformLayer.cpp:-108 >>>> - ::glBindFramebuffer(GraphicsContextGLOpenGL::FRAMEBUFFER, m_context.m_state.boundDrawFBO); >>> >>> This seems to indicate that following never fires >>> #If USE(TEXTURE_MAPPER) >>> #If !USE(COORDINATED_GRAPHICS) >>> #If !USE(ANGLE) >>> #error this never fires >>> #endif >>> #endif >>> #endif >>> >>> If this is the case, maybe it'd be possible to put this assertion in the CMake system, so it'd not come as surprise for somebody doing related work. >> >> I don't think this can cause a surprise. >> USE_ANGLE is a switch to use the new WebGL implementation. >> After all port will migrate to the new one, we can remove the old code and USE_ANGLE macro. >> >> All non-cocoa ports are using TextureMapper as the compositor if they enable Accelerated Compositing. >> And, only WinCairo port isn't using Coordinated Graphics. >> USE(TEXTURE_MAPPER) && !USE(COORDINATED_GRAPHICS) is used only by WinCairo port. >> After this patch will land, WinCairo won't swich back to !USE(ANGLE). >> WinCairo won't support !USE(ANGLE) build configuration. > > If WinCairo is the only one who can potentially build this file would it be more appropriate to rename or rewrite it, to get rid of LGPL parts? Umm, I don't understand your opinion. I don't know what is confusing. TextureMapperGCGLPlatformLayer sounds a natural name for me. It means TextureMapper's PlatformLayer for WebGL context. Can you suggest any other better name?

It seems that GTK port no longer uses !USE(NICOSIA) code. I will remove unsued code in a follow-up patch.

(In reply to Fujii Hironori from comment #19) > Comment on attachment 416037 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416037&action=review > > >>>> Source/WebCore/platform/graphics/texmap/TextureMapperGCGLPlatformLayer.cpp:-108 > >>>> - ::glBindFramebuffer(GraphicsContextGLOpenGL::FRAMEBUFFER, m_context.m_state.boundDrawFBO); > >>> > >>> This seems to indicate that following never fires > >>> #If USE(TEXTURE_MAPPER) > >>> #If !USE(COORDINATED_GRAPHICS) > >>> #If !USE(ANGLE) > >>> #error this never fires > >>> #endif > >>> #endif > >>> #endif > >>> > >>> If this is the case, maybe it'd be possible to put this assertion in the CMake system, so it'd not come as surprise for somebody doing related work. > >> > >> I don't think this can cause a surprise. > >> USE_ANGLE is a switch to use the new WebGL implementation. > >> After all port will migrate to the new one, we can remove the old code and USE_ANGLE macro. > >> > >> All non-cocoa ports are using TextureMapper as the compositor if they enable Accelerated Compositing. > >> And, only WinCairo port isn't using Coordinated Graphics. > >> USE(TEXTURE_MAPPER) && !USE(COORDINATED_GRAPHICS) is used only by WinCairo port. > >> After this patch will land, WinCairo won't swich back to !USE(ANGLE). > >> WinCairo won't support !USE(ANGLE) build configuration. > > > > If WinCairo is the only one who can potentially build this file would it be more appropriate to rename or rewrite it, to get rid of LGPL parts? > > Umm, I don't understand your opinion. I don't know what is confusing. > TextureMapperGCGLPlatformLayer sounds a natural name for me. It means > TextureMapper's PlatformLayer for WebGL context. > Can you suggest any other better name? The confusing part is that this is now a file that requires USE_ANGLE to be enabled. Its not going to be used by GTK/WPE since that is always USE_NICOSIA. It sounds like its WinCairo/ANGLE specific so maybe it should go into `Source/WebCore/platform/graphics/texmap/wincairo` or `Source/WebCore/platform/graphics/angle`?

Comment on attachment 416037 [details] Patch Think there should be some renaming or moving around of the file but its not essential

Comment on attachment 416037 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416037&action=review Thank you for the review. >>>>>> Source/WebCore/platform/graphics/texmap/TextureMapperGCGLPlatformLayer.cpp:-108 >>>>>> - ::glBindFramebuffer(GraphicsContextGLOpenGL::FRAMEBUFFER, m_context.m_state.boundDrawFBO); >>>>> >>>>> This seems to indicate that following never fires >>>>> #If USE(TEXTURE_MAPPER) >>>>> #If !USE(COORDINATED_GRAPHICS) >>>>> #If !USE(ANGLE) >>>>> #error this never fires >>>>> #endif >>>>> #endif >>>>> #endif >>>>> >>>>> If this is the case, maybe it'd be possible to put this assertion in the CMake system, so it'd not come as surprise for somebody doing related work. >>>> >>>> I don't think this can cause a surprise. >>>> USE_ANGLE is a switch to use the new WebGL implementation. >>>> After all port will migrate to the new one, we can remove the old code and USE_ANGLE macro. >>>> >>>> All non-cocoa ports are using TextureMapper as the compositor if they enable Accelerated Compositing. >>>> And, only WinCairo port isn't using Coordinated Graphics. >>>> USE(TEXTURE_MAPPER) && !USE(COORDINATED_GRAPHICS) is used only by WinCairo port. >>>> After this patch will land, WinCairo won't swich back to !USE(ANGLE). >>>> WinCairo won't support !USE(ANGLE) build configuration. >>> >>> If WinCairo is the only one who can potentially build this file would it be more appropriate to rename or rewrite it, to get rid of LGPL parts? >> >> Umm, I don't understand your opinion. I don't know what is confusing. >> TextureMapperGCGLPlatformLayer sounds a natural name for me. It means TextureMapper's PlatformLayer for WebGL context. >> Can you suggest any other better name? > > The confusing part is that this is now a file that requires USE_ANGLE to be enabled. Its not going to be used by GTK/WPE since that is always USE_NICOSIA. It sounds like its WinCairo/ANGLE specific so maybe it should go into `Source/WebCore/platform/graphics/texmap/wincairo` or `Source/WebCore/platform/graphics/angle`? Yes, GraphicsLayerTextureMapper is used only by WinCairo, but TextureMapperGCGLPlatformLayer is a part of TextureMapper. It'd be sad if TextureMapper related source files are separated into multiple directories.

Committed r270899: <https://trac.webkit.org/changeset/270899>

<rdar://problem/72394320>