[webkit-reviews] review granted: [Bug 194258] [Web GPU] Vertex Buffers/Input State API updates for MVP : [Attachment 370873] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Justin Fan
<justin_fan at apple.com>'s request for review:
Bug 194258: [Web GPU] Vertex Buffers/Input State API updates for MVP
https://bugs.webkit.org/show_bug.cgi?id=194258

Attachment 370873: Patch

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




--- Comment #8 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 370873
  --> https://bugs.webkit.org/attachment.cgi?id=370873
Patch

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

> Source/WebCore/ChangeLog:3
> +	   [Web GPU] Vertex Buffers/Input State API updates for MVP

Nit: remove "for MVP"

It's not like we're going to revert this patch after the MVP.

> Source/WebCore/Modules/webgpu/GPUVertexBufferDescriptor.idl:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

2019

> Source/WebCore/platform/graphics/gpu/GPUVertexBufferDescriptor.h:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

Ditto for all the other files

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:205
> +	   if (attributes.size() + attributeIndex > 16) {

magic constant. At least make it a constexpr variable with a name

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:216
> +	   auto convertedBufferIndex =
WHLSL::Metal::calculateVertexBufferIndex(index);

You can probably move this out of WHLSL. It doesn't need to be there.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:226
> +		   LOG(WebGPU, "%s: Invalid shaderLocation %u for vertex
attribute!", functionName, attribute.shaderLocation);

Shouldn't this say "duplicate" instead of "invalid"?

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:246
> +	       auto mtlAttributeDesc = retainPtr([attributeArray
objectAtIndexedSubscript:attributeIndex]);
> +	       [mtlAttributeDesc
setFormat:mtlVertexFormatForGPUVertexFormat(attribute.format)];
> +	       [mtlAttributeDesc setOffset:offset];
> +	       [mtlAttributeDesc setBufferIndex:convertedBufferIndex];
> +	       END_BLOCK_OBJC_EXCEPTIONS;
> +
> +	       if (whlslDescriptor)
> +		   whlslDescriptor->vertexAttributes.append({
convertVertexFormat(attribute.format), attribute.shaderLocation, attributeIndex
});
> +
> +	       ++attributeIndex;

Cool. This is quite elegant.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:573
> +    // FIXME: depthStencilAttachmentDescriptor isn't implemented yet for
WHLSL compiler.

Can we have a new bugzilla bug and link here?