[Impeller] Improve resolution of text scaling. #43533

dnfield · 2023-07-10T22:18:16Z

This patch does a few things:

Changes ownership of the LazyGlyphAtlas to ContentContext from Canvas. This means that drawings with multiple canvases (e.g. calls to DrawPicture) share the same lazy atlas.
Moves the scale property from Font::Metrics to FontGlyphPair. Font::Metrics contains properties related to the font, whereas scale has to do with the properties of the drawing at render time and is independent of the font.
Makes the lazy atlas manage FontGlyphPair::Set rather than vectors of TextFrames.
Makes the determination of font scaling at EntityPass::Render rather than Canvas::DrawTextFrame. The scaling may be altered by calls to Canvas::DrawPicture by render time and otherwise get missed. This is done via a new method: Contents::PopulateGlyphAtlas, which is only implemented by TextContents in this patch.
Fixes up some miscelleaneous bugs in Font::Metrics hashing and unused/dead code.

This improves over prior attempts: LazyGlyphAtlas ownership is now unambiguous, and glyph scaling happens correctly for all rendered glyphs regardless of the order of canvas operations in Aiks.

Fixes flutter/flutter#130142

This patch does a few things: - Changes ownership of the LazyGlyphAtlas to ContentContext from Canvas. This means that drawings with multiple canvases (e.g. calls to DrawPicture) share the same lazy atlas. - Moves the scale property from Font::Metrics to FontGlyphPair. Font::Metrics contains properties related to the font, whereas scale has to do with the properties of the drawing at render time and is independent of the font. - Makes the lazy atlas manage FontGlyphPair::Set rather than vectors of TextFrames. - Makes the determination of font scaling at EntityPass::Render rather than Canvas::DrawTextFrame. The scaling may be altered by calls to Canvas::DrawPicture by render time and otherwise get missed. This is done via a new method: Contents::PopulateGlyphAtlas, which is only implemented by TextContents in this patch. This improves over prior attempts: LazyGlyphAtlas ownership is now unambiguous, and glyph scaling happens correctly for all rendered glyphs regardless of the order of canvas operations in Aiks. Fixes flutter/flutter#130142

jonahwilliams

This approach LGTM, assuming there are no serious performance changes on apps with large numbers of entities.

auto-submit · 2023-07-10T22:59:05Z

auto label is removed for flutter/engine, pr: 43533, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

bdero

LGTM

bdero · 2023-07-10T22:58:01Z

impeller/entity/contents/text_contents.cc

+  }
+
+  auto type = frame_.GetAtlasType();
+  auto scale = entity.GetTransformation().GetMaxBasisLengthXY();


Nit: Might make sense to have a static Scalar TextContents::DeriveTextScaleFromEntity(const Entity& entity) const or maybe even Scalar Entity::DeriveTextScale() const and use that in both spots. Our way of computing the scale has changed a few times.

bdero · 2023-07-10T22:59:46Z

impeller/typographer/backends/skia/text_render_context_skia.cc

