Minor refactor for vulkan renderer#176
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors Vulkan memory type retrieval by moving FindMemoryType from individual texture classes to TizenRendererVulkan, adds a null check for pixel_buffer->buffer, and introduces error handling for vkMapMemory. Review feedback suggests refactoring CopyBufferToImage to propagate mapping failures and checking for null command buffers to prevent crashes. Additionally, it is recommended to ensure subclasses of ExternalTextureSurfaceVulkanBuffer can still access FindMemoryType, and to use pointers instead of references for output parameters in FindMemoryType to align with the Google C++ Style Guide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| VkResult result = | ||
| vkMapMemory(GetDevice(), staging_buffer_memory_, 0, size, 0, &data); | ||
| if (result != VK_SUCCESS) { | ||
| FT_LOG(Error) << "Failed to map staging buffer memory"; | ||
| return; | ||
| } |
There was a problem hiding this comment.
There are two correctness and stability issues in this error handling path:
- Lack of Error Propagation:
CopyBufferToImagereturnsvoid. IfvkMapMemoryfails, it returns early, but the callerPopulateVulkanTextureis unaware of the failure and will proceed to returntruewith an uninitialized or outdated texture. Consider refactoringCopyBufferToImageto return abooland checking its return value inPopulateVulkanTextureto handle the failure gracefully. - Unchecked Command Buffer: Although not shown in this specific diff hunk,
vulkan_renderer_->BeginSingleTimeCommands()(called on line 193) can returnVK_NULL_HANDLEon failure. If it does, callingvkCmdCopyBufferToImageandEndSingleTimeCommandswith a null command buffer will result in undefined behavior or a crash. Please add a check to ensurecommand_bufferis notVK_NULL_HANDLEbefore proceeding.
| bool FindMemoryType(uint32_t memory_type_bits_requirement, | ||
| VkMemoryPropertyFlags required_properties, | ||
| uint32_t& index_out); | ||
| VkDevice GetDevice() const; |
There was a problem hiding this comment.
Since ExternalTextureSurfaceVulkanBuffer is an abstract base class with pure virtual methods like AllocateAndBindMemory (which subclasses must implement), those subclasses will likely need to call FindMemoryType to allocate Vulkan memory.
However, FindMemoryType has been removed from this class, and vulkan_renderer_ is private. This means subclasses cannot access vulkan_renderer_->FindMemoryType directly.
To resolve this without duplicating the vulkan_renderer_ pointer in subclasses (which would violate the 'Avoid duplicating state' principle), consider making vulkan_renderer_ protected, providing a protected getter, or keeping a protected FindMemoryType helper that forwards the call to vulkan_renderer_.
References
- Avoid duplicating state: Keep only one source of truth. (link)
| bool FindMemoryType(uint32_t type_filter, | ||
| VkMemoryPropertyFlags properties, | ||
| uint32_t& index_out); |
There was a problem hiding this comment.
According to the Google C++ Style Guide, output parameters should be passed as pointers rather than references (e.g., uint32_t* index_out instead of uint32_t& index_out). This makes it clear at the call site that the argument may be modified.
| bool FindMemoryType(uint32_t type_filter, | |
| VkMemoryPropertyFlags properties, | |
| uint32_t& index_out); | |
| bool FindMemoryType(uint32_t type_filter, | |
| VkMemoryPropertyFlags properties, | |
| uint32_t* index_out); |
References
- Google C++ Style Guide: Prefer using return values instead of output parameters. If you must use output parameters, they should be pointers, not references. (link)
| bool TizenRendererVulkan::FindMemoryType(uint32_t type_filter, | ||
| VkMemoryPropertyFlags properties, | ||
| uint32_t& index_out) { | ||
| VkPhysicalDeviceMemoryProperties memory_properties; | ||
| vkGetPhysicalDeviceMemoryProperties(physical_device_, &memory_properties); | ||
|
|
||
| for (uint32_t i = 0; i < memory_properties.memoryTypeCount; i++) { | ||
| if ((type_filter & (1 << i)) && | ||
| (memory_properties.memoryTypes[i].propertyFlags & properties) == | ||
| properties) { | ||
| index_out = i; | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Update the implementation of FindMemoryType to accept a pointer for the output parameter index_out instead of a reference, adhering to the Google C++ Style Guide.
bool TizenRendererVulkan::FindMemoryType(uint32_t type_filter,
VkMemoryPropertyFlags properties,
uint32_t* index_out) {
VkPhysicalDeviceMemoryProperties memory_properties;
vkGetPhysicalDeviceMemoryProperties(physical_device_, &memory_properties);
for (uint32_t i = 0; i < memory_properties.memoryTypeCount; i++) {
if ((type_filter & (1 << i)) &&
(memory_properties.memoryTypes[i].propertyFlags & properties) ==
properties) {
if (index_out) {
*index_out = i;
}
return true;
}
}
return false;
}References
- Google C++ Style Guide: Prefer using return values instead of output parameters. If you must use output parameters, they should be pointers, not references. (link)
| if (!vulkan_renderer_->FindMemoryType(memory_requirements.memoryTypeBits, | ||
| properties, memory_type_index)) { |
There was a problem hiding this comment.
Update the call to FindMemoryType to pass the address of memory_type_index as a pointer, matching the updated signature.
| if (!vulkan_renderer_->FindMemoryType(memory_requirements.memoryTypeBits, | |
| properties, memory_type_index)) { | |
| if (!vulkan_renderer_->FindMemoryType(memory_requirements.memoryTypeBits, | |
| properties, &memory_type_index)) { |
References
- Google C++ Style Guide: Prefer using return values instead of output parameters. If you must use output parameters, they should be pointers, not references. (link)
Main changes: