Add export_replay for standalone replay with gapir.#2159
Add export_replay for standalone replay with gapir.#2159AWoloszyn merged 2 commits intogoogle:masterfrom
Conversation
|
I can't see any build results, local linux build seems to be fine. |
gapis/replay/export_replay.go
Outdated
| return log.Errf(ctx, nil, "Unknown device %v", pDevice.ID.ID()) | ||
| } | ||
|
|
||
| // executeCounter.Increment() |
There was a problem hiding this comment.
Remove commented out code
cmd/gapir/cc/main.cpp
Outdated
| for (int i = 1; i < argc; i++) { | ||
| if (strcmp(argv[i], "--auth-token-file") == 0) { | ||
| if (strcmp(argv[i], "--replay-archive") == 0) { | ||
| replayArchive = argv[++i]; |
There was a problem hiding this comment.
if (i + 1 >= argc) {
GAPID_FATAL(....
}
There was a problem hiding this comment.
And I think 'using archive' and 'connecting to gapis' are mutual exclusive, using both at the same time might be invalid (at least for this CL, and same to options like: port, idle-timeout-sec), so probably we should document this somewhere.
gapir/cc/replay_archive.cpp
Outdated
|
|
||
| namespace gapir { | ||
|
|
||
| std::unique_ptr<ReplayArchive::Payload> ReplayArchive::getPayload() { |
There was a problem hiding this comment.
ReplayConnection::Payload? Not sure which one is better.
cmd/gapir/cc/main.cpp
Outdated
| core::Debugger::waitForAttach(); | ||
| } | ||
|
|
||
| if (replayArchive) { |
There was a problem hiding this comment.
Now that we have a big mode-switch at the start of the replay, I would split this out into 2 methods. One that starts an "archived replay" and one that opens a connection.
It is a bit weird to have a "start the program in an entirely different way" early-out in the middle of main().
| return nullptr; | ||
| } | ||
| bool ReplayArchive::sendReplayFinished() { return true; } | ||
| bool ReplayArchive::sendCrashDump(const std::string& filepath, |
There was a problem hiding this comment.
We should be writing out the crash dump here. (And logging it's location).
| uint32_t api_index, uint64_t label, | ||
| const std::string& msg, const void* data, | ||
| uint32_t data_size) { | ||
| return true; |
There was a problem hiding this comment.
We should log any notifications that we got here.
There was a problem hiding this comment.
GAPIR already write those to log.
core/archive/archive.go
Outdated
| return C.archive_create(cstr) | ||
| } | ||
|
|
||
| // Dispose flush and close the underlying archive. |
gapis/api/vulkan/replay.go
Outdated
|
|
||
| // ExportReplayRequest returns request type for standalone replay. | ||
| func (a API) ExportReplayRequest() replay.Request { | ||
| return issuesRequest{} |
There was a problem hiding this comment.
We really should have a new type of request, instead of using issuesRequest, eventually. And using issuesRequest won't have any framebuffer data postback, so can be hard to tell if the replay is correct or not.
|
Build failure is a flake, merging. |
| Intent{pDevice, pCapture}, | ||
| Config(&struct{}{}), | ||
| []RequestAndResult{{ | ||
| Request: Request(generator.(interface{ ExportReplayRequest() Request }).ExportReplayRequest()), |
There was a problem hiding this comment.
Instead of (ab)using the requests, could we not just add a ForExport bool field to the Intent struct? That way it is clearly documented, and not using magic interfaces.
4 participants
No description provided.