[Windows] Add support for AdvancedColorInfo info and change callback.#118339
Conversation
bruvzg
force-pushed
the
winrt_color
branch
4 times, most recently
from
7172109 to
5e06576
Compare
|
🤩
Any chance this PR can be updated to include this? Although this PR does solve the problem of the reference luminance not updating until the window regains focus, the real high-priority impetus for this change is to improve performance significantly by not needing to poll for changes in colour space and max luminance. It will also be critically important to test if the |
Derp. My mistake! I think this PR already does this! I'll give this a thorough test. It would be great to have this for 4.7... |
|
I just tested this on my "problematic" computer now. I was able to produce a bug in this PR where the screen would be in HDR output mode (from pressing the Win + Alt + B shortcut), but Godot would think the Window did not support HDR output. This appears to be because a But, the new screen information includes a https://github.com/allenwp/godot/tree/winrt_color My branch has some remaining issues. Specifically, I needed to remove the Also, in this PR, I'd be fine only including the items we actually will use in the dictionary that After doing some more testing, I am now no longer able to reproduce the issue with this PR... but the commit just before this PR ( I'll continue trying to reproduce these two issues to firmly verify what is going on here and that the behaviour in my branch is the best approach for Godot... |
What's causing it not to work with
If we only need color type,
No idea, but it's possible that working WinRT dispatcher queue for the window can result in DirectX getting some extra system events as well. |
|
I'll apply your changes (+ const fix and change from dict to return args) and retest in a bit later today. |
Co-authored-by: Allen Pestaluky <allenpestaluky@gmail.com>
|
Updated it and it seems to be working fine. I was not able to reproduce issue with Win + Alt + B switching, all HDR data for new and old method is always the same. |
|
Thanks, if my computer is still producing the weird bug with the dev releases, I’ll post an issue and demonstrate that this PR fixes it. Yesterday, at least, it became very consistent. I must have some funny NVIDIA drivers or something for my 5070 Ti… |
|
I've created a bug report. This PR fixes the bug: #118618 |
|
I've requested review from blueskythlikesclouds, also cc @DarkKilauea. You two are both familiar with this code, so I figure you might want to take a look. In short, this PR should prevent any of the "legacy" HDR screen info functions from running and instead exclusively use the new WinRT advanced colour info functions and event when they are available. This should improve performance by not needing to poll anything and also seems to fix two bugs that I had been experiencing relating to the colour space that I have reviewed the changes to |
Calinou
left a comment
Calinou
left a comment
There was a problem hiding this comment.
Tested locally on Windows 11 24H2, it works as expected.
I've tested changes to the SDR content brightness slider at runtime, as well as toggling HDR on the system while the project is running (on -> off, or off -> on). Everything updates in real-time (no more need to focus the window to update SDR content brightness).
blueskythlikesclouds
left a comment
blueskythlikesclouds
left a comment
There was a problem hiding this comment.
Looks good to me. The CPU cost is also basically gone.
|
Thanks! |
…RT queue on shutdown Follow-up on godotengine#118339. Two correctness fixes in the WinRT AdvancedColorInfo path: 1. _winrt_adv_color_info_cb and _get_screen_hdr_data access windows[p_window] without the windows.has(...) guard that every other entry point on this class already uses. The WinRT event is delivered via call_deferred, so a window can be closed between the event firing and the deferred call running. HashMap::operator[] then auto-inserts a default WindowData (non-const) or hits UB (const), neither of which is intended. 2. WinRTUtils::destroy_queue calls controller.ShutdownQueueAsync() without awaiting the returned IAsyncAction. If an AdvancedColorInfoChanged callback is in flight when ~DisplayServerWindows runs, it can execute against partially destroyed members. Block on .get() during shutdown so teardown waits for in-flight events to drain.
Follow-up on godotengine#118339, addressing review feedback on godotengine#118669. Move the windows.has(...) guard from the internal _get_screen_hdr_data helper to the public HDR methods that own the WindowID lookup, so the check sits at the API boundary like every other public method on DisplayServerWindows. Also keep the guard on the WinRT-deferred callback _winrt_adv_color_info_cb, where the callback can fire after the window is gone. Public methods guarded: - window_is_hdr_output_supported - window_request_hdr_output - window_set_hdr_output_reference_luminance - window_set_hdr_output_max_luminance The previously proposed controller.ShutdownQueueAsync().get() change in winrt_utils.cpp was dropped: bruvzg confirmed it would deadlock, since destroy_queue runs on the main thread after the event pump has already stopped, so .get() would never return.
6 participants
See #117837 (comment)
Note: this will not work on Wine and some older Windows 10 versions (pre-1809 / Redstone 5), so we still need to keep polling code for this case.
Fixes #118618