Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Added helper messaging around validation usage #42708

Merged
auto-submit merged 6 commits into from
Jun 9, 2023
Merged

[Impeller] Added helper messaging around validation usage #42708

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1c6a055
[Impeller] Added helper messaging around validation usage
gaaclarke Jun 9, 2023
1afb9f4
dan feedback
gaaclarke Jun 9, 2023
384fef1
reverted accidental commit
gaaclarke Jun 9, 2023
859ae2c
dan feedback
gaaclarke Jun 9, 2023
1bdec1e
switched to a defined check
gaaclarke Jun 9, 2023
205d48f
switched to FML_LOG(ERROR)
gaaclarke Jun 9, 2023
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
5 changes: 5 additions & 0 deletions impeller/renderer/backend/vulkan/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.

import("//flutter/vulkan/config.gni")
import("../../../tools/impeller.gni")

impeller_component("vulkan_unittests") {
Expand Down Expand Up @@ -96,4 +97,8 @@ impeller_component("vulkan") {
"//third_party/vulkan-deps/vulkan-headers/src:vulkan_headers",
"//third_party/vulkan_memory_allocator",
]

if (enable_vulkan_validation_layers) {
defines = [ "IMPELLER_ENABLE_VULKAN_VALIDATION_LAYERS" ]
}
}
11 changes: 11 additions & 0 deletions impeller/renderer/backend/vulkan/context_vk.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,17 @@ void ContextVK::Setup(Settings settings) {

vk::InstanceCreateInfo instance_info;
if (caps->AreValidationsEnabled()) {
std::stringstream ss;
ss << "Enabling validation layers, features: [";
for (const auto& validation : enabled_validations) {
ss << vk::to_string(validation) << " ";
}
ss << "]";
FML_DLOG(ERROR) << ss.str();
#if !IMPELLER_ENABLE_VULKAN_VALIDATION_LAYERS && FML_OS_ANDROID
VALIDATION_LOG << "Attempting to use validation layers without "
Copy link
Contributor

@dnfield dnfield Jun 9, 2023

Choose a reason for hiding this comment

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

I don't think VALIDATION_LOG is right here - unless I'm misunderstanding and it's an error to end up in this state (although it seems like there are multiple ways now to turn on validation and I'm not clear on which path(s) are allowed or not).

Copy link
Member Author

@gaaclarke gaaclarke Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, it's an error. We want people to be using the validation layers that we are building. There is no reason to turn on validation layers without building and packaging up validation layers. Jonah ran into this the other day where he thought just turning on the runtime flag was sufficient then had to go through the rigamarole to package prebuilt layers.

Copy link
Contributor

@dnfield dnfield Jun 9, 2023

Choose a reason for hiding this comment

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

Perhaps make the message a bit more strongly worded, e.g.

"Vulkan validation layers found but command line argument --enable-vulkan-validation missing. Rerun with --enable-vulkan-validation"

Copy link
Member Author

@gaaclarke gaaclarke Jun 9, 2023

Choose a reason for hiding this comment

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

I updated it. I made it a bit stronger and also made sure to mention gn (as opposed to runtime arguments).

Copy link
Member Author

@gaaclarke gaaclarke Jun 9, 2023

Choose a reason for hiding this comment

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

(sorry, uploaded now, i accidentally committed the local hack i need to make scenario_app work on my machine instead)

Copy link
Contributor

@jonahwilliams jonahwilliams Jun 9, 2023

Choose a reason for hiding this comment

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

VALIDATION_LOG isn't fatal by default and won't show up in logcat unless fatal validations are enabled

Copy link
Member Author

@gaaclarke gaaclarke Jun 9, 2023

Choose a reason for hiding this comment

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

So, this will crash in a way that is hard to know what is going on if the runtime flag is on, but it wasn't built to support it. Sounds like we want an FML_LOG(ERROR) instead.

Copy link
Member Author

@gaaclarke gaaclarke Jun 9, 2023

Choose a reason for hiding this comment

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

Switched both things to FML_LOG(ERROR). There is no need to use DLOG since using validation layers should never happen in the wild anyways. ERROR since we know it will probably crash.

"--enable-vulkan-validation-layers";
#endif
instance_info.pNext = &validation;
}
instance_info.setPEnabledLayerNames(enabled_layers_c);
Expand Down