Graphics: (d3d12 pt I) report configured backend and unify D3D12 device#208
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughBackend template members are now value-initialized to deterministic defaults. D3D12Backend changes device initialization to source context and device from new origins: context from options and device from the swap chain via COM guard. Device cleanup is made explicit with null-check release. Renderer additionally caches the selected backend enum in a member field. ChangesBackend Device Initialization and Caching
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/framework/src/graphics/backend/d3d12.cpp`:
- Around line 23-27: The swapChain->GetDevice() call returns a COM interface
with an incremented reference count that must be released to avoid memory leaks.
Add a Release() call to _device in the Shutdown() method to properly balance the
reference count obtained during Init(). Additionally, to prevent leaks from
early returns in Init() (such as the return false at line 25), ensure that any
early exit properly handles reference management, or structure the code so that
_device is only assigned after all initialization steps that could fail have
completed successfully.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9fdd052-bdd1-4696-9f5e-92790f1af363
📒 Files selected for processing (2)
code/framework/src/graphics/backend/d3d12.cppcode/framework/src/graphics/renderer.cpp
Kheartz
force-pushed
the
d3d12-renderer-backend
branch
from
cb8c1a1 to
f672e3f
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/framework/src/graphics/backend/d3d12.cpp (1)
78-81:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResource leak: orphaned command allocator.
The
allocatorcreated at line 79 is never used and never released. The frame context allocators are created separately in the loop at lines 83-87, making this allocation dead code that leaks a COM object on everyInit()call.Remove this unused allocation:
🐛 Proposed fix
{ - ID3D12CommandAllocator *allocator {nullptr}; - if (pD3DDevice->CreateCommandAllocator(D3D12_COMMAND_LIST_TYPE_DIRECT, IID_PPV_ARGS(&allocator)) != S_OK) { - return false; - } - for (size_t i = 0; i < _frameBufferCount; i++) { if (pD3DDevice->CreateCommandAllocator(D3D12_COMMAND_LIST_TYPE_DIRECT, IID_PPV_ARGS(&_frameContext[i]._commandAllocator)) != S_OK) { return false; } }🤖 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 `@code/framework/src/graphics/backend/d3d12.cpp` around lines 78 - 81, Remove the unused command allocator creation block (the allocator variable declaration and the pD3DDevice->CreateCommandAllocator call in the if statement) as it is dead code that never gets used or released. The frame context allocators are created separately in the subsequent loop at lines 83-87, making this allocation a COM object resource leak that occurs on every Init() call.
🤖 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.
Outside diff comments:
In `@code/framework/src/graphics/backend/d3d12.cpp`:
- Around line 78-81: Remove the unused command allocator creation block (the
allocator variable declaration and the pD3DDevice->CreateCommandAllocator call
in the if statement) as it is dead code that never gets used or released. The
frame context allocators are created separately in the subsequent loop at lines
83-87, making this allocation a COM object resource leak that occurs on every
Init() call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 936ee22b-6787-45eb-9eb3-08af8c066627
📒 Files selected for processing (2)
code/framework/src/graphics/backend/d3d12.cppcode/framework/src/graphics/renderer.cpp
✅ Files skipped from review due to trivial changes (1)
- code/framework/src/graphics/renderer.cpp
|
@Segfaultd allocator leak coderabbit flagged seems legit, but not caused by these changes. Lemme know if you still want it addressed here. |
|
@Kheartz I'm ok with the changes, but please reduce the amount of block comments that your AI is putting everywhere. Most of them are explaining obvious stuffs :-) |
Kheartz
force-pushed
the
d3d12-renderer-backend
branch
from
f672e3f to
200d093
Compare
Kheartz
force-pushed
the
d3d12-renderer-backend
branch
2 times, most recently
from
9d7e9e7 to
89c2627
Compare
The renderer never set _backend, so GetBackendType() always returned the D3D11 default and D3D12 consumers silently built no-op views. Set it from the configuration at Init. The D3D12 backend created its descriptor heaps and RTVs from the swapchain's device but stored opts.d3d12.device, so resources later created through GetDevice() could come from a different device and be rejected by D3D12. Use the swapchain device as the single device. GetDevice() AddRefs the returned device; own that reference in a ComPtr so every Init early-out frees it (Renderer::Init drops the backend without calling Shutdown on failure) and Detach it to _device only on success, where Shutdown releases it.
Kheartz
force-pushed
the
d3d12-renderer-backend
branch
from
4726649 to
5fd25f9
Compare
2 participants
Part 1/N:
The renderer never set
_backend, soGetBackendType()always returned the D3D11 default and D3D12 consumers silently built no-op views. Set it from the configuration at Init.The D3D12 backend created its descriptor heaps and RTVs from the swapchain's device but stored opts.d3d12.device, so resources later created through
GetDevice()could come from a different device and be rejected by D3D12. Use the swapchain device as the single device.With a device, the next PR will propose to allocate an additional pool D3D12 descriptors for our CEF webview textures (max 64)
Summary by CodeRabbit
Refactor
Bug Fixes