From 9ebea5a90a0ab51b99a49054f1099b39cb427b42 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 25 Sep 2024 11:20:47 -0700 Subject: [PATCH] [Impeller] use public API for subpixel/color/emoji --- .../backends/skia/text_frame_skia.cc | 46 ++++++------------- .../backends/skia/text_frame_skia.h | 3 +- impeller/typographer/typographer_unittests.cc | 9 ++-- third_party/txt/src/skia/paragraph_skia.cc | 32 +++++++++---- 4 files changed, 44 insertions(+), 46 deletions(-) diff --git a/impeller/typographer/backends/skia/text_frame_skia.cc b/impeller/typographer/backends/skia/text_frame_skia.cc index 55f8ad9084c1e..22604c7690422 100644 --- a/impeller/typographer/backends/skia/text_frame_skia.cc +++ b/impeller/typographer/backends/skia/text_frame_skia.cc @@ -14,30 +14,10 @@ #include "third_party/skia/include/core/SkFontMetrics.h" #include "third_party/skia/include/core/SkPaint.h" #include "third_party/skia/include/core/SkRect.h" -#include "third_party/skia/src/core/SkStrikeSpec.h" // nogncheck #include "third_party/skia/src/core/SkTextBlobPriv.h" // nogncheck namespace impeller { -/// @brief Convert a Skia axis alignment into an Impeller alignment. -/// -/// This does not include a case for AxisAlignment::kNone, that should -/// be used if SkFont::isSubpixel is false. -static AxisAlignment ToAxisAligment(SkAxisAlignment aligment) { - switch (aligment) { - case SkAxisAlignment::kNone: - // Skia calls this case none, meaning alignment in both X and Y. - // Impeller will call it "all" since that is less confusing. "none" - // is reserved for no subpixel alignment. - return AxisAlignment::kAll; - case SkAxisAlignment::kX: - return AxisAlignment::kX; - case SkAxisAlignment::kY: - return AxisAlignment::kY; - } - FML_UNREACHABLE(); -} - static Font ToFont(const SkTextBlobRunIterator& run, AxisAlignment alignment) { auto& font = run.font(); auto typeface = std::make_shared(font.refTypeface()); @@ -59,18 +39,21 @@ static Rect ToRect(const SkRect& rect) { } std::shared_ptr MakeTextFrameFromTextBlobSkia( - const sk_sp& blob) { - bool has_color = false; + const sk_sp& blob, + bool contains_color_glyphs) { + Glyph::Type type = + contains_color_glyphs ? Glyph::Type::kBitmap : Glyph::Type::kPath; std::vector runs; + for (SkTextBlobRunIterator run(blob.get()); !run.done(); run.next()) { - // TODO(112005): Ask Skia for a public API to look this up. This is using a - // private API today. - SkStrikeSpec strikeSpec = SkStrikeSpec::MakeWithNoDevice(run.font()); - SkBulkGlyphMetricsAndPaths paths{strikeSpec}; AxisAlignment alignment = AxisAlignment::kNone; if (run.font().isSubpixel()) { - alignment = ToAxisAligment( - strikeSpec.createScalerContext()->computeAxisAlignmentForHText()); + // If baseline snap or color glyphs, then no axis alignment + if (run.font().isBaselineSnap() || contains_color_glyphs) { + alignment = AxisAlignment::kNone; + } else { + alignment = AxisAlignment::kX; + } } const auto glyph_count = run.glyphCount(); @@ -83,10 +66,6 @@ std::shared_ptr MakeTextFrameFromTextBlobSkia( // kFull_Positioning has two scalars per glyph. const SkPoint* glyph_points = run.points(); const SkPoint* point = glyph_points + i; - Glyph::Type type = paths.glyph(glyphs[i])->isColor() - ? Glyph::Type::kBitmap - : Glyph::Type::kPath; - has_color |= type == Glyph::Type::kBitmap; positions.emplace_back( TextRun::GlyphPosition{Glyph{glyphs[i], type}, Point{ point->x(), @@ -102,7 +81,8 @@ std::shared_ptr MakeTextFrameFromTextBlobSkia( continue; } } - return std::make_shared(runs, ToRect(blob->bounds()), has_color); + return std::make_shared(runs, ToRect(blob->bounds()), + contains_color_glyphs); } } // namespace impeller diff --git a/impeller/typographer/backends/skia/text_frame_skia.h b/impeller/typographer/backends/skia/text_frame_skia.h index fb69bab359f20..37e2f11142888 100644 --- a/impeller/typographer/backends/skia/text_frame_skia.h +++ b/impeller/typographer/backends/skia/text_frame_skia.h @@ -12,7 +12,8 @@ namespace impeller { std::shared_ptr MakeTextFrameFromTextBlobSkia( - const sk_sp& blob); + const sk_sp& blob, + bool contains_color_glyphs = false); } // namespace impeller diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc index 7c8e9cd08852c..5279fb8ba436b 100644 --- a/impeller/typographer/typographer_unittests.cc +++ b/impeller/typographer/typographer_unittests.cc @@ -140,7 +140,8 @@ TEST_P(TypographerTest, LazyAtlasTracksColor) { lazy_atlas.AddTextFrame(*frame, 1.0f, {0, 0}, {}); frame = MakeTextFrameFromTextBlobSkia( - SkTextBlob::MakeFromString("😀 ", emoji_font)); + SkTextBlob::MakeFromString("😀 ", emoji_font), + /*contains_color_glyphs=*/true); ASSERT_TRUE(frame->GetAtlasType() == GlyphAtlas::Type::kColorBitmap); @@ -305,9 +306,11 @@ TEST_P(TypographerTest, GlyphColorIsPartOfCacheKey) { // Create two frames with the same character and a different color, expect // that it adds a character. auto frame = MakeTextFrameFromTextBlobSkia( - SkTextBlob::MakeFromString("😂", emoji_font)); + SkTextBlob::MakeFromString("😂", emoji_font), + /*contains_color_glyphs=*/true); auto frame_2 = MakeTextFrameFromTextBlobSkia( - SkTextBlob::MakeFromString("😂", emoji_font)); + SkTextBlob::MakeFromString("😂", emoji_font), + /*contains_color_glyphs=*/true); auto properties = { GlyphProperties{.color = Color::Red()}, GlyphProperties{.color = Color::Blue()}, diff --git a/third_party/txt/src/skia/paragraph_skia.cc b/third_party/txt/src/skia/paragraph_skia.cc index b3521dd529515..2a30909ad0efa 100644 --- a/third_party/txt/src/skia/paragraph_skia.cc +++ b/third_party/txt/src/skia/paragraph_skia.cc @@ -67,10 +67,13 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { /// See https://github.com/flutter/flutter/issues/126673. It /// probably makes sense to eventually make this a compile-time /// decision (i.e. with `#ifdef`) instead of a runtime option. - DisplayListParagraphPainter(DisplayListBuilder* builder, - const std::vector& dl_paints, - bool impeller_enabled) + DisplayListParagraphPainter( + DisplayListBuilder* builder, + const std::unique_ptr& paragraph, + const std::vector& dl_paints, + bool impeller_enabled) : builder_(builder), + paragraph_(paragraph), dl_paints_(dl_paints), impeller_enabled_(impeller_enabled) {} @@ -92,7 +95,10 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { // If there is no path, this is an emoji and should be drawn as is, // ignoring the color source. if (path.isEmpty()) { - builder_->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(blob), + auto contains_color_glyphs = + paragraph_->containsColorFontOrBitmap(blob.get()); + builder_->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia( + blob, contains_color_glyphs), x, y, dl_paints_[paint_id]); return; @@ -103,8 +109,11 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { builder_->DrawPath(transformed, dl_paints_[paint_id]); return; } - builder_->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(blob), x, - y, dl_paints_[paint_id]); + auto contains_color_glyphs = + paragraph_->containsColorFontOrBitmap(blob.get()); + builder_->DrawTextFrame( + impeller::MakeTextFrameFromTextBlobSkia(blob, contains_color_glyphs), + x, y, dl_paints_[paint_id]); return; } #endif // IMPELLER_SUPPORTS_RENDERING @@ -126,8 +135,11 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { paint.setMaskFilter(&filter); } if (impeller_enabled_) { - builder_->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(blob), x, - y, paint); + auto contains_color_glyphs = + paragraph_->containsColorFontOrBitmap(blob.get()); + builder_->DrawTextFrame( + impeller::MakeTextFrameFromTextBlobSkia(blob, contains_color_glyphs), + x, y, paint); return; } builder_->DrawTextBlob(blob, x, y, paint); @@ -207,6 +219,7 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter { } DisplayListBuilder* builder_; + const std::unique_ptr& paragraph_; const std::vector& dl_paints_; const bool impeller_enabled_; }; @@ -303,7 +316,8 @@ void ParagraphSkia::Layout(double width) { } bool ParagraphSkia::Paint(DisplayListBuilder* builder, double x, double y) { - DisplayListParagraphPainter painter(builder, dl_paints_, impeller_enabled_); + DisplayListParagraphPainter painter(builder, paragraph_, dl_paints_, + impeller_enabled_); paragraph_->paint(&painter, x, y); return true; }