[Impeller] fix colr/bitmap font color drawing. (#52871)

Jonah Williams · web-flow · commit 24f8fecb5bf0 · 2024-05-17T19:29:06.000Z
Fixes flutter/flutter#126546 Track the paint color used by Bitmap/Emoji/COLR fonts and use it as a cache key. This allows non-COLR glyphs in a COLR font to get the correct text color. For other kinds of fonts, we always record the color as black so there are no cache efficiency hits in general.
diff --git a/impeller/entity/contents/text_contents.cc b/impeller/entity/contents/text_contents.cc
@@ -166,7 +166,7 @@ bool TextContents::Render(const ContentContext& renderer,
           Scalar rounded_scale = TextFrame::RoundScaledFontSize(
               scale_, font.GetMetrics().point_size);
           const FontGlyphAtlas* font_atlas =
-              atlas->GetFontGlyphAtlas(font, rounded_scale);
+              atlas->GetFontGlyphAtlas(font, rounded_scale, frame_->GetColor());
           if (!font_atlas) {
             VALIDATION_LOG << "Could not find font in the atlas.";
             continue;
diff --git a/impeller/geometry/color.h b/impeller/geometry/color.h
@@ -253,6 +253,16 @@ struct Color {
     return {r, g, b, a};
   }
 
+  /**
+   * @brief Convert to ARGB 32 bit color.
+   *
+   * @return constexpr uint32_t
+   */
+  constexpr uint32_t ToARGB() const {
+    std::array<uint8_t, 4> result = ToR8G8B8A8();
+    return result[3] << 24 | result[0] << 16 | result[1] << 8 | result[2];
+  }
+
   static constexpr Color White() { return {1.0f, 1.0f, 1.0f, 1.0f}; }
 
   static constexpr Color Black() { return {0.0f, 0.0f, 0.0f, 1.0f}; }
diff --git a/impeller/typographer/backends/skia/BUILD.gn b/impeller/typographer/backends/skia/BUILD.gn
@@ -15,6 +15,7 @@ impeller_component("typographer_skia_backend") {
   ]
 
   public_deps = [
+    "//flutter/display_list",
     "//flutter/impeller/typographer",
     "//flutter/skia",
   ]
diff --git a/impeller/typographer/backends/skia/text_frame_skia.cc b/impeller/typographer/backends/skia/text_frame_skia.cc
@@ -6,6 +6,7 @@
 
 #include <vector>
 
+#include "display_list/dl_color.h"
 #include "flutter/fml/logging.h"
 #include "impeller/typographer/backends/skia/typeface_skia.h"
 #include "include/core/SkRect.h"
@@ -16,6 +17,15 @@
 
 namespace impeller {
 
+static Color ToColor(const flutter::DlColor& color) {
+  return {
+      static_cast<Scalar>(color.getRedF()),    //
+      static_cast<Scalar>(color.getGreenF()),  //
+      static_cast<Scalar>(color.getBlueF()),   //
+      static_cast<Scalar>(color.getAlphaF())   //
+  };
+}
+
 static Font ToFont(const SkTextBlobRunIterator& run) {
   auto& font = run.font();
   auto typeface = std::make_shared<TypefaceSkia>(font.refTypeface());
@@ -39,8 +49,10 @@ static Rect ToRect(const SkRect& rect) {
 static constexpr Scalar kScaleSize = 64.0f;
 
 std::shared_ptr<TextFrame> MakeTextFrameFromTextBlobSkia(
-    const sk_sp<SkTextBlob>& blob) {
+    const sk_sp<SkTextBlob>& blob,
+    flutter::DlColor dl_color) {
   bool has_color = false;
+  Color color = ToColor(dl_color);
   std::vector<TextRun> runs;
   for (SkTextBlobRunIterator run(blob.get()); !run.done(); run.next()) {
     // TODO(jonahwilliams): ask Skia for a public API to look this up.
@@ -89,7 +101,8 @@ std::shared_ptr<TextFrame> MakeTextFrameFromTextBlobSkia(
         continue;
     }
   }
-  return std::make_shared<TextFrame>(runs, ToRect(blob->bounds()), has_color);
+  return std::make_shared<TextFrame>(runs, ToRect(blob->bounds()), has_color,
+                                     has_color ? color : Color::Black());
 }
 
 }  // namespace impeller
diff --git a/impeller/typographer/backends/skia/text_frame_skia.h b/impeller/typographer/backends/skia/text_frame_skia.h
@@ -5,13 +5,15 @@
 #ifndef FLUTTER_IMPELLER_TYPOGRAPHER_BACKENDS_SKIA_TEXT_FRAME_SKIA_H_
 #define FLUTTER_IMPELLER_TYPOGRAPHER_BACKENDS_SKIA_TEXT_FRAME_SKIA_H_
 
+#include "display_list/dl_color.h"
 #include "impeller/typographer/text_frame.h"
 #include "third_party/skia/include/core/SkTextBlob.h"
 
 namespace impeller {
 
 std::shared_ptr<impeller::TextFrame> MakeTextFrameFromTextBlobSkia(
-    const sk_sp<SkTextBlob>& blob);
+    const sk_sp<SkTextBlob>& blob,
+    flutter::DlColor color = flutter::DlColor::kBlack());
 
 }  // namespace impeller
 
diff --git a/impeller/typographer/backends/skia/typographer_context_skia.cc b/impeller/typographer/backends/skia/typographer_context_skia.cc
@@ -242,7 +242,7 @@ static void DrawGlyph(SkCanvas* canvas,
   sk_font.setHinting(SkFontHinting::kSlight);
   sk_font.setEmbolden(metrics.embolden);
 
-  auto glyph_color = has_color ? SK_ColorWHITE : SK_ColorBLACK;
+  auto glyph_color = has_color ? scaled_font.color.ToARGB() : SK_ColorBLACK;
 
   SkPaint glyph_paint;
   glyph_paint.setColor(glyph_color);
@@ -331,8 +331,8 @@ std::shared_ptr<GlyphAtlas> TypographerContextSkia::CreateGlyphAtlas(
   std::vector<FontGlyphPair> new_glyphs;
   for (const auto& font_value : font_glyph_map) {
     const ScaledFont& scaled_font = font_value.first;
-    const FontGlyphAtlas* font_glyph_atlas =
-        last_atlas->GetFontGlyphAtlas(scaled_font.font, scaled_font.scale);
+    const FontGlyphAtlas* font_glyph_atlas = last_atlas->GetFontGlyphAtlas(
+        scaled_font.font, scaled_font.scale, scaled_font.color);
     if (font_glyph_atlas) {
       for (const Glyph& glyph : font_value.second) {
         if (!font_glyph_atlas->FindGlyphBounds(glyph)) {
diff --git a/impeller/typographer/backends/stb/text_frame_stb.cc b/impeller/typographer/backends/stb/text_frame_stb.cc
@@ -63,7 +63,7 @@ std::shared_ptr<TextFrame> MakeTextFrameSTB(
 
   std::vector<TextRun> runs = {run};
   return std::make_shared<TextFrame>(
-      runs, result.value_or(Rect::MakeLTRB(0, 0, 0, 0)), false);
+      runs, result.value_or(Rect::MakeLTRB(0, 0, 0, 0)), false, Color::Black());
 }
 
 }  // namespace impeller
diff --git a/impeller/typographer/backends/stb/typographer_context_stb.cc b/impeller/typographer/backends/stb/typographer_context_stb.cc
@@ -412,8 +412,8 @@ std::shared_ptr<GlyphAtlas> TypographerContextSTB::CreateGlyphAtlas(
   std::vector<FontGlyphPair> new_glyphs;
   for (const auto& font_value : font_glyph_map) {
     const ScaledFont& scaled_font = font_value.first;
-    const FontGlyphAtlas* font_glyph_atlas =
-        last_atlas->GetFontGlyphAtlas(scaled_font.font, scaled_font.scale);
+    const FontGlyphAtlas* font_glyph_atlas = last_atlas->GetFontGlyphAtlas(
+        scaled_font.font, scaled_font.scale, scaled_font.color);
     if (font_glyph_atlas) {
       for (const Glyph& glyph : font_value.second) {
         if (!font_glyph_atlas->FindGlyphBounds(glyph)) {
diff --git a/impeller/typographer/font_glyph_pair.h b/impeller/typographer/font_glyph_pair.h
@@ -20,6 +20,7 @@ namespace impeller {
 struct ScaledFont {
   Font font;
   Scalar scale;
+  Color color;
 };
 
 using FontGlyphMap = std::unordered_map<ScaledFont, std::unordered_set<Glyph>>;
@@ -40,15 +41,16 @@ struct FontGlyphPair {
 template <>
 struct std::hash<impeller::ScaledFont> {
   constexpr std::size_t operator()(const impeller::ScaledFont& sf) const {
-    return fml::HashCombine(sf.font.GetHash(), sf.scale);
+    return fml::HashCombine(sf.font.GetHash(), sf.scale, sf.color.ToARGB());
   }
 };
 
 template <>
 struct std::equal_to<impeller::ScaledFont> {
   constexpr bool operator()(const impeller::ScaledFont& lhs,
                             const impeller::ScaledFont& rhs) const {
-    return lhs.font.IsEqual(rhs.font) && lhs.scale == rhs.scale;
+    return lhs.font.IsEqual(rhs.font) && lhs.scale == rhs.scale &&
+           lhs.color == rhs.color;
   }
 };
 
diff --git a/impeller/typographer/glyph_atlas.cc b/impeller/typographer/glyph_atlas.cc
@@ -78,8 +78,9 @@ std::optional<Rect> GlyphAtlas::FindFontGlyphBounds(
 }
 
 const FontGlyphAtlas* GlyphAtlas::GetFontGlyphAtlas(const Font& font,
-                                                    Scalar scale) const {
-  const auto& found = font_atlas_map_.find({font, scale});
+                                                    Scalar scale,
+                                                    Color color) const {
+  const auto& found = font_atlas_map_.find({font, scale, color});
   if (found == font_atlas_map_.end()) {
     return nullptr;
   }
diff --git a/impeller/typographer/glyph_atlas.h b/impeller/typographer/glyph_atlas.h
@@ -126,7 +126,9 @@ class GlyphAtlas {
   ///             scale are not available in the atlas.  The pointer is only
   ///             valid for the lifetime of the GlyphAtlas.
   ///
-  const FontGlyphAtlas* GetFontGlyphAtlas(const Font& font, Scalar scale) const;
+  const FontGlyphAtlas* GetFontGlyphAtlas(const Font& font,
+                                          Scalar scale,
+                                          Color color) const;
 
  private:
   const Type type_;
diff --git a/impeller/typographer/text_frame.cc b/impeller/typographer/text_frame.cc
@@ -3,13 +3,20 @@
 // found in the LICENSE file.
 
 #include "impeller/typographer/text_frame.h"
+#include "impeller/typographer/font_glyph_pair.h"
 
 namespace impeller {
 
 TextFrame::TextFrame() = default;
 
-TextFrame::TextFrame(std::vector<TextRun>& runs, Rect bounds, bool has_color)
-    : runs_(std::move(runs)), bounds_(bounds), has_color_(has_color) {}
+TextFrame::TextFrame(std::vector<TextRun>& runs,
+                     Rect bounds,
+                     bool has_color,
+                     Color color)
+    : runs_(std::move(runs)),
+      bounds_(bounds),
+      has_color_(has_color),
+      color_(color) {}
 
 TextFrame::~TextFrame() = default;
 
@@ -30,6 +37,10 @@ GlyphAtlas::Type TextFrame::GetAtlasType() const {
                     : GlyphAtlas::Type::kAlphaBitmap;
 }
 
+Color TextFrame::GetColor() const {
+  return color_;
+}
+
 bool TextFrame::MaybeHasOverlapping() const {
   if (runs_.size() > 1) {
     return true;
@@ -78,7 +89,7 @@ void TextFrame::CollectUniqueFontGlyphPairs(FontGlyphMap& glyph_map,
     const Font& font = run.GetFont();
     auto rounded_scale =
         RoundScaledFontSize(scale, font.GetMetrics().point_size);
-    auto& set = glyph_map[{font, rounded_scale}];
+    auto& set = glyph_map[ScaledFont{font, rounded_scale, color_}];
     for (const TextRun::GlyphPosition& glyph_position :
          run.GetGlyphPositions()) {
 #if false
diff --git a/impeller/typographer/text_frame.h b/impeller/typographer/text_frame.h
@@ -20,7 +20,10 @@ class TextFrame {
  public:
   TextFrame();
 
-  TextFrame(std::vector<TextRun>& runs, Rect bounds, bool has_color);
+  TextFrame(std::vector<TextRun>& runs,
+            Rect bounds,
+            bool has_color,
+            Color color);
 
   ~TextFrame();
 
@@ -60,6 +63,15 @@ class TextFrame {
   ///             to apply opacity peephole optimizations to text blobs.
   bool MaybeHasOverlapping() const;
 
+  //----------------------------------------------------------------------------
+  /// @brief      Returns the paint color this text frame was recorded with.
+  ///
+  ///             Non-bitmap/COLR fonts always use a black text color here, but
+  ///             COLR fonts can potentially use the paint color in the glyph
+  ///             atlas, so this color must be considered as part of the cache
+  ///             key.
+  Color GetColor() const;
+
   //----------------------------------------------------------------------------
   /// @brief      The type of atlas this run should be emplaced in.
   GlyphAtlas::Type GetAtlasType() const;
@@ -71,7 +83,8 @@ class TextFrame {
  private:
   std::vector<TextRun> runs_;
   Rect bounds_;
-  bool has_color_ = false;
+  bool has_color_;
+  Color color_;
 };
 
 }  // namespace impeller
diff --git a/impeller/typographer/typographer_unittests.cc b/impeller/typographer/typographer_unittests.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "display_list/dl_color.h"
 #include "flutter/display_list/testing/dl_test_snippets.h"
 #include "flutter/testing/testing.h"
 #include "gtest/gtest.h"
@@ -10,6 +11,7 @@
 #include "impeller/playground/playground_test.h"
 #include "impeller/typographer/backends/skia/text_frame_skia.h"
 #include "impeller/typographer/backends/skia/typographer_context_skia.h"
+#include "impeller/typographer/font_glyph_pair.h"
 #include "impeller/typographer/lazy_glyph_atlas.h"
 #include "impeller/typographer/rectangle_packer.h"
 #include "third_party/skia/include/core/SkFont.h"
@@ -42,6 +44,22 @@ static std::shared_ptr<GlyphAtlas> CreateGlyphAtlas(
                                                atlas_context, font_glyph_map);
 }
 
+static std::shared_ptr<GlyphAtlas> CreateGlyphAtlas(
+    Context& context,
+    const TypographerContext* typographer_context,
+    HostBuffer& host_buffer,
+    GlyphAtlas::Type type,
+    Scalar scale,
+    const std::shared_ptr<GlyphAtlasContext>& atlas_context,
+    const std::vector<std::shared_ptr<TextFrame>>& frames) {
+  FontGlyphMap font_glyph_map;
+  for (auto& frame : frames) {
+    frame->CollectUniqueFontGlyphPairs(font_glyph_map, scale);
+  }
+  return typographer_context->CreateGlyphAtlas(context, type, host_buffer,
+                                               atlas_context, font_glyph_map);
+}
+
 TEST_P(TypographerTest, CanConvertTextBlob) {
   SkFont font = flutter::testing::CreateTestFontOfSize(12);
   auto blob = SkTextBlob::MakeFromString(
@@ -281,6 +299,61 @@ TEST_P(TypographerTest, MaybeHasOverlapping) {
   ASSERT_FALSE(frame_2->MaybeHasOverlapping());
 }
 
+TEST_P(TypographerTest, GlyphColorIsPartOfCacheKey) {
+  auto host_buffer = HostBuffer::Create(GetContext()->GetResourceAllocator());
+#if FML_OS_MACOSX
+  auto mapping = flutter::testing::OpenFixtureAsSkData("Apple Color Emoji.ttc");
+#else
+  auto mapping = flutter::testing::OpenFixtureAsSkData("NotoColorEmoji.ttf");
+#endif
+  ASSERT_TRUE(mapping);
+  sk_sp<SkFontMgr> font_mgr = txt::GetDefaultFontManager();
+  SkFont emoji_font(font_mgr->makeFromData(mapping), 50.0);
+
+  auto context = TypographerContextSkia::Make();
+  auto atlas_context =
+      context->CreateGlyphAtlasContext(GlyphAtlas::Type::kColorBitmap);
+
+  // Create two frames with the same character and a different color, expect
+  // that it adds a character.
+  auto frame = MakeTextFrameFromTextBlobSkia(
+      SkTextBlob::MakeFromString("😂", emoji_font), flutter::DlColor::kCyan());
+  auto frame_2 = MakeTextFrameFromTextBlobSkia(
+      SkTextBlob::MakeFromString("😂", emoji_font),
+      flutter::DlColor::kMagenta());
+
+  auto next_atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer,
+                                     GlyphAtlas::Type::kColorBitmap, 1.0f,
+                                     atlas_context, {frame, frame_2});
+
+  EXPECT_EQ(next_atlas->GetGlyphCount(), 2u);
+}
+
+TEST_P(TypographerTest, GlyphColorIsIgnoredForNonEmojiFonts) {
+  auto host_buffer = HostBuffer::Create(GetContext()->GetResourceAllocator());
+  sk_sp<SkFontMgr> font_mgr = txt::GetDefaultFontManager();
+  sk_sp<SkTypeface> typeface =
+      font_mgr->matchFamilyStyle("Arial", SkFontStyle::Normal());
+  SkFont sk_font(typeface, 0.5f);
+
+  auto context = TypographerContextSkia::Make();
+  auto atlas_context =
+      context->CreateGlyphAtlasContext(GlyphAtlas::Type::kColorBitmap);
+
+  // Create two frames with the same character and a different color, but as a
+  // non-emoji font the text frame constructor will ignore it.
+  auto frame = MakeTextFrameFromTextBlobSkia(
+      SkTextBlob::MakeFromString("A", sk_font), flutter::DlColor::kCyan());
+  auto frame_2 = MakeTextFrameFromTextBlobSkia(
+      SkTextBlob::MakeFromString("A", sk_font), flutter::DlColor::kMagenta());
+
+  auto next_atlas = CreateGlyphAtlas(*GetContext(), context.get(), *host_buffer,
+                                     GlyphAtlas::Type::kColorBitmap, 1.0f,
+                                     atlas_context, {frame, frame_2});
+
+  EXPECT_EQ(next_atlas->GetGlyphCount(), 1u);
+}
+
 TEST_P(TypographerTest, RectanglePackerAddsNonoverlapingRectangles) {
   auto packer = RectanglePacker::Factory(200, 100);
   ASSERT_NE(packer, nullptr);
diff --git a/third_party/txt/src/skia/paragraph_skia.cc b/third_party/txt/src/skia/paragraph_skia.cc
@@ -93,7 +93,7 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter {
         return;
       }
 
-      builder_->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(blob), x,
+      builder_->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(blob, dl_paints_[paint_id].getColor()), x,
                               y, dl_paints_[paint_id]);
       return;
     }
@@ -116,7 +116,7 @@ class DisplayListParagraphPainter : public skt::ParagraphPainter {
       paint.setMaskFilter(&filter);
     }
     if (impeller_enabled_) {
-      builder_->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(blob), x,
+      builder_->DrawTextFrame(impeller::MakeTextFrameFromTextBlobSkia(blob, paint.getColor()), x,
                               y, paint);
       return;
     }

Original file line number	Diff line number	Diff line change
`@@ -15,6 +15,7 @@ impeller_component("typographer_skia_backend") {`
`15`	`15`	`]`
`16`	`16`
`17`	`17`	`public_deps = [`
	`18`	`+ "//flutter/display_list",`
`18`	`19`	`"//flutter/impeller/typographer",`
`19`	`20`	`"//flutter/skia",`
`20`	`21`	`]`
Original file line number	Diff line number	Diff line change
`@@ -63,7 +63,7 @@ std::shared_ptr<TextFrame> MakeTextFrameSTB(`
`63`	`63`
`64`	`64`	`std::vector<TextRun> runs = {run};`
`65`	`65`	`return std::make_shared<TextFrame>(`
`66`		`- runs, result.value_or(Rect::MakeLTRB(0, 0, 0, 0)), false);`
	`66`	`+ runs, result.value_or(Rect::MakeLTRB(0, 0, 0, 0)), false, Color::Black());`
`67`	`67`	`}`
`68`	`68`
`69`	`69`	`} // namespace impeller`
Original file line number	Diff line number	Diff line change
`@@ -78,8 +78,9 @@ std::optional<Rect> GlyphAtlas::FindFontGlyphBounds(`
`78`	`78`	`}`
`79`	`79`
`80`	`80`	`const FontGlyphAtlas* GlyphAtlas::GetFontGlyphAtlas(const Font& font,`
`81`		`- Scalar scale) const {`
`82`		`- const auto& found = font_atlas_map_.find({font, scale});`
	`81`	`+ Scalar scale,`
	`82`	`+ Color color) const {`
	`83`	`+ const auto& found = font_atlas_map_.find({font, scale, color});`
`83`	`84`	`if (found == font_atlas_map_.end()) {`
`84`	`85`	`return nullptr;`
`85`	`86`	`}`