[LinuxBSD] Add support for HDR output (Wayland)#102987
[LinuxBSD] Add support for HDR output (Wayland)#102987ArchercatNEO wants to merge 1 commit intogodotengine:masterfrom
Conversation
|
This looks like a good start, I had no idea HDR support in Wayland was this far along. Unfortunately I can't speak to the correctness of the Wayland code as I don't have experience with it. You may want to look at the latest commit for Windows support and gradually implement the remaining methods and features. Generally I'd expect a full implementation to support:
|
e522785 to
b67a8c6
Compare
|
Update on what I have understood each bit of the protocol to mean and what I'm trying to work with: Does the DisplayServer support HDR?
Does the Screen (Wayland
How to manipulate HDR values of this Window (Wayland
What is the preferred screen luminance (for
On the concrete impl side of things
If I got the relation between wayland API and godot hdr rendering stuff correct then it seems like what's left is largely state management in wayland. If you don't mind @Riteo I'd also like to get your feedback on this specially with where to put the more stateful stuff like the compositors prefered icc profile. |
Yea sure! :D I'm currently very busy with a thing I'm working on but I could not resist taking a very quick look at the protocol. I can't really comment on the feature itself as I have very little experience with HDR. That said, the logic you're describing seems great! My only concern is how much the godot-side API depends on the screen since 99% of Wayland stuff revolves around windows, with the screen being a way smaller piece of context compared to traditional servers. Regarding the data layout, the general rule of thumb is to put all state for an object in an aptly-named Actually, you don't even need to make a "global" variable in the thread class; a common approach I've chosen is to consider the objects themselves... objects, and just So you could just do an: ColorManagerState *state = wp_color_manager_get_state(the_actual_color_manager_proxy);
if (state) {
// Stuff!
}And always have a valid state reference. If you implement it like the other methods, it will also automatically check for Crap, this was supposed to be a quick message. There's a lot of similar things I have to document somewhere. I hope that was not too much. If you need some further clarifications feel free to tell me. |
It also needs to support parametric image descriptions, and the primaries and transfer function you want to use. Ofc if you use Vulkan to set the colorspace and HDR metadata, the Vulkan driver also has to support them.
You can output HDR content with Vulkan, but it doesn't (yet) actually have any API to figure out what the compositor is asking for / what the display is capable of.
Please don't do that. Outside of special cases, the color management output should not be used. HDR support should only be determined by whether or not the compositor supports it, the display isn't really relevant beyond the luminance ranges (which you can get from the preferred image description). If you output scRGB or BT2020PQ, the compositor will do the needed transformations to make it work on any display. KWin for example does proper HDR on SDR laptops (as proper as the displays can do at least); while the primary use case for that is HDR videos, it would be cool if that would work with games too. If you want to ensure that you don't waste power presenting HDR content when it's not necessary, you can instead check if the preferred image description has a maximum target luminance that's higher than the reference luminance. If it is higher, displaying HDR makes sense, if it isn't, then it's probably not useful.
The image description just describes what you get it from, so they're about the screen if you get it from the color management output, or about the window if you get the preferred image description from the surface feedback.
Note that while you can have multiple If you want to use the protocol directly, you can do that if you set You can use https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/32038 for testing either approach. There's one more important topic I want to touch on: Reference luminance / paper white. Does Godot already have a centralized setting for it? With Vulkan, the assumption on Wayland is that the paper white is 203 cd/m² with PQ, and 80cd/m² with scRGB, and the compositor will map that to the system wide user setting - so using anything else as the paper white will cause undesired outcomes. With the protocol directly, you can set the reference luminance with Ideally, games would either always use the reference luminance for the transfer function they use, or use the same reference luminance the compositor would like to have, and the preference for this would be completely hidden on Wayland. Same with maximum luminance, it should just be read from the compositor, so that it can automatically adjust to brightness settings changes and to different displays without the user having to change the settings of each game. I'm not sure how you handle that on Windows - technically it also has such settings, but I know the very vast majority of games just ignore them completely. This has already caused some user confusion on Plasma, as the Windows game settings don't necessarily directly translate to the brightness values of the screen, so it would be really great if we could avoid that mess and always have them configure things system wide instead. Sorry for the wall of text, after working on it so long I'm really exited for some great HDR experiences on Linux :) |
|
@DarkKilauea should the Just doing ctrl + shift + f gives no places where these methods are called so I can't tell what the expected use is. In efficiency scenarios |
|
@ArchercatNEO The The general recommendation for games is to avoid the display or compositor performing a conversion to the final values, due to poor results on many displays (if the display/compositor even bothers). That is the primary reason my PR does not provide HDR10 metadata to the system. We should avoid doing the same for Wayland. |
|
The surface feedback is simply the profile the compositor would like us to use according to docs. There is no reason why if the compositor is not able to transform values itself it shouldn’t return the actual profile of the screen and perform no transformation on the incoming data. My fear with not using the feedback and using the screen format is that a compositor may then transform it into its preferred format to do processing and then back into the screens native format when we could have used the compositors format from the beginning. The way I see it there are 3 ways we could do this.
As of now I have no reason to believe compositors would implement a buggy transformation instead of performing no transformation at all which makes me partial to 3. If enumerating screens is a desirable property I would go with 2 because assuming the compositor is buggy and will not perform transformations on its preferred format correctly (as 1 assumes) does not seem that reasonable. For people with more experience with wayland: when a compositor implements a protocol, is assuming a correct implementation reasonable? I don’t have much experience with the reliability of different compositors beyond knowing mutter doesn’t support SSD so knowing if they are so unreliable we should not rely on |
b67a8c6 to
95411c9
Compare
|
Finally got round to a In terms of what I think has to be done before this stops being a draft I think the most important is making sure that if the preferred profile changes (whether that be from the screen itself or from the compositor) and screen_luminance is enabled we set the window luminances. We should also destroy all the wayland objects that we aren't already destroying. After that I think just cleaning up error messages and state management (not setting values that are already equal, remembering whether we tried to enable hdr, etc) are the main things for a mergable PR. I went with |
That's definitely sensible, you don't want to stack multiple tone mapping processes on top of each other if it can be avoided. Even if the compositor does really good tone mapping, it may have a performance cost, so if you do tone mapping either way, you should do all of it in one go. That does not mean you should use the output image description though, as that could require additional conversions. The preferred image description is the most optimal one for the window.
That won't work. Wayland is not like Windows, where HDR metadata at best does nothing and at worst causes issues. If you don't provide HDR metadata, the compositor may (and KWin always does) clip the image to SDR range.
Indeed, that is a likely scenario. If a compositor does blending in linear, if the game renders in linear, converts to BT2020PQ and sends that to the compositor, it'll convert back to linear, then do its compositing, and then convert to BT2020PQ for the display. The first two conversions could be optimized out by providing the compositor with scRGB instead of BT2020PQ for example.
Yes. If you can't trust a compositor far enough to provide an optimal image description for the window, why would you trust it with the output image description? The minimal implementation for the protocol just returns the image description of the output the window is on anyways. |
|
|
||
| void WaylandThread::_wp_color_manager_on_supported_primaries_named(void *data, struct wp_color_manager_v1 *wp_color_manager_v1, uint32_t primaries) { | ||
| IccManagementState *ims = (IccManagementState *)data; | ||
| ims->supported_primaries = ims->supported_primaries | (1 << primaries); |
There was a problem hiding this comment.
primaries, like transfer functions, features and intents are not bit fields but just normal enum values. You need to store them in a list
That's a little concerning and would suggest a project setting is needed to control whether Godot outputs HDR10 metadata to the system. Neither macOS nor Windows seems to require this, so it is odd that Wayland does. It is a new protocol though, so this may change as compositors run into programs that don't provide the required metadata. I wonder if gamescope works the same way? |
|
The mesa drivers for vulkan make use of this protocol. Unless we explicitly tell the vulkan driver to not give the compositor the information it needs (in which case we’d have to do so ourselves) then the compositor will receive the metadata. I read through some of the “what this protocol is and what it isn’t”
In all situations it is better to supply metadata (or just let vulkan do it for us) because otherwise every compositor that implements this protocol will assume we are using sRGB and attempt to fix our rendering. https://gitlab.freedesktop.org/pq/color-and-hdr/-/blob/main/doc/design_goals.md |
It won't change. Proper HDR metadata is fundamental to good HDR support, and without it you will always get a suboptimal experience. KWin does have a fallback for existing but obviously crazy HDR metadata (many Windows games claim to have 10 million nits max_cll, because some engine has really broken HDR support) - but that fallback makes assumptions that may cause tone mapping to be applied when the game wouldn't need it. You do not want that to be applied to Godot. MacOS does need HDR metadata too afaik. It might not clip in all situations, but for example their equivalent of HDR on SDR laptop displays can't really work without it.
Gamescope does Windows behavior as far as it can. Afaik it completely ignores HDR metadata and doesn't do anything in regard to reference luminance. |
|
We seem to be talking past each other a lot. You seem very persistent is asking whether or not the screen our window is on supports HDR or not. As far as I can tell this question does not make sense under Wayland (even if it may make sense in other systems). According to the above link KWin (and I suspect other compositors too) are able to render HDR content on SDR displays and do so in a way that somewhat preserves the HDR nature of the content. The question of SDR/HDR then needs to move on from does the screen support HDR. If we are outputting standard SDR content on an HDR monitor we should default to If we are outputting HDR content on an SDR screen then things may be more complex. Since on wayland the capabilities of the physical screen do not seem relevant we will not use them. If So then to avoid outputting HDR content on a potentially SDR monitor we should make I believe we should aim for as much transparency as possible when implementing this feature and this implementation of |
95411c9 to
321826a
Compare
|
Without a change to how the
But there's a bit more I could do without that so that's this. I'd like it if there was more consistency but I think just pushing developers towards |
I thought there was a preferred vs. supported distinction? But if not, just setting supported only to true if the image description has |
321826a to
0951581
Compare
|
As per a recommendation by riteo: now the color state of screens is in |
|
I have undone the changes relating to |
368f54e to
453bd21
Compare
a3afcb6 to
a7138a7
Compare
Calinou
left a comment
There was a problem hiding this comment.
Tested locally on https://github.com/DarkKilauea/godot-hdr-output (rebased on top of master bf95b62), it works as expected. Code looks good to me.
This time, it's working well on NVIDIA with no special layers installed.
PC specifications
- CPU: AMD Ryzen 9 9950X3D
- GPU: NVIDIA GeForce RTX 5090 (driver 580.119.02)
- RAM: 64 GB (2×32 GB DDR5-6000 CL30)
- SSD: Solidigm P44 Pro 2 TB
- OS: Linux (Fedora 43)
Operating System: Fedora Linux 43
KDE Plasma Version: 6.5.5
KDE Frameworks Version: 6.22.0
Qt Version: 6.10.1
Kernel Version: 6.18.8-200.fc43.x86_64 (64-bit)
Graphics Platform: Wayland
Some notes:
- Custom reference and max luminance are not supported. Values are always read from the system settings.
ERROR: DisplayServer does not support setting a manual reference luminance
at: window_set_hdr_output_reference_luminance (platform/linuxbsd/wayland/display_server_wayland.cpp:1537)
GDScript backtrace (most recent call first):
[0] _on_custom_reference_luminance_toggled (res://hdr_controls.gd:85)
ERROR: DisplayServer does not support setting a manual maximum luminance
at: window_set_hdr_output_max_luminance (platform/linuxbsd/wayland/display_server_wayland.cpp:1555)
GDScript backtrace (most recent call first):
[0] _on_custom_max_luminance_toggled (res://hdr_controls.gd:96)
- This is printed when starting the HDR demo:
WARNING: HDR output requested but no HDR compatible format was found, falling back to SDR.
at: _determine_swap_chain_format (drivers/vulkan/rendering_device_driver_vulkan.cpp:3506)
However, HDR output clearly appears to be functional (and looks correct, with only highlights becoming brighter and more detailed).
- HDR output does not work correctly when using llvmpipe (
--gpu-index 1). You can enable HDR, but everything will look washed out, almost as if you were looking at linear output in sRGB space. This is probably out of our control, but perhaps we should document it if it can be consistently reproduced across systems.
| // If the surface requests HDR output, try to get an HDR format. | ||
| if (context_driver->get_colorspace_externally_managed()) { | ||
| // When the colorspace is externally managed, only the data format matters. | ||
| // TODO: determine if there are any drivers which do not support VK_FORMAT_R16G16B16A16_SFLOAT at all |
There was a problem hiding this comment.
This might be the issue with llvmpipe, I don't know.
There was a problem hiding this comment.
I have investigated this now and have some findings.
First of all, we have indeed found a driver which doesn't support VK_FORMAT_R16G16B16A16_SFLOAT, but it does support VK_FORMAT_R16G16B16A16_UNORM. If I set the format to that we are able to get HDR but get a lot of banding.

Secondly, the thing about it being washed out is actually our fault. Effectively when we enable HDR, the Wayland display server assumes that it actually worked and sets the color space to linear but since it failed due to missing format support, we render sRGB and decode with linear which is what happens here.
In theory we could query the rendering context to ask for the colorspace, but that's only updated the frame after we attempt to set HDR and I don't know if there's any loop in the display server which is synced with frames.
There was a problem hiding this comment.
In theory we could query the rendering context to ask for the colorspace, but that's only updated the frame after we attempt to set HDR and I don't know if there's any loop in the display server which is synced with frames.
process_events is a direct part of OS run loop, always called right before Main::iteration:
godot/platform/linuxbsd/os_linuxbsd.cpp
Lines 986 to 998 in bf95b62
Now, as far as I understand it this means that, as long as low consumption mode is not enabled, you will get a frame after process_events (and thus an eventual wl_surface.commit).
If low consumption mode is enabled you will need to check for RenderingServer::get_singleton()->has_changed(). Then you know that it will render a frame.
This is how I handled frame throttling ("vsync emulation"), see the relevant DisplayServerWayland code:
godot/platform/linuxbsd/wayland/display_server_wayland.cpp
Lines 1870 to 1894 in bf95b62
There was a problem hiding this comment.
I have addressed this thread in general in 2 commits: the first adds support for checking if enabling HDR failed or not. It's a bit dirty because we are only able to check what colorspace the driver used on the frame after we enable HDR so there will be a frame where we claim the surface is using ext linear when it was actually rendered with nonlinear sRGB. I'm not sure if there's a way to make sure the colorspace we report is always correct with API modifications but at least HDR failing will fallback to SDR eventually.
The second simply adds supports for using VK_FORMAT_R16G16B16A16_UNORM so that llvmpipe can do HDR. If there's no major concerns about this we can add it, otherwise we can drop the commit (and I can edit the TODO to mention llvmpipe).
| static constexpr struct wp_color_manager_v1_listener wp_color_manager_listener = { | ||
| .supported_intent = _wp_color_manager_on_supported_intent, | ||
| .supported_feature = _wp_color_manager_on_supported_feature, | ||
| .supported_tf_named = _wp_color_manager_on_supported_tf_named, | ||
| .supported_primaries_named = _wp_color_manager_on_supported_primaries_named, | ||
| .done = _wp_color_manager_on_done, | ||
| }; |
There was a problem hiding this comment.
Designated initializers are a C++20 feature, do we accept them on master yet? Or is this a special case where they could be used in C++17?
There was a problem hiding this comment.
I followed the convention for wayland protocol callbacks that was already here. @deralmas could probably explain why an exception was made here better than me.
There was a problem hiding this comment.
This convention happened because I originally had no idea it was a C only thing xD
Given that the rest of the code does it, I'd keep it like this and figure out if it's worth to get rid of this feature (by removing the initalizers and put the field names in a comment or something) or just wait for the bump to C++20 if it's near.
a7138a7 to
ca756fd
Compare
|
Thank you for the review! And good to know Nvidia HDR is working without any problems now. I applied several of the suggested comment changes, hopefully got them right. As for the warning that gets printed, that should also be fixed now, it was an oversight where we were checking the vulkan colorspace (which may or not represent the actual colorspace we're targetting) instead of the RDD colorspace we will actually use. As for llvmpipe, not sure what's happening there. I will try to test it at some point and see if there's anything I can do on our end but at least it should be a minor enough edgecase. |
|
From a quick glance at the code, I don't see anything that stands out as being obviously incorrect, but I haven't done a thorough review because I don't currently have a setup that I can test with. I'm happy to see that Calinou has been able to test this now, thanks for that! 🎉 Especially with the Steam Machine, I we'll need to see how Steam OS handles reference and max luminance. If they provide very clear user-facing ways to adjust brightness (reference white luminance) and max luminance of external displays, the current state of this PR will be good. If they don't, then we'll need to add the ability to change these two parameters in Godot instead of this PR's current behaviour. This can obviously be done in a followup PR when the time comes, and only if necessary. Thanks for your work on this, ArchercatNEO! |
deralmas
left a comment
There was a problem hiding this comment.
I know basically nothing about HDR so I did a review of the Wayland-specific parts. Just a few nitpicks and notes before I approve, but overall this looks awesome!
(The Ref<T> comment was meant to be part of this review, sorry 😅 )
b67c1ee to
aefc2cd
Compare
thirdparty/wayland-protocols/staging/color-management/color-management-v1.xml
Show resolved
Hide resolved
aefc2cd to
f25114c
Compare
f25114c to
d348794
Compare
d348794 to
3065375
Compare
deralmas
left a comment
There was a problem hiding this comment.
The Wayland side of things looks great!
bruvzg
left a comment
There was a problem hiding this comment.
Code looks good. I was not able to test it, my Fedora install refuse to detect HDR monitor (which works fine on Windows on the same system).
DarkKilauea
left a comment
There was a problem hiding this comment.
I'm ok with the current state of this PR.
We'll have to make some changes in the Android PR if this gets merged before that to address setting the RDD::Colorspace correctly. It might be worth having a follow up PR after all of the HDR PRs are merged to streamline the selection logic for the format and color space.
|
@ArchercatNEO Would you mind updating this PR's description with details about what is required for HDR output? Without considering Linux, this is what I have so far: Steps to enable HDR output in your project:
Does there need to be something about the |
|
For HDR output we need the Wayland display server, "default" for Linux is still X11 so we need to explicitly enable it. For the project you need to set The editor uses a different method for choosing a display server ( I will update main PR description, though do I need to mention any of this in actual doc changes in this PR? |
3065375 to
6bcfed7
Compare
|
Thanks!
I see you already have this in the changes to
Hmm. I'm curious why there is a separate editor setting at all, it sounds like a recipe for problems and also adds complexity to things like the documentation I'm writing for HDR output. Might be worth revisiting this in a separate proposal/PR and removing the editor setting entirely so the editor just follows the project settings if there isn't a good reason that still exists to have a separate editor setting. |
The main rationale is that people might want to use the editor through Wayland but still export with the default display driver order (X11, Wayland) for stability. This was especially relevant when the Wayland backend was still single-window only, but I think that it's still useful to have this distinction until it becomes the default. |
6bcfed7 to
d11c617
Compare
Description updated for 2026/02/23.
Based on #94496 (merged to master)
Implements godotengine/godot-proposals#10817 for Wayland
Testing/Sample project: https://github.com/DarkKilauea/godot-hdr-output
This PR implements the luminance based features of the wp-color-management protocol as well as the minimum amount of colorspace + transfer function features necessary for HDR output.
Due to the unreliability of HDR applying glare compensation or not, SDR over HDR is assumed to be performed against a monitor which does not apply glare compensation which differs from the assumptions made in the Windows and Mac implementations for HDR. The Wayland protocol was made with this assumption so we will not deviate from it despite having the possibility to do so.
Since this feature is exclusive to Wayland, the display server must be set to Wayland in both project settings and editor settings (not setting both to Wayland leads to inconsistencies in display server selection).
For editor:
run/platforms/linuxbsd/prefer_waylandmust be enabled.For project:
display/display_server/driver.linuxbsdset to "wayland" (X11 is still set chosen when "deafault" is selected).