[Impeller] Run DisplayList render tests. #46422

flar · 2023-09-29T20:43:34Z

Revisiting this project as it may help with [attribute] X [rendering op] matrix testing of Impeller. It's not an exhaustive combinatorial test of all combinations of rendering attributes, but it attempts each op with each attribute both default and non-default.

For Impeller it only tests that the rendering produced output (i.e wasn't a NOP due to missing implementation), that the output changed if the given attribute was supposed to change the output, and that it rendered within the computed bounds that the rest of the engine will be assuming.

These changes will not result in any automatic CI testing of Impeller as is since they are only run on the Metal backend which is not enabled by default. Running out/host_[variant]/display_list_rendertests --enable-metal manually on a Mac host, though, will run the tests through Impeller and there will be a lot of error output. Also, there is one known crash as reported in flutter/flutter#135766

jonahwilliams · 2023-09-30T00:13:25Z

display_list/testing/dl_test_surface_metal.cc

+  const uint32_t ints_per_row_;
+};
+
+sk_sp<DlPixelData> DlMetalSurfaceProvider::ImpellerSnapshot(


I don't think you need to use the metal screenshotter here. provided you have access to an impeller context, you should be able to read back the texture data without worrying about timing issues either.

See https://github.com/flutter/engine/blob/main/lib/ui/painting/image_encoding_impeller.cc#L108-L129

I'm also adding a helper method to do this but its still WIP: https://github.com/flutter/engine/blob/main/lib/ui/painting/image_encoding_impeller.cc#L108-L129

you'd need to use something like fml::WaitableEvent as a latch as the submission callback will be invoked later on a different thread

Those point to the same code. When will the helper method be available? Is it worth using ScreenShotter until then as a temporary measure?

Oops! helper is #46394 . Uh, won't be done today for sure. But you can use the code from image_encoding_impeller today and then this works on all backends.

I looked into bringing over the changes there and there were quite a few dependencies I had to resolve. Would it be possible to just put this in as is using the ScreenShotter and then shift over to the new convenience mechanism when that lands?

Even though this doesn't get run by default right now, it does identify a number of Impeller bugs related to ImageFilter bounds and ignoring ImageFilters, so it would be useful to have this tool in the tree so that we can start fixing those problems. When the failing tests go to zero we can look at how to get it running under CI as a regression test. Many of the failures are dups (if we don't do something under one rendering op then we don't do it on many of them) so the number of problems that actually need fixing is less than the number of failing cases. I looked at the output and saw 3 or 4 problems that kept triggering over and over, but that's not an exhaustive search...

I think that is fine. we should probably just refactor the screenshotter to have implementations for all platforms

flar · 2023-10-03T00:41:31Z

I tagged 4 people for the next round of reviews for various reasons:

Jonah already looked at it before
Brandon might end up being the primary engineer for many of the bugs
Matan and Dan because this might factor into our testing plans
Chinmay because the others have a bunch of meetings on their plates... ;)

jonahwilliams · 2023-10-06T15:37:01Z

Why do we specify the Impeller/Skia cases separately? Could we use the DL to specify both cases?

flar · 2023-10-06T21:12:01Z

Why do we specify the Impeller/Skia cases separately? Could we use the DL to specify both cases?

All tests are specified in 2 forms - SkCanvas calls and DlCanvas calls - we need both of these because one of the goals of the tests is to compare the results of rendering DLCanvas through Skia and rendering SkCanvas through Skia. That was one of the original goals of the tests. Basically it compares "rendering a thick line with a blur" through SkCanvas calls and "rendering a thick line with a blur" in DlCanvas calls and verifies that both render the same thing when using Skia as a backend.

To add Impeller, I'm not adding a new specification of the test code, I reuse the DL code, but I have to run it against Impeller. So, we have 2 specs for everything - one in SkCanvas and one in DlCanvas - and then we have 3 executors for them - one is SkCanvas rendering with Skia, another is DlCanvas rendering with Skia, and a 3rd is DlCanvas rendering with Impeller.

We could eliminate the SkCanvas rendering code at some point, but we'd have to shift to something like a golden image suite to do that. Essentially, for now, we are using a cross-check with Skia as if it were a golden image suite for "rendering DL through Skia".

The new nascent Impeller support isn't comparing output to Skia or goldens, so it is mostly just verifying independently measurable things like "that should have caused a change" or "that went out of bounds". Even with those basic tests (when run manually) it is finding faults in Impeller.

jonahwilliams

LGTM

)

…136220) flutter/engine@73da593...8f2fa3c 2023-10-09 skia-flutter-autoroll@skia.org Roll Skia from 26d70e6999f6 to bd77f099b4f9 (3 revisions) (flutter/engine#46685) 2023-10-09 flar@google.com [Impeller] Run DisplayList render tests. (flutter/engine#46422) 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 chinmaygarde@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

…lutter#136220) flutter/engine@73da593...8f2fa3c 2023-10-09 skia-flutter-autoroll@skia.org Roll Skia from 26d70e6999f6 to bd77f099b4f9 (3 revisions) (flutter/engine#46685) 2023-10-09 flar@google.com [Impeller] Run DisplayList render tests. (flutter/engine#46422) 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 chinmaygarde@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

Revisiting this project as it may help with [attribute] X [rendering op] matrix testing of Impeller. It's not an exhaustive combinatorial test of all combinations of rendering attributes, but it attempts each op with each attribute both default and non-default. For Impeller it only tests that the rendering produced output (i.e wasn't a NOP due to missing implementation), that the output changed if the given attribute was supposed to change the output, and that it rendered within the computed bounds that the rest of the engine will be assuming. These changes will not result in any automatic CI testing of Impeller as is since they are only run on the Metal backend which is not enabled by default. Running `out/host_[variant]/display_list_rendertests --enable-metal` manually on a Mac host, though, will run the tests through Impeller and there will be a lot of error output. Also, there is one known crash as reported in flutter/flutter#135766

github-actions bot added the e: impeller label Sep 29, 2023

chinmaygarde changed the title ~~Run DisplayList render tests on Impeller~~ [Impeller] Run DisplayList render tests. Sep 29, 2023

jonahwilliams reviewed Sep 30, 2023

View reviewed changes

flar added 2 commits October 2, 2023 14:53

Run DisplayList render tests on Impeller

114b640

better error output filenames and auto-create output dir

e23f76a

flar force-pushed the dl-rendertests-on-impeller branch from a6b12fd to e23f76a Compare October 2, 2023 23:28

use provided temp directory mechanism

60e4c4a

flar requested review from jonahwilliams, bdero, matanlurey and chinmaygarde October 3, 2023 00:38

flar requested a review from dnfield October 3, 2023 00:46

chinmaygarde assigned flar Oct 3, 2023

jonahwilliams approved these changes Oct 6, 2023

View reviewed changes

flar added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 9, 2023

auto-submit bot merged commit a8a3eeb into flutter:main Oct 9, 2023

engine-flutter-autoroll mentioned this pull request Oct 9, 2023

Roll Flutter Engine from 73da593bac54 to 8f2fa3ceb8a0 (2 revisions) flutter/flutter#136220

Merged

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

a8a3eeb0c [Impeller] Run DisplayList render tests. (flutter/engine#46422

9cfeec0

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] Run DisplayList render tests. #46422

[Impeller] Run DisplayList render tests. #46422

flar commented Sep 29, 2023 •

edited

Loading

jonahwilliams Sep 30, 2023

jonahwilliams Sep 30, 2023

flar Sep 30, 2023

jonahwilliams Sep 30, 2023

flar Oct 2, 2023

jonahwilliams Oct 3, 2023

flar commented Oct 3, 2023 •

edited

Loading

jonahwilliams commented Oct 6, 2023

flar commented Oct 6, 2023 •

edited

Loading

jonahwilliams left a comment

[Impeller] Run DisplayList render tests. #46422

[Impeller] Run DisplayList render tests. #46422

Conversation

flar commented Sep 29, 2023 • edited Loading

jonahwilliams Sep 30, 2023

Choose a reason for hiding this comment

jonahwilliams Sep 30, 2023

Choose a reason for hiding this comment

flar Sep 30, 2023

Choose a reason for hiding this comment

jonahwilliams Sep 30, 2023

Choose a reason for hiding this comment

flar Oct 2, 2023

Choose a reason for hiding this comment

jonahwilliams Oct 3, 2023

Choose a reason for hiding this comment

flar commented Oct 3, 2023 • edited Loading

jonahwilliams commented Oct 6, 2023

flar commented Oct 6, 2023 • edited Loading

jonahwilliams left a comment

Choose a reason for hiding this comment

flar commented Sep 29, 2023 •

edited

Loading

flar commented Oct 3, 2023 •

edited

Loading

flar commented Oct 6, 2023 •

edited

Loading