From 9c51b1b7fb2992e8fca760acd83315618585726a Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Wed, 8 Feb 2023 00:53:08 -0800 Subject: [PATCH] [Impeller] Fix sampling management problems --- .../display_list/display_list_dispatcher.cc | 5 ++- impeller/entity/contents/contents.cc | 12 +++++- impeller/entity/contents/contents.h | 2 + .../contents/filters/blend_filter_contents.cc | 37 ++++++++++++------- .../contents/filters/filter_contents.cc | 1 + .../entity/contents/filters/filter_contents.h | 1 + .../filters/inputs/contents_filter_input.cc | 4 +- impeller/entity/contents/texture_contents.cc | 17 +++++---- impeller/entity/contents/texture_contents.h | 1 + impeller/renderer/sampler_descriptor.cc | 12 +++++- impeller/renderer/sampler_descriptor.h | 7 ++++ impeller/renderer/snapshot.h | 7 +++- 12 files changed, 78 insertions(+), 28 deletions(-) diff --git a/impeller/display_list/display_list_dispatcher.cc b/impeller/display_list/display_list_dispatcher.cc index 0bb93308accc1..58e826ab1654e 100644 --- a/impeller/display_list/display_list_dispatcher.cc +++ b/impeller/display_list/display_list_dispatcher.cc @@ -136,6 +136,9 @@ static impeller::SamplerDescriptor ToSamplerDescriptor( desc.label = "Nearest Sampler"; break; case flutter::DlImageSampling::kLinear: + // Impeller doesn't support cubic sampling, but linear is closer to correct + // than nearest for this case. + case flutter::DlImageSampling::kCubic: desc.min_filter = desc.mag_filter = impeller::MinMagFilter::kLinear; desc.label = "Linear Sampler"; break; @@ -144,8 +147,6 @@ static impeller::SamplerDescriptor ToSamplerDescriptor( desc.mip_filter = impeller::MipFilter::kLinear; desc.label = "Mipmap Linear Sampler"; break; - default: - break; } return desc; } diff --git a/impeller/entity/contents/contents.cc b/impeller/entity/contents/contents.cc index 5de83a10b4c4f..f6f52cd519f63 100644 --- a/impeller/entity/contents/contents.cc +++ b/impeller/entity/contents/contents.cc @@ -41,6 +41,7 @@ Contents::StencilCoverage Contents::GetStencilCoverage( std::optional Contents::RenderToSnapshot( const ContentContext& renderer, const Entity& entity, + const std::optional& sampler_descriptor, bool msaa_enabled) const { auto coverage = GetCoverage(entity); if (!coverage.has_value()) { @@ -64,8 +65,15 @@ std::optional Contents::RenderToSnapshot( return std::nullopt; } - return Snapshot{.texture = texture, - .transform = Matrix::MakeTranslation(coverage->origin)}; + auto snapshot = Snapshot{ + .texture = texture, + .transform = Matrix::MakeTranslation(coverage->origin), + }; + if (sampler_descriptor.has_value()) { + snapshot.sampler_descriptor = sampler_descriptor.value(); + } + + return snapshot; } bool Contents::ShouldRender(const Entity& entity, diff --git a/impeller/entity/contents/contents.h b/impeller/entity/contents/contents.h index 78ccc3fc7d324..ec9919097c17b 100644 --- a/impeller/entity/contents/contents.h +++ b/impeller/entity/contents/contents.h @@ -10,6 +10,7 @@ #include "flutter/fml/macros.h" #include "impeller/geometry/rect.h" +#include "impeller/renderer/sampler_descriptor.h" #include "impeller/renderer/snapshot.h" #include "impeller/renderer/texture.h" @@ -61,6 +62,7 @@ class Contents { virtual std::optional RenderToSnapshot( const ContentContext& renderer, const Entity& entity, + const std::optional& sampler_descriptor = std::nullopt, bool msaa_enabled = true) const; virtual bool ShouldRender(const Entity& entity, diff --git a/impeller/entity/contents/filters/blend_filter_contents.cc b/impeller/entity/contents/filters/blend_filter_contents.cc index e363158782afe..ddfc723379c99 100644 --- a/impeller/entity/contents/filters/blend_filter_contents.cc +++ b/impeller/entity/contents/filters/blend_filter_contents.cc @@ -14,6 +14,7 @@ #include "impeller/entity/contents/solid_color_contents.h" #include "impeller/entity/entity.h" #include "impeller/geometry/path_builder.h" +#include "impeller/renderer/formats.h" #include "impeller/renderer/render_pass.h" #include "impeller/renderer/sampler_library.h" @@ -107,8 +108,9 @@ static std::optional AdvancedBlend( typename FS::BlendInfo blend_info; - auto sampler = renderer.GetContext()->GetSamplerLibrary()->GetSampler({}); - FS::BindTextureSamplerDst(cmd, dst_snapshot->texture, sampler); + auto dst_sampler = renderer.GetContext()->GetSamplerLibrary()->GetSampler( + dst_snapshot->sampler_descriptor); + FS::BindTextureSamplerDst(cmd, dst_snapshot->texture, dst_sampler); blend_info.dst_y_coord_scale = dst_snapshot->texture->GetYCoordScale(); blend_info.dst_input_alpha = absorb_opacity ? dst_snapshot->opacity : 1.0; @@ -118,10 +120,12 @@ static std::optional AdvancedBlend( // This texture will not be sampled from due to the color factor. But // this is present so that validation doesn't trip on a missing // binding. - FS::BindTextureSamplerSrc(cmd, dst_snapshot->texture, sampler); + FS::BindTextureSamplerSrc(cmd, dst_snapshot->texture, dst_sampler); } else { + auto src_sampler = renderer.GetContext()->GetSamplerLibrary()->GetSampler( + src_snapshot->sampler_descriptor); blend_info.color_factor = 0; - FS::BindTextureSamplerSrc(cmd, src_snapshot->texture, sampler); + FS::BindTextureSamplerSrc(cmd, src_snapshot->texture, src_sampler); blend_info.src_y_coord_scale = src_snapshot->texture->GetYCoordScale(); } auto blend_uniform = host_buffer.EmplaceUniform(blend_info); @@ -145,7 +149,10 @@ static std::optional AdvancedBlend( return Snapshot{.texture = out_texture, .transform = Matrix::MakeTranslation(coverage.origin), - .sampler_descriptor = dst_snapshot->sampler_descriptor, + // Since we absorbed the transform of the inputs and used the + // respective snapshot sampling modes when blending, pass on + // the default NN clamp sampler. + .sampler_descriptor = {}, .opacity = (absorb_opacity ? 1.0f : dst_snapshot->opacity) * alpha.value_or(1.0)}; } @@ -168,8 +175,6 @@ static std::optional PipelineBlend( RenderPass& pass) { auto& host_buffer = pass.GetTransientsBuffer(); - auto sampler = renderer.GetContext()->GetSamplerLibrary()->GetSampler({}); - Command cmd; cmd.label = "Pipeline Blend Filter"; auto options = OptionsFromPass(pass); @@ -183,6 +188,8 @@ static std::optional PipelineBlend( return false; } + auto sampler = renderer.GetContext()->GetSamplerLibrary()->GetSampler( + input->sampler_descriptor); FS::BindTextureSamplerSrc(cmd, input->texture, sampler); auto size = input->texture->GetSize(); @@ -262,13 +269,14 @@ static std::optional PipelineBlend( } out_texture->SetLabel("Pipeline Blend Filter Texture"); - return Snapshot{ - .texture = out_texture, - .transform = Matrix::MakeTranslation(coverage.origin), - .sampler_descriptor = - inputs[0]->GetSnapshot(renderer, entity)->sampler_descriptor, - .opacity = (absorb_opacity ? 1.0f : dst_snapshot->opacity) * - alpha.value_or(1.0)}; + return Snapshot{.texture = out_texture, + .transform = Matrix::MakeTranslation(coverage.origin), + // Since we absorbed the transform of the inputs and used the + // respective snapshot sampling modes when blending, pass on + // the default NN clamp sampler. + .sampler_descriptor = {}, + .opacity = (absorb_opacity ? 1.0f : dst_snapshot->opacity) * + alpha.value_or(1.0)}; } #define BLEND_CASE(mode) \ @@ -346,6 +354,7 @@ std::optional BlendFilterContents::RenderFilter( foreground_color_, GetAbsorbOpacity(), GetAlpha()); } + FML_UNREACHABLE(); } diff --git a/impeller/entity/contents/filters/filter_contents.cc b/impeller/entity/contents/filters/filter_contents.cc index d1d76f3a4babb..cf10504d244c7 100644 --- a/impeller/entity/contents/filters/filter_contents.cc +++ b/impeller/entity/contents/filters/filter_contents.cc @@ -237,6 +237,7 @@ std::optional FilterContents::GetFilterCoverage( std::optional FilterContents::RenderToSnapshot( const ContentContext& renderer, const Entity& entity, + const std::optional& sampler_descriptor, bool msaa_enabled) const { Entity entity_with_local_transform = entity; entity_with_local_transform.SetTransformation( diff --git a/impeller/entity/contents/filters/filter_contents.h b/impeller/entity/contents/filters/filter_contents.h index 779cfb07f19cc..74c10397eccb0 100644 --- a/impeller/entity/contents/filters/filter_contents.h +++ b/impeller/entity/contents/filters/filter_contents.h @@ -119,6 +119,7 @@ class FilterContents : public Contents { std::optional RenderToSnapshot( const ContentContext& renderer, const Entity& entity, + const std::optional& sampler_descriptor = std::nullopt, bool msaa_enabled = true) const override; virtual Matrix GetLocalTransform(const Matrix& parent_transform) const; diff --git a/impeller/entity/contents/filters/inputs/contents_filter_input.cc b/impeller/entity/contents/filters/inputs/contents_filter_input.cc index 749515798aebc..e6e40224a4cff 100644 --- a/impeller/entity/contents/filters/inputs/contents_filter_input.cc +++ b/impeller/entity/contents/filters/inputs/contents_filter_input.cc @@ -4,6 +4,7 @@ #include "impeller/entity/contents/filters/inputs/contents_filter_input.h" +#include #include namespace impeller { @@ -22,7 +23,8 @@ std::optional ContentsFilterInput::GetSnapshot( const ContentContext& renderer, const Entity& entity) const { if (!snapshot_.has_value()) { - snapshot_ = contents_->RenderToSnapshot(renderer, entity, msaa_enabled_); + snapshot_ = contents_->RenderToSnapshot(renderer, entity, std::nullopt, + msaa_enabled_); } return snapshot_; } diff --git a/impeller/entity/contents/texture_contents.cc b/impeller/entity/contents/texture_contents.cc index f5b44e5402ec5..fc127a2622b36 100644 --- a/impeller/entity/contents/texture_contents.cc +++ b/impeller/entity/contents/texture_contents.cc @@ -67,6 +67,7 @@ std::optional TextureContents::GetCoverage(const Entity& entity) const { std::optional TextureContents::RenderToSnapshot( const ContentContext& renderer, const Entity& entity, + const std::optional& sampler_descriptor, bool msaa_enabled) const { auto bounds = path_.GetBoundingBox(); if (!bounds.has_value()) { @@ -78,14 +79,16 @@ std::optional TextureContents::RenderToSnapshot( if (is_rect_ && source_rect_ == Rect::MakeSize(texture_->GetSize()) && (opacity_ >= 1 - kEhCloseEnough || defer_applying_opacity_)) { auto scale = Vector2(bounds->size / Size(texture_->GetSize())); - return Snapshot{.texture = texture_, - .transform = entity.GetTransformation() * - Matrix::MakeTranslation(bounds->origin) * - Matrix::MakeScale(scale), - .sampler_descriptor = sampler_descriptor_, - .opacity = opacity_}; + return Snapshot{ + .texture = texture_, + .transform = entity.GetTransformation() * + Matrix::MakeTranslation(bounds->origin) * + Matrix::MakeScale(scale), + .sampler_descriptor = sampler_descriptor.value_or(sampler_descriptor_), + .opacity = opacity_}; } - return Contents::RenderToSnapshot(renderer, entity); + return Contents::RenderToSnapshot( + renderer, entity, sampler_descriptor.value_or(sampler_descriptor_)); } bool TextureContents::Render(const ContentContext& renderer, diff --git a/impeller/entity/contents/texture_contents.h b/impeller/entity/contents/texture_contents.h index 06ef9244b6e23..fec23fba6505a 100644 --- a/impeller/entity/contents/texture_contents.h +++ b/impeller/entity/contents/texture_contents.h @@ -55,6 +55,7 @@ class TextureContents final : public Contents { std::optional RenderToSnapshot( const ContentContext& renderer, const Entity& entity, + const std::optional& sampler_descriptor = std::nullopt, bool msaa_enabled = true) const override; // |Contents| diff --git a/impeller/renderer/sampler_descriptor.cc b/impeller/renderer/sampler_descriptor.cc index 35251848bc742..b2e10cab10d49 100644 --- a/impeller/renderer/sampler_descriptor.cc +++ b/impeller/renderer/sampler_descriptor.cc @@ -3,9 +3,19 @@ // found in the LICENSE file. #include "impeller/renderer/sampler_descriptor.h" +#include "fml/logging.h" namespace impeller { -// +SamplerDescriptor::SamplerDescriptor() = default; + +SamplerDescriptor::SamplerDescriptor(std::string label, + MinMagFilter min_filter, + MinMagFilter mag_filter, + MipFilter mip_filter) + : min_filter(min_filter), + mag_filter(mag_filter), + mip_filter(mip_filter), + label(std::move(label)) {} } // namespace impeller diff --git a/impeller/renderer/sampler_descriptor.h b/impeller/renderer/sampler_descriptor.h index 6ebb96e4e70cf..01d0f293b9054 100644 --- a/impeller/renderer/sampler_descriptor.h +++ b/impeller/renderer/sampler_descriptor.h @@ -26,6 +26,13 @@ struct SamplerDescriptor final : public Comparable { std::string label = "NN Clamp Sampler"; + SamplerDescriptor(); + + SamplerDescriptor(std::string label, + MinMagFilter min_filter, + MinMagFilter mag_filter, + MipFilter mip_filter); + // Comparable std::size_t GetHash() const override { return fml::HashCombine(min_filter, mag_filter, mip_filter, diff --git a/impeller/renderer/snapshot.h b/impeller/renderer/snapshot.h index 26fe49d894d2d..41ad4071445b7 100644 --- a/impeller/renderer/snapshot.h +++ b/impeller/renderer/snapshot.h @@ -11,6 +11,7 @@ #include "flutter/fml/macros.h" #include "impeller/geometry/matrix.h" #include "impeller/geometry/rect.h" +#include "impeller/renderer/formats.h" #include "impeller/renderer/sampler_descriptor.h" #include "impeller/renderer/texture.h" @@ -25,7 +26,11 @@ struct Snapshot { /// The transform that should be applied to this texture for rendering. Matrix transform; - SamplerDescriptor sampler_descriptor; + SamplerDescriptor sampler_descriptor = + SamplerDescriptor("Default Snapshot Sampler", + MinMagFilter::kLinear, + MinMagFilter::kLinear, + MipFilter::kLinear); Scalar opacity = 1.0f;