Bug 218896

<rdar://problem/71624036>

Created attachment 415038 [details] Adding initial wip patch

Created attachment 415057 [details] Updated patch

Created attachment 415091 [details] Updated patch to address build errors. This is still WIP.

Created attachment 415160 [details] Patch

Created attachment 415165 [details] Patch

Created attachment 415167 [details] Patch

Comment on attachment 415167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415167&action=review > Source/WebCore/platform/graphics/angle/ExtensionsGLANGLE.cpp:215 > +#if 0 Why do this instead of deleting? Same question everywhere we are using #if 0.

Comment on attachment 415167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415167&action=review >> Source/WebCore/platform/graphics/angle/ExtensionsGLANGLE.cpp:215 >> +#if 0 > > Why do this instead of deleting? Same question everywhere we are using #if 0. My bad. Rini, the #if 0 hunks were examples of function ranges that probably are removable. So if the functions inside the #if 0 blocks are removable, you could just remove them.

(In reply to Kimmo Kinnunen from comment #9) > Comment on attachment 415167 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=415167&action=review > > >> Source/WebCore/platform/graphics/angle/ExtensionsGLANGLE.cpp:215 > >> +#if 0 > > > > Why do this instead of deleting? Same question everywhere we are using #if 0. > > My bad. > Rini, the #if 0 hunks were examples of function ranges that probably are > removable. So if the functions inside the #if 0 blocks are removable, you > could just remove them. Okay, will remove those.

Created attachment 415232 [details] Patch

Comment on attachment 415232 [details] Patch The failures in the iOS-wk2 EWS bot seem to be WebGL-related.

Comment on attachment 415232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415232&action=review > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:743 > You can test the test failures by leaving the context->getExtensions() call here. If the tests pass on the currently failing platform, then there is a bug somewhere, perhaps something to do with makecurrent. > Source/WebCore/platform/graphics/ExtensionsGL.h:304 > }; So this hunk is ok to remove, i.e it should be dead code. Also applies tho the cpp file. However, see below, the corresponding change is not ok for the OpenGL.h/OpenGLES.h > Source/WebCore/platform/graphics/opengl/ExtensionsGLOpenGLCommon.h:-62 > - bool isNVIDIA() override { return m_isNVIDIA; } So these functions cannot be removed from here. So for non-ANGLE implementation, this hunk below is not dead code. So it cannot be removed, and the corresponding code in the .cpp cannot be removed either. > Source/WebCore/platform/graphics/opengl/ExtensionsGLOpenGLCommon.h:-126 > - These are not dead code, cannot be removed. > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLBase.cpp:-69 > - if (!attrs.alpha && getExtensions().isNVIDIA()) { cannot be removed. So you must change getExtensions() with static_cast<ExtensionsGLOpenGLCommon&>(getExtensions()).isNVIDIA() or similar construct. > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLBase.cpp:291 > #endif same thing for all of these below that use getExtensions() --- these are not dead code.

Created attachment 415338 [details] Patch

Created attachment 415353 [details] Patch

Comment on attachment 415353 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415353&action=review > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:746 > auto& extensions = context->getExtensions(); > - if (extensions.supports("GL_EXT_debug_marker"_s)) > - extensions.pushGroupMarkerEXT("WebGLRenderingContext"_s); > + if (extensions.supports("GL_EXT_debug_marker"_s)) { } Why can’t we delete this whole paragraph of code, instead of just the pushGroupMarkerEXT call? Is there a side effect of calling getExtensions or supports? > Source/WebCore/platform/graphics/angle/ExtensionsGLANGLE.h:39 > + ExtensionsGLANGLE(GraphicsContextGLOpenGL*); Should add the "explicit" keyword to this constructor. > Source/WebCore/platform/graphics/opengl/ExtensionsGLOpenGLCommon.h:69 > + bool isNVIDIA() { return m_isNVIDIA; } > + bool isAMD() { return m_isAMD; } > + bool isIntel() { return m_isIntel; } > + bool isImagination() { return m_isImagination; } > + String vendor() { return m_vendor; } > + > + bool requiresBuiltInFunctionEmulation() { return m_requiresBuiltInFunctionEmulation; } > + bool requiresRestrictedMaximumTextureSize() { return m_requiresRestrictedMaximumTextureSize; } These should probably all be const. > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLBase.cpp:69 > + if (!attrs.alpha && static_cast<ExtensionsGLOpenGLCommon&>(getExtensions()).isNVIDIA()) { This cast seems inelegant. Why is it safe? Do we really need to make this change? > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLBase.cpp:292 > + if (static_cast<ExtensionsGLOpenGLCommon&>(getExtensions()).requiresRestrictedMaximumTextureSize()) Ditto. > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLBase.cpp:297 > + if (static_cast<ExtensionsGLOpenGLCommon&>(getExtensions()).requiresRestrictedMaximumTextureSize()) Ditto.

(In reply to Darin Adler from comment #16) > Comment on attachment 415353 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=415353&action=review > > > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:746 > > auto& extensions = context->getExtensions(); > > - if (extensions.supports("GL_EXT_debug_marker"_s)) > > - extensions.pushGroupMarkerEXT("WebGLRenderingContext"_s); > > + if (extensions.supports("GL_EXT_debug_marker"_s)) { } > > Why can’t we delete this whole paragraph of code, instead of just the > pushGroupMarkerEXT call? Is there a side effect of calling getExtensions or > supports? I left it there to test if the ios-wk2 test failure had something to do with it. > > > Source/WebCore/platform/graphics/angle/ExtensionsGLANGLE.h:39 > > + ExtensionsGLANGLE(GraphicsContextGLOpenGL*); > > Should add the "explicit" keyword to this constructor. > > > Source/WebCore/platform/graphics/opengl/ExtensionsGLOpenGLCommon.h:69 > > + bool isNVIDIA() { return m_isNVIDIA; } > > + bool isAMD() { return m_isAMD; } > > + bool isIntel() { return m_isIntel; } > > + bool isImagination() { return m_isImagination; } > > + String vendor() { return m_vendor; } > > + > > + bool requiresBuiltInFunctionEmulation() { return m_requiresBuiltInFunctionEmulation; } > > + bool requiresRestrictedMaximumTextureSize() { return m_requiresRestrictedMaximumTextureSize; } > > These should probably all be const. Will change it. > > > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLBase.cpp:69 > > + if (!attrs.alpha && static_cast<ExtensionsGLOpenGLCommon&>(getExtensions()).isNVIDIA()) { > > This cast seems inelegant. Why is it safe? Do we really need to make this > change? > > > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLBase.cpp:292 > > + if (static_cast<ExtensionsGLOpenGLCommon&>(getExtensions()).requiresRestrictedMaximumTextureSize()) > > Ditto. > > > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLBase.cpp:297 > > + if (static_cast<ExtensionsGLOpenGLCommon&>(getExtensions()).requiresRestrictedMaximumTextureSize()) > > Ditto. We no longer have these queries in ExtensionsGL, and the default method getExtensions() returns the type ExtensionsGL, which is a parent to ExtensionsGLOpenGLCommon. So I think static_cast should be safe in this case?

(In reply to Darin Adler from comment #16) > Why can’t we delete this whole paragraph of code, instead of just the > pushGroupMarkerEXT call? Is there a side effect of calling getExtensions or > supports? Yeah, we should delete the whole paragraph. The intention was for Rini to test if the source of the test failure was removing this hunk, which it was. The call has a side-effect of making the underlying OpenGL context current. After removing the hunk, the context is not current when some buggy function is being called. Historically the implementations (ANGLE, OpenGL, OpenGL ES) have been very buggy wrt this. I tried to fix this recently with a patch that makes the call for every public entry point. I must have missed some, or there is some other kind of logic problem related to this. I'll sync up with Rini on how to spot this. > > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGLBase.cpp:69 > > + if (!attrs.alpha && static_cast<ExtensionsGLOpenGLCommon&>(getExtensions()).isNVIDIA()) { > > This cast seems inelegant. Why is it safe? Do we really need to make this > change? So the "extensions" object is conceptually the same object as the "context" object. This is safe because this is modifying "OpenGLCommon" context, which only uses "OpenGLCommon" extensions implementation. For a given compile, there's only these implementations of the interfaces. The underlying problem is that the Extensions interface was not supposed to be used like this inside the Context interface implementation. Rather, I would imagine the original intention was that the Extensions interface was used from the outside, e.g. WebGL implementation level. We are trying to simplify by removing the extensions interface altogether. However, since the OpenGL and OpenGL ES implementations of these are relatively unmaintained, unimplemented and buggy, it's not entirely simple to just remove the interface. This is a transitional commit towards a scenario where the getExtensions call would be removed from the common interface. After that, the old OpenGL / OpenGL ES implementations can add their exact type to the getExtensions call, or follow through with the removal of the Extensions/Context separation altogether. We need to make this change to eventually remove the Extensions interface from the main code. The interface is just full of empty functions and it does not abstract anything related to extending the context object. As it stands today, the interfaces are very much tied together. Also we need to make the change because the abstract Extensions interface contains concrete properties that are not common to all the Extensions implementors. The properties "isNVIDIA()" etc are used in the OpenGL/OpenGLES _implementation_ of the GraphicsContextGLOpenGL If you so feel, we can extend the ifdefing in the header to change the getExtensions to covariant return value, which should side-step the need for casting. I felt it would complicate the matter more than simplify, especially given that the code would stay as-is hopefully for somewhat short amount of time anyway.

(In reply to Kimmo Kinnunen from comment #18) > If you so feel, we can extend the ifdefing in the header to change the > getExtensions to covariant return value, which should side-step the need for > casting. I felt it would complicate the matter more than simplify, > especially given that the code would stay as-is hopefully for somewhat short > amount of time anyway. I think we should. Adding one function does not sound like it would complicate things much.

Created attachment 415439 [details] Patch

Created attachment 415464 [details] Patch

Created attachment 415466 [details] Patch

Created attachment 415473 [details] Patch

Created attachment 415480 [details] Patch

Sorry for the noise. I'm not able to figure out this build error in gtk/wpe/wincario platforms. __invalid covariant return type for ‘virtual WebCore::ExtensionsGLOpenGLCommon& WebCore::GraphicsContextGLOpenGL::getExtensions()’__ I'm using the covariant return type for getExtensions() in !USE(ANGLE) case. Am I supposed to ifdef something else for these platforms?

Comment on attachment 415480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415480&action=review > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:487 > + // Use covariant return type for OPENGL/OPENGL_ES I think this doesn't follow the covariant return type pattern. The return type should be the derived class (GraphicsContextGLOpenGL&) for the covariant return type in this case.

(In reply to Fujii Hironori from comment #26) > Comment on attachment 415480 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=415480&action=review > > > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:487 > > + // Use covariant return type for OPENGL/OPENGL_ES > > I think this doesn't follow the covariant return type pattern. > The return type should be the derived class (GraphicsContextGLOpenGL&) for > the covariant return type in this case. According to this https://en.wikipedia.org/wiki/Covariant_return_type page, since ExtensionsGLOpenGLCommon is derived from ExtensionsGL, this type of covariant return type should've been possible.

(In reply to Rini Patel from comment #27) > According to this https://en.wikipedia.org/wiki/Covariant_return_type page, > since ExtensionsGLOpenGLCommon is derived from ExtensionsGL, this type of > covariant return type should've been possible. It's a Java example. There is a link to C++ in the page. https://www.lwithers.me.uk/articles/covariant.html

In the example, Derived::clone() returns Derived*. NetServerTCP::acceptConnection() returns NetServerTCP*. NetServerSCTP::acceptConnection() returns NetClientSCTP*. However, in your patch, GraphicsContextGLOpenGL::getExtensions() returns ExtensionsGLOpenGLCommon&. GraphicsContextGLOpenGL::getExtensions() should return GraphicsContextGLOpenGL& for the covariant return type.

(In reply to Fujii Hironori from comment #29) > In the example, > Derived::clone() returns Derived*. > NetServerTCP::acceptConnection() returns NetServerTCP*. > NetServerSCTP::acceptConnection() returns NetClientSCTP*. > > However, in your patch, > GraphicsContextGLOpenGL::getExtensions() returns ExtensionsGLOpenGLCommon&. > GraphicsContextGLOpenGL::getExtensions() should return > GraphicsContextGLOpenGL& for the covariant return type. NetServerTCP::acceptConnection() returns NetClientTCP*, where NetClientTCP is child of NetClient. According to example code snippet: /* Inheritance hierarchies NetServer NetClient | | ^ ^ / \ / \ NetServerTCP NetServerSCTP NetClientTCP NetClientSCTP */ In my case I have following hierarchy, seems like I'm following it correctly? /* Inheritance hierarchies GraphicsContextGL ExtensionsGL | | GraphicsContextGLOpenGL ExtensionsGLOpenGLCommon */ class GraphicsContextGL { public: virtual ExtensionsGL& getExtensions() = 0; }; class GraphicsContextGLOpenGL : public GraphicsContextGL { public: ExtensionsGLOpenGLCommon& getExtensions(); }; Is it possible that the failure could be related improper ifdef on gtk/wpe platforms?

I'm sorry. You are right. https://godbolt.org/z/df8cWv The problem seems that you don't include ExtensionsGLOpenGL.h in GraphicsContextGLOpenGL.h.

Comment on attachment 415480 [details] Patch Need a new version that includes the header for ExtensionsGLOpenGLCommon so it compiles on GTK, WPE, and Windows. Do you understand the failures on the iOS-WK2 build bot?

(In reply to Darin Adler from comment #32) > Comment on attachment 415480 [details] > Patch > > Need a new version that includes the header for ExtensionsGLOpenGLCommon so > it compiles on GTK, WPE, and Windows. > I see, the forward decl isn't working for covariant return type. > Do you understand the failures on the iOS-WK2 build bot? Not yet. Kimmo had submitted some patches that might help with that. Will try to update the trunk.

Created attachment 415602 [details] Patch

Created attachment 415603 [details] Patch

Created attachment 415826 [details] Patch

Created attachment 415827 [details] Patch

Created attachment 415829 [details] Patch

Created attachment 415831 [details] Patch

Created attachment 415834 [details] Patch

Created attachment 415955 [details] Patch

Created attachment 416296 [details] Patch

Comment on attachment 416296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416296&action=review > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:164 > internalDepthStencilFormat = GL_DEPTH_COMPONENT16; So the problem is the above getExtensions call? The size change logic is a bit hacky at the moment, so maybe it'd be better not to move it from the start of the reshape code. E.g. obvious places to change are: a) start of the function (change always succeeds) b) ends f the function (change takes effect when the function succeeds) Now moving it to middle is surprising, since the reason is just a side-effect of using a function in the implementation that happens to need the previous size. (The makeContextCurrent using the size seems to be source of bugs, that should be removed and the problem solved in another manner, but it's a bit bigger change). Instead, you can change the above like so: if (attrs.stencil) { // We don't allow the logic where stencil is required and depth is not. // See GraphicsContextGLOpenGL::validateAttributes. internalDepthStencilFormat = GL_DEPTH24_STENCIL8_OES; } else if (attrs.depth) internalDepthStencilFormat = GL_DEPTH_COMPONENT16; The attribute validation already checks for the extension and modifies the attributes accordingly.

(In reply to Kimmo Kinnunen from comment #43) > Comment on attachment 416296 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416296&action=review > > > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:164 > > internalDepthStencilFormat = GL_DEPTH_COMPONENT16; > > So the problem is the above getExtensions call? > The size change logic is a bit hacky at the moment, so maybe it'd be better > not to move it from the start of the reshape code. > E.g. obvious places to change are: > a) start of the function (change always succeeds) > b) ends f the function (change takes effect when the function succeeds) > > Now moving it to middle is surprising, since the reason is just a > side-effect of using a function in the implementation that happens to need > the previous size. > (The makeContextCurrent using the size seems to be source of bugs, that > should be removed and the problem solved in another manner, but it's a bit > bigger change). > > > Instead, you can change the above like so: > > if (attrs.stencil) { > // We don't allow the logic where stencil is required and depth is > not. > // See GraphicsContextGLOpenGL::validateAttributes. > internalDepthStencilFormat = GL_DEPTH24_STENCIL8_OES; > } else if (attrs.depth) > internalDepthStencilFormat = GL_DEPTH_COMPONENT16; > > Okay. > The attribute validation already checks for the extension and modifies the > attributes accordingly. Seemed like side effect was coming from call to supports(). If extensions are initialized for the first time in reshapeFBOs, the makeContextCurrent call expects the framebuffer size to be non-zero. But by that time, we've already set the size parameters.

Created attachment 417020 [details] Patch

Created attachment 418327 [details] Patch

Comment on attachment 418327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418327&action=review Looking again, not really wanting to fully review until EWS passes. > Source/WebCore/platform/graphics/angle/ExtensionsGLANGLE.h:39 > + explicit ExtensionsGLANGLE(GraphicsContextGLOpenGL*); Not needed in this patch, but a note for future refinement: Why is this remaining argument a pointer rather than a reference? > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:2094 > + if (!makeContextCurrent()) > + return; Change log does not mention or explain this change. > Source/WebCore/platform/graphics/opengl/ExtensionsGLOpenGLCommon.h:35 > namespace WebCore { > +class GraphicsContextGLOpenGL; Normally we’d leave a blank line here. I see some other OpenGL-related WebKit headers not doing that, but it’s our normal format to do that. > Source/WebCore/platform/graphics/opengl/ExtensionsGLOpenGLES.h:30 > +#include "GraphicsContextGLOpenGL.h" Why does this header have an include rather than a forward declaration? I think this could be a forward declaration instead.

Comment on attachment 418327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418327&action=review >> Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:2094 >> + return; > > Change log does not mention or explain this change. I don’t understand how this change fits in.

Thanks Darin for the review. (In reply to Darin Adler from comment #47) > Comment on attachment 418327 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=418327&action=review > > Looking again, not really wanting to fully review until EWS passes. > > > Source/WebCore/platform/graphics/angle/ExtensionsGLANGLE.h:39 > > + explicit ExtensionsGLANGLE(GraphicsContextGLOpenGL*); > > Not needed in this patch, but a note for future refinement: Why is this > remaining argument a pointer rather than a reference? > > > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:2094 > > + if (!makeContextCurrent()) > > + return; > > Change log does not mention or explain this change. Ah yes, this was added for the debugging purpose for failing iOS-WK2 tests, as initial suspect was missing makeContextCurrent() call at some of the public entry points. Will remove it. > > > Source/WebCore/platform/graphics/opengl/ExtensionsGLOpenGLCommon.h:35 > > namespace WebCore { > > +class GraphicsContextGLOpenGL; > > Normally we’d leave a blank line here. I see some other OpenGL-related > WebKit headers not doing that, but it’s our normal format to do that. > > > Source/WebCore/platform/graphics/opengl/ExtensionsGLOpenGLES.h:30 > > +#include "GraphicsContextGLOpenGL.h" > > Why does this header have an include rather than a forward declaration? I > think this could be a forward declaration instead. I'll address the suggested refinements in next patch. Thanks.

Created attachment 418458 [details] Patch

Comment on attachment 418458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418458&action=review > Source/WebCore/platform/graphics/angle/ExtensionsGLANGLE.cpp:36 > +ExtensionsGLANGLE::ExtensionsGLANGLE(GraphicsContextGLOpenGL* context) +1 to Darin's comment that this should be a ref parameter/member, but not important now. I think Kimmo wants to get rid of ExtensionsGL anyway. > Source/WebCore/platform/graphics/opengl/ExtensionsGLOpenGLCommon.h:67 > + bool isNVIDIA() const { return m_isNVIDIA; } > + bool isAMD() const { return m_isAMD; } > + bool isIntel() const { return m_isIntel; } > + bool isImagination() const { return m_isImagination; } > + String vendor() const { return m_vendor; } It's not relevant for this patch, but I'm surprised that other ports are still using these. > Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:495 > +#if !USE(ANGLE) Is there a case where someone would include this header while using ANGLE?

Comment on attachment 418458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418458&action=review >> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:495 >> +#if !USE(ANGLE) > > Is there a case where someone would include this header while using ANGLE? Yeah, it is included in GraphicsContextGLANGLE.cpp and ExtensionsGLANGLE.

rini_patel@apple.com does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json. Rejecting attachment 418458 [details] from commit queue.

Committed r272217: <https://trac.webkit.org/changeset/272217> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418458 [details].

New warnings after this, https://bugs.webkit.org/show_bug.cgi?id=221409