Attachment 360380 Details for Bug 193926 – Patch

[patch] Patch

bug-193926-20190128150928.patch (text/plain), 11.43 KB, created by Justin Fan on 2019-01-28 15:09:29 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Justin Fan

Created: 2019-01-28 15:09:29 PST

Size: 11.43 KB

patch

obsolete

>Subversion Revision: 240498
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 354ef06152994edd80b746eac4ec0a2d923278ed..69e2292b9d08ba5c7518894292d88a552dd50fe6 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,21 @@
>+2019-01-28  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 NOBODY (OOPS!).
>+
>+        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-25  Brent Fulgham  <bfulgham@apple.com>
> 
>         Activate the WebResourceLoadStatisticsStore in the NetworkProcess and deactivate it in the UIProcess.
>diff --git a/Source/WebCore/Modules/webgpu/WebGPUVertexInputDescriptor.idl b/Source/WebCore/Modules/webgpu/WebGPUVertexInputDescriptor.idl
>index 307e3153049e34595d1887d68886f98684d982e4..872f3535b0f240456c8df5d94b3bd9f3d9a41b6a 100644
>--- a/Source/WebCore/Modules/webgpu/WebGPUVertexInputDescriptor.idl
>+++ b/Source/WebCore/Modules/webgpu/WebGPUVertexInputDescriptor.idl
>@@ -31,6 +31,7 @@ typedef u32 WebGPUInputStepModeEnum;
>     Conditional=WEBGPU,
>     EnabledAtRuntime=WebGPU
> ] dictionary WebGPUVertexInputDescriptor {
>+    u32 inputSlot;
>     u32 stride;
>     WebGPUInputStepModeEnum stepMode;
> };
>diff --git a/Source/WebCore/platform/graphics/gpu/GPUVertexInputDescriptor.h b/Source/WebCore/platform/graphics/gpu/GPUVertexInputDescriptor.h
>index 0619b226e83b591579c18d2520e841b39bbd0ebb..c6ffecdf6e2b98ae998ec95673fe5605e11ff881 100644
>--- a/Source/WebCore/platform/graphics/gpu/GPUVertexInputDescriptor.h
>+++ b/Source/WebCore/platform/graphics/gpu/GPUVertexInputDescriptor.h
>@@ -42,6 +42,7 @@ public:
> };
> 
> struct GPUVertexInputDescriptor {
>+    unsigned long inputSlot;
>     unsigned long stride;
>     GPUInputStepModeEnum stepMode;
> };
>diff --git a/Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm b/Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm
>index 8232b967f753e82643f5dbbed89465229f88e3f0..fa2e064ff96f5cbcc895e8a0ecbd68a158ff0ab0 100644
>--- a/Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm
>+++ b/Source/WebCore/platform/graphics/gpu/cocoa/GPURenderPipelineMetal.mm
>@@ -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 supported on all Metal platforms.
>+        if (location > 30) {
>+            LOG(WebGPU, "%s: Invalid shaderLocation %lu for vertex attribute!", functionName, location);
>+            return false;
>+        }
>+        // Maximum number of vertex buffers supported on all Metal platforms.
>+        if (attributes[i].inputSlot > 30) {
>+            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: Setting this to an invalidly large value causes pipelineState creation to SIGABT.
>+        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 > 30) {
>+            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 populateMtlDescriptorSuccess = false;
>+
>+    BEGIN_BLOCK_OBJC_EXCEPTIONS;
>+
>+    populateMtlDescriptorSuccess = setFunctionsForPipelineDescriptor(functionName, mtlDescriptor.get(), descriptor)
>+        && setInputStateForPipelineDescriptor(functionName, mtlDescriptor.get(), descriptor);
>+
>+    END_BLOCK_OBJC_EXCEPTIONS;
>+
>+    if (!populateMtlDescriptorSuccess)
>         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)));
> }
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index d3f3445e83cc0ecb383c3f7863af330d7c1d5d22..9d8002d596e3b7932e5f4c3f5d7aead5434fddda 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-01-28  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 NOBODY (OOPS!).
>+
>+        Updated test for new vertex input logic. Now provides color as a vertex attribute.
>+
>+        * webgpu/vertex-buffer-triangle-strip.html:
>+
> 2019-01-25  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         iOS: inputmode="none" disables hardware keyboard's globe key
>diff --git a/LayoutTests/webgpu/vertex-buffer-triangle-strip.html b/LayoutTests/webgpu/vertex-buffer-triangle-strip.html
>index 611fee4a215422bd871fbd019d78564ffa3b6873..667a0a5a5720bbacae1a25489205ad53feb335f3 100644
>--- a/LayoutTests/webgpu/vertex-buffer-triangle-strip.html
>+++ b/LayoutTests/webgpu/vertex-buffer-triangle-strip.html
>@@ -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
>         }]
>     }

Actions: View | Formatted Diff | Diff

Attachments on bug 193926: 360380 | 360485