Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db2e9f9
Creating test cases that fail
Honeybunch Nov 26, 2024
9440589
Fixing failure to debug pixels that use imageLoad or imageStore
Honeybunch Nov 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 69 additions & 22 deletions renderdoc/driver/vulkan/vk_shaderdebug.cpp
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

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);
               }

Original file line number Diff line number Diff line change
Expand Up @@ -1705,35 +1705,73 @@ class VulkanAPIWrapper : public rdcspv::DebugAPIWrapper

if(imgData.view != ResourceId())
{
const VulkanCreationInfo::ImageView &viewProps =
const VulkanCreationInfo::ImageView &imgViewProps =
m_Creation.m_ImageView[m_pDriver->GetResourceManager()->GetLiveID(imgData.view)];
const VulkanCreationInfo::Image &imageProps = m_Creation.m_Image[viewProps.image];
const VulkanCreationInfo::BufferView &bufViewProps =
m_Creation.m_BufferView[m_pDriver->GetResourceManager()->GetLiveID(imgData.view)];

uint32_t mip = viewProps.range.baseMipLevel;
bool resourceIsImage = false;
uint32_t mip = 0;
uint32_t numSlices = 0;
uint32_t numSamples = 0;

data.width = RDCMAX(1U, imageProps.extent.width >> mip);
data.height = RDCMAX(1U, imageProps.extent.height >> mip);
if(imageProps.type == VK_IMAGE_TYPE_3D)
if(imgViewProps.image != ResourceId())
{
data.depth = RDCMAX(1U, imageProps.extent.depth >> mip);
resourceIsImage = true;
const VulkanCreationInfo::Image &imageProps = m_Creation.m_Image[imgViewProps.image];

mip = imgViewProps.range.baseMipLevel;

data.width = RDCMAX(1U, imageProps.extent.width >> mip);
data.height = RDCMAX(1U, imageProps.extent.height >> mip);
if(imageProps.type == VK_IMAGE_TYPE_3D)
{
data.depth = RDCMAX(1U, imageProps.extent.depth >> mip);
}
else
{
data.depth = imgViewProps.range.layerCount;
if(data.depth == VK_REMAINING_ARRAY_LAYERS)
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);

numSlices = imageProps.type == VK_IMAGE_TYPE_3D ? 1 : data.depth;
numSamples = (uint32_t)imageProps.samples;
}
else
else if(bufViewProps.buffer != ResourceId())
{
data.depth = viewProps.range.layerCount;
if(data.depth == VK_REMAINING_ARRAY_LAYERS)
data.depth = imageProps.arrayLayers - viewProps.range.baseArrayLayer;
}
const VulkanCreationInfo::Buffer &bufferProps = m_Creation.m_Buffer[bufViewProps.buffer];

ResourceFormat fmt = MakeResourceFormat(bufViewProps.format);

ResourceFormat fmt = MakeResourceFormat(imageProps.format);
mip = 0;
data.width = (uint32_t)(bufferProps.size / (fmt.compByteWidth * fmt.compCount));
data.height = 1;
data.depth = 1;

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);
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);

const uint32_t numSlices = imageProps.type == VK_IMAGE_TYPE_3D ? 1 : data.depth;
const uint32_t numSamples = (uint32_t)imageProps.samples;
numSlices = 1;
numSamples = data.width;
}
else
{
// Should be unreachable. If we have a view to neither a valid image or buffer, something is wrong
RDCASSERT(false);
}

data.bytes.reserve(size_t(data.samplePitch * numSamples));

