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

A handful of interface changes to support OpenXR future rebasing #1846

Merged
merged 8 commits into from
Oct 31, 2024
Merged

A handful of interface changes to support OpenXR future rebasing #1846

merged 8 commits into from
Oct 31, 2024

Conversation

jzulauf-lunarg
Copy link
Contributor

@jzulauf-lunarg jzulauf-lunarg commented Oct 29, 2024 •
edited
Loading 

Add needed decoder and interface extensions
Add interface to get/set capture mode
Expose dispatch table getters
Add initialize to StructPointerDecoder allocation
Add vulkan guard to consumer and decode variables
Split OverrideCreateInstance/Device
Add VkPhysicalDevice aliasing support
Reorganize common options

Not sure if these will interfere with @panos-lunarg pending changes, will wait if needed

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 290663.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5232 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5232 passed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 291019.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5237 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5237 passed.

MarkY-LunarG
MarkY-LunarG approved these changes Oct 30, 2024
View reviewed changes
Copy link
Contributor

@MarkY-LunarG MarkY-LunarG left a comment

Choose a reason for hiding this comment

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

Small nit, but otherwise LGTM

framework/decode/struct_pointer_decoder.h

GFXRECON_BEGIN_NAMESPACE(gfxrecon)
GFXRECON_BEGIN_NAMESPACE(decode)
template <typename T, typename = void>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Put a blank line above this.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 291462.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5244 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5244 passed.

jzulauf-lunarg
jzulauf-lunarg commented Oct 30, 2024
View reviewed changes
Copy link
Contributor Author

@jzulauf-lunarg jzulauf-lunarg left a comment

Choose a reason for hiding this comment

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

@davidd-lunarg , @bradgrantham-lunarg -- this change, particularly, I'd like your review of

jzulauf-lunarg
jzulauf-lunarg commented Oct 30, 2024
View reviewed changes
framework/decode/vulkan_replay_consumer_base.cpp
const StructPointerDecoder<Decoded_VkInstanceCreateInfo>* pCreateInfo,
const StructPointerDecoder<Decoded_VkAllocationCallbacks>* pAllocator,
HandlePointerDecoder<VkInstance>* pInstance)
void VulkanReplayConsumerBase::ModifyCreateInstanceInfo(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidd-lunarg -- @bradgrantham-lunarg ... starting here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it keeps the original functionality but split into separate pieces. Looks OK to me.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 291493.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5245 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5245 passed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 291531.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5246 running.

@jzulauf-lunarg
Copy link
Contributor Author

That's all folks.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5246 passed.

davidd-lunarg
davidd-lunarg reviewed Oct 31, 2024
View reviewed changes
framework/decode/struct_pointer_decoder.h
return output_data_;
}

typename T::struct_type* AllocateOutputData(size_t len, const typename T::struct_type& init)
{
assert(!DecoderHasOutputAllocator<T>::value);
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 to assert. The CMake option GFXRECON_ENABLE_RELEASE_ASSERTS can be enabled to log failed GFXRECON_ASSERTs as errors in release builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The rest of this file is all assert(...)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine. For new code please prefer GFXRECON_ASSERT. There may come a day when we change as many assert as we can to GFXRECON_ASSERT

framework/decode/vulkan_replay_consumer_base.cpp
const StructPointerDecoder<Decoded_VkInstanceCreateInfo>* pCreateInfo,
const StructPointerDecoder<Decoded_VkAllocationCallbacks>* pAllocator,
HandlePointerDecoder<VkInstance>* pInstance)
void VulkanReplayConsumerBase::ModifyCreateInstanceInfo(
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it keeps the original functionality but split into separate pieces. Looks OK to me.

framework/decode/struct_pointer_decoder.h
@@ -68,12 +92,13 @@ class StructPointerDecoder : public PointerDecoderBase
typename T::struct_type* AllocateOutputData(size_t len)
{
output_len_ = len;
output_data_ = DecodeAllocator::Allocate<typename T::struct_type>(len);
output_data_ = StructPointerOutputDataAllocator(decoded_structs_, len);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change for a new use of StructPointerDecoder? Or is there an existing use that wasn't being handled correclty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New use.

@bradgrantham-lunarg bradgrantham-lunarg removed their request for review October 31, 2024 15:57
@bradgrantham-lunarg
Copy link
Contributor

If @davidd-lunarg and @MarkY-LunarG approve the PR I trust their approval.

@jzulauf-lunarg
vulkan: Expose dispatch table getters
ceb0d2e 
For layered API support.
@jzulauf-lunarg
Add interface to get/set capture mode
e4c807b 
For layered API support.
@jzulauf-lunarg
Add needed decoder and interface extensions
a7988bf 
Layered API support
@jzulauf-lunarg
Add initialze to StructPointerDecoder allocation
3037a86 
Prevents uninitialized next/pNext.  The lower level decoders aren't
always setting values. Set a good state at initialization (zeros)

Layered API support.
@jzulauf-lunarg
Add vulkan guard to consumer and decode variables
d18f7d9 
Prevent collisions with other API consumers and decoders

Layered API support.
@jzulauf-lunarg
vulkan: Split OverrideCreateInstance/Device
267d30b 
Split vulkan instance and device creation override into reusable
pieces suitable for layered API support (specifically OpenXR)

Make neeed information public for other replay consumers

Layered API support.
@jzulauf-lunarg
Add VkPhysicalDevice aliasing support
fe0b46d 
Vulkan captures unwrapped physical device handles, Layered API (like OpenXR)
captures wrapped handles.  During replay two HandleId's will reference the
same VkPhysical device.  The vulkan_alias is the handleId as known by the
vulkan_consumer, which will be created/updated, etc, by all Vulkan replay
calls.
@jzulauf-lunarg
Reorganize common options
a08a7ca 
Move replay options around that are shared between multiple
replay paths.

For layered API support.
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 292063.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5252 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5252 passed.

@jzulauf-lunarg jzulauf-lunarg merged commit 837e74f into LunarG:dev Oct 31, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidd-lunarg davidd-lunarg davidd-lunarg left review comments

@bradgrantham-lunarg bradgrantham-lunarg bradgrantham-lunarg left review comments

@MarkY-LunarG MarkY-LunarG MarkY-LunarG approved these changes

@panos-lunarg panos-lunarg Awaiting requested review from panos-lunarg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jzulauf-lunarg @ci-tester-lunarg @bradgrantham-lunarg @MarkY-LunarG @davidd-lunarg