Merge pull request 'perf(image): release static texture staging after upload (#114)' (#136) from claude/issue-114 into master

implementations/Crafter.Graphics-FontAtlas.cpp

@@ -39,7 +39,11 @@ void FontAtlas::Initialize(GraphicsCommandBuffer cmd) {
         VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL,
         // The atlas is sampled by the UI text *compute* shader, not an RT
         // pipeline — scope the upload barriers to the real consumer.
-        VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT
+        VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT,
+        // Streamed: glyphs are rasterized into buffer.value on the CPU and
+        // re-uploaded every frame via UpdateRegion, so the persistent staging
+        // map must outlive the first upload rather than being released (#114).
+        /*streamed*/ true
     );
     std::memset(image.buffer.value, 0, kAtlasSize * kAtlasSize);
 #else

interfaces/Crafter.Graphics-ImageVulkan.cppm

@@ -90,12 +90,24 @@ export namespace Crafter {
         // consumer. Defaults to RT — the font atlas overrides it to COMPUTE,
         // since UI text is rendered by a compute shader, not an RT pipeline.
         VkPipelineStageFlags consumerStage = VK_PIPELINE_STAGE_RAY_TRACING_SHADER_BIT_KHR;
+        // Whether this image is continuously re-uploaded from a persistent
+        // CPU-side staging map. False (the default) marks a static texture
+        // uploaded once — e.g. Sponza albedo: the first Update's buffer→image
+        // copy is the only reader of the host-visible staging `buffer`, so it
+        // is released to the fence-keyed deletion queue (#101/#102) right after
+        // the copy instead of pinning HOST_VISIBLE / small-BAR memory for the
+        // image's whole life (issue #114). True keeps `buffer` alive: a streamed
+        // image (the FontAtlas) re-fills buffer.value on the CPU and re-uploads
+        // every frame, so the persistent map must survive — its uploads go
+        // through UpdateRegion, which never releases the staging.
+        bool streamed = false;
 
-        void Create(std::uint16_t width, std::uint16_t height, std::uint8_t mipLevels, VkCommandBuffer cmd, VkFormat format, VkImageCreateFlags flags, VkImageLayout layout, VkPipelineStageFlags consumerStage = VK_PIPELINE_STAGE_RAY_TRACING_SHADER_BIT_KHR) {
+        void Create(std::uint16_t width, std::uint16_t height, std::uint8_t mipLevels, VkCommandBuffer cmd, VkFormat format, VkImageCreateFlags flags, VkImageLayout layout, VkPipelineStageFlags consumerStage = VK_PIPELINE_STAGE_RAY_TRACING_SHADER_BIT_KHR, bool streamed = false) {
             this->width = width;
             this->height = height;
             this->mipLevels = mipLevels;
             this->consumerStage = consumerStage;
+            this->streamed = streamed;
             buffer.Create(
                 VK_BUFFER_USAGE_TRANSFER_SRC_BIT | VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT,
                 VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT,
@@ -212,6 +224,7 @@ export namespace Crafter {
             } else {
                 TransitionImageLayout(cmd, image, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, layout, VK_PIPELINE_STAGE_TRANSFER_BIT, consumerStage, VK_ACCESS_TRANSFER_WRITE_BIT, VK_ACCESS_SHADER_READ_BIT, 0, mipLevels);
             }
+            ReleaseStaging();
         }
 
         // Upload only the sub-rectangle [x, x+w) × [y, y+h) of the staging
@@ -365,15 +378,37 @@ export namespace Crafter {
             } else {
                 TransitionImageLayout(cmd, image, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, layout, VK_PIPELINE_STAGE_TRANSFER_BIT, consumerStage, VK_ACCESS_TRANSFER_WRITE_BIT, VK_ACCESS_SHADER_READ_BIT, 0, mipLevels);
             }
+            ReleaseStaging();
         }
 
         void Destroy() {
             vkDestroyImageView(Device::device, imageView, nullptr);
             vkDestroyImage(Device::device, image, nullptr);
             vkFreeMemory(Device::device, imageMemory, nullptr);
+            // Free any staging this image still owns: a streamed image's
+            // persistent `buffer`, or either staging buffer on an image
+            // destroyed before its first upload released them (issue #114).
+            // ReleaseStaging()/Update already null released handles, so these
+            // are no-ops then; Clear() is gated on a live handle so a static
+            // texture (whose `buffer` was deferred-cleared) can't double-free.
+            if (buffer.buffer != VK_NULL_HANDLE) buffer.Clear();
+            if (compressedStaging.buffer != VK_NULL_HANDLE) compressedStaging.Clear();
         }
 
         private:
+        // Release the host-visible staging `buffer` after a one-shot upload,
+        // unless this is a streamed image (FontAtlas) whose persistent map must
+        // survive. The recorded buffer→image copy still references buffer.buffer,
+        // so route through the fence-keyed deletion queue (#101/#102) instead of
+        // an immediate Clear(): DeferredClear nulls the handle (so re-entry and
+        // Destroy treat the staging as already gone) while the allocation
+        // outlives this submit's frame. Issue #114.
+        void ReleaseStaging() {
+            if (!streamed) {
+                buffer.DeferredClear();
+            }
+        }
+
         void TransitionImageLayout(VkCommandBuffer cmd, VkImage image, VkImageLayout oldLayout, VkImageLayout newLayout, VkPipelineStageFlags sourceStage, VkPipelineStageFlags destinationStage, VkAccessFlags srcAccessMask, VkAccessFlags dstAccessMask, std::uint32_t mipLevel, std::uint32_t count) {
             VkImageMemoryBarrier barrier = {};
             barrier.sType = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER;

project.cpp

@@ -651,6 +651,40 @@ extern "C" Configuration CrafterBuildProject(std::span<const std::string_view> a
         msc.GetInterfacesAndImplementations(ifaces, meshStagingImpls);
         cfg.tests.push_back(std::move(meshStagingTest));
 
+        // Issue #114: a static ImageVulkan no longer pins its host-visible
+        // staging `buffer` for the image's life — Update releases it via
+        // DeferredClear() right after recording the buffer→image copy, so the
+        // fence-keyed deletion queue (#101/#102) frees it once that submit's
+        // frame clears, while a `streamed` image (the FontAtlas) keeps its
+        // persistent map. Drives the real upload path on a headless device (no
+        // swapchain/window — a buffer→image copy + readback only needs the queue
+        // + command pool) and asserts the staging is enqueued (not pinned), the
+        // image reads back byte-equal (the released staging outlived the submit),
+        // the entry retires only after framesInFlight frames, and a streamed
+        // image keeps + then frees its staging on Destroy. Shares the native
+        // build settings.
+        Test imageStagingTest;
+        Configuration& isc = imageStagingTest.config;
+        isc.path                 = cfg.path;
+        isc.name                 = "ImageStagingRelease";
+        isc.outputName           = "ImageStagingRelease";
+        isc.type                 = ConfigurationType::Executable;
+        isc.target               = cfg.target;
+        isc.march                = cfg.march;
+        isc.mtune                = cfg.mtune;
+        isc.debug                = cfg.debug;
+        isc.sysroot              = cfg.sysroot;
+        isc.dependencies         = cfg.dependencies;
+        isc.externalDependencies = cfg.externalDependencies;
+        isc.compileFlags         = cfg.compileFlags;
+        isc.linkFlags            = cfg.linkFlags;
+        isc.defines              = cfg.defines;
+        isc.cFiles               = cfg.cFiles;
+        std::vector<fs::path> imageStagingImpls(impls.begin(), impls.end());
+        imageStagingImpls.emplace_back("tests/ImageStagingRelease/main");
+        isc.GetInterfacesAndImplementations(ifaces, imageStagingImpls);
+        cfg.tests.push_back(std::move(imageStagingTest));
+
         // Issue #89: Device::PreferDirectDeviceWrite chooses the upload strategy
         // for a CPU-written, GPU-read buffer — direct HOST_VISIBLE|DEVICE_LOCAL
         // map+write on ReBAR/UMA vs. staged-into-pure-DEVICE_LOCAL on a small

tests/ImageStagingRelease/main.cpp

@@ -0,0 +1,238 @@
+/*
+Crafter®.Graphics
+Copyright (C) 2026 Catcrafts®
+catcrafts.net
+
+This library is free software; you can redistribute it and/or
+modify it under the terms of the GNU Lesser General Public
+License version 3.0 as published by the Free Software Foundation;
+
+This library is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+Lesser General Public License for more details.
+
+You should have received a copy of the GNU Lesser General Public
+License along with this library; if not, write to the Free Software
+Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301 USA
+*/
+
+// Issue #114: a static ImageVulkan used to keep its host-visible staging
+// `buffer` (sized w*h, persistently mapped) alive for the whole life of the
+// image, pinning HOST_VISIBLE / small-BAR memory long after the one upload that
+// reads it has retired — and Destroy() never freed it. Update now releases it
+// via VulkanBuffer::DeferredClear() right after recording the buffer→image
+// copy, so the fence-keyed deletion queue (#101/#102) frees it once that
+// submit's frame has cleared. A `streamed` image (the FontAtlas) keeps its
+// staging, since it re-fills the persistent map and re-uploads every frame.
+//
+// This drives the real upload path on a headless device — no swapchain/window
+// needed, a buffer→image copy only touches the queue + command pool — and
+// asserts:
+//   - After a static Update, image.buffer owns no handle (released), and
+//     exactly that one allocation was handed to the deletion queue tagged with
+//     the current frame (so it retires later, not now).
+//   - The upload + readback still complete with ZERO validation errors and the
+//     image reads back byte-equal to the staged source — proof the staging
+//     genuinely outlived the submit (the queue had not yet retired it), so
+//     releasing it is no use-after-free.
+//   - The deferred entry retires (and frees) only once framesInFlight frames
+//     elapse, and not before.
+//   - A `streamed` image keeps its staging across Update and UpdateRegion (the
+//     FontAtlas contract), and Destroy() then frees it validation-clean.
+
+#include "vulkan/vulkan.h"
+#include <cstring>
+
+import Crafter.Graphics;
+import std;
+
+using namespace Crafter;
+
+namespace {
+
+int failures = 0;
+
+void Check(bool ok, std::string_view what) {
+    std::println("{}  {}", ok ? "PASS" : "FAIL", what);
+    if (!ok) ++failures;
+}
+
+VkCommandBuffer BeginCmd() {
+    VkCommandBufferAllocateInfo allocInfo {
+        .sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_ALLOCATE_INFO,
+        .commandPool = Device::commandPool,
+        .level = VK_COMMAND_BUFFER_LEVEL_PRIMARY,
+        .commandBufferCount = 1,
+    };
+    VkCommandBuffer cmd = VK_NULL_HANDLE;
+    Device::CheckVkResult(vkAllocateCommandBuffers(Device::device, &allocInfo, &cmd));
+    VkCommandBufferBeginInfo beginInfo {
+        .sType = VK_STRUCTURE_TYPE_COMMAND_BUFFER_BEGIN_INFO,
+        .flags = VK_COMMAND_BUFFER_USAGE_ONE_TIME_SUBMIT_BIT,
+    };
+    Device::CheckVkResult(vkBeginCommandBuffer(cmd, &beginInfo));
+    return cmd;
+}
+
+void SubmitWait(VkCommandBuffer cmd) {
+    Device::CheckVkResult(vkEndCommandBuffer(cmd));
+    VkSubmitInfo submitInfo {
+        .sType = VK_STRUCTURE_TYPE_SUBMIT_INFO,
+        .commandBufferCount = 1,
+        .pCommandBuffers = &cmd,
+    };
+    Device::CheckVkResult(vkQueueSubmit(Device::queue, 1, &submitInfo, VK_NULL_HANDLE));
+    Device::CheckVkResult(vkQueueWaitIdle(Device::queue));
+    vkFreeCommandBuffers(Device::device, Device::commandPool, 1, &cmd);
+}
+
+// RGBA8 texel as a 4-byte word, matching VK_FORMAT_R8G8B8A8_UNORM.
+using Texel = std::uint32_t;
+
+constexpr std::uint16_t kW = 4;
+constexpr std::uint16_t kH = 4;
+constexpr std::uint32_t kCount = static_cast<std::uint32_t>(kW) * kH;
+
+} // namespace
+
+int main() {
+    Device::Initialize();
+    Device::validationErrorCount = 0;
+
+    // Control the deferred-deletion clock: enqueue at frame 0, retire after 2.
+    Device::framesInFlight = 2;
+    Device::frameCounter   = 0;
+    Device::deletionQueue.clear();
+
+    // ── Static texture: staging is released right after the upload copy. ──────
+    ImageVulkan<Texel> tex;
+    {
+        VkCommandBuffer cmd = BeginCmd();
+        tex.Create(kW, kH, /*mipLevels*/ 1, cmd,
+                   VK_FORMAT_R8G8B8A8_UNORM,
+                   VK_IMAGE_USAGE_TRANSFER_DST_BIT | VK_IMAGE_USAGE_TRANSFER_SRC_BIT
+                       | VK_IMAGE_USAGE_SAMPLED_BIT,
+                   VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL);
+
+        Check(tex.buffer.buffer != VK_NULL_HANDLE,
+              "Create allocated the host-visible staging buffer");
+        Check(!tex.streamed, "a default ImageVulkan is not streamed");
+
+        // Fill the staging map with a known pattern, then upload.
+        for (std::uint32_t i = 0; i < kCount; ++i) {
+            tex.buffer.value[i] = 0xA0B0C0D0u + i;
+        }
+        tex.Update(cmd, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL);
+
+        // ── The fix: staging released to the queue at record time, not pinned. ─
+        Check(tex.buffer.buffer == VK_NULL_HANDLE,
+              "static Update releases the staging buffer (no handle pinned)");
+        Check(Device::deletionQueue.size() == 1,
+              "exactly one allocation (the staging) handed to the deletion queue");
+        const bool taggedNow = !Device::deletionQueue.empty()
+            && Device::deletionQueue.front().retireAfter == 0
+            && Device::deletionQueue.front().buffer != VK_NULL_HANDLE;
+        Check(taggedNow,
+              "deferred staging is tagged with the enqueue frame and still a live handle");
+
+        // Read the image back to prove the copy saw valid staging. The upload's
+        // recorded copy still references the now-released staging; it must
+        // outlive THIS submit (queue retires at frame 2, we're at 0). Move the
+        // image to TRANSFER_SRC and copy into a host-visible readback buffer in
+        // the same command buffer.
+        VkImageMemoryBarrier toSrc {
+            .sType = VK_STRUCTURE_TYPE_IMAGE_MEMORY_BARRIER,
+            .srcAccessMask = VK_ACCESS_SHADER_READ_BIT,
+            .dstAccessMask = VK_ACCESS_TRANSFER_READ_BIT,
+            .oldLayout = VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL,
+            .newLayout = VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL,
+            .srcQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED,
+            .dstQueueFamilyIndex = VK_QUEUE_FAMILY_IGNORED,
+            .image = tex.image,
+            .subresourceRange = { VK_IMAGE_ASPECT_COLOR_BIT, 0, 1, 0, 1 },
+        };
+        vkCmdPipelineBarrier(cmd, VK_PIPELINE_STAGE_RAY_TRACING_SHADER_BIT_KHR,
+                             VK_PIPELINE_STAGE_TRANSFER_BIT, 0, 0, nullptr,
+                             0, nullptr, 1, &toSrc);
+
+        VulkanBuffer<Texel, true> readback;
+        readback.Create(VK_BUFFER_USAGE_TRANSFER_DST_BIT | VK_BUFFER_USAGE_SHADER_DEVICE_ADDRESS_BIT,
+                        VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT, kCount);
+        VkBufferImageCopy region {};
+        region.imageSubresource = { VK_IMAGE_ASPECT_COLOR_BIT, 0, 0, 1 };
+        region.imageExtent = { kW, kH, 1 };
+        vkCmdCopyImageToBuffer(cmd, tex.image, VK_IMAGE_LAYOUT_TRANSFER_SRC_OPTIMAL,
+                               readback.buffer, 1, &region);
+
+        SubmitWait(cmd);
+        Check(Device::validationErrorCount == 0,
+              "static upload + readback raised no validation errors");
+
+        readback.FlushHost();
+        bool match = true;
+        for (std::uint32_t i = 0; i < kCount; ++i) {
+            if (readback.value[i] != 0xA0B0C0D0u + i) { match = false; break; }
+        }
+        Check(match,
+              "image reads back byte-equal to the staged source "
+              "(released staging outlived the submit)");
+    }
+
+    // ── Retire timing: the staging frees once framesInFlight frames elapse,
+    //    and not a frame before. We've already wait-idled, so freeing is safe. ─
+    Device::ReclaimDeletions();
+    Check(Device::deletionQueue.size() == 1,
+          "staging is NOT freed before framesInFlight frames have elapsed");
+    Device::frameCounter = 2;  // retireAfter(0) + framesInFlight(2) <= 2
+    Device::ReclaimDeletions();
+    Check(Device::deletionQueue.empty(),
+          "staging is freed once its retire frame is reached");
+
+    // Destroying the image after its staging was released must not double-free
+    // (buffer handle is already null) and must stay validation-clean.
+    Device::validationErrorCount = 0;
+    tex.Destroy();
+    Check(Device::validationErrorCount == 0,
+          "Destroy after a released-staging upload is validation-clean");
+
+    // ── Streamed image: the persistent staging is kept across uploads. ────────
+    Device::frameCounter = 0;
+    Device::deletionQueue.clear();
+    Device::validationErrorCount = 0;
+    {
+        ImageVulkan<Texel> atlas;
+        VkCommandBuffer cmd = BeginCmd();
+        atlas.Create(kW, kH, /*mipLevels*/ 1, cmd,
+                     VK_FORMAT_R8G8B8A8_UNORM,
+                     VK_IMAGE_USAGE_TRANSFER_DST_BIT | VK_IMAGE_USAGE_SAMPLED_BIT,
+                     VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL,
+                     VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT,
+                     /*streamed*/ true);
+        Check(atlas.streamed, "streamed=true is recorded on the image");
+
+        for (std::uint32_t i = 0; i < kCount; ++i) atlas.buffer.value[i] = i;
+        atlas.Update(cmd, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL);
+        Check(atlas.buffer.buffer != VK_NULL_HANDLE,
+              "streamed Update keeps the persistent staging buffer");
+        // UpdateRegion (the FontAtlas re-upload path) must also keep it.
+        atlas.UpdateRegion(cmd, VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL, 0, 0, kW, kH);
+        Check(atlas.buffer.buffer != VK_NULL_HANDLE,
+              "streamed UpdateRegion keeps the persistent staging buffer");
+        Check(Device::deletionQueue.empty(),
+              "a streamed image enqueues nothing for deferred deletion");
+        SubmitWait(cmd);
+        Check(Device::validationErrorCount == 0,
+              "streamed upload raised no validation errors");
+
+        // Destroy frees the still-live persistent staging (the leak #114 names).
+        atlas.Destroy();
+        Check(atlas.buffer.buffer == VK_NULL_HANDLE,
+              "Destroy frees the streamed image's persistent staging buffer");
+        Check(Device::validationErrorCount == 0,
+              "streamed Destroy (freeing live staging) is validation-clean");
+    }
+
+    std::println("{}", failures == 0 ? "ALL PASS" : "FAILURES PRESENT");
+    return failures == 0 ? 0 : 1;
+}