Vulkan: Correct logic for handling target and window size changes

Should fix a possible reference to deleted framebuffers, as well as fixing
the issues with the render area being correct if the game's source area
changes, or auto-scaling is enabled.

Source/Core/VideoBackends/Vulkan/Renderer.cpp

@@ -42,20 +42,25 @@
 
 namespace Vulkan
 {
-Renderer::Renderer()
+Renderer::Renderer(std::unique_ptr<SwapChain> swap_chain) : m_swap_chain(std::move(swap_chain))
 {
+  g_Config.bRunning = true;
+  UpdateActiveConfig();
+
   // Set to something invalid, forcing all states to be re-initialized.
   for (size_t i = 0; i < m_sampler_states.size(); i++)
     m_sampler_states[i].bits = std::numeric_limits<decltype(m_sampler_states[i].bits)>::max();
 
-  // Set default size so a decent EFB is created initially.
-  s_backbuffer_width = MAX_XFB_WIDTH;
-  s_backbuffer_height = MAX_XFB_HEIGHT;
+  // These have to be initialized before FramebufferManager is created.
+  // If running surfaceless, assume a window size of MAX_XFB_{WIDTH,HEIGHT}.
   FramebufferManagerBase::SetLastXfbWidth(MAX_XFB_WIDTH);
   FramebufferManagerBase::SetLastXfbHeight(MAX_XFB_HEIGHT);
-  PixelShaderManager::SetEfbScaleChanged();
+  s_backbuffer_width = m_swap_chain ? m_swap_chain->GetWidth() : MAX_XFB_WIDTH;
+  s_backbuffer_height = m_swap_chain ? m_swap_chain->GetHeight() : MAX_XFB_HEIGHT;
+  s_last_efb_scale = g_ActiveConfig.iEFBScale;
   UpdateDrawRectangle(s_backbuffer_width, s_backbuffer_height);
   CalculateTargetSize(s_backbuffer_width, s_backbuffer_height);
+  PixelShaderManager::SetEfbScaleChanged();
 }
 
 Renderer::~Renderer()
@@ -67,14 +72,9 @@ Renderer::~Renderer()
   DestroySemaphores();
 }
 
-bool Renderer::Initialize(FramebufferManager* framebuffer_mgr, void* window_handle,
-                          VkSurfaceKHR surface)
+bool Renderer::Initialize(FramebufferManager* framebuffer_mgr)
 {
   m_framebuffer_mgr = framebuffer_mgr;
-  g_Config.bRunning = true;
-  UpdateActiveConfig();
-
-  // Create state tracker, doesn't require any resources
   m_state_tracker = std::make_unique<StateTracker>();
   BindEFBToStateTracker();
 
@@ -112,24 +112,6 @@ bool Renderer::Initialize(FramebufferManager* framebuffer_mgr, void* window_hand
                                    m_bounding_box->GetGPUBufferSize());
   }
 
-  // Initialize annoying statics
-  s_last_efb_scale = g_ActiveConfig.iEFBScale;
-
-  // Create swap chain
-  if (surface)
-  {
-    // Update backbuffer dimensions
-    m_swap_chain = SwapChain::Create(window_handle, surface);
-    if (!m_swap_chain)
-    {
-      PanicAlert("Failed to create swap chain.");
-      return false;
-    }
-
-    // Update render rectangle etc.
-    OnSwapChainResized();
-  }
-
   // Various initialization routines will have executed commands on the command buffer.
   // Execute what we have done before beginning the first frame.
   g_command_buffer_mgr->PrepareToSubmitCommandBuffer();
