From c7d7768b6a21ff906e728a92493beb3488a7ed7c Mon Sep 17 00:00:00 2001 From: Kaushik Iska Date: Wed, 15 Feb 2023 12:56:46 -0500 Subject: [PATCH 1/3] [Impeller] Device default attachment pixel formats fixes: https://github.com/flutter/flutter/issues/120498 --- impeller/entity/contents/content_context.cc | 8 +++-- impeller/entity/contents/content_context.h | 3 +- .../contents/runtime_effect_contents.cc | 9 +++-- .../backend/metal/playground_impl_mtl.mm | 3 +- .../renderer/backend/gles/context_gles.cc | 13 ++++--- .../renderer/backend/metal/context_mtl.mm | 13 ++++--- .../renderer/backend/metal/surface_mtl.mm | 3 +- .../renderer/backend/vulkan/context_vk.cc | 13 ++++--- impeller/renderer/context.cc | 2 +- impeller/renderer/device_capabilities.cc | 36 +++++++++++++++++-- impeller/renderer/device_capabilities.h | 17 ++++++++- impeller/renderer/formats.h | 4 +-- impeller/renderer/pipeline_builder.h | 3 +- impeller/renderer/render_target.cc | 6 ++-- .../runtime_stage/runtime_stage_unittests.cc | 6 ++-- 15 files changed, 105 insertions(+), 34 deletions(-) diff --git a/impeller/entity/contents/content_context.cc b/impeller/entity/contents/content_context.cc index 70d63679281bd..edff251665ae9 100644 --- a/impeller/entity/contents/content_context.cc +++ b/impeller/entity/contents/content_context.cc @@ -28,7 +28,9 @@ void ContentContextOptions::ApplyToPipelineDescriptor( desc.SetSampleCount(sample_count); ColorAttachmentDescriptor color0 = *desc.GetColorAttachmentDescriptor(0u); - color0.format = color_attachment_pixel_format; + FML_CHECK(color_attachment_pixel_format.has_value()) + << "Color attachment pixel format must be set."; + color0.format = *color_attachment_pixel_format; color0.alpha_blend_op = BlendOperation::kAdd; color0.color_blend_op = BlendOperation::kAdd; @@ -145,7 +147,9 @@ static std::unique_ptr CreateDefaultPipeline( return nullptr; } // Apply default ContentContextOptions to the descriptor. - ContentContextOptions{}.ApplyToPipelineDescriptor(*desc); + const auto default_color_fmt = context.GetColorAttachmentPixelFormat(); + ContentContextOptions{.color_attachment_pixel_format = default_color_fmt} + .ApplyToPipelineDescriptor(*desc); return std::make_unique(context, desc); } diff --git a/impeller/entity/contents/content_context.h b/impeller/entity/contents/content_context.h index af69e4cfb4dae..a435abd9a3ee8 100644 --- a/impeller/entity/contents/content_context.h +++ b/impeller/entity/contents/content_context.h @@ -5,6 +5,7 @@ #pragma once #include +#include #include #include "flutter/fml/hash_combine.h" @@ -265,7 +266,7 @@ struct ContentContextOptions { CompareFunction stencil_compare = CompareFunction::kEqual; StencilOperation stencil_operation = StencilOperation::kKeep; PrimitiveType primitive_type = PrimitiveType::kTriangle; - PixelFormat color_attachment_pixel_format = PixelFormat::kDefaultColor; + std::optional color_attachment_pixel_format = std::nullopt; bool has_stencil_attachment = true; struct Hash { diff --git a/impeller/entity/contents/runtime_effect_contents.cc b/impeller/entity/contents/runtime_effect_contents.cc index 24a91d1149e00..213585256eb96 100644 --- a/impeller/entity/contents/runtime_effect_contents.cc +++ b/impeller/entity/contents/runtime_effect_contents.cc @@ -102,6 +102,11 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer, /// Get or create runtime stage pipeline. /// + const auto& device_capabilities = context->GetDeviceCapabilities(); + const auto color_attachment_format = context->GetColorAttachmentPixelFormat(); + const auto stencil_attachment_format = + device_capabilities.GetDefaultStencilFormat(); + using VS = RuntimeEffectVertexShader; PipelineDescriptor desc; desc.SetLabel("Runtime Stage"); @@ -115,9 +120,9 @@ bool RuntimeEffectContents::Render(const ContentContext& renderer, } desc.SetVertexDescriptor(std::move(vertex_descriptor)); desc.SetColorAttachmentDescriptor( - 0u, {.format = PixelFormat::kDefaultColor, .blending_enabled = true}); + 0u, {.format = color_attachment_format, .blending_enabled = true}); desc.SetStencilAttachmentDescriptors({}); - desc.SetStencilPixelFormat(PixelFormat::kDefaultStencil); + desc.SetStencilPixelFormat(stencil_attachment_format); auto options = OptionsFromPassAndEntity(pass, entity); if (geometry_result.prevent_overdraw) { diff --git a/impeller/playground/backend/metal/playground_impl_mtl.mm b/impeller/playground/backend/metal/playground_impl_mtl.mm index c3f9e0c33d8e4..4b94c68b3dcf2 100644 --- a/impeller/playground/backend/metal/playground_impl_mtl.mm +++ b/impeller/playground/backend/metal/playground_impl_mtl.mm @@ -75,7 +75,8 @@ data_->metal_layer = [CAMetalLayer layer]; data_->metal_layer.device = ContextMTL::Cast(*context).GetMTLDevice(); // This pixel format is one of the documented supported formats. - data_->metal_layer.pixelFormat = ToMTLPixelFormat(PixelFormat::kDefaultColor); + const auto color_fmt = context->GetColorAttachmentPixelFormat(); + data_->metal_layer.pixelFormat = ToMTLPixelFormat(color_fmt); data_->metal_layer.framebufferOnly = NO; cocoa_window.contentView.layer = data_->metal_layer; cocoa_window.contentView.wantsLayer = YES; diff --git a/impeller/renderer/backend/gles/context_gles.cc b/impeller/renderer/backend/gles/context_gles.cc index 64ead0a60de6d..a592823e2d8c2 100644 --- a/impeller/renderer/backend/gles/context_gles.cc +++ b/impeller/renderer/backend/gles/context_gles.cc @@ -71,11 +71,14 @@ ContextGLES::ContextGLES(std::unique_ptr gl, // Create the device capabilities. { - device_capabilities_ = DeviceCapabilitiesBuilder() - .SetHasThreadingRestrictions(true) - .SetSupportsOffscreenMSAA(false) - .SetSupportsSSBO(false) - .Build(); + device_capabilities_ = + DeviceCapabilitiesBuilder() + .SetHasThreadingRestrictions(true) + .SetSupportsOffscreenMSAA(false) + .SetSupportsSSBO(false) + .SetDefaultColorFormat(PixelFormat::kB8G8R8A8UNormInt) + .SetDefaultStencilFormat(PixelFormat::kS8UInt) + .Build(); } is_valid_ = true; diff --git a/impeller/renderer/backend/metal/context_mtl.mm b/impeller/renderer/backend/metal/context_mtl.mm index 8e5901f00ecfb..5ccf543169305 100644 --- a/impeller/renderer/backend/metal/context_mtl.mm +++ b/impeller/renderer/backend/metal/context_mtl.mm @@ -90,11 +90,14 @@ #endif { - device_capabilities_ = DeviceCapabilitiesBuilder() - .SetHasThreadingRestrictions(false) - .SetSupportsOffscreenMSAA(true) - .SetSupportsSSBO(true) - .Build(); + device_capabilities_ = + DeviceCapabilitiesBuilder() + .SetHasThreadingRestrictions(false) + .SetSupportsOffscreenMSAA(true) + .SetSupportsSSBO(true) + .SetDefaultColorFormat(PixelFormat::kB8G8R8A8UNormInt) + .SetDefaultStencilFormat(PixelFormat::kS8UInt) + .Build(); } is_valid_ = true; diff --git a/impeller/renderer/backend/metal/surface_mtl.mm b/impeller/renderer/backend/metal/surface_mtl.mm index 09302c26dcd26..4ddcb2dd40613 100644 --- a/impeller/renderer/backend/metal/surface_mtl.mm +++ b/impeller/renderer/backend/metal/surface_mtl.mm @@ -81,7 +81,8 @@ stencil0_tex.storage_mode = StorageMode::kDeviceTransient; stencil0_tex.type = TextureType::kTexture2DMultisample; stencil0_tex.sample_count = SampleCount::kCount4; - stencil0_tex.format = PixelFormat::kDefaultStencil; + stencil0_tex.format = + context->GetDeviceCapabilities().GetDefaultStencilFormat(); stencil0_tex.size = color0_tex_desc.size; stencil0_tex.usage = static_cast(TextureUsage::kRenderTarget); diff --git a/impeller/renderer/backend/vulkan/context_vk.cc b/impeller/renderer/backend/vulkan/context_vk.cc index 9418d4332998c..c8aa938610596 100644 --- a/impeller/renderer/backend/vulkan/context_vk.cc +++ b/impeller/renderer/backend/vulkan/context_vk.cc @@ -499,11 +499,14 @@ ContextVK::ContextVK( transfer_queue_ = device_->getQueue(transfer_queue->family, transfer_queue->index); - device_capabilities_ = DeviceCapabilitiesBuilder() - .SetHasThreadingRestrictions(false) - .SetSupportsOffscreenMSAA(true) - .SetSupportsSSBO(false) - .Build(); + device_capabilities_ = + DeviceCapabilitiesBuilder() + .SetHasThreadingRestrictions(false) + .SetSupportsOffscreenMSAA(true) + .SetSupportsSSBO(false) + .SetDefaultColorFormat(PixelFormat::kB8G8R8A8UNormInt) + .SetDefaultStencilFormat(PixelFormat::kS8UInt) + .Build(); is_valid_ = true; } diff --git a/impeller/renderer/context.cc b/impeller/renderer/context.cc index 26484a9af2f68..9d8aad55e681a 100644 --- a/impeller/renderer/context.cc +++ b/impeller/renderer/context.cc @@ -15,7 +15,7 @@ std::shared_ptr Context::GetGPUTracer() const { } PixelFormat Context::GetColorAttachmentPixelFormat() const { - return PixelFormat::kDefaultColor; + return GetDeviceCapabilities().GetDefaultColorFormat(); } } // namespace impeller diff --git a/impeller/renderer/device_capabilities.cc b/impeller/renderer/device_capabilities.cc index f4a9ee6818ac7..bddb854e95a20 100644 --- a/impeller/renderer/device_capabilities.cc +++ b/impeller/renderer/device_capabilities.cc @@ -8,10 +8,14 @@ namespace impeller { IDeviceCapabilities::IDeviceCapabilities(bool threading_restrictions, bool offscreen_msaa, - bool supports_ssbo) + bool supports_ssbo, + PixelFormat default_color_format, + PixelFormat default_stencil_format) : threading_restrictions_(threading_restrictions), offscreen_msaa_(offscreen_msaa), - supports_ssbo_(supports_ssbo) {} + supports_ssbo_(supports_ssbo), + default_color_format_(default_color_format), + default_stencil_format_(default_stencil_format) {} IDeviceCapabilities::~IDeviceCapabilities() = default; @@ -27,6 +31,14 @@ bool IDeviceCapabilities::SupportsSSBO() const { return supports_ssbo_; } +PixelFormat IDeviceCapabilities::GetDefaultColorFormat() const { + return default_color_format_; +} + +PixelFormat IDeviceCapabilities::GetDefaultStencilFormat() const { + return default_stencil_format_; +} + DeviceCapabilitiesBuilder::DeviceCapabilitiesBuilder() = default; DeviceCapabilitiesBuilder::~DeviceCapabilitiesBuilder() = default; @@ -49,9 +61,27 @@ DeviceCapabilitiesBuilder& DeviceCapabilitiesBuilder::SetSupportsSSBO( return *this; } +DeviceCapabilitiesBuilder& DeviceCapabilitiesBuilder::SetDefaultColorFormat( + PixelFormat value) { + default_color_format_ = value; + return *this; +} + +DeviceCapabilitiesBuilder& DeviceCapabilitiesBuilder::SetDefaultStencilFormat( + PixelFormat value) { + default_stencil_format_ = value; + return *this; +} + std::unique_ptr DeviceCapabilitiesBuilder::Build() { + FML_CHECK(default_color_format_.has_value()) + << "Default color format not set"; + FML_CHECK(default_stencil_format_.has_value()) + << "Default stencil format not set"; + IDeviceCapabilities* capabilities = new IDeviceCapabilities( - threading_restrictions_, offscreen_msaa_, supports_ssbo_); + threading_restrictions_, offscreen_msaa_, supports_ssbo_, + *default_color_format_, *default_stencil_format_); return std::unique_ptr(capabilities); } diff --git a/impeller/renderer/device_capabilities.h b/impeller/renderer/device_capabilities.h index 094f2cd2a4a09..3224e51939b83 100644 --- a/impeller/renderer/device_capabilities.h +++ b/impeller/renderer/device_capabilities.h @@ -7,6 +7,7 @@ #include #include "flutter/fml/macros.h" +#include "impeller/renderer/formats.h" namespace impeller { @@ -20,16 +21,24 @@ class IDeviceCapabilities { bool SupportsSSBO() const; + PixelFormat GetDefaultColorFormat() const; + + PixelFormat GetDefaultStencilFormat() const; + private: IDeviceCapabilities(bool threading_restrictions, bool offscreen_msaa, - bool supports_ssbo); + bool supports_ssbo, + PixelFormat default_color_format, + PixelFormat default_stencil_format); friend class DeviceCapabilitiesBuilder; bool threading_restrictions_ = false; bool offscreen_msaa_ = false; bool supports_ssbo_ = false; + PixelFormat default_color_format_; + PixelFormat default_stencil_format_; FML_DISALLOW_COPY_AND_ASSIGN(IDeviceCapabilities); }; @@ -46,12 +55,18 @@ class DeviceCapabilitiesBuilder { DeviceCapabilitiesBuilder& SetSupportsSSBO(bool value); + DeviceCapabilitiesBuilder& SetDefaultColorFormat(PixelFormat value); + + DeviceCapabilitiesBuilder& SetDefaultStencilFormat(PixelFormat value); + std::unique_ptr Build(); private: bool threading_restrictions_ = false; bool offscreen_msaa_ = false; bool supports_ssbo_ = false; + std::optional default_color_format_ = std::nullopt; + std::optional default_stencil_format_ = std::nullopt; FML_DISALLOW_COPY_AND_ASSIGN(DeviceCapabilitiesBuilder); }; diff --git a/impeller/renderer/formats.h b/impeller/renderer/formats.h index 51a25ac31faf1..3beeee535cbf9 100644 --- a/impeller/renderer/formats.h +++ b/impeller/renderer/formats.h @@ -101,8 +101,8 @@ enum class PixelFormat { // // On Metal, this is a support format for layer drawable and can be used to // specify the format of the resolve texture if needed. - kDefaultColor = kB8G8R8A8UNormInt, - kDefaultStencil = kS8UInt, + // kDefaultColor = kB8G8R8A8UNormInt, + // kDefaultStencil = kS8UInt, }; enum class BlendFactor { diff --git a/impeller/renderer/pipeline_builder.h b/impeller/renderer/pipeline_builder.h index f41e0e337c180..efaab15e65865 100644 --- a/impeller/renderer/pipeline_builder.h +++ b/impeller/renderer/pipeline_builder.h @@ -127,7 +127,8 @@ struct PipelineBuilder { StencilAttachmentDescriptor stencil0; stencil0.stencil_compare = CompareFunction::kEqual; desc.SetStencilAttachmentDescriptors(stencil0); - desc.SetStencilPixelFormat(PixelFormat::kDefaultStencil); + desc.SetStencilPixelFormat( + context.GetDeviceCapabilities().GetDefaultStencilFormat()); } return true; diff --git a/impeller/renderer/render_target.cc b/impeller/renderer/render_target.cc index aeca24ab68793..24215a5ebb46f 100644 --- a/impeller/renderer/render_target.cc +++ b/impeller/renderer/render_target.cc @@ -225,7 +225,8 @@ RenderTarget RenderTarget::CreateOffscreen( if (stencil_attachment_config.has_value()) { TextureDescriptor stencil_tex0; stencil_tex0.storage_mode = stencil_attachment_config->storage_mode; - stencil_tex0.format = PixelFormat::kDefaultStencil; + stencil_tex0.format = + context.GetDeviceCapabilities().GetDefaultStencilFormat(); stencil_tex0.size = size; stencil_tex0.usage = static_cast(TextureUsage::kRenderTarget); @@ -318,7 +319,8 @@ RenderTarget RenderTarget::CreateOffscreenMSAA( stencil_tex0.storage_mode = stencil_attachment_config->storage_mode; stencil_tex0.type = TextureType::kTexture2DMultisample; stencil_tex0.sample_count = SampleCount::kCount4; - stencil_tex0.format = PixelFormat::kDefaultStencil; + stencil_tex0.format = + context.GetDeviceCapabilities().GetDefaultStencilFormat(); stencil_tex0.size = size; stencil_tex0.usage = static_cast(TextureUsage::kRenderTarget); diff --git a/impeller/runtime_stage/runtime_stage_unittests.cc b/impeller/runtime_stage/runtime_stage_unittests.cc index 1c4a9acf5e1e3..896ecab89e824 100644 --- a/impeller/runtime_stage/runtime_stage_unittests.cc +++ b/impeller/runtime_stage/runtime_stage_unittests.cc @@ -242,12 +242,14 @@ TEST_P(RuntimeStageTest, CanCreatePipelineFromRuntimeStage) { ASSERT_TRUE(vertex_descriptor->SetStageInputs(VS::kAllShaderStageInputs)); desc.SetVertexDescriptor(std::move(vertex_descriptor)); ColorAttachmentDescriptor color0; - color0.format = PixelFormat::kDefaultColor; + color0.format = GetContext()->GetColorAttachmentPixelFormat(); StencilAttachmentDescriptor stencil0; stencil0.stencil_compare = CompareFunction::kEqual; desc.SetColorAttachmentDescriptor(0u, color0); desc.SetStencilAttachmentDescriptors(stencil0); - desc.SetStencilPixelFormat(PixelFormat::kDefaultStencil); + const auto stencil_fmt = + GetContext()->GetDeviceCapabilities().GetDefaultStencilFormat(); + desc.SetStencilPixelFormat(stencil_fmt); auto pipeline = GetContext()->GetPipelineLibrary()->GetPipeline(desc).Get(); ASSERT_NE(pipeline, nullptr); } From 135b1f0a9e3b1f47c78f2a074015a0ab0cee814e Mon Sep 17 00:00:00 2001 From: Kaushik Iska Date: Thu, 16 Feb 2023 13:37:43 -0500 Subject: [PATCH 2/3] remove commented out defaults --- impeller/renderer/formats.h | 8 -------- 1 file changed, 8 deletions(-) diff --git a/impeller/renderer/formats.h b/impeller/renderer/formats.h index 3beeee535cbf9..a939610b574c1 100644 --- a/impeller/renderer/formats.h +++ b/impeller/renderer/formats.h @@ -95,14 +95,6 @@ enum class PixelFormat { // Depth and stencil formats. kS8UInt, kD32FloatS8UInt, - - // Defaults. If you don't know which ones to use, these are usually a safe - // bet. - // - // On Metal, this is a support format for layer drawable and can be used to - // specify the format of the resolve texture if needed. - // kDefaultColor = kB8G8R8A8UNormInt, - // kDefaultStencil = kS8UInt, }; enum class BlendFactor { From 994f021ebed8ccb4b22efb37b01ea64d17bbe6f7 Mon Sep 17 00:00:00 2001 From: Kaushik Iska Date: Fri, 17 Feb 2023 13:12:09 -0500 Subject: [PATCH 3/3] fix CR comments --- impeller/entity/contents/content_context.cc | 8 ++++++-- impeller/entity/contents/content_context.h | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/impeller/entity/contents/content_context.cc b/impeller/entity/contents/content_context.cc index edff251665ae9..2799a9164d4c1 100644 --- a/impeller/entity/contents/content_context.cc +++ b/impeller/entity/contents/content_context.cc @@ -28,8 +28,12 @@ void ContentContextOptions::ApplyToPipelineDescriptor( desc.SetSampleCount(sample_count); ColorAttachmentDescriptor color0 = *desc.GetColorAttachmentDescriptor(0u); - FML_CHECK(color_attachment_pixel_format.has_value()) - << "Color attachment pixel format must be set."; + if (!color_attachment_pixel_format.has_value()) { + VALIDATION_LOG << "Color attachment pixel format must be set."; + color0.format = PixelFormat::kB8G8R8A8UNormInt; + } else { + color0.format = *color_attachment_pixel_format; + } color0.format = *color_attachment_pixel_format; color0.alpha_blend_op = BlendOperation::kAdd; color0.color_blend_op = BlendOperation::kAdd; diff --git a/impeller/entity/contents/content_context.h b/impeller/entity/contents/content_context.h index a435abd9a3ee8..65bd2a020bcfe 100644 --- a/impeller/entity/contents/content_context.h +++ b/impeller/entity/contents/content_context.h @@ -266,7 +266,7 @@ struct ContentContextOptions { CompareFunction stencil_compare = CompareFunction::kEqual; StencilOperation stencil_operation = StencilOperation::kKeep; PrimitiveType primitive_type = PrimitiveType::kTriangle; - std::optional color_attachment_pixel_format = std::nullopt; + std::optional color_attachment_pixel_format; bool has_stencil_attachment = true; struct Hash {