[webkit-reviews] review denied: [Bug 191084] [WebGPU] Experimental prototype for MSL shaders and render pipelines : [Attachment 353416] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Justin Fan <justin_fan at apple.com>'s
request for review:
Bug 191084: [WebGPU] Experimental prototype for MSL shaders and render
pipelines
https://bugs.webkit.org/show_bug.cgi?id=191084

Attachment 353416: Patch

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




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

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

> Source/WebCore/ChangeLog:10
> +	   Begin implementation for WebGPUDevice, WebGPUSwapChain,
WebGPUShaderModule, WebGPURenderPipeline,
> +	   and any associated descriptor objects. This prototype compiles Metal
Shading Language, which will not 
> +	   be available for the final implementation.

These should be split up into separate patches. One for creating the shader
modules, one for the swap chain, and then one for the render pipeline.

> Source/WebCore/Modules/webgpu/GPUDevice.h:30
> +#include <wtf/Ref.h>

Don't need this.

> Source/WebCore/Modules/webgpu/GPUDevice.h:34
> +OBJC_PROTOCOL(MTLDevice);

Guard with USE(METAL)

> Source/WebCore/Modules/webgpu/GPUDevice.h:41
> +struct WebGPURenderPipelineDescriptor;

Needs to be GPURenderPipelineDescriptor. See below.

