Skip to content

Native Vulkan upscaler selection with DLSS, XeSS, and FSR 3 - #5

Merged
NightHammer1000 merged 11 commits into
mainfrom
codex/upscaler-native
Sep 26, 2026
Merged

NightHammer1000 merged 11 commits into
mainfrom
codex/upscaler-native

Conversation

@NightHammer1000

@NightHammer1000 NightHammer1000 commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Add a Native Vulkan upscaler path with dynamic DLSS Super Resolution, XeSS, and FSR 3 selection. Reflex and frame generation remain follow-up work.
  • Keep the world render target separate from the display-size post chain and UI. Restore native TAA when upscaling is off.
  • Stabilize GTAO before temporal reconstruction and make the AO debug view show the AO that reaches the screen.
  • Retain clipped TAA history only at sparse distant depth edges such as subpixel foliage. Solid silhouettes and flat disocclusions reset normally; current-frame glow avoids stale bloom ghosts.
  • Bring the headless capture harness into this branch for unattended image and attachment debugging.

Review follow-up

  • Guard NGX session and feature lifetimes, including dispose/evaluate races; bound native extension lists and make backend startup failures independent.
  • Use the shared LOD-bias rule for all three vendors and handle invalid mutable upscaler settings at their consumption points.
  • Restore every existing Vulkan feature-chain node when XeSS feature negotiation fails, with a regression test.
  • Test two available requirement contributors in the same chain and run the Vulkan renderer test project in PR CI.
  • Update the settings-tab assertion for the added Vulkan tab.

Verification

  • Windows PR CI passed: 732 general tests, 50 launcher tests, and 280 Vulkan tests; the Vulkan suite skipped 296 hardware-dependent cases on the hosted runner.
  • dotnet build VintageStory.slnx -c Release --no-restore passed locally.
  • Optimum.Tests: 732 passed, 34 skipped locally.
  • Focused Vulkan review and SSAO tests: 17 passed; TAA resolve tests: 19 passed.
  • Headless Vulkan capture with upscalers off and TAA on: distant leaf hard resets 1.03%, below the 1.50% gate. The original 3x3 depth mismatch rate was 2.68%.
  • The TAA and AO fixes are also pushed to feat/vulkan-taa; the sparse-edge refinement is at 57cefab0.

Review notes

  • FSR 3 is the Vulkan fallback path. FSR 4, Reflex, and frame generation are outside this PR.
  • Vendor pixel tests require their SDK runtimes and GPU support; CI runs the available tests and reports those skips.
  • Please also check the supplied in-game viewpoint during review.

@NightHammer1000

Copy link
Copy Markdown
Collaborator Author

@claude review and report every single finding you make

@claude

claude Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Claude finished @NightHammer1000's task in 2m 8s —— View job


Review complete — full findings report

Reviewed the entire diff (git diff origin/main HEAD, 81 files, +10,595/-538) across native interop, render graph, shaders, config/patches, and tests. All todos below are done; findings are grouped by severity.

Todo list:

  • Review native upscaler backend selection & lifecycle
  • Review NGX interop / native bridge code
  • Review FSR3 native bridge
  • Review XeSS native wrapper
  • Review render graph / frame buffer / post-chain changes
  • Review TAA / GTAO shader and resolve changes
  • Review config/patch files
  • Review test coverage added
  • Compile and report all findings

High severity

1. NGX use-after-free: Dispose() cannot null its own readonly pointer fields
Optimum.Render.Vulkan/Upscale/Ngx/NgxSession.cs:33-35, 371-378 — _projectId, _engineVersion, _applicationDataPath are readonly IntPtr fields allocated in the ctor and freed via Marshal.FreeHGlobal in Dispose(), but being readonly they can't be zeroed. Any later call to Discovery()/Initialize() on a disposed session hands NGX dangling pointers. No ObjectDisposedException guard exists anywhere in the class.

2. No synchronization between Dispose() and in-flight Evaluate()/Discovery() (native use-after-free / double-free)
Optimum.Render.Vulkan/Upscale/Ngx/NgxDlssFeature.cs:254-344, NgxSession.cs Dispose — Dispose() uses the classic non-atomic if (_disposed) return; _disposed = true; with no lock, and Evaluate() doesn't check disposal state either. Two threads racing Dispose()/Dispose() double-frees the native handle; Dispose() racing Evaluate() frees NGX objects while evaluate is still writing into them. Only NgxLifetimeOwner (a different type in this family) actually takes a lock.

