Bug 220895 – Failures with Metal ANGLE backend

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Dean Jackson]

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)

[Radar WebKit Bug Importer]

<rdar://problem/73539537>

[Kyle Piddington]

Created attachment 419865 [details]
Patch

[EWS Watchlist]

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

[Kenneth Russell]

Comment on attachment 419865 [details]
Patch

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.

[Dean Jackson]

Committed r273188 (234374@main): <https://commits.webkit.org/234374@main>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 222218

[Chris Dumez]

Reverted r273188 for reason:

Broke the build: Is missing a cpp file

Committed r273189 (234375@main): <https://commits.webkit.org/234375@main>

[Dean Jackson]

Committed r273200 (234386@main): <https://commits.webkit.org/234386@main>