HDR: Implement surface supports HDR output.

[ArchercatNEO]

Fixes #118903
Fixes #119094
Previously the Wayland display server would attempt to enable HDR output and try to detect if it failed afterwards which had some issues. Now we can query the rendering driver for support and avoid ever enabling HDR output when this would fail.

The Windows and Mac display servers have not been modified because D3D12/Metal are guaranteed to support the formats we need and the only vulkan driver for Mac we support is MoltenVk which happens to support the colorspaces because it depends on Metal.

If we ever wish to enable HDR outwith with Vulkan on Windows or in the Android implementation of HDR it would be useful to query this before enabling HDR then.

CC @allenwp

[allenwp]

EDIT: deleted bogus comment.

---

I'm also curious if there is an alternative solution that involves calling a resize when its needed.

[ArchercatNEO]

I'm also curious if there is an alternative solution that involves calling a resize when its needed.

There likely is but personally this feels like the more "correct" solution, I feel that if HDR will fail we shouldn't even try to enable it, instead of trying and then checking if it failed. When I was testing llvmpipe previously I would see one washed out frame where we rendered in SDR but the compositor believed we were in HDR before this entire mechanism rectified everything. With this PR, when I try to enable HDR under llvmpipe I simply get a warning that says it's not supported and never see this misrendered frame.

[allenwp]

This seems to fix the problem, but one time I got this being spammed to my debugger:

 ERROR: A swap chain was not created for the screen.

EDIT: this bug happens with Godot 4.7 beta 1 and is not related to this PR.

Edit again: It seems that this problem is already reported here: #112589 (comment)

[allenwp]

I've tested this PR and it appears to be working well. It fixes both #119094 and #118903.

@ArchercatNEO please add the "fixes #119094" to the description once you are comfortable with this PR being the correct and proper fix for that issue.

[ArchercatNEO]

I don't know how the checkbox in the game view works but I suspect it was victim to my "late enabling HDR" trick where we would disable HDR but check the colorspace before a resize happened so the request was ignored and HDR stayed on. That's just speculation though, and I verified this PR fixes it so unless there are some other issues we should be fine to along with this.

[DarkKilauea]

Looks generally good to me.

A small nitpick is that it would be nice to have the other DisplayServers check this flag like the Wayland server does, just in case a renderer that can't guarantee the HDR formats for its swap chains is added in the future. An example would be if we ever enabled Vulkan HDR in Windows in the future. It would be one less thing that renderer implementations would need to consider when implementing HDR output.

[AceOfSpades-JFK]

I've been taking a look at this PR. This definitely fix things--and in a more concrete than my PR that just refreshes the editor (#118904). Everything works on Plasma 6.6 and GNOME 49. I open the editor on the HDR Output demo and I'm greeted with incredibly bright balls when I open res://tonemap/tonemap.tscn.

I do want to know what the deal was with the removal of lines 1897-1931 in platform/linuxbsd/wayland/display_server_wayland.cpp. I was looking at those lines last night, thinking it'd be related to that. I guess they were unnecessary but idk why that approach was chosen when it would mess with some screen output settings. And what did you replace that with?

[ArchercatNEO]

@DarkKilauea sure, I’ll address the issues you raised in the next rebase, but on the note of nits, is my error comment OK? The only way I’m actually able to trigger it is by selecting llvmpipe which is not a default device, should I mention this somewhere there?

@AceOfSpades-JFK originally, we didn’t have these lines to check whether enabling HDR worked because we assumed it always would. Then we found that llvmpipe didn’t support the formats we need for HDR and I implemented a hack to check if HDR was enabled after it had already failed (to avoid adding new API surface) but that led to a variant of the issue you raised. I again patched over it by checking that if HDR was enabled in the driver we should enable HDR in the display server. Then we had a regression which caused your issue and also a different one, both stemming from this error detection code.

That mechanism was very flaky (as shown by the issues this fixes). Now instead of checking whether enabling HDR failed after trying to enable it, we check that it will actually work before and therefore don’t need to check for failure later.

[DarkKilauea]

DarkKilauea sure, I’ll address the issues you raised in the next rebase, but on the note of nits, is my error comment OK? The only way I’m actually able to trigger it is by selecting llvmpipe which is not a default device, should I mention this somewhere there?

I wonder if we should say in the error that we couldn't find a supported format or color space for HDR and leave it at that. The documentation can have a note mentioning that llvmpipe does not support the needed format/color space for HDR.

It may be helpful for debugging the issue if the error is clear about what is missing.

[allenwp]

Just looked at the new diff that adds the check to all platforms; I'm fine this consistency between all platforms to prevent a bug slipping in sometime in the future unknown.

Defaulting to say HDR isn't supported is always a good fallback, so that other change sounds good to me.

[DarkKilauea]

Latest changes look great to me, thanks!