Skip to content

texcomp/texpipe: KTX2 reader + full decoder set; fix 8 codec bugs; live browser demo - #258

Merged
syoyo merged 19 commits into
releasefrom
texcomp-ktx2-reader
Jul 11, 2026
Merged

texcomp/texpipe: KTX2 reader + full decoder set; fix 8 codec bugs; live browser demo#258
syoyo merged 19 commits into
releasefrom
texcomp-ktx2-reader

Conversation

@syoyo

@syoyo syoyo commented Jul 11, 2026

Copy link
Copy Markdown
Owner

Adds the consumer side of the KTX2 writers: a KTX2 asset can now be loaded and transcoded/decoded per device, Basis-free, built on the uni (UASTC) intermediate.

The PR started there. Reviewing it, and then validating each codec against an independent implementation, turned up two security bugs in the parser and eight conformance bugs in codecs that nothing had ever decoded — including one that made every ETC2/EAC texture this tool has ever written display wrong on a GPU, and an ETC2 encoder sitting 22 dB behind BC1. It also grew a live browser demo, which is what found the last two. Details below.


What you can now do

Read a KTX2, and decode or transcode any codec texpipe can write.

path codecs
tp_ktx2_decode_level_rgba8 / _slice_rgba8 uni, BC1, BC3, BC5, BC7, ETC2 RGB/RGBA, EAC R11/RG11, ASTC LDR
tp_ktx2_decode_level_rgbaf / _slice_rgbaf BC6H (uf16 + sf16), ASTC HDR
  • tp_ktx2_read — identifier / header / level index / DFD / KVD, VkFormattp_codec via new tp_vk_format_describe, uni intermediate detected at vkFormat == 0. Zero-copy: level pointers alias the input.
  • tp_ktx2_read_zstd — supercompressionScheme 2, via a caller-supplied decompressor callback (the library carries no zstd dependency). BasisLZ is still refused.
  • tp_ktx2_decode_slice_rgba8 / _rgbaf — cube faces and array layers, not just level 0.
  • tp_ktx2_kv_lookup — key/value data (KTXorientation and friends).
  • tp_ktx2_write_uni / tp_ktx2_uni_size — serialize pre-encoded uni levels as a transcodable KTX2.
  • texcomp gains the public decoders this needs: tc_bc1/bc3/bc5_decompress_rgba8, tc_etc2_decompress_rgba8, tc_eac_decompress_rgba8, tc_bc7_decompress_rgba8 (already existed), tc_astc_decompress_rgba8, tc_bc6h_decompress_rgb16f/_rgbaf, tc_astc_hdr_decompress_rgbaf.

The BC6H and ASTC HDR decoders were promoted, not written — they already existed in test/ as oracles for the encoder gates, and were duplicating machinery the library decoders already had. They now live in the library, with the test headers reduced to shims, so there is one implementation instead of two that can drift.


Security: the parser handles untrusted input

Two bugs, both reachable from a crafted file:

Heap overflow in the Zstd path. The per-level uncompressedByteLength fields were summed into total with no overflow check, and each was only lower-bounded. Two levels declaring ~2^63 wrap that sum to 16, so the single buffer every level inflates into is allocated 16 bytes wide — and zdec is then handed dst_cap = 2^63 pointing into it. A decompressor honours the capacity it is given. ASAN reports heap-buffer-overflow on the regression test against the pre-fix code. Fixed by pinning uncompressedByteLength to the level's exact block-data size, which also kills the decompression-bomb shape.

Level-size lower bound could overflow to zero. With pixelWidth = pixelHeight = 0xFFFFFFFF and any 4×4/16-byte codec, bw * bh * block_bytes is exactly 2^64 — i.e. 0 — so the truncation check accepted a zero-length level. Fixed by bounding the header fields, which also keeps the arithmetic clear of a wrap.

Also: header counts are now range-checked while still unsigned (a top-bit-set levelCount/layerCount used to narrow to a negative int and sail past its limit), the DFD and KVD ranges are bounds-checked, and w*h*4 is computed in 64 bits so it cannot wrap a 32-bit size_t.


Conformance: five bugs found by decoding foreign blocks

Every codec here had round-trip tests, and every one of them passed. Round-trip tests cannot find these bugs — if the encoder and decoder share a wrong assumption they agree with each other perfectly, and if the encoder never emits a shape then nothing ever decodes it. Two things break the circle: point each decoder at an independent encoder's output, and hold each encoder to a rival codec's quality on identical content. Together they found:

1. Every ETC2/EAC block we wrote was transposed. ETC numbers texels down columns (index = x*4 + y); our encoders gathered them row-major while using etcpak-derived bit packing that assumes column-major. A horizontal ramp encoded as a vertical one. Every ETC2/EAC asset this tool has produced would display transposed on a real GPU — including texpipe's EAC_R11 roughness companion and the uni→ETC2 transcode. It survived because nothing in the tree had ever decoded ETC2. Ground truth: Basis Universal, already vendored (basisu_transcoder.cpp:599).

2. BC5_SNORM was unorm data wearing a signed tag. tc_bc5_compress_rgba8 did (void)opt — it ignored its options entirely and stored UNORM bytes — while the DDS and KTX2 writers tagged the container as SNORM (DXGI 84 / VK 142). A GPU sampling that asset reads a stored 200 as −56. Found in a follow-up audit of this branch: the new reader plumbs is_signed from the VkFormat, and the BC5 decoder was ignoring it. Now genuinely implemented — signed int8 endpoints, the signed 6-value palette, and snorm threaded through both the encoder and the decoder.

3. The ETC2 encoder was 22 dB behind BC1. Its ETC1 differential mode packed the 5-bit base at bit 0 and the 3-bit signed delta at bit 3 — overlapping, and both shifted. Per channel the byte is [base:5][delta:3], base in the high bits. Every differential block decoded to a wrong base colour. ETC2 scored 13 dB where BC1 scored 35 dB on the same image; it now scores 36.8 dB, just ahead of BC1, which is what the format should do. Found by building the demo.

4. ASTC HDR CEM 15 endpoints overlapped their own weight data. The dual-plane path wrote 8 endpoint values at colour quant 256 (64 bits from bit 17) underneath 32 weight symbols already occupying the top 52 bits. And the decoder derives the colour quant level from the space the weights leave behind — so it read the already-corrupted bits back at a different level. An ordinary smooth block of 0.65–0.97 decoded to 30208. Also found by the demo. Fixed by deriving the quant level from the space actually available, exactly as the decoder does.

5. ETC2 H-mode distance — wrong bit assembly. 6. ASTC CEM 15 alpha used weight plane 0, so a dual-plane block selecting alpha as its second plane decoded wrong. 7. ASTC LDR interpolation was done in 8 bits; the spec (and every GPU) bit-replicates endpoints to 16 bits, interpolates there, and takes the top byte — off by 1 LSB on 2807 of 4480 blocks. The old comment knew, and waved it through as "agrees within 1 LSB". 8. BC7 undefined behaviour — the alpha lane of ep[][] is read uninitialized for the modes with no alpha bits.

Plus a large ASTC HDR coverage gap: of 1160 astcenc-produced blocks, 535 were rejected and 25 decoded wrong. Mixed-CEM partitions alone were 38% of blocks. Also missing: CEM 2/3 (HDR luminance), CEM 14, LDR modes inside an HDR texture, and CEM 1/5/9/13 (the LDR base+offset modes) — the last of which was a hole in the LDR decoder too.


Every decoder is now validated against an implementation from outside this tree

decoder oracle coverage
BC1 / BC3 / BC5 in-file S3TC reference all modes, partial edge blocks
BC5_SNORM in-file signed reference (no external oracle exists) see caveat below
BC6H bcdec port all 14 modes, uf16 + sf16
BC7 upstream bcdec 320k random blocks, all 8 modes
ETC2 / EAC Mesa 200k random blocks, all 5 RGB modes
ASTC LDR astcenc 4480 astcenc-encoded blocks, exact
ASTC HDR astcenc 2240 encoded + 1856 mutated-CEM + 64 void-extent, exact

Two techniques worth calling out, both now permanent gates:

  • Foreign-block sweeps. astcenc encodes, we decode, and the two must agree texel for texel. This is what caught the ASTC bugs.
  • CEM mutation. astcenc's HDR profile never emits CEM 14, the LDR modes, or an LDR void-extent — no amount of content would test them. But CEMs within one class encode the same number of endpoint values, so rewriting a block's CEM field to another CEM of the same class yields a still-valid block that astcenc will decode. That gives an oracle for the modes it refuses to produce.

Mesa and bcdec were used in a scratchpad to certify in-tree reference decoders and generate golden vectors; neither is vendored, and no new dependency is added.

The tests also gained the missing half — encoder quality floors. ETC2 sat 22 dB behind BC1 for as long as nothing decoded it, because every test pinned the decoder or the encoder's orientation, and none asserted the encoder was any good. test_etc2_quality now holds ETC2 to BC1's standard on identical content (both 4 bpp), and the ASTC HDR RGBA path is pinned by the exact block that broke it.

Every new test was verified non-vacuous by re-breaking the code it covers — e.g. forcing the cube slice offset to zero fails the face test 6×; restoring the old ASTC interpolation fails the LDR gate with exactly 2807 mismatches.


Live browser demo

▶ syoyo.github.io/tinyexr/texcomp/web/texcomp/

One ~600 KB wasm module linking five pure-C11 pieces of the tree: the v3 EXR
decoder (so scene-linear HDR loads directly), tir (resize), texcomp
(compress and decompress), texpipe (mips, KTX2/DDS) and envmap
(equirect → cube/octa). Everything runs locally; no image leaves the tab.

Five panels, each built around a case where the interesting thing is not the compression ratio:

  • Resize & compress — any codec, with the decompressed result and an amplified error view, plus a downloadable .ktx2/.dds written by the real texpipe pipeline and parsed back by our own KTX2 reader.
  • HDR — BC6H / ASTC HDR vs BC7 on a scene-linear EXR. Sweep the exposure and BC7's highlights go flat white: an 8-bit codec clips above 1.0 at encode time, so the data is gone, not merely quantised.
  • Normal maps — ranks codecs by mean angular error in degrees. On the bundled sample: EAC_RG11 4.54°, BC5 4.67°, BC7 12.54° — even though BC7's RGB PSNR is 20.8 dB against BC5's 6.0. PSNR on the raw channels does not tell you whether the surface will light correctly.
  • Cubemap / octahedral, and Mip chain (content-aware, with alpha-coverage preservation).

Sources: a bundled sample, any local file, or fetched live from openexr-images.
Verified by driving the built page in headless Chrome — wasm boots, BC7 hits
36.65 dB, all canvases render, the ranking populates, 9 mip levels build.

Docs: doc/texcomp.md, a README section, make texcomp-web,
and the Pages workflow now publishes it at /texcomp/.


