Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions impeller/aiks/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: remove

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

"//third_party/vulkan_validation_layers:vulkan_gen_json_files",
]
if (defined(invoker.public_configs)) {
public_configs = invoker.public_configs
Expand Down
1 change: 1 addition & 0 deletions impeller/playground/playground_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ std::unique_ptr<PlaygroundImpl> PlaygroundImpl::Create(
#endif // IMPELLER_ENABLE_OPENGLES
#if IMPELLER_ENABLE_VULKAN
case PlaygroundBackend::kVulkan:
switches.enable_vulkan_validation = true;
return std::make_unique<PlaygroundImplVK>(switches);
#endif // IMPELLER_ENABLE_VULKAN
default:
Expand Down
2 changes: 1 addition & 1 deletion impeller/renderer/backend/vulkan/capabilities_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I'm keeping this fatal for testing.

So we have two approaches here:

  1. Keep the existing behavior but add checks that it is running as expected on CI.
  2. Make it fatal but do more work to make sure users don't fall down the path.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<< "Requested Impeller context creation with validations but the "
"validation layers could not be found. Expect no Vulkan validation "
"checks!";
Expand Down
4 changes: 2 additions & 2 deletions impeller/tools/impeller.gni
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ declare_args() {

# Whether the OpenGLES backend is enabled.
impeller_enable_opengles =
(is_linux || is_win || is_android) && target_os != "fuchsia"
(is_linux || is_win || is_android || is_mac) && target_os != "fuchsia"

# 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 ||

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets try it

enable_unittests) && target_os != "fuchsia"

# Whether playgrounds should run with Vulkan.
Expand Down
15 changes: 1 addition & 14 deletions tools/gn
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


if args.enable_impeller_opengles:
gn_args['impeller_enable_opengles'] = True

if args.prebuilt_impellerc is not None:
gn_args['impeller_use_prebuilt_impellerc'] = args.prebuilt_impellerc

Expand Down Expand Up @@ -1253,19 +1247,12 @@ def parse_args(args):

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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,
action='store_true',
help='Enable the Impeller OpenGL ES backend.'
)

parser.add_argument(
'--prebuilt-impellerc',
default=None,
Expand Down