[Impeller] migrate blur tests to display list. #54519

jonahwilliams · 2024-08-12T23:14:18Z

Part of DL interop. Currently experiencing some ... issues with blur radius/sigma.
EDIT: was rrect_blur vs gaussian: fixed now.

Part of flutter/flutter#142054

display_list/dl_color.h

…o more_more_tests

flutter-dashboard · 2024-08-13T15:43:22Z

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 #54519 at sha 85e42bc

flutter-dashboard · 2024-08-13T18:36:08Z

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

Changes reported for pull request #54519 at sha ab97fbe

chinmaygarde

Didn't pay super close attention assuming it was a mechanical move. Happy to look at something specific if needed.

jonahwilliams · 2024-08-14T00:45:14Z

The sigma's seem off somewhat, I'm not sure why

bdero · 2024-08-14T20:38:02Z

impeller/display_list/aiks_dl_blur_unittests.cc

+  paint.setColor(
+      DlColor::RGBA(128.0f / 255.0f, 128.0f / 255.0f, 128.0f / 255.0f, 1.0f));
+
+  Scalar sigma = 1 + 20 / 2;


Originally this test was converting Radius(20) to Sigma via Impeller's conversion utility. The conversion utility does radius > 0 ? radius / kKernelRadiusPerSigma + 0.5f : 0.0f, which doesn't match this conversion. I'd continue to use Impeller's Radius/Sigma tool when porting over all these tests to make sure they're behaving the same. You can just pull the Scalar out of them after converting.

(note that kKernelRadiusPerSigma is an irrational number)

I actually used that originally and it was still wrong, so this was a shot in the dark change.

or, I was trying to use the coversions:

Sigma sigma = Radius(..); blur(sigma.sigma)l

Which hopefully does the same thing?

Or I guess replace all these conversions with radius / kKernelRadiusPerSigma + 0.5f inline... both are kind of evil but I guess this would be further away from Impeller's internals.

I think I started the trend of authoring these tests in terms kernel radii, which was an oversight on my part. The radius->sigma conversion is a pure implementation detail subject to change.

or, I was trying to use the coversions:

Sigma sigma = Radius(..); blur(sigma.sigma)l

Which hopefully does the same thing?

Yes, I didn't see this message. Recommended reading for GitHub:

I would expect this to be correct.

Sigma sigma = Radius(20); blur(sigma.sigma);

flutter-dashboard · 2024-08-15T02:13:28Z

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

Changes reported for pull request #54519 at sha d452702

flutter-dashboard · 2024-08-15T03:27:28Z

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

Changes reported for pull request #54519 at sha f600638

jonahwilliams · 2024-08-15T04:18:38Z

reason for revert: post submit golden issues.

This reverts commit d143913.

…4564) Reverts: #54519 Initiated by: jonahwilliams Reason for reverting: post submit golden issues. Original PR Author: jonahwilliams Reviewed By: {chinmaygarde} This change reverts the following previous change: Part of DL interop. Currently experiencing some ... issues with blur radius/sigma. EDIT: was rrect_blur vs gaussian: fixed now. Part of flutter/flutter#142054

…ine#54519)

