Pull swapchain handling code into separate library #1163
Conversation
First step in preparing to remove swapchain behavior from being hard-coded inside the decode framework so it can be used to build a compatibility library for use in additional scenarios.
Modify the swapchain items located in the compatibility library to be in the compatibility namespace.
Rely on only the items inside the decode::DeviceInfo struct so that we can start breaking tight coupling.
Remove more decode "_info" structs used in the swapchain interface in the steps towards making a truly separate library.
Remove unnecessary swapchain functions (which did nothing but really passthrough to the final func). This actually increased debug build performance on Linux for my Talos trace from 19+ seconds of runtime to 17+ seconds, but I'm not sure how universal that will be. Also replace a few more "decode" structures with the Vulkan handle usage instead for breaking some of the tight bindings.
The AcquireNextImage swapchain logic was mostly a passthrough with only debug logging. Removing it to remove an additional dependency in the library that is unneeded.
Remove decode::ImageInfo usage. Move virtual swapchain only items into the class.
Move the swapchain miage tracker header into the compability library, and then expose it via the swapchain class. Also remove the need for the object tracker in a few methods.
Remove more usage of the Decode::swapchain_info in the compatibility library. This again is intended to break the tight coupling so that the compatibility library can eventually be a stand-alone library.
- Remove namespace define usage - Replace using the GFXRECON_LOG_MESSAGE functions with the use of passed in function pointers to break coupling. - Also, use a new create structure to pass in more info without having to use the stack.
Remove the use of the function pointer tables pulled in from the decode namespace and use one specific to the compatibility library.
An external struct was only ever used with the swapchain class and could just as easily reference the swapchain version so that we don't have to copy data into/out of the struct when it was referenced internally by the swapchain class.
MarkY-LunarG
requested review from
locke-lunarg,
bradgrantham-lunarg,
davidd-lunarg,
aitor-lunarg and
andrew-lunarg
|
CI gfxreconstruct build queued with queue ID 1069. |
|
CI gfxreconstruct build # 2888 running. |
|
CI gfxreconstruct build # 2888 failed. |
|
CI gfxreconstruct build queued with queue ID 1091. |
|
CI gfxreconstruct build # 2889 running. |
|
CI gfxreconstruct build # 2889 failed. |
|
CI gfxreconstruct build queued with queue ID 1148. |
|
CI gfxreconstruct build # 2890 running. |
|
CI gfxreconstruct build # 2890 failed. |
|
CI gfxreconstruct build queued with queue ID 1280. |
|
CI gfxreconstruct build # 2893 running. |
|
CI gfxreconstruct build # 2893 passed. |
A C-API would make sense if third-party tools were really going to use it without building from source (we are open source: why do they need a binary?) or if we were going to release it entirely outside of GFXR on a different cadence from our own releases, but just to reuse a library internally that is built at the same time using the same compilers as the tools that use it, or for external users who build from source, it seems like a lot of faff for us and some overhead to thunk through a I'm guessing you are thinking of something like the hourglass API idiom. |
|
@andrew-lunarg, my current plan is to leave it as a C++ interface because of the overhead of converting all the C++ items (vectors, etc) into C could be a pain. Yes, it would allow us to have a more ABI-resistant interface than C++, but I figure that we can just require anyone dumping out C++ code to eventually need the GFXReconstruct code base to build their samples. I'm not completely against eventually going the C route, but I figured at the very least, this was a good lower impact first step. |
bradgrantham-lunarg
added
the
P2
| @@ -14,10 +14,12 @@ target_sources(gfxrecon_application | |||
|
|
|||
| target_include_directories(gfxrecon_application | |||
| PUBLIC | |||
| ${GFXRECON_SOURCE_DIR} | |||
There was a problem hiding this comment.
Can compatibility be in framework with other libraries like graphics and util?
| ${CMAKE_SOURCE_DIR}/compatibility/vulkan_captured_swapchain.h | ||
| ${CMAKE_SOURCE_DIR}/compatibility/vulkan_captured_swapchain.cpp | ||
| ${CMAKE_SOURCE_DIR}/compatibility/vulkan_virtual_swapchain.h | ||
| ${CMAKE_SOURCE_DIR}/compatibility/vulkan_virtual_swapchain.cpp |
There was a problem hiding this comment.
I think rather than compiling these into decode here, decode should link against libgfxrecon_compatibility. Or maybe the tools have to link against it, not sure. Can you take a look?
bradgrantham-lunarg
added
P1
|
Raised to P1 because |
|
|
||
| #include "vulkan/vulkan.h" | ||
|
|
||
| #include <unordered_map> | ||
|
|
||
| GFXRECON_BEGIN_NAMESPACE(gfxrecon) | ||
| GFXRECON_BEGIN_NAMESPACE(decode) | ||
| namespace gfxrecon |
There was a problem hiding this comment.
Should we use GFXRECON_BEGIN_NAMESPACE to unified code style?
GFXRECON_BEGIN_NAMESPACE is in util\defines.h. I feel gfxrecon_util.lib is the lowest level library. compatibility should be fine to link gfxrecon_util.lib.
| @@ -172,11 +173,13 @@ VulkanReplayConsumerBase::VulkanReplayConsumerBase(std::shared_ptr<application:: | |||
| // Process option to select swapchain handler. The options is '--use-captured-swapchain-indices'. | |||
| if (options.enable_use_captured_swapchain_indices) | |||
| { | |||
| swapchain_ = std::make_unique<VulkanCapturedSwapchain>(); | |||
| swapchain_ = std::make_unique<compatibility::VulkanCapturedSwapchain>(); | |||
| validate_swapchain_image_indices_ = true; | |||
There was a problem hiding this comment.
Could we just use options.enable_use_captured_swapchain_indices, instead of validate_swapchain_image_indices_?
If options.enable_use_captured_swapchain_indices is not enough straightforward, I might add a function, like IsValidateSwapchainImageIndices(). The function returns !options.enable_use_captured_swapchain_indices.
| @@ -5525,8 +5753,15 @@ VulkanReplayConsumerBase::OverrideQueuePresentKHR(PFN_vkQueuePresentKHR | |||
| // Only attempt to find imported or shadow semaphores if we know at least one around. | |||
| if ((!have_imported_semaphores_) && (shadow_semaphores_.empty()) && (modified_present_info.swapchainCount != 0)) | |||
| { | |||
| result = swapchain_->QueuePresentKHR( | |||
| func, capture_image_indices_, swapchain_infos_, queue_info, &modified_present_info); | |||
| if (queue_info == nullptr || queue_info->handle == VK_NULL_HANDLE) | |||
There was a problem hiding this comment.
In the begin of OverrideQueuePresentKHR, it has checked queue_info != nullptr. We could also check queue_info->handle != VK_NULL_HANDLE there, so we don't need to check them here again.
| VkSwapchainKHR swapchain = VK_NULL_HANDLE; | ||
| VkSemaphore semaphore = VK_NULL_HANDLE; | ||
| VkFence fence = VK_NULL_HANDLE; | ||
| if (swapchain_info != nullptr) |
There was a problem hiding this comment.
In the begin of OverrideAcquireNextImageKHR, it has checked swapchain_info != nullptr. We don't need to check again.
| void LogErrorMessage(const char* message, ...) | ||
| { | ||
| va_list args; | ||
| va_start(args, message); | ||
| GFXRECON_LOG_ERROR(message, args); | ||
| va_end(args); | ||
| } | ||
|
|
||
| void LogWarningMessage(const char* message, ...) | ||
| { | ||
| va_list args; | ||
| va_start(args, message); | ||
| GFXRECON_LOG_WARNING(message, args); | ||
| va_end(args); | ||
| } | ||
|
|
||
| void LogInfoMessage(const char* message, ...) | ||
| { | ||
| va_list args; | ||
| va_start(args, message); | ||
| GFXRECON_LOG_INFO(message, args); | ||
| va_end(args); | ||
| } |
There was a problem hiding this comment.
GFXRECON_LOG is in utils\loggings.h. I feel gfxrecon_util.lib is the lowest level library. compatibility should be fine to link gfxrecon_util.lib.
There was a problem hiding this comment.
@locke-lunarg : This is going to be compiled into non-GFXR code (i.e. the generated "toCpp" output). My original design did not intend for that source to have to link back into GFXR source since they may not have it on the generating system. Is this important to maintain? If so, it will likely require more changes.
| CreateImageResource, | ||
| DestroyImageResource, | ||
| AllocateDeviceMemoryResource, | ||
| FreeDeviceMemoryResource, | ||
| BindImageResourceToDeviceMemoryResource }, |
There was a problem hiding this comment.
Decode is upper level library. Compatibility is lower level library. I don't know if it's common that lower level library use the function of upper level library?
| VkImage* image, | ||
| uintptr_t* resource_id) | ||
| { | ||
| gfxrecon::decode::VulkanResourceAllocator* resource_allocator = |
There was a problem hiding this comment.
Maybe we should move gfxrecon::decode::VulkanResourceAllocator to gfxrecon_graphics.lib. Compatibility could link gfxrecon_graphics.lib, so it doesn't need to send function pointer from decode to compatibility. But I'm not sure if it's a good idea.
There was a problem hiding this comment.
This is going to require more work because the VulkanResourceAllocator uses objects out of the decode path. (The various Info structures and the HandleId as examples). The plan is to eventually port it over, but this change is already massive as it is.
|
My opinion about library levels is |
Pull all of the swapchain handling code into a separate library with the intent of having it be stand-alone for use by other GFXReconstruct utilities.
NOTES:
compatibilitybecause I intend to move some other universal functionality into it eventually alongside of the swapchain functionality currently in there.