Skip to content

Adds custom dive vulkan consumer, annnotation processor #223

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

GrantComm
Copy link
Collaborator

Adds a custom dive vulkan consumer and annnotation processor which will be used for the display of vulkan commands on the ui. Follow up pr utilizes the consumer and annotation processor to display the commands in the ui.

@GrantComm GrantComm marked this pull request as ready for review May 27, 2025 17:15
@angela28chen
Copy link
Collaborator

I'm curious, is DiveAnnotationProcessor implemented in Dive rather than third_party/gfxreconstruct mainly because you want to use DiveVector for it? Does it make a big speed difference for this use case? I was thinking if it makes sense to implement it in the subtree instead, alongside the other dive-specific GFXR code, to have a simpler organization.

Also side note, maybe we should have DiveVector in some kind of util directory rather than in dive_core?

@GrantComm
Copy link
Collaborator Author

I'm curious, is DiveAnnotationProcessor implemented in Dive rather than third_party/gfxreconstruct mainly because you want to use DiveVector for it? Does it make a big speed difference for this use case? I was thinking if it makes sense to implement it in the subtree instead, alongside the other dive-specific GFXR code, to have a simpler organization.

Also side note, maybe we should have DiveVector in some kind of util directory rather than in dive_core?

Yes, it is in dive so that the DiveVector could be used. I can move it to the GFXR side instead and just use a regular std::vector. I am not sure about the speed difference between the two.

Also agree that a separate dive util directory makes sense.

@angela28chen
Copy link
Collaborator

I'm curious, is DiveAnnotationProcessor implemented in Dive rather than third_party/gfxreconstruct mainly because you want to use DiveVector for it? Does it make a big speed difference for this use case? I was thinking if it makes sense to implement it in the subtree instead, alongside the other dive-specific GFXR code, to have a simpler organization.
Also side note, maybe we should have DiveVector in some kind of util directory rather than in dive_core?

Yes, it is in dive so that the DiveVector could be used. I can move it to the GFXR side instead and just use a regular std::vector. I am not sure about the speed difference between the two.

Also agree that a separate dive util directory makes sense.

I'm not sure if it makes a big difference but my instinct is that we should try and group the changes we're making to GFXR, keep it under third_party/gfxreconstruct, and this approach would help with avoiding circular dependencies. Implementing a new derived class of AnnotationHandler is different from adding stuff to the FileProcessor class though, so maybe it's fine for it to be inside Dive source code? If we want to move the dive-specific code currently under third_party/gfxreconstruct into dive_core (DiveFileProcessor, DiveBlockData) while leaving the changes to FileProcessor there, I think that could also be done, but maybe it's not worth the effort since we have to bring in the tooling too for our changes to be part of replay apk.

@wangra-google Do you have any thoughts about code structure for this? Should we prefer our changes to GFXR to be implemented in the Dive source code or in the GFXR subtree?

@GrantComm GrantComm requested a review from wangra-google May 29, 2025 18:02
@wangra-google
Copy link
Collaborator

wangra-google commented May 29, 2025

I'm curious, is DiveAnnotationProcessor implemented in Dive rather than third_party/gfxreconstruct mainly because you want to use DiveVector for it? Does it make a big speed difference for this use case? I was thinking if it makes sense to implement it in the subtree instead, alongside the other dive-specific GFXR code, to have a simpler organization.
Also side note, maybe we should have DiveVector in some kind of util directory rather than in dive_core?

Yes, it is in dive so that the DiveVector could be used. I can move it to the GFXR side instead and just use a regular std::vector. I am not sure about the speed difference between the two.
Also agree that a separate dive util directory makes sense.

I'm not sure if it makes a big difference but my instinct is that we should try and group the changes we're making to GFXR, keep it under third_party/gfxreconstruct, and this approach would help with avoiding circular dependencies. Implementing a new derived class of AnnotationHandler is different from adding stuff to the FileProcessor class though, so maybe it's fine for it to be inside Dive source code? If we want to move the dive-specific code currently under third_party/gfxreconstruct into dive_core (DiveFileProcessor, DiveBlockData) while leaving the changes to FileProcessor there, I think that could also be done, but maybe it's not worth the effort since we have to bring in the tooling too for our changes to be part of replay apk.

@wangra-google Do you have any thoughts about code structure for this? Should we prefer our changes to GFXR to be implemented in the Dive source code or in the GFXR subtree?

Thanks Angela for mentioning this! (I totally forgot to check this part) I think this is a good topic that you should bring into the chatroom for further discussion, but here are some of my thoughts:

Option 1: adding into Dive project folder:
Pros:

  • It is easier for maintenance and development. The new files are co-located with the Dive project's source code. This makes it simpler for developers to find, modify, and debug the DiveAnnotationProcessor and related logic.
  • There is a clear separation between Dive and gfxreconstruct. This is beneficial for understanding the codebase and for onboarding new developers.
  • This will simplify potential merges from upstream gfxreconstruct.

Cons:

  • Potential for circular dependencies as Angela mentioned
  • We need to ensure the Dive build system correctly links against the gfxreconstruct libraries and includes the necessary headers.
  • Conceptually, the DiveAnnotationProcessor is an extension to gfxreconstruct. Placing it outside the gfxreconstruct directory might feel slightly less intuitive.

Option 2: adding into gfxreconstruct subtree folder:

Pros:

  • Placing it alongside other gfxreconstruct decode components can seem logically appealing.
  • It is probably simpler to add the files to the gfxreconstruct building system.

Cons:

  • Every time we pull updates from LunarG/gfxreconstruct, we run a high risk of merge conflicts with the dive specific files (including CMakeLists.txt). gfxreconstruct updates might involve restructuring directories, moving files around, or adding new subdirectories.
  • Placing it within gfxreconstruct's folder, while not upstreamed, creates a false sense of integration.

Personally, I am leaning towards Option 1, but I am open for discussion, there could be something that I have not thought of.

DiveVector<std::unique_ptr<SubmitInfo>> m_submits;
uint64_t m_block_index;
SubmitInfo* m_curr_submit = nullptr;
uint32_t m_command_buffer_count = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe for the members here that are all related can be named similarly. If I understand correctly, at least m_pre_submit_commands and m_command_buffer_count are both about the current submit before it's added into the larger m_submits vector. I'm not really sure what m_curr_submit is for but I see that it's cleared at the same time. I think these three should have a same prefix or some other indicator that they're dealing with the "current" submit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed to m_current_submit_commands and m_current_submit_command_buffer_count.

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.

3 participants