Adding an image load failure case to the shader debug zoo and implementing a fix

[Honeybunch]

Spun off from #3480

I ran into an assert when trying to debug a SPV shader that performed an imageLoad operation on a buffer texture so I implemented a repro case in the shader debug zoo and what seemed like a reasonable fix to me.

This was specifically when accessing a uint texel buffer with an imageLoad operation. In the case of a float format ReadTexel's default case handling would behave properly.

[Zorro666]

Thank you for chasing this down and authoring a test case for the code fix.

Overall this is a good starting point and be good to evolve the code implementation and test design before merging the PR.

My suggestions are:

1. Author a test as a stand alone commit that fails (currently for me the newly added test passes if I remove the code support for the feature).
2. Perhaps it could be two tests to verify out of bounds behaviour and in-bounds behaviour.
3. Code implementation wise : I think it would be good to explore what is required to extend ReadTexel() to support this feature. I think it should be possible perhaps by treating the BufferView as a 1D non-MSAA Texture. This would reduce the amount of new and copy'n'pasted code and allow more code reuse.
4. For completeness of the feature it would be good to also support write as well as read i.e. imageStore : ideally with a test :)

One small comment on the test change : the new binding could be at the end of the existing bindings just to reduce the amount of code change and re-numbering. I have a personal preference (which is just a personal preference) to avoid code change noise where it is possible/makes sense.

[Honeybunch]

Sorry this took me so long to get to. I think this approach here is a little more compact 😄

[Zorro666]

Minor comment : this test does pass without the code change purely because the value at the offset is 0 and 0 is also returned by the failure case.

Locally this change makes this test fail, by reading a non-zero value

> git diff
diff --git a/util/test/demos/vk/vk_shader_debug_zoo.cpp b/util/test/demos/vk/vk_shader_debug_zoo.cpp
index a19996cd5..f9d2058ff 100644
--- a/util/test/demos/vk/vk_shader_debug_zoo.cpp
+++ b/util/test/demos/vk/vk_shader_debug_zoo.cpp
@@ -1580,7 +1580,7 @@ void main()
     }
     case 177:
     {
-      Color = vec4(imageLoad(texBufferUint, int(zeroi+2)).x, 1, 0, 1);
+      Color = vec4(imageLoad(texBufferUint, int(zeroi+31*4)).x, 1, 0, 1);
       break;
     }
     case 178:
@@ -4552,6 +4552,10 @@ OpMemberDecorate %cbuffer_struct 17 Offset 216    ; double doublePackSource
     memcpy(&cbufferdata[13].x, &unpackDouble, sizeof(unpackDouble));
     memcpy(&cbufferdata[14].z, &unpackDouble, sizeof(unpackDouble));

+    // ubufferimage load/store
+    uint32_t uintValue = 123;
+    memcpy(&cbufferdata[15].x, &uintValue, sizeof(uintValue));
+
     // move to account for offset

[Zorro666]

Thank you for adding this test : it does fail before the code change and passes after the code change

[Zorro666]

This is getting called multiple times when it could be a single call

[Zorro666]

I think more simplification can be done still and also the call to GetBufferData() should be a single call to read all the required buffer data at once. Currently GetBufferData() is getting called to read the entire buffer x the number of elements in the buffer (which is too much data)

Purely as an example here is an idea of how things can be simplified further and the buffer data retrieved in a single call.

diff --git a/renderdoc/driver/vulkan/vk_shaderdebug.cpp b/renderdoc/driver/vulkan/vk_shaderdebug.cpp
index 348c4ffff..644159274 100644
--- a/renderdoc/driver/vulkan/vk_shaderdebug.cpp
+++ b/renderdoc/driver/vulkan/vk_shaderdebug.cpp
@@ -1709,11 +1709,11 @@ private:
               m_Creation.m_ImageView[m_pDriver->GetResourceManager()->GetLiveID(imgData.view)];
           const VulkanCreationInfo::BufferView &bufViewProps =
               m_Creation.m_BufferView[m_pDriver->GetResourceManager()->GetLiveID(imgData.view)];

           bool resourceIsImage = false;
           uint32_t mip = 0;
           uint32_t numSlices = 0;
           uint32_t numSamples = 0;
