chore(vulkan): Add Vulkan frame sync coordinator (#13058) bcdfe4fe5f Add a VulkanFrameSyncCoordinator class for use with the android runtime (or any other runtimes where there could be multiple swap chains but a single render context) Co-authored-by: Josh Jersild <joshua@rive.app>
diff --git a/.rive_head b/.rive_head index d529896..19585ff 100644 --- a/.rive_head +++ b/.rive_head
@@ -1 +1 @@ -1c69c3de41c9286dc5156049ec4ad9c7127f4c64 +bcdfe4fe5fcee41b74d3bf96489bd67cac7236a8
diff --git a/renderer/rive_vk_bootstrap/include/rive_vk_bootstrap/vulkan_frame_sync_coordinator.hpp b/renderer/rive_vk_bootstrap/include/rive_vk_bootstrap/vulkan_frame_sync_coordinator.hpp new file mode 100644 index 0000000..a2b6895 --- /dev/null +++ b/renderer/rive_vk_bootstrap/include/rive_vk_bootstrap/vulkan_frame_sync_coordinator.hpp
@@ -0,0 +1,184 @@ +#pragma once + +// Only include this file if we aren't building unit tests (the unit tests have +// a custom VulkanFrameSynchronizer type so that we can test without needing an +// actual vulkan-enabled build) +// +// Note that this is also why this file's implementation was done inline vs in +// a cpp file - for ease of unit testing +#ifndef RIVE_UNIT_TEST_FRAME_SYNC_COORDINATOR +#include "rive_vk_bootstrap/vulkan_frame_synchronizer.hpp" +#endif + +#include <algorithm> +#include <cassert> +#include <cstdint> +#include <deque> +#include <vector> + +namespace rive_vkb +{ + +class VulkanFrameSyncCoordinator +{ +public: + // Add a newly-created frame synchronizer that we want to coordinate with + // others. + void addFrameSynchronizer(VulkanFrameSynchronizer* sync); + + // Remove a frame synchronizer that is about to be destroyed so it no longer + // factors into the coordination. + void removeFrameSynchronizer(VulkanFrameSynchronizer* sync); + + // Whenever a frame synchronizer begins a new frame, this should be called + // afterwards to track the overall current and safe frames across all + // synchronizers. + void onFrameStart(VulkanFrameSynchronizer* synchronizer); + + uint64_t currentFrameNumber() const { return m_currentCoordinatedFrame; } + uint64_t safeFrameNumber() const { return m_coordinatedSafeFrame; } + +private: + struct FramePair + { + uint64_t synchronizerFrame; + uint64_t coordinatedFrame; + }; + + struct SynchronizerData + { + VulkanFrameSynchronizer* synchronizer; + + // using `deque` instead of `queue` because it has a clear() method. + std::deque<FramePair> frames; + }; + + std::vector<SynchronizerData>::iterator findSyncData( + VulkanFrameSynchronizer* s) + { + auto found = std::find_if(m_synchronizers.begin(), + m_synchronizers.end(), + [&](auto& d) { return d.synchronizer == s; }); + return found; + } + + std::vector<SynchronizerData> m_synchronizers; + uint64_t m_currentCoordinatedFrame = 0; + uint64_t m_coordinatedSafeFrame = 0; +}; + +inline void VulkanFrameSyncCoordinator::addFrameSynchronizer( + VulkanFrameSynchronizer* sync) +{ + assert(sync != nullptr); + +#ifndef NDEBUG + // The synchronizer being added should not already be in the list. + auto&& found = findSyncData(sync); + assert(found == m_synchronizers.end()); +#endif + + m_synchronizers.push_back({.synchronizer = sync}); +} + +inline void VulkanFrameSyncCoordinator::removeFrameSynchronizer( + VulkanFrameSynchronizer* sync) +{ + assert(sync != nullptr); + + auto&& found = findSyncData(sync); + assert(found != m_synchronizers.end()); + + m_synchronizers.erase(found); +} + +inline void VulkanFrameSyncCoordinator::onFrameStart( + VulkanFrameSynchronizer* synchronizer) +{ + auto synchronizerCurrentFrame = synchronizer->currentFrameNumber(); + auto synchronizerSafeFrame = synchronizer->safeFrameNumber(); + + // First up is updating the current synchronizer. + // Note that this could be done as a map instead of a vector but in + // practice this is likely to be updated infrequently and small, so a + // linear search will likely be as fast or faster than a map lookup. + auto&& found = findSyncData(synchronizer); + assert(found != m_synchronizers.end()); + + assert(found->frames.empty() || + found->frames.back().synchronizerFrame < synchronizerCurrentFrame); + // We no longer need to track any frames older than the safe frame. Also + // update the safe frame tracking to start at this synchronizer's safe + // frame (we'll back it up to the earliest needed one later) + if (found->frames.empty() && + synchronizerSafeFrame != synchronizerCurrentFrame) + { + // This is a synchronizer we don't already have tracking info for. Since + // the logic of this class assumes we have a frame entry for every frame + // from a synchronizer, add an entry for every frame from the + // synchronizer's safe frame to just before its current frame, pinning + // each of them to the frame before the one we're starting (so that they + // all have to roll out of this synchronizer before we move the safe + // frame up to point at what is now the newest frame) + for (auto frame = synchronizerSafeFrame; + frame < synchronizerCurrentFrame; + frame++) + { + found->frames.push_back({ + .synchronizerFrame = frame, + .coordinatedFrame = m_currentCoordinatedFrame, + }); + } + } + else + { + while (!found->frames.empty() && + found->frames.front().synchronizerFrame < synchronizerSafeFrame) + { + found->frames.pop_front(); + } + } + + m_currentCoordinatedFrame++; + + // Add our new frame to the list for tracking. + found->frames.push_back({ + .synchronizerFrame = synchronizerCurrentFrame, + .coordinatedFrame = m_currentCoordinatedFrame, + }); + + m_coordinatedSafeFrame = found->frames.front().coordinatedFrame; + + // Now check all the other synchronizers and find the earliest safe + // frame + for (auto& s : m_synchronizers) + { + if (s.frames.empty() || + s.frames.front().coordinatedFrame >= m_coordinatedSafeFrame) + { + // Either no frames have rendered yet or the safe frame for this + // synchronizer is newer than the one we're already tracking as + // the earliest. Either way, this synchronizer won't change the + // minimum safe frame. + continue; + } + + if (s.synchronizer->checkMostRecentFrameCompletion()) + { + // If the most-recent frame for this synchronizer is complete, + // then all frames for it are safe and we can clear the + // tracking as an optimization. + s.frames.clear(); + } + else + { + // The synchronizer hasn't run through its queued frames so + // honor its safe frame. + m_coordinatedSafeFrame = s.frames.front().coordinatedFrame; + } + } + + assert(m_currentCoordinatedFrame > m_coordinatedSafeFrame); +} + +} // namespace rive_vkb
diff --git a/renderer/rive_vk_bootstrap/include/rive_vk_bootstrap/vulkan_frame_synchronizer.hpp b/renderer/rive_vk_bootstrap/include/rive_vk_bootstrap/vulkan_frame_synchronizer.hpp index 33e6d47..01858c1 100644 --- a/renderer/rive_vk_bootstrap/include/rive_vk_bootstrap/vulkan_frame_synchronizer.hpp +++ b/renderer/rive_vk_bootstrap/include/rive_vk_bootstrap/vulkan_frame_synchronizer.hpp
@@ -74,6 +74,8 @@ VkQueue graphicsQueue() const { return m_graphicsQueue; } + bool checkMostRecentFrameCompletion(); + protected: struct Options { @@ -155,6 +157,9 @@ // frame, and one for the currently-building frame. std::vector<InFlightFrame> m_inFlightFrames; + bool m_isFrameStarted = false; + bool m_isMostRecentFrameDone = false; + // These are all the commands the swapchain needs to do its work - this // macro is also used to load them in the .cpp #define RIVE_VK_FRAME_SYNC_INSTANCE_COMMANDS(F) \ @@ -167,6 +172,7 @@ F(vkAllocateCommandBuffers) \ F(vkFreeCommandBuffers) \ F(vkWaitForFences) \ + F(vkGetFenceStatus) \ F(vkCmdPipelineBarrier) \ F(vkQueueSubmit) \ F(vkGetDeviceQueue) \
diff --git a/renderer/rive_vk_bootstrap/src/vulkan_frame_synchronizer.cpp b/renderer/rive_vk_bootstrap/src/vulkan_frame_synchronizer.cpp index d4a8ca2..f4ad602 100644 --- a/renderer/rive_vk_bootstrap/src/vulkan_frame_synchronizer.cpp +++ b/renderer/rive_vk_bootstrap/src/vulkan_frame_synchronizer.cpp
@@ -140,6 +140,11 @@ VkResult VulkanFrameSynchronizer::waitForFenceAndBeginFrame( VkSemaphore* optionalOutSemaphore) { + assert(!m_isFrameStarted); + + // There is a new most-recent frame, so clear this flag if it was set. + m_isMostRecentFrameDone = false; + // Before we can use the command buffers/semaphores for the current frame, // we need to wait on its fence to stall the CPU until it's ready. static constexpr auto NO_TIMEOUT = std::numeric_limits<uint64_t>::max(); @@ -167,12 +172,14 @@ *optionalOutSemaphore = current().semaphore; } + m_isFrameStarted = true; return VK_SUCCESS; } VkResult VulkanFrameSynchronizer::endFrame( std::optional<VkSemaphore> externalSignalSemaphore) { + assert(m_isFrameStarted); auto& frame = current(); // This frame is done - reset the fence so that the submit can signal it. @@ -222,6 +229,7 @@ m_pixelReadState = PixelReadState::Ready; } + m_isFrameStarted = false; return VK_SUCCESS; } @@ -388,4 +396,28 @@ m_vkDestroySemaphore(m_device, semaphore, nullptr); } +bool VulkanFrameSynchronizer::checkMostRecentFrameCompletion() +{ + if (m_isFrameStarted) + { + assert(!m_isMostRecentFrameDone); + return false; + } + + if (!m_isMostRecentFrameDone) + { + auto& frame = prev(); + if (m_vkGetFenceStatus(m_device, frame.fence) == VK_SUCCESS) + { + // The fence for the last-completed frame is signaled, so that frame + // has run through completely. At this point, it is safe to free any + // per-frame resources for anything in frames rendered to this swap + // chain. + m_isMostRecentFrameDone = true; + } + } + + return m_isMostRecentFrameDone; +} + } // namespace rive_vkb \ No newline at end of file
diff --git a/tests/unit_tests/premake5.lua b/tests/unit_tests/premake5.lua index 327d4b5..714c046 100644 --- a/tests/unit_tests/premake5.lua +++ b/tests/unit_tests/premake5.lua
@@ -58,6 +58,7 @@ '../../include', '../../decoders/include', '../../renderer/include', + '../../renderer/rive_vk_bootstrap/include', '../../renderer/src', '../../../rive_native/native/include', '../../../texture_compressor/src',
diff --git a/tests/unit_tests/renderer/vulkan_frame_sync_coordinator_test.cpp b/tests/unit_tests/renderer/vulkan_frame_sync_coordinator_test.cpp new file mode 100644 index 0000000..866c109 --- /dev/null +++ b/tests/unit_tests/renderer/vulkan_frame_sync_coordinator_test.cpp
@@ -0,0 +1,283 @@ +#include <catch.hpp> + +// Before including the frame sync coordinator header, declare a unit test +// synchronizer class and the RIVE_UNIT_TEST_FRAME_SYNC_COORDINATOR, which +// allows us to use this mock version instead of the real one. + +namespace rive::tests +{ +class VulkanFrameSynchronizer +{ +public: + // Implement the minimum surface area that the coordinator cares about. + bool checkMostRecentFrameCompletion() { return m_isMostRecentFrameDone; } + uint64_t currentFrameNumber() const { return m_currentFrameNumber; } + uint64_t safeFrameNumber() const { return m_safeFrameNumber; } + + // Testing functions (not standard VulkanFrameSynchronizer functions) + void tickFrameAndSafe() + { + m_isMostRecentFrameDone = false; + m_currentFrameNumber++; + m_safeFrameNumber++; + } + + bool m_isMostRecentFrameDone = false; + uint64_t m_currentFrameNumber = 0; + uint64_t m_safeFrameNumber = 0; +}; +} // namespace rive::tests + +using namespace rive::tests; + +#define RIVE_UNIT_TEST_FRAME_SYNC_COORDINATOR +#include "rive_vk_bootstrap/vulkan_frame_sync_coordinator.hpp" + +using namespace rive_vkb; + +TEST_CASE("Single Frame Sync", "[vulkan_frame_sync_coordinator]") +{ + VulkanFrameSynchronizer sync; + VulkanFrameSyncCoordinator coordinator; + + sync.m_currentFrameNumber = 2; + + coordinator.addFrameSynchronizer(&sync); + + sync.tickFrameAndSafe(); + coordinator.onFrameStart(&sync); + CHECK(coordinator.currentFrameNumber() == 1); + + // On a new frame synchronizer the safe frame should be no later than the + // frame before the current frame (otherwise the renderer will clean up the + // assets used during the current frame which is decidedly not what we want) + CHECK(coordinator.safeFrameNumber() == 0); + + sync.tickFrameAndSafe(); + coordinator.onFrameStart(&sync); + CHECK(coordinator.currentFrameNumber() == 2); + CHECK(coordinator.safeFrameNumber() == 0); + + // The next frame is when the safe frame should finally move forward, as + // we've now incremented up past the first frame we submitted to the + // coordinator. + for (auto f = 3u; f < 20u; f++) + { + sync.tickFrameAndSafe(); + coordinator.onFrameStart(&sync); + CHECK(coordinator.currentFrameNumber() == f); + CHECK(coordinator.safeFrameNumber() == f - 2); + } +} + +TEST_CASE("Replaced Frame Sync", "[vulkan_frame_sync_coordinator]") +{ + VulkanFrameSynchronizer syncA; + syncA.m_currentFrameNumber = 1; + VulkanFrameSyncCoordinator coordinator; + + coordinator.addFrameSynchronizer(&syncA); + + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + CHECK(coordinator.currentFrameNumber() == 1); + CHECK(coordinator.safeFrameNumber() == 0); + + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + CHECK(coordinator.currentFrameNumber() == 2); + CHECK(coordinator.safeFrameNumber() == 1); + + VulkanFrameSynchronizer syncB; + // Pick arbitrarily different frame numbers + syncB.m_currentFrameNumber = 1000; + syncB.m_safeFrameNumber = 999; + + coordinator.removeFrameSynchronizer(&syncA); + coordinator.addFrameSynchronizer(&syncB); + + coordinator.onFrameStart(&syncB); + // After the replacement, the new current frame should increase (separate + // from the value from the sync) and, as stated above, the safe frame should + // end up as the frame *before* the current frame + CHECK(coordinator.currentFrameNumber() == 3); + CHECK(coordinator.safeFrameNumber() == 2); + + syncB.tickFrameAndSafe(); + coordinator.onFrameStart(&syncB); + CHECK(coordinator.currentFrameNumber() == 4); + CHECK(coordinator.safeFrameNumber() == 3); + + syncB.tickFrameAndSafe(); + coordinator.onFrameStart(&syncB); + CHECK(coordinator.currentFrameNumber() == 5); + CHECK(coordinator.safeFrameNumber() == 4); +} + +TEST_CASE("Two Frame Syncs", "[vulkan_frame_sync_coordinator]") +{ + VulkanFrameSynchronizer syncA; + VulkanFrameSynchronizer syncB; + + VulkanFrameSyncCoordinator coordinator; + + coordinator.addFrameSynchronizer(&syncA); + coordinator.addFrameSynchronizer(&syncB); + + syncA.m_currentFrameNumber = 1000; + syncA.m_safeFrameNumber = syncA.m_currentFrameNumber - 2; + + syncB.m_currentFrameNumber = 500000; + syncB.m_safeFrameNumber = syncB.m_currentFrameNumber - 1; + + // coordinator frame 1 is syncA frame 1001 (with its safe frame of 999) + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + + CHECK(coordinator.currentFrameNumber() == 1); + CHECK(coordinator.safeFrameNumber() == 0); + + // Tick syncB a few times - note that the safe frame won't update for this + // because it's being held back by sync A. + // This gets the coordinator's frame up to 5. + for (auto f = 2u; f <= 5u; f++) + { + syncB.tickFrameAndSafe(); + coordinator.onFrameStart(&syncB); + CHECK(coordinator.currentFrameNumber() == f); + CHECK(coordinator.safeFrameNumber() == 0); + } + + // syncA's next frame is 6, but internally it's 1002 (and safe is now 1000, + // which is still before coordinated frame 1), then after that is 7 which is + // internally 1003/1001, which finally moves the safe frame up by 1. + // The safe frame counter is tested in the loop as f - 6 (which is the + // above-mentioned 0 then 1) + for (auto f = 6u; f <= 7u; f++) + { + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + CHECK(coordinator.currentFrameNumber() == f); + CHECK(coordinator.safeFrameNumber() == (f - 6)); + } + + // Ticking syncA forward again now moves its safe frame past syncB, so now + // syncB's sync frame takes over + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + CHECK(coordinator.currentFrameNumber() == 8); + CHECK(coordinator.safeFrameNumber() == 4); + + // Now we'll simulate the scenario where syncB has fallen so far behind that + // it has actually rendered all the way through its most recent frame, which + // should spring the safe frame forward to be within 2 of current (as syncA + // is now the only deciding factor) + syncB.m_isMostRecentFrameDone = true; + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + CHECK(coordinator.currentFrameNumber() == 9); + CHECK(coordinator.safeFrameNumber() == 7); + + // Now if syncB ticks again it will start to matter again (but syncA will + // hold the safe frame back) + for (auto f = 10u; f <= 15u; f++) + { + syncB.tickFrameAndSafe(); + coordinator.onFrameStart(&syncB); + CHECK(coordinator.currentFrameNumber() == f); + CHECK(coordinator.safeFrameNumber() == 7); + } + + // ticking Sync A forward twice will move the safe frame twice as well (as + // it's still pinned by A's previous ticks) + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + CHECK(coordinator.currentFrameNumber() == 16); + CHECK(coordinator.safeFrameNumber() == 8); + + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + CHECK(coordinator.currentFrameNumber() == 17); + CHECK(coordinator.safeFrameNumber() == 9); + + // ticking Sync A a third time will now move it past sync B's safe frame, + // which will take over as the new current safe frame. + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + CHECK(coordinator.currentFrameNumber() == 18); + CHECK(coordinator.safeFrameNumber() == 14); + + // Now if we remove sync B, the coordinator's safe frame will only rely on A + // again + coordinator.removeFrameSynchronizer(&syncB); + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + CHECK(coordinator.currentFrameNumber() == 19); + CHECK(coordinator.safeFrameNumber() == 17); + + // Adding sync B but not ticking it means that it won't affect anything + // (note that in practice we're not expecting synchronizers to get removed + // and re-added - in the real runtime only *new* synchronizers should be + // getting added - but for testing, this is fine) + coordinator.addFrameSynchronizer(&syncB); + for (auto f = 20u; f <= 25u; f++) + { + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + CHECK(coordinator.currentFrameNumber() == f); + CHECK(coordinator.safeFrameNumber() == f - 2); + } + + // Now if we tick syncB and remove syncA, its next frame will be the + // baseline (but the safe frame should be the frame before the new current + // frame) + coordinator.removeFrameSynchronizer(&syncA); + syncB.tickFrameAndSafe(); + coordinator.onFrameStart(&syncB); + CHECK(coordinator.currentFrameNumber() == 26); + CHECK(coordinator.safeFrameNumber() == 25); +} + +TEST_CASE("Resumed synchronizer", "[vulkan_frame_sync_coordinator]") +{ + VulkanFrameSynchronizer syncA; + VulkanFrameSyncCoordinator coordinator; + coordinator.addFrameSynchronizer(&syncA); + + // syncA's frame 2 is coordinator frame 1 (safe frame is still 0) + syncA.m_currentFrameNumber = 2; + coordinator.onFrameStart(&syncA); + + // Mark it as fully completed (emulating its frames have fully run through) + syncA.m_isMostRecentFrameDone = true; + + // Temporarily add a second synchronizer and tick it once to let the + // coordinator catch that syncA is fully completed. This will increase the + // coordinator frame to 2 (and its safe frame to 1). + { + VulkanFrameSynchronizer syncB; + coordinator.addFrameSynchronizer(&syncB); + syncB.m_currentFrameNumber = 1; + coordinator.onFrameStart(&syncB); + CHECK(coordinator.safeFrameNumber() == 1); + coordinator.removeFrameSynchronizer(&syncB); + } + + // Now A ticks again, which should move the safe frame up to 2 (the frame + // before the current frame number of 3) + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + CHECK(coordinator.safeFrameNumber() == 2); + + // Run another frame: the safe frame should still be behind the first + // resumed frame (as syncA's safe frame has not caught up to that frame yet) + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + CHECK(coordinator.safeFrameNumber() == 2); + + // Finally ensure that the next frame *does* tick the safe frame forward (as + // we have now gotten syncA's safe frame past the first resumed frame) + syncA.tickFrameAndSafe(); + coordinator.onFrameStart(&syncA); + CHECK(coordinator.safeFrameNumber() == 3); +} \ No newline at end of file