From 63c1ba294aea636ea28b3056438ba40485a0fc82 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Wed, 23 Sep 2026 20:30:11 +0000 Subject: [PATCH] fix: keep a path-tracked texture published across UpdateTexture's recreate [patch] UpdateTexture's delete-and-recreate fallback routed through DeleteTexture, which drops every cache entry pointing at the deleted handle - including the entry GetOrLoadTexture published for that path - and then never restored it. The texture stayed live on the GPU while TryGetTexture reported it missing, the next GetOrLoadTexture uploaded a duplicate, and CleanupAllTextures, which walks Textures.Keys, could no longer reach either handle to free it. The fallback now captures the keys the instance is published under, by reference equality so only its own entries come back, and restores them after the recreate. Capture, delete, recreate and republish run inside one Invoker.Invoke for the same reason GetOrLoadTexture publishes inside its own: between the delete and the republish the path is absent from the cache, so a concurrent load would otherwise miss and upload a second GPU texture. Fixes #450 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AjdYTWFr5EVfbmSobYcw3G --- ImGui.App/ImGuiApp.cs | 48 ++++++++++--- tests/ImGui.App.Tests/ImGuiAppTests.cs | 97 ++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 8 deletions(-) diff --git a/ImGui.App/ImGuiApp.cs b/ImGui.App/ImGuiApp.cs index 0df85bb4..2163d86f 100644 --- a/ImGui.App/ImGuiApp.cs +++ b/ImGui.App/ImGuiApp.cs @@ -1907,7 +1907,10 @@ public static ImGuiAppTextureInfo CreateTexture(ReadOnlySpan rgba, int wid /// Texture height in pixels. /// /// Falls back to deleting and recreating the texture when the active renderer backend cannot update - /// in place, mutating to carry the new handle. + /// in place, mutating to carry the new handle. A texture that came + /// from stays published in the path-keyed cache + /// across that fallback, so + /// keeps finding it and CleanupAllTextures keeps owning it. /// /// is null. /// The span length does not equal width * height * 4. @@ -1923,15 +1926,44 @@ public static void UpdateTexture(ImGuiAppTextureInfo textureInfo, ReadOnlySpan { - textureInfo.TextureRef = new ImTextureRef(default, textureInfo.TextureId); - } + // DeleteTexture removes every cache entry whose handle matches the one being deleted, + // which for a path-tracked texture is the entry GetOrLoadTexture published. The recreate + // below gives textureInfo a live handle again but restores no entry, so without this + // republish the texture stays on the GPU while TryGetTexture reports it missing, the next + // GetOrLoadTexture uploads a duplicate, and CleanupAllTextures - which walks + // Textures.Keys - can no longer reach either handle to free it. + // + // Reference equality is the test rather than a handle comparison: only the entry this + // instance is actually published under should come back, never one that a different + // ImGuiAppTextureInfo happens to share a handle with. + List publishedKeys = [.. Textures.Where(entry => ReferenceEquals(entry.Value, textureInfo)).Select(entry => entry.Key)]; + + DeleteTexture(textureInfo.TextureId); + textureInfo.TextureId = UploadTextureRGBA(pixels, width, height); + unsafe + { + textureInfo.TextureRef = new ImTextureRef(default, textureInfo.TextureId); + } + + textureInfo.Width = width; + textureInfo.Height = height; - textureInfo.Width = width; - textureInfo.Height = height; + foreach (AbsoluteFilePath key in publishedKeys) + { + Textures[key] = textureInfo; + } + }); } private static void ValidatePixelSpan(ReadOnlySpan rgba, int width, int height) diff --git a/tests/ImGui.App.Tests/ImGuiAppTests.cs b/tests/ImGui.App.Tests/ImGuiAppTests.cs index 129131f0..e859c3c5 100644 --- a/tests/ImGui.App.Tests/ImGuiAppTests.cs +++ b/tests/ImGui.App.Tests/ImGuiAppTests.cs @@ -702,6 +702,103 @@ public void UpdateTexture_WhenBackendDeclines_FallsBackToRecreate() Assert.AreEqual(2, backend.CreateTextureCallCount, "A declined update must recreate the texture"); } + // Writes a decodable PNG to a temp file and returns its path. The caller deletes it. + private static string WriteTempPng(int width, int height) + { + byte[] pixels = new byte[width * height * 4]; + Array.Fill(pixels, (byte)0xFF); + byte[] png = TestImageBuilder.Png(width, height, colorType: 6, bitDepth: 8, pixels); + string file = Path.Join(Path.GetTempPath(), $"{Guid.NewGuid():N}.png"); + File.WriteAllBytes(file, png); + return file; + } + + [TestMethod] + public void UpdateTexture_OnAPathTrackedTexture_KeepsItPublishedInTheCache() + { + // UpdateTexture's recreate fallback went through DeleteTexture, which drops every cache entry + // pointing at the deleted handle - including the entry GetOrLoadTexture published for this + // path - and then never restored it. The texture stayed live on the GPU but became invisible + // to TryGetTexture, so callers were told a loaded texture was missing. + ResetState(); + ImGuiApp.Invoker = new Invoker.Invoker(); + FakeRendererBackend backend = new() { NextHandle = 4242 }; + ImGuiApp.renderer = backend; + ImGuiApp.controller = null; + + string file = WriteTempPng(2, 2); + try + { + AbsoluteFilePath path = file.As(); + ImGuiAppTextureInfo info = ImGuiApp.GetOrLoadTexture(path); + + // A size change forces the delete/recreate fallback regardless of what the backend can do + // in place, which is the path the issue reports. + ImGuiApp.UpdateTexture(info, new byte[1 * 1 * 4], 1, 1); + + Assert.IsTrue(ImGuiApp.TryGetTexture(path, out ImGuiAppTextureInfo? cached), "A path-tracked texture must stay published after a resize"); + Assert.AreSame(info, cached, "The cache should still hold the instance the caller is using"); + Assert.AreEqual(1, cached!.Width, "The published entry must carry the new size"); + Assert.AreEqual(1, cached.Height, "The published entry must carry the new size"); + Assert.AreEqual(backend.NextHandle, cached.TextureId, "The published entry must carry the recreated handle"); + } + finally + { + File.Delete(file); + } + } + + [TestMethod] + public void UpdateTexture_OnAPathTrackedTexture_DoesNotUploadADuplicateOnTheNextLoad() + { + // The downstream cost of the desync, which the cache-presence assertion above does not by + // itself pin: with the path evicted, the next GetOrLoadTexture missed, re-decoded the file and + // uploaded a second GPU texture, leaving the first one reachable only through the caller's own + // ImGuiAppTextureInfo. CleanupAllTextures walks Textures.Keys, so whichever handle was not + // published could never be freed - the leak the issue reports. + ResetState(); + ImGuiApp.Invoker = new Invoker.Invoker(); + FakeRendererBackend backend = new() { NextHandle = 4242 }; + ImGuiApp.renderer = backend; + ImGuiApp.controller = null; + + string file = WriteTempPng(2, 2); + try + { + AbsoluteFilePath path = file.As(); + ImGuiAppTextureInfo info = ImGuiApp.GetOrLoadTexture(path); + ImGuiApp.UpdateTexture(info, new byte[1 * 1 * 4], 1, 1); + + int uploadsAfterUpdate = backend.CreateTextureCallCount; + ImGuiAppTextureInfo reloaded = ImGuiApp.GetOrLoadTexture(path); + + Assert.AreSame(info, reloaded, "Re-loading the path should hand back the texture already in use"); + Assert.AreEqual(uploadsAfterUpdate, backend.CreateTextureCallCount, "Re-loading a still-cached path must not upload a duplicate GPU texture"); + CollectionAssert.Contains(ImGuiApp.Textures.Keys.ToList(), path, "CleanupAllTextures walks Textures.Keys, so the path must still be among them for the handle to be freeable"); + } + finally + { + File.Delete(file); + } + } + + [TestMethod] + public void UpdateTexture_OnAnUntrackedTexture_StaysOutOfTheCache() + { + // The republish must restore only entries this instance was already published under. A texture + // from CreateTexture has no path and was never in the cache, so a resize must not add one. + ResetState(); + ImGuiApp.Invoker = new Invoker.Invoker(); + FakeRendererBackend backend = new() { NextHandle = 42 }; + ImGuiApp.renderer = backend; + ImGuiApp.controller = null; + ImGuiAppTextureInfo info = ImGuiApp.CreateTexture(new byte[2 * 2 * 4], 2, 2); + + ImGuiApp.UpdateTexture(info, new byte[1 * 1 * 4], 1, 1); + + Assert.AreEqual(0, ImGuiApp.Textures.Count, "A memory texture must stay out of the path-keyed cache across a resize"); + } + [TestMethod] public void PerformanceSettings_DefaultValues_AreCorrect() {