Repository navigation
Native Vulkan upscaler selection with DLSS, XeSS, and FSR 3 - #5
Conversation
|
@claude review and report every single finding you make |
|
Claude finished @NightHammer1000's task in 2m 8s —— View job Review complete — full findings reportReviewed the entire diff ( Todo list:
High severity1. NGX use-after-free: 2. No synchronization between 3. XeSS mip-LOD bias is missing the shared sharpening offset every other backend applies 4. 5. Medium severity
Low severity
Nitpicks
Verified correct (no defect)GL/Vulkan shader parity for the reviewed TAA/GTAO logic is byte-for-byte identical between Overall assessment: the architecture (render/display resolution separation, deferred GPU resource retirement, the |
|
@claude review |
|
Claude finished @NightHammer1000's task in 3m 51s —— View job ReviewTodo list:
Two commits landed since my last full pass ( Prior High-severity findings — verified fixed
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)
One correction to my own prior review: I had flagged #17 ("stale AO-applied latch never resets") — rereading New from this pass
OverallThis 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. |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 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. |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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
| plan = new UpscalerPlan((int)optimal.Width, (int)optimal.Height, displayWidth, displayHeight, quality, | ||
| OptimumConfig.RecommendedUpscalerLodBias((int)optimal.Width, displayWidth)); |
There was a problem hiding this comment.
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 👍 / 👎.
| if [[ -n "$value" ]] && ! [[ "$value" =~ ^[0-9]+$ ]]; then | ||
| echo "${pair%%:*} takes a non-negative integer (got '${value}')" >&2; exit 2 | ||
| fi |
There was a problem hiding this comment.
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 👍 / 👎.
| if (( FILES == 0 )); then | ||
| echo "the capture line appeared but no frames were written" >&2 | ||
| exit 1 |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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 👍 / 👎.
Summary
Review follow-up
Verification
dotnet build VintageStory.slnx -c Release --no-restorepassed locally.Optimum.Tests: 732 passed, 34 skipped locally.feat/vulkan-taa; the sparse-edge refinement is at57cefab0.Review notes