Graphics: (d3d12 pt II) SRV descriptor slot allocator#213
Open
Kheartz wants to merge 1 commit into
Open
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
ChangesD3D12Backend SRV Slot Pool
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
🧹 Nitpick comments (2)
code/framework/src/graphics/backend/d3d12.cpp (2)
108-122: 💤 Low valueInconsistent nulling of heap pointers after Release.
_srvHeapis set tonullptrafter release (line 112), but_rtvHeapis not. For consistency and to guard against accidental use-after-free, consider nulling both.♻️ Proposed fix
// release objects _rtvHeap->Release(); + _rtvHeap = nullptr; _srvHeap->Release(); _srvHeap = nullptr;🤖 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 108 - 122, In the D3D12Backend::Shutdown() method, after calling _rtvHeap->Release(), add _rtvHeap = nullptr; to match the pattern already applied to _srvHeap. This ensures consistency in nulling heap pointers after release and guards against potential use-after-free errors. The _srvHeap is already correctly set to nullptr after its Release() call, so apply the same pattern to _rtvHeap immediately following its Release() call.
50-59: 💤 Low valueConsider renaming
extraSrvStartfor clarity.The variable name
extraSrvStartis misleading since it equals_frameBufferCount - 1, which is one less than the actual starting index of extra slots. The math is correct (slots_frameBufferCountthrough_frameBufferCount + 63are pushed), but the name suggests it's the start index when it's actually used as an offset base.♻️ Proposed refactor for clarity
// Manage slots that are not reserved for ImGui and headroom _srvDescriptorSize = pD3DDevice->GetDescriptorHandleIncrementSize(D3D12_DESCRIPTOR_HEAP_TYPE_CBV_SRV_UAV); _srvHeapSize = desc.NumDescriptors; _srvSlotInUse.assign(_srvHeapSize, false); _freeSrvSlots.clear(); - const auto extraSrvStart = _frameBufferCount - 1; - for (UINT i = kExtraSrvSlots; i > 0; i--) { - _freeSrvSlots.push_back(extraSrvStart + i); + const UINT extraSrvStart = _frameBufferCount; + for (UINT i = 0; i < kExtraSrvSlots; i++) { + _freeSrvSlots.push_back(extraSrvStart + kExtraSrvSlots - 1 - i); }Or simply:
- const auto extraSrvStart = _frameBufferCount - 1; - for (UINT i = kExtraSrvSlots; i > 0; i--) { - _freeSrvSlots.push_back(extraSrvStart + i); - } + for (UINT i = 0; i < kExtraSrvSlots; i++) { + _freeSrvSlots.push_back(_frameBufferCount + kExtraSrvSlots - 1 - i); + }🤖 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 50 - 59, The variable extraSrvStart is misleadingly named because it is assigned the value _frameBufferCount - 1 and used as an offset base (added to loop counter i in the subsequent for loop), not as an actual starting index. Rename extraSrvStart to a more descriptive name that better reflects its purpose as an offset base value (such as extraSrvBaseOffset or slotBaseOffset) to clarify that it represents an adjustment factor rather than a true starting index, then update the reference to this variable in the _freeSrvSlots.push_back 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.
Inline comments:
In `@code/framework/src/graphics/backend/d3d12.h`:
- Around line 63-67: Correct the spelling error in the documentation comments
for GetSRVSlotCPUHandle and GetSRVSlotGPUHandle methods by changing "Retreives"
to "Retrieves" in both comment lines to fix the grammar and improve
documentation quality.
---
Nitpick comments:
In `@code/framework/src/graphics/backend/d3d12.cpp`:
- Around line 108-122: In the D3D12Backend::Shutdown() method, after calling
_rtvHeap->Release(), add _rtvHeap = nullptr; to match the pattern already
applied to _srvHeap. This ensures consistency in nulling heap pointers after
release and guards against potential use-after-free errors. The _srvHeap is
already correctly set to nullptr after its Release() call, so apply the same
pattern to _rtvHeap immediately following its Release() call.
- Around line 50-59: The variable extraSrvStart is misleadingly named because it
is assigned the value _frameBufferCount - 1 and used as an offset base (added to
loop counter i in the subsequent for loop), not as an actual starting index.
Rename extraSrvStart to a more descriptive name that better reflects its purpose
as an offset base value (such as extraSrvBaseOffset or slotBaseOffset) to
clarify that it represents an adjustment factor rather than a true starting
index, then update the reference to this variable in the _freeSrvSlots.push_back
call.
🪄 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: 3bde4d04-db29-44f9-95ec-f70b9eb39c51
📒 Files selected for processing (2)
code/framework/src/graphics/backend/d3d12.cppcode/framework/src/graphics/backend/d3d12.h
Adds a bounded, shader-visible SRV slot pool on the D3D12 backend for upcoming web-view textures. The heap grows by a fixed kExtraSrvSlots (64) past the ImGui-reserved slots; AllocateSRVSlot/FreeSRVSlot hand out and reclaim indices into that range, and the slots share ImGui's heap so handles double as ImTextureID. Guards, since the eventual consumer (CEF views) renders partly untrusted content into a heap shared with the trusted in-game UI: - the slot getters bounds-check and return a null handle for any out-of-range index, so an exhausted -1 or a stray id can never produce an out-of-heap descriptor - FreeSRVSlot rejects out-of-pool indices and double-frees via an in-use bitmap, so a bad id can't re-enter the free list and alias a live slot - a mutex serializes allocate/free against concurrent view lifecycle Bounded by the fixed 64-slot cap. Gating (opt-in reservation) is left as a follow-up pending the rendering backend's configuration story; no consumer allocates slots yet.
Kheartz
force-pushed
the
d3d12-srv-allocator
branch
from
79b9dbf to
1fb9e4a
Compare
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
1 participant
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.
Adds a bounded, shader-visible SRV slot pool on the D3D12 backend for upcoming web-view textures. The heap grows by a fixed
kExtraSrvSlots(64) past the ImGui-reserved slots;AllocateSRVSlot/FreeSRVSlothand out and reclaim indices into that range, and the slots share ImGui's heap so handles double asImTextureID.Guards, since the eventual consumer (CEF views) renders partly untrusted content into a heap shared with the trusted in-game UI:
FreeSRVSlotrejects out-of-pool indices and double-frees via an in-use bitmap, so a bad id can't re-enter the free list and alias a live slotBounded by the fixed 64-slot cap. Gating (opt-in reservation) is left as a follow-up pending the rendering backend's configuration story; no consumer allocates slots yet.
Summary by CodeRabbit