fix(HDR): reduce false display detections#2186
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors HDR detection to be swap-chain–centric: uses Changes
Sequence Diagram(s)sequenceDiagram
participant App as "App (HDRDisplay)"
participant SwapChain as "IDXGISwapChain"
participant DXGIOutput as "IDXGIOutput / IDXGIOutput6"
participant DisplayConfig as "DisplayConfigGetDeviceInfo"
App->>SwapChain: GetContainingOutput()
SwapChain-->>App: IDXGIOutput (or fail)
alt GetContainingOutput succeeded
App->>DXGIOutput: GetDesc() -> viewGdiDeviceName
App->>DisplayConfig: DisplayConfigGetDeviceInfo(viewGdiDeviceName) [Compat_ADV_COLOR_INFO_2 -> ADV_COLOR_INFO]
DisplayConfig-->>App: {highDynamicRangeSupported, activeColorMode}
App->>App: set isHDRCapableMonitor (supported) / isHDRMonitor (active)
else GetContainingOutput failed
App->>DXGIOutput: enumerate outputs via adapter / OutputWindow -> IDXGIOutput6::GetDesc1()
DXGIOutput-->>App: ColorSpace
App->>App: infer active state from ColorSpace -> set capability & active accordingly
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No actionable suggestions for changed features. |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Features/HDRDisplay.cpp (1)
128-146: Optional: returntruefrom the DXGI fallback when it successfully populates the out params.The fallback block sets
enabled/supportedviaIDXGIOutput6::GetDesc1but then alwaysreturn false;. CurrentlyDetectHDR()at line 236 discards the return value, so behavior is correct today, but returningfalseafter a successful query makes the function's contract ambiguous (did we fail to detect, or did we detect SDR?) and is a latent footgun for future callers that might gate on the return value.♻️ Proposed refactor
winrt::com_ptr<IDXGIOutput> output; if (SUCCEEDED(swapChain->GetContainingOutput(output.put()))) { winrt::com_ptr<IDXGIOutput6> output6; if (SUCCEEDED(output->QueryInterface(IID_PPV_ARGS(output6.put())))) { DXGI_OUTPUT_DESC1 desc1{}; if (SUCCEEDED(output6->GetDesc1(&desc1))) { enabled = desc1.ColorSpace == DXGI_COLOR_SPACE_RGB_FULL_G2084_NONE_P2020; supported = enabled; logger::debug("[HDR] DXGI fallback detection: colorSpace={}", static_cast<int>(desc1.ColorSpace)); + return true; } } } return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` around lines 128 - 146, The DXGI fallback currently populates enabled/supported via IDXGIOutput6::GetDesc1 but unconditionally returns false; change the fallback in the GetDisplayConfigPathInfo failure branch so that after a successful QueryInterface/GetDesc1 and setting enabled and supported it returns true (only return false if the fallback fails to obtain the output or GetDesc1 fails). Update the return logic around swapChain/GetContainingOutput/QueryInterface/GetDesc1 so GetDisplayConfigPathInfo, DetectHDR, and any future callers can trust the boolean return; keep the existing behavior of logging via logger::debug and only return false on actual failure to populate the out params.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Features/HDRDisplay.cpp`:
- Around line 128-146: The DXGI fallback currently populates enabled/supported
via IDXGIOutput6::GetDesc1 but unconditionally returns false; change the
fallback in the GetDisplayConfigPathInfo failure branch so that after a
successful QueryInterface/GetDesc1 and setting enabled and supported it returns
true (only return false if the fallback fails to obtain the output or GetDesc1
fails). Update the return logic around
swapChain/GetContainingOutput/QueryInterface/GetDesc1 so
GetDisplayConfigPathInfo, DetectHDR, and any future callers can trust the
boolean return; keep the existing behavior of logging via logger::debug and only
return false on actual failure to populate the out params.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ec2a3b1-4d33-4881-8c74-2d5b74f897dc
📒 Files selected for processing (2)
src/Features/HDRDisplay.cppsrc/Features/HDRDisplay.h
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Features/HDRDisplay.cpp (1)
232-243: GuardDetectHDR()against a nullglobals::d3d::swapChain.
IsHDRSupportedAndEnabledandGetSwapChainOutputDeviceNamedereferenceswapChainwithout a null check (line 67).LoadSettingsalready defers auto-detection untilSetupResourcesspecifically because the swap chain isn't valid early (lines 523-524), butDetectHDR()is also invoked fromRestoreDefaultSettings()(line 541) and ispublic static, so a future caller can easily trip this. Cheap defensive guard:🛡️ Proposed fix
bool HDRDisplay::DetectHDR() { bool hdrSupported = false; bool hdrEnabled = false; - IsHDRSupportedAndEnabled(globals::d3d::swapChain, hdrSupported, hdrEnabled); + if (globals::d3d::swapChain) { + IsHDRSupportedAndEnabled(globals::d3d::swapChain, hdrSupported, hdrEnabled); + } else { + logger::warn("[HDR] DetectHDR called before swap chain available"); + } isHDRMonitor = hdrEnabled; isHDRCapableMonitor = hdrSupported;As per coding guidelines: "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` around lines 232 - 243, DetectHDR currently calls IsHDRSupportedAndEnabled using globals::d3d::swapChain without validating the swapChain pointer; add a null check at the start of HDRDisplay::DetectHDR() to return false (and set isHDRMonitor/isHDRCapableMonitor to false) if globals::d3d::swapChain is null so callers like RestoreDefaultSettings or any external caller cannot dereference a null swapChain; update only DetectHDR (referencing globals::d3d::swapChain, IsHDRSupportedAndEnabled, isHDRMonitor, isHDRCapableMonitor) to perform this defensive guard and preserve existing logging behavior when swapChain is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/HDRDisplay.cpp`:
- Around line 80-147: GetDisplayConfigPathInfo currently logs a warning when
GetSwapChainOutputDeviceName fails, which causes a scary warning even when
IsHDRSupportedAndEnabled later recovers via IDXGIOutput6 fallback; also the
fallback redundantly calls GetContainingOutput again so the comment is
misleading. Remove the unconditional logger::warn from GetDisplayConfigPathInfo
(do not emit logs inside that helper), let it simply return false on failure,
and move/log a single warn or debug in IsHDRSupportedAndEnabled after both the
display-config path attempt and the IDXGIOutput6 fallback have failed;
alternatively, if you want to preserve distinct messages, make
GetDisplayConfigPathInfo return an error enum or set an out-flag (e.g., bool
swapchainOutputMissing) so IsHDRSupportedAndEnabled can log a downgraded debug
message when the fallback succeeds and only warn when both methods fail. Ensure
references to GetDisplayConfigPathInfo, IsHDRSupportedAndEnabled,
GetContainingOutput, and IDXGIOutput6::GetDesc1 are updated accordingly.
---
Nitpick comments:
In `@src/Features/HDRDisplay.cpp`:
- Around line 232-243: DetectHDR currently calls IsHDRSupportedAndEnabled using
globals::d3d::swapChain without validating the swapChain pointer; add a null
check at the start of HDRDisplay::DetectHDR() to return false (and set
isHDRMonitor/isHDRCapableMonitor to false) if globals::d3d::swapChain is null so
callers like RestoreDefaultSettings or any external caller cannot dereference a
null swapChain; update only DetectHDR (referencing globals::d3d::swapChain,
IsHDRSupportedAndEnabled, isHDRMonitor, isHDRCapableMonitor) to perform this
defensive guard and preserve existing logging behavior when swapChain is
present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a504a4e8-6177-4831-afee-c6e75cc1b7c2
📒 Files selected for processing (1)
src/Features/HDRDisplay.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Features/HDRDisplay.cpp (1)
21-52:⚠️ Potential issue | 🟠 MajorFix the fallback enum value for
DISPLAYCONFIG_DEVICE_INFO_GET_ADVANCED_COLOR_INFO_2.The polyfill defines this as
13, but the Windows DisplayConfig enum reserves12and13for monitor specialization operations before this request. The correct value is15. With older SDKs lacking this enum, the hardcoded value13causesDisplayConfigGetDeviceInfoto query the wrong request type (SET_MONITOR_SPECIALIZATION instead of GET_ADVANCED_COLOR_INFO_2), breaking HDR capability detection.Also update the struct bitfield
wideColorEnforcedandreservedto match the SDK definition:wideColorEnforcedshould be split intowideColorSupported(1 bit) andwideColorUserEnabled(1 bit), withreservedreduced to 24 bits.Proposed fix
`#ifndef` DISPLAYCONFIG_DEVICE_INFO_GET_ADVANCED_COLOR_INFO_2 -# define DISPLAYCONFIG_DEVICE_INFO_GET_ADVANCED_COLOR_INFO_2 ((DISPLAYCONFIG_DEVICE_INFO_TYPE)13) +# define DISPLAYCONFIG_DEVICE_INFO_GET_ADVANCED_COLOR_INFO_2 ((DISPLAYCONFIG_DEVICE_INFO_TYPE)15) @@ - UINT32 wideColorEnforced: 1; - UINT32 reserved: 25; + UINT32 wideColorSupported: 1; + UINT32 wideColorUserEnabled: 1; + UINT32 reserved: 24;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` around lines 21 - 52, The fallback enum macro DISPLAYCONFIG_DEVICE_INFO_GET_ADVANCED_COLOR_INFO_2 is incorrect (currently 13) — change its value to 15 so DisplayConfigGetDeviceInfo uses GET_ADVANCED_COLOR_INFO_2; in the DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2 struct adjust the bitfield to match the SDK: replace wideColorEnforced with two 1-bit fields wideColorSupported and wideColorUserEnabled, and shrink reserved to 24 bits (keeping the other bit names unchanged), ensuring the union still exposes the UINT32 value member.
♻️ Duplicate comments (1)
src/Features/HDRDisplay.cpp (1)
125-144:⚠️ Potential issue | 🟡 MinorThe DXGI fallback still cannot recover
GetContainingOutputwrapper failures.When
GetDisplayConfigPathInfo()fails becauseGetSwapChainOutputDeviceName()cannot callGetContainingOutput(), this fallback repeats the sameGetContainingOutput()call at Line 131. So the Streamline/frame-gen case described in the comment still returnsfalseinstead of falling back.Possible direction
- if (!GetDisplayConfigPathInfo(swapChain, pathInfo)) { - // GetContainingOutput can fail under Streamline/frame-gen wrappers. Fall back to - // IDXGIOutput6::GetDesc1 which reads the output's current color space directly. + if (!GetDisplayConfigPathInfo(swapChain, pathInfo)) { + // DisplayConfig matching can fail even when the presenting output is available. + // Fall back to IDXGIOutput6::GetDesc1 for those cases.If wrapper recovery is still required, this path needs a fallback that does not depend on
IDXGISwapChain::GetContainingOutput()again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` around lines 125 - 144, The fallback currently re-calls IDXGISwapChain::GetContainingOutput (same failing call) inside the GetDisplayConfigPathInfo failure path; change the fallback to not depend on GetContainingOutput: call swapChain->GetDesc1() (use the IDXGISwapChain1/ GetDesc1 path) to obtain the OutputWindow, use MonitorFromWindow/MonitorFromRect to get the HMONITOR, then enumerate DXGI adapters/outputs (IDXGIFactory::EnumAdapters1 / IDXGIAdapter::EnumOutputs) and call IDXGIOutput::GetDesc1 on each to find the matching output for that HMONITOR; once you find the matching IDXGIOutput/IDXGIOutput6, call GetDesc1 and derive enabled/supported as before (checking desc1.ColorSpace), logging the same debug/warn messages and returning true/false accordingly (refer to GetDisplayConfigPathInfo, GetContainingOutput, swapChain->GetDesc1, MonitorFromWindow, EnumAdapters1, EnumOutputs, IDXGIOutput::GetDesc1, IDXGIOutput6).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Features/HDRDisplay.cpp`:
- Around line 21-52: The fallback enum macro
DISPLAYCONFIG_DEVICE_INFO_GET_ADVANCED_COLOR_INFO_2 is incorrect (currently 13)
— change its value to 15 so DisplayConfigGetDeviceInfo uses
GET_ADVANCED_COLOR_INFO_2; in the DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2 struct
adjust the bitfield to match the SDK: replace wideColorEnforced with two 1-bit
fields wideColorSupported and wideColorUserEnabled, and shrink reserved to 24
bits (keeping the other bit names unchanged), ensuring the union still exposes
the UINT32 value member.
---
Duplicate comments:
In `@src/Features/HDRDisplay.cpp`:
- Around line 125-144: The fallback currently re-calls
IDXGISwapChain::GetContainingOutput (same failing call) inside the
GetDisplayConfigPathInfo failure path; change the fallback to not depend on
GetContainingOutput: call swapChain->GetDesc1() (use the IDXGISwapChain1/
GetDesc1 path) to obtain the OutputWindow, use MonitorFromWindow/MonitorFromRect
to get the HMONITOR, then enumerate DXGI adapters/outputs
(IDXGIFactory::EnumAdapters1 / IDXGIAdapter::EnumOutputs) and call
IDXGIOutput::GetDesc1 on each to find the matching output for that HMONITOR;
once you find the matching IDXGIOutput/IDXGIOutput6, call GetDesc1 and derive
enabled/supported as before (checking desc1.ColorSpace), logging the same
debug/warn messages and returning true/false accordingly (refer to
GetDisplayConfigPathInfo, GetContainingOutput, swapChain->GetDesc1,
MonitorFromWindow, EnumAdapters1, EnumOutputs, IDXGIOutput::GetDesc1,
IDXGIOutput6).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 546a5afd-f194-42e7-8005-c642afedbcba
📒 Files selected for processing (1)
src/Features/HDRDisplay.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/Features/HDRDisplay.cpp (2)
1-1: Nit: conventional commit scope casing.PR title
fix(HDR): reduce false display detectionsuses an uppercase scope; the repo's conventional-commit guideline examples (feat(vr),fix(water)) use lowercase. Considerfix(hdr): reduce false display detectionsto stay consistent. As per coding guidelines: "Format: type(scope): description … Examples: - feat(vr): add cross-eye sampling".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` at line 1, Update the PR title to use a lowercase conventional-commit scope: change the current title `fix(HDR): reduce false display detections` to `fix(hdr): reduce false display detections`; this aligns with the repo guideline examples and the existing symbol/name casing such as HDRDisplay/HDRDisplay.h referenced in the diff.
147-178: Optional: simplify DXGI factory acquisition via the swap chain.The
globals::d3d::device→IDXGIDevice→IDXGIAdapter→IDXGIFactory1chain is longer than needed and is not guarded against a nullglobals::d3d::device. Since you already have the swap chain, you can get its adapter/factory directly, which is also more correct on multi-GPU systems because it resolves the adapter actually presenting this swap chain rather than the one backingglobals::d3d::device:♻️ Proposed refactor
- winrt::com_ptr<IDXGIDevice> dxgiDevice; - winrt::com_ptr<IDXGIAdapter> adapter; - winrt::com_ptr<IDXGIFactory1> factory; - if (SUCCEEDED(globals::d3d::device->QueryInterface(IID_PPV_ARGS(dxgiDevice.put()))) && - SUCCEEDED(dxgiDevice->GetAdapter(adapter.put())) && - SUCCEEDED(adapter->GetParent(IID_PPV_ARGS(factory.put())))) { + winrt::com_ptr<IDXGIAdapter> scAdapter; + winrt::com_ptr<IDXGIFactory1> factory; + if (SUCCEEDED(swapChain->GetParent(IID_PPV_ARGS(scAdapter.put()))) && + SUCCEEDED(scAdapter->GetParent(IID_PPV_ARGS(factory.put())))) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` around lines 147 - 178, Replace the long globals::d3d::device → QueryInterface(IID_PPV_ARGS(dxgiDevice)) → GetAdapter → GetParent chain with the swap chain's output lookup: call the existing swap chain's GetContainingOutput to obtain the IDXGIOutput for the presenting surface, then QueryInterface that output to IDXGIOutput6 and call GetDesc1 to inspect ColorSpace (instead of enumerating factory adapters/outputs). Also add a null-check for globals::d3d::device before any use, and remove the unused IDXGIDevice/IDXGIAdapter/IDXGIFactory1 acquisition (the code paths using QueryInterface, GetAdapter, and GetParent should be replaced). Ensure you still handle DXGI_ERROR_NOT_FOUND and FAILED/ SUCCEEDED checks around GetContainingOutput, QueryInterface(out6), and GetDesc1, and keep the existing logic that sets enabled/supported and logs desc1.ColorSpace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/HDRDisplay.cpp`:
- Around line 138-144: The fallback that picks the primary monitor when
OutputWindow is null (hMonitor = MonitorFromRect(..., MONITOR_DEFAULTTOPRIMARY))
can misidentify the active display; update the logic around OutputWindow /
hMonitor (the MonitorFromWindow/MonitorFromRect path) to avoid silently guessing
the primary monitor: either (preferred) bail out and return false when no
reliable monitor can be determined, or at minimum log the picked monitor’s
identity by querying the chosen IDXGIOutput6's description (outDesc.DeviceName)
so misdetection is diagnosable; ensure the change is applied where hMonitor is
derived and before any ColorSpace/IDXGIOutput6 lookups.
---
Nitpick comments:
In `@src/Features/HDRDisplay.cpp`:
- Line 1: Update the PR title to use a lowercase conventional-commit scope:
change the current title `fix(HDR): reduce false display detections` to
`fix(hdr): reduce false display detections`; this aligns with the repo guideline
examples and the existing symbol/name casing such as HDRDisplay/HDRDisplay.h
referenced in the diff.
- Around line 147-178: Replace the long globals::d3d::device →
QueryInterface(IID_PPV_ARGS(dxgiDevice)) → GetAdapter → GetParent chain with the
swap chain's output lookup: call the existing swap chain's GetContainingOutput
to obtain the IDXGIOutput for the presenting surface, then QueryInterface that
output to IDXGIOutput6 and call GetDesc1 to inspect ColorSpace (instead of
enumerating factory adapters/outputs). Also add a null-check for
globals::d3d::device before any use, and remove the unused
IDXGIDevice/IDXGIAdapter/IDXGIFactory1 acquisition (the code paths using
QueryInterface, GetAdapter, and GetParent should be replaced). Ensure you still
handle DXGI_ERROR_NOT_FOUND and FAILED/ SUCCEEDED checks around
GetContainingOutput, QueryInterface(out6), and GetDesc1, and keep the existing
logic that sets enabled/supported and logs desc1.ColorSpace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea9dda17-663d-4a62-a246-bdef4b22adc7
📒 Files selected for processing (1)
src/Features/HDRDisplay.cpp
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Features/HDRDisplay.cpp (1)
20-53:⚠️ Potential issue | 🔴 CriticalUse namespace-scoped compatibility names to avoid redefining Win11 SDK symbols.
The
#ifndef DISPLAYCONFIG_DEVICE_INFO_GET_ADVANCED_COLOR_INFO_2guard cannot detect enum values—only preprocessor macros. In newer Windows SDKs, this identifier is an enum value inDISPLAYCONFIG_DEVICE_INFO_TYPE, so the preprocessor cannot see it. The guard evaluates to TRUE even with modern headers, causing the code to redeclareDISPLAYCONFIG_ADVANCED_COLOR_MODEandDISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2, which already exist in the SDK, resulting in typedef redefinition errors.Replace the conflicting typedefs with namespace-scoped compatibility names to prevent collisions across SDK versions.
🛠️ Proposed compatibility-safe approach
-#ifndef DISPLAYCONFIG_DEVICE_INFO_GET_ADVANCED_COLOR_INFO_2 -# define DISPLAYCONFIG_DEVICE_INFO_GET_ADVANCED_COLOR_INFO_2 ((DISPLAYCONFIG_DEVICE_INFO_TYPE)15) - -typedef enum +namespace Win11AdvancedColorCompat { - DISPLAYCONFIG_ADVANCED_COLOR_MODE_SDR = 0, - DISPLAYCONFIG_ADVANCED_COLOR_MODE_WCG = 1, - DISPLAYCONFIG_ADVANCED_COLOR_MODE_HDR = 2 -} DISPLAYCONFIG_ADVANCED_COLOR_MODE; + constexpr auto kGetAdvancedColorInfo2 = static_cast<DISPLAYCONFIG_DEVICE_INFO_TYPE>(15); -typedef struct DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2 -{ - DISPLAYCONFIG_DEVICE_INFO_HEADER header; - union + enum DISPLAYCONFIG_ADVANCED_COLOR_MODE_COMPAT { - struct + DISPLAYCONFIG_ADVANCED_COLOR_MODE_SDR_COMPAT = 0, + DISPLAYCONFIG_ADVANCED_COLOR_MODE_WCG_COMPAT = 1, + DISPLAYCONFIG_ADVANCED_COLOR_MODE_HDR_COMPAT = 2 + }; + + struct DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2_COMPAT + { + DISPLAYCONFIG_DEVICE_INFO_HEADER header; + union { + struct + { - UINT32 advancedColorSupported: 1; - UINT32 advancedColorActive: 1; - UINT32 reserved1: 1; - UINT32 advancedColorLimitedByPolicy: 1; - UINT32 highDynamicRangeSupported: 1; - UINT32 highDynamicRangeUserEnabled: 1; - UINT32 wideColorSupported: 1; - UINT32 wideColorUserEnabled: 1; - UINT32 reserved: 24; + UINT32 advancedColorSupported: 1; + UINT32 advancedColorActive: 1; + UINT32 reserved1: 1; + UINT32 advancedColorLimitedByPolicy: 1; + UINT32 highDynamicRangeSupported: 1; + UINT32 highDynamicRangeUserEnabled: 1; + UINT32 wideColorSupported: 1; + UINT32 wideColorUserEnabled: 1; + UINT32 reserved: 24; + }; + UINT32 value; }; - UINT32 value; + DISPLAYCONFIG_COLOR_ENCODING colorEncoding; + UINT32 bitsPerColorChannel; + DISPLAYCONFIG_ADVANCED_COLOR_MODE_COMPAT activeColorMode; }; - DISPLAYCONFIG_COLOR_ENCODING colorEncoding; - UINT32 bitsPerColorChannel; - DISPLAYCONFIG_ADVANCED_COLOR_MODE activeColorMode; -} DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2; -#endif +}- DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2 colorInfo2{}; - colorInfo2.header.type = DISPLAYCONFIG_DEVICE_INFO_GET_ADVANCED_COLOR_INFO_2; + Win11AdvancedColorCompat::DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2_COMPAT colorInfo2{}; + colorInfo2.header.type = Win11AdvancedColorCompat::kGetAdvancedColorInfo2; colorInfo2.header.size = sizeof(colorInfo2); colorInfo2.header.adapterId = pathInfo.targetInfo.adapterId; colorInfo2.header.id = pathInfo.targetInfo.id; if (DisplayConfigGetDeviceInfo(&colorInfo2.header) == ERROR_SUCCESS) { supported = colorInfo2.highDynamicRangeSupported != 0; - enabled = colorInfo2.activeColorMode == DISPLAYCONFIG_ADVANCED_COLOR_MODE_HDR; + enabled = colorInfo2.activeColorMode == Win11AdvancedColorCompat::DISPLAYCONFIG_ADVANCED_COLOR_MODE_HDR_COMPAT;Also applies to: lines 184-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` around lines 20 - 53, The preprocessor guard using DISPLAYCONFIG_DEVICE_INFO_GET_ADVANCED_COLOR_INFO_2 fails on newer SDKs because that identifier is an enum member (not a macro), causing your typedefs DISPLAYCONFIG_ADVANCED_COLOR_MODE and DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2 to be unconditionally redeclared; to fix, stop redefining the SDK symbols and instead add compatibility-scoped names (e.g., Compat_DISPLAYCONFIG_ADVANCED_COLOR_MODE and Compat_DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2) in your project namespace and only define them when the SDK types are not available, then update any local usage to those compatibility names (or add conditional using/type-alias that maps Compat_* to the SDK types when they already exist); apply the same change for the similar block at the later lines referenced (lines 184-191) so you never directly typedef/enum the SDK identifiers DISPLAYCONFIG_ADVANCED_COLOR_MODE or DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Features/HDRDisplay.cpp`:
- Around line 20-53: The preprocessor guard using
DISPLAYCONFIG_DEVICE_INFO_GET_ADVANCED_COLOR_INFO_2 fails on newer SDKs because
that identifier is an enum member (not a macro), causing your typedefs
DISPLAYCONFIG_ADVANCED_COLOR_MODE and DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2 to
be unconditionally redeclared; to fix, stop redefining the SDK symbols and
instead add compatibility-scoped names (e.g.,
Compat_DISPLAYCONFIG_ADVANCED_COLOR_MODE and
Compat_DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2) in your project namespace and
only define them when the SDK types are not available, then update any local
usage to those compatibility names (or add conditional using/type-alias that
maps Compat_* to the SDK types when they already exist); apply the same change
for the similar block at the later lines referenced (lines 184-191) so you never
directly typedef/enum the SDK identifiers DISPLAYCONFIG_ADVANCED_COLOR_MODE or
DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d37c4a02-5f92-4b73-9ad1-0625799ec11a
📒 Files selected for processing (1)
src/Features/HDRDisplay.cpp
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/HDRDisplay.cpp`:
- Around line 24-35: When building against the newer SDK branch (the `#if` path)
the enum value constants like Compat_DISPLAYCONFIG_ADVANCED_COLOR_MODE_HDR are
missing; add compat aliases in that branch by defining constexpr values (or enum
constants) that map Compat_DISPLAYCONFIG_ADVANCED_COLOR_MODE_SDR/WCG/HDR to the
corresponding DISPLAYCONFIG_ADVANCED_COLOR_MODE_* values from
DISPLAYCONFIG_ADVANCED_COLOR_MODE so code using
Compat_DISPLAYCONFIG_ADVANCED_COLOR_MODE_HDR (and the other enum constants)
compiles; update the `#if` block where Compat_DISPLAYCONFIG_ADVANCED_COLOR_MODE
and Compat_DISPLAYCONFIG_GET_ADVANCED_COLOR_INFO_2 are aliased to also declare
the three Compat_DISPLAYCONFIG_ADVANCED_COLOR_MODE_* constant aliases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2181cb8f-d0e0-417c-8ad3-5ef19c780960
📒 Files selected for processing (1)
src/Features/HDRDisplay.cpp
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Features/HDRDisplay.cpp (1)
156-189: Enumerating the full factory is unnecessary — the device's adapter is already known.You already have
adapter(the DXGI adapter backingglobals::d3d::device). The swap chain is tied to that adapter, so you only need to enumerate its outputs rather than re-enumerating every adapter in the system viafactory->EnumAdapters1. This simplifies the fallback and avoids touching unrelated GPUs on multi-adapter systems (e.g. iGPU + dGPU laptops).♻️ Proposed simplification
- winrt::com_ptr<IDXGIDevice> dxgiDevice; - winrt::com_ptr<IDXGIAdapter> adapter; - winrt::com_ptr<IDXGIFactory1> factory; - if (globals::d3d::device && - SUCCEEDED(globals::d3d::device->QueryInterface(IID_PPV_ARGS(dxgiDevice.put()))) && - SUCCEEDED(dxgiDevice->GetAdapter(adapter.put())) && - SUCCEEDED(adapter->GetParent(IID_PPV_ARGS(factory.put())))) { - for (UINT ai = 0;; ++ai) { - winrt::com_ptr<IDXGIAdapter1> adap; - if (factory->EnumAdapters1(ai, adap.put()) == DXGI_ERROR_NOT_FOUND) - break; - for (UINT oi = 0;; ++oi) { - winrt::com_ptr<IDXGIOutput> out; - if (adap->EnumOutputs(oi, out.put()) == DXGI_ERROR_NOT_FOUND) - break; - DXGI_OUTPUT_DESC outDesc{}; - if (FAILED(out->GetDesc(&outDesc))) - continue; - if (outDesc.Monitor != hMonitor) - continue; - winrt::com_ptr<IDXGIOutput6> out6; - if (SUCCEEDED(out->QueryInterface(IID_PPV_ARGS(out6.put())))) { - DXGI_OUTPUT_DESC1 desc1{}; - if (SUCCEEDED(out6->GetDesc1(&desc1))) { - enabled = desc1.ColorSpace == DXGI_COLOR_SPACE_RGB_FULL_G2084_NONE_P2020; - supported = enabled; - logger::debug("[HDR] DXGI fallback detection: colorSpace={}", static_cast<int>(desc1.ColorSpace)); - return true; - } - } - } - } - } + winrt::com_ptr<IDXGIDevice> dxgiDevice; + winrt::com_ptr<IDXGIAdapter> adapter; + if (globals::d3d::device && + SUCCEEDED(globals::d3d::device->QueryInterface(IID_PPV_ARGS(dxgiDevice.put()))) && + SUCCEEDED(dxgiDevice->GetAdapter(adapter.put()))) { + for (UINT oi = 0;; ++oi) { + winrt::com_ptr<IDXGIOutput> out; + if (adapter->EnumOutputs(oi, out.put()) == DXGI_ERROR_NOT_FOUND) + break; + DXGI_OUTPUT_DESC outDesc{}; + if (FAILED(out->GetDesc(&outDesc)) || outDesc.Monitor != hMonitor) + continue; + winrt::com_ptr<IDXGIOutput6> out6; + if (SUCCEEDED(out->QueryInterface(IID_PPV_ARGS(out6.put())))) { + DXGI_OUTPUT_DESC1 desc1{}; + if (SUCCEEDED(out6->GetDesc1(&desc1))) { + enabled = desc1.ColorSpace == DXGI_COLOR_SPACE_RGB_FULL_G2084_NONE_P2020; + supported = enabled; + logger::debug("[HDR] DXGI fallback detection: colorSpace={}", static_cast<int>(desc1.ColorSpace)); + return true; + } + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` around lines 156 - 189, The code needlessly re-enumerates all adapters via factory->EnumAdapters1; since you already obtained the device's adapter (adapter) from globals::d3d::device, restrict output enumeration to that adapter only. Replace the outer factory/EnumAdapters1 loop and local adap with a single loop calling adapter->EnumOutputs(oi, out.put()) (keep the existing inner output handling and QueryInterface to IDXGIOutput6/getDesc1), remove factory and adap usage, and ensure enabled/supported and the debug log remain unchanged; this prevents touching unrelated GPUs on multi-adapter systems.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Features/HDRDisplay.cpp`:
- Around line 156-189: The code needlessly re-enumerates all adapters via
factory->EnumAdapters1; since you already obtained the device's adapter
(adapter) from globals::d3d::device, restrict output enumeration to that adapter
only. Replace the outer factory/EnumAdapters1 loop and local adap with a single
loop calling adapter->EnumOutputs(oi, out.put()) (keep the existing inner output
handling and QueryInterface to IDXGIOutput6/getDesc1), remove factory and adap
usage, and ensure enabled/supported and the debug log remain unchanged; this
prevents touching unrelated GPUs on multi-adapter systems.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b2257c23-028d-404b-aa32-d0f29ca28a95
📒 Files selected for processing (1)
src/Features/HDRDisplay.cpp
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Features/HDRDisplay.cpp (1)
1-1: Consider normalizing the PR title scope casing.Current title is close, but the guideline examples use lowercase scopes. Suggested title:
fix(hdr): reduce false display detections. If this maps to a tracked bug, also addFixes #...orRelated to #...in the PR description.As per coding guidelines, “Conventional Commit Titles … Format: type(scope): description … Style: lowercase description, no ending period” and “Issue References … Suggest adding appropriate GitHub keywords.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` at line 1, The PR title scope should use lowercase per Conventional Commit rules; rename the PR title from something like "fix(HDRDisplay): ..." to "fix(hdr): reduce false display detections" and update the PR description to include an issue reference using GitHub keywords (e.g., "Fixes #<issue>" or "Related to #<issue>"); also ensure any commit messages touching HDRDisplay.cpp / HDRDisplay.h follow the same lowercase scope convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/HDRDisplay.cpp`:
- Around line 137-143: The EnumOutputs loop in HDRDisplay.cpp currently only
checks for DXGI_ERROR_NOT_FOUND and then unconditionally calls out->GetDesc(),
which can dereference a null com_ptr on other failures; change the
adapter->EnumOutputs handling to capture the HRESULT (e.g., hr =
adapter->EnumOutputs(oi, out.put())), then: if hr == DXGI_ERROR_NOT_FOUND break
the loop; if FAILED(hr) continue (or log and continue) so you never call
out->GetDesc() when EnumOutputs failed; then call out->GetDesc(&outDesc) only
when EnumOutputs succeeded and out is valid.
---
Nitpick comments:
In `@src/Features/HDRDisplay.cpp`:
- Line 1: The PR title scope should use lowercase per Conventional Commit rules;
rename the PR title from something like "fix(HDRDisplay): ..." to "fix(hdr):
reduce false display detections" and update the PR description to include an
issue reference using GitHub keywords (e.g., "Fixes #<issue>" or "Related to
#<issue>"); also ensure any commit messages touching HDRDisplay.cpp /
HDRDisplay.h follow the same lowercase scope convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 289a4060-741c-4930-ae12-b99eaeb5e584
📒 Files selected for processing (1)
src/Features/HDRDisplay.cpp
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Features/HDRDisplay.cpp (1)
137-143:⚠️ Potential issue | 🟠 MajorBreak on
EnumOutputsfailures to prevent unbounded loop during HDR detection.When
EnumOutputsreturns a persistent failure likeDXGI_ERROR_NOT_CURRENTLY_AVAILABLE(in Session 0 processes), the current code continues indefinitely, incrementingoithrough the fullUINTrange. This can cause hangs during plugin initialization. Break on non-DXGI_ERROR_NOT_FOUNDfailures and log the error for debugging.Proposed fix
for (UINT oi = 0;; ++oi) { winrt::com_ptr<IDXGIOutput> out; HRESULT hr = adapter->EnumOutputs(oi, out.put()); if (hr == DXGI_ERROR_NOT_FOUND) break; - if (FAILED(hr)) - continue; + if (FAILED(hr)) { + logger::warn("[HDR] DXGI fallback EnumOutputs failed at index {}: 0x{:08X}", oi, static_cast<std::uint32_t>(hr)); + break; + } DXGI_OUTPUT_DESC outDesc{};PR metadata note: consider normalizing the title to
fix(hdr): reduce false display detectionsand addingFixes #.../Addresses #...if tracking a known issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` around lines 137 - 143, In HDRDisplay.cpp inside the loop using adapter->EnumOutputs(oi, out.put()) (the for loop that declares UINT oi and winrt::com_ptr<IDXGIOutput> out), change behavior so that when EnumOutputs returns a failure other than DXGI_ERROR_NOT_FOUND (e.g., DXGI_ERROR_NOT_CURRENTLY_AVAILABLE) you break out of the loop instead of continuing; also log the HRESULT error for debugging before breaking (use the existing logging facility in this module). This prevents iterating oi to the UINT wraparound on persistent EnumOutputs failures and keeps normal behavior of breaking on DXGI_ERROR_NOT_FOUND.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/HDRDisplay.cpp`:
- Around line 186-192: The legacy DisplayConfigGetDeviceInfo path currently sets
enabled = colorInfo.advancedColorEnabled != 0 unconditionally; change it to gate
HDR enablement on the swap-chain output color space like the DXGI fallback: keep
supported = colorInfo.advancedColorSupported != 0 but set enabled =
(colorInfo.advancedColorEnabled != 0) && (swapChainOutputColorSpace ==
DXGI_COLOR_SPACE_RGB_FULL_G2084_NONE_P2020) (use the same variable/name used
elsewhere for the current output color space), and update the debug log to
reflect the gated result; reference DisplayConfigGetDeviceInfo,
colorInfo.advancedColorEnabled, enabled, supported, and
DXGI_COLOR_SPACE_RGB_FULL_G2084_NONE_P2020.
---
Duplicate comments:
In `@src/Features/HDRDisplay.cpp`:
- Around line 137-143: In HDRDisplay.cpp inside the loop using
adapter->EnumOutputs(oi, out.put()) (the for loop that declares UINT oi and
winrt::com_ptr<IDXGIOutput> out), change behavior so that when EnumOutputs
returns a failure other than DXGI_ERROR_NOT_FOUND (e.g.,
DXGI_ERROR_NOT_CURRENTLY_AVAILABLE) you break out of the loop instead of
continuing; also log the HRESULT error for debugging before breaking (use the
existing logging facility in this module). This prevents iterating oi to the
UINT wraparound on persistent EnumOutputs failures and keeps normal behavior of
breaking on DXGI_ERROR_NOT_FOUND.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 374d96ac-5e61-4dab-a16e-805656d00642
📒 Files selected for processing (1)
src/Features/HDRDisplay.cpp
|
✅ A pre-release build is available for this PR: |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
3 participants
Attempt at reducing false positives and negatives. Also added more information for the user to understand what is going on.
Still needs to be tested, but works on my machine.
based on methods given to me by shortfuse(RenoDX) and Pumbo.
Summary by CodeRabbit
Bug Fixes
New Features