[webkit-reviews] review granted: [Bug 220895] Failures with Metal ANGLE backend : [Attachment 419865] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Kyle Piddington
<kpiddington at apple.com>'s request for review:
Bug 220895: Failures with Metal ANGLE backend
https://bugs.webkit.org/show_bug.cgi?id=220895

Attachment 419865: Patch

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




--- Comment #4 from Kenneth Russell <kbr at google.com> ---
Comment on attachment 419865
  --> https://bugs.webkit.org/attachment.cgi?id=419865
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/FixTypeCo
nstructors.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/FixTypeCo
nstructors.cpp:23
> +	   bool visitAggregate(Visit visit, TIntermAggregate *aggregateNode)
override

Strange indentation.

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

Space before "(".

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

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

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

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

>
Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/FixTypeCo
nstructors.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/ProgramPr
elude.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/ReduceInt
erfaceBlocks.cpp:24
>  class Reducer : public TIntermRebuild

Possible to document what this (preexisting) class does?

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

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

>
Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/ReduceInt
erfaceBlocks.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.