3. XeSS mip-LOD bias is missing the shared sharpening offset every other backend applies
Optimum.Render.Vulkan/Upscale/XessBackend.cs:115-116 computes LodBias = MathF.Log2(renderScale), but Fsr3Backend.cs:78-79 and the OptimumConfig default both subtract 1 (UpscalerLodBiasOffset). XeSS textures will be visibly blurrier than DLSS/FSR3 at identical render scale. Worse: XessTests.cs:141 asserts the buggy formula, so the test enshrines the bug instead of catching it (contrast Fsr3Tests.cs:51, which asserts the correct -1f form).

4. OptimumConfig.UpscalerQuality = null crashes the render thread for XeSS/FSR3
Optimum.Render.Vulkan/Upscale/XessBackend.cs:102-106 and Fsr3Backend.cs:66-70 — QualityOf(string quality)'s switch has no null guard. OptimumConfig.UpscalerQuality (sources/VintagestoryApi/Config/OptimumConfig.cs:550) is a public mutable static field with validation only in Load(); any mod (or bad config round-trip) setting it directly to null throws an unhandled NullReferenceException from TryPlanUpscale on the next resize/frame-buffer rebuild. DlssUpscaler.QualityOf (DlssUpscaler.cs:353-358) already handles this safely via string.Equals(..., OrdinalIgnoreCase) — the three vendor paths are inconsistently hardened against the same documented hazard.

5. distantDepthEdge TAA heuristic is not specific to transient foliage — it suppresses hard resets at any permanent distant edge
sources/shaders/taa-resolve.fsh:273-281, sources/shaders-vk/taa-resolve.glsl:336-344 — distantDepthEdge = closestLinearDepth > 20.0 && farthestDepth - closestDepth > 2e-4 is true for a one-frame subpixel leaf flicker, but equally true for a permanent silhouette (rooftop, cliff, tree line) beyond 20 blocks. Nothing distinguishes "transient thin geometry" from "stable depth discontinuity." Failure: panning past any real distant edge keeps colour-clipped stale history + raw current-frame glow instead of a clean disocclusion reset → persistent ghosting/smear trail. No test exercises a genuine permanent distant-edge disocclusion (existing tests use depth-uniform "flat" cases, never a real edge), and scripts/dev/taa-rejection.py's gate metric subtracts distant_edge out of the numerator before the pass/fail threshold, so broadening this shader condition mechanically drives the reported "hard reset rate" toward 0% — the parity guard can no longer catch a regression here.