@@ -376,23 +351,23 @@ std::shared_ptr<GlyphAtlas> TextRenderContextSkia::CreateGlyphAtlas(
    // added.

    // ---------------------------------------------------------------------------
-    // Step 4: Record the positions in the glyph atlas of the newly added
+    // Step 3a: Record the positions in the glyph atlas of the newly added


Nit: Why the as and bs?

Because there are two branches that go down here that both were reusing the same step numbers.

Ah yeah, makes sense.

auto-submit · 2023-07-10T23:16:05Z

auto label is removed for flutter/engine, pr: 43533, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

The status or check suite Mac mac_clang_tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.
The status or check suite Linux linux_clang_tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2023-07-10T23:21:10Z

auto label is removed for flutter/engine, pr: 43533, due to - The status or check suite Linux linux_arm_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2023-07-11T02:32:18Z

auto label is removed for flutter/engine, pr: 43533, due to - The status or check suite Mac mac_host_engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

flutter-dashboard · 2023-07-11T04:06:44Z

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 #43533 at sha 1dc36de

…ine#43533)

…130313) flutter/engine@153d9e1...767f2fb 2023-07-11 dnfield@google.com [Impeller] Improve resolution of text scaling. (flutter/engine#43533) 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 aaclarke@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Reverts #43533 Cause of flutter/flutter#130476 I was trying to add a test for this but it's taking me a while to get a working test so here's the revert.

…130538) flutter/engine@0f0436b...8a42bfc 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 52613fcc0780 to 9e4f5cc3aeb4 (1 revision) (flutter/engine#43659) 2023-07-13 jonahwilliams@google.com [Impeller] no-op fragment program on Android until it works. (flutter/engine#43657) 2023-07-13 31859944+LongCatIsLooong@users.noreply.github.com Reland #43118 "Add a flag to ParagraphBuilder for rounding hack migration" (flutter/engine#43647) 2023-07-13 ychris@google.com Unmerge threads if the current merger is the only one that's merged. (flutter/engine#43652) 2023-07-13 dnfield@google.com Revert flutter/engine#43533 (flutter/engine#43654) 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 743ad92f5de2 to 52613fcc0780 (9 revisions) (flutter/engine#43655) 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 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

This reverts commit 255ef6c.

This patch does a few things: - Changes ownership of the LazyGlyphAtlas to ContentContext from Canvas. This means that drawings with multiple canvases (e.g. calls to DrawPicture) share the same lazy atlas. - Moves the scale property from Font::Metrics to FontGlyphPair. Font::Metrics contains properties related to the font, whereas scale has to do with the properties of the drawing at render time and is independent of the font. - Makes the lazy atlas manage FontGlyphPair::Set rather than vectors of TextFrames. - Makes the determination of font scaling at EntityPass::Render rather than Canvas::DrawTextFrame. The scaling may be altered by calls to Canvas::DrawPicture by render time and otherwise get missed. This is done via a new method: Contents::PopulateGlyphAtlas, which is only implemented by TextContents in this patch. - Fixes up some miscelleaneous bugs in Font::Metrics hashing and unused/dead code. This improves over prior attempts: LazyGlyphAtlas ownership is now unambiguous, and glyph scaling happens correctly for all rendered glyphs regardless of the order of canvas operations in Aiks. Fixes flutter/flutter#130142

Reverts flutter#43533 Cause of flutter/flutter#130476 I was trying to add a test for this but it's taking me a while to get a working test so here's the revert.

This reverts commit 255ef6c.

Reverts flutter#43533 Cause of flutter/flutter#130476 I was trying to add a test for this but it's taking me a while to get a working test so here's the revert.

…lutter#130538) flutter/engine@0f0436b...8a42bfc 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 52613fcc0780 to 9e4f5cc3aeb4 (1 revision) (flutter/engine#43659) 2023-07-13 jonahwilliams@google.com [Impeller] no-op fragment program on Android until it works. (flutter/engine#43657) 2023-07-13 31859944+LongCatIsLooong@users.noreply.github.com Reland flutter#43118 "Add a flag to ParagraphBuilder for rounding hack migration" (flutter/engine#43647) 2023-07-13 ychris@google.com Unmerge threads if the current merger is the only one that's merged. (flutter/engine#43652) 2023-07-13 dnfield@google.com Revert flutter/engine#43533 (flutter/engine#43654) 2023-07-13 skia-flutter-autoroll@skia.org Roll Skia from 743ad92f5de2 to 52613fcc0780 (9 revisions) (flutter/engine#43655) 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 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#130313) flutter/engine@153d9e1...767f2fb 2023-07-11 dnfield@google.com [Impeller] Improve resolution of text scaling. (flutter/engine#43533) 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 aaclarke@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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

dnfield added 2 commits July 10, 2023 15:08

merge

aa2922a

dnfield requested review from bdero and jonahwilliams July 10, 2023 22:18

github-actions bot added the e: impeller label Jul 10, 2023

jonahwilliams approved these changes Jul 10, 2023

View reviewed changes

Update aiks_unittests.cc

cef2a39

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 10, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 10, 2023

bdero approved these changes Jul 10, 2023

View reviewed changes

review and tidy

4692ab7

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 10, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 10, 2023

Merge branch 'tc_scale_3' of github.com:dnfield/engine into tc_scale_3

5c92998

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 10, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 10, 2023

chinmaygarde assigned dnfield Jul 11, 2023

Update typographer_unittests.cc

6f6f252

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 11, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jul 11, 2023

dnfield added 2 commits July 10, 2023 20:05

Merge branch 'main' into tc_scale_3

cb392b8

ColorSourceTextContents too

1dc36de

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 11, 2023

flutter-dashboard bot added the will affect goldens label Jul 11, 2023

auto-submit bot merged commit 767f2fb into flutter:main Jul 11, 2023
29 checks passed

engine-flutter-autoroll mentioned this pull request Jul 11, 2023

Roll Flutter Engine from 153d9e1d598a to 767f2fb8ab03 (1 revision) flutter/flutter#130313

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 11, 2023

767f2fb8a [Impeller] Improve resolution of text scaling. (flutter/eng…

f8d0197

…ine#43533)

dnfield deleted the tc_scale_3 branch July 13, 2023 16:13

dnfield mentioned this pull request Jul 13, 2023

Revert https://github.com/flutter/engine/pull/43533 #43654

Merged

auto-submit bot pushed a commit that referenced this pull request Jul 13, 2023

Revert #43533 (#43654)

255ef6c

Reverts #43533 Cause of flutter/flutter#130476 I was trying to add a test for this but it's taking me a while to get a working test so here's the revert.

engine-flutter-autoroll mentioned this pull request Jul 13, 2023

Roll Flutter Engine from 0f0436b28430 to 255ef6cb38d1 (2 revisions) flutter/flutter#130531

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2023

255ef6cb3 Revert flutter/engine#43533 (flutter/engine#43654)

fb8f457

engine-flutter-autoroll mentioned this pull request Jul 13, 2023

Roll Flutter Engine from 0f0436b28430 to 8a42bfc80e3a (6 revisions) flutter/flutter#130538

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 13, 2023

255ef6cb3 Revert flutter/engine#43533 (flutter/engine#43654)

ea58955

dnfield added a commit to dnfield/engine that referenced this pull request Jul 14, 2023

Reland flutter#43533

8318bbc

This reverts commit 255ef6c.

dnfield added a commit to dnfield/engine that referenced this pull request Jul 14, 2023

Revert "Revert flutter#43533 (flutter#43654)"

8e6f1d0

This reverts commit 255ef6c.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Improve resolution of text scaling. #43533

[Impeller] Improve resolution of text scaling. #43533

dnfield commented Jul 10, 2023

jonahwilliams left a comment

auto-submit bot commented Jul 10, 2023

bdero left a comment

bdero Jul 10, 2023

dnfield Jul 10, 2023

bdero Jul 10, 2023

dnfield Jul 10, 2023

bdero Jul 10, 2023

auto-submit bot commented Jul 10, 2023

auto-submit bot commented Jul 10, 2023

auto-submit bot commented Jul 11, 2023

flutter-dashboard bot commented Jul 11, 2023

[Impeller] Improve resolution of text scaling. #43533

[Impeller] Improve resolution of text scaling. #43533

Conversation

dnfield commented Jul 10, 2023

jonahwilliams left a comment

Choose a reason for hiding this comment

auto-submit bot commented Jul 10, 2023

bdero left a comment

Choose a reason for hiding this comment

bdero Jul 10, 2023

Choose a reason for hiding this comment

dnfield Jul 10, 2023

Choose a reason for hiding this comment

bdero Jul 10, 2023

Choose a reason for hiding this comment

dnfield Jul 10, 2023

Choose a reason for hiding this comment

bdero Jul 10, 2023

Choose a reason for hiding this comment

auto-submit bot commented Jul 10, 2023

auto-submit bot commented Jul 10, 2023

auto-submit bot commented Jul 11, 2023

flutter-dashboard bot commented Jul 11, 2023