Bug 225842 – [ANGLE Metal] Support provoking vertex emulation, pass fragmentOutput tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Kyle Piddington]

[ANGLE Metal] Support provoking vertex emulation, pass fragmentOutput tests

[Kyle Piddington]

Created attachment 428719 [details]
Patch

[EWS Watchlist]

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

[Dean Jackson]

Comment on attachment 428719 [details]
Patch

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

> Source/ThirdParty/ANGLE/ChangeLog:10
> +        Add support for provoking vertex emulation. Metal only supports using the first vertex
> +        Of a primitive as a provoking vertex. To adapt, rewrite the index buffer on the fly when provoking Vertex support is required. This method does *not* rewrite any primitives that would be 
> +        Culled by primitive restart, such as simple triangles and lines, but should rewrite line and tri paths.

The capitalisation is a bit weird here.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:620
> +        if(drawIdxBuffer == nullptr)

If this is ANGLE style that's fine, but if not we typically just do (!drawIdxBuffer) in WebKit.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ContextMtl.mm:637
> +    

Intentional?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ProgramMtl.mm:498
> +  

Intentional?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ProvokingVertexHelper.h:36
> +    mtl::BufferRef  preconditionIndexBuffer(ContextMtl * context,
> +                                                                   mtl::BufferRef indexBuffer,
> +                                                                   size_t indexCount,
> +                                                                   size_t indexOffset,
> +                                                                   bool primitiveRestartEnabled,
> +                                                                   gl::PrimitiveMode primitiveMode,
> +                                                                   gl::DrawElementsType elementsType,
> +                                                                   size_t & outIndexcount,
> +                                                                   gl::PrimitiveMode & outPrimitiveMode);

Are these indented too much?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ProvokingVertexHelper.mm:146
> +}
> +
> +
> +
> +void ProvokingVertexHelper::ensureCommandBufferReady()
> +{
> +    if (!mCommandBuffer.valid())
> +    {
> +        mCommandBuffer.restart();
> +    }
> +    ASSERT(mCommandBuffer.valid());
> +}
> +
> +
> +
> +static uint buildIndexBufferKey(const mtl::ProvokingVertexComputePipelineDesc &pipelineDesc)

I guess the extra empty lines will be caught when upstreaming to ANGLE.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/ProvokingVertexHelper.mm:160
> +            indexBufferKey |= MtlFixIndexBufferKeyUint16 <<     MtlFixIndexBufferKeyInShift;
> +            indexBufferKey |= MtlFixIndexBufferKeyUint16 << MtlFixIndexBufferKeyOutShift;
> +            break;
> +        case gl::DrawElementsType::UnsignedInt:
> +            indexBufferKey |= MtlFixIndexBufferKeyUint32 <<     MtlFixIndexBufferKeyInShift;
> +            indexBufferKey |= MtlFixIndexBufferKeyUint32 << MtlFixIndexBufferKeyOutShift;

The KeyInShift identation is wrong.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/shaders/mtl_default_shaders_src_autogen.inc:2881
> +        {

Can we add comments with the name of the buffer mode here and below?

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/shaders/mtl_default_shaders_src_autogen.inc:3001
> +            break;

Indentation is off.

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/shaders/rewrite_indices.metal:40
> +        }
> +        if(indexBufferIsUint32)

else if

> Source/ThirdParty/ANGLE/src/libANGLE/renderer/metal/shaders/rewrite_indices.metal:99
> +            break;

And here (and below in this function).

[Kyle Piddington]

Created attachment 428837 [details]
Patch

[Kyle Piddington]

Created attachment 428840 [details]
Patch

[EWS]

commit-queue failed to commit attachment 428840 [details] to WebKit repository. To retry, please set cq+ flag again.

[Kyle Piddington]

Created attachment 428979 [details]
Patch for Landing

[EWS]

Committed r277741 (237913@main): <https://commits.webkit.org/237913@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/78217333>