[Impeller] Fix text scaling issues again, this time with perspective when a save layer is involved #43695

dnfield · 2023-07-14T18:20:21Z

Alternative to #43662

Records the basis vector of the current CTM into text contents and reuses that rather than trying to get it again at render time. That method breaks if perspective is involved in the CTM and a subpass gets translated, which can modify the scale (and rotation) of the matrix.

We're definitely not doing things quite right with perspective here, but the real fix to that is to either record the fully transformed glyph into the atlas or to use SDF/path based rendering.

Fixes flutter/flutter#130476

I still have some concerns about how EntityPass::Render is a mix of mutations and constness but we can try to address that independently.

I expect this to re-improve the regression noted in flutter/flutter#130594

This reverts commit 255ef6c.

flutter-dashboard · 2023-07-14T18:58:56Z

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 #43695 at sha c288069

impeller/entity/contents/text_contents.h

bdero · 2023-07-14T19:42:25Z

impeller/aiks/aiks_unittests.cc

@@ -2877,6 +2877,35 @@ TEST_P(AiksTest, CanCanvasDrawPicture) {
  ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
 }

+TEST_P(AiksTest, DrawPictureWithText) {


Sorta nit: The text seems to either be invisible or hard to see in the goldens with the golden service's white background. Maybe throw in black backgrounds with DrawPaint?

Actually, seems like the new goldens aren't rendering anything for some reason.

Did the gold test part have a chance to run?

The text is red, I thought that should render on either black or white background fine :\

But before this change, the new tests would've had validation errors (the atlas would not contain the scale and the test would crash)

Co-authored-by: Brandon DeRosier <x@bdero.me>

flutter-dashboard · 2023-07-14T21:10:52Z

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

Changes reported for pull request #43695 at sha 9fdf775

dnfield · 2023-07-14T21:36:01Z

I'm really unclear about why the goldens show nothing.

bdero

If they're only broken on Skia gold, maybe land now and add a bug for investigating later?

dnfield · 2023-07-14T22:36:57Z

Verified the golden harness produces an image locally, filed an issue to track for later.

…rspective when a save layer is involved (flutter/engine#43695)

…130643) flutter/engine@403866d...bf6d4bf 2023-07-14 dnfield@google.com [Impeller] Fix text scaling issues again, this time with perspective when a save layer is involved (flutter/engine#43695) 2023-07-14 flar@google.com Reland "add non-rendering operation culling to DisplayListBuilder" (#41463) (flutter/engine#43698) 2023-07-14 skia-flutter-autoroll@skia.org Roll Skia from 5f6578398870 to 271b2b6d5aaa (2 revisions) (flutter/engine#43706) 2023-07-14 jonahwilliams@google.com [Impeller] Ensure that missing color attachment 0u does not cause crash in embedder API (flutter/engine#43705) 2023-07-14 skia-flutter-autoroll@skia.org Roll Skia from 315c7f08c731 to 5f6578398870 (4 revisions) (flutter/engine#43704) 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

…rspective when a save layer is involved (flutter/engine#43695)

…sions) (#130666) Manual roll requested by zra@google.com flutter/engine@403866d...6830877 2023-07-15 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from DEENqWMCYI1SMYsYH... to rmzZN2ZAgpbjAFi5_... (flutter/engine#43722) 2023-07-15 zanderso@users.noreply.github.com Revert "Reland "add non-rendering operation culling to DisplayListBuilder" (#41463)" (flutter/engine#43721) 2023-07-15 skia-flutter-autoroll@skia.org Roll Clang from 6d667d4b261e to ebd0b8a0472b (flutter/engine#43673) 2023-07-15 skia-flutter-autoroll@skia.org Roll Dart SDK from 671bbdf6c542 to 0bd185c282d2 (1 revision) (flutter/engine#43719) 2023-07-15 26625149+0xZOne@users.noreply.github.com Optimizing performance by avoiding multiple GC operations caused by multiple surface destruction notifications (flutter/engine#43587) 2023-07-15 skia-flutter-autoroll@skia.org Roll Skia from 975eb1250431 to 6fb535aede4e (1 revision) (flutter/engine#43716) 2023-07-15 skia-flutter-autoroll@skia.org Roll Dart SDK from d1fcadf22aad to 671bbdf6c542 (2 revisions) (flutter/engine#43715) 2023-07-15 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from Z-1lzZAOYHvVrdjQ8... to oeYLDNShuD-FTgGwU... (flutter/engine#43714) 2023-07-15 skia-flutter-autoroll@skia.org Roll Skia from 271b2b6d5aaa to 975eb1250431 (2 revisions) (flutter/engine#43712) 2023-07-15 goderbauer@google.com Move ViewConfiguration ownership to FlutterView (flutter/engine#43701) 2023-07-14 yjbanov@google.com [web] always add secondary role managers (flutter/engine#43663) 2023-07-14 dnfield@google.com [Impeller] Fix text scaling issues again, this time with perspective when a save layer is involved (flutter/engine#43695) 2023-07-14 flar@google.com Reland "add non-rendering operation culling to DisplayListBuilder" (#41463) (flutter/engine#43698) 2023-07-14 skia-flutter-autoroll@skia.org Roll Skia from 5f6578398870 to 271b2b6d5aaa (2 revisions) (flutter/engine#43706) 2023-07-14 jonahwilliams@google.com [Impeller] Ensure that missing color attachment 0u does not cause crash in embedder API (flutter/engine#43705) 2023-07-14 skia-flutter-autoroll@skia.org Roll Skia from 315c7f08c731 to 5f6578398870 (4 revisions) (flutter/engine#43704) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from DEENqWMCYI1S to rmzZN2ZAgpbj fuchsia/sdk/core/mac-amd64 from Z-1lzZAOYHvV to oeYLDNShuD-F 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

…when a save layer is involved (flutter#43695) Alternative to flutter#43662 Records the basis vector of the current CTM into text contents and reuses that rather than trying to get it again at render time. That method breaks if perspective is involved in the CTM and a subpass gets translated, which can modify the scale (and rotation) of the matrix. We're definitely not doing things quite right with perspective here, but the real fix to that is to either record the fully transformed glyph into the atlas or to use SDF/path based rendering. Fixes flutter/flutter#130476 I still have some concerns about how `EntityPass::Render` is a mix of mutations and constness but we can try to address that independently. I expect this to re-improve the regression noted in flutter/flutter#130594

…lutter#130643) flutter/engine@403866d...bf6d4bf 2023-07-14 dnfield@google.com [Impeller] Fix text scaling issues again, this time with perspective when a save layer is involved (flutter/engine#43695) 2023-07-14 flar@google.com Reland "add non-rendering operation culling to DisplayListBuilder" (flutter#41463) (flutter/engine#43698) 2023-07-14 skia-flutter-autoroll@skia.org Roll Skia from 5f6578398870 to 271b2b6d5aaa (2 revisions) (flutter/engine#43706) 2023-07-14 jonahwilliams@google.com [Impeller] Ensure that missing color attachment 0u does not cause crash in embedder API (flutter/engine#43705) 2023-07-14 skia-flutter-autoroll@skia.org Roll Skia from 315c7f08c731 to 5f6578398870 (4 revisions) (flutter/engine#43704) 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

…sions) (flutter#130666) Manual roll requested by zra@google.com flutter/engine@403866d...6830877 2023-07-15 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from DEENqWMCYI1SMYsYH... to rmzZN2ZAgpbjAFi5_... (flutter/engine#43722) 2023-07-15 zanderso@users.noreply.github.com Revert "Reland "add non-rendering operation culling to DisplayListBuilder" (flutter#41463)" (flutter/engine#43721) 2023-07-15 skia-flutter-autoroll@skia.org Roll Clang from 6d667d4b261e to ebd0b8a0472b (flutter/engine#43673) 2023-07-15 skia-flutter-autoroll@skia.org Roll Dart SDK from 671bbdf6c542 to 0bd185c282d2 (1 revision) (flutter/engine#43719) 2023-07-15 26625149+0xZOne@users.noreply.github.com Optimizing performance by avoiding multiple GC operations caused by multiple surface destruction notifications (flutter/engine#43587) 2023-07-15 skia-flutter-autoroll@skia.org Roll Skia from 975eb1250431 to 6fb535aede4e (1 revision) (flutter/engine#43716) 2023-07-15 skia-flutter-autoroll@skia.org Roll Dart SDK from d1fcadf22aad to 671bbdf6c542 (2 revisions) (flutter/engine#43715) 2023-07-15 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from Z-1lzZAOYHvVrdjQ8... to oeYLDNShuD-FTgGwU... (flutter/engine#43714) 2023-07-15 skia-flutter-autoroll@skia.org Roll Skia from 271b2b6d5aaa to 975eb1250431 (2 revisions) (flutter/engine#43712) 2023-07-15 goderbauer@google.com Move ViewConfiguration ownership to FlutterView (flutter/engine#43701) 2023-07-14 yjbanov@google.com [web] always add secondary role managers (flutter/engine#43663) 2023-07-14 dnfield@google.com [Impeller] Fix text scaling issues again, this time with perspective when a save layer is involved (flutter/engine#43695) 2023-07-14 flar@google.com Reland "add non-rendering operation culling to DisplayListBuilder" (flutter#41463) (flutter/engine#43698) 2023-07-14 skia-flutter-autoroll@skia.org Roll Skia from 5f6578398870 to 271b2b6d5aaa (2 revisions) (flutter/engine#43706) 2023-07-14 jonahwilliams@google.com [Impeller] Ensure that missing color attachment 0u does not cause crash in embedder API (flutter/engine#43705) 2023-07-14 skia-flutter-autoroll@skia.org Roll Skia from 315c7f08c731 to 5f6578398870 (4 revisions) (flutter/engine#43704) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from DEENqWMCYI1S to rmzZN2ZAgpbj fuchsia/sdk/core/mac-amd64 from Z-1lzZAOYHvV to oeYLDNShuD-F 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

dnfield added 2 commits July 14, 2023 11:03

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

8e6f1d0

This reverts commit 255ef6c.

Record the CTM scale independent of how EntityPass::Render may change it

c288069

dnfield requested review from bdero and jonahwilliams July 14, 2023 18:20

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

dnfield mentioned this pull request Jul 14, 2023

[Impeller] Fixes text scaling issues again, this time with perspective #43662

Closed

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

chinmaygarde assigned dnfield Jul 14, 2023

bdero reviewed Jul 14, 2023

View reviewed changes

Update impeller/entity/contents/text_contents.h

9fdf775

Co-authored-by: Brandon DeRosier <x@bdero.me>

bdero approved these changes Jul 14, 2023

View reviewed changes

dnfield mentioned this pull request Jul 14, 2023

[Impeller] Goldens not rendering on gold but render locally. flutter/flutter#130633

Closed

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

auto-submit bot merged commit bf6d4bf into flutter:main Jul 14, 2023
28 checks passed

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

Roll Flutter Engine from 403866d16137 to bf6d4bfe27cc (5 revisions) flutter/flutter#130643

Merged

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

bf6d4bfe2 [Impeller] Fix text scaling issues again, this time with pe…

7f37f96

…rspective when a save layer is involved (flutter/engine#43695)

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

Manual roll Flutter Engine from 403866d16137 to 683087731feb (16 revisions) flutter/flutter#130666

Merged

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

bf6d4bfe2 [Impeller] Fix text scaling issues again, this time with pe…

edd1703

…rspective when a save layer is involved (flutter/engine#43695)

dnfield deleted the fix_text2 branch July 17, 2023 16:37

bdero mentioned this pull request Jul 17, 2023

Optional STB based Glyph Atlas Support bdero/impeller-cmake#12

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Fix text scaling issues again, this time with perspective when a save layer is involved #43695

[Impeller] Fix text scaling issues again, this time with perspective when a save layer is involved #43695

dnfield commented Jul 14, 2023

flutter-dashboard bot commented Jul 14, 2023

bdero Jul 14, 2023

bdero Jul 14, 2023

dnfield Jul 14, 2023

dnfield Jul 14, 2023

flutter-dashboard bot commented Jul 14, 2023

dnfield commented Jul 14, 2023

bdero left a comment

dnfield commented Jul 14, 2023

[Impeller] Fix text scaling issues again, this time with perspective when a save layer is involved #43695

[Impeller] Fix text scaling issues again, this time with perspective when a save layer is involved #43695

Conversation

dnfield commented Jul 14, 2023

flutter-dashboard bot commented Jul 14, 2023

bdero Jul 14, 2023

Choose a reason for hiding this comment

bdero Jul 14, 2023

Choose a reason for hiding this comment

dnfield Jul 14, 2023

Choose a reason for hiding this comment

dnfield Jul 14, 2023

Choose a reason for hiding this comment

flutter-dashboard bot commented Jul 14, 2023

dnfield commented Jul 14, 2023

bdero left a comment

Choose a reason for hiding this comment

dnfield commented Jul 14, 2023