[WebGPU] Fix and add validation to WebGPURenderPipeline and MTLVertexDescriptor
authorjustin_fan@apple.com <justin_fan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Jan 2019 20:50:17 +0000 (20:50 +0000)
committerjustin_fan@apple.com <justin_fan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Jan 2019 20:50:17 +0000 (20:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=193926
<rdar://problem/47327648>

Reviewed by Myles C. Maxfield.

Source/WebCore:

Update vertex input to properly utilize inputSlot and shaderLocation fields, and add some validation.

Test: webgpu/vertex-buffer-triangle-strip.html

* Modules/webgpu/WebGPUVertexInputDescriptor.idl:
* platform/graphics/gpu/GPUVertexInputDescriptor.h:
* platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:
(WebCore::setInputStateForPipelineDescriptor): Properly retain Metal types.
(WebCore::GPURenderPipeline::create): Provide error logging for MTLRenderPipelineState creation.

LayoutTests:

Updated test for new vertex input logic. Now provides color as a vertex attribute.

* webgpu/vertex-buffer-triangle-strip.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240672 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/webgpu/vertex-buffer-triangle-strip.html
Source/WebCore/ChangeLog
Source/WebCore/Modules/webgpu/WebGPUVertexInputDescriptor.idl
Source/WebCore/platform/graphics/gpu/GPUVertexInputDescriptor.h
Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm

index d6aff6d..949fb4a 100644 (file)
@@ -1,3 +1,15 @@
+2019-01-29  Justin Fan  <justin_fan@apple.com>
+
+        [WebGPU] Fix and add validation to WebGPURenderPipeline and MTLVertexDescriptor
+        https://bugs.webkit.org/show_bug.cgi?id=193926
+        <rdar://problem/47327648>
+
+        Reviewed by Myles C. Maxfield.
+
+        Updated test for new vertex input logic. Now provides color as a vertex attribute.
+
+        * webgpu/vertex-buffer-triangle-strip.html:
+
 2019-01-29  Devin Rousso  <drousso@apple.com>
 
         Web Inspector: provide a way to edit page WebRTC settings on a remote target
index 611fee4..667a0a5 100644 (file)
@@ -15,52 +15,46 @@ using namespace metal;
 struct VertexIn
 {
     float4 position [[attribute(0)]];
+    float green [[attribute(1)]];
 };
 
 struct VertexOut
 {
     float4 position [[position]];
+    float4 color;
 };
 
 vertex VertexOut vertex_main(VertexIn vertexIn [[stage_in]])
 {
     VertexOut vOut;
     vOut.position = vertexIn.position;
+    vOut.color = float4(0, vertexIn.green, 0, 1);
     return vOut;
 }
 
-fragment float4 fragment_main()
+fragment float4 fragment_main(VertexOut v [[stage_in]])
 {
-    return float4(0.0, 1.0, 0.0, 1.0);
+    return v.color;
 }
 `
 
 function createVertexBuffer(device) {
-    const bufferSize = 4 * 4 * 4;
+    const bufferSize = 4 * 5 * 4;
     const buffer = device.createBuffer({ size: bufferSize, usage: WebGPUBufferUsage.MAP_WRITE });
     
-    let arrayBuffer = buffer.mapping;
-    let floatArray = new Float32Array(arrayBuffer);
+    let floatArray = new Float32Array(buffer.mapping);
 
-    floatArray[0] = -1;
-    floatArray[1] = 1;
-    floatArray[2] = 0;
-    floatArray[3] = 1;
+    const vertices = [
+        // float4 xyzw, float g
+        -1, 1, 0, 1, 1,
+        -1, -1, 0, 1, 1,
+        1, 1, 0, 1, 1,
+        1, -1, 0, 1, 1
+    ];
 
-    floatArray[4] = -1;
-    floatArray[5] = -1;
-    floatArray[6] = 0;
-    floatArray[7] = 1;
-
-    floatArray[8] = 1;
-    floatArray[9] = 1;
-    floatArray[10] = 0;
-    floatArray[11] = 1;
-
-    floatArray[12] = 1;
-    floatArray[13] = -1;
-    floatArray[14] = 0;
-    floatArray[15] = 1;
+    for (let i = 0; i < vertices.length; ++i) {
+        floatArray[i] = vertices[i];
+    }
 
     return buffer;
 }
@@ -73,9 +67,15 @@ function createInputStateDescriptor() {
             inputSlot: 0,
             offset: 0,
             format: WebGPUVertexFormat.FLOAT_R32_G32_B32_A32
+        }, {
+            shaderLocation: 1,
+            inputSlot: 0,
+            offset: 4 * 4,
+            format: WebGPUVertexFormat.FLOAT_R32
         }],
         inputs: [{
-            stride: 4 * 4,
+            inputSlot: 0,
+            stride: 4 * 5,
             stepMode: WebGPUInputStepMode.VERTEX
         }]
     }
index 90e64bc..f4cd4df 100644 (file)
@@ -1,3 +1,21 @@
+2019-01-29  Justin Fan  <justin_fan@apple.com>
+
+        [WebGPU] Fix and add validation to WebGPURenderPipeline and MTLVertexDescriptor
+        https://bugs.webkit.org/show_bug.cgi?id=193926
+        <rdar://problem/47327648>
+
+        Reviewed by Myles C. Maxfield.
+
+        Update vertex input to properly utilize inputSlot and shaderLocation fields, and add some validation.
+
+        Test: webgpu/vertex-buffer-triangle-strip.html
+
+        * Modules/webgpu/WebGPUVertexInputDescriptor.idl: 
+        * platform/graphics/gpu/GPUVertexInputDescriptor.h:
+        * platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm:
+        (WebCore::setInputStateForPipelineDescriptor): Properly retain Metal types.
+        (WebCore::GPURenderPipeline::create): Provide error logging for MTLRenderPipelineState creation.
+
 2019-01-29  Keith Rollin  <krollin@apple.com>
 
         Add .xcfilelists to Run Script build phases
index 307e315..872f353 100644 (file)
@@ -31,6 +31,7 @@ typedef u32 WebGPUInputStepModeEnum;
     Conditional=WEBGPU,
     EnabledAtRuntime=WebGPU
 ] dictionary WebGPUVertexInputDescriptor {
+    u32 inputSlot;
     u32 stride;
     WebGPUInputStepModeEnum stepMode;
 };
index 0619b22..c6ffecd 100644 (file)
@@ -42,6 +42,7 @@ public:
 };
 
 struct GPUVertexInputDescriptor {
+    unsigned long inputSlot;
     unsigned long stride;
     GPUInputStepModeEnum stepMode;
 };
index 8232b96..b4e8760 100644 (file)
@@ -118,42 +118,56 @@ static bool setInputStateForPipelineDescriptor(const char* const functionName, M
 #endif
     auto mtlVertexDescriptor = adoptNS([MTLVertexDescriptor new]);
 
-    // Populate vertex attributes, if any.
     const auto& attributes = descriptor.inputState.attributes;
 
-    // FIXME: What kind of validation is needed here?
-    MTLVertexAttributeDescriptorArray *attributeArray = mtlVertexDescriptor.get().attributes;
+    auto attributeArray = retainPtr(mtlVertexDescriptor.get().attributes);
 
     for (size_t i = 0; i < attributes.size(); ++i) {
+        auto location = attributes[i].shaderLocation;
+        // Maximum number of vertex attributes to be supported by Web GPU.
+        if (location > 16) {
+            LOG(WebGPU, "%s: Invalid shaderLocation %lu for vertex attribute!", functionName, location);
+            return false;
+        }
+        // Maximum number of vertex buffers supported.
+        if (attributes[i].inputSlot > 16) {
+            LOG(WebGPU, "%s: Invalid inputSlot %lu for vertex attribute %lu!", functionName, attributes[i].inputSlot, location);
+            return false;
+        }
         auto mtlFormat = validateAndConvertVertexFormatToMTLVertexFormat(attributes[i].format);
         if (!mtlFormat) {
-            LOG(WebGPU, "%s: Invalid WebGPUVertexFormatEnum for vertex attribute!", functionName);
+            LOG(WebGPU, "%s: Invalid WebGPUVertexFormatEnum for vertex attribute %lu!", functionName, location);
             return false;
         }
 
-        MTLVertexAttributeDescriptor *mtlAttributeDesc = [attributeArray objectAtIndexedSubscript:i];
-        mtlAttributeDesc.format = *mtlFormat;
-        mtlAttributeDesc.offset = attributes[i].offset;
-        mtlAttributeDesc.bufferIndex = attributes[i].inputSlot;
-        [mtlVertexDescriptor.get().attributes setObject:mtlAttributeDesc atIndexedSubscript:i];
+        auto mtlAttributeDesc = retainPtr([attributeArray objectAtIndexedSubscript:location]);
+        mtlAttributeDesc.get().format = *mtlFormat;
+        mtlAttributeDesc.get().offset = attributes[i].offset; // FIXME: After adding more vertex formats, ensure offset < buffer's stride + format's data size.
+        mtlAttributeDesc.get().bufferIndex = attributes[i].inputSlot;
+        [mtlVertexDescriptor.get().attributes setObject:mtlAttributeDesc.get() atIndexedSubscript:location];
     }
 
-    // Populate vertex buffer layouts, if any.
     const auto& inputs = descriptor.inputState.inputs;
 
-    MTLVertexBufferLayoutDescriptorArray *layoutArray = mtlVertexDescriptor.get().layouts;
+    auto layoutArray = retainPtr(mtlVertexDescriptor.get().layouts);
 
     for (size_t j = 0; j < inputs.size(); ++j) {
+        auto slot = inputs[j].inputSlot;
+        if (inputs[j].inputSlot > 16) {
+            LOG(WebGPU, "%s: Invalid inputSlot %d for vertex buffer!", functionName, slot);
+            return false;
+        }
+
         auto mtlStepFunction = validateAndConvertStepModeToMTLStepFunction(inputs[j].stepMode);
         if (!mtlStepFunction) {
-            LOG(WebGPU, "%s: Invalid WebGPUInputStepMode for vertex input!", functionName);
+            LOG(WebGPU, "%s: Invalid WebGPUInputStepMode for vertex buffer at slot %lu!", functionName, slot);
             return false;
         }
 
-        MTLVertexBufferLayoutDescriptor *mtlLayoutDesc = [layoutArray objectAtIndexedSubscript:j];
-        mtlLayoutDesc.stepFunction = *mtlStepFunction;
-        mtlLayoutDesc.stride = inputs[j].stride;
-        [mtlVertexDescriptor.get().layouts setObject:mtlLayoutDesc atIndexedSubscript:j];
+        auto mtlLayoutDesc = retainPtr([layoutArray objectAtIndexedSubscript:slot]);
+        mtlLayoutDesc.get().stepFunction = *mtlStepFunction;
+        mtlLayoutDesc.get().stride = inputs[j].stride;
+        [mtlVertexDescriptor.get().layouts setObject:mtlLayoutDesc.get() atIndexedSubscript:slot];
     }
 
     mtlDescriptor.vertexDescriptor = mtlVertexDescriptor.get();
@@ -183,8 +197,16 @@ RefPtr<GPURenderPipeline> GPURenderPipeline::create(const GPUDevice& device, GPU
         return nullptr;
     }
 
-    if (!setFunctionsForPipelineDescriptor(functionName, mtlDescriptor.get(), descriptor)
-        || !setInputStateForPipelineDescriptor(functionName, mtlDescriptor.get(), descriptor))
+    bool didSetFunctions = false, didSetInputState = false;
+
+    BEGIN_BLOCK_OBJC_EXCEPTIONS;
+
+    didSetFunctions = setFunctionsForPipelineDescriptor(functionName, mtlDescriptor.get(), descriptor);
+    didSetInputState = setInputStateForPipelineDescriptor(functionName, mtlDescriptor.get(), descriptor);
+
+    END_BLOCK_OBJC_EXCEPTIONS;
+
+    if (!didSetFunctions || !didSetInputState)
         return nullptr;
 
     // FIXME: Get the pixelFormat as configured for the context/CAMetalLayer.
@@ -194,14 +216,15 @@ RefPtr<GPURenderPipeline> GPURenderPipeline::create(const GPUDevice& device, GPU
 
     BEGIN_BLOCK_OBJC_EXCEPTIONS;
 
-    pipeline = adoptNS([device.platformDevice() newRenderPipelineStateWithDescriptor:mtlDescriptor.get() error:nil]);
+    NSError *error = [NSError errorWithDomain:@"com.apple.WebKit.GPU" code:1 userInfo:nil];
+    pipeline = adoptNS([device.platformDevice() newRenderPipelineStateWithDescriptor:mtlDescriptor.get() error:&error]);
+    if (!pipeline)
+        LOG(WebGPU, "%s: %s!", functionName, error.localizedDescription.UTF8String);
 
     END_BLOCK_OBJC_EXCEPTIONS;
 
-    if (!pipeline) {
-        LOG(WebGPU, "%s: Error creating MTLRenderPipelineState!", functionName);
+    if (!pipeline)
         return nullptr;
-    }
 
     return adoptRef(new GPURenderPipeline(WTFMove(pipeline), WTFMove(descriptor)));
 }