Font Texture Atlas leaks when ImGui_ImplVulkan_CreateFontsTexture is called more than once

[guybrush77]

Version/Branch of Dear ImGui:

Version: 1.79
Branch: v1.79

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_vulkan.h + imgui_impl_glfw.cpp
Compiler: VS 2019
Operating System: Windows 10

My Issue/Question:

I would like my application to correctly handle GUI scaling. The current guideline suggests: "For correct scaling, prefer to reload font + rebuild ImFontAtlas + call style.ScaleAllSizes()".

To rebuild the texture atlas, I use ImGui_ImplVulkan_CreateFontsTexture() function. This works if the function is called just once. However, if it is called more than once, the old texture atlas leaks (g_FontImage, g_FontView, g_FontMemory are over-written).

One way to get around this would be to call ImGui_ImplVulkan_Shutdown, and then recreate Vulkan context from scratch. But this seems quite heavy/excessive.

It would be nice to have a function that destroys the font atlas. This function could be called explicitly by the user, or it could be called implicitly when invoking ImGui_ImplVulkan_CreateFontsTexture . For example:

void ImGui_ImplVulkan_DestroyFontsTexture()
{
    ImGui_ImplVulkan_InitInfo* v = &g_VulkanInitInfo;

    if (g_FontView) {
        vkDestroyImageView(v->Device, g_FontView, v->Allocator);
        g_FontView = VK_NULL_HANDLE;
    }
    if (g_FontImage) {
        vkDestroyImage(v->Device, g_FontImage, v->Allocator);
        g_FontImage = VK_NULL_HANDLE;
    }
    if (g_FontMemory) {
        vkFreeMemory(v->Device, g_FontMemory, v->Allocator);
        g_FontMemory = VK_NULL_HANDLE;
    }
}

[ocornut]

You are absolutely right we need that function in.
Presently I'm not familiar enough with Vulkan to provide or use this correctly.
ImGui_ImplVulkan_CreateFontsTexture() stores the the value of g_FontSampler, g_FontView into a descriptor would that need cleaning? If I naively create a reload test before NewFrame()

static int font_reload = -1;
if (font_reload != -1)
{
    io.Fonts->Clear();
    if (font_reload == 0)
        io.Fonts->AddFontDefault();
    if (font_reload == 1)
        io.Fonts->AddFontFromFileTTF("../../misc/fonts/Roboto-Medium.ttf", 16.0f);
    if (font_reload == 2)
        io.Fonts->AddFontFromFileTTF("../../misc/fonts/Cousine-Regular.ttf", 15.0f);
    if (font_reload == 3)
        io.Fonts->AddFontFromFileTTF("../../misc/fonts/DroidSans.ttf", 16.0f);

    VkResult err = vkDeviceWaitIdle(g_Device);
    VkCommandBuffer command_buffer = wd->Frames[wd->FrameIndex].CommandBuffer;
    ImGui_ImplVulkan_DestroyFontsTexture();
    ImGui_ImplVulkan_CreateFontsTexture(command_buffer);
    font_reload = -1;
}

The call to vkCmdPipelineBarrier() in ImGui_ImplVulkan_CreateFontsTexture() gives me:

[vulkan] Debug report from ObjectType: 6
Message:  [ UNASSIGNED-CoreValidation-DrawState-InvalidCommandBuffer-VkImageView ] Object: 0x20cd9706450 (Type = 6) | You are adding vkCmdPipelineBarrier() to VkCommandBuffer 0x20cd9706450[] that is invalid because bound VkImageView 0xdcba38000000001d[] was destroyed.

I didn't investigate more than that. If you can provide a PR with ImGui_ImplVulkan_DestroyFontsTexture() that pass validation + a way to use it I can integrate it.

There are few old related issues #914 #2697 which needs solving together and maybe this can come with it.

Also note that the shadows branch has experimental work to make it a general feature in backend to easily reload texture. Ref 3519b9c 4e7239c

[guybrush77]

Hello Omar,

Sorry for not getting back to you sooner.

This is what the ImGui_ImplVulkan_CreateFontsTexture function does:

1. Get actual font atlas data (pixels)
2. Create a new device texture and allocate memory for it (g_FontImage and g_FontMemory)
3. Create a new texture view (g_FontView)
4. Update the descriptor set (g_DescriptorSet)
5. Create a temporary upload buffer (g_UploadBuffer and g_UploadBufferMemory)
6. Copy pixels to the upload buffer
7. Record commands that transfer the font atlas data from the upload buffer (g_UploadBuffer) to the device texture (g_FontImage); these commands are recorded into a command buffer supplied by the user.

In your example, you use the per-frame command buffer. This buffer has already been recorded and it holds a reference to the g_FontImage. Calling ImGui_ImplVulkan_DestroyFontsTexture produces a dangling reference in that command buffer. This is not a problem as long as we don't use the command buffer in its current (invalid) state. But a call to ImGui_ImplVulkan_CreateFontsTexture attempts to record commands into it (step 7). At this point, the validation layer detects that the command buffer is no longer in a valid state and produces the InvalidCommandBuffer error.

We can get around this, but I think it would be cleaner to side-step the issue, and create a temporary command buffer. Its only purpose will be to upload the font atlas to the device. The code might look like this:

void UploadFontsTexture(VkDevice device, uint32_t queue_family_index, VkQueue queue, VkAllocationCallbacks allocator)
{
    // Destroy the old fonts texture (if any) to avoid resource leaks in steps 2 and 3
    ImGui_ImplVulkan_DestroyFontsTexture();

    // Create temporary command pool
    VkCommandPoolCreateInfo command_pool_info = {};
    command_pool_info.sType                   = VK_STRUCTURE_TYPE_COMMAND_POOL_CREATE_INFO;
    command_pool_info.flags                   = VK_COMMAND_POOL_CREATE_TRANSIENT_BIT;
    command_pool_info.queueFamilyIndex        = queue_family_index;

    VkCommandPool command_pool;
    vkCreateCommandPool(device, &command_pool_info, allocator, &command_pool);

    // Allocate temporary command buffer
    VkCommandBufferAllocateInfo command_buffer_info = {};
    command_buffer_info.sType                       = VK_STRUCTURE_TYPE_COMMAND_BUFFER_ALLOCATE_INFO;
    command_buffer_info.commandPool                 = command_pool;
    command_buffer_info.level                       = VK_COMMAND_BUFFER_LEVEL_PRIMARY;
    command_buffer_info.commandBufferCount          = 1;

    VkCommandBuffer command_buffer;
    vkAllocateCommandBuffers(device, &command_buffer_info, &command_buffer);

    // Begin recording
    VkCommandBufferBeginInfo begin_info = {};
    begin_info.sType                    = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO;
    begin_info.flags                    = VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT;

    vkBeginCommandBuffer(command_buffer, &begin_info);

    // Create new fonts texture, update descriptor, and record transfer commands
    ImGui_ImplVulkan_CreateFontsTexture(command_buffer);

    // End recording
    vkEndCommandBuffer(command_buffer);

    // Actually transfer the texture to the device
    VkSubmitInfo submit_info       = {};
    submit_info.sType              = VK_STRUCTURE_TYPE_SUBMIT_INFO;
    submit_info.commandBufferCount = 1;
    submit_info.pCommandBuffers    = &command_buffer;

    vkQueueSubmit(queue, 1, &submit_info, NULL);

    // Wait until the texture transfer is finished
    vkDeviceWaitIdle(device);

    // Destroy the temporary command pool
    vkDestroyCommandPool(device, command_pool, allocator);

    // Destroy the temporary upload buffer
    ImGui_ImplVulkan_DestroyFontUploadObjects();
}

I have omitted error checking for clarity. Now your example should work:

static int font_reload = -1;
if (font_reload != -1)
{
    io.Fonts->Clear();
    if (font_reload == 0)
        io.Fonts->AddFontDefault();
    if (font_reload == 1)
        io.Fonts->AddFontFromFileTTF("../../misc/fonts/Roboto-Medium.ttf", 16.0f);
    if (font_reload == 2)
        io.Fonts->AddFontFromFileTTF("../../misc/fonts/Cousine-Regular.ttf", 15.0f);
    if (font_reload == 3)
        io.Fonts->AddFontFromFileTTF("../../misc/fonts/DroidSans.ttf", 16.0f);

    // Same queue_family_index, queue, and allocator as previously passed to ImGui_ImplGlfw_InitForVulkan
    UploadFontsTexture(g_device, queue_family_index, queue, allocator);
    font_reload = -1;
}

You other concerns relate to g_FontView and g_FontSampler. ImGui_ImplVulkan_CreateFontsTexture creates a new font view, updates g_FontView and updates the descriptor. This is fine and, in fact, necessary. We would not want the descriptor to refer to the old (destroyed) font view. If you look at steps 1 - 7, ImGui_ImplVulkan_CreateFontsTexture does not create a new sampler; there is no danger of resource leak. We would only want to re-create the sampler if the font texture sampling depends on font size. Looking at ImGui_ImplVulkan_CreateFontSampler, this is not the case.

[ocornut]

@guybrush77 I apologize for my late answer to this. Thank you for your thoughtful post.
Would you be able to make a PR for it?

[thomasherzog]

The problem occurs because the command buffer with the bound image view may still be in use. To solve the problem, we would have to wait for all command buffers, which are not prerecorded according to the implementation, to finish execution. Can we assume that the command buffers are submitted to the queue inside ImGui_ImplVulkan_InitInfo? If this was the case, we could simply wait for the queue to finish any execution.

Unless I've missed something, you wouldn't even need a temporary command pool, since you generally create a temporary one for the execution of ImGui_ImplVulkan_CreateFontsTexture. I was able to solve the validation layers issues with vkQueueWaitIdle, but assuming that the main rendering process is running on the queue inside ImGui_ImplVulkan_InitInfo. Otherwise you would have to wait for the device with vkDeviceWaitIdle.

I opened the pull request rather quickly (unfortunately too quickly) as I found the problem to be a very large and breaking one. I would be willing to work on the pull request and fix any issues, depending on whether @guybrush77 would be willing to create a new pull request. Then I would close it without hesitation of course. Apologies for any inconvenience caused by my pull request!

[ocornut]

See fixes and simplification here:
#6943 (comment)
Thanks everyone !