-
Notifications
You must be signed in to change notification settings - Fork 6k
[Impeller] attempt to get validation errors from CI unittests. #51341
Changes from 23 commits
f86860c
e06c9c9
dcd1c5e
29c7edc
408e7f5
be3271b
43e202a
1c05893
1cecd26
d72221b
fd68de3
a453f7f
ab9a072
26dc17f
4e2eeac
2cf6407
79231ec
9e378b1
b8bcfcb
f3ee710
eea1bce
7917811
fb6e3ee
b7031ba
a38397f
de0fa35
2d6fdfb
c7f4b80
dfa2b37
dfd121f
a8f9e41
1a2b88d
7b6514c
bf9d430
50fcdde
412912b
2cec37c
34ce5b6
c53ff77
50cfa9d
3d105f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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_" // | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: turn off clang format so these are all on one line
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| "RenderCoverageMatchesGetCoverageRotated_Vulkan", // | ||
| "impeller_Play_SceneTest_FlutterLogo_Vulkan", // | ||
| "impeller_Play_SceneTest_CuboidUnlit_Vulkan", // | ||
| "impeller_Play_GaussianBlurFilterContentsTest_" | ||
| "TextureContentsWithEffectTransform_Vulkan", // | ||
| "impeller_Play_GaussianBlurFilterContentsTest_" | ||
| "RenderCoverageMatchesGetCoverageTranslate_Vulkan", // | ||
| "impeller_Play_GaussianBlurFilterContentsTest_" | ||
| "RenderCoverageMatchesGetCoverageRotated_Vulkan", // | ||
| "impeller_Play_GaussianBlurFilterContentsTest_" | ||
| "TextureContentsWithDestinationRect_Vulkan", // | ||
| "impeller_Play_GaussianBlurFilterContentsTest_" | ||
| "RenderCoverageMatchesGetCoverage_Vulkan", // | ||
| "impeller_Play_GaussianBlurFilterContentsTest_" | ||
| "TextureContentsWithDestinationRectScaled_Vulkan", // | ||
| "impeller_Play_RuntimeStageTest_CanCreatePipelineFromRuntimeStage_" | ||
| "Vulkan", // | ||
| }; | ||
|
|
||
| namespace { | ||
| std::string GetTestName() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,16 +60,7 @@ void PlaygroundImplVK::DestroyWindowHandle(WindowHandle handle) { | |
|
|
||
| PlaygroundImplVK::PlaygroundImplVK(PlaygroundSwitches switches) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally this would be a static function that returns https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can just lift the check out of the constructor, since its a static method. I made this an FML_CHECK
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| : PlaygroundImpl(switches), handle_(nullptr, &DestroyWindowHandle) { | ||
| if (!::glfwVulkanSupported()) { | ||
| #ifdef TARGET_OS_MAC | ||
| VALIDATION_LOG << "Attempted to initialize a Vulkan playground on macOS " | ||
| "where Vulkan cannot be found. It can be installed via " | ||
| "MoltenVK and make sure to install it globally so " | ||
| "dlopen can find it."; | ||
| #else | ||
| VALIDATION_LOG << "Attempted to initialize a Vulkan playground on a system " | ||
| "that does not support Vulkan."; | ||
| #endif | ||
| if (!IsVulkanDriverPresent()) { | ||
| return; | ||
| } | ||
|
|
||
|
|
@@ -224,4 +215,20 @@ fml::Status PlaygroundImplVK::SetCapabilities( | |
| "PlaygroundImplVK doesn't support setting the capabilities."); | ||
| } | ||
|
|
||
| bool PlaygroundImplVK::IsVulkanDriverPresent() { | ||
| if (::glfwVulkanSupported()) { | ||
| return true; | ||
| } | ||
| #ifdef TARGET_OS_MAC | ||
| FML_LOG(ERROR) << "Attempting to initialize a Vulkan playground on macOS " | ||
| "where Vulkan cannot be found. It can be installed via " | ||
| "MoltenVK and make sure to install it globally so " | ||
| "dlopen can find it."; | ||
| #else // TARGET_OS_MAC | ||
| FML_LOG(ERROR) << "Attempting to initialize a Vulkan playground on a system " | ||
| "that does not support Vulkan."; | ||
| #endif // TARGET_OS_MAC | ||
| return false; | ||
| } | ||
|
|
||
| } // namespace impeller | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
| // found in the LICENSE file. | ||
|
|
||
| #include "impeller/playground/playground_impl.h" | ||
| #include "flutter/testing/testing.h" | ||
|
|
||
| #define GLFW_INCLUDE_NONE | ||
| #include "third_party/glfw/include/GLFW/glfw3.h" | ||
|
|
@@ -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 = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's share this instead of duplicating it twice.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| "impeller_Play_GaussianBlurFilterContentsTest_" // | ||
| "RenderCoverageMatchesGetCoverageRotated_Vulkan", // | ||
| "impeller_Play_SceneTest_FlutterLogo_Vulkan", // | ||
| "impeller_Play_SceneTest_CuboidUnlit_Vulkan", // | ||
| "impeller_Play_GaussianBlurFilterContentsTest_" | ||
| "TextureContentsWithEffectTransform_Vulkan", // | ||
| "impeller_Play_GaussianBlurFilterContentsTest_" | ||
| "RenderCoverageMatchesGetCoverageTranslate_Vulkan", // | ||
| "impeller_Play_GaussianBlurFilterContentsTest_" | ||
| "RenderCoverageMatchesGetCoverageRotated_Vulkan", // | ||
| "impeller_Play_GaussianBlurFilterContentsTest_" | ||
| "TextureContentsWithDestinationRect_Vulkan", // | ||
| "impeller_Play_GaussianBlurFilterContentsTest_" | ||
| "RenderCoverageMatchesGetCoverage_Vulkan", // | ||
| "impeller_Play_GaussianBlurFilterContentsTest_" | ||
| "TextureContentsWithDestinationRectScaled_Vulkan" // | ||
| "impeller_Play_RuntimeStageTest_CanCreatePipelineFromRuntimeStage_" | ||
| "Vulkan", // | ||
| }; | ||
|
|
||
| std::string GetTestName() { | ||
| std::string suite_name = | ||
| ::testing::UnitTest::GetInstance()->current_test_suite()->name(); | ||
| std::string test_name = | ||
| ::testing::UnitTest::GetInstance()->current_test_info()->name(); | ||
| std::stringstream ss; | ||
| ss << "impeller_" << suite_name << "_" << test_name; | ||
| std::string result = ss.str(); | ||
| // Make sure there are no slashes in the test name. | ||
| std::replace(result.begin(), result.end(), '/', '_'); | ||
| return result; | ||
| } | ||
|
|
||
| bool ShouldTestHaveVulkanValidations() { | ||
| std::string test_name = GetTestName(); | ||
| FML_LOG(ERROR) << "Checking: " << test_name; | ||
| return std::find(kVulkanDenyValidationTests.begin(), | ||
| kVulkanDenyValidationTests.end(), | ||
| test_name) == kVulkanDenyValidationTests.end(); | ||
| } | ||
| } // namespace | ||
|
|
||
| namespace impeller { | ||
|
|
||
| std::unique_ptr<PlaygroundImpl> PlaygroundImpl::Create( | ||
|
|
@@ -35,6 +80,7 @@ std::unique_ptr<PlaygroundImpl> PlaygroundImpl::Create( | |
| #endif // IMPELLER_ENABLE_OPENGLES | ||
| #if IMPELLER_ENABLE_VULKAN | ||
| case PlaygroundBackend::kVulkan: | ||
| switches.enable_vulkan_validation = ShouldTestHaveVulkanValidations(); | ||
| return std::make_unique<PlaygroundImplVK>(switches); | ||
| #endif // IMPELLER_ENABLE_VULKAN | ||
| default: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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!"; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,19 +19,12 @@ 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 || | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this one have
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
| # | ||
| # impeller_enable_vulkan may be true in build environments that run tests but | ||
| # do not have a Vulkan ICD present. | ||
| impeller_enable_vulkan_playgrounds = | ||
| (is_linux || is_win || is_android) && target_os != "fuchsia" | ||
|
|
||
| # Whether to use a prebuilt impellerc. | ||
| # If this is the empty string, impellerc will be built. | ||
| # If it is non-empty, it should be the absolute path to impellerc. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an issue for a disabled test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not disabling it here anymore, i filled one generic skip test issue