Skip to content

Commit

Permalink
feat: Add workaround for HDR high-contrast color bug (#57)
Browse files Browse the repository at this point in the history
  • Loading branch information
@FrogTheFrog
FrogTheFrog authored Jul 13, 2024
1 parent 704295f commit e1281ca
Show file tree
Hide file tree
Showing 8 changed files with 444 additions and 41 deletions.
21 changes: 16 additions & 5 deletions src/windows/include/displaydevice/windows/settingsmanager.h
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

// system includes
#include <chrono>
#include <memory>

// local includes
Expand Down Expand Up @@ -52,21 +53,23 @@ namespace display_device {
* @param config Configuration to be used for preparing topology.
* @param topology_before_changes The current topology before any changes.
* @param release_context Specifies whether the audio context should be released at the very end IF everything else has succeeded.
* @param system_settings_touched Inticates whether a "write" operation could have been performed on the OS.
* @return A tuple of (new_state that is to be updated/persisted, device_to_configure, additional_devices_to_configure).
*/
[[nodiscard]] std::optional<std::tuple<SingleDisplayConfigState, std::string, std::set<std::string>>>
prepareTopology(const SingleDisplayConfiguration &config, const ActiveTopology &topology_before_changes, bool &release_context);
prepareTopology(const SingleDisplayConfiguration &config, const ActiveTopology &topology_before_changes, bool &release_context, bool &system_settings_touched);

/**
* @brief Changes or restores the primary device based on the cached state, new state and configuration.
* @param config Configuration to be used for preparing primary device.
* @param device_to_configure The main device to be used for preparation.
* @param guard_fn Reference to the guard function which will be set to restore original state (if needed) in case something else fails down the line.
* @param new_state Reference to the new state which is to be updated accordingly.
* @param system_settings_touched Inticates whether a "write" operation could have been performed on the OS.
* @return True if no errors have occured, false otherwise.
*/
[[nodiscard]] bool
preparePrimaryDevice(const SingleDisplayConfiguration &config, const std::string &device_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state);
preparePrimaryDevice(const SingleDisplayConfiguration &config, const std::string &device_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state, bool &system_settings_touched);

/**
* @brief Changes or restores the display modes based on the cached state, new state and configuration.
Expand All @@ -75,10 +78,11 @@ namespace display_device {
* @param additional_devices_to_configure Additional devices that should be configured.
* @param guard_fn Reference to the guard function which will be set to restore original state (if needed) in case something else fails down the line.
* @param new_state Reference to the new state which is to be updated accordingly.
* @param system_settings_touched Inticates whether a "write" operation could have been performed on the OS.
* @return True if no errors have occured, false otherwise.
*/
[[nodiscard]] bool
prepareDisplayModes(const SingleDisplayConfiguration &config, const std::string &device_to_configure, const std::set<std::string> &additional_devices_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state);
prepareDisplayModes(const SingleDisplayConfiguration &config, const std::string &device_to_configure, const std::set<std::string> &additional_devices_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state, bool &system_settings_touched);

/**
* @brief Changes or restores the HDR states based on the cached state, new state and configuration.
Expand All @@ -87,22 +91,29 @@ namespace display_device {
* @param additional_devices_to_configure Additional devices that should be configured.
* @param guard_fn Reference to the guard function which will be set to restore original state (if needed) in case something else fails down the line.
* @param new_state Reference to the new state which is to be updated accordingly.
* @param system_settings_touched Inticates whether a "write" operation could have been performed on the OS.
* @return True if no errors have occured, false otherwise.
*/
[[nodiscard]] bool
prepareHdrStates(const SingleDisplayConfiguration &config, const std::string &device_to_configure, const std::set<std::string> &additional_devices_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state);
prepareHdrStates(const SingleDisplayConfiguration &config, const std::string &device_to_configure, const std::set<std::string> &additional_devices_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state, bool &system_settings_touched);

/**
* @brief Try to revert the modified settings.
* @param current_topology Topology before this method is called.
* @param system_settings_touched Inticates whether a "write" operation could have been performed on the OS.
* @returns True on success, false otherwise.
* @warning The method assumes that the caller will ensure restoring the topology
* in case of a failure!
*/
[[nodiscard]] bool
revertModifiedSettings();
revertModifiedSettings(const ActiveTopology &current_topology, bool &system_settings_touched);

std::shared_ptr<WinDisplayDeviceInterface> m_dd_api;
std::shared_ptr<AudioContextInterface> m_audio_context_api;
std::unique_ptr<PersistentState> m_persistence_state;

private:
/** @see win_utils::blankHdrStates for more details. */
std::chrono::milliseconds m_hdr_blank_delay { 500 }; // 500ms should be more than enough...
};
} // namespace display_device
24 changes: 24 additions & 0 deletions src/windows/include/displaydevice/windows/settingsutils.h
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

