From a0887c0b3b8524d998361ea86f317c88bd820fa3 Mon Sep 17 00:00:00 2001 From: David Donaldson Date: Wed, 28 Jun 2023 15:52:09 -0700 Subject: [PATCH] Refactor parsing of integer ranges for options --- framework/encode/capture_manager.cpp | 23 +++-- framework/encode/capture_manager.h | 10 +- framework/encode/capture_settings.cpp | 141 ++------------------------ framework/encode/capture_settings.h | 63 +++++------- framework/util/options.cpp | 61 ++++++----- framework/util/options.h | 4 +- tools/tool_settings.h | 3 +- 7 files changed, 97 insertions(+), 208 deletions(-) diff --git a/framework/encode/capture_manager.cpp b/framework/encode/capture_manager.cpp index 7e94271b7e..175a50d2a4 100644 --- a/framework/encode/capture_manager.cpp +++ b/framework/encode/capture_manager.cpp @@ -188,14 +188,14 @@ void CaptureManager::DestroyInstance(std::function GetI } } -std::vector CalcScreenshotIndices(std::vector ranges) +std::vector CalcScreenshotIndices(std::vector ranges) { // Take a range of frames and convert it to a flat list of indices std::vector indices; for (uint32_t i = 0; i < ranges.size(); ++i) { - util::FrameRange& range = ranges[i]; + util::UintRange& range = ranges[i]; uint32_t diff = range.last - range.first + 1; @@ -320,7 +320,13 @@ bool CaptureManager::Initialize(std::string base_filename, const CaptureSettings { // Override default kModeWrite capture mode. trim_enabled_ = true; - trim_ranges_ = trace_settings.trim_ranges; + std::transform(trace_settings.trim_ranges.begin(), + trace_settings.trim_ranges.end(), + std::back_inserter(trim_ranges_), + [](const util::UintRange& range) { + GFXRECON_ASSERT(range.last >= range.first); + return TrimRange({ range.first, (range.last - range.first + 1) }); + }); // Determine if trim starts at the first frame if (!trace_settings.trim_ranges.empty()) @@ -610,8 +616,8 @@ void CaptureManager::CheckContinueCaptureForWriteMode() { // Trimming was configured to capture two consecutive frames, so we need to start a new capture // file for the current frame. - const CaptureSettings::TrimRange& trim_range = trim_ranges_[trim_current_range_]; - bool success = CreateCaptureFile(CreateTrimFilename(base_filename_, trim_range)); + const TrimRange& trim_range = trim_ranges_[trim_current_range_]; + bool success = CreateCaptureFile(CreateTrimFilename(base_filename_, trim_range)); if (success) { ActivateTrimming(); @@ -641,8 +647,8 @@ void CaptureManager::CheckStartCaptureForTrackMode() { if (trim_ranges_[trim_current_range_].first == current_frame_) { - const CaptureSettings::TrimRange& trim_range = trim_ranges_[trim_current_range_]; - bool success = CreateCaptureFile(CreateTrimFilename(base_filename_, trim_range)); + const TrimRange& trim_range = trim_ranges_[trim_current_range_]; + bool success = CreateCaptureFile(CreateTrimFilename(base_filename_, trim_range)); if (success) { ActivateTrimming(); @@ -730,8 +736,7 @@ void CaptureManager::EndFrame() } } -std::string CaptureManager::CreateTrimFilename(const std::string& base_filename, - const CaptureSettings::TrimRange& trim_range) +std::string CaptureManager::CreateTrimFilename(const std::string& base_filename, const TrimRange& trim_range) { assert(trim_range.total > 0); diff --git a/framework/encode/capture_manager.h b/framework/encode/capture_manager.h index 444b79a871..3be7bd352a 100644 --- a/framework/encode/capture_manager.h +++ b/framework/encode/capture_manager.h @@ -201,6 +201,12 @@ class CaptureManager std::vector scratch_buffer_; }; + struct TrimRange + { + uint32_t first{ 0 }; // First frame to capture. + uint32_t total{ 0 }; // Total number of frames to capture. + }; + protected: static bool CreateInstance(std::function GetInstanceFunc, std::function NewInstanceFunc); @@ -240,7 +246,7 @@ class CaptureManager bool GetDisableDxrSetting() const { return disable_dxr_; } auto GetAccelStructPaddingSetting() const { return accel_struct_padding_; } - std::string CreateTrimFilename(const std::string& base_filename, const CaptureSettings::TrimRange& trim_range); + std::string CreateTrimFilename(const std::string& base_filename, const TrimRange& trim_range); bool CreateCaptureFile(const std::string& base_filename); void ActivateTrimming(); void DeactivateTrimming(); @@ -308,7 +314,7 @@ class CaptureManager bool page_guard_signal_handler_watcher_; PageGuardMemoryMode page_guard_memory_mode_; bool trim_enabled_; - std::vector trim_ranges_; + std::vector trim_ranges_; std::string trim_key_; uint32_t trim_key_frames_; uint32_t trim_key_first_frame_; diff --git a/framework/encode/capture_settings.cpp b/framework/encode/capture_settings.cpp index ce85cd8d46..f88a9c9a8a 100644 --- a/framework/encode/capture_settings.cpp +++ b/framework/encode/capture_settings.cpp @@ -470,7 +470,8 @@ void CaptureSettings::ProcessOptions(OptionsMap* options, CaptureSettings* setti // trim ranges and trim hotkey are exclusive // with trim key will be parsed only // if trim ranges is empty, else it will be ignored - ParseTrimRangeString(FindOption(options, kOptionKeyCaptureFrames), &settings->trace_settings_.trim_ranges); + ParseUintRangeList( + FindOption(options, kOptionKeyCaptureFrames), &settings->trace_settings_.trim_ranges, "capture frames"); std::string trim_key_option = FindOption(options, kOptionKeyCaptureTrigger); std::string trim_key_frames_option = FindOption(options, kOptionKeyCaptureTriggerFrames); if (!trim_key_option.empty()) @@ -524,7 +525,9 @@ void CaptureSettings::ProcessOptions(OptionsMap* options, CaptureSettings* setti // Screenshot options settings->trace_settings_.screenshot_dir = FindOption(options, kOptionKeyScreenshotDir, settings->trace_settings_.screenshot_dir); - ParseFramesList(FindOption(options, kOptionKeyScreenshotFrames), &settings->trace_settings_.screenshot_ranges); + ParseUintRangeList(FindOption(options, kOptionKeyScreenshotFrames), + &settings->trace_settings_.screenshot_ranges, + "screenshot frames"); settings->trace_settings_.screenshot_format = ParseScreenshotFormatString( FindOption(options, kOptionKeyScreenshotFormat), settings->trace_settings_.screenshot_format); @@ -767,143 +770,19 @@ util::Log::Severity CaptureSettings::ParseLogLevelString(const std::string& val return result; } -void CaptureSettings::ParseTrimRangeString(const std::string& value_string, - std::vector* ranges) -{ - assert(ranges != nullptr); - - if (!value_string.empty()) - { - std::istringstream value_string_input; - value_string_input.str(value_string); - - for (std::string range; std::getline(value_string_input, range, ',');) - { - if (range.empty() || (std::count(range.begin(), range.end(), '-') > 1)) - { - GFXRECON_LOG_WARNING("Settings Loader: Ignoring invalid capture frame range \"%s\"", range.c_str()); - continue; - } - - util::strings::RemoveWhitespace(range); - - // Split string on '-' delimiter. - bool invalid = false; - std::vector values; - std::istringstream range_input; - range_input.str(range); - - for (std::string value; std::getline(range_input, value, '-');) - { - if (value.empty()) - { - break; - } - - // Check that the value string only contains numbers. - size_t count = std::count_if(value.begin(), value.end(), ::isdigit); - if (count == value.length()) - { - values.push_back(value); - } - else - { - GFXRECON_LOG_WARNING("Settings Loader: Ignoring invalid capture frame range \"%s\", which contains " - "non-numeric values", - range.c_str()); - invalid = true; - break; - } - } - - if (!invalid) - { - CaptureSettings::TrimRange trim_range; - - if (values.size() == 1) - { - if (std::count(range.begin(), range.end(), '-') == 0) - { - trim_range.first = std::stoi(values[0]); - trim_range.total = 1; - } - else - { - GFXRECON_LOG_WARNING("Settings Loader: Ignoring invalid capture frame range \"%s\"", - range.c_str()); - continue; - } - } - else if (values.size() == 2) - { - trim_range.first = std::stoi(values[0]); - - uint32_t last = std::stoi(values[1]); - if (last >= trim_range.first) - { - trim_range.total = (last - trim_range.first) + 1; - } - else - { - GFXRECON_LOG_WARNING( - "Settings Loader: Ignoring invalid capture frame range \"%s\", where first " - "frame is greater than last frame", - range.c_str()); - continue; - } - } - else - { - GFXRECON_LOG_WARNING("Settings Loader: Ignoring invalid capture frame range \"%s\"", range.c_str()); - continue; - } - - // Check for invalid start frame of 0. - if (trim_range.first == 0) - { - GFXRECON_LOG_WARNING( - "Settings Loader: Ignoring invalid capture frame range \"%s\", with first frame equal to zero", - range.c_str()); - continue; - } - - uint32_t next_allowed = 0; - - // Check that start frame is outside the bounds of the previous range. - if (!ranges->empty()) - { - // This produces the number of the frame after the last frame in the range. - next_allowed = ranges->back().first + ranges->back().total; - } - - if (trim_range.first >= next_allowed) - { - ranges->emplace_back(trim_range); - } - else - { - GFXRECON_LOG_WARNING("Settings Loader: Ignoring invalid capture frame range \"%s\", " - "where start frame precedes the end of the previous range \"%u-%u\"", - range.c_str(), - ranges->back().first, - (next_allowed - 1)); - } - } - } - } -} - -void CaptureSettings::ParseFramesList(const std::string& value_string, std::vector* frames) +void CaptureSettings::ParseUintRangeList(const std::string& value_string, + std::vector* frames, + const char* option_name) { GFXRECON_ASSERT(frames != nullptr); if (!value_string.empty()) { - std::vector frame_ranges = gfxrecon::util::GetFrameRanges(value_string); + std::vector frame_ranges = util::GetUintRanges(value_string.c_str(), option_name); for (uint32_t i = 0; i < frame_ranges.size(); ++i) { - util::FrameRange range{}; + util::UintRange range{}; range.first = frame_ranges[i].first; range.last = frame_ranges[i].last; frames->push_back(range); diff --git a/framework/encode/capture_settings.h b/framework/encode/capture_settings.h index 7430114c7c..500dd5935c 100644 --- a/framework/encode/capture_settings.h +++ b/framework/encode/capture_settings.h @@ -64,12 +64,6 @@ class CaptureSettings kDisabled = 2 }; - struct TrimRange - { - uint32_t first{ 0 }; // First frame to capture. - uint32_t total{ 0 }; // Total number of frames to capture. - }; - const static char kDefaultCaptureFileName[]; struct ResourveValueAnnotationInfo @@ -83,32 +77,32 @@ class CaptureSettings struct TraceSettings { - std::string capture_file{ kDefaultCaptureFileName }; - format::EnabledOptions capture_file_options; - bool time_stamp_file{ true }; - bool force_flush{ false }; - MemoryTrackingMode memory_tracking_mode{ kPageGuard }; - std::string screenshot_dir; - std::vector screenshot_ranges; - util::ScreenshotFormat screenshot_format; - std::vector trim_ranges; - std::string trim_key; - uint32_t trim_key_frames{ 0 }; - RuntimeTriggerState runtime_capture_trigger{ kNotUsed }; - int page_guard_signal_handler_watcher_max_restores{ 1 }; - bool page_guard_copy_on_map{ util::PageGuardManager::kDefaultEnableCopyOnMap }; - bool page_guard_separate_read{ util::PageGuardManager::kDefaultEnableSeparateRead }; - bool page_guard_persistent_memory{ false }; - bool page_guard_align_buffer_sizes{ false }; - bool page_guard_track_ahb_memory{ false }; - bool page_guard_unblock_sigsegv{ false }; - bool page_guard_signal_handler_watcher{ false }; - bool debug_layer{ false }; - bool debug_device_lost{ false }; - bool disable_dxr{ false }; - uint32_t accel_struct_padding{ 0 }; - bool force_command_serialization{ false }; - bool queue_zero_only{ false }; + std::string capture_file{ kDefaultCaptureFileName }; + format::EnabledOptions capture_file_options; + bool time_stamp_file{ true }; + bool force_flush{ false }; + MemoryTrackingMode memory_tracking_mode{ kPageGuard }; + std::string screenshot_dir; + std::vector screenshot_ranges; + util::ScreenshotFormat screenshot_format; + std::vector trim_ranges; + std::string trim_key; + uint32_t trim_key_frames{ 0 }; + RuntimeTriggerState runtime_capture_trigger{ kNotUsed }; + int page_guard_signal_handler_watcher_max_restores{ 1 }; + bool page_guard_copy_on_map{ util::PageGuardManager::kDefaultEnableCopyOnMap }; + bool page_guard_separate_read{ util::PageGuardManager::kDefaultEnableSeparateRead }; + bool page_guard_persistent_memory{ false }; + bool page_guard_align_buffer_sizes{ false }; + bool page_guard_track_ahb_memory{ false }; + bool page_guard_unblock_sigsegv{ false }; + bool page_guard_signal_handler_watcher{ false }; + bool debug_layer{ false }; + bool debug_device_lost{ false }; + bool disable_dxr{ false }; + uint32_t accel_struct_padding{ 0 }; + bool force_command_serialization{ false }; + bool queue_zero_only{ false }; // An optimization for the page_guard memory tracking mode that eliminates the need for shadow memory by // overriding vkAllocateMemory so that all host visible allocations use the external memory extension with a @@ -175,9 +169,8 @@ class CaptureSettings static util::Log::Severity ParseLogLevelString(const std::string& value_string, util::Log::Severity default_value); - static void ParseFramesList(const std::string& value_string, std::vector* frames); - - static void ParseTrimRangeString(const std::string& value_string, std::vector* ranges); + static void + ParseUintRangeList(const std::string& value_string, std::vector* frames, const char* option_name); static std::string ParseTrimKeyString(const std::string& value_string); diff --git a/framework/util/options.cpp b/framework/util/options.cpp index b8c95a9c74..c69609c057 100644 --- a/framework/util/options.cpp +++ b/framework/util/options.cpp @@ -27,24 +27,26 @@ #include "util/platform.h" #include "util/logging.h" +#include #include #include GFXRECON_BEGIN_NAMESPACE(gfxrecon) GFXRECON_BEGIN_NAMESPACE(util) -std::vector GetFrameRanges(const std::string& args) +std::vector GetUintRanges(const char* args, const char* option_name) { - std::vector ranges; + std::vector ranges; std::istringstream value_input; value_input.str(args); + std::string previous_range = ""; for (std::string range; std::getline(value_input, range, ',');) { if (range.empty() || (std::count(range.begin(), range.end(), '-') > 1)) { - GFXRECON_LOG_WARNING("Ignoring invalid screenshot frame range \"%s\"", range.c_str()); + GFXRECON_LOG_WARNING("Ignoring invalid range \"%s\" for %s", range, option_name); continue; } @@ -72,7 +74,7 @@ std::vector GetFrameRanges(const std::string& args) else { GFXRECON_LOG_WARNING( - "Ignoring invalid screenshot frame range \"%s\", which contains non-numeric values", range.c_str()); + "Ignoring invalid range \"%s\" for %s, which contains non-numeric values", range, option_name); invalid = true; break; } @@ -80,68 +82,71 @@ std::vector GetFrameRanges(const std::string& args) if (!invalid) { - FrameRange screenshot_range; + UintRange uint_range; if (values.size() == 1) { if (std::count(range.begin(), range.end(), '-') == 0) { - screenshot_range.first = std::stoi(values[0]); - screenshot_range.last = screenshot_range.first; + uint_range.first = std::stoi(values[0]); + uint_range.last = uint_range.first; } else { - GFXRECON_LOG_WARNING("Ignoring invalid screenshot frame range \"%s\"", range.c_str()); + GFXRECON_LOG_WARNING("Ignoring invalid range \"%s\" for %s", range, option_name); continue; } } else if (values.size() == 2) { - screenshot_range.first = std::stoi(values[0]); - screenshot_range.last = std::stoi(values[1]); - if (screenshot_range.first > screenshot_range.last) + uint_range.first = std::stoi(values[0]); + uint_range.last = std::stoi(values[1]); + if (uint_range.first > uint_range.last) { - GFXRECON_LOG_WARNING("Ignoring invalid screenshot frame range \"%s\", where first frame is " - "greater than last frame", - range.c_str()); + GFXRECON_LOG_WARNING( + "Ignoring invalid range \"%s\" for %s, where range start is greater than range end", + range, + option_name); continue; } } else { - GFXRECON_LOG_WARNING("Ignoring invalid screenshot frame range \"%s\"", range.c_str()); + GFXRECON_LOG_WARNING("Ignoring invalid range \"%s\" for %s", range, option_name); continue; } - // Check for invalid start frame of 0. - if (screenshot_range.first == 0) + // Check for invalid start range. + if (uint_range.first == 0) { - GFXRECON_LOG_WARNING("Ignoring invalid screenshot frame range \"%s\", with first frame equal to zero", - range.c_str()); + GFXRECON_LOG_WARNING( + "Ignoring invalid range \"%s\" for %s, with range start equal to zero", range, option_name); continue; } uint32_t next_allowed = 0; - // Check that first frame is outside the bounds of the previous range. + // Check that range start is outside the bounds of the previous range. if (!ranges.empty()) { - // The number of the frame after the last frame of the last range. + // The value of the next integer after the end of the last range. next_allowed = ranges.back().last + 1; } - if (screenshot_range.first >= next_allowed) + if (uint_range.first >= next_allowed) { - ranges.emplace_back(std::move(screenshot_range)); + ranges.emplace_back(std::move(uint_range)); + previous_range = range; } else { const auto& back = ranges.back(); - GFXRECON_LOG_WARNING("Ignoring invalid screenshot frame range \"%s\", " - "where start frame precedes the end of the previous range \"%u-%u\"", - range.c_str(), - back.first, - back.last); + GFXRECON_LOG_WARNING("Ignoring invalid range \"%s\" for %s, where the range \"%s\" overlaps with the " + "previous range \"%s\"", + range, + option_name, + range, + previous_range); } } } diff --git a/framework/util/options.h b/framework/util/options.h index 89adc462f6..199da1f0cc 100644 --- a/framework/util/options.h +++ b/framework/util/options.h @@ -33,13 +33,13 @@ GFXRECON_BEGIN_NAMESPACE(gfxrecon) GFXRECON_BEGIN_NAMESPACE(util) -struct FrameRange +struct UintRange { uint32_t first{ 0 }; uint32_t last{ 0 }; }; -std::vector GetFrameRanges(const std::string& args); +std::vector GetUintRanges(const char* args, const char* option_name); enum class ScreenshotFormat : uint32_t { diff --git a/tools/tool_settings.h b/tools/tool_settings.h index 55daabc820..256c681fa9 100644 --- a/tools/tool_settings.h +++ b/tools/tool_settings.h @@ -481,7 +481,8 @@ GetScreenshotRanges(const gfxrecon::util::ArgumentParser& arg_parser) if (!value.empty()) { - std::vector frame_ranges = gfxrecon::util::GetFrameRanges(value); + std::vector frame_ranges = + gfxrecon::util::GetUintRanges(value.c_str(), "screenshot frames"); for (uint32_t i = 0; i < frame_ranges.size(); ++i) {