Skip to content

Fix minor VVL warnings #94

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

Merged
natetrost merged 2 commits into from
May 29, 2025
Merged

Fix minor VVL warnings #94

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
b1ffbc2
Fix minor VVL warnings
natetrost May 28, 2025
bf12294
Merge remote-tracking branch 'refs/remotes/origin/main'
natetrost May 28, 2025
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
4 changes: 2 additions & 2 deletions agdk/agdktunnel/app/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ android {
applicationId 'com.google.sample.agdktunnel'
minSdkVersion 24
targetSdkVersion 36
versionCode 124
versionName '1.2.4'
versionCode 125
versionName '1.2.5'

externalNativeBuild {
cmake {
Expand Down
3 changes: 3 additions & 0 deletions agdk/common/base_game_framework/src/android/platform_util_vulkan_android.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ void PlatformUtilVulkan::GetRefreshRates(VkPhysicalDevice physical_device,
uint32_t queue_family_index,
std::vector<DisplayManager::DisplaySwapInterval> &
swap_intervals) {
swap_intervals.clear();
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 29, 2025

Choose a reason for hiding this comment

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

medium

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.

SwappyVk_setQueueFamilyIndex(device, queue, queue_family_index);

uint64_t refresh_duration = 0;
Expand Down Expand Up @@ -228,6 +229,7 @@ std::vector<const char *> PlatformUtilVulkan::GetRequiredInstanceExtensions(
instance_extensions.push_back(VK_KHR_ANDROID_SURFACE_EXTENSION_NAME);
if (enable_validation_layers) {
instance_extensions.push_back(VK_EXT_DEBUG_UTILS_EXTENSION_NAME);
instance_extensions.push_back(VK_EXT_VALIDATION_FEATURES_EXTENSION_NAME);
}
// Request the physical device properties 2 extension on 1.0, is core from
// 1.1+
Expand All @@ -240,6 +242,7 @@ std::vector<const char *> PlatformUtilVulkan::GetRequiredInstanceExtensions(

void PlatformUtilVulkan::GetScreenResolutions(const VkSurfaceCapabilitiesKHR &capabilities,
std::vector<DisplayManager::DisplayResolution> &display_resolutions) {
display_resolutions.clear();
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 29, 2025

Choose a reason for hiding this comment

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

medium

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.

const bool rotated = (capabilities.currentTransform & VK_SURFACE_TRANSFORM_ROTATE_90_BIT_KHR ||
capabilities.currentTransform & VK_SURFACE_TRANSFORM_ROTATE_270_BIT_KHR);
const int32_t display_width = capabilities.currentExtent.width;
Expand Down
49 changes: 34 additions & 15 deletions agdk/common/base_game_framework/src/vulkan/graphics_api_vulkan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,6 @@ void GraphicsAPIVulkan::QueryCapabilities() {
if (vk_surface_ != VK_NULL_HANDLE) {
vk_physical_device_ = GetPhysicalDevice();
if (vk_physical_device_ != VK_NULL_HANDLE) {
QuerySurfaceCapabilities();
QueryDeviceCapabilities(vk_physical_device_);
VulkanAPIUtils::GetDisplayFormats(vk_physical_device_, vk_surface_, display_formats_);
PlatformUtilVulkan::GetScreenResolutions(surface_capabilities_, display_resolutions_);

QueueFamilyIndices queue_indices = VulkanAPIUtils::FindQueueFamilies(
vk_physical_device_, vk_surface_);
if (CreateDevice(true, queue_indices)) {
Expand Down Expand Up @@ -607,7 +602,7 @@ DisplayManager::SwapchainFrameHandle GraphicsAPIVulkan::GetCurrentSwapchainFrame
DisplayManager::SwapchainFrameHandle GraphicsAPIVulkan::PresentCurrentSwapchainFrame() {
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_]};
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 29, 2025

Choose a reason for hiding this comment

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

high

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.


VkPresentInfoKHR present_info{};
present_info.sType = VK_STRUCTURE_TYPE_PRESENT_INFO_KHR;
Expand Down Expand Up @@ -649,9 +644,6 @@ DisplayManager::SwapchainFrameHandle GraphicsAPIVulkan::PresentCurrentSwapchainF
swapchain_info_.swapchain_current_frame_index_ =
(swapchain_info_.swapchain_current_frame_index_ + 1) %
swapchain_info_.swapchain_frame_count_;
swapchain_info_.swapchain_current_depth_stencil_frame_index_ =
(swapchain_info_.swapchain_current_depth_stencil_frame_index_ + 1) %
swapchain_info_.swapchain_image_count_;
} else {
DebugManager::Log(DebugManager::kLog_Channel_Default,
DebugManager::kLog_Level_Error,
Expand Down Expand Up @@ -705,6 +697,9 @@ bool GraphicsAPIVulkan::GetSwapchainFrameResourcesVk(
"vkAcquireNextImageKHR failed: %d", result);
return false;
}
// TODO: this could just be the number of in-flight frames, but since they
// are now transient they shouldn't have memoery overhead (at least on mobile)
swapchain_info_.swapchain_current_depth_stencil_frame_index_ = swapchain_info_.swapchain_current_image_index_;
Comment on lines +700 to +702
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 29, 2025

Choose a reason for hiding this comment

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

medium

The TODO comment here is helpful for future optimization. Assigning swapchain_info_.swapchain_current_depth_stencil_frame_index_ to swapchain_info_.swapchain_current_image_index_ aligns with the idea that each swapchain image might have its own transient depth/stencil buffer. This seems like a reasonable approach for now, especially with transient attachments.

swapchain_acquired_current_frame_image_ = true;

vkResetFences(vk_device_, 1,
Expand All @@ -716,7 +711,7 @@ bool GraphicsAPIVulkan::GetSwapchainFrameResourcesVk(
frame_resources.image_available =
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_];
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 29, 2025

Choose a reason for hiding this comment

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

high

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.

frame_resources.swapchain_color_image_view =
swapchain_info_.swapchain_image_views_[swapchain_info_.swapchain_current_image_index_];
frame_resources.swapchain_depth_stencil_image_view =
Expand Down Expand Up @@ -839,6 +834,13 @@ bool GraphicsAPIVulkan::CreateDevice(bool is_preflight_check,
vkCreateDebugUtilsMessengerEXT(vk_instance_, &messenger_create_info, nullptr,
&debug_messenger_);
}

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_);
}
Comment on lines +838 to +843
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 29, 2025

Choose a reason for hiding this comment

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

medium

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?

return (vk_device_ != VK_NULL_HANDLE);
}

Expand Down Expand Up @@ -885,6 +887,16 @@ void GraphicsAPIVulkan::CreateInstance(bool is_preflight_check) {
PlatformUtilVulkan::InitMessengerCreateInfo(messenger_create_info);
messenger_create_info.pfnUserCallback = messengerCallback;
create_info.pNext = &messenger_create_info;
VkValidationFeaturesEXT features = {};
VkValidationFeatureEnableEXT enables[] = {
// Best practices is a little spammy from Swappy at the moment
// VK_VALIDATION_FEATURE_ENABLE_BEST_PRACTICES_EXT,
VK_VALIDATION_FEATURE_ENABLE_SYNCHRONIZATION_VALIDATION_EXT
};
features.sType = VK_STRUCTURE_TYPE_VALIDATION_FEATURES_EXT;
features.enabledValidationFeatureCount = sizeof(enables) / sizeof(enables[0]);
features.pEnabledValidationFeatures = enables;
messenger_create_info.pNext = &features;
} else {
create_info.enabledLayerCount = 0;
create_info.ppEnabledLayerNames = nullptr;
Expand Down Expand Up @@ -1061,13 +1073,16 @@ void GraphicsAPIVulkan::CreateSwapchainImages() {
ds_image_info.format = swapchain_info_.swapchain_depth_stencil_format_;
ds_image_info.tiling = VK_IMAGE_TILING_OPTIMAL;
ds_image_info.initialLayout = VK_IMAGE_LAYOUT_UNDEFINED;
ds_image_info.usage = VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT;
ds_image_info.usage = VK_IMAGE_USAGE_DEPTH_STENCIL_ATTACHMENT_BIT |
VK_IMAGE_USAGE_TRANSIENT_ATTACHMENT_BIT;
Comment on lines +1076 to +1077
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 29, 2025

Choose a reason for hiding this comment

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

medium

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.

ds_image_info.sharingMode = VK_SHARING_MODE_EXCLUSIVE;
ds_image_info.samples = VK_SAMPLE_COUNT_1_BIT;
ds_image_info.flags = 0;

VmaAllocationCreateInfo ds_image_alloc_create_info = {};
ds_image_alloc_create_info.usage = VMA_MEMORY_USAGE_AUTO;
// Depth-stencil images paired with a swapchain will be don't care for store
// so they can be lazily allocated
ds_image_alloc_create_info.usage = VMA_MEMORY_USAGE_GPU_LAZILY_ALLOCATED;
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 29, 2025

Choose a reason for hiding this comment

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

medium

Changing VMA usage to VMA_MEMORY_USAGE_GPU_LAZILY_ALLOCATED for transient depth-stencil images is an excellent choice. This allows the memory allocator and driver to potentially optimize memory usage significantly for these attachments, possibly avoiding backing them with physical memory if they are only used on-tile. This should improve memory efficiency.


VulkanAPIUtils::CheckVkResult(
vmaCreateImage(allocator_, &ds_image_info,
Expand Down Expand Up @@ -1122,8 +1137,8 @@ void GraphicsAPIVulkan::DestroySwapchainImages() {
void GraphicsAPIVulkan::CreateSynchronizationObjects() {
const size_t in_flight_frame_count = DisplayManager::GetInstance().GetDisplayBufferMode();
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_);
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 29, 2025

Choose a reason for hiding this comment

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

high

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.


VkSemaphoreCreateInfo semaphore_info{};
semaphore_info.sType = VK_STRUCTURE_TYPE_SEMAPHORE_CREATE_INFO;
Expand All @@ -1132,11 +1147,13 @@ void GraphicsAPIVulkan::CreateSynchronizationObjects() {
fence_info.sType = VK_STRUCTURE_TYPE_FENCE_CREATE_INFO;
fence_info.flags = VK_FENCE_CREATE_SIGNALED_BIT;

for (size_t i = 0; i < in_flight_frame_count; ++i) {
for (size_t i = 0; i < swapchain_info_.swapchain_image_count_; ++i) {
VulkanAPIUtils::CheckVkResult(vkCreateSemaphore(vk_device_, &semaphore_info, nullptr,
&frame_render_completion_semaphore_[i]),
"vkCreateSemaphore");
}
Comment on lines +1150 to +1154
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 29, 2025

Choose a reason for hiding this comment

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

high

The loop for creating frame_render_completion_semaphore_ now correctly iterates up to swapchain_info_.swapchain_image_count_. This aligns with the resizing logic and ensures all necessary semaphores are created.


for (size_t i = 0; i < in_flight_frame_count; ++i) {
VulkanAPIUtils::CheckVkResult(vkCreateSemaphore(vk_device_, &semaphore_info, nullptr,
&swapchain_image_semaphore_[i]),
"vkCreateSemaphore");
Expand All @@ -1148,8 +1165,10 @@ void GraphicsAPIVulkan::CreateSynchronizationObjects() {
}

void GraphicsAPIVulkan::DestroySynchronizationObjects() {
for (size_t i = 0; i < in_flight_frame_fence_.size(); ++i) {
for (size_t i = 0; i < frame_render_completion_semaphore_.size(); ++i) {
vkDestroySemaphore(vk_device_, frame_render_completion_semaphore_[i], nullptr);
}
Comment on lines +1168 to +1170
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 29, 2025

Choose a reason for hiding this comment

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

high

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.

for (size_t i = 0; i < in_flight_frame_fence_.size(); ++i) {
vkDestroySemaphore(vk_device_, swapchain_image_semaphore_[i], nullptr);
vkDestroyFence(vk_device_, in_flight_frame_fence_[i], nullptr);
}
Expand Down
1 change: 1 addition & 0 deletions agdk/common/base_game_framework/src/vulkan/graphics_api_vulkan_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ void VulkanAPIUtils::GetDepthStencilFormats(VkPhysicalDevice physical_device,

void VulkanAPIUtils::GetDisplayFormats(VkPhysicalDevice physical_device, VkSurfaceKHR surface,
std::vector<DisplayManager::DisplayFormat> &display_formats) {
display_formats.clear();
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 29, 2025

Choose a reason for hiding this comment

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

medium

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.

std::vector<DisplayManager::DisplayFormat> depth_stencil_formats;
VulkanAPIUtils::GetDepthStencilFormats(physical_device, depth_stencil_formats);

Expand Down
4 changes: 2 additions & 2 deletions agdk/common/simple_renderer/renderer_render_pass_vk.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ RenderPassVk::RenderPassVk(const RenderPassCreationParams& params)
subpass_dependency.dstSubpass = 0;
subpass_dependency.srcStageMask = VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT;
subpass_dependency.srcAccessMask = 0;
subpass_dependency.dstStageMask = VK_PIPELINE_STAGE_COLOR_ATTACHMENT_OUTPUT_BIT;
subpass_dependency.dstAccessMask = VK_ACCESS_COLOR_ATTACHMENT_WRITE_BIT;
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;
Comment on lines +71 to +72
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot May 29, 2025

Choose a reason for hiding this comment

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

high

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.


VkRenderPassCreateInfo render_pass_create_info =
{ VK_STRUCTURE_TYPE_RENDER_PASS_CREATE_INFO };
Expand Down