Skip to content

fix(vk): Use host timestamps in frame captures - #1763

Merged
hckrman101 merged 2 commits into
NotRexed:mainfrom
klimovich008:codex/framelog-host-clock
Nov 10, 2026
Merged

hckrman101 merged 2 commits into
NotRexed:mainfrom
klimovich008:codex/framelog-host-clock

Conversation

@klimovich008

@klimovich008 klimovich008 commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

What this fixes

RX_VK_FRAMELOG timestamps can use a different epoch from native present traces, preventing reliable alignment of captured images with those traces.

Why merge

  • One-line change: use the existing host clock used by native traces.
  • Keep sampling before submission; these remain recording times, not display times.
  • Preserve the binary layout, readers, capture-off path, profiler clock and main's readback barrier.

Change

  • gpu/vk/vk_replay.cpp: use l.at = host->now_ns().
  • scripts/framelog.py: label image spacing as capture samples.
  • tests/test_framelog_clock.py: execute the actual assignment and declare its compiler for the current runner.
  • docs/tested-configurations.md: record the AMD diagnostic run and its limits.

Tested

  • python -I -B tests/run_all.py --jobs 1 --strict framelog_clock framelog_integrity framelog_sink capture_slot_ownership present_semaphores png_publication run_all_declarations: seven checks pass.
  • Fails on the old code, passes now. Commented-out assignment control is rejected.
  • RX 9070 XT: 96-second boot/load capture; all 1,461 samples covered by the flushed native trace align one-to-one. No new validation IDs. No game data is included.

Not covered

Use the host nanosecond clock for recording-time samples so their epoch agrees with native-present traces. Preserve the raw record layout and all other profiler clocks, and label sampling intervals accurately.

Assisted-by: OpenAI Codex
@klimovich008
klimovich008 marked this pull request as ready for review November 10, 2026 16:31

@hckrman101 hckrman101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. host->now_ns() is the same clock the native traces use (vk_api.h already exposes it as int64 ns), and it's sampled at the same spot as before, so the record layout and readers are unchanged. The test extracts and compiles the real assignment, is registered in run_all.py, and CI is green. The docs and framelog.py wording changes are accurate.

@hckrman101

Copy link
Copy Markdown
Collaborator

Heads-up: this conflicts with main now (a lot landed today). Could you merge main into the branch (or rebase) and resolve the conflicts? Thanks!

@klimovich008

klimovich008 commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor Author

Merged current main and resolved all three conflicts. Kept main's capture readback barrier and both tested-configuration rows. The clock fixture now declares RUN_ALL_ARGS in its own file, matching the new runner.

Seven focused checks pass, including the runner declarations check. The old profiler-clock assignment and commented-out assignment both fail the clock test. The runtime change is still one line; fresh CI is running.

CI update: the clock check passes on Linux and Windows. The source-suite failures are unchanged main fixtures: test_draw_parameters.py lacks Ctx::mesh_fallback, and Windows test_nup_out_of_space.py expects a path without its extended-path prefix. Both fixes are tracked in #1793. The other build checks pass.

@hckrman101 hckrman101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving after the merge of main. The PR's own diff is unchanged in substance (host->now_ns() in the frame-log slot, the framelog.py wording, the docs row), and the conflicts are resolved with main's capture readback barrier kept. The test now declares RUN_ALL_ARGS itself, which is what #1822 expects, and run_all.py is no longer touched. CI was still running when I looked, so worth a glance before merging.

@hckrman101
hckrman101 merged commit b2ed731 into NotRexed:main Nov 10, 2026
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants