-
-
Notifications
You must be signed in to change notification settings - Fork 553
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add reshade::api::display in order to implement HDR display mastering metadata. #336
base: main
Are you sure you want to change the base?
Conversation
…hanges, and HDR mastering metadata support to sk_hdr_png
/// <summary> | ||
/// Describes quantities defined precisely as the ratio of two integers, such as refresh rates. | ||
/// </summary> | ||
struct rational |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with other parts of the API, I'd forego this type. swapchain_desc::fullscreen_refresh_rate
e.g. is a floating point value describing the refresh rate, not a rational type (and there is some logic at https://github.com/crosire/reshade/blob/main/source/dxgi/dxgi.cpp#L21 to convert between the two), so it would be strange if the display type suddenly expressed the refresh rate in a different manner.
return api::color_space::hdr10_hlg; | ||
default: | ||
return api::color_space::unknown; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could include d3d12/d3d12_impl_type_convert.hpp
and just call return d3d12::convert_color_space(_desc.ColorSpace);
here to avoid some code duplication.
else | ||
{ | ||
log::message(log::level::warning, "Unable to map runtime %p's swap chain to a containing display output!", this); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be decoupled from the effect runtime and instead be put in the swapchain implementation, or maybe just into dxgi/dxgi_swapchain.cpp
directly? There can technically be swap chains without an effect runtime initialized for them, in which case the events wouldn't fire.
/// <remarks> | ||
/// This device name is not valid as a persistent display identifier for storage in configuration files, it may not refer to the same display device after a reboot. | ||
/// </remarks> | ||
virtual const wchar_t* get_device_name() const final; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ReShade API uses UTF-8 strings everywhere else, so ideally these should be converted too. I'm still in favor of a reshade::api::device::get_property
alike instead of all the individual getter functions, but maybe that's just me =P
In order to finish HDR PNG screenshot integration, the capabilities of the display the image is captured on are required.
Thus, I have added
reshade::api::display
as a new api object to encapsulate display devices (its current implementation has a backingIDXGIOutput
that exposes most of its functionality) and an event for AddOns to subscribe to so that they can be notified when display settings change or when the SwapChain's window moves to a different display.Display Change is detected by checking the position of the SwapChain's window each frame for movement, and checking the associated monitor for the HWND on any frame where the window moves.
There is a broader indication of display change, sent to all top-level windows in the form of
WM_DISPLAYCHANGE
. It does not indicate which display has changed, but it is a signal that at least one display on the desktop has changed and any cached display data should be invalidated.WM_DISPLAYCHANGE
from input.cpp since it is the only code in ReShade that is actually processing Window Messages, I hope that is not a problem.