[Impeller] flutter_tester --enable-impeller #46389

dnfield · 2023-09-28T23:16:12Z

This patch does the following:

Updates flutter_tester to set up an Impeller rendering context and surface if --enable-impeller is set to true, using the Vulkan backend with Swiftshader.
Updates run_tests.py to run all tests except the smoke test (that one really has no rendering impact whatsoever) with and without --enable-impeller.
Updates a few tests to work that were trivial:
- A couple tests needed updated goldens for very minor rendering differences. Filed //testing/dart/*.dart should use Skia gold instead of hand rolled golden testing. flutter#135684 to track using Skia gold for this instead.
- Disabled SKP screenshotting if Impeller is enabled, and updated the test checking that to verify an error is thrown if an SKP is requested.
- The Dart GPU based test now asserts that the gpu context is available if Impeller is enabled, and does not deadlock if run in a single threaded mode.
- We were missing some trace events around Canvas::SaveLayer for Impeller as compared to Skia.
- A couple other tests had strict checks about exception messages that are slightly different between Skia and Impeller.
I've filed bugs for other tests that may require a little more work, and skipped them for now. For FragmentProgram on Vulkan I reused an existing bug.
Decouples whether playgrounds run with Vulkan from whether Impeller was built with Vulkan. This means a new GN argument must be set to run the playgrounds with Vulkan (impeller_enable_vulkan_playgrounds), and avoids any messy logic around whether we fallback to swiftshader or not.

This is part of my attempt to address flutter/flutter#135693, although @chinmaygarde and I had slightly different ideas about how to do this.

The goals here are:

Run the Dart unit tests we already have with Impeller enabled.
Enable running more of the framework tests (including gold tests) with Impeller enabled.
Run all of these tests via public dart:ui API rather than mucking around in C++ internals in the engine.

bdero

LGTM, just nits!

bdero · 2023-09-29T17:44:58Z

shell/testing/BUILD.gn

