Repository navigation
fix(videoout): support permitted high-frame-rate output for Quake II - #3886
Conversation
adriadam10
left a comment
There was a problem hiding this comment.
Thanks, the approach (support from the host display, permission checked at configure, per-config vblank cadence) looks reasonable. Points to fix:
Output.cpp:242masksattribute3 & 0x40, but the citedWINE_PS5_BUILD.mddoesn't mentionattribute3or this bit (checked), and PS5_Vulkan R92/R51 only measured the full value0x80040. Please cite the source that isolates0x40, or add a TechnicalDebt line saying the single-bit mask is inferred from the full0x80040measurement.Flip.cpp:291-292andGuestVideoOutOutputMode.cpp:55-56now accept either result, and every CI job runs with the offscreen SDL driver where these tests skip. Nothing in CI pins the unsupported branch any more. Please keep a check that fails when the display reports < 120 Hz (e.g. assert unsupported in the skip-free path you can reach, or expose the refresh-rate source so a test can inject it).VideoOutDriver.cpp:613callsSDL_GetWindowDisplayIndex+SDL_GetCurrentDisplayModeon every present-loop iteration. Refresh only when the window reports a display change/move event, or measure that the per-frame cost is negligible.- Not verified on our side: vblank periods and Quake II need a real 120 Hz display; the new test could not complete here (it times out on a real window, like the untouched
resolution_status/wqhd_detectiontests on this host, so I don't attribute that to the PR). Build andparam_json_parserpass.
|
Addressed the three actionable review points in cef9742:
The revised Release libs build passes. video_out_output_mode_policy and param_json_parser pass 2/2; conventions, test registration and git diff --check pass. Native vblank/title testing was not rerun for this update, and Linux/full CTest were not run. This update does not claim Quake II gameplay; a later P20 shader stop is reported separately in #1413. AI-assisted: yes (Codex). |
|
Linux result for this PR, with a gap that the Windows run can't show. Setup: Ubuntu (GNOME, Wayland, so SDL runs through XWayland), GCC, Release, NVIDIA RTX 2080 SUPER, monitor at 143.86 Hz. Local merge of The gap. With this PR merged, Quake II still logged With XRandR, Quake II gets So on Linux, this PR depends on SDL's XRandR support, and neither
|
|
@Recapi thanks for the Linux reproduction. Could you verify commit 5c19601 on your Linux setup? In separate clean bundled-SDL configure directories, please confirm that missing XRandR development files (or SDL_X11_XRANDR=OFF) fail with the new diagnostic, while installed/enabled XRandR configures successfully. Then please rerun the focused tests and Quake II startup to confirm it still reaches the menu and the known P20 stop. Linux verification of this follow-up is pending; my guard harness and Windows results are below. Implemented the configure-time approach from your report in 5c19601.
Verification: the actual new CMake gate, extracted into a local directory-property harness, passes with the SDL capability true and rejects both false and unset with the expected message. This is a guard smoke test, not a native Linux build; this machine has no Linux environment. Windows 11 / WinLibs GCC 15.2 r7: the Release libs build and five focused tests pass, including video_out_output_mode_policy, guest_video_out_high_frame_rate, guest_video_out_output_mode, param_json_parser and param_json_utf8. Native timing for 60 vblanks was 1.001 s default, 0.500 s high and 1.001 s restored. Two offscreen flip tests skipped because that SDL driver lacks Vulkan. Conventions, test registration and diff checks pass. The Windows build/test comparison includes the existing #4021 fix for main's unrelated HostPoll compile failure. That commit is isolated on a local test branch and is not part of this PR update. Full CTest, Linux runtime and Quake II gameplay were not rerun. AI-assisted: yes (Codex). |
|
@PrintOrb verified 5c19601 on Linux: Ubuntu, GCC, Release, NVIDIA RTX 2080 SUPER, 143.86 Hz monitor, SDL on X11 through XWayland. Each configure was in its own clean build directory with the bundled SDL:
The focused tests on the PR head pass 3/3 ( Quake II (PPSA99007, catalog 0.2.0-alpha.1), relinked and run against the PR head alone:
|
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
|
adriadam10
left a comment
There was a problem hiding this comment.
Re-checked the new head (XRandR configure gate, libxrandr-dev in both workflows and BUILD.md): built on current main with the full ctest run (979 passed), including video_out_output_mode_policy, param_json_parser and guest_video_out_output_mode. @Recapi's Linux run covers the configure failure with SDL_X11_XRANDR=OFF and guest_video_out_high_frame_rate running unskipped on a 144 Hz display. Merging.
What
Yamagi Quake II (PPSA99007) stops before its first frame on main
a11c8f7f7:Its packaged param.json declares
attribute3=0x80040, but VideoOut rejects mode 15 unconditionally. Report mode 15 as supported when the window's current display runs at 120 Hz or above, require the HFR permission bit0x40to configure it, and deliver 119.88 Hz vblanks for that port. Mode 1 restores 59.94 Hz. Slower displays retain the unsupported-mode error; display-query failures throw. No game-specific branch, fallback, retry, or runtime environment switch is added.Semantics: PS5_Vulkan R92 console measurements establish that support queries are independent of title permission, configuration requires HFR permission, and mode 15 halves the vblank period. The single-bit permission mask
attribute3 & 0x40is inferred from the measured full value0x80040; it has not been isolated by a console measurement and is recorded as technical debt. Host display mode switching and VRR remain unimplemented and are recorded in TechnicalDebt.Tested
Windows 11 Pro 10.0.26300, WinLibs GCC 15.2.0 r7, AMD Radeon RX 5600 XT, driver 32.0.21045.5002; current display 1920x1080 at 120 Hz. Release runtime
libsbuilt successfully, default SPIRV-Tools setting OFF.guest_video_out_high_frame_rate,guest_video_out_output_mode,guest_video_out_resolution_status, andparam_json_parserpass. Sixty vblanks measure 1.001 s in default mode, 0.500 s in mode 15, and 1.001 s after restoring mode 1. The test uses only HFR bit0x40, and the existing output-mode test checks rejection when permission is absent.--require-high-frame-rate, against preserved main libraries fails with120 Hz output not reported as supported; it passes with this change.video_out_flip_tests controlspasses on the native display. Its CTest offscreen invocation skips because that SDL backend has no Vulkan support here.support=00000001 preset=00000000 result=00000000, the main menu renders, and the first run records at least 2,280 frames before a separatesubmission retirement failedtermination. This fixes the startup blocker; sustained gameplay and keyboard input are not established.video_out_output_mode_policytests the production mode validator directly with refresh rates -1, 0, 30, 59, 60, 119, 120, 144 and 240 Hz, plus option validation and error precedence. It opens no window and does not skip. The revised runtimelibsbuild and this test plusparam_json_parserpass. Removing the slower-display rejection makes the policy test fail at its threshold assertion; restoring it passes.SDL_GetWindowDisplayIndex+SDL_GetCurrentDisplayModepair over five 200,000-call rounds: 78.7-97.4 ns per pair. The per-present-loop query is retained; this does not benchmark other SDL backends or multi-display setups.git diff --checkpass. Full CTest and Linux execution were not run. The earlier native vblank and title results above predate this review follow-up; they were not rerun.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)