RESOLVED FIXED 197755
Add preliminary ANGLE backend to WebCore
https://bugs.webkit.org/show_bug.cgi?id=197755
Summary Add preliminary ANGLE backend to WebCore
Kenneth Russell
Reported 2019-05-09 15:13:25 PDT
Currently the WebGL implementation delegates to GraphicsContext3D and its OpenGL-based backends (GraphicsContext3DOpenGL.cpp, GraphicsContextOpenGLES.cpp, GraphicsContext3DOpenGLCommon.cpp). ANGLE was just updated to top-of-tree here: https://bugs.webkit.org/show_bug.cgi?id=197676 and now would be an excellent time to try running the WebGL implementation on top of it. Some previous work has been done in these issues: Progress towards using ANGLE to do WebGL rendering https://bugs.webkit.org/show_bug.cgi?id=165864 [WebGL] ANGLEWebKitBridge should support ESSL platforms https://bugs.webkit.org/show_bug.cgi?id=92295 One idea to avoid breaking anything would be to add a new build flag to govern whether ANGLE is used, and to make a new GraphicsContext3DANGLE.cpp, conditionally compiled, which delegates all of the calls to ANGLE. When using ANGLE to create the underlying context, the WebGL compatibility modes can and should be used in order to do exactly the validation which WebGL needs.
Attachments
Patch (283.61 KB, patch)
2019-05-09 17:45 PDT, Alex Christensen
no flags
Patch (117.37 KB, patch)
2019-05-13 14:10 PDT, Alex Christensen
no flags
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.34 MB, application/zip)
2019-05-13 15:15 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews100 for mac-highsierra (3.44 MB, application/zip)
2019-05-13 15:16 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews115 for mac-highsierra (1.51 MB, application/zip)
2019-05-13 15:42 PDT, EWS Watchlist
no flags
Patch (73.17 KB, patch)
2019-06-03 14:28 PDT, Kenneth Russell
no flags
1. Add preliminary ANGLE backend to WebCore (160.92 KB, patch)
2019-06-07 02:43 PDT, Kenneth Russell
no flags
1. Add preliminary ANGLE backend to WebCore (160.92 KB, patch)
2019-06-10 11:29 PDT, Kenneth Russell
no flags
1. Add preliminary ANGLE backend to WebCore (161.00 KB, patch)
2019-06-10 15:34 PDT, Kenneth Russell
no flags
1. Add preliminary ANGLE backend to WebCore (161.16 KB, patch)
2019-06-10 17:22 PDT, Kenneth Russell
no flags
Patch (161.22 KB, patch)
2019-06-13 18:43 PDT, Kenneth Russell
no flags
1. Add preliminary ANGLE backend to WebCore (161.13 KB, patch)
2019-06-14 11:21 PDT, Kenneth Russell
no flags
Patch (161.27 KB, patch)
2019-06-17 18:10 PDT, Kenneth Russell
no flags
Patch (161.06 KB, patch)
2019-06-18 10:43 PDT, Kenneth Russell
dino: review+
Radar WebKit Bug Importer
Comment 1 2019-05-09 15:57:37 PDT
Alex Christensen
Comment 2 2019-05-09 17:45:15 PDT
Alex Christensen
Comment 3 2019-05-09 17:47:29 PDT
This is a resurrection of a several-year-old prototype of this that at one point worked except drawing to the screen from what I could tell. It currently compiles, runs, and asserts when you do anything with webgl. This would be a good starting point for further prototyping.
Alex Christensen
Comment 4 2019-05-10 10:49:34 PDT
I'm upstreaming the build system part of this in https://bugs.webkit.org/show_bug.cgi?id=197787
Alex Christensen
Comment 5 2019-05-13 14:10:44 PDT
Alex Christensen
Comment 6 2019-05-13 14:25:40 PDT
Comment on attachment 369780 [details] Patch There we go. It builds on Mac. That's as far as I plan to take this right now. Happy prototyping.
EWS Watchlist
Comment 7 2019-05-13 15:15:07 PDT
Comment on attachment 369780 [details] Patch Attachment 369780 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12181249 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 8 2019-05-13 15:15:09 PDT
Created attachment 369786 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
EWS Watchlist
Comment 9 2019-05-13 15:16:18 PDT
Comment on attachment 369780 [details] Patch Attachment 369780 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12181307 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 10 2019-05-13 15:16:20 PDT
Created attachment 369787 [details] Archive of layout-test-results from ews100 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-highsierra Platform: Mac OS X 10.13.6
EWS Watchlist
Comment 11 2019-05-13 15:42:12 PDT
Comment on attachment 369780 [details] Patch Attachment 369780 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12181352 Number of test failures exceeded the failure limit.
EWS Watchlist
Comment 12 2019-05-13 15:42:14 PDT
Created attachment 369793 [details] Archive of layout-test-results from ews115 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Kenneth Russell
Comment 13 2019-05-14 10:05:45 PDT
Linking to most recent ANGLE roll, which was needed for this work.
Konstantin Tokarev
Comment 14 2019-05-14 12:50:03 PDT
Could you explain, what's the point of having dedicated ANGLE backend when we already have OpenGL ES backend which can work with ANGLE in the same way as with other OpenGL ES implementations, and will have to stay because embedded Linux systems use OpenGL ES?
Dean Jackson
Comment 15 2019-05-14 17:45:18 PDT
(In reply to Konstantin Tokarev from comment #14) > Could you explain, what's the point of having dedicated ANGLE backend when > we already have OpenGL ES backend which can work with ANGLE in the same way > as with other OpenGL ES implementations, and will have to stay because > embedded Linux systems use OpenGL ES? Because ANGLE will do the WebGL validation for us. As long as it isn't a significant performance hit to go from WebGL -> OpenGLES to WebGL -> ANGLE -> OpenGLES then we'll be able to remove code in WebKit. Also, ANGLE will allow us to implement WebGL 2 with much less effort.
Don Olmstead
Comment 16 2019-05-16 09:39:10 PDT
(In reply to Dean Jackson from comment #15) > (In reply to Konstantin Tokarev from comment #14) > > Could you explain, what's the point of having dedicated ANGLE backend when > > we already have OpenGL ES backend which can work with ANGLE in the same way > > as with other OpenGL ES implementations, and will have to stay because > > embedded Linux systems use OpenGL ES? > > Because ANGLE will do the WebGL validation for us. As long as it isn't a > significant performance hit to go from WebGL -> OpenGLES to WebGL -> ANGLE > -> OpenGLES then we'll be able to remove code in WebKit. > > Also, ANGLE will allow us to implement WebGL 2 with much less effort. It would be nice to collapse the GL stuff so its just targeting ES. We could really cut down the class hierarchy and general complexity there. For linux there's vulkan support so that would potentially be nice for GTK or WPE assuming the hardware support is there. With WinCairo using ANGLE already that's not a big concern for us there. With PlayStation we have an ES implementation and don't support WebGL outside of some legacy system apps so if this work moves forward I'd want a way to just use that rather than wrapping ES in ANGLE. Maybe this is a similar concern for WPE? In the long term though how might this play with WebGPU? We're very interested in adding support for that outside of Apple ports once we're fully in trunk and moving to using that for rendering.
Dean Jackson
Comment 17 2019-05-16 10:00:53 PDT
How to implement WebGPU on multiple platforms is still an unknown. We've made as much as possible platform independent, but there is still a lot of Metal stuff in the GPU* classes.
Alex Christensen
Comment 18 2019-05-16 10:39:49 PDT
(In reply to Don Olmstead from comment #16) > With WinCairo using ANGLE already that's not a big concern for us there. > With PlayStation we have an ES implementation and don't support WebGL > outside of some legacy system apps so if this work moves forward I'd want a > way to just use that rather than wrapping ES in ANGLE. Maybe this is a > similar concern for WPE? In the long term we hope to remove the security checks in WebCore because they will be redundant with security checks in ANGLE. WebGL implementations in WebKit will be just thin wrappers around ANGLE, which can wrap GLES. Either way, there will need to be something wrapping the OpenGL(ES) implementation doing security checks, we are just planning to move those checks to ANGLE and simplify WebCore. (In reply to Dean Jackson from comment #17) > How to implement WebGPU on multiple platforms is still an unknown. We've > made as much as possible platform independent, but there is still a lot of > Metal stuff in the GPU* classes. ...but WebGPU is designed to be implementable on other platforms so it should be possible.
Kenneth Russell
Comment 19 2019-06-03 14:28:07 PDT
Kenneth Russell
Comment 20 2019-06-07 02:43:40 PDT
Created attachment 371576 [details] 1. Add preliminary ANGLE backend to WebCore
Don Olmstead
Comment 21 2019-06-07 11:16:41 PDT
(In reply to Kenneth Russell from comment #20) > Created attachment 371576 [details] > 1. Add preliminary ANGLE backend to WebCore I would have thought we would just have a GraphicsContext3D backed by only OpenGL ES and have ANGLE used as the ES implementations for all ports.
Kenneth Russell
Comment 22 2019-06-10 10:30:59 PDT
(In reply to Don Olmstead from comment #21) > (In reply to Kenneth Russell from comment #20) > > Created attachment 371576 [details] > > 1. Add preliminary ANGLE backend to WebCore > > I would have thought we would just have a GraphicsContext3D backed by only > OpenGL ES and have ANGLE used as the ES implementations for all ports. Trying to call ANGLE's publicly exposed OpenGL ES API entry points will probably cause symbol collision problems with the system's OpenGL ES libraries on iOS. I'm not sure, but this is probably the reason the WebKit team started this prototype by calling into ANGLE's internal namespaced entry points. This seems like a reasonable solution, and in conversations with Dean, he and I both preferred to bring up a separate ANGLE backend using this approach and then switch the ports over one by one. Does this sound OK to you? Fixing the iOS build issue in the above patch now.
Kenneth Russell
Comment 23 2019-06-10 11:29:16 PDT
Created attachment 371764 [details] 1. Add preliminary ANGLE backend to WebCore
Kenneth Russell
Comment 24 2019-06-10 15:34:39 PDT
Created attachment 371784 [details] 1. Add preliminary ANGLE backend to WebCore
Kenneth Russell
Comment 25 2019-06-10 17:21:48 PDT
Sorry for the EWS churn. I've now followed the instructions to set up the iOS Simulator build and the next patch will fix the compilation errors on that platform.
Kenneth Russell
Comment 26 2019-06-10 17:22:41 PDT
Created attachment 371798 [details] 1. Add preliminary ANGLE backend to WebCore
EWS Watchlist
Comment 27 2019-06-10 20:50:32 PDT
Comment on attachment 371798 [details] 1. Add preliminary ANGLE backend to WebCore Attachment 371798 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/12439740 New failing tests: mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla apiTests
Dean Jackson
Comment 28 2019-06-12 03:10:50 PDT
Comment on attachment 371798 [details] 1. Add preliminary ANGLE backend to WebCore View in context: https://bugs.webkit.org/attachment.cgi?id=371798&action=review This is great. Thanks Ken. I'm happy to land this now and start incrementally getting things working. > Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:2615 > + shellScript = "# Type a script or drag a script file from your workspace to insert its path.\n/bin/sh $SRCROOT/adjust-angle-include-paths.sh\n"; Could you remove the comment line and just have the part that calls the shell script? > Source/ThirdParty/ANGLE/adjust-angle-include-paths.sh:38 > +' $i > $i.new > + mv $i.new $i You could do sed -i '' to edit the file in place and avoid the mv and .new steps. > Source/WebCore/SourcesCocoa.txt:225 > +platform/graphics/angle/Extensions3DANGLE.cpp @no-unify > +platform/graphics/angle/GraphicsContext3DANGLE.cpp @no-unify > +platform/graphics/angle/TemporaryANGLESetting.cpp @no-unify It's probably fine to unify these sources, but that's not important for this patch. I've tried to unify the few remaining WebGL files a few times but keep making mistakes and getting rolled out, so I can just add these to that job. Edit: Oh I see. You can't unify them in case they get merged with files that use the system OpenGL. So we'll address this later. > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:89 > + // Enable support in ANGLE (if not enabled already) Nit: End comment with . (same with other comments in this function) Actually, maybe we don't even need these comments? > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:94 > + compiler.setResources(ANGLEResources); Is getting/setting the resources expensive? If not, it might read easier if this function just does something like: ANGLEWebKitBridge& compiler = m_context->m_compiler; ShBuiltInResources ANGLEResources = compiler.getResources(); if (name == "GL_OES_standard_derivatives") ANGLEResources.OES_standard_derivatives = 1; else if .... compiler.setResources(ANGLEResources); > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:128 > + ANGLEWebKitBridge& compiler = m_context->m_compiler; > + return compiler.getResources().OES_standard_derivatives; I wouldn't bother with the local variable here. > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:154 > + return ""; // Invalid shader type. Use emptyString(); > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:159 > + return ""; Use emptyString(); > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:182 > + return ""; emptyString(); > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.h:69 > + // EXT Robustness - uses getGraphicsResetStatusARB() > + void readnPixelsEXT(int x, int y, GC3Dsizei width, GC3Dsizei height, GC3Denum format, GC3Denum type, GC3Dsizei bufSize, void *data) override; > + void getnUniformfvEXT(GC3Duint program, int location, GC3Dsizei bufSize, float *params) override; > + void getnUniformivEXT(GC3Duint program, int location, GC3Dsizei bufSize, int *params) override; Should we just leave these out for now since they aren't implemented? > Source/WebCore/platform/graphics/angle/Extensions3DANGLE.h:88 > + // Weak pointer back to GraphicsContext3D Nit: End with . > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:33 > +#include "GraphicsContext3DIOS.h" I really need to get rid of this horrible hack. Adopting ANGLE will definitely help! > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:277 > + // clamp the maximum to 8192, which is big enough for a 5K display. Oops. We just announced a 6K display :(
Dean Jackson
Comment 29 2019-06-12 03:11:54 PDT
The test failure is unrelated. Ken, if you upload a new patch I'll land it for you. Thanks again!
Kenneth Russell
Comment 30 2019-06-13 18:42:38 PDT
Comment on attachment 371798 [details] 1. Add preliminary ANGLE backend to WebCore View in context: https://bugs.webkit.org/attachment.cgi?id=371798&action=review >> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:2615 >> + shellScript = "# Type a script or drag a script file from your workspace to insert its path.\n/bin/sh $SRCROOT/adjust-angle-include-paths.sh\n"; > > Could you remove the comment line and just have the part that calls the shell script? Yes. Done. >> Source/ThirdParty/ANGLE/adjust-angle-include-paths.sh:38 >> + mv $i.new $i > > You could do sed -i '' to edit the file in place and avoid the mv and .new steps. Thanks, done. >> Source/WebCore/SourcesCocoa.txt:225 >> +platform/graphics/angle/TemporaryANGLESetting.cpp @no-unify > > It's probably fine to unify these sources, but that's not important for this patch. I've tried to unify the few remaining WebGL files a few times but keep making mistakes and getting rolled out, so I can just add these to that job. > > Edit: Oh I see. You can't unify them in case they get merged with files that use the system OpenGL. So we'll address this later. Right. I've added a comment indicating the reason they can't be unified. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:89 >> + // Enable support in ANGLE (if not enabled already) > > Nit: End comment with . (same with other comments in this function) > Actually, maybe we don't even need these comments? Updated these comments - this file is essentially a vanilla copy of Extensions3DOpenGLCommon and Extensions3DOpenGL combined, so I'd rather minimize the number of changes in the initial commit. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:94 >> + compiler.setResources(ANGLEResources); > > Is getting/setting the resources expensive? If not, it might read easier if this function just does something like: > > ANGLEWebKitBridge& compiler = m_context->m_compiler; > ShBuiltInResources ANGLEResources = compiler.getResources(); > > if (name == "GL_OES_standard_derivatives") > ANGLEResources.OES_standard_derivatives = 1; > else if .... > > compiler.setResources(ANGLEResources); See above comment; this is essentially a copy of existing (known working) sources so I'd rather not change them too much during the initial commit. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:128 >> + return compiler.getResources().OES_standard_derivatives; > > I wouldn't bother with the local variable here. See above comment about copy of sources. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:154 >> + return ""; // Invalid shader type. > > Use emptyString(); Seems simple enough change. Done. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:159 >> + return ""; > > Use emptyString(); Done. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:182 >> + return ""; > > emptyString(); Done. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.h:69 >> + void getnUniformivEXT(GC3Duint program, int location, GC3Dsizei bufSize, int *params) override; > > Should we just leave these out for now since they aren't implemented? I don't think that's an option; they're pure virtual in the base class. >> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.h:88 >> + // Weak pointer back to GraphicsContext3D > > Nit: End with . Done. >> Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:33 >> +#include "GraphicsContext3DIOS.h" > > I really need to get rid of this horrible hack. Adopting ANGLE will definitely help! Yes, we can eliminate this easily in forthcoming patches. This is essentially a copy of GraphicsContext3DOpenGLCommon combined with GraphicsContext3DOpenGL. >> Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:277 >> + // clamp the maximum to 8192, which is big enough for a 5K display. > > Oops. We just announced a 6K display :( This code should handle that too, but we can fix it in the future if not.
Kenneth Russell
Comment 31 2019-06-13 18:43:36 PDT
Kenneth Russell
Comment 32 2019-06-13 18:45:00 PDT
(In reply to Dean Jackson from comment #29) > The test failure is unrelated. Ken, if you upload a new patch I'll land it > for you. Thanks again! New patch has been uploaded. (Still rebuilding locally, so apologies if it fails the EWS. Will fix if so.) Thank you for reviewing this and accepting these changes!
Kenneth Russell
Comment 33 2019-06-13 20:35:11 PDT
Is there any way to re-trigger an EWS bot ("win", in this case)? It looks to me like the build failure is unrelated to this patch, and probably already resolved on the bot.
Michael Catanzaro
Comment 34 2019-06-14 06:43:59 PDT
(In reply to Kenneth Russell from comment #33) > Is there any way to re-trigger an EWS bot ("win", in this case)? Reupload the same patch :(
Don Olmstead
Comment 35 2019-06-14 10:28:20 PDT
Comment on attachment 372093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372093&action=review > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:830 > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) || PLATFORM(WPE) Dean can we find a better guard for these methods? I know this is just copy paste on Ken's part but these guards are fundamentally wrong. If we fix it I can just enable WebGL 2 on WinCairo. Its been low priority so I've never gotten around to it.
Kenneth Russell
Comment 36 2019-06-14 11:16:22 PDT
(In reply to Don Olmstead from comment #35) > Comment on attachment 372093 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372093&action=review > > > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:830 > > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) || PLATFORM(WPE) > > Dean can we find a better guard for these methods? I know this is just copy > paste on Ken's part but these guards are fundamentally wrong. If we fix it I > can just enable WebGL 2 on WinCairo. Its been low priority so I've never > gotten around to it. I can remove those guards in this file. Please note that WebKit's current WebGL 2 implementation is far from conformant and I would strongly recommend not enabling it on further platforms as doing so will only increase developer confusion. This ANGLE backend will be conformant, and should work for all WebKit ports.
Kenneth Russell
Comment 37 2019-06-14 11:18:03 PDT
(In reply to Michael Catanzaro from comment #34) > (In reply to Kenneth Russell from comment #33) > > Is there any way to re-trigger an EWS bot ("win", in this case)? > > Reupload the same patch :( Thanks for the tip. :(
Kenneth Russell
Comment 38 2019-06-14 11:21:25 PDT
Created attachment 372129 [details] 1. Add preliminary ANGLE backend to WebCore
Kenneth Russell
Comment 39 2019-06-14 11:23:10 PDT
(In reply to Kenneth Russell from comment #36) > (In reply to Don Olmstead from comment #35) > > Comment on attachment 372093 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=372093&action=review > > > > > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:830 > > > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) || PLATFORM(WPE) > > > > Dean can we find a better guard for these methods? I know this is just copy > > paste on Ken's part but these guards are fundamentally wrong. If we fix it I > > can just enable WebGL 2 on WinCairo. Its been low priority so I've never > > gotten around to it. > > I can remove those guards in this file. Please note that WebKit's current > WebGL 2 implementation is far from conformant and I would strongly recommend > not enabling it on further platforms as doing so will only increase > developer confusion. This ANGLE backend will be conformant, and should work > for all WebKit ports. Removed in the current patch. Taking the opportunity to do another EWS run which will hopefully be green.
Don Olmstead
Comment 40 2019-06-14 11:30:01 PDT
(In reply to Kenneth Russell from comment #39) > (In reply to Kenneth Russell from comment #36) > > (In reply to Don Olmstead from comment #35) > > > Comment on attachment 372093 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=372093&action=review > > > > > > > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:830 > > > > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) || PLATFORM(WPE) > > > > > > Dean can we find a better guard for these methods? I know this is just copy > > > paste on Ken's part but these guards are fundamentally wrong. If we fix it I > > > can just enable WebGL 2 on WinCairo. Its been low priority so I've never > > > gotten around to it. > > > > I can remove those guards in this file. Please note that WebKit's current > > WebGL 2 implementation is far from conformant and I would strongly recommend > > not enabling it on further platforms as doing so will only increase > > developer confusion. This ANGLE backend will be conformant, and should work > > for all WebKit ports. > > Removed in the current patch. Taking the opportunity to do another EWS run > which will hopefully be green. It was more of a comment for Dean since those are actually in the header. I would think it should take into account whether the OpenGL, or OpenGL ES, has those functions, or whether its the ANGLE backend. The WebGL2 support would just be enabled as experimental so if there are bugs thats fine.
Kenneth Russell
Comment 41 2019-06-14 14:55:55 PDT
(In reply to Don Olmstead from comment #40) > (In reply to Kenneth Russell from comment #39) > > (In reply to Kenneth Russell from comment #36) > > > (In reply to Don Olmstead from comment #35) > > > > Comment on attachment 372093 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=372093&action=review > > > > > > > > > Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:830 > > > > > +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) || PLATFORM(WPE) > > > > > > > > Dean can we find a better guard for these methods? I know this is just copy > > > > paste on Ken's part but these guards are fundamentally wrong. If we fix it I > > > > can just enable WebGL 2 on WinCairo. Its been low priority so I've never > > > > gotten around to it. > > > > > > I can remove those guards in this file. Please note that WebKit's current > > > WebGL 2 implementation is far from conformant and I would strongly recommend > > > not enabling it on further platforms as doing so will only increase > > > developer confusion. This ANGLE backend will be conformant, and should work > > > for all WebKit ports. > > > > Removed in the current patch. Taking the opportunity to do another EWS run > > which will hopefully be green. > > It was more of a comment for Dean since those are actually in the header. I > would think it should take into account whether the OpenGL, or OpenGL ES, > has those functions, or whether its the ANGLE backend. > > The WebGL2 support would just be enabled as experimental so if there are > bugs thats fine. As you pointed out, I copied that #if from GraphicsContext3DOpenGLCommon.cpp and it isn't necessary in the ANGLE backend. GraphicsContext3D.h doesn't seem to conditionalize the declaration of mapBufferRange, unmapBuffer and these other entry points, so with USE_ANGLE=1 the WinCairo port will get these definitions. Let's work together to make the ANGLE backend happen for WinCairo; I'll have to focus exclusively on the desktop macOS port.
Dean Jackson
Comment 42 2019-06-17 11:21:51 PDT
Comment on attachment 372093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372093&action=review >>>>>> Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:830 >>>>>> +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY) || PLATFORM(WPE) >>>>> >>>>> Dean can we find a better guard for these methods? I know this is just copy paste on Ken's part but these guards are fundamentally wrong. If we fix it I can just enable WebGL 2 on WinCairo. Its been low priority so I've never gotten around to it. >>>> >>>> I can remove those guards in this file. Please note that WebKit's current WebGL 2 implementation is far from conformant and I would strongly recommend not enabling it on further platforms as doing so will only increase developer confusion. This ANGLE backend will be conformant, and should work for all WebKit ports. >>> >>> Removed in the current patch. Taking the opportunity to do another EWS run which will hopefully be green. >> >> It was more of a comment for Dean since those are actually in the header. I would think it should take into account whether the OpenGL, or OpenGL ES, has those functions, or whether its the ANGLE backend. >> >> The WebGL2 support would just be enabled as experimental so if there are bugs thats fine. > > As you pointed out, I copied that #if from GraphicsContext3DOpenGLCommon.cpp and it isn't necessary in the ANGLE backend. GraphicsContext3D.h doesn't seem to conditionalize the declaration of mapBufferRange, unmapBuffer and these other entry points, so with USE_ANGLE=1 the WinCairo port will get these definitions. Let's work together to make the ANGLE backend happen for WinCairo; I'll have to focus exclusively on the desktop macOS port. Yeah, I can't remember why they are here. It should be a USE definition in Platform.h. Let's fix this after the patch lands (both here and GC3DOpenGLCommon)
Dean Jackson
Comment 43 2019-06-17 11:25:34 PDT
Kenneth Russell
Comment 44 2019-06-17 11:27:57 PDT
May I reopen this? I'd been hoping to land a couple of patches under this bug.
Don Olmstead
Comment 45 2019-06-17 12:16:12 PDT
(In reply to Kenneth Russell from comment #44) > May I reopen this? I'd been hoping to land a couple of patches under this > bug. This probably should've been a meta bug then. Think its preferable to open separate bugs for each change.
Ryan Haddad
Comment 46 2019-06-17 13:23:52 PDT
Reverted r246501 for reason: Breaks Apple internal builds. Committed r246512: <https://trac.webkit.org/changeset/246512>
Ryan Haddad
Comment 47 2019-06-17 13:24:38 PDT
(In reply to Ryan Haddad from comment #46) > Reverted r246501 for reason: > > Breaks Apple internal builds. > > Committed r246512: <https://trac.webkit.org/changeset/246512> I shared the failed build log with Dean, hoping that he will take a look soon.
Keith Rollin
Comment 48 2019-06-17 15:29:42 PDT
Instead of using: output_dir=$BUILT_PRODUCTS_DIR/usr/local/include/ANGLE Maybe make use of the following variable: PUBLIC_HEADERS_FOLDER_PATH = $(ANGLE_INSTALL_PATH_PREFIX)/usr/local/include/ANGLE; I haven't tried that change, but since that's where the headers are actually copied to, it seems to make sense.
Kenneth Russell
Comment 49 2019-06-17 18:03:00 PDT
(In reply to Don Olmstead from comment #45) > (In reply to Kenneth Russell from comment #44) > > May I reopen this? I'd been hoping to land a couple of patches under this > > bug. > > This probably should've been a meta bug then. Think its preferable to open > separate bugs for each change. Thanks for the suggestion. Filed a new meta-bug, made it depend on this one, and changed the summary.
Kenneth Russell
Comment 50 2019-06-17 18:06:16 PDT
(In reply to Keith Rollin from comment #48) > Instead of using: > > output_dir=$BUILT_PRODUCTS_DIR/usr/local/include/ANGLE > > Maybe make use of the following variable: > > PUBLIC_HEADERS_FOLDER_PATH = > $(ANGLE_INSTALL_PATH_PREFIX)/usr/local/include/ANGLE; > > I haven't tried that change, but since that's where the headers are actually > copied to, it seems to make sense. Thanks for the suggestion. It looks like ANGLE_INSTALL_PATH_PREFIX isn't set for external WebKit builds, so PUBLIC_HEADERS_FOLDER_PATH isn't set up in that way - is it for Apple-internal builds? Uploading a new patch now which attempts to make both cases work. Appreciate your or anyone else's help testing and/or modifying it.
Kenneth Russell
Comment 51 2019-06-17 18:10:29 PDT
Kenneth Russell
Comment 52 2019-06-17 18:12:53 PDT
Current patch changes how Source/ThirdParty/ANGLE/adjust-angle-include-paths.sh attempts to find the output directory. Also adjusted the ChangeLog messages to reflect the new bug description. Those are the only changes compared to the earlier version which was rolled out. Could someone from Apple please try this in an Apple-internal build setting and see whether it works? I'd greatly appreciate any help. Thanks.
Keith Rollin
Comment 53 2019-06-17 20:47:59 PDT
(In reply to Kenneth Russell from comment #50) > (In reply to Keith Rollin from comment #48) > > Instead of using: > > > > output_dir=$BUILT_PRODUCTS_DIR/usr/local/include/ANGLE > > > > Maybe make use of the following variable: > > > > PUBLIC_HEADERS_FOLDER_PATH = > > $(ANGLE_INSTALL_PATH_PREFIX)/usr/local/include/ANGLE; > > > > I haven't tried that change, but since that's where the headers are actually > > copied to, it seems to make sense. > > Thanks for the suggestion. It looks like ANGLE_INSTALL_PATH_PREFIX isn't set > for external WebKit builds, so PUBLIC_HEADERS_FOLDER_PATH isn't set up in > that way - is it for Apple-internal builds? > > Uploading a new patch now which attempts to make both cases work. Appreciate > your or anyone else's help testing and/or modifying it. I should have thought this through a little bit more in my first posting. I think that what we want here is: output_dir=$BUILT_PRODUCTS_DIR/$PUBLIC_HEADERS_FOLDER_PATH I'll try this out and post back with the results. Go ahead and test for both of those before using them if you like, but otherwise I don't think there's any need to differentiate between "internal" and "external" builds. ANGLE_INSTALL_PATH_PREFIX may be undefined or set to an empty string in some configurations, but that's expected. In those cases, output_dir degenerates to what you originally had.
Keith Rollin
Comment 54 2019-06-17 23:37:47 PDT
I ended up with the following. I didn't see a build variable that held the desired path in both contexts. if [ "$DEPLOYMENT_LOCATION" == "YES" ] ; then output_dir=$DSTROOT/$PUBLIC_HEADERS_FOLDER_PATH else output_dir=$BUILT_PRODUCTS_DIR/$PUBLIC_HEADERS_FOLDER_PATH fi
Kenneth Russell
Comment 55 2019-06-18 10:43:23 PDT
Kenneth Russell
Comment 56 2019-06-18 10:46:21 PDT
Thanks Keith, and Dean also for figuring out a similar solution he sent via email. Have incorporated that fix. I also accidentally introduced a bug with my earlier changes to that script, now fixed in the current reland patch.
Dean Jackson
Comment 57 2019-06-18 11:53:36 PDT
I'm testing on normal and production/deployment builds, and then I'll land.
Dean Jackson
Comment 58 2019-06-18 12:00:19 PDT
It built fine!
Dean Jackson
Comment 59 2019-06-18 12:02:06 PDT
Kenneth Russell
Comment 60 2019-06-18 13:01:54 PDT
(In reply to Dean Jackson from comment #58) > It built fine! Thank you Dean for your help shepherding this fix through!
Zan Dobersek
Comment 61 2019-06-18 23:35:07 PDT
USE_ANGLE can be left undefined on some configurations, leading to a bunch of compiler warnings when the incompatibility check in Platform.h is done. Uploaded a patch that defines the macro value to 0 in case it's not been previously defined in bug #198991, please take a look.
Note You need to log in before you can comment on or make changes to this bug.