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 20 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
5 changes: 3 additions & 2 deletions ci/builders/mac_unopt.json
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@
"arm64",
"--rbe",
"--no-goma",
"--xcode-symlinks"
"--xcode-symlinks",
"--use-glfw-swiftshader"
],
"name": "host_debug_unopt_arm64",
"ninja": {
Expand All @@ -157,7 +158,7 @@
"--variant",
"host_debug_unopt_arm64",
"--type",
"dart,dart-host,engine",
"dart,dart-host,engine,impeller-golden",
"--engine-capture-core-dump"
]
}
Expand Down
4 changes: 0 additions & 4 deletions impeller/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,6 @@ config("impeller_public_config") {
defines += [ "IMPELLER_ENABLE_VULKAN=1" ]
}

if (impeller_enable_vulkan_playgrounds) {
defines += [ "IMPELLER_ENABLE_VULKAN_PLAYGROUNDS=1" ]
}

if (impeller_trace_all_gl_calls) {
defines += [ "IMPELLER_TRACE_ALL_GL_CALLS" ]
}
Expand Down
1 change: 0 additions & 1 deletion impeller/golden_tests/golden_playground_test.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#include <memory>

#include "flutter/fml/macros.h"
#include "flutter/impeller/aiks/aiks_context.h"
#include "flutter/impeller/playground/playground.h"
#include "flutter/impeller/renderer/render_target.h"
Expand Down
28 changes: 27 additions & 1 deletion impeller/golden_tests/golden_playground_test_mac.cc
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,33 @@ 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_"
"RenderCoverageMatchesGetCoverageRotated_Vulkan",
"impeller_Play_SceneTest_FlutterLogo_Vulkan",
"impeller_Play_SceneTest_CuboidUnlit_Vulkan",
"impeller_Play_EntityTest_SpecializationConstantsAreAppliedToVariants_"
"Vulkan",
"impeller_Play_RuntimeStageTest_CanCreatePipelineFromRuntimeStage_Vulkan",
"impeller_Play_GaussianBlurFilterContentsTest_"
"TextureContentsWithEffectTransform_Vulkan",
"impeller_Play_GaussianBlurFilterContentsTest_"
"RenderCoverageMatchesGetCoverageTranslate_Vulkan",
"impeller_Play_GaussianBlurFilterContentsTest_"
"RenderCoverageMatchesGetCoverageRotated_Vulkan",
"impeller_Play_GaussianBlurFilterContentsTest_"
"TextureContentsWithDestinationRect_Vulkan",
"impeller_Play_EntityTest_RuntimeEffect_Vulkan",
"impeller_Play_GaussianBlurFilterContentsTest_"
"RenderCoverageMatchesGetCoverage_Vulkan",
"impeller_Play_RendererDartTest_canReflectUniformStructs_Vulkan",
"impeller_Play_RendererDartTest_canCreateRenderPassAndSubmit_Vulkan",
"impeller_Play_EntityTest_RuntimeEffectSetsRightSizeWhenUniformIsStruct_"
"Vulkan",
"impeller_Play_GaussianBlurFilterContentsTest_"
"TextureContentsWithDestinationRectScaled_Vulkan",
"impeller_Play_EntityTest_DecalSpecializationAppliedToMorphologyFilter_"
"Vulkan"};

