Revert "[Impeller] cache even more text frame data to skip lookups. (#56798)"
This reverts commit 65c8c6e8d7581c1a87504960c51fa4b21282f9bb.
diff --git a/impeller/display_list/dl_dispatcher.cc b/impeller/display_list/dl_dispatcher.cc
index 3fb650d..86974a8 100644
--- a/impeller/display_list/dl_dispatcher.cc
+++ b/impeller/display_list/dl_dispatcher.cc
@@ -1105,9 +1105,8 @@
// 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 4d4a130..dfa4c42 100644
--- a/impeller/entity/contents/text_contents.cc
+++ b/impeller/entity/contents/text_contents.cc
@@ -175,7 +175,8 @@
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 @@
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<FrameBounds> maybe_atlas_glyph_bounds =
font_atlas->FindGlyphBounds(SubpixelGlyph{
diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc
index 756203a..8fee465 100644
--- a/impeller/entity/entity_unittests.cc
+++ b/impeller/entity/entity_unittests.cc
@@ -2120,11 +2120,11 @@
}
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 13dc254..80b4993 100644
--- a/impeller/typographer/backends/skia/typographer_context_skia.cc
+++ b/impeller/typographer/backends/skia/typographer_context_skia.cc
@@ -413,20 +413,18 @@
const std::vector<std::shared_ptr<TextFrame>>& text_frames) {
std::vector<FontGlyphPair> new_glyphs;
std::vector<Rect> 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 @@
if (atlas_context->GetAtlasSize().height >= max_texture_height ||
context.GetBackendType() == Context::BackendType::kOpenGLES) {
blit_old_atlas = false;
- new_atlas = std::make_shared<GlyphAtlas>(
- type, /*initial_generation=*/last_atlas->GetAtlasGeneration() + 1);
+ new_atlas = std::make_shared<GlyphAtlas>(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 e3df2da..e530f5b 100644
--- a/impeller/typographer/font_glyph_pair.h
+++ b/impeller/typographer/font_glyph_pair.h
@@ -20,17 +20,6 @@
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 @@
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 9803866..0935938 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<GlyphAtlas>(type, /*initial_generation=*/0)),
- atlas_size_(ISize(0, 0)) {}
+ : atlas_(std::make_shared<GlyphAtlas>(type)), atlas_size_(ISize(0, 0)) {}
GlyphAtlasContext::~GlyphAtlasContext() {}
@@ -46,8 +45,7 @@
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 @@
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 bf11ff0..87c5490 100644
--- a/impeller/typographer/glyph_atlas.h
+++ b/impeller/typographer/glyph_atlas.h
@@ -58,9 +58,8 @@
/// @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 @@
///
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> texture_;
- size_t generation_ = 0;
std::unordered_map<ScaledFont,
FontGlyphAtlas,
diff --git a/impeller/typographer/text_frame.cc b/impeller/typographer/text_frame.cc
index 3535fb2..b72bb62 100644
--- a/impeller/typographer/text_frame.cc
+++ b/impeller/typographer/text_frame.cc
@@ -3,25 +3,11 @@
// found in the LICENSE file.
#include "impeller/typographer/text_frame.h"
-#include "impeller/geometry/scalar.h"
#include "impeller/typographer/font.h"
#include "impeller/typographer/font_glyph_pair.h"
namespace impeller {
-namespace {
-static bool TextPropertiesEquals(const std::optional<GlyphProperties>& a,
- const std::optional<GlyphProperties>& 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<TextRun>& runs, Rect bounds, bool has_color)
@@ -51,7 +37,7 @@
}
// 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 @@
void TextFrame::SetPerFrameData(Scalar scale,
Point offset,
std::optional<GlyphProperties> 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 @@
}
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 067c8eb..0604d6c 100644
--- a/impeller/typographer/text_frame.h
+++ b/impeller/typographer/text_frame.h
@@ -32,7 +32,7 @@
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 @@
Point offset,
std::optional<GlyphProperties> 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 @@
void ClearFrameBounds();
- void SetAtlasGeneration(size_t value);
-
std::vector<TextRun> 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<FrameBounds> bound_values_;
Scalar scale_ = 0;
- size_t generation_ = 0;
Point offset_;
std::optional<GlyphProperties> properties_;
};
diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc
index 4b8cc1c..6367314 100644
--- a/impeller/typographer/typographer_unittests.cc
+++ b/impeller/typographer/typographer_unittests.cc
@@ -508,7 +508,7 @@
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 @@
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
