diff --git a/src/backend/Framebuffer.cpp b/src/backend/Framebuffer.cpp index 5f9cff8ce1..ba832902af 100644 --- a/src/backend/Framebuffer.cpp +++ b/src/backend/Framebuffer.cpp @@ -103,19 +103,10 @@ namespace backend { return nullptr; } - // TODO(kainino@chromium.org): Remove this flag when the - // null=backbuffer hack is removed. Then, using null texture views can - // be completely disallowed. - bool usingBackbufferHack = false; for (auto& textureView : textureViews) { if (!textureView) { - if (usingBackbufferHack) { - // TODO(kainino@chromium.org) update this too - HandleError("Framebuffer has more than one null attachment"); - return nullptr; - } - usingBackbufferHack = true; - continue; + HandleError("Framebuffer attachment not set"); + return nullptr; } // TODO(cwallez@chromium.org): Adjust for the mip-level once that is supported. diff --git a/src/backend/RenderPass.cpp b/src/backend/RenderPass.cpp index b2e04a3dd5..3746b252f5 100644 --- a/src/backend/RenderPass.cpp +++ b/src/backend/RenderPass.cpp @@ -94,6 +94,23 @@ namespace backend { } } + for (const auto& subpass : subpasses) { + for (unsigned int location : IterateBitSet(subpass.colorAttachmentsSet)) { + uint32_t slot = subpass.colorAttachments[location]; + if (TextureFormatHasDepthOrStencil(attachments[slot].format)) { + HandleError("Render pass color attachment is not of a color format"); + return nullptr; + } + } + if (subpass.depthStencilAttachmentSet) { + uint32_t slot = subpass.depthStencilAttachment; + if (!TextureFormatHasDepthOrStencil(attachments[slot].format)) { + HandleError("Render pass depth/stencil attachment is not of a depth/stencil format"); + return nullptr; + } + } + } + return device->CreateRenderPass(this); } diff --git a/src/backend/opengl/CommandBufferGL.cpp b/src/backend/opengl/CommandBufferGL.cpp index 9d18e3f078..1b9a7759be 100644 --- a/src/backend/opengl/CommandBufferGL.cpp +++ b/src/backend/opengl/CommandBufferGL.cpp @@ -97,6 +97,11 @@ namespace opengl { { commands.NextCommand(); + // TODO(kainino@chromium.org): This is added to possibly + // work around an issue seen on Windows/Intel. It should + // break any feedback loop before the clears, even if + // there shouldn't be any negative effects from this. + // Investigate whether it's actually needed. glBindFramebuffer(GL_READ_FRAMEBUFFER, 0); // TODO(kainino@chromium.org): possible future // optimization: create these framebuffers at diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt index 02a469bf8c..9137e9571f 100644 --- a/src/tests/CMakeLists.txt +++ b/src/tests/CMakeLists.txt @@ -75,6 +75,7 @@ add_executable(nxt_end2end_tests ${END2END_TESTS_DIR}/DepthStencilStateTests.cpp ${END2END_TESTS_DIR}/InputStateTests.cpp ${END2END_TESTS_DIR}/PrimitiveTopologyTests.cpp + ${END2END_TESTS_DIR}/RenderPassLoadOpTests.cpp ${TESTS_DIR}/End2EndTestsMain.cpp ${TESTS_DIR}/NXTTest.cpp ${TESTS_DIR}/NXTTest.h diff --git a/src/tests/NXTTest.cpp b/src/tests/NXTTest.cpp index 94f4a4ff21..411eddf7ae 100644 --- a/src/tests/NXTTest.cpp +++ b/src/tests/NXTTest.cpp @@ -18,6 +18,7 @@ #include "common/Constants.h" #include "common/Math.h" #include "utils/BackendBinding.h" +#include "utils/NXTHelpers.h" #include "utils/SystemUtils.h" #include "GLFW/glfw3.h" diff --git a/src/tests/NXTTest.h b/src/tests/NXTTest.h index 8e12068299..f188dfdf23 100644 --- a/src/tests/NXTTest.h +++ b/src/tests/NXTTest.h @@ -31,7 +31,7 @@ AddTextureExpectation(__FILE__, __LINE__, texture, x, y, 1, 1, 0, sizeof(RGBA8), new detail::ExpectEq(expected)) #define EXPECT_TEXTURE_RGBA8_EQ(expected, texture, x, y, width, height, level) \ - AddTextureExpectation(__FILE__, __LINE__, texture, x, y, width, height, level, sizeof(RGBA8), new detail::ExpectEq(expected, width * height)) + AddTextureExpectation(__FILE__, __LINE__, texture, x, y, width, height, level, sizeof(RGBA8), new detail::ExpectEq(expected, (width) * (height))) struct RGBA8 { constexpr RGBA8() : RGBA8(0,0,0,0) {} diff --git a/src/tests/end2end/RenderPassLoadOpTests.cpp b/src/tests/end2end/RenderPassLoadOpTests.cpp new file mode 100644 index 0000000000..970c783c16 --- /dev/null +++ b/src/tests/end2end/RenderPassLoadOpTests.cpp @@ -0,0 +1,188 @@ +// Copyright 2017 The NXT 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/NXTTest.h" + +#include "utils/NXTHelpers.h" + +#include + +constexpr static unsigned int kRTSize = 16; + +class DrawQuad { + public: + DrawQuad() {} + DrawQuad(nxt::Device* device, const char* vsSource, const char* fsSource) + : device(device) { + vsModule = utils::CreateShaderModule(*device, nxt::ShaderStage::Vertex, vsSource); + fsModule = utils::CreateShaderModule(*device, nxt::ShaderStage::Fragment, fsSource); + + pipelineLayout = device->CreatePipelineLayoutBuilder() + .GetResult(); + } + + void Draw(const nxt::RenderPass& renderpass, nxt::CommandBufferBuilder* builder) { + auto renderPipeline = device->CreateRenderPipelineBuilder() + .SetSubpass(renderpass, 0) + .SetLayout(pipelineLayout) + .SetStage(nxt::ShaderStage::Vertex, vsModule, "main") + .SetStage(nxt::ShaderStage::Fragment, fsModule, "main") + .GetResult(); + + builder->SetRenderPipeline(renderPipeline); + builder->DrawArrays(6, 1, 0, 0); + } + + private: + nxt::Device* device = nullptr; + nxt::ShaderModule vsModule = {}; + nxt::ShaderModule fsModule = {}; + nxt::PipelineLayout pipelineLayout = {}; +}; + +class RenderPassLoadOpTests : public NXTTest { + protected: + void SetUp() override { + NXTTest::SetUp(); + + renderTarget = device.CreateTextureBuilder() + .SetDimension(nxt::TextureDimension::e2D) + .SetExtent(kRTSize, kRTSize, 1) + .SetFormat(nxt::TextureFormat::R8G8B8A8Unorm) + .SetMipLevels(1) + .SetAllowedUsage(nxt::TextureUsageBit::OutputAttachment | nxt::TextureUsageBit::TransferSrc) + .SetInitialUsage(nxt::TextureUsageBit::OutputAttachment) + .GetResult(); + + renderTargetView = renderTarget.CreateTextureViewBuilder().GetResult(); + + RGBA8 zero(0, 0, 0, 0); + std::fill(expectZero.begin(), expectZero.end(), zero); + + RGBA8 green(0, 255, 0, 255); + std::fill(expectGreen.begin(), expectGreen.end(), green); + + RGBA8 blue(0, 0, 255, 255); + std::fill(expectBlue.begin(), expectBlue.end(), blue); + + // draws a blue quad on the right half of the screen + const char* vsSource = R"( + #version 450 + void main() { + const vec2 pos[6] = vec2[6]( + vec2(0, -1), vec2(1, -1), vec2(0, 1), + vec2(0, 1), vec2(1, -1), vec2(1, 1)); + gl_Position = vec4(pos[gl_VertexIndex], 0.f, 1.f); + } + )"; + const char* fsSource = R"( + #version 450 + out vec4 color; + void main() { + color = vec4(0.f, 0.f, 1.f, 1.f); + } + )"; + blueQuad = DrawQuad(&device, vsSource, fsSource); + } + + nxt::Texture renderTarget; + nxt::TextureView renderTargetView; + + std::array expectZero; + std::array expectGreen; + std::array expectBlue; + + DrawQuad blueQuad = {}; +}; + +// Tests clearing, loading, and drawing into color attachments +TEST_P(RenderPassLoadOpTests, ColorClearThenLoadAndDraw) { + if (IsOpenGL()) { + // TODO(kainino@chromium.org): currently fails on OpenGL backend + return; + } + + // Part 1: clear once, check to make sure it's cleared + + auto renderpass1 = device.CreateRenderPassBuilder() + .SetAttachmentCount(1) + .SetSubpassCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + .AttachmentSetColorLoadOp(0, nxt::LoadOp::Clear) + .SubpassSetColorAttachment(0, 0, 0) + .GetResult(); + auto framebuffer1 = device.CreateFramebufferBuilder() + .SetRenderPass(renderpass1) + .SetDimensions(kRTSize, kRTSize) + .SetAttachment(0, renderTargetView) + .GetResult(); + + auto commands1 = device.CreateCommandBufferBuilder() + .BeginRenderPass(renderpass1, framebuffer1) + .BeginRenderSubpass() + // Clear should occur implicitly + // Store should occur implicitly + .EndRenderSubpass() + .EndRenderPass() + .GetResult(); + + framebuffer1.AttachmentSetClearColor(0, 0.0f, 0.0f, 0.0f, 0.0f); // zero + queue.Submit(1, &commands1); + // Cleared to zero + EXPECT_TEXTURE_RGBA8_EQ(expectZero.data(), renderTarget, 0, 0, kRTSize, kRTSize, 0); + + framebuffer1.AttachmentSetClearColor(0, 0.0f, 1.0f, 0.0f, 1.0f); // green + queue.Submit(1, &commands1); + // Now cleared to green + EXPECT_TEXTURE_RGBA8_EQ(expectGreen.data(), renderTarget, 0, 0, kRTSize, kRTSize, 0); + + // Part 2: draw a blue quad into the right half of the render target, and check result + + auto renderpass2 = device.CreateRenderPassBuilder() + .SetAttachmentCount(1) + .SetSubpassCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + .AttachmentSetColorLoadOp(0, nxt::LoadOp::Load) + .SubpassSetColorAttachment(0, 0, 0) + .GetResult(); + auto framebuffer2 = device.CreateFramebufferBuilder() + .SetRenderPass(renderpass2) + .SetDimensions(kRTSize, kRTSize) + .SetAttachment(0, renderTargetView) + .GetResult(); + framebuffer2.AttachmentSetClearColor(0, 1.0f, 0.0f, 0.0f, 1.0f); // red + + nxt::CommandBuffer commands2; + { + auto builder = device.CreateCommandBufferBuilder() + .BeginRenderPass(renderpass2, framebuffer2) + .BeginRenderSubpass() + // Clear should occur implicitly + .Clone(); + blueQuad.Draw(renderpass2, &builder); + commands2 = builder + // Store should occur implicitly + .EndRenderSubpass() + .EndRenderPass() + .GetResult(); + } + + queue.Submit(1, &commands2); + // Left half should still be green + EXPECT_TEXTURE_RGBA8_EQ(expectGreen.data(), renderTarget, 0, 0, kRTSize / 2, kRTSize, 0); + // Right half should now be blue + EXPECT_TEXTURE_RGBA8_EQ(expectBlue.data(), renderTarget, kRTSize / 2, 0, kRTSize / 2, kRTSize, 0); +} + +NXT_INSTANTIATE_TEST(RenderPassLoadOpTests, D3D12Backend, MetalBackend, OpenGLBackend) diff --git a/src/tests/unittests/validation/FramebufferValidationTests.cpp b/src/tests/unittests/validation/FramebufferValidationTests.cpp index 255c7904fe..2429a9c772 100644 --- a/src/tests/unittests/validation/FramebufferValidationTests.cpp +++ b/src/tests/unittests/validation/FramebufferValidationTests.cpp @@ -29,6 +29,21 @@ class FramebufferValidationTest : public ValidationTest { return attachment.CreateTextureViewBuilder() .GetResult(); } + + nxt::Framebuffer CreateFramebufferWithAttachment(uint32_t width, uint32_t height, nxt::TextureFormat format) { + auto renderpass = AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, format) + .SetSubpassCount(1) + .SubpassSetColorAttachment(0, 0, 0) + .GetResult(); + nxt::TextureView attachment = Create2DAttachment(width, height, format); + return AssertWillBeSuccess(device.CreateFramebufferBuilder()) + .SetRenderPass(renderpass) + .SetAttachment(0, attachment) + .SetDimensions(100, 100) + .GetResult(); + } }; // Test for an empty framebuffer builder @@ -73,12 +88,29 @@ TEST_F(FramebufferValidationTest, BasicWithEmptyAttachment) { .SetSubpassCount(1) .SubpassSetColorAttachment(0, 0, 0) .GetResult(); - auto framebuffer = AssertWillBeSuccess(device.CreateFramebufferBuilder()) + AssertWillBeError(device.CreateFramebufferBuilder()) .SetRenderPass(renderpass) .SetDimensions(100, 100) .GetResult(); } +// Test for a basic framebuffer with one attachment +TEST_F(FramebufferValidationTest, BasicWithOneAttachment) { + CreateFramebufferWithAttachment(100, 100, nxt::TextureFormat::R8G8B8A8Unorm); +} + +// Tests for setting clear values +TEST_F(FramebufferValidationTest, ClearValues) { + auto framebuffer = CreateFramebufferWithAttachment(100, 100, nxt::TextureFormat::R8G8B8A8Unorm); + + // Set clear color value for attachment 0 + framebuffer.AttachmentSetClearColor(0, 0.0f, 0.0f, 0.0f, 0.0f); + // Set clear depth/stencil values for attachment 0 - ok, but ignored, since it's a color attachment + framebuffer.AttachmentSetClearDepthStencil(0, 0.0f, 0); + // Set clear color value for attachment 1 - should fail + ASSERT_DEVICE_ERROR(framebuffer.AttachmentSetClearColor(1, 0.0f, 0.0f, 0.0f, 0.0f)); +} + // Check validation that the attachment size must be the same as the framebuffer size. // TODO(cwallez@chromium.org): Investigate this constraint more, for example Vulkan requires // that the attachment sizes are *at least* the framebuffer size. @@ -121,3 +153,75 @@ TEST_F(FramebufferValidationTest, AttachmentSizeMatchFramebufferSize) { // TODO(cwallez@chromium.org): also test with a mismatches depth / stencil } + +TEST_F(FramebufferValidationTest, AttachmentFormatMatchTextureFormat) { + // Control case: attach color attachment to color slot + { + auto renderpass = AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + .SetSubpassCount(1) + .SubpassSetColorAttachment(0, 0, 0) + .GetResult(); + + nxt::TextureView attachment = Create2DAttachment(100, 100, nxt::TextureFormat::R8G8B8A8Unorm); + auto framebuffer = AssertWillBeSuccess(device.CreateFramebufferBuilder()) + .SetRenderPass(renderpass) + .SetAttachment(0, attachment) + .SetDimensions(100, 100) + .GetResult(); + } + + // Error: attach color attachment to depth slot, setting the attachment format first + { + auto renderpass = AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetAttachmentCount(1) + .SetSubpassCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::D32FloatS8Uint) + .SubpassSetDepthStencilAttachment(0, 0) + .GetResult(); + + nxt::TextureView attachment = Create2DAttachment(100, 100, nxt::TextureFormat::R8G8B8A8Unorm); + auto framebuffer = AssertWillBeError(device.CreateFramebufferBuilder()) + .SetRenderPass(renderpass) + .SetAttachment(0, attachment) + .SetDimensions(100, 100) + .GetResult(); + } + + // Error: attach color attachment to depth slot, but setting the attachment format last + { + auto renderpass = AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .SubpassSetDepthStencilAttachment(0, 0) + .AttachmentSetFormat(0, nxt::TextureFormat::D32FloatS8Uint) + .GetResult(); + + nxt::TextureView attachment = Create2DAttachment(100, 100, nxt::TextureFormat::R8G8B8A8Unorm); + auto framebuffer = AssertWillBeError(device.CreateFramebufferBuilder()) + .SetRenderPass(renderpass) + .SetAttachment(0, attachment) + .SetDimensions(100, 100) + .GetResult(); + } + + // Error: attach depth texture to color slot + { + auto renderpass = AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + .SetSubpassCount(1) + .SubpassSetColorAttachment(0, 0, 0) + .GetResult(); + + nxt::TextureView attachment = Create2DAttachment(100, 100, nxt::TextureFormat::D32FloatS8Uint); + auto framebuffer = AssertWillBeError(device.CreateFramebufferBuilder()) + .SetRenderPass(renderpass) + .SetAttachment(0, attachment) + .SetDimensions(100, 100) + .GetResult(); + } + + // TODO(kainino@chromium.org): also check attachment samples, etc. +} diff --git a/src/tests/unittests/validation/RenderPassValidationTests.cpp b/src/tests/unittests/validation/RenderPassValidationTests.cpp index 071d58b8c3..cf19e2d6ee 100644 --- a/src/tests/unittests/validation/RenderPassValidationTests.cpp +++ b/src/tests/unittests/validation/RenderPassValidationTests.cpp @@ -37,6 +37,123 @@ TEST_F(RenderPassValidationTest, OneSubpassOneAttachment) { .SetSubpassCount(1) .SetAttachmentCount(1) .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + // without a load op + .SubpassSetColorAttachment(0, 0, 0) + .GetResult(); +} + +// Tests for setting attachment load ops +TEST_F(RenderPassValidationTest, AttachmentLoadOps) { + AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + // with a load op + .AttachmentSetColorLoadOp(0, nxt::LoadOp::Clear) + .SubpassSetColorAttachment(0, 0, 0) + .GetResult(); + + AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + // with a load op of the wrong type - this is okay, just ignored + .AttachmentSetDepthStencilLoadOps(0, nxt::LoadOp::Clear, nxt::LoadOp::Clear) + .SubpassSetColorAttachment(0, 0, 0) + .GetResult(); +} + +// Test for attachment slot arguments out of bounds +TEST_F(RenderPassValidationTest, AttachmentOutOfBounds) { + // Control case + AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + .GetResult(); + + AssertWillBeError(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + // Test AttachmentSetFormat slot out of bounds + .AttachmentSetFormat(1, nxt::TextureFormat::R8G8B8A8Unorm) + .GetResult(); + + AssertWillBeError(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + // Test AttachmentSetColorLoadOp slot out of bounds + .AttachmentSetColorLoadOp(1, nxt::LoadOp::Clear) + .GetResult(); + + AssertWillBeError(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + // Test AttachmentSetDepthStencilLoadOps slot out of bounds + .AttachmentSetDepthStencilLoadOps(1, nxt::LoadOp::Clear, nxt::LoadOp::Clear) + .GetResult(); + + AssertWillBeError(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + // Test SubpassSetColorAttachment attachment slot out of bounds + .SubpassSetColorAttachment(0, 0, 1) + .GetResult(); + + AssertWillBeError(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + // Test SubpassSetDepthStencilAttachment attachment slot out of bounds + .SubpassSetDepthStencilAttachment(0, 1) + .GetResult(); +} + +// Test for subpass arguments out of bounds +TEST_F(RenderPassValidationTest, SubpassOutOfBounds) { + // Control case + AssertWillBeSuccess(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + .SubpassSetColorAttachment(0, 0, 0) + .GetResult(); + + AssertWillBeError(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + .SubpassSetColorAttachment(0, 0, 0) + // Test SubpassSetColorAttachment subpass out of bounds + .SubpassSetColorAttachment(1, 0, 0) + .GetResult(); + + AssertWillBeError(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::D32FloatS8Uint) + // Test SubpassSetDepthStencilAttachment subpass out of bounds + .SubpassSetDepthStencilAttachment(1, 0) + .GetResult(); +} + +// Test attaching depth/stencil textures to color attachments and vice versa +TEST_F(RenderPassValidationTest, SubpassAttachmentWrongAspect) { + AssertWillBeError(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::R8G8B8A8Unorm) + .SubpassSetDepthStencilAttachment(0, 0) + .GetResult(); + + AssertWillBeError(device.CreateRenderPassBuilder()) + .SetSubpassCount(1) + .SetAttachmentCount(1) + .AttachmentSetFormat(0, nxt::TextureFormat::D32FloatS8Uint) .SubpassSetColorAttachment(0, 0, 0) .GetResult(); }