clamp origin texel before narrowing to int in load_image_block#648
clamp origin texel before narrowing to int in load_image_block#648sahvx655-wq wants to merge 1 commit into
Conversation
|
As per other PR - I'm going to investigate clamping input values on load. |
|
Makes sense, same as the other PR. Handling it on load is the better place: it fixes the whole class once at the boundary and keeps the clamp out of the per-block path, so what I added here becomes redundant. Happy to drop this in favour of that. One thing worth flagging so the load-time scrub covers this case: unlike the angular one, this isn't inf/NaN. Under the HDR-RGB/LDR-alpha profile the RGB lanes are already LNS-bounded to [0, 65535], but the alpha lane takes the unorm route (input scaled by 65535) and stays unbounded, so a large but finite alpha (input around 60000) reaches float_to_int at roughly 3.9e9, which is above INT_MAX. UBSan trapped it at astcenc_image.cpp:254 straight from a plain astcenc_compress_image call. So the entry clamp wants to bound finite alpha magnitude as well as strip the non-finite lanes, otherwise this path stays exposed. I'll leave the PR open in case you'd rather I retarget it, but no objection to you handling it on load. |
|
Yep - all the "unorm" input paths need to be clamped to ensure that the value is not scaled up more than it should be. Unit tests and input clamps added in #651 - rejecting this one. Thanks for the bug report. |
When compressing with the HDR-RGB/LDR-alpha profile, load_image_block builds the origin texel for each block and feeds blk.texel(0) straight into float_to_int() so it can be re-encoded through the LNS path. The RGB lanes have already passed through float_to_lns() and sit in [0, 65535], but the alpha lane takes the LDR unorm route and is just the input value scaled by 65535. A crafted HDR source (an .exr or .hdr carrying a large alpha) therefore presents a float well above INT_MAX to the scalar float_to_int(), which is undefined behaviour. UBSan flags it at astcenc_image.cpp:254 from an ordinary astcenc_compress_image() call: "3.9321e+09 is outside the range of representable values of type int".
The narrowed alpha lane is discarded by the later lns_mask select, so the value is never used for that channel, but the conversion still runs across all four lanes and trips the UB regardless. Clamping the vector into the valid LNS range [0, 65535] before the narrowing mirrors the clampzo guard this file already applies ahead of every other float_to_int/float_to_int_rtn, keeps the result bit-identical for in-range data, and folds NaN onto the low bound. Putting the clamp at the conversion site keeps the encoder sound whatever the image loaders hand it.