From 2ff38d22fc8eeea8c79b0ed5bca95c64a25dcb1e Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 09:42:51 -0700 Subject: [PATCH 01/10] [Impeller] transitioned mock vulkan context to the builder pattern --- .../vulkan/blit_command_vk_unittests.cc | 8 ++--- .../vulkan/command_encoder_vk_unittests.cc | 4 +-- .../backend/vulkan/context_vk_unittests.cc | 15 ++++---- .../vulkan/pass_bindings_cache_unittests.cc | 8 ++--- .../backend/vulkan/test/mock_vulkan.cc | 7 ++-- .../backend/vulkan/test/mock_vulkan.h | 34 ++++++++++++------- .../vulkan/test/mock_vulkan_unittests.cc | 2 +- 7 files changed, 45 insertions(+), 33 deletions(-) diff --git a/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc b/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc index 44206c2e7ddb5..e1172651353b7 100644 --- a/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/blit_command_vk_unittests.cc @@ -11,7 +11,7 @@ namespace impeller { namespace testing { TEST(BlitCommandVkTest, BlitCopyTextureToTextureCommandVK) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); auto pool = CommandPoolVK::GetThreadLocal(context.get()); auto encoder = std::make_unique(context)->Create(); BlitCopyTextureToTextureCommandVK cmd; @@ -28,7 +28,7 @@ TEST(BlitCommandVkTest, BlitCopyTextureToTextureCommandVK) { } TEST(BlitCommandVkTest, BlitCopyTextureToBufferCommandVK) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); auto encoder = std::make_unique(context)->Create(); BlitCopyTextureToBufferCommandVK cmd; cmd.source = context->GetResourceAllocator()->CreateTexture({ @@ -44,7 +44,7 @@ TEST(BlitCommandVkTest, BlitCopyTextureToBufferCommandVK) { } TEST(BlitCommandVkTest, BlitCopyBufferToTextureCommandVK) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); auto encoder = std::make_unique(context)->Create(); BlitCopyBufferToTextureCommandVK cmd; cmd.destination = context->GetResourceAllocator()->CreateTexture({ @@ -62,7 +62,7 @@ TEST(BlitCommandVkTest, BlitCopyBufferToTextureCommandVK) { } TEST(BlitCommandVkTest, BlitGenerateMipmapCommandVK) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); auto encoder = std::make_unique(context)->Create(); BlitGenerateMipmapCommandVK cmd; cmd.texture = context->GetResourceAllocator()->CreateTexture({ diff --git a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc index 0fa38fdb82985..2314796f54415 100644 --- a/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/command_encoder_vk_unittests.cc @@ -18,7 +18,7 @@ TEST(CommandEncoderVKTest, DeleteEncoderAfterThreadDies) { // command buffers before it cleans up its command pool. std::shared_ptr> called_functions; { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); called_functions = GetMockVulkanFunctions(context->GetDevice()); std::shared_ptr encoder; std::thread thread([&] { @@ -46,7 +46,7 @@ TEST(CommandEncoderVKTest, CleanupAfterSubmit) { { fml::AutoResetWaitableEvent wait_for_submit; fml::AutoResetWaitableEvent wait_for_thread_join; - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); std::thread thread([&] { CommandEncoderFactoryVK factory(context); std::shared_ptr encoder = factory.Create(); diff --git a/impeller/renderer/backend/vulkan/context_vk_unittests.cc b/impeller/renderer/backend/vulkan/context_vk_unittests.cc index 8a577be6d79bc..48ae3a08be1d4 100644 --- a/impeller/renderer/backend/vulkan/context_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/context_vk_unittests.cc @@ -14,7 +14,7 @@ TEST(ContextVKTest, DeletesCommandPools) { std::weak_ptr weak_context; std::weak_ptr weak_pool; { - std::shared_ptr context = CreateMockVulkanContext(); + std::shared_ptr context = MockVulkanContextBuilder().Build(); std::shared_ptr pool = CommandPoolVK::GetThreadLocal(context.get()); weak_pool = pool; @@ -30,7 +30,7 @@ TEST(ContextVKTest, DeletePipelineAfterContext) { std::shared_ptr> pipeline; std::shared_ptr> functions; { - std::shared_ptr context = CreateMockVulkanContext(); + std::shared_ptr context = MockVulkanContextBuilder().Build(); PipelineDescriptor pipeline_desc; pipeline_desc.SetVertexDescriptor(std::make_shared()); PipelineFuture pipeline_future = @@ -49,7 +49,7 @@ TEST(ContextVKTest, DeleteShaderFunctionAfterContext) { std::shared_ptr shader_function; std::shared_ptr> functions; { - std::shared_ptr context = CreateMockVulkanContext(); + std::shared_ptr context = MockVulkanContextBuilder().Build(); PipelineDescriptor pipeline_desc; pipeline_desc.SetVertexDescriptor(std::make_shared()); std::vector data = {0x03, 0x02, 0x23, 0x07}; @@ -71,7 +71,7 @@ TEST(ContextVKTest, DeletePipelineLibraryAfterContext) { std::shared_ptr pipeline_library; std::shared_ptr> functions; { - std::shared_ptr context = CreateMockVulkanContext(); + std::shared_ptr context = MockVulkanContextBuilder().Build(); PipelineDescriptor pipeline_desc; pipeline_desc.SetVertexDescriptor(std::make_shared()); pipeline_library = context->GetPipelineLibrary(); @@ -86,8 +86,11 @@ TEST(ContextVKTest, DeletePipelineLibraryAfterContext) { 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 = CreateMockVulkanContext( - [](auto& settings) { settings.enable_validation = true; }); + auto context = MockVulkanContextBuilder() + .SetSettingsCallback([](auto& settings) { + settings.enable_validation = true; + }) + .Build(); ASSERT_NE(context, nullptr); } diff --git a/impeller/renderer/backend/vulkan/pass_bindings_cache_unittests.cc b/impeller/renderer/backend/vulkan/pass_bindings_cache_unittests.cc index bab2a25db7f35..e76213c200493 100644 --- a/impeller/renderer/backend/vulkan/pass_bindings_cache_unittests.cc +++ b/impeller/renderer/backend/vulkan/pass_bindings_cache_unittests.cc @@ -24,7 +24,7 @@ int32_t CountStringViewInstances(const std::vector& strings, } // namespace TEST(PassBindingsCacheTest, bindPipeline) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); PassBindingsCache cache; auto encoder = std::make_unique(context)->Create(); auto buffer = encoder->GetCommandBuffer(); @@ -38,7 +38,7 @@ TEST(PassBindingsCacheTest, bindPipeline) { } TEST(PassBindingsCacheTest, setStencilReference) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); PassBindingsCache cache; auto encoder = std::make_unique(context)->Create(); auto buffer = encoder->GetCommandBuffer(); @@ -53,7 +53,7 @@ TEST(PassBindingsCacheTest, setStencilReference) { } TEST(PassBindingsCacheTest, setScissor) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); PassBindingsCache cache; auto encoder = std::make_unique(context)->Create(); auto buffer = encoder->GetCommandBuffer(); @@ -66,7 +66,7 @@ TEST(PassBindingsCacheTest, setScissor) { } TEST(PassBindingsCacheTest, setViewport) { - auto context = CreateMockVulkanContext(); + auto context = MockVulkanContextBuilder().Build(); PassBindingsCache cache; auto encoder = std::make_unique(context)->Create(); auto buffer = encoder->GetCommandBuffer(); diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index aafb3cf410153..726ecf304abf9 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -513,13 +513,12 @@ PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, } // namespace -std::shared_ptr CreateMockVulkanContext( - const std::function& settings_callback) { +std::shared_ptr MockVulkanContextBuilder::Build() { auto message_loop = fml::ConcurrentMessageLoop::Create(); ContextVK::Settings settings; settings.proc_address_callback = GetMockVulkanProcAddress; - if (settings_callback) { - settings_callback(settings); + if (settings_callback_) { + settings_callback_(settings); } return ContextVK::Create(std::move(settings)); } diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.h b/impeller/renderer/backend/vulkan/test/mock_vulkan.h index 2984db473ff09..fababd3997f26 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.h +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.h @@ -16,18 +16,28 @@ namespace testing { std::shared_ptr> GetMockVulkanFunctions( VkDevice device); -//------------------------------------------------------------------------------ -/// @brief Create a Vulkan context with Vulkan functions mocked. The caller -/// is given a chance to tinker on the settings right before a -/// context is created. -/// -/// @param[in] settings_callback The settings callback -/// -/// @return A context if one can be created. -/// -std::shared_ptr CreateMockVulkanContext( - const std::function& settings_callback = - nullptr); +class MockVulkanContextBuilder { + public: + //------------------------------------------------------------------------------ + /// @brief Create a Vulkan context with Vulkan functions mocked. The + /// caller is given a chance to tinker on the settings right + /// before a context is created. + /// + /// @return A context if one can be created. + /// + std::shared_ptr Build(); + + /// A callback that allows the modification of the ContextVK::Settings before + /// the context is made. + MockVulkanContextBuilder& SetSettingsCallback( + const std::function& settings_callback) { + settings_callback_ = settings_callback; + return *this; + } + + private: + std::function settings_callback_; +}; } // namespace testing } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc index 05cc7682e004d..de28491e69fed 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc @@ -14,7 +14,7 @@ TEST(MockVulkanContextTest, IsThreadSafe) { // In a typical app, there is a single ContextVK per app, shared b/w threads. // // This test ensures that the (mock) ContextVK is thread-safe. - auto const context = CreateMockVulkanContext(); + auto const context = MockVulkanContextBuilder().Build(); // Spawn two threads, and have them create a CommandPoolVK each. std::thread thread1([&context]() { From 60b871d888583b9707d6a907e862a3e854b25b51 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 10:44:09 -0700 Subject: [PATCH 02/10] made creating a context with validation layers work --- .../backend/vulkan/context_vk_unittests.cc | 12 +++++ .../backend/vulkan/test/mock_vulkan.cc | 54 ++++++++++++++++--- .../backend/vulkan/test/mock_vulkan.h | 16 ++++++ 3 files changed, 74 insertions(+), 8 deletions(-) diff --git a/impeller/renderer/backend/vulkan/context_vk_unittests.cc b/impeller/renderer/backend/vulkan/context_vk_unittests.cc index 48ae3a08be1d4..b69cdfdf4bfaf 100644 --- a/impeller/renderer/backend/vulkan/context_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/context_vk_unittests.cc @@ -94,5 +94,17 @@ TEST(ContextVKTest, CanCreateContextInAbsenceOfValidationLayers) { ASSERT_NE(context, nullptr); } +TEST(ContextVKTest, CanCreateContextWithValidationLayers) { + auto context = + MockVulkanContextBuilder() + .SetSettingsCallback( + [](auto& settings) { settings.enable_validation = true; }) + .SetInstanceExtensions( + {"VK_KHR_surface", "VK_MVK_macos_surface", "VK_EXT_debug_utils"}) + .SetInstanceLayers({"VK_LAYER_KHRONOS_validation"}) + .Build(); + ASSERT_NE(context, nullptr); +} + } // namespace testing } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index 726ecf304abf9..3272fb2bd17c5 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -54,25 +54,39 @@ class MockDevice final { void noop() {} +thread_local std::vector g_instance_extensions; + VkResult vkEnumerateInstanceExtensionProperties( const char* pLayerName, uint32_t* pPropertyCount, VkExtensionProperties* pProperties) { if (!pProperties) { - *pPropertyCount = 2; - + *pPropertyCount = g_instance_extensions.size(); } else { - strcpy(pProperties[0].extensionName, "VK_KHR_surface"); - pProperties[0].specVersion = 0; - strcpy(pProperties[1].extensionName, "VK_MVK_macos_surface"); - pProperties[1].specVersion = 0; + uint32_t count = 0; + for (const std::string& ext : g_instance_extensions) { + strcpy(pProperties[count].extensionName, ext.c_str()); + pProperties[count].specVersion = 0; + count++; + } } return VK_SUCCESS; } +thread_local std::vector g_instance_layers; + VkResult vkEnumerateInstanceLayerProperties(uint32_t* pPropertyCount, VkLayerProperties* pProperties) { - *pPropertyCount = 0; + if (!pProperties) { + *pPropertyCount = g_instance_layers.size(); + } else { + uint32_t count = 0; + for (const std::string& layer : g_instance_layers) { + strcpy(pProperties[count].layerName, layer.c_str()); + pProperties[count].specVersion = 0; + count++; + } + } return VK_SUCCESS; } @@ -415,6 +429,20 @@ VkResult vkGetFenceStatus(VkDevice device, VkFence fence) { return VK_SUCCESS; } +VkResult vkCreateDebugUtilsMessengerEXT( + VkInstance instance, + const VkDebugUtilsMessengerCreateInfoEXT* pCreateInfo, + const VkAllocationCallbacks* pAllocator, + VkDebugUtilsMessengerEXT* pMessenger) { + return VK_SUCCESS; +} + +VkResult vkSetDebugUtilsObjectNameEXT( + VkDevice device, + const VkDebugUtilsObjectNameInfoEXT* pNameInfo) { + return VK_SUCCESS; +} + PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, const char* pName) { if (strcmp("vkEnumerateInstanceExtensionProperties", pName) == 0) { @@ -507,12 +535,19 @@ PFN_vkVoidFunction GetMockVulkanProcAddress(VkInstance instance, return (PFN_vkVoidFunction)vkWaitForFences; } else if (strcmp("vkGetFenceStatus", pName) == 0) { return (PFN_vkVoidFunction)vkGetFenceStatus; + } else if (strcmp("vkCreateDebugUtilsMessengerEXT", pName) == 0) { + return (PFN_vkVoidFunction)vkCreateDebugUtilsMessengerEXT; + } else if (strcmp("vkSetDebugUtilsObjectNameEXT", pName) == 0) { + return (PFN_vkVoidFunction)vkSetDebugUtilsObjectNameEXT; } return noop; } } // namespace +MockVulkanContextBuilder::MockVulkanContextBuilder() + : instance_extensions_({"VK_KHR_surface", "VK_MVK_macos_surface"}) {} + std::shared_ptr MockVulkanContextBuilder::Build() { auto message_loop = fml::ConcurrentMessageLoop::Create(); ContextVK::Settings settings; @@ -520,7 +555,10 @@ std::shared_ptr MockVulkanContextBuilder::Build() { if (settings_callback_) { settings_callback_(settings); } - return ContextVK::Create(std::move(settings)); + g_instance_extensions = instance_extensions_; + g_instance_layers = instance_layers_; + std::shared_ptr result = ContextVK::Create(std::move(settings)); + return result; } std::shared_ptr> GetMockVulkanFunctions( diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.h b/impeller/renderer/backend/vulkan/test/mock_vulkan.h index fababd3997f26..41c82c6da2a90 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.h +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.h @@ -18,6 +18,8 @@ std::shared_ptr> GetMockVulkanFunctions( class MockVulkanContextBuilder { public: + MockVulkanContextBuilder(); + //------------------------------------------------------------------------------ /// @brief Create a Vulkan context with Vulkan functions mocked. The /// caller is given a chance to tinker on the settings right @@ -35,8 +37,22 @@ class MockVulkanContextBuilder { return *this; } + MockVulkanContextBuilder& SetInstanceExtensions( + const std::vector& instance_extensions) { + instance_extensions_ = instance_extensions; + return *this; + } + + MockVulkanContextBuilder& SetInstanceLayers( + const std::vector& instance_layers) { + instance_layers_ = instance_layers; + return *this; + } + private: std::function settings_callback_; + std::vector instance_extensions_; + std::vector instance_layers_; }; } // namespace testing From 67e90553c80f3d69044ab1c7f1285e31dc25018a Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 10:50:11 -0700 Subject: [PATCH 03/10] added capabilties asserts --- impeller/renderer/backend/vulkan/context_vk_unittests.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/impeller/renderer/backend/vulkan/context_vk_unittests.cc b/impeller/renderer/backend/vulkan/context_vk_unittests.cc index b69cdfdf4bfaf..11f8b1599bc21 100644 --- a/impeller/renderer/backend/vulkan/context_vk_unittests.cc +++ b/impeller/renderer/backend/vulkan/context_vk_unittests.cc @@ -92,6 +92,9 @@ TEST(ContextVKTest, CanCreateContextInAbsenceOfValidationLayers) { }) .Build(); ASSERT_NE(context, nullptr); + const CapabilitiesVK* capabilites_vk = + reinterpret_cast(context->GetCapabilities().get()); + ASSERT_FALSE(capabilites_vk->AreValidationsEnabled()); } TEST(ContextVKTest, CanCreateContextWithValidationLayers) { @@ -104,6 +107,9 @@ TEST(ContextVKTest, CanCreateContextWithValidationLayers) { .SetInstanceLayers({"VK_LAYER_KHRONOS_validation"}) .Build(); ASSERT_NE(context, nullptr); + const CapabilitiesVK* capabilites_vk = + reinterpret_cast(context->GetCapabilities().get()); + ASSERT_TRUE(capabilites_vk->AreValidationsEnabled()); } } // namespace testing From ba4bc1ebaa197ec844e6e1cf9d4949f25574a53d Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 12:29:48 -0700 Subject: [PATCH 04/10] moved to fml thread_local --- impeller/renderer/backend/vulkan/test/mock_vulkan.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index 3272fb2bd17c5..5067426d83885 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -5,6 +5,7 @@ #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" #include #include "fml/macros.h" +#include "fml/thread_local.h" #include "impeller/base/thread_safety.h" namespace impeller { @@ -54,7 +55,7 @@ class MockDevice final { void noop() {} -thread_local std::vector g_instance_extensions; +FML_THREAD_LOCAL std::vector g_instance_extensions; VkResult vkEnumerateInstanceExtensionProperties( const char* pLayerName, @@ -73,7 +74,7 @@ VkResult vkEnumerateInstanceExtensionProperties( return VK_SUCCESS; } -thread_local std::vector g_instance_layers; +FML_THREAD_LOCAL std::vector g_instance_layers; VkResult vkEnumerateInstanceLayerProperties(uint32_t* pPropertyCount, VkLayerProperties* pProperties) { From 004f79b7ba8062d235fd277dd392db8353c01502 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 12:50:37 -0700 Subject: [PATCH 05/10] removed strcpy --- impeller/renderer/backend/vulkan/test/mock_vulkan.cc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index 5067426d83885..4d8e1ddbb152b 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" +#include #include #include "fml/macros.h" #include "fml/thread_local.h" @@ -13,6 +14,12 @@ namespace testing { namespace { +void StrcpyChecked(char* dst, const char* src) { + static constexpr size_t kMaxStrSize = 1024; + size_t result = strlcpy(dst, src, kMaxStrSize); + FML_CHECK(result < kMaxStrSize); +} + struct MockCommandBuffer { explicit MockCommandBuffer( std::shared_ptr> called_functions) @@ -66,7 +73,7 @@ VkResult vkEnumerateInstanceExtensionProperties( } else { uint32_t count = 0; for (const std::string& ext : g_instance_extensions) { - strcpy(pProperties[count].extensionName, ext.c_str()); + StrcpyChecked(pProperties[count].extensionName, ext.c_str()); pProperties[count].specVersion = 0; count++; } @@ -83,7 +90,7 @@ VkResult vkEnumerateInstanceLayerProperties(uint32_t* pPropertyCount, } else { uint32_t count = 0; for (const std::string& layer : g_instance_layers) { - strcpy(pProperties[count].layerName, layer.c_str()); + StrcpyChecked(pProperties[count].layerName, layer.c_str()); pProperties[count].specVersion = 0; count++; } From da1aada63eba01bfbeb55ddda96f010d99e05b7e Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 13:07:00 -0700 Subject: [PATCH 06/10] switched to strncpy --- impeller/renderer/backend/vulkan/test/mock_vulkan.cc | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index 4d8e1ddbb152b..75cee3d40d8b3 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -14,12 +14,6 @@ namespace testing { namespace { -void StrcpyChecked(char* dst, const char* src) { - static constexpr size_t kMaxStrSize = 1024; - size_t result = strlcpy(dst, src, kMaxStrSize); - FML_CHECK(result < kMaxStrSize); -} - struct MockCommandBuffer { explicit MockCommandBuffer( std::shared_ptr> called_functions) @@ -73,7 +67,8 @@ VkResult vkEnumerateInstanceExtensionProperties( } else { uint32_t count = 0; for (const std::string& ext : g_instance_extensions) { - StrcpyChecked(pProperties[count].extensionName, ext.c_str()); + strncpy(pProperties[count].extensionName, ext.c_str(), + VK_MAX_EXTENSION_NAME_SIZE); pProperties[count].specVersion = 0; count++; } @@ -90,7 +85,8 @@ VkResult vkEnumerateInstanceLayerProperties(uint32_t* pPropertyCount, } else { uint32_t count = 0; for (const std::string& layer : g_instance_layers) { - StrcpyChecked(pProperties[count].layerName, layer.c_str()); + strncpy(pProperties[count].layerName, layer.c_str(), + VK_MAX_EXTENSION_NAME_SIZE); pProperties[count].specVersion = 0; count++; } From 630f91593d3a833acbad94dbd75e1ec7757bfd23 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Thu, 14 Sep 2023 13:10:32 -0700 Subject: [PATCH 07/10] moved to sizeof since its a bit more robust --- impeller/renderer/backend/vulkan/test/mock_vulkan.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index 75cee3d40d8b3..ed311ac017a81 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -68,7 +68,7 @@ VkResult vkEnumerateInstanceExtensionProperties( uint32_t count = 0; for (const std::string& ext : g_instance_extensions) { strncpy(pProperties[count].extensionName, ext.c_str(), - VK_MAX_EXTENSION_NAME_SIZE); + sizeof(VkExtensionProperties::extensionName)); pProperties[count].specVersion = 0; count++; } @@ -86,7 +86,7 @@ VkResult vkEnumerateInstanceLayerProperties(uint32_t* pPropertyCount, uint32_t count = 0; for (const std::string& layer : g_instance_layers) { strncpy(pProperties[count].layerName, layer.c_str(), - VK_MAX_EXTENSION_NAME_SIZE); + sizeof(VkLayerProperties::layerName)); pProperties[count].specVersion = 0; count++; } From c89d30d84ebd2074b05e8f3bd52b4fa11162657d Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 14 Sep 2023 17:01:29 -0700 Subject: [PATCH 08/10] Add MockFence and MockFence::SetStatus. --- .../backend/vulkan/test/mock_vulkan.cc | 18 +++++++++-- .../backend/vulkan/test/mock_vulkan.h | 30 +++++++++++++++++++ .../vulkan/test/mock_vulkan_unittests.cc | 22 ++++++++++++++ 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index ed311ac017a81..01749168695d1 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -4,10 +4,13 @@ #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" #include +#include #include #include "fml/macros.h" #include "fml/thread_local.h" #include "impeller/base/thread_safety.h" +#include "vulkan/vulkan_core.h" +#include "vulkan/vulkan_enums.hpp" namespace impeller { namespace testing { @@ -42,6 +45,11 @@ class MockDevice final { called_functions_->push_back(function); } + void SetFenceFactory( + std::function()> fence_factory) { + fence_factory_ = std::move(fence_factory); + } + private: FML_DISALLOW_COPY_AND_ASSIGN(MockDevice); @@ -52,6 +60,8 @@ class MockDevice final { Mutex command_buffers_mutex_; std::vector> command_buffers_ IPLR_GUARDED_BY(command_buffers_mutex_); + + std::function()> fence_factory_; }; void noop() {} @@ -410,7 +420,8 @@ VkResult vkCreateFence(VkDevice device, const VkFenceCreateInfo* pCreateInfo, const VkAllocationCallbacks* pAllocator, VkFence* pFence) { - *pFence = reinterpret_cast(0xfe0ce); + MockDevice* mock_device = reinterpret_cast(device); + *pFence = reinterpret_cast(new MockFence()); return VK_SUCCESS; } @@ -430,7 +441,9 @@ VkResult vkWaitForFences(VkDevice device, } VkResult vkGetFenceStatus(VkDevice device, VkFence fence) { - return VK_SUCCESS; + MockDevice* mock_device = reinterpret_cast(device); + MockFence* mock_fence = reinterpret_cast(fence); + return mock_fence->GetStatus(); } VkResult vkCreateDebugUtilsMessengerEXT( @@ -562,6 +575,7 @@ std::shared_ptr MockVulkanContextBuilder::Build() { g_instance_extensions = instance_extensions_; g_instance_layers = instance_layers_; std::shared_ptr result = ContextVK::Create(std::move(settings)); + return result; } diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.h b/impeller/renderer/backend/vulkan/test/mock_vulkan.h index 41c82c6da2a90..fa440f586c8da 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.h +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.h @@ -5,10 +5,12 @@ #pragma once #include +#include #include #include #include "impeller/renderer/backend/vulkan/context_vk.h" +#include "vulkan/vulkan_enums.hpp" namespace impeller { namespace testing { @@ -16,6 +18,28 @@ namespace testing { std::shared_ptr> GetMockVulkanFunctions( VkDevice device); +// A test-controlled version of |vk::Fence|. +class MockFence final { + public: + MockFence() = default; + + // Returns the result that was set in the constructor or |SetStatus|. + VkResult GetStatus() { return static_cast(result_); } + + // Sets the result that will be returned by `GetFenceStatus`. + static void SetStatus(vk::UniqueFence& fence, vk::Result result) { + // Cast the fence to a MockFence and set the result. + VkFence raw_fence = fence.get(); + MockFence* mock_fence = reinterpret_cast(raw_fence); + mock_fence->result_ = result; + } + + private: + vk::Result result_ = vk::Result::eSuccess; + + FML_DISALLOW_COPY_AND_ASSIGN(MockFence); +}; + class MockVulkanContextBuilder { public: MockVulkanContextBuilder(); @@ -53,6 +77,12 @@ class MockVulkanContextBuilder { std::function settings_callback_; std::vector instance_extensions_; std::vector instance_layers_; + + static std::shared_ptr DefaultFenceCallback() { + return std::make_shared(); + } + std::function()> fence_factory_ = + DefaultFenceCallback; }; } // namespace testing diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc index de28491e69fed..1aa57b0345f21 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan_unittests.cc @@ -6,6 +6,7 @@ #include "gtest/gtest.h" #include "impeller/renderer/backend/vulkan/command_pool_vk.h" #include "impeller/renderer/backend/vulkan/test/mock_vulkan.h" +#include "vulkan/vulkan_enums.hpp" namespace impeller { namespace testing { @@ -33,5 +34,26 @@ TEST(MockVulkanContextTest, IsThreadSafe) { context->Shutdown(); } +TEST(MockVulkanContextTest, DefaultFenceAlwaysReportsSuccess) { + auto const context = MockVulkanContextBuilder().Build(); + auto const device = context->GetDevice(); + + auto fence = device.createFenceUnique({}).value; + EXPECT_EQ(vk::Result::eSuccess, device.getFenceStatus(*fence)); +} + +TEST(MockVulkanContextTest, MockedFenceReportsStatus) { + auto const context = MockVulkanContextBuilder().Build(); + + auto const device = context->GetDevice(); + auto fence = device.createFenceUnique({}).value; + MockFence::SetStatus(fence, vk::Result::eNotReady); + + EXPECT_EQ(vk::Result::eNotReady, device.getFenceStatus(fence.get())); + + MockFence::SetStatus(fence, vk::Result::eSuccess); + EXPECT_EQ(vk::Result::eSuccess, device.getFenceStatus(*fence)); +} + } // namespace testing } // namespace impeller From ce6a1c517d7c8c8afe689374b192e0e083e9f204 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Thu, 14 Sep 2023 17:12:00 -0700 Subject: [PATCH 09/10] Remove unused code. --- impeller/renderer/backend/vulkan/test/mock_vulkan.h | 6 ------ 1 file changed, 6 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.h b/impeller/renderer/backend/vulkan/test/mock_vulkan.h index fa440f586c8da..71a88a42e02eb 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.h +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.h @@ -77,12 +77,6 @@ class MockVulkanContextBuilder { std::function settings_callback_; std::vector instance_extensions_; std::vector instance_layers_; - - static std::shared_ptr DefaultFenceCallback() { - return std::make_shared(); - } - std::function()> fence_factory_ = - DefaultFenceCallback; }; } // namespace testing From d9d7cef9005e1a149399df3f990fd2cb3b043c49 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 15 Sep 2023 10:20:23 -0700 Subject: [PATCH 10/10] Remove dead code. --- impeller/renderer/backend/vulkan/test/mock_vulkan.cc | 7 ------- 1 file changed, 7 deletions(-) diff --git a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc index dc69987ed5787..a33f88624b89c 100644 --- a/impeller/renderer/backend/vulkan/test/mock_vulkan.cc +++ b/impeller/renderer/backend/vulkan/test/mock_vulkan.cc @@ -45,11 +45,6 @@ class MockDevice final { called_functions_->push_back(function); } - void SetFenceFactory( - std::function()> fence_factory) { - fence_factory_ = std::move(fence_factory); - } - private: FML_DISALLOW_COPY_AND_ASSIGN(MockDevice); @@ -60,8 +55,6 @@ class MockDevice final { Mutex command_buffers_mutex_; std::vector> command_buffers_ IPLR_GUARDED_BY(command_buffers_mutex_); - - std::function()> fence_factory_; }; void noop() {}