WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(117.37 KB, patch)
2019-05-13 14:10 PDT
,
Alex Christensen
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
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
Details
Patch
(73.17 KB, patch)
2019-06-03 14:28 PDT
,
Kenneth Russell
no flags
Details
Formatted Diff
Diff
1. Add preliminary ANGLE backend to WebCore
(160.92 KB, patch)
2019-06-07 02:43 PDT
,
Kenneth Russell
no flags
Details
Formatted Diff
Diff
1. Add preliminary ANGLE backend to WebCore
(160.92 KB, patch)
2019-06-10 11:29 PDT
,
Kenneth Russell
no flags
Details
Formatted Diff
Diff
1. Add preliminary ANGLE backend to WebCore
(161.00 KB, patch)
2019-06-10 15:34 PDT
,
Kenneth Russell
no flags
Details
Formatted Diff
Diff
1. Add preliminary ANGLE backend to WebCore
(161.16 KB, patch)
2019-06-10 17:22 PDT
,
Kenneth Russell
no flags
Details
Formatted Diff
Diff
Patch
(161.22 KB, patch)
2019-06-13 18:43 PDT
,
Kenneth Russell
no flags
Details
Formatted Diff
Diff
1. Add preliminary ANGLE backend to WebCore
(161.13 KB, patch)
2019-06-14 11:21 PDT
,
Kenneth Russell
no flags
Details
Formatted Diff
Diff
Patch
(161.27 KB, patch)
2019-06-17 18:10 PDT
,
Kenneth Russell
no flags
Details
Formatted Diff
Diff
Patch
(161.06 KB, patch)
2019-06-18 10:43 PDT
,
Kenneth Russell
dino
: review+
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2019-05-09 15:57:37 PDT
<
rdar://problem/50642205
>
Alex Christensen
Comment 2
2019-05-09 17:45:15 PDT
Created
attachment 369534
[details]
Patch
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
Created
attachment 369780
[details]
Patch
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
Created
attachment 371212
[details]
Patch
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
Created
attachment 372093
[details]
Patch
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
Committed
r246501
: <
https://trac.webkit.org/changeset/246501
>
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
Created
attachment 372309
[details]
Patch
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
Created
attachment 372349
[details]
Patch
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
Committed
r246554
: <
https://trac.webkit.org/changeset/246554
>
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.
Top of Page
Format For Printing
XML
Clone This Bug