Summary: | [Web GPU] Blitting function prototypes | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Fan <justin_fan> | ||||||||
Component: | New Bugs | Assignee: | 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
Justin Fan
2019-03-01 13:07:38 PST
Created attachment 363391 [details]
Patch
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 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 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. Created attachment 363550 [details]
Patch
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 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. Created attachment 363567 [details]
Patch for landing
Comment on attachment 363567 [details] Patch for landing Clearing flags on attachment: 363567 Committed r242404: <https://trac.webkit.org/changeset/242404> All reviewed patches have been landed. Closing bug. |