Skip to content

Commit 970a32a

Browse files
committed
Address PR review comments for Vulkan unit test framework
- Fix switch warning by explicitly handling VK_PHYSICAL_DEVICE_TYPE_OTHER and VK_PHYSICAL_DEVICE_TYPE_MAX_ENUM cases - Fix unused format parameter warning in transitionImageLayout - Move input/output buffers to high bindings (100, 101) to avoid conflicts with OCIO's uniform binding at 0, eliminating need to edit shader text - Refactor GPUUnitTest.cpp to share code between GL and Vulkan: - Extract PrepareInputValues helper for UpdateImageTexture - Extract ValidateResults and ValidateImageTextureImpl template for ValidateImageTexture - Preserve Mac ARM ifdef for NaN/Infinity test disabling in Vulkan path - Update texture binding start to 1 (was 2) since binding 0 is now used for OCIO uniforms
1 parent 5f983f7 commit 970a32a

2 files changed

Lines changed: 108 additions & 361 deletions

File tree

src/libutils/oglapphelpers/vulkanapp.cpp

Lines changed: 50 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,8 @@ void VulkanApp::initVulkan()
127127
case VK_PHYSICAL_DEVICE_TYPE_INTEGRATED_GPU: std::cout << "Integrated GPU"; break;
128128
case VK_PHYSICAL_DEVICE_TYPE_VIRTUAL_GPU: std::cout << "Virtual GPU"; break;
129129
case VK_PHYSICAL_DEVICE_TYPE_CPU: std::cout << "CPU (Software)"; break;
130+
case VK_PHYSICAL_DEVICE_TYPE_OTHER:
131+
case VK_PHYSICAL_DEVICE_TYPE_MAX_ENUM:
130132
default: std::cout << "Other"; break;
131133
}
132134
std::cout << std::endl;
@@ -408,11 +410,13 @@ void VulkanApp::setShader(GpuShaderDescRcPtr & shaderDesc)
408410
void VulkanApp::createComputePipeline()
409411
{
410412
// Create descriptor set layout
413+
// Use high binding numbers (100, 101) for input/output buffers to avoid conflicts with OCIO bindings
414+
// OCIO uses binding 0 for uniforms and 1+ for textures
411415
std::vector<VkDescriptorSetLayoutBinding> bindings = {
412416
// Input buffer binding
413-
{0, VK_DESCRIPTOR_TYPE_STORAGE_BUFFER, 1, VK_SHADER_STAGE_COMPUTE_BIT, nullptr},
417+
{100, VK_DESCRIPTOR_TYPE_STORAGE_BUFFER, 1, VK_SHADER_STAGE_COMPUTE_BIT, nullptr},
414418
// Output buffer binding
415-
{1, VK_DESCRIPTOR_TYPE_STORAGE_BUFFER, 1, VK_SHADER_STAGE_COMPUTE_BIT, nullptr}
419+
{101, VK_DESCRIPTOR_TYPE_STORAGE_BUFFER, 1, VK_SHADER_STAGE_COMPUTE_BIT, nullptr}
416420
};
417421

418422
// Add texture and uniform bindings from shader builder
@@ -494,10 +498,12 @@ void VulkanApp::createComputePipeline()
494498
outputBufferInfo.offset = 0;
495499
outputBufferInfo.range = bufferSize;
496500

501+
// Use high binding numbers (100, 101) to avoid conflicts with OCIO bindings
502+
// OCIO uses binding 0 for uniforms and 1+ for textures
497503
std::vector<VkWriteDescriptorSet> descriptorWrites = {
498-
{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET, nullptr, m_descriptorSet, 0, 0, 1,
504+
{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET, nullptr, m_descriptorSet, 100, 0, 1,
499505
VK_DESCRIPTOR_TYPE_STORAGE_BUFFER, nullptr, &inputBufferInfo, nullptr},
500-
{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET, nullptr, m_descriptorSet, 1, 0, 1,
506+
{VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET, nullptr, m_descriptorSet, 101, 0, 1,
501507
VK_DESCRIPTOR_TYPE_STORAGE_BUFFER, nullptr, &outputBufferInfo, nullptr}
502508
};
503509

@@ -772,7 +778,7 @@ VkSampler VulkanBuilder::createSampler(Interpolation interpolation)
772778
return sampler;
773779
}
774780

775-
void VulkanBuilder::transitionImageLayout(VkImage image, VkFormat format,
781+
void VulkanBuilder::transitionImageLayout(VkImage image, VkFormat /*format*/,
776782
VkImageLayout oldLayout, VkImageLayout newLayout)
777783
{
778784
VkCommandBufferAllocateInfo allocInfo{};
@@ -1263,82 +1269,27 @@ void VulkanBuilder::buildShader(GpuShaderDescRcPtr & shaderDesc)
12631269
shader << "\n";
12641270
shader << "layout(local_size_x = 16, local_size_y = 16, local_size_z = 1) in;\n";
12651271
shader << "\n";
1266-
shader << "layout(std430, set = 0, binding = 0) readonly buffer InputBuffer {\n";
1272+
// Use high binding numbers for input/output buffers to avoid conflicts with OCIO bindings.
1273+
// OCIO uses binding 0 for uniforms and 1+ for textures (via setDescriptorSetIndex(0, 1)).
1274+
// By using bindings 100 and 101 for I/O buffers, we avoid needing to edit OCIO's shader text.
1275+
shader << "layout(std430, set = 0, binding = 100) readonly buffer InputBuffer {\n";
12671276
shader << " vec4 inputPixels[];\n";
12681277
shader << "};\n";
12691278
shader << "\n";
1270-
shader << "layout(std430, set = 0, binding = 1) writeonly buffer OutputBuffer {\n";
1279+
shader << "layout(std430, set = 0, binding = 101) writeonly buffer OutputBuffer {\n";
12711280
shader << " vec4 outputPixels[];\n";
12721281
shader << "};\n";
12731282
shader << "\n";
12741283

12751284
// OCIO generates texture sampler declarations with correct bindings when using
1276-
// GPU_LANGUAGE_GLSL_VK_4_6 and setDescriptorSetIndex(0, 2) is called on the shader descriptor.
1277-
// Textures start at binding 2 (0=input buffer, 1=output buffer).
1278-
// The uniform block binding is set to 0 by OCIO but we need it after all textures.
1285+
// GPU_LANGUAGE_GLSL_VK_4_6 and setDescriptorSetIndex(0, 1) is called on the shader descriptor.
1286+
// OCIO uses binding 0 for uniforms (by design) and 1+ for textures.
12791287

1280-
// Get OCIO shader text - it already contains sampler declarations with correct bindings
1288+
// Get OCIO shader text - it already contains sampler and uniform declarations with correct bindings
12811289
const char * shaderText = shaderDesc->getShaderText();
12821290
if (shaderText && strlen(shaderText) > 0)
12831291
{
1284-
std::string ocioShader(shaderText);
1285-
1286-
// Calculate the binding for uniform blocks (after input/output buffers and textures)
1287-
// OCIO sets uniform block binding to 0, but we need it after all textures
1288-
uint32_t uniformBinding = 2 + static_cast<uint32_t>(m_textures3D.size() + m_textures1D2D.size());
1289-
1290-
std::string result;
1291-
std::istringstream stream(ocioShader);
1292-
std::string line;
1293-
1294-
while (std::getline(stream, line))
1295-
{
1296-
// Fix uniform block bindings and add std140 layout
1297-
// OCIO generates: layout (set = 0, binding = 0) uniform BlockName
1298-
// We need to change binding = 0 to binding = uniformBinding (after textures)
1299-
if (line.find("uniform") != std::string::npos &&
1300-
line.find("layout") != std::string::npos &&
1301-
line.find("binding") != std::string::npos &&
1302-
line.find("sampler") == std::string::npos)
1303-
{
1304-
// Replace binding = X with our calculated binding
1305-
size_t bindingPos = line.find("binding");
1306-
if (bindingPos != std::string::npos)
1307-
{
1308-
size_t eqPos = line.find("=", bindingPos);
1309-
if (eqPos != std::string::npos)
1310-
{
1311-
size_t numStart = eqPos + 1;
1312-
while (numStart < line.size() && (line[numStart] == ' ' || line[numStart] == '\t'))
1313-
numStart++;
1314-
size_t numEnd = numStart;
1315-
while (numEnd < line.size() && std::isdigit(line[numEnd]))
1316-
numEnd++;
1317-
1318-
if (numEnd > numStart)
1319-
{
1320-
line = line.substr(0, numStart) + std::to_string(uniformBinding) + line.substr(numEnd);
1321-
}
1322-
}
1323-
}
1324-
1325-
// Add std140 layout qualifier if not present
1326-
// OCIO calculates uniform buffer offsets using std140 rules
1327-
if (line.find("std140") == std::string::npos)
1328-
{
1329-
size_t layoutPos = line.find("layout");
1330-
size_t parenPos = line.find("(", layoutPos);
1331-
if (parenPos != std::string::npos)
1332-
{
1333-
line = line.substr(0, parenPos + 1) + "std140, " + line.substr(parenPos + 1);
1334-
}
1335-
}
1336-
}
1337-
1338-
result += line + "\n";
1339-
}
1340-
1341-
shader << result;
1292+
shader << shaderText;
13421293
}
13431294

13441295
shader << "\n";
@@ -1457,20 +1408,21 @@ std::vector<VkDescriptorSetLayoutBinding> VulkanBuilder::getDescriptorSetLayoutB
14571408
{
14581409
std::vector<VkDescriptorSetLayoutBinding> bindings;
14591410

1460-
// Add bindings for 3D LUT textures (starting at binding 2)
1461-
for (const auto & tex : m_textures3D)
1411+
// Add uniform buffer binding at binding 0 (OCIO's default)
1412+
// OCIO generates uniform blocks with binding = 0 by design
1413+
if (hasUniforms())
14621414
{
14631415
VkDescriptorSetLayoutBinding binding{};
1464-
binding.binding = tex.binding;
1465-
binding.descriptorType = VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER;
1416+
binding.binding = 0;
1417+
binding.descriptorType = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER;
14661418
binding.descriptorCount = 1;
14671419
binding.stageFlags = VK_SHADER_STAGE_COMPUTE_BIT;
14681420
binding.pImmutableSamplers = nullptr;
14691421
bindings.push_back(binding);
14701422
}
14711423

1472-
// Add bindings for 1D/2D LUT textures
1473-
for (const auto & tex : m_textures1D2D)
1424+
// Add bindings for 3D LUT textures (starting at binding 1 via setDescriptorSetIndex(0, 1))
1425+
for (const auto & tex : m_textures3D)
14741426
{
14751427
VkDescriptorSetLayoutBinding binding{};
14761428
binding.binding = tex.binding;
@@ -1481,13 +1433,12 @@ std::vector<VkDescriptorSetLayoutBinding> VulkanBuilder::getDescriptorSetLayoutB
14811433
bindings.push_back(binding);
14821434
}
14831435

1484-
// Add uniform buffer binding after textures (matches shader generation)
1485-
if (hasUniforms())
1436+
// Add bindings for 1D/2D LUT textures
1437+
for (const auto & tex : m_textures1D2D)
14861438
{
1487-
uint32_t uniformBinding = 2 + static_cast<uint32_t>(m_textures3D.size() + m_textures1D2D.size());
14881439
VkDescriptorSetLayoutBinding binding{};
1489-
binding.binding = uniformBinding;
1490-
binding.descriptorType = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER;
1440+
binding.binding = tex.binding;
1441+
binding.descriptorType = VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER;
14911442
binding.descriptorCount = 1;
14921443
binding.stageFlags = VK_SHADER_STAGE_COMPUTE_BIT;
14931444
binding.pImmutableSamplers = nullptr;
@@ -1531,7 +1482,25 @@ void VulkanBuilder::updateDescriptorSet(VkDescriptorSet descriptorSet)
15311482
// Reserve space to prevent reallocation
15321483
imageInfos.reserve(m_textures3D.size() + m_textures1D2D.size());
15331484

1534-
// Add 3D texture bindings first (starting at binding 2)
1485+
// Add uniform buffer binding at binding 0 (OCIO's default)
1486+
if (hasUniforms())
1487+
{
1488+
uniformBufferInfo.buffer = m_uniformBuffer;
1489+
uniformBufferInfo.offset = 0;
1490+
uniformBufferInfo.range = m_uniformBufferSize;
1491+
1492+
VkWriteDescriptorSet uniformWrite{};
1493+
uniformWrite.sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET;
1494+
uniformWrite.dstSet = descriptorSet;
1495+
uniformWrite.dstBinding = 0;
1496+
uniformWrite.dstArrayElement = 0;
1497+
uniformWrite.descriptorType = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER;
1498+
uniformWrite.descriptorCount = 1;
1499+
uniformWrite.pBufferInfo = &uniformBufferInfo;
1500+
descriptorWrites.push_back(uniformWrite);
1501+
}
1502+
1503+
// Add 3D texture bindings (starting at binding 1 via setDescriptorSetIndex(0, 1))
15351504
for (const auto & tex : m_textures3D)
15361505
{
15371506
VkDescriptorImageInfo imageInfo{};
@@ -1571,25 +1540,6 @@ void VulkanBuilder::updateDescriptorSet(VkDescriptorSet descriptorSet)
15711540
descriptorWrites.push_back(imageWrite);
15721541
}
15731542

1574-
// Add uniform buffer binding after textures (matches shader generation)
1575-
if (hasUniforms())
1576-
{
1577-
uint32_t uniformBinding = 2 + static_cast<uint32_t>(m_textures3D.size() + m_textures1D2D.size());
1578-
uniformBufferInfo.buffer = m_uniformBuffer;
1579-
uniformBufferInfo.offset = 0;
1580-
uniformBufferInfo.range = m_uniformBufferSize;
1581-
1582-
VkWriteDescriptorSet uniformWrite{};
1583-
uniformWrite.sType = VK_STRUCTURE_TYPE_WRITE_DESCRIPTOR_SET;
1584-
uniformWrite.dstSet = descriptorSet;
1585-
uniformWrite.dstBinding = uniformBinding;
1586-
uniformWrite.dstArrayElement = 0;
1587-
uniformWrite.descriptorType = VK_DESCRIPTOR_TYPE_UNIFORM_BUFFER;
1588-
uniformWrite.descriptorCount = 1;
1589-
uniformWrite.pBufferInfo = &uniformBufferInfo;
1590-
descriptorWrites.push_back(uniformWrite);
1591-
}
1592-
15931543
if (!descriptorWrites.empty())
15941544
{
15951545
vkUpdateDescriptorSets(m_device, static_cast<uint32_t>(descriptorWrites.size()),

0 commit comments

Comments
 (0)