Fix NGX Lifetime Management and Manual FSR FOV Conversion#875
Open
ZachHembree wants to merge 8 commits intooptiscaler:masterfrom
Open
Fix NGX Lifetime Management and Manual FSR FOV Conversion#875ZachHembree wants to merge 8 commits intooptiscaler:masterfrom
ZachHembree wants to merge 8 commits intooptiscaler:masterfrom
Conversation
- Refactored CreateFeature() and EvaluateFeature() into smaller functions with shared utilities and distinct code paths for internal OptiScaler and passthrough features. - Added documentation to clarify internal implementation details and behavior required by the NGX API. - Added more descriptive error messages. - Fixed a potential memory leak in GetParameters(). Per the DLSS Programming Guide (310.5.0), this deprecated API requires the SDK to manage the lifetime of the returned parameter table, as legacy applications do not free this memory. - Identified a memory management issue: NVNGX_Parameter tables are incorrectly handled in Allocate/Destroy and FeatureProvider. C-style free() is being used on tables allocated with 'new' and on native NGX tables, leading to memory leaks and undefined behavior. Using free() here is actually worse than just willfully leaking memory. This needs fixing.
Problem: The previous implementation incorrectly used C-style free() on NVSDK_NGX_Parameter objects within the DLSS feature providers. This was unsafe for several reasons: - Non-trivial types: These structs often contain internal data/tables that free() does not account for, bypassing necessary destructors. - Ambiguous ownership: Tables were inconsistently sourced from internal allocations (OptiScaler), external NGX API calls, or legacy persistent pointers. - Heap/Stack corruption: Using free() on memory managed by the NGX API or on stack-allocated memory from legacy GetParameters() calls. This risked immediate crashes or heap instability. Solution: Introduced a tagging system to explicitly track memory ownership and determine the correct deallocation strategy. When a destruction request is made, the system now checks the table's tag and acts accordingly: 1. InternPersistent: Internal OptiScaler tables for legacy APIs. Lifetime is managed by the SDK; these persist until process exit. 2. InternDynamic: Internal tables explicitly created/destroyed by the client. Now correctly freed using delete with a static_cast to the implementation type. 3. NVPersistent: Native NGX legacy tables. These are ignored as the native SDK manages their lifetime. 4. NVDynamic: Native NGX modern tables. These are now properly forwarded to the native NGX DestroyParameters() function.
- Started organizing string literals being used as unique identifiers for things like feature selection and parameter lookup into a dedicated header. Less error prone and actually works with intellisense. - The actual usages of these strings have yet to be replaced, but I am using them for new code.
- The conversions for manually configured horizontal FOV to vertical FOV used in upscaling have been corrected to use the following formula: vFov = 2 * arctan( tan( hFov / 2 ) * ( h / w ) ) - Added convenience variables for global singletons to Eval functions to help readability - Streamlined ffxResolveTypelessFormat usages. More readable.
Fixed a preexisting edge case that could cause the NGX handle to be leaked if the D3D12 device could not be acquired.
- Vulkan: Correct default upscaler key from FSR 2.1 to FSR 2.2. - DX11: Removed redundant table initialization in DLSS inputs caused by copy-paste error.
Contributor
Author
|
The PR has been updated to address AI-generated feedback from Copilot and resolve a pre-existing edge case found in
AI Feedback Clarifications
|
Collaborator
|
I'm kinda torn on the OptiKeys idea. On one hands it's nice to clean up all the loose strings but on the other OptiKeys mixes opti-specific ngx params with opti's internal ids. In my opinion it would be best to group them into their own namespaces depending on the role they play. For example OptiNGX::FSR_NearPlane, UpscalerID::XeSS, InputID::VkProvider. It would be awesome to actually get rid of those IDs and replace them with actual enums but that's for the future someone to worry about I suppose :) Beside that, if it works and doesn't have regressions then lgmt |
Contributor
Author
|
Those are valid points. The OptiKeys idea is still pretty open ended at this point. The goal was mainly to provide an easy to integrate baseline that can be gradually integrated and iterated on. I would like namespaced groups or enums myself, but that definitely seems like something for the future. I've done regression testing on DLSS inputs with my 9060 XT on Vulkan, DX11, and DX12 (using BG3, Control, and CP2077), with and without FG where supported. I also traced the new lifetime management logic in the debugger to verify the internal behavior. I didn't find any regressions or issues with NGX parameter lifetime management. |
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
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.
This PR includes fixes for a memory corruption risk in DLSS inputs, corrects a math error in the configurable FSR FOV conversion, and a couple housekeeping items.
Fixed NGX Memory Management: Replaced unsafe C-style
free()calls onNVSDK_NGX_Parameterobjects with a new tagging system. This ensures that memory owned by the NGX API or internal trackers is deallocated correctly, preventing potential heap corruption and crashes.Corrected FSR FOV Conversion: Fixed the math for (all five) users manually setting a custom Horizontal FOV. It now correctly scales to Vertical FOV for the upscaler using the proper trigonometric formula.
DLSS DX12 Input Refactor: In the process of investigating the memory management issue, I ended up cleaning up
CreateFeatureandEvaluateFeatureand adding some inline documentatioin.Code Housekeeping: Started moving string identifiers into a dedicated
OptiTexts.hheader to replace error-prone raw strings literals and enable IntelliSense support. Usages for these identifiers haven't been replaced, but I think it would be a good idea to start using them going forward.