[Impeller] Use scaled font to determine bounds, match Skia position rounding behavior, add subpixel X/Y/All/None positioning. #53042

jonahwilliams · 2024-05-25T20:00:12Z

Multiple fixes to text rendering that match skia behavior on almost all bugs I've found, except for the glyphs are still slightly too fine for some CJK text. The fixes are:

Compute the gylph size in the typographer context, using text size * scale factor text, instead of computing smaller bounds and scaling it up. This was not accurate and as a result we would positon glyphs incorrect by multiple pixels sometimes, causing uneven rows.
Match Skia's rounding behavior. previously we were rounding in multiple places, Skia rounds once. This is important to prevent jumping.
Use 4 subpixel X positions for rendering. This is the big one that ensures the visible layout matches exactly. Adds support for Y, both, and none positioning too. I couldn't find any examples of just Y or both. Some fonts may specify that have no subpixel positioning. So we don't bother to compute it for those.

Fixes flutter/flutter#138386 / mostly, except slightly not bold enough.
Fixes flutter/flutter#147577 / mostly, except slightly not bold enough.
Fixes flutter/flutter#140475
Fixes flutter/flutter#141467
Fixes flutter/flutter#135523
Fixes flutter/flutter#127815

flutter-dashboard · 2024-05-25T20:00:14Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

impeller/typographer/backends/skia/text_frame_skia.cc

jonahwilliams · 2024-05-27T00:26:17Z

