[webkit-reviews] review granted: [Bug 191291] [WebGPU] Experimental prototype for WebGPURenderPipeline and WebGPUSwapChain : [Attachment 354034] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Justin Fan
<justin_fan at apple.com>'s request for review:
Bug 191291: [WebGPU] Experimental prototype for WebGPURenderPipeline and
WebGPUSwapChain
https://bugs.webkit.org/show_bug.cgi?id=191291

Attachment 354034: Patch

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




--- Comment #15 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 354034
  --> https://bugs.webkit.org/attachment.cgi?id=354034
Patch

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

> Source/WebCore/Modules/webgpu/GPUPipelineDescriptorBase.h:37
> +    Vector<GPUPipelineStageDescriptor> stages;

Are you sure this is the right internal design? We need to differentiate
between the vertex shader and the fragment shader when we compile the state.
That shouldn't have to iterate through a list of things just to find which item
is for the fragment shader.

> Source/WebCore/Modules/webgpu/GPUPipelineStageDescriptor.h:37
> +    const GPUShaderModule& module;

What happens when the module gets garbage collected? Will this be dangling?

> Source/WebCore/Modules/webgpu/GPURenderPipelineDescriptor.h:37
> +struct GPURenderPipelineDescriptor : GPUPipelineDescriptorBase {

Why struct?

> Source/WebCore/Modules/webgpu/GPURenderPipelineDescriptor.h:52
> +    int primitiveTopology;

This shouldn't be an int.

> Source/WebCore/Modules/webgpu/WebGPUPipelineStageDescriptor.h:39
> +    const WebGPUShaderModule* module = nullptr;

Ditto about the garbage collector

> Source/WebCore/Modules/webgpu/WebGPUShaderStage.h:36
> +    enum {

enum class

> Source/WebCore/Modules/webgpu/WebGPUShaderStage.h:39
> +	   VERTEX = 0,
> +	   FRAGMENT = 1,
> +	   COMPUTE = 2

Can we use different internal names for these? Our style is not to use ALL
CAPS.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipeline.h:50
> +#else
> +using PlatformRenderPipeline = void;
> +using PlatformRenderPipelineSmartPtr = RefPtr<void>;
> +#endif

I think it's fine to assume we're not using metal for now. We can break that
assumption one day when the linux ports want WebGPU.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:88
> +    BEGIN_BLOCK_OBJC_EXCEPTIONS;

We should be deliberate about where we use these.

> Source/WebCore/bindings/js/WebCoreBuiltinNames.h:182
> +    macro(WebGPURenderPipeline) \
> +    macro(WebGPUShaderStage) \

Why aren't the other objects in here?

> LayoutTests/webgpu/render-pipelines.html:5
> +<script id="library" type="x-shader/x-metal">

😐

> LayoutTests/webgpu/webgpu-basics.html:129
> +    // let commandBuffer = defaultDevice.createCommandBuffer();
> +    // if (!commandBuffer) {
> +    //     testFailed("Could not create WebGPUCommandBuffer!");
> +    //     return;
> +    // }
> +
> +    // let texture = context.getNextTexture();
> +    // if (!texture) {
> +    //     testFailed("Could not get next WebGPUTexture!");
> +    //     return;
> +    // }
> +
> +    // let textureView = context.createTextureView();
> +    // if (!textureView) {
> +    //     testFailed("Could not create WebGPUTextureView!");
> +    //     return;
> +    // }
> +
> +    // // FIXME: Flesh out the rest of WebGPURenderPassDescriptor. Default
loadOp, storeOp, clearColor for now.
> +    // let renderPassDescriptor = {
> +    //     attachment : textureView
> +    // };
> +
> +    // let renderPassEncoder =
commandBuffer.beginRenderPass(renderPassDescriptor);
> +    // renderPassEncoder.setPipeline(renderPipeline);
> +
> +    // // Note that we didn't attach any buffers - the shader is generating
the vertices for us.
> +    // renderPassEncoder.draw(3, 1, 0, 0);
> +    // renderPassEncoder.endPass();
> +
> +    // let queue = device.getQueue();
> +    // if (!queue) {
> +    //     testFailed("Unable to create default WebGPUQueue!");
> +    //     return;
> +    // }
> +    // queue.submit([commandBuffer]);
> +
> +    // context.present();

We don't leave code commented in WebKit.