GUI: D3D12 web view backend and disk-served UI schemes#202
Closed
Kheartz wants to merge 2 commits into
Closed
Conversation
WalkthroughDirect3D 12 GUI rendering is added via ViewD3D12 with SRV-slot pooling and shutdown-safe GPU draining. CEF scheme-handler and pixel buffers gain mutex protection; Manager stages retired views for delayed GPU cleanup. Utilities provide secure path resolution and process-shutdown detection; unit tests validate path resolution. ChangesD3D12 GUI Rendering with Thread-Safe CEF Integration
Sequence Diagram(s)sequenceDiagram
participant Manager
participant ViewD3D12
participant RenderHandler
participant D3D12Backend
participant ImGui
Manager->>ViewD3D12: Render()
ViewD3D12->>RenderHandler: LockPixels()
ViewD3D12->>ViewD3D12: UploadPixels() with copy/barriers
ViewD3D12->>D3D12Backend: AllocateSRVSlot() and GetCurrentFrameIndex()
Manager->>ViewD3D12: SubmitImGuiDraw()
ViewD3D12->>ImGui: draw texture via SRV GPU handle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 4
🤖 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 215-218: D3D12Backend::WaitForGpu currently ignores the result of
WaitForSingleObject after fence->SetEventOnCompletion(1, event) which can allow
a timeout to be treated as success and let
ViewD3D12::CreateResources()/ReleaseResources reuse resources still in-flight;
change WaitForGpu to check the WaitForSingleObject return (WAIT_OBJECT_0 vs
WAIT_TIMEOUT/FAILED), then verify fence completion with
fence->GetCompletedValue() (or return a failure status) and propagate that
status to callers so CreateResources/ReleaseResources abort (and log) if the
fence did not actually complete, while still allowing special-cases for process
shutdown or device-removed paths.
- Around line 198-199: The backend must use the swap-chain device everywhere and
treat wait timeouts as failures: in D3D12Backend::Init assign the device
returned by swapChain->GetDevice() into opts.d3d12.device and _device so all
later creations (descriptor heaps, RTVs, SRVs, textures, fences) use the same
ID3D12Device; ensure D3D12Backend::Init uses that device when calling
CreateFence (the local 'fence' must be created from the swap-chain device stored
in _device/opts.d3d12.device). In D3D12Backend::WaitForGpu, check the result of
WaitForSingleObject(event, WAIT_TIMEOUT) and treat WAIT_TIMEOUT as a failure
(log/error and return a failure status instead of silently proceeding to free
GPU-referenced resources). Use the symbols D3D12Backend::Init,
swapChain->GetDevice(), opts.d3d12.device, _device, D3D12Backend::WaitForGpu,
WaitForSingleObject, and the local 'fence' to locate and update the code.
In `@code/framework/src/gui/cef/life_span_handler.cpp`:
- Around line 15-18: OnAfterCreated currently only sets _browser and increments
s_liveBrowserCount but doesn't invoke the stored callback; update
LifeSpanHandler::OnAfterCreated so that after assigning _browser and
incrementing s_liveBrowserCount it checks if _onAfterCreated is set and invokes
it (passing the CefRefPtr<CefBrowser> browser), and then clear or reset
_onAfterCreated if intended to fire only once; keep the invocation guarded
(null-check) to avoid crashes.
In `@code/tests/modules/path_resolve_ut.h`:
- Around line 18-22: Replace the non-portable preprocessor check using WIN32
with the standard _WIN32 macro so kPathSepIsWindows is reliably set on Windows;
update the conditional around the kPathSepIsWindows definition (the inline
constexpr bool kPathSepIsWindows symbol) to use `#ifdef` _WIN32 / `#else` / `#endif`
instead of `#ifdef` WIN32 so Windows toolchains consistently pick the Windows
branch.
🪄 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: 88ca901c-cc8f-4d28-bd46-72a45f3c8f81
📒 Files selected for processing (23)
code/framework/CMakeLists.txtcode/framework/src/graphics/backend/d3d12.cppcode/framework/src/graphics/backend/d3d12.hcode/framework/src/graphics/renderer.cppcode/framework/src/gui/backend/view_d3d11.cppcode/framework/src/gui/backend/view_d3d12.cppcode/framework/src/gui/backend/view_d3d12.hcode/framework/src/gui/cef/app.cppcode/framework/src/gui/cef/app.hcode/framework/src/gui/cef/disk_resource_handler.cppcode/framework/src/gui/cef/disk_resource_handler.hcode/framework/src/gui/cef/life_span_handler.cppcode/framework/src/gui/cef/life_span_handler.hcode/framework/src/gui/cef/render_handler.cppcode/framework/src/gui/cef/render_handler.hcode/framework/src/gui/manager.cppcode/framework/src/gui/manager.hcode/framework/src/gui/view.hcode/framework/src/utils/path.cppcode/framework/src/utils/path.hcode/framework/src/utils/process_shutdown.hcode/tests/framework_ut.cppcode/tests/modules/path_resolve_ut.h
Adds a D3D12 path for the CEF web UI (previously D3D11-only) and a
way to serve mod-bundled HTML UIs from disk.
Graphics:
- Renderer reports the configured backend type; it previously always
returned the D3D11 default, so D3D12 consumers silently built
no-op views
- D3D12Backend grows an SRV descriptor slot allocator (64 slots after
the ImGui font descriptors) and a fenced WaitForGpu
GUI:
- ViewD3D12 (CPU OSR): CEF OnPaint buffers upload through
per-frame-in-flight, row-pitch-aligned upload buffers and
CopyTextureRegion on the backend command list, drawn through the
ImGui background draw list (SRV handles double as ImTextureID since
ImGui initializes against the same heap). GPU-accelerated OSR stays
D3D11-only; ViewD3D12 falls back to the CPU path.
- Threading: Manager::Render (uploads) runs on the render thread
between backend Begin and the ImGui render; SubmitImGuiDraws runs
on the game thread inside the ImGui frame. The CEF pump runs
outside the render mutex: it dispatches the game window's messages
mid-tick, and a handler blocking on the render thread (the exit
flow's RHI flush) deadlocked against Render's lock on the render
thread - observed as a minutes-long exit stall. All _views readers
lock the (mutable) recursive mutex.
- RenderHandler owns its pixel-buffer synchronization: OnPaint
(running inside the pump) locks while writing, and both view
backends hold the same lock for the whole read - previously the
manager mutex around the pump was incidentally the only thing
serializing these.
- Destroyed views retire into a dying list for 8 ticks (draw data
built on the game thread can reference a texture for a couple of
frames after removal) and drain the GPU before freeing; the resize
path drains likewise before recreating texture, upload buffers and
the in-place SRV.
- CreateDiskResourceHandler serves files from a directory tree
through RegisterSchemeHandlerFactory: the URL path resolves against
a root dir via Utils::ResolvePathUnderRoot ("/" maps to index.html,
root-escape and drive-injection rejected, unit-tested), then
streams via CefStreamResourceHandler with Chromium's own mime table
(CefGetMimeType) and Cache-Control no-store (local files change
while iterating on UIs). Misses yield a 404 handler, never nullptr
- that would fall through to the network stack. File paths hand
CEF the wide string: path::string() narrows to the ANSI codepage on
Windows while CefString decodes std::string as UTF-8, so non-ASCII
install paths would 404.
- The scheme handler registry is guarded (Create runs per request on
the CEF IO thread, registration on the game thread) and factories
are cleared before CefShutdown - outstanding references can stall
it.
- Shutdown pumps the message loop until every browser has passed
OnBeforeClose (3s cap) plus a settle pass before CefShutdown,
skips CefShutdown during process teardown (CEF's threads are
already gone and it would deadlock), and writes an unbuffered
stage trace to logs/web_shutdown_trace.log - shutdown is exactly
where the regular log can no longer be read.
- Manager::Init resolves the CEF subprocess path from the module
owning the framework code (a game-injected DLL ships its own CEF
binaries next to itself) and fails loudly when that resolution
fails; the CEF cache is scoped per process id so two clients on
one machine don't fight over the process-singleton lock.
Utils:
- ResolvePathUnderRoot: lexical URL-path-under-root resolution with
traversal/injection rejection, extracted CEF-free so it unit-tests
(code/tests/modules/path_resolve_ut.h - 7 cases incl. dot-dot,
backslash and drive injection)
- IsProcessShutdownInProgress (RtlDllShutdownInProgress wrapper)
Verified in HogwartsMP (D3D12): interactive fullscreen views,
transparent HTML chat overlay served from a local scheme, clean ~1s
CEF teardown on game exit. FrameworkTests: 100 tests, the single
js_features failure is pre-existing on the base revision.
Known limitations: CPU OSR only on D3D12, full-buffer uploads (no
dirty-rect optimization), no HTTP Range support in the disk handler
(CefStreamResourceHandler serves whole files), swapchain resize
remains a passthrough.
The D3D12 backend cached the swapchain back buffers for the whole process lifetime and rendered the overlay to them through its own command list. Holding those references made the game's IDXGISwapChain::ResizeBuffers fail with DXGI_ERROR_INVALID_CALL, which UE4 treats as fatal — crashing on any fullscreen toggle or resolution change (F11, Alt+Enter, the Settings > Display menu). Acquire the current back buffer fresh in Begin() and release it in End(), so no swapchain reference is held between frames; the game owns the resize entirely and we no longer touch ResizeBuffers. CEF views still follow the viewport via Manager::Resize.
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 191-196: The FreeSRVSlot method currently only validates that the
slot is non-negative but fails to check if the slot is within the valid range,
if it has already been freed, or if it is a reserved index for ImGui. Add bounds
checking to ensure the slot is within the maximum valid descriptor range, add a
check to prevent duplicate frees by verifying the slot is not already in the
_freeSrvSlots list, and add validation to skip any ImGui-reserved slot indices.
Apply identical validation logic to the other slot-freeing methods referenced at
lines 198-205 and 207-214 to prevent invalid descriptor handles and aliasing
issues across all similar functions.
🪄 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: 276c867b-f4b1-4658-8409-51db98d5c37d
📒 Files selected for processing (24)
code/framework/CMakeLists.txtcode/framework/src/graphics/backend/d3d12.cppcode/framework/src/graphics/backend/d3d12.hcode/framework/src/graphics/renderer.cppcode/framework/src/gui/backend/view_d3d11.cppcode/framework/src/gui/backend/view_d3d12.cppcode/framework/src/gui/backend/view_d3d12.hcode/framework/src/gui/cef/app.cppcode/framework/src/gui/cef/app.hcode/framework/src/gui/cef/disk_resource_handler.cppcode/framework/src/gui/cef/disk_resource_handler.hcode/framework/src/gui/cef/life_span_handler.cppcode/framework/src/gui/cef/life_span_handler.hcode/framework/src/gui/cef/render_handler.cppcode/framework/src/gui/cef/render_handler.hcode/framework/src/gui/manager.cppcode/framework/src/gui/manager.hcode/framework/src/gui/view.cppcode/framework/src/gui/view.hcode/framework/src/utils/path.cppcode/framework/src/utils/path.hcode/framework/src/utils/process_shutdown.hcode/tests/framework_ut.cppcode/tests/modules/path_resolve_ut.h
✅ Files skipped from review due to trivial changes (1)
- code/framework/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (17)
- code/framework/src/gui/cef/render_handler.cpp
- code/tests/framework_ut.cpp
- code/framework/src/utils/path.cpp
- code/framework/src/gui/cef/life_span_handler.cpp
- code/framework/src/gui/backend/view_d3d11.cpp
- code/framework/src/gui/cef/disk_resource_handler.h
- code/framework/src/gui/cef/render_handler.h
- code/framework/src/gui/cef/app.cpp
- code/framework/src/gui/cef/life_span_handler.h
- code/framework/src/graphics/renderer.cpp
- code/framework/src/gui/manager.h
- code/framework/src/gui/backend/view_d3d12.h
- code/tests/modules/path_resolve_ut.h
- code/framework/src/graphics/backend/d3d12.h
- code/framework/src/utils/path.h
- code/framework/src/gui/backend/view_d3d12.cpp
- code/framework/src/gui/manager.cpp
Comment on lines
+191
to
+196
| void D3D12Backend::FreeSRVSlot(int slot) { | ||
| if (slot < 0) { | ||
| return; | ||
| } | ||
| _freeSrvSlots.push_back(static_cast<UINT>(slot)); | ||
| } |
There was a problem hiding this comment.
Validate SRV slot bounds and prevent duplicate frees
Line 195 currently accepts any non-negative slot, including ImGui-reserved indices and out-of-range values. Combined with Lines 203/212, that can generate invalid descriptor handles and alias descriptors after accidental double-free.
Suggested minimal hardening
+#include <algorithm>
...
void D3D12Backend::FreeSRVSlot(int slot) {
- if (slot < 0) {
+ const int minSlot = static_cast<int>(_frameBufferCount);
+ const int maxSlot = static_cast<int>(_frameBufferCount + kExtraSrvSlots);
+ if (slot < minSlot || slot >= maxSlot) {
return;
}
- _freeSrvSlots.push_back(static_cast<UINT>(slot));
+ const UINT uSlot = static_cast<UINT>(slot);
+ if (std::find(_freeSrvSlots.begin(), _freeSrvSlots.end(), uSlot) != _freeSrvSlots.end()) {
+ return;
+ }
+ _freeSrvSlots.push_back(uSlot);
}
D3D12_CPU_DESCRIPTOR_HANDLE D3D12Backend::GetSRVSlotCPUHandle(int slot) const {
- if (!_srvHeap) {
+ const int minSlot = static_cast<int>(_frameBufferCount);
+ const int maxSlot = static_cast<int>(_frameBufferCount + kExtraSrvSlots);
+ if (!_srvHeap || slot < minSlot || slot >= maxSlot) {
return {};
}
...
D3D12_GPU_DESCRIPTOR_HANDLE D3D12Backend::GetSRVSlotGPUHandle(int slot) const {
- if (!_srvHeap) {
+ const int minSlot = static_cast<int>(_frameBufferCount);
+ const int maxSlot = static_cast<int>(_frameBufferCount + kExtraSrvSlots);
+ if (!_srvHeap || slot < minSlot || slot >= maxSlot) {
return {};
}Also applies to: 198-205, 207-214
🤖 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 191 - 196, The
FreeSRVSlot method currently only validates that the slot is non-negative but
fails to check if the slot is within the valid range, if it has already been
freed, or if it is a reserved index for ImGui. Add bounds checking to ensure the
slot is within the maximum valid descriptor range, add a check to prevent
duplicate frees by verifying the slot is not already in the _freeSrvSlots list,
and add validation to skip any ImGui-reserved slot indices. Apply identical
validation logic to the other slot-freeing methods referenced at lines 198-205
and 207-214 to prevent invalid descriptor handles and aliasing issues across all
similar functions.
Contributor
Author
|
This PR will be abandoned for a multi-part PR starting with #208 |
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.
Hi,
Currently developing the mod for hogwarts legacy and currently experimenting with using CEF to draw custom UI, but ran into a couple of issues:
https://mafiamp.web.appas the webmanager, but for local testing, this PR introducesCreateDiskResourceHandlerto allow for local hosting of resource files.Full detailed description below:
==========================================
Adds a D3D12 path for the CEF web UI (previously D3D11-only) and a way to serve mod-bundled HTML UIs from disk.
Graphics:
GUI:
Utils:
Verified in HogwartsMP (D3D12): interactive fullscreen views, transparent HTML chat overlay served from a local scheme, clean ~1s CEF teardown on game exit. FrameworkTests: 100 tests, the single js_features failure is pre-existing on the base revision.
Known limitations: CPU OSR only on D3D12, full-buffer uploads (no dirty-rect optimization), no HTTP Range support in the disk handler (CefStreamResourceHandler serves whole files), swapchain resize remains a passthrough.
Summary by CodeRabbit
New Features
Improvements
Tests