feat: d3d12 interoperability#2430
Open
Gistix wants to merge 14 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughCentralizes D3D11↔D3D12 interop into a DX12Interop singleton, adds WrappedResource and InteropContext, and rewires Upscaling, DX12SwapChain, FidelityFX, and global initialization to use shared resources, D3D12 device/queue/fence state, and coordinated Execute-based execution. ChangesDX12 Interop Centralization
Sequence Diagram(s)sequenceDiagram
participant SwapChain as DX12SwapChain::Present
participant DXI as DX12Interop::Execute
participant D11 as D3D11_Device/Context
participant D12 as D3D12_Device/Queue
participant FFX as FidelityFX
SwapChain->>DXI: Execute(lambdaCopyAndDispatch, continuationPresent)
DXI->>DXI: Select frameContext by frame%kMaxFramesInFlight
DXI->>D12: Wait on previous D3D12 fence
DXI->>D12: Reset allocator & commandList
DXI->>D11: Signal D3D11 fence (DX11→DX12 handoff)
DXI->>D12: Wait on D3D12 fence
DXI->>SwapChain: Invoke lambda(commandList)
SwapChain->>D12: Record barriers & CopyResource to sharedResources
SwapChain->>FFX: fidelityFX.Present(commandList, ...)
FFX->>D12: Frame-gen dispatch (if enabled)
DXI->>D12: Signal D3D12 fence
DXI->>D11: Wait on D3D11 fence (DX12→DX11 handoff)
DXI->>SwapChain: Invoke continuation (swapChain->Present)
SwapChain->>D11: Clear UI RTV via dx12Interop->d3d11Context
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.22.0)OpenGrep fatal error (exit code 2): [00.15][ERROR]: Error: exception Unix_error: No such file or directory stat src/DX12Interop.cpp 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Features/DX12Interop.cpp`:
- Around line 203-230: DX12Interop::SetupResources currently overwrites raw
pointers in sharedResources (depth, motionVector, main, reactiveMask) with new
WrappedResource(...) without freeing prior allocations; update the method to
release existing resources before reallocation by checking each pointer
(sharedResources.depth, sharedResources.motionVector, sharedResources.main,
sharedResources.reactiveMask), deleting or calling their Release/Destroy method
as appropriate and setting them to nullptr, then allocate the new
WrappedResource and assign it; ensure allocation failures are handled gracefully
(log error and leave prior pointer null) and use the WrappedResource constructor
symbols to locate where to insert the cleanup.
- Around line 63-86: GetLatestWinPixGpuCapturerPath currently assumes
FOLDERID_ProgramFiles exists and that "Microsoft PIX" is present, which lets
std::filesystem::directory_iterator throw and leaves a TODO; update the function
to validate SHGetKnownFolderPath's HRESULT and programFilesPath, call
CoTaskMemFree(programFilesPath) after use, check
std::filesystem::exists(pixInstallationPath) && is_directory before iterating,
wrap the directory iteration in a try/catch for
std::filesystem::filesystem_error to avoid exceptions escaping, and if no PIX
installation or any error occurs return an empty std::wstring (instead of
leaving a TODO) so callers can degrade gracefully; refer to
GetLatestWinPixGpuCapturerPath, pixInstallationPath, newestVersionFound and the
returned WinPixGpuCapturer.dll path when making changes.
- Around line 217-220: The code mistakenly sets mainDesc.BindFlags instead of
the texture's descriptor, preventing UAV creation for motion vectors; change the
assignment to modify texDesc.BindFlags (i.e., set texDesc.BindFlags |=
D3D11_BIND_UNORDERED_ACCESS) before constructing sharedResources.motionVector so
the WrappedResource is created with the correct UAV-capable descriptor for
motionVector.
- Around line 117-132: The Init method currently sets the member flag active =
true before calling InitializePIX(), CreateD3D12Device(), and CreateInterop(),
which can leave the object marked active if those calls fail; move the
assignment of active = true to after all initialization steps succeed (i.e.,
after InitializePIX(), CreateD3D12Device(a_adapter), and CreateInterop()
complete), and ensure any failures from those functions cause Init to return
early without setting active; use the existing D3D12Mode() guard and preserve
the calls to SetD3D11Device and SetD3D11DeviceContext as-is.
In `@src/Features/DX12Interop.h`:
- Around line 84-131: The Execute method currently dereferences interop
resources (d3d11Context, d3d11Fence, d3d12Fence, commandQueue and frameContexts
entries) without verifying initialization; add a defensive guard at the top of
Execute that verifies the interop is initialized (e.g., a boolean like
initialized_ or by checking that d3d11Context, d3d11Fence, d3d12Fence and
commandQueue are non-null and frameContexts has expected size) and if not
initialized either return early or throw a clear exception; make sure to
reference the same symbols (Execute, d3d11Context, d3d11Fence, d3d12Fence,
commandQueue, frameContexts, GetFrameContextIndex) so callers and future
reviewers can see why the check is required and avoid dereferencing invalid
pointers.
In `@src/Features/DX12Interop/WrappedResource.cpp`:
- Around line 58-63: The delegating constructor
WrappedResource::WrappedResource(D3D11_TEXTURE2D_DESC) uses
globals::features::dx12Interop.d3d11Device.get() and d3d12Device.get() directly
and will crash if those globals are null; replace the delegating ctor with a
non-delegating implementation that first checks
globals::features::dx12Interop.d3d11Device and
globals::features::dx12Interop.d3d12Device (call .get() and validate non-null),
and if valid forward to the existing initialization routine (either call the
other constructor logic via a shared InitFromDevices(...) helper or throw/return
an error (e.g., throw std::runtime_error) when they are null); update
WrappedResource::WrappedResource(D3D11_TEXTURE2D_DESC, ...) usage accordingly so
the null-check happens before any device dereference.
In `@src/Features/DX12Interop/WrappedResource.h`:
- Around line 8-20: WrappedResource currently allows implicit copies which can
duplicate ownership of raw COM pointers (resource11, srv, uav, rtv, resource)
and cause double Release(); make the class non-copyable by deleting the copy
constructor and copy assignment operator (declare WrappedResource(const
WrappedResource&) = delete; and WrappedResource& operator=(const
WrappedResource&) = delete;) and optionally provide/move-enable a move
constructor and move assignment (WrappedResource(WrappedResource&&) noexcept and
WrappedResource& operator=(WrappedResource&&) noexcept) that transfer pointer
ownership and null out the source members; update any call sites that rely on
copying to use moves or explicit ownership semantics.
In `@src/Features/Upscaling.cpp`:
- Around line 128-132: The proxy path must only run when DX12 interop is
actually ready: wrap the upscaling.CreateProxySwapChain(...) and
upscaling.CreateProxyInterop() calls in the same readiness check used for
dx12Interop.Init (i.e., require dx12Interop.loaded and validate
dx12Interop.d3d12Device, dx12Interop.commandQueue and dx12Interop.d3d11Context
are non-null/valid) so you avoid dereferencing null D3D12/D3D11 pointers; if
those checks fail, skip the proxy calls and fall back to the non-proxy code
path. Ensure you reference
dx12Interop.loaded/d3d12Device/commandQueue/d3d11Context and the
upscaling.CreateProxySwapChain/CreateProxyInterop calls when making the change.
In `@src/Features/Upscaling/DX12SwapChain.cpp`:
- Around line 174-178: DX12SwapChain::GetDevice is returning a borrowed pointer
via globals::features::dx12Interop.d3d11Device.get(), violating COM _COM_Outptr_
semantics; instead call QueryInterface on the internal D3D11 device to return an
AddRef'd pointer. Replace the direct assignment to *ppDevice with a
QueryInterface(uuid, ppDevice) against
globals::features::dx12Interop.d3d11Device (or obtain the raw
IUnknown/IInspectable and call QueryInterface) and return its HRESULT so the out
parameter is properly AddRef'd.
🪄 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 Plus
Run ID: ce5d3cb9-c56c-4152-9c9f-3ed14766a17f
📒 Files selected for processing (15)
features/DX12 Interop/COREfeatures/DX12 Interop/Shaders/Features/DX12Interop.inisrc/Feature.cppsrc/Features/DX12Interop.cppsrc/Features/DX12Interop.hsrc/Features/DX12Interop/WrappedResource.cppsrc/Features/DX12Interop/WrappedResource.hsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/Upscaling/DX12SwapChain.cppsrc/Features/Upscaling/DX12SwapChain.hsrc/Features/Upscaling/FidelityFX.cppsrc/Features/Upscaling/FidelityFX.hsrc/Globals.cppsrc/Globals.h
💤 Files with no reviewable changes (1)
- src/Features/Upscaling.h
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/Features/DX12Interop.cpp (1)
116-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail DX12Interop initialization closed (and guard SetupResources/shared resource descriptors + cleanup)
DX12Interop::Init()setsactive = trueand then callsInitializePIX(),CreateD3D12Device(), andCreateInterop(); these useDX::ThrowIfFailed(...), so a bad adapter/debug-layer/interop setup can throw and take down the feature stack instead of leavingDX12Interopinactive.DX12Interop::SetupResources()is reached for loaded features without guarding on successful init, yet it buildssharedResources.depthandsharedResources.motionVectorby reusingmain.texture->GetDesc()and mutating onlyFormat/BindFlags(descriptor fidelity gaps), andDX12Interop::~DX12Interop()doesn’tdeletethenew WrappedResource(...)allocations forsharedResources.*(leaks).Proposed fix
void DX12Interop::Init(ID3D11Device* a_d3d11Device, ID3D11DeviceContext* a_immediateContext, IDXGIAdapter* a_adapter) { + active = false; + if (!D3D12Mode()) return; - active = true; - - SetD3D11Device(a_d3d11Device); - SetD3D11DeviceContext(a_immediateContext); - - InitializePIX(); - - CreateD3D12Device(a_adapter); - - CreateInterop(); + try { + SetD3D11Device(a_d3d11Device); + SetD3D11DeviceContext(a_immediateContext); + InitializePIX(); + CreateD3D12Device(a_adapter); + CreateInterop(); + active = true; + } catch (const std::exception& e) { + logger::error("[DX12Interop] Initialization failed: {}", e.what()); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/DX12Interop.cpp` around lines 116 - 131, DX12Interop::Init currently flips active=true before calling InitializePIX(), CreateD3D12Device(), and CreateInterop(), so thrown errors leave the feature half-initialized; change Init to perform initialization steps inside a guarded/try block and only set active=true after all DX::ThrowIfFailed calls succeed (on failure set active=false and return/clean up). Add an early guard in SetupResources to skip resource setup unless active is true. When building sharedResources.depth and sharedResources.motionVector, don't mutate only Format/BindFlags on the main.texture->GetDesc() result—create a fresh D3D11_TEXTURE2D_DESC copied from main.texture's desc and fully specify all fields to preserve descriptor fidelity. Finally, free WrappedResource allocations in DX12Interop::~DX12Interop (or convert them to smart pointers) to avoid leaks (release/delete sharedResources.depth and sharedResources.motionVector WrappedResource instances and any other new WrappedResource allocations).src/Features/Upscaling.cpp (1)
128-132:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winOnly enter the proxy path when DX12 interop is actually ready.
This still calls
CreateProxySwapChain()/CreateProxyInterop()even when interop is disabled orInit()failed to populate the D3D11/D3D12 objects they depend on. That converts an optional feature failure into a crash instead of a fallback.As per coding guidelines, "Include proper resource management and graceful degradation error handling in all DirectX code to prevent crashes from malformed configurations".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/Upscaling.cpp` around lines 128 - 132, Wrap the proxy creation calls so they only run when DX12 interop is actually ready: call dx12Interop.Init(*ppDevice, *ppImmediateContext, pAdapter) as you already do, but then check dx12Interop.loaded (and validate the D3D11/D3D12 objects are non-null if available) before invoking upscaling.CreateProxySwapChain(pAdapter, *pSwapChainDesc) and upscaling.CreateProxyInterop(); if Init fails/loaded is false, skip those CreateProxy* calls and log or fall back to the non-proxy path so the optional interop failure does not cause a crash.src/Features/Upscaling/DX12SwapChain.cpp (1)
174-178:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn an AddRef'd D3D11 device from
GetDevice().Assigning
d3d11Device.get()here hands out a borrowed COM pointer._COM_Outptr_callers are allowed toRelease()what they receive, so this should go throughQueryInterfaceand fail cleanly if interop never initialized.🛠️ Proposed fix
if (uuid == __uuidof(ID3D11Device) || uuid == __uuidof(ID3D11Device1) || uuid == __uuidof(ID3D11Device2) || uuid == __uuidof(ID3D11Device3) || uuid == __uuidof(ID3D11Device4) || uuid == __uuidof(ID3D11Device5)) { - *ppDevice = globals::features::dx12Interop.d3d11Device.get(); - return S_OK; + auto* device = globals::features::dx12Interop.d3d11Device.get(); + return device ? device->QueryInterface(uuid, ppDevice) : E_NOINTERFACE; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/Upscaling/DX12SwapChain.cpp` around lines 174 - 178, DX12SwapChain::GetDevice is returning a borrowed pointer from globals::features::dx12Interop.d3d11Device.get(), which lets callers Release() an un-AddRef'd COM pointer; change the logic to return an AddRef'd interface pointer (use the d3d11Device's QueryInterface for the requested REFIID or call AddRef on the matching interface) and store the result into *ppDevice, returning an appropriate HRESULT on failure (e.g., E_NOINTERFACE or a failure from QueryInterface) so callers can safely Release() what they receive. Ensure the function fails cleanly if dx12Interop/d3d11Device is not initialized.
🧹 Nitpick comments (1)
src/Features/Upscaling/DX12SwapChain.cpp (1)
140-152: ⚡ Quick winAvoid heap churn in
Present()barrier setup.This path allocates two
std::vectors per present just to submit two barriers each time. A fixed-size stack array is cheaper here.♻️ Proposed refactor
- { - std::vector<D3D12_RESOURCE_BARRIER> barriers; - barriers.push_back(CD3DX12_RESOURCE_BARRIER::Transition(fakeSwapChain, D3D12_RESOURCE_STATE_COMMON, D3D12_RESOURCE_STATE_COPY_SOURCE)); - barriers.push_back(CD3DX12_RESOURCE_BARRIER::Transition(realSwapChain, D3D12_RESOURCE_STATE_PRESENT, D3D12_RESOURCE_STATE_COPY_DEST)); - commandList->ResourceBarrier(static_cast<UINT>(barriers.size()), barriers.data()); - } + { + const std::array barriers = { + CD3DX12_RESOURCE_BARRIER::Transition(fakeSwapChain, D3D12_RESOURCE_STATE_COMMON, D3D12_RESOURCE_STATE_COPY_SOURCE), + CD3DX12_RESOURCE_BARRIER::Transition(realSwapChain, D3D12_RESOURCE_STATE_PRESENT, D3D12_RESOURCE_STATE_COPY_DEST) + }; + commandList->ResourceBarrier(static_cast<UINT>(barriers.size()), barriers.data()); + } @@ - { - std::vector<D3D12_RESOURCE_BARRIER> barriers; - barriers.push_back(CD3DX12_RESOURCE_BARRIER::Transition(fakeSwapChain, D3D12_RESOURCE_STATE_COPY_SOURCE, D3D12_RESOURCE_STATE_COMMON)); - barriers.push_back(CD3DX12_RESOURCE_BARRIER::Transition(realSwapChain, D3D12_RESOURCE_STATE_COPY_DEST, D3D12_RESOURCE_STATE_PRESENT)); - commandList->ResourceBarrier(static_cast<UINT>(barriers.size()), barriers.data()); - } + { + const std::array barriers = { + CD3DX12_RESOURCE_BARRIER::Transition(fakeSwapChain, D3D12_RESOURCE_STATE_COPY_SOURCE, D3D12_RESOURCE_STATE_COMMON), + CD3DX12_RESOURCE_BARRIER::Transition(realSwapChain, D3D12_RESOURCE_STATE_COPY_DEST, D3D12_RESOURCE_STATE_PRESENT) + }; + commandList->ResourceBarrier(static_cast<UINT>(barriers.size()), barriers.data()); + }As per coding guidelines, "Flag potential performance impacts on rendering performance when implementing graphics features and suggest user toggles for feature control".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Features/Upscaling/DX12SwapChain.cpp` around lines 140 - 152, Present() currently allocates two std::vector<D3D12_RESOURCE_BARRIER> per frame to build two-barrier lists for transitions; replace these heap allocations with fixed-size stack arrays (e.g., D3D12_RESOURCE_BARRIER barriers[2]) when preparing the barrier lists for commandList->ResourceBarrier to eliminate per-present heap churn. Update both the first barrier block that transitions fakeSwapChain->COPY_SOURCE and realSwapChain->COPY_DEST and the second block that transitions them back to COMMON/PRESENT, ensuring you pass the correct count (2) and pointer to commandList->ResourceBarrier and keep using fakeSwapChain and realSwapChain names as the referenced resources.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Features/DX12Interop.cpp`:
- Around line 214-223: Currently sharedResources.depth and
sharedResources.motionVector are created from the same mainDesc (used for color)
which can leave SampleDesc/ArraySize/flags mismatched; fix by building the
D3D11_DESC for each from the source textures via depth.texture->GetDesc(desc)
and motionVector.texture->GetDesc(desc) respectively, then only override Format
and BindFlags before calling new WrappedResource(...); additionally add a
runtime compatibility check comparing critical fields (SampleDesc, ArraySize,
misc flags, bind flags) and if incompatible disable/fallback the DX12 interop
path using the project's existing runtime-detection/fallback pattern (the
REL::RelocateMember() style used elsewhere) so interop is gracefully avoided
when descriptors don’t match.
In `@src/Features/DX12Interop/WrappedResource.cpp`:
- Around line 20-27: The SRV/RTV creation logic in WrappedResource.cpp
incorrectly always uses D3D11_SRV_DIMENSION_TEXTURE2D /
D3D11_RTV_DIMENSION_TEXTURE2D; update the branches that call
CreateShaderResourceView and CreateRenderTargetView to check a_texDesc.ArraySize
and use the Array variants when ArraySize > 1: set srvDesc.ViewDimension =
D3D11_SRV_DIMENSION_TEXTURE2DARRAY and fill
srvDesc.Texture2DArray.{MostDetailedMip, MipLevels, FirstArraySlice = 0,
ArraySize = a_texDesc.ArraySize}; similarly set rtvDesc.ViewDimension =
D3D11_RTV_DIMENSION_TEXTURE2DARRAY and fill rtvDesc.Texture2DArray.{MipSlice =
0, FirstArraySlice = 0, ArraySize = a_texDesc.ArraySize}; ensure these changes
are applied where CreateShaderResourceView/CreateRenderTargetView and variables
srv/rtv are used.
---
Duplicate comments:
In `@src/Features/DX12Interop.cpp`:
- Around line 116-131: DX12Interop::Init currently flips active=true before
calling InitializePIX(), CreateD3D12Device(), and CreateInterop(), so thrown
errors leave the feature half-initialized; change Init to perform initialization
steps inside a guarded/try block and only set active=true after all
DX::ThrowIfFailed calls succeed (on failure set active=false and return/clean
up). Add an early guard in SetupResources to skip resource setup unless active
is true. When building sharedResources.depth and sharedResources.motionVector,
don't mutate only Format/BindFlags on the main.texture->GetDesc() result—create
a fresh D3D11_TEXTURE2D_DESC copied from main.texture's desc and fully specify
all fields to preserve descriptor fidelity. Finally, free WrappedResource
allocations in DX12Interop::~DX12Interop (or convert them to smart pointers) to
avoid leaks (release/delete sharedResources.depth and
sharedResources.motionVector WrappedResource instances and any other new
WrappedResource allocations).
In `@src/Features/Upscaling.cpp`:
- Around line 128-132: Wrap the proxy creation calls so they only run when DX12
interop is actually ready: call dx12Interop.Init(*ppDevice, *ppImmediateContext,
pAdapter) as you already do, but then check dx12Interop.loaded (and validate the
D3D11/D3D12 objects are non-null if available) before invoking
upscaling.CreateProxySwapChain(pAdapter, *pSwapChainDesc) and
upscaling.CreateProxyInterop(); if Init fails/loaded is false, skip those
CreateProxy* calls and log or fall back to the non-proxy path so the optional
interop failure does not cause a crash.
In `@src/Features/Upscaling/DX12SwapChain.cpp`:
- Around line 174-178: DX12SwapChain::GetDevice is returning a borrowed pointer
from globals::features::dx12Interop.d3d11Device.get(), which lets callers
Release() an un-AddRef'd COM pointer; change the logic to return an AddRef'd
interface pointer (use the d3d11Device's QueryInterface for the requested REFIID
or call AddRef on the matching interface) and store the result into *ppDevice,
returning an appropriate HRESULT on failure (e.g., E_NOINTERFACE or a failure
from QueryInterface) so callers can safely Release() what they receive. Ensure
the function fails cleanly if dx12Interop/d3d11Device is not initialized.
---
Nitpick comments:
In `@src/Features/Upscaling/DX12SwapChain.cpp`:
- Around line 140-152: Present() currently allocates two
std::vector<D3D12_RESOURCE_BARRIER> per frame to build two-barrier lists for
transitions; replace these heap allocations with fixed-size stack arrays (e.g.,
D3D12_RESOURCE_BARRIER barriers[2]) when preparing the barrier lists for
commandList->ResourceBarrier to eliminate per-present heap churn. Update both
the first barrier block that transitions fakeSwapChain->COPY_SOURCE and
realSwapChain->COPY_DEST and the second block that transitions them back to
COMMON/PRESENT, ensuring you pass the correct count (2) and pointer to
commandList->ResourceBarrier and keep using fakeSwapChain and realSwapChain
names as the referenced resources.
🪄 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 Plus
Run ID: e226f954-be91-465a-9534-397ab40b9e05
📒 Files selected for processing (15)
features/DX12 Interop/COREfeatures/DX12 Interop/Shaders/Features/DX12Interop.inisrc/Feature.cppsrc/Features/DX12Interop.cppsrc/Features/DX12Interop.hsrc/Features/DX12Interop/WrappedResource.cppsrc/Features/DX12Interop/WrappedResource.hsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/Upscaling/DX12SwapChain.cppsrc/Features/Upscaling/DX12SwapChain.hsrc/Features/Upscaling/FidelityFX.cppsrc/Features/Upscaling/FidelityFX.hsrc/Globals.cppsrc/Globals.h
💤 Files with no reviewable changes (1)
- src/Features/Upscaling.h
|
✅ A pre-release build is available for this PR: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Features/Upscaling.cpp`:
- Around line 1625-1628: Upscaling::LoadUpscalingSDKs currently only calls
fidelityFX.LoadFFX() when globals::features::dx12Interop.loaded is true, but
FidelityFX::LoadFFX() both loads the common AMD loader and conditionally the
frame-generation DLL; split this into two steps so the common loader/function
initialization (the part that loads amd_fidelityfx_loader_dx12.dll and calls
ffxLoadFunctions(&ffxModule, module)) always runs when FSR can be used, and only
the frame-generation-specific load (loading
amd_fidelityfx_framegeneration_dx12.dll and setting featureFSR3FG) remains gated
by globals::features::dx12Interop.loaded. Update Upscaling::LoadUpscalingSDKs to
invoke the new common init unconditionally when FSR is selectable, and keep
frame-gen init conditional; also add a runtime guard/assert in
Upscaling::Upscale()/CreateFSRResources()/fidelityFX.Upscale() to ensure the
common FFX functions (ffxGetInterfaceDX11 / ffxFsr3ContextCreate /
ffxFsr3ContextDispatchUpscale) are loaded before calling DX11 FSR APIs.
🪄 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 Plus
Run ID: a403a56c-434f-4a53-9729-7dc9aa51acc3
📒 Files selected for processing (3)
src/Features/DX12Interop.cppsrc/Features/Upscaling.cppsrc/Features/Upscaling/DX12SwapChain.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/DX12Interop.cpp
Collaborator
|
@coderabbitai will this lead to any problems? does it cover all interop cases we need handled, in a performant way to minimise stutter? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat src/Features/DX12Interop.h
echo "---SPLIT---"
cat src/Features/DX12Interop.cpp
echo "---SPLIT---"
cat src/Features/DX12Interop/WrappedResource.cpp
echo "---SPLIT---"
cat src/Features/DX12Interop/WrappedResource.h
echo "---SPLIT---"
cat src/Features/Upscaling/DX12SwapChain.cpp
echo "---SPLIT---"
cat src/Features/Upscaling/DX12SwapChain.hLength of output: 1073 🏁 Script executed: #!/bin/bash
fd -e cpp -e h . --full-path | grep -iE "(DX12Interop|WrappedResource|DX12SwapChain)" | head -30
echo "---SPLIT---"
# Try to get the PR diff directly
gh pr view 2430 --repo community-shaders/skyrim-community-shaders --json files --jq '.files[].path' 2>/dev/null || trueLength of output: 816 🏁 Script executed: #!/bin/bash
# Fetch file contents from the PR branch using GitHub API
REPO="community-shaders/skyrim-community-shaders"
BRANCH="feat-dx12interop"
for f in "src/Features/DX12Interop.h" "src/Features/DX12Interop.cpp" "src/Features/DX12Interop/WrappedResource.cpp" "src/Features/DX12Interop/WrappedResource.h"; do
echo "=== $f ==="
gh api "repos/$REPO/contents/$f?ref=$BRANCH" --jq '.content' 2>/dev/null | base64 -d 2>/dev/null || echo "FAILED"
echo ""
doneLength of output: 715 🏁 Script executed: #!/bin/bash
gh pr diff 2430 --repo community-shaders/skyrim-community-shaders 2>/dev/null | head -2000Length of output: 43378
🔴 Critical Bug — Legacy vs. NT Shared HandleThe biggest correctness issue is in // New code — WRONG for D3D12
a_texDesc.MiscFlags |= D3D11_RESOURCE_MISC_SHARED; // no _NTHANDLE
dxgiResource->GetSharedHandle(&sharedHandle); // legacy handle
a_d3d12Device->OpenSharedHandle(sharedHandle, IID_PPV_ARGS(...)); // requires NT handle!But // Old code — correct
a_texDesc.MiscFlags |= D3D11_RESOURCE_MISC_SHARED | D3D11_RESOURCE_MISC_SHARED_NTHANDLE;
dxgiResource->CreateSharedHandle(nullptr, DXGI_SHARED_RESOURCE_READ | DXGI_SHARED_RESOURCE_WRITE, nullptr, &sharedHandle);
a_d3d12Device->OpenSharedHandle(sharedHandle, IID_PPV_ARGS(...));
CloseHandle(sharedHandle); // NT handle must be explicitly closedThe new code will fail at runtime ( 🟡 Potential GPU Hazard — Multiple
|
| Gap | Detail |
|---|---|
| Resize not handled | sharedResources (main RT, depth, motion vectors, reactive mask) are created once in SetupResources(). If the swap chain is resized, the wrapped resources will be stale. A OnResize / resource recreation path is missing. |
| Reactive mask not populated | sharedResources.reactiveMask is allocated but never written to or passed to the FidelityFX dispatch. FidelityFX will use a zero reactive mask — may be fine for now but limits quality with transparent/particle objects. |
| Compute & copy queues unused | computeCommandQueue and copyCommandQueue are created but Execute() only submits to commandQueue (direct). Future features expecting to use async compute via this API will have no path. |
🟢 Performance — Mostly Fine, Minor Notes
- CPU stall (
WaitForSingleObject(INFINITE)) when the GPU is behind by more thankMaxFramesInFlightframes is expected and correct. With 2-in-flight this is standard. std::vectorbarrier allocations insidePresent()on every frame are a small avoidable allocation. Fixed-size arrays (D3D12_RESOURCE_BARRIER barriers[2]) would be cleaner.- The synchronisation pattern itself (DX11→DX12 fence signal/wait → GPU work → DX12→DX11 fence signal/wait) is the standard approach and shouldn't introduce extra stutter beyond what was there before — assuming the NT handle bug is fixed so resources are actually shared correctly.
TL;DR for merging: The legacy-vs-NT handle bug will prevent the wrapped resources from functioning at all, so this needs fixing before merge. The multiple-Execute-per-frame hazard and NTDDI_VERSION in the header should also be addressed. The missing resize path is worth a follow-up issue since it exists in the current codebase too.
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/DX12Interop/WrappedResource.cpp (1)
7-18:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix D3D11→D3D12 shared-handle interop in
src/DX12Interop/WrappedResource.cpp
ID3D12Device::OpenSharedHandleonly works with NT shared handles. The current code creates the D3D11 texture withD3D11_RESOURCE_MISC_SHAREDand gets a handle viaIDXGIResource1::GetSharedHandle, so theOpenSharedHandle(...)call can fail at runtime. Create the D3D11 texture withD3D11_RESOURCE_MISC_SHARED_NTHANDLE, obtain the NT handle viaIDXGIResource1::CreateSharedHandle(...), then pass that handle toOpenSharedHandle(and ensure the NT handle is closed after use).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/DX12Interop/WrappedResource.cpp` around lines 7 - 18, The code uses D3D11_RESOURCE_MISC_SHARED and IDXGIResource1::GetSharedHandle which yields a non-NT handle that can make ID3D12Device::OpenSharedHandle fail; change the resource flags on a_texDesc to include D3D11_RESOURCE_MISC_SHARED_NTHANDLE before calling CreateTexture2D (instead of D3D11_RESOURCE_MISC_SHARED), obtain an NT handle by calling IDXGIResource1::CreateSharedHandle on the created resource11 (use the same dxgiResource from QueryInterface), pass that NT handle to a_d3d12Device->OpenSharedHandle(...) for resource, and ensure you call CloseHandle on the NT handle after OpenSharedHandle returns (and propagate/check errors from CreateSharedHandle/OpenSharedHandle appropriately).
🧹 Nitpick comments (1)
src/Globals.h (1)
265-265: 🏗️ Heavy liftKeep DX12 interop under the existing globals namespaces.
Exposing
dx12Interopatglobals::root creates a third lookup path for renderer state. Please register it underglobals::d3dorglobals::featuresso new call sites follow the same access pattern as the rest of the renderer.As per coding guidelines, "Access core systems and feature registry through globals::features and globals::d3d namespaces defined in src/Globals.h".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Globals.h` at line 265, The extern declaration for DX12Interop (dx12Interop) is placed at the globals:: root which introduces a third access path; move the extern DX12Interop* dx12Interop declaration into the appropriate existing namespace (globals::d3d or globals::features) in src/Globals.h, update any call sites that reference globals::dx12Interop to use the chosen namespace (e.g., globals::d3d::dx12Interop or globals::features::dx12Interop), and ensure corresponding definition/registration and includes are adjusted to match the new namespace to keep renderer state access consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/State.cpp`:
- Around line 216-218: The code unconditionally calls
globals::dx12Interop->SetupResources() even when Init() may have skipped
creating interop devices; modify the startup so you only call
dx12Interop->SetupResources() when interop is actually active (e.g., check
globals::dx12Interop != nullptr && globals::dx12Interop->Active() or an
equivalent device-check method) and otherwise skip it cleanly; leave
Feature::ForEachLoadedFeature("SetupResources", ...) and
globals::deferred->SetupResources() unchanged, but ensure any wrapped-resource
creation inside DX12Interop::SetupResources() is guarded by that Active()/device
check to avoid using null DX11/DX12 devices.
---
Outside diff comments:
In `@src/DX12Interop/WrappedResource.cpp`:
- Around line 7-18: The code uses D3D11_RESOURCE_MISC_SHARED and
IDXGIResource1::GetSharedHandle which yields a non-NT handle that can make
ID3D12Device::OpenSharedHandle fail; change the resource flags on a_texDesc to
include D3D11_RESOURCE_MISC_SHARED_NTHANDLE before calling CreateTexture2D
(instead of D3D11_RESOURCE_MISC_SHARED), obtain an NT handle by calling
IDXGIResource1::CreateSharedHandle on the created resource11 (use the same
dxgiResource from QueryInterface), pass that NT handle to
a_d3d12Device->OpenSharedHandle(...) for resource, and ensure you call
CloseHandle on the NT handle after OpenSharedHandle returns (and propagate/check
errors from CreateSharedHandle/OpenSharedHandle appropriately).
---
Nitpick comments:
In `@src/Globals.h`:
- Line 265: The extern declaration for DX12Interop (dx12Interop) is placed at
the globals:: root which introduces a third access path; move the extern
DX12Interop* dx12Interop declaration into the appropriate existing namespace
(globals::d3d or globals::features) in src/Globals.h, update any call sites that
reference globals::dx12Interop to use the chosen namespace (e.g.,
globals::d3d::dx12Interop or globals::features::dx12Interop), and ensure
corresponding definition/registration and includes are adjusted to match the new
namespace to keep renderer state access consistent.
🪄 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 Plus
Run ID: 92312bad-1e12-424f-8881-5fb72723a86c
📒 Files selected for processing (11)
src/DX12Interop.cppsrc/DX12Interop.hsrc/DX12Interop/WrappedResource.cppsrc/DX12Interop/WrappedResource.hsrc/Features/Upscaling.cppsrc/Features/Upscaling/DX12SwapChain.cppsrc/Features/Upscaling/DX12SwapChain.hsrc/Features/Upscaling/FidelityFX.cppsrc/Globals.cppsrc/Globals.hsrc/State.cpp
💤 Files with no reviewable changes (2)
- src/DX12Interop/WrappedResource.h
- src/DX12Interop.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Features/Upscaling/FidelityFX.cpp
- src/Features/Upscaling/DX12SwapChain.h
- src/Features/Upscaling/DX12SwapChain.cpp
Contributor
Author
|
The reasoning for using legacy handles is that NT handles are more strict and thus format support is limited.
With legacy handles we can also potentially share the original render targets, without relying on copies. Leftovers from RT have been removed:
|
Contributor
|
Actionable comments posted: 0 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Features/Upscaling/DX12SwapChain.cpp (1)
296-299:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRecreate wrapped/shared interop resources on
ResizeBuffers()after a successful resizeIn
DXGISwapChainProxy::ResizeBuffers, the code just forwards to the underlying swap chain and never refreshes the resources thatPresent()uses:HRESULT STDMETHODCALLTYPE DXGISwapChainProxy::ResizeBuffers(UINT BufferCount, UINT Width, UINT Height, DXGI_FORMAT NewFormat, UINT SwapChainFlags) { return swapChain->ResizeBuffers(BufferCount, Width, Height, NewFormat, SwapChainFlags); }After
ResizeBuffers(),DX12SwapChainstill holds the originalswapChainBuffers[],swapChainBufferWrapped, anduiBufferWrappedcreated inCreateSwapChain()/CreateInterop()(no resize path updates them).Present()then copies/clears using those stale pointers (CopyResource(... swapChainBuffers[frameIndex] ...),ClearRenderTargetView(uiBufferWrapped->rtv, ...)), causing correctness issues when the backbuffer size changes.Also,
globals::dx12Interop->sharedResourcesis only (re)built inDX12Interop::SetupResources()anddx12Interop->SetupResources()is called only fromState::Setup(), so shared interop descriptors can also become stale after a resize.Suggested direction
HRESULT STDMETHODCALLTYPE DXGISwapChainProxy::ResizeBuffers(UINT BufferCount, UINT Width, UINT Height, DXGI_FORMAT NewFormat, UINT SwapChainFlags) { - return swapChain->ResizeBuffers(BufferCount, Width, Height, NewFormat, SwapChainFlags); + auto hr = swapChain->ResizeBuffers(BufferCount, Width, Height, NewFormat, SwapChainFlags); + if (FAILED(hr)) { + return hr; + } + + // Release and reacquire swap-chain-local wrapped resources/back buffers here. + // Then rebuild the global DX12 interop shared resources so their descriptors + // match the resized render targets. + return hr; }
♻️ Duplicate comments (2)
src/DX12Interop.cpp (2)
81-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRelease the previous wrapped resources before rebuilding them.
SetupResources()overwrites every raw pointer insharedResources, so any reinit/resize path leaks the old allocations and leaves cleanup to process exit.Suggested fix
void DX12Interop::SetupResources() { if (!active) return; + + auto release = [](WrappedResource*& resource) { + delete resource; + resource = nullptr; + }; + release(sharedResources.main); + release(sharedResources.depth); + release(sharedResources.motionVector); + release(sharedResources.reactiveMask); auto renderer = globals::game::renderer;As per coding guidelines,
src/**/*.{h,cpp}: Include proper resource management and graceful degradation error handling in all DirectX code to prevent crashes from malformed configurations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/DX12Interop.cpp` around lines 81 - 109, SetupResources currently overwrites raw pointers in sharedResources (sharedResources.main, .depth, .motionVector, .reactiveMask) and leaks previous WrappedResource instances; modify SetupResources to release existing resources before reassigning by checking each pointer and deleting (or calling appropriate Release/Destroy methods) and setting them to nullptr, or better convert those members to smart pointers (std::unique_ptr<WrappedResource>) and call reset/new so previous instances are freed automatically; ensure any failure to create a new WrappedResource leaves the corresponding sharedResources.* in a null/clean state to avoid use-after-free in cleanup paths.
15-28:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet
activeonly after interop finishes initializing.If
CreateD3D12Device()orCreateInterop()fails, this singleton stays marked active with incomplete state, and later callers will treat interop as usable.Suggested fix
void DX12Interop::Init(ID3D11Device* a_d3d11Device, ID3D11DeviceContext* a_immediateContext, IDXGIAdapter* a_adapter) { if (!D3D12Mode()) return; - active = true; + active = false; SetD3D11Device(a_d3d11Device); SetD3D11DeviceContext(a_immediateContext); CreateD3D12Device(a_adapter); CreateInterop(); + + active = true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/DX12Interop.cpp` around lines 15 - 28, The Init method sets active=true too early; move setting of the DX12Interop::active flag to after CreateD3D12Device(...) and CreateInterop() succeed so the singleton is only marked active on full initialization. Modify Init to call SetD3D11Device(...) and SetD3D11DeviceContext(...) first, then call CreateD3D12Device(...) and CreateInterop(), check their success (return values or exceptions) and only then set active = true; on failure ensure active remains false and perform any necessary cleanup of partial state. Reference: Init, CreateD3D12Device, CreateInterop, SetD3D11Device, SetD3D11DeviceContext.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Features/Upscaling/DX12SwapChain.cpp`:
- Around line 136-165: The completion lambda passed to interopContext->Execute
currently calls DX::ThrowIfFailed(swapChain->Present(...)), which hides and
throws HRESULT failures instead of returning them from the COM entrypoint
DX12SwapChain::Present (and thus DXGISwapChainProxy::Present); replace the
ThrowIfFailed call with explicit HRESULT handling: call HRESULT hr =
swapChain->Present(SyncInterval, Flags) inside the completion lambda,
capture/record that hr (e.g., store into a local variable visible to the outer
DX12SwapChain::Present scope or return it from the Execute call if supported),
and ensure DX12SwapChain::Present returns that HRESULT value instead of always
returning S_OK; update any control flow to propagate the actual hr back to
DXGISwapChainProxy::Present.
---
Duplicate comments:
In `@src/DX12Interop.cpp`:
- Around line 81-109: SetupResources currently overwrites raw pointers in
sharedResources (sharedResources.main, .depth, .motionVector, .reactiveMask) and
leaks previous WrappedResource instances; modify SetupResources to release
existing resources before reassigning by checking each pointer and deleting (or
calling appropriate Release/Destroy methods) and setting them to nullptr, or
better convert those members to smart pointers
(std::unique_ptr<WrappedResource>) and call reset/new so previous instances are
freed automatically; ensure any failure to create a new WrappedResource leaves
the corresponding sharedResources.* in a null/clean state to avoid
use-after-free in cleanup paths.
- Around line 15-28: The Init method sets active=true too early; move setting of
the DX12Interop::active flag to after CreateD3D12Device(...) and CreateInterop()
succeed so the singleton is only marked active on full initialization. Modify
Init to call SetD3D11Device(...) and SetD3D11DeviceContext(...) first, then call
CreateD3D12Device(...) and CreateInterop(), check their success (return values
or exceptions) and only then set active = true; on failure ensure active remains
false and perform any necessary cleanup of partial state. Reference: Init,
CreateD3D12Device, CreateInterop, SetD3D11Device, SetD3D11DeviceContext.
🪄 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 Plus
Run ID: 8627ec6c-a2ed-4b47-8ae2-77c47a8dd785
📒 Files selected for processing (5)
src/DX12Interop.cppsrc/DX12Interop.hsrc/DX12Interop/InteropContext.hsrc/Features/Upscaling/DX12SwapChain.cppsrc/Features/Upscaling/DX12SwapChain.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/Upscaling/DX12SwapChain.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Decouples D3D12 interoperability from upscaling/framegen, allowing future features to also take advantage of the API.
The only difference is that the original interop indexed double buffering on
IDXGISwapChain3::GetCurrentBackBufferIndex()while I do it onframeCount % kMaxFramesInFlight, the reason being thatDX12Interop::Executecan be called from anywhere in the code and not just right beforePresent.Summary by CodeRabbit
New Features
Refactor