Bug 233179

Summary: [WebGPU] Start preparing for serializing commands to the GPU process
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ross.kirsling, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch achristensen: review+, ews-feeder: commit-queue-

Description Myles C. Maxfield 2021-11-16 01:19:27 PST
[WebGPU] Start preparing for serializing commands to the GPU process
Comment 1 Myles C. Maxfield 2021-11-16 01:44:32 PST
Created attachment 444358 [details]
Patch
Comment 2 Alex Christensen 2021-11-16 07:54:04 PST
Comment on attachment 444358 [details]
Patch

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

r=me with many nits.  Comments about namespaces and std::function apply several times in the patch.

> Source/WebKit/Shared/WebGPU/WebGPUBindGroupDescriptor.h:34
> +namespace WebKit {
> +namespace WebGPU {

namespace WebKit::WebGPU

> Source/WebKit/Shared/WebGPU/WebGPUBindGroupEntry.h:32
> +#include <utility>
> +#include <variant>

I don't see these used directly

> Source/WebKit/Shared/WebGPU/WebGPUBindGroupEntry.h:37
> +enum class BindingResourceType {

: uint8_t

> Source/WebKit/Shared/WebGPU/WebGPUConvertFromBackingContext.h:68
> +    virtual const PAL::WebGPU::Adapter* convertAdapterFromBacking(WebGPUIdentifier) = 0;

Do we want to return raw pointers here?  What will be the lifetime of these objects?

> Source/WebKit/Shared/WebGPU/WebGPUDepthStencilState.h:40
> +    bool depthWriteEnabled;

Do we want to have default initializers for these to avoid uninitialized memory?

> Source/WebKit/Shared/WebGPU/WebGPUDeviceDescriptor.h:40
> +    // Vector<KeyValuePair<String, uint64_t>> requiredLimits;

Can we avoid adding commented code like this?

> Source/WebKit/Shared/WebGPU/WebGPUOrigin3D.h:43
> +}

You have comments elsewhere indicating that this closes a namespace.

> Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteAdapterProxy.h:62
> +    Deque<std::function<void(Ref<PAL::WebGPU::Device>)>> m_callbacks;

Can we use WTF::Function instead of std::function?
Comment 3 Myles C. Maxfield 2021-11-16 12:50:39 PST
Comment on attachment 444358 [details]
Patch

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

>> Source/WebKit/Shared/WebGPU/WebGPUConvertFromBackingContext.h:68
>> +    virtual const PAL::WebGPU::Adapter* convertAdapterFromBacking(WebGPUIdentifier) = 0;
> 
> Do we want to return raw pointers here?  What will be the lifetime of these objects?

These return non-owning nullable references.

This is used in the GPU Process. WebGPU objects will be owned by a registry, and the registry will receive messages to create / delete things. When the GPU Process receives a rendering command, that command will have one of the WebKit::WebGPU structs as its parameter, which will hold WebGPUIdentifiers inside it. There will be a function to convert a WebKit::WebGPU struct to a PAL::WebGPU struct, thereby replacing the WebGPUIdentifiers with references to objects inside the registry. That function will take one of these ConvertFromBackingContext objects as a parameter. This interface will be implemented by the registry, and it will receive the WebGPUIdentifier and return a non-owning reference to the WebGPU object it owns internally. Then, the PAL::WebGPU class complete with non-owning references will be passed down into WebGPU.framework where the rendering command will be executed.

>> Source/WebKit/Shared/WebGPU/WebGPUDepthStencilState.h:40
>> +    bool depthWriteEnabled;
> 
> Do we want to have default initializers for these to avoid uninitialized memory?

Yes. That's a big change, though, so I'll do it in a follow-up.
Comment 4 Myles C. Maxfield 2021-11-16 13:19:59 PST
Committed r285881 (244307@main): <https://commits.webkit.org/244307@main>
Comment 5 Radar WebKit Bug Importer 2021-11-16 13:20:31 PST
<rdar://problem/85472586>
Comment 6 Ross Kirsling 2021-11-16 18:44:12 PST
This broke WinCairo, as your cq- shows... :(
Comment 7 Ross Kirsling 2021-11-16 20:29:49 PST
Fixed cmake ENABLE(GPU_PROCESS) build in r285881.