[WebGPU] Map WebGPUBindGroupLayoutBindings from the BindGroupLayoutDe…

…scriptor for error checking and later referencing https://bugs.webkit.org/show_bug.cgi?id=193405 Reviewed by Dean Jackson. Source/WebCore: When creating a WebGPUBindGroupLayout, cache the WebGPUBindGroupLayoutDescriptor's list of BindGroupLayoutBindings in a HashMap, keyed by binding number, for quick reference during the WebGPUProgrammablePassEncoder::setBindGroups implementation to follow. Also add error-checking e.g. detecting duplicate binding numbers in the same WebGPUBindGroupLayout and non-existent binding numbers when creating the WebGPUBindGroup. No new tests. BindGroups and BindGroupLayouts reflect the (canonical?) strategy of returning empty objects upon creation failure and reporting errors elswhere. Since error reporting is not yet implemented, the error checks aren't testable from LayoutTests right now. Expected behavior unchanged and covered by existing tests. * Modules/webgpu/WebGPUDevice.cpp: (WebCore::WebGPUDevice::createBindGroup const): Number of bindings must be consistent between bindings and layout bindings. BindGroupBindings should only refer to existing BindGroupLayoutBindings. * platform/graphics/gpu/GPUBindGroup.h: * platform/graphics/gpu/GPUBindGroupLayout.h: (WebCore::GPUBindGroupLayout::bindingsMap const): Added. Cache map of BindGroupLayoutBindings. * platform/graphics/gpu/cocoa/GPUBindGroupLayoutMetal.mm: Disallow duplicate binding numbers in BindGroupLayoutBindings. (WebCore::GPUBindGroupLayout::tryCreate): (WebCore::GPUBindGroupLayout::GPUBindGroupLayout): LayoutTests: Small fixes that do not alter behavior. * webgpu/bind-groups.html: * webgpu/pipeline-layouts.html: Canonical link: https://commits.webkit.org/207919@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239944 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Jan 14, 2019 · b951c7e · b951c7e
1 parent 3fdde71
commit b951c7e
Show file tree

Hide file tree

Showing 8 changed files with 71 additions and 10 deletions.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,15 @@
+2019-01-14  Justin Fan  <justin_fan@apple.com>
+
+        [WebGPU] Map WebGPUBindGroupLayoutBindings from the BindGroupLayoutDescriptor for error checking and later referencing
+        https://bugs.webkit.org/show_bug.cgi?id=193405
+
+        Reviewed by Dean Jackson.
+
+        Small fixes that do not alter behavior.
+
+        * webgpu/bind-groups.html:
+        * webgpu/pipeline-layouts.html:
+
 2019-01-14  Zalan Bujtas  <zalan@apple.com>
 
         [LFC][BFC] Add basic box-sizing support.

diff --git a/LayoutTests/webgpu/bind-groups.html b/LayoutTests/webgpu/bind-groups.html
@@ -17,7 +17,7 @@
         type: "storageBuffer"
     };
 
-    const bindGroupLayout = device.createBindGroupLayout({ bindings: [bufferLayoutBinding]});
+    const bindGroupLayout = device.createBindGroupLayout({ bindings: [bufferLayoutBinding] });
 
     const buffer = device.createBuffer({ size: 16, usage: WebGPUBufferUsage.STORAGE });
     const bufferBinding = { buffer: buffer, offset: 0, size: 16 };

diff --git a/LayoutTests/webgpu/pipeline-layouts.html b/LayoutTests/webgpu/pipeline-layouts.html
@@ -22,7 +22,7 @@
 }, "Create a basic WebGPUBindGroupLayoutDescriptor."); 
 
 promise_test(async () => {
-    const device = await window.getBasicDevice();
+    const device = await getBasicDevice();
     const bindGroupLayout = device.createBindGroupLayout({ bindings: [createBindGroupLayoutBinding()] });
     assert_true(bindGroupLayout instanceof WebGPUBindGroupLayout, "createBindGroupLayout returned a WebGPUBindGroupLayout");
 

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,30 @@
+2019-01-14  Justin Fan  <justin_fan@apple.com>
+
+        [WebGPU] Map WebGPUBindGroupLayoutBindings from the BindGroupLayoutDescriptor for error checking and later referencing
+        https://bugs.webkit.org/show_bug.cgi?id=193405
+
+        Reviewed by Dean Jackson.
+
+        When creating a WebGPUBindGroupLayout, cache the WebGPUBindGroupLayoutDescriptor's list of BindGroupLayoutBindings
+        in a HashMap, keyed by binding number, for quick reference during the WebGPUProgrammablePassEncoder::setBindGroups 
+        implementation to follow. Also add error-checking e.g. detecting duplicate binding numbers in the same WebGPUBindGroupLayout
+        and non-existent binding numbers when creating the WebGPUBindGroup.
+
+        No new tests. BindGroups and BindGroupLayouts reflect the (canonical?) strategy of returning empty 
+        objects upon creation failure and reporting errors elswhere. Since error reporting is not yet implemented, 
+        the error checks aren't testable from LayoutTests right now. Expected behavior unchanged and covered by existing tests.
+
+        * Modules/webgpu/WebGPUDevice.cpp:
+        (WebCore::WebGPUDevice::createBindGroup const): 
+                Number of bindings must be consistent between bindings and layout bindings.
+                BindGroupBindings should only refer to existing BindGroupLayoutBindings.
+        * platform/graphics/gpu/GPUBindGroup.h: 
+        * platform/graphics/gpu/GPUBindGroupLayout.h:
+        (WebCore::GPUBindGroupLayout::bindingsMap const): Added. Cache map of BindGroupLayoutBindings.
+        * platform/graphics/gpu/cocoa/GPUBindGroupLayoutMetal.mm: Disallow duplicate binding numbers in BindGroupLayoutBindings.
+        (WebCore::GPUBindGroupLayout::tryCreate):
+        (WebCore::GPUBindGroupLayout::GPUBindGroupLayout):
+
 2019-01-14  Myles C. Maxfield  <mmaxfield@apple.com>
 
         [WHLSL] Assorted cleanup

diff --git a/Source/WebCore/Modules/webgpu/WebGPUDevice.cpp b/Source/WebCore/Modules/webgpu/WebGPUDevice.cpp
@@ -97,7 +97,12 @@ Ref<WebGPUPipelineLayout> WebGPUDevice::createPipelineLayout(WebGPUPipelineLayou
 Ref<WebGPUBindGroup> WebGPUDevice::createBindGroup(WebGPUBindGroupDescriptor&& descriptor) const
 {
     if (!descriptor.layout || !descriptor.layout->bindGroupLayout()) {
-        LOG(WebGPU, "WebGPUDevice::createBindGroup: Invalid WebGPUBindGroupLayout!");
+        LOG(WebGPU, "WebGPUDevice::createBindGroup(): Invalid WebGPUBindGroupLayout!");
+        return WebGPUBindGroup::create(nullptr);
+    }
+
+    if (descriptor.bindings.size() != descriptor.layout->bindGroupLayout()->bindingsMap().size()) {
+        LOG(WebGPU, "WebGPUDevice::createBindGroup(): Mismatched number of WebGPUBindGroupLayoutBindings and WebGPUBindGroupBindings!");
         return WebGPUBindGroup::create(nullptr);
     }
 
@@ -115,11 +120,16 @@ Ref<WebGPUBindGroup> WebGPUDevice::createBindGroup(WebGPUBindGroupDescriptor&& d
     bindGroupBindings.reserveCapacity(descriptor.bindings.size());
 
     for (const auto& binding : descriptor.bindings) {
+        if (!descriptor.layout->bindGroupLayout()->bindingsMap().contains(binding.binding)) {
+            LOG(WebGPU, "WebGPUDevice::createBindGroup(): WebGPUBindGroupBinding %lu not found in WebGPUBindGroupLayout!", binding.binding);
+            return WebGPUBindGroup::create(nullptr);
+        }
+
         auto bindingResource = WTF::visit(bindingResourceVisitor, binding.resource);
         if (bindingResource)
             bindGroupBindings.uncheckedAppend(GPUBindGroupBinding { binding.binding, WTFMove(bindingResource.value()) });
         else {
-            LOG(WebGPU, "WebGPUDevice::createBindGroup: Invalid WebGPUBindingResource in WebGPUBindGroupBindings!");
+            LOG(WebGPU, "WebGPUDevice::createBindGroup(): Invalid WebGPUBindingResource for binding %lu in WebGPUBindGroupBindings!", binding.binding);
             return WebGPUBindGroup::create(nullptr);
         }
     }

diff --git a/Source/WebCore/platform/graphics/gpu/GPUBindGroup.h b/Source/WebCore/platform/graphics/gpu/GPUBindGroup.h
@@ -42,7 +42,7 @@ class GPUBindGroup : public RefCounted<GPUBindGroup> {
     static Ref<GPUBindGroup> create(GPUBindGroupDescriptor&&);
 
 private:
-    GPUBindGroup(GPUBindGroupDescriptor&&);
+    explicit GPUBindGroup(GPUBindGroupDescriptor&&);
 
     Ref<GPUBindGroupLayout> m_layout;
     Vector<GPUBindGroupBinding> m_bindings;

diff --git a/Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h b/Source/WebCore/platform/graphics/gpu/GPUBindGroupLayout.h
@@ -29,6 +29,7 @@
 
 #include "GPUBindGroupLayoutDescriptor.h"
 
+#include <wtf/HashMap.h>
 #include <wtf/RefCounted.h>
 #include <wtf/RefPtr.h>
 #include <wtf/RetainPtr.h>
@@ -49,9 +50,13 @@ class GPUBindGroupLayout : public RefCounted<GPUBindGroupLayout> {
 
     static RefPtr<GPUBindGroupLayout> tryCreate(const GPUDevice&, GPUBindGroupLayoutDescriptor&&);
 
+    using BindingsMapType = HashMap<unsigned long long, GPUBindGroupLayoutBinding, WTF::IntHash<unsigned long long>, WTF::UnsignedWithZeroKeyHashTraits<unsigned long long>>;
+    const BindingsMapType& bindingsMap() const { return m_bindingsMap; }
+
 private:
-    GPUBindGroupLayout(ArgumentEncoders&&);
+    GPUBindGroupLayout(BindingsMapType&&, ArgumentEncoders&&);
 
+    const BindingsMapType m_bindingsMap;
     ArgumentEncoders m_argumentEncoders;
 };
 

diff --git a/Source/WebCore/platform/graphics/gpu/cocoa/GPUBindGroupLayoutMetal.mm b/Source/WebCore/platform/graphics/gpu/cocoa/GPUBindGroupLayoutMetal.mm
@@ -76,16 +76,22 @@ static void appendArgumentToArray(ArgumentArray& array, RetainPtr<MTLArgumentDes
 RefPtr<GPUBindGroupLayout> GPUBindGroupLayout::tryCreate(const GPUDevice& device, GPUBindGroupLayoutDescriptor&& descriptor)
 {
     ArgumentArray vertexArguments, fragmentArguments, computeArguments;
+    BindingsMapType bindingsMap;
 
     for (const auto& binding : descriptor.bindings) {
+        if (!bindingsMap.add(binding.binding, binding)) {
+            LOG(WebGPU, "GPUBindGroupLayout::tryCreate(): Duplicate binding %lu found in WebGPUBindGroupLayoutDescriptor!", binding.binding);
+            return nullptr;
+        }
+
         RetainPtr<MTLArgumentDescriptor> mtlArgument;
 
         BEGIN_BLOCK_OBJC_EXCEPTIONS;
         mtlArgument = adoptNS([MTLArgumentDescriptor new]);
         END_BLOCK_OBJC_EXCEPTIONS;
 
         if (!mtlArgument) {
-            LOG(WebGPU, "GPUBindGroupLayout::tryCreate(): Unable to create MTLArgumentDescriptor!");
+            LOG(WebGPU, "GPUBindGroupLayout::tryCreate(): Unable to create MTLArgumentDescriptor for binding %lu!", binding.binding);
             return nullptr;
         }
 
@@ -115,11 +121,12 @@ static void appendArgumentToArray(ArgumentArray& array, RetainPtr<MTLArgumentDes
             return nullptr;
     }
 
-    return adoptRef(new GPUBindGroupLayout(WTFMove(encoders)));
+    return adoptRef(new GPUBindGroupLayout(WTFMove(bindingsMap), WTFMove(encoders)));
 }
 
-GPUBindGroupLayout::GPUBindGroupLayout(ArgumentEncoders&& encoders)
-    : m_argumentEncoders(WTFMove(encoders))
+GPUBindGroupLayout::GPUBindGroupLayout(BindingsMapType&& bindingsMap, ArgumentEncoders&& encoders)
+    : m_bindingsMap(WTFMove(bindingsMap))
+    , m_argumentEncoders(WTFMove(encoders))
 {
 }