Bug 195224

Summary: [Web GPU] Blitting function prototypes
Product: WebKit Reporter: Justin Fan <justin_fan>
Component: New BugsAssignee: Justin Fan <justin_fan>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dbates, dino, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Justin Fan 2019-03-01 13:07:38 PST
[Web GPU] Web GPU copyBufferToTexture prototype
Comment 1 Justin Fan 2019-03-01 17:10:56 PST
Created attachment 363391 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2019-03-02 16:02:17 PST
<rdar://problem/48538902>
Comment 3 Daniel Bates 2019-03-02 16:10:29 PST
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<>
Comment 4 Justin Fan 2019-03-04 12:48:17 PST
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.
Comment 5 Justin Fan 2019-03-04 13:19:19 PST
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.
Comment 6 Justin Fan 2019-03-04 13:40:53 PST
Created attachment 363550 [details]
Patch
Comment 7 Dean Jackson 2019-03-04 15:40:09 PST
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.
Comment 8 Justin Fan 2019-03-04 16:21:39 PST
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.
Comment 9 Justin Fan 2019-03-04 16:28:10 PST
Created attachment 363567 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2019-03-04 16:55:22 PST
Comment on attachment 363567 [details]
Patch for landing

Clearing flags on attachment: 363567

Committed r242404: <https://trac.webkit.org/changeset/242404>
Comment 11 WebKit Commit Bot 2019-03-04 16:55:23 PST
All reviewed patches have been landed.  Closing bug.