+    } else if (is_mac) {
+      libs += [ "$root_out_dir/libvk_swiftshader.dylib" ]
+    } else if (is_linux) {
+      libs += [ "$root_out_dir/libvk_swiftshader.so" ]


Nit: I think we have these swiftshader lib names copied around in 3 or 4 places at this point (including in some source files). Not sure if there's one good place we could consolidate them.

I ended up moving this to //flutter/vulkan/swiftshader_path.h

bdero · 2023-09-29T18:11:57Z

shell/testing/tester_main.cc

+    if (res != impeller::vk::Result::eSuccess) {
+      VALIDATION_LOG << "Could not create surface for tester "
+                     << impeller::vk::to_string(res);
+      return -1;


Suggested change

return -1;

return EXIT_FAILURE;

done here and elsewhere

bdero · 2023-09-29T18:12:06Z

shell/testing/tester_main.cc

+    impeller_surface_context = impeller_context->CreateSurfaceContext();
+    if (!impeller_surface_context->SetWindowSurface(std::move(surface))) {
+      VALIDATION_LOG << "Could not set up surface for context.";
+      return -1;


Suggested change

return -1;

return EXIT_FAILURE;

bdero · 2023-09-29T18:13:21Z

shell/testing/tester_main.cc

+    impeller_context = impeller::ContextVK::Create(std::move(context_settings));
+    if (!impeller_context || !impeller_context->IsValid()) {
+      VALIDATION_LOG << "Could not create Vulkan context.";
+      return -1;


Suggested change

return -1;

return EXIT_FAILURE;

bdero · 2023-09-29T18:14:54Z

shell/testing/tester_main.cc

+            impeller_modern_shaders_vk_data, impeller_modern_shaders_vk_length),
+#if IMPELLER_ENABLE_3D
+        std::make_shared<fml::NonOwnedMapping>(
+            impeller_scene_shaders_vk_data, impeller_scene_shaders_vk_length),


Thank you for taking care of Scene. 🙏

zanderso · 2023-10-06T20:53:43Z

testing/dart/canvas_test.dart


 typedef CanvasCallback = void Function(Canvas canvas);

+bool get impellerEnabled => Platform.executableArguments.contains('--enable-impeller');


Consider factoring this definition out into a .dart file under testing/dart, and importing it where needed.

zanderso · 2023-10-06T20:58:50Z

testing/run_tests.py

        )
-        yield gather_dart_test(build_dir, dart_test_file, True, True)
-        yield gather_dart_test(build_dir, dart_test_file, False, True)
+        yield gather_dart_test(build_dir, dart_test_file, True, False, True)


This probably should have been done earlier, but 4 unnamed boolean flags seems like too many. Maybe the simplest thing is just to always pass the arguments as named and always pass them all?

zanderso · 2023-10-10T19:33:22Z

Many tests appear to be segfaulting. Maybe the tests that are failing under asan are a good starting point?

…imp_swift

dnfield · 2023-10-11T17:24:01Z

Many tests appear to be segfaulting. Maybe the tests that are failing under asan are a good starting point?

Most of those were an issue where the swiftshader dylib was getting loaded and unloaded too quickly. There's still one ASAN issue I'm looking at.

dnfield · 2023-10-11T20:57:20Z

I'm not seeing segfaults anymore. I do see some flaky failures that seem unrelated to this patch, filed some bugs. Looks like this will be g2g now.

…#46389)

…136422) flutter/engine@8bf1460...05e26c1 2023-10-11 [email protected] Run the binary size treemap script from the buildroot directory (flutter/engine#46740) 2023-10-11 [email protected] [Impeller] flutter_tester --enable-impeller (flutter/engine#46389) 2023-10-11 [email protected] Switch to Chrome For Testing instead of Chromium (flutter/engine#46683) 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 [email protected],[email protected],[email protected] 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

#46895) Follows up on #46389 That patch was too permissive in cases where a build system enables impeller but not vulkan. This change makes the build succeed in such systems.

flutter#46895) Follows up on flutter#46389 That patch was too permissive in cases where a build system enables impeller but not vulkan. This change makes the build succeed in such systems.

@chinmaygarde

This patch does the following: - Updates `flutter_tester` to set up an Impeller rendering context and surface if `--enable-impeller` is set to true, using the Vulkan backend with Swiftshader. - Updates `run_tests.py` to run all tests except the smoke test (that one really has no rendering impact whatsoever) with and without `--enable-impeller`. - Updates a few tests to work that were trivial: - A couple tests needed updated goldens for very minor rendering differences. Filed flutter/flutter#135684 to track using Skia gold for this instead. - Disabled SKP screenshotting if Impeller is enabled, and updated the test checking that to verify an error is thrown if an SKP is requested. - The Dart GPU based test now asserts that the gpu context is available if Impeller is enabled, and does not deadlock if run in a single threaded mode. - We were missing some trace events around `Canvas::SaveLayer` for Impeller as compared to Skia. - A couple other tests had strict checks about exception messages that are slightly different between Skia and Impeller. - I've filed bugs for other tests that may require a little more work, and skipped them for now. For FragmentProgram on Vulkan I reused an existing bug. This is part of my attempt to address flutter/flutter#135693, although @chinmaygarde and I had slightly different ideas about how to do this. The goals here are: - Run the Dart unit tests we already have with Impeller enabled. - Enable running more of the framework tests (including gold tests) with Impeller enabled. - Run all of these tests via public `dart:ui` API rather than mucking around in C++ internals in the engine.

#46895) Follows up on #46389 That patch was too permissive in cases where a build system enables impeller but not vulkan. This change makes the build succeed in such systems.

dnfield added 3 commits September 28, 2023 16:02

[Impeller] flutter_tester --enable-impeller

3fb8de3

Update run_tests.py

af399e7

Update run_tests.py

28f759b

dnfield requested review from bdero, chinmaygarde and zanderso September 28, 2023 23:16

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

dnfield mentioned this pull request Sep 28, 2023

[Impeller] Create a dart:ui fixtures based native test harness that renders talks to Skia Gold directly. flutter/flutter#135693

Closed

dnfield added 4 commits September 29, 2023 10:16

missing guard

c91fb44

windows build

0e49330

include

bf08e66

shutdown, fix non-impeller build

d4aef8a

bdero approved these changes Sep 29, 2023

View reviewed changes

dnfield added 3 commits September 29, 2023 16:46

fuchsia builds

ee377fc

Merge remote-tracking branch 'upstream/main' into test_imp_swift

1c35a29

one more guard for fuchsia

4f06076

chinmaygarde assigned dnfield Oct 3, 2023

dnfield added 2 commits October 4, 2023 10:28

more skips for linux, specify pixel format

de4abfa

fix condition

5975e14

zanderso reviewed Oct 6, 2023

View reviewed changes

dnfield added 3 commits October 9, 2023 16:24

make swiftshader lookup dynamic

11d53f2

nits

79f99c9

fix header

7cd05f6

zanderso approved these changes Oct 10, 2023

View reviewed changes

dnfield added 5 commits October 10, 2023 13:52

Keep swiftshader lib open

effb21b

fix

28eea9a

Merge branch 'test_imp_swift' of github.com:dnfield/engine into test_…

5f78735

…imp_swift

refactor to simpler struct without forward decls

f801e47

Fixes

58b1cb6

dnfield added 2 commits October 11, 2023 10:25

merge

0b137b9

more

fb4c806

This was referenced Oct 11, 2023

Flaky ASAN failure in Dart_Cleanup flutter/flutter#136402

Closed

SecondaryVsyncCallbackShouldBeCalledAfterVsyncCallback is flaky flutter/flutter#136403

Open

dnfield added 2 commits October 11, 2023 13:08

Fix analysis issues

48a6d52

oops

46a393a

pylint

91804dc

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

auto-submit bot merged commit 791e90a into flutter:main Oct 11, 2023

dnfield deleted the test_imp_swift branch October 11, 2023 21:44

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

Roll Flutter Engine from 8bf1460892c6 to 05e26c1b2c79 (3 revisions) flutter/flutter#136422

Merged

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

791e90aad [Impeller] flutter_tester --enable-impeller (flutter/engine…

375d12e

…#46389)

dnfield mentioned this pull request Oct 13, 2023

[Impeller] Only allow Impeller in flutter_tester if vulkan is enabled. #46895

Merged

XilaiZhang mentioned this pull request Oct 13, 2023

[flutter roll cp][Impeller] Only allow Impeller in flutter_tester if vulkan is enabled… #46904

Closed


		typedef CanvasCallback = void Function(Canvas canvas);

		bool get impellerEnabled => Platform.executableArguments.contains('--enable-impeller');

[Impeller] flutter_tester --enable-impeller #46389

[Impeller] flutter_tester --enable-impeller #46389

Uh oh!

Conversation

dnfield commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanderso commented Oct 10, 2023

Uh oh!

dnfield commented Oct 11, 2023

Uh oh!

dnfield commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dnfield commented Sep 28, 2023 •

edited

Loading