// system includes
#include <chrono>
#include <tuple>

// local includes
Expand Down Expand Up @@ -133,6 +134,29 @@ namespace display_device::win_utils {
const std::set<std::string> &additional_devices_to_configure,
const HdrStateMap &original_states);

/**
* @brief Toggle enabled HDR states off and on again if quick succession.
*
* This is a workaround for a HDR highcontrast color bug which prominently
* happens for IDD HDR displays, but also sometimes (very VERY rarely) for
* dongles.
*
* The bug is cause my more or less any change to the display settings, such as
* enabling HDR display, enabling HDR state, changing display mode of OTHER
* device and so on.
*
* The workaround is to simply turn of HDR, wait a little and then turn it back
* on.
*
* This is what this function does, but only if there are HDR enabled displays
* at the moment.
*
* @param win_dd Interface for interacting with the OS.
* @param delay Delay between OFF and ON states (ON -> OFF -> DELAY -> ON).
*/
void
blankHdrStates(WinDisplayDeviceInterface &win_dd, std::chrono::milliseconds delay);

/**
* @brief Make guard function for the topology.
* @param win_dd Interface for interacting with the OS.
Expand Down
39 changes: 29 additions & 10 deletions src/windows/settingsmanagerapply.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,13 @@ namespace display_device {
DD_LOG(info) << "Active topology before any changes:\n"
<< toJson(topology_before_changes);

bool system_settings_touched { false };
boost::scope::scope_exit hdr_blank_always_executed_guard { [this, &system_settings_touched]() {
if (system_settings_touched) {
win_utils::blankHdrStates(*m_dd_api, m_hdr_blank_delay);
}
} };

bool release_context { false };
boost::scope::scope_exit topology_prep_guard { [this, topology = topology_before_changes, was_captured = m_audio_context_api->isCaptured(), &release_context]() {
// It is possible that during topology preparation, some settings will be reverted for the modified topology.
Expand All @@ -61,7 +68,7 @@ namespace display_device {
}
} };

const auto &prepped_topology_data { prepareTopology(config, topology_before_changes, release_context) };
const auto &prepped_topology_data { prepareTopology(config, topology_before_changes, release_context, system_settings_touched) };
if (!prepped_topology_data) {
// Error already logged
return ApplyResult::DevicePrepFailed;
Expand All @@ -70,21 +77,21 @@ namespace display_device {

DdGuardFn primary_guard_fn { noopFn };
boost::scope::scope_exit<DdGuardFn &> primary_guard { primary_guard_fn };
if (!preparePrimaryDevice(config, device_to_configure, primary_guard_fn, new_state)) {
if (!preparePrimaryDevice(config, device_to_configure, primary_guard_fn, new_state, system_settings_touched)) {
// Error already logged
return ApplyResult::PrimaryDevicePrepFailed;
}

DdGuardFn mode_guard_fn { noopFn };
boost::scope::scope_exit<DdGuardFn &> mode_guard { mode_guard_fn };
if (!prepareDisplayModes(config, device_to_configure, additional_devices_to_configure, mode_guard_fn, new_state)) {
if (!prepareDisplayModes(config, device_to_configure, additional_devices_to_configure, mode_guard_fn, new_state, system_settings_touched)) {
// Error already logged
return ApplyResult::DisplayModePrepFailed;
}

DdGuardFn hdr_state_guard_fn { noopFn };
boost::scope::scope_exit<DdGuardFn &> hdr_state_guard { hdr_state_guard_fn };
if (!prepareHdrStates(config, device_to_configure, additional_devices_to_configure, hdr_state_guard_fn, new_state)) {
if (!prepareHdrStates(config, device_to_configure, additional_devices_to_configure, hdr_state_guard_fn, new_state, system_settings_touched)) {
// Error already logged
return ApplyResult::HdrStatePrepFailed;
}
Expand All @@ -110,7 +117,7 @@ namespace display_device {
}

std::optional<std::tuple<SingleDisplayConfigState, std::string, std::set<std::string>>>
SettingsManager::prepareTopology(const SingleDisplayConfiguration &config, const ActiveTopology &topology_before_changes, bool &release_context) {
SettingsManager::prepareTopology(const SingleDisplayConfiguration &config, const ActiveTopology &topology_before_changes, bool &release_context, bool &system_settings_touched) {
const EnumeratedDeviceList devices { m_dd_api->enumAvailableDevices() };
if (devices.empty()) {
DD_LOG(error) << "Failed to enumerate display devices!";
Expand Down Expand Up @@ -161,7 +168,7 @@ namespace display_device {
if (change_is_needed) {
if (cached_state && !m_dd_api->isTopologyTheSame(cached_state->m_modified.m_topology, new_topology)) {
DD_LOG(warning) << "To apply new display device settings, previous modifications must be undone! Trying to undo them now.";
if (!revertModifiedSettings()) {
if (!revertModifiedSettings(topology_before_changes, system_settings_touched)) {
DD_LOG(error) << "Failed to apply new configuration, because the previous settings could not be reverted!";
return std::nullopt;
}
Expand All @@ -182,6 +189,7 @@ namespace display_device {
}
}

system_settings_touched = true;
if (!m_dd_api->setTopology(new_topology)) {
DD_LOG(error) << "Failed to apply new configuration, because a new topology could not be set!";
return std::nullopt;
Expand All @@ -196,7 +204,7 @@ namespace display_device {
}

bool
SettingsManager::preparePrimaryDevice(const SingleDisplayConfiguration &config, const std::string &device_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state) {
SettingsManager::preparePrimaryDevice(const SingleDisplayConfiguration &config, const std::string &device_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state, bool &system_settings_touched) {
const auto &cached_state { m_persistence_state->getState() };
const auto cached_primary_device { cached_state ? cached_state->m_modified.m_original_primary_device : std::string {} };
const bool ensure_primary { config.m_device_prep == SingleDisplayConfiguration::DevicePreparation::EnsurePrimary };
Expand All @@ -214,6 +222,8 @@ namespace display_device {

const auto try_change { [&](const std::string &new_device, const auto info_preamble, const auto error_log) {
if (current_primary_device != new_device) {
system_settings_touched = true;

DD_LOG(info) << info_preamble << toJson(new_device);
if (!m_dd_api->setAsPrimary(new_device)) {
DD_LOG(error) << error_log;
Expand Down Expand Up @@ -251,7 +261,7 @@ namespace display_device {
}

bool
SettingsManager::prepareDisplayModes(const SingleDisplayConfiguration &config, const std::string &device_to_configure, const std::set<std::string> &additional_devices_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state) {
SettingsManager::prepareDisplayModes(const SingleDisplayConfiguration &config, const std::string &device_to_configure, const std::set<std::string> &additional_devices_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state, bool &system_settings_touched) {
const auto &cached_state { m_persistence_state->getState() };
const auto cached_display_modes { cached_state ? cached_state->m_modified.m_original_modes : DeviceDisplayModeMap {} };
const bool change_required { config.m_resolution || config.m_refresh_rate };
Expand All @@ -270,11 +280,18 @@ namespace display_device {
if (current_display_modes != new_modes) {
DD_LOG(info) << info_preamble << toJson(new_modes);
if (!m_dd_api->setDisplayModes(new_modes)) {
system_settings_touched = true;
DD_LOG(error) << error_log;
return false;
}

guard_fn = win_utils::modeGuardFn(*m_dd_api, current_display_modes);
// It is possible that the display modes will not actually change even though the "current != new" condition is true.
// This is because of some additional internal checks that determine whether the change is actually needed.
// Therefore we should check the current display modes after the fact!
if (current_display_modes != m_dd_api->getCurrentDisplayModes(win_utils::flattenTopology(new_state.m_modified.m_topology))) {
system_settings_touched = true;
guard_fn = win_utils::modeGuardFn(*m_dd_api, current_display_modes);
}
}

return true;
Expand Down Expand Up @@ -307,7 +324,7 @@ namespace display_device {
}

[[nodiscard]] bool
SettingsManager::prepareHdrStates(const SingleDisplayConfiguration &config, const std::string &device_to_configure, const std::set<std::string> &additional_devices_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state) {
SettingsManager::prepareHdrStates(const SingleDisplayConfiguration &config, const std::string &device_to_configure, const std::set<std::string> &additional_devices_to_configure, DdGuardFn &guard_fn, SingleDisplayConfigState &new_state, bool &system_settings_touched) {
const auto &cached_state { m_persistence_state->getState() };
const auto cached_hdr_states { cached_state ? cached_state->m_modified.m_original_hdr_states : HdrStateMap {} };
const bool change_required { config.m_hdr_state };
Expand All @@ -324,6 +341,8 @@ namespace display_device {

const auto try_change { [&](const HdrStateMap &new_states, const auto info_preamble, const auto error_log) {
if (current_hdr_states != new_states) {
system_settings_touched = true;

DD_LOG(info) << info_preamble << toJson(new_states);
if (!m_dd_api->setHdrStates(new_states)) {
DD_LOG(error) << error_log;
Expand Down
Loading

0 comments on commit e1281ca

Please sign in to comment.