@@ -494,9 +476,6 @@ void Renderer::SwapImpl(u32 xfb_addr, u32 fb_width, u32 fb_stride, u32 fb_height
   // Scale the source rectangle to the selected internal resolution.
   TargetRectangle source_rc = Renderer::ConvertEFBRectangle(rc);
 
-  // Target rectangle can change if the VI aspect ratio changes.
-  UpdateDrawRectangle(s_backbuffer_width, s_backbuffer_height);
-
   // Transition the EFB render target to a shader resource.
   VkRect2D src_region = {{0, 0},
                          {m_framebuffer_mgr->GetEFBWidth(), m_framebuffer_mgr->GetEFBHeight()}};
@@ -522,6 +501,10 @@ void Renderer::SwapImpl(u32 xfb_addr, u32 fb_width, u32 fb_stride, u32 fb_height
       StopFrameDump();
   }
 
+  // Restore the EFB color texture to color attachment ready for rendering the next frame.
+  m_framebuffer_mgr->GetEFBColorTexture()->TransitionToLayout(
+      g_command_buffer_mgr->GetCurrentCommandBuffer(), VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL);
+
   // Ensure the worker thread is not still submitting a previous command buffer.
   // In other words, the last frame has been submitted (otherwise the next call would
   // be a race, as the image may not have been consumed yet).
@@ -546,22 +529,28 @@ void Renderer::SwapImpl(u32 xfb_addr, u32 fb_width, u32 fb_stride, u32 fb_height
     g_command_buffer_mgr->SubmitCommandBuffer(true);
   }
 
+  // NOTE: It is important that no rendering calls are made to the EFB between submitting the
+  // (now-previous) frame and after the below config checks are completed. If the target size
+  // changes, as the resize methods to not defer the destruction of the framebuffer, the current
+  // command buffer will contain references to a now non-existent framebuffer.
+
   // Prep for the next frame (get command buffer ready) before doing anything else.
   BeginFrame();
 
-  // Restore the EFB color texture to color attachment ready for rendering.
-  m_framebuffer_mgr->GetEFBColorTexture()->TransitionToLayout(
-      g_command_buffer_mgr->GetCurrentCommandBuffer(), VK_IMAGE_LAYOUT_COLOR_ATTACHMENT_OPTIMAL);
+  // Determine what (if anything) has changed in the config.
+  CheckForConfigChanges();
 
-  // Clean up stale textures
-  TextureCache::Cleanup(frameCount);
-
-  // Handle window resizes.
-  CheckForTargetResize(fb_width, fb_stride, fb_height);
+  // Handle host window resizes.
   CheckForSurfaceChange();
 
-  // Determine what has changed in the config.
-  CheckForConfigChanges();
+  // Handle output size changes from the guest.
+  // There is a downside to doing this here is that if the game changes its XFB source area,
+  // the changes will be delayed by one frame. For the moment it has to be done here because
+  // this can cause a target size change, which would result in a black frame if done earlier.
+  CheckForTargetResize(fb_width, fb_stride, fb_height);
+
+  // Clean up stale textures.
+  TextureCacheBase::Cleanup(frameCount);
 }
 
 void Renderer::DrawScreen(const TargetRectangle& src_rect, const Texture2D* src_tex)
