From b4fa6e596e09b169c68e129d5aff288e9e3568cf Mon Sep 17 00:00:00 2001 From: catbot Date: Wed, 17 Jun 2026 19:40:35 +0000 Subject: [PATCH] perf(image): release static texture staging after upload (#114) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ImageVulkan kept its host-visible staging `buffer` (sized w*h, persistently mapped) alive for the whole life of the image and Destroy() never freed it, so static textures (e.g. Sponza albedo) pinned HOST_VISIBLE / small-BAR memory forever. Update now releases the staging via VulkanBuffer::DeferredClear() right after recording the buffer→image copy — the same fence-keyed deletion queue (#101/#102) the compressed Mesh path already uses — so it is freed once the upload submit's frame has cleared. A new `streamed` flag (set by FontAtlas) keeps the persistent map for images re-uploaded from a CPU-side staging buffer every frame; its uploads go through UpdateRegion, which never releases. Destroy now also frees any staging the image still owns, gated on a live handle so a released static texture can't double-free. Adds tests/ImageStagingRelease driving the real upload + readback on a headless device: asserts the staging is enqueued (not pinned), the image reads back byte-equal (released staging outlived the submit), the entry retires only after framesInFlight frames, and a streamed image keeps then frees its staging. Co-Authored-By: Claude Opus 4.8 --- .../Crafter.Graphics-FontAtlas.cpp | 6 +- interfaces/Crafter.Graphics-ImageVulkan.cppm | 37 ++- project.cpp | 34 +++ tests/ImageStagingRelease/main.cpp | 238 ++++++++++++++++++ 4 files changed, 313 insertions(+), 2 deletions(-) create mode 100644 tests/ImageStagingRelease/main.cpp diff --git a/implementations/Crafter.Graphics-FontAtlas.cpp b/implementations/Crafter.Graphics-FontAtlas.cpp index 051d04a..d6f14e7 100644 --- a/implementations/Crafter.Graphics-FontAtlas.cpp +++ b/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 diff --git a/interfaces/Crafter.Graphics-ImageVulkan.cppm b/interfaces/Crafter.Graphics-ImageVulkan.cppm index 2c40a01..8723ea7 100644 --- a/interfaces/Crafter.Graphics-ImageVulkan.cppm +++ b/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; diff --git a/project.cpp b/project.cpp index 97d51b5..485c8d5 100644 --- a/project.cpp +++ b/project.cpp @@ -651,6 +651,40 @@ extern "C" Configuration CrafterBuildProject(std::span 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 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 diff --git a/tests/ImageStagingRelease/main.cpp b/tests/ImageStagingRelease/main.cpp new file mode 100644 index 0000000..226226f --- /dev/null +++ b/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 + +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(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 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 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, ®ion); + + 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 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; +} -- 2.54.0