Ensure correct shader reflection for texture_external

Adds some handling that ensures correct reflection of texture_external now
that Dawn uses Tint by default. Enables the previously written tests.

dawn: 1004
Change-Id: I8fd43292e9fe243aee037c70fe47b79ace8b672f
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/58540
Commit-Queue: Brandon Jones (Intel) <brandon1.jones@intel.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>

src/dawn_native/PipelineLayout.cpp

@@ -144,7 +144,9 @@ namespace dawn_native {
 
         // Does the trivial conversions from a ShaderBindingInfo to a BindGroupLayoutEntry
         auto ConvertMetadataToEntry =
-            [](const EntryPointMetadata::ShaderBindingInfo& shaderBinding) -> BindGroupLayoutEntry {
+            [](const EntryPointMetadata::ShaderBindingInfo& shaderBinding,
+               const ExternalTextureBindingLayout* externalTextureBindingEntry)
+            -> BindGroupLayoutEntry {
             BindGroupLayoutEntry entry = {};
             switch (shaderBinding.bindingType) {
                 case BindingInfoType::Buffer:
@@ -195,13 +197,7 @@ namespace dawn_native {
                     entry.storageTexture.viewDimension = shaderBinding.storageTexture.viewDimension;
                     break;
                 case BindingInfoType::ExternalTexture:
-                    // TODO(dawn:728) On backend configurations that use SPIRV-Cross to reflect
+                    entry.nextInChain = externalTextureBindingEntry;
-                    // shader info - the shader must have been already transformed prior to
-                    // reflecting the shader. During transformation, all instances of
-                    // texture_external are changed to texture_2d<f32>. This means that when
-                    // extracting shader info, external textures will be seen as sampled 2d
-                    // textures. In the future when Dawn no longer uses SPIRV-Cross, we should
-                    // handle external textures here.
                     break;
             }
             return entry;
@@ -230,6 +226,13 @@ namespace dawn_native {
         ityp::array<BindGroupIndex, std::map<BindingNumber, BindGroupLayoutEntry>, kMaxBindGroups>
             entryData = {};
 
+        // External texture binding layouts are chained structs that are set as a pointer within
+        // the bind group layout entry. We declare an entry here so that it can be used when needed
+        // in each BindGroupLayoutEntry and so it can stay alive until the call to
+        // GetOrCreateBindGroupLayout. Because ExternalTextureBindingLayout is an empty struct,
+        // there's no issue with using the same struct multiple times.
+        ExternalTextureBindingLayout externalTextureBindingLayout;
+
         // Loops over all the reflected BindGroupLayoutEntries from shaders.
         for (const StageAndDescriptor& stage : stages) {
             const EntryPointMetadata& metadata = stage.module->GetEntryPoint(stage.entryPoint);
@@ -240,7 +243,8 @@ namespace dawn_native {
                     const EntryPointMetadata::ShaderBindingInfo& shaderBinding = bindingIt.second;
 
                     // Create the BindGroupLayoutEntry
-                    BindGroupLayoutEntry entry = ConvertMetadataToEntry(shaderBinding);
+                    BindGroupLayoutEntry entry =
+                        ConvertMetadataToEntry(shaderBinding, &externalTextureBindingLayout);
                     entry.binding = static_cast<uint32_t>(bindingNumber);
                     entry.visibility = StageBit(stage.shaderStage);
 

src/dawn_native/ShaderModule.cpp

@@ -506,20 +506,10 @@ namespace dawn_native {
                 const BindingInfo& layoutInfo = layout->GetBindingInfo(bindingIndex);
 
                 if (layoutInfo.bindingType != shaderInfo.bindingType) {
-                    // TODO(dawn:728) On backend configurations that use SPIRV-Cross to reflect
-                    // shader info - the shader must have been already transformed prior to
-                    // reflecting the shader. During transformation, all instances of
-                    // texture_external are changed to texture_2d<f32>. This means that when
-                    // extracting shader info, external textures will be seen as sampled 2d
-                    // textures. In the future when Dawn no longer uses SPIRV-Cross, the
-                    // if-statement below should be removed.
-                    if (layoutInfo.bindingType != BindingInfoType::ExternalTexture ||
-                        shaderInfo.bindingType != BindingInfoType::Texture) {
                     return DAWN_VALIDATION_ERROR(
                         "The binding type of the bind group layout entry conflicts " +
                         GetShaderDeclarationString(group, bindingNumber));
                 }
-                }
 
                 if ((layoutInfo.visibility & StageBit(entryPoint.stage)) == 0) {
                     return DAWN_VALIDATION_ERROR("The bind group layout entry for " +
@@ -582,13 +572,11 @@ namespace dawn_native {
                     }
 
                     case BindingInfoType::ExternalTexture: {
-                        // TODO(dawn:728) On backend configurations that use SPIRV-Cross to reflect
+                        if (shaderInfo.bindingType != BindingInfoType::ExternalTexture) {
-                        // shader info - the shader must have been already transformed prior to
+                            return DAWN_VALIDATION_ERROR(
-                        // reflecting the shader. During transformation, all instances of
+                                "The external texture bind group layout entry conflicts with " +
-                        // texture_external are changed to texture_2d<f32>. This means that when
+                                GetShaderDeclarationString(group, bindingNumber));
-                        // extracting shader info, external textures will be seen as sampled 2d
+                        }
-                        // textures. In the future when Dawn no longer uses SPIRV-Cross, we should
-                        // handle external textures here.
                         break;
                     }
 

src/tests/end2end/ExternalTextureTests.cpp

@@ -62,6 +62,10 @@ TEST_P(ExternalTextureTests, CreateExternalTextureSuccess) {
 }
 
 TEST_P(ExternalTextureTests, SampleExternalTexture) {
+    // SPIRV-Cross is unable to reflect texture_external correctly, which causes errors during
+    // validation.
+    DAWN_SUPPRESS_TEST_IF(!HasToggleEnabled("use_tint_generator"));
+
     wgpu::ShaderModule vsModule = utils::CreateShaderModule(device, R"(
         [[stage(vertex)]] fn main([[builtin(vertex_index)]] VertexIndex : u32) -> [[builtin(position)]] vec4<f32> {
             var positions : array<vec4<f32>, 3> = array<vec4<f32>, 3>(

src/tests/unittests/validation/BindGroupValidationTests.cpp

@@ -2126,8 +2126,9 @@ TEST_F(BindGroupLayoutCompatibilityTest, TextureViewDimension) {
                                       wgpu::TextureViewDimension::e2D}})}));
 }
 
-// TODO(dawn:728) Enable this test when Dawn no longer relies on SPIRV-Cross to extract shader info.
+// Test that a bgl with an external texture is compatible with texture_external in a shader and that
-TEST_F(BindGroupLayoutCompatibilityTest, DISABLED_ExternalTextureBindGroupLayoutCompatibility) {
+// an error is returned when the binding in the shader does not match.
+TEST_F(BindGroupLayoutCompatibilityTest, ExternalTextureBindGroupLayoutCompatibility) {
     wgpu::BindGroupLayout bgl = utils::MakeBindGroupLayout(
         device, {{0, wgpu::ShaderStage::Fragment, &utils::kExternalTextureBindingLayout}});
 
@@ -2135,7 +2136,7 @@ TEST_F(BindGroupLayoutCompatibilityTest, DISABLED_ExternalTextureBindGroupLayout
     CreateFSRenderPipeline(R"(
             [[group(0), binding(0)]] var myExternalTexture: texture_external;
             [[stage(fragment)]] fn main() {
-                textureDimensions(myExternalTexture);
+                ignore(myExternalTexture);
             })",
                            {bgl});
 
@@ -2143,7 +2144,7 @@ TEST_F(BindGroupLayoutCompatibilityTest, DISABLED_ExternalTextureBindGroupLayout
     ASSERT_DEVICE_ERROR(CreateFSRenderPipeline(R"(
             [[group(0), binding(0)]] var myTexture: texture_2d<f32>;
             [[stage(fragment)]] fn main() {
-            ignore(textureDimensions(myTexture));
+                ignore(myTexture);
             })",
                                                {bgl}));
 }

src/tests/unittests/validation/GetBindGroupLayoutValidationTests.cpp

@@ -390,10 +390,9 @@ TEST_F(GetBindGroupLayoutTests, BindingType) {
     }
 }
 
-// Test that an external texture binding type matches a shader using texture_external.
+// Tests that the external texture binding type matches with a texture_external declared in the
-// TODO(dawn:728) Enable this test once Dawn no longer relies on SPIRV-Cross to extract shader info.
+// shader.
-// Consider combining with the similar test above.
+TEST_F(GetBindGroupLayoutTests, ExternalTextureBindingType) {
-TEST_F(GetBindGroupLayoutTests, DISABLED_ExternalTextureBindingType) {
     // This test works assuming Dawn Native's object deduplication.
     // Getting the same pointer to equivalent bind group layouts is an implementation detail of Dawn
     // Native.
@@ -412,7 +411,7 @@ TEST_F(GetBindGroupLayoutTests, DISABLED_ExternalTextureBindingType) {
             [[group(0), binding(0)]] var myExternalTexture: texture_external;
 
             [[stage(fragment)]] fn main() {
-               textureDimensions(myExternalTexture);
+               ignore(myExternalTexture);
             })");
     EXPECT_EQ(device.CreateBindGroupLayout(&desc).Get(), pipeline.GetBindGroupLayout(0).Get());
 }