diff --git a/src/dawn_native/BindGroup.cpp b/src/dawn_native/BindGroup.cpp index 9de7fd0e62..db46993cc0 100644 --- a/src/dawn_native/BindGroup.cpp +++ b/src/dawn_native/BindGroup.cpp @@ -36,7 +36,7 @@ namespace dawn_native { const uint64_t maxBindingSize) { if (entry.buffer == nullptr || entry.sampler != nullptr || entry.textureView != nullptr) { - return DAWN_VALIDATION_ERROR("expected buffer binding"); + return DAWN_VALIDATION_ERROR("Expected buffer binding"); } DAWN_TRY(device->ValidateObject(entry.buffer)); @@ -98,32 +98,41 @@ namespace dawn_native { const BindingInfo& bindingInfo) { if (entry.textureView == nullptr || entry.sampler != nullptr || entry.buffer != nullptr) { - return DAWN_VALIDATION_ERROR("expected texture binding"); + return DAWN_VALIDATION_ERROR("Expected texture binding"); } DAWN_TRY(device->ValidateObject(entry.textureView)); - TextureBase* texture = entry.textureView->GetTexture(); + TextureViewBase* view = entry.textureView; + Aspect aspect = view->GetAspects(); + if (!HasOneBit(aspect)) { + return DAWN_VALIDATION_ERROR("Texture view must select a single aspect"); + } + + TextureBase* texture = view->GetTexture(); if (!(texture->GetUsage() & requiredUsage)) { - return DAWN_VALIDATION_ERROR("texture binding usage mismatch"); + return DAWN_VALIDATION_ERROR("Texture binding usage mismatch"); } if (texture->IsMultisampledTexture() != multisampled) { - return DAWN_VALIDATION_ERROR("texture multisampling mismatch"); + return DAWN_VALIDATION_ERROR("Texture multisampling mismatch"); } switch (requiredUsage) { case wgpu::TextureUsage::Sampled: { - if (!texture->GetFormat().HasComponentType( - Format::TextureComponentTypeToFormatType( - bindingInfo.textureComponentType))) { - return DAWN_VALIDATION_ERROR("texture component type usage mismatch"); + ComponentTypeBit supportedTypes = + texture->GetFormat().GetAspectInfo(aspect).supportedComponentTypes; + ComponentTypeBit requiredType = + ToComponentTypeBit(bindingInfo.textureComponentType); + + if ((supportedTypes & requiredType) == 0) { + return DAWN_VALIDATION_ERROR("Texture component type usage mismatch"); } break; } case wgpu::TextureUsage::Storage: { if (texture->GetFormat().format != bindingInfo.storageTextureFormat) { - return DAWN_VALIDATION_ERROR("storage texture format mismatch"); + return DAWN_VALIDATION_ERROR("Storage texture format mismatch"); } break; } @@ -133,7 +142,7 @@ namespace dawn_native { } if (entry.textureView->GetDimension() != bindingInfo.viewDimension) { - return DAWN_VALIDATION_ERROR("texture view dimension mismatch"); + return DAWN_VALIDATION_ERROR("Texture view dimension mismatch"); } return {}; @@ -144,7 +153,7 @@ namespace dawn_native { wgpu::BindingType bindingType) { if (entry.sampler == nullptr || entry.textureView != nullptr || entry.buffer != nullptr) { - return DAWN_VALIDATION_ERROR("expected sampler binding"); + return DAWN_VALIDATION_ERROR("Expected sampler binding"); } DAWN_TRY(device->ValidateObject(entry.sampler)); diff --git a/src/dawn_native/Format.cpp b/src/dawn_native/Format.cpp index 0fbd80822b..5bf672c590 100644 --- a/src/dawn_native/Format.cpp +++ b/src/dawn_native/Format.cpp @@ -24,44 +24,38 @@ namespace dawn_native { namespace { - static const AspectInfo kStencil8AspectInfo = {{1, 1, 1}}; - + static const AspectInfo kStencil8AspectInfo = {{1, 1, 1}, + wgpu::TextureComponentType::Uint, + ComponentTypeBit::Uint}; } // Format - // static - Format::Type Format::TextureComponentTypeToFormatType( - wgpu::TextureComponentType componentType) { - switch (componentType) { + ComponentTypeBit ToComponentTypeBit(wgpu::TextureComponentType type) { + switch (type) { case wgpu::TextureComponentType::Float: case wgpu::TextureComponentType::Sint: case wgpu::TextureComponentType::Uint: + // When the compiler complains that you need to add a case statement here, please + // also add a corresponding static assert below! break; } - // Check that Type correctly mirrors TextureComponentType except for "Other". - static_assert(static_cast(wgpu::TextureComponentType::Float) == Type::Float, ""); - static_assert(static_cast(wgpu::TextureComponentType::Sint) == Type::Sint, ""); - static_assert(static_cast(wgpu::TextureComponentType::Uint) == Type::Uint, ""); - return static_cast(componentType); - } - // static - wgpu::TextureComponentType Format::FormatTypeToTextureComponentType(Type type) { - switch (type) { - case Type::Float: - case Type::Sint: - case Type::Uint: - break; - - case Type::Other: - UNREACHABLE(); - } - // Check that Type correctly mirrors TextureComponentType except for "Other". - static_assert(static_cast(wgpu::TextureComponentType::Float) == Type::Float, ""); - static_assert(static_cast(wgpu::TextureComponentType::Sint) == Type::Sint, ""); - static_assert(static_cast(wgpu::TextureComponentType::Uint) == Type::Uint, ""); - return static_cast(type); + // Check that ComponentTypeBit bits are in the same position / order as the respective + // wgpu::TextureComponentType value. + static_assert(ComponentTypeBit::Float == + static_cast( + 1 << static_cast(wgpu::TextureComponentType::Float)), + ""); + static_assert(ComponentTypeBit::Uint == + static_cast( + 1 << static_cast(wgpu::TextureComponentType::Uint)), + ""); + static_assert(ComponentTypeBit::Sint == + static_cast( + 1 << static_cast(wgpu::TextureComponentType::Sint)), + ""); + return static_cast(1 << static_cast(type)); } bool Format::IsColor() const { @@ -80,10 +74,6 @@ namespace dawn_native { return (aspects & (Aspect::Depth | Aspect::Stencil)) != 0; } - bool Format::HasComponentType(Type componentType) const { - return componentType == type; - } - const AspectInfo& Format::GetAspectInfo(wgpu::TextureAspect aspect) const { return GetAspectInfo(ConvertAspect(*this, aspect)); } @@ -121,7 +111,7 @@ namespace dawn_native { FormatTable table; std::bitset formatsSet; - using Type = Format::Type; + using Type = wgpu::TextureComponentType; auto AddFormat = [&table, &formatsSet](Format format) { size_t index = ComputeFormatIndex(format.format); @@ -145,15 +135,16 @@ namespace dawn_native { internalFormat.isSupported = true; internalFormat.supportsStorageUsage = supportsStorageUsage; internalFormat.aspects = Aspect::Color; - internalFormat.type = type; internalFormat.firstAspect.block.byteSize = byteSize; internalFormat.firstAspect.block.width = 1; internalFormat.firstAspect.block.height = 1; + internalFormat.firstAspect.baseType = type; + internalFormat.firstAspect.supportedComponentTypes = ToComponentTypeBit(type); AddFormat(internalFormat); }; auto AddDepthStencilFormat = [&AddFormat](wgpu::TextureFormat format, Aspect aspects, - uint32_t byteSize) { + uint32_t byteSize, bool isDepthSampleable) { Format internalFormat; internalFormat.format = format; internalFormat.isRenderable = true; @@ -161,26 +152,15 @@ namespace dawn_native { internalFormat.isSupported = true; internalFormat.supportsStorageUsage = false; internalFormat.aspects = aspects; - internalFormat.type = Type::Other; - internalFormat.firstAspect.block.byteSize = byteSize; - internalFormat.firstAspect.block.width = 1; - internalFormat.firstAspect.block.height = 1; - AddFormat(internalFormat); - }; - - auto AddDepthFormat = [&AddFormat](wgpu::TextureFormat format, uint32_t byteSize, - Type type) { - Format internalFormat; - internalFormat.format = format; - internalFormat.isRenderable = true; - internalFormat.isCompressed = false; - internalFormat.isSupported = true; - internalFormat.supportsStorageUsage = false; - internalFormat.aspects = Aspect::Depth; - internalFormat.type = type; internalFormat.firstAspect.block.byteSize = byteSize; internalFormat.firstAspect.block.width = 1; internalFormat.firstAspect.block.height = 1; + internalFormat.firstAspect.baseType = wgpu::TextureComponentType::Float; + if (isDepthSampleable) { + internalFormat.firstAspect.supportedComponentTypes = ComponentTypeBit::Float; + } else { + internalFormat.firstAspect.supportedComponentTypes = ComponentTypeBit::None; + } AddFormat(internalFormat); }; @@ -193,10 +173,11 @@ namespace dawn_native { internalFormat.isSupported = isSupported; internalFormat.supportsStorageUsage = false; internalFormat.aspects = Aspect::Color; - internalFormat.type = Type::Float; internalFormat.firstAspect.block.byteSize = byteSize; internalFormat.firstAspect.block.width = width; internalFormat.firstAspect.block.height = height; + internalFormat.firstAspect.baseType = wgpu::TextureComponentType::Float; + internalFormat.firstAspect.supportedComponentTypes = ComponentTypeBit::Float; AddFormat(internalFormat); }; @@ -249,15 +230,13 @@ namespace dawn_native { AddColorFormat(wgpu::TextureFormat::RGBA32Sint, true, true, 16, Type::Sint); AddColorFormat(wgpu::TextureFormat::RGBA32Float, true, true, 16, Type::Float); - // Depth only formats - AddDepthFormat(wgpu::TextureFormat::Depth32Float, 4, Type::Float); - - // Packed depth/depth-stencil formats - AddDepthStencilFormat(wgpu::TextureFormat::Depth24Plus, Aspect::Depth, 4); + // Depth-stencil formats + AddDepthStencilFormat(wgpu::TextureFormat::Depth32Float, Aspect::Depth, 4, true); + AddDepthStencilFormat(wgpu::TextureFormat::Depth24Plus, Aspect::Depth, 4, false); // TODO(cwallez@chromium.org): It isn't clear if this format should be copyable // because its size isn't well defined, is it 4, 5 or 8? AddDepthStencilFormat(wgpu::TextureFormat::Depth24PlusStencil8, - Aspect::Depth | Aspect::Stencil, 4); + Aspect::Depth | Aspect::Stencil, 4, false); // BC compressed formats bool isBCFormatSupported = device->IsExtensionEnabled(Extension::TextureCompressionBC); diff --git a/src/dawn_native/Format.h b/src/dawn_native/Format.h index 3052f679be..392254ea1e 100644 --- a/src/dawn_native/Format.h +++ b/src/dawn_native/Format.h @@ -28,6 +28,17 @@ namespace dawn_native { enum class Aspect : uint8_t; class DeviceBase; + // This mirrors wgpu::TextureComponentType as a bitmask instead. + enum class ComponentTypeBit : uint8_t { + None = 0x0, + Float = 0x1, + Sint = 0x2, + Uint = 0x4, + }; + + // Converts an wgpu::TextureComponentType to its bitmask representation. + ComponentTypeBit ToComponentTypeBit(wgpu::TextureComponentType type); + struct TexelBlockInfo { uint32_t byteSize; uint32_t width; @@ -36,6 +47,8 @@ namespace dawn_native { struct AspectInfo { TexelBlockInfo block; + wgpu::TextureComponentType baseType; + ComponentTypeBit supportedComponentTypes; }; // The number of formats Dawn knows about. Asserts in BuildFormatTable ensure that this is the @@ -47,30 +60,18 @@ namespace dawn_native { // A wgpu::TextureFormat along with all the information about it necessary for validation. struct Format { - enum class Type { - Float, - Sint, - Uint, - Other, - }; - wgpu::TextureFormat format; bool isRenderable; bool isCompressed; // A format can be known but not supported because it is part of a disabled extension. bool isSupported; bool supportsStorageUsage; - Type type; Aspect aspects; - static Type TextureComponentTypeToFormatType(wgpu::TextureComponentType componentType); - static wgpu::TextureComponentType FormatTypeToTextureComponentType(Type type); - bool IsColor() const; bool HasDepth() const; bool HasStencil() const; bool HasDepthOrStencil() const; - bool HasComponentType(Type componentType) const; const AspectInfo& GetAspectInfo(wgpu::TextureAspect aspect) const; const AspectInfo& GetAspectInfo(Aspect aspect) const; @@ -96,4 +97,13 @@ namespace dawn_native { } // namespace dawn_native +namespace wgpu { + + template <> + struct IsDawnBitmask { + static constexpr bool enable = true; + }; + +} // namespace wgpu + #endif // DAWNNATIVE_FORMAT_H_ diff --git a/src/dawn_native/RenderPipeline.cpp b/src/dawn_native/RenderPipeline.cpp index 8d267b6d71..c00990f985 100644 --- a/src/dawn_native/RenderPipeline.cpp +++ b/src/dawn_native/RenderPipeline.cpp @@ -163,7 +163,7 @@ namespace dawn_native { return DAWN_VALIDATION_ERROR("Color format must be color renderable"); } if (fragmentWritten && - fragmentOutputBaseType != Format::FormatTypeToTextureComponentType(format->type)) { + fragmentOutputBaseType != format->GetAspectInfo(Aspect::Color).baseType) { return DAWN_VALIDATION_ERROR( "Color format must match the fragment stage output type"); } diff --git a/src/dawn_native/d3d12/RenderPassBuilderD3D12.cpp b/src/dawn_native/d3d12/RenderPassBuilderD3D12.cpp index be30ab2df7..12216f5913 100644 --- a/src/dawn_native/d3d12/RenderPassBuilderD3D12.cpp +++ b/src/dawn_native/d3d12/RenderPassBuilderD3D12.cpp @@ -63,12 +63,12 @@ namespace dawn_native { namespace d3d12 { // RESOLVE_MODE_AVERAGE is only valid for non-integer formats. // TODO: Investigate and determine how integer format resolves should work in WebGPU. - switch (resolveDestination->GetFormat().type) { - case Format::Type::Sint: - case Format::Type::Uint: + switch (resolveDestination->GetFormat().GetAspectInfo(Aspect::Color).baseType) { + case wgpu::TextureComponentType::Sint: + case wgpu::TextureComponentType::Uint: resolveParameters.ResolveMode = D3D12_RESOLVE_MODE_MAX; break; - default: + case wgpu::TextureComponentType::Float: resolveParameters.ResolveMode = D3D12_RESOLVE_MODE_AVERAGE; break; } diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index 141ae67ea2..f75f8544dc 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -928,21 +928,27 @@ namespace dawn_native { namespace opengl { if (attachmentInfo->loadOp == wgpu::LoadOp::Clear) { gl.ColorMaski(i, true, true, true, true); - const Format& attachmentFormat = attachmentInfo->view->GetFormat(); - if (attachmentFormat.HasComponentType(Format::Type::Float)) { - const std::array appliedClearColor = - ConvertToFloatColor(attachmentInfo->clearColor); - gl.ClearBufferfv(GL_COLOR, i, appliedClearColor.data()); - } else if (attachmentFormat.HasComponentType(Format::Type::Uint)) { - const std::array appliedClearColor = - ConvertToUnsignedIntegerColor(attachmentInfo->clearColor); - gl.ClearBufferuiv(GL_COLOR, i, appliedClearColor.data()); - } else if (attachmentFormat.HasComponentType(Format::Type::Sint)) { - const std::array appliedClearColor = - ConvertToSignedIntegerColor(attachmentInfo->clearColor); - gl.ClearBufferiv(GL_COLOR, i, appliedClearColor.data()); - } else { - UNREACHABLE(); + wgpu::TextureComponentType baseType = + attachmentInfo->view->GetFormat().GetAspectInfo(Aspect::Color).baseType; + switch (baseType) { + case wgpu::TextureComponentType::Float: { + const std::array appliedClearColor = + ConvertToFloatColor(attachmentInfo->clearColor); + gl.ClearBufferfv(GL_COLOR, i, appliedClearColor.data()); + break; + } + case wgpu::TextureComponentType::Uint: { + const std::array appliedClearColor = + ConvertToUnsignedIntegerColor(attachmentInfo->clearColor); + gl.ClearBufferuiv(GL_COLOR, i, appliedClearColor.data()); + break; + } + case wgpu::TextureComponentType::Sint: { + const std::array appliedClearColor = + ConvertToSignedIntegerColor(attachmentInfo->clearColor); + gl.ClearBufferiv(GL_COLOR, i, appliedClearColor.data()); + break; + } } } diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp index 73a2f970f5..2138f985f3 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.cpp +++ b/src/dawn_native/vulkan/CommandBufferVk.cpp @@ -308,29 +308,33 @@ namespace dawn_native { namespace vulkan { attachments[attachmentCount] = view->GetHandle(); - const Format& attachmentFormat = view->GetFormat(); - if (attachmentFormat.HasComponentType(Format::Type::Float)) { - const std::array appliedClearColor = - ConvertToFloatColor(attachmentInfo.clearColor); - for (uint32_t i = 0; i < 4; ++i) { - clearValues[attachmentCount].color.float32[i] = appliedClearColor[i]; + switch (view->GetFormat().GetAspectInfo(Aspect::Color).baseType) { + case wgpu::TextureComponentType::Float: { + const std::array appliedClearColor = + ConvertToFloatColor(attachmentInfo.clearColor); + for (uint32_t i = 0; i < 4; ++i) { + clearValues[attachmentCount].color.float32[i] = + appliedClearColor[i]; + } + break; } - } else if (attachmentFormat.HasComponentType(Format::Type::Uint)) { - const std::array appliedClearColor = - ConvertToUnsignedIntegerColor(attachmentInfo.clearColor); - for (uint32_t i = 0; i < 4; ++i) { - clearValues[attachmentCount].color.uint32[i] = appliedClearColor[i]; + case wgpu::TextureComponentType::Uint: { + const std::array appliedClearColor = + ConvertToUnsignedIntegerColor(attachmentInfo.clearColor); + for (uint32_t i = 0; i < 4; ++i) { + clearValues[attachmentCount].color.uint32[i] = appliedClearColor[i]; + } + break; } - } else if (attachmentFormat.HasComponentType(Format::Type::Sint)) { - const std::array appliedClearColor = - ConvertToSignedIntegerColor(attachmentInfo.clearColor); - for (uint32_t i = 0; i < 4; ++i) { - clearValues[attachmentCount].color.int32[i] = appliedClearColor[i]; + case wgpu::TextureComponentType::Sint: { + const std::array appliedClearColor = + ConvertToSignedIntegerColor(attachmentInfo.clearColor); + for (uint32_t i = 0; i < 4; ++i) { + clearValues[attachmentCount].color.int32[i] = appliedClearColor[i]; + } + break; } - } else { - UNREACHABLE(); } - attachmentCount++; } diff --git a/src/dawn_native/vulkan/TextureVk.cpp b/src/dawn_native/vulkan/TextureVk.cpp index 882866d9fd..5c0bff9f79 100644 --- a/src/dawn_native/vulkan/TextureVk.cpp +++ b/src/dawn_native/vulkan/TextureVk.cpp @@ -1039,28 +1039,25 @@ namespace dawn_native { namespace vulkan { } else { ASSERT(aspects == Aspect::Color); VkClearColorValue clearColorValue; - switch (GetFormat().type) { - case Format::Type::Float: + switch (GetFormat().GetAspectInfo(Aspect::Color).baseType) { + case wgpu::TextureComponentType::Float: clearColorValue.float32[0] = fClearColor; clearColorValue.float32[1] = fClearColor; clearColorValue.float32[2] = fClearColor; clearColorValue.float32[3] = fClearColor; break; - case Format::Type::Sint: + case wgpu::TextureComponentType::Sint: clearColorValue.int32[0] = sClearColor; clearColorValue.int32[1] = sClearColor; clearColorValue.int32[2] = sClearColor; clearColorValue.int32[3] = sClearColor; break; - case Format::Type::Uint: + case wgpu::TextureComponentType::Uint: clearColorValue.uint32[0] = uClearColor; clearColorValue.uint32[1] = uClearColor; clearColorValue.uint32[2] = uClearColor; clearColorValue.uint32[3] = uClearColor; break; - - case Format::Type::Other: - UNREACHABLE(); } device->fn.CmdClearColorImage(recordingContext->commandBuffer, GetHandle(), VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, diff --git a/src/tests/unittests/validation/BindGroupValidationTests.cpp b/src/tests/unittests/validation/BindGroupValidationTests.cpp index 1460851c0e..d1f9cec6a7 100644 --- a/src/tests/unittests/validation/BindGroupValidationTests.cpp +++ b/src/tests/unittests/validation/BindGroupValidationTests.cpp @@ -309,6 +309,42 @@ TEST_F(BindGroupValidationTest, TextureComponentType) { ASSERT_DEVICE_ERROR(utils::MakeBindGroup(device, layout, {{0, uintTextureView}})); } +// Test which depth-stencil formats are allowed to be sampled. +// This is a regression test for a change in dawn_native mistakenly allowing the depth24plus formats +// to be sampled without proper backend support. +TEST_F(BindGroupValidationTest, SamplingDepthTexture) { + wgpu::BindGroupLayout layout = utils::MakeBindGroupLayout( + device, {{0, wgpu::ShaderStage::Fragment, wgpu::BindingType::SampledTexture}}); + + wgpu::TextureDescriptor desc; + desc.size = {1, 1, 1}; + desc.usage = wgpu::TextureUsage::Sampled; + + // Depth32Float is allowed to be sampled. + { + desc.format = wgpu::TextureFormat::Depth32Float; + wgpu::Texture texture = device.CreateTexture(&desc); + + utils::MakeBindGroup(device, layout, {{0, texture.CreateView()}}); + } + + // Depth24Plus is not allowed to be sampled. + { + desc.format = wgpu::TextureFormat::Depth24Plus; + wgpu::Texture texture = device.CreateTexture(&desc); + + ASSERT_DEVICE_ERROR(utils::MakeBindGroup(device, layout, {{0, texture.CreateView()}})); + } + + // Depth24PlusStencil8 is not allowed to be sampled. + { + desc.format = wgpu::TextureFormat::Depth24PlusStencil8; + wgpu::Texture texture = device.CreateTexture(&desc); + + ASSERT_DEVICE_ERROR(utils::MakeBindGroup(device, layout, {{0, texture.CreateView()}})); + } +} + // Check that a texture must have the correct dimension TEST_F(BindGroupValidationTest, TextureDimension) { wgpu::BindGroupLayout layout = utils::MakeBindGroupLayout(