From fc5cae6a1916cb66f2d688dfc5b298ec5425199c Mon Sep 17 00:00:00 2001 From: Brandon Jones Date: Thu, 21 Oct 2021 16:37:45 +0000 Subject: [PATCH] Improving OpenGL backend validation messages. Improves validation messages in various OpenGL backend files: - BackendGL.cpp - BindGroupGL.cpp - CommandBufferGL.cpp - DeviceGL.cpp - PipelineGL.cpp - QueueGL.cpp - ShaderModuleGL.cpp Bug: dawn:563 Change-Id: Idd5751b6f68ea435e5f3c045dcbfd0e5c049fce6 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/67144 Reviewed-by: Corentin Wallez Reviewed-by: Austin Eng Commit-Queue: Brandon Jones --- src/dawn_native/opengl/BackendGL.cpp | 9 +-- src/dawn_native/opengl/BindGroupGL.cpp | 13 ++-- src/dawn_native/opengl/CommandBufferGL.cpp | 8 +- src/dawn_native/opengl/DeviceGL.cpp | 35 +++++---- src/dawn_native/opengl/PipelineGL.cpp | 9 +-- src/dawn_native/opengl/QueueGL.cpp | 5 +- src/dawn_native/opengl/ShaderModuleGL.cpp | 88 +++++++++------------- 7 files changed, 71 insertions(+), 96 deletions(-) diff --git a/src/dawn_native/opengl/BackendGL.cpp b/src/dawn_native/opengl/BackendGL.cpp index 9a66779a95..1907162e82 100644 --- a/src/dawn_native/opengl/BackendGL.cpp +++ b/src/dawn_native/opengl/BackendGL.cpp @@ -278,17 +278,14 @@ namespace dawn_native { namespace opengl { const AdapterDiscoveryOptionsBase* optionsBase) { // TODO(cwallez@chromium.org): For now only create a single OpenGL adapter because don't // know how to handle MakeCurrent. - if (mCreatedAdapter) { - return DAWN_VALIDATION_ERROR("The OpenGL backend can only create a single adapter"); - } + DAWN_INVALID_IF(mCreatedAdapter, "The OpenGL backend can only create a single adapter."); ASSERT(static_cast(optionsBase->backendType) == GetType()); const AdapterDiscoveryOptions* options = static_cast(optionsBase); - if (options->getProc == nullptr) { - return DAWN_VALIDATION_ERROR("AdapterDiscoveryOptions::getProc must be set"); - } + DAWN_INVALID_IF(options->getProc == nullptr, + "AdapterDiscoveryOptions::getProc must be set"); std::unique_ptr adapter = std::make_unique( GetInstance(), static_cast(optionsBase->backendType)); diff --git a/src/dawn_native/opengl/BindGroupGL.cpp b/src/dawn_native/opengl/BindGroupGL.cpp index 721189b9db..c726f295e4 100644 --- a/src/dawn_native/opengl/BindGroupGL.cpp +++ b/src/dawn_native/opengl/BindGroupGL.cpp @@ -33,12 +33,13 @@ namespace dawn_native { namespace opengl { if (bindingInfo.bindingType == BindingInfoType::StorageTexture) { ASSERT(entry.textureView != nullptr); const uint32_t textureViewLayerCount = entry.textureView->GetLayerCount(); - if (textureViewLayerCount != 1 && - textureViewLayerCount != entry.textureView->GetTexture()->GetArrayLayers()) { - return DAWN_VALIDATION_ERROR( - "Currently the OpenGL backend only supports either binding a layer or " - "the entire texture as storage texture."); - } + DAWN_INVALID_IF( + textureViewLayerCount != 1 && + textureViewLayerCount != entry.textureView->GetTexture()->GetArrayLayers(), + "%s binds %u layers. Currently the OpenGL backend only supports either binding " + "1 layer or the all layers (%u) for storage texture.", + entry.textureView, textureViewLayerCount, + entry.textureView->GetTexture()->GetArrayLayers()); } } diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index 788eb9e275..b34935a0c3 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -645,10 +645,10 @@ namespace dawn_native { namespace opengl { auto& dst = copy->destination; Buffer* buffer = ToBackend(src.buffer.Get()); - if (dst.aspect == Aspect::Stencil) { - return DAWN_VALIDATION_ERROR( - "Copies to stencil textures unsupported on OpenGL"); - } + DAWN_INVALID_IF( + dst.aspect == Aspect::Stencil, + "Copies to stencil textures are unsupported on the OpenGL backend."); + ASSERT(dst.aspect == Aspect::Color); buffer->EnsureDataInitialized(); diff --git a/src/dawn_native/opengl/DeviceGL.cpp b/src/dawn_native/opengl/DeviceGL.cpp index 08e544c3c4..db5e06e55a 100644 --- a/src/dawn_native/opengl/DeviceGL.cpp +++ b/src/dawn_native/opengl/DeviceGL.cpp @@ -162,7 +162,7 @@ namespace dawn_native { namespace opengl { Surface* surface, NewSwapChainBase* previousSwapChain, const SwapChainDescriptor* descriptor) { - return DAWN_VALIDATION_ERROR("New swapchains not implemented."); + return DAWN_FORMAT_VALIDATION_ERROR("New swapchains not implemented."); } ResultOrError> Device::CreateTextureImpl(const TextureDescriptor* descriptor) { return AcquireRef(new Texture(this, descriptor)); @@ -181,26 +181,23 @@ namespace dawn_native { namespace opengl { MaybeError Device::ValidateEGLImageCanBeWrapped(const TextureDescriptor* descriptor, ::EGLImage image) { - if (descriptor->dimension != wgpu::TextureDimension::e2D) { - return DAWN_VALIDATION_ERROR("EGLImage texture must be 2D"); - } + DAWN_INVALID_IF(descriptor->dimension != wgpu::TextureDimension::e2D, + "Texture dimension (%s) is not %s.", descriptor->dimension, + wgpu::TextureDimension::e2D); - if (descriptor->usage & - (wgpu::TextureUsage::TextureBinding | wgpu::TextureUsage::StorageBinding)) { - return DAWN_VALIDATION_ERROR("EGLImage texture cannot have sampled or storage usage"); - } + DAWN_INVALID_IF(descriptor->mipLevelCount != 1, "Mip level count (%u) is not 1.", + descriptor->mipLevelCount); - if (descriptor->mipLevelCount != 1) { - return DAWN_VALIDATION_ERROR("EGLImage mip level count must be 1"); - } + DAWN_INVALID_IF(descriptor->size.depthOrArrayLayers != 1, + "Array layer count (%u) is not 1.", descriptor->size.depthOrArrayLayers); - if (descriptor->size.depthOrArrayLayers != 1) { - return DAWN_VALIDATION_ERROR("EGLImage array layer count must be 1"); - } + DAWN_INVALID_IF(descriptor->sampleCount != 1, "Sample count (%u) is not 1.", + descriptor->sampleCount); - if (descriptor->sampleCount != 1) { - return DAWN_VALIDATION_ERROR("EGLImage sample count must be 1"); - } + DAWN_INVALID_IF(descriptor->usage & (wgpu::TextureUsage::TextureBinding | + wgpu::TextureUsage::StorageBinding), + "Texture usage (%s) cannot have %s or %s.", descriptor->usage, + wgpu::TextureUsage::TextureBinding, wgpu::TextureUsage::StorageBinding); return {}; } @@ -229,7 +226,9 @@ namespace dawn_native { namespace opengl { if (textureDescriptor->size.width != static_cast(width) || textureDescriptor->size.height != static_cast(height) || textureDescriptor->size.depthOrArrayLayers != 1) { - ConsumedError(DAWN_VALIDATION_ERROR("EGLImage size doesn't match descriptor")); + ConsumedError(DAWN_FORMAT_VALIDATION_ERROR( + "EGLImage size (width: %u, height: %u, depth: 1) doesn't match descriptor size %s.", + width, height, &textureDescriptor->size)); gl.DeleteTextures(1, &tex); return nullptr; } diff --git a/src/dawn_native/opengl/PipelineGL.cpp b/src/dawn_native/opengl/PipelineGL.cpp index c2fcbe6eac..614a125c17 100644 --- a/src/dawn_native/opengl/PipelineGL.cpp +++ b/src/dawn_native/opengl/PipelineGL.cpp @@ -68,9 +68,8 @@ namespace dawn_native { namespace opengl { if (infoLogLength > 1) { std::vector buffer(infoLogLength); gl.GetShaderInfoLog(shader, infoLogLength, nullptr, &buffer[0]); - std::stringstream ss; - ss << source << "\nProgram compilation failed:\n" << buffer.data(); - return DAWN_VALIDATION_ERROR(ss.str().c_str()); + return DAWN_FORMAT_VALIDATION_ERROR("%s\nProgram compilation failed:\n%s", + source, buffer.data()); } } return shader; @@ -123,9 +122,7 @@ namespace dawn_native { namespace opengl { if (infoLogLength > 1) { std::vector buffer(infoLogLength); gl.GetProgramInfoLog(mProgram, infoLogLength, nullptr, &buffer[0]); - std::stringstream ss; - ss << "Program link failed:\n" << buffer.data(); - return DAWN_VALIDATION_ERROR(ss.str().c_str()); + return DAWN_FORMAT_VALIDATION_ERROR("Program link failed:\n%s", buffer.data()); } } diff --git a/src/dawn_native/opengl/QueueGL.cpp b/src/dawn_native/opengl/QueueGL.cpp index 977f8bf9b8..ec98b64bb4 100644 --- a/src/dawn_native/opengl/QueueGL.cpp +++ b/src/dawn_native/opengl/QueueGL.cpp @@ -56,9 +56,8 @@ namespace dawn_native { namespace opengl { const void* data, const TextureDataLayout& dataLayout, const Extent3D& writeSizePixel) { - if (destination.aspect == wgpu::TextureAspect::StencilOnly) { - return DAWN_VALIDATION_ERROR("Writes to stencil textures unsupported on OpenGL"); - } + DAWN_INVALID_IF(destination.aspect == wgpu::TextureAspect::StencilOnly, + "Writes to stencil textures unsupported on the OpenGL backend."); TextureCopy textureCopy; textureCopy.texture = destination.texture; diff --git a/src/dawn_native/opengl/ShaderModuleGL.cpp b/src/dawn_native/opengl/ShaderModuleGL.cpp index 5019593ba6..414fb79aef 100644 --- a/src/dawn_native/opengl/ShaderModuleGL.cpp +++ b/src/dawn_native/opengl/ShaderModuleGL.cpp @@ -80,29 +80,25 @@ namespace dawn_native { namespace opengl { const spirv_cross::Compiler& compiler, BindingInfoType bindingType, BindingInfoArray* bindings, bool isStorageBuffer = false) -> MaybeError { for (const auto& resource : resources) { - if (!compiler.get_decoration_bitset(resource.id).get(spv::DecorationBinding)) { - return DAWN_VALIDATION_ERROR("No Binding decoration set for resource"); - } + DAWN_INVALID_IF( + !compiler.get_decoration_bitset(resource.id).get(spv::DecorationBinding), + "No Binding decoration set for resource"); - if (!compiler.get_decoration_bitset(resource.id) - .get(spv::DecorationDescriptorSet)) { - return DAWN_VALIDATION_ERROR("No Descriptor Decoration set for resource"); - } + DAWN_INVALID_IF( + !compiler.get_decoration_bitset(resource.id).get(spv::DecorationDescriptorSet), + "No Descriptor Decoration set for resource"); BindingNumber bindingNumber( compiler.get_decoration(resource.id, spv::DecorationBinding)); BindGroupIndex bindGroupIndex( compiler.get_decoration(resource.id, spv::DecorationDescriptorSet)); - if (bindGroupIndex >= kMaxBindGroupsTyped) { - return DAWN_VALIDATION_ERROR("Bind group index over limits in the SPIRV"); - } + DAWN_INVALID_IF(bindGroupIndex >= kMaxBindGroupsTyped, + "Bind group index over limits in the SPIRV"); const auto& it = (*bindings)[bindGroupIndex].emplace(bindingNumber, ShaderBindingInfo{}); - if (!it.second) { - return DAWN_VALIDATION_ERROR("Shader has duplicate bindings"); - } + DAWN_INVALID_IF(!it.second, "Shader has duplicate bindings"); ShaderBindingInfo* info = &it.first->second; info->id = resource.id; @@ -123,17 +119,14 @@ namespace dawn_native { namespace opengl { SpirvBaseTypeToSampleTypeBit(textureComponentType); if (imageType.depth) { - if ((info->texture.compatibleSampleTypes & SampleTypeBit::Float) == 0) { - return DAWN_VALIDATION_ERROR( - "Depth textures must have a float type"); - } + DAWN_INVALID_IF( + (info->texture.compatibleSampleTypes & SampleTypeBit::Float) == 0, + "Depth textures must have a float type"); info->texture.compatibleSampleTypes = SampleTypeBit::Depth; } - if (imageType.ms && imageType.arrayed) { - return DAWN_VALIDATION_ERROR( - "Multisampled array textures aren't supported"); - } + DAWN_INVALID_IF(imageType.ms && imageType.arrayed, + "Multisampled array textures aren't supported"); break; } case BindingInfoType::Buffer: { @@ -162,33 +155,28 @@ namespace dawn_native { namespace opengl { } case BindingInfoType::StorageTexture: { spirv_cross::Bitset flags = compiler.get_decoration_bitset(resource.id); - if (flags.get(spv::DecorationNonReadable)) { - info->storageTexture.access = wgpu::StorageTextureAccess::WriteOnly; - } else { - return DAWN_VALIDATION_ERROR( - "Read-write storage textures are not supported"); - } + DAWN_INVALID_IF(!flags.get(spv::DecorationNonReadable), + "Read-write storage textures are not supported."); + info->storageTexture.access = wgpu::StorageTextureAccess::WriteOnly; spirv_cross::SPIRType::ImageType imageType = compiler.get_type(info->base_type_id).image; wgpu::TextureFormat storageTextureFormat = SpirvImageFormatToTextureFormat(imageType.format); - if (storageTextureFormat == wgpu::TextureFormat::Undefined) { - return DAWN_VALIDATION_ERROR( - "Invalid image format declaration on storage image"); - } + DAWN_INVALID_IF(storageTextureFormat == wgpu::TextureFormat::Undefined, + "Invalid image format declaration on storage image."); + const Format& format = device->GetValidInternalFormat(storageTextureFormat); - if (!format.supportsStorageUsage) { - return DAWN_VALIDATION_ERROR( - "The storage texture format is not supported"); - } - if (imageType.ms) { - return DAWN_VALIDATION_ERROR( - "Multisampled storage textures aren't supported"); - } - if (imageType.depth) { - return DAWN_VALIDATION_ERROR("Depth storage textures aren't supported"); - } + DAWN_INVALID_IF(!format.supportsStorageUsage, + "The storage texture format (%s) is not supported.", + storageTextureFormat); + + DAWN_INVALID_IF(imageType.ms, + "Multisampled storage textures aren't supported."); + + DAWN_INVALID_IF(imageType.depth, + "Depth storage textures aren't supported."); + info->storageTexture.format = storageTextureFormat; info->storageTexture.viewDimension = SpirvDimToTextureViewDimension(imageType.dim, imageType.arrayed); @@ -199,7 +187,7 @@ namespace dawn_native { namespace opengl { break; } case BindingInfoType::ExternalTexture: { - return DAWN_VALIDATION_ERROR("External textures are not supported."); + return DAWN_FORMAT_VALIDATION_ERROR("External textures are not supported."); } } } @@ -264,11 +252,8 @@ namespace dawn_native { namespace opengl { tint::writer::spirv::Options options; options.disable_workgroup_init = GetDevice()->IsToggleEnabled(Toggle::DisableWorkgroupInit); auto result = tint::writer::spirv::Generate(GetTintProgram(), options); - if (!result.success) { - std::ostringstream errorStream; - errorStream << "Generator: " << result.error << std::endl; - return DAWN_VALIDATION_ERROR(errorStream.str().c_str()); - } + DAWN_INVALID_IF(!result.success, "An error occured while generating SPIR-V: %s.", + result.error); DAWN_TRY_ASSIGN(mGLBindings, ReflectShaderUsingSPIRVCross(GetDevice(), result.spirv)); @@ -293,11 +278,8 @@ namespace dawn_native { namespace opengl { tintOptions.disable_workgroup_init = GetDevice()->IsToggleEnabled(Toggle::DisableWorkgroupInit); auto result = tint::writer::spirv::Generate(&program, tintOptions); - if (!result.success) { - std::ostringstream errorStream; - errorStream << "Generator: " << result.error << std::endl; - return DAWN_VALIDATION_ERROR(errorStream.str().c_str()); - } + DAWN_INVALID_IF(!result.success, "An error occured while generating SPIR-V: %s.", + result.error); std::vector spirv = std::move(result.spirv); DAWN_TRY(