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.

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

Jump to bottom

Make vkFrameBoundaryANDROID trigger screenshots #1498

Merged
merged 3 commits into from
Jun 10, 2024
Merged

Make vkFrameBoundaryANDROID trigger screenshots #1498

merged 3 commits into from
Jun 10, 2024

Conversation

marius-pelegrin-arm
Copy link
Contributor

This commit is part of what is requested in #1494

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 168788.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4013 running.

@marius-pelegrin-arm marius-pelegrin-arm mentioned this pull request Apr 17, 2024
Open
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 168811.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4014 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4014 passed.

@marius-pelegrin-arm
Make vkFrameBoundaryANDROID trigger screenshots
df498f6 
This commit is part of what is requested in LunarG#1494

Change-Id: Ie448deebfb9858588779200ba6e78a9a0c5d6424
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 169478.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4023 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4023 passed.

@bradgrantham-lunarg
Copy link
Contributor

@panos-lunarg @locke-lunarg could you take a look at this PR, please? It would be helpful to merge for other sponsors so we should prioritize it. Thank you!

@locke-lunarg
Copy link
Contributor

locke-lunarg commented May 9, 2024 •
edited
Loading

These code is duplicated with VulkanReplayConsumerBase::WriteScreenshots. It will be better if it could have a function for these two cases, VulkanReplayConsumerBase::OverrideFrameBoundaryANDROID and VulkanReplayConsumerBase::WriteScreenshots.

@panos-lunarg
Copy link
Contributor

Some thoughts on vkFrameBoundaryANDROID and virtual swapchains:
VulkanReplayConsumerBase::WriteScreenshots takes the image information from the SwapchainKHRInfo which should take care of virtual swapchains.
On the other hand vkFrameBoundaryANDROID takes as a direct argument a handle to the VkImage that's going to be presented/swapped. I assume that the image handles are acquired through vkGetSwapchainImagesKHR which takes care of virtual swapchains. So it should be fine.
@locke-lunarg : does this sound correct to you?

@locke-lunarg
Copy link
Contributor

locke-lunarg commented May 10, 2024 •
edited
Loading

I'm not sure if the image on vkFrameBoundaryANDROID is from vkGetSwapchainImagesKHR. My option is quit simple that it could have a function, like VulkanReplayConsumerBase::WriteScreenshots(ImageInfo* image_info).

And then

VulkanReplayConsumerBase::WriteScreenshots(const Decoded_VkPresentInfoKHR* meta_info)
{
   ...
          WriteScreenshots(image_info);
   ...
}

VulkanReplayConsumerBase::OverrideFrameBoundaryANDROID(....)
{
   ...
          WriteScreenshots(image_info);
   ...
}

to avoid the duplicated code.

panos-lunarg
panos-lunarg reviewed May 12, 2024
View reviewed changes
framework/decode/vulkan_replay_consumer_base.cpp Outdated
instance_table->GetPhysicalDeviceMemoryProperties(device_info->parent, &memory_properties);

screenshot_handler_->WriteImage(filename_prefix,
device,
Copy link
Contributor

Choose a reason for hiding this comment

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

devivce_info instead of device

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure to understand... WriteImage takes a VkDevice as an argument, not a DeviceInfo

Copy link
Contributor

@panos-lunarg panos-lunarg May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Right, I saw a compilation error when I cherry picked this patch in dev. In this branch this is correct but from what I see in current dev, ScreenshotHandler::WriteImage has changed.

@panos-lunarg
Copy link
Contributor

I'm not sure if the image on vkFrameBoundaryANDROID is from vkGetSwapchainImagesKHR. My option is quit simple that it could have a function, like VulkanReplayConsumerBase::WriteScreenshots(ImageInfo* image_info).

Yes, I agree with this suggestion.
Basically my question was about whether this change is compatible with virtual swapchain. As far as I can tell I think it is.

@locke-lunarg
Copy link
Contributor

locke-lunarg commented May 13, 2024 •
edited
Loading

Yes, I agree with this suggestion. Basically my question was about whether this change is compatible with virtual swapchain. As far as I can tell I think it is.

Good point. For virtual swapchain, GetSwapchainImagesKHR returns virtual swapchain images, not real swapchain images, so vkFrameBoundaryANDROID would use output a virtual swapchain image, instead of real swapchain images. VulkanReplayConsumerBase::WriteScreenshots also output a virtual swapchain image, so it's not a problem.

@marius-pelegrin-arm
Copy link
Contributor Author

I'm not sure if the image on vkFrameBoundaryANDROID is from vkGetSwapchainImagesKHR. My option is quit simple that it could have a function, like VulkanReplayConsumerBase::WriteScreenshots(ImageInfo* image_info).

And then

VulkanReplayConsumerBase::WriteScreenshots(const Decoded_VkPresentInfoKHR* meta_info)
{
   ...
          WriteScreenshots(image_info);
   ...
}

VulkanReplayConsumerBase::OverrideFrameBoundaryANDROID(....)
{
   ...
          WriteScreenshots(image_info);
   ...
}

to avoid the duplicated code.

Sure the duplication is ugly, but the only code "really" duplicated here is the computation of the screenshot size. I cannot create the proposed function simply because there is no image_info accessible from the swapchain, only a vector of VkImage.
All the code that could be factorized was factorized in WriteImage

locke-lunarg
locke-lunarg approved these changes May 21, 2024 •
edited
Loading
View reviewed changes
@locke-lunarg locke-lunarg reopened this May 21, 2024
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 189483.

@locke-lunarg
Copy link
Contributor

Sorry. That's a wrong click.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4230 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4230 passed.

@locke-lunarg
Copy link
Contributor

@marius-pelegrin-arm Please solve the conflict, and then it's good to merge.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 198014.

@bradgrantham-lunarg
Copy link
Contributor

Hi, @locke-lunarg ! I resolved a conflict but that wasn't sufficient; there are compile errors. Will you check out Marius' branch and see if the fix is obvious?

@bradgrantham-lunarg
Copy link
Contributor

Hi, @locke-lunarg ! I resolved a conflict but that wasn't sufficient; there are compile errors. Will you check out Marius' branch and see if the fix is obvious?

Never mind, I think I see the issue.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 198040.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4257 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 4257 passed.

@locke-lunarg locke-lunarg merged commit ae62f2e into LunarG:dev Jun 10, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panos-lunarg panos-lunarg panos-lunarg left review comments

@locke-lunarg locke-lunarg locke-lunarg approved these changes

Assignees

@bradgrantham-lunarg bradgrantham-lunarg

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@marius-pelegrin-arm @ci-tester-lunarg @bradgrantham-lunarg @locke-lunarg @panos-lunarg