Bug 194258 – [Web GPU] Vertex Buffers/Input State API updates for MVP

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Justin Fan]

The Web GPU API will be updated for MVP according to https://github.com/gpuweb/gpuweb/pull/151, https://github.com/gpuweb/gpuweb/pull/174, and https://github.com/gpuweb/gpuweb/pull/186. 

WebKit Web GPU and affected demos and tests should be updated to match.

[Radar WebKit Bug Importer]

<rdar://problem/47806127>

[Justin Fan]

Created attachment 370829 [details]
Patch

[EWS Watchlist]

Comment on attachment 370829 [details]
Patch

Attachment 370829 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12315373

New failing tests:
http/wpt/beacon/cors/cors-preflight-cookie.html
webgpu/whlsl-dereference-pointer-should-type-check.html

[EWS Watchlist]

Created attachment 370832 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Darin Adler]

Comment on attachment 370829 [details]
Patch

Looks like the test webgpu/whlsl-dereference-pointer-should-type-check.html needs to be fixed.

[Justin Fan]

(In reply to Darin Adler from comment #5)
> Comment on attachment 370829 [details]
> Patch
> 
> Looks like the test webgpu/whlsl-dereference-pointer-should-type-check.html
> needs to be fixed.

Strange, this one must have been introduced when I rebased ToT.

[Justin Fan]

Created attachment 370873 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 370873 [details]
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?

[Justin Fan]

Created attachment 370973 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 370973 [details]
Patch for landing

Clearing flags on attachment: 370973

Committed r245905: <https://trac.webkit.org/changeset/245905>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.