Bug 194258 - [Web GPU] Vertex Buffers/Input State API updates for MVP
Summary: [Web GPU] Vertex Buffers/Input State API updates for MVP
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-04 17:07 PST by Justin Fan
Modified: 2019-05-30 14:05 PDT (History)
6 users (show)

See Also:


Attachments
Patch (75.06 KB, patch)
2019-05-28 22:15 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.74 MB, application/zip)
2019-05-28 23:40 PDT, EWS Watchlist
no flags Details
Patch (77.27 KB, patch)
2019-05-29 12:09 PDT, Justin Fan
no flags Details | Formatted Diff | Diff
Patch for landing (77.29 KB, patch)
2019-05-30 13:25 PDT, Justin Fan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Fan 2019-02-04 17:07:30 PST
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.
Comment 1 Radar WebKit Bug Importer 2019-02-04 17:07:56 PST
<rdar://problem/47806127>
Comment 2 Justin Fan 2019-05-28 22:15:01 PDT
Created attachment 370829 [details]
Patch
Comment 3 EWS Watchlist 2019-05-28 23:40:41 PDT
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
Comment 4 EWS Watchlist 2019-05-28 23:40:43 PDT
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
Comment 5 Darin Adler 2019-05-29 09:49:36 PDT
Comment on attachment 370829 [details]
Patch

Looks like the test webgpu/whlsl-dereference-pointer-should-type-check.html needs to be fixed.
Comment 6 Justin Fan 2019-05-29 12:02:46 PDT
(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.
Comment 7 Justin Fan 2019-05-29 12:09:41 PDT
Created attachment 370873 [details]
Patch
Comment 8 Myles C. Maxfield 2019-05-29 17:53:31 PDT
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?
Comment 9 Justin Fan 2019-05-30 13:25:53 PDT
Created attachment 370973 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2019-05-30 14:05:22 PDT
Comment on attachment 370973 [details]
Patch for landing

Clearing flags on attachment: 370973

Committed r245905: <https://trac.webkit.org/changeset/245905>
Comment 11 WebKit Commit Bot 2019-05-30 14:05:24 PDT
All reviewed patches have been landed.  Closing bug.