[webkit-reviews] review granted: [Bug 192134] [WebGPU] WebGPURenderPassEncoder::setPipeline, draw, and endPass prototypes : [Attachment 355975] 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 192134: [WebGPU] WebGPURenderPassEncoder::setPipeline, draw, and endPass
prototypes
https://bugs.webkit.org/show_bug.cgi?id=192134

Attachment 355975: Patch

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




--- Comment #3 from Dean Jackson <dino at apple.com> ---
Comment on attachment 355975
  --> https://bugs.webkit.org/attachment.cgi?id=355975
Patch

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

> Source/WebCore/Modules/webgpu/WebGPUProgrammablePassEncoder.cpp:46
> +Ref<WebGPUCommandBuffer> WebGPUProgrammablePassEncoder::endPass()
> +{
> +    passEncoder().endPass();
> +    return m_commandBuffer.copyRef();
> +}

I know the IDL specifies it, but I wonder why endPass returns a command buffer.
Surely the author would have created it and held a reference? Maybe this is an
issue for the spec?

> Source/WebCore/Modules/webgpu/WebGPUProgrammablePassEncoder.idl:34
> +    // FIXME: Only support render pipelines for demo.

s/demo/prototype/

> Source/WebCore/Modules/webgpu/WebGPURenderPassEncoder.cpp:52
> +void WebGPURenderPassEncoder::draw(
> +    unsigned long vertexCount,
> +    unsigned long instanceCount,
> +    unsigned long firstVertex,
> +    unsigned long firstInstance
> +    )

Put all this on one line.

> Source/WebCore/Modules/webgpu/WebGPURenderPassEncoder.cpp:54
> +{
> +    m_passEncoder->draw(vertexCount, instanceCount, firstVertex,
firstInstance);

Could you put a // FIXME in here saying we need to do some validation. e.g. all
values are > 0, etc.

> Source/WebCore/platform/graphics/gpu/GPURenderPassEncoder.h:58
> +    ~GPURenderPassEncoder() { endPass(); } // The MTLCommandEncoder must end
encoding before it is released. 

Remember that this file will ultimately be used by backends other than Metal.
So maybe the comment should be "Ensure that the encoding has ended before
destruction."

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPassEncoderMetal.mm:104
> +void GPURenderPassEncoder::draw(
> +    unsigned long vertexCount,
> +    unsigned long instanceCount,
> +    unsigned long firstVertex,
> +    unsigned long firstInstance
> +    )

One line.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPassEncoderMetal.mm:111
> +    [m_platformRenderPassEncoder 
> +   
drawPrimitives:primitiveTypeForGPUPrimitiveTopology(m_pipeline->primitiveTopolo
gy())
> +    vertexStart:firstVertex
> +    vertexCount:vertexCount
> +    instanceCount:instanceCount
> +    baseInstance:firstInstance];

Either everything on a single line, or the 2nd+ lines should be indented.

> LayoutTests/webgpu/render-command-encoding.html:12
> +let canvas, commandBuffer, renderPassEncoder;

canvas is only used inside the promise test.

I also wonder if the other variables should be passed into the functions as
parameters.