feat(frame generation): improve frame generation quality#1125
Conversation
WalkthroughThis update introduces new API query types and structures for provider versioning, expands frame generation debug and context options, and implements detailed camera info dispatching. It removes frame buffer caching and D3D11 context hooks from the Streamline module, transferring these responsibilities to the Upscaling module, which now manages D3D11 device context hooks and frame buffer caching. Additional Streamline Reflex marker integration and update phase hooks are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant FidelityFX
participant FrameGenContext
App->>FidelityFX: Present()
FidelityFX->>FrameGenContext: Dispatch(framegen parameters)
FidelityFX->>FrameGenContext: Dispatch(camera info: position, orientation)
FrameGenContext-->>FidelityFX: Status
FidelityFX-->>App: Return
sequenceDiagram
participant D3D11Context
participant Upscaling
D3D11Context->>Upscaling: Map(resource)
Upscaling->>Upscaling: If resource == per-frame, store mapped pointer
D3D11Context->>Upscaling: Unmap(resource)
Upscaling->>Upscaling: If resource == per-frame and mapped, CacheFramebuffer()
Upscaling-->>D3D11Context: Call original Unmap
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)src/FidelityFX.cpp (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (4)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Docstrings generation was requested by @doodlum. * #1125 (comment) The following files were modified: * `src/FidelityFX.cpp` * `src/Hooks.cpp` * `src/Streamline.cpp` * `src/Streamline.h` * `src/Upscaling.cpp` * `src/Upscaling.h`
|
Note Generated docstrings for this pull request at #1126 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
src/Hooks.cpp (1)
1081-1084: Consider documenting why these hooks are VR-excluded.The code correctly checks for VR mode before installing the update hooks, but it would be helpful to document why VR doesn't need these Reflex markers.
if (!REL::Module::IsVR()) { + // VR has different frame timing requirements and doesn't use Reflex markers stl::write_thunk_call<Main_Update_Begin>(REL::RelocationID(35565, 36564).address() + REL::Relocate(0x53, 0x6E)); stl::write_thunk_call<Main_Update_Swap>(REL::RelocationID(35565, 36564).address() + REL::Relocate(0x5D2, 0xA92)); }include/ffx_framegeneration.h (1)
170-175: Consider improving comment formatting.The struct implementation is correct, but the comment could be reformatted for better readability.
-//Pass this optional linked struct at FG context creation to enable app to use different hudlessBackBufferformat (IE.RGBA8_UNORM) from backBufferFormat (IE. BGRA8_UNORM) +// Pass this optional linked struct at FG context creation to enable app to use different hudlessBackBufferFormat +// (e.g., RGBA8_UNORM) from backBufferFormat (e.g., BGRA8_UNORM)src/Upscaling.h (1)
201-204: Consider adding documentation for the caching mechanism.The frame buffer caching implementation looks correct, but it would benefit from documentation explaining the lifecycle and usage.
+ // Pointer to the currently mapped frame buffer resource D3D11_MAPPED_SUBRESOURCE* mappedFrameBuffer = nullptr; + // Cached frame buffer data from the last Map/Unmap operation FrameBuffer frameBufferCached{}; + // Caches the current frame buffer data from the mapped resource void CacheFramebuffer();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
features/Frame Generation/SKSE/Plugins/FidelityFX/amd_fidelityfx_dx12.dllis excluded by!**/*.dll
📒 Files selected for processing (10)
include/ffx_api.h(1 hunks)include/ffx_api_types.h(2 hunks)include/ffx_framegeneration.h(6 hunks)include/ffx_framegeneration.hpp(1 hunks)src/FidelityFX.cpp(1 hunks)src/Hooks.cpp(4 hunks)src/Streamline.cpp(3 hunks)src/Streamline.h(0 hunks)src/Upscaling.cpp(1 hunks)src/Upscaling.h(1 hunks)
💤 Files with no reviewable changes (1)
- src/Streamline.h
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (12)
include/ffx_api_types.h (1)
26-26: Good addition for C standard compliance.Adding
<stdbool.h>enables properbooltype usage and boolean literals, improving C standard compliance and type safety.include/ffx_api.h (1)
103-109: Well-structured API extension for provider version querying.The new query descriptor follows established patterns and provides useful provider version information for API introspection. The sequential constant value and struct design are consistent with existing API conventions.
src/FidelityFX.cpp (1)
153-176: Well-implemented camera info dispatch for FSR 3.1.4.The camera info dispatch implementation correctly:
- Extracts camera orientation vectors from the view matrix
- Retrieves camera position from cached frame buffer data
- Follows the established error handling pattern
- Supports the FSR 3.1.4 frame generation improvements mentioned in the PR objectives
The dispatch structure and error logging are consistent with existing code patterns.
include/ffx_framegeneration.hpp (1)
68-76: Consistent C++ wrapper additions for new API structures.The template specializations and struct definitions follow the established patterns in the file and provide proper C++ wrapper support for the new hudless and camera info API structures introduced in FSR 3.1.4.
src/Streamline.cpp (1)
207-209:Details
❓ Verification inconclusive
Verify thread safety when accessing shared framebuffer data.
The code now accesses
globals::upscaling->frameBufferCachedwithout any synchronization mechanism. Since the framebuffer is cached in the Upscaling module's Map/Unmap hooks (potentially on different threads), this could lead to data races.Consider adding proper synchronization or verify that Map/Unmap operations always complete before this code executes:
Also applies to: 215-215
🏁 Script executed:
#!/bin/bash # Search for other accesses to frameBufferCached to understand the access pattern rg -A 5 "frameBufferCached" --type cpp # Check if there's any existing synchronization mechanism rg -A 5 "(mutex|lock|atomic).*frame" --type cppLength of output: 3282
Thread-safety of Upscaling framebuffer access
The
frameBufferCachedstruct is written insrc/Upscaling.cpp(via the Map/Unmap hook/CacheFramebuffer) and then read in multiple places without any locks or atomics:
- src/Streamline.cpp (lines 207–209, 215)
- src/FidelityFX.cpp (similar matrix and position reads)
No
std::mutex,std::lock_guard, orstd::atomicprotecting these reads and writes. If the hook and the Streamline/FidelityFX code ever run on different threads, you risk torn or stale data.• Verify that both the write (Map/Unmap hook) and all reads always occur on the same thread and in a deterministic order.
• If there’s any chance of concurrent execution, introduce a lightweight synchronization aroundframeBufferCached(e.g. astd::mutexor atomic copy with memory fences), or restructure the code so that all access remains single-threaded.src/Hooks.cpp (1)
393-393: LGTM! Architectural change properly implemented.The change from
streamline->InstallHookstoupscaling->InstallD3DHookscorrectly reflects the migration of D3D11 context hooking responsibility to the Upscaling module.include/ffx_framegeneration.h (3)
41-41: LGTM!The debug flag follows the existing bitfield pattern and provides useful runtime validation capability.
51-52: Good practice to reserve flags for future extensibility.The reserved flags follow the proper bitfield pattern and provide room for future features.
72-82: Clean typedef usage improves API clarity.Converting structs to typedefs is a good practice for C APIs, making the types easier to use.
Also applies to: 85-97
src/Upscaling.h (3)
180-180: LGTM!The method declaration follows the class's naming conventions and has an appropriate parameter type.
182-199: Well-structured frame buffer data.The FrameBuffer struct is well-organized with consistent naming conventions for camera transformation data.
206-216: Proper D3D11 hook implementation.The hook structures correctly match the D3D11 DeviceContext interface signatures and follow the established thunk pattern.
| void Upscaling::InstallD3DHooks(ID3D11DeviceContext* a_context) | ||
| { | ||
| stl::detour_vfunc<14, ID3D11DeviceContext_Map>(a_context); | ||
| stl::detour_vfunc<15, ID3D11DeviceContext_Unmap>(a_context); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for virtual function detouring.
The stl::detour_vfunc calls could fail, but there's no error checking or fallback mechanism.
Consider adding error handling:
void Upscaling::InstallD3DHooks(ID3D11DeviceContext* a_context)
{
- stl::detour_vfunc<14, ID3D11DeviceContext_Map>(a_context);
- stl::detour_vfunc<15, ID3D11DeviceContext_Unmap>(a_context);
+ if (!stl::detour_vfunc<14, ID3D11DeviceContext_Map>(a_context)) {
+ logger::error("[Upscaling] Failed to detour ID3D11DeviceContext::Map");
+ }
+ if (!stl::detour_vfunc<15, ID3D11DeviceContext_Unmap>(a_context)) {
+ logger::error("[Upscaling] Failed to detour ID3D11DeviceContext::Unmap");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void Upscaling::InstallD3DHooks(ID3D11DeviceContext* a_context) | |
| { | |
| stl::detour_vfunc<14, ID3D11DeviceContext_Map>(a_context); | |
| stl::detour_vfunc<15, ID3D11DeviceContext_Unmap>(a_context); | |
| } | |
| void Upscaling::InstallD3DHooks(ID3D11DeviceContext* a_context) | |
| { | |
| if (!stl::detour_vfunc<14, ID3D11DeviceContext_Map>(a_context)) { | |
| logger::error("[Upscaling] Failed to detour ID3D11DeviceContext::Map"); | |
| } | |
| if (!stl::detour_vfunc<15, ID3D11DeviceContext_Unmap>(a_context)) { | |
| logger::error("[Upscaling] Failed to detour ID3D11DeviceContext::Unmap"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Upscaling.cpp around lines 783 to 787, the calls to stl::detour_vfunc for
detouring virtual functions lack error handling. Modify the code to check the
return value of each stl::detour_vfunc call, and if it indicates failure, log an
appropriate error message or handle the failure gracefully to ensure the program
can respond properly to detour setup issues.
| HRESULT Upscaling::ID3D11DeviceContext_Map::thunk(ID3D11DeviceContext* This, ID3D11Resource* pResource, UINT Subresource, D3D11_MAP MapType, UINT MapFlags, D3D11_MAPPED_SUBRESOURCE* pMappedResource) | ||
| { | ||
| HRESULT hr = func(This, pResource, Subresource, MapType, MapFlags, pMappedResource); | ||
| if (hr == S_OK) { | ||
| if (*globals::game::perFrame.get() == pResource) | ||
| globals::upscaling->mappedFrameBuffer = pMappedResource; | ||
| } | ||
| return hr; | ||
| } |
There was a problem hiding this comment.
Add null pointer checks for global pointers.
The code accesses globals::game::perFrame and globals::upscaling without verifying they are valid.
HRESULT Upscaling::ID3D11DeviceContext_Map::thunk(ID3D11DeviceContext* This, ID3D11Resource* pResource, UINT Subresource, D3D11_MAP MapType, UINT MapFlags, D3D11_MAPPED_SUBRESOURCE* pMappedResource)
{
HRESULT hr = func(This, pResource, Subresource, MapType, MapFlags, pMappedResource);
if (hr == S_OK) {
- if (*globals::game::perFrame.get() == pResource)
- globals::upscaling->mappedFrameBuffer = pMappedResource;
+ if (globals::game::perFrame && globals::upscaling && *globals::game::perFrame.get() == pResource)
+ globals::upscaling->mappedFrameBuffer = pMappedResource;
}
return hr;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| HRESULT Upscaling::ID3D11DeviceContext_Map::thunk(ID3D11DeviceContext* This, ID3D11Resource* pResource, UINT Subresource, D3D11_MAP MapType, UINT MapFlags, D3D11_MAPPED_SUBRESOURCE* pMappedResource) | |
| { | |
| HRESULT hr = func(This, pResource, Subresource, MapType, MapFlags, pMappedResource); | |
| if (hr == S_OK) { | |
| if (*globals::game::perFrame.get() == pResource) | |
| globals::upscaling->mappedFrameBuffer = pMappedResource; | |
| } | |
| return hr; | |
| } | |
| HRESULT Upscaling::ID3D11DeviceContext_Map::thunk(ID3D11DeviceContext* This, ID3D11Resource* pResource, UINT Subresource, D3D11_MAP MapType, UINT MapFlags, D3D11_MAPPED_SUBRESOURCE* pMappedResource) | |
| { | |
| HRESULT hr = func(This, pResource, Subresource, MapType, MapFlags, pMappedResource); | |
| if (hr == S_OK) { | |
| if (globals::game::perFrame && globals::upscaling && *globals::game::perFrame.get() == pResource) | |
| globals::upscaling->mappedFrameBuffer = pMappedResource; | |
| } | |
| return hr; | |
| } |
🤖 Prompt for AI Agents
In src/Upscaling.cpp around lines 789 to 797, the code uses
globals::game::perFrame and globals::upscaling without checking if these
pointers are null. Add null pointer checks before dereferencing these globals to
ensure they are valid. Specifically, verify globals::game::perFrame.get() and
globals::upscaling are not null before accessing their members to prevent
potential crashes.
| void Upscaling::ID3D11DeviceContext_Unmap::thunk(ID3D11DeviceContext* This, ID3D11Resource* pResource, UINT Subresource) | ||
| { | ||
| if (*globals::game::perFrame.get() == pResource && globals::upscaling->mappedFrameBuffer) | ||
| globals::upscaling->CacheFramebuffer(); | ||
| func(This, pResource, Subresource); | ||
| } |
There was a problem hiding this comment.
Add null pointer checks for global pointers.
Similar to the Map thunk, this code needs defensive null checks.
void Upscaling::ID3D11DeviceContext_Unmap::thunk(ID3D11DeviceContext* This, ID3D11Resource* pResource, UINT Subresource)
{
- if (*globals::game::perFrame.get() == pResource && globals::upscaling->mappedFrameBuffer)
- globals::upscaling->CacheFramebuffer();
+ if (globals::game::perFrame && globals::upscaling &&
+ *globals::game::perFrame.get() == pResource && globals::upscaling->mappedFrameBuffer)
+ globals::upscaling->CacheFramebuffer();
func(This, pResource, Subresource);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void Upscaling::ID3D11DeviceContext_Unmap::thunk(ID3D11DeviceContext* This, ID3D11Resource* pResource, UINT Subresource) | |
| { | |
| if (*globals::game::perFrame.get() == pResource && globals::upscaling->mappedFrameBuffer) | |
| globals::upscaling->CacheFramebuffer(); | |
| func(This, pResource, Subresource); | |
| } | |
| void Upscaling::ID3D11DeviceContext_Unmap::thunk(ID3D11DeviceContext* This, ID3D11Resource* pResource, UINT Subresource) | |
| { | |
| if (globals::game::perFrame && globals::upscaling && | |
| *globals::game::perFrame.get() == pResource && globals::upscaling->mappedFrameBuffer) | |
| globals::upscaling->CacheFramebuffer(); | |
| func(This, pResource, Subresource); | |
| } |
🤖 Prompt for AI Agents
In src/Upscaling.cpp around lines 799 to 804, the function uses global pointers
without checking if they are null, which can cause crashes. Add null pointer
checks for globals::game::perFrame and globals::upscaling before dereferencing
or accessing their members. Only proceed with the CacheFramebuffer call if these
pointers are valid to ensure safe execution.
| void Upscaling::CacheFramebuffer() | ||
| { | ||
| auto frameBuffer = (FrameBuffer*)mappedFrameBuffer->pData; | ||
| frameBufferCached = *frameBuffer; | ||
| mappedFrameBuffer = nullptr; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding size validation and thread safety.
The CacheFramebuffer method assumes the mapped data size matches the FrameBuffer struct size, which could lead to buffer overruns if assumptions are incorrect.
Consider validating the data size and adding thread safety:
void Upscaling::CacheFramebuffer()
{
+ std::lock_guard<std::mutex> lock(frameBufferMutex); // Add mutex member
auto frameBuffer = (FrameBuffer*)mappedFrameBuffer->pData;
frameBufferCached = *frameBuffer;
mappedFrameBuffer = nullptr;
}Run this script to check the FrameBuffer struct definition:
🏁 Script executed:
#!/bin/bash
# Find the FrameBuffer struct definition to understand its size
ast-grep --pattern 'struct FrameBuffer {
$$$
}'
# Check if there's a size field in D3D11_MAPPED_SUBRESOURCE
rg -A 10 "D3D11_MAPPED_SUBRESOURCE" --type cppLength of output: 3663
I’ll locate the FrameBuffer type definition to confirm its layout and size before finalizing the recommendation.
🏁 Script executed:
#!/bin/bash
# Search for FrameBuffer struct or class definition in headers and source
rg -n "struct FrameBuffer" -t cpp -t h -A5 -B5
rg -n "class FrameBuffer" -t cpp -t h -A5 -B5Length of output: 505
Couldn’t locate the full FrameBuffer definition above. Let’s dump a larger range around its declaration to verify its size and layout:
🏁 Script executed:
#!/bin/bash
# Show full FrameBuffer struct for size/layout inspection
sed -n '180,260p' src/Upscaling.hLength of output: 1149
Add data-pointer validation, size checks, and thread safety in CacheFramebuffer
Currently, CacheFramebuffer() unconditionally casts and dereferences mappedFrameBuffer->pData and updates shared members without synchronization. This can lead to UB if mapping failed or if the underlying buffer size doesn’t match sizeof(FrameBuffer), and to data races if called from multiple threads.
Suggested changes around src/Upscaling.cpp Lines ~806–811:
- Introduce a
std::mutex frameBufferMutex;(in Upscaling.hpp) - In
CacheFramebuffer():void Upscaling::CacheFramebuffer() {
- std::lock_guardstd::mutex lock(frameBufferMutex);
- if (!mappedFrameBuffer || !mappedFrameBuffer->pData) {
-
return; // nothing to cache - }
- // Ensure the mapped resource’s byte width (captured in Map hook via GetDesc)
- // is at least sizeof(FrameBuffer)
- if (bufferByteWidth < sizeof(FrameBuffer)) {
-
return; // mapped data too small - }
auto frameBuffer = reinterpret_cast<FrameBuffer*>(mappedFrameBuffer->pData);
frameBufferCached = *frameBuffer;
mappedFrameBuffer = nullptr;
}
- In your Map hook (ID3D11DeviceContext_Map::thunk), capture both the mapped subresource **and** the resource’s `ByteWidth` (via `ID3D11Buffer::GetDesc`) and store them in `Upscaling` so `CacheFramebuffer` can perform the size check.
These additions will prevent buffer overruns and data-race conditions.
> Committable suggestion skipped: line range outside the PR's diff.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In src/Upscaling.cpp around lines 806 to 811, the CacheFramebuffer method casts
and dereferences mappedFrameBuffer->pData without validating the pointer or
verifying the data size matches sizeof(FrameBuffer), risking buffer overruns and
undefined behavior. Additionally, it updates shared members without
synchronization, causing potential data races. To fix this, add a std::mutex
frameBufferMutex in Upscaling.hpp and lock it in CacheFramebuffer to ensure
thread safety. Modify the Map hook to capture the mapped subresource and the
resource's ByteWidth via ID3D11Buffer::GetDesc, store these in Upscaling, and in
CacheFramebuffer validate that the mapped data size is at least
sizeof(FrameBuffer) before copying. Also, check that mappedFrameBuffer and pData
are not null before accessing them.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| struct Main_Update_Swap | ||
| { | ||
| static void thunk(void* This) | ||
| { | ||
| if (globals::streamline->featureReflex) { | ||
| sl::FrameToken* frameToken; | ||
| globals::streamline->slGetNewFrameToken(frameToken, &globals::state->frameCount); | ||
| globals::streamline->slReflexSetMarker(sl::ReflexMarker::eSimulationEnd, *frameToken); | ||
| globals::streamline->slReflexSetMarker(sl::ReflexMarker::eRenderSubmitStart, *frameToken); | ||
| } | ||
| func(This); | ||
| } | ||
| static inline REL::Relocation<decltype(thunk)> func; | ||
| }; |
There was a problem hiding this comment.
Add null pointer check for globals::streamline.
Similar to Main_Update_Begin, this needs defensive null checking.
static void thunk(void* This)
{
- if (globals::streamline->featureReflex) {
+ if (globals::streamline && globals::streamline->featureReflex) {
sl::FrameToken* frameToken;
globals::streamline->slGetNewFrameToken(frameToken, &globals::state->frameCount);
globals::streamline->slReflexSetMarker(sl::ReflexMarker::eSimulationEnd, *frameToken);
globals::streamline->slReflexSetMarker(sl::ReflexMarker::eRenderSubmitStart, *frameToken);
}
func(This);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| struct Main_Update_Swap | |
| { | |
| static void thunk(void* This) | |
| { | |
| if (globals::streamline->featureReflex) { | |
| sl::FrameToken* frameToken; | |
| globals::streamline->slGetNewFrameToken(frameToken, &globals::state->frameCount); | |
| globals::streamline->slReflexSetMarker(sl::ReflexMarker::eSimulationEnd, *frameToken); | |
| globals::streamline->slReflexSetMarker(sl::ReflexMarker::eRenderSubmitStart, *frameToken); | |
| } | |
| func(This); | |
| } | |
| static inline REL::Relocation<decltype(thunk)> func; | |
| }; | |
| struct Main_Update_Swap | |
| { | |
| static void thunk(void* This) | |
| { | |
| if (globals::streamline && globals::streamline->featureReflex) { | |
| sl::FrameToken* frameToken; | |
| globals::streamline->slGetNewFrameToken(frameToken, &globals::state->frameCount); | |
| globals::streamline->slReflexSetMarker(sl::ReflexMarker::eSimulationEnd, *frameToken); | |
| globals::streamline->slReflexSetMarker(sl::ReflexMarker::eRenderSubmitStart, *frameToken); | |
| } | |
| func(This); | |
| } | |
| static inline REL::Relocation<decltype(thunk)> func; | |
| }; |
🤖 Prompt for AI Agents
In src/Hooks.cpp around lines 487 to 500, add a null pointer check for
globals::streamline before accessing its members in the thunk function of
Main_Update_Swap. This means verifying globals::streamline is not null before
calling slGetNewFrameToken and slReflexSetMarker to prevent potential crashes.
Wrap the existing code that uses globals::streamline inside an if statement that
checks for its validity.
| struct Main_Update_Begin | ||
| { | ||
| static void thunk(RE::PlayerCharacter* a_player) | ||
| { | ||
| if (globals::streamline->featureReflex) { | ||
| sl::FrameToken* frameToken; | ||
| globals::streamline->slGetNewFrameToken(frameToken, &globals::state->frameCount); | ||
| globals::streamline->slReflexSetMarker(sl::ReflexMarker::eInputSample, *frameToken); | ||
| globals::streamline->slReflexSetMarker(sl::ReflexMarker::eSimulationStart, *frameToken); | ||
| } | ||
| func(a_player); | ||
| } | ||
| static inline REL::Relocation<decltype(thunk)> func; | ||
| }; |
There was a problem hiding this comment.
Add null pointer check for globals::streamline.
The thunk accesses globals::streamline without verifying it's valid.
static void thunk(RE::PlayerCharacter* a_player)
{
- if (globals::streamline->featureReflex) {
+ if (globals::streamline && globals::streamline->featureReflex) {
sl::FrameToken* frameToken;
globals::streamline->slGetNewFrameToken(frameToken, &globals::state->frameCount);
globals::streamline->slReflexSetMarker(sl::ReflexMarker::eInputSample, *frameToken);
globals::streamline->slReflexSetMarker(sl::ReflexMarker::eSimulationStart, *frameToken);
}
func(a_player);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| struct Main_Update_Begin | |
| { | |
| static void thunk(RE::PlayerCharacter* a_player) | |
| { | |
| if (globals::streamline->featureReflex) { | |
| sl::FrameToken* frameToken; | |
| globals::streamline->slGetNewFrameToken(frameToken, &globals::state->frameCount); | |
| globals::streamline->slReflexSetMarker(sl::ReflexMarker::eInputSample, *frameToken); | |
| globals::streamline->slReflexSetMarker(sl::ReflexMarker::eSimulationStart, *frameToken); | |
| } | |
| func(a_player); | |
| } | |
| static inline REL::Relocation<decltype(thunk)> func; | |
| }; | |
| struct Main_Update_Begin | |
| { | |
| static void thunk(RE::PlayerCharacter* a_player) | |
| { | |
| if (globals::streamline && globals::streamline->featureReflex) { | |
| sl::FrameToken* frameToken; | |
| globals::streamline->slGetNewFrameToken(frameToken, &globals::state->frameCount); | |
| globals::streamline->slReflexSetMarker(sl::ReflexMarker::eInputSample, *frameToken); | |
| globals::streamline->slReflexSetMarker(sl::ReflexMarker::eSimulationStart, *frameToken); | |
| } | |
| func(a_player); | |
| } | |
| static inline REL::Relocation<decltype(thunk)> func; | |
| }; |
🤖 Prompt for AI Agents
In src/Hooks.cpp around lines 472 to 485, the thunk function accesses
globals::streamline without checking if it is null, which can cause a crash. Add
a null pointer check before accessing globals::streamline to ensure it is valid.
Only proceed with the existing logic if globals::streamline is not null.
| #define FFX_API_DISPATCH_DESC_TYPE_FRAMEGENERATION_PREPARE_CAMERAINFO 0x0002000au | ||
| //Link this struct after ffxDispatchDescFrameGenerationPrepare. This is a required input to FSR3.1.4 and onwards for best quality. | ||
| struct ffxDispatchDescFrameGenerationPrepareCameraInfo | ||
| { | ||
| ffxConfigureDescHeader header; | ||
| float cameraPosition[3]; ///< The camera position in world space | ||
| float cameraUp[3]; ///< The camera up normalized vector in world space. | ||
| float cameraRight[3]; ///< The camera right normalized vector in world space. | ||
| float cameraForward[3]; ///< The camera forward normalized vector in world space. | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the header type usage.
The struct uses ffxConfigureDescHeader while being a dispatch descriptor type. This might be intentional for chaining, but please verify this is the correct header type.
🏁 Script executed:
#!/bin/bash
# Description: Check if other dispatch descriptor types use ffxConfigureDescHeader for chaining
# Search for other dispatch descriptor types and their header usage
ast-grep --pattern 'struct $STRUCT {
$HEADER header;
$$$
}' | grep -B1 -A1 "FFX_API_DISPATCH_DESC_TYPE"Length of output: 107
I’ll check all dispatch descriptor definitions and inspect their header fields for consistency:
🏁 Script executed:
#!/bin/bash
# List all dispatch descriptor macros and the next line (struct name)
rg "#define FFX_API_DISPATCH_DESC_TYPE" -n -A1
echo "----"
# Show each dispatch descriptor struct definition and its first few lines to see which header type is used
rg "struct ffxDispatchDesc" -n include -A5Length of output: 3375
Use ffxDispatchDescHeader for dispatch descriptors
The camera-info dispatch struct is the only FFX_API_DISPATCH_DESC_TYPE_* that uses ffxConfigureDescHeader—all other dispatch descriptors use ffxDispatchDescHeader. To keep the chaining consistent, update the header type:
• File: include/ffx_framegeneration.h
Lines: ~181
Change:
- ffxConfigureDescHeader header;
+ ffxDispatchDescHeader header;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #define FFX_API_DISPATCH_DESC_TYPE_FRAMEGENERATION_PREPARE_CAMERAINFO 0x0002000au | |
| //Link this struct after ffxDispatchDescFrameGenerationPrepare. This is a required input to FSR3.1.4 and onwards for best quality. | |
| struct ffxDispatchDescFrameGenerationPrepareCameraInfo | |
| { | |
| ffxConfigureDescHeader header; | |
| float cameraPosition[3]; ///< The camera position in world space | |
| float cameraUp[3]; ///< The camera up normalized vector in world space. | |
| float cameraRight[3]; ///< The camera right normalized vector in world space. | |
| float cameraForward[3]; ///< The camera forward normalized vector in world space. | |
| }; | |
| #define FFX_API_DISPATCH_DESC_TYPE_FRAMEGENERATION_PREPARE_CAMERAINFO 0x0002000au | |
| //Link this struct after ffxDispatchDescFrameGenerationPrepare. This is a required input to FSR3.1.4 and onwards for best quality. | |
| struct ffxDispatchDescFrameGenerationPrepareCameraInfo | |
| { | |
| ffxDispatchDescHeader header; | |
| float cameraPosition[3]; ///< The camera position in world space | |
| float cameraUp[3]; ///< The camera up normalized vector in world space. | |
| float cameraRight[3]; ///< The camera right normalized vector in world space. | |
| float cameraForward[3]; ///< The camera forward normalized vector in world space. | |
| }; |
🤖 Prompt for AI Agents
In include/ffx_framegeneration.h around lines 177 to 186, the struct
ffxDispatchDescFrameGenerationPrepareCameraInfo currently uses
ffxConfigureDescHeader as its header type, but all other dispatch descriptor
structs use ffxDispatchDescHeader for chaining consistency. Update the header
type in this struct from ffxConfigureDescHeader to ffxDispatchDescHeader to
align with the other dispatch descriptors.
Docstrings generation was requested by @doodlum. * #1125 (comment) The following files were modified: * `src/FidelityFX.cpp` * `src/Hooks.cpp` * `src/Streamline.cpp` * `src/Streamline.h` * `src/Upscaling.cpp` * `src/Upscaling.h` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/Upscaling.cpp (4)
795-799: Add error handling for virtual function detouring.The
stl::detour_vfunccalls could fail, but there's no error checking or fallback mechanism.
808-816: Add null pointer checks for global pointers.The code accesses
globals::game::perFrameandglobals::upscalingwithout verifying they are valid.
823-828: Add null pointer checks for global pointers.Similar to the Map thunk, this code needs defensive null checks.
835-840: Add data-pointer validation, size checks, and thread safety in CacheFramebufferCurrently,
CacheFramebuffer()unconditionally casts and dereferencesmappedFrameBuffer->pDataand updates shared members without synchronization. This can lead to UB if mapping failed or if the underlying buffer size doesn't matchsizeof(FrameBuffer), and to data races if called from multiple threads.src/Hooks.cpp (3)
237-249: Potential frame token mismatch with multiple slGetNewFrameToken calls.The code calls
slGetNewFrameTokenhere and also inMain_Update_BeginandMain_Update_Swap, which could lead to mismatched frame tokens for the same frame.
498-507: Add null pointer check for globals::streamline.The thunk accesses
globals::streamlinewithout verifying it's valid.
518-527: Add null pointer check for globals::streamline.Similar to Main_Update_Begin, this needs defensive null checking.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/FidelityFX.cpp(2 hunks)src/Hooks.cpp(7 hunks)src/Streamline.cpp(4 hunks)src/Streamline.h(1 hunks)src/Upscaling.cpp(2 hunks)src/Upscaling.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/FidelityFX.cpp
- src/Upscaling.h
- src/Streamline.h
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
| auto viewMatrix = globals::upscaling->frameBufferCached.CameraViewInverse.Transpose(); | ||
| auto cameraViewToClip = globals::upscaling->frameBufferCached.CameraProjUnjittered.Transpose(); |
There was a problem hiding this comment.
Add null pointer check for globals::upscaling.
The code accesses globals::upscaling->frameBufferCached without verifying that globals::upscaling is valid, which could lead to a crash if the upscaling module is not initialized.
- auto viewMatrix = globals::upscaling->frameBufferCached.CameraViewInverse.Transpose();
- auto cameraViewToClip = globals::upscaling->frameBufferCached.CameraProjUnjittered.Transpose();
+ if (!globals::upscaling) {
+ logger::error("[Streamline] Upscaling module not initialized");
+ return;
+ }
+ auto viewMatrix = globals::upscaling->frameBufferCached.CameraViewInverse.Transpose();
+ auto cameraViewToClip = globals::upscaling->frameBufferCached.CameraProjUnjittered.Transpose();Also update line 220:
- slConstants.cameraPos = *(sl::float3*)&globals::upscaling->frameBufferCached.CameraPosAdjust;
+ slConstants.cameraPos = *(sl::float3*)&globals::upscaling->frameBufferCached.CameraPosAdjust;Also applies to: 220-220
🤖 Prompt for AI Agents
In src/Streamline.cpp around lines 212 to 213 and line 220, add a null pointer
check for globals::upscaling before accessing its members. Verify that
globals::upscaling is not null to prevent potential crashes if the upscaling
module is uninitialized. Wrap the existing code that uses
globals::upscaling->frameBufferCached with an if statement checking for its
validity.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
✅ A pre-release build is available for this PR: |
3 similar comments
|
✅ A pre-release build is available for this PR: |
|
✅ A pre-release build is available for this PR: |
|
✅ A pre-release build is available for this PR: |
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Generally I'd keep the docstrings in the .h.
|
|
||
| void InstallD3DHooks(ID3D11DeviceContext* a_context); | ||
|
|
||
| struct FrameBuffer |
There was a problem hiding this comment.
Is this structure flat only?
|
I'm going to approve this since my comments aren't blocking. |
…haders#1125) * feat(frame generation): update to latest FSR * fix: errors * fix: use correct streamline frame markers * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * docs: add docstrings to fg-updates (community-shaders#1126) Docstrings generation was requested by @doodlum. * community-shaders#1125 (comment) The following files were modified: * `src/FidelityFX.cpp` * `src/Hooks.cpp` * `src/Streamline.cpp` * `src/Streamline.h` * `src/Upscaling.cpp` * `src/Upscaling.h` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: remove statics --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…haders#1125) * feat(frame generation): update to latest FSR * fix: errors * fix: use correct streamline frame markers * style: 🎨 apply pre-commit.ci formatting Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details. * docs: add docstrings to fg-updates (community-shaders#1126) Docstrings generation was requested by @doodlum. * community-shaders#1125 (comment) The following files were modified: * `src/FidelityFX.cpp` * `src/Hooks.cpp` * `src/Streamline.cpp` * `src/Streamline.h` * `src/Upscaling.cpp` * `src/Upscaling.h` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * chore: remove statics --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Updates FSR to 3.1.4 and includes the camera struct for future-proofing and quality improvements.
Implements proper frame markers for Streamline/DLSSG
Moved frame buffer capture hooks to share between classes.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Refactor