Repository navigation
perf(vk): keep big ASTC textures as BC7 (RX_VK_ASTC_BC7) - #2226
Conversation
11a5df5 to
d62b09c
Compare
hckrman101
left a comment
There was a problem hiding this comment.
Thanks, this is a nice opt-in: off unless RX_VK_ASTC_BC7=1, the BC3 path untouched, and RX_VK_ASTC_RGBA8 still wins.
What I checked and looks right:
- Mode 6 packing: mode bits, R0 R1 G0 G1 B0 B1 A0 A1 at 7 bits, the two p-bits, a 3-bit anchor index then 15 4-bit ones (128 bits). The anchor swap is sound because the weight table is symmetric (w[15-k] = 64 - w[k]).
- sRGB vs UNORM: the image is BC7_UNORM and
view_format_ofpicks BC7_SRGB/UNORM throughlinear_twinexactly as BC3 did. The scratch holds the stored bytes, so sRGB data stays sRGB. - Layout:
bc_encode_intoalways pushesbc3 = 1, out_offset = 0, so the shader ignoringbc3is fine, and 16 bytes a block matches the copy. - Storage/attachment views of a
bctexture are still refused, partial uploads and CPU writes still go through thetex.bcchecks, and the AMD resized path goes throughbc_encode_intotoo. HDR ASTC is unchanged (both paths encode the decoder's RGBA8). - Budget:
createcounts anyx.bcimage at 1 byte a texel, so BC7 costs the same as BC3, including after #2288. - GPU support: Vulkan 1.3 is required, so the SPIR-V 1.5 header is fine, and BC7 sampling is universal on desktop.
What I'd like before merge:
- A test. There's no check in
tests/. The repo's convention (seetests/test_fg_scale_sharpen.py) is that the shader's test holds the glslang command and checks that the_spv.hwas built from this exact.comp(a hash on its first line). With a standalone script, the header can drift from the source without anyone noticing. A game-free check of the packing would also be cheap, e.g. a small CPU decode of a mode-6 block cut from the shader's logic, or a comparison againstgpu/render/rx_bc7.cpp. Please register it with run_all.py the usual way. - Screenshots (CONTRIBUTING.md asks for them when a change shows on screen): BC3 vs BC7 of the same spot. Please include alpha-tested foliage (grass, tree leaves) as well as the ground. Mode 6 fits RGB and alpha on one shared line, while BC3 encodes alpha in its own block. So for cutout textures, where alpha doesn't follow colour, BC7 mode 6 can do worse at the alpha-test edge than BC3, and "much better alpha" may not hold everywhere. If it shows, mode 5 (separate alpha) is the usual fix.
- Encode cost off NVIDIA. It was only tested on a 3080. Each invocation keeps
px[16][4], a 16x4 palette and twoFits with 16 indices, which is heavy on registers and may spill on AMD and Intel iGPUs. The 90-layer material arrays are encoded one layer per run. If anyone can,vk: slow uploadlog lines for BC3 vs BC7 on an AMD or Intel card would show whether loading areas stutter more.
CI: the Windows build passes. The AppImage failure is the same apt network timeout main has right now (not this PR). The Python source checks have not run yet (pending).
| VkFormatProperties fp{}; | ||
| vkGetPhysicalDeviceFormatProperties(dev.physical, VK_FORMAT_BC3_UNORM_BLOCK, &fp); | ||
| vkGetPhysicalDeviceFormatProperties(dev.physical, bc_format, &fp); | ||
| if (!(fp.optimalTilingFeatures & VK_FORMAT_FEATURE_SAMPLED_IMAGE_BIT)) return false; |
There was a problem hiding this comment.
If BC7 isn't sampleable here, or the BC7 pipeline fails at line 731, bc_ready returns false and every big ASTC texture becomes RGBA8, which is 4x the memory. Falling back to BC3 (clear a flag so bc7_enabled() reads false afterwards) would be safer. It's unlikely on desktop, so this is a nit.
| for (int a = 0; a < 4; ++a) | ||
| for (int b = 0; b < 4; ++b) cov[a][b] += d[a] * d[b]; | ||
| } | ||
| float axis[4] = {cov[0][0], cov[1][1], cov[2][2], cov[3][3]}; |
There was a problem hiding this comment.
Nit (inherited from rx_bc7.cpp): starting the power iteration from the diagonal can land exactly orthogonal to the principal axis. One example is R and G anticorrelated with equal variance (a red-to-green ramp), B and A constant: n comes out 0, the loop breaks, every texel projects to 0, and the block becomes flat at the mean. The refit can't rescue it because every index is the same, so det = 0. Restarting from the covariance row with the largest norm when the first step collapses would avoid it.
| const VkFormat f = view_format_of(tex->format, tex->astc, tex->bc, nf, use, force_format); | ||
| if (f == VK_FORMAT_UNDEFINED) return VK_NULL_HANDLE; | ||
| const bool depth = is_depth_format(tex->format); | ||
|
|
There was a problem hiding this comment.
Nit: stray blank line.
| // encoder of the WebGPU back end (gpu/render/rx_bc7.cpp, RX_ASTC_BC7) as a compute shader: BC7's | ||
| // mode 6 keeps gradients and alpha far better than BC3 at the same 8 bits a texel. | ||
| // | ||
| // Binding 0: the RGBA8 scratch (one layer per dispatch z). |
There was a problem hiding this comment.
Nit: the other host shaders and their _spv.h headers start with SPDX lines and a provenance note ("NOT a Nintendo file ... compiled from ..."). It would be good to match that here and in bc7_encoder_comp_spv.h.
c931060 to
cdbb233
Compare
Joey-Magnus
left a comment
There was a problem hiding this comment.
Opt-in, so the risk is low, but a few things:
- The new shader isn't in the shader-header drift check. Main builds headers from the
SHADERSlist inscripts/host_shaders.py, whichtests/test_host_shaders.pychecks. This PR adds its ownscripts/make_bc7_shader.pywith a different target (vulkan1.2), sobc7_encoder_comp_spv.hcan drift without anything failing. Please addbc7_encoderto that list and drop the private script. - If the BC7 pipeline fails to build (
vk_texture.cpp:731),bc_ready()returns false and turns off BC3 too, so big ASTC textures become RGBA8 (about 4x the memory) instead of falling back to BC3. - The earlier review asks are still open: a packing test, BC3 vs BC7 screenshots, and a check on alpha-tested foliage (mode 6 fits colour and alpha on one line).
- The
test_native_astc_texture_count.pyedit duplicates #2397 and can be dropped.
hckrman101
left a comment
There was a problem hiding this comment.
Re-checked after the push. It's a rebase onto main: the PR's diff is byte-for-byte the same as the commit I reviewed, so nothing from the first round has changed yet. CI is all green now (the Python source checks were still pending last time) and it merges cleanly.
Still open:
- The header can drift from its source. I agree with @Joey-Magnus here, and that's the simplest way to cover the first half of my point 1: add
bc7_encodertoSHADERSinscripts/host_shaders.pyand dropscripts/make_bc7_shader.py.tests/test_host_shaders.pythen recompiles it and fails on a mismatch, and the header gets the same opening comment as the others. Note thathost_shaders.pybuilds with--target-env vulkan1.1, not thevulkan1.2the private script uses, so the header has to be regenerated that way. - A packing check (a CPU decode of a mode-6 block, or a comparison with
gpu/render/rx_bc7.cpp) would still be nice to have, but with item 1 done I wouldn't hold the PR for it. - Screenshots: BC3 vs BC7 at the same spot, including alpha-tested foliage (mode 6 fits colour and alpha on one line).
- The fallback when the BC7 pipeline fails (inline thread on
vk_texture.cpp): the textures become RGBA8 rather than BC3. Joey raised it too. It's unlikely on desktop, but it's a one-flag fix.
The nits (the power-iteration restart, the SPDX/provenance lines, the stray blank line) are also unchanged and still optional. Joey's note about the test_native_astc_texture_count.py edit doesn't apply: the PR's diff doesn't touch that file (main already has #2397).
BC3 and BC7 are both 8 bits a texel, a quarter of RGBA8. BC7's mode 6 keeps the colours' gradients and alpha much better, so the big decoded ASTC textures (terrain, walls, the material arrays) lose less when they are re-encoded. The encoder is host_shaders/bc7_encoder.comp, a compute shader port of the WebGPU back end's CPU encoder (gpu/render/rx_bc7.cpp): a covariance fit, 7-bit endpoints with p-bits and 4-bit indices, 16 bytes a block as the BC3 path writes. Off unless RX_VK_ASTC_BC7=1; the BC3 path is unchanged. Tested at 1920x1080 on an RTX 3080: Kokiri Forest holds 60 fps, the same graphics memory as BC3, and no texture artifacts.
bc7_encoder is in scripts/host_shaders.py's SHADERS, so tests/test_host_shaders.py recompiles it with the others (vulkan1.1) and the checked-in header cannot drift; the private make_bc7_shader.py is gone. The shader and its header carry SPDX/provenance lines. A GPU without BC7, or a BC7 pipeline that cannot be made, now uses BC3 (bc7_ok) instead of turning every big ASTC texture into RGBA8. The encoder's power iteration restarts once from the covariance row with the largest norm when the diagonal collapses (R/G anticorrelated, B/A constant), so the block is not fitted to an orthogonal axis.
cdbb233 to
57e53f8
Compare
|
Thanks — the new head (
I have local BC3-vs-BC7 comparisons at the same spot, including alpha-tested foliage; I can attach them here if game screenshots are acceptable in the thread. The packing check is not in yet; the header check covers the source/header drift, and I can add the CPU mode-6 decode if you want to hold for it. |
Test report — BC7 for big ASTC texturesMachine
Checks
BC3 vs BC7, same save and cameraSame save, Kokiri Forest/Kakariko entrance, 1920x1080, 300-frame screenshots. The only change is
The two PNGs are local, and I have not uploaded them. I can provide sanitized copies if game screenshots are acceptable in the thread. Limits
|
Big decoded ASTC textures (terrain, walls, the material arrays) are re-encoded to BC3 today to keep them at 8 bits a texel. BC7 mode 6 uses the same 8 bits with much better gradients and alpha, so the grass, paths, stone and tree textures lose visibly less.
RX_VK_ASTC_BC7=1keeps those textures as BC7. The encoder is a compute shader port of the WebGPU back end's CPU encoder (gpu/render/rx_bc7.cpp): a covariance fit, 7-bit endpoints with p-bits and 4-bit indices, 16 bytes a block, layer-major as the BC3 path writes. Off unless set; the BC3 path is unchanged, andRX_VK_ASTC_RGBA8=1still wins when both are set.Tested at 1920x1080 on an RTX 3080: Kokiri Forest holds 60 fps, the same graphics memory as BC3, and no texture artifacts. The shader source and the generated header are checked in;
scripts/make_bc7_shader.pyrebuilds the header with glslang.