[Web GPU] GPUTexture and GPUTextureView updates, and related GPUBindGroup updates
Created attachment 363717 [details] Patch
Comment on attachment 363717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363717&action=review > LayoutTests/webgpu/texture-triangle-strip.html:45 > + constexpr sampler s(coord::normalized, address::clamp_to_zero, filter::nearest); This is temporary until GPUSamplers are added in the next big patch.
Attachment 363717 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:64: The parameter name "const" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:65: The parameter name "const" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 56 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 363717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363717&action=review > Source/WebCore/Modules/webgpu/WebGPUTexture.cpp:45 > +WebGPUTexture::~WebGPUTexture() I'm no longer calling destroy() in the destructor here as it would expose when garbage collection happens.
Comment on attachment 363717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363717&action=review > Source/WebCore/ChangeLog:8 > + Reviewed by NOBODY (OOPS!). > + > + Test: webgpu/texture-triangle-strip.html It would be nice if you could provide a description of what this patch actually does. > Source/WebCore/Modules/webgpu/GPUBindGroupLayoutBinding.h:31 > +#include <wtf/OptionSet.h> I don't think you need this. > Source/WebCore/Modules/webgpu/GPUBindGroupLayoutBinding.h:46 > + GPUShaderStageFlags visibility; Shouldn't this be GPUShaderStageBit::Flags? > Source/WebCore/Modules/webgpu/WebGPURenderPassDescriptor.cpp:61 > + if (!colorAttachment.attachment > + || !colorAttachment.attachment->texture() > + || !colorAttachment.attachment->texture()->isOutputAttachment()) { This could all go on one line. > Source/WebCore/Modules/webgpu/WebGPUTexture.cpp:68 > + if (!m_texture) > + LOG(WebGPU, "GPUTexture::destroy(): Invalid operation!"); > + else > + destroyImpl(); Not sure this will compile in Release configuration. > Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:80 > + endBlitEncoding(); I would prefer to put the conditional in here, and an ASSERT in the endBlitEncoding. if (m_blitEncoder) endBlitEncoding(); > Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:91 > + if (!m_blitEncoder) > + return; And here would just be ASSERT(m_blitEncoder), unless you call it from other places.
Comment on attachment 363717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363717&action=review >> Source/WebCore/Modules/webgpu/GPUBindGroupLayoutBinding.h:46 >> + GPUShaderStageFlags visibility; > > Shouldn't this be GPUShaderStageBit::Flags? The IDL calls for an unsigned int and bindings doesn't know how to convert, even though it's not a strongly-typed enum. >> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:91 >> + return; > > And here would just be ASSERT(m_blitEncoder), unless you call it from other places. Correct, this function is also called by GPUQueue::submit(), which doesn't need to know whether the command buffer actually has an active blit encoder or not.
<rdar://problem/48654801>
Created attachment 363800 [details] Patch for landing
Comment on attachment 363800 [details] Patch for landing Clearing flags on attachment: 363800 Committed r242575: <https://trac.webkit.org/changeset/242575>
All reviewed patches have been landed. Closing bug.