@@ -832,13 +821,23 @@ void Renderer::StopFrameDump()
 
 void Renderer::CheckForTargetResize(u32 fb_width, u32 fb_stride, u32 fb_height)
 {
-  if (FramebufferManagerBase::LastXfbWidth() != fb_stride ||
-      FramebufferManagerBase::LastXfbHeight() != fb_height)
+  if (FramebufferManagerBase::LastXfbWidth() == fb_stride &&
+      FramebufferManagerBase::LastXfbHeight() == fb_height)
   {
-    u32 last_w = (fb_stride < 1 || fb_stride > MAX_XFB_WIDTH) ? MAX_XFB_WIDTH : fb_stride;
-    u32 last_h = (fb_height < 1 || fb_height > MAX_XFB_HEIGHT) ? MAX_XFB_HEIGHT : fb_height;
-    FramebufferManagerBase::SetLastXfbWidth(last_w);
-    FramebufferManagerBase::SetLastXfbHeight(last_h);
+    return;
+  }
+
+  u32 new_width = (fb_stride < 1 || fb_stride > MAX_XFB_WIDTH) ? MAX_XFB_WIDTH : fb_stride;
+  u32 new_height = (fb_height < 1 || fb_height > MAX_XFB_HEIGHT) ? MAX_XFB_HEIGHT : fb_height;
+  FramebufferManagerBase::SetLastXfbWidth(new_width);
+  FramebufferManagerBase::SetLastXfbHeight(new_height);
+
+  // Changing the XFB source area will likely change the final drawing rectangle.
+  UpdateDrawRectangle(s_backbuffer_width, s_backbuffer_height);
+  if (CalculateTargetSize(s_backbuffer_width, s_backbuffer_height))
+  {
+    PixelShaderManager::SetEfbScaleChanged();
+    ResizeEFBTextures();
   }
 
   // This call is needed for auto-resizing to work.
@@ -853,9 +852,6 @@ void Renderer::CheckForSurfaceChange()
   u32 old_width = m_swap_chain ? m_swap_chain->GetWidth() : 0;
   u32 old_height = m_swap_chain ? m_swap_chain->GetHeight() : 0;
 
-  // Wait for the GPU to catch up since we're going to destroy the swap chain.
-  g_command_buffer_mgr->WaitForGPUIdle();
-
   // Fast path, if the surface handle is the same, the window has just been resized.
   if (m_swap_chain && s_new_surface_handle == m_swap_chain->GetNativeHandle())
   {
@@ -869,6 +865,9 @@ void Renderer::CheckForSurfaceChange()
   }
   else
   {
+    // Wait for the GPU to catch up since we're going to destroy the swap chain.
+    g_command_buffer_mgr->WaitForGPUIdle();
+
     // Did we previously have a swap chain?
     if (m_swap_chain)
     {
@@ -976,13 +975,12 @@ void Renderer::OnSwapChainResized()
 {
   s_backbuffer_width = m_swap_chain->GetWidth();
   s_backbuffer_height = m_swap_chain->GetHeight();
-  FramebufferManagerBase::SetLastXfbWidth(MAX_XFB_WIDTH);
-  FramebufferManagerBase::SetLastXfbHeight(MAX_XFB_HEIGHT);
   UpdateDrawRectangle(s_backbuffer_width, s_backbuffer_height);
   if (CalculateTargetSize(s_backbuffer_width, s_backbuffer_height))
+  {
+    PixelShaderManager::SetEfbScaleChanged();
     ResizeEFBTextures();
-
-  PixelShaderManager::SetEfbScaleChanged();
+  }
 }
 
 void Renderer::BindEFBToStateTracker()

Source/Core/VideoBackends/Vulkan/Renderer.h

@@ -25,13 +25,13 @@ class RasterFont;
 class Renderer : public ::Renderer
 {
 public:
-  Renderer();
+  Renderer(std::unique_ptr<SwapChain> swap_chain);
   ~Renderer();
 
   SwapChain* GetSwapChain() const { return m_swap_chain.get(); }
   StateTracker* GetStateTracker() const { return m_state_tracker.get(); }
   BoundingBox* GetBoundingBox() const { return m_bounding_box.get(); }
-  bool Initialize(FramebufferManager* framebuffer_mgr, void* window_handle, VkSurfaceKHR surface);
+  bool Initialize(FramebufferManager* framebuffer_mgr);
 
   void RenderText(const std::string& pstr, int left, int top, u32 color) override;
   u32 AccessEFB(EFBAccessType type, u32 x, u32 y, u32 poke_data) override;

Source/Core/VideoBackends/Vulkan/main.cpp

@@ -165,6 +165,18 @@ bool VideoBackend::Initialize(void* window_handle)
     return false;
   }
 
+  // Create swap chain. This has to be done early so that the target size is correct for auto-scale.
+  std::unique_ptr<SwapChain> swap_chain;
+  if (surface != VK_NULL_HANDLE)
+  {
+    swap_chain = SwapChain::Create(window_handle, surface);
+    if (!swap_chain)
+    {
+      PanicAlert("Failed to create Vulkan swap chain.");
+      return false;
+    }
+  }
+
   // Create command buffers. We do this separately because the other classes depend on it.
   g_command_buffer_mgr = std::make_unique<CommandBufferManager>(g_Config.bBackendMultithreading);
   if (!g_command_buffer_mgr->Initialize())
@@ -180,7 +192,7 @@ bool VideoBackend::Initialize(void* window_handle)
   // Create main wrapper instances.
   g_object_cache = std::make_unique<ObjectCache>();
   g_framebuffer_manager = std::make_unique<FramebufferManager>();
-  g_renderer = std::make_unique<Renderer>();
+  g_renderer = std::make_unique<Renderer>(std::move(swap_chain));
 
   // Cast to our wrapper classes, so we can call the init methods.
   Renderer* renderer = static_cast<Renderer*>(g_renderer.get());
@@ -191,7 +203,7 @@ bool VideoBackend::Initialize(void* window_handle)
   // These have to be done before the others because the destructors
   // for the remaining classes may call methods on these.
   if (!g_object_cache->Initialize() || !framebuffer_mgr->Initialize() ||
-      !renderer->Initialize(framebuffer_mgr, window_handle, surface))
+      !renderer->Initialize(framebuffer_mgr))
   {
     PanicAlert("Failed to initialize Vulkan classes.");
     g_renderer.reset();