search

ocornut / imgui

Dear ImGui: Bloat-free Graphical User interface for C++ with minimal dependencies
MIT License
60.37k stars 10.21k forks source link

WebGPU backend leaks when using ImGui::Image #8027

Open Jairard opened 1 week ago

Jairard commented 1 week ago

Version/Branch of Dear ImGui:

Version 1.90.4, Branch: docking

Back-ends:

imgui_impl_wgpu.cpp + imgui_impl_glfw.cpp

Compiler, OS:

Windows 10 + clang-cl 18.1.8

Full config/build information:

Dear ImGui 1.90.4 (19040)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 2, sizeof(ImDrawVert): 20
define: __cplusplus=202400
define: _WIN32
define: _WIN64
define: _MSC_VER=1939
define: _MSVC_LANG=202004
define: __clang_version__=17.0.6 
define: IMGUI_HAS_VIEWPORT
define: IMGUI_HAS_DOCK
--------------------------------
io.BackendPlatformName: imgui_impl_glfw
io.BackendRendererName: imgui_impl_webgpu
io.ConfigFlags: 0x00000000
io.ConfigViewportsNoDecoration
io.ConfigInputTextCursorBlink
io.ConfigWindowsResizeFromEdges
io.ConfigMemoryCompactTimer = 60.0
io.BackendFlags: 0x00000C0E
 HasMouseCursors
 HasSetMousePos
 PlatformHasViewports
 HasMouseHoveredViewport
 RendererHasVtxOffset
--------------------------------
io.Fonts: 1 fonts, Flags: 0x00000000, TexSize: 512,64
io.DisplaySize: 640.00,480.00
io.DisplayFramebufferScale: 1.00,1.00
--------------------------------
style.WindowPadding: 8.00,8.00
style.WindowBorderSize: 1.00
style.FramePadding: 4.00,3.00
style.FrameRounding: 0.00
style.FrameBorderSize: 0.00
style.ItemSpacing: 8.00,4.00
style.ItemInnerSpacing: 4.00,4.00

Details:

Whenever an ImGui::Image is used with WebGPU backend, memory leaks are reported

Here is the output I get when running the minimal code sample and closing the window:

D3D12 WARNING: Process is terminating. Using simple reporting. Please call ReportLiveObjects() at runtime for standard reporting. [ STATE_CREATION WARNING #0: UNKNOWN]
D3D12 WARNING: Live Producer at 0x0000027177755518, Refcount: 6. [ STATE_CREATION WARNING #0: UNKNOWN]
D3D12 WARNING:  Live Object at 0x0000027177896250, Refcount: 0. [ STATE_CREATION WARNING #0: UNKNOWN]
D3D12 WARNING:  Live Object at 0x00000271777E6870, Refcount: 0. [ STATE_CREATION WARNING #0: UNKNOWN]
D3D12 WARNING:  Live Object at 0x000002717781AF00, Refcount: 0. [ STATE_CREATION WARNING #0: UNKNOWN]
D3D12 WARNING:  Live Object at 0x0000027174F25F30, Refcount: 0. [ STATE_CREATION WARNING #0: UNKNOWN]
D3D12 WARNING:  Live Object at 0x000002717774BBA0, Refcount: 0. [ STATE_CREATION WARNING #0: UNKNOWN]
D3D12 WARNING:  Live Object at 0x00000271777E88B0, Refcount: 0. [ STATE_CREATION WARNING #0: UNKNOWN]
D3D12 WARNING:  Live Object at 0x000002717774C320, Refcount: 0. [ STATE_CREATION WARNING #0: UNKNOWN]
D3D12 WARNING:  Live Object at 0x0000027174C7C700, Refcount: 0. [ STATE_CREATION WARNING #0: UNKNOWN]
D3D12 WARNING:  Live Object at 0x000002717774DD50, Refcount: 1. [ STATE_CREATION WARNING #0: UNKNOWN]
D3D12 WARNING:  Live Object at 0x0000027174F25310, Refcount: 1. [ STATE_CREATION WARNING #0: UNKNOWN]
D3D12 WARNING:  Live Object at 0x0000027177953050, Refcount: 0. [ STATE_CREATION WARNING #0: UNKNOWN]
D3D12 WARNING:  Live Object at 0x00000271779537D0, Refcount: 1. [ STATE_CREATION WARNING #0: UNKNOWN]

As soon as I comment the ImGui::Image line, this log disappears.

Investigation

I investigated the imgui_impl_wgpu.cpp file and it appears that the WGPUBindGroups contained in ImageBindGroups (of the RenderResources structure) are not released in static void SafeRelease(RenderResources& res), that is called by ImGui_ImplWGPU_Shutdown().

Initially, I stumbled upon this while having the issue mentioned reported in https://github.com/ocornut/imgui/issues/7765. It appears that clearing the WGPUBindGroups each frame could also solve the issue, if it is done at the end of the call to ImGui_ImplWGPU_RenderDrawData, but I think the groups need to survive longer than this. Anyway, the SafeRelease functions still do not do their job properly.

That's why I chose to open a distinct issue.

Local hack to fix the issue

Locally, I hacked my way through the issue but in a very ugly way. I expose the code here in order to show my way of thinking about this issue:

// imgui_mock is a namespace where I copy-pasted structs that are only defined in imgui_impl_wgpu.cpp
static auto clear_image_bind_groups() -> void {
    imgui_mock::ImGui_ImplWGPU_Data & bd =
        *reinterpret_cast<imgui_mock::ImGui_ImplWGPU_Data *>(ImGui::GetIO().BackendRendererUserData);
    auto & storage = bd.renderResources.ImageBindGroups;
    // This group is already released but stored in the same place
    auto const font_bind_group = bd.renderResources.ImageBindGroup;

    for (auto const & pair : storage.Data) {
        if (pair.val_p != nullptr && pair.val_p != font_bind_group)
            wgpuBindGroupRelease(reinterpret_cast<WGPUBindGroup>(pair.val_p));
    }

    storage.Clear();
}

void render_frame()
{
    // Configure everything
    WGPURenderPassEncoder render_pass = /* ... */;

    // Draw ImGui data
    ImGui::Render();
    ImGui_ImplWGPU_RenderDrawData(ImGui::GetDrawData(), render_pass);

    // Other rendering stuff
    wgpuQueueSubmit(/* ... */);
    wgpuSurfacePresent(/* ... */);

    // When everything is done, clean bind groups
    clear_image_bind_groups();
}

Fix proposal

I do believe that adding some cleaning code in SafeRelease would fix the issue in a clean way, something that would look like that:

static void SafeRelease(RenderResources& res)
{
    SafeRelease(res.FontTexture);
    SafeRelease(res.FontTextureView);
    SafeRelease(res.Sampler);
    SafeRelease(res.Uniforms);
    SafeRelease(res.CommonBindGroup);

    // Add this loop
    for (int i = 0; i < res.ImageBindGroups.Data.Size; i++)
    {
        if (res.ImageBindGroups.Data[i].val_p != res.ImageBindGroup)
            SafeRelease((WGPUBindGroup)res.ImageBindGroups.Data[i].val_p);
    }

    SafeRelease(res.ImageBindGroup);
    SafeRelease(res.ImageBindGroupLayout);
};

Note that we avoid releasing twice the WGPUBindGroup that is both stored in ImageBindGroup and ImageBindGroups. This bind group is used for the default font atlas and is correctly released. When calling ImGui::ShowDemoWindow(), there is no leak because the only call to ImGui::Image is to display the default font atlas.

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

// At init time, create or load a texture:
WGPUTextureView my_texture_view = /* ... */;

// In main loop, call ImGui::Image
ImGui::Begin("Example Bug");
ImGui::Image((void *)my_texture_view, ImVec2(my_image_width, my_image_height));
ImGui::End();
Jairard commented 2 days ago

Should I propose a PR ?

ocornut commented 2 days ago

Yes that would be good. One that possibly tackle this and #7765 (if meaningful you can split in multiple commits). I don't know WGPU very well so any explanation is useful as we tend to return back to those issues later.

Note that it is legal for other backends call RenderDrawData() multiple time so I wouldn't perform any clear there. If anything they can be in backend NewFrame() and Shutdown() function if needed.

Jairard commented 2 days ago

Ok, so I could tackle both issues.

I tried implementing the solution you proposed in https://github.com/ocornut/imgui/issues/7765#issuecomment-2206436309, in order to remove the permanent storage of WGPUBindGroups. Unfortunately, it's not feasible because we would have to release the WGPUBindGroups in RenderDrawData(), but they need to stay alive longer.

So the solution I went with keeps the permanent storage but clears it in NewFrame() as it was also suggested. An additional cleaning is added in Shutdown() via the SafeRelease() functions to get rid of the WGPUBindGroups allocated in the last frame. As a side effect, I could remove the ImageBindGroup member of RenderResources to make things more clear.

As for a global explanation, I think it is pretty clear in https://github.com/ocornut/imgui/issues/7765 but I can try to sum it up here. In the backend, we provide a way to draw a texture via its WGPUTextureView. But we can't keep this view in memory because the user can release it at any point and we have no way to notify this to the backend (and it's no wishable to introduce one as it would be error-prone). Moreover, the user can release a WGPUTextureView and create a new one, which could have the exact same value since on WebGPU's side, the previous one doesn't exist anymore so the new one can be allocated with the same id. Hence we need to clear the backend cache before each new frame. This cache is an ImGuiStorage that contains WGPUBindGroups referenced by the hash of the corresponding WGPUTextureView. So in order to clear it we need to release each WGPUBindGroup and call the Clear() method. This way, we recreate the bind groups as needed by each frame.

N.B: PR incoming, I just wanted to post this before