impeller/typographer/backends/skia/text_frame_skia.cc

    const auto* glyphs = run.glyphs();
    switch (run.positioning()) {
      case SkTextBlobRunIterator::kFull_Positioning: {
-        std::vector<SkRect> glyph_bounds;


These bounds are wrong, this computation is moved into the typographer contents where the exact scale is applied to the font size.

impeller/typographer/backends/skia/typographer_context_skia.cc

impeller/typographer/backends/stb/text_frame_stb.cc

flutter-dashboard · 2024-05-27T04:42:13Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #53042 at sha 051254a

jonahwilliams · 2024-05-27T17:45:37Z

Remaining issue is that many CJK fonts have fractional Y positions too, fixing that will truly fix all issues.

jonahwilliams · 2024-05-27T19:12:12Z

Actually its not y alignment,it is just slightly less bold. Filled flutter/flutter#149142

jonahwilliams · 2024-05-27T19:17:04Z

impeller/aiks/canvas.cc

  text_contents->SetTextFrame(text_frame);
  text_contents->SetColor(paint.color);
  text_contents->SetForceTextColor(paint.mask_blur_descriptor.has_value());
+  text_contents->SetOffset(position);


We need to use the original offset to compute the subpixel position now.

impeller/entity/contents/text_contents.cc

flutter-dashboard · 2024-05-27T20:26:00Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53042 at sha 8a08278

chinmaygarde · 2024-05-27T20:10:17Z

impeller/typographer/backends/skia/text_frame_skia.cc

+#include "impeller/typographer/font.h"
+#include "impeller/typographer/glyph.h"
+
+#include "include/core/SkPaint.h"


Nit: No newline here and include the third_party/skia before the include like the rest.

chinmaygarde · 2024-05-27T20:12:06Z

impeller/entity/contents/text_contents.cc


 bool TextContents::CanInheritOpacity(const Entity& entity) const {
-  return !frame_->MaybeHasOverlapping();
+  return false;


Not sure I follow why this isn't true anymore. Add a comment maybe?

comment added

chinmaygarde · 2024-05-27T20:14:29Z

impeller/typographer/backends/skia/text_frame_skia.cc

+      switch (
+          strikeSpec.createScalerContext()->computeAxisAlignmentForHText()) {
+        case SkAxisAlignment::kNone:
+          // Skia calls this case none, meaning alignment in both X and Y.


Good call and thanks for documenting. Could also move it into a method AxisAlignment ToAxisAligment(SkAxixalignment align); with the requisite documentation.

chinmaygarde · 2024-05-27T20:18:22Z

impeller/typographer/backends/skia/typographer_context_skia.cc

-        location_in_atlas.y() + height_adjustment,  //
-        glyph_size.width,                           //
-        glyph_size.height                           //
+        location_in_atlas.x() + 1,                      //


I don't follow this adjustment. Perhaps a short comment? If I had to guess, perhaps to account for errors in sample locations.

I see the correction to the position later. But still not sure why?

This is part of the glyph padding so that we can avoid clearing the atlas. Though I didn't check in this PR, eventually we should be able to remove the clear.

chinmaygarde · 2024-05-27T20:22:16Z

impeller/typographer/backends/skia/typographer_context_skia.cc

  // ---------------------------------------------------------------------------
  // Step 1: Determine if the atlas type and font glyph pairs are compatible
-  //         with the current atlas and reuse if possible.
+  //         with the current atlas and reuse if possible. Compute glyph sizes


Also comute glyph sizes.

Added more details

chinmaygarde · 2024-05-27T20:24:33Z

impeller/typographer/font.h

+//------------------------------------------------------------------------------
+/// @brief      Determines the axis along which there is subpixel positioning.
+///
+enum class AxisAlignment : uint8_t {


This is find of course and not suggesting this be modified. But we also have type safe masks now. So this could be mask values with a mask defined as using AxisAlignmentMask = Mask<AxisAlignment>;.

I'm not really using this as a Mask right now, at least I dont think. I think I could rewrite things a bit to treat axis alignment as an off or on mask for which of X/Y have subpixel positioning?

Nah, this is fine.

chinmaygarde · 2024-05-27T20:25:08Z

impeller/typographer/font_glyph_pair.h

+///
+struct SubpixelGlyph {
+  Glyph glyph;
+  Point subpixel;


Perhaps subpixel_offset?

flutter-dashboard · 2024-05-28T05:38:02Z

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #53042 at sha 4ed6ce2

…position rounding behavior, add subpixel X/Y/All/None positioning. (flutter/engine#53042)

…149169) flutter/engine@b175108...d032390 2024-05-28 [email protected] Roll Skia from 312160c7c920 to 848d9498fd68 (1 revision) (flutter/engine#53059) 2024-05-28 [email protected] [Impeller] Use scaled font to determine bounds, match Skia position rounding behavior, add subpixel X/Y/All/None positioning. (flutter/engine#53042) 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 [email protected],[email protected],[email protected] 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

…lutter#149169) flutter/engine@b175108...d032390 2024-05-28 [email protected] Roll Skia from 312160c7c920 to 848d9498fd68 (1 revision) (flutter/engine#53059) 2024-05-28 [email protected] [Impeller] Use scaled font to determine bounds, match Skia position rounding behavior, add subpixel X/Y/All/None positioning. (flutter/engine#53042) 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 [email protected],[email protected],[email protected] 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

delfme · 2024-11-23T18:47:30Z

Thx you for these fixes! If related, it would be great if this other issue could also be addressed: flutter/flutter#120857

Currently, fontWeight is inconsistent on iOS: text appears thinner in light mode and thicker in dark mode. For instance, in light mode, a 'normal' fontWeight looks closer to 'light' than to 'normal'.

The only viable workaround we’ve found is to wrap two Text into a Stack, using the second one to apply an outer stroke that tricks the user’s eye into perceiving a native-like font weight. However, this solution requires Text.rich and results in dirt code.

jonahwilliams added 6 commits May 22, 2024 19:10

work in progress.

abb1db0

++

6721d40

bounds

455fc3d

minor refactoring.

4691b26

hack turn off STB

fd0b3d3

++

06b1fa8

github-actions bot added the e: impeller label May 25, 2024

jonahwilliams added 4 commits May 25, 2024 14:50

blendin'

40dac0d

++

ed6d6cd

hacky compute subpixel positions.

f5d39bf

format

e6d61d1

jonahwilliams changed the title ~~[Impeller] WIP use scaled font to determine bounds, match Skia position rounding behavior.~~ [Impeller] WIP use scaled font to determine bounds, match Skia position rounding behavior, add subpixel X positioning. May 26, 2024

jonahwilliams commented May 26, 2024

View reviewed changes

impeller/typographer/backends/skia/text_frame_skia.cc Outdated Show resolved Hide resolved

Refactoring.

c32c532

jonahwilliams commented May 27, 2024

View reviewed changes

impeller/typographer/backends/skia/typographer_context_skia.cc Outdated Show resolved Hide resolved

jonahwilliams commented May 27, 2024

View reviewed changes

impeller/typographer/backends/stb/text_frame_stb.cc Show resolved Hide resolved

jonahwilliams added 6 commits May 26, 2024 17:48

testing pass.

33a410f

++

949b2eb

++

93bfcde

++

177e86a

stb fixes and golden update.

d8e910d

++

051254a

jonahwilliams changed the title ~~[Impeller] WIP use scaled font to determine bounds, match Skia position rounding behavior, add subpixel X positioning.~~ [Impeller] Use scaled font to determine bounds, match Skia position rounding behavior, add subpixel X positioning. May 27, 2024

jonahwilliams mentioned this pull request May 27, 2024

test scaled text rendering. #53050

Closed

flutter-dashboard bot added the will affect goldens label May 27, 2024

support Y subpixel and non-subpixel rendering.

d5e039b

jonahwilliams mentioned this pull request May 27, 2024

[Impeller] font weight and alignment looks off with CJK characters. flutter/flutter#138386

Closed

2 tasks

jonahwilliams changed the title ~~[Impeller] Use scaled font to determine bounds, match Skia position rounding behavior, add subpixel X positioning.~~ [Impeller] Use scaled font to determine bounds, match Skia position rounding behavior, add subpixel X/Y/All/None positioning. May 27, 2024

jonahwilliams requested review from bdero and chinmaygarde May 27, 2024 19:16

jonahwilliams commented May 27, 2024

View reviewed changes

impeller/entity/contents/text_contents.cc Outdated Show resolved Hide resolved

jonahwilliams added 2 commits May 27, 2024 12:19

++

d389be7

position rounding.

8a08278

chinmaygarde approved these changes May 27, 2024

View reviewed changes

jonahwilliams added 3 commits May 27, 2024 21:17

chinmay review.

b2204f7

++

ec4f560

Merge branch 'main' into clamp

4ed6ce2

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label May 28, 2024

auto-submit bot merged commit 9f448d3 into flutter:main May 28, 2024

jonahwilliams deleted the clamp branch May 28, 2024 06:43

engine-flutter-autoroll mentioned this pull request May 28, 2024

Roll Flutter Engine from b1751088c7e9 to d0323905fc2f (2 revisions) flutter/flutter#149169

Merged

jonahwilliams mentioned this pull request Jun 3, 2024

Cyrillic alphabet rendered incorrectly for Impeller in Android flutter/flutter#149576

Closed

mrcendre mentioned this pull request Jun 4, 2024

[Impeller] Jittery glyphs when animating text transformation flutter/flutter#149652

Closed

jonahwilliams mentioned this pull request Jul 11, 2024

Icon is misaligned when using RTL with Impeller on iOS flutter/flutter#151521

Closed

This comment was marked as off-topic.

Sign in to view

[Impeller] Use scaled font to determine bounds, match Skia position rounding behavior, add subpixel X/Y/All/None positioning. #53042

[Impeller] Use scaled font to determine bounds, match Skia position rounding behavior, add subpixel X/Y/All/None positioning. #53042

Uh oh!

Conversation

jonahwilliams commented May 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented May 25, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

flutter-dashboard bot commented May 27, 2024

Uh oh!

jonahwilliams commented May 27, 2024

Uh oh!

jonahwilliams commented May 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flutter-dashboard bot commented May 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented May 28, 2024

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

delfme commented Nov 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonahwilliams commented May 25, 2024 •

edited

Loading