Bug 233358

Summary: [WebGPU] Implement GPU process's message receivers
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
dino: review+
Patch for committing
none
Patch for committing
mmaxfield: commit-queue-
Patch for committing none

Description Myles C. Maxfield 2021-11-19 04:26:00 PST
[WebGPU] Implement GPU process's message receivers
Comment 1 Myles C. Maxfield 2021-11-19 04:34:06 PST
Created attachment 444800 [details]
Patch
Comment 2 Dean Jackson 2021-11-19 10:03:15 PST
Comment on attachment 444800 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteBuffer.cpp:52
> +    // FIXME: Implement this.

ASSERT_NOT_IMPLEMENTED()?

> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteCommandEncoder.cpp:72
> +    auto computePassEncoder = m_backing->beginComputePass(*convertedDescriptor);

What happens if convertedDescriptor still doesn't have a value at this point?

> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteCommandEncoder.cpp:88
> +    auto convertedSource = m_objectRegistry.convertBufferFromBacking(source);
> +    ASSERT(convertedSource);
> +    auto convertedDestination = m_objectRegistry.convertBufferFromBacking(source);
> +    ASSERT(convertedDestination);
> +    m_backing->copyBufferToBuffer(*convertedSource, sourceOffset, *convertedDestination, destinationOffset, size);

Should this (and the other similar code) actually test that the converted buffers exist rather than just ASSERT?
Comment 3 Myles C. Maxfield 2021-11-19 13:14:32 PST
Comment on attachment 444800 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteCommandEncoder.cpp:72
>> +    auto computePassEncoder = m_backing->beginComputePass(*convertedDescriptor);
> 
> What happens if convertedDescriptor still doesn't have a value at this point?

RemoteComputePassEncoder::create takes an optional. If the input was nullopt, we should be passing nullopt to the consumer, but if the input wasn't nullopt, we should verify that it could be converted successfully.

>> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteCommandEncoder.cpp:88
>> +    m_backing->copyBufferToBuffer(*convertedSource, sourceOffset, *convertedDestination, destinationOffset, size);
> 
> Should this (and the other similar code) actually test that the converted buffers exist rather than just ASSERT?

oh, you mean only call the backing function if the lookup was successful? I'll update the patch to do that.
Comment 4 Myles C. Maxfield 2021-11-19 14:56:22 PST
Created attachment 444858 [details]
Patch for committing
Comment 5 Myles C. Maxfield 2021-11-19 15:07:19 PST
Comment on attachment 444800 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteBuffer.cpp:52
>> +    // FIXME: Implement this.
> 
> ASSERT_NOT_IMPLEMENTED()?

It looks like doing this forces adding [[ noreturn ]] to the function signature in the header, which I think is pollution. I'll just implement this ASAP.
Comment 6 Myles C. Maxfield 2021-11-19 15:13:32 PST
Created attachment 444862 [details]
Patch for committing
Comment 7 Myles C. Maxfield 2021-11-19 16:31:35 PST
Created attachment 444867 [details]
Patch for committing
Comment 8 EWS 2021-11-19 18:33:50 PST
Committed r286087 (244474@main): <https://commits.webkit.org/244474@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444867 [details].
Comment 9 Radar WebKit Bug Importer 2021-11-19 18:34:23 PST
<rdar://problem/85627098>