Vulkan backend: Fix the use of the incorrect fence in wait for fence.

[FunMiles]

The fence being waited upon was not the one associated with the current frame.
This results in validation error detecting a reset of command buffers still in
use and resetting fences while still in use.

[ocornut]

Hello,

I have installed latest VulkanSDK 1.2.176.1 on Windows and can't reproduce a validation error without your PR.
Could you clarify where/what does happen?

I don't fully understand that code but it seemed to be like it was waiting in the case that the resources used by the last frame that used this FrameData was made available again?

[FunMiles]

The issue will show up in validation only under some load situation that is hard to systematically reproduce. It will not happen I think if you haven't pulled a window outside of the main rendering window.

Furthermore, even when validation errors happen, they do not necessarily happen for every frame. But here is a summary of the problem. There are two frames. Without the PR, the lines:

		for (;;)
		{
			err = vkWaitForFences(v->Device, 1, &fd->Fence, VK_TRUE, 100);
			if (err == VK_SUCCESS) break;
			if (err == VK_TIMEOUT) continue;
			check_vk_result(err);
		}

we are waiting for the fence of let's say frame 0. Then the third line in the next block switch which frame fd refers to:

fd = &wd->Frames[wd->FrameIndex];

Which is now frame 1. And then the frame is being used:

err = vkResetCommandPool(v->Device, fd->CommandPool, 0);

But we've waited for the wrong fence, the command buffer of the command pool fd->CommandPool may still be busy. So I randomly get a validation error saying the command buffer is in use.
And further down, we reset a fence that can still be busy and submit the new command buffer with a the same fence object that is still busy, hence I see random validation errors on these two lines:

			err = vkResetFences(v->Device, 1, &fd->Fence);
			err = vkQueueSubmit(v->Queue, 1, &info, fd->Fence);

With the PR, the fence we wait for is the one with the buffer we want to use and it is the fence that is used in all the lines that I have highlighted, so there cannot be an inconsistency anymore.

I'll admit to not fully understand some details but it could be due to a misunderstanding on how FrameIndex is updated when some window is outside the main rendering window. The detail I do not understand is that it seems that without the PR, we wait for the last submitted command buffer, so when we go the next, it should have been waited for already.
However, there is no reason to wait for the just submitted command buffer to become available. We just need to wait for the previous one so we can use it. Thus in all cases, it seems to me that the PR is a performance optimization as well.

I hope my explanations clarify things enough.

[Donione]

we are waiting for the fence of let's say frame 0. Then the third line in the next block switch which frame fd refers to:

fd = &wd->Frames[wd->FrameIndex];

Which is now frame 1. And then the frame is being used:

I believe this is not true.
vkAcquireNextImageKHR will return into wd->FrameIndex the index of next available presentable image, it will most likely be the same index that was already there.
I belive the line

fd = &wd->Frames[wd->FrameIndex];

is there for the case when vkAcquireNextImageKHR would return an out-of-order index. This happens when the window is being resized, for example.

Notice that the FrameIndex is also used as a presentIndex when we swap buffers and is then incremented for the next frame.

That being said, I might be wrong, I'm new to Vulkan.

[FunMiles]

Aha!
I had not seen the fact that vkAcquireNextImageKHR can update the wd->FrameIndex.
Thanks for solving my not fully understanding the way wd->FrameIndex is changed. There are two places. One is the direct increment and does not cause an issue but the second way is by this call.

Following this understanding, I have put back the code in my test to the original and added a check to see if there was a correlation between the validation error and when vkAcquireNextImageKHR does change the FrameIndex.
The result is that there is complete and full correlation. But the change is random, sometimes it happens, sometime it does not. However, as stated in the correlation statement, every time it changes, the validation error happens.
It is likely that this randomness is related to timing of the request, vs the graphic card's state as to which image is available and how the Vulkan implementation deals with images in the swap chain. I am seeing the issue on Mac OS with MoltenVK.

My statement that you highlighted is still true. If vkAcquireNextImageKHR changes the frame index, we end up having waited for the wrong fence! And that causes the validation issue.
In any case, swapping the fence waiting and the getting the next image is certainly cleaner and clearer, and as demonstrated on my machine, safer. It does guarantee that the fence and command buffer are ready to be safely reset/reused.

Edit: Timing seems critical. I think it is also related to the newer MoltenVk. Now, frames are being rendered faster than 60fps while the monitor is at 60fps. In the past this would not have happened. I think I tracked the cause. The available presentation modes with this version of MoltenVk are only IMMEDIATE and FIFO. ImGui choses FIFO IMMEDIATE and that may explain why suddenly the frame is being changed by vkAcquireNextImageKHR. Perhaps you can try to force your Windows code to pick FIFO IMMEDIATE and see if you now see the validation error (assuming you've enabled the validation layers).

[Donione]

I think you're right. When we get an out-of-order index we have waited for a wrong fence and we need to wait on the correct one. Your solution seems to do the trick. In this tutorial (scroll near the end, there's a snippet from a drawFrame function) they do it a little differently but the idea is the same, I think.
I haven't been able to reproduce your issue on Windows with latest Vulkan SDK and FIFO present mode

[FunMiles]

I made a mistake when I said ImGui choses FIFO. It choses IMMEDIATE because in the list of preferred mode, IMMEDIATE precedes FIFO. Try that to see if you get the issue. Not FIFO.
I think IMMEDIATE is fine, even if the specs says it may cause tearing. The GUI that's in a separate windows is --in most cases-- not going to have a lot of animation in it. FIFO might slow down rendering, as the acquisition of the next image might not be available until the next v-sync.

[ocornut]

Thank you @FunMiles and @Donione for investigating this.
This is now merged: 90deb09 and I added references to this in the commit.

It also makes sense that this code in ImGui_ImplVulkan_RenderWindow() (used for secondary viewports entirely managed by the backends) now matches the equivalent logic in the FrameRender() function of both main.cpp (which are meant to demonstrate the user-side creation of an initial/main platform window).

Also note that those main.cpp also handle vkAcquireNextImageKHR() returning VK_SUBOPTIMAL_KHR (see #3881) and VK_ERROR_OUT_OF_DATE_KHR (see b73305b) and I think we will want ImGui_ImplVulkan_RenderWindow() to handle them eventually (could be a new PR).

[FunMiles]

Also note that those main.cpp also handle vkAcquireNextImageKHR() returning VK_SUBOPTIMAL_KHR (see #3881) and VK_ERROR_OUT_OF_DATE_KHR (see b73305b) and I think we will want ImGui_ImplVulkan_RenderWindow() to handle them eventually (could be a new PR).

You are absolutely correct. I have not checked for that. The VK_ERROR_OUT_OF_DATE_KHR should happen when the window is resized. I will check for that and try to address it for another PR.