[Impeller] Set 'enable_vulkan_validation_layers' if --unopt and Vulkan is enabled. #44055
Conversation
jonahwilliams
left a comment
There was a problem hiding this comment.
Unlike the glGetError, to see Vulkan validation errors I believe you'll need to opt in from the AndroidManifest. Maybe we should also update context_vk.cc to also automatically enabled vulkan validation in unopt modes?
chinmaygarde
left a comment
There was a problem hiding this comment.
As @jonahwilliams pointed out, if we request validation but the layers are absent, we will terminate context setup and eventually abort shell setup.
I think requiring validation in unopt modes without other guidance on how to setup validation layers puts an onerous burden on unopt engine tinkerers who might not care about Vulkan at all.
Perhaps we can relax the restriction to just log (loudly) that validations were requested but the layers weren't found but still continue with context setup? You can change the validation log to an error log there and not return nullopt.
Otherwise LGTM.
tools/gn
Outdated
|
|
||
| if args.enable_impeller_vulkan: | ||
| gn_args['impeller_enable_vulkan'] = True | ||
| # As of 2023-07-26, --unopt adds "glGetError" calls for the OpenGL backend, |
There was a problem hiding this comment.
I don't think this comment adds anything. For the context as of 07/26, we have git. Your call on whether to keep this though.
There was a problem hiding this comment.
Sounds good, removed!
|
What I mean is sort of the opposite though. It's fine to compile in validation layers, but they won't do anything without a separate step to opt in. Maybe we should make that second part automatic too? |
I believe validation layers can be added to the Vulkan loader from a separate APK too. I'm afraid having some Vulkan debugging related APK installed would enable validation on Flutter apps automatically. But perhaps I am overthinking it? Maybe we should just enable it for now and deal with it later. Having validations on with the least overhead possible is going to be super valuable in the short run. |
|
Sounds good, thanks all. I'm setting up a test phone so I can use AGI to make sure I'm doing this all correctly - I'll convert back from a draft PR when it's ready for review! |
|
Done and verified! Woot! 
|
…nd Vulkan is enabled. (flutter/engine#44055)
flutter/engine@dc8618d...cfa5427 2023-07-28 skia-flutter-autoroll@skia.org Roll Dart SDK from aab0233f76b7 to 1b85515a995b (1 revision) (flutter/engine#44096) 2023-07-28 matanlurey@users.noreply.github.com [Impeller] Set 'enable_vulkan_validation_layers' if --unopt and Vulkan is enabled. (flutter/engine#44055) 2023-07-28 skia-flutter-autoroll@skia.org Roll Skia from f2362d22abcf to 8739fd6d422c (1 revision) (flutter/engine#44095) 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 jonahwilliams@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
…1464) flutter/engine@dc8618d...cfa5427 2023-07-28 skia-flutter-autoroll@skia.org Roll Dart SDK from aab0233f76b7 to 1b85515a995b (1 revision) (flutter/engine#44096) 2023-07-28 matanlurey@users.noreply.github.com [Impeller] Set 'enable_vulkan_validation_layers' if --unopt and Vulkan is enabled. (flutter/engine#44055) 2023-07-28 skia-flutter-autoroll@skia.org Roll Skia from f2362d22abcf to 8739fd6d422c (1 revision) (flutter/engine#44095) 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 jonahwilliams@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
…1464) flutter/engine@dc8618d...cfa5427 2023-07-28 skia-flutter-autoroll@skia.org Roll Dart SDK from aab0233f76b7 to 1b85515a995b (1 revision) (flutter/engine#44096) 2023-07-28 matanlurey@users.noreply.github.com [Impeller] Set 'enable_vulkan_validation_layers' if --unopt and Vulkan is enabled. (flutter/engine#44055) 2023-07-28 skia-flutter-autoroll@skia.org Roll Skia from f2362d22abcf to 8739fd6d422c (1 revision) (flutter/engine#44095) 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 jonahwilliams@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
…n is enabled. (flutter#44055) Context: > @matanlurey: > Would it make sense for `--unopt` to imply `--enable-vulkan-validation-layers` if `--enable-vulkan` is set? > The reason I ask is because it does seem to imply `glGetError` checks, which (I could be wrong) is roughly similar? > @chinmaygarde: > Makes sense. ... so uh, here it is (let me know if we prefer anything different).
Context:
... so uh, here it is (let me know if we prefer anything different).