+          VkFormat srcFormat = VK_FORMAT_UNDEFINED;
 
           if(imgViewProps.image != ResourceId())
           {
@@ -1735,13 +1735,7 @@ private:
                 data.depth = imageProps.arrayLayers - imgViewProps.range.baseArrayLayer;
             }
 
-            ResourceFormat fmt = MakeResourceFormat(imageProps.format);
-
-            data.fmt = MakeResourceFormat(imageProps.format);
-            data.texelSize = (uint32_t)GetByteSize(1, 1, 1, imageProps.format, 0);
-            data.rowPitch = (uint32_t)GetByteSize(data.width, 1, 1, imageProps.format, 0);
-            data.slicePitch = GetByteSize(data.width, data.height, 1, imageProps.format, 0);
-            data.samplePitch = GetByteSize(data.width, data.height, data.depth, imageProps.format, 0);
+            srcFormat = imageProps.format;
 
             numSlices = imageProps.type == VK_IMAGE_TYPE_3D ? 1 : data.depth;
             numSamples = (uint32_t)imageProps.samples;
@@ -1750,22 +1744,16 @@ private:
           {
             const VulkanCreationInfo::Buffer &bufferProps = m_Creation.m_Buffer[bufViewProps.buffer];
 
-            ResourceFormat fmt = MakeResourceFormat(bufViewProps.format);
+            srcFormat = bufViewProps.format;
+            ResourceFormat fmt = MakeResourceFormat(srcFormat);
 
             mip = 0;
             data.width = (uint32_t)(bufferProps.size / (fmt.compByteWidth * fmt.compCount));
             data.height = 1;
             data.depth = 1;
 
-            data.fmt = fmt;
-            data.texelSize = (uint32_t)GetByteSize(1, 1, 1, bufViewProps.format, 0);
-            data.rowPitch = (uint32_t)GetByteSize(data.width, 1, 1, bufViewProps.format, 0);
-            data.slicePitch = GetByteSize(data.width, data.height, 1, bufViewProps.format, 0);
-            data.samplePitch =
-                GetByteSize(data.width, data.height, data.depth, bufViewProps.format, 0);
-
             numSlices = 1;
-            numSamples = data.width;
+            numSamples = 1;
           }
           else
           {
@@ -1773,6 +1761,12 @@ private:
             RDCASSERT(false);
           }
 
+          data.fmt = MakeResourceFormat(srcFormat);
+          data.texelSize = (uint32_t)GetByteSize(1, 1, 1, srcFormat, 0);
+          data.rowPitch = (uint32_t)GetByteSize(data.width, 1, 1, srcFormat, 0);
+          data.slicePitch = GetByteSize(data.width, data.height, 1, srcFormat, 0);
+          data.samplePitch = GetByteSize(data.width, data.height, data.depth, srcFormat, 0);
+
           data.bytes.reserve(size_t(data.samplePitch * numSamples));
 
           // defaults are fine - no interpretation. Maybe we could use the view's typecast?
@@ -1790,8 +1784,10 @@ private:
               }
               else
               {
-                const uint64_t length = data.samplePitch;
-                const uint64_t offset = bufViewProps.offset + ((slice * numSamples) + sample);
+                RDCASSERTEQUAL(slice, 0);
+                RDCASSERTEQUAL(sample, 0);
+                const uint64_t length = data.samplePitch;
+                const uint64_t offset = bufViewProps.offset;
                 m_pDriver->GetReplay()->GetBufferData(bufViewProps.buffer, offset, length, subBytes);
               }

[Zorro666]

Thank you this is a great step forwards from the previous PR.

There are more simplification that could be done to keep the code simpler.

For the buffer case it its reading too much data by calling GetBufferData too much

One of the tests could be improved to guarantee failure before the code change

[baldurk]

Closing this due to lack of activity from the PR author and no response to feedback.

If you are the author and you'd like to revisit this change please open a new PR and address the feedback above.