Notes for review

  • Four encoder fixes change shipped output, all of them because the old output was wrong: the ETC2/EAC transposition (every ETC2/EAC block), the ETC2 differential-mode packing (every differential block — and a 23 dB quality jump), BC5_SNORM (whenever bc5.snorm is set), and ASTC HDR CEM 15 (the RGBA HDR path). Existing PSNR floors all still pass.
  • One regression test I wrote was vacuous, and I caught it by A/B-ing. My first ASTC HDR CEM 15 test was a synthetic "photo-like" image that passed identically against the broken encoder — it never entered the dual-plane path. The test that ships uses the exact 4×4 block from the photo (1.34 dB broken, 33.75 dB fixed). Every new test in this PR was checked the same way, by re-breaking the code it covers.
  • BC5_SNORM is the one decoder here validated to a weaker standard. No external oracle exists for signed BC4 — bcdec's BC4/BC5 is unsigned-only and astcenc has no BC codecs — so unlike every other decoder in this PR it is checked against a spec-written reference in the test file plus behavioural pins (the encoded bytes must change with the option; a mid-grey input, meaning x = 0, must store as ~0 signed rather than ~128 unsigned; the round-trip error must be no worse than the unorm path). Worth knowing when reviewing it.
  • A dead RGB8A1 punch-through branch was removed from the new ETC2 decoder: unreachable (nothing maps the RGB8A1 VkFormats), not covered by the Mesa cross-check, and its modifier logic was wrong. The golden vectors are unchanged by the removal, which confirms it was truly dead. Unvalidated code that looks authoritative is worse than absent code.
  • TP_ERROR_NOT_FOUND is a new tp_result value (for tp_ktx2_kv_lookup missing a key). -Werror=switch caught the missing tp_result_string case, which is a good argument for keeping exhaustive switches.
  • make tools-test-all — including the astcenc conformance cross-checks — is green.

🤖 Generated with Claude Code

syoyo and others added 2 commits July 11, 2026 12:17
Add the consumer side of the KTX2 writers so a KTX2 asset can be loaded and
transcoded/decoded per device (Basis-free, built on the uni UASTC intermediate):

- texcomp: expose tc_astc_decompress_rgba8 (public wrapper over the internal
  ASTC LDR decoder) so the transcodable carrier set (uni / ASTC 4x4 / BC7) is
  fully decodable via public API.
- texpipe: tp_ktx2_read (parse identifier/header/level index/DFD, map VkFormat
  -> tp_codec via new tp_vk_format_describe, detect the uni intermediate at
  vkFormat==0; supercompressionScheme 0 only, Zstd/BasisLZ deferred),
  tp_ktx2_decode_level_rgba8 (uni/BC7/ASTC LDR), and tp_ktx2_write_uni /
  tp_ktx2_uni_size (serialize pre-encoded uni levels as a transcodable KTX2).
- test_texpipe: BC7/ASTC/uni write->read->decode round-trips (+ uni->BC7
  transcode, out-of-bounds guards).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Real-world KTX2 (esp. UASTC) is usually Zstd-supercompressed
(supercompressionScheme 2). Add tp_ktx2_read_zstd: like tp_ktx2_read but,
given a tp_zstd_decompress_fn callback, decompresses each level into one
allocator-owned buffer (freed with tp_ktx2_image_free); level data pointers
alias it. The library stays zstd-free — the host wraps its own ZSTD_decompress.
tp_ktx2_read now delegates (scheme 0 = zero-copy as before). Bounds + declared
block-size validation applies to both schemes.

Test: a passthrough (identity) decompressor exercises the scheme-2 allocation /
per-level callback path (byte-identical decode vs scheme 0), plus the
no-decompressor and out-of-bounds rejections.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 11, 2026 06:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds the “consumer side” for KTX2 in texpipe (read + decode/transcode-on-load) and exposes an ASTC LDR RGBA8 decoder in texcomp, enabling a Basis-free workflow built around the uni/UASTC intermediate.

Changes:

  • Add tp_ktx2_read() + tp_ktx2_decode_level_rgba8() for parsing KTX2 headers/level index and decoding supported formats (uni/BC7/ASTC LDR).
  • Add tp_vk_format_describe() to map VkFormattp_codec_desc + tp_codec (+ sRGB flag).
  • Expose tc_astc_decompress_rgba8() and add unit tests covering BC7/ASTC/uni KTX2 round-trips and guards.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
tools/texpipe/test/test_texpipe.c Adds KTX2 read/decode/transcode round-trip tests and malformed-input guards.
tools/texpipe/src/texpipe.c Implements tp_vk_format_describe() and ASTC VkFormat reverse mapping.
tools/texpipe/src/texpipe_internal.h Declares tp_vk_format_describe() for internal use.
tools/texpipe/src/texpipe_container.c Implements KTX2 reader/decoder and uni-in-KTX2 writer helpers.
tools/texpipe/include/texpipe.h Exposes the new public KTX2 read/decode and uni writer APIs/types.
tools/texcomp/src/texcomp_astc_decode.c Adds public ASTC LDR decode wrapper tc_astc_decompress_rgba8().
tools/texcomp/include/texcomp.h Declares tc_astc_decompress_rgba8() in the public API.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +304 to +313
vk = tp_rd_u32(data + 12);
out->width = tp_rd_u32(data + 20);
out->height = tp_rd_u32(data + 24);
layer_count = tp_rd_u32(data + 32);
face_count = tp_rd_u32(data + 36);
level_count = tp_rd_u32(data + 40);
scheme = tp_rd_u32(data + 44);
out->supercompression = scheme;
if (scheme != 0u) return TP_ERROR_UNSUPPORTED; /* Zstd/BasisLZ deferred */

Comment thread tools/texpipe/src/texpipe_container.c Outdated
Comment on lines +355 to +360
bw = ((uint64_t)w + (uint64_t)out->block_w - 1u) / (uint64_t)out->block_w;
bh = ((uint64_t)h + (uint64_t)out->block_h - 1u) / (uint64_t)out->block_h;
expected = bw * bh * (uint64_t)out->block_bytes;
if ((uint64_t)out->num_faces > 1u) expected *= (uint64_t)out->num_faces;
if (out->num_layers > 1) expected *= (uint64_t)out->num_layers;
if (len < expected) return TP_ERROR_INVALID_ARGUMENT; /* truncated level */
Comment on lines +422 to +426
for (l = n - 1; l >= 0; --l) { /* smallest-first, aligned to 16 */
cursor = tp_align_up(cursor, 16u);
if (loff) loff[l] = cursor;
cursor += sizes[l];
}
Comment on lines +447 to +448
need = tp_ktx2_uni_layout(uni_sizes, num_levels, loff);
if (out_size < need) return TP_ERROR_INVALID_ARGUMENT;
Comment on lines +545 to +546
need = (size_t)width * (size_t)height * 4u;
if (out_size < need) return TC_ERROR_INVALID_ARGUMENT;
Comment on lines +378 to +383
w = img->levels[level].width;
h = img->levels[level].height;
need = (size_t)w * (size_t)h * 4u;
if (out_size < need) return TP_ERROR_INVALID_ARGUMENT;
blocks = img->levels[level].data;

Comment on lines +314 to +320
nlev = level_count ? (int)level_count : 1; /* 0 = "generate", treat as 1 */
if (nlev > TP_KTX2_MAX_LEVELS) return TP_ERROR_UNSUPPORTED;
out->num_levels = nlev;
out->num_faces = face_count ? (int)face_count : 1;
out->num_layers = (int)layer_count;
out->vk_format = vk;

Comment thread tools/texpipe/src/texpipe.c Outdated
Comment on lines +188 to +191
/* ASTC block dimensions in VkFormat index order (must match tp_astc_vk_format). */
static const uint8_t tp_astc_dims[14][2] = {
{4, 4}, {5, 4}, {5, 5}, {6, 5}, {6, 6}, {8, 5}, {8, 6},
{8, 8}, {10, 5}, {10, 6}, {10, 8}, {10, 10}, {12, 10},{12, 12}};
@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review: texcomp/texpipe KTX2 reader + transcode-on-load

Nice, well-scoped addition — the consumer side mirrors the existing writer conventions closely (same tp_wr_u32/tp_align_up helpers, same DFD-writing style, same TP_OK/tp_codec_desc patterns), and the round-trip tests (BC7/ASTC/uni write→read→decode + uni→BC7 transcode) are a solid sanity net for the happy path.

Correctness / security — the parser is handling untrusted input, and two spots need hardening

tp_ktx2_read is explicitly documented as guarding "OOB reads on crafted files," which is the right instinct for a format parser, but two of its own arithmetic checks can be defeated by attacker-controlled header fields:

1. expected (level-size lower bound) can integer-overflow to 0 (texpipe_container.c, tp_ktx2_read)

bw = ((uint64_t)w + (uint64_t)out->block_w - 1u) / (uint64_t)out->block_w;
bh = ((uint64_t)h + (uint64_t)out->block_h - 1u) / (uint64_t)out->block_h;
expected = bw * bh * (uint64_t)out->block_bytes;
...
if (len < expected) return TP_ERROR_INVALID_ARGUMENT; /* truncated level */

For a crafted header with pixelWidth = pixelHeight = 0xFFFFFFFF and any 4×4/16‑byte-block codec (BC7, ASTC 4x4, or the uni intermediate), bw = bh = (0xFFFFFFFF+3)/4 = 2^30 exactly, so expected = 2^30 * 2^30 * 16 = 2^64 ≡ 0 (mod 2^64). Since expected is uint64_t, len < 0 is never true, so this check accepts a level with len = 0 (or any tiny length) even though the header claims a ~4-billion-pixel image. tp_ktx2_read then returns TP_SUCCESS with out.levels[0].data/.size pointing at a near-empty region while width/height say otherwise.

This particular width/height pair happens to still be caught later by tp_ktx2_decode_level_rgba8's own out_size < need check (need = w*h*4 doesn't collapse to something small for this pair, so that path is safe) — but the parser's own "prevents OOB reads" contract is broken, and any other consumer of tp_ktx2_image that follows the pattern the API itself recommends (the doc comment for tp_ktx2_decode_level_rgba8 says unsupported codecs should "upload their blocks directly instead," i.e. compute the expected block-data size from width/height/block_w/block_h and read that many bytes from level.data) would read far past the end of the tiny actual buffer — a heap out-of-bounds read/crash on a crafted file.

Suggest validating width/height against a sane maximum (e.g. reject values that would make bw*bh*block_bytes overflow uint64_t, or just cap dimensions to something like 65536) before doing the multiplication, rather than relying on the multiplication itself to saturate correctly.

2. level_count/layer_count/face_count are narrowed from uint32_t to int before range-checking

nlev = level_count ? (int)level_count : 1;
if (nlev > TP_KTX2_MAX_LEVELS) return TP_ERROR_UNSUPPORTED;
...
out->num_layers = (int)layer_count;

