webgl/1.0.3/conformance/glsl/functions/glsl-function-atan-xy.html webgl/1.0.3/conformance/ogles/GL/atan/atan_009_to_012.html webgl/2.0.0/conformance/glsl/functions/glsl-function-atan-xy.html webgl/2.0.0/conformance/ogles/GL/atan/atan_009_to_012.html History: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&suite=layout-tests&suite=layout-tests&test=webgl%2F1.0.3%2Fconformance%2Fglsl%2Ffunctions%2Fglsl-function-atan-xy.html&test=webgl%2F1.0.3%2Fconformance%2Fogles%2FGL%2Fatan%2Fatan_009_to_012.html&test=webgl%2F2.0.0%2Fconformance%2Fglsl%2Ffunctions%2Fglsl-function-atan-xy.html&test=webgl%2F2.0.0%2Fconformance%2Fogles%2FGL%2Fatan%2Fatan_009_to_012.html It looks like the tests have started failing at r279466 https://build.webkit.org/results/Apple-BigSur-Release-WK1-Tests/r279475%20(3797)/results.html --- /Volumes/Data/worker/bigsur-release-tests-wk1/build/layout-test-results/webgl/1.0.3/conformance/glsl/functions/glsl-function-atan-xy-expected.txt +++ /Volumes/Data/worker/bigsur-release-tests-wk1/build/layout-test-results/webgl/1.0.3/conformance/glsl/functions/glsl-function-atan-xy-actual.txt @@ -1,5 +1,30 @@ This test runs the WebGL Test listed below in an iframe and reports PASS or FAIL. Test: ../../../resources/webgl_test_files/conformance/glsl/functions/glsl-function-atan-xy.html -[ PASS ] All tests passed +[ 1: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 2: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 3: FAIL ] images are different: at (0,0): ref=(187,0,0,255) test=(255,0,0,255) tolerance=5 +[ 4: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 5: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 6: FAIL ] images are different: at (0,0): ref=(187,187,0,255) test=(255,255,0,255) tolerance=5 +[ 7: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 8: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 9: FAIL ] images are different: at (0,0): ref=(187,192,187,255) test=(255,255,255,255) tolerance=5 +[ 10: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 11: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 12: FAIL ] images are different: at (0,0): ref=(137,192,187,191) test=(137,255,255,255) tolerance=5 +[ 13: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 14: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 15: PASS ] images are the same +[ 16: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 17: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 18: PASS ] images are the same +[ 19: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 20: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 21: FAIL ] images are different: at (0,0): ref=(187,192,186,255) test=(186,255,185,255) tolerance=5 +[ 22: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 23: PASS ] getError was expected value: NO_ERROR : no errors from draw +[ 24: FAIL ] images are different: at (0,0): ref=(137,192,186,191) test=(137,255,185,190) tolerance=5 +[ 25: PASS ] successfullyParsed is true +[ FAIL ] 6 failures reported
<rdar://problem/80045012>
Let's mark these as expected failures in TestExpectations and follow up. If just the arctangent function has exceptionally poor precision when fastMathEnabled is used, we could plausibly increase the tolerance on these specific WebGL conformance tests. I think that we shouldn't revert the changes from Bug 227226 . Without fastMathEnabled, it seems likely that a significant amount of WebGL content will be slower with ANGLE's Metal backend than the OpenGL backend. Would it be feasible or appropriate for Apple engineers to also file a bug against Metal to understand why precision of this one trigonometric function is so poor?
Created attachment 432749 [details] Patch
Comment on attachment 432749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432749&action=review This seems like a good solution; didn't realize these options could be changed per-shader. r+ once this builds and passes tests. > Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_utils.mm:603 > + //Fastmath on downlevel Intel causes some issues (Webkit bug 227596) Space after //.
Comment on attachment 432749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432749&action=review > Source/ThirdParty/ANGLE/ChangeLog:7 > + Proposed workaround: Disable fastmath on shaders when atan or invariants are used > + on Intel systems This patch seems to only possibly enable fastMath, never disable it?
Comment on attachment 432749 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432749&action=review >> Source/ThirdParty/ANGLE/ChangeLog:7 >> + on Intel systems > > This patch seems to only possibly enable fastMath, never disable it? It's disabled either if the shader uses atan, or uses the invariant qualifier - am I misreading the code below?
Created attachment 432834 [details] Patch
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Created attachment 432836 [details] Patch
Comment on attachment 432836 [details] Patch LGTM
Comment on attachment 432836 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=432836&action=review Looks good. r+ also based on johncunningham@'s review. One small comment. Note to save time that you don't need to request review on subsequent patches - you can just change the ChangeLogs to say "Reviewed by Kenneth Russell." and cq+. > Source/ThirdParty/ANGLE/include/platform/FeaturesMtl.h:133 > + Feature intelDisableFastMathWorkaround = { This feature doesn't need to end in "Workaround", and this name is confusing to read - it sounds like it's disabling a workaround related to fast math. intelDisableFastMath would be clearer in my opinion. > Source/ThirdParty/ANGLE/include/platform/FeaturesMtl.h:134 > + "intel_disable_fast_math_workarounds", FeatureCategory::MetalWorkarounds, Please name the same as the feature - workaround instead of workarounds in this case.
Created attachment 432956 [details] Patch for landing
Committed r279606 (239429@main): <https://commits.webkit.org/239429@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 432956 [details].