Bug 220895

Summary: Failures with Metal ANGLE backend
Product: WebKit Reporter: Dean Jackson <dino>
Component: WebGLAssignee: Dean Jackson <dino>
Severity: Normal CC: cdumez, commit-queue, dino, ews-watchlist, graouts, kbr, kondapallykalyan, kpiddington, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 222218    
Bug Blocks: 220076    
Description Flags
Patch kbr: review+

Description Dean Jackson 2021-01-23 13:56:44 PST
In order to enable the ANGLE Metal backend, we need to not regress any LayoutTests. At least the following are failing (just 1.0.3):

[160/580] webgl/1.0.3/conformance/extensions/ext-frag-depth.html failed unexpectedly (text diff)                              
[209/580] webgl/1.0.3/conformance/glsl/constructors/glsl-construct-bvec2.html failed unexpectedly (text diff)
[222/580] webgl/1.0.3/conformance/context/context-hidden-alpha.html failed unexpectedly (text diff)
[287/580] webgl/1.0.3/conformance/textures/copy-tex-image-2d-formats.html failed unexpectedly (text diff)
[314/580] webgl/1.0.3/conformance/renderbuffers/framebuffer-object-attachment.html failed unexpectedly (text diff)
[344/580] webgl/1.0.3/conformance/programs/program-test.html passed unexpectedly         
[360/580] webgl/1.0.3/conformance/rendering/line-loop-tri-fan.html failed unexpectedly (text diff)
[371/580] webgl/1.0.3/conformance/glsl/constructors/glsl-construct-ivec2.html failed unexpectedly (text diff)
[403/580] webgl/1.0.3/conformance/uniforms/out-of-bounds-uniform-array-access.html failed unexpectedly (text diff)
[445/580] webgl/1.0.3/conformance/glsl/constructors/glsl-construct-vec2.html failed unexpectedly (text diff)
[450/580] webgl/1.0.3/conformance/glsl/constructors/glsl-construct-vec-mat-corner-cases.html failed unexpectedly (text diff)
[521/580] webgl/1.0.3/conformance/ogles/GL/mat/mat_041_to_046.html failed unexpectedly (test timed out, text diff)
[521/580] webgl/1.0.3/conformance/ogles/GL/functions/functions_113_to_120.html failed unexpectedly (test timed out, text diff)
[540/580] webgl/1.0.3/conformance/glsl/misc/shaders-with-invariance.html failed unexpectedly (text diff)      
[547/580] webgl/1.0.3/conformance/glsl/misc/shaders-with-uniform-structs.html failed unexpectedly (text diff)
[568/580] webgl/1.0.3/conformance/glsl/misc/struct-specifiers-in-uniforms.html failed unexpectedly (text diff)
[580/580] webgl/1.0.3/conformance/ogles/GL/functions/functions_121_to_126.html failed unexpectedly (test timed out, text diff)
Comment 1 Radar WebKit Bug Importer 2021-01-23 13:57:22 PST
Comment 2 Kyle Piddington 2021-02-10 10:53:56 PST
Created attachment 419865 [details]
Comment 3 EWS Watchlist 2021-02-10 10:54:59 PST
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
Comment 4 Kenneth Russell 2021-02-15 19:48:13 PST
Comment on attachment 419865 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=419865&action=review

Nice work Kyle. Looks good to me overall. A few minor comments. r+

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect.cpp:611
> +    // Replace gl_FragData() with our globally defined fragdata

gl_FragData is a variable rather than a function, so suggest removing the ().

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect.cpp:1139
> +                else if ( outputVarying.name == "gl_FragDepthEXT")

Remove inconsistent space after (.

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect.cpp:1420
> +    

Strange whitespace.

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/EmitMetal.cpp:1896
> +    putAngle("texture2DRect");

Hoping these additions don't accidentally add support for rectangular textures in WebGL shaders. I think there are enough negative tests to prevent that.

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/FixTypeConstructors.cpp:18
> +class FixTypeTraverser : public TIntermTraverser

Could you please add a per-class comment about what this pass does?

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/FixTypeConstructors.cpp:23
> +        bool visitAggregate(Visit visit, TIntermAggregate *aggregateNode) override

Strange indentation.

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/FixTypeConstructors.cpp:29
> +        if(aggregateNode->isConstructor())

Space before "(".

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/FixTypeConstructors.cpp:34
> +                

Please leave a comment as to why nothing needs to be done.

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/FixTypeConstructors.cpp:36
> +            else if(retType.isVector())

Space before "(". Here again, and throughout.

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/FixTypeConstructors.cpp:70
> +                            assert(!"Should not be reached in case of 0, or 4");

Why not? Please add comment.

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/ProgramPrelude.cpp:648
> +ANGLE_ALWAYS_INLINE int ANGLE_int_clamp(int value, int minValue, int maxValue)

Note: it might not be guaranteed that these ANGLE_ declarations won't conflict with something the user declares. See WebGL's reserved identifiers in https://www.khronos.org/registry/webgl/specs/latest/1.0/#4.3 . It's fine for this patch, just FYI.

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/ReduceInterfaceBlocks.cpp:24
>  class Reducer : public TIntermRebuild

Possible to document what this (preexisting) class does?

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/ReduceInterfaceBlocks.cpp:29
> +    IdGen & mIdGen;

No space after "&" per ANGLE style. Here and below.

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/ReduceInterfaceBlocks.cpp:51
> +                    //Create instance variable

Could you document a little bit more what this instance variable is being created for?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/IOSurfaceSurfaceMtl.mm:438
> +        mIOSurfaceTexture->setColorWritableMask(MTLColorWriteMaskAll & (~MTLColorWriteMaskAlpha));

It's great that disabling alpha writes into these IOSurfaces works with Metal. When attempting to do this with OpenGL there were some cases that were broken - for example BlitFramebuffer into the texture would destroy the alpha channel. There are workarounds at higher levels (in Chromium, at least) that should be removed once the Metal backend's turned on.
Comment 5 Dean Jackson 2021-02-19 18:09:41 PST
Committed r273188 (234374@main): <https://commits.webkit.org/234374@main>
Comment 6 WebKit Commit Bot 2021-02-19 19:07:22 PST
Re-opened since this is blocked by bug 222218
Comment 7 Chris Dumez 2021-02-19 19:07:58 PST
Reverted r273188 for reason:

Broke the build: Is missing a cpp file

Committed r273189 (234375@main): <https://commits.webkit.org/234375@main>
Comment 8 Dean Jackson 2021-02-20 10:15:19 PST
Committed r273200 (234386@main): <https://commits.webkit.org/234386@main>