Expand All @@ -1745,8 +1783,17 @@ class VulkanAPIWrapper : public rdcspv::DebugAPIWrapper
for(uint32_t slice = 0; slice < numSlices; slice++)
{
bytebuf subBytes;
m_pDriver->GetReplay()->GetTextureData(
viewProps.image, Subresource(mip, slice, sample), params, subBytes);
if(resourceIsImage)
{
m_pDriver->GetReplay()->GetTextureData(
imgViewProps.image, Subresource(mip, slice, sample), params, subBytes);
}
else
{
const uint64_t length = data.samplePitch;
const uint64_t offset = bufViewProps.offset + ((slice * numSamples) + sample);
m_pDriver->GetReplay()->GetBufferData(bufViewProps.buffer, offset, length, subBytes);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}

// fast path, swap into output if there's only one slice and one sample (common case)
if(numSlices == 1 && numSamples == 1)
Expand Down
24 changes: 24 additions & 0 deletions util/test/demos/vk/vk_shader_debug_zoo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,8 @@ layout(set = 0, binding = 31) uniform sampler2DMSArray queryTestMS;

layout(set = 0, binding = 32) uniform texture2D depthImage;

layout(set = 0, binding = 33, rgba8ui) uniform uimageBuffer texBufferUint;
layout(set = 0, binding = 34, rgba8ui) uniform coherent uimageBuffer storeTexBufferUint;
#if TEST_DESC_INDEXING

layout(set = 1, binding = 1) uniform sampler pointSamplers[14];
Expand Down Expand Up @@ -1576,6 +1578,17 @@ void main()
Color += vec4(1.0, 1.0, 1.0, 1.0);
break;
}
case 177:
{
Color = vec4(imageLoad(texBufferUint, int(zeroi+2)).x, 1, 0, 1);
Copy link
Collaborator

@Zorro666 Zorro666 Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

break;
}
case 178:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

{
imageStore(storeTexBufferUint, int(zeroi+2), uvec4(3, 4, 5, 2));
Color = imageLoad(storeTexBufferUint, int(zeroi+2));
break;
}
default: break;
}
}
Expand Down Expand Up @@ -4118,6 +4131,8 @@ OpMemberDecorate %cbuffer_struct 17 Offset 216 ; double doublePackSource
{30, VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER, 1, VK_SHADER_STAGE_FRAGMENT_BIT},
{31, VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER, 1, VK_SHADER_STAGE_FRAGMENT_BIT},
{32, VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE, 1, VK_SHADER_STAGE_FRAGMENT_BIT},
{33, VK_DESCRIPTOR_TYPE_UNIFORM_TEXEL_BUFFER, 1, VK_SHADER_STAGE_FRAGMENT_BIT},
{34, VK_DESCRIPTOR_TYPE_STORAGE_TEXEL_BUFFER, 1, VK_SHADER_STAGE_FRAGMENT_BIT},
}));

std::vector<VkDescriptorSetLayout> setLayouts = {setlayout0};
Expand Down Expand Up @@ -4643,6 +4658,11 @@ OpMemberDecorate %cbuffer_struct 17 Offset 216 ; double doublePackSource
VkBufferView store_bufview = createBufferView(
vkh::BufferViewCreateInfo(store_texbuffer.buffer, VK_FORMAT_R32G32B32A32_SFLOAT));

VkBufferView bufview_uint =
createBufferView(vkh::BufferViewCreateInfo(texbuffer.buffer, VK_FORMAT_R8G8B8A8_UINT));
VkBufferView store_bufview_uint =
createBufferView(vkh::BufferViewCreateInfo(store_texbuffer.buffer, VK_FORMAT_R8G8B8A8_UINT));

setName(pointsampler, "pointsampler");
setName(linearsampler, "linearsampler");
setName(mipsampler, "mipsampler");
Expand Down Expand Up @@ -4717,6 +4737,10 @@ OpMemberDecorate %cbuffer_struct 17 Offset 216 ; double doublePackSource
vkh::WriteDescriptorSet(
descset0, 32, VK_DESCRIPTOR_TYPE_SAMPLED_IMAGE,
{vkh::DescriptorImageInfo(shadowview, VK_IMAGE_LAYOUT_GENERAL, VK_NULL_HANDLE)}),
vkh::WriteDescriptorSet(descset0, 33, VK_DESCRIPTOR_TYPE_UNIFORM_TEXEL_BUFFER,
{bufview_uint}),
vkh::WriteDescriptorSet(descset0, 34, VK_DESCRIPTOR_TYPE_STORAGE_TEXEL_BUFFER,
{store_bufview_uint}),
});

if(descIndexing)
Expand Down