Medium severity

  1. BringUpUpscaler() has no per-backend exception isolation (VulkanClientPlatform.Upscale.cs:66-73) — unlike ShutDownUpscaler(), which wraps each backend's Shutdown() in its own try/catch, a failing backend's BringUp/Shutdown exception here propagates out and aborts the entire InstallGraphics call, falling back to OpenGL entirely instead of just disabling one upscaler. This also breaks the documented invariant in NgxLifetime.cs:200-217 that shutdown exceptions must never escape.
  2. Unvalidated driver-supplied count indexes native memory — NgxSession.cs:337-341 (Collect) loops properties[i] for i < count where count comes straight from the NGX/driver extension-requirements call, with no upper bound before dereferencing. A malformed/mismatched-ABI NGX runtime returning a bogus large count causes an OOB native read per extension name.
  3. Contradictory IsHdr documentation — likely a real colour-format bug — NgxDlssFeature.cs:14 says colour is LDR RGBA8 so HDR "stays off," but ContractFlags (lines 52-54) sets IsHdr unconditionally and its own comment claims linear RGBA16F requires it. Whichever is stale, NGX may be misinterpreting the colour range, producing silently wrong exposure in DLSS output — worth reconciling against the actual attachment format passed to NgxResourceVk.Texture in VulkanDevice.Dlss.cs:282.
  4. Unfiltered glow at persistent distant-edge pixels — when finding Native Vulkan upscaler selection with DLSS, XeSS, and FSR 3 #5's branch fires every frame at a stable edge, historyGlowSample = glow (fsh:not-clipped path) means glow is never temporally accumulated at that pixel, trading "stale bloom ghost" for raw per-frame glow flicker/aliasing — one of the exact artifacts TAA exists to remove.
  5. XessBackend.FinalizeDeviceFeatures chain-restore is fragile and untested on the failure path (XessBackend.cs:61-87) — restores only the three struct types/PNext links it snapshotted; if native code touches any other chained struct before failing, that mutation isn't undone, and no test forces this native call to fail.
  6. The FSR3/XeSS tests that would actually validate the PR's pixel/context claims never run in this repo's CI — Fsr3Tests.SignedVulkanRuntimeProducesPixelsAndRecreatesContexts and the equivalent XeSS tests are [SkippableFact] gated on real hardware + proprietary SDK DLLs being staged; grep of .github/workflows shows Optimum.Render.Vulkan.Tests isn't built/run by any workflow at all. Only ABI/struct-layout asserts and trivial config-state tests actually execute in CI.
  7. UpscalerDeviceRequirementsTests.cs only tests the "everything refused" path — never exercises an accepted extension, FinalizeDeviceFeatures chain composition, or two contributors (e.g. DLSS+XeSS) chaining simultaneously, which IDeviceRequirementContributor.cs:84-85's own doc comment cites as the reason the chain design exists.
  8. OptimumConfig.Upscaler/UpscalerQuality are unvalidated public mutable fields (OptimumConfig.cs:549-550) — root cause of finding DLSS frame generation groundwork: HUD-less frame, UI target, OpenGL and Cecil fixes #4; any external caller can bypass Load()'s validation entirely.
  9. disabledUpscalers HashSet is unsynchronized (OptimumConfig.cs:552-560) — DisableUpscalerAtRuntime()/IsUpscalerDisabled() do check-then-add with no lock; DlssUpscaler.cs:526 calls this from native-failure handling, a plausible off-render-thread path, risking a corrupted HashSet or inconsistent render-scale/LOD-bias pair for a frame.
  10. BuildFsr3Bridge MSBuild target can hard-fail the whole build; the NGX shim target cannot (Optimum.Render.Vulkan.csproj:104-146) — the NGX build scripts degrade gracefully (missing compiler → warn + exit 0), but native/optimum-fsr3/build.ps1 uses $ErrorActionPreference='Stop'/throws on a partial SDK checkout or missing g++, with no ContinueOnError on the MSBuild target — turning "FSR3 unavailable" into "the whole dotnet build fails," unlike every other optional native SDK in this PR.
  11. Stale SSAO screenSize math never follows the upscaler's render scale (VulkanClientPlatform.NativeSsao.cs:74-90, 137-159) — still derives "render resolution" from ssaaLevel/ClientSettings.SSAA (the older FSR/SSAA setting), while the actual G-buffer resolution now comes from TryPlanUpscale's planned width/height, an independent value. Setting AmbientOcclusion = "vanilla" with an upscaler active samples/reconstructs AO at the wrong scale.
  12. Stale "AO applied" latch never resets (patches/.../ClientPlatformWindows.cs.patch, optimumSsaoInScene) — set true in ApplyOptimumSceneSsao but never reset to false anywhere. Any RebuildFrameBuffers() (including the default handler fired on every upscaler/quality dropdown change) can skip a frame or two of TAA-history-gated SSAO application while the stale flag still reports "AO already in scene," so the final composite silently skips applying AO — SSAO visibly disappears for a few frames after every upscaler/quality change or resize.

