Skip to content

Commit fa66161

Browse files
JiaboLi-GitHubclaude
JiaboLi-GitHub
and
claude
committed
fix: address high/medium priority issues from code review
- Extract duplicated toGraphicsApi/countAllEvents/countDrawCalls into shared action_helpers.h/cpp, removing copies from session, diff_session, and info modules - Add std::call_once to Session::ensureReplayInitialized() for thread safety - Replace raw stbi_load pointers with RAII unique_ptr wrapper to prevent memory leaks on exception paths; check stbi_write_png return value - Add NaN/Inf detection in assertPixel() float comparison - Remove redundant null check in diffPipeline() (accessors already throw) - Extract CLI argument parsing into cli_parse.h/cpp with proper numeric error handling (parseUint32/parseFloat/parseUint64/parseDouble) - Add 24 unit tests + 3 death tests for CLI argument parsing Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 72485ad commit fa66161

14 files changed

Lines changed: 610 additions & 342 deletions

‎CMakeLists.txt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ target_link_libraries(renderdoc-mcp-proto PUBLIC nlohmann_json::nlohmann_json)
2525
# --- renderdoc-core and full targets (require RENDERDOC_DIR) ---
2626
if(RENDERDOC_DIR)
2727
add_library(renderdoc-core STATIC
28+
src/core/action_helpers.cpp
2829
src/core/errors.cpp
2930
src/core/session.cpp
3031
src/core/events.cpp
@@ -125,7 +126,7 @@ if(RENDERDOC_DIR)
125126
target_link_libraries(renderdoc-mcp PRIVATE renderdoc-mcp-lib)
126127

127128
# renderdoc-cli — one-shot compound-command CLI
128-
add_executable(renderdoc-cli src/cli/main.cpp src/core/replay_marker.cpp)
129+
add_executable(renderdoc-cli src/cli/main.cpp src/cli/cli_parse.cpp src/core/replay_marker.cpp)
129130
target_link_libraries(renderdoc-cli PRIVATE renderdoc-core)
130131

131132
# Locate RenderDoc runtime files for local execution and packaging.

‎src/cli/cli_parse.cpp‎

Lines changed: 236 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,236 @@
1+
#include "cli/cli_parse.h"
2+
3+
#include <cstdlib>
4+
#include <iostream>
5+
#include <stdexcept>
6+
#include <string>
7+
8+
namespace renderdoc::cli {
9+
10+
std::optional<renderdoc::core::ShaderStage> parseStage(const std::string& s) {
11+
if (s == "vs" || s == "VS") return renderdoc::core::ShaderStage::Vertex;
12+
if (s == "hs" || s == "HS") return renderdoc::core::ShaderStage::Hull;
13+
if (s == "ds" || s == "DS") return renderdoc::core::ShaderStage::Domain;
14+
if (s == "gs" || s == "GS") return renderdoc::core::ShaderStage::Geometry;
15+
if (s == "ps" || s == "PS") return renderdoc::core::ShaderStage::Pixel;
16+
if (s == "cs" || s == "CS") return renderdoc::core::ShaderStage::Compute;
17+
return std::nullopt;
18+
}
19+
20+
uint32_t parseUint32(const std::string& str, const std::string& flagName) {
21+
try {
22+
unsigned long val = std::stoul(str);
23+
return static_cast<uint32_t>(val);
24+
} catch (const std::invalid_argument&) {
25+
std::cerr << "error: invalid value '" << str << "' for " << flagName
26+
<< " (expected unsigned integer)\n";
27+
std::exit(1);
28+
} catch (const std::out_of_range&) {
29+
std::cerr << "error: value '" << str << "' for " << flagName
30+
<< " is out of range\n";
31+
std::exit(1);
32+
}
33+
}
34+
35+
float parseFloat(const std::string& str, const std::string& flagName) {
36+
try {
37+
return std::stof(str);
38+
} catch (const std::invalid_argument&) {
39+
std::cerr << "error: invalid value '" << str << "' for " << flagName
40+
<< " (expected number)\n";
41+
std::exit(1);
42+
} catch (const std::out_of_range&) {
43+
std::cerr << "error: value '" << str << "' for " << flagName
44+
<< " is out of range\n";
45+
std::exit(1);
46+
}
47+
}
48+
49+
uint64_t parseUint64(const std::string& str, const std::string& flagName) {
50+
try {
51+
return std::stoull(str);
52+
} catch (const std::invalid_argument&) {
53+
std::cerr << "error: invalid value '" << str << "' for " << flagName
54+
<< " (expected unsigned integer)\n";
55+
std::exit(1);
56+
} catch (const std::out_of_range&) {
57+
std::cerr << "error: value '" << str << "' for " << flagName
58+
<< " is out of range\n";
59+
std::exit(1);
60+
}
61+
}
62+
63+
double parseDouble(const std::string& str, const std::string& flagName) {
64+
try {
65+
return std::stod(str);
66+
} catch (const std::invalid_argument&) {
67+
std::cerr << "error: invalid value '" << str << "' for " << flagName
68+
<< " (expected number)\n";
69+
std::exit(1);
70+
} catch (const std::out_of_range&) {
71+
std::cerr << "error: value '" << str << "' for " << flagName
72+
<< " is out of range\n";
73+
std::exit(1);
74+
}
75+
}
76+
77+
void printUsage(const char* argv0) {
78+
std::cerr << "Usage: " << argv0 << " <capture.rdc> <command> [options]\n\n"
79+
<< "Commands:\n"
80+
<< " info\n"
81+
<< " events [--filter TEXT]\n"
82+
<< " draws [--filter TEXT]\n"
83+
<< " pipeline [-e EID]\n"
84+
<< " shader STAGE [-e EID] (STAGE: vs|hs|ds|gs|ps|cs)\n"
85+
<< " resources [--type TYPE]\n"
86+
<< " export-rt IDX -o DIR [-e EID]\n"
87+
<< " capture EXE [-w DIR] [-a ARGS] [-d N] [-o PATH]\n"
88+
<< " pixel X Y [-e EID] [--target N]\n"
89+
<< " pick-pixel X Y [-e EID] [--target N]\n"
90+
<< " debug pixel X Y -e EID [--trace] [--primitive N]\n"
91+
<< " debug vertex VTX -e EID [--trace] [--instance N] [--index N] [--view N]\n"
92+
<< " debug thread GX GY GZ TX TY TZ -e EID [--trace]\n"
93+
<< " tex-stats RES_ID [-e EID] [--mip N] [--slice N] [--histogram]\n"
94+
<< " shader-encodings\n"
95+
<< " shader-build FILE --stage STAGE --encoding ENC [--entry NAME]\n"
96+
<< " shader-replace EID STAGE --with SHADER_ID\n"
97+
<< " shader-restore EID STAGE\n"
98+
<< " shader-restore-all\n"
99+
<< " mesh EID [--stage vs-out|gs-out] [--format obj|json] [-o FILE]\n"
100+
<< " snapshot EID -o DIR\n"
101+
<< " usage RES_ID\n"
102+
<< " assert-pixel EID X Y --expect R G B A [--tolerance T] [--target N]\n"
103+
<< " assert-state EID PATH --expect VALUE\n"
104+
<< " assert-image EXPECTED ACTUAL [--threshold T] [--diff-output PATH]\n"
105+
<< " assert-count WHAT --expect N [--op eq|gt|lt|ge|le]\n"
106+
<< " assert-clean [--min-severity high|medium|low|info]\n"
107+
<< " diff FILE_A FILE_B [--draws|--resources|--stats|--pipeline MARKER|--framebuffer]\n"
108+
<< " pass-stats\n"
109+
<< " pass-deps\n"
110+
<< " unused-targets\n";
111+
}
112+
113+
Args parseArgs(int argc, char* argv[]) {
114+
if (argc < 2) {
115+
printUsage(argv[0]);
116+
std::exit(1);
117+
}
118+
119+
Args a;
120+
121+
// Special case: "capture" command doesn't take a .rdc as first arg
122+
if (std::string(argv[1]) == "capture") {
123+
a.command = "capture";
124+
int i = 2;
125+
while (i < argc) {
126+
std::string tok = argv[i];
127+
if ((tok == "-w" || tok == "--working-dir") && i + 1 < argc) {
128+
a.workingDir = argv[++i];
129+
} else if ((tok == "-a" || tok == "--args") && i + 1 < argc) {
130+
a.cmdLineArgs = argv[++i];
131+
} else if ((tok == "-d" || tok == "--delay-frames") && i + 1 < argc) {
132+
a.delayFrames = parseUint32(argv[++i], "--delay-frames");
133+
} else if ((tok == "-o" || tok == "--output") && i + 1 < argc) {
134+
a.outputDir = argv[++i];
135+
} else {
136+
a.positional.push_back(tok);
137+
}
138+
++i;
139+
}
140+
return a;
141+
}
142+
143+
// Standard commands: <capture.rdc> <command> [options]
144+
if (argc < 3) {
145+
printUsage(argv[0]);
146+
std::exit(1);
147+
}
148+
149+
a.capturePath = argv[1];
150+
a.command = argv[2];
151+
152+
int i = 3;
153+
while (i < argc) {
154+
std::string tok = argv[i];
155+
if ((tok == "-e" || tok == "--event") && i + 1 < argc) {
156+
a.eventId = parseUint32(argv[++i], "-e/--event");
157+
} else if (tok == "--filter" && i + 1 < argc) {
158+
a.filter = argv[++i];
159+
} else if (tok == "--type" && i + 1 < argc) {
160+
a.typeFilter = argv[++i];
161+
} else if (tok == "-o" && i + 1 < argc) {
162+
a.outputDir = argv[++i];
163+
} else if (tok == "--target" && i + 1 < argc) {
164+
a.targetIndex = parseUint32(argv[++i], "--target");
165+
} else if (tok == "--mip" && i + 1 < argc) {
166+
a.mipLevel = parseUint32(argv[++i], "--mip");
167+
} else if (tok == "--slice" && i + 1 < argc) {
168+
a.sliceIndex = parseUint32(argv[++i], "--slice");
169+
} else if (tok == "--instance" && i + 1 < argc) {
170+
a.instance = parseUint32(argv[++i], "--instance");
171+
} else if (tok == "--primitive" && i + 1 < argc) {
172+
a.primitive = parseUint32(argv[++i], "--primitive");
173+
} else if (tok == "--index" && i + 1 < argc) {
174+
a.index = parseUint32(argv[++i], "--index");
175+
} else if (tok == "--view" && i + 1 < argc) {
176+
a.view = parseUint32(argv[++i], "--view");
177+
} else if (tok == "--trace") {
178+
a.trace = true;
179+
} else if (tok == "--histogram") {
180+
a.histogram = true;
181+
} else if (tok == "--encoding" && i + 1 < argc) {
182+
a.encoding = argv[++i];
183+
} else if (tok == "--entry" && i + 1 < argc) {
184+
a.entry = argv[++i];
185+
} else if (tok == "--with" && i + 1 < argc) {
186+
a.withShaderId = parseUint64(argv[++i], "--with");
187+
} else if (tok == "--format" && i + 1 < argc) {
188+
a.format = argv[++i];
189+
} else if (tok == "--expect" && i + 1 < argc) {
190+
// Try to parse as 4 floats (for assert-pixel); fall back to string/int
191+
if (i + 4 < argc) {
192+
bool allNumeric = true;
193+
for (int k = 1; k <= 4; k++) {
194+
std::string s = argv[i + k];
195+
bool numeric = !s.empty() && (s[0] == '-' || s[0] == '.' ||
196+
(s[0] >= '0' && s[0] <= '9'));
197+
if (!numeric) { allNumeric = false; break; }
198+
}
199+
if (allNumeric) {
200+
a.expectRGBA[0] = parseFloat(argv[++i], "--expect R");
201+
a.expectRGBA[1] = parseFloat(argv[++i], "--expect G");
202+
a.expectRGBA[2] = parseFloat(argv[++i], "--expect B");
203+
a.expectRGBA[3] = parseFloat(argv[++i], "--expect A");
204+
a.hasExpectRGBA = true;
205+
} else {
206+
// Single value: string or int (int parsing is best-effort)
207+
a.expectStr = argv[++i];
208+
try { a.expectCount = std::stoi(a.expectStr); } catch (...) {}
209+
}
210+
} else {
211+
// Single value
212+
a.expectStr = argv[++i];
213+
try { a.expectCount = std::stoi(a.expectStr); } catch (...) {}
214+
}
215+
} else if (tok == "--tolerance" && i + 1 < argc) {
216+
a.tolerance = parseFloat(argv[++i], "--tolerance");
217+
} else if (tok == "--op" && i + 1 < argc) {
218+
a.opStr = argv[++i];
219+
} else if (tok == "--min-severity" && i + 1 < argc) {
220+
a.minSeverity = argv[++i];
221+
} else if (tok == "--diff-output" && i + 1 < argc) {
222+
a.diffOutput = argv[++i];
223+
} else if (tok == "--threshold" && i + 1 < argc) {
224+
a.threshold = parseDouble(argv[++i], "--threshold");
225+
} else if (tok == "--stage" && i + 1 < argc) {
226+
a.stageStr = argv[++i];
227+
} else {
228+
a.positional.push_back(tok);
229+
}
230+
++i;
231+
}
232+
233+
return a;
234+
}
235+
236+
} // namespace renderdoc::cli

‎src/cli/cli_parse.h‎

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
#pragma once
2+
3+
#include "core/types.h"
4+
#include <optional>
5+
#include <string>
6+
#include <vector>
7+
8+
namespace renderdoc::cli {
9+
10+
struct Args {
11+
std::string capturePath;
12+
std::string command;
13+
std::vector<std::string> positional;
14+
std::optional<uint32_t> eventId;
15+
std::string filter;
16+
std::string typeFilter;
17+
std::string outputDir;
18+
std::string workingDir;
19+
std::string cmdLineArgs;
20+
uint32_t delayFrames = 100;
21+
// Phase 1 additions
22+
uint32_t targetIndex = 0;
23+
uint32_t mipLevel = 0;
24+
uint32_t sliceIndex = 0;
25+
uint32_t instance = 0;
26+
uint32_t primitive = 0xFFFFFFFF;
27+
uint32_t index = 0xFFFFFFFF;
28+
uint32_t view = 0;
29+
bool trace = false;
30+
bool histogram = false;
31+
// Phase 2
32+
std::string encoding;
33+
std::string entry = "main";
34+
uint64_t withShaderId = 0;
35+
std::string format = "obj";
36+
std::string expectStr;
37+
float expectRGBA[4] = {};
38+
bool hasExpectRGBA = false;
39+
float tolerance = 0.01f;
40+
std::string opStr = "eq";
41+
int expectCount = 0;
42+
std::string minSeverity = "high";
43+
std::string diffOutput;
44+
double threshold = 0.0;
45+
std::string stageStr = "vs-out";
46+
};
47+
48+
std::optional<renderdoc::core::ShaderStage> parseStage(const std::string& s);
49+
50+
// Numeric parse helpers — print error to stderr and call std::exit(1) on failure.
51+
// For testability, provide throwing variants.
52+
uint32_t parseUint32(const std::string& str, const std::string& flagName);
53+
float parseFloat(const std::string& str, const std::string& flagName);
54+
uint64_t parseUint64(const std::string& str, const std::string& flagName);
55+
double parseDouble(const std::string& str, const std::string& flagName);
56+
57+
void printUsage(const char* argv0);
58+
Args parseArgs(int argc, char* argv[]);
59+
60+
} // namespace renderdoc::cli

0 commit comments

Comments
 (0)