Conversation
Fixes a couple minor Vulkan Validation Layer warnings: * Enable additional validation layer features * Move physical device surface queries to after device creation * Mark depth/stencil attachments as transient and fix their dependency flag bits * Fix issue where frame completion semaphore count needed to match the number of potential swapchain images, not just the in-flight frame count
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @natetrost, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request. This PR aims to address several minor warnings reported by the Vulkan Validation Layers (VVL) in the AGDK Tunnel sample. The changes involve enabling additional validation features, adjusting the timing of physical device surface queries, correctly marking depth/stencil attachments as transient with appropriate dependency flags, and fixing an issue where the frame completion semaphore count was tied to the number of in-flight frames rather than the total number of swapchain images.
Highlights
- Validation Layers: Enables the
VK_EXT_VALIDATION_FEATURES_EXTENSION_NAMEand specifically theVK_VALIDATION_FEATURE_ENABLE_SYNCHRONIZATION_VALIDATION_EXTfeature to catch more synchronization-related issues. - Query Timing: Moves the querying of surface capabilities, device capabilities, display formats, and screen resolutions to occur after the Vulkan device has been successfully created, which is a more appropriate time.
- Depth/Stencil Attachments: Marks depth/stencil images as
VK_IMAGE_USAGE_TRANSIENT_ATTACHMENT_BITand changes their memory allocation usage toVMA_MEMORY_USAGE_GPU_LAZILY_ALLOCATED. Also updates the render pass subpass dependency to includeVK_PIPELINE_STAGE_EARLY_FRAGMENT_TESTS_BITandVK_ACCESS_DEPTH_STENCIL_ATTACHMENT_WRITE_BITfor depth/stencil. - Synchronization Objects: Corrects the sizing and indexing logic for the
frame_render_completion_semaphore_array, ensuring it matches the total number of swapchain images rather than just the number of in-flight frames. This fixes a potential issue where the semaphore index could go out of bounds.
Changelog
Click here to see the changelog
- agdk/agdktunnel/app/build.gradle
- Updated application
versionCodeandversionNamefrom 1.2.4 to 1.2.5.
- Updated application
- agdk/common/base_game_framework/src/android/platform_util_vulkan_android.cpp
- Added
swap_intervals.clear()before populating the vector inGetRefreshRates. - Added
VK_EXT_VALIDATION_FEATURES_EXTENSION_NAMEto required instance extensions when validation layers are enabled. - Added
display_resolutions.clear()before populating the vector inGetScreenResolutions.
- Added
- agdk/common/base_game_framework/src/vulkan/graphics_api_vulkan.cpp
- Removed physical device surface/device capability queries from
QueryCapabilities. - Corrected the index used for
frame_render_completion_semaphore_toswapchain_current_image_index_inPresentCurrentSwapchainFrameandGetSwapchainFrameResourcesVk. - Removed redundant depth/stencil frame index increment logic in
PresentCurrentSwapchainFrame. - Set
swapchain_current_depth_stencil_frame_index_toswapchain_current_image_index_after acquiring the next image. - Moved physical device surface/device capability queries to
CreateDevice, after device creation. - Added
VK_VALIDATION_FEATURE_ENABLE_SYNCHRONIZATION_VALIDATION_EXTto enabled validation features viaVkValidationFeaturesEXT. - Added
VK_IMAGE_USAGE_TRANSIENT_ATTACHMENT_BITusage flag and changed VMA usage toVMA_MEMORY_USAGE_GPU_LAZILY_ALLOCATEDfor depth/stencil images. - Resized
frame_render_completion_semaphore_toswapchain_image_count_instead ofin_flight_frame_count. - Adjusted the loop bounds for creating and destroying
frame_render_completion_semaphore_to matchswapchain_image_count_.
- Removed physical device surface/device capability queries from
- agdk/common/base_game_framework/src/vulkan/graphics_api_vulkan_utils.cpp
- Added
display_formats.clear()before populating the vector inGetDisplayFormats.
- Added
- agdk/common/simple_renderer/renderer_render_pass_vk.cpp
- Updated the subpass dependency's
dstStageMaskto includeVK_PIPELINE_STAGE_EARLY_FRAGMENT_TESTS_BITanddstAccessMaskto includeVK_ACCESS_DEPTH_STENCIL_ATTACHMENT_WRITE_BIT.
- Updated the subpass dependency's
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Vulkan warnings cease,
Transient depth, sync increase,
Validation peace.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request addresses several Vulkan Validation Layer warnings and improves overall correctness and efficiency. The changes include enabling more validation features, correctly positioning physical device queries, marking depth/stencil attachments as transient, fixing semaphore counts and indexing, and adjusting subpass dependency flags. These are valuable improvements.
Summary of Findings
- Semaphore Indexing and Sizing: Critical fixes were made to ensure
frame_render_completion_semaphore_usesswapchain_current_image_index_for indexing and is sized according toswapchain_image_count_. This is essential for correct Vulkan synchronization and resource management. - Transient Depth/Stencil Attachments: Depth/stencil attachments are now marked as transient (
VK_IMAGE_USAGE_TRANSIENT_ATTACHMENT_BIT) and allocated withVMA_MEMORY_USAGE_GPU_LAZILY_ALLOCATED. This is a good performance and memory optimization. - Subpass Dependency Correction: The subpass dependency flags (
dstStageMaskanddstAccessMask) were updated to correctly include stages and access types for depth/stencil operations. This is important for rendering correctness and resolving validation warnings. - Output Vector Clearing: Several functions that populate output vectors (
swap_intervals,display_resolutions,display_formats) now clear these vectors at the beginning. This is good defensive programming to prevent data accumulation from reused vector instances. - Query Relocation: Physical device surface queries were moved to after logical device creation, which is a more robust approach.
- Validation Layer Feature Enablement (Not Commented): The PR enables
VK_EXT_VALIDATION_FEATURES_EXTENSION_NAMEand specificallyVK_VALIDATION_FEATURE_ENABLE_SYNCHRONIZATION_VALIDATION_EXT. This is a low-severity improvement beneficial for debugging and was not commented on directly due to review settings.
Merge Readiness
The pull request implements several important fixes and improvements for Vulkan handling, particularly around synchronization, resource management, and validation. The changes appear to correctly address the issues described. Given the high severity of some of the addressed issues (like semaphore handling and subpass dependencies), these fixes are crucial. I recommend merging these changes after any outstanding discussions are resolved. As an AI, I am not authorized to approve pull requests; please ensure a human reviewer provides final approval.
| in_flight_frame_fence_.resize(in_flight_frame_count); | ||
| frame_render_completion_semaphore_.resize(in_flight_frame_count); | ||
| swapchain_image_semaphore_.resize(in_flight_frame_count); | ||
| frame_render_completion_semaphore_.resize(swapchain_info_.swapchain_image_count_); |
There was a problem hiding this comment.
Resizing frame_render_completion_semaphore_ to swapchain_info_.swapchain_image_count_ instead of in_flight_frame_count is a critical fix. This ensures there's a semaphore for each potential swapchain image, preventing out-of-bounds access or insufficient synchronization primitives if the number of swapchain images exceeds the number of in-flight frames.
| subpass_dependency.dstStageMask = VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT | VK_PIPELINE_STAGE_EARLY_FRAGMENT_TESTS_BIT; | ||
| subpass_dependency.dstAccessMask = VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT | VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_WRITE_BIT; |
There was a problem hiding this comment.
Updating dstStageMask to include VK_PIPELINE_STAGE_EARLY_FRAGMENT_TESTS_BIT and dstAccessMask to include VK_ACCESS_DEPTH_STENCIL_ATTACHMENT_WRITE_BIT in the subpass dependency is crucial for correctness, especially when dealing with depth/stencil attachments. This ensures that operations on the depth/stencil buffer are correctly synchronized with the color attachment operations. This likely resolves validation warnings related to missing synchronization for depth/stencil writes.
| if (swapchain_acquired_current_frame_image_) { | ||
| VkSemaphore wait_semaphores[] = | ||
| {frame_render_completion_semaphore_[swapchain_info_.swapchain_current_frame_index_]}; | ||
| {frame_render_completion_semaphore_[swapchain_info_.swapchain_current_image_index_]}; |
There was a problem hiding this comment.
Using swapchain_info_.swapchain_current_image_index_ for frame_render_completion_semaphore_ seems correct, especially given the PR description about matching semaphore count to potential swapchain images. Could you confirm if swapchain_current_image_index_ is always a valid index for frame_render_completion_semaphore_ which is now sized to swapchain_image_count_? This change is critical for correct synchronization.
| for (size_t i = 0; i < frame_render_completion_semaphore_.size(); ++i) { | ||
| vkDestroySemaphore(vk_device_, frame_render_completion_semaphore_[i], nullptr); | ||
| } |
There was a problem hiding this comment.
The loop for destroying frame_render_completion_semaphore_ now correctly iterates over frame_render_completion_semaphore_.size(), which reflects the swapchain_image_count_. This ensures all created semaphores are properly destroyed, preventing resource leaks.
| swapchain_image_semaphore_[swapchain_info_.swapchain_current_frame_index_]; | ||
| frame_resources.render_complete = | ||
| frame_render_completion_semaphore_[swapchain_info_.swapchain_current_frame_index_]; | ||
| frame_render_completion_semaphore_[swapchain_info_.swapchain_current_image_index_]; |
There was a problem hiding this comment.
Consistent with the change in PresentCurrentSwapchainFrame, using swapchain_info_.swapchain_current_image_index_ for indexing frame_render_completion_semaphore_ is important for correctness. This ensures the correct semaphore corresponding to the acquired image is used.
| ds_image_info.usage = VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT | | ||
| VK_IMAGE_USAGE_TRANSIENT_ATTACHMENT_BIT; |
There was a problem hiding this comment.
Adding VK_IMAGE_USAGE_TRANSIENT_ATTACHMENT_BIT to ds_image_info.usage is a good optimization for depth/stencil images that don't need to be preserved beyond the render pass. This aligns well with the change to VMA_MEMORY_USAGE_GPU_LAZILY_ALLOCATED.
| if (vk_device_ != VK_NULL_HANDLE) { | ||
| QuerySurfaceCapabilities(); | ||
| QueryDeviceCapabilities(vk_physical_device_); | ||
| VulkanAPIUtils::GetDisplayFormats(vk_physical_device_, vk_surface_, display_formats_); | ||
| PlatformUtilVulkan::GetScreenResolutions(surface_capabilities_, display_resolutions_); | ||
| } |
There was a problem hiding this comment.
Moving the surface and device capability queries to after vk_device_ creation (and ensuring it's not VK_NULL_HANDLE) is a good refactoring. This ensures that these queries are performed at a stage where the physical device is fully enumerated and a logical device context is available, which can be necessary for some queries or for consistency. Does this new placement resolve specific validation warnings or improve robustness?
| uint32_t queue_family_index, | ||
| std::vector<DisplayManager::DisplaySwapInterval> & | ||
| swap_intervals) { | ||
| swap_intervals.clear(); |
There was a problem hiding this comment.
Is swap_intervals always expected to be empty when this function is called, or is this clear() added as a defensive measure? If it's possible for swap_intervals to contain pre-existing data that should be preserved or handled differently under certain conditions, clearing it unconditionally might hide a logic issue elsewhere. However, if this function is always meant to populate it from a clean state, then this clear() is good practice to ensure no stale data persists from previous calls, especially if the vector instance is reused.
|
|
||
| void VulkanAPIUtils::GetDisplayFormats(VkPhysicalDevice physical_device, VkSurfaceKHR surface, | ||
| std::vector<DisplayManager::DisplayFormat> &display_formats) { | ||
| display_formats.clear(); |
There was a problem hiding this comment.
Clearing display_formats at the beginning of GetDisplayFormats is good practice, ensuring that the output vector is always populated from a clean state. This prevents accidental accumulation of data if the vector instance is reused across multiple calls.
|
|
||
| void PlatformUtilVulkan::GetScreenResolutions(const VkSurfaceCapabilitiesKHR &capabilities, | ||
| std::vector<DisplayManager::DisplayResolution> &display_resolutions) { | ||
| display_resolutions.clear(); |
There was a problem hiding this comment.
Similar to the swap_intervals.clear() comment, is display_resolutions intended to be populated from a clean slate here? If so, this clear() is a good safeguard against accumulating data across multiple calls with a reused vector instance. If there are scenarios where display_resolutions might have relevant pre-existing data, this could mask other issues. Assuming it's meant to be freshly populated, this is a good addition.
1 participant
Fixes a couple minor Vulkan Validation Layer warnings:
dependency flag bits
to match the number of potential swapchain images, not just
the in-flight frame count