Low severity

  1. Fsr3GetDeviceProcAddr only patches one KHR-promoted entry point (vkGetBufferMemoryRequirements2KHR); any other KHR alias the signed FFX binary queries returns null with no downstream null-check → crash via null function pointer (native/optimum-fsr3/bridge.cpp:41-48).
  2. Check-then-act race on the native-library-singleton cache in Fsr3Native.cs:32-59/XessNative.cs:36-48 (not lock-protected, not volatile); worst case is a leaked NativeLibrary handle on concurrent init (neither class is IDisposable).
  3. No thread-affinity enforcement between Evaluate() and context teardown in Fsr3Backend.cs:105-112/XessBackend.cs:141-148 — relies entirely on GPU-side deferred deletion, not CPU-side mutual exclusion.
  4. XessBackend.cs:45,58 — Marshal.PtrToStringUTF8(...)! and unindexed names[] access with no null-check if the SDK ever returns a null entry.
  5. Fsr3Native/XessNative.TryLoad re-attempt NativeLibrary.Load and re-log on every PrepareUpscaler() cycle when the DLL is genuinely absent — log spam / wasted I/O, not a correctness bug.
  6. [UnmanagedCallersOnly] NGX log callback (NgxSession.cs:76-81) has no try/catch; any exception escaping a callback invoked from native code fails the whole process rather than being catchable.
  7. NgxFeatureRequirement.MinOsVersion NUL-termination relies incidentally on default zero-init rather than an explicit contract (NgxInterop.cs:153-160); would only bite if a future refactor stack-allocates without zeroing.
  8. The 20.0-block distant-edge threshold is an unexplained, unconfigurable magic literal, duplicated in taa-rejection.py:110, with no boundary test.
  9. UpscalerLodBiasOffset clamp (OptimumConfig.cs:1232) doesn't guard NaN (Math.Clamp passes NaN through unchanged).
  10. Possible double-fire in new dropdown callbacks — onOptimumUpscalerChanged/onOptimumUpscalerQualityChanged (GuiCompositeSettings.cs.patch:347,364) lack the if (!selected) return; guard that onOptimumRendererChanged uses, though an idempotency check likely neutralizes visible effect (unconfirmed — decompiled dropdown-delegate semantics not present in repo).
  11. Five new en.json localization keys (optimum-upscaler-dlaa, -quality-mode, -balanced, -performance, -ultra-performance) are never referenced anywhere; actual UI builds different key names (optimum-upscaler-preset-*).
  12. XessBackend/Fsr3Backend's Check() never self-disables via OptimumConfig.DisableUpscalerAtRuntime() on persistent failure the way DlssUpscaler.Fail does — a broken XeSS/FSR3 install keeps retrying TryPlan and re-logging every resize instead of parking for the session.
  13. optimum-render-device.cs (+274 lines) is entirely the headless-capture harness, not upscaler-selection API as the PR description's file list implies — the real config surface and ~3,871 lines of backend implementation live elsewhere and weren't in this file at all.
  14. RestoreWorldDrawBuffers can issue a draw-buffer declaration against a mismatched (1-attachment) framebuffer if NativePostPipeline returns null after upscaledThisFrame was already set (NativePostFinal.cs:328-340) — currently harmless because it's immediately superseded by LegacyFinalComposition's rebind, but latent if that assumption changes.
  15. LoadFrameBuffer(Primary) redirection to the display-resolution composite target is unscoped for the rest of the frame once set, not just the documented "late overlays" window (FrameBuffers.cs:19-29) — a future caller expecting the true render-resolution Primary later in the same frame would silently get the wrong target.
  16. If the display-resolution composite target fails to allocate mid-SetupDefaultFrameBuffers, the local upscaling flag for that same call stays true even after DisableUpscaler runs, producing one frame with neither TAA nor upscaling active (self-heals next frame via rebuildUpscalerTargetsPending) (FrameBuffers.cs:288-301).
  17. taaRequested in ClientPlatformWindows.cs.patch:672 wasn't migrated to EffectiveTemporalPipeline like its four sibling call sites — harmless on this OpenGL-only class, but worth explicitly verifying VulkanClientPlatform's equivalent buffer-setup path doesn't carry the same oversight (there it would be Critical: no motion vectors allocated for "upscaler on, TAA checkbox off").
  18. Six updateVAO/updateIndices overloads in ClientPlatformWindows.cs.patch add an unused Stopwatch.GetTimestamp() call on the mesh-upload hot path — dead instrumentation, unrelated to the upscaler feature.
  19. Uncommitted, unstaged sources/shaders/chunkliquid.fsh in the working tree is pure CRLF→LF line-ending noise (confirmed via --ignore-all-space), not part of any commit on this branch — a local environment artifact, but worth a git checkout -- before it accidentally lands in a future commit as 598 lines of noise.

Nitpicks

  • NgxSession.cs:52 allocates one inert pointer slot for zero feature paths (harmless, freed later).
  • NgxDlssFeature.cs bypasses the NgxInterop.UseDirectCalls diagnostic seam for Create/Evaluate/Dispose while its paired DestroyParameters call honors it — likely intentional (only init/discovery needed the bypass) but undocumented.
  • NgxLifetime.cs:105's OwningThread is recorded but never asserted against on ShutDown, despite the doc comment implying it should be.
  • VulkanClientPlatform.Upscale.cs:127 logs the raw OptimumConfig.Upscaler instead of EffectiveUpscaler in DisableUpscaler — cosmetic only.

Verified correct (no defect)