Both level_count and layer_count come straight from the file header. A value with the top bit set (e.g. levelCount = 0xFFFFFFFF) becomes a negative int after the cast, which sails past nlev > TP_KTX2_MAX_LEVELS (a negative number is never > 32). The subsequent size < 80u + (size_t)nlev * 24u check also gets fooled: (size_t)nlev wraps to a huge value whose *24u + 80u itself wraps around size_t a second time, landing back on a small number for nlev == -1 — so the truncation guard silently passes too. The per-level loop then doesn't execute (0 < -1 is false), so there's no direct OOB inside this function, but tp_ktx2_read returns TP_SUCCESS with a tp_ktx2_image whose num_levels/num_layers are negative. That's surprising for a "validated" struct, and it also disables the num_layers > 1 guard in tp_ktx2_decode_level_rgba8 for the same reason (a genuinely huge/array layerCount can be smuggled through as a small negative number, skipping the "single-face / non-array only" rejection and having expected computed without the layer multiplier).

Recommend comparing the unsigned level_count/layer_count/face_count against their limits before casting to int, e.g. if (level_count > TP_KTX2_MAX_LEVELS) return TP_ERROR_UNSUPPORTED; ahead of the cast.

Minor / style

  • tp_ktx2_write_uni: (void)level_h; right before if (... || !level_h || ...) and later level_h[0] is actually dereferenced — the (void) cast looks like a leftover from an earlier revision where the parameter was unused; it's misleading as-is since level_h is used.
  • Also in tp_ktx2_write_uni: level_w/level_h are accepted as full per-level arrays, but only index [0] is ever read — per-level dimensions for levels 1..num_levels-1 are silently ignored (the reader re-derives them via width >> l, matching the KTX2 spec's implicit-mip-chain convention, so this is consistent, just a slightly misleading signature). Might be worth a one-line comment noting that levels beyond 0 must follow the standard shift-by-level pyramid, since nothing currently enforces or documents that constraint for the caller.
  • Test coverage is good for the happy paths and for one corrupted-offset case, but doesn't exercise the overflow class above (huge pixelWidth/pixelHeight, or levelCount/layerCount with the top bit set). Given the module explicitly targets "crafted files," a regression test pinning tp_ktx2_read's behavior on those inputs (and, once fixed, asserting rejection) would be valuable alongside the existing "reject out-of-bounds level offset" case.

Everything else

  • tp_astc_decompress_rgba8's new public wrapper looks correctly bounded (tmp[144*4] sized for the max 12×12 footprint, and block_x/block_y > 12 is rejected before the internal decode touches that buffer).
  • tp_vk_format_describe's VkFormat → codec mapping (BC/ETC2/EAC fixed table + ASTC LDR/HDR ranges) looks consistent with the existing writer-side tp_codec_describe/vk_format assignment, and the block-dimension table order matches the documented VkFormat enumeration order.
  • tp_from_tc's switch over tc_result is exhaustive against the enum in texcomp.h, so no silent fallthrough there.

Nice, self-contained PR overall — the two arithmetic-overflow gaps above are worth closing before this parser is pointed at untrusted/downloaded KTX2 assets, but the rest of the design (block-codec dispatch, uni transcode path, layout math) is solid and matches the surrounding code's conventions.

…review)

The KTX2 parser is documented as guarding OOB reads on crafted files, but
several of its own arithmetic checks could be defeated by header fields:

- pixelWidth/pixelHeight = 0xFFFFFFFF with a 4x4/16B codec made the level-size
  lower bound bw*bh*block_bytes exactly 2^64, i.e. 0, so the truncated-level
  check accepted a zero-length level.
- levelCount/layerCount/faceCount were narrowed to int before range-checking,
  so a top-bit-set value became negative and slipped past the limits (and past
  the num_layers guard in tp_ktx2_decode_level_rgba8).

Range-check those fields while still unsigned, against explicit bounds
(TP_KTX2_MAX_DIM / TP_KTX2_MAX_LAYERS / faces <= 6), which also keeps the
level-size product well clear of a 64-bit wrap. Also:

- validate the declared DFD range is in-bounds, per tp_ktx2_read's contract
- compute w*h*4 in 64 bits in tp_ktx2_decode_level_rgba8 and
  tc_astc_decompress_rgba8 (wraps on a 32-bit size_t, defeating out_size)
- tp_ktx2_uni_layout: return 0 on size_t overflow, rejected by
  tp_ktx2_write_uni / tp_ktx2_uni_size (uni_sizes[] is public-API caller data)
- hoist the duplicated ASTC block-dimension table to one file-scope copy
- drop the stale (void)level_h in tp_ktx2_write_uni; document that only
  level_w[0]/level_h[0] reach the header (levels 1.. are the implicit pyramid)

Tests: crafted pixelWidth/pixelHeight/levelCount/layerCount/faceCount, an
out-of-bounds DFD range, and an overflowing uni level-size array. Each new
CHECK fails without the corresponding guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@syoyo

syoyo commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Review feedback applied in the latest push (all 8 Copilot comments + the two arithmetic-overflow findings from the Claude review).

Reader hardening (texpipe_container.c)

  • Header counts (levelCount/layerCount/faceCount) are now range-checked while still unsigned, before the int cast — a top-bit-set value used to become negative and sail past nlev > TP_KTX2_MAX_LEVELS (and past the num_layers > 1 guard in tp_ktx2_decode_level_rgba8).
  • pixelWidth/pixelHeight are bounded by a new TP_KTX2_MAX_DIM (65536). This closes the 0xFFFFFFFF x 0xFFFFFFFF case where bw*bh*block_bytes was exactly 2^64 ≡ 0, making the truncated-level check accept a zero-length level. With dims/faces/layers bounded, the level-size product tops out around 2^46, so it cannot wrap.
  • The declared DFD range is now validated as in-bounds (and not overlapping the level index), per tp_ktx2_read’s documented contract.
  • tp_ktx2_decode_level_rgba8 and tc_astc_decompress_rgba8 compute w*h*4 in 64 bits first — it wraps on a 32-bit size_t, which would let the out_size check pass.
  • tp_ktx2_uni_layout returns 0 on size_t overflow; tp_ktx2_write_uni rejects need == 0 (and tp_ktx2_uni_size already returns 0) — uni_sizes[] is caller data on a public API.

Cleanups

  • The ASTC block-dimension table is hoisted to one file-scope tp_astc_dims used by both tp_astc_vk_format and tp_vk_format_describe (the 14/27 range literals now derive from it too).
  • Dropped the stale (void)level_h; in tp_ktx2_write_uni, and documented that only level_w[0]/level_h[0] reach the header — levels 1.. must be the standard halving pyramid, which is how the reader re-derives them.

Tests — new guards in test_texpipe.c for crafted pixelWidth/pixelHeight/levelCount/layerCount/faceCount, an out-of-bounds DFD range, and an overflowing uni level-size array. I verified each new CHECK fails when its guard is removed, so they are real regression tests rather than tautologies. make texpipe-c11-gate texpipe-test texcomp-c11-gate texcomp-test all pass.

@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review: KTX2 reader + transcode-on-load (texcomp/texpipe)

Overall this is a solid, well-documented addition — the doc comments are thorough, the write→read→decode round-trip tests (BC7/ASTC/uni) are good, and the earlier "review" commit already hardened several header-parsing arithmetic paths against crafted files (unsigned range-checks before narrowing to int, 64-bit size products, etc.). One issue in the new Zstd path looks like a real, exploitable bug though.

Bug: heap buffer overflow in tp_ktx2_read_zstd via wraparound of summed level sizes

tools/texpipe/src/texpipe_container.c:413-450

uint64_t total = 0;
...
for (l = 0; l < nlev; ++l)
    total += tp_rd_u64(data + 80u + (size_t)l * 24u + 16u);   // ulen, fully attacker-controlled
if (total == 0u || total > (uint64_t)(size_t)-1)
    return TP_ERROR_INVALID_ARGUMENT;
owned = (uint8_t *)tp_alloc(a, (size_t)total);
...
for (l = 0; l < nlev; ++l) {
    ...
    uint64_t ulen = tp_rd_u64(e + 16);
    ...
    if (off > size || clen > (uint64_t)size - off ||
        ulen < tp_ktx2_level_expected(out, w, h)) {   // only a LOWER bound on ulen
        ...
    }
    got = zdec(user, owned + cursor, (size_t)ulen, data + (size_t)off, (size_t)clen);
    ...
    cursor += (size_t)ulen;
}

Each level's uncompressedByteLength (ulen) is read straight from the file with no upper bound — the only check is ulen < tp_ktx2_level_expected(...), i.e. "at least this big." With up to TP_KTX2_MAX_LEVELS (32) levels, an attacker can pick per-level ulen values that individually are huge but whose 64-bit sum wraps around to something tiny. Concretely, with 2 levels: ulen[0] = 2^63, ulen[1] = 2^63 + 100total wraps to 100. total == 0 is false and total > (size_t)-1 is never true on a 64-bit build (size_t == uint64_t, so this check is dead code there), so tp_alloc(a, 100) succeeds.

Then, per level, ulen is used unwrapped as dst_cap for the host's decompressor: zdec(user, owned + cursor, (size_t)ulen /* = 2^63 */, ...). The actual allocation is only 100 bytes, but the callback is told it has ~2^63 bytes of room. If the attacker's crafted (Zstd-)compressed payload for that level decompresses to anything more than the real remaining buffer (e.g. a few hundred bytes — trivial for a compression bomb, and well within dst_cap), the decompressor writes past owned's actual allocation — a heap buffer overflow driven entirely by the KTX2 file contents. The later got != ulen check can't help; the OOB write already happened inside zdec.

This defeats the exact threat model the earlier hardening commit called out ("crafted files" with header arithmetic designed to slip past guards) — it just does it through the per-level ulen field instead of pixelWidth/pixelHeight/counts.

Suggested fix directions:

  • Track remaining capacity explicitly and pass dst_cap = total - cursor (clamped to the real allocation) instead of the raw ulen.
  • Detect overflow incrementally while summing (if (total + ulen < total) → reject) rather than trusting the finished sum, and treat "total > SIZE_MAX" as meaningful only on real 32-bit builds.
  • Since block-compressed formats have a deterministic level size, consider requiring ulen == tp_ktx2_level_expected(...) (exact match) rather than just a lower bound — that alone would close this without the capacity bookkeeping.

Test coverage gap

test_ktx2_zstd_scheme (tools/texpipe/test/test_texpipe.c) only exercises a single-level BC7 KTX2 through the scheme-2 path, plus one truncated-payload case. It doesn't cover multiple levels, so the summation-wraparound above wouldn't be caught by the new tests. Worth adding a regression case with ≥2 levels and an intentionally overflowing ulen pair (mirroring the existing tp_ktx2_uni_layout overflow test, which is a nice pattern already used elsewhere in this PR).

