Summary: | [WebGPU] Implement GPU process's message receivers | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||
Component: | New Bugs | Assignee: | 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
Myles C. Maxfield
2021-11-19 04:26:00 PST
Created attachment 444800 [details]
Patch
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 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. Created attachment 444858 [details]
Patch for committing
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. Created attachment 444862 [details]
Patch for committing
Created attachment 444867 [details]
Patch for committing
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]. |