Repository navigation
VAE tiling: decode 2x2 tiles again at 456-504 px while keeping the seam fix - #26
Conversation
…ler one When the requested VAE tile leaves two tiles barely overlapping, #21 took the nearest smaller size that overlaps by half the target, which needs a third tile per axis: 496 px on an 8x VAE decoded 3x3 tiles instead of 2x2. Pick the size that decodes the fewest latents instead, overlapping by the full target up to 8 latents (half the target left a line on Qwen-Image-2.1's 16x VAE), within 25% of the requested size. 496 px gets two 35-latent tiles, Qwen-Image-2.1 480 px two 19-latent tiles. Sizes whose requested tile already overlaps enough are unchanged.
|
@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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48488df079
ℹ️ 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".
| const int tiles = static_cast<int>(sd_tiling_plan_axis(small_dim, size, tile_overlap_factor, false).size()); | ||
| const int64_t cost = static_cast<int64_t>(tiles) * size; | ||
| const bool better = best_size == 0 || cost < best_cost || | ||
| (cost == best_cost && (tiles < best_tiles || | ||
| (tiles == best_tiles && std::abs(size - tile_size) < std::abs(best_size - tile_size)))); |
There was a problem hiding this comment.
When the requested tile fails the initial overlap check, this ranks every size down to 4 solely by tile_count * size, allowing many tiny tiles to win. For an 8x VAE axis of 2960 pixels (small_dim=370) with --vae-tile-size 512 --vae-tile-overlap 0.1, the parent selects size 62 and 7 tiles, whereas this selects size 25 and 16 tiles because 400 is less than 434; on a square input, process_tiles_2d consequently performs 256 separate VAE evaluations instead of 49. Constrain the candidate size/tile count or account for per-tile invocation overhead so this performance optimization does not cause severe regressions for supported overlap settings.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0d71e56: candidates may not need more tiles than the nearest passing size (#21's choice), so your case now gets 6 tiles of 67 instead of 16 of 25 (master: 7 of 62). A sweep over every axis up to 400 latents, tiles 4 to 64, overlaps 0.1/0.25/0.5 (66978 cases) finds no case using more tiles than master.
| const int capped = std::min(small_dim - 1, tile_size + tile_size / 4); | ||
| const int widest = std::min(small_dim - 1, 2 * tile_size); | ||
| for (bool replacement : {true, false}) { | ||
| int size = pick(min_tile_size, capped, replacement); | ||
| if (size == 0) { | ||
| size = pick(capped + 1, widest, replacement); |
There was a problem hiding this comment.
docs/performance.md lines 51-55 still state that an unsafe tile is adjusted to the nearest qualifying size, smaller first and then larger up to twice the request. This code instead searches all sizes, prioritizes stricter-overlap candidates by decoded extent, and may grow within 25% or shrink far past the nearest size, so users relying on the documented memory and performance behavior receive incorrect guidance. Update the VAE tiling documentation alongside this public behavior change.
AGENTS.md reference: AGENTS.md:L74-L76
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0d71e56: docs/performance.md now describes the cheapest-qualifying choice, the 25% growth preference and the tile-count bound.
…ent the tile choice
|
@codex review |
1 similar comment
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…possible tie-break A halved retry request can be widened back to the failed tile size by the seam-safe size search, which made the retry loop give up. Lower the request of an axis that did not shrink one latent at a time instead. First attempts are unchanged.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 312a8b3eb1
ℹ️ 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".
| const bool lower_w = tile_size_w >= last_w && tiling_params.tile_size_w > 4 * scale_factor; | ||
| const bool lower_h = tile_size_h >= last_h && tiling_params.tile_size_h > 4 * scale_factor; | ||
| if (!lower_w && !lower_h) { | ||
| return tile_size_w < last_w || tile_size_h < last_h; |
There was a problem hiding this comment.
When the requested size is already the four-latent minimum, the new selector can still enlarge it beyond the nearest seam-safe size, but this condition prevents the retry from backing off. For example, on a 42-latent axis with 25% overlap and a four-latent request, the new policy selects 8 while the nearest passing size is 7; if an 8x8 tile allocation fails but 7x7 fits, prepare_vae_retry_tiling resets the request to four, this helper recomputes eight, and both lower_* flags remain false, so encode/decode aborts without trying the viable smaller candidate. Let an allocation retry bypass the stricter replacement choice or otherwise fall back to the nearest seam-safe size at the minimum request.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b1c2503. After an allocation failure the VAE now passes the failed tile size minus one as an upper bound to the seam-safe search, so the retry picks the best seam-safe size below the failed one instead of re-widening the 4-latent request. Your example (42 latents, 25% overlap) now goes 8 -> 7. An exhaustive check over 391k bounded cases confirms the bound is always respected and every retry ladder from the minimum request shrinks to 4; without a bound the search returns the same size as before on 192k cases, so first attempts are unchanged.
At the 4-latent minimum request the seam-safe search can still widen the tile (42 latents at 25% overlap: 4 -> 8), and lowering the request cannot go further, so a failed 8-latent tile aborted even when 7 would fit. After an allocation failure the VAE now passes the failed size minus one as an upper bound to the search, which picks the best seam-safe size under it. First attempts carry no bound and are unchanged.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1c2503c47
ℹ️ 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".
| return last_w <= 0 || !tiling_params.enabled || tile_size_w < last_w || tile_size_h < last_h; | ||
| } | ||
|
|
||
| // Clears the retry bound when a VAE pass ends. |
There was a problem hiding this comment.
This comment only paraphrases the destructor assignments immediately below it and adds no invariant or non-obvious rationale. Remove it to comply with the repository's explicit prohibition on comments that merely describe what the code does.
AGENTS.md reference: AGENTS.md:L128-L135
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79f183cb3d
ℹ️ 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".
| int sd_tiling_seam_safe_tile_size(int small_dim, int tile_size, float tile_overlap_factor, int max_tile_size) { | ||
| // Overlaps of 1-2 latents (32-latent tiles on a 62 axis) show as a seam; require >= half the target and >= 2. | ||
| const int min_tile_size = 4; | ||
| const int limit = max_tile_size >= min_tile_size ? max_tile_size : INT_MAX; |
There was a problem hiding this comment.
Fresh evidence in the final tree after the earlier retry fix: when one non-circular axis has already failed at the 4-latent minimum, sd_vae_tile_sizes passes max_tile_size == 3, but this line treats that as no limit. The seam-safe chooser can therefore widen that axis back to 8 while the other shrinks, and the OR-based progress check accepts it. With a 42×42 latent, 25% overlap, a requested 4×64 tile, and persistent allocation failures, the actual retries cycle 4×7 → 8×4 → 7×8 → 4×7, so encode/decode never returns. Preserve a cap of 4, or reject retries that increase either axis, once the bound drops below the minimum.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Problem
#21 added
sd_tiling_seam_safe_tile_sizeso a VAE axis is never split into two tiles that overlap by only 1 or 2 latents. When the requested tile fails that check it takes the nearest smaller size that passes. For an axis a little under twice the tile, every smaller size that passes needs a third tile, so the fix traded the seam for 50% more tiles per axis:At 496x496 that is 9 tiles instead of 4 on every
--vae-tilingdecode. Reported on an RTX 3090: Z-Image 496 tiled decode 0.38 s on upstream, 0.53 s on master; on an M1 (Metal) 54.3 s to 62.1 s end to end.Change
sd_tiling_seam_safe_tile_size(src/runtime/tiling.cpp), one function:--vae-tile-overlap 0.1and 64-latent tiles on a 370-latent axis, Keep VAE tiles overlapping on the out-of-memory retry (no more seam lines), and retry the encode too #21 gives 7 tiles of 62 and this gives 6 tiles of 67. Over every axis from tile+1 to 400 latents, tiles 4 to 64, overlaps 0.1 / 0.25 / 0.5 (66978 cases), this branch never needs more tiles than master.docs/performance.mddescribes the new choice.The tile planner and the stitching are unchanged. The out-of-memory retry still halves the request, but the seam-safe search could widen a halved request back to the size that failed (at the 4-latent minimum it can even widen 4 to 8), so after an allocation failure the search is now bounded to sizes below the failed tile. Every retry therefore shrinks the tile or stops at the minimum; first attempts carry no bound and pick the same size as without the retry change (checked on 192k size / request / overlap combinations).
Results
Base is master
afac7a7c, head is this branch, built the same way; upstreamf89d9b1as a third arm where it matters. Same prompt, seed 42,--rng cpu.Decode time,
--vae-tiling,decode_first_stageseconds, NVIDIA L4 (Colab, one job per VM), 4 interleaved rounds (master, fix, fix, master), 8 runs per arm:The spread within an arm is 0.02 to 0.03 s, the differences are 0.23 s (28%) and 0.41 s (23%). An earlier L4 session of the same Z-Image case gave 0.79 / 0.57.
Seams: each arm's tiled decode against the same arm's untiled decode of the same latent. FLUX.1-schnell stands in for Z-Image (same 16-channel 8x VAE,
ae.safetensors) because Z-Image does not render reproducibly (see Not covered).--circulary, B200So this keeps #21's seam fix (no error peak on the tile edge, and closer to the untiled decode than master's 3x3 in every case) while decoding 4 tiles again. The gain over master comes from the extra overlap. The Qwen-Image-2.1 numbers repeated exactly in two separate B200 sessions.
Everywhere the requested tile already overlapped enough, this branch gives byte-identical output to master on a B200 (sm100) and an RTX PRO 6000 Blackwell (sm120): untiled renders of SD1.5, SDXL, Qwen-Image-2.1, Qwen-Image-2.1-Turbo, FLUX.1-schnell, FLUX.2-klein-4B and MiniMax-H3 (24 frames, with and without
--circularx);--vae-tilingat 512, 992, 1008, 1184x1184, 1184x864 and 1344x768;--circularxat 512; FLUX img2img with a tiled encode at 1008; the out-of-memory retry at 1008x1008, 1184x864 and 1344x768 with a ballast leaving 14 and 11 GiB free (all three arms retry with 32-latent tiles and give identical images); SD1.5 and SDXL at 496 and 1008 (aligned to 64 px, so never in the changed range).Retry bound (b1c2503) against the previous head on an RTX PRO 6000 Blackwell (Colab), FLUX.1-schnell Q4_K_M, 4 steps, seed 42, with a torch ballast so the decode really runs out of memory: 43 of 43 paired images are byte-identical, untiled and
--vae-tilingat 1008, 1184x864, 1344x768, 992 and 496 with no ballast, and every real retry (1008 to 256x256 tiles, 496 to 248 then 120, 336 to 168 then 80) logs the reduction and finishes. The minimum-request case the bound exists for (a 4-latent request widened to 8) needs a third or fourth retry, which this model never reached before sampling itself ran out of memory, so that path is covered by the standalone check only.Tests
sd_tiling_seam_safe_tile_sizelinked against the built library: 14 named cases (62, 63, 57 latents at 32-latent tiles; 30 and 31 at 16; the default 512 to 1344 px axes) require the minimum tile count, growth within 25% and the overlap rule; every axis from tile+1 to 400 latents for tiles 4 to 64 at overlaps 0.25 and 0.5 requires every replacement to overlap by the full target up to 8 latents. Master fails 14 checks, this branch passes all.sd-cli(GGML_NATIVE=OFF) locally, CUDA 13.0 sm_89 (L4) and sm_120 (RTX PRO 6000) on Colab; no warnings intiling.cpp.--vae-tiling --vae-tile-size 128give byte-identical images on upstream, master and this branch.Not covered
buffer is nil), so Metal was not exercised. Vulkan and ROCm were not run; the change is host-side integer tile planning and backend independent.--vae-tile-sizechosen to just fit could fail its first attempt; the retry then picks a smaller seam-safe tile and finishes.