Minor / nice-to-haves

  • tp_astc_dims dedup (shared by tp_astc_vk_format and the new tp_vk_format_describe) is a good cleanup — same data, no behavior change.
  • tc_astc_decompress_rgba8 and tp_ktx2_decode_level_rgba8 correctly do the widthheight4 overflow check in 64 bits before narrowing to size_t — consistent with the pattern used elsewhere in the header-parsing fixes.
  • tp_ktx2_uni_layout's overflow guards (cursor > smax - 15u, sizes[l] > smax - cursor) are correct and well-tested (test_ktx2_read_roundtrip's "uni layout overflow" case).
  • Everything on the scheme-0 (zero-copy) path looks safe: len/off are bounded by the actual input buffer size directly (len <= size - off), so it doesn't share the scheme-2 problem above.

Nice work on the defensive parsing overall — the one Zstd-path issue above is the only thing I'd block on before merging.

…gth)

tp_ktx2_read_zstd summed the per-level uncompressedByteLength fields into
`total` with no overflow check, and only lower-bounded each one against the
level's block-data size. Two levels declaring ~2^63 wrap the sum to a small
number, so the single `owned` buffer every level inflates into was allocated
that small -- and zdec was then called with dst_cap = ulen = 2^63 pointing into
it. A decompressor honours dst_cap, so a crafted file overflows the heap. ASAN
reports heap-buffer-overflow on the new regression test without this fix.

Pin uncompressedByteLength to the level's exact block-data size (per KTX2 the
levels are tightly packed, so equality holds) instead of lower-bounding it.
With the dimension/level caps already in place that bounds `total` by
TP_KTX2_MAX_LEVELS * 2^46, so the sum cannot wrap, and it also rejects the
decompression-bomb shape (a tiny file claiming a huge inflated size).

Tests: crafted wrapping ulen pair, oversized ulen, and a re-read of the
untouched stream to pin that the exactness check does not reject valid input.
Also document the input-size contract of tc_astc_decompress_rgba8 (the block
stream length is not passed) and tp_ktx2_uni_size's 0 return.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@syoyo

syoyo commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Second local review round — found and fixed one heap overflow that the first round (and both bot reviews) missed, in the scheme-2 (Zstd) path of tp_ktx2_read_zstd.

The bug. The per-level uncompressedByteLength fields were summed into total with no overflow check, and each one was only lower-bounded against the level's block-data size (ulen < expected → reject). A file declaring two levels of ~2^63 wraps that sum to 16, so the single buffer that all levels inflate into is allocated 16 bytes wide — and zdec is then invoked as zdec(user, owned + cursor, /*dst_cap=*/ulen, ...) with ulen = 2^63. A decompressor honours the capacity it is given, so a real ZSTD_decompress writes straight past the allocation. Under ASAN the new regression test reports heap-buffer-overflow against the pre-fix code.

The fix. uncompressedByteLength is now pinned to the level's exact block-data size rather than lower-bounded. Per KTX2 the levels are tightly packed block data, so equality is the correct contract, and it does double duty:

  • total is bounded by TP_KTX2_MAX_LEVELS * 2^46 given the dimension caps added last round, so the sum provably cannot wrap;
  • it also rejects the decompression-bomb shape (a tiny file claiming a huge inflated size, which previously just meant a huge malloc).

This is a nice illustration of why the dimension caps from round 1 were worth adding: they are what makes the new bound on total provable rather than merely plausible.

Also in this push: documented the input-size contract of tc_astc_decompress_rgba8 (the block-stream length is not a parameter, so the caller owns that bound) and tp_ktx2_uni_size's 0-on-error return.

Tests: crafted wrapping ulen pair, oversized ulen, plus a re-read of the untouched stream to pin that the new exactness check does not reject valid input. Verified the wrap test ASAN-aborts without the fix. texpipe-c11-gate, texpipe-test, texcomp-c11-gate, texcomp-test all pass.

@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review: KTX2 reader + transcode-on-load

Nicely scoped addition — it mirrors the existing writer's conventions closely (same tp_rd_u32/tp_wr_u32 helpers, tir_allocator pattern, tp_codec_desc shape) and the hostile-input hardening is thorough and well-commented (64-bit overflow guards before every size computation, exact-match uncompressedByteLength checks to block decompression-bomb style attacks in the Zstd path, narrow-before-int-cast reasoning on the header counts). Test coverage for the malicious-input paths (wrapping products, negative-after-narrowing counts, out-of-bounds offsets, DFD range) is a real strength — good adversarial test design.

Correctness

  • tp_ktx2_write_uni silently ignores level_w[1..]/level_h[1..] (texpipe_container.c ~L565-609). The function takes full level_w/level_h arrays but only ever reads index 0 (confirmed only out+20/out+24 are written from them). The header doc does call this out ("Only level_w[0]/level_h[0] reach the header... must be the standard halving pyramid, which is how the reader re-derives them"), so it's intentional, but the API shape invites a caller bug: passing per-level dimensions that don't actually follow max(1, w>>l) produces no error — the file is written and reads back successfully, just decoding to the wrong dimensions on any level > 0 (silent corruption, not a crash). Either validate level_w[l]/level_h[l] against the halving formula and reject on mismatch, or simplify the signature to take only width0/height0 so the ignored-array footgun disappears.

  • Test coverage gap for the above: test_ktx2_read_roundtrip only exercises tp_ktx2_write_uni/tp_ktx2_read with num_levels == 1. The multi-level uni path — the one with the documented halving-pyramid contract — isn't round-tripped at all. Worth adding a 2+ level uni case (matching the existing BC7/ASTC tests, which do check img.levels[1].width).

  • tp_ktx2_image_free leaves dangling level pointers: after freeing _owned and nulling it, img->levels[*].data still points into the freed buffer. Calling tp_ktx2_decode_level_rgba8 on an already-freed image is a use-after-free rather than a clean failure. Low severity (API-misuse only, not attacker-reachable via file content), but cheap to harden — e.g. zero the levels[] array in tp_ktx2_image_free too.

  • faceCount isn't validated against {1, 6}: values 2–5 pass the <= 6 bound check in tp_ktx2_read and parse "successfully" (just later rejected by tp_ktx2_decode_level_rgba8's num_faces != 1 check). Not a safety issue, just a minor spec-conformance gap — a faceCount of 3 isn't a valid KTX2 cubemap declaration and arguably should be rejected at parse time.

Security / robustness

  • The 64-bit arithmetic hardening throughout tp_ktx2_read_zstd is solid: the tp_ktx2_level_expected overflow analysis is correct (worst case ~2^46, nowhere near wrapping), the "pin uncompressedByteLength to the exact expected size" choice (rather than just lower-bounding it) correctly prevents both the sum-overflow and decompression-bomb variants, and the tests directly targeting these (wrapping pixelWidth/layerCount/etc., wrapping uncompressedByteLength sums, oversized single-level lengths) demonstrate the guards actually work rather than just asserting they exist.
  • tc_astc_decompress_rgba8's bound on block_x/block_y (> 12 rejected) is exactly the ASTC 2D footprint ceiling and lines up with the internal tmp[144*4] scratch buffer in tc_astc_decode_image_rgba8 — verified the write pattern in the block decoder ((y*bx+x)*4+c for y<by, x<bx) can't exceed that buffer for any bx,by <= 12, including non-canonical footprints, so this is memory-safe even for a caller-supplied block_x/block_y that isn't one of the 14 standard ASTC block sizes.
  • As documented, tc_astc_decompress_rgba8/tc_bc7_decompress_rgba8 don't take an input-buffer-size parameter and trust the caller's bound on astc/bc7 — consistent with the rest of the texcomp decoder family, and safely used from the KTX2 path only because tp_ktx2_read already validated level.size against the expected block-data size before handing it off. Worth keeping in mind if these decoders ever get called directly from a new, less-careful call site.

Minor

  • tp_from_tc's mapping of TC_ERROR_CORRUPT -> TP_ERROR_INVALID_ARGUMENT loses a bit of error-code fidelity (a genuinely corrupt block stream reads the same as a bad argument to the caller), but tp_result has no CORRUPT variant to map to, so this is a reasonable compromise given the existing enum.

Overall this is a careful, well-tested piece of parsing code for an untrusted format — the main thing worth addressing before merge is the tp_ktx2_write_uni multi-level contract (either enforce it or drop the unused parameters), plus a test that actually exercises multi-level uni round-tripping.

syoyo and others added 4 commits July 11, 2026 16:54
…utomatic

It ran on every pull_request open/synchronize, i.e. a full review on every push.
Trigger it on demand instead:

  gh workflow run claude-code-review.yml -f pr_number=<N>

Mentioning @claude in a PR comment still works and is unchanged (claude.yml,
which is already comment-gated).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ads back

tp_ktx2_decode_level_rgba8 could only decode uni, BC7 and ASTC LDR -- the other
codecs texpipe can *write* as KTX2 came back TP_ERROR_UNSUPPORTED, so the reader
could not round-trip its own output.

Add tc_bc1/bc3/bc5_decompress_rgba8 (public, mirroring tc_bc7_decompress_rgba8)
and dispatch them from the KTX2 decoder. The BC1 colour block and the BC4 block
are shared, matching how the encoders already share tc_encode_bc1_color_block /
tc_encode_bc4_block: BC3 is a BC4 alpha block plus a 4-colour BC1 block, BC5 is
two BC4 blocks. BC1 honours the 3-colour punch-through mode (c0 <= c1) even
though our encoder never emits it, since foreign files do. BC5 carries two
channels, so it decodes to R=x, G=y, B=0, A=255 (documented; a normal-map
consumer reconstructs z itself).

Still unsupported, by nature rather than omission: BC6H (HDR -- RGBA8 is the
wrong target) and ETC2/EAC (no decoder yet).

Tests: test_texcomp cross-checks all three decoders texel-for-texel against the
independent reference block decoders already in that file (written from the S3TC
spec, not from the library) on a 13x7 surface so the edge blocks are partial --
bit-exact, plus short-output/short-stride/null rejection. test_texpipe adds
BC1/BC3/BC5 KTX2 write->read->decode round-trips (38.6 / 38.6 / 51.7 dB).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tp_ktx2_decode_level_rgba8 bailed with TP_ERROR_UNSUPPORTED on anything with
faceCount 6 or layerCount > 1, so a cubemap or array written by tp_write_ktx2 /
tp_write_ktx2_array could be parsed but never read back.

Add tp_ktx2_decode_slice_rgba8(img, level, layer, face, ...): a level stores its
slices in KTX2 order (layer, face), each a tightly packed block image of the
level's dimensions, so the slice offset is (layer * num_faces + face) * the
per-image block size. tp_ktx2_decode_level_rgba8 is now the (0, 0) case and no
longer rejects cube/array input -- it decodes the first slice.

The slice is bounds-checked against the level's size before dispatch: the block
decoders take no input length, and the reader only lower-bounds a level, so a
hand-built tp_ktx2_image could otherwise walk off the end.

Tests: the cube test paints face f with a constant blue f*40 and asserts each
decoded face reports its own blue, which pins the per-face offsets and their
order (verified non-vacuous: forcing slice=0 fails it 6x). The array test
decodes all 3 layers and checks identical layers decode identically, plus
out-of-range layer/face rejection in both.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The reader ignored the KVD block entirely -- it did not expose it, and (like the
DFD before the last round) did not even bounds-check the declared range, so a
crafted kvdByteOffset/Length went unnoticed.

Bounds-check the KVD range in tp_ktx2_read and record it on tp_ktx2_image, then
walk it lazily in tp_ktx2_kv_lookup: entries are {u32 keyAndValueByteLength, key
NUL value, pad to 4}. The walk rejects an entry that runs past the block and one
whose key has no NUL (a strcmp there would read past the entry). The KVD is
stored uncompressed even in a supercompressed file, so it aliases the source for
both schemes. New TP_ERROR_NOT_FOUND distinguishes "no such key" from "malformed
KVD"; tp_result_string covers it (-Werror=switch caught the omission).

Tests: our writers emit no KVD, so the test hand-builds a KTX2 (which also gives
the reader its first test against a file it did not produce). It pins the value
of KTXorientation, reaches a second key that is only findable through correct
padding arithmetic (verified non-vacuous: dropping the pad fails it), and covers
prefix-is-not-a-match, absent key, overlong entry, an out-of-bounds KVD range,
and a file with no KVD at all.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@syoyo

syoyo commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Closed the reader-side gaps this PR had deliberately deferred. Three commits, each with its tests verified non-vacuous (I re-broke the code and confirmed the new checks fail).

1. BC1/BC3/BC5 decoders — the reader could not round-trip its own writer. tp_ktx2_decode_level_rgba8 handled only uni, BC7 and ASTC LDR; every other codec texpipe can write as KTX2 came back TP_ERROR_UNSUPPORTED. Added tc_bc1/bc3/bc5_decompress_rgba8 (public, mirroring tc_bc7_decompress_rgba8) and dispatched them. The BC1 colour block and the BC4 block are shared between them, mirroring how the encoders already share tc_encode_bc1_color_block / tc_encode_bc4_block. BC1 honours the 3-colour punch-through mode even though our encoder never emits it, since foreign files do; BC5 decodes to R=x, G=y, B=0, A=255 (documented — a normal-map consumer reconstructs z itself).

Still unsupported, by nature rather than omission: BC6H (HDR, so RGBA8 is the wrong target) and ETC2/EAC (no decoder yet).

The tests cross-check all three decoders texel-for-texel against the independent reference block decoders already living in test_texcomp.c — written from the S3TC spec, not from the library — on a 13x7 surface so the edge blocks are partial. Bit-exact.

2. Cube faces and array layers (tp_ktx2_decode_slice_rgba8). Decoding bailed on faceCount == 6 or layerCount > 1, so a cubemap or array written by tp_write_ktx2 / tp_write_ktx2_array parsed but could never be read back. A level stores its slices in KTX2 order (layer, face), so the offset is (layer * num_faces + face) * per-image block size; tp_ktx2_decode_level_rgba8 is now the (0, 0) case. The slice is bounds-checked against the level size before dispatch — the block decoders take no input length, and the reader only lower-bounds a level.

The cube test paints face f with a constant blue of f*40 and asserts each decoded face reports its own blue, which pins the per-face offsets and their order rather than just that a decode succeeded.

3. Key/value data (tp_ktx2_kv_lookup). The KVD block was ignored entirely — not exposed, and (like the DFD before the last round) not even bounds-checked, so a crafted kvdByteOffset/Length went unnoticed. Now range-checked at parse and walked lazily on lookup, rejecting an entry that runs past the block or a key with no NUL (a strcmp there would read past the entry). New TP_ERROR_NOT_FOUND separates "no such key" from "malformed KVD" — -Werror=switch usefully caught the missing tp_result_string case.

Since our writers emit no KVD, the test hand-builds a KTX2, which incidentally gives the reader its first test against a file it did not produce itself.

make tools-test is green.

…coders

Adding the decoders exposed a conformance bug in the encoders that had gone
unnoticed because nothing in the tree had ever decoded ETC2: every block we
emitted was transposed.

ETC numbers the texels of a block down columns first -- texel i is at
(x = i/4, y = i%4). The bit packing in texcomp_etc2.c/texcomp_eac.c is
etcpak-derived and already assumes that (selector i -> bit i, and the flip and
planar axes are both derived from i the same way), but the block *gather* was
row-major (block[yy*4+xx]). So a horizontal ramp encoded as a vertical one, and
a horizontal ramp picked flip=1 (top/bottom) where the spec wants flip=0
(left/right). Everything we wrote as ETC2/EAC -- including texpipe's EAC_R11
roughness companion and the uni->ETC2 transcode -- would have displayed
transposed on a real GPU. Ground truth: Basis Universal, vendored in
deps/basisu, computes the selector index as x*4+y (basisu_transcoder.cpp:599).

Fix: gather column-major. That one change also makes tc_etc1_subset_for_split
and the planar encoder agree with the spec -- they read i as (i&3, i>>2) and
(i/4, i&3) respectively, i.e. they disagreed with each other under the old
gather and both land on the spec's meaning under the new one. Quality is
unaffected (orientation only); the existing PSNR floors all still pass.

