[Impeller] attempt to get validation errors from CI unittests. #51341

jonahwilliams · 2024-03-11T22:43:51Z

Enables Vulkan validation layers for macOS and linux builds, enables macOS validation for macOS builds.

There may still be remaining issues with macOS validation, I need to verify I can reproduce flutter/flutter#143324 . I plan to do that separately

jonahwilliams · 2024-03-12T00:43:18Z

impeller/aiks/BUILD.gn

@@ -116,6 +116,8 @@ template("aiks_unittests_component") {
      "//flutter/impeller/typographer/backends/stb:typographer_stb_backend",
      "//flutter/testing:testing_lib",
      "//flutter/third_party/txt",
+      "//third_party/vulkan_validation_layers",


TODO: remove

flutter-dashboard · 2024-03-12T00:44:36Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

jonahwilliams · 2024-03-12T04:55:24Z

testing/run_tests.py

@@ -543,7 +546,8 @@ def make_test(name, flags=None, extra_env=None):
        shuffle_flags + [
            '--enable_vulkan_validation',
            # TODO(https://github.com/flutter/flutter/issues/142642): Remove this.
-            '--gtest_filter=-*OpenGLES',
+            # TODO EVEN MORE
+            '--gtest_filter=*Metal',


The vulkan variants crash, I think this is because they weren't running before? I'd need to double check. Either that or they are just hitting validation checks that are fatal in a way the test harness can't handle

gaaclarke · 2024-03-12T15:27:54Z

impeller/entity/entity_unittests.cc

@@ -2267,7 +2277,7 @@ TEST_P(EntityTest, RuntimeEffectCanSuccessfullyRender) {
                  .has_value());
 }

-TEST_P(EntityTest, RuntimeEffectSetsRightSizeWhenUniformIsStruct) {
+TEST_P(EntityTest, DISABLED_RuntimeEffectSetsRightSizeWhenUniformIsStruct) {


Please add an issue for a disabled test.

I'm not disabling it here anymore, i filled one generic skip test issue

gaaclarke · 2024-03-12T15:29:38Z

impeller/golden_tests/golden_playground_test_mac.cc

@@ -69,7 +69,26 @@ static const std::vector<std::string> kSkipTests = {
    "impeller_Play_AiksTest_CanRenderClippedRuntimeEffects_Vulkan",
 };

-static const std::vector<std::string> kVulkanDenyValidationTests = {};
+static const std::vector<std::string> kVulkanDenyValidationTests = {
+    "impeller_Play_GaussianBlurFilterContentsTest_"    //


nit: turn off clang format so these are all on one line

gaaclarke · 2024-03-12T15:31:57Z

impeller/playground/backend/vulkan/playground_impl_vk.cc

@@ -60,16 +60,7 @@ void PlaygroundImplVK::DestroyWindowHandle(WindowHandle handle) {

 PlaygroundImplVK::PlaygroundImplVK(PlaygroundSwitches switches)


Ideally this would be a static function that returns StatusOr<std::unique_ptr<PlaygroundImplVK>> instead of short circuiting a constructor, leaving a half initialized object.

https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors

I think we can just lift the check out of the constructor, since its a static method. I made this an FML_CHECK

gaaclarke · 2024-03-12T15:32:51Z

impeller/playground/playground_impl.cc

@@ -19,6 +20,50 @@
 #include "impeller/playground/backend/vulkan/playground_impl_vk.h"
 #endif  // IMPELLER_ENABLE_VULKAN

+namespace {
+static const std::vector<std::string> kVulkanDenyValidationTests = {


Let's share this instead of duplicating it twice.

gaaclarke · 2024-03-12T15:33:56Z

impeller/renderer/backend/vulkan/capabilities_vk.cc

@@ -41,7 +41,7 @@ CapabilitiesVK::CapabilitiesVK(bool enable_validations) {
  validations_enabled_ =
      enable_validations && HasLayer("VK_LAYER_KHRONOS_validation");
  if (enable_validations && !validations_enabled_) {
-    FML_LOG(ERROR)
+    FML_LOG(FATAL)


I think the safer thing is to add a test that makes sure this doesn't get printed out. I'm concerned about sending users down this path on accident.

For now I'm keeping this fatal for testing.

So we have two approaches here:

Keep the existing behavior but add checks that it is running as expected on CI.

Make it fatal but do more work to make sure users don't fall down the path.

What I'm doing for impeller validations is that it is off by default (existing behavior) but I swap it on in the main() for the test runner and adding a test to make sure it it is on.

I'm moving this back to error, as a follow up we need to make sure accidentally turning off validations causes a test somewhere to fail - ideally once for each shard.

flutter/flutter#145041

jason-simmons · 2024-03-12T23:34:05Z

testing/run_tests.py

+  return extra_env
+
+
+def metal_validation_env(build_dir):


This contains both Metal and Vulkan configuration variables.

Rename this to metal_and_vulkan_validation_env?

Or have the caller create a merged dictionary from metal_validation_env and vulkan_validation_env?

jason-simmons · 2024-03-13T01:19:10Z

tools/gn

@@ -788,15 +788,9 @@ def to_gn_args(args):
  if args.enable_impeller_trace_canvas:
    gn_args['impeller_trace_canvas'] = True

-  if args.enable_impeller_vulkan:
-    gn_args['impeller_enable_vulkan'] = True
-
  if args.enable_impeller_vulkan_playgrounds:
    gn_args['impeller_enable_vulkan_playgrounds'] = True


Remove impeller_enable_vulkan_playgrounds from tools/gn now that it no longer exists in the GN scripts

flutter-dashboard · 2024-03-13T02:32: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.

Changes reported for pull request #51341 at sha c53ff77

zanderso · 2024-03-13T03:21:15Z

tools/gn

      action='store_true',
      help='Enable the Impeller Vulkan Playgrounds. ' +
      'The Impeller Vulkan backend needs to be enabled.'
  )

  parser.add_argument(
      '--enable-impeller-opengles',
-      default=False,
+      default=True,


Should the default value for --enable-impeller-vulkan also be switched?

Lets try it

zanderso · 2024-03-13T03:21:46Z

impeller/tools/impeller.gni


  # Whether the Vulkan backend is enabled.
-  impeller_enable_vulkan = (is_linux || is_win || is_android ||
+  impeller_enable_vulkan = (is_linux || is_win || is_android || is_mac ||


Why does this one have enable_unittests but the opengles one doesn't?

Lets try it

zanderso · 2024-03-13T03:22:39Z

tools/gn

@@ -1253,15 +1246,15 @@ def parse_args(args):

  parser.add_argument(
      '--enable-impeller-vulkan-playgrounds',
-      default=False,
+      default=True,


The GN invocations all live in this repo, so if this flag is a no-op now, and is no longer passed in the json files, then it can be safely removed from here.

matanlurey

Looks a lot better! Thanks for pushing on this!

gaaclarke

lgtm!

…ts. (flutter/engine#51341)

…145106) flutter/engine@6ceccf8...9551b49 2024-03-13 jonahwilliams@google.com [Impeller] fix heap selection process for YUV textures. (flutter/engine#51262) 2024-03-13 6844906+zijiehe-google-com@users.noreply.github.com [Fuchsia] Enable sound null safety everywhere (flutter/engine#51355) 2024-03-13 30870216+gaaclarke@users.noreply.github.com [Impeller] added missing golden test for DrawScaledTextWithPerspectiveSaveLayer (flutter/engine#51368) 2024-03-13 skia-flutter-autoroll@skia.org Roll Skia from dc7443fa4d88 to 1ad0d0fabe7a (1 revision) (flutter/engine#51373) 2024-03-13 tugorez@users.noreply.github.com Implement PlatformDispatcher.requestViewFocusChange on web. (flutter/engine#50535) 2024-03-13 jonahwilliams@google.com [Impeller] attempt to get validation errors from CI unittests. (flutter/engine#51341) 2024-03-13 skia-flutter-autoroll@skia.org Roll Dart SDK from b19e0995f317 to 18ec60df0774 (1 revision) (flutter/engine#51374) 2024-03-13 jonahwilliams@google.com [scenario app] make image matching fuzzier. (flutter/engine#51376) 2024-03-13 skia-flutter-autoroll@skia.org Roll Skia from bbe453e3525d to dc7443fa4d88 (1 revision) (flutter/engine#51371) 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 bdero@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://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

testing.

f86860c

github-actions bot added the e: impeller label Mar 11, 2024

jonahwilliams mentioned this pull request Mar 11, 2024

[Impeller] Vulkan playground tests aren't running on CI (with validations) flutter/flutter#144967

Closed

jonahwilliams added 3 commits March 11, 2024 16:25

make fatal

e06c9c9

Add validation layers to deps?

dcd1c5e

more validation

29c7edc

jonahwilliams commented Mar 12, 2024

View reviewed changes

jonahwilliams changed the title ~~testing.~~ [Impeller] attempt to get validation errors from CI unittests. Mar 12, 2024

jonahwilliams added 16 commits March 11, 2024 17:57

more cleanups

408e7f5

start with test skips.

be3271b

++

43e202a

++

1c05893

add back gles flag.

1cecd26

add more skips.

d72221b

++

fd68de3

++

a453f7f

more skips.

ab9a072

++

26dc17f

++

4e2eeac

++

2cf6407

++

79231ec

++

9e378b1

++

b8bcfcb

++

f3ee710

jonahwilliams commented Mar 12, 2024

View reviewed changes

jonahwilliams added 3 commits March 11, 2024 22:33

more fixes

eea1bce

point field fixes.

7917811

++

fb6e3ee

gaaclarke reviewed Mar 12, 2024

View reviewed changes

++

bf9d430

jonahwilliams requested review from gaaclarke, matanlurey, zanderso and jason-simmons March 12, 2024 22:49

jason-simmons reviewed Mar 12, 2024

View reviewed changes

jonahwilliams added 4 commits March 12, 2024 17:02

merge dictionaries.

50fcdde

++

412912b

remove unused build_dir.

2cec37c

++

34ce5b6

jason-simmons reviewed Mar 13, 2024

View reviewed changes

remove unused gn flag.

c53ff77

jason-simmons approved these changes Mar 13, 2024

View reviewed changes

This was referenced Mar 13, 2024

[Impeller] use DefaultUniformAlignment for compute buffers. #51336

Closed

Enable Metal validations for impeller_golden_tests #51346

Closed

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

Merge branch 'main' into turn_on_validation

50cfa9d

zanderso reviewed Mar 13, 2024

View reviewed changes

more adjustments.

3d105f0

matanlurey approved these changes Mar 13, 2024

View reviewed changes

gaaclarke approved these changes Mar 13, 2024

View reviewed changes

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

auto-submit bot merged commit 67e6328 into flutter:main Mar 13, 2024
28 checks passed

engine-flutter-autoroll mentioned this pull request Mar 13, 2024

Roll Flutter Engine from 6ceccf894c41 to 67e632882c8b (4 revisions) flutter/flutter#145103

Closed

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

67e632882 [Impeller] attempt to get validation errors from CI unittes…

a960e87

…ts. (flutter/engine#51341)

engine-flutter-autoroll mentioned this pull request Mar 13, 2024

Roll Flutter Engine from 6ceccf894c41 to 9551b49b3fa1 (9 revisions) flutter/flutter#145106

Merged

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

67e632882 [Impeller] attempt to get validation errors from CI unittes…

6bca49f

…ts. (flutter/engine#51341)

jonahwilliams mentioned this pull request Mar 13, 2024

[Impeller] validation errors on debug builds of AiksTest.CanDrawPoints in impeller_golden_tests flutter/flutter#144959

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] attempt to get validation errors from CI unittests. #51341

[Impeller] attempt to get validation errors from CI unittests. #51341

jonahwilliams commented Mar 11, 2024 •

edited

Loading

jonahwilliams Mar 12, 2024

jonahwilliams Mar 12, 2024

flutter-dashboard bot commented Mar 12, 2024

jonahwilliams Mar 12, 2024

gaaclarke Mar 12, 2024

jonahwilliams Mar 12, 2024

gaaclarke Mar 12, 2024

jonahwilliams Mar 12, 2024

gaaclarke Mar 12, 2024

jonahwilliams Mar 12, 2024

jonahwilliams Mar 12, 2024

gaaclarke Mar 12, 2024

jonahwilliams Mar 12, 2024

gaaclarke Mar 12, 2024

jonahwilliams Mar 12, 2024

gaaclarke Mar 12, 2024

jonahwilliams Mar 12, 2024

jonahwilliams Mar 12, 2024

jason-simmons Mar 12, 2024

jonahwilliams Mar 13, 2024

jason-simmons Mar 13, 2024

jonahwilliams Mar 13, 2024

flutter-dashboard bot commented Mar 13, 2024

zanderso Mar 13, 2024

jonahwilliams Mar 13, 2024

zanderso Mar 13, 2024

jonahwilliams Mar 13, 2024

zanderso Mar 13, 2024

jonahwilliams Mar 13, 2024

matanlurey left a comment

gaaclarke left a comment

		@@ -60,16 +60,7 @@ void PlaygroundImplVK::DestroyWindowHandle(WindowHandle handle) {

		PlaygroundImplVK::PlaygroundImplVK(PlaygroundSwitches switches)

[Impeller] attempt to get validation errors from CI unittests. #51341

[Impeller] attempt to get validation errors from CI unittests. #51341

Conversation

jonahwilliams commented Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matanlurey left a comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

jonahwilliams commented Mar 11, 2024 •

edited

Loading