KhronosGroup / Vulkan-Docs

The Vulkan API Specification and related tools
Other
2.77k stars 465 forks source link

"Copying Data Between Buffers and Images" specifies texels super far outside the specified range will be accessed #2377

Closed litherum closed 2 months ago

litherum commented 4 months ago

https://github.com/KhronosGroup/Vulkan-Docs/commit/7a319840243ea33aa4caa42cdce0143b150e02bb added the following text:

For a set of coordinates (x,y,z,layer), where:

x is in the range [imageOffset.x / blockWidth, ⌈(imageOffset.x + imageExtent.width) / blockWidth⌉),

y is in the range [imageOffset.y / blockHeight, ⌈(imageOffset.y + imageExtent.height) / blockHeight⌉),

z is in the range [imageOffset.z / blockDepth, ⌈(imageOffset.z + imageExtent.depth) / blockDepth⌉),

layer is in the range [imageSubresource.baseArrayLayer, imageSubresource.baseArrayLayer + imageSubresource.layerCount),

and where blockWidth, blockHeight, and blockDepth are the dimensions of the texel block extent of the image’s format.

(Sure, so x, y, and z are in the unit of blocks. Sounds good.)

It then goes on to say:

For each (x,y,z,layer) coordinate, texels in the image layer selected by layer are accessed in the following ranges:

[x × blockWidth, max( (x × blockWidth) + blockWidth, imageWidth) )

[y × blockHeight, max( (y × blockHeight) + blockHeight, imageHeight) )

[z × blockDepth, max( (z × blockDepth) + blockDepth, imageDepth) )

This max() seems particularly surprising.

For example, consider if blockWidth is 1, imageWidth is 4096, and x is within range [4, 5). In this situation, this paragraph would seem to indicate that texels are accessed in the range [x, 4096), regardless of what imageExtent is set to. So this is saying that, if imageOffset and imageExtent identify pixels on the left side of the image, pixels from that point all the way to the right edge of the image will be accessed. This doesn't seem right.

(I'd guess the max() is probably trying to say that blocks straddling the edges of the image will be accessed in their entirety, but as it's written, the text doesn't seem to actually describe that.)

oddhack commented 4 months ago

@litherum we have an internal MR raised separately that may address this and your other issue, but it will take a little while to verify that and get it signed off. @tobski may want to say more since the MRs are theirs.

litherum commented 4 months ago

(What does "MR" stand for? Merge Request?)

spencer-lunarg commented 4 months ago

@litherum yes, we use Gitlab internally which has "Merge Request" instead of "Pull Request"

oddhack commented 2 months ago

This should be fixed in the 1.3.290 spec update. If you do not agree, please reopen with additional context.