Bug 227596 - REGRESSION (r279466): [Big Sur] webgl/1.0.3/conformance & webgl/2.0.0/conformance are failing
Summary: REGRESSION (r279466): [Big Sur] webgl/1.0.3/conformance & webgl/2.0.0/confor...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: ANGLE (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac (Intel) Unspecified
: P2 Normal
Assignee: Kyle Piddington
URL:
Keywords: InRadar
Depends on: 227226
Blocks:
  Show dependency treegraph
 
Reported: 2021-07-01 14:03 PDT by ayumi_kojima
Modified: 2021-07-06 12:33 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.05 KB, patch)
2021-07-01 16:57 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (16.96 KB, patch)
2021-07-02 17:34 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch (17.48 KB, patch)
2021-07-02 17:39 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff
Patch for landing (17.43 KB, patch)
2021-07-06 11:52 PDT, Kyle Piddington
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ayumi_kojima 2021-07-01 14:03:48 PDT
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
Comment 1 Radar WebKit Bug Importer 2021-07-01 14:06:33 PDT
<rdar://problem/80045012>
Comment 2 Kenneth Russell 2021-07-01 15:49:06 PDT
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?
Comment 3 Kyle Piddington 2021-07-01 16:57:07 PDT
Created attachment 432749 [details]
Patch
Comment 4 Kenneth Russell 2021-07-01 17:35:28 PDT
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 5 Alexey Proskuryakov 2021-07-01 18:43:20 PDT
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 6 Kenneth Russell 2021-07-01 18:47:41 PDT
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?
Comment 7 Kyle Piddington 2021-07-02 17:34:34 PDT
Created attachment 432834 [details]
Patch
Comment 8 EWS Watchlist 2021-07-02 17:35:32 PDT
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 9 Kyle Piddington 2021-07-02 17:39:19 PDT
Created attachment 432836 [details]
Patch
Comment 10 John Cunningham 2021-07-02 17:44:11 PDT
Comment on attachment 432836 [details]
Patch

LGTM
Comment 11 Kenneth Russell 2021-07-04 18:31:59 PDT
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.
Comment 12 Kyle Piddington 2021-07-06 11:52:02 PDT
Created attachment 432956 [details]
Patch for landing
Comment 13 EWS 2021-07-06 12:33:17 PDT
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].