Split Replay into Features for easier expansion#2927
Conversation
MarkY-LunarG
added
approved-to-run-ci
MarkY-LunarG
force-pushed
the
marky-dev-split-up-replay
branch
from
0b31d0f to
c85801a
Compare
|
|
Problem was we did not free replay_consumers until after we had already released the logging. So, add them to be released during hte Cleanup stage.
All 3 features supported here had a lot of common functionality which could be reduced down using a template base.
Add a second template base for pre-processing support to clean up some more duplication
This adds in the initial simpler feature interface and includes the necessary Windows and Android fixes.
MarkY-LunarG
force-pushed
the
marky-dev-split-up-replay
branch
from
bc5161a to
24a322e
Compare
mikes-lunarg
left a comment
mikes-lunarg
left a comment
There was a problem hiding this comment.
I can see why you didn't do this one first, lots to untangle here!
| kApplicationName, g_file_processor.get(), VK_KHR_ANDROID_SURFACE_EXTENSION_NAME, app); | ||
|
|
||
| if (replay_options.capture) | ||
| for (auto& feature : g_features) |
There was a problem hiding this comment.
There is a non-trivial amount of logic duplicated between desktop_main.cpp and android_main.cpp, we should definitely pull that into a common implementation
There was a problem hiding this comment.
I was just making this change. I have a pending change which pulls out a majority of the commonality between these two files. I agree, having duplicate code in two separate files is prone to breakage. I'll push it up when I address the other comments.
There was a problem hiding this comment.
@mikes-lunarg , the changes are in for the common code split
| feature->QueryFpsInfoOptions(quit_after_measurement_frame_range, | ||
| flush_measurement_frame_range, | ||
| flush_inside_measurement_range, | ||
| preload_measurement_frame_range, | ||
| quit_after_frame); |
There was a problem hiding this comment.
I think it makes more sense for the replay main to parse the common options. Here, the different features duplicate work and could potentially stomp on each other.
There was a problem hiding this comment.
The problem is that each API has it's own "ReplayOptions" structure that this info is stored inside. For example, for Dx12, they use DxReplayOptions and Vulkan uses VulkanReplayOptions structures.
That's why it's a Feature call.
| file_processor_->SetPrintBlockInfoFlag( | ||
| replay_options_.enable_print_block_info, replay_options_.block_index_from, replay_options_.block_index_to); |
There was a problem hiding this comment.
I think this can be moved out to replay main, it isn't vulkan specific
There was a problem hiding this comment.
replay_options_ is Vulkan specific and even in the old path these were only set if Vulkan was enabled. No change has been made to behavior here.
Both android_main.cpp and desktop_main.cpp shared a lot of common code. This is bad and could lead to seperation in behaviors in the long run. So, pull out the common code instead.
This includes pulling out the core of desktop_main.cpp and android_main.cpp into a new set of files (a .h and a .cpp) which allows the common code to exist in a single location.
3 participants
This change continues on the Split previously done on both the
gfxrrecon-infoand thegfxrecon-converttools to attempt to make it easier to expand new APIs into the tools without conflicting with existing changes.