Bug 195224 – [Web GPU] Blitting function prototypes

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

[Justin Fan]

[Web GPU] Web GPU copyBufferToTexture prototype

[Justin Fan]

Created attachment 363391 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/48538902>

[Daniel Bates]

Comment on attachment 363391 [details]
Patch

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

This patch contains a lot of logging. Is it all necessary?

> Source/WebCore/platform/graphics/gpu/GPUTexture.h:63
> +    GPUTextureUsage::Flags m_usage;

OptionSet<>?

> Source/WebCore/platform/graphics/gpu/GPUTextureUsage.h:40
> +        TransferSrc = 1,

1 << 0

Name bothers me, but not by much. I like spelling things out so “TransferSource”.

> Source/WebCore/platform/graphics/gpu/GPUTextureUsage.h:41
> +        TransferDst = 2,

1 << 1

Name bothers me, but not by much. I like spelling things out so “TransferDestination”.

> Source/WebCore/platform/graphics/gpu/GPUTextureUsage.h:42
> +        Sampled = 4,

see where I’m going with this? This is not the correct style for a flag enum. Greg the code.

> Source/WebCore/platform/graphics/gpu/GPUTextureUsage.h:44
> +        OutputAttachment = 16

Trailing comma to make this future proof?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:78
> +MTLBlitCommandEncoder *GPUCommandBuffer::currentBlitEncoder()

* on the wrong side.

Why isn’t this function const? It should be, and make the ivar mutable, because this lazy retaining business is an implementation detail.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:80
> +    if (!m_blitEncoder)

Ok as-is. The most common case is we have the encoder right? So, I would early return if we have it. Otherwise, assign to ivar and return it. This would be slightly more efficient that what you have now.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:81
> +        m_blitEncoder = retainPtr([m_platformCommandBuffer blitCommandEncoder]);

retainPtr() is not necessary.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:82
> +

Ok as-is. I don’t like this empty line. I find it distracting and not improving the readability of this code. I would remove it.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:98
> +#endif // PLATFORM(MAC)

Ok as-is. I don’t like the inline comment. Who’s window/monitor is so small that they can’t keep 6 lines on screen? Or are we accounting for people with Alzheimer’s?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:123
> +void GPUCommandBuffer::copyBufferToTexture(GPUBufferCopyView&& srcBuffer, GPUTextureCopyView&& dstTexture, GPUExtent3D&& size)

