From 4196a546bf44e4788d7f01cd7319f33cf4ce1d49 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Fri, 16 Oct 2020 14:13:16 +0000 Subject: [PATCH] Add wgpu::TextureComponentType::DepthComparison And deprecate using ::Float in the bind group layout for "shadow textures" in the pipeline (along with a deprecation test). Adds the ability to be used with DepthComparison only to depth textures, this could potentially a breaking change if users where doing depth-comparison on float32 textures but that's not supported in WebGPU. Bug: dawn:527 Change-Id: Ib28b0443e3002e0aa2811713b9e843c2417e13e7 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/30240 Commit-Queue: Corentin Wallez Reviewed-by: Stephen White --- dawn.json | 3 +- src/dawn_native/BindGroupLayout.cpp | 4 + src/dawn_native/Format.cpp | 9 ++- src/dawn_native/Format.h | 1 + src/dawn_native/Pipeline.cpp | 4 +- src/dawn_native/Pipeline.h | 2 +- src/dawn_native/PipelineLayout.cpp | 3 +- src/dawn_native/ShaderModule.cpp | 49 ++++++++++-- src/dawn_native/ShaderModule.h | 3 +- .../d3d12/RenderPassBuilderD3D12.cpp | 3 + src/dawn_native/opengl/CommandBufferGL.cpp | 3 + src/dawn_native/vulkan/CommandBufferVk.cpp | 3 + src/dawn_native/vulkan/TextureVk.cpp | 2 + src/tests/end2end/DeprecatedAPITests.cpp | 44 +++++++++++ src/tests/end2end/DepthSamplingTests.cpp | 18 +++-- .../validation/BindGroupValidationTests.cpp | 76 ++++++++++++++++++- 16 files changed, 204 insertions(+), 23 deletions(-) diff --git a/dawn.json b/dawn.json index 6dd1304d5a..6f2221869f 100644 --- a/dawn.json +++ b/dawn.json @@ -1549,7 +1549,8 @@ "values": [ {"value": 0, "name": "float"}, {"value": 1, "name": "sint"}, - {"value": 2, "name": "uint"} + {"value": 2, "name": "uint"}, + {"value": 3, "name": "depth comparison"} ] }, "texture copy view": { diff --git a/src/dawn_native/BindGroupLayout.cpp b/src/dawn_native/BindGroupLayout.cpp index e59aa9d721..93e6777bbe 100644 --- a/src/dawn_native/BindGroupLayout.cpp +++ b/src/dawn_native/BindGroupLayout.cpp @@ -121,6 +121,10 @@ namespace dawn_native { if (viewDimension != wgpu::TextureViewDimension::e2D) { return DAWN_VALIDATION_ERROR("Multisampled binding must be 2D."); } + if (entry.textureComponentType == wgpu::TextureComponentType::DepthComparison) { + return DAWN_VALIDATION_ERROR( + "Multisampled binding must not be DepthComparison."); + } break; case wgpu::BindingType::WriteonlyStorageTexture: diff --git a/src/dawn_native/Format.cpp b/src/dawn_native/Format.cpp index 5bf672c590..90d37191ad 100644 --- a/src/dawn_native/Format.cpp +++ b/src/dawn_native/Format.cpp @@ -36,6 +36,7 @@ namespace dawn_native { case wgpu::TextureComponentType::Float: case wgpu::TextureComponentType::Sint: case wgpu::TextureComponentType::Uint: + case wgpu::TextureComponentType::DepthComparison: // When the compiler complains that you need to add a case statement here, please // also add a corresponding static assert below! break; @@ -55,6 +56,11 @@ namespace dawn_native { static_cast( 1 << static_cast(wgpu::TextureComponentType::Sint)), ""); + static_assert( + ComponentTypeBit::DepthComparison == + static_cast( + 1 << static_cast(wgpu::TextureComponentType::DepthComparison)), + ""); return static_cast(1 << static_cast(type)); } @@ -157,7 +163,8 @@ namespace dawn_native { internalFormat.firstAspect.block.height = 1; internalFormat.firstAspect.baseType = wgpu::TextureComponentType::Float; if (isDepthSampleable) { - internalFormat.firstAspect.supportedComponentTypes = ComponentTypeBit::Float; + internalFormat.firstAspect.supportedComponentTypes = + ComponentTypeBit::Float | ComponentTypeBit::DepthComparison; } else { internalFormat.firstAspect.supportedComponentTypes = ComponentTypeBit::None; } diff --git a/src/dawn_native/Format.h b/src/dawn_native/Format.h index 392254ea1e..ec4d6b44ff 100644 --- a/src/dawn_native/Format.h +++ b/src/dawn_native/Format.h @@ -34,6 +34,7 @@ namespace dawn_native { Float = 0x1, Sint = 0x2, Uint = 0x4, + DepthComparison = 0x8, }; // Converts an wgpu::TextureComponentType to its bitmask representation. diff --git a/src/dawn_native/Pipeline.cpp b/src/dawn_native/Pipeline.cpp index d7ebbe2aef..928f7f2920 100644 --- a/src/dawn_native/Pipeline.cpp +++ b/src/dawn_native/Pipeline.cpp @@ -22,7 +22,7 @@ namespace dawn_native { - MaybeError ValidateProgrammableStageDescriptor(const DeviceBase* device, + MaybeError ValidateProgrammableStageDescriptor(DeviceBase* device, const ProgrammableStageDescriptor* descriptor, const PipelineLayoutBase* layout, SingleShaderStage stage) { @@ -36,7 +36,7 @@ namespace dawn_native { if (layout != nullptr) { const EntryPointMetadata& metadata = module->GetEntryPoint(descriptor->entryPoint, stage); - DAWN_TRY(ValidateCompatibilityWithPipelineLayout(metadata, layout)); + DAWN_TRY(ValidateCompatibilityWithPipelineLayout(device, metadata, layout)); } return {}; } diff --git a/src/dawn_native/Pipeline.h b/src/dawn_native/Pipeline.h index 9bc249c4b6..395410a9db 100644 --- a/src/dawn_native/Pipeline.h +++ b/src/dawn_native/Pipeline.h @@ -28,7 +28,7 @@ namespace dawn_native { - MaybeError ValidateProgrammableStageDescriptor(const DeviceBase* device, + MaybeError ValidateProgrammableStageDescriptor(DeviceBase* device, const ProgrammableStageDescriptor* descriptor, const PipelineLayoutBase* layout, SingleShaderStage stage); diff --git a/src/dawn_native/PipelineLayout.cpp b/src/dawn_native/PipelineLayout.cpp index 29981940ac..883d9f9ead 100644 --- a/src/dawn_native/PipelineLayout.cpp +++ b/src/dawn_native/PipelineLayout.cpp @@ -207,7 +207,8 @@ namespace dawn_native { for (const StageAndDescriptor& stage : stages) { const EntryPointMetadata& metadata = stage.second->module->GetEntryPoint(stage.second->entryPoint, stage.first); - ASSERT(ValidateCompatibilityWithPipelineLayout(metadata, pipelineLayout).IsSuccess()); + ASSERT(ValidateCompatibilityWithPipelineLayout(device, metadata, pipelineLayout) + .IsSuccess()); } return pipelineLayout; diff --git a/src/dawn_native/ShaderModule.cpp b/src/dawn_native/ShaderModule.cpp index 25c9407e4d..f81461ac05 100644 --- a/src/dawn_native/ShaderModule.cpp +++ b/src/dawn_native/ShaderModule.cpp @@ -369,7 +369,8 @@ namespace dawn_native { return std::move(result); } - MaybeError ValidateCompatibilityWithBindGroupLayout(BindGroupIndex group, + MaybeError ValidateCompatibilityWithBindGroupLayout(DeviceBase* device, + BindGroupIndex group, const EntryPointMetadata& entryPoint, const BindGroupLayoutBase* layout) { const BindGroupLayoutBase::BindingMap& layoutBindings = layout->GetBindingMap(); @@ -424,10 +425,22 @@ namespace dawn_native { case wgpu::BindingType::SampledTexture: case wgpu::BindingType::MultisampledTexture: { if (layoutInfo.textureComponentType != shaderInfo.textureComponentType) { - return DAWN_VALIDATION_ERROR( - "The textureComponentType of the bind group layout entry is " - "different from " + - GetShaderDeclarationString(group, bindingNumber)); + // TODO(dawn:527): Remove once the deprecation timeline is complete. + if (layoutInfo.textureComponentType == + wgpu::TextureComponentType::Float && + shaderInfo.textureComponentType == + wgpu::TextureComponentType::DepthComparison) { + device->EmitDeprecationWarning( + "Using depth texture in the shader with " + "TextureComponentType::Float is deprecated use " + "TextureComponentType::DepthComparison in the bind group " + "layout instead."); + } else { + return DAWN_VALIDATION_ERROR( + "The textureComponentType of the bind group layout entry is " + "different from " + + GetShaderDeclarationString(group, bindingNumber)); + } } if (layoutInfo.viewDimension != shaderInfo.viewDimension) { @@ -554,11 +567,26 @@ namespace dawn_native { SpirvDimToTextureViewDimension(imageType.dim, imageType.arrayed); info->textureComponentType = SpirvBaseTypeToTextureComponentType(textureComponentType); + if (imageType.ms) { info->type = wgpu::BindingType::MultisampledTexture; } else { info->type = wgpu::BindingType::SampledTexture; } + + if (imageType.depth) { + if (imageType.ms) { + return DAWN_VALIDATION_ERROR( + "Multisampled depth textures aren't supported"); + } + if (info->textureComponentType != + wgpu::TextureComponentType::Float) { + return DAWN_VALIDATION_ERROR( + "Depth textures must have a float type"); + } + info->textureComponentType = + wgpu::TextureComponentType::DepthComparison; + } break; } case wgpu::BindingType::StorageBuffer: { @@ -600,7 +628,11 @@ namespace dawn_native { } if (imageType.ms) { return DAWN_VALIDATION_ERROR( - "Multisampled storage texture aren't supported"); + "Multisampled storage textures aren't supported"); + } + if (imageType.depth) { + return DAWN_VALIDATION_ERROR( + "Depth storage textures aren't supported"); } info->storageTextureFormat = storageTextureFormat; info->viewDimension = @@ -749,10 +781,11 @@ namespace dawn_native { return bufferSizes; } - MaybeError ValidateCompatibilityWithPipelineLayout(const EntryPointMetadata& entryPoint, + MaybeError ValidateCompatibilityWithPipelineLayout(DeviceBase* device, + const EntryPointMetadata& entryPoint, const PipelineLayoutBase* layout) { for (BindGroupIndex group : IterateBitSet(layout->GetBindGroupLayoutsMask())) { - DAWN_TRY(ValidateCompatibilityWithBindGroupLayout(group, entryPoint, + DAWN_TRY(ValidateCompatibilityWithBindGroupLayout(device, group, entryPoint, layout->GetBindGroupLayout(group))); } diff --git a/src/dawn_native/ShaderModule.h b/src/dawn_native/ShaderModule.h index 5178412b15..ef1f8adb07 100644 --- a/src/dawn_native/ShaderModule.h +++ b/src/dawn_native/ShaderModule.h @@ -41,7 +41,8 @@ namespace dawn_native { MaybeError ValidateShaderModuleDescriptor(DeviceBase* device, const ShaderModuleDescriptor* descriptor); - MaybeError ValidateCompatibilityWithPipelineLayout(const EntryPointMetadata& entryPoint, + MaybeError ValidateCompatibilityWithPipelineLayout(DeviceBase* device, + const EntryPointMetadata& entryPoint, const PipelineLayoutBase* layout); RequiredBufferSizes ComputeRequiredBufferSizesForLayout(const EntryPointMetadata& entryPoint, diff --git a/src/dawn_native/d3d12/RenderPassBuilderD3D12.cpp b/src/dawn_native/d3d12/RenderPassBuilderD3D12.cpp index 12216f5913..f24a39f5a4 100644 --- a/src/dawn_native/d3d12/RenderPassBuilderD3D12.cpp +++ b/src/dawn_native/d3d12/RenderPassBuilderD3D12.cpp @@ -71,6 +71,9 @@ namespace dawn_native { namespace d3d12 { case wgpu::TextureComponentType::Float: resolveParameters.ResolveMode = D3D12_RESOLVE_MODE_AVERAGE; break; + + case wgpu::TextureComponentType::DepthComparison: + UNREACHABLE(); } resolveParameters.SubresourceCount = 1; diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index f75f8544dc..65dd9e068c 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -949,6 +949,9 @@ namespace dawn_native { namespace opengl { gl.ClearBufferiv(GL_COLOR, i, appliedClearColor.data()); break; } + + case wgpu::TextureComponentType::DepthComparison: + UNREACHABLE(); } } diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp index 2138f985f3..7b02f62a62 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.cpp +++ b/src/dawn_native/vulkan/CommandBufferVk.cpp @@ -334,6 +334,9 @@ namespace dawn_native { namespace vulkan { } break; } + + case wgpu::TextureComponentType::DepthComparison: + UNREACHABLE(); } attachmentCount++; } diff --git a/src/dawn_native/vulkan/TextureVk.cpp b/src/dawn_native/vulkan/TextureVk.cpp index 5c0bff9f79..8de770aedd 100644 --- a/src/dawn_native/vulkan/TextureVk.cpp +++ b/src/dawn_native/vulkan/TextureVk.cpp @@ -1058,6 +1058,8 @@ namespace dawn_native { namespace vulkan { clearColorValue.uint32[2] = uClearColor; clearColorValue.uint32[3] = uClearColor; break; + case wgpu::TextureComponentType::DepthComparison: + UNREACHABLE(); } device->fn.CmdClearColorImage(recordingContext->commandBuffer, GetHandle(), VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, diff --git a/src/tests/end2end/DeprecatedAPITests.cpp b/src/tests/end2end/DeprecatedAPITests.cpp index cd92d5b14d..4b3222d7e4 100644 --- a/src/tests/end2end/DeprecatedAPITests.cpp +++ b/src/tests/end2end/DeprecatedAPITests.cpp @@ -118,6 +118,50 @@ TEST_P(DeprecationTests, BGLEntryMultisampledBooleanTracking) { utils::MakeBindGroup(device, bgl, {{0, texture4Sample.CreateView()}}); } +// Test that compiling a pipeline with TextureComponentType::Float in the BGL when ::DepthComparison +// is expected emits a deprecation warning but isn't an error. +TEST_P(DeprecationTests, TextureComponentTypeFloatWhenDepthComparisonIsExpected) { + wgpu::ShaderModule module = + utils::CreateShaderModule(device, utils::SingleShaderStage::Compute, R"( + #version 450 + layout(set = 0, binding = 0) uniform samplerShadow samp; + layout(set = 0, binding = 1) uniform texture2D tex; + + void main() { + texture(sampler2DShadow(tex, samp), vec3(0.5, 0.5, 0.5)); + } + )"); + + { + wgpu::BindGroupLayout goodBgl = utils::MakeBindGroupLayout( + device, + {{0, wgpu::ShaderStage::Compute, wgpu::BindingType::ComparisonSampler}, + {1, wgpu::ShaderStage::Compute, wgpu::BindingType::SampledTexture, false, 0, false, + wgpu::TextureViewDimension::e2D, wgpu::TextureComponentType::DepthComparison}}); + + wgpu::ComputePipelineDescriptor goodDesc; + goodDesc.layout = utils::MakeBasicPipelineLayout(device, &goodBgl); + goodDesc.computeStage.module = module; + goodDesc.computeStage.entryPoint = "main"; + + device.CreateComputePipeline(&goodDesc); + } + + { + wgpu::BindGroupLayout badBgl = utils::MakeBindGroupLayout( + device, {{0, wgpu::ShaderStage::Compute, wgpu::BindingType::ComparisonSampler}, + {1, wgpu::ShaderStage::Compute, wgpu::BindingType::SampledTexture, false, 0, + false, wgpu::TextureViewDimension::e2D, wgpu::TextureComponentType::Float}}); + + wgpu::ComputePipelineDescriptor badDesc; + badDesc.layout = utils::MakeBasicPipelineLayout(device, &badBgl); + badDesc.computeStage.module = module; + badDesc.computeStage.entryPoint = "main"; + + EXPECT_DEPRECATION_WARNING(device.CreateComputePipeline(&badDesc)); + } +} + DAWN_INSTANTIATE_TEST(DeprecationTests, D3D12Backend(), MetalBackend(), diff --git a/src/tests/end2end/DepthSamplingTests.cpp b/src/tests/end2end/DepthSamplingTests.cpp index 29ec5e4b19..14d6d4f740 100644 --- a/src/tests/end2end/DepthSamplingTests.cpp +++ b/src/tests/end2end/DepthSamplingTests.cpp @@ -151,9 +151,11 @@ class DepthSamplingTest : public DawnTest { // TODO(dawn:367): Cannot use GetBindGroupLayout for comparison samplers without shader // reflection data. wgpu::BindGroupLayout bgl = utils::MakeBindGroupLayout( - device, {{0, wgpu::ShaderStage::Fragment, wgpu::BindingType::ComparisonSampler}, - {1, wgpu::ShaderStage::Fragment, wgpu::BindingType::SampledTexture}, - {2, wgpu::ShaderStage::Fragment, wgpu::BindingType::UniformBuffer}}); + device, + {{0, wgpu::ShaderStage::Fragment, wgpu::BindingType::ComparisonSampler}, + {1, wgpu::ShaderStage::Fragment, wgpu::BindingType::SampledTexture, false, 0, false, + wgpu::TextureViewDimension::e2D, wgpu::TextureComponentType::DepthComparison}, + {2, wgpu::ShaderStage::Fragment, wgpu::BindingType::UniformBuffer}}); utils::ComboRenderPipelineDescriptor pipelineDescriptor(device); pipelineDescriptor.vertexStage.module = vsModule; @@ -185,10 +187,12 @@ class DepthSamplingTest : public DawnTest { // TODO(dawn:367): Cannot use GetBindGroupLayout without shader reflection data. wgpu::BindGroupLayout bgl = utils::MakeBindGroupLayout( - device, {{0, wgpu::ShaderStage::Compute, wgpu::BindingType::ComparisonSampler}, - {1, wgpu::ShaderStage::Compute, wgpu::BindingType::SampledTexture}, - {2, wgpu::ShaderStage::Compute, wgpu::BindingType::UniformBuffer}, - {3, wgpu::ShaderStage::Compute, wgpu::BindingType::StorageBuffer}}); + device, + {{0, wgpu::ShaderStage::Compute, wgpu::BindingType::ComparisonSampler}, + {1, wgpu::ShaderStage::Compute, wgpu::BindingType::SampledTexture, false, 0, false, + wgpu::TextureViewDimension::e2D, wgpu::TextureComponentType::DepthComparison}, + {2, wgpu::ShaderStage::Compute, wgpu::BindingType::UniformBuffer}, + {3, wgpu::ShaderStage::Compute, wgpu::BindingType::StorageBuffer}}); wgpu::ComputePipelineDescriptor pipelineDescriptor; pipelineDescriptor.layout = utils::MakeBasicPipelineLayout(device, &bgl); diff --git a/src/tests/unittests/validation/BindGroupValidationTests.cpp b/src/tests/unittests/validation/BindGroupValidationTests.cpp index d1f9cec6a7..28d54e43cf 100644 --- a/src/tests/unittests/validation/BindGroupValidationTests.cpp +++ b/src/tests/unittests/validation/BindGroupValidationTests.cpp @@ -345,6 +345,41 @@ TEST_F(BindGroupValidationTest, SamplingDepthTexture) { } } +// Check that a texture must have a correct format for DepthComparison +TEST_F(BindGroupValidationTest, TextureComponentTypeDepthComparison) { + wgpu::BindGroupLayout depthLayout = utils::MakeBindGroupLayout( + device, + {{0, wgpu::ShaderStage::Fragment, wgpu::BindingType::SampledTexture, false, 0, false, + wgpu::TextureViewDimension::e2D, wgpu::TextureComponentType::DepthComparison}}); + + // Control case: setting a depth texture works. + wgpu::Texture depthTexture = + CreateTexture(wgpu::TextureUsage::Sampled, wgpu::TextureFormat::Depth32Float, 1); + utils::MakeBindGroup(device, depthLayout, {{0, depthTexture.CreateView()}}); + + // Error case: setting a Float typed texture view fails. + ASSERT_DEVICE_ERROR(utils::MakeBindGroup(device, depthLayout, {{0, mSampledTextureView}})); +} + +// Check that a depth texture is allowed to be used for both TextureComponentType::Float and +// ::DepthComparison +TEST_F(BindGroupValidationTest, TextureComponentTypeForDepthTexture) { + wgpu::BindGroupLayout depthLayout = utils::MakeBindGroupLayout( + device, + {{0, wgpu::ShaderStage::Fragment, wgpu::BindingType::SampledTexture, false, 0, false, + wgpu::TextureViewDimension::e2D, wgpu::TextureComponentType::DepthComparison}}); + + wgpu::BindGroupLayout floatLayout = utils::MakeBindGroupLayout( + device, {{0, wgpu::ShaderStage::Fragment, wgpu::BindingType::SampledTexture, false, 0, + false, wgpu::TextureViewDimension::e2D, wgpu::TextureComponentType::Float}}); + + wgpu::Texture depthTexture = + CreateTexture(wgpu::TextureUsage::Sampled, wgpu::TextureFormat::Depth32Float, 1); + + utils::MakeBindGroup(device, depthLayout, {{0, depthTexture.CreateView()}}); + utils::MakeBindGroup(device, floatLayout, {{0, depthTexture.CreateView()}}); +} + // Check that a texture must have the correct dimension TEST_F(BindGroupValidationTest, TextureDimension) { wgpu::BindGroupLayout layout = utils::MakeBindGroupLayout( @@ -900,7 +935,7 @@ TEST_F(BindGroupLayoutValidationTest, DynamicBufferNumberLimit) { } // Test that multisampled textures must be 2D sampled textures -TEST_F(BindGroupLayoutValidationTest, MultisampledTextures) { +TEST_F(BindGroupLayoutValidationTest, MultisampledTextureViewDimension) { // Multisampled 2D texture works. utils::MakeBindGroupLayout( device, { @@ -958,6 +993,45 @@ TEST_F(BindGroupLayoutValidationTest, MultisampledTextures) { })); } +// Test that multisampled textures cannot be DepthComparison +TEST_F(BindGroupLayoutValidationTest, MultisampledTextureComponentType) { + // Multisampled float component type works. + utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::BindingType::MultisampledTexture, false, + 0, false, wgpu::TextureViewDimension::e2D, wgpu::TextureComponentType::Float}, + }); + + // Multisampled float (defaulted) component type works. + utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::BindingType::MultisampledTexture, false, + 0, false, wgpu::TextureViewDimension::e2D}, + }); + + // Multisampled uint component type works. + utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::BindingType::MultisampledTexture, false, + 0, false, wgpu::TextureViewDimension::e2D, wgpu::TextureComponentType::Uint}, + }); + + // Multisampled sint component type works. + utils::MakeBindGroupLayout( + device, { + {0, wgpu::ShaderStage::Compute, wgpu::BindingType::MultisampledTexture, false, + 0, false, wgpu::TextureViewDimension::e2D, wgpu::TextureComponentType::Sint}, + }); + + // Multisampled depth comparison component typeworks. + ASSERT_DEVICE_ERROR(utils::MakeBindGroupLayout( + device, + { + {0, wgpu::ShaderStage::Compute, wgpu::BindingType::MultisampledTexture, false, 0, false, + wgpu::TextureViewDimension::e2D, wgpu::TextureComponentType::DepthComparison}, + })); +} + // Test that it is an error to pass multisampled=true for non-texture bindings TEST_F(BindGroupLayoutValidationTest, MultisampledMustBeTexture) { // Base: Multisampled 2D texture works.