From eaf1b6917905ca2e7974d3b66a7c93c84efd312f Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Thu, 28 Nov 2024 18:08:41 -0800 Subject: [PATCH] Revert "[Impeller] cache even more text frame data to skip lookups. (#56798)" This reverts commit 65c8c6e8d7581c1a87504960c51fa4b21282f9bb. --- impeller/display_list/dl_dispatcher.cc | 5 +- impeller/entity/contents/text_contents.cc | 7 +- impeller/entity/entity_unittests.cc | 10 +- .../backends/skia/typographer_context_skia.cc | 17 ++-- impeller/typographer/font_glyph_pair.h | 19 ++-- impeller/typographer/glyph_atlas.cc | 14 +-- impeller/typographer/glyph_atlas.h | 16 +--- impeller/typographer/text_frame.cc | 32 +------ impeller/typographer/text_frame.h | 14 +-- impeller/typographer/typographer_unittests.cc | 93 +------------------ 10 files changed, 35 insertions(+), 192 deletions(-) diff --git a/impeller/display_list/dl_dispatcher.cc b/impeller/display_list/dl_dispatcher.cc index 3fb650d36a29d..86974a84e4a79 100644 --- a/impeller/display_list/dl_dispatcher.cc +++ b/impeller/display_list/dl_dispatcher.cc @@ -1105,9 +1105,8 @@ void FirstPassDispatcher::drawTextFrame( // we do not double-apply the alpha. properties.color = paint_.color.WithAlpha(1.0); } - auto scale = TextFrame::RoundScaledFontSize( - (matrix_ * Matrix::MakeTranslation(Point(x, y))).GetMaxBasisLengthXY()); - + auto scale = + (matrix_ * Matrix::MakeTranslation(Point(x, y))).GetMaxBasisLengthXY(); renderer_.GetLazyGlyphAtlas()->AddTextFrame( text_frame, // scale, // diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index 4d4a130480157..dfa4c42198a94 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/impeller/entity/contents/text_contents.cc @@ -175,7 +175,8 @@ bool TextContents::Render(const ContentContext& renderer, size_t bounds_offset = 0u; for (const TextRun& run : frame_->GetRuns()) { const Font& font = run.GetFont(); - Scalar rounded_scale = TextFrame::RoundScaledFontSize(scale_); + Scalar rounded_scale = TextFrame::RoundScaledFontSize( + scale_, font.GetMetrics().point_size); FontGlyphAtlas* font_atlas = nullptr; // Adjust glyph position based on the subpixel rounding @@ -219,9 +220,9 @@ bool TextContents::Render(const ContentContext& renderer, VALIDATION_LOG << "Could not find font in the atlas."; continue; } + // Note: uses unrounded scale for more accurate subpixel position. Point subpixel = TextFrame::ComputeSubpixelPosition( - glyph_position, font.GetAxisAlignment(), offset_, - rounded_scale); + glyph_position, font.GetAxisAlignment(), offset_, scale_); std::optional maybe_atlas_glyph_bounds = font_atlas->FindGlyphBounds(SubpixelGlyph{ diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 756203ab2f04a..8fee465fabe9c 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -2120,11 +2120,11 @@ TEST_P(EntityTest, ColorFilterContentsWithLargeGeometry) { } TEST_P(EntityTest, TextContentsCeilsGlyphScaleToDecimal) { - ASSERT_EQ(TextFrame::RoundScaledFontSize(0.4321111f), 0.43f); - ASSERT_EQ(TextFrame::RoundScaledFontSize(0.5321111f), 0.53f); - ASSERT_EQ(TextFrame::RoundScaledFontSize(2.1f), 2.1f); - ASSERT_EQ(TextFrame::RoundScaledFontSize(0.0f), 0.0f); - ASSERT_EQ(TextFrame::RoundScaledFontSize(100000000.0f), 48.0f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(0.4321111f, 12), 0.43f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(0.5321111f, 12), 0.53f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(2.1f, 12), 2.1f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(0.0f, 12), 0.0f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(100000000.0f, 12), 48.0f); } TEST_P(EntityTest, SpecializationConstantsAreAppliedToVariants) { diff --git a/impeller/typographer/backends/skia/typographer_context_skia.cc b/impeller/typographer/backends/skia/typographer_context_skia.cc index 13dc254e67dce..80b499352b557 100644 --- a/impeller/typographer/backends/skia/typographer_context_skia.cc +++ b/impeller/typographer/backends/skia/typographer_context_skia.cc @@ -413,20 +413,18 @@ TypographerContextSkia::CollectNewGlyphs( const std::vector>& text_frames) { std::vector new_glyphs; std::vector glyph_sizes; - size_t generation_id = atlas->GetAtlasGeneration(); for (const auto& frame : text_frames) { - if (frame->IsFrameComplete() && - frame->GetAtlasGeneration() == generation_id && - !frame->GetFrameBounds(0).is_placeholder) { - continue; - } + // TODO(jonahwilliams): unless we destroy the atlas (which we know about), + // we could probably guarantee that a text frame that is complete does not + // need to be processed unless the scale or properties changed. I'm leaving + // this as a future optimization. frame->ClearFrameBounds(); - frame->SetAtlasGeneration(generation_id); for (const auto& run : frame->GetRuns()) { auto metrics = run.GetFont().GetMetrics(); - auto rounded_scale = TextFrame::RoundScaledFontSize(frame->GetScale()); + auto rounded_scale = + TextFrame::RoundScaledFontSize(frame->GetScale(), metrics.point_size); ScaledFont scaled_font{.font = run.GetFont(), .scale = rounded_scale}; FontGlyphAtlas* font_glyph_atlas = @@ -568,8 +566,7 @@ std::shared_ptr TypographerContextSkia::CreateGlyphAtlas( if (atlas_context->GetAtlasSize().height >= max_texture_height || context.GetBackendType() == Context::BackendType::kOpenGLES) { blit_old_atlas = false; - new_atlas = std::make_shared( - type, /*initial_generation=*/last_atlas->GetAtlasGeneration() + 1); + new_atlas = std::make_shared(type); auto [update_glyphs, update_sizes] = CollectNewGlyphs(new_atlas, text_frames); diff --git a/impeller/typographer/font_glyph_pair.h b/impeller/typographer/font_glyph_pair.h index e3df2dae50c11..e530f5b3edcda 100644 --- a/impeller/typographer/font_glyph_pair.h +++ b/impeller/typographer/font_glyph_pair.h @@ -20,17 +20,6 @@ struct GlyphProperties { Join stroke_join = Join::kMiter; Scalar stroke_miter = 4.0; bool stroke = false; - - struct Equal { - constexpr bool operator()(const impeller::GlyphProperties& lhs, - const impeller::GlyphProperties& rhs) const { - return lhs.color.ToARGB() == rhs.color.ToARGB() && - lhs.stroke == rhs.stroke && lhs.stroke_cap == rhs.stroke_cap && - lhs.stroke_join == rhs.stroke_join && - lhs.stroke_miter == rhs.stroke_miter && - lhs.stroke_width == rhs.stroke_width; - } - }; }; //------------------------------------------------------------------------------ @@ -96,8 +85,12 @@ struct SubpixelGlyph { lhs.glyph.type == rhs.glyph.type && lhs.subpixel_offset == rhs.subpixel_offset && lhs.properties.has_value() && rhs.properties.has_value() && - GlyphProperties::Equal{}(lhs.properties.value(), - rhs.properties.value()); + lhs.properties->color.ToARGB() == rhs.properties->color.ToARGB() && + lhs.properties->stroke == rhs.properties->stroke && + lhs.properties->stroke_cap == rhs.properties->stroke_cap && + lhs.properties->stroke_join == rhs.properties->stroke_join && + lhs.properties->stroke_miter == rhs.properties->stroke_miter && + lhs.properties->stroke_width == rhs.properties->stroke_width; } }; }; diff --git a/impeller/typographer/glyph_atlas.cc b/impeller/typographer/glyph_atlas.cc index 98038668fb501..093593804ea5f 100644 --- a/impeller/typographer/glyph_atlas.cc +++ b/impeller/typographer/glyph_atlas.cc @@ -12,8 +12,7 @@ namespace impeller { GlyphAtlasContext::GlyphAtlasContext(GlyphAtlas::Type type) - : atlas_(std::make_shared(type, /*initial_generation=*/0)), - atlas_size_(ISize(0, 0)) {} + : atlas_(std::make_shared(type)), atlas_size_(ISize(0, 0)) {} GlyphAtlasContext::~GlyphAtlasContext() {} @@ -46,8 +45,7 @@ void GlyphAtlasContext::UpdateRectPacker( rect_packer_ = std::move(rect_packer); } -GlyphAtlas::GlyphAtlas(Type type, size_t initial_generation) - : type_(type), generation_(initial_generation) {} +GlyphAtlas::GlyphAtlas(Type type) : type_(type) {} GlyphAtlas::~GlyphAtlas() = default; @@ -67,14 +65,6 @@ void GlyphAtlas::SetTexture(std::shared_ptr texture) { texture_ = std::move(texture); } -size_t GlyphAtlas::GetAtlasGeneration() const { - return generation_; -} - -void GlyphAtlas::SetAtlasGeneration(size_t generation) { - generation_ = generation; -} - void GlyphAtlas::AddTypefaceGlyphPositionAndBounds(const FontGlyphPair& pair, Rect position, Rect bounds) { diff --git a/impeller/typographer/glyph_atlas.h b/impeller/typographer/glyph_atlas.h index bf11ff056ae24..87c5490318387 100644 --- a/impeller/typographer/glyph_atlas.h +++ b/impeller/typographer/glyph_atlas.h @@ -58,9 +58,8 @@ class GlyphAtlas { /// @brief Create an empty glyph atlas. /// /// @param[in] type How the glyphs are represented in the texture. - /// @param[in] initial_generation the atlas generation. /// - GlyphAtlas(Type type, size_t initial_generation); + explicit GlyphAtlas(Type type); ~GlyphAtlas(); @@ -143,22 +142,9 @@ class GlyphAtlas { /// FontGlyphAtlas* GetOrCreateFontGlyphAtlas(const ScaledFont& scaled_font); - //---------------------------------------------------------------------------- - /// @brief Retrieve the generation id for this glyph atlas. - /// - /// The generation id is used to match with a TextFrame to - /// determine if the frame is guaranteed to already be populated - /// in the atlas. - size_t GetAtlasGeneration() const; - - //---------------------------------------------------------------------------- - /// @brief Update the atlas generation. - void SetAtlasGeneration(size_t value); - private: const Type type_; std::shared_ptr texture_; - size_t generation_ = 0; std::unordered_map& a, - const std::optional& b) { - if (!a.has_value() && !b.has_value()) { - return true; - } - if (a.has_value() && b.has_value()) { - return GlyphProperties::Equal{}(a.value(), b.value()); - } - return false; -} -} // namespace - TextFrame::TextFrame() = default; TextFrame::TextFrame(std::vector& runs, Rect bounds, bool has_color) @@ -51,7 +37,7 @@ bool TextFrame::HasColor() const { } // static -Scalar TextFrame::RoundScaledFontSize(Scalar scale) { +Scalar TextFrame::RoundScaledFontSize(Scalar scale, Scalar point_size) { // An arbitrarily chosen maximum text scale to ensure that regardless of the // CTM, a glyph will fit in the atlas. If we clamp significantly, this may // reduce fidelity but is preferable to the alternative of failing to render. @@ -101,12 +87,6 @@ Point TextFrame::ComputeSubpixelPosition( void TextFrame::SetPerFrameData(Scalar scale, Point offset, std::optional properties) { - if (!ScalarNearlyEqual(scale_, scale) || - !ScalarNearlyEqual(offset_.x, offset.x) || - !ScalarNearlyEqual(offset_.y, offset.y) || - !TextPropertiesEquals(properties_, properties)) { - bound_values_.clear(); - } scale_ = scale; offset_ = offset; properties_ = properties; @@ -141,15 +121,7 @@ bool TextFrame::IsFrameComplete() const { } const FrameBounds& TextFrame::GetFrameBounds(size_t index) const { - return bound_values_[index]; -} - -size_t TextFrame::GetAtlasGeneration() const { - return generation_; -} - -void TextFrame::SetAtlasGeneration(size_t value) { - generation_ = value; + return bound_values_.at(index); } } // namespace impeller diff --git a/impeller/typographer/text_frame.h b/impeller/typographer/text_frame.h index 067c8ebd05486..0604d6cc1eb6b 100644 --- a/impeller/typographer/text_frame.h +++ b/impeller/typographer/text_frame.h @@ -32,7 +32,7 @@ class TextFrame { Point offset, Scalar scale); - static Scalar RoundScaledFontSize(Scalar scale); + static Scalar RoundScaledFontSize(Scalar scale, Scalar point_size); //---------------------------------------------------------------------------- /// @brief The conservative bounding box for this text frame. @@ -84,18 +84,13 @@ class TextFrame { Point offset, std::optional properties); - // A generation id for the glyph atlas this text run was associated - // with. As long as the frame generation matches the atlas generation, - // the contents are guaranteed to be populated and do not need to be - // processed. - size_t GetAtlasGeneration() const; - TextFrame& operator=(TextFrame&& other) = default; TextFrame(const TextFrame& other) = default; private: friend class TypographerContextSkia; + friend class TypographerContextSTB; friend class LazyGlyphAtlas; Scalar GetScale() const; @@ -108,17 +103,14 @@ class TextFrame { void ClearFrameBounds(); - void SetAtlasGeneration(size_t value); - std::vector runs_; Rect bounds_; bool has_color_; // Data that is cached when rendering the text frame and is only - // valid for the current atlas generation. + // valid for a single frame. std::vector bound_values_; Scalar scale_ = 0; - size_t generation_ = 0; Point offset_; std::optional properties_; }; diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc index 4b8cc1cbc9606..6367314d9b397 100644 --- a/impeller/typographer/typographer_unittests.cc +++ b/impeller/typographer/typographer_unittests.cc @@ -508,7 +508,7 @@ TEST_P(TypographerTest, TextFrameInitialBoundsArePlaceholder) { GetContext()->GetIdleWaiter()); auto atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, - GlyphAtlas::Type::kAlphaBitmap, /*scale=*/1.0f, + GlyphAtlas::Type::kAlphaBitmap, 1.0f, atlas_context, frame); // The glyph position in the atlas was not known when this value @@ -517,101 +517,14 @@ TEST_P(TypographerTest, TextFrameInitialBoundsArePlaceholder) { EXPECT_TRUE(frame->GetFrameBounds(0).is_placeholder); atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, - GlyphAtlas::Type::kAlphaBitmap, /*scale=*/1.0f, - atlas_context, frame); + GlyphAtlas::Type::kAlphaBitmap, 1.0f, atlas_context, + frame); // The second time the glyph is rendered, the bounds are correcly known. EXPECT_TRUE(frame->IsFrameComplete()); EXPECT_FALSE(frame->GetFrameBounds(0).is_placeholder); } -TEST_P(TypographerTest, TextFrameInvalidationWithScale) { - SkFont font = flutter::testing::CreateTestFontOfSize(12); - auto blob = SkTextBlob::MakeFromString( - "the quick brown fox jumped over the lazy dog.", font); - ASSERT_TRUE(blob); - auto frame = MakeTextFrameFromTextBlobSkia(blob); - - EXPECT_FALSE(frame->IsFrameComplete()); - - auto context = TypographerContextSkia::Make(); - auto atlas_context = - context->CreateGlyphAtlasContext(GlyphAtlas::Type::kAlphaBitmap); - auto host_buffer = HostBuffer::Create(GetContext()->GetResourceAllocator(), - GetContext()->GetIdleWaiter()); - - auto atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, - GlyphAtlas::Type::kAlphaBitmap, /*scale=*/1.0f, - atlas_context, frame); - - // The glyph position in the atlas was not known when this value - // was recorded. It is marked as a placeholder. - EXPECT_TRUE(frame->IsFrameComplete()); - EXPECT_TRUE(frame->GetFrameBounds(0).is_placeholder); - - // Change the scale and the glyph data will still be a placeholder, as the - // old data is no longer valid. - atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, - GlyphAtlas::Type::kAlphaBitmap, /*scale=*/2.0f, - atlas_context, frame); - - // The second time the glyph is rendered, the bounds are correcly known. - EXPECT_TRUE(frame->IsFrameComplete()); - EXPECT_TRUE(frame->GetFrameBounds(0).is_placeholder); -} - -TEST_P(TypographerTest, TextFrameAtlasGenerationTracksState) { - SkFont font = flutter::testing::CreateTestFontOfSize(12); - auto blob = SkTextBlob::MakeFromString( - "the quick brown fox jumped over the lazy dog.", font); - ASSERT_TRUE(blob); - auto frame = MakeTextFrameFromTextBlobSkia(blob); - - EXPECT_FALSE(frame->IsFrameComplete()); - - auto context = TypographerContextSkia::Make(); - auto atlas_context = - context->CreateGlyphAtlasContext(GlyphAtlas::Type::kAlphaBitmap); - auto host_buffer = HostBuffer::Create(GetContext()->GetResourceAllocator(), - GetContext()->GetIdleWaiter()); - - auto atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, - GlyphAtlas::Type::kAlphaBitmap, /*scale=*/1.0f, - atlas_context, frame); - - // The glyph position in the atlas was not known when this value - // was recorded. It is marked as a placeholder. - EXPECT_TRUE(frame->IsFrameComplete()); - EXPECT_TRUE(frame->GetFrameBounds(0).is_placeholder); - if (GetBackend() == PlaygroundBackend::kOpenGLES) { - // OpenGLES must always increase the atlas backend if the texture grows. - EXPECT_EQ(frame->GetAtlasGeneration(), 1u); - } else { - EXPECT_EQ(frame->GetAtlasGeneration(), 0u); - } - - atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, - GlyphAtlas::Type::kAlphaBitmap, /*scale=*/1.0f, - atlas_context, frame); - - // The second time the glyph is rendered, the bounds are correcly known. - EXPECT_TRUE(frame->IsFrameComplete()); - EXPECT_FALSE(frame->GetFrameBounds(0).is_placeholder); - if (GetBackend() == PlaygroundBackend::kOpenGLES) { - EXPECT_EQ(frame->GetAtlasGeneration(), 1u); - } else { - EXPECT_EQ(frame->GetAtlasGeneration(), 0u); - } - - // Force increase the generation. - atlas_context->GetGlyphAtlas()->SetAtlasGeneration(2u); - atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer, - GlyphAtlas::Type::kAlphaBitmap, /*scale=*/1.0f, - atlas_context, frame); - - EXPECT_EQ(frame->GetAtlasGeneration(), 2u); -} - } // namespace testing } // namespace impeller