Bug 195347 – [Web GPU] Enable drawing GPUTextures and GPUTextureViews in the render pipeline, and related GPUBindGroup updates

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

[Justin Fan]

[Web GPU] GPUTexture and GPUTextureView updates, and related GPUBindGroup updates

[Justin Fan]

Created attachment 363717 [details]
Patch

[Justin Fan]

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.

[EWS Watchlist]

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.

[Justin Fan]

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.

[Dean Jackson]

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.

[Justin Fan]

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.

[Radar WebKit Bug Importer]

<rdar://problem/48654801>

[Justin Fan]

Created attachment 363800 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 363800 [details]
Patch for landing

Clearing flags on attachment: 363800

Committed r242575: <https://trac.webkit.org/changeset/242575>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.