Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

jonahwilliams
Copy link
Contributor

@jonahwilliams jonahwilliams commented May 16, 2024

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.

@jonahwilliams jonahwilliams marked this pull request as ready for review May 17, 2024 02:55
@@ -253,6 +253,11 @@ struct Color {
return {r, g, b, a};
}

constexpr uint32_t ToSkia() const {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably call this ToARGB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -60,6 +63,8 @@ class TextFrame {
/// to apply opacity peephole optimizations to text blobs.
bool MaybeHasOverlapping() const;

Color GetColor() const;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need doc comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

jonahwilliams added 2 commits May 17, 2024 10:38
Copy link
Contributor

@matanlurey matanlurey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me. Nice tests and doc comments!

@@ -40,15 +41,17 @@ 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.red,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can simplify by calling sf.color.ToARGB() here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh good idea!

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label May 17, 2024
@auto-submit auto-submit bot merged commit 24f8fec into flutter:main May 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 17, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 17, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request May 17, 2024
…148581)

flutter/engine@5b3bf9a...93f1b5a

2024-05-17 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from ywxGmpIdjxBl2i7s2... to jKdOTTgE2Uq5OmJzT... (flutter/engine#52909)
2024-05-17 jonahwilliams@google.com [Impeller] fix colr/bitmap font color drawing. (flutter/engine#52871)
2024-05-17 skia-flutter-autoroll@skia.org Roll Skia from 6f7cb3d360b7 to 3f4c5038da37 (1 revision) (flutter/engine#52907)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from ywxGmpIdjxBl to jKdOTTgE2Uq5

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Impeller] Unable to change color of a bitmap font.
3 participants