From da33eab7765576f01c834871ef8a9029f002aab1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Thu, 22 Oct 2026 23:08:52 +0200 Subject: [PATCH] Outputs: Defensive checks, and don't wait for a Metal drawable twice in a frame - Metal: when a layer has no drawable to give (it can take a second to say so), don't ask again in the same frame for every pass to it. - Swapchains other than the primary are created, resized and destroyed between frames (Metal asserts it), and destroyed with the last reference to them (ReleaseAssertLast, Metal and Vulkan). - BindFramebufferSamplesAsTexture asserts it isn't given a swapchain, like BindFramebufferAsTexture. - An offscreen Metal swapchain can't be resized, asserted. - VulkanRenderManager asserts that no additional swapchain outlives it. - Native_RemoveOutputAndWait asserts it isn't called on the thread that handles the requests, where it would always time out. Co-Authored-By: Claude Opus 5.5 (1M context) --- Common/GPU/Metal/MetalRenderManager.h | 4 ++++ Common/GPU/Metal/MetalRenderManager.mm | 5 ++++- Common/GPU/Metal/thin3d_metal.mm | 10 +++++++++- Common/GPU/Vulkan/VulkanRenderManager.cpp | 2 ++ Common/GPU/Vulkan/thin3d_vulkan.cpp | 4 +++- UI/NativeApp.cpp | 4 ++++ 6 files changed, 26 insertions(+), 3 deletions(-) diff --git a/Common/GPU/Metal/MetalRenderManager.h b/Common/GPU/Metal/MetalRenderManager.h index b18d3e1c7f..f25aa48fca 100644 --- a/Common/GPU/Metal/MetalRenderManager.h +++ b/Common/GPU/Metal/MetalRenderManager.h @@ -235,6 +235,8 @@ struct MetalSwapchain { bool drawn = false; // Something has rendered to it. // Replay side. Acquired by the first pass that renders to it, presented at the end of the frame. id drawable; + // The replayed frame nextDrawable gave none in. Not asked again that frame, as each try can wait a second. + uint64_t noDrawableFrame = UINT64_MAX; }; struct MetalRecordedPass { @@ -383,6 +385,7 @@ public: void DestroySwapchain(MetalSwapchain *swapchain); void Resize(MetalSwapchain *swapchain, int width, int height); void Resize(int width, int height) { Resize(backbuffer_, width, height); } + bool InFrame() const { return inFrame_; } int BackbufferWidth() const { return backbuffer_->width; } int BackbufferHeight() const { return backbuffer_->height; } MTLPixelFormat BackbufferFormat() const { return MTLPixelFormatBGRA8Unorm; } @@ -556,6 +559,7 @@ private: id rtUploadBlit_; id rtUploadCompute_; std::vector rtAcquired_; // Swapchains with a drawable to present. + uint64_t replayFrame_ = 0; // Counts the presents replayed. // Replay scratch, reused between chunks. std::vector rtSteps_; size_t rtStepCount_ = 0; diff --git a/Common/GPU/Metal/MetalRenderManager.mm b/Common/GPU/Metal/MetalRenderManager.mm index ef936d9071..6ba5afa4cf 100644 --- a/Common/GPU/Metal/MetalRenderManager.mm +++ b/Common/GPU/Metal/MetalRenderManager.mm @@ -926,12 +926,14 @@ id MetalRenderManager::ReplaySwapchainTexture(MetalSwapchain *swapch if (!swapchain->layer) { return swapchain->offscreen; } - if (!swapchain->drawable) { + if (!swapchain->drawable && swapchain->noDrawableFrame != replayFrame_) { @autoreleasepool { swapchain->drawable = [swapchain->layer nextDrawable]; } if (swapchain->drawable) { rtAcquired_.push_back(swapchain); + } else { + swapchain->noDrawableFrame = replayFrame_; } } return swapchain->drawable ? swapchain->drawable.texture : nil; @@ -1039,6 +1041,7 @@ void MetalRenderManager::Replay(MetalCommandChunk *chunk) { swapchain->drawable = nil; } rtAcquired_.clear(); + replayFrame_++; } dispatch_semaphore_t frameSemaphore = chunk->end == MetalChunkEnd::PRESENT ? frameSemaphore_ : nullptr; diff --git a/Common/GPU/Metal/thin3d_metal.mm b/Common/GPU/Metal/thin3d_metal.mm index f64fbc8b4c..9b2466cefb 100644 --- a/Common/GPU/Metal/thin3d_metal.mm +++ b/Common/GPU/Metal/thin3d_metal.mm @@ -1247,6 +1247,7 @@ void MetalDrawContext::BindFramebufferAsTexture(Framebuffer *fbo, int binding, A void MetalDrawContext::BindFramebufferSamplesAsTexture(Framebuffer *fbo, int binding, int layer) { _dbg_assert_(binding < (int)MAX_TEXTURE_SLOTS); + _dbg_assert_(fbo && !fbo->IsSwapchain()); MetalFramebuffer *fb = (MetalFramebuffer *)fbo; boundTextures_[binding] = fb->msaaColor ? fb->msaaColor : fb->color; boundThin3DTextures_[binding] = nullptr; @@ -1263,6 +1264,8 @@ void MetalDrawContext::GetFramebufferDimensions(Framebuffer *fbo, int *w, int *h } Swapchain *MetalDrawContext::CreateSwapchain(const SwapchainDesc &desc, std::string *error) { + // Swapchains other than the primary come and go between frames. + _dbg_assert_(!rm_->InFrame()); if (desc.winsys != WINDOWSYSTEM_METAL_EXT) { *error = "Metal swapchains need a CAMetalLayer"; return nullptr; @@ -1278,15 +1281,20 @@ Swapchain *MetalDrawContext::CreateSwapchain(const SwapchainDesc &desc, std::str void MetalDrawContext::DestroySwapchain(Swapchain *swapchain) { _dbg_assert_(swapchain && swapchain != primarySwapchain_); + _dbg_assert_(!rm_->InFrame()); MetalSwapchainFB *fb = (MetalSwapchainFB *)swapchain; rm_->DestroySwapchain(fb->swapchain); fb->swapchain = nullptr; - fb->Release(); + // Nothing else may hold it, it no longer has a swapchain to be. + fb->ReleaseAssertLast(); } void MetalDrawContext::ResizeSwapchain(Swapchain *swapchain, int width, int height) { MetalSwapchain *metalSwapchain = ToSwapchain(swapchain); _dbg_assert_(metalSwapchain); + // An offscreen one keeps its size, as there's no layer to resize. + _dbg_assert_(metalSwapchain->layer || swapchain == primarySwapchain_); + _dbg_assert_(swapchain == primarySwapchain_ || !rm_->InFrame()); rm_->Resize(metalSwapchain, width, height); if (swapchain == primarySwapchain_) { primarySwapchain_->SetSize(metalSwapchain->width, metalSwapchain->height); diff --git a/Common/GPU/Vulkan/VulkanRenderManager.cpp b/Common/GPU/Vulkan/VulkanRenderManager.cpp index 0be3f64eae..069cf961c9 100644 --- a/Common/GPU/Vulkan/VulkanRenderManager.cpp +++ b/Common/GPU/Vulkan/VulkanRenderManager.cpp @@ -759,6 +759,8 @@ void VulkanRenderManager::CheckNothingPending() { VulkanRenderManager::~VulkanRenderManager() { INFO_LOG(Log::G3D, "VulkanRenderManager destructor"); + // Their owners (the outputs) destroy them first, on device loss. Left over, they'd outlive the device. + _dbg_assert_(extraSwapchains_.empty()); { std::unique_lock lock(compileQueueMutex_); diff --git a/Common/GPU/Vulkan/thin3d_vulkan.cpp b/Common/GPU/Vulkan/thin3d_vulkan.cpp index 358d40d7b5..1f5554d2f4 100644 --- a/Common/GPU/Vulkan/thin3d_vulkan.cpp +++ b/Common/GPU/Vulkan/thin3d_vulkan.cpp @@ -1999,6 +1999,7 @@ void VKContext::BindFramebufferAsTexture(Framebuffer *fbo, int binding, Aspect c } void VKContext::BindFramebufferSamplesAsTexture(Framebuffer *fbo, int binding, int layer) { + _dbg_assert_(fbo && !fbo->IsSwapchain()); VKFramebuffer *fb = (VKFramebuffer *)fbo; _assert_(binding >= 0 && binding < MAX_BOUND_TEXTURES); _assert_(fb != curFramebuffer_); @@ -2034,7 +2035,8 @@ void VKContext::DestroySwapchain(Swapchain *swapchain) { VKSwapchain *vkSwapchain = (VKSwapchain *)swapchain; renderManager_.DestroyExtraSwapchain(vkSwapchain->swapchain); vkSwapchain->swapchain = nullptr; - vkSwapchain->Release(); + // Nothing else may hold it, it no longer has a swapchain to be. + vkSwapchain->ReleaseAssertLast(); } void VKContext::ResizeSwapchain(Swapchain *swapchain, int width, int height) { diff --git a/UI/NativeApp.cpp b/UI/NativeApp.cpp index f944aee7cb..bbbf6cf386 100644 --- a/UI/NativeApp.cpp +++ b/UI/NativeApp.cpp @@ -217,6 +217,8 @@ static uint64_t g_outputRequestsQueued = 0; static uint64_t g_outputRequestsDone = 0; // Whether the outputs have swapchains, between NativeInitGraphics and NativeShutdownGraphics. static bool g_outputsLive = false; +// The thread that handles the requests, which mustn't wait for them. +static std::thread::id g_outputRequestThread; static std::atomic g_nextOutputId{1}; static void ProcessOutputRequests() { @@ -236,6 +238,7 @@ static void ProcessOutputRequests() { } std::lock_guard guard(g_outputRequestLock); g_outputRequestsDone = queued; + g_outputRequestThread = std::this_thread::get_id(); g_outputRequestsDoneCond.notify_all(); } @@ -2098,6 +2101,7 @@ void Native_RemoveOutput(int id) { bool Native_RemoveOutputAndWait(int id) { Native_RemoveOutput(id); std::unique_lock lock(g_outputRequestLock); + _dbg_assert_msg_(std::this_thread::get_id() != g_outputRequestThread, "Waiting for output requests on the thread that handles them"); const uint64_t queued = g_outputRequestsQueued; // Without a device, the outputs have no swapchains to let go of. const bool done = g_outputRequestsDoneCond.wait_for(lock, std::chrono::seconds(3), [=] {