…153487) flutter/engine@76a1c64...971ddd9 2024-08-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "macOS: Refactor create_macos_framework.py (#54557)" (flutter/engine#54566) 2024-08-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] migrate blur tests to display list. (#54519)" (flutter/engine#54564) 2024-08-15 jonahwilliams@google.com [Impeller] migrate blur tests to display list. (flutter/engine#54519) 2024-08-14 skia-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from J1EXQ2ozMIQEa6F7H... to -XDS8eBA0Le-zCEfW... (flutter/engine#54561) 2024-08-14 chris@bracken.jp macOS: Refactor create_macos_framework.py (flutter/engine#54557) 2024-08-14 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from MeV5i7xXXFPHF5sBK... to qpfC-GAVZvOwAQU5f... (flutter/engine#54559) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from MeV5i7xXXFPH to qpfC-GAVZvOw 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://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#153487) flutter/engine@76a1c64...971ddd9 2024-08-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "macOS: Refactor create_macos_framework.py (flutter#54557)" (flutter/engine#54566) 2024-08-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] migrate blur tests to display list. (flutter#54519)" (flutter/engine#54564) 2024-08-15 jonahwilliams@google.com [Impeller] migrate blur tests to display list. (flutter/engine#54519) 2024-08-14 skia-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from J1EXQ2ozMIQEa6F7H... to -XDS8eBA0Le-zCEfW... (flutter/engine#54561) 2024-08-14 chris@bracken.jp macOS: Refactor create_macos_framework.py (flutter/engine#54557) 2024-08-14 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from MeV5i7xXXFPHF5sBK... to qpfC-GAVZvOwAQU5f... (flutter/engine#54559) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from MeV5i7xXXFPH to qpfC-GAVZvOw 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://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] migrate blur tests to display list.

d901668

github-actions bot added the e: impeller label Aug 12, 2024

++

beb1c4a

jonahwilliams commented Aug 13, 2024

View reviewed changes

display_list/dl_color.h Outdated Show resolved Hide resolved

jonahwilliams added 7 commits August 12, 2024 18:23

more changes.

30087bc

++

8936b87

Update dl_color.h

4e6b98c

++

1576e98

Merge branch 'more_more_tests' of github.com:jonahwilliams/engine int…

74ccfe9

…o more_more_tests

++

eb9bbb3

++

85e42bc

flutter-dashboard bot added the will affect goldens label Aug 13, 2024

jonahwilliams added 4 commits August 13, 2024 10:11

++

173a1e4

migrate even more tests.

23d1187

++

c479344

++

ab97fbe

chinmaygarde approved these changes Aug 14, 2024

View reviewed changes

bdero reviewed Aug 14, 2024

View reviewed changes

jonahwilliams added 2 commits August 14, 2024 17:34

adjustments.

9f5cd2c

fix alpha.

d452702

++

f600638

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

auto-submit bot merged commit d143913 into flutter:main Aug 15, 2024
31 checks passed

jonahwilliams added the revert Label used to revert changes in a closed and merged pull request. label Aug 15, 2024

auto-submit bot pushed a commit that referenced this pull request Aug 15, 2024

Revert "[Impeller] migrate blur tests to display list. (#54519)"

7cab2a1

This reverts commit d143913.

auto-submit bot mentioned this pull request Aug 15, 2024

Reverts "[Impeller] migrate blur tests to display list. (#54519)" #54564

Merged

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Aug 15, 2024

engine-flutter-autoroll mentioned this pull request Aug 15, 2024

Roll Flutter Engine from 76a1c64f4e63 to 6a65b8c649c6 (5 revisions) flutter/flutter#153485

Closed

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

d143913f1 [Impeller] migrate blur tests to display list. (flutter/eng…

6e38dc4

…ine#54519)

engine-flutter-autoroll mentioned this pull request Aug 15, 2024

Roll Flutter Engine from 76a1c64f4e63 to 971ddd9fe1bf (6 revisions) flutter/flutter#153487

Merged

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

d143913f1 [Impeller] migrate blur tests to display list. (flutter/eng…

9e25673

…ine#54519)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] migrate blur tests to display list. #54519

[Impeller] migrate blur tests to display list. #54519

jonahwilliams commented Aug 12, 2024 •

edited

Loading

flutter-dashboard bot commented Aug 13, 2024

flutter-dashboard bot commented Aug 13, 2024

chinmaygarde left a comment

jonahwilliams commented Aug 14, 2024

bdero Aug 14, 2024

bdero Aug 14, 2024

jonahwilliams Aug 14, 2024

jonahwilliams Aug 14, 2024

bdero Aug 14, 2024

bdero Aug 14, 2024

flutter-dashboard bot commented Aug 15, 2024

flutter-dashboard bot commented Aug 15, 2024

jonahwilliams commented Aug 15, 2024

[Impeller] migrate blur tests to display list. #54519

[Impeller] migrate blur tests to display list. #54519

Conversation

jonahwilliams commented Aug 12, 2024 • edited Loading

flutter-dashboard bot commented Aug 13, 2024

flutter-dashboard bot commented Aug 13, 2024

chinmaygarde left a comment

Choose a reason for hiding this comment

jonahwilliams commented Aug 14, 2024

bdero Aug 14, 2024

Choose a reason for hiding this comment

bdero Aug 14, 2024

Choose a reason for hiding this comment

jonahwilliams Aug 14, 2024

Choose a reason for hiding this comment

jonahwilliams Aug 14, 2024

Choose a reason for hiding this comment

bdero Aug 14, 2024

Choose a reason for hiding this comment

bdero Aug 14, 2024

Choose a reason for hiding this comment

flutter-dashboard bot commented Aug 15, 2024

flutter-dashboard bot commented Aug 15, 2024

jonahwilliams commented Aug 15, 2024

jonahwilliams commented Aug 12, 2024 •

edited

Loading