feat(VR): add DeepDVC dynamic vibrance#2091
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:
📝 WalkthroughWalkthroughIntroduces a new DeepDVC feature with configuration persistence, ImGui settings panel, and Streamline integration. The feature includes VR-specific post-tone-mapping shader hooks, texture processing via proxy buffers, and multiple safety checks for missing dependencies. DeepDVC is registered globally and conditionally available based on VR support and Streamline requirements. Changes
Sequence Diagram(s)sequenceDiagram
participant Game as Game Engine
participant DVC as DeepDVC Feature
participant SL as Streamline
participant D3D as D3D11 Device
rect rgb(100, 150, 200, 0.5)
Note over Game,D3D: DeepDVC Evaluate Flow
Game->>DVC: Evaluate() called
DVC->>DVC: Check prerequisites<br/>(VR, Streamline, D3D context)
alt Prerequisites met
DVC->>DVC: Create/update proxyBuffer UAV
DVC->>D3D: Copy source texture → proxyBuffer
DVC->>SL: Register scaling tags
DVC->>SL: slDeepDVCSetOptions(options)
DVC->>SL: slEvaluateFeature(kFeatureDeepDVC)
DVC->>D3D: Copy proxyBuffer → target texture
else Prerequisites missing
DVC->>DVC: Log warning, early exit
end
end
rect rgb(150, 100, 200, 0.5)
Note over Game,D3D: Shader Hook Installation
Game->>DVC: PostPostLoad() called
alt Running in VR
DVC->>DVC: Install vtable hooks<br/>on tone-mapping shaders
DVC->>DVC: Hooks call Evaluate()
DVC->>DVC: Log installation complete
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/DeepDVC.cpp`:
- Around line 35-38: DeepDVC::LoadSettings currently assigns settings directly;
after deserialization, validate and clamp fields: ensure the "mode" field is
bounded to [0, modes.size()-1] (if missing or invalid, set to a safe default
like 0), clamp all float parameters to their UI slider ranges (match the same
min/max used by the UI code), and sanitize any other enum/int fields to valid
values; if clamping fails or required keys are missing, restore sensible
defaults in settings and avoid passing an out-of-range mode into the evaluation
path or indexing modes[] so the renderer and evaluation functions use only
validated values.
- Around line 162-176: slEvaluateFeature may be missing or can fail, and in
those cases proxyBuffer is unprocessed and must not overwrite targetBuffer;
change the logic around upscaling.streamline.slEvaluateFeature / sl::Result res
to track success (e.g., bool evalSucceeded = false), set evalSucceeded = true
only when slEvaluateFeature is present and returns sl::Result::eOk, and then
only perform context->CopyResource(targetBuffer, proxyBuffer) when evalSucceeded
&& proxyBuffer && targetBuffer; reference
upscaling.streamline.slEvaluateFeature, sl::Result res, frameToken, inputsEval,
context, proxyBuffer and targetBuffer to locate and update the code.
- Around line 114-121: The proxy recreation check only compares
Width/Height/Format; update the validation in the block that calls
proxyBuffer->GetDesc(&proxyDesc) to compare all Texture2D shape fields
(ArraySize, MipLevels, SampleDesc.Count, SampleDesc.Quality in addition to
Width, Height, Format) against the current desc and release/reset proxyBuffer if
any differ so that CopyResource (used later) always has identical resource
shapes; reference the D3D11_TEXTURE2D_DESC from GetDesc, the local desc
variable, proxyBuffer, and the CopyResource call sites when making the change.
In `@src/Features/Upscaling/Streamline.cpp`:
- Around line 249-262: The DeepDVC feature handling must mirror the Reflex/PCL
pattern: replace direct slIsFeatureLoaded()/slGetFeatureRequirements() usage
with a call to checkFeatureAvailability(sl::kFeatureDeepDVC, featureDeepDVC,
adapterInfo) to validate loading and support, then bind function pointers via
bindFeatureFn(sl::kFeatureDeepDVC, /*target function pointers*/ ) and if
bindFeatureFn or checkFeatureAvailability returns failure clear featureDeepDVC
to false; specifically update the code paths around slIsFeatureLoaded,
slGetFeatureFunction and featureDeepDVC so that any failed availability check or
failed slGetFeatureFunction binding will set featureDeepDVC = false and log the
error consistently (use the existing checkFeatureAvailability and bindFeatureFn
helpers and the featureDeepDVC flag).
In `@src/Hooks.cpp`:
- Around line 234-242: The shared relocation "func" in
BSImagespaceShaderHDRTonemapBlendCinematic_Render means the same write_vfunc
hook type is being installed on two different vtables which causes one thunk to
call the wrong vanilla implementation and stomps the FrameAnnotations hooks; fix
this by creating two distinct hook types/thunks (e.g., separate structs for
Render and Fade) so each vtable slot has its own unique relocation instead of a
shared "func", and move or call globals::features::deepDVC.Evaluate() inside the
existing FrameAnnotations hook path (or chain the FrameAnnotations handler) so
DeepDVC logic is executed by that owner rather than installing duplicate
write_vfunc hooks.
🪄 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: a41947a3-f6f1-430e-8b07-f01b87ec9477
⛔ Files ignored due to path filters (2)
features/Upscaling/Shaders/Upscaling/Streamline/nvngx_deepdvc.dllis excluded by!**/*.dllfeatures/Upscaling/Shaders/Upscaling/Streamline/sl.deepdvc.dllis excluded by!**/*.dll
📒 Files selected for processing (9)
features/DeepDVC/Shaders/Features/DeepDVC.inisrc/Feature.cppsrc/Features/DeepDVC.cppsrc/Features/DeepDVC.hsrc/Features/Upscaling/Streamline.cppsrc/Features/Upscaling/Streamline.hsrc/Globals.cppsrc/Globals.hsrc/Hooks.cpp
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Tested in VR and enabled in AE. I'm seeing a watermark when using this to contact nvidia. NVIDIA-RTX/Streamline#107. We'll need to figure out the license issues before having this. AI comments should also be resolved (some may be invalid). Some initial thoughts.
- Should this be a separate feature or should we combine with Upscaling like Reflex? Part of that also turns on if you want to distribute separately or just wanted the feature available. Please note that Nexus does not allow CS mods to collect DP, if that is one of your goals. If DP is important, let us know and we can discuss with you further. It may be easiest to join us on Discord.
- The hook in hooks.cpp violates the feature boundary (which we're trying to enforce more). We may need to expose a central hook you can register to. This may be waived because other recent features are still doing it.
- It's coming with a separate ini file. All features should use the master config file for individual settings. The DeepDVC.ini is reserved for integrating with CS but isn't meant as a separate storage location.
- Not for you necessarily, but we need to use cmake to load the dependencies instead of copying raw dlls in to the repo.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Features/Upscaling/Streamline.cpp (1)
248-261:⚠️ Potential issue | 🟠 MajorHandle DeepDVC probe/bind failures the same way as Reflex/PCL.
Line 248 ignores the
slIsFeatureLoadedresult, and Lines 329-330 bindslDeepDVCSetOptionswithout clearing/checking the pointer. That can leavefeatureDeepDVCtrue whilesrc/Features/DeepDVC.cpp, Lines 18-20 still advertises the feature even though the DeepDVC entry point is unavailable. Please mirror the existingcheckFeatureAvailability/bindFeatureFnpath here and clearfeatureDeepDVCif either step fails.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".
Also applies to: 329-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling/Streamline.cpp` around lines 248 - 261, The DeepDVC probe/bind path must mirror the checkFeatureAvailability/bindFeatureFn flow: use the result of slIsFeatureLoaded and slIsFeatureSupported/slGetFeatureRequirements to decide availability, attempt to bind the entry point (slDeepDVCSetOptions) and if either the probe or bind step fails clear featureDeepDVC (set to false) and avoid calling the function pointer; update the Streamline.cpp logic around slIsFeatureLoaded/slIsFeatureSupported/slGetFeatureRequirements and the code that binds slDeepDVCSetOptions so it performs atomic check+bind like checkFeatureAvailability and bindFeatureFn, logs the failure reason (using magic_enum::enum_name(result) where applicable), and ensures DeepDVC.cpp does not advertise the feature when the entry point is unavailable.src/Features/DeepDVC.cpp (2)
111-117:⚠️ Potential issue | 🟠 MajorRecreate the proxy on every
Texture2Dshape change.Lines 114-117 only compare width, height, and format.
ID3D11DeviceContext::CopyResourcealso requires matching array size, mip count, and sample description. A reallocated target with the same dimensions but differentArraySize,MipLevels, or MSAA state will pass this check and then fail at Lines 139 and 172.Suggested change
- if (proxyDesc.Width != desc.Width || proxyDesc.Height != desc.Height || proxyDesc.Format != desc.Format) { + if (proxyDesc.Width != desc.Width || + proxyDesc.Height != desc.Height || + proxyDesc.Format != desc.Format || + proxyDesc.ArraySize != desc.ArraySize || + proxyDesc.MipLevels != desc.MipLevels || + proxyDesc.SampleDesc.Count != desc.SampleDesc.Count || + proxyDesc.SampleDesc.Quality != desc.SampleDesc.Quality) { proxyBuffer->Release(); proxyBuffer = nullptr; }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/DeepDVC.cpp` around lines 111 - 117, The proxy recreation check is incomplete: extend the comparison in the proxyBuffer handling so that before keeping the existing proxy you also compare proxyDesc.ArraySize, proxyDesc.MipLevels, and proxyDesc.SampleDesc.Count/Quality against desc.ArraySize, desc.MipLevels, and desc.SampleDesc to detect MSAA changes; if any of those differ, Release() and null out proxyBuffer (same as existing pattern). Update the check around proxyBuffer->GetDesc(...) (references: proxyBuffer, proxyDesc, desc) to include these additional fields so CopyResource will not be attempted with mismatched resource shapes.
32-35:⚠️ Potential issue | 🟠 MajorClamp deserialized settings before using them.
Line 34 trusts the persisted JSON as-is, but Line 46 indexes
modes[settings.mode]and Lines 148-150 forward those values into Streamline. A malformed config can therefore read pastmodes[]or send invalid DeepDVC parameters. Please clampmodeto[0, 1]and clamp the float settings to the same[0.0f, 1.0f]range exposed by the UI.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/DeepDVC.cpp` around lines 32 - 35, DeepDVC::LoadSettings currently assigns settings = o_json without validation; after that assignment clamp settings.mode to the integer range [0,1] (set to 0 if out of range) and clamp any float parameters that get forwarded to Streamline to the [0.0f,1.0f] range (e.g., the specific float fields used later around the Streamline call at lines ~148-150); implement clamping in LoadSettings (use std::clamp or explicit checks) so modes[settings.mode] cannot be indexed out-of-bounds and Streamline always receives normalized floats, and ensure the code falls back to a safe default if values are invalid.
🤖 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/DeepDVC.cpp`:
- Around line 82-88: DeepDVC::Evaluate() currently reads
upscaling.streamline.frameToken (and then uses frameToken) without first
refreshing it; call EnsureFrameToken() at the start of DeepDVC::Evaluate()
before accessing upscaling.streamline.frameToken (or using local frameToken) so
the cached token is up-to-date even when DLSS/Reflex are disabled; ensure the
call happens before the lines that read context/device/frameToken and before the
early return that checks frameToken.
---
Duplicate comments:
In `@src/Features/DeepDVC.cpp`:
- Around line 111-117: The proxy recreation check is incomplete: extend the
comparison in the proxyBuffer handling so that before keeping the existing proxy
you also compare proxyDesc.ArraySize, proxyDesc.MipLevels, and
proxyDesc.SampleDesc.Count/Quality against desc.ArraySize, desc.MipLevels, and
desc.SampleDesc to detect MSAA changes; if any of those differ, Release() and
null out proxyBuffer (same as existing pattern). Update the check around
proxyBuffer->GetDesc(...) (references: proxyBuffer, proxyDesc, desc) to include
these additional fields so CopyResource will not be attempted with mismatched
resource shapes.
- Around line 32-35: DeepDVC::LoadSettings currently assigns settings = o_json
without validation; after that assignment clamp settings.mode to the integer
range [0,1] (set to 0 if out of range) and clamp any float parameters that get
forwarded to Streamline to the [0.0f,1.0f] range (e.g., the specific float
fields used later around the Streamline call at lines ~148-150); implement
clamping in LoadSettings (use std::clamp or explicit checks) so
modes[settings.mode] cannot be indexed out-of-bounds and Streamline always
receives normalized floats, and ensure the code falls back to a safe default if
values are invalid.
In `@src/Features/Upscaling/Streamline.cpp`:
- Around line 248-261: The DeepDVC probe/bind path must mirror the
checkFeatureAvailability/bindFeatureFn flow: use the result of slIsFeatureLoaded
and slIsFeatureSupported/slGetFeatureRequirements to decide availability,
attempt to bind the entry point (slDeepDVCSetOptions) and if either the probe or
bind step fails clear featureDeepDVC (set to false) and avoid calling the
function pointer; update the Streamline.cpp logic around
slIsFeatureLoaded/slIsFeatureSupported/slGetFeatureRequirements and the code
that binds slDeepDVCSetOptions so it performs atomic check+bind like
checkFeatureAvailability and bindFeatureFn, logs the failure reason (using
magic_enum::enum_name(result) where applicable), and ensures DeepDVC.cpp does
not advertise the feature when the entry point is unavailable.
🪄 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: d49b8c12-cca6-4e73-9856-bcd13ba85188
📒 Files selected for processing (3)
src/Features/DeepDVC.cppsrc/Features/Upscaling/Streamline.cppsrc/Hooks.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Hooks.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/Features/DeepDVC.cpp (2)
123-130:⚠️ Potential issue | 🟠 MajorProxy descriptor check is still incomplete — CopyResource can fail.
The comparison at line 126 only covers
Width/Height/Format.ID3D11DeviceContext::CopyResourcerequires the full texture shape to match, so iftargetBufferis reallocated with a differentArraySize,MipLevels, orSampleDesc, the staleproxyBufferwill be kept and bothCopyResourcecalls (lines 151 and 184) will silently fail (or emit D3D errors).🩹 Suggested fix
- if (proxyDesc.Width != desc.Width || proxyDesc.Height != desc.Height || proxyDesc.Format != desc.Format) { + if (proxyDesc.Width != desc.Width || + proxyDesc.Height != desc.Height || + proxyDesc.Format != desc.Format || + proxyDesc.ArraySize != desc.ArraySize || + proxyDesc.MipLevels != desc.MipLevels || + proxyDesc.SampleDesc.Count != desc.SampleDesc.Count || + proxyDesc.SampleDesc.Quality != desc.SampleDesc.Quality) { proxyBuffer->Release(); proxyBuffer = nullptr; }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/DeepDVC.cpp` around lines 123 - 130, The proxy texture validation is incomplete: extend the descriptor comparison in the proxyBuffer check to include ArraySize, MipLevels and the SampleDesc (at least Count and Quality) in addition to Width/Height/Format so the proxy is released when any of those differ; update the logic around proxyDesc and desc (the symbols proxyBuffer, proxyDesc, desc) so mismatched ArraySize/MipLevels/SampleDesc fields cause proxyBuffer->Release() and proxyBuffer = nullptr to avoid CopyResource failures (also ensure later CopyResource calls on targetBuffer/proxyBuffer are guarded by a non-null proxyBuffer).
171-185:⚠️ Potential issue | 🟠 MajorDon't copy proxyBuffer back to target when evaluation didn't succeed.
If
slEvaluateFeatureis missing (null function pointer) or returns non-eOk,proxyBufferstill holds the pre-evaluation copy and is unconditionally blitted overtargetBufferat lines 183-184. That's a no-op on the pixels but it's still a guaranteed GPU copy per frame, and more importantly it masks eval failures: any future change that might leaveproxyBufferin an undefined state (e.g. partial writes, transient DLL reload) would corrupt the final image. Gate the copy on a success flag.🩹 Suggested fix
+ bool evalOk = false; const sl::BaseStructure* inputsEval[] = { &viewport }; if (upscaling.streamline.slEvaluateFeature) { sl::Result res = upscaling.streamline.slEvaluateFeature(sl::kFeatureDeepDVC, *frameToken, inputsEval, _countof(inputsEval), context); if (res != sl::Result::eOk) { static int failCount = 0; if (failCount < 100) { logger::warn("[DeepDVC] slEvaluateFeature failed: {}", magic_enum::enum_name(res)); failCount++; } + } else { + evalOk = true; } } - if (proxyBuffer && targetBuffer) { + if (evalOk && proxyBuffer && targetBuffer) { context->CopyResource(targetBuffer, proxyBuffer); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/DeepDVC.cpp` around lines 171 - 185, The code unconditionally copies proxyBuffer to targetBuffer even when slEvaluateFeature is null or returns non-eOk; change DeepDVC's logic to track success (e.g., a bool like evalSucceeded) when upscaling.streamline.slEvaluateFeature is present and slEvaluateFeature(sl::kFeatureDeepDVC, *frameToken, inputsEval, _countof(inputsEval), context) returns sl::Result::eOk, and only perform context->CopyResource(targetBuffer, proxyBuffer) when evalSucceeded is true and both proxyBuffer and targetBuffer are valid; refer to the slEvaluateFeature call, sl::kFeatureDeepDVC, frameToken, inputsEval, context, proxyBuffer, and targetBuffer to locate where to add the flag and gate the copy.
🧹 Nitpick comments (1)
src/Features/Upscaling/Streamline.cpp (1)
116-120: Remove (or wire up) the unusedfeaturesToLoadVRarray.
featuresToLoadVRis declared at line 117 but never referenced —pref.featuresToLoadis still set fromfeaturesToLoadon line 119, andnumFeaturesToLoaduses_countof(featuresToLoad). Since the two arrays are currently identical, this just adds dead code. Either drop it, or select the appropriate array at runtime based onglobals::game::isVR.♻️ Proposed fix (drop the unused array)
sl::Feature featuresToLoad[] = { sl::kFeatureDLSS, sl::kFeatureReflex, sl::kFeaturePCL, sl::kFeatureDeepDVC }; - sl::Feature featuresToLoadVR[] = { sl::kFeatureDLSS, sl::kFeatureReflex, sl::kFeaturePCL, sl::kFeatureDeepDVC };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling/Streamline.cpp` around lines 116 - 120, The extra array featuresToLoadVR is declared but never used; either remove it or wire it up based on runtime VR state. Fix by selecting which array to assign to pref.featuresToLoad and pref.numFeaturesToLoad: use featuresToLoad when !globals::game::isVR and featuresToLoadVR when globals::game::isVR (update pref.featuresToLoad and pref.numFeaturesToLoad accordingly), or simply delete the unused featuresToLoadVR declaration if VR-specific branching is not needed. Ensure you reference the existing symbols featuresToLoad, featuresToLoadVR, pref.featuresToLoad, pref.numFeaturesToLoad, and globals::game::isVR when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Features/DeepDVC.cpp`:
- Around line 123-130: The proxy texture validation is incomplete: extend the
descriptor comparison in the proxyBuffer check to include ArraySize, MipLevels
and the SampleDesc (at least Count and Quality) in addition to
Width/Height/Format so the proxy is released when any of those differ; update
the logic around proxyDesc and desc (the symbols proxyBuffer, proxyDesc, desc)
so mismatched ArraySize/MipLevels/SampleDesc fields cause proxyBuffer->Release()
and proxyBuffer = nullptr to avoid CopyResource failures (also ensure later
CopyResource calls on targetBuffer/proxyBuffer are guarded by a non-null
proxyBuffer).
- Around line 171-185: The code unconditionally copies proxyBuffer to
targetBuffer even when slEvaluateFeature is null or returns non-eOk; change
DeepDVC's logic to track success (e.g., a bool like evalSucceeded) when
upscaling.streamline.slEvaluateFeature is present and
slEvaluateFeature(sl::kFeatureDeepDVC, *frameToken, inputsEval,
_countof(inputsEval), context) returns sl::Result::eOk, and only perform
context->CopyResource(targetBuffer, proxyBuffer) when evalSucceeded is true and
both proxyBuffer and targetBuffer are valid; refer to the slEvaluateFeature
call, sl::kFeatureDeepDVC, frameToken, inputsEval, context, proxyBuffer, and
targetBuffer to locate where to add the flag and gate the copy.
---
Nitpick comments:
In `@src/Features/Upscaling/Streamline.cpp`:
- Around line 116-120: The extra array featuresToLoadVR is declared but never
used; either remove it or wire it up based on runtime VR state. Fix by selecting
which array to assign to pref.featuresToLoad and pref.numFeaturesToLoad: use
featuresToLoad when !globals::game::isVR and featuresToLoadVR when
globals::game::isVR (update pref.featuresToLoad and pref.numFeaturesToLoad
accordingly), or simply delete the unused featuresToLoadVR declaration if
VR-specific branching is not needed. Ensure you reference the existing symbols
featuresToLoad, featuresToLoadVR, pref.featuresToLoad, pref.numFeaturesToLoad,
and globals::game::isVR when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26118c8a-195c-448e-b569-5783e530c99a
📒 Files selected for processing (5)
features/DeepDVC/Shaders/Features/DeepDVC.inisrc/Features/DeepDVC.cppsrc/Features/Upscaling/Streamline.cppsrc/FrameAnnotations.cppsrc/Hooks.cpp
✅ Files skipped from review due to trivial changes (2)
- features/DeepDVC/Shaders/Features/DeepDVC.ini
- src/Hooks.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/Features/DeepDVC.cpp (2)
171-185:⚠️ Potential issue | 🟠 MajorOnly copy back after a successful DeepDVC evaluation.
If
slEvaluateFeatureis missing or returns a non-OK result, Line 184 still overwritestargetBufferwithproxyBuffer. Track evaluation success and copy back only when Streamline actually processed the resource.Suggested change
+ bool evalSucceeded = false; const sl::BaseStructure* inputsEval[] = { &viewport }; if (upscaling.streamline.slEvaluateFeature) { sl::Result res = upscaling.streamline.slEvaluateFeature(sl::kFeatureDeepDVC, *frameToken, inputsEval, _countof(inputsEval), context); if (res != sl::Result::eOk) { static int failCount = 0; if (failCount < 100) { logger::warn("[DeepDVC] slEvaluateFeature failed: {}", magic_enum::enum_name(res)); failCount++; } + } else { + evalSucceeded = true; } } - if (proxyBuffer && targetBuffer) { + if (evalSucceeded && proxyBuffer && targetBuffer) { context->CopyResource(targetBuffer, proxyBuffer); }To verify the current failure path:
#!/bin/bash # Inspect whether final target copy is gated by slEvaluateFeature success. sed -n '159,186p' src/Features/DeepDVC.cppAs 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/DeepDVC.cpp` around lines 171 - 185, The code always copies proxyBuffer to targetBuffer even when Streamline evaluation fails or slEvaluateFeature is missing; modify the logic to track success of slEvaluateFeature (check that upscaling.streamline.slEvaluateFeature exists and that sl::Result res == sl::Result::eOk when calling upscaling.streamline.slEvaluateFeature(sl::kFeatureDeepDVC, *frameToken, inputsEval, _countof(inputsEval), context)) and only call context->CopyResource(targetBuffer, proxyBuffer) when that success flag is true and both proxyBuffer and targetBuffer are valid; ensure the flag is initialized false, set true on success, and used to gate the CopyResource call.
123-130:⚠️ Potential issue | 🟠 MajorCompare the full
Texture2Dshape before reusingproxyBuffer.Line 126 still only checks width, height, and format. If
ArraySize,MipLevels, orSampleDescchanges, the laterCopyResourcecalls can fail because the cached proxy no longer matches the target resource shape.Suggested change
- if (proxyDesc.Width != desc.Width || proxyDesc.Height != desc.Height || proxyDesc.Format != desc.Format) { + if (proxyDesc.Width != desc.Width || + proxyDesc.Height != desc.Height || + proxyDesc.Format != desc.Format || + proxyDesc.ArraySize != desc.ArraySize || + proxyDesc.MipLevels != desc.MipLevels || + proxyDesc.SampleDesc.Count != desc.SampleDesc.Count || + proxyDesc.SampleDesc.Quality != desc.SampleDesc.Quality) { proxyBuffer->Release(); proxyBuffer = nullptr; }To verify the current state:
#!/bin/bash # Confirm proxyBuffer shape validation and CopyResource call sites. sed -n '116,185p' src/Features/DeepDVC.cppAs 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/DeepDVC.cpp` around lines 123 - 130, The proxyBuffer reuse check only compares Width, Height, and Format but must validate the entire D3D11_TEXTURE2D_DESC to avoid CopyResource failures; update the validation inside the proxyBuffer block to compare ArraySize, MipLevels and SampleDesc (Count and Quality) in addition to Width/Height/Format, and also ensure any BindFlags or MiscFlags that affect compatibility are considered before retaining proxyBuffer (functions/variables: proxyBuffer, D3D11_TEXTURE2D_DESC, desc, CopyResource); if any field differs, Release() and null out proxyBuffer as already done.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Features/DeepDVC.cpp`:
- Around line 171-185: The code always copies proxyBuffer to targetBuffer even
when Streamline evaluation fails or slEvaluateFeature is missing; modify the
logic to track success of slEvaluateFeature (check that
upscaling.streamline.slEvaluateFeature exists and that sl::Result res ==
sl::Result::eOk when calling
upscaling.streamline.slEvaluateFeature(sl::kFeatureDeepDVC, *frameToken,
inputsEval, _countof(inputsEval), context)) and only call
context->CopyResource(targetBuffer, proxyBuffer) when that success flag is true
and both proxyBuffer and targetBuffer are valid; ensure the flag is initialized
false, set true on success, and used to gate the CopyResource call.
- Around line 123-130: The proxyBuffer reuse check only compares Width, Height,
and Format but must validate the entire D3D11_TEXTURE2D_DESC to avoid
CopyResource failures; update the validation inside the proxyBuffer block to
compare ArraySize, MipLevels and SampleDesc (Count and Quality) in addition to
Width/Height/Format, and also ensure any BindFlags or MiscFlags that affect
compatibility are considered before retaining proxyBuffer (functions/variables:
proxyBuffer, D3D11_TEXTURE2D_DESC, desc, CopyResource); if any field differs,
Release() and null out proxyBuffer as already done.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 64180157-84c1-4a0f-843c-2650bcff4495
📒 Files selected for processing (5)
src/Feature.cppsrc/Features/DeepDVC.cppsrc/Features/Upscaling/Streamline.hsrc/FrameAnnotations.cppsrc/Globals.cpp
✅ Files skipped from review due to trivial changes (1)
- src/Globals.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Feature.cpp
- src/Features/Upscaling/Streamline.h
- src/FrameAnnotations.cpp
Thanks for the review! All feedback has been addressed in the latest push.
|
Ok we can leave it separate for now.
Apologies Frame Annotations is also not the best place. Since hooks should chain, can you see if you can keep the hook in DeepDVC.cpp? If our existing lifecycle events don't work, let me know. Please also check the stray edit in hooks.cpp. We should make sure we don't have random churn in the files. Lastly, we enter into a RC cycle for the last half of April. We won't be merging new feats until May and will be focusing on perfs and fixes (and other non feats). Thanks for the patience. I know you have other feats pending, so if you want to work on them, we'll continue to review. If you have any interest on contributing during the RC period, we'd appreciate any help you feel comfortable in giving including reviewing other PRs. |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/Features/Upscaling/Streamline.cpp (1)
319-331:⚠️ Potential issue | 🟡 MinorDisable DeepDVC if the Streamline binding entry point is unavailable.
When
featureDeepDVCis true butslGetFeatureFunctionis null, this block silently skips binding and leaves DeepDVC advertised as supported. Clear the flag in that branch so the feature degrades consistently.Suggested change
slDeepDVCSetOptions = nullptr; if (featureDeepDVC && slGetFeatureFunction) { auto fn = (void*)nullptr; const sl::Result bindResult = slGetFeatureFunction(sl::kFeatureDeepDVC, "slDeepDVCSetOptions", fn); if (bindResult == sl::Result::eOk && fn) { slDeepDVCSetOptions = reinterpret_cast<decltype(slDeepDVCSetOptions)>(fn); logger::info("[Streamline] DeepDVC runtime controls are available"); } else { logger::warn("[Streamline] DeepDVC options bind failed with {}; DeepDVC will be disabled", magic_enum::enum_name(bindResult)); featureDeepDVC = false; } + } else if (featureDeepDVC) { + logger::warn("[Streamline] slGetFeatureFunction is unavailable; DeepDVC will be disabled"); + featureDeepDVC = false; }Please verify with the Streamline package/version used for distribution that missing feature-function binding hides DeepDVC from the UI and skips runtime evaluation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling/Streamline.cpp` around lines 319 - 331, The code leaves DeepDVC advertised when featureDeepDVC is true but slGetFeatureFunction is null; update the branch where slGetFeatureFunction is false to clear featureDeepDVC and log a warning so the feature is disabled consistently — specifically modify the block around slDeepDVCSetOptions / slGetFeatureFunction to set featureDeepDVC = false (and add a logger::warn mentioning slGetFeatureFunction unavailable) when slGetFeatureFunction is null so DeepDVC is hidden from the UI and not evaluated at runtime.
🧹 Nitpick comments (1)
src/Features/DeepDVC.h (1)
44-54: Prefer RAII for the owned D3D texture.
proxyBufferis an owning COM pointer with manualRelease()paths in both the destructor and recreation logic. Consider using the project’s existing COM smart-pointer convention to make reset/destruction exception-safe and copy-safe.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/DeepDVC.h` around lines 44 - 54, The destructor/manual Release on the raw ID3D11Texture2D* proxyBuffer should be replaced with the project’s COM smart-pointer convention (e.g., ComPtr/ScopedComPtr/WRL::ComPtr) to ensure RAII, exception-safety and correct copy/move semantics; change the member declaration from "ID3D11Texture2D* proxyBuffer" to the smart-pointer type, remove manual Release() from ~DeepDVC() and any recreation code, and use the smart pointer’s Reset/Release/Attach methods when replacing the texture; also include the appropriate smart-pointer header and ensure DeepDVC’s copy/move constructors/operators follow the project convention (delete or default them) so resource ownership is well-defined.
🤖 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/DeepDVC.cpp`:
- Line 52: The ImGui usage casts uint32_t to int* unsafely (e.g.,
ImGui::SliderInt("Mode", (int*)&settings.mode)) — replace this pattern by using
a local int temp variable for the slider, call ImGui::SliderInt with &temp,
clamp/validate temp, then assign back to settings.mode (or change settings.mode
to int if you intentionally want signed storage) to avoid unsafe pointer casts;
apply the same fix consistently for settings.mode usage in DeepDVC.cpp and the
analogous patterns in Upscaling, ScreenSpaceGI, SubsurfaceScattering,
ScreenSpaceShadows, and WetnessEffects, or alternatively document/suppress the
pattern if that is the agreed project-wide approach.
- Around line 156-180: Add upfront null checks for the Streamline entry points
and fail-fast when options cannot be applied: ensure
upscaling.streamline.slSetTag is non-null before calling slSetTag(viewport,
inputs, _countof(inputs), context); check
upscaling.streamline.slDeepDVCSetOptions before creating and setting options and
if slDeepDVCSetOptions exists but returns a non-eOk result, do not proceed to
evaluation (return or skip); finally guard
upscaling.streamline.slEvaluateFeature before calling
slEvaluateFeature(sl::kFeatureDeepDVC, *frameToken, inputsEval,
_countof(inputsEval), context). Reference symbols: slSetTag,
slDeepDVCSetOptions, slEvaluateFeature, viewport, inputs, context, options,
upscaling.streamline, frameToken.
In `@src/Features/DeepDVC.h`:
- Line 30: The DeepDVC feature is currently enabled by default via the variable
"mode" in DeepDVC.h; change its default from 1 to 0 so the post-process is off
until users opt in. Locate the "uint32_t mode" declaration in the DeepDVC.h
header and set the initializer to 0 (ensure any comments or documentation in the
file reflect the new default), and run a quick build/test to verify no other
code assumes mode defaults to 1.
In `@src/Features/Upscaling/Streamline.cpp`:
- Around line 116-120: The code currently always assigns the non-VR array to
pref.featuresToLoad causing SE/AE to request kFeatureDeepDVC even though it's
VR-only; update the logic to choose between featuresToLoad and featuresToLoadVR
at runtime (e.g., detect VR variant via the existing runtime detection helper
used in this module or using REL::RelocateMember() pattern if variant-specific
members are used) and set pref.featuresToLoad and pref.numFeaturesToLoad from
the chosen array so that kFeatureDeepDVC is only included when running the VR
build; reference the arrays featuresToLoad and featuresToLoadVR and the fields
pref.featuresToLoad / pref.numFeaturesToLoad and ensure the runtime branch uses
REL::RelocateMember() where required for compatibility across SE/AE/VR.
---
Duplicate comments:
In `@src/Features/Upscaling/Streamline.cpp`:
- Around line 319-331: The code leaves DeepDVC advertised when featureDeepDVC is
true but slGetFeatureFunction is null; update the branch where
slGetFeatureFunction is false to clear featureDeepDVC and log a warning so the
feature is disabled consistently — specifically modify the block around
slDeepDVCSetOptions / slGetFeatureFunction to set featureDeepDVC = false (and
add a logger::warn mentioning slGetFeatureFunction unavailable) when
slGetFeatureFunction is null so DeepDVC is hidden from the UI and not evaluated
at runtime.
---
Nitpick comments:
In `@src/Features/DeepDVC.h`:
- Around line 44-54: The destructor/manual Release on the raw ID3D11Texture2D*
proxyBuffer should be replaced with the project’s COM smart-pointer convention
(e.g., ComPtr/ScopedComPtr/WRL::ComPtr) to ensure RAII, exception-safety and
correct copy/move semantics; change the member declaration from
"ID3D11Texture2D* proxyBuffer" to the smart-pointer type, remove manual
Release() from ~DeepDVC() and any recreation code, and use the smart pointer’s
Reset/Release/Attach methods when replacing the texture; also include the
appropriate smart-pointer header and ensure DeepDVC’s copy/move
constructors/operators follow the project convention (delete or default them) so
resource ownership is well-defined.
🪄 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: 39605255-f4dc-4890-8621-adbb287cc7c5
⛔ Files ignored due to path filters (2)
features/Upscaling/Shaders/Upscaling/Streamline/nvngx_deepdvc.dllis excluded by!**/*.dllfeatures/Upscaling/Shaders/Upscaling/Streamline/sl.deepdvc.dllis excluded by!**/*.dll
📒 Files selected for processing (8)
features/DeepDVC/Shaders/Features/DeepDVC.inisrc/Feature.cppsrc/Features/DeepDVC.cppsrc/Features/DeepDVC.hsrc/Features/Upscaling/Streamline.cppsrc/Features/Upscaling/Streamline.hsrc/Globals.cppsrc/Globals.h
✅ Files skipped from review due to trivial changes (4)
- src/Globals.cpp
- src/Feature.cpp
- src/Globals.h
- features/DeepDVC/Shaders/Features/DeepDVC.ini
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/Upscaling/Streamline.h
There was a problem hiding this comment.
when i tried this is just made things look washed out. maybe there needs to be a contrast slider or something?
Also it brightens up things that are already bright, overexposing them.
Please try looking at my local tonemapping code here instead https://github.com/doodlum/skyrim-community-shaders/tree/ltm-nonsense
|
Thanks for the feedback. |
DVC Technical Background DVC is a complete black box — no papers or mechanism documentation found, only that it's a deep neural network accelerated by Tensor Cores. NVIDIA's programming guide recommends running it after tonemapping, before film grain, at full resolution (post-SR), accepting only SDR input. DVC exposes only two parameters: intensity and saturation boost. Comparing input vs output, the intensity parameter likely controls multiple dimensions internally — brightness, chrominance, contrast, grey levels, etc. Unlike simply adjusting post-processing sliders, DVC operates at a much finer granularity — it doesn't do uniform linear processing as the exposed parameters might suggest. It likely uses attention mechanisms and ViT-based strategies, applying different processing strengths to different regions of different scenes. This makes it better at avoiding localized overexposure and detail loss compared to traditional approaches. However, DVC can only avoid over-processing when performing its own enhancement — it cannot suppress over-processing introduced by other sources. This AI filter works correctly in The Witcher and several other titles. I've tested it on vanilla Skyrim and earlier CS builds with no issues. Back in January I used it on the jiayev fork to enhance some PP presets from the channel — for example, certain cinematic presets have terrible shadow detail in specific scenes/weather conditions, and DVC significantly improves them. In bright scenes the effect is subtle; in dark caves it noticeably improves visibility and makes the image more vivid. LTM Testing I don't have an SE/AE client, so I tested the LTM branch in VR. After disabling Sky_UpdateColors and Sky_SetDirectionalAmbientColors (which use non-VR addresses), ENB post-processing had incorrect transparency handling scope; with my setup, enabling useeffect darkened the entire image, and all sub-effect toggles and sliders had no effect whatsoever. So I couldn't evaluate LTM's actual results under normal working conditions. Regarding the "washed out" issue: ruling out other factors, it's most likely double lifting — both LTM and DVC boost shadow detail, so dark areas get lifted twice. Meanwhile, after LTM processing, DVC's further enhancement pushes highlights beyond the ideal range of the ACES Filmic curve, causing overexposure. This problem likely can't be solved from the DVC side. An Interesting Finding Some users have reported that CS's DLSS performance is worse than PureDark's. I experimented with moving DLSS execution to after post-processing to save performance (currently DLSSEnhancer uses a rather convoluted method to process post-processing at render resolution before compositing to display resolution to avoid DLSS artifacts — simply moving DLSS after PP like PureDark does would be cleaner and simpler). On vanilla Skyrim, even with SL exposure/HDR flags set correctly, the Quality preset causes highlights to become blurry with slight color shifts. But if DVC runs after post-processing and before DLSS, it saves performance while producing a more visually comfortable result. My guess is that DLSS's training data is based on HDR input, and DVC happens to adjust the LDR output into a form closer to what DLSS expects. Could this approach be worth trying in CS? Would it conflict with any existing development plans? |
An New Finding Some users have reported that CS's DLSS performance is worse than PureDark's. I experimented with moving DLSS execution to after post-processing to save performance (currently DLSSEnhancer uses a rather convoluted method to process post-processing at render resolution before compositing to display resolution to avoid DLSS artifacts — simply moving DLSS after PP like PureDark does would be cleaner and simpler). On vanilla Skyrim, even with SL exposure/HDR flags set correctly, the Quality preset causes highlights to become blurry with slight color shifts. But if DVC runs after post-processing and before DLSS, it saves performance while producing a more visually comfortable result. My guess is that DLSS's training data is based on HDR input, and DVC happens to adjust the LDR output into a form closer to what DLSS expects. Could this approach be worth trying in CS? Would it conflict with any existing development plans? And Is there a Discord channel or thread for VR dev discussion? I'm often busy with work and don't know where to start with the flood of messages. I appreciate what you and VRnord do for VR — where do you usually chat? (doodlum seems to be everywhere XD, but I rarely see you two.) |
|
ltm needs to be ported to be separate to enb system, so it modifies vanilla tonemapping system. moving dlss to after post processing is acceptable for vr. for se/ae it needs to stay as is. we talk on the official discord server for cs. |
|
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Please note nvidia says we need to talk to the developer relations rep before we include this. Please handle. We can't merge until that's resolved. If they can't provide it to an open source project, then we can't use the dll.
@doodlum fyi.
NVIDIA Deep Learning Dynamic Vibrance Control (DeepDVC) via Streamline SDK.
AI-based post-process that enhances color vibrance and dark-area detail while preventing overexposure.
work after tone mapping.
VR-only for now; flat-screen adaptation is straightforward.
Summary by CodeRabbit
New Features