New texcomp_etc2_decode.c: tc_etc2_decompress_rgba8 (all five RGB modes --
individual, differential, T, H, planar -- plus ETC2 RGBA's EAC alpha half) and
tc_eac_decompress_rgba8 (R11/RG11, 11-bit reconstruction scaled to 8). Wired
into tp_ktx2_decode_level_rgba8, so the only codec the KTX2 reader still cannot
decode is BC6H, which is HDR and needs a float target rather than RGBA8.

Validation: cross-checked against Mesa's independent ETC decoder
(src/mesa/main/texcompress_etc.c, MIT) over 200k random blocks -- zero
mismatches, with all five modes hit (individual 100k, differential 82k, T 6.2k,
H 6.0k, planar 5.6k). That found two real bugs in my first draft: the H-mode
distance bit assembly, and applying a unit step for a zero EAC-alpha multiplier
(R11 does that, the alpha variant does not). Golden vectors decoded by Mesa are
baked into test_texcomp.c so all five modes stay pinned in-tree -- our encoder
only emits three of them, so a round-trip test alone can never reach T and H.
Plus orientation tests (encode->decode must preserve an X ramp) that fail on the
old row-major gather, and ETC2/EAC KTX2 round-trips in test_texpipe.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@syoyo

syoyo commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Added the ETC2/EAC decoders — and doing so uncovered a conformance bug in the ETC2/EAC encoders: every block we emit was transposed.

The encoder bug

ETC numbers the texels of a block down columns first — texel i is at (x = i/4, y = i%4). The bit packing in texcomp_etc2.c / texcomp_eac.c is etcpak-derived and already assumes exactly that (selector i goes to bit i, and the flip axis and the planar axes are both derived from i the same way). But the block gather was row-major (block[yy*4+xx]).

Consequence: a horizontal ramp encodes as a vertical one, and a horizontal ramp selects flip=1 (top/bottom) where the spec wants flip=0 (left/right). Everything we wrote as ETC2/EAC would have displayed transposed on a real GPU — including texpipe's EAC_R11 roughness companion and the uni→ETC2 transcode. It survived because nothing in the tree had ever decoded ETC2: no reference decoder, no conformance test, and a round-trip test cannot catch it when the encoder is the only thing that reads its own output.

Ground truth is in-tree: Basis Universal (deps/basisu/basisu_transcoder.cpp:599) computes the selector index as x * 4 + y.

The fix is one line per encoder — gather column-major. Notably that also repairs tc_etc1_subset_for_split and the planar encoder, which read i as (i&3, i>>2) and (i/4, i&3) respectively: they disagreed with each other under the old gather and both land on the spec's meaning under the new one, which is strong evidence the gather was the single defect. Quality is unaffected (orientation only) — all existing PSNR floors still pass.

The decoders

New texcomp_etc2_decode.c: tc_etc2_decompress_rgba8 (all five RGB modes — individual, differential, T, H, planar — plus ETC2 RGBA's EAC alpha half) and tc_eac_decompress_rgba8 (R11/RG11). Wired into tp_ktx2_decode_level_rgba8, so the only codec the KTX2 reader still cannot decode is BC6H, which is HDR and needs a float target rather than RGBA8.

Validation

You asked me to fetch a reference decoder rather than ship T/H unvalidated — that paid off. I cross-checked against Mesa's independent ETC decoder (texcompress_etc.c, MIT) over 200k random blocks: zero mismatches, with every mode hit (individual 100k, differential 82k, T 6.2k, H 6.0k, planar 5.6k). It caught two real bugs in my first draft that no round-trip test could have: the H-mode distance bit assembly, and applying a unit step for a zero EAC-alpha multiplier (R11 does that; the alpha variant does not).

Golden vectors decoded by Mesa are baked into test_texcomp.c so all five modes stay pinned in-tree — our encoder only ever emits three of them, so a round-trip test alone can never reach T and H. Mesa itself is used only as a scratchpad oracle; nothing is vendored. Plus orientation tests (encode→decode must preserve an X ramp; they fail on the old gather) and ETC2/EAC KTX2 round-trips.

make tools-test is green. KTX2 read+decode PSNR across the full set: bc7 45.2, bc1 38.6, bc3 38.6, bc5 51.7, etc2_rgb 50.2, etc2_rgba 50.2, eac_r11 51.3, eac_rg11 51.3, astc 41.1, uni 45.9 dB.

BC6H was the last codec the KTX2 reader could not decode, and it could not be
fixed on the RGBA8 path: it is HDR, so 8-bit is the wrong target.

texcomp gains tc_bc6h_decompress_rgb16f (FP16, BC6H's natural output) and
tc_bc6h_decompress_rgbaf (float RGBA, alpha 1 -- BC6H carries no alpha).
`is_signed` selects sf16 over uf16; the two disagree on endpoint unquantisation,
so it has to be right.

The decoder is the bcdec port that already lived in test/bc6h_ref_decode.h as
the oracle for texcomp-bc6h-gate. It moves to src/texcomp_bc6h_decode.c, so
there is one implementation rather than two copies that can drift; the test
header becomes a shim over the public API and both BC6H gates keep working
unchanged. Their purpose is unchanged too: they still validate the in-house
BC6H encoder against a decoder of independent lineage. The gate reports exactly
the same PSNR as before the move (50.07 / 34.90 dB), which pins the behaviour.

texpipe gains tp_ktx2_decode_level_rgbaf / tp_ktx2_decode_slice_rgbaf. The
slice-locating and bounds-checking logic is now a shared helper, so the float
and 8-bit paths cannot drift in their overflow checks (the surface size is
computed in 64 bits for both). tp_ktx2_image grows is_signed, mapped from the
VkFormat (BC6H_UFLOAT 143 vs BC6H_SFLOAT 144).

The LDR codecs deliberately stay on the RGBA8 path rather than being widened to
float, and ASTC HDR still has no decoder; both return TP_ERROR_UNSUPPORTED from
the float path, and BC6H returns TP_ERROR_UNSUPPORTED from the 8-bit one.

Tests: BC6H KTX2 write -> read -> float decode for both uf16 and sf16 (41.2 /
39.8 dB), alpha == 1.0, short-output rejection, and the rgba8 path refusing
BC6H. Verified non-vacuous: hard-coding is_signed = 0 drops sf16 to 7.9 dB and
fails the floor.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@syoyo

syoyo commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Added the BC6H float decode path — the KTX2 reader now decodes every codec texpipe can write, except ASTC HDR (noted below).

BC6H could not be fixed on the RGBA8 path, because it is HDR and 8-bit is simply the wrong target. So texcomp gains tc_bc6h_decompress_rgb16f (FP16 — BC6H's natural output) and tc_bc6h_decompress_rgbaf (float RGBA, alpha 1.0 since BC6H carries no alpha), and texpipe gains tp_ktx2_decode_level_rgbaf / tp_ktx2_decode_slice_rgbaf.

No new decoder was written. The bcdec port already lived in test/bc6h_ref_decode.h as the oracle for texcomp-bc6h-gate; it moves to src/texcomp_bc6h_decode.c so there is one implementation rather than two copies that can drift apart. The test header becomes a thin shim over the public API, and both BC6H gates keep working unchanged — including their purpose, since they still validate the in-house BC6H encoder against a decoder of independent lineage. The gate reports exactly the same PSNR as before the move (50.07 / 34.90 dB), which pins that the move changed no behaviour.

is_signed matters and is now plumbed through. BC6H's sf16 and uf16 variants disagree on endpoint unquantisation, so decoding one as the other is badly wrong. tp_ktx2_image grows an is_signed flag mapped from the VkFormat (BC6H_UFLOAT 143 vs BC6H_SFLOAT 144). The test pins this rather than assuming it: hard-coding is_signed = 0 drops the sf16 round-trip from 39.8 dB to 7.9 dB and fails the floor.

While here I factored the slice-locating and bounds-checking logic into a shared helper, so the float and 8-bit paths cannot drift apart in their overflow checks — the surface size is computed in 64 bits for both, parameterised by the output texel size.

Deliberate non-goals, each returning TP_ERROR_UNSUPPORTED rather than guessing: the LDR codecs stay on the RGBA8 path rather than being widened to float, BC6H is refused on the 8-bit path, and ASTC HDR still has no decoder — it is the one remaining gap in the writer set. There is a pure-C ASTC HDR reference decoder in test/astc_hdr_ref_decode.h that could be promoted the same way BC6H just was, if you want that closed too.

Tests: BC6H KTX2 write → read → float decode for both variants (uf16 41.2 dB, sf16 39.8 dB), alpha == 1.0, short-output rejection, and the rgba8 path correctly refusing BC6H. make tools-test is green.

…r's last gap

ASTC HDR was the one codec texpipe could write but not read back. Like BC6H it
has no meaningful RGBA8 form, so it belongs on the float path.

The pure-C HDR decoder already existed as test/astc_hdr_ref_decode.h, the oracle
for texcomp-astc-hdr-gate. It moves into src/texcomp_astc_decode.c -- not as a
copy, but retargeted onto that file's existing block-mode / ISE / partition /
infill machinery, which was until now duplicated by the parallel aref_* copy in
the test tree. The test header becomes a shim over the public API, so the HDR
tests and the astcenc cross-check keep working, and their purpose is unchanged:
texcomp-astc-hdr-gate still validates the decoder block for block against
astcenc's conformant HDR decoder, and reports the same numbers as before the
move (const 99.00, gradient 58.12, cem15 54.74 dB).

New public tc_astc_hdr_decompress_rgbaf; texpipe dispatches TP_CODEC_ASTC_HDR to
it from tp_ktx2_decode_slice_rgbaf.

Coverage is stated honestly in the header rather than implied: the set the
texcomp HDR encoder emits -- HDR void-extent, CEM 7 (RGB base+scale), CEM 11
(RGB direct) and CEM 15 (RGB + HDR alpha), all subsets sharing one CEM. A
foreign file using CEM 14 (HDR RGB + LDR alpha), mixed-CEM partitions, or an LDR
block / LDR void-extent inside an HDR texture gets TC_ERROR_UNSUPPORTED rather
than a guess.

With this, tp_ktx2_decode_* covers every codec texpipe can write: BC1/3/5/7,
ETC2, EAC, ASTC LDR and uni on the RGBA8 path; BC6H and ASTC HDR on the float
path.

Tests: ASTC HDR KTX2 write -> read -> float decode (43.5 dB), plus the rgba8
path correctly refusing it. make tools-test-all (incl. the astcenc conformance
cross-checks) is green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@syoyo

syoyo commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Promoted the ASTC HDR decoder — the KTX2 reader now decodes every codec texpipe can write.

Like BC6H, ASTC HDR has no meaningful RGBA8 form, so it belongs on the float path. New public tc_astc_hdr_decompress_rgbaf, dispatched from tp_ktx2_decode_slice_rgbaf.

It is not a copy. The pure-C HDR decoder already existed as test/astc_hdr_ref_decode.h (the oracle for texcomp-astc-hdr-gate), but it was written against astc_ref_decode.h — a parallel copy of the block-mode / ISE / partition / infill machinery that the library decoder (src/texcomp_astc_decode.c) already has. So rather than move 185 lines of duplication into the library, I retargeted the HDR block decode onto the library's existing tacd_* machinery, which happens to mirror the test's aref_* helpers one-for-one. The test header becomes a shim over the public API.

The validation story is unchanged and still strong: texcomp-astc-hdr-gate cross-checks the decoder block for block against astcenc's conformant HDR decoder, and it reports exactly the same numbers as before the move (const 99.00, gradient 58.12, anti-correlated 47.87, brightness-ramp 34.15, cem15 54.74 dB) — which is what pins that the retarget changed no behaviour.

Coverage is stated honestly in the header rather than implied. This decodes the set the texcomp HDR encoder emits: HDR void-extent, CEM 7 (RGB base+scale), CEM 11 (RGB direct) and CEM 15 (RGB + HDR alpha), with all subsets sharing one CEM. A foreign file using CEM 14 (HDR RGB + LDR alpha), mixed-CEM partitions, or an LDR block / LDR void-extent inside an HDR texture gets TC_ERROR_UNSUPPORTED rather than a guess. That is a real limit for third-party ASTC HDR assets, and it is the one thing here I would flag for follow-up — astcenc is vendored, so extending to the full HDR set could be validated the same way the existing gate does.

Where the reader stands now

path codecs
tp_ktx2_decode_*_rgba8 uni, BC1, BC3, BC5, BC7, ETC2 RGB/RGBA, EAC R11/RG11, ASTC LDR
tp_ktx2_decode_*_rgbaf BC6H (uf16 + sf16), ASTC HDR

Tests: ASTC HDR KTX2 write → read → float decode at 43.5 dB, plus the rgba8 path correctly refusing it. make tools-test-all — which includes the astcenc conformance cross-checks — is green.

…lta modes)

The promoted HDR decoder only handled the corner of the format our own encoder
emits. Measuring it against blocks astcenc produces showed how big that gap was:
of 1160 astcenc HDR blocks, 535 were rejected outright and 25 decoded wrong.

Closed, and the measurement is now a gate. What was missing:

- Mixed-CEM partitions -- 439 of those 1160 blocks (38%). The per-subset CEMs
  are a 3n-bit string whose low 4 bits sit in the header field and whose high
  (3n-4) bits sit just below the weights; the dual-plane selector then sits below
  *those*, which the old code also got wrong whenever both were present.
- CEM 2 / 3 (HDR luminance, large and small range) -- 96 blocks. astcenc reaches
  for these on near-gray content, so they are common in real assets.
- CEM 14 (HDR RGB + LDR alpha), which astcenc never emits but a foreign encoder
  may.
- LDR endpoint modes and LDR void-extent inside an HDR texture: legal, and they
  decode through the UNORM16 rule rather than the LNS one.
- CEM 1 / 5 / 9 / 13, the LDR base+offset (delta) modes, which tacd_decode_cem
  never implemented at all. This also closes the same hole in the *LDR* ASTC
  decoder, which the KTX2 reader uses -- a foreign ASTC LDR file using a delta
  mode was previously rejected.
- A real bug in the existing CEM 15 path: alpha always used weight plane 0, so a
  dual-plane block that selects alpha as its second plane decoded wrong. That is
  the 25 mismatches; our encoder never emits that shape, so nothing caught it.

The decode model now mirrors the spec's HDR profile exactly: endpoints in a
16-bit domain (LDR ones bit-replicated up from 8-bit), lerped as
(e0*(64-w) + e1*w + 32) >> 6, then converted to FP16 per lane by either the LNS
rule (HDR lanes) or the UNORM16 rule (LDR lanes).

Validation, now permanent in texcomp-astc-hdr-gate: astcenc encodes, we decode,
and the two must agree texel for texel -- 2240 blocks over 6 block footprints,
3 presets and 10 content kinds. Because astcenc's HDR profile never emits CEM 14,
the LDR modes or an LDR void-extent, a second sweep reaches them by mutation:
CEMs within one class encode the same number of endpoint values, so rewriting a
block's CEM field to another CEM of the same class yields a still-valid block
that astcenc will decode -- giving an oracle for the modes it will not produce.
That covers CEM 0/1/4/5/6/8/9/10/12/13/14 (1856 blocks) plus LDR void-extent
(64). All 16 endpoint modes, both void-extent kinds and mixed-CEM now cross-check
against astcenc with zero mismatches.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@syoyo

syoyo commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Closed the CEM 14 / mixed-CEM gap — and the measurement turned it into a much bigger find than expected.

The gap was far larger than "CEM 14 + mixed-CEM"

Rather than reason from the spec, I pointed the decoder at blocks astcenc produces (the vendored, conformant encoder) and measured. Of 1160 astcenc HDR blocks: 535 rejected outright and 25 decoded wrong.

What was actually missing:

  • Mixed-CEM partitions — 439 of 1160 blocks (38%). The per-subset CEMs are a 3n-bit string whose low 4 bits sit in the header field and whose high (3n−4) bits sit just below the weights; the dual-plane selector then sits below those. The old code had the selector at a fixed offset, so it was also wrong whenever both were present.
  • CEM 2 / 3 (HDR luminance, large and small range) — 96 blocks. astcenc reaches for these on near-gray content, so they are common in real assets. Neither of us listed these in the original gap.
  • CEM 14, as flagged.
  • LDR endpoint modes and LDR void-extent inside an HDR texture — legal, and they decode via the UNORM16 rule rather than the LNS one.
  • CEM 1/5/9/13, the LDR base+offset (delta) modes — never implemented in tacd_decode_cem at all. This also closes the same hole in the LDR ASTC decoder that the KTX2 reader uses: a foreign ASTC LDR file using a delta mode was previously rejected.
  • A real bug in the existing CEM 15 path: alpha always used weight plane 0, so a dual-plane block selecting alpha as its second plane decoded wrong. Those are the 25 mismatches. Our encoder never emits that shape, so nothing had ever caught it.

Validation is now a permanent gate, not a one-off

texcomp-astc-hdr-gate gained two sweeps that must agree with astcenc texel for texel:

  1. Foreign blocks — astcenc encodes, we decode: 2240 blocks across 6 block footprints, 3 presets, 10 content kinds.
  2. Mutated CEMs — astcenc's HDR profile never emits CEM 14, the LDR modes, or an LDR void-extent, so those would stay untested no matter how much content I threw at it. But CEMs within one class encode the same number of endpoint values, so rewriting a block's CEM field to another CEM of the same class yields a still-valid block that astcenc will decode — an oracle for the modes it refuses to produce. That reaches CEM 0/1/4/5/6/8/9/10/12/13/14 (1856 blocks) plus LDR void-extent (64).

Result: all 16 endpoint modes, both void-extent kinds, and mixed-CEM cross-check against astcenc with zero mismatches.

The decode model now mirrors the spec's HDR profile exactly — endpoints in a 16-bit domain (LDR ones bit-replicated up from 8-bit), lerped as (e0*(64-w) + e1*w + 32) >> 6, then converted to FP16 per lane by the LNS rule (HDR lanes) or the UNORM16 rule (LDR lanes).

make tools-test-all is green.

…terpolation)

Pointed the LDR decoder at blocks astcenc produces, the same way as the HDR one.
No block was rejected -- the delta-mode and mixed-CEM work from the previous
commit already covered the format -- but 2807 of 4480 blocks decoded *wrong*,
every one of them by exactly 1 LSB.

That was systematic, not a bug in any one mode: we interpolated the texels in 8
bits, (e0*(64-w) + e1*w + 32) >> 6, while the spec (and therefore every GPU)
bit-replicates the 8-bit endpoints to 16 bits, interpolates there, and takes the
top byte. The two agree within 1 LSB, which is why it never showed up in a PSNR
test and why the old comment waved it through as "agrees within 1 LSB" -- but a
decoder whose job is to reproduce what the GPU shows should reproduce it exactly.

Fixed in both the library decoder and the test-tree reference decoder (which is
the LDR encoder's oracle, so it was measuring the encoder against a target no GPU
would produce).

texcomp-astc-arm-gate gains the sweep that found this: astcenc encodes, we
decode, and the two must agree exactly -- 4480 blocks over 6 block footprints, 3
presets and 5 content kinds, covering the base+offset endpoint modes, mixed-CEM
partitions and dual-plane blocks. Verified it fails (2807 mismatches) against the
old interpolation.

Both ASTC decoders are now cross-checked against astcenc on foreign blocks: LDR
4480 blocks and HDR 2240 + 1856 mutated-CEM + 64 LDR void-extent, all exact.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@syoyo

syoyo commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Validated the LDR ASTC decoder against astcenc too — and it found a systematic conformance bug.

2807 of 4480 blocks decoded wrong, every one by exactly 1 LSB

Pointing the LDR decoder at astcenc-produced blocks, nothing was rejected — the delta-mode and mixed-CEM work from the previous commit already covered the format. But 2807 of 4480 blocks decoded wrong, and the error was always exactly 1 LSB, never 2. That shape says systematic, not a bug in any one mode.

The cause: we interpolated texels in 8 bits, (e0*(64-w) + e1*w + 32) >> 6. The spec — and therefore every GPU — bit-replicates the 8-bit endpoints to 16 bits, interpolates there, and takes the top byte.

The two agree within 1 LSB, which is exactly why no PSNR test ever caught it. In fact the reference decoder's own header comment waved it through:

"Final texel interpolation uses the 8-bit model ... the spec's 16-bit endpoint expansion agrees with this within 1 LSB."

That is true, and it is still the wrong call for a decoder whose job is to reproduce what the GPU displays. A KTX2 consumer decoding an ASTC texture should get the texels the hardware would get, not ones that are merely close.

Fixed in both the library decoder and the test-tree reference decoder — the latter matters because it is the LDR encoder's oracle, so the encoder was being validated against a target no GPU would produce.

The sweep is now a permanent gate

texcomp-astc-arm-gate gained the check that found this: astcenc encodes, we decode, and the two must agree exactly — 4480 blocks across 6 block footprints, 3 presets and 5 content kinds, covering the base+offset endpoint modes, mixed-CEM partitions and dual-plane blocks. I verified it is non-vacuous by restoring the old interpolation: it fails with 2807 mismatches.

Both ASTC decoders are now bit-exact against astcenc on foreign blocks

decoder foreign-block coverage result
ASTC LDR 4480 astcenc-encoded blocks 0 mismatched
ASTC HDR 2240 astcenc-encoded + 1856 mutated-CEM + 64 LDR void-extent 0 mismatched

make tools-test-all is green.

Worth noting the pattern this PR keeps repeating: every decoder, once pointed at an independent encoder's output, revealed bugs that round-trip tests structurally cannot reach — transposed ETC2 blocks, the ETC2 H-mode distance, the ASTC CEM 15 alpha plane, and now the ASTC LDR interpolation model. Four separate defects, all in code paths our own encoder never exercises.

…osed)

Last decoder without foreign-block validation. The xbc7 gate already cross-checks
the library BC7 decoder against test/bc7_ref_decode.h, but only on blocks our own
encoder produced, which reach a handful of the eight modes.

Cross-checked instead on random blocks, which reach every mode, partition,
rotation, p-bit and index-selection combination a foreign encoder might emit (a
BC7 block has no invalid bit patterns once its mode prefix is set). Three-way,
against upstream bcdec (iOrange, public domain / MIT) as an external oracle:
320k blocks, 40k per mode, library vs bcdec, in-tree reference vs bcdec, and the
two of ours against each other -- zero mismatches everywhere. The decoder is
correct; unlike ETC2 and ASTC, nothing was wrong with it.

It did expose undefined behaviour, though. For the modes with no alpha bits
(0/1/3) the alpha lane of ep[][] is never read from the block, yet the p-bit and
bit-expansion steps run over all four lanes -- so it shifted an indeterminate,
possibly negative int. The decoded output was fine (that lane is overwritten with
0xFF for those modes), but it is UB, and UBSan trips on it the moment the blocks
stop coming from our own encoder. Zero-initialize ep[][].

The random-block sweep is now permanent in texcomp-test (2000 blocks per mode,
all 8 modes) against the in-tree reference decoder -- which this commit certifies
bit-exact against upstream bcdec, so agreeing with it is a conformance statement
rather than self-consistency. No new dependency: bcdec was used in the scratchpad
to certify the reference, not vendored.

Every block decoder in texcomp is now validated against an implementation from
outside this tree: BC1/3/5 vs the in-file S3TC reference, BC7 vs bcdec, ETC2/EAC
vs Mesa, ASTC LDR and HDR vs astcenc, BC6H vs the bcdec port.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@syoyo

syoyo commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Validated the BC7 decoder against foreign blocks — the last one without that coverage.

The decoder is correct

Unlike ETC2 and ASTC, nothing was wrong with it. Cross-checked on random blocks, which reach every mode, partition, rotation, p-bit and index-selection combination a foreign encoder might emit (a BC7 block has no invalid bit patterns once its mode prefix is set). Three-way, with upstream bcdec (iOrange, public domain / MIT) as an external oracle:

comparison blocks mismatches
library decoder vs bcdec 320k (40k × 8 modes) 0
in-tree bc7_ref_decode.h vs bcdec 320k 0
library vs in-tree reference 320k 0

It did expose undefined behaviour

For the modes with no alpha bits (0/1/3), the alpha lane of ep[][] is never read from the block — but the p-bit and bit-expansion steps run over all four lanes, so it was shifting an indeterminate, possibly negative int. The decoded output was fine (that lane is overwritten with 0xFF for those modes), which is why it never surfaced. But it is UB, and UBSan trips on it the instant the blocks stop coming from our own encoder. Zero-initializing ep[][] fixes it; the output is unchanged (still bit-exact vs bcdec afterwards).

That is a nice illustration of the point: the existing xbc7 gate already cross-checked this decoder against the in-tree reference — but only on blocks our own encoder produced, so it never fed it a mode-0/1/3 block with arbitrary bits.

Permanent, with no new dependency

The random-block sweep is now in texcomp-test: 2000 blocks per mode, all 8 modes, against the in-tree reference decoder — which this commit certifies bit-exact against upstream bcdec, so agreeing with it is a conformance statement rather than self-consistency. bcdec was used in the scratchpad to certify the reference, not vendored.

Every decoder in texcomp is now externally validated

decoder oracle coverage
BC1 / BC3 / BC5 in-file S3TC reference all modes, partial edge blocks
BC6H bcdec port all 14 modes, uf16 + sf16
BC7 upstream bcdec 320k random blocks, all 8 modes
ETC2 / EAC Mesa 200k random blocks, all 5 RGB modes
ASTC LDR astcenc 4480 astcenc-encoded blocks
ASTC HDR astcenc 2240 encoded + 1856 mutated-CEM + 64 void-extent

Final tally for this line of work — pointing each decoder at an independent encoder found five defects that round-trip tests structurally cannot reach: transposed ETC2/EAC blocks (every asset we shipped), the ETC2 H-mode distance, the ASTC CEM 15 alpha plane, the ASTC LDR interpolation model, and now BC7 UB. All in code paths our own encoder never exercises.

make tools-test-all is green.

@syoyo syoyo changed the title texcomp/texpipe: KTX2 reader + transcode-on-load, expose ASTC decoder texcomp/texpipe: KTX2 reader + full decoder set; harden parser; fix transposed ETC2/EAC output Jul 11, 2026
…gned tag)

Audit finding. tc_bc5_compress_rgba8 did `(void)opt` -- it ignored its options
entirely and stored UNORM bytes -- while tc_dds_write_bc5_memory and
tp_codec_describe *tagged the container* as SNORM (DXGI 84 / VK 142) whenever
bc5.snorm was set. So a BC5_SNORM asset we wrote held unsigned data under a
signed label, and a GPU sampling it read a stored 200 as -56. Same class of bug
as the transposed ETC2 blocks: output that is silently wrong on real hardware.
Pre-existing, and not reachable from either CLI -- only a library caller setting
opt.bc5.snorm hits it -- but the KTX2 reader added in this branch made it worse,
since it plumbed is_signed from the VkFormat and then the BC5 decoder ignored it.

Now implemented rather than mislabelled:

- Encode: endpoints chosen in signed space and stored as int8
  (tc_encode_bc4_block_snorm). The public API still takes UNORM8, so `snorm`
  selects the storage form, not the caller's convention: a byte u denotes
  x = 2*(u/255) - 1 in [-1,1] -- the normal-map convention -- and is stored as
  round(x * 127), which is exactly what a GPU sampling BC5_SNORM reads back.
- Decode: tc_decode_bc4_block_snorm reads int8 endpoints and uses the signed
  6-value palette (-1.0 / +1.0 in place of the unsigned 0 / 255), re-expressed
  as UNORM8 on the way out. tc_bc5_decompress_rgba8 takes a `snorm` argument to
  match, and the KTX2 reader passes img->is_signed.

