feat(vk): frame generation Auto: a frame for each of the display's refreshes #1121
No reviewers
Labels
No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
No milestone
No assignees
1 participant
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Hookshot!1121
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/framegen-auto-pr"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What
Frame generation gets Auto in the Insert menu (its value 1, the step the row skipped so far). Auto makes a frame for each of the display's refreshes and no more:
The fixed 2x to 8x are unchanged.
How
auto_slots).tbetween the two game frames.fg_synthalready takes anyt. A refresh att = 1gets the game's own frame.OutSet::due), instead of spreading the frames evenly over the game frame.The refresh rate comes from the display the window is on, and an unknown refresh counts as 60 Hz:
SDL_GetDisplayForWindow+SDL_GetCurrentDisplayMode(SDL2:SDL_GetWindowDisplayIndex+SDL_GetCurrentDisplayMode). Both are looked up optionally, are asked on the main thread once a second, and are handed to the renderer through a newrefresh_mhzin the window bridge (MKWVK_WINDOW_VERSION2; a version 1 bridge still starts).EnumDisplaySettingsWfor the window's monitor, cached for a second.Why
With fixed multipliers, a game running between the display's rate and half of it either makes too many frames or too few:
Auto follows the display, also when the same install is played on a TV one day and a monitor the next.
Tested
RX 7900 XTX, RADV (Mesa 26.2.6), Linux (Hyprland, native Wayland), v2.0.7 plus this change, Kokiri Forest:
EnumDisplaySettingsWpath is written but not built or run; I have no Windows machine. The Windows builder CI will compile it. Its rate is whole Hz, so one under a multiple of 6 (59, 119, 143, ...) is taken as that rate's /1.001 (59.94 Hz etc.), also untested.tests/test_framegen_auto.pyruns the realauto_cap,multiplierandauto_slotsover refresh rates and game rates. It covers 45 fps on 60 Hz (60 shown, three of four made at 1/4 to 3/4), 60 on 60, 75 on 60, 45 on 120, 30 on 240 (capped) and a restarted timeline. It also checks thatdisplay_refresh_mhzis declared outside the unnamed namespace: an earlier draft declared it inside, and the shared library linked with the symbol undefined.tests/test_framegen_gate.pynow counts Auto as asking for interpolation.Not covered
🤖 Generated with Claude Code
Nice idea, and the plumbing mostly checks out. What I verified:
refresh_rateat offset 20 ofSDL_DisplayMode(displayID, format, w, h, pixel_density, refresh_rate), which is right. The SDL2 struct and theSDL_GetWindowDisplayIndex/SDL_GetCurrentDisplayMode(int, mode*) == 0calls also match, and both lookups are optional, so an older SDL still starts.refresh_nowis set beforerenderer_mainstarts, soensure_sizesees a real rate on the first frame.refresh_mhzis only read whenversion >= 2.min(fg.mult, kMaxMult), soset->frame[k]never goes past the framesensure_sizeallocated. The Auto early return keeps the same barriers and state updates as the fixed path, and a set withshown == 0is still released.framegen_made_imagestays consistent, because the game's frame can only be the last slot.set_rowandmenu_initis all the menu needed. No preset uses 1, and there's no newHOS_OPT_*id or API version bump to collide with.auto_cap/multiplier/auto_slots, which I like.Main issues:
Auto at a steady 60 fps on 60 Hz interpolates almost every frame and drops some of the game's frames.
game[k]is only true when a refresh slot lands exactly on the game frame's timestamp (t > 0.999). The test hits that because it uses exact integer frame times. With real present jitter (the pacer comment says frames arrive 12 to 28 ms apart), slots drift aroundnow. I re-ran theauto_slotsarithmetic at 60/60 with only ±0.2 ms of jitter: about 95% of shown frames were synthesized (median t ≈ 0.99), and about 14% of game frames got no refresh at all, with their neighbour getting 2. At ±1 ms it's 99% synthesized. So the "60 fps on 60 Hz: none" row in the description doesn't hold in practice. Auto pays for flow + synth on every frame and shows interpolated frames, with their artefacts, where the real frame was available. It also isn't equivalent to Off when the game already matches the display. Snapping slots within some tolerance ofnowto the game's frame (say, within a quarter of a refresh) would fix the common case. The same goes for slots just afterlast, which could reuse the previous frame instead of a near-0 synth.The overlay's "game N" undercounts with Auto.
menu_note_frameis only called inside the show loop, so a game frame withshown == 0isn't counted. That happens whenever the game is at or above the display rate, and per (1) also at 60/60 with jitter. The overlay then reports roughly the refresh rate instead of the game's real rate, while the description says each game frame is counted once. Counting game frames where the job is queued, independent of how many refreshes it got, would fix it.The PR conflicts with main in
vk_framegen.cpp,vk_main.cppandvk_menu.cpp. Main now waits withwait_until_near()instead ofsleep_for, the bridge check sits next toprep_window = w, andset_rowhasstep_output_choice/stops_at_ends. None of these are hard, but the display-loop hunk needs to keepwait_until_near. Also heads-up: #1088 edits the samevk_windowinitializer indisplay_sdl.c.Smaller:
dmDisplayFrequencyis an integer, and many 59.94 Hz modes report 59. Auto would then make 59 frames a second for a ~60 Hz display, which means a repeated refresh roughly every second. Linux SDL3 gives the fractional rate, so it's only a Windows issue.DwmGetCompositionTimingInfo(rateRefresh) orQueryDisplayConfiggives the exact rational rate.RX_VK_PACE=0) no longer has any effect, becausetimedignorespace. That's fine, but a word in the help text would save confusion.framegen_wantedstill says "2x and up is the interpolation".When
shown == 0this loop never runs, so that game frame isn't counted toward "game N". With Auto at or above the display rate, the overlay then shows about the refresh rate rather than the game's. Maybe count the game frame once per job outside the loop (only the shown counter needs to be per present).Minor: with
timedthepacesetting is ignored entirely, so the Frame pacing row does nothing under Auto. That's fine behaviour, but worth mentioning in the row's help text.This only matches when a slot lands exactly on
now, which in practice only happens when the timeline restarts. With normal frame jitter at 60/60, nearly every slot ends up at t ≈ 0.95 to 0.99 and gets synthesized, and some game frames get no slot at all (see the review body). Could slots within a tolerance ofnow(e.g.(now - at[k]) < period / 4) use the game's frame, and skip flow when all of them do?dmDisplayFrequencyis whole Hz, and 59.94 Hz modes often report 59 here. Auto would then pace 59 frames a second on a ~60 Hz display, which gives one repeated refresh about every second.DwmGetCompositionTimingInfo(rateRefreshnumerator/denominator) gives the exact rate. At minimum, treating 59 as 59.94 would cover the common case.Thanks for the careful review, all points taken:
auto_slotsfor a minute of frames with ±0.2 / ±1 ms jitter: at 60 fps on 59.94, 60 and 60.06 Hz, 0 % synthesized and no game frame without a refresh. 45 and 30 fps on 60 Hz, and 45/60/75 fps on 120 and 144 Hz, still give the display its refresh count. The test now includes the jittered cases. Flow is skipped when every slot is the game's frame (that was already the case).menu_note_frame(true, 0)).wait_until_near, the bridge check sits next toprep_window = w, andset_rowkeepsstep_output_choice/stops_at_ends.Smaller:
framegen_wanted's comment now names Auto.Played on Linux (RX 7900 XTX, RADV, 120 Hz) on current main plus this PR: the game runs at 45 to 60 fps, and Auto holds a steady 120.
Thanks, the fixes look good. One thing blocks it though: the Windows Vulkan back end check fails to compile
vk_framegen.cpp(details inline, it's thenearmacro from windows.h).The rest, re-checked:
119c981went away when you merged main's hints, but the Auto hint on Frame generation says it now, which is enough.wait_until_near,prep_window,stops_at_ends/step_output_choiceand the new hints are all kept correctly.Once Windows builds I'm happy to approve.
@ -91,7 +101,43 @@ int64_t now_ns() {return std::chrono::duration_cast<std::chrono::nanoseconds>(std::chrono::steady_clock::now().time_since_epoch()).count();This breaks the Windows build:
windows.h(minwindef.h) has#define near(empty), so this becomesconst int64_t = period / 4;(CI:vk_framegen.cpp:132:24: error: expected unqualified-id, plus 133 and 136). Renaming it (snap,within, ...) fixes it. Linux didn't catch it because the test fixture doesn't include windows.h.Renamed it to
snap(d156a6d), thanks for catching that. The description already says the Windows path is untested (only compiled by CI). It now also covers the 59 -> 59.94 rule.Re-checked at
d156a6d. The windows.hnearcollision is fixed: the variable is nowsnapin auto_slots, with both uses renamed and a comment saying why. The only change since my last review is that rename. I grepped the other files in the PR for near/far/min/max-style macro hazards and found none. Windows Vulkan back end (llvm-mingw) and all the other checks are green. The logic I verified last time is unchanged. Good to merge from my side.