From 077440458f55e04cb7dff6afff59eb616bd9e30e Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Thu, 27 Jul 2023 13:27:37 -0700 Subject: [PATCH] [Impeller] Move glyph atlas state from context into lazy glyph atlas. (flutter/engine#43748) No impact on behavior. Rather that separately cache the lazy glyph atlas and the underlying state (bitmap, atlas, texture), move the state into the lazy glyph atlas. To do make this behave correctly, we need to "reset" the lazy glyph atlas instead of deleting it on each frame. Reseting it clears out the text runs and cached atlas, which has the same impact as deleting it in the previous behavior. This refactor is required in order to move to platform specific glyph atlases, to solve issues such as https://github.com/flutter/flutter/issues/126104 --- .../impeller/entity/contents/content_context.cc | 9 +-------- .../impeller/entity/contents/content_context.h | 10 ---------- .../impeller/entity/contents/text_contents.cc | 7 ++----- .../impeller/entity/contents/text_contents.h | 1 - .../src/flutter/impeller/entity/entity_pass.cc | 3 +-- .../impeller/typographer/lazy_glyph_atlas.cc | 16 ++++++++++++---- .../impeller/typographer/lazy_glyph_atlas.h | 5 ++++- .../typographer/typographer_unittests.cc | 6 ++---- 8 files changed, 22 insertions(+), 35 deletions(-) diff --git a/engine/src/flutter/impeller/entity/contents/content_context.cc b/engine/src/flutter/impeller/entity/contents/content_context.cc index 6b89e36efe6..ac398a11c5a 100644 --- a/engine/src/flutter/impeller/entity/contents/content_context.cc +++ b/engine/src/flutter/impeller/entity/contents/content_context.cc @@ -160,9 +160,8 @@ static std::unique_ptr CreateDefaultPipeline( ContentContext::ContentContext(std::shared_ptr context) : context_(std::move(context)), + lazy_glyph_atlas_(std::make_shared()), tessellator_(std::make_shared()), - alpha_glyph_atlas_context_(std::make_shared()), - color_glyph_atlas_context_(std::make_shared()), scene_context_(std::make_shared(context_)) { if (!context_ || !context_->IsValid()) { return; @@ -411,12 +410,6 @@ std::shared_ptr ContentContext::GetTessellator() const { return tessellator_; } -std::shared_ptr ContentContext::GetGlyphAtlasContext( - GlyphAtlas::Type type) const { - return type == GlyphAtlas::Type::kAlphaBitmap ? alpha_glyph_atlas_context_ - : color_glyph_atlas_context_; -} - std::shared_ptr ContentContext::GetContext() const { return context_; } diff --git a/engine/src/flutter/impeller/entity/contents/content_context.h b/engine/src/flutter/impeller/entity/contents/content_context.h index 55f0a488c87..cab89c12533 100644 --- a/engine/src/flutter/impeller/entity/contents/content_context.h +++ b/engine/src/flutter/impeller/entity/contents/content_context.h @@ -680,9 +680,6 @@ class ContentContext { std::shared_ptr GetContext() const; - std::shared_ptr GetGlyphAtlasContext( - GlyphAtlas::Type type) const; - const Capabilities& GetDeviceCapabilities() const; void SetWireframe(bool wireframe); @@ -697,11 +694,6 @@ class ContentContext { const SubpassCallback& subpass_callback, bool msaa_enabled = true) const; - void SetLazyGlyphAtlas( - const std::shared_ptr& lazy_glyph_atlas) { - lazy_glyph_atlas_ = lazy_glyph_atlas; - } - std::shared_ptr GetLazyGlyphAtlas() const { return lazy_glyph_atlas_; } @@ -861,8 +853,6 @@ class ContentContext { bool is_valid_ = false; std::shared_ptr tessellator_; - std::shared_ptr alpha_glyph_atlas_context_; - std::shared_ptr color_glyph_atlas_context_; std::shared_ptr scene_context_; bool wireframe_ = false; diff --git a/engine/src/flutter/impeller/entity/contents/text_contents.cc b/engine/src/flutter/impeller/entity/contents/text_contents.cc index fe6963d1684..9f28e9e9799 100644 --- a/engine/src/flutter/impeller/entity/contents/text_contents.cc +++ b/engine/src/flutter/impeller/entity/contents/text_contents.cc @@ -32,12 +32,10 @@ void TextContents::SetTextFrame(const TextFrame& frame) { std::shared_ptr TextContents::ResolveAtlas( GlyphAtlas::Type type, const std::shared_ptr& lazy_atlas, - std::shared_ptr atlas_context, std::shared_ptr context) const { FML_DCHECK(lazy_atlas); if (lazy_atlas) { - return lazy_atlas->CreateOrGetGlyphAtlas(type, std::move(atlas_context), - std::move(context)); + return lazy_atlas->CreateOrGetGlyphAtlas(type, std::move(context)); } return nullptr; @@ -92,8 +90,7 @@ bool TextContents::Render(const ContentContext& renderer, auto type = frame_.GetAtlasType(); auto atlas = - ResolveAtlas(type, renderer.GetLazyGlyphAtlas(), - renderer.GetGlyphAtlasContext(type), renderer.GetContext()); + ResolveAtlas(type, renderer.GetLazyGlyphAtlas(), renderer.GetContext()); if (!atlas || !atlas->IsValid()) { VALIDATION_LOG << "Cannot render glyphs without prepared atlas."; diff --git a/engine/src/flutter/impeller/entity/contents/text_contents.h b/engine/src/flutter/impeller/entity/contents/text_contents.h index c75daa0a506..4499586735a 100644 --- a/engine/src/flutter/impeller/entity/contents/text_contents.h +++ b/engine/src/flutter/impeller/entity/contents/text_contents.h @@ -65,7 +65,6 @@ class TextContents final : public Contents { std::shared_ptr ResolveAtlas( GlyphAtlas::Type type, const std::shared_ptr& lazy_atlas, - std::shared_ptr atlas_context, std::shared_ptr context) const; FML_DISALLOW_COPY_AND_ASSIGN(TextContents); diff --git a/engine/src/flutter/impeller/entity/entity_pass.cc b/engine/src/flutter/impeller/entity/entity_pass.cc index ee4d0c9c051..ec070920678 100644 --- a/engine/src/flutter/impeller/entity/entity_pass.cc +++ b/engine/src/flutter/impeller/entity/entity_pass.cc @@ -254,9 +254,8 @@ bool EntityPass::Render(ContentContext& renderer, return false; } - renderer.SetLazyGlyphAtlas(std::make_shared()); fml::ScopedCleanupClosure reset_lazy_glyph_atlas( - [&renderer]() { renderer.SetLazyGlyphAtlas(nullptr); }); + [&renderer]() { renderer.GetLazyGlyphAtlas()->ResetTextFrames(); }); IterateAllEntities([lazy_glyph_atlas = renderer.GetLazyGlyphAtlas()](const Entity& entity) { diff --git a/engine/src/flutter/impeller/typographer/lazy_glyph_atlas.cc b/engine/src/flutter/impeller/typographer/lazy_glyph_atlas.cc index 94c738e37b7..f98f8af6d62 100644 --- a/engine/src/flutter/impeller/typographer/lazy_glyph_atlas.cc +++ b/engine/src/flutter/impeller/typographer/lazy_glyph_atlas.cc @@ -11,7 +11,9 @@ namespace impeller { -LazyGlyphAtlas::LazyGlyphAtlas() = default; +LazyGlyphAtlas::LazyGlyphAtlas() + : alpha_context_(std::make_shared()), + color_context_(std::make_shared()) {} LazyGlyphAtlas::~LazyGlyphAtlas() = default; @@ -24,9 +26,14 @@ void LazyGlyphAtlas::AddTextFrame(const TextFrame& frame, Scalar scale) { } } +void LazyGlyphAtlas::ResetTextFrames() { + alpha_set_.clear(); + color_set_.clear(); + atlas_map_.clear(); +} + std::shared_ptr LazyGlyphAtlas::CreateOrGetGlyphAtlas( GlyphAtlas::Type type, - std::shared_ptr atlas_context, std::shared_ptr context) const { { auto atlas_it = atlas_map_.find(type); @@ -40,8 +47,9 @@ std::shared_ptr LazyGlyphAtlas::CreateOrGetGlyphAtlas( return nullptr; } auto& set = type == GlyphAtlas::Type::kAlphaBitmap ? alpha_set_ : color_set_; - auto atlas = - text_context->CreateGlyphAtlas(type, std::move(atlas_context), set); + auto atlas_context = + type == GlyphAtlas::Type::kAlphaBitmap ? alpha_context_ : color_context_; + auto atlas = text_context->CreateGlyphAtlas(type, atlas_context, set); if (!atlas || !atlas->IsValid()) { VALIDATION_LOG << "Could not create valid atlas."; return nullptr; diff --git a/engine/src/flutter/impeller/typographer/lazy_glyph_atlas.h b/engine/src/flutter/impeller/typographer/lazy_glyph_atlas.h index f79b46db9d3..7289538eb4b 100644 --- a/engine/src/flutter/impeller/typographer/lazy_glyph_atlas.h +++ b/engine/src/flutter/impeller/typographer/lazy_glyph_atlas.h @@ -21,14 +21,17 @@ class LazyGlyphAtlas { void AddTextFrame(const TextFrame& frame, Scalar scale); + void ResetTextFrames(); + std::shared_ptr CreateOrGetGlyphAtlas( GlyphAtlas::Type type, - std::shared_ptr atlas_context, std::shared_ptr context) const; private: FontGlyphPair::Set alpha_set_; FontGlyphPair::Set color_set_; + std::shared_ptr alpha_context_; + std::shared_ptr color_context_; mutable std::unordered_map> atlas_map_; diff --git a/engine/src/flutter/impeller/typographer/typographer_unittests.cc b/engine/src/flutter/impeller/typographer/typographer_unittests.cc index b2da0e6b80e..02e8e790f83 100644 --- a/engine/src/flutter/impeller/typographer/typographer_unittests.cc +++ b/engine/src/flutter/impeller/typographer/typographer_unittests.cc @@ -122,13 +122,11 @@ TEST_P(TypographerTest, LazyAtlasTracksColor) { lazy_atlas.AddTextFrame(frame, 1.0f); // Creates different atlases for color and alpha bitmap. - auto color_context = std::make_shared(); - auto bitmap_context = std::make_shared(); auto color_atlas = lazy_atlas.CreateOrGetGlyphAtlas( - GlyphAtlas::Type::kColorBitmap, color_context, GetContext()); + GlyphAtlas::Type::kColorBitmap, GetContext()); auto bitmap_atlas = lazy_atlas.CreateOrGetGlyphAtlas( - GlyphAtlas::Type::kAlphaBitmap, bitmap_context, GetContext()); + GlyphAtlas::Type::kAlphaBitmap, GetContext()); ASSERT_FALSE(color_atlas == bitmap_atlas); }