[Impeller] adjust coverage origin when rounding out SaveLayer bounds. #161838

jonahwilliams · 2025-01-17T23:05:11Z

When we round out the texture size for a saveLayer, also round out the coverage so that the drawing origin is correct. Otherwise we may actually cut off the top left edge of rendering.

… adjust_bounds

flutter-dashboard · 2025-01-22T19:10:30Z

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.

For more guidance, visit Writing a golden file test for package:flutter.

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

Changes reported for pull request #161838 at sha 3428685

jonahwilliams · 2025-01-22T20:02:45Z

Leaving up the goldens for comparison. If you look at the before/after you can see the top left edge appearing where it previously was cut off.

flutter-dashboard · 2025-01-22T21:50:27Z

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

For more guidance, visit Writing a golden file test for package:flutter.

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

Changes reported for pull request #161838 at sha 53fd56f

gaaclarke · 2025-01-22T23:08:46Z

Leaving up the goldens for comparison. If you look at the before/after you can see the top left edge appearing where it previously was cut off.

I looked through the goldens and didn't notice anything problematic. The space between the rects in material.raw_chip.disabled.delete_button looks slightly better in the before, but looking at other tests that seems to be a fluke and not directly related to this change, as far as I can tell.

gaaclarke

Test and code looks good to me. I just have one question about why it is happening in one branch of the code.

engine/src/flutter/impeller/display_list/canvas.cc

gaaclarke · 2025-01-22T23:12:47Z

engine/src/flutter/impeller/display_list/canvas.cc

@@ -1156,7 +1163,8 @@ void Canvas::SaveLayer(const Paint& paint,
                                             subpass_size,              //
                                             Color::BlackTransparent()  //
                                             )));
-  save_layer_state_.push_back(SaveLayerState{paint_copy, subpass_coverage});
+  save_layer_state_.push_back(SaveLayerState{
+      paint_copy, subpass_coverage.Shift(-coverage_origin_adjustment)});


no action required: I double checked this is right:

top_left = 1.9 adjustment = 1.9 - 1.0 = 0.9 shift(-0.9)

gaaclarke · 2025-01-22T23:15:09Z

engine/src/flutter/impeller/display_list/canvas.cc

