Bug 191663 – [WebGPU] WebGPUCommandBuffer prototype

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Justin Fan]

[WebGPU] WebGPUCommandBuffer prototype

[Justin Fan]

Created attachment 354864 [details]
Patch

[Justin Fan]

Patch doesn't apply because it relies on 191383, which will land soon.

[Dean Jackson]

Comment on attachment 354864 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Begin implementation of WebGPUCommandBuffers as well as GPUQueues (MTLCommandBuffer, MTLCommandQueue).

This patch has a bit more than just that.

Also, I suggest you get into the habit of writing more detail in the section below, file by file. Look at one of Darin's patches for an example of how helpful that is. There isn't a good reason to simply list the set of files changed without explaining what happened.

> Source/WebCore/Modules/webgpu/GPUCommandBuffer.h:2
> +/*
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

Not for this patch, but I think all the GPU* files should be under platform, not modules. The WebGPU* files can live here.

> Source/WebCore/Modules/webgpu/GPUDevice.h:31
> +#include "GPUCommandBuffer.h"
> +#include "GPUQueue.h"

Can't we just forward declare these?

> Source/WebCore/Modules/webgpu/GPUQueue.h:47
> +    PlatformQueue *platformQueue() const { return m_platformQueue.get(); }

Nit: * after PlatformQueue

> Source/WebCore/Modules/webgpu/WebGPUCommandBuffer.cpp:46
> +    UNUSED_PARAM(m_commandBuffer);

Why do you need this? Is it because the member variable is never used?

> Source/WebCore/Modules/webgpu/cocoa/GPUCommandBufferMetal.mm:57
> +    buffer = [mtlQueue commandBuffer];

Don't you need to adoptNS on this?

> Source/WebCore/Modules/webgpu/cocoa/GPUQueueMetal.mm:50
> +    queue = [device.platformDevice() newCommandQueue];

Needs adoptNS

> LayoutTests/webgpu/command-buffers.html:38
> +<script id="library" type="x-shader/x-metal">
> +    #include <metal_stdlib>
> +    
> +    using namespace metal;
> +    
> +    struct Vertex
> +    {
> +        float4 position [[position]];
> +    };
> +    
> +    vertex Vertex vertex_main(uint vid [[vertex_id]])
> +    {
> +        Vertex v;
> +        switch (vid) {
> +        case 0:
> +            v.position = float4(-.75, -.75, 0, 1);
> +            break;
> +        case 1:
> +            v.position = float4(.75, -.75, 0, 1);
> +            break;
> +        case 2:
> +            v.position = float4(0, .75, 0, 1);
> +            break;
> +        default:
> +            v.position = float4(0, 0, 0, 1);
> +        }
> +        return v;
> +    }
> +    
> +    fragment float4 fragment_main(Vertex vertexIn [[stage_in]])
> +    {
> +        return float4(1.0, 0.0, 0.0, 1.0);
> +    }
> +</script>

You don't need a shader for this test.

> LayoutTests/webgpu/command-buffers.html:40
> +'use strict';

Unimportant, but this isn't necessary here.

> LayoutTests/webgpu/webgpu-basics.html:91
> +function render() {

How is this different from the test above? Shouldn't it have a more accurate name for the moment?

[Justin Fan]

(In reply to Dean Jackson from comment #3)
> Comment on attachment 354864 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354864&action=review
> 
> > Source/WebCore/Modules/webgpu/GPUDevice.h:31
> > +#include "GPUCommandBuffer.h"
> > +#include "GPUQueue.h"
> 
> Can't we just forward declare these?


Hmm, the only other place I can put them and have my code still compile (problems in Ref.h template code) is WebGPUDevice.h, and that makes even less sense since there is no mention of either class.

[Justin Fan]

Created attachment 354970 [details]
Patch

[Justin Fan]

(In reply to Dean Jackson from comment #3)
> Comment on attachment 354864 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354864&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Begin implementation of WebGPUCommandBuffers as well as GPUQueues (MTLCommandBuffer, MTLCommandQueue).
> 
> This patch has a bit more than just that.

Hmm, I can't really come up with a better description for this. Suggestions?

There's a lot of code but the fact is, implementing this stuff is quite a bit of boilerplate.

[Dean Jackson]

Comment on attachment 354970 [details]
Patch

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

> Source/WebCore/Modules/webgpu/cocoa/GPUQueueMetal.mm:50
> +    queue = adoptNS([device.platformDevice() newCommandQueue]);

Why did you use retainPtr in the other file and adoptNS here?

[WebKit Commit Bot]

Comment on attachment 354970 [details]
Patch

Clearing flags on attachment: 354970

Committed r238245: <https://trac.webkit.org/changeset/238245>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46105786>