Download image via staging buffer by CrabExtra · Pull Request #1095 · Devsh-Graphics-Programming/Nabla

CrabExtra · 2026-09-21T11:26:57Z

Description

Testing

TODO list:

Erfan-Ahmadi · 2026-09-23T10:58:53Z

+    , srcImageFormat(image->getCreationParameters().format)
+    , dstImageFormat(image->getCreationParameters().format)


src and destination format can differ, and the image filter copy things can do different format copies.
that's one of the selling points of this utility. you can copy RGB8 to RGBA8 and those filter will handle that automagically. (the decode and then encode)

ok this is probably a default thing

Erfan-Ahmadi · 2026-09-29T09:46:04Z

        // --------------
        // downloadImageViaStagingBuffer
        // --------------
+        //! Records the commands needed to copy `regions` of `srcImage` into the staging buffer, and copies the staged data into `dest` once it arrives.


dest -> dst

Erfan-Ahmadi · 2026-09-29T09:49:22Z

+        //! Records the commands needed to copy `regions` of `srcImage` into the staging buffer, and copies the staged data into `dest` once it arrives.
+        //! The result is equivalent to a single `copyImageToBuffer` of `regions` into a buffer backing `dest`: every region's texels land at
+        //! `region.bufferOffset` laid out according to `region.bufferRowLength`/`bufferImageHeight` (0 meaning tightly packed), in region order.
+        //! No format conversion is performed, the data is laid out in `srcImage`'s format.


would've been nice to have but makes sense you usually only want to promote format because GPU can't support something when uploading your data.

Erfan-Ahmadi · 2026-09-29T09:55:11Z

+        bool copySuccess = true;
+        if(stagingBufferPointer)
+        {
+            core::smart_refctd_ptr<asset::ICPUImage> inCPUImage;
+            core::smart_refctd_ptr<asset::ICPUImage> outCPUImage;
+            createMockInOutCPUImagesForFilter(inCPUImage, outCPUImage, slicesToUploadMemorySize);

-        const auto inOffsetBaseLayer = core::vector4du32_SIMD(currentBlockInRow * texelBlockDim.x, currentRowInSlice * texelBlockDim.y, currentSliceInLayer * texelBlockDim.z, currentLayerInRegion);
-        bool copySuccess = performIntermediateCopy(srcImageFormat, dstImageFormat, inOffsetBaseLayer, inCPUImage, outCPUImage, regionToCopyNext);
+            const auto inOffsetBaseLayer = core::vector4du32_SIMD(currentBlockInRow * texelBlockDim.x, currentRowInSlice * texelBlockDim.y, currentSliceInLayer * texelBlockDim.z, currentLayerInRegion);
+            copySuccess = performIntermediateCopy(srcImageFormat, dstImageFormat, inOffsetBaseLayer, inCPUImage, outCPUImage, regionToCopyNext);
+        }


Ok so for uploading we perform a copy from users data to mapped staging memory using this here like before.
but when downloading we only fill regionToCopyNext and do not do any memcpies. makes sense because the memcpy needs to happen in another stage when the submit actually signals hinting that the mapped buffer is now filled with data

Erfan-Ahmadi · 2026-09-29T10:01:54Z

+    if (core::alignUp(maxRowPitch,m_allocationAlignmentForBufferImageCopy)>m_defaultDownloadBuffer->get_total_size())
+    {
+        m_logger.log("Download staging buffer of %u bytes cannot hold a single %u byte row, cannot `downloadImageViaStagingBuffer`.",system::ILogger::ELL_ERROR,m_defaultDownloadBuffer->get_total_size(),maxRowPitch);
+        return false;
+    }


good, do the same for upload path

Erfan-Ahmadi · 2026-09-29T10:25:46Z

+                    const asset::IImage::SBufferCopy& sub = subRegions[i];
+                    const asset::IImage::SBufferCopy& parent = dstRegions[i];


hmm something smells here. you can have multiple subRegion per parent regions.
Testing: I suggest you limit your example test's IUtilities allocated download buffer (down to something super small like 3-4MB instead of default 64MB) and test if your downloads break under multiple submits

what are parent regions anyways? why do you need to keep them?

Erfan-Ahmadi · 2026-09-29T10:27:55Z

+            if (availableDownloadBufferMemory > 0u && regionIterator.advance(nextRegionToCopy, availableDownloadBufferMemory, currentDownloadBufferOffset))
+            {
+                regionsToCopy.push_back(nextRegionToCopy);
+                parentRegions.push_back(parentRegion);


you're pushing back the same data per parentRegion, couldn't that be simple index per region to index regions with?

Erfan-Ahmadi · 2026-09-29T10:32:56Z

+                    const auto srcByteStrides = sub.getByteStrides(blockInfo);
+                    const auto dstByteStrides = parent.getByteStrides(blockInfo);
+                    const auto extentInBlocks = blockInfo.convertTexelsToBlocks(core::vector3du32_SIMD(sub.imageExtent.width,sub.imageExtent.height,sub.imageExtent.depth));
+                    const auto localBlockOffset = blockInfo.convertTexelsToBlocks(core::vector3du32_SIMD(
+                        sub.imageOffset.x-parent.imageOffset.x,
+                        sub.imageOffset.y-parent.imageOffset.y,
+                        sub.imageOffset.z-parent.imageOffset.z
+                    ));
+                    const uint32_t localLayer = sub.imageSubresource.baseArrayLayer-parent.imageSubresource.baseArrayLayer;
+
+                    const size_t rowByteSize = size_t(extentInBlocks.x)*blockByteSize;
+                    const uint8_t* srcBase = reinterpret_cast<const uint8_t*>(srcPtr)+(sub.bufferOffset-localOffset);
+                    uint8_t* dstBase = reinterpret_cast<uint8_t*>(dest)+parent.bufferOffset;
+                    for (uint32_t l=0u; l<sub.imageSubresource.layerCount; l++)
+                    for (uint32_t z=0u; z<extentInBlocks.z; z++)
+                    for (uint32_t y=0u; y<extentInBlocks.y; y++)
+                    {
+                        const uint8_t* src = srcBase+size_t(l)*srcByteStrides[3]+size_t(z)*srcByteStrides[2]+size_t(y)*srcByteStrides[1];
+                        assert(src+rowByteSize<=reinterpret_cast<const uint8_t*>(srcPtr)+size);
+                        uint8_t* dst = dstBase+asset::IImage::SBufferCopy::getLocalByteOffset(
+                            core::vector4du32_SIMD(localBlockOffset.x,localBlockOffset.y+y,localBlockOffset.z+z,localLayer+l),dstByteStrides
+                        );
+                        memcpy(dst,src,rowByteSize);


I have a feeling all of these are already calculated, or could be done in ImageRegionIterator.
for upload path the image region iterator is already doing the buffer offset caclulation to figure out which offset to copy into the staging area.
I think what you need here is just for the ImageRegionIterator to give you enough information to do the copy which includes the proper buffer offset needed to do the copy.
we need to think more about that and the resposibilities of image region iterator

Erfan-Ahmadi · 2026-09-29T10:51:17Z

+                    const auto srcByteStrides = sub.getByteStrides(blockInfo);
+                    const auto dstByteStrides = parent.getByteStrides(blockInfo);
+                    const auto extentInBlocks = blockInfo.convertTexelsToBlocks(core::vector3du32_SIMD(sub.imageExtent.width,sub.imageExtent.height,sub.imageExtent.depth));
+                    const auto localBlockOffset = blockInfo.convertTexelsToBlocks(core::vector3du32_SIMD(
+                        sub.imageOffset.x-parent.imageOffset.x,
+                        sub.imageOffset.y-parent.imageOffset.y,
+                        sub.imageOffset.z-parent.imageOffset.z
+                    ));
+                    const uint32_t localLayer = sub.imageSubresource.baseArrayLayer-parent.imageSubresource.baseArrayLayer;
+
+                    const size_t rowByteSize = size_t(extentInBlocks.x)*blockByteSize;
+                    const uint8_t* srcBase = reinterpret_cast<const uint8_t*>(srcPtr)+(sub.bufferOffset-localOffset);
+                    uint8_t* dstBase = reinterpret_cast<uint8_t*>(dest)+parent.bufferOffset;
+                    for (uint32_t l=0u; l<sub.imageSubresource.layerCount; l++)
+                    for (uint32_t z=0u; z<extentInBlocks.z; z++)
+                    for (uint32_t y=0u; y<extentInBlocks.y; y++)
+                    {
+                        const uint8_t* src = srcBase+size_t(l)*srcByteStrides[3]+size_t(z)*srcByteStrides[2]+size_t(y)*srcByteStrides[1];
+                        assert(src+rowByteSize<=reinterpret_cast<const uint8_t*>(srcPtr)+size);
+                        uint8_t* dst = dstBase+asset::IImage::SBufferCopy::getLocalByteOffset(
+                            core::vector4du32_SIMD(localBlockOffset.x,localBlockOffset.y+y,localBlockOffset.z+z,localLayer+l),dstByteStrides
+                        );
+                        memcpy(dst,src,rowByteSize);


I also don't like copying by the finest granularity which is row by row.
for a simple 1024x1024 image, you'd have 1000x memcpy calls and it's bad.
it's simply because you don't have the information image region iterator had, which is "are we copying full layers?" "are we copying full slices?" or "are we copying row by row" and that information is lost here so that's why I think you have to deduce it from comparing parent region to copy region

CrabExtra added 3 commits September 21, 2026 11:55

downloadImageViaStagingBuffer impl

0b7179a

Update submodule

3036773

update IUtilities

3850390

Erfan-Ahmadi reviewed Sep 23, 2026

View reviewed changes

Erfan-Ahmadi reviewed Sep 29, 2026

View reviewed changes

Navigation Menu

Download image via staging buffer - #1095

Download image via staging buffer#1095
CrabExtra wants to merge 3 commits into
masterfrom
downloadImageViaStagingBuffer

CrabExtra commented Sep 21, 2026

Uh oh!

Erfan-Ahmadi Sep 23, 2026

Uh oh!

Erfan-Ahmadi Sep 23, 2026

Uh oh!

Erfan-Ahmadi Sep 29, 2026

Uh oh!

Erfan-Ahmadi Sep 29, 2026

Uh oh!

Erfan-Ahmadi Sep 29, 2026

Uh oh!

Erfan-Ahmadi Sep 29, 2026

Uh oh!

Erfan-Ahmadi Sep 29, 2026

Uh oh!

Erfan-Ahmadi Sep 29, 2026

Uh oh!

Erfan-Ahmadi Sep 29, 2026

Uh oh!

Erfan-Ahmadi Sep 29, 2026

Uh oh!

Erfan-Ahmadi Sep 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		, srcImageFormat(image->getCreationParameters().format)
		, dstImageFormat(image->getCreationParameters().format)

		const asset::IImage::SBufferCopy& sub = subRegions[i];
		const asset::IImage::SBufferCopy& parent = dstRegions[i];

Conversation

CrabExtra commented Sep 21, 2026

Description

Testing

TODO list:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants