Bug 192611 - [WebGPU] Vertex buffers and WebGPUInputState
Summary: [WebGPU] Vertex buffers and WebGPUInputState
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Fan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-12-11 20:28 PST by Justin Fan
Modified: 2018-12-12 17:26 PST (History)
3 users (show)

See Also:


Attachments
Patch (89.42 KB, patch)
2018-12-11 21:43 PST, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (88.02 KB, patch)
2018-12-11 21:57 PST, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (86.05 KB, patch)
2018-12-11 22:14 PST, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (88.15 KB, patch)
2018-12-12 13:29 PST, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (88.14 KB, patch)
2018-12-12 13:56 PST, Justin Fan
no flags Details | Formatted Diff | Diff
Patch (88.18 KB, patch)
2018-12-12 15:37 PST, 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 2018-12-11 20:28:13 PST
[WebGPU] Vertex buffers and WebGPUInputState
Comment 1 Justin Fan 2018-12-11 20:29:16 PST
<rdar://problem/46608344>
Comment 2 Justin Fan 2018-12-11 21:43:08 PST
Created attachment 357100 [details]
Patch
Comment 3 Justin Fan 2018-12-11 21:57:33 PST
Created attachment 357102 [details]
Patch
Comment 4 Justin Fan 2018-12-11 22:14:41 PST
Created attachment 357104 [details]
Patch
Comment 5 Dean Jackson 2018-12-12 12:37:44 PST
Comment on attachment 357104 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        Add symbols and files for WebGPUIndexFormat, WebGPUInputStateDescriptor, WebGPUInputStepMode, 
> +        WebGPUVertexAttributeDescriptor, WebGPUVertexFormat, WebGPUVertexInputDescriptor:

I know it's annoying, but I suggest for the future that each of these could be a different patch. While this means you'll have to stage them and get multiple reviews, it's better to have small changes than big ones. Basically, if you're adding more than one class in a patch, there should be a good reason.

Don't do it for this patch now, but in the future.

> Source/WebCore/Modules/webgpu/WebGPUIndexFormat.idl:37
> +] interface WebGPUIndexFormat {
> +    const u32 UINT16 = 0;
> +    const u32 UINT32 = 1;
> +};

I'm going to file a bug on the spec asking why this isn't an enum.

> Source/WebCore/Modules/webgpu/WebGPUInputStepMode.idl:31
> +    DoNotCheckConstants,

Did we decide if we need this?

> Source/WebCore/Modules/webgpu/WebGPUInputStepMode.idl:37
> +] interface WebGPUInputStepMode {
> +    const u32 VERTEX = 0;
> +    const u32 INSTANCE = 1;
> +};

And this one.

> Source/WebCore/platform/graphics/gpu/GPUVertexAttributeDescriptor.h:44
> +class GPUVertexFormat : public RefCounted<GPUVertexFormat> {
> +public:
> +    enum Enum : GPUVertexFormatEnum {
> +        FloatR32G32B32A32 = 0,
> +        FloatR32G32B32 = 1,
> +        FloatR32G32 = 2,
> +        FloatR32 = 3
> +    };
> +};

And if we made these enums in the IDL, we wouldn't need an instance.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPassEncoderMetal.mm:90
> +    // FIXME: Only worry about the first buffer for now, and treat startSlot as the index.
> +    [m_platformRenderPassEncoder setVertexBuffer:buffers[0]->platformBuffer() offset:offsets[0] atIndex:index];

Please check for empty vectors.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:118
> +    // FIXME: What happens here when mtlVertexDescriptor RetainPtr is destroyed?
> +    mtlDescriptor.vertexDescriptor = mtlVertexDescriptor.get();

I assume that the MTLVertexDescriptor retains the MTLRenderPipelineDescriptor.

> LayoutTests/webgpu/vertex-buffer-triangle-strip.html:57
> +    let arrayBuffer = buffer.mapping;
> +    let floatArray = new Float32Array(arrayBuffer);
> +
> +    floatArray[0] = -1;
> +    floatArray[1] = 1;
> +    floatArray[2] = 0;
> +    floatArray[3] = 1;
> +
> +    floatArray[4] = -1;
> +    floatArray[5] = -1;
> +    floatArray[6] = 0;
> +    floatArray[7] = 1;
> +
> +    floatArray[8] = 1;
> +    floatArray[9] = 1;
> +    floatArray[10] = 0;
> +    floatArray[11] = 1;
> +
> +    floatArray[12] = 1;
> +    floatArray[13] = -1;
> +    floatArray[14] = 0;
> +    floatArray[15] = 1;
> +

I don't know how this works without mapping it to the vertex buffer.
Comment 6 Justin Fan 2018-12-12 13:29:06 PST
Created attachment 357160 [details]
Patch
Comment 7 Justin Fan 2018-12-12 13:56:44 PST
Created attachment 357164 [details]
Patch
Comment 8 Justin Fan 2018-12-12 15:37:52 PST
Created attachment 357179 [details]
Patch
Comment 9 WebKit Commit Bot 2018-12-12 17:26:40 PST
Comment on attachment 357179 [details]
Patch

Clearing flags on attachment: 357179

Committed r239139: <https://trac.webkit.org/changeset/239139>
Comment 10 WebKit Commit Bot 2018-12-12 17:26:42 PST
All reviewed patches have been landed.  Closing bug.