GL/Vulkan shader parity for the reviewed TAA/GTAO logic is byte-for-byte identical between .fsh/.glsl variants; the AO debug view correctly reflects post-temporal AO end-to-end; GtaoSettings.ForUpscaler presets match their tests; P/Invoke struct layouts for NGX/XeSS/FSR3 match their native headers exactly (verified against existing ABI-assertion tests); deferred-deletion discipline for GPU resources (textures, native upscaler contexts) consistently routes through the frame ring, avoiding use-after-free while a frame is in flight; barriers are placed explicitly before every vendor Evaluate call; provider dispatch safely resolves an unknown/garbage provider name to "no upscaling" rather than throwing; resize handling recomputes display and planned render size together from one live ClientSize with no stale cross-field caching; packaging scripts existence-guard every optional native artifact so builds without a given SDK degrade gracefully (except FSR3's MSBuild target, see #15).


Overall assessment: the architecture (render/display resolution separation, deferred GPU resource retirement, the IDeviceRequirementContributor device-feature chain, and the NGX lifetime owner's locking discipline) is sound and the three vendor backends are structured consistently. The concrete bugs worth fixing before merge are the two NGX use-after-free/race issues (#1, #2), the XeSS LOD-bias miscalculation with a self-confirming bad test (#3), the null-quality crash reachable from mod code (#4), and the overbroad distant-edge TAA heuristic that can ghost on real geometry rather than just foliage (#5). The CI gap (#11) means none of this native-path behavior is actually verified by automation in this repository today — that's worth flagging to reviewers evaluating the "42 passed" verification claim in the PR description.
· Branch: codex/upscaler-native

@NightHammer1000

Copy link
Copy Markdown
Collaborator Author

@claude review

@claude

claude Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Claude finished @NightHammer1000's task in 3m 51s —— View job


Review

Todo list:


Two commits landed since my last full pass (499e297 Prevent stale glow at distant TAA depth edges, 0caa070 Harden upscaler lifetimes and distant TAA rejection). I diffed both against the state I last reviewed and verified each fix against the code, not just the commit message.

Prior High-severity findings — verified fixed

  • Vulkan rendering backend behind the platform seam #1/Add TAA and Motion Vectors #2 NGX use-after-free / Dispose-vs-Evaluate race (NgxSession.cs, NgxDlssFeature.cs): both classes now guard every entry point with a shared _gate lock, Dispose() is idempotent under the lock, _projectId/_engineVersion/_applicationDataPath are zeroed on dispose, and every public call (Discovery, InstanceExtensions, DeviceExtensions, Requirements, Initialize, Evaluate) now throws ObjectDisposedException via ThrowIfDisposed()/IsValid. New test NgxFeaturePathTests.DisposedSessionRejectsNativeDiscoveryAndCanBeDisposedAgain covers this directly.
  • Feat/dlss #3 XeSS LOD-bias missing the shared offset: XessBackend.TryPlan and Fsr3Backend.TryPlan both now call the single OptimumConfig.RecommendedUpscalerLodBias(...) helper (also used by DlssUpscaler.LodBias), so all three vendors compute bias identically. The tests that previously enshrined the wrong formula (XessTests.cs:141, Fsr3Tests.cs:51) now assert against the shared helper instead of a hand-rolled formula.
  • DLSS frame generation groundwork: HUD-less frame, UI target, OpenGL and Cecil fixes #4 UpscalerQuality = null crash: XessBackend.QualityOf/Fsr3Backend.QualityOf are now string? with null-safe ?. switches, and TryPlan normalizes null/whitespace quality to "quality" before use. New tests MissingQualityUsesTheDefaultPreset cover both vendors.
  • Native Vulkan upscaler selection with DLSS, XeSS, and FSR 3 #5 overbroad distantDepthEdge heuristic: the shader now counts 3x3 neighborhood taps within 2e-4 of the closest depth (nearDepthTaps) and only treats the pixel as sparse/transient when nearDepthTaps <= 2; a solid silhouette (≥3 near taps) takes the ordinary hard reset. scripts/dev/taa-rejection.py's gate metric was updated in lockstep with a matching near_taps computation, closing the gap where the Python gate no longer matched the shader. A new self-test in the script and a new GPU test (SparseDistantEdgeKeepsHistoryWhileSolidDisocclusionResets) now explicitly assert a solid distant edge resets while the sparse leaf case doesn't — this is exactly the missing coverage I flagged.
  • feat: add Linux native bundle script, packaging docs and Cairo font fix #9 unfiltered glow at persistent edges: no longer applies — since Native Vulkan upscaler selection with DLSS, XeSS, and FSR 3 #5 now scopes the exception to sparse coverage, a permanent edge takes the normal reset path and never hits the raw-glow branch.
  • Add Claude Code GitHub Workflow #6 BringUpUpscaler exception isolation: now wraps BringUp/Shutdown per backend in try/catch, logs and removes the failed backend from upscalers instead of aborting InstallGraphics for every backend.
  • The game crashes immediately after the menu loads #7 unvalidated native extension count: NgxSession.Collect and the new XessBackend.CheckExtensionList both bound the count (<= 256) and reject a null properties/names pointer with a nonzero count before indexing.
  • Mod has no effect #8 stale IsHdr doc comment: corrected to match the actual (unconditionally-set) flag.
  • #14 unsynchronized disabledUpscalers: now guarded by disabledUpscalersGate, and DisableUpscalerAtRuntime uses HashSet.Add's return value instead of check-then-add.
  • #15 FSR3 MSBuild target hard-failing the build: ContinueOnError="true" added to the Exec, and the LICENSE.txt copy is now Condition="Exists(...)"-guarded.
  • #16 stale SSAO screenSize math: NativeVanillaSsaoPass now derives its uniform from the actual allocated frame-buffer targets (primary.Width/Height, target.Width/Height) instead of ssaaLevel/window client size.
  • #19 native-library-singleton race: Fsr3Native.TryLoad/XessNative.TryLoad now serialize under a loadGate lock.
  • #21 unindexed names[]: XeSS extension enumeration now null-checks each entry before PtrToStringUTF8 and rejects blank/whitespace extension names.
  • #23 native log callback exceptions: NgxSession.LogNgx now wraps its body in try/catch.
  • #26 NaN not guarded in LOD-bias clamp: RecommendedUpscalerLodBiasForScale now checks float.IsFinite on both renderScale and UpscalerLodBiasOffset before clamping.

That's a genuinely thorough response — essentially every High-severity item and most of the Medium/Low items with concrete repro paths were fixed with matching test coverage, not just silenced.

Still open from the prior review (unchanged by these two commits)

  • GTAO creates a line across grass at head height #11 CI gap: Optimum.Render.Vulkan.Tests still isn't wired into any GitHub Actions workflow, so none of this (including the new hardening) runs in CI on this repo — worth flagging again since the PR description's "42 passed" claim still can't be verified by automation here.
  • [0.1.4] Client item-loading crash after VulkanStory refuses mods #12 UpscalerDeviceRequirementsTests only exercises the "everything refused" path — still no test for an accepted extension or two contributors chaining together.
  • The mod is not activating #13 OptimumConfig.Upscaler/UpscalerQuality remain unvalidated public mutable fields — lower-severity now that QualityOf and TryPlan are null-safe, but still bypassable outside Load().
  • crash upon entering the world #10 XessBackend.FinalizeDeviceFeatures chain-restore — still only restores the three snapshotted struct types/PNext links, still untested on a native failure path.
  • #18/#20/#22/#24/#27–#35 (low-severity/nitpick items) — untouched; none are correctness-blocking.

One correction to my own prior review: I had flagged #17 ("stale AO-applied latch never resets") — rereading ClientPlatformWindows.cs.patch, optimumSsaoInScene is actually reset to false at the top of OptimumPostAmbientOcclusion every frame that method runs (line ~2664), before it's conditionally set back to true. I don't see a code path where it survives across frames, so that finding looks like a false positive on my part rather than a real bug — apologies for the noise there.

New from this pass

  • sources/shaders/chunkliquid.fsh is still showing as a modified-but-uncommitted file in the working tree (pure CRLF→LF churn, ~598 lines, verified with --ignore-all-space). It's not part of any commit on this branch, so it won't land in the PR, but worth a local git checkout -- sources/shaders/chunkliquid.fsh before it accidentally gets staged in a future commit.
  • No new correctness issues found in the two fix commits themselves — the added locking is scoped correctly (Dispose/Evaluate share one lock per session/feature instance, no cross-instance contention), and the new nearDepthTaps shader logic and Python parity script stayed in sync.

Overall

This is a strong, targeted follow-up: it resolves the two NGX lifetime races, the LOD-bias/test inconsistency, the null-quality crash, the overbroad TAA heuristic (with real regression coverage now in place), and several medium/low items, all with matching tests. The main gap that still matters for merge confidence is unchanged — the Vulkan test project isn't run in CI, so this hardening (like the original feature) is currently only verified by local/manual runs.
· Branch: codex/upscaler-native

@NightHammer1000

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-25T17:01:22.860822Z 0d11558 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0d11558dc1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".


internal sealed unsafe class XessBackend : IUpscalerBackend, IDeviceRequirementContributor
{
internal const uint InitFlags = 1u << 8; // HDR, low-res unjittered motion, forward depth, auto exposure.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use the XeSS auto-exposure flag for HDR input

When XeSS is selected, 1u << 8 is XESS_INIT_FLAG_LDR_INPUT_COLOR, not the auto-exposure flag described by the comment. The renderer supplies linear HDR color and no exposure texture, so XeSS interprets that input as LDR while auto exposure remains disabled, producing incorrectly exposed output. Use XESS_INIT_FLAG_ENABLE_AUTOEXPOSURE (1u << 4) and leave the LDR flag unset.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the bundled XeSS SDK header and Intel upstream: XESS_INIT_FLAG_ENABLE_AUTOEXPOSURE is 1 << 8, XESS_INIT_FLAG_LDR_INPUT_COLOR is 1 << 6, and 1 << 4 selects NDC motion vectors. The existing bit 8 is correct for our HDR input, so changing it to bit 4 would break the motion-vector interpretation. I made the flag mapping explicit and added a test in cfd948a. Intel header: https://github.com/intel/xess/blob/main/inc/xess/xess.h#L1205-L1244

Comment on lines +175 to +176
plan = new UpscalerPlan((int)optimal.Width, (int)optimal.Height, displayWidth, displayHeight, quality,
OptimumConfig.RecommendedUpscalerLodBias((int)optimal.Width, displayWidth));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Compute XeSS's provider-specific mip bias

For every XeSS mode below native resolution, this helper applies the global default offset and produces log2(render/display) - 1, while the XeSS contract documented in docs/upscaler.md calls for log2(render/display) without the extra DLSS/FSR offset. That selects textures one full mip sharper than intended and increases aliasing and temporal shimmer in the XeSS path; pass XeSS's provider-specific bias instead of the shared recommendation.

Useful? React with 👍 / 👎.

Comment on lines +122 to +124
if [[ -n "$value" ]] && ! [[ "$value" =~ ^[0-9]+$ ]]; then
echo "${pair%%:*} takes a non-negative integer (got '${value}')" >&2; exit 2
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject zero-frame captures before launching

When the harness is invoked with --count 0, this validation accepts the value, but the managed side creates an empty frame plan, never enables capture, and therefore never emits the completion line or performs the requested clean self-exit. The script waits for the full capture timeout and then signals the invisible client during cleanup; require a positive count before launching.

Useful? React with 👍 / 👎.

Comment thread scripts/dev/headless-capture.sh Outdated
Comment on lines +314 to +316
if (( FILES == 0 )); then
echo "the capture line appeared but no frames were written" >&2
exit 1

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fail captures when any requested frame is missing

If readback or writing fails for some requested frames, the client still reaches the last planned frame and emits its completion line, while this check returns success as long as one PPM exists. A partially captured experiment can therefore be reported as successful, and two runs that omit the same frames can compare only the surviving subset without exposing the loss; verify the file count against the requested unique frame count rather than checking only for zero.

Useful? React with 👍 / 👎.

UpscalerPlan plan = allocatedUpscalePlan;
var frame = new UpscalerFrame(primary.ColorTextureIds[0], primary.DepthTextureId,
primary.ColorTextureIds[MotionAttachmentIndex], output.ColorTextureIds[0], OptimumTemporal.Context);
if (!selected.Evaluate(plan, frame, out string? error))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Contain exceptions from optional upscaler evaluation

On the first XeSS/FSR frame after selection, resize, or quality change, Evaluate lazily creates the provider context and RG16F motion texture; those managed allocation/device calls can throw on allocation or driver errors. Unlike provider bring-up and output-target allocation, this call is not exception-contained, so such a failure escapes the render loop and crashes the client instead of invoking the existing runtime-disable fallback. Catch provider exceptions here, retire the partial feature, and call DisableUpscaler.

Useful? React with 👍 / 👎.

@NightHammer1000
NightHammer1000 merged commit cfd948a into main Sep 26, 2026
9 checks passed
@NightHammer1000
NightHammer1000 deleted the codex/upscaler-native branch October 11, 2026 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant