add ycbcr support to json_gen layer - #17
Conversation
|
|
||
| if (!ycbcrFeatures.samplerYcbcrConversion) { | ||
| std::cout << "YCbCr conversion not supported, skipping YCbCr sampler test" << std::endl; | ||
| return VK_SUCCESS; |
There was a problem hiding this comment.
I think this VK_SUCCESS and print is OK behavior for feature not supported in the test jsonGenLayerTests/main.cpp .
comment here to confirm.
There was a problem hiding this comment.
on further reflection, this would cause tests to fail if the test system doesn't support ycbcr, since UUID & JSON output would change.
We would probably need to accept multiple pipeline UUIDs / golden JSONs. It looks like the CI runs fail because of other issues, and (IIUC) this code is intended to be largely rewritten soon. Should we resolve that before merging this patch, or just ignore it for now?
| #define INDENT(sz) s_num_spaces += (sz); | ||
|
|
||
| // Forward declaration for layer-provided accessor used by serializer | ||
| int GetYcbcrConversionIdForPrint(VkSamplerYcbcrConversion conversion); |
There was a problem hiding this comment.
print_VkSamplerYcbcrConversion calls the declared function GetYcbcrConversionIdForPrint .
will the symbol get resolved at link time, or at dynamic load time ?
There was a problem hiding this comment.
it should be resolved at link time. IIRC, LLM code generation put the implementation in vulkan_json_layer.hpp because s_pipe wasn't accessible from vulkan_json_data.hpp. But, if vulkan_json_data.hpp gets reused by CTS, that'll probably cause problems too. :/
I'll think about this more tomorrow.
| } | ||
| } else { | ||
| if (commaNeeded) | ||
| _OUT << "\"NULL\"" << "," << std::endl; |
There was a problem hiding this comment.
review-code.mdc comment, between these lines :
-
_OUT << "\"\"" << "," << std::endl; -
_OUT << "\"NULL\"" << "," << std::endl;
Inconsistent Error Handling: The function returns different values for invalid conversion IDs (empty string vs "NULL").
Evidence: Lines 14537-40 return empty string, while lines 14559-62 return "NULL"
Proposed Comment to be Included: Standardize error return values for invalid YCbCr conversion IDs (use either empty string or "NULL" consistently)
Justification for Commenting: This is valid (inconsistent error handling exists), critical (affects API consistency), actionable (can standardize return values), and restrained in scope (only affects error return values)
There was a problem hiding this comment.
standardized error case to empty string in both cases
| while (pNext != nullptr) {{ | ||
| if (pNext->sType == VK_STRUCTURE_TYPE_SAMPLER_YCBCR_CONVERSION_INFO) {{ | ||
| const VkSamplerYcbcrConversionInfo* ycbcrInfo = reinterpret_cast<const VkSamplerYcbcrConversionInfo*>(pNext); | ||
| vk_json::s_pipe.setYcbcrSamplerConversionAssociation(*pSampler, ycbcrInfo->conversion); |
There was a problem hiding this comment.
setSamplerInfo , which this vkCreateSampler template calls above , also calls setYcbcrSamplerConversionAssociation .
in vulkan_json_layer.hpp :
// Store a copy of the YCbCr conversion info
m_samplerYcbcrInfoMap.insert(std::make_pair(*pSampler, *ycbcrInfo));
// Update the sampler create info to point to our persistent copy
samplerInfo.pNext = &m_samplerYcbcrInfoMap[*pSampler];
// Also store the association
setYcbcrSamplerConversionAssociation(*pSampler, ycbcrInfo->conversion);
There was a problem hiding this comment.
removed the redundant code in json_gen_generator.py, thanks for spotting this
3 participants
No description provided.