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.
<rdar://problem/72750062>
Created attachment 417121 [details] Patch
Note that there are important steps to take when updating ANGLE. See https://trac.webkit.org/wiki/UpdatingANGLE
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?
> 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.
(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.
Created attachment 417308 [details] Patch
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.
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.
Committed r271334: <https://trac.webkit.org/changeset/271334> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417308 [details].