Bug 227596 – REGRESSION (r279466): [Big Sur] webgl/1.0.3/conformance & webgl/2.0.0/conformance are failing

• Component: ANGLE
• Status: RESOLVED FIXED

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

[Radar WebKit Bug Importer]

<rdar://problem/80045012>

[Kenneth Russell]

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?

[Kyle Piddington]

Created attachment 432749 [details]
Patch

[Kenneth Russell]

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 //.

[Alexey Proskuryakov]

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?

[Kenneth Russell]

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?

[Kyle Piddington]

Created attachment 432834 [details]
Patch

[EWS Watchlist]

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

[Kyle Piddington]

Created attachment 432836 [details]
Patch

[John Cunningham]

Comment on attachment 432836 [details]
Patch

LGTM

[Kenneth Russell]

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.

[Kyle Piddington]

Created attachment 432956 [details]
Patch for landing

[EWS]

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].