Bug 220895 - Failures with Metal ANGLE backend
Summary: Failures with Metal ANGLE backend
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on: 222218
Blocks: 220076
  Show dependency treegraph
 
Reported: 2021-01-23 13:56 PST by Dean Jackson
Modified: 2021-02-20 10:15 PST (History)
9 users (show)

See Also:


Attachments
Patch (60.73 KB, patch)
2021-02-10 10:53 PST, Kyle Piddington
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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
<rdar://problem/73539537>
Comment 2 Kyle Piddington 2021-02-10 10:53:56 PST
Created attachment 419865 [details]
Patch
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]
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.
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>