fix(HDR): fix UI and blackscreens when HDR unloaded#2221
Conversation
…LIP_DISCARD compatibility
…redundant logging
|
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:
📝 WalkthroughWalkthroughAdds HDRDisplay::IsFGCompositingThisFrame() and centralizes FG UI-compositing checks; updates SetUIBuffer to route UI at runtime between the wrapped swap-chain back buffer and the dedicated UI buffer based on HDR/FG compositor availability; swap-chain uses FLIP_DISCARD with BufferCount >= 2 and non-HDR sRGB formats map to non-sRGB when HDR is disabled. Changes
Sequence DiagramsequenceDiagram
participant Caller as SetUIBuffer
participant HDR as HDRCompositor
participant FG as FrameGenCompositor
participant Swap as SwapChainBuffer/RTV
rect rgba(200,100,0,0.5)
Note over Caller,HDR: D3D12 flip-model runtime routing
Caller->>HDR: Query "Will HDR composite UI?"
Caller->>FG: Query IsFGCompositingThisFrame()
alt HDR or FG will composite UI
Caller->>Swap: Clear & bind `uiBufferWrapped` (RTV)
else No compositor reads UI
Caller->>Swap: Clear & bind `swapChainBufferWrapped` (RTV) so UI survives Present
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 858-861: The ffxWillComposite predicate in HDRDisplay.cpp is
missing the same main/loading-menu guard used by its sister predicates, which
can route UI into uiBufferWrapped during menus and reintroduce the black-UI bug;
update the ffxWillComposite expression (the line that computes ffxWillComposite
using globals::game::ui and upscaling.settings.frameGenerationMode) to include
the same checks as HDRDisplay::UpdateHDRData and
HDRDisplay::ScaleUIBrightnessForFG—specifically add && !isMainOrLoadingMenu &&
!isVR—so its semantics match the other fg-compositing checks.
🪄 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: e1375e4e-302b-4bfb-97c4-23353b2255dc
📒 Files selected for processing (2)
src/Features/HDRDisplay.cppsrc/Features/Upscaling.cpp
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Features/HDRDisplay.cpp (2)
853-856: Minor: validation onuiBufferWrappedshort-circuits the fallback path unnecessarily.The early return at lines 853–854 bails out when
uiBufferWrapped/rtvis missing, even whenneedsUIBufferwill befalseand onlyswapChainBufferWrappedis actually used. In practice these two wrappers are created/torn down together indx12SwapChain, so this is effectively a no-op today, but it does couple the safe fallback path to a resource it doesn’t consume — ifuiBufferWrappedis ever made optional/lazy, the HDR-unloaded + FG-off case (the scenario this PR fixes) would silently regress to the previous black-screen.Consider checking each wrapper only on the path that actually uses it:
- if (!upscaling.dx12SwapChain.uiBufferWrapped || !upscaling.dx12SwapChain.uiBufferWrapped->rtv) - return; if (!upscaling.dx12SwapChain.swapChainBufferWrapped || !upscaling.dx12SwapChain.swapChainBufferWrapped->rtv) return; @@ - bool needsUIBuffer = hdrWillComposite || ffxWillComposite; + bool needsUIBuffer = hdrWillComposite || ffxWillComposite; + if (needsUIBuffer && + (!upscaling.dx12SwapChain.uiBufferWrapped || !upscaling.dx12SwapChain.uiBufferWrapped->rtv)) + return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` around lines 853 - 856, The current early returns unconditionally bail when upscaling.dx12SwapChain.uiBufferWrapped or its rtv is missing, which prevents the fallback path when needsUIBuffer is false; change the logic so you only validate uiBufferWrapped/rtv when needsUIBuffer is true (e.g., check needsUIBuffer before inspecting upscaling.dx12SwapChain.uiBufferWrapped), and keep the validation for upscaling.dx12SwapChain.swapChainBufferWrapped/rtv as-is so the swap-chain path still fails fast; update the checks around uiBufferWrapped, needsUIBuffer, and swapChainBufferWrapped to perform per-path validation instead of the unconditional early returns.
858-862: Optional: extract the FG-compositing predicate to a single helper.The “FFX FG will composite UI this frame” condition is now expressed in three places with subtly different shapes:
HDRDisplay::SetUIBuffer(this block) — implicitly relies on the VR early-return and the surroundingd3d12SwapChainActivebranch.HDRDisplay::ScaleUIBrightnessForFG(line ~1230).HDRDisplay::UpdateHDRData(line ~1333).They must agree exactly or the UI routing in
SetUIBufferwill desync from what the FG/HDR compositors actually consume. A single private helper (e.g.bool HDRDisplay::IsFGCompositingThisFrame() const) would centralize the rule and eliminate the risk of one site drifting from the others in future changes.♻️ Sketch
+bool HDRDisplay::IsFGCompositingThisFrame() const +{ + auto& upscaling = globals::features::upscaling; + auto* ui = globals::game::ui; + const bool isMainOrLoadingMenu = + globals::state->isMainMenuOpen || globals::state->isLoadingMenuOpen; + return upscaling.d3d12SwapChainActive && + upscaling.settings.frameGenerationMode && + ui && !ui->GameIsPaused() && + !isMainOrLoadingMenu && + !globals::game::isVR; +}Then
SetUIBuffer,ScaleUIBrightnessForFG, andUpdateHDRDataall call this helper instead of re-deriving the condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` around lines 858 - 862, Extract the FG-compositing predicate into a single private helper (e.g. bool HDRDisplay::IsFGCompositingThisFrame() const) that returns the exact condition currently computed in SetUIBuffer: upscaling.settings.frameGenerationMode is true, globals::game::ui exists and !gameUi->GameIsPaused(), and globals::state is not main or loading menu (and preserve any VR/d3d12SwapChainActive preconditions by documenting/handling them where helper is called). Replace the duplicated logic in HDRDisplay::SetUIBuffer, HDRDisplay::ScaleUIBrightnessForFG, and HDRDisplay::UpdateHDRData to call IsFGCompositingThisFrame() so all three sites use the single canonical rule.
🤖 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 853-856: The current early returns unconditionally bail when
upscaling.dx12SwapChain.uiBufferWrapped or its rtv is missing, which prevents
the fallback path when needsUIBuffer is false; change the logic so you only
validate uiBufferWrapped/rtv when needsUIBuffer is true (e.g., check
needsUIBuffer before inspecting upscaling.dx12SwapChain.uiBufferWrapped), and
keep the validation for upscaling.dx12SwapChain.swapChainBufferWrapped/rtv as-is
so the swap-chain path still fails fast; update the checks around
uiBufferWrapped, needsUIBuffer, and swapChainBufferWrapped to perform per-path
validation instead of the unconditional early returns.
- Around line 858-862: Extract the FG-compositing predicate into a single
private helper (e.g. bool HDRDisplay::IsFGCompositingThisFrame() const) that
returns the exact condition currently computed in SetUIBuffer:
upscaling.settings.frameGenerationMode is true, globals::game::ui exists and
!gameUi->GameIsPaused(), and globals::state is not main or loading menu (and
preserve any VR/d3d12SwapChainActive preconditions by documenting/handling them
where helper is called). Replace the duplicated logic in
HDRDisplay::SetUIBuffer, HDRDisplay::ScaleUIBrightnessForFG, and
HDRDisplay::UpdateHDRData to call IsFGCompositingThisFrame() so all three sites
use the single canonical rule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a55f7d7-9223-4e6c-9804-ed2146b606fb
📒 Files selected for processing (1)
src/Features/HDRDisplay.cpp
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 871-873: The hdr routing allows hdrWillComposite to be true even
when the HDR compute shader is missing, causing UI to be sent to uiBufferWrapped
but never copied back; change the hdrWillComposite/needsUIBuffer check to also
require that GetHDROutputCS() returns non-null (i.e., shader available) before
routing UI to uiBufferWrapped (symbols: hdrWillComposite, needsUIBuffer,
GetHDROutputCS(), SetUIBuffer(), uiBufferWrapped, outputTexture, hdrDataCB,
loaded, ffxWillComposite), and update the ApplyHDR() fallback path (where only
kFRAMEBUFFER is copied) to also copy the uiBufferWrapped contents back when the
HDR shader is not present so UI remains visible in degraded shader-missing runs.
🪄 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: f8429479-09d4-4800-95dc-41bde58a0e25
📒 Files selected for processing (2)
src/Features/HDRDisplay.cppsrc/Features/HDRDisplay.h
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Features/HDRDisplay.cpp (1)
872-877: Avoid eager shader lookup when HDR runtime is inactive.
GetHDROutputCS()is queried even when HDR runtime state is not ready (loaded/hdrDataCB/outputTexturefalse). Short-circuiting avoids unnecessary shader compile attempts/log churn in HDR-unloaded sessions.As per coding guidelines: `src/Features/**/*.{h,hpp,cpp,hlsl}` — "Consider GPU workload and performance impact when implementing graphics features, with special attention to shader compilation and runtime performance".Proposed patch
- bool ffxWillComposite = IsFGCompositingThisFrame(); - bool hdrShaderAvailable = GetHDROutputCS() != nullptr; - bool hdrWillComposite = loaded && hdrDataCB && outputTexture && hdrShaderAvailable; + bool ffxWillComposite = IsFGCompositingThisFrame(); + const bool hdrRuntimeReady = loaded && hdrDataCB && outputTexture; + const bool hdrShaderAvailable = hdrRuntimeReady && (GetHDROutputCS() != nullptr); + bool hdrWillComposite = hdrRuntimeReady && hdrShaderAvailable; bool needsUIBuffer = hdrWillComposite || ffxWillComposite; - bool hdrWillFallbackCopy = loaded && hdrDataCB && outputTexture && !hdrShaderAvailable; + bool hdrWillFallbackCopy = hdrRuntimeReady && !hdrShaderAvailable;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` around lines 872 - 877, Currently GetHDROutputCS() is called unconditionally which can trigger shader lookup when HDR runtime isn't ready; change the logic so you first check the HDR readiness (loaded && hdrDataCB && outputTexture) and only then call GetHDROutputCS() to set hdrShaderAvailable and hdrWillComposite. Concretely, test ready = (loaded && hdrDataCB && outputTexture); if ready then hdrShaderAvailable = GetHDROutputCS() != nullptr and set hdrWillComposite = ready && hdrShaderAvailable and hdrWillFallbackCopy = ready && !hdrShaderAvailable; otherwise set hdrShaderAvailable/hdrWillComposite/hdrWillFallbackCopy to false; keep ffxWillComposite computed as before using IsFGCompositingThisFrame().
🤖 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 872-877: Currently GetHDROutputCS() is called unconditionally
which can trigger shader lookup when HDR runtime isn't ready; change the logic
so you first check the HDR readiness (loaded && hdrDataCB && outputTexture) and
only then call GetHDROutputCS() to set hdrShaderAvailable and hdrWillComposite.
Concretely, test ready = (loaded && hdrDataCB && outputTexture); if ready then
hdrShaderAvailable = GetHDROutputCS() != nullptr and set hdrWillComposite =
ready && hdrShaderAvailable and hdrWillFallbackCopy = ready &&
!hdrShaderAvailable; otherwise set
hdrShaderAvailable/hdrWillComposite/hdrWillFallbackCopy to false; keep
ffxWillComposite computed as before using IsFGCompositingThisFrame().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16ad8526-a1e9-48c6-a66e-aeedf1a6d619
📒 Files selected for processing (1)
src/Features/HDRDisplay.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Features/HDRDisplay.cpp (1)
867-895: Routing logic is correct; minor optional cleanup on the readiness booleans.The three-way routing (uiBufferWrapped when a compositor will read it, kFRAMEBUFFER for the ApplyHDR shader-missing fallback, otherwise the wrapped back buffer) correctly fixes the HDR-unloaded blackscreen path and preserves the existing fallback in
ApplyHDR(lines 1021-1024). Theloaded && hdrDataCB && outputTexture && GetHDROutputCS()chain also resolves the previously-flagged shader-availability concern.Tiny optional simplification:
hdrShaderAvailablealready implieshdrReady, sohdrReady && hdrShaderAvailableon line 875 is redundant.♻️ Optional simplification
bool ffxWillComposite = IsFGCompositingThisFrame(); bool hdrReady = loaded && hdrDataCB && outputTexture; - bool hdrShaderAvailable = hdrReady && GetHDROutputCS() != nullptr; - bool hdrWillComposite = hdrReady && hdrShaderAvailable; + bool hdrWillComposite = hdrReady && GetHDROutputCS() != nullptr; bool needsUIBuffer = hdrWillComposite || ffxWillComposite; - bool hdrWillFallbackCopy = hdrReady && !hdrShaderAvailable; + bool hdrWillFallbackCopy = hdrReady && !hdrWillComposite;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/HDRDisplay.cpp` around lines 867 - 895, The variable hdrWillComposite is computed redundantly as "hdrReady && hdrShaderAvailable" even though hdrShaderAvailable is already defined as "hdrReady && GetHDROutputCS() != nullptr"; simplify by changing hdrWillComposite to just use hdrShaderAvailable (i.e., set hdrWillComposite = hdrShaderAvailable) and remove the redundant use of hdrReady in that expression; update references to hdrWillComposite if needed and ensure hdrShaderAvailable remains defined as currently (using GetHDROutputCS()) to preserve the readiness check.
🤖 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 867-895: The variable hdrWillComposite is computed redundantly as
"hdrReady && hdrShaderAvailable" even though hdrShaderAvailable is already
defined as "hdrReady && GetHDROutputCS() != nullptr"; simplify by changing
hdrWillComposite to just use hdrShaderAvailable (i.e., set hdrWillComposite =
hdrShaderAvailable) and remove the redundant use of hdrReady in that expression;
update references to hdrWillComposite if needed and ensure hdrShaderAvailable
remains defined as currently (using GetHDROutputCS()) to preserve the readiness
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f660339a-db06-43be-9da3-0d9c4c58a4d6
📒 Files selected for processing (1)
src/Features/HDRDisplay.cpp
3 participants
fixes #2215
Summary by CodeRabbit