fix(vk): share validated source crops across presentation paths #1448

Merged

Arkex-ai merged 4 commits from fix/presentation-source-crop into main

2026-11-10 16:30:20 +00:00

Arkex-ai commented

2026-11-10 03:08:49 +00:00

(Migrated from github.com)

Use one validated source rectangle for aspect calculation, direct/capture blits, scaling and interpolation. Clip by available source area before addition, preserve the full-image convention for inactive crops, and reject origins/dimensions that cannot produce valid signed image offsets. Empty direct/capture regions clear to opaque black; scaling/interpolation decline them before setup. Valid crops keep their offsets, filter and output rectangle.

Validation:

Production blit/capture/interpolation CPU consumers: exact command offsets, black clear/skip, allocation dimensions and copy push constants; 4096 deterministic clipped cases plus boundaries. Existing independent-scaling fixture executes the production scaler.
Baseline submits the negative offset; compiled baseline under UBSan reports the signed overflow. Candidate passes Linux GCC and ASan/UBSan. Five compiled mutations are detected.
Current-base strict suite: candidate189 passed/4 failed/16 skipped versus main188/4/16, with the same local environment/toolchain failures. Integrated F01/F02/F03/F04 plus candidate:193/4/17.
Official integrated host/backend build and two RTX5070Ti checkpoint smokes (direct+RCAS and fixed2x+RCAS, windowed/maximized) passed. Malformed crops were exercised by CPU spies/sanitizers, not injected into the GPU during gameplay.

Based on main 9a2e80e. No game data, generated guest code, new settings or quality-policy changes. Checked overlap with #1339, #1445, #1352 and #1386; this source-crop behavior is separate from interpolation allocation rollback in #1445.

The current main HDR consumer now uses the same validated source rectangle. It previously added signed crop coordinates before conversion, allowing overflow and invalid UV input. This update validates before HDR initialization/dispatch and computes normalized endpoints from the clipped, bounded rectangle; valid crop behavior is preserved.

Current head 14e001c on b2e1c5a: existing production-consumer tests plus the actual HDR function pass with Windows LLVM, Linux GCC and GCC ASan/UBSan, including 4096 clipped-overflow cases, disabled crops and invalid extents. Reinstating only main's original HDR function produces an assertion failure; the candidate passes. No HDR-capable physical-display game run is claimed. Older runtime evidence remains tied to its original hashes. Shared regression fixtures are repaired separately in #1498 and the duplicate backend formatter was fixed in merged #1496.

Current-head validation

Current head f20afae, based on main ba40a1a. Earlier local/runtime results describe their recorded source hashes.

Fork runs are separate from upstream Actions approval; no maintainer approval gate is bypassed.

Presentation's content-size helper rejects invalid crop origins, but direct/capture blits still add signed coordinates and extents before clipping. For example, x=1,width=INT_MAX overflows; a negative active origin reaches the Vulkan blit. Interpolation also uses a separate crop calculation. Use one validated source rectangle for aspect calculation, direct/capture blits, scaling and interpolation. Clip by available source area before addition, preserve the full-image convention for inactive crops, and reject origins/dimensions that cannot produce valid signed image offsets. Empty direct/capture regions clear to opaque black; scaling/interpolation decline them before setup. Valid crops keep their offsets, filter and output rectangle. Validation: - Production blit/capture/interpolation CPU consumers: exact command offsets, black clear/skip, allocation dimensions and copy push constants; 4096 deterministic clipped cases plus boundaries. Existing independent-scaling fixture executes the production scaler. - Baseline submits the negative offset; compiled baseline under UBSan reports the signed overflow. Candidate passes Linux GCC and ASan/UBSan. Five compiled mutations are detected. - Current-base strict suite: candidate189 passed/4 failed/16 skipped versus main188/4/16, with the same local environment/toolchain failures. Integrated F01/F02/F03/F04 plus candidate:193/4/17. - Official integrated host/backend build and two RTX5070Ti checkpoint smokes (direct+RCAS and fixed2x+RCAS, windowed/maximized) passed. Malformed crops were exercised by CPU spies/sanitizers, not injected into the GPU during gameplay. Based on main `9a2e80e`. No game data, generated guest code, new settings or quality-policy changes. Checked overlap with #1339, #1445, #1352 and #1386; this source-crop behavior is separate from interpolation allocation rollback in #1445.  The current main HDR consumer now uses the same validated source rectangle. It previously added signed crop coordinates before conversion, allowing overflow and invalid UV input. This update validates before HDR initialization/dispatch and computes normalized endpoints from the clipped, bounded rectangle; valid crop behavior is preserved. Current head `14e001c` on `b2e1c5a`: existing production-consumer tests plus the actual HDR function pass with Windows LLVM, Linux GCC and GCC ASan/UBSan, including 4096 clipped-overflow cases, disabled crops and invalid extents. Reinstating only main's original HDR function produces an assertion failure; the candidate passes. No HDR-capable physical-display game run is claimed. Older runtime evidence remains tied to its original hashes. Shared regression fixtures are repaired separately in #1498 and the duplicate backend formatter was fixed in merged #1496.  ### Current-head validation Current head `f20afae`, based on main `ba40a1a`. Earlier local/runtime results describe their recorded source hashes. - [Linux AppImage](https://github.com/Arkex-ai/Hookshot/actions/runs/42809178488): success. - [Windows builder](https://github.com/Arkex-ai/Hookshot/actions/runs/42809174654): success. - [Game-free source regressions](https://github.com/Arkex-ai/Hookshot/actions/runs/42809170775): success. Fork runs are separate from upstream Actions approval; no maintainer approval gate is bypassed.

hckrman101 (Migrated from github.com) approved these changes

2026-11-10 04:15:14 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks good. present_source_rect is the same logic as the old present_content_size, now shared, and the three consumers that were doing their own math (the direct/capture blit, framegen_scale and framegen_record) all use it. I checked the arithmetic: source.x + source.w <= sw <= INT32_MAX, so the (int) casts on the blit offsets can't overflow any more, and the x=1,width=INT_MAX case is fixed. The inactive-crop convention is the same as before.

The black fallback is safe in the present path. An empty source makes present_rect return the full swapchain rect (content is 0), so has_bars is false and clearing the whole image doesn't wipe a letterbox. The capture/log destinations are always full-rect too. framegen_record returning false on a bad crop just falls back to the normal present, as for !src. The new test is registered in run_all.py with the same placeholders as its neighbours. No new settings ids.

On the related PRs:

#1339 still works on top of this. Its aspect branch in present_rect comes after the !content.w early return, so an invalid crop still gets the full rect and the full-image clear stays correct. It will need a small textual rebase in vk_present_rect.h (the present_content_size hunk).
#1352 conflicts in framegen_scale (same lines as the origin check here). It also adds another crop calculation of its own for the blurred-sides small image: pw = crop && crop[2] > 0 ? crop[2] : sw, which isn't clamped. Whichever lands second should switch that to source.w/source.h so it doesn't drift from this contract.

CI: no upstream checks yet (fork runs need approval). On the fork at 8846353, Linux AppImage and Windows builder passed; the game-free regressions run was still in progress when I looked.

Looks good. `present_source_rect` is the same logic as the old `present_content_size`, now shared, and the three consumers that were doing their own math (the direct/capture blit, `framegen_scale` and `framegen_record`) all use it. I checked the arithmetic: `source.x + source.w <= sw <= INT32_MAX`, so the `(int)` casts on the blit offsets can't overflow any more, and the `x=1,width=INT_MAX` case is fixed. The inactive-crop convention is the same as before. The black fallback is safe in the present path. An empty source makes `present_rect` return the full swapchain rect (`content` is 0), so `has_bars` is false and clearing the whole image doesn't wipe a letterbox. The capture/log destinations are always full-rect too. `framegen_record` returning false on a bad crop just falls back to the normal present, as for `!src`. The new test is registered in `run_all.py` with the same placeholders as its neighbours. No new settings ids. On the related PRs: - #1339 still works on top of this. Its aspect branch in `present_rect` comes after the `!content.w` early return, so an invalid crop still gets the full rect and the full-image clear stays correct. It will need a small textual rebase in `vk_present_rect.h` (the `present_content_size` hunk). - #1352 conflicts in `framegen_scale` (same lines as the origin check here). It also adds another crop calculation of its own for the blurred-sides small image: `pw = crop && crop[2] > 0 ? crop[2] : sw`, which isn't clamped. Whichever lands second should switch that to `source.w`/`source.h` so it doesn't drift from this contract. CI: no upstream checks yet (fork runs need approval). On the fork at 8846353, Linux AppImage and Windows builder passed; the game-free regressions run was still in progress when I looked.

gpu/vk/vk_replay.cpp

Nit: this clears all of dst, not just rect. That's fine for every caller today, because an empty source always comes with a full-destination rect. But the doc comment says "into a rectangle of dst", so a one-line note that the empty-source fallback clears the whole image would help a future caller that passes a subrect.

Nit: this clears all of `dst`, not just `rect`. That's fine for every caller today, because an empty source always comes with a full-destination rect. But the doc comment says "into a rectangle of dst", so a one-line note that the empty-source fallback clears the whole image would help a future caller that passes a subrect.

Arkex-ai (Migrated from github.com) reviewed

2026-11-10 04:22:52 +00:00

gpu/vk/vk_replay.cpp

Added the full-image clear note to blit_crop_to_rect. The production source-crop regression passes; refreshing the three fork workflows on the comment-only head. Commit 5e3d79c.

Added the full-image clear note to blit_crop_to_rect. The production source-crop regression passes; refreshing the three fork workflows on the comment-only head. Commit `5e3d79c`.

hckrman101 (Migrated from github.com) approved these changes

2026-11-10 16:20:02 +00:00

hckrman101 (Migrated from github.com) left a comment

Re-reviewed at f20afae. Still good.

The blit_crop_to_rect nit is fixed: the doc comment now says the empty-source fallback clears the whole image.
New since my last pass: framegen_hdr now goes through present_source_rect too. The early return comes before hdr.init, so a bad crop makes no HDR resources and falls back to the SDR picture. The UVs come from the clipped rect, so they stay in [0,1] without the old std::min(1.0f, ...), and the old negative-origin UVs are gone. Valid crops give the same UVs as before. The scaled-HDR call passes nullptr and gets the full rect, so nothing changes there.
The FSR3 frame-generation test additions look right: fsr3fi_frame gets the validated extent, the fallback copy uses the shared crop, and an invalid crop never reaches FSR3.

On #1008 (FSR 4 / TemporalEngine), which merged today: no overlap. The temporal/FSR 4 work runs on the render side (vk_temporal.cpp, vk_fsr4.cpp, vk_draw.cpp) and doesn't read the present crop. The present-side consumers this PR touches (blit_crop_to_rect, framegen_scale, framegen_hdr, framegen_record) are the same on main as on your base, and vk_framegen.cpp on main matches your base apart from this diff. The PR applies cleanly on current main (only a line offset in vk_replay.cpp and run_all.py), and the new test's source slicing still finds its markers there.

Upstream CI passes on this head (AppImage, Windows back end, Python checks on both OSes, SPIR-V/F2I lowering, actionlint), and GitHub shows it as mergeable.

Re-reviewed at f20afae. Still good. - The `blit_crop_to_rect` nit is fixed: the doc comment now says the empty-source fallback clears the whole image. - New since my last pass: `framegen_hdr` now goes through `present_source_rect` too. The early return comes before `hdr.init`, so a bad crop makes no HDR resources and falls back to the SDR picture. The UVs come from the clipped rect, so they stay in [0,1] without the old `std::min(1.0f, ...)`, and the old negative-origin UVs are gone. Valid crops give the same UVs as before. The scaled-HDR call passes `nullptr` and gets the full rect, so nothing changes there. - The FSR3 frame-generation test additions look right: `fsr3fi_frame` gets the validated extent, the fallback copy uses the shared crop, and an invalid crop never reaches FSR3. On #1008 (FSR 4 / TemporalEngine), which merged today: no overlap. The temporal/FSR 4 work runs on the render side (`vk_temporal.cpp`, `vk_fsr4.cpp`, `vk_draw.cpp`) and doesn't read the present crop. The present-side consumers this PR touches (`blit_crop_to_rect`, `framegen_scale`, `framegen_hdr`, `framegen_record`) are the same on main as on your base, and `vk_framegen.cpp` on main matches your base apart from this diff. The PR applies cleanly on current main (only a line offset in `vk_replay.cpp` and `run_all.py`), and the new test's source slicing still finds its markers there. Upstream CI passes on this head (AppImage, Windows back end, Python checks on both OSes, SPIR-V/F2I lowering, actionlint), and GitHub shows it as mergeable.

Arkex-ai commented

2026-11-10 16:27:03 +00:00

(Migrated from github.com)

Thanks for the re-review. Rechecked f20afae95120c090c4c08802db9443248cdda9a4: all eight executed upstream checks passed (the two release-upload checks were skipped), and the exact-head fork runs Linux AppImage, Windows builder, and game-free regressions all completed successfully. There is no new requested correction in this review, so this follow-up leaves the reviewed commit unchanged. No additional native/HDR-display runtime validation is claimed.

Thanks for the re-review. Rechecked `f20afae95120c090c4c08802db9443248cdda9a4`: all eight executed upstream checks passed (the two release-upload checks were skipped), and the exact-head fork runs [Linux AppImage](https://github.com/Arkex-ai/Hookshot/actions/runs/42809178488), [Windows builder](https://github.com/Arkex-ai/Hookshot/actions/runs/42809174654), and [game-free regressions](https://github.com/Arkex-ai/Hookshot/actions/runs/42809170775) all completed successfully. There is no new requested correction in this review, so this follow-up leaves the reviewed commit unchanged. No additional native/HDR-display runtime validation is claimed.

Commenting is not possible because the repository is archived.

No reviewers

hckrman101