[Impeller] turned on validations for all debug builds #45350

gaaclarke · 2023-08-31T21:50:42Z

This adds the following shared libraries to the flutter.jar for debug builds on arm64:

lib/arm64-v8a/libVkLayer_khronos_validation.so
lib/arm64-v8a/libc++_shared.so

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jonahwilliams

LGTM if licensing is good.

We can also run a debug build smoke test (later) in the devicelab to make sure we're not hitting validation errors.

flutter-dashboard · 2023-08-31T23:13:16Z

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 Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on 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.

gaaclarke · 2023-08-31T23:35:52Z

This runs fine locally but fails fails to build on CI. Presumably the api level is coming from the buildroot (https://github.com/flutter/buildroot/blob/6b0243e97d94836767ea8b94a477d7d15ee3bb91/build/config/android/config.gni#L47).

The API guard isn't even apropos for us since we don't load the dynamic library unless we are using the vulkan backend (and thus are on a platform that supports vulkan, ie >= 23).

I'll have to find a way to override this API setting, probably editing the buildroot tomorrow.

zanderso · 2023-08-31T23:46:01Z

shell/platform/android/BUILD.gn

-  if (enable_vulkan_validation_layers) {
+  if (enable_vulkan_validation_layers ||
+      (impeller_enable_vulkan && current_cpu == "arm64" &&
+       flutter_runtime_mode == "debug")) {
    assert(impeller_enable_vulkan)
    deps += [ "//third_party/vulkan_validation_layers" ]


What's the difference between this and //third_party/vulkan-deps/vulkan-validation-layers ?

Oh, I see. They're the same thing https://github.com/flutter/buildroot/blob/master/build/secondary/third_party/vulkan_validation_layers/BUILD.gn.

gaaclarke · 2023-09-01T18:05:35Z

@zanderso @jonahwilliams actually this may not be possible to implement because of limitations in GN. So the problem is that we need android >= 23 to build the validation layers. The engine supports android 22. It's not a problem at runtime because we can check the android version before loading the dylib.

The way we get around this build problem today is that if you are building a local engine with validation layers enabled we globally set the android api level to 26. We can't do the same trick when building the engine for everyone. There doesn't seem to be a way to have different android api levels for different targets in GN. I'll verify this.

gaaclarke · 2023-09-01T18:07:05Z

The other option would be to split our build process into multiple ninja invocations then bundle everything together. I'm not sure how we'd do that, but at face value sounds involved.

jonahwilliams · 2023-09-01T18:08:13Z

I think that if this is too more work, instead we should just point folks and the triagers to https://developer.android.com/ndk/guides/graphics/validation-layer

zanderso · 2023-09-01T18:19:10Z

I'd like to study this a bit to see if I can make a suggestion. I can sometimes bend GN to my will.

gaaclarke · 2023-09-01T18:24:42Z

I'd like to study this a bit to see if I can make a suggestion. I can sometimes bend GN to my will.

Here is the override we currently use for reference:

engine/tools/gn

Line 652 in 069354f

gn_args['android_api_level'] = 26

Our options as I see them:

Bump the engine to only support android >= 23
Split our build into multiple GN builds and rely on a build script that runs ninja twice and combines the result
(forbidden) ~~package prebuilt validation layers~~
Rely on showing users how to inject the validation layers. Maybe there is something we can do to grease the wheels here? A script that modifies an apk?

gaaclarke · 2023-09-01T23:02:54Z

Heads up I'm putting this into the autosubmit queue. I've been in contact with the reviewers about changes we made since approval. I just wanted to notify everyone since it has technically changed.

jonahwilliams

Still LGTM, thanks @gaaclarke and @zanderso !

zanderso · 2023-09-02T04:44:55Z

tools/gn

@@ -646,7 +646,9 @@ def to_gn_args(args):
  if args.unoptimized and args.target_os == 'android' and args.android_cpu == 'arm64':
    enable_vulkan_validation = True

-  if enable_vulkan_validation:
+  if enable_vulkan_validation or (args.enable_impeller_vulkan and


With the changes to the GN build for the validation layers .so, does the API level for the whole build still need to be bumped up to 26 down on line 654?

Nope, not necessary anymore. I can rip it out.

…ter/engine#45350)

…sions) (#133924) Manual roll requested by [email protected] flutter/engine@489c399...e496eec 2023-09-02 [email protected] Roll Skia from 2d8849f9f0cc to 15f77147a3ec (1 revision) (flutter/engine#45414) 2023-09-02 [email protected] Roll Fuchsia Mac SDK from OF4TS05qlWCjukWw6... to MesZPNdj-uw8VdCyV... (flutter/engine#45413) 2023-09-02 [email protected] Remove --disable-service-auth-codes (flutter/engine#45356) 2023-09-02 [email protected] [Impeller] Import cstring for memcpy. (flutter/engine#45408) 2023-09-02 [email protected] Roll Dart SDK from cdf1ce0c6d7e to a5c7102af509 (1 revision) (flutter/engine#45412) 2023-09-02 [email protected] Roll ANGLE from 179bd7762ffa to ebf1e7163216 (1 revision) (flutter/engine#45411) 2023-09-02 [email protected] Remove deprecated MOCK_METHODx calls (flutter/engine#45307) 2023-09-02 [email protected] [Impeller] Better demonstrate blur and draw picture? (flutter/engine#45388) 2023-09-02 [email protected] [Impeller] Make paths externally immutable, update all tests to use PathBuilder to create Path. (flutter/engine#45393) 2023-09-02 [email protected] Roll ANGLE from 962fdf7b7882 to 179bd7762ffa (1 revision) (flutter/engine#45409) 2023-09-02 [email protected] Cull the RTree bounds when they are forwarded in DrawDisplayList (flutter/engine#45358) 2023-09-02 [email protected] Roll Skia from fedff79a6afc to 2d8849f9f0cc (3 revisions) (flutter/engine#45407) 2023-09-02 [email protected] [impeller] premultiply vertices colors. (flutter/engine#45406) 2023-09-01 [email protected] Roll ANGLE from 6a09e41ce6ea to 962fdf7b7882 (224 revisions) (flutter/engine#45400) 2023-09-01 [email protected] Roll Skia from 22ae23891e8e to fedff79a6afc (1 revision) (flutter/engine#45405) 2023-09-01 [email protected] [Impeller] turned on validations for all debug builds (flutter/engine#45350) 2023-09-01 [email protected] Roll Fuchsia Mac SDK from sk7JBGzW1Jw10Wy-T... to OF4TS05qlWCjukWw6... (flutter/engine#45403) 2023-09-01 [email protected] Roll Skia from 2c0405489966 to 22ae23891e8e (1 revision) (flutter/engine#45402) 2023-09-01 [email protected] [Windows] Update vsync on raster thread (flutter/engine#45310) 2023-09-01 [email protected] Roll Dart SDK from a2ea759c16cc to cdf1ce0c6d7e (1 revision) (flutter/engine#45397) 2023-09-01 [email protected] Roll Skia from f3f6c733c7e6 to 2c0405489966 (1 revision) (flutter/engine#45396) 2023-09-01 [email protected] Roll Skia from 02fa14799c6c to f3f6c733c7e6 (1 revision) (flutter/engine#45394) 2023-09-01 [email protected] Roll Skia from d5d3b0d4ee77 to 02fa14799c6c (2 revisions) (flutter/engine#45392) 2023-09-01 [email protected] [ios][ios17][text_input]fix text input system highlight in iOS 17 Beta 7 with firstRectForRange (flutter/engine#45303) 2023-09-01 [email protected] Roll Skia from d6266ef14a7e to d5d3b0d4ee77 (2 revisions) (flutter/engine#45389) 2023-09-01 [email protected] Roll Dart SDK from 0c121a6431cc to a2ea759c16cc (1 revision) (flutter/engine#45384) Also rolling transitive DEPS: fuchsia/sdk/core/mac-amd64 from sk7JBGzW1Jw1 to MesZPNdj-uw8 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://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

not necessary after: #45350 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

gbak · 2024-07-19T13:28:05Z

@gaaclarke is there a way to exclude this library from the Android build? The Vulkan library doubles the size of our debug apks. I've seen the suggestion of a jniLibs exclude, but didn't know if that would have any adverse behaviors.
excludes.add("**/libVkLayer_khronos_validation.so")

gaaclarke · 2024-07-23T15:33:43Z

@gaaclarke is there a way to exclude this library from the Android build? The Vulkan library doubles the size of our debug apks. I've seen the suggestion of a jniLibs exclude, but didn't know if that would have any adverse behaviors.
excludes.add("**/libVkLayer_khronos_validation.so")

Yes, you can just remove it. I'm not familiar with android build systems so I'm not sure if that will invalidate your codesigning. Otherwise, there should be some gradle command to strip it. I believe that if you remove the dynamic libraries that flutter will still function correctly, it should just print out an error message that it couldn't find the validation layers. If you find that isn't the case I'd file a bug for it.

github-actions bot added the platform-android label Aug 31, 2023

gaaclarke force-pushed the validation-layers-for-everyone branch 2 times, most recently from db335be to 639425d Compare August 31, 2023 22:43

[Impeller] turned on validations for all debug builds

602d4d6

gaaclarke force-pushed the validation-layers-for-everyone branch from 639425d to 602d4d6 Compare August 31, 2023 23:01

gaaclarke marked this pull request as ready for review August 31, 2023 23:05

gaaclarke requested review from zanderso, jonahwilliams and Hixie August 31, 2023 23:05

jonahwilliams approved these changes Aug 31, 2023

View reviewed changes

zanderso reviewed Aug 31, 2023

View reviewed changes

gaaclarke mentioned this pull request Sep 1, 2023

[Impeller] Make Vulkan validation on Android for end users self-serve. flutter/flutter#133794

Closed

zanderso mentioned this pull request Sep 1, 2023

[Impeller] turned on validations for all debug builds #45401

Closed

zanderso and others added 4 commits September 1, 2023 15:21

Build vulkan validation layers with api level 26, and bundle

ed15378

moved the logic to turn on the validation layers to gn

ba3544d

added log statement about using validation layers

8e1c91a

added log statement

27f5515

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 1, 2023

jonahwilliams approved these changes Sep 1, 2023

View reviewed changes

auto-submit bot merged commit b6224e2 into flutter:main Sep 1, 2023

zanderso reviewed Sep 2, 2023

View reviewed changes

engine-flutter-autoroll mentioned this pull request Sep 2, 2023

Roll Flutter Engine from 489c399e3dc6 to e496eec40e21 (26 revisions) flutter/flutter#133909

Closed

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

b6224e2c7 [Impeller] turned on validations for all debug builds (flut…

05f0c88

…ter/engine#45350)

engine-flutter-autoroll mentioned this pull request Sep 2, 2023

Roll Flutter Engine from 489c399e3dc6 to e496eec40e21 (26 revisions) flutter/flutter#133916

Closed

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

b6224e2c7 [Impeller] turned on validations for all debug builds (flut…

efe621c

…ter/engine#45350)

engine-flutter-autoroll mentioned this pull request Sep 2, 2023

Manual roll Flutter Engine from 489c399e3dc6 to e496eec40e21 (26 revisions) flutter/flutter#133924

Merged

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

b6224e2c7 [Impeller] turned on validations for all debug builds (flut…

1e7196d

…ter/engine#45350)

gaaclarke mentioned this pull request Sep 5, 2023

Remove android API 26 bump for validation layers #45468

Merged

8 tasks

auto-submit bot pushed a commit that referenced this pull request Sep 6, 2023

Remove android API 26 bump for validation layers (#45468)

f811f81

not necessary after: #45350 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

gnprice mentioned this pull request Sep 11, 2023

Window resizing is broken on windows desktop. flutter/flutter#134262

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] turned on validations for all debug builds #45350

[Impeller] turned on validations for all debug builds #45350

gaaclarke commented Aug 31, 2023 •

edited

Loading

jonahwilliams left a comment

flutter-dashboard bot commented Aug 31, 2023

gaaclarke commented Aug 31, 2023 •

edited

Loading

zanderso Aug 31, 2023

zanderso Aug 31, 2023

gaaclarke commented Sep 1, 2023

gaaclarke commented Sep 1, 2023

jonahwilliams commented Sep 1, 2023

zanderso commented Sep 1, 2023

gaaclarke commented Sep 1, 2023

gaaclarke commented Sep 1, 2023 •

edited

Loading

jonahwilliams left a comment

zanderso Sep 2, 2023

gaaclarke Sep 5, 2023

gbak commented Jul 19, 2024

gaaclarke commented Jul 23, 2024

[Impeller] turned on validations for all debug builds #45350

[Impeller] turned on validations for all debug builds #45350

Conversation

gaaclarke commented Aug 31, 2023 • edited Loading

Pre-launch Checklist

jonahwilliams left a comment

Choose a reason for hiding this comment

flutter-dashboard bot commented Aug 31, 2023

gaaclarke commented Aug 31, 2023 • edited Loading

zanderso Aug 31, 2023

Choose a reason for hiding this comment

zanderso Aug 31, 2023

Choose a reason for hiding this comment

gaaclarke commented Sep 1, 2023

gaaclarke commented Sep 1, 2023

jonahwilliams commented Sep 1, 2023

zanderso commented Sep 1, 2023

gaaclarke commented Sep 1, 2023

gaaclarke commented Sep 1, 2023 • edited Loading

jonahwilliams left a comment

Choose a reason for hiding this comment

zanderso Sep 2, 2023

Choose a reason for hiding this comment

gaaclarke Sep 5, 2023

Choose a reason for hiding this comment

gbak commented Jul 19, 2024

gaaclarke commented Jul 23, 2024

gaaclarke commented Aug 31, 2023 •

edited

Loading

gaaclarke commented Aug 31, 2023 •

edited

Loading

gaaclarke commented Sep 1, 2023 •

edited

Loading