Tests: the encoded bytes must actually change with the option (they did not
before); the block decode must match an independent signed reference decoder
written from the spec in the test file; a mid-grey input (x = 0) must store as
~0 signed rather than ~128 unsigned -- the check that fails if the encoder
quietly stores unorm bytes; and the round-trip error must be no worse than the
unorm path (measured: 6 vs 5 LSB worst case). texpipe adds a BC5_SNORM KTX2
write -> read -> decode round trip (53.6 dB), verified non-vacuous: ignoring
is_signed drops it to 6.0 dB.

Also drops the RGB8A1 punch-through branch from the new ETC2 decoder. It was
unreachable (nothing maps the RGB8A1 VkFormats), unvalidated by the Mesa
cross-check, and its modifier logic was wrong -- a trap for whoever wires up
RGB8A1 later. The ETC2 golden vectors are unchanged by its removal.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@syoyo

syoyo commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Local audit round: two findings, both fixed

Re-reviewed the branch with fresh eyes, focused on the code this PR adds.

1. BC5_SNORM was unorm data wearing a signed tag (real, GPU-visible)

tc_bc5_compress_rgba8 did (void)opt — it ignored its options entirely and stored UNORM bytes — while tc_dds_write_bc5_memory and tp_codec_describe tagged the container as SNORM (DXGI 84 / VK 142) whenever bc5.snorm was set.

So a BC5_SNORM asset we wrote held unsigned data under a signed label: a GPU sampling it reads a stored 200 as −56. Same class as the transposed ETC2 blocks — output that is silently wrong on real hardware.

It is pre-existing and not reachable from either CLI (only a library caller setting opt.bc5.snorm hits it). But this branch made it worse: the new reader plumbs is_signed from the VkFormat, and the BC5 decoder then ignored it — so we would have misdecoded a foreign BC5_SNORM file too, and left a set-but-unused flag as a trap.

Now actually implemented:

  • Encode — endpoints chosen in signed space, stored as int8. The public API still takes UNORM8, so snorm selects the storage form, not the callers convention: a byte u denotes x = 2*(u/255) - 1 (the normal-map convention) and is stored as round(x * 127) — exactly what a GPU sampling BC5_SNORM reads back.
  • Decode — int8 endpoints and the signed 6-value palette (−1.0 / +1.0 in place of the unsigned 0 / 255); tc_bc5_decompress_rgba8 gains a snorm argument and the KTX2 reader passes img->is_signed.

Tests pin all of it: the encoded bytes must actually change with the option (they did not before); the block decode must match an independent signed reference decoder written from the spec; a mid-grey input (x = 0) must store as ~0 signed, not ~128 unsigned — that is the check that fails the moment the encoder quietly stores unorm bytes; and the round-trip error must be no worse than the unorm path (measured 6 vs 5 LSB). Plus a BC5_SNORM KTX2 round trip at 53.6 dB, verified non-vacuous — ignoring is_signed drops it to 6.0 dB.

2. Dead ETC2 punch-through branch (cleanliness / trap)

The new ETC2 decoder carried an RGB8A1 branch that was unreachable (nothing maps the RGB8A1 VkFormats), not covered by the Mesa cross-check, and whose modifier logic I had written wrong. Unvalidated code that looks authoritative is worse than absent code, so I removed it. The ETC2 golden vectors are unchanged by the removal, confirming it was truly dead.

Everything else audited clean

The parser hardening, the Zstd exactness check, the slice/bounds helper shared by the 8-bit and float paths, the KVD walk, and the ETC2/ASTC/BC7/BC6H decoders all held up on re-read — the last four are pinned by the foreign-block gates anyway.

make tools-test-all is green.

syoyo and others added 4 commits July 11, 2026 19:45
macos-arm64-clang-tools-c failed to build: the BC5_SNORM test added in a6b8327
calls abs(), but test_texcomp.c never included <stdlib.h>. glibc leaks the
declaration in through another header, so gcc/Linux built it fine; clang does
not, and treats the implicit declaration as an error.

Verified with clang -Werror -Wimplicit-function-declaration locally, and swept
the other tool test files for the same latent gap (none).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ping bits

Found while building the browser demo: ETC2 scored ~13 dB where BC1 scored ~35 dB
on the same image. That is not a format limitation -- ETC2 has a planar mode and
should edge BC1 out -- so it was a bug.

Per channel an ETC1 differential byte is [base:5][delta:3]: the 5-bit base
occupies the HIGH five bits and the 3-bit signed delta the low three. The
encoder packed the base at bit 0 and the delta at bit 3, so the two overlapped
and both were shifted. Every differential block decoded to a wrong base colour.
The flat and individual paths pack correctly, which is why solid blocks and some
flat-ish blocks still looked right, and why the orientation test and the Mesa
golden vectors (which only pin the decoder) all passed.

With the fix, ETC2 goes from 13.03 dB to 36.79 dB on the probe image -- now just
ahead of BC1 at 35.16 dB, which is what the format should do.

This is the second bug in this encoder that existed only because nothing ever
decoded ETC2 (the first was the transposed block gather). The root cause of both
is the same gap in the tests: they pinned the decoder against Mesa and pinned the
encoder's *orientation*, but nothing asserted the encoder was any GOOD -- only
that it round-tripped the modes it happened to emit.

So close that gap: test_etc2_quality holds ETC2 to BC1's standard on identical
content (both 4 bpp), with an absolute 30 dB floor and a "within 2 dB of BC1"
floor. Verified non-vacuous -- it fails at 13.56 dB against the old packing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
web/texcomp: one ~600 KB wasm module linking five pure-C11 pieces of the tree --
the v3 EXR decoder (so scene-linear HDR sources load directly), tir (resize),
texcomp (compress + decompress), texpipe (mips, KTX2/DDS) and envmap (equirect ->
cube/octa). Everything runs locally; no image leaves the tab.

Five panels, each built around a case where the interesting thing is not the
compression ratio:

- Resize & compress: any codec, side by side with the decompressed result and an
  amplified error view, plus a downloadable .ktx2/.dds that a GPU tool will open
  (written by the real texpipe pipeline, and parsed back by our own KTX2 reader).
- HDR: BC6H / ASTC HDR against BC7 on a scene-linear EXR. Sweep the exposure and
  BC7's highlights go flat white -- an 8-bit codec clips above 1.0 at *encode*
  time, so the data is gone, not merely quantised.
- Normal maps: ranks codecs by mean angular error in degrees rather than PSNR.
  On the bundled sample it reports EAC_RG11 4.54deg and BC5 4.67deg ahead of BC7
  at 12.54deg -- even though BC7's RGB PSNR is 20.8 dB against BC5's 6.0. That is
  the whole point: PSNR on the raw channels does not tell you whether the surface
  will light correctly.
- Cubemap / octahedral: reproject a latlong EXR and compress with BC6H.
- Mip chain: texpipe's content-aware pyramid.

Sources: a bundled sample, any local file, or fetched live from
AcademySoftwareFoundation/openexr-images (same index the viewer demo uses).

The wasm API (tcw_*) is pipeline-shaped rather than a 1:1 mirror of the C
libraries: the module holds the source, the working image, the last payload and
the last decode, and JS just asks for tonemapped previews -- so no large buffers
cross the boundary per frame.

Also: doc/texcomp.md (codecs, containers, the CG/VFX cases, and the
decoder-validation story), a README section pointing at both, `make texcomp-web`,
and the Pages workflow now publishes the demo at /texcomp/.

Verified by driving the built page in headless Chrome: wasm boots, the sample
loads, BC7 compresses to 36.65 dB, all three canvases render, the normal-map
ranking populates and the mip strip builds 9 levels.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The RGBA (CEM 15) HDR path emitted blocks that decoded to values like 30208 from
perfectly ordinary content -- a smooth block of 0.65..0.97. astcenc decodes those
blocks to the same garbage, which is how we know the encoder was at fault and not
our decoder.

The dual-plane sub-path wrote its 8 endpoint values at colour quant 256 -- 64
bits starting at bit 17 -- underneath 32 dual-plane weight symbols that already
occupied the top 52 bits. The two overlapped. Worse, the decoder derives the
colour quant level from whatever space the weights leave behind, so it read the
(already corrupted) bits back at a *different* level entirely.

Fix: derive the colour quant level from the space actually available, exactly as
the decoder does, and pack, score and write the endpoints at that level. The
single-plane path did have room for quant 256, but it was assuming so rather than
checking -- and that assumption is precisely what broke the dual-plane path, so
it now derives the level too.

On the photo that surfaced this: -39.36 dB -> 33.29 dB, and the decoded range
goes from [0, 30208] back to [0.017, 1.09]. The astcenc cross-check gate is
unchanged (cem15 gradient still 54.74 dB), so nothing regressed.

Regression test: the exact 4x4 block from the photo. Worth noting my first
attempt at this test was VACUOUS -- a synthetic "photo-like" image that passed
identically against the broken encoder, because it never entered the dual-plane
path at all. The real block fails at 1.34 dB and passes at 33.75 dB. The range
assertion is the load-bearing one: a blown-up endpoint shows up there long before
a PSNR number explains why.

Also refresh doc/texcomp.md (this gap is closed; the bug count from
foreign-block/rival-codec validation is now eight) and rebuild the web demo,
which can go back to noting that CEM 15 is fine.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@syoyo syoyo changed the title texcomp/texpipe: KTX2 reader + full decoder set; harden parser; fix transposed ETC2/EAC output texcomp/texpipe: KTX2 reader + full decoder set; fix 8 codec bugs; live browser demo Jul 11, 2026
@syoyo
syoyo merged commit 4820536 into release Jul 11, 2026
14 checks passed
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.

2 participants