> Source/WebCore/Modules/webgpu/GPUDevice.h:43
> +class GPUDevice {

This should be a RefCounted object.

public RefCounted<GPUDevice>

> Source/WebCore/Modules/webgpu/GPUDevice.h:45
> +    GPUDevice();

Make this private. Use a static RefPtr<GPUDevice> create() method to
initialise.

> Source/WebCore/Modules/webgpu/GPUDevice.h:48
> +    GPURenderPipeline createRenderPipeline(WebGPURenderPipelineDescriptor&&)
const;

This should not take a WebGPU object as a parameter. Everything that is GPUFoo*
needs to be ignorant of WebGPU*.

> Source/WebCore/Modules/webgpu/GPUDevice.h:51
> +    MTLDevice *mtlDevice() const { return m_mtlDevice.get(); }

We often call these accessors platformDevice(), and have a typedef for
PlatformDevice.

I'm not sure if we should do this right away, since Metal is the only backend
for now. It might be a good idea though.

> Source/WebCore/Modules/webgpu/GPUShaderModule.h:33
> +OBJC_PROTOCOL(MTLLibrary);

USE(METAL)

> Source/WebCore/Modules/webgpu/GPUShaderModule.h:39
> +class GPUShaderModule {

RefCounted.

> Source/WebCore/Modules/webgpu/WebGPUDevice.cpp:51
> +    return
WebGPUShaderModule::create(m_device.createShaderModule(descriptor.code));

I feel like the shader module should take the entire descriptor.

> Source/WebCore/Modules/webgpu/WebGPUDevice.h:62
> +    GPUDevice m_device;

This becomes a RefPtr<GPUDevice>

> Source/WebCore/Modules/webgpu/WebGPUDevice.idl:55
> +*/
>      WebGPUShaderModule createShaderModule(WebGPUShaderModuleDescriptor
descriptor);
> +/*
>      WebGPUAttachmentState
createAttachmentState(WebGPUAttachmentStateDescriptor descriptor);
>      WebGPUComputePipeline
createComputePipeline(WebGPUComputePipelineDescriptor descriptor);
> +*/
>      WebGPURenderPipeline createRenderPipeline(WebGPURenderPipelineDescriptor
descriptor);
> -
> +/*

I suggest keeping the unused objects in a group, rather than commenting bits
and pieces out. We can clean up the finished IDL later to look like the
official one.

> Source/WebCore/Modules/webgpu/WebGPUPipelineDescriptorBase.h:37
> +struct WebGPUPipelineDescriptorBase {
> +    Vector<WebGPUPipelineStageDescriptor> stages;
> +};

This is the base class. I wonder if it needs to be RefCounted. We do hold onto
them.

> Source/WebCore/Modules/webgpu/WebGPUPipelineDescriptorBase.idl:28
> +    // FIXME: Figure out what PipelineLayout is for.

Remove this comment.

> Source/WebCore/Modules/webgpu/WebGPURenderPipeline.h:44
> +    GPURenderPipeline m_renderPipeline;

Becomes Ref<>

> Source/WebCore/Modules/webgpu/WebGPURenderPipelineDescriptor.idl:42
> +    /*
> +    sequence<WebGPUBlendState> blendStates;
> +    WebGPUDepthStencilState depthStencilState;
> +    WebGPUInputState inputState;
> +    WebGPUAttachmentsState attachmentsState; */
> +    // TODO other properties

Make this more clear that they are not implemented. Put // in front rather than
a block comment.

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

Even though this is a u32 in the IDL, it doesn't need to be in the
implementation.

> Source/WebCore/Modules/webgpu/WebGPUSwapChain.h:40
> +	   // FIXME: More texture properties.

Remove this, or link to a bug.

> Source/WebCore/Modules/webgpu/WebGPUSwapChain.h:69
> +    GPUSwapChain m_swapChain;

Needs to be a Ref or RefPtr.

> Source/WebCore/Modules/webgpu/cocoa/GPUDeviceMetal.mm:41
> +    : m_mtlDevice { adoptNS(MTLCreateSystemDefaultDevice()) }

I would prefer this go in the function body. Eventually it won't be a call to
creating the default device.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipeline.h:40
> +class GPURenderPipeline {

Should be RefCounted.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipeline.h:42
> +    GPURenderPipeline(const GPUDevice&, WebGPURenderPipelineDescriptor&&);

See above about disconnecting from WebGPU objects.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:41
> +	   return; // FIXME: Throw relevant error before returning.

I think we should do this now. Alas, this probably means a bit of code to throw
exceptions from the WebGPURenderPipeline.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:48
> +	   // FIXME: How many encapsulation laws does this break??

All of them :)

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:49
> +	   auto mtlLibrary =
retainPtr(stageDescriptor.module->m_module.mtlLibrary());

If we access the mtlLibrary this way, the accessor might as well give us a
RetainPtr<> directly.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:52
> +	   if (!mtlLibrary)
> +	       return; // FIXME: Throw relevant error before returning.

Yeah. This means the shader didn't compile.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:56
> +	       [mtlDescriptor setVertexFunction:[mtlLibrary
newFunctionWithName:stageDescriptor.entryPoint]];

We need to throw an error if we were unable to find the function, otherwise we
simply crash when we go to draw.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:67
> +    // FIXME: Get the pixelFormat as configured for the
context/CAMetalLayer.

File a bug and link to it here.

> Source/WebCore/Modules/webgpu/cocoa/GPURenderPipelineMetal.mm:70
> +    // FIXME: Assert that both vertex and fragment functions have been
assigned?

Yes.

> Source/WebCore/Modules/webgpu/cocoa/GPUShaderModuleMetal.mm:45
> +    NSError *error = [NSError errorWithDomain:@"com.apple.WebKit.GPU" code:1
userInfo:nil];
> +    m_mtlLibrary = adoptNS([device.mtlDevice() newLibraryWithSource:code
options:nil error:&error]);
> +    if (!m_mtlLibrary)
> +	   LOG(WebGPU, "Shader compilation error: %s", [[error
localizedDescription] UTF8String]);

Again, we need to handle this properly, otherwise we'll crash horrifically :)

Maybe the IDL doesn't yet expose a way to throw an exception, but we should add
one.

> Source/WebCore/Modules/webgpu/cocoa/GPUSwapChain.h:32
> +OBJC_CLASS CAMetalLayer;

USE(METAL)

> Source/WebCore/Modules/webgpu/cocoa/GPUSwapChain.h:42
> +    void setDevice(const GPUDevice&);

This should take a Ref<>. I'm not sure how the ownership will happen - I guess
the WebGPUSwapChain can go away and break the ref.

> Source/WebCore/Modules/webgpu/cocoa/GPUSwapChainMetal.mm:38
> +    : m_layer(adoptNS([[CAMetalLayer alloc] init]))

Move into function body.

> Source/WebCore/Modules/webgpu/cocoa/GPUSwapChainMetal.mm:43
> +    // FIXME: For now, default to these settings.

File a bug and link to it.

> Source/WebCore/Modules/webgpu/cocoa/GPUSwapChainMetal.mm:51
> +    if (!device.mtlDevice())
> +	   return; // FIXME: Throw appropriate error

We should do this now.

> LayoutTests/ChangeLog:10
> +	   * webgpu/webgpu-basics.html: Added.

You should make new test files to exercise creating the render pipeline and
shader modules.

> LayoutTests/webgpu/webgpu-basics.html:56
> +    window.webgpu.requestAdapter({ powerPreference: "default"
}).then(adapter_ => {

You can remove this { powerPreference } parameter.