Add support for RenderDoc API to offload test suite to capture for Vulkan on Windows #651
Conversation
damyanp
left a comment
damyanp
left a comment
There was a problem hiding this comment.
Some code conventions things mentioned, plus I do think we should fix the error handling from GetAPI.
| (pRENDERDOC_GetAPI)GetProcAddress(Mod, "RENDERDOC_GetAPI"); | ||
| int Ret = RENDERDOC_GetAPI(eRENDERDOC_API_Version_1_4_1, (void **)&RDocAPI); | ||
| assert(Ret == 1); | ||
| llvm::outs() << "RenderDoc API initialized.\n"; |
There was a problem hiding this comment.
This doesn't really check the return code from GetAPI - so if the user has, say, an older version of renderdoc.dll then it'll happily say "RenderDoc API initialized" and then not take a capture.
Related to this, I don't think we want to handle runtime errors like this with an assert.
| llvm::outs() << "RenderDoc API initialized.\n"; | ||
| } else { | ||
| DWORD error = GetLastError(); | ||
| llvm::outs() << "Failed to get renderdoc dll: " << error << "\n"; |
There was a problem hiding this comment.
llvm::mapLastWindowsError() might be interesting to look at here. Looks like it should convert to strings as well.
Kinda hard to find the right balance for polish for a feature like this.
| } else if(Capture) | ||
| llvm::outs() << "RenderDoc frame capture NOT started because we failed to \ | ||
| initialize RenderDoc API. It is only supported on Windows."; |
There was a problem hiding this comment.
| } else if(Capture) | |
| llvm::outs() << "RenderDoc frame capture NOT started because we failed to \ | |
| initialize RenderDoc API. It is only supported on Windows."; | |
| } else if(Capture) { | |
| llvm::outs() << "RenderDoc frame capture NOT started because we failed to \ | |
| initialize RenderDoc API. It is only supported on Windows."; | |
| } |
I don't know why the diffs on suggestions is so broken these days - anyway suggestion is to add braces around the else if.
Inconsistent bracing within if/else if/else chains (if one block requires braces, all must)
| if (RDocAPI) { | ||
| uint32_t Ret = RDocAPI->EndFrameCapture(NULL, NULL); | ||
| if (Ret) { | ||
| llvm::outs() << "RenderDoc frame capture ended.\n"; |
| if (Capture) { | ||
| // Initialize RenderDoc on Windows | ||
| #if defined(_WIN32) | ||
| if (HMODULE Mod = GetModuleHandleA("renderdoc.dll")) { |
There was a problem hiding this comment.
This leaks the module. I don't know if we care about that though.
2 participants
Add support to get a frame capture for Vulkan on Windows using the RenderDoc API.