Skip to content
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.

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

Fix trim with external formats #1942

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

ziga-lunarg
Copy link
Contributor

Closes #1901

Replaces #1695

This only fixes the crash, there is a Todo left that is waiting on #1812

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 336995.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5690 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 337014.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5691 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5691 failed.

@bradgrantham-lunarg bradgrantham-lunarg requested review from antonio-lunarg and removed request for bradgrantham-lunarg January 7, 2025 17:23
@bradgrantham-lunarg
Copy link
Contributor

I've removed myself from reviewers for this one instance. If @mikes-lunarg and @antonio-lunarg approve the PR, please merge it.

@@ -5113,6 +5121,19 @@ VulkanReplayConsumerBase::OverrideCreateImage(PFN_vkCreateImage
{
image_info->queue_family_index = 0;
}

auto external_format_android = graphics::vulkan_struct_get_pnext<VkExternalFormatANDROID>(replay_create_info);
if (external_format_android && external_format_android->externalFormat != 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use auto * and explicitly check with != nullptr

@@ -4745,6 +4745,14 @@ VkResult VulkanReplayConsumerBase::OverrideBindImageMemory(PFN_vkBindImageMemory
image_info->handle, image_info->allocator_data, memory_info->allocator_data);
}

if (image_info->external_format)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add this comment here as well.

// Memory requirements for image with external format can only be queried after the memory is bound

auto external_format_android = graphics::vulkan_struct_get_pnext<VkExternalFormatANDROID>(replay_create_info);
if (external_format_android && external_format_android->externalFormat != 0)
{
image_info->external_format = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, add this comment here as well.

// Memory requirements for image with external format can only be queried after the memory is bound

{
const VulkanDeviceTable* device_table = vulkan_wrappers::GetDeviceTable(device);
VkMemoryRequirements image_mem_reqs;
assert(wrapper->handle != VK_NULL_HANDLE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer GFXRECON_ASSERT, but I think you can use image directly.

@@ -652,6 +652,12 @@ inline void InitializeState<VkDevice, vulkan_wrappers::ImageWrapper, VkImageCrea
assert(wrapper->handle != VK_NULL_HANDLE);
device_table->GetImageMemoryRequirements(parent_handle, wrapper->handle, &image_mem_reqs);
wrapper->size = image_mem_reqs.size;

auto external_format_android = graphics::vulkan_struct_get_pnext<VkExternalFormatANDROID>(create_info);
if (external_format_android && external_format_android->externalFormat != 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@@ -652,6 +652,12 @@ inline void InitializeState<VkDevice, vulkan_wrappers::ImageWrapper, VkImageCrea
assert(wrapper->handle != VK_NULL_HANDLE);
device_table->GetImageMemoryRequirements(parent_handle, wrapper->handle, &image_mem_reqs);
wrapper->size = image_mem_reqs.size;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the result here when the format is external?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, it is not valid to call vkGetImageMemoryRequirements before memory is bound when you have external memory.

}
else
{
snapshot_info.resource_size =
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we use wrapper->size here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because snapshot_info.level_sizes needs to be filled in. AHB cannot have mip levels so it is easier there

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 340003.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5721 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5721 failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android games that use AHB will crash on trim
4 participants