From acaa12fd4555f956a137c390caa28f861b0f26fc Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 17 Jul 2023 17:29:57 -0700 Subject: [PATCH 1/8] [Impeller] round scale for glyph atlas cache to 1 decimal place. --- impeller/entity/contents/text_contents.cc | 11 +++++++++-- impeller/entity/contents/text_contents.h | 5 +++++ impeller/entity/entity_unittests.cc | 7 +++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index 339bb27549166..396f4fc6ae880 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/impeller/entity/contents/text_contents.cc @@ -25,6 +25,12 @@ TextContents::TextContents() = default; TextContents::~TextContents() = default; +// static +Scalar TextContents::RoundFontScale(Scalar value) { + auto res = std::ceil(value * 10.0) / 10.0; + return res; +} + void TextContents::SetTextFrame(const TextFrame& frame) { frame_ = frame; } @@ -78,8 +84,9 @@ std::optional TextContents::GetCoverage(const Entity& entity) const { void TextContents::PopulateGlyphAtlas( const std::shared_ptr& lazy_glyph_atlas, Scalar scale) { - lazy_glyph_atlas->AddTextFrame(frame_, scale); - scale_ = scale; + auto rounded = RoundFontScale(scale); + lazy_glyph_atlas->AddTextFrame(frame_, rounded); + scale_ = rounded; } bool TextContents::Render(const ContentContext& renderer, diff --git a/impeller/entity/contents/text_contents.h b/impeller/entity/contents/text_contents.h index c75daa0a50660..4220018875f57 100644 --- a/impeller/entity/contents/text_contents.h +++ b/impeller/entity/contents/text_contents.h @@ -55,6 +55,11 @@ class TextContents final : public Contents { const Entity& entity, RenderPass& pass) const override; + /// @brief Round font scales from arbitrary floating point values + /// (2.4223123) to one decimal place (2.4). This ensures that small + /// transform changes do not invalidate glyphs unecessarily. + static Scalar RoundFontScale(Scalar value); + private: TextFrame frame_; Scalar scale_ = 1.0; diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 05818e84270f7..da0b436c45c87 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -2435,6 +2435,13 @@ TEST_P(EntityTest, ColorFilterContentsWithLargeGeometry) { ASSERT_TRUE(OpenPlaygroundHere(entity)); } +TEST_P(EntityTest, TextContentsCeilsGlyphScaleToDecimal) { + ASSERT_EQ(TextContents::RoundFontScale(0.4321111f), 0.5f); + ASSERT_EQ(TextContents::RoundFontScale(0.5321111f), 0.6f); + ASSERT_EQ(TextContents::RoundFontScale(2.1f), 2.1f); + ASSERT_EQ(TextContents::RoundFontScale(0.0f), 0.0f); +} + } // namespace testing } // namespace impeller From db80bc4a9325cf8973a8e98f07e78da88f8deff0 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 17 Jul 2023 17:35:07 -0700 Subject: [PATCH 2/8] ++ --- impeller/entity/contents/text_contents.cc | 3 +-- impeller/entity/entity_unittests.cc | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index 396f4fc6ae880..6db7843f74a92 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/impeller/entity/contents/text_contents.cc @@ -27,8 +27,7 @@ TextContents::~TextContents() = default; // static Scalar TextContents::RoundFontScale(Scalar value) { - auto res = std::ceil(value * 10.0) / 10.0; - return res; + return std::round(value * 10.0) / 10.0; } void TextContents::SetTextFrame(const TextFrame& frame) { diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index da0b436c45c87..7c366be8c2c25 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -2436,8 +2436,8 @@ TEST_P(EntityTest, ColorFilterContentsWithLargeGeometry) { } TEST_P(EntityTest, TextContentsCeilsGlyphScaleToDecimal) { - ASSERT_EQ(TextContents::RoundFontScale(0.4321111f), 0.5f); - ASSERT_EQ(TextContents::RoundFontScale(0.5321111f), 0.6f); + ASSERT_EQ(TextContents::RoundFontScale(0.4321111f), 0.4f); + ASSERT_EQ(TextContents::RoundFontScale(0.5321111f), 0.5f); ASSERT_EQ(TextContents::RoundFontScale(2.1f), 2.1f); ASSERT_EQ(TextContents::RoundFontScale(0.0f), 0.0f); } From 15eba97168509470106ed8a8b9156aa95494000a Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 20 Jul 2023 19:29:59 -0700 Subject: [PATCH 3/8] ++ --- impeller/entity/contents/text_contents.cc | 14 +++++--------- impeller/entity/contents/text_contents.h | 5 ----- impeller/entity/entity_unittests.cc | 15 +++++++++++---- impeller/typographer/text_frame.cc | 11 ++++++++++- impeller/typographer/text_frame.h | 2 ++ 5 files changed, 28 insertions(+), 19 deletions(-) diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index 6db7843f74a92..e498a70b8483b 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/impeller/entity/contents/text_contents.cc @@ -25,11 +25,6 @@ TextContents::TextContents() = default; TextContents::~TextContents() = default; -// static -Scalar TextContents::RoundFontScale(Scalar value) { - return std::round(value * 10.0) / 10.0; -} - void TextContents::SetTextFrame(const TextFrame& frame) { frame_ = frame; } @@ -83,9 +78,8 @@ std::optional TextContents::GetCoverage(const Entity& entity) const { void TextContents::PopulateGlyphAtlas( const std::shared_ptr& lazy_glyph_atlas, Scalar scale) { - auto rounded = RoundFontScale(scale); - lazy_glyph_atlas->AddTextFrame(frame_, rounded); - scale_ = rounded; + lazy_glyph_atlas->AddTextFrame(frame_, scale); + scale_ = scale; } bool TextContents::Render(const ContentContext& renderer, @@ -185,8 +179,10 @@ bool TextContents::Render(const ContentContext& renderer, size_t vertex_offset = 0; for (const auto& run : frame_.GetRuns()) { const Font& font = run.GetFont(); + auto rounded_scale = TextFrame::RoundScaledFontSize(scale_, font.GetMetrics().point_size); + for (const auto& glyph_position : run.GetGlyphPositions()) { - FontGlyphPair font_glyph_pair{font, glyph_position.glyph, scale_}; + FontGlyphPair font_glyph_pair{font, glyph_position.glyph, rounded_scale}; auto maybe_atlas_glyph_bounds = atlas->FindFontGlyphBounds(font_glyph_pair); if (!maybe_atlas_glyph_bounds.has_value()) { diff --git a/impeller/entity/contents/text_contents.h b/impeller/entity/contents/text_contents.h index 4220018875f57..c75daa0a50660 100644 --- a/impeller/entity/contents/text_contents.h +++ b/impeller/entity/contents/text_contents.h @@ -55,11 +55,6 @@ class TextContents final : public Contents { const Entity& entity, RenderPass& pass) const override; - /// @brief Round font scales from arbitrary floating point values - /// (2.4223123) to one decimal place (2.4). This ensures that small - /// transform changes do not invalidate glyphs unecessarily. - static Scalar RoundFontScale(Scalar value); - private: TextFrame frame_; Scalar scale_ = 1.0; diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 7c366be8c2c25..8e009d6a83466 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -2436,10 +2436,17 @@ TEST_P(EntityTest, ColorFilterContentsWithLargeGeometry) { } TEST_P(EntityTest, TextContentsCeilsGlyphScaleToDecimal) { - ASSERT_EQ(TextContents::RoundFontScale(0.4321111f), 0.4f); - ASSERT_EQ(TextContents::RoundFontScale(0.5321111f), 0.5f); - ASSERT_EQ(TextContents::RoundFontScale(2.1f), 2.1f); - ASSERT_EQ(TextContents::RoundFontScale(0.0f), 0.0f); + // Font size < 25 + ASSERT_EQ(TextFrame::RoundScaledFontSize(0.4321111f, 12), 0.4f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(0.5321111f, 12), 0.5f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(2.1f, 12), 2.1f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(0.0f, 12), 0.0f); + + // Font size > 25 + ASSERT_EQ(TextFrame::RoundScaledFontSize(0.4321111f, 50), 0.43f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(0.5321111f, 50), 0.53f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(2.1f, 50), 2.1f); + ASSERT_EQ(TextFrame::RoundScaledFontSize(0.0f, 50), 0.0f); } } // namespace testing diff --git a/impeller/typographer/text_frame.cc b/impeller/typographer/text_frame.cc index 4191446f05462..9c4e4cf2dd597 100644 --- a/impeller/typographer/text_frame.cc +++ b/impeller/typographer/text_frame.cc @@ -79,13 +79,22 @@ bool TextFrame::MaybeHasOverlapping() const { return false; } +// static +Scalar TextFrame::RoundScaledFontSize(Scalar scale, Scalar point_size) { + if (point_size <= 25) { + return std::round(scale * 10) / 10; + } + return std::round(point_size * 100) / 100; +} + void TextFrame::CollectUniqueFontGlyphPairs(FontGlyphPair::Set& set, Scalar scale) const { for (const TextRun& run : GetRuns()) { const Font& font = run.GetFont(); + auto rounded_scale = RoundScaledFontSize(scale, font.GetMetrics().point_size); for (const TextRun::GlyphPosition& glyph_position : run.GetGlyphPositions()) { - set.insert({font, glyph_position.glyph, scale}); + set.insert({font, glyph_position.glyph, rounded_scale}); } } } diff --git a/impeller/typographer/text_frame.h b/impeller/typographer/text_frame.h index 512f7b705e3e3..834528c72b0c1 100644 --- a/impeller/typographer/text_frame.h +++ b/impeller/typographer/text_frame.h @@ -24,6 +24,8 @@ class TextFrame { void CollectUniqueFontGlyphPairs(FontGlyphPair::Set& set, Scalar scale) const; + static Scalar RoundScaledFontSize(Scalar scale, Scalar point_size); + //---------------------------------------------------------------------------- /// @brief The conservative bounding box for this text frame. /// From 39bb1392bbd777f21253888b183c662b0424c503 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Thu, 20 Jul 2023 19:31:48 -0700 Subject: [PATCH 4/8] ++ --- impeller/entity/contents/text_contents.cc | 6 ++++-- impeller/typographer/text_frame.cc | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc index e498a70b8483b..9b311c1a280bc 100644 --- a/impeller/entity/contents/text_contents.cc +++ b/impeller/entity/contents/text_contents.cc @@ -179,10 +179,12 @@ bool TextContents::Render(const ContentContext& renderer, size_t vertex_offset = 0; for (const auto& run : frame_.GetRuns()) { const Font& font = run.GetFont(); - auto rounded_scale = TextFrame::RoundScaledFontSize(scale_, font.GetMetrics().point_size); + auto rounded_scale = TextFrame::RoundScaledFontSize( + scale_, font.GetMetrics().point_size); for (const auto& glyph_position : run.GetGlyphPositions()) { - FontGlyphPair font_glyph_pair{font, glyph_position.glyph, rounded_scale}; + FontGlyphPair font_glyph_pair{font, glyph_position.glyph, + rounded_scale}; auto maybe_atlas_glyph_bounds = atlas->FindFontGlyphBounds(font_glyph_pair); if (!maybe_atlas_glyph_bounds.has_value()) { diff --git a/impeller/typographer/text_frame.cc b/impeller/typographer/text_frame.cc index 9c4e4cf2dd597..8312da43c5357 100644 --- a/impeller/typographer/text_frame.cc +++ b/impeller/typographer/text_frame.cc @@ -91,7 +91,8 @@ void TextFrame::CollectUniqueFontGlyphPairs(FontGlyphPair::Set& set, Scalar scale) const { for (const TextRun& run : GetRuns()) { const Font& font = run.GetFont(); - auto rounded_scale = RoundScaledFontSize(scale, font.GetMetrics().point_size); + auto rounded_scale = + RoundScaledFontSize(scale, font.GetMetrics().point_size); for (const TextRun::GlyphPosition& glyph_position : run.GetGlyphPositions()) { set.insert({font, glyph_position.glyph, rounded_scale}); From 046be21ea607daf967d6f242da8e559dd21a68ca Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Fri, 21 Jul 2023 08:58:40 -0700 Subject: [PATCH 5/8] ++ --- impeller/typographer/text_frame.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/typographer/text_frame.cc b/impeller/typographer/text_frame.cc index 8312da43c5357..1b56ecc5f3234 100644 --- a/impeller/typographer/text_frame.cc +++ b/impeller/typographer/text_frame.cc @@ -84,7 +84,7 @@ Scalar TextFrame::RoundScaledFontSize(Scalar scale, Scalar point_size) { if (point_size <= 25) { return std::round(scale * 10) / 10; } - return std::round(point_size * 100) / 100; + return std::round(scale * 100) / 100; } void TextFrame::CollectUniqueFontGlyphPairs(FontGlyphPair::Set& set, From 8670e5a0b1bfbc6c8c9f4f4863012e61b3c82cbb Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 24 Jul 2023 17:39:05 -0700 Subject: [PATCH 6/8] ++ --- impeller/typographer/text_frame.cc | 3 --- 1 file changed, 3 deletions(-) diff --git a/impeller/typographer/text_frame.cc b/impeller/typographer/text_frame.cc index 1b56ecc5f3234..2b6031feb08af 100644 --- a/impeller/typographer/text_frame.cc +++ b/impeller/typographer/text_frame.cc @@ -81,9 +81,6 @@ bool TextFrame::MaybeHasOverlapping() const { // static Scalar TextFrame::RoundScaledFontSize(Scalar scale, Scalar point_size) { - if (point_size <= 25) { - return std::round(scale * 10) / 10; - } return std::round(scale * 100) / 100; } From 2545fb5654d2a0441b6bfdf39d95728b31f28368 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 24 Jul 2023 17:40:18 -0700 Subject: [PATCH 7/8] ++ --- impeller/entity/entity_unittests.cc | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 9685cb53e4c07..2b5dc770f367b 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -2427,17 +2427,10 @@ TEST_P(EntityTest, ColorFilterContentsWithLargeGeometry) { } TEST_P(EntityTest, TextContentsCeilsGlyphScaleToDecimal) { - // Font size < 25 - ASSERT_EQ(TextFrame::RoundScaledFontSize(0.4321111f, 12), 0.4f); - ASSERT_EQ(TextFrame::RoundScaledFontSize(0.5321111f, 12), 0.5f); + 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); - - // Font size > 25 - ASSERT_EQ(TextFrame::RoundScaledFontSize(0.4321111f, 50), 0.43f); - ASSERT_EQ(TextFrame::RoundScaledFontSize(0.5321111f, 50), 0.53f); - ASSERT_EQ(TextFrame::RoundScaledFontSize(2.1f, 50), 2.1f); - ASSERT_EQ(TextFrame::RoundScaledFontSize(0.0f, 50), 0.0f); } } // namespace testing From d4f4231e0ccc858929f4505f753a532c6f7ac760 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Mon, 24 Jul 2023 20:24:00 -0700 Subject: [PATCH 8/8] add some debugging help text --- impeller/typographer/text_frame.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/impeller/typographer/text_frame.cc b/impeller/typographer/text_frame.cc index 2b6031feb08af..c62023666e93a 100644 --- a/impeller/typographer/text_frame.cc +++ b/impeller/typographer/text_frame.cc @@ -92,6 +92,13 @@ void TextFrame::CollectUniqueFontGlyphPairs(FontGlyphPair::Set& set, RoundScaledFontSize(scale, font.GetMetrics().point_size); for (const TextRun::GlyphPosition& glyph_position : run.GetGlyphPositions()) { +#if false +// Glyph size error due to RoundScaledFontSize usage above. +if (rounded_scale != scale) { + auto delta = std::abs(rounded_scale - scale); + FML_LOG(ERROR) << glyph_position.glyph.bounds.size * delta; +} +#endif set.insert({font, glyph_position.glyph, rounded_scale}); } }