From 5490e9aca17eb2f58633b00d823a3fc1e58f2b61 Mon Sep 17 00:00:00 2001 From: Jiawei Shao Date: Wed, 27 Mar 2019 00:01:33 +0000 Subject: [PATCH] Fix clip space on D3D12 and OpenGL The range of the Z-coordinate in clipping volume is [-w, w] on OpenGL, while it is [0, w] on D3D12, Metal and Vulkan. In this patch, the "fixup_clipspace" flag of SPIRV-Cross is enabled on OpenGL backend and disabled on D3D12 backend to unify the behaviour of clip space on all Dawn backends. An end2end test is also added for this fix. This patch also fix a bug when clearing depth stencil attachments on OpenGL backend. Before clearing depth stencil attachments, we should enable depth stencil writing by properly setting depth and stencil masks. We do not need to set the depth and stencil masks back because they will be set again when applying the render pipeline. The newly added test will fail without this fix when running the test together with all the end2ends. BUG=dawn:122 TEST=dawn_end2end_tests Change-Id: I4f50ce3eb1f16d731ee4cffc12a56e17844b4675 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/5860 Reviewed-by: Kai Ninomiya Commit-Queue: Jiawei Shao --- BUILD.gn | 1 + src/dawn_native/d3d12/ShaderModuleD3D12.cpp | 1 - src/dawn_native/opengl/CommandBufferGL.cpp | 17 ++++ src/dawn_native/opengl/ShaderModuleGL.cpp | 6 ++ src/fuzzers/DawnSPIRVCrossGLSLFastFuzzer.cpp | 1 + src/fuzzers/DawnSPIRVCrossHLSLFastFuzzer.cpp | 1 - src/tests/end2end/ClipSpaceTests.cpp | 101 +++++++++++++++++++ 7 files changed, 126 insertions(+), 2 deletions(-) create mode 100644 src/tests/end2end/ClipSpaceTests.cpp diff --git a/BUILD.gn b/BUILD.gn index 196cfe295c..d975e7655f 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -640,6 +640,7 @@ test("dawn_end2end_tests") { "src/tests/end2end/BasicTests.cpp", "src/tests/end2end/BindGroupTests.cpp", "src/tests/end2end/BufferTests.cpp", + "src/tests/end2end/ClipSpaceTests.cpp", "src/tests/end2end/ColorStateTests.cpp", "src/tests/end2end/ComputeCopyStorageBufferTests.cpp", "src/tests/end2end/CopyTests.cpp", diff --git a/src/dawn_native/d3d12/ShaderModuleD3D12.cpp b/src/dawn_native/d3d12/ShaderModuleD3D12.cpp index b9ec960d2c..2973bb244d 100644 --- a/src/dawn_native/d3d12/ShaderModuleD3D12.cpp +++ b/src/dawn_native/d3d12/ShaderModuleD3D12.cpp @@ -37,7 +37,6 @@ namespace dawn_native { namespace d3d12 { // If these options are changed, the values in DawnSPIRVCrossHLSLFastFuzzer.cpp need to be // updated. spirv_cross::CompilerGLSL::Options options_glsl; - options_glsl.vertex.fixup_clipspace = true; options_glsl.vertex.flip_vert_y = true; compiler.set_common_options(options_glsl); diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index 73a8fd67b7..9aaf17fb5e 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -106,6 +106,15 @@ namespace dawn_native { namespace opengl { } } + GLint GetStencilMaskFromStencilFormat(dawn::TextureFormat depthStencilFormat) { + switch (depthStencilFormat) { + case dawn::TextureFormat::D32FloatS8Uint: + return 0xFF; + default: + UNREACHABLE(); + } + } + // Push constants are implemented using OpenGL uniforms, however they aren't part of the // global OpenGL state but are part of the program state instead. This means that we have to // reapply push constants on pipeline change. @@ -610,6 +619,14 @@ namespace dawn_native { namespace opengl { (attachmentInfo.depthLoadOp == dawn::LoadOp::Clear); bool doStencilClear = TextureFormatHasStencil(attachmentFormat) && (attachmentInfo.stencilLoadOp == dawn::LoadOp::Clear); + + if (doDepthClear) { + glDepthMask(GL_TRUE); + } + if (doStencilClear) { + glStencilMask(GetStencilMaskFromStencilFormat(attachmentFormat)); + } + if (doDepthClear && doStencilClear) { glClearBufferfi(GL_DEPTH_STENCIL, 0, attachmentInfo.clearDepth, attachmentInfo.clearStencil); diff --git a/src/dawn_native/opengl/ShaderModuleGL.cpp b/src/dawn_native/opengl/ShaderModuleGL.cpp index ab24940bd0..ae3d1f9ba2 100644 --- a/src/dawn_native/opengl/ShaderModuleGL.cpp +++ b/src/dawn_native/opengl/ShaderModuleGL.cpp @@ -54,6 +54,12 @@ namespace dawn_native { namespace opengl { // updated. spirv_cross::CompilerGLSL::Options options; + // The range of Z-coordinate in the clipping volume of OpenGL is [-w, w], while it is [0, w] + // in D3D12, Metal and Vulkan, so we should normalize it in shaders in all backends. + // See the documentation of spirv_cross::CompilerGLSL::Options::vertex::fixup_clipspace for + // more details. + options.vertex.fixup_clipspace = true; + // TODO(cwallez@chromium.org): discover the backing context version and use that. #if defined(DAWN_PLATFORM_APPLE) options.version = 410; diff --git a/src/fuzzers/DawnSPIRVCrossGLSLFastFuzzer.cpp b/src/fuzzers/DawnSPIRVCrossGLSLFastFuzzer.cpp index 2a36cf1d41..414f23042e 100644 --- a/src/fuzzers/DawnSPIRVCrossGLSLFastFuzzer.cpp +++ b/src/fuzzers/DawnSPIRVCrossGLSLFastFuzzer.cpp @@ -29,6 +29,7 @@ namespace { // Using the options that are used by Dawn, they appear in ShaderModuleGL.cpp shaderc_spvc::CompileOptions options; options.SetOutputLanguageVersion(440); + options.SetFixupClipspace(true); compiler.CompileSpvToGlsl(input.data(), input.size(), options); }); diff --git a/src/fuzzers/DawnSPIRVCrossHLSLFastFuzzer.cpp b/src/fuzzers/DawnSPIRVCrossHLSLFastFuzzer.cpp index 6c75b39608..3a3953727b 100644 --- a/src/fuzzers/DawnSPIRVCrossHLSLFastFuzzer.cpp +++ b/src/fuzzers/DawnSPIRVCrossHLSLFastFuzzer.cpp @@ -30,7 +30,6 @@ namespace { shaderc_spvc::CompileOptions options; // Using the options that are used by Dawn, they appear in ShaderModuleD3D12.cpp - options.SetFixupClipspace(true); options.SetFlipVertY(true); options.SetShaderModel(51); compiler.CompileSpvToHlsl(input.data(), input.size(), options); diff --git a/src/tests/end2end/ClipSpaceTests.cpp b/src/tests/end2end/ClipSpaceTests.cpp new file mode 100644 index 0000000000..bd5e9f194c --- /dev/null +++ b/src/tests/end2end/ClipSpaceTests.cpp @@ -0,0 +1,101 @@ +// Copyright 2019 The Dawn Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "tests/DawnTest.h" + +#include "utils/ComboRenderPipelineDescriptor.h" +#include "utils/DawnHelpers.h" + +class ClipSpaceTest : public DawnTest { + protected: + dawn::RenderPipeline CreatePipelineForTest() { + utils::ComboRenderPipelineDescriptor pipelineDescriptor(device); + + // Draw two triangles: + // 1. The depth value of the top-left one is >= 0.5 + // 2. The depth value of the bottom-right one is <= 0.5 + const char* vs = + R"(#version 450 + const vec3 pos[6] = vec3[6](vec3(-1.0f, -1.0f, 1.0f), + vec3(-1.0f, 1.0f, 0.5f), + vec3( 1.0f, -1.0f, 0.5f), + vec3( 1.0f, -1.0f, 0.5f), + vec3(-1.0f, 1.0f, 0.5f), + vec3( 1.0f, 1.0f, 0.0f)); + void main() { + gl_Position = vec4(pos[gl_VertexIndex], 1.0); + })"; + pipelineDescriptor.cVertexStage.module = + utils::CreateShaderModule(device, dawn::ShaderStage::Vertex, vs); + + const char* fs = + "#version 450\n" + "layout(location = 0) out vec4 fragColor;" + "void main() {\n" + " fragColor = vec4(1.0, 0.0, 0.0, 1.0);\n" + "}\n"; + pipelineDescriptor.cFragmentStage.module = + utils::CreateShaderModule(device, dawn::ShaderStage::Fragment, fs); + + pipelineDescriptor.cDepthStencilState.depthCompare = dawn::CompareFunction::LessEqual; + pipelineDescriptor.depthStencilState = &pipelineDescriptor.cDepthStencilState; + + return device.CreateRenderPipeline(&pipelineDescriptor); + } + + dawn::Texture Create2DTextureForTest(dawn::TextureFormat format) { + dawn::TextureDescriptor textureDescriptor; + textureDescriptor.dimension = dawn::TextureDimension::e2D; + textureDescriptor.format = format; + textureDescriptor.usage = + dawn::TextureUsageBit::OutputAttachment | dawn::TextureUsageBit::TransferSrc; + textureDescriptor.arrayLayerCount = 1; + textureDescriptor.mipLevelCount = 1; + textureDescriptor.sampleCount = 1; + textureDescriptor.size = {kSize, kSize, 1}; + return device.CreateTexture(&textureDescriptor); + } + + static constexpr uint32_t kSize = 4; +}; + +// Test that the clip space is correctly configured. +TEST_P(ClipSpaceTest, ClipSpace) { + dawn::Texture colorTexture = Create2DTextureForTest(dawn::TextureFormat::R8G8B8A8Unorm); + dawn::Texture depthStencilTexture = Create2DTextureForTest(dawn::TextureFormat::D32FloatS8Uint); + + utils::ComboRenderPassDescriptor renderPassDescriptor( + {colorTexture.CreateDefaultTextureView()}, depthStencilTexture.CreateDefaultTextureView()); + renderPassDescriptor.cColorAttachmentsInfoPtr[0]->clearColor = {0.0, 1.0, 0.0, 1.0}; + renderPassDescriptor.cColorAttachmentsInfoPtr[0]->loadOp = dawn::LoadOp::Clear; + + // Clear the depth stencil attachment to 0.5f, so only the bottom-right triangle should be + // drawn. + renderPassDescriptor.cDepthStencilAttachmentInfo.clearDepth = 0.5f; + renderPassDescriptor.cDepthStencilAttachmentInfo.depthLoadOp = dawn::LoadOp::Clear; + + dawn::CommandEncoder commandEncoder = device.CreateCommandEncoder(); + dawn::RenderPassEncoder renderPass = commandEncoder.BeginRenderPass(&renderPassDescriptor); + renderPass.SetPipeline(CreatePipelineForTest()); + renderPass.Draw(6, 1, 0, 0); + renderPass.EndPass(); + dawn::CommandBuffer commandBuffer = commandEncoder.Finish(); + dawn::Queue queue = device.CreateQueue(); + queue.Submit(1, &commandBuffer); + + EXPECT_PIXEL_RGBA8_EQ(RGBA8(255, 0, 0, 255), colorTexture, kSize - 1, kSize - 1); + EXPECT_PIXEL_RGBA8_EQ(RGBA8(0, 255, 0, 255), colorTexture, 0, 0); +} + +DAWN_INSTANTIATE_TEST(ClipSpaceTest, D3D12Backend, MetalBackend, OpenGLBackend, VulkanBackend);