[webkit-reviews] review granted: [Bug 193926] [WebGPU] Fix and add validation to WebGPURenderPipeline and MTLVertexDescriptor : [Attachment 360380] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 28 19:03:21 PST 2019


Myles C. Maxfield <mmaxfield at apple.com> has granted Justin Fan
<justin_fan at apple.com>'s request for review:
Bug 193926: [WebGPU] Fix and add validation to WebGPURenderPipeline and
MTLVertexDescriptor
https://bugs.webkit.org/show_bug.cgi?id=193926

Attachment 360380: Patch

https://bugs.webkit.org/attachment.cgi?id=360380&action=review




--- Comment #6 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 360380
  --> https://bugs.webkit.org/attachment.cgi?id=360380
Patch

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

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:123
> +    auto attributeArray = retainPtr(mtlVertexDescriptor.get().attributes);

Are you sure our style guide permits use of Objective-C dot syntax?

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:128
> +	   if (location > 30) {

I think this limit needs to be 16, not 30. It is/will be in the spec.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:133
> +	   if (attributes[i].inputSlot > 30) {

Ditto.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:145
> +	   mtlAttributeDesc.get().offset = attributes[i].offset; // FIXME:
Setting this to an invalidly large value causes pipelineState creation to
SIGABT.

This offset, plus the byte size of the data type, need to be less than or equal
to the buffer's stride.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:156
> +	   if (inputs[j].inputSlot > 30) {

Ditto about 16

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:205
> +    populateMtlDescriptorSuccess =
setFunctionsForPipelineDescriptor(functionName, mtlDescriptor.get(),
descriptor)
> +	   && setInputStateForPipelineDescriptor(functionName,
mtlDescriptor.get(), descriptor);

I would separate these out into separate statements.

> Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:219
> +    NSError *error = [NSError errorWithDomain:@"com.apple.WebKit.GPU" code:1
userInfo:nil];

wat


More information about the webkit-reviews mailing list