From 87098b0e1e58608272cf1a94951138f4c7b19a34 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Tue, 19 Sep 2023 15:14:47 -0700 Subject: [PATCH 1/3] [Impeller] Fix validation errors in RendererTest. Vulkan validation was tripping on the fact that renderer tests were rendering to the root render pass. This root render pass doesn't contain a stencil buffer. However, `MakeDefaultPipelineDescriptor` assumes a stencil and color attachment. The other backends are resilient to this mismatch since there is no compat render pass created upfront. But Vulkan was sad. This should only happen in the low level tests. In the higher levels of the framework, we have variants that make sure there is a pipeline pass and render pass compatibility. Fixes validations of the kind: ``` --- Vulkan Debug Report ---------------------------------------- | Severity: Error | Type: { Validation } | ID Name: VUID-vkCmdDraw-renderPass-02684 | ID Number: 1349015333 | Queue Breadcrumbs: [NONE] | CMD Buffer Breadcrumbs: [NONE] | Related Objects: RenderPass [16305153808034431137] [Playground Render Pass], RenderPass [18100546345029861533] [Compat Render Pass: BoxFade Pipeline] | Trigger: Validation Error: [ VUID-vkCmdDraw-renderPass-02684 ] Object 0: handle = 0xe2478b00000000a1, name = Playground Render Pass, type = VK_OBJECT_TYPE_RENDER_PASS; Object 1: handle = 0xfb320f000000009d, name = Compat Render Pass: BoxFade Pipeline, type = VK_OBJECT_TYPE_RENDER_PASS; | MessageID = 0x50685725 | vkCmdDraw: RenderPasses incompatible between active render pass w/ VkRenderPass 0xe2478b00000000a1[Playground Render Pass] and pipeline state object w/ VkRenderPass 0xfb320f000000009d[Compat Render Pass: BoxFade Pipeline] Attachment 4294967295 is not compatible with 1: The first is unused while the second is not.. The Vulkan spec states: The current render pass must be compatible with the renderPass member of the VkGraphicsPipelineCreateInfo structure specified when creating the VkPipeline bound to VK_PIPELINE_BIND_POINT_GRAPHICS (https://vulkan.lunarg.com/doc/view/1.3.224.1/mac/1.3-extensions/vkspec.html#VUID-vkCmdDraw-renderPass-02684) ----------------------------------------------------------------- ``` --- .../backend/vulkan/pipeline_library_vk.cc | 15 ++++++++++++--- impeller/renderer/renderer_unittests.cc | 14 +++++++++++++- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc index b47c9d57237ac..075574e718de2 100644 --- a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc +++ b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc @@ -75,8 +75,9 @@ static vk::AttachmentDescription CreatePlaceholderAttachmentDescription( /// spec: /// https://www.khronos.org/registry/vulkan/specs/1.3-extensions/html/chap8.html#renderpass-compatibility /// -static vk::UniqueRenderPass CreateRenderPass(const vk::Device& device, - const PipelineDescriptor& desc) { +static vk::UniqueRenderPass CreateCompatRenderPassForPipeline( + const vk::Device& device, + const PipelineDescriptor& desc) { std::vector attachments; std::vector color_refs; @@ -128,6 +129,13 @@ static vk::UniqueRenderPass CreateRenderPass(const vk::Device& device, return {}; } + // This pass is not used with the render pass. It is only necessary to tell + // Vulkan the expected render pass layout. The acutal pass will be create + // later during render pass setup and will need to be compatible with this + // one. + ContextVK::SetDebugName(device, pass.get(), + "Compat Render Pass: " + desc.GetLabel()); + return std::move(pass); } @@ -332,7 +340,8 @@ std::unique_ptr PipelineLibraryVK::CreatePipeline( return nullptr; } - auto render_pass = CreateRenderPass(strong_device->GetDevice(), desc); + auto render_pass = + CreateCompatRenderPassForPipeline(strong_device->GetDevice(), desc); if (render_pass) { pipeline_info.setBasePipelineHandle(VK_NULL_HANDLE); pipeline_info.setSubpass(0); diff --git a/impeller/renderer/renderer_unittests.cc b/impeller/renderer/renderer_unittests.cc index 575178f229bf2..48772d09198e1 100644 --- a/impeller/renderer/renderer_unittests.cc +++ b/impeller/renderer/renderer_unittests.cc @@ -58,6 +58,7 @@ TEST_P(RendererTest, CanCreateBoxPrimitive) { auto desc = BoxPipelineBuilder::MakeDefaultPipelineDescriptor(*context); ASSERT_TRUE(desc.has_value()); desc->SetSampleCount(SampleCount::kCount4); + desc->SetStencilAttachmentDescriptors(std::nullopt); // Vertex buffer. VertexBufferBuilder vertex_builder; @@ -130,6 +131,7 @@ TEST_P(RendererTest, CanRenderPerspectiveCube) { desc->SetCullMode(CullMode::kBackFace); desc->SetWindingOrder(WindingOrder::kCounterClockwise); desc->SetSampleCount(SampleCount::kCount4); + desc->SetStencilAttachmentDescriptors(std::nullopt); auto pipeline = context->GetPipelineLibrary()->GetPipeline(std::move(desc)).Get(); ASSERT_TRUE(pipeline); @@ -219,6 +221,7 @@ TEST_P(RendererTest, CanRenderMultiplePrimitives) { auto desc = BoxPipelineBuilder::MakeDefaultPipelineDescriptor(*context); ASSERT_TRUE(desc.has_value()); desc->SetSampleCount(SampleCount::kCount4); + desc->SetStencilAttachmentDescriptors(std::nullopt); auto box_pipeline = context->GetPipelineLibrary()->GetPipeline(std::move(desc)).Get(); ASSERT_TRUE(box_pipeline); @@ -420,7 +423,9 @@ TEST_P(RendererTest, CanRenderInstanced) { ->GetPipelineLibrary() ->GetPipeline(PipelineBuilder::MakeDefaultPipelineDescriptor( *GetContext()) - ->SetSampleCount(SampleCount::kCount4)) + ->SetSampleCount(SampleCount::kCount4) + .SetStencilAttachmentDescriptors(std::nullopt)) + .Get(); ASSERT_TRUE(pipeline && pipeline->IsValid()); @@ -459,6 +464,7 @@ TEST_P(RendererTest, CanBlitTextureToTexture) { auto desc = PipelineBuilder::MakeDefaultPipelineDescriptor(*context); ASSERT_TRUE(desc.has_value()); desc->SetSampleCount(SampleCount::kCount4); + desc->SetStencilAttachmentDescriptors(std::nullopt); auto mipmaps_pipeline = context->GetPipelineLibrary()->GetPipeline(std::move(desc)).Get(); ASSERT_TRUE(mipmaps_pipeline); @@ -571,6 +577,7 @@ TEST_P(RendererTest, CanBlitTextureToBuffer) { auto desc = PipelineBuilder::MakeDefaultPipelineDescriptor(*context); ASSERT_TRUE(desc.has_value()); desc->SetSampleCount(SampleCount::kCount4); + desc->SetStencilAttachmentDescriptors(std::nullopt); auto mipmaps_pipeline = context->GetPipelineLibrary()->GetPipeline(std::move(desc)).Get(); ASSERT_TRUE(mipmaps_pipeline); @@ -702,6 +709,7 @@ TEST_P(RendererTest, CanGenerateMipmaps) { auto desc = PipelineBuilder::MakeDefaultPipelineDescriptor(*context); ASSERT_TRUE(desc.has_value()); desc->SetSampleCount(SampleCount::kCount4); + desc->SetStencilAttachmentDescriptors(std::nullopt); auto mipmaps_pipeline = context->GetPipelineLibrary()->GetPipeline(std::move(desc)).Get(); ASSERT_TRUE(mipmaps_pipeline); @@ -818,6 +826,7 @@ TEST_P(RendererTest, TheImpeller) { PipelineBuilder::MakeDefaultPipelineDescriptor(*context); ASSERT_TRUE(pipeline_descriptor.has_value()); pipeline_descriptor->SetSampleCount(SampleCount::kCount4); + pipeline_descriptor->SetStencilAttachmentDescriptors(std::nullopt); auto pipeline = context->GetPipelineLibrary()->GetPipeline(pipeline_descriptor).Get(); ASSERT_TRUE(pipeline && pipeline->IsValid()); @@ -878,6 +887,7 @@ TEST_P(RendererTest, ArrayUniforms) { PipelineBuilder::MakeDefaultPipelineDescriptor(*context); ASSERT_TRUE(pipeline_descriptor.has_value()); pipeline_descriptor->SetSampleCount(SampleCount::kCount4); + pipeline_descriptor->SetStencilAttachmentDescriptors(std::nullopt); auto pipeline = context->GetPipelineLibrary()->GetPipeline(pipeline_descriptor).Get(); ASSERT_TRUE(pipeline && pipeline->IsValid()); @@ -934,6 +944,7 @@ TEST_P(RendererTest, InactiveUniforms) { PipelineBuilder::MakeDefaultPipelineDescriptor(*context); ASSERT_TRUE(pipeline_descriptor.has_value()); pipeline_descriptor->SetSampleCount(SampleCount::kCount4); + pipeline_descriptor->SetStencilAttachmentDescriptors(std::nullopt); auto pipeline = context->GetPipelineLibrary()->GetPipeline(pipeline_descriptor).Get(); ASSERT_TRUE(pipeline && pipeline->IsValid()); @@ -1120,6 +1131,7 @@ TEST_P(RendererTest, StencilMask) { ASSERT_TRUE(vertex_buffer); desc->SetSampleCount(SampleCount::kCount4); + desc->SetStencilAttachmentDescriptors(std::nullopt); auto bridge = CreateTextureForFixture("bay_bridge.jpg"); auto boston = CreateTextureForFixture("boston.jpg"); From a8d49470b3aeb29f23decff34f4ac7e3fa280a2d Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Tue, 19 Sep 2023 15:19:56 -0700 Subject: [PATCH 2/3] Update impeller/renderer/backend/vulkan/pipeline_library_vk.cc Co-authored-by: Jonah Williams --- impeller/renderer/backend/vulkan/pipeline_library_vk.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc index 075574e718de2..d612ff7f7cd04 100644 --- a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc +++ b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc @@ -130,7 +130,7 @@ static vk::UniqueRenderPass CreateCompatRenderPassForPipeline( } // This pass is not used with the render pass. It is only necessary to tell - // Vulkan the expected render pass layout. The acutal pass will be create + // Vulkan the expected render pass layout. The acutal pass will be created // later during render pass setup and will need to be compatible with this // one. ContextVK::SetDebugName(device, pass.get(), From 28a662229042a2b57474290bb022f391d5b9f7e9 Mon Sep 17 00:00:00 2001 From: Chinmay Garde Date: Tue, 19 Sep 2023 15:41:43 -0700 Subject: [PATCH 3/3] Update impeller/renderer/backend/vulkan/pipeline_library_vk.cc Co-authored-by: Brandon DeRosier --- impeller/renderer/backend/vulkan/pipeline_library_vk.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc index d612ff7f7cd04..830d2d08120e1 100644 --- a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc +++ b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc @@ -130,7 +130,7 @@ static vk::UniqueRenderPass CreateCompatRenderPassForPipeline( } // This pass is not used with the render pass. It is only necessary to tell - // Vulkan the expected render pass layout. The acutal pass will be created + // Vulkan the expected render pass layout. The actual pass will be created // later during render pass setup and will need to be compatible with this // one. ContextVK::SetDebugName(device, pass.get(),