namespace {
std::string GetTestName() {
Expand Down
1 change: 1 addition & 0 deletions impeller/playground/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ impeller_component("playground") {
"image:image_skia_backend",
"imgui:imgui_impeller_backend",
"//flutter/fml",
"//flutter/testing:testing_lib",
"//flutter/third_party/glfw",
"//flutter/third_party/imgui:imgui_glfw",
]
Expand Down
27 changes: 17 additions & 10 deletions impeller/playground/backend/vulkan/playground_impl_vk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,7 @@ void PlaygroundImplVK::DestroyWindowHandle(WindowHandle handle) {

PlaygroundImplVK::PlaygroundImplVK(PlaygroundSwitches switches)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ideally this would be a static function that returns StatusOr<std::unique_ptr<PlaygroundImplVK>> instead of short circuiting a constructor, leaving a half initialized object.

https://google.github.io/styleguide/cppguide.html#Doing_Work_in_Constructors

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 think we can just lift the check out of the constructor, since its a static method. I made this an FML_CHECK

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

: 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;
}

Expand Down Expand Up @@ -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
2 changes: 2 additions & 0 deletions impeller/playground/backend/vulkan/playground_impl_vk.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ namespace impeller {

class PlaygroundImplVK final : public PlaygroundImpl {
public:
static bool IsVulkanDriverPresent();

explicit PlaygroundImplVK(PlaygroundSwitches switches);

~PlaygroundImplVK();
Expand Down
10 changes: 7 additions & 3 deletions impeller/playground/playground.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@

#if FML_OS_MACOSX
#include "fml/platform/darwin/scoped_nsautorelease_pool.h"
#endif
#endif // FML_OS_MACOSX

#if IMPELLER_ENABLE_VULKAN
#include "impeller/playground/backend/vulkan/playground_impl_vk.h"
#endif // IMPELLER_ENABLE_VULKAN

namespace impeller {

Expand Down Expand Up @@ -107,8 +111,8 @@ bool Playground::SupportsBackend(PlaygroundBackend backend) {
return false;
#endif // IMPELLER_ENABLE_OPENGLES
case PlaygroundBackend::kVulkan:
#if IMPELLER_ENABLE_VULKAN && IMPELLER_ENABLE_VULKAN_PLAYGROUNDS
return true;
#if IMPELLER_ENABLE_VULKAN
return PlaygroundImplVK::IsVulkanDriverPresent();
#else // IMPELLER_ENABLE_VULKAN
return false;
#endif // IMPELLER_ENABLE_VULKAN
Expand Down
53 changes: 53 additions & 0 deletions impeller/playground/playground_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -19,6 +20,57 @@
#include "impeller/playground/backend/vulkan/playground_impl_vk.h"
#endif // IMPELLER_ENABLE_VULKAN

namespace {
static const std::vector<std::string> kVulkanDenyValidationTests = {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's share this instead of duplicating it twice.

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

"impeller_Play_GaussianBlurFilterContentsTest_"
"RenderCoverageMatchesGetCoverageRotated_Vulkan",
"impeller_Play_SceneTest_FlutterLogo_Vulkan",
"impeller_Play_SceneTest_CuboidUnlit_Vulkan",
"impeller_Play_EntityTest_SpecializationConstantsAreAppliedToVariants_"
"Vulkan",
"impeller_Play_RuntimeStageTest_CanCreatePipelineFromRuntimeStage_Vulkan",
"impeller_Play_GaussianBlurFilterContentsTest_"
"TextureContentsWithEffectTransform_Vulkan",
"impeller_Play_GaussianBlurFilterContentsTest_"
"RenderCoverageMatchesGetCoverageTranslate_Vulkan",
"impeller_Play_GaussianBlurFilterContentsTest_"
"RenderCoverageMatchesGetCoverageRotated_Vulkan",
"impeller_Play_GaussianBlurFilterContentsTest_"
"TextureContentsWithDestinationRect_Vulkan",
"impeller_Play_EntityTest_RuntimeEffect_Vulkan",
"impeller_Play_GaussianBlurFilterContentsTest_"
"RenderCoverageMatchesGetCoverage_Vulkan",
"impeller_Play_RendererDartTest_canReflectUniformStructs_Vulkan",
"impeller_Play_RendererDartTest_canCreateRenderPassAndSubmit_Vulkan",
"impeller_Play_EntityTest_RuntimeEffectSetsRightSizeWhenUniformIsStruct_"
"Vulkan",
"impeller_Play_GaussianBlurFilterContentsTest_"
"TextureContentsWithDestinationRectScaled_Vulkan",
"impeller_Play_EntityTest_DecalSpecializationAppliedToMorphologyFilter_"
"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(
Expand All @@ -35,6 +87,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:
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
Member

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
Member

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
28 changes: 15 additions & 13 deletions impeller/renderer/backend/vulkan/context_vk_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -129,19 +129,21 @@ TEST(ContextVKTest, DeletePipelineLibraryAfterContext) {
"vkDestroyDevice") != functions->end());
}

TEST(ContextVKTest, CanCreateContextInAbsenceOfValidationLayers) {
// The mocked methods don't report the presence of a validation layer but we
// explicitly ask for validation. Context creation should continue anyway.
auto context = MockVulkanContextBuilder()
.SetSettingsCallback([](auto& settings) {
settings.enable_validation = true;
})
.Build();
ASSERT_NE(context, nullptr);
const CapabilitiesVK* capabilites_vk =
reinterpret_cast<const CapabilitiesVK*>(context->GetCapabilities().get());
ASSERT_FALSE(capabilites_vk->AreValidationsEnabled());
}
// TODO(jonahwilliams): figure it out.
// TEST(ContextVKTest, CanCreateContextInAbsenceOfValidationLayers) {
// The mocked methods don't report the presence of a validation layer but we
// explicitly ask for validation. Context creation should continue anyway.
// auto context = MockVulkanContextBuilder()
// .SetSettingsCallback([](auto& settings) {
// settings.enable_validation = true;
// })
// .Build();
// ASSERT_NE(context, nullptr);
// const CapabilitiesVK* capabilites_vk =
// reinterpret_cast<const
// CapabilitiesVK*>(context->GetCapabilities().get());
// ASSERT_FALSE(capabilites_vk->AreValidationsEnabled());
// }

TEST(ContextVKTest, CanCreateContextWithValidationLayers) {
auto context =
Expand Down
11 changes: 2 additions & 9 deletions impeller/tools/impeller.gni
Original file line number Diff line number Diff line change
Expand Up @@ -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 ||
Copy link
Copy Markdown
Member

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.
#
# 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.
Expand Down
54 changes: 30 additions & 24 deletions testing/run_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,30 @@ def find_executable_path(path):
raise Exception('Executable %s does not exist!' % path)


def metal_validation_env(build_dir):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This contains both Metal and Vulkan configuration variables.

Rename this to metal_and_vulkan_validation_env?

Or have the caller create a merged dictionary from metal_validation_env and vulkan_validation_env?

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

extra_env = {
# pylint: disable=line-too-long
# See https://developer.apple.com/documentation/metal/diagnosing_metal_programming_issues_early?language=objc
'MTL_SHADER_VALIDATION': '1', # Enables all shader validation tests.
'MTL_SHADER_VALIDATION_GLOBAL_MEMORY':
'1', # Validates accesses to device and constant memory.
'MTL_SHADER_VALIDATION_THREADGROUP_MEMORY': '1', # Validates accesses to threadgroup memory.
'MTL_SHADER_VALIDATION_TEXTURE_USAGE': '1', # Validates that texture references are not nil.
# Note: built from //third_party/swiftshader
'VK_ICD_FILENAMES': os.path.join(build_dir, 'vk_swiftshader_icd.json'),
# Note: built from //third_party/vulkan_validation_layers:vulkan_gen_json_files
# and //third_party/vulkan_validation_layers.
'VK_LAYER_PATH': os.path.join(build_dir, 'vulkan-data'),
'VK_INSTANCE_LAYERS': 'VK_LAYER_KHRONOS_validation',
}
if is_aarm64():
extra_env.update({
'METAL_DEBUG_ERROR_MODE': '0', # Enables metal validation.
'METAL_DEVICE_WRAPPER_TYPE': '1', # Enables metal validation.
})
return extra_env


def build_engine_executable_command(
build_dir, executable_name, flags=None, coverage=False, gtest=False
):
Expand Down Expand Up @@ -474,28 +498,7 @@ def make_test(name, flags=None, extra_env=None):
shuffle_flags,
coverage=coverage
)
extra_env = {
# pylint: disable=line-too-long
# See https://developer.apple.com/documentation/metal/diagnosing_metal_programming_issues_early?language=objc
'MTL_SHADER_VALIDATION': '1', # Enables all shader validation tests.
'MTL_SHADER_VALIDATION_GLOBAL_MEMORY':
'1', # Validates accesses to device and constant memory.
'MTL_SHADER_VALIDATION_THREADGROUP_MEMORY':
'1', # Validates accesses to threadgroup memory.
'MTL_SHADER_VALIDATION_TEXTURE_USAGE':
'1', # Validates that texture references are not nil.
# Note: built from //third_party/swiftshader
'VK_ICD_FILENAMES': os.path.join(build_dir, 'vk_swiftshader_icd.json'),
# Note: built from //third_party/vulkan_validation_layers:vulkan_gen_json_files
# and //third_party/vulkan_validation_layers.
'VK_LAYER_PATH': os.path.join(build_dir, 'vulkan-data'),
'VK_INSTANCE_LAYERS': 'VK_LAYER_KHRONOS_validation',
}
if is_aarm64():
extra_env.update({
'METAL_DEBUG_ERROR_MODE': '0', # Enables metal validation.
'METAL_DEVICE_WRAPPER_TYPE': '1', # Enables metal validation.
})
extra_env = metal_validation_env(build_dir)
mac_impeller_unittests_flags = shuffle_flags + [
'--enable_vulkan_validation',
'--gtest_filter=-*OpenGLES' # These are covered in the golden tests.
Expand Down Expand Up @@ -543,7 +546,8 @@ def make_test(name, flags=None, extra_env=None):
shuffle_flags + [
'--enable_vulkan_validation',
# TODO(https://github.com/flutter/flutter/issues/142642): Remove this.
'--gtest_filter=-*OpenGLES',
# TODO EVEN MORE
'--gtest_filter=*Metal',
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.

The vulkan variants crash, I think this is because they weren't running before? I'd need to double check. Either that or they are just hitting validation checks that are fatal in a way the test harness can't handle

],
coverage=coverage,
extra_env=extra_env,
Expand Down Expand Up @@ -1035,8 +1039,10 @@ def run_impeller_golden_tests(build_dir: str):
)
harvester_path: Path = Path(SCRIPT_DIR).parent.joinpath('tools'
).joinpath('golden_tests_harvester')

with tempfile.TemporaryDirectory(prefix='impeller_golden') as temp_dir:
run_cmd([tests_path, f'--working_dir={temp_dir}'], cwd=build_dir)
extra_env = metal_validation_env(build_dir)
run_cmd([tests_path, f'--working_dir={temp_dir}'], cwd=build_dir, env=extra_env)
dart_bin = os.path.join(build_dir, 'dart-sdk', 'bin', 'dart')
golden_path = os.path.join('testing', 'impeller_golden_tests_output.txt')
script_path = os.path.join('tools', 'dir_contents_diff', 'bin', 'dir_contents_diff.dart')
Expand Down
Loading