[webkit-reviews] review granted: [Bug 225842] [ANGLE Metal] Support provoking vertex emulation, pass fragmentOutput tests : [Attachment 428719] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Kyle Piddington
<kpiddington at apple.com>'s request for review:
Bug 225842: [ANGLE Metal] Support provoking vertex emulation, pass
fragmentOutput tests
https://bugs.webkit.org/show_bug.cgi?id=225842

Attachment 428719: Patch

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




--- Comment #3 from Dean Jackson <dino at apple.com> ---
Comment on attachment 428719
  --> https://bugs.webkit.org/attachment.cgi?id=428719
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:14
6
> +}
> +
> +
> +
> +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:16
0
> +	       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.met
al:40
> +	   }
> +	   if(indexBufferIsUint32)

else if

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

And here (and below in this function).