Bug 220137 – Failures of attribute location conformance tests with Metal backend

• Component: WebGL
• Status: RESOLVED FIXED

[Kenneth Russell]

Per Bug 220076, the following three WebGL 1.0.3 conformance tests are failing with the Metal backend:

webgl/1.0.3/conformance/attribs/gl-disabled-vertex-attrib.html
webgl/1.0.3/conformance/attribs/gl-vertex-attrib-render.html
webgl/1.0.3/conformance/attribs/gl-vertex-attrib-zero-issues.html

This is basic functionality; the Metal backend must not be enabled by default before these bugs are tracked down and fixed.

[Radar WebKit Bug Importer]

<rdar://problem/72750062>

[Kyle Piddington]

Created attachment 417121 [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 417121 [details]
Patch

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

Great work handling this! A few questions, but assuming this passes tests, r+.

> Source/ThirdParty/ANGLE/src/compiler/translator/TranslatorMetalDirect/EmitMetal.cpp:1229
> +     mOut << " __unassigned_attribute__ ";

Is the indentaion off here?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_glslang_mtl_utils.mm:151
> +    //Build string to attrib map.

Nit: space after //.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_glslang_mtl_utils.mm:152
> +    const std::vector<sh::ShaderVariable> programAttributes = programState.getProgramInputs();

How about "const auto&"? I think this will perform a copy as written.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_glslang_mtl_utils.mm:163
> +                stream << attribute.name << "_" << std::to_string(i) << " __unassigned_attribute__";

How about a variable for the constant string " __unassigned_attribute__"?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_glslang_mtl_utils.mm:177
> +    for(std::unordered_map<std::string, uint32_t>::iterator it = attributeBindings.begin(); it!=attributeBindings.end(); ++it)

Is "for (auto it = attributeBindings::begin(); ..." workable here?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_glslang_mtl_utils.mm:186
> +    }

What happens if a given attribute wasn't assigned a location via glBindAttribLocation? Would the " __unassigned_attribute__" strings cause MSL compilation failures?

[Kyle Piddington]

> What happens if a given attribute wasn't assigned a location via glBindAttribLocation? Would the " __unassigned_attribute__" strings cause MSL compilation failures?

Any active attribute should get a location, even if they aren't specifically bound. This is handled in the linkAttributes() (See Program.cpp) step in the Angle frontend.

Any non-active attributes should be optimized out, but if something went wrong, we would have a compilation failure.

[Kenneth Russell]

(In reply to Kyle Piddington from comment #5)
> > What happens if a given attribute wasn't assigned a location via glBindAttribLocation? Would the " __unassigned_attribute__" strings cause MSL compilation failures?
> 
> Any active attribute should get a location, even if they aren't specifically
> bound. This is handled in the linkAttributes() (See Program.cpp) step in the
> Angle frontend.
> 
> Any non-active attributes should be optimized out, but if something went
> wrong, we would have a compilation failure.

Thanks for your confirmation. Sounds fine.

[Kyle Piddington]

Created attachment 417308 [details]
Patch

[Dean Jackson]

Comment on attachment 417308 [details]
Patch

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

Looks good if the unrelated changes are removed.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_common.h:-126
> -
> -// NOTE(hqle): support variable max number of vertex attributes
> -constexpr uint32_t kMaxVertexAttribs = gl::MAX_VERTEX_ATTRIBS;
> -// NOTE(hqle): support variable max number of render targets
> -constexpr uint32_t kMaxRenderTargets = 4;
> -

I don't think you meant to include the changes in this file.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_constants.h:35
> +constexpr uint32_t kMaxShaderXFBs = gl::IMPLEMENTATION_MAX_TRANSFORM_FEEDBACK_SEPARATE_ATTRIBS;
> +
> +// The max size of a buffer that will be allocated in shared memory.
> +// NOTE(hqle): This is just a hint. There is no official document on what is the max allowed size
> +// for shared memory.
> +constexpr size_t kSharedMemBufferMaxBufSizeHint = 128 * 1024;

I think this is from another patch.

[Dean Jackson]

Comment on attachment 417308 [details]
Patch

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

>> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/mtl_common.h:-126
>> -
> 
> I don't think you meant to include the changes in this file.

Kyle explained that this was intentional. Lots of these were defined twice.

[EWS]

Committed r271334: <https://trac.webkit.org/changeset/271334>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417308 [details].