This function is clearly incomplete. But I don’t think all these arguments need to be rvalue refs do they?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:130
> +    /* FIXME: Add Metal validation?

FIXME: Use C++ style comment for all of these lines? :D

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:162
> +void GPUCommandBuffer::copyTextureToBuffer(GPUTextureCopyView&& srcTexture, GPUBufferCopyView&& dstBuffer, GPUExtent3D&& size)

I don’t think all of these arguments need to be rvalue refs, do they?

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:189
> +void GPUCommandBuffer::copyTextureToTexture(GPUTextureCopyView&& src, GPUTextureCopyView&& dst, GPUExtent3D&& size)

Ditto.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUTextureMetal.mm:57
> +static Optional<MTLTextureUsage> mtlTextureUsageForGPUTextureUsageFlags(GPUTextureUsageFlags flags, const char* const functionName)

flags should be an OptionSet<>

[Justin Fan]

Comment on attachment 363391 [details]
Patch

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

>> Source/WebCore/platform/graphics/gpu/GPUTexture.h:63
>> +    GPUTextureUsage::Flags m_usage;
> 
> OptionSet<>?

Thanks for the review! I'd never heard of that before, I'll check it out.

>> Source/WebCore/platform/graphics/gpu/GPUTextureUsage.h:40
>> +        TransferSrc = 1,
> 
> 1 << 0
> 
> Name bothers me, but not by much. I like spelling things out so “TransferSource”.

I have to match the Web API for our bindings code to match it properly. I can tell bindings to ignore constants checking if I want to change the name, but I'd rather not.

>> Source/WebCore/platform/graphics/gpu/GPUTextureUsage.h:44
>> +        OutputAttachment = 16
> 
> Trailing comma to make this future proof?

OK!

>> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:78
>> +MTLBlitCommandEncoder *GPUCommandBuffer::currentBlitEncoder()
> 
> * on the wrong side.
> 
> Why isn’t this function const? It should be, and make the ivar mutable, because this lazy retaining business is an implementation detail.

* is separate from the Objective-C type. I didn't think 'const'ing this and making the ivar mutable was an option as I'd never used that pattern.

>> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:80
>> +    if (!m_blitEncoder)
> 
> Ok as-is. The most common case is we have the encoder right? So, I would early return if we have it. Otherwise, assign to ivar and return it. This would be slightly more efficient that what you have now.

Ok!

>> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:81
>> +        m_blitEncoder = retainPtr([m_platformCommandBuffer blitCommandEncoder]);
> 
> retainPtr() is not necessary.

Ok!

>> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:130
>> +    /* FIXME: Add Metal validation?
> 
> FIXME: Use C++ style comment for all of these lines? :D

OK

>> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:162
>> +void GPUCommandBuffer::copyTextureToBuffer(GPUTextureCopyView&& srcTexture, GPUBufferCopyView&& dstBuffer, GPUExtent3D&& size)
> 
> I don’t think all of these arguments need to be rvalue refs, do they?

You're right, they can be raw refs.

[Justin Fan]

Comment on attachment 363391 [details]
Patch

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

>>> Source/WebCore/platform/graphics/gpu/GPUTextureUsage.h:40
>>> +        TransferSrc = 1,
>> 
>> 1 << 0
>> 
>> Name bothers me, but not by much. I like spelling things out so “TransferSource”.
> 
> I have to match the Web API for our bindings code to match it properly. I can tell bindings to ignore constants checking if I want to change the name, but I'd rather not.

On second thought, since we're already ignoring the checks, I've updated the name.

[Justin Fan]

Created attachment 363550 [details]
Patch

[Dean Jackson]

Comment on attachment 363550 [details]
Patch

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

> Source/WebCore/Modules/webgpu/WebGPUCommandBuffer.cpp:49
> +    return GPUBufferCopyView(buffer->buffer().releaseNonNull(), *this);

I think we tend to use syntax like this now:

return GPUBufferCopyView { buffer->buffer().releaseNonNull(), *this };

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:120
> +void GPUCommandBuffer::copyBufferToTexture(const GPUBufferCopyView& srcBuffer, const GPUTextureCopyView& dstTexture, const GPUExtent3D& size)

It's kind of weird that we can have a const destination just because ObjC.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUCommandBufferMetal.mm:127
> +    // FIXME: Add Metal validation?

I don't think this is a question.

> Source/WebCore/platform/graphics/gpu/cocoa/GPUTextureMetal.mm:62
> +    if (flags.containsAny({ GPUTextureUsage::Flags::TransferSource, GPUTextureUsage::Flags::Sampled }) && (flags & GPUTextureUsage::Flags::Storage)) {

You should consistently use .contains everywhere if you're going OptionSet<>

> LayoutTests/webgpu/blit-commands.html:33
> +        usage: GPUBufferUsage.TRANSFER_SRC | GPUBufferUsage.TRANSFER_DST

I think bufferA is SRC only and bufferB is both.

[Justin Fan]

Comment on attachment 363550 [details]
Patch

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

>> Source/WebCore/platform/graphics/gpu/cocoa/GPUTextureMetal.mm:62
>> +    if (flags.containsAny({ GPUTextureUsage::Flags::TransferSource, GPUTextureUsage::Flags::Sampled }) && (flags & GPUTextureUsage::Flags::Storage)) {
> 
> You should consistently use .contains everywhere if you're going OptionSet<>

I use contains for the getters because the bool() operator is marked explicit, but in this case (and others around WebKit) it looks like for single-bit checks a normal & is ok.

>> LayoutTests/webgpu/blit-commands.html:33
>> +        usage: GPUBufferUsage.TRANSFER_SRC | GPUBufferUsage.TRANSFER_DST
> 
> I think bufferA is SRC only and bufferB is both.

BufferA needs to be TRANSFER_DST if I want to use setSubData on it.

[Justin Fan]

Created attachment 363567 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 363567 [details]
Patch for landing

Clearing flags on attachment: 363567

Committed r242404: <https://trac.webkit.org/changeset/242404>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.