@@ -1017,12 +1017,19 @@ void Canvas::SaveLayer(const Paint& paint,
  // sampling mode.
  ISize subpass_size;
  bool did_round_out = false;
+  Point coverage_origin_adjustment = Point{0, 0};
  if (paint.image_filter) {
    subpass_size = ISize(subpass_coverage.GetSize());


How come this isn't an issue?

I don't fully understand how the coverage computation for image filters are working today. most are pretty destructuve to the exact pixels (and that no one has reported an issue). Given that, I think we should leave well enough alone for now.

Co-authored-by: gaaclarke <30870216+gaaclarke@users.noreply.github.com>

gaaclarke

lgtm

… bounds. (flutter/flutter#161838)

Manual roll requested by tarrinneal@google.com flutter/flutter@b2f515f...c1561a4 2025-01-23 matanlurey@users.noreply.github.com Add `windows_host_engine_test` to presubmit, remove lint exceptions. (flutter/flutter#162032) 2025-01-23 matanlurey@users.noreply.github.com Shift `mac_clang_tidy` to a Linux orchestrator. (flutter/flutter#162042) 2025-01-23 jonahwilliams@google.com [Impeller] check both linear sampling props for AHBs. (flutter/flutter#162043) 2025-01-23 jason-simmons@users.noreply.github.com [Impeller] Implement inherited opacity for ColorFilterContents (flutter/flutter#161834) 2025-01-23 robert.ancell@canonical.com Keyboard tidy ups (flutter/flutter#162054) 2025-01-23 koji.wakamiya@gmail.com fix: Call codec.dispose in tests of `engine/src/flutter` (flutter/flutter#161115) 2025-01-23 koji.wakamiya@gmail.com fix: Call codec.dispose in `flutter/test` (flutter/flutter#161127) 2025-01-23 koji.wakamiya@gmail.com fix: Call codec.dispose in `flutter_test` (flutter/flutter#161131) 2025-01-23 koji.wakamiya@gmail.com fix: Call codec.dispose in `dev/` (flutter/flutter#161112) 2025-01-23 robert.ancell@canonical.com Replace hacky code creating fake devices (flutter/flutter#162056) 2025-01-23 dacoharkes@google.com [native assets] Roll dependencies (flutter/flutter#162068) 2025-01-23 dacoharkes@google.com [native assets] Roll dependencies (flutter/flutter#162017) 2025-01-23 bruno.leroux@gmail.com Fix DropdownButtonFormField clips text when large text scale is used (flutter/flutter#159975) 2025-01-23 matanlurey@users.noreply.github.com Add a better error message when `flutter drive --target` is used incorrectly. (flutter/flutter#162023) 2025-01-23 jason-simmons@users.noreply.github.com Revert "Move the analyzer_benchmark to Mac arm64 devicelab bots" (flutter/flutter#161822) 2025-01-23 jonahwilliams@google.com [Impeller] adjust coverage origin when rounding out SaveLayer bounds. (flutter/flutter#161838) 2025-01-23 34871572+gmackall@users.noreply.github.com Add a README with instructions for editing and running tests for the FGP (flutter/flutter#161830) 2025-01-23 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Migrate unit tests off of Skia geometry classes (#161855)" (flutter/flutter#162046) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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

…flutter#161838) Fixes flutter#161374 When we round out the texture size for a saveLayer, also round out the coverage so that the drawing origin is correct. Otherwise we may actually cut off the top left edge of rendering. --------- Co-authored-by: gaaclarke <30870216+gaaclarke@users.noreply.github.com>

…nds (#161838) (#162189) Fixes #161374 When we round out the texture size for a saveLayer, also round out the coverage so that the drawing origin is correct. Otherwise we may actually cut off the top left edge of rendering.

… bounds. (flutter/flutter#161838)

) Manual roll requested by tarrinneal@google.com flutter/flutter@b2f515f...c1561a4 2025-01-23 matanlurey@users.noreply.github.com Add `windows_host_engine_test` to presubmit, remove lint exceptions. (flutter/flutter#162032) 2025-01-23 matanlurey@users.noreply.github.com Shift `mac_clang_tidy` to a Linux orchestrator. (flutter/flutter#162042) 2025-01-23 jonahwilliams@google.com [Impeller] check both linear sampling props for AHBs. (flutter/flutter#162043) 2025-01-23 jason-simmons@users.noreply.github.com [Impeller] Implement inherited opacity for ColorFilterContents (flutter/flutter#161834) 2025-01-23 robert.ancell@canonical.com Keyboard tidy ups (flutter/flutter#162054) 2025-01-23 koji.wakamiya@gmail.com fix: Call codec.dispose in tests of `engine/src/flutter` (flutter/flutter#161115) 2025-01-23 koji.wakamiya@gmail.com fix: Call codec.dispose in `flutter/test` (flutter/flutter#161127) 2025-01-23 koji.wakamiya@gmail.com fix: Call codec.dispose in `flutter_test` (flutter/flutter#161131) 2025-01-23 koji.wakamiya@gmail.com fix: Call codec.dispose in `dev/` (flutter/flutter#161112) 2025-01-23 robert.ancell@canonical.com Replace hacky code creating fake devices (flutter/flutter#162056) 2025-01-23 dacoharkes@google.com [native assets] Roll dependencies (flutter/flutter#162068) 2025-01-23 dacoharkes@google.com [native assets] Roll dependencies (flutter/flutter#162017) 2025-01-23 bruno.leroux@gmail.com Fix DropdownButtonFormField clips text when large text scale is used (flutter/flutter#159975) 2025-01-23 matanlurey@users.noreply.github.com Add a better error message when `flutter drive --target` is used incorrectly. (flutter/flutter#162023) 2025-01-23 jason-simmons@users.noreply.github.com Revert "Move the analyzer_benchmark to Mac arm64 devicelab bots" (flutter/flutter#161822) 2025-01-23 jonahwilliams@google.com [Impeller] adjust coverage origin when rounding out SaveLayer bounds. (flutter/flutter#161838) 2025-01-23 34871572+gmackall@users.noreply.github.com Add a README with instructions for editing and running tests for the FGP (flutter/flutter#161830) 2025-01-23 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] Migrate unit tests off of Skia geometry classes (#161855)" (flutter/flutter#162046) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC stuartmorgan@google.com,tarrinneal@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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

[Impeller] adjust coverage origin when rounding out SaveLayer bounds.

73ffc69

github-actions bot added engine flutter/engine repository. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Jan 17, 2025

jonahwilliams added 3 commits January 17, 2025 16:23

Merge branch 'master' into adjust_bounds

225a2bb

Add test case.

5532efe

Merge branch 'adjust_bounds' of github.com:jonahwilliams/flutter into…

3428685

… adjust_bounds

jonahwilliams marked this pull request as ready for review January 22, 2025 17:59

flutter-dashboard bot added the will affect goldens Changes to golden files label Jan 22, 2025

jonahwilliams added 2 commits January 22, 2025 11:53

update goldens file

b109a87

Merge branch 'master' into adjust_bounds

53fd56f

jonahwilliams requested a review from gaaclarke January 22, 2025 20:00

gaaclarke reviewed Jan 22, 2025

View reviewed changes

Update engine/src/flutter/impeller/display_list/canvas.cc

37db513

Co-authored-by: gaaclarke <30870216+gaaclarke@users.noreply.github.com>

gaaclarke approved these changes Jan 22, 2025

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 23, 2025

auto-submit bot added this pull request to the merge queue Jan 23, 2025

Merged via the queue into flutter:master with commit 948e31d Jan 23, 2025
173 of 174 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 23, 2025

engine-flutter-autoroll mentioned this pull request Jan 23, 2025

Roll Flutter from b2f515f45ef5 to 663d02078813 (8 revisions) flutter/packages#8484

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 23, 2025

948e31d [Impeller] adjust coverage origin when rounding out SaveLayer…

9133b86

… bounds. (flutter/flutter#161838)

engine-flutter-autoroll mentioned this pull request Jan 23, 2025

Roll Flutter from b2f515f45ef5 to 6e9b863e6d29 (9 revisions) flutter/packages#8486

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 23, 2025

948e31d [Impeller] adjust coverage origin when rounding out SaveLayer…

0017106

… bounds. (flutter/flutter#161838)

engine-flutter-autoroll mentioned this pull request Jan 23, 2025

Roll Flutter from b2f515f45ef5 to d59995bac316 (12 revisions) flutter/packages#8488

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 23, 2025

948e31d [Impeller] adjust coverage origin when rounding out SaveLayer…

f72fce5

… bounds. (flutter/flutter#161838)

engine-flutter-autoroll mentioned this pull request Jan 23, 2025

Roll Flutter from b2f515f45ef5 to b46479c6a00d (16 revisions) flutter/packages#8489

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 23, 2025

948e31d [Impeller] adjust coverage origin when rounding out SaveLayer…

0edd3f1

… bounds. (flutter/flutter#161838)

jonahwilliams mentioned this pull request Jan 23, 2025

[Impeller] Opacity rearranges text. #161202

Closed

engine-flutter-autoroll mentioned this pull request Jan 23, 2025

Roll Flutter from b2f515f45ef5 to 211b46fcb102 (17 revisions) flutter/packages#8490

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 23, 2025

948e31d [Impeller] adjust coverage origin when rounding out SaveLayer…

d3cb92a

… bounds. (flutter/flutter#161838)

engine-flutter-autoroll mentioned this pull request Jan 23, 2025

Manual roll Flutter from b2f515f45ef5 to c1561a41c530 (18 revisions) flutter/packages#8491

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 23, 2025

948e31d [Impeller] adjust coverage origin when rounding out SaveLayer…

44f65b1

… bounds. (flutter/flutter#161838)

engine-flutter-autoroll mentioned this pull request Jan 23, 2025

Roll Flutter from b2f515f45ef5 to c1561a41c530 (18 revisions) flutter/packages#8492

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 23, 2025

948e31d [Impeller] adjust coverage origin when rounding out SaveLayer…

b6b7200

… bounds. (flutter/flutter#161838)

reidbaker mentioned this pull request Jan 30, 2025

3.29 engine bump and dart rev #162471

Merged

9 tasks

WyvernPirate mentioned this pull request Apr 3, 2025

Updates Flutter to 3.29.2 stable release WyvernPirate/flutter#1

Open

reidbaker mentioned this pull request Apr 4, 2025

Unbreak presubmit for stable #166594

Merged

9 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

948e31d [Impeller] adjust coverage origin when rounding out SaveLayer…

c2af097

… bounds. (flutter/flutter#161838)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025

948e31d [Impeller] adjust coverage origin when rounding out SaveLayer…

de2da7b

… bounds. (flutter/flutter#161838)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2025

948e31d [Impeller] adjust coverage origin when rounding out SaveLayer…

bdbdcde

… bounds. (flutter/flutter#161838)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] adjust coverage origin when rounding out SaveLayer bounds. #161838

[Impeller] adjust coverage origin when rounding out SaveLayer bounds. #161838

Uh oh!

jonahwilliams commented Jan 17, 2025 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Jan 22, 2025

Uh oh!

jonahwilliams commented Jan 22, 2025

Uh oh!

flutter-dashboard bot commented Jan 22, 2025

Uh oh!

gaaclarke commented Jan 22, 2025

Uh oh!

gaaclarke left a comment

Uh oh!

Uh oh!

gaaclarke Jan 22, 2025

Uh oh!

gaaclarke Jan 22, 2025

Uh oh!

jonahwilliams Jan 22, 2025

Uh oh!

gaaclarke left a comment

Uh oh!

Uh oh!

Uh oh!

[Impeller] adjust coverage origin when rounding out SaveLayer bounds. #161838

[Impeller] adjust coverage origin when rounding out SaveLayer bounds. #161838

Uh oh!

Conversation

jonahwilliams commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Jan 22, 2025

Uh oh!

jonahwilliams commented Jan 22, 2025

Uh oh!

flutter-dashboard bot commented Jan 22, 2025

Uh oh!

gaaclarke commented Jan 22, 2025

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gaaclarke Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

gaaclarke Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonahwilliams commented Jan 17, 2025 •

edited

Loading