Skip to content

Support texture readback through copyTextureRegion - #114

Open
IsaacMarovitz wants to merge 1 commit into
renderbag:mainfrom
IsaacMarovitz:readback
Open

IsaacMarovitz wants to merge 1 commit into
renderbag:mainfrom
IsaacMarovitz:readback

Conversation

@IsaacMarovitz

Copy link
Copy Markdown
Contributor

Explicitly adds support for copyTextureRegion calls in the other direction.

@squidbus squidbus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't vouch for the D3D12 but the others look okay to me.

@dcvz dcvz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I noticed is that it ignores dstX/Y/Z and sizes the copy from the footprint instead of srcBox which is not how D3D12 does it.

Comment thread plume_metal.cpp
// Calculate block size based on source texture format
const uint32_t blockWidth = RenderFormatBlockWidth(srcTexture->desc.format);

const MTL::Size size = { dstLocation.placedFootprint.width, dstLocation.placedFootprint.height, dstLocation.placedFootprint.depth };

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This takes the copy size from the footprint, D3D12 uses srcBox or the whole subresource when there's no box:

Suggested change
const MTL::Size size = { dstLocation.placedFootprint.width, dstLocation.placedFootprint.height, dstLocation.placedFootprint.depth };
const uint32_t mipLevel = srcLocation.subresource.mipLevel;
const MTL::Size size = (srcBox != nullptr)
? MTL::Size { NS::UInteger(srcBox->right - srcBox->left), NS::UInteger(srcBox->bottom - srcBox->top), NS::UInteger(srcBox->back - srcBox->front) }
: MTL::Size { std::max(srcTexture->desc.width >> mipLevel, 1U), std::max(srcTexture->desc.height >> mipLevel, 1U), std::max(srcTexture->desc.depth >> mipLevel, 1U) };

Comment thread plume_metal.cpp
const uint32_t horizontalBlocks = (dstLocation.placedFootprint.rowWidth + blockWidth - 1) / blockWidth;
const uint32_t verticalBlocks = (dstLocation.placedFootprint.height + blockWidth - 1) / blockWidth;
const uint32_t bytesPerRow = horizontalBlocks * RenderFormatSize(srcTexture->desc.format);
const uint32_t bytesPerImage = bytesPerRow * verticalBlocks;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dstX/Y/Z place the region inside the footprint, so they need to go into the buffer offset:

Suggested change
const uint32_t bytesPerImage = bytesPerRow * verticalBlocks;
const uint32_t bytesPerImage = bytesPerRow * verticalBlocks;
const uint64_t dstOffset = dstLocation.placedFootprint.offset + dstZ * bytesPerImage + (dstY / blockWidth) * bytesPerRow + (dstX / blockWidth) * RenderFormatSize(srcTexture->desc.format);

Comment thread plume_metal.cpp
srcOrigin,
size,
dstBuffer->mtl,
dstLocation.placedFootprint.offset,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dstLocation.placedFootprint.offset,
dstOffset,

Comment thread plume_vulkan.cpp
Comment on lines +3237 to +3238
VkBufferImageCopy imageCopy = {};
imageCopy.bufferOffset = dstLocation.placedFootprint.offset;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same two things on Vulkan, the offset:

Suggested change
VkBufferImageCopy imageCopy = {};
imageCopy.bufferOffset = dstLocation.placedFootprint.offset;
const uint32_t blockSize = RenderFormatSize(srcTexture->desc.format);
const uint32_t bytesPerRow = ((dstLocation.placedFootprint.rowWidth + blockWidth - 1) / blockWidth) * blockSize;
const uint32_t bytesPerImage = bytesPerRow * ((dstLocation.placedFootprint.height + blockWidth - 1) / blockWidth);
const uint32_t mipLevel = srcLocation.subresource.mipLevel;
VkBufferImageCopy imageCopy = {};
imageCopy.bufferOffset = dstLocation.placedFootprint.offset + dstZ * bytesPerImage + (dstY / blockWidth) * bytesPerRow + (dstX / blockWidth) * blockSize;

Comment thread plume_vulkan.cpp
Comment on lines +3248 to +3250
imageCopy.imageExtent.width = dstLocation.placedFootprint.width;
imageCopy.imageExtent.height = dstLocation.placedFootprint.height;
imageCopy.imageExtent.depth = dstLocation.placedFootprint.depth;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the size:

Suggested change
imageCopy.imageExtent.width = dstLocation.placedFootprint.width;
imageCopy.imageExtent.height = dstLocation.placedFootprint.height;
imageCopy.imageExtent.depth = dstLocation.placedFootprint.depth;
imageCopy.imageExtent.width = srcBox != nullptr ? uint32_t(srcBox->right - srcBox->left) : std::max(srcTexture->desc.width >> mipLevel, 1U);
imageCopy.imageExtent.height = srcBox != nullptr ? uint32_t(srcBox->bottom - srcBox->top) : std::max(srcTexture->desc.height >> mipLevel, 1U);
imageCopy.imageExtent.depth = srcBox != nullptr ? uint32_t(srcBox->back - srcBox->front) : std::max(srcTexture->desc.depth >> mipLevel, 1U);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants