[webkit-reviews] review granted: [Bug 193457] (WIP) [WebGPU] WebGPUProgrammablePassEncoder::setBindGroup prototype : [Attachment 359510] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jan 18 11:50:19 PST 2019
Dean Jackson <dino at apple.com> has granted Justin Fan <justin_fan at apple.com>'s
request for review:
Bug 193457: (WIP) [WebGPU] WebGPUProgrammablePassEncoder::setBindGroup
prototype
https://bugs.webkit.org/show_bug.cgi?id=193457
Attachment 359510: Patch
https://bugs.webkit.org/attachment.cgi?id=359510&action=review
--- Comment #18 from Dean Jackson <dino at apple.com> ---
Comment on attachment 359510
--> https://bugs.webkit.org/attachment.cgi?id=359510
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=359510&action=review
> Source/WebCore/ChangeLog:11
> + Support for compute pipelines to follow shortly.
> + Texture and sampler resources to follow after their requisite
interfaces are implemented.
I wouldn't worry about adding these comments.
> Source/WebCore/ChangeLog:25
> + * platform/graphics/gpu/GPUBindGroupLayout.h: Added
ArgumentEncoderBuffer struct to retain ptr tot both MTLArgumentEncoders and
their argument MTLBuffers.
Typo tot
> Source/WebCore/WebCore.xcodeproj/project.pbxproj:17048
> + 1CA0C2F621EEDAD200A11860 /* AST */ = {
> + isa = PBXGroup;
> + children = (
> + 1C840B9021EC30F900D0500D /* WHLSLAddressSpace.h
*/,
> + C21BF72521CD89E200227979 /*
WHLSLArrayReferenceType.h */,
> + C21BF70921CD89CA00227979 /* WHLSLArrayType.h
*/,
> + C21BF73021CD89ED00227979 /*
WHLSLAssignmentExpression.h */,
> + C21BF70A21CD89CB00227979 /*
WHLSLBaseFunctionAttribute.h */,
> + C21BF6FA21CD89BE00227979 /* WHLSLBaseSemantic.h
*/,
> + C21BF71E21CD89DC00227979 /* WHLSLBlock.h */,
> + C21BF6F621CD89B700227979 /*
WHLSLBooleanLiteral.h */,
> + C21BF71A21CD89D800227979 /* WHLSLBreak.h */,
> + C2138A1321DDECD300F516BA /*
WHLSLBuiltInSemantic.cpp */,
> + C21BF72221CD89DF00227979 /*
WHLSLBuiltInSemantic.h */,
> + C21BF71621CD89D500227979 /*
WHLSLCallExpression.h */,
What is this change?
> Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h:38
> OBJC_PROTOCOL(MTLArgumentEncoder);
> +OBJC_PROTOCOL(MTLBuffer);
This should probably be inside #if USE(METAL) too
> Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h:55
> + bool operator!() const { return !encoder || !buffer; }
I think this would be better as an isValid() function rather than an operator.
> Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h:64
> + GPUBindGroupLayout(BindingsMapType&&, ArgumentEncoderBuffer&&,
ArgumentEncoderBuffer&&, ArgumentEncoderBuffer&&);
You should probably give these parameters names here. The order isn't obvious.
> Source/WebCore/platform/graphics/gpu/GPUProgrammablePassEncoder.h:63
> + virtual void useResource(MTLResource *, unsigned long) = 0;
> +
> + // Render command encoder methods.
> + virtual void setVertexBuffer(MTLBuffer *, unsigned long, unsigned long)
{ }
> + virtual void setFragmentBuffer(MTLBuffer *, unsigned long, unsigned
long) { }
MTLFoo are ObjC types, so should be MTLFoo* not MTLFoo *.
> Source/WebCore/platform/graphics/gpu/GPURenderPassEncoder.h:69
> + void useResource(MTLResource *, unsigned long) final;
> + void setVertexBuffer(MTLBuffer *, unsigned long, unsigned long) final;
> + void setFragmentBuffer(MTLBuffer *, unsigned long, unsigned long) final;
Ditto.
More information about the webkit-reviews
mailing list