[webkit-reviews] review granted: [Bug 192611] [WebGPU] Vertex buffers and WebGPUInputState : [Attachment 357104] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Justin Fan <justin_fan at apple.com>'s
request for review:
Bug 192611: [WebGPU] Vertex buffers and WebGPUInputState
https://bugs.webkit.org/show_bug.cgi?id=192611

Attachment 357104: Patch

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




--- Comment #5 from Dean Jackson <dino at apple.com> ---
Comment on attachment 357104
  --> https://bugs.webkit.org/attachment.cgi?id=357104
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.