From 5a1ae5d65d4ed0146ed7c32bdb47cd1494b00177 Mon Sep 17 00:00:00 2001 From: Richard Geldreich Date: Tue, 18 Nov 2025 17:27:51 -0500 Subject: [PATCH] Fixing a very rare 1-bit overflow in ETC1S SSE optimized colorspace error calculations (which are 32-bit vs. the plain C++ code's 64-bit). Ultimately the output would still be correct and transcodable, but this could cause a compressor assertion check to fail. Found during compressor fuzz testing. --- encoder/basisu_enc.cpp | 128 +++++++++++++++++++++++++++++ encoder/basisu_enc.h | 153 +++++++++++++++++++++++------------ encoder/basisu_etc.cpp | 5 +- encoder/basisu_kernels_imp.h | 89 ++++++++++---------- 4 files changed, 280 insertions(+), 95 deletions(-) diff --git a/encoder/basisu_enc.cpp b/encoder/basisu_enc.cpp index 2c31474..ba20a2e 100644 --- a/encoder/basisu_enc.cpp +++ b/encoder/basisu_enc.cpp @@ -3987,5 +3987,133 @@ namespace basisu return true; } + + static void rasterize_line(image& dst, int xs, int ys, int xe, int ye, int pred, int inc_dec, int e, int e_inc, int e_no_inc, const color_rgba& color) + { + int start, end, var; + + if (pred) + { + start = ys; end = ye; var = xs; + for (int i = start; i <= end; i++) + { + dst.set_clipped(var, i, color); + if (e < 0) + e += e_no_inc; + else + { + var += inc_dec; + e += e_inc; + } + } + } + else + { + start = xs; end = xe; var = ys; + for (int i = start; i <= end; i++) + { + dst.set_clipped(i, var, color); + if (e < 0) + e += e_no_inc; + else + { + var += inc_dec; + e += e_inc; + } + } + } + } + void draw_line(image& dst, int xs, int ys, int xe, int ye, const color_rgba& color) + { + if (xs > xe) + { + std::swap(xs, xe); + std::swap(ys, ye); + } + + int dx = xe - xs, dy = ye - ys; + if (!dx) + { + if (ys > ye) + std::swap(ys, ye); + for (int i = ys; i <= ye; i++) + dst.set_clipped(xs, i, color); + } + else if (!dy) + { + for (int i = xs; i < xe; i++) + dst.set_clipped(i, ys, color); + } + else if (dy > 0) + { + if (dy <= dx) + { + int e = 2 * dy - dx, e_no_inc = 2 * dy, e_inc = 2 * (dy - dx); + rasterize_line(dst, xs, ys, xe, ye, 0, 1, e, e_inc, e_no_inc, color); + } + else + { + int e = 2 * dx - dy, e_no_inc = 2 * dx, e_inc = 2 * (dx - dy); + rasterize_line(dst, xs, ys, xe, ye, 1, 1, e, e_inc, e_no_inc, color); + } + } + else + { + dy = -dy; + if (dy <= dx) + { + int e = 2 * dy - dx, e_no_inc = 2 * dy, e_inc = 2 * (dy - dx); + rasterize_line(dst, xs, ys, xe, ye, 0, -1, e, e_inc, e_no_inc, color); + } + else + { + int e = 2 * dx - dy, e_no_inc = (2 * dx), e_inc = 2 * (dx - dy); + rasterize_line(dst, xe, ye, xs, ys, 1, -1, e, e_inc, e_no_inc, color); + } + } + } + + // Used for generating random test data + void draw_circle(image& dst, int cx, int cy, int r, const color_rgba& color) + { + assert(r >= 0); + if (r < 0) + return; + + int x = r; + int y = 0; + int err = 1 - x; + + while (x >= y) + { + dst.set_clipped(cx + x, cy + y, color); + dst.set_clipped(cx + y, cy + x, color); + dst.set_clipped(cx - y, cy + x, color); + dst.set_clipped(cx - x, cy + y, color); + dst.set_clipped(cx - x, cy - y, color); + dst.set_clipped(cx - y, cy - x, color); + dst.set_clipped(cx + y, cy - x, color); + dst.set_clipped(cx + x, cy - y, color); + + ++y; + + if (err < 0) + { + err += 2 * y + 1; + } + else + { + --x; + err += 2 * (y - x) + 1; + } + } + } + + void set_image_alpha(image& img, uint32_t a) + { + for (uint32_t y = 0; y < img.get_height(); y++) + for (uint32_t x = 0; x < img.get_width(); x++) + img(x, y).a = (uint8_t)a; + } } // namespace basisu diff --git a/encoder/basisu_enc.h b/encoder/basisu_enc.h index 5bdb783..9c5d63d 100644 --- a/encoder/basisu_enc.h +++ b/encoder/basisu_enc.h @@ -31,9 +31,6 @@ // This module is really just a huge grab bag of classes and helper functions needed by the encoder. -// If BASISU_USE_HIGH_PRECISION_COLOR_DISTANCE is 1, quality in perceptual mode will be slightly greater, but at a large increase in encoding CPU time. -#define BASISU_USE_HIGH_PRECISION_COLOR_DISTANCE (0) - #if BASISU_SUPPORT_SSE // Declared in basisu_kernels_imp.h, but we can't include that here otherwise it would lead to circular type errors. extern void update_covar_matrix_16x16_sse41(uint32_t num_vecs, const void* pWeighted_vecs, const void* pOrigin, const uint32_t *pVec_indices, void* pMatrix16x16); @@ -175,6 +172,20 @@ namespace basisu inline int bounds_check_incl(int v, int l, int h) { (void)v; (void)l; (void)h; assert(v >= l && v <= h); return v; } inline uint32_t bounds_check_incl(uint32_t v, uint32_t l, uint32_t h) { (void)v; (void)l; (void)h; assert(v >= l && v <= h); return v; } + inline bool equal_rel_tol(float a, float b, float rel_tol) + { + float diff = std::fabs(a - b); + float max_abs = std::max(std::fabs(a), std::fabs(b)); + return diff <= (max_abs * rel_tol); + } + + inline bool equal_rel_tol(double a, double b, double rel_tol) + { + double diff = std::fabs(a - b); + double max_abs = std::max(std::fabs(a), std::fabs(b)); + return diff <= (max_abs * rel_tol); + } + inline uint32_t clz(uint32_t x) { if (!x) @@ -1072,48 +1083,15 @@ namespace basisu return color_distance(c0.r, c0.g, c0.b, c1.r, c1.g, c1.b); } - // TODO: Allow user to control channel weightings. - inline uint32_t color_distance(bool perceptual, const color_rgba &e1, const color_rgba &e2, bool alpha) + // Original library color_distance(), for testing + inline uint32_t color_distance_orig(bool perceptual, const color_rgba& e1, const color_rgba& e2, bool alpha) { if (perceptual) { -#if BASISU_USE_HIGH_PRECISION_COLOR_DISTANCE - const float l1 = e1.r * .2126f + e1.g * .715f + e1.b * .0722f; - const float l2 = e2.r * .2126f + e2.g * .715f + e2.b * .0722f; - - const float cr1 = e1.r - l1; - const float cr2 = e2.r - l2; - - const float cb1 = e1.b - l1; - const float cb2 = e2.b - l2; - - const float dl = l1 - l2; - const float dcr = cr1 - cr2; - const float dcb = cb1 - cb2; - - uint32_t d = static_cast(32.0f*4.0f*dl*dl + 32.0f*2.0f*(.5f / (1.0f - .2126f))*(.5f / (1.0f - .2126f))*dcr*dcr + 32.0f*.25f*(.5f / (1.0f - .0722f))*(.5f / (1.0f - .0722f))*dcb*dcb); - - if (alpha) - { - int da = static_cast(e1.a) - static_cast(e2.a); - d += static_cast(128.0f*da*da); - } - - return d; -#elif 1 int dr = e1.r - e2.r; int dg = e1.g - e2.g; int db = e1.b - e2.b; -#if 0 - int delta_l = dr * 27 + dg * 92 + db * 9; - int delta_cr = dr * 128 - delta_l; - int delta_cb = db * 128 - delta_l; - - uint32_t id = ((uint32_t)(delta_l * delta_l) >> 7U) + - ((((uint32_t)(delta_cr * delta_cr) >> 7U) * 26U) >> 7U) + - ((((uint32_t)(delta_cb * delta_cb) >> 7U) * 3U) >> 7U); -#else int64_t delta_l = dr * 27 + dg * 92 + db * 9; int64_t delta_cr = dr * 128 - delta_l; int64_t delta_cb = db * 128 - delta_l; @@ -1121,7 +1099,6 @@ namespace basisu uint32_t id = ((uint32_t)((delta_l * delta_l) >> 7U)) + ((((uint32_t)((delta_cr * delta_cr) >> 7U)) * 26U) >> 7U) + ((((uint32_t)((delta_cb * delta_cb) >> 7U)) * 3U) >> 7U); -#endif if (alpha) { @@ -1131,32 +1108,67 @@ namespace basisu } return id; -#else + } + else + { + return color_distance(e1, e2, alpha); + } + } + + inline uint32_t color_distance(bool perceptual, const color_rgba &e1, const color_rgba &e2, bool alpha) + { + if (perceptual) + { int dr = e1.r - e2.r; int dg = e1.g - e2.g; int db = e1.b - e2.b; - int64_t delta_l = dr * 27 + dg * 92 + db * 9; - int64_t delta_cr = dr * 128 - delta_l; - int64_t delta_cb = db * 128 - delta_l; + // This calc can't overflow or the SSE variants will overflow too. + int delta_l = dr * 14 + dg * 45 + db * 5; + int delta_cr = dr * 64 - delta_l; + int delta_cb = db * 64 - delta_l; + + // not >> 6, so the output is scaled by 7 bits, not 6 (to match the original function which scaled by 7, but had rare overflow issues) + uint32_t id = ((uint32_t)(delta_l * delta_l) >> 5U) + + ((((uint32_t)(delta_cr * delta_cr) >> 5U) * 26U) >> 7U) + + ((((uint32_t)(delta_cb * delta_cb) >> 5U) * 3U) >> 7U); - int64_t id = ((delta_l * delta_l) * 128) + - ((delta_cr * delta_cr) * 26) + - ((delta_cb * delta_cb) * 3); +#if defined(DEBUG) || defined(_DEBUG) + // Shouldn't need 64-bit now, but make sure + { + int64_t alt_delta_l = dr * 14 + dg * 45 + db * 5; + int64_t alt_delta_cr = dr * 64 - alt_delta_l; + int64_t alt_delta_cb = db * 64 - alt_delta_l; + + int64_t alt_id = ((alt_delta_l * alt_delta_l) >> 5) + + ((((alt_delta_cr * alt_delta_cr) >> 5) * 26) >> 7) + + ((((alt_delta_cb * alt_delta_cb) >> 5) * 3) >> 7); + + assert(alt_id == id); + } +#endif if (alpha) { - int64_t da = (e1.a - e2.a); - id += (da * da) * 128; + int da = (e1.a - e2.a) << 7; + + // This shouldn't overflow if da is 255 or -255: 29.99 bits after squaring. + uint32_t ea = ((uint32_t)(da * da) >> 7U); + id += ea; + +#if defined(DEBUG) || defined(_DEBUG) + // Make sure it can't overflow + assert((((int64_t)da * (int64_t)da) >> 7) == ea); +#endif + } - int d = (id + 8192) >> 14; - - return d; -#endif + return id; } else + { return color_distance(e1, e2, alpha); + } } static inline uint32_t color_distance_la(const color_rgba& a, const color_rgba& b) @@ -2906,6 +2918,9 @@ namespace basisu image &fill_box(uint32_t x, uint32_t y, uint32_t w, uint32_t h, const color_rgba &c) { + assert((int)w >= 0); + assert((int)h >= 0); + for (uint32_t iy = 0; iy < h; iy++) for (uint32_t ix = 0; ix < w; ix++) set_clipped(x + ix, y + iy, c); @@ -2914,6 +2929,9 @@ namespace basisu image& fill_box_alpha(uint32_t x, uint32_t y, uint32_t w, uint32_t h, const color_rgba& c) { + assert((int)w >= 0); + assert((int)h >= 0); + for (uint32_t iy = 0; iy < h; iy++) for (uint32_t ix = 0; ix < w; ix++) set_clipped_alpha(x + ix, y + iy, c); @@ -3197,6 +3215,7 @@ namespace basisu void debug_text(uint32_t x_ofs, uint32_t y_ofs, uint32_t x_scale, uint32_t y_scale, const color_rgba &fg, const color_rgba *pBG, bool alpha_only, const char* p, ...); + // bilinear filtering vec4F get_filtered_vec4F(float x, float y) const { x -= .5f; @@ -3244,6 +3263,36 @@ namespace basisu color_rgba_vec m_pixels; }; + void draw_line(image& dst, int xs, int ys, int xe, int ye, const color_rgba& color); + void draw_circle(image& dst, int cx, int cy, int r, const color_rgba& color); + + inline bool is_solid_block(uint32_t n, const color_rgba* pPixels) + { + assert(n); + + if (n <= 1) + return true; + + const color_rgba c(pPixels[0]); + + for (uint32_t i = 1; i < n; i++) + if (c != pPixels[i]) + return false; + + return true; + } + + inline bool is_alpha_block(uint32_t n, const color_rgba* pPixels) + { + assert(n); + + for (uint32_t i = 0; i < n; i++) + if (pPixels[i][3] != 255) + return true; + + return false; + } + // Float images typedef basisu::vector vec4F_vec; diff --git a/encoder/basisu_etc.cpp b/encoder/basisu_etc.cpp index ba1c142..d817d27 100644 --- a/encoder/basisu_etc.cpp +++ b/encoder/basisu_etc.cpp @@ -811,7 +811,8 @@ namespace basisu #if defined(DEBUG) || defined(_DEBUG) { - // sanity check the returned error + // Ultimate sanity check on the returned error. + // If this check fails, it likely means the SSE code diverged from C++ somehow, or there was an overflow somewhere. color_rgba block_colors[4]; m_best_solution.m_coords.get_block_colors(block_colors); @@ -1234,6 +1235,8 @@ namespace basisu perceptual_distance_rgb_4_N_sse41((int64_t*)&total_error, pSelectors_to_use, block_colors, pSrc_pixels, n, trial_solution.m_error); else linear_distance_rgb_4_N_sse41((int64_t*)&total_error, pSelectors_to_use, block_colors, pSrc_pixels, n, trial_solution.m_error); + for (uint32_t i = 0; i < n; i++) + m_temp_selectors[i] = pSelectors_to_use[i]; } else { diff --git a/encoder/basisu_kernels_imp.h b/encoder/basisu_kernels_imp.h index 123862b..48a4fc2 100644 --- a/encoder/basisu_kernels_imp.h +++ b/encoder/basisu_kernels_imp.h @@ -17,6 +17,11 @@ using namespace CPPSPMD; namespace CPPSPMD_NAME(basisu_kernels_namespace) { + static inline int64_t reduce_add64(const vint &x) + { + return (int64_t)VINT_EXTRACT(x, 0) + (int64_t)VINT_EXTRACT(x, 1) + + (int64_t)VINT_EXTRACT(x, 2) + (int64_t)VINT_EXTRACT(x, 3); + } struct perceptual_distance_rgb_4_N : spmd_kernel { void _call(int64_t* pDistance, @@ -66,15 +71,15 @@ namespace CPPSPMD_NAME(basisu_kernels_namespace) vint dg = base_g - g; vint db = base_b - b; - vint delta_l = dr * 27 + dg * 92 + db * 9; - vint delta_cr = dr * 128 - delta_l; - vint delta_cb = db * 128 - delta_l; + vint delta_l = dr * 14 + dg * 45 + db * 5; + vint delta_cr = dr * 64 - delta_l; + vint delta_cb = db * 64 - delta_l; - vint id = ((delta_l * delta_l) >> 7) + - ((((delta_cr * delta_cr) >> 7) * 26) >> 7) + - ((((delta_cb * delta_cb) >> 7) * 3) >> 7); + vint id = ((delta_l * delta_l) >> 5) + + ((((delta_cr * delta_cr) >> 5) * 26) >> 7) + + ((((delta_cb * delta_cb) >> 5) * 3) >> 7); - *pDistance += reduce_add(id); + *pDistance += reduce_add64(id); if (*pDistance >= early_out_err) return; } @@ -90,13 +95,13 @@ namespace CPPSPMD_NAME(basisu_kernels_namespace) int dg = base_g - g; int db = base_b - b; - int delta_l = dr * 27 + dg * 92 + db * 9; - int delta_cr = dr * 128 - delta_l; - int delta_cb = db * 128 - delta_l; + int delta_l = dr * 14 + dg * 45 + db * 5; + int delta_cr = dr * 64 - delta_l; + int delta_cb = db * 64 - delta_l; - int id = ((delta_l * delta_l) >> 7) + - ((((delta_cr * delta_cr) >> 7) * 26) >> 7) + - ((((delta_cb * delta_cb) >> 7) * 3) >> 7); + int id = ((delta_l * delta_l) >> 5) + + ((((delta_cr * delta_cr) >> 5) * 26) >> 7) + + ((((delta_cb * delta_cb) >> 5) * 3) >> 7); *pDistance += id; if (*pDistance >= early_out_err) @@ -156,7 +161,7 @@ namespace CPPSPMD_NAME(basisu_kernels_namespace) vint id = dr * dr + dg * dg + db * db; - *pDistance += reduce_add(id); + *pDistance += reduce_add64(id); if (*pDistance >= early_out_err) return; } @@ -191,13 +196,13 @@ namespace CPPSPMD_NAME(basisu_kernels_namespace) vint dg = base_g - g; vint db = base_b - b; - vint delta_l = dr * 27 + dg * 92 + db * 9; - vint delta_cr = dr * 128 - delta_l; - vint delta_cb = db * 128 - delta_l; + vint delta_l = dr * 14 + dg * 45 + db * 5; + vint delta_cr = dr * 64 - delta_l; + vint delta_cb = db * 64 - delta_l; - vint id = VINT_SHIFT_RIGHT(delta_l * delta_l, 7) + - VINT_SHIFT_RIGHT(VINT_SHIFT_RIGHT(delta_cr * delta_cr, 7) * 26, 7) + - VINT_SHIFT_RIGHT(VINT_SHIFT_RIGHT(delta_cb * delta_cb, 7) * 3, 7); + vint id = VINT_SHIFT_RIGHT(delta_l * delta_l, 5) + + VINT_SHIFT_RIGHT(VINT_SHIFT_RIGHT(delta_cr * delta_cr, 5) * 26, 7) + + VINT_SHIFT_RIGHT(VINT_SHIFT_RIGHT(delta_cb * delta_cb, 5) * 3, 7); return id; } @@ -243,7 +248,7 @@ namespace CPPSPMD_NAME(basisu_kernels_namespace) __m128i vsels = shuffle_epi8(sels.m_value, shuf); storeu_si32((void *)(pSelectors + i), vsels); - *pDistance += reduce_add(min_dist); + *pDistance += reduce_add64(min_dist); if (*pDistance >= early_out_err) return; } @@ -261,13 +266,13 @@ namespace CPPSPMD_NAME(basisu_kernels_namespace) int dg = base_g - g; int db = base_b - b; - int delta_l = dr * 27 + dg * 92 + db * 9; - int delta_cr = dr * 128 - delta_l; - int delta_cb = db * 128 - delta_l; + int delta_l = dr * 14 + dg * 45 + db * 5; + int delta_cr = dr * 64 - delta_l; + int delta_cb = db * 64 - delta_l; - int id = ((delta_l * delta_l) >> 7) + - ((((delta_cr * delta_cr) >> 7) * 26) >> 7) + - ((((delta_cb * delta_cb) >> 7) * 3) >> 7); + int id = ((delta_l * delta_l) >> 5) + + ((((delta_cr * delta_cr) >> 5) * 26) >> 7) + + ((((delta_cb * delta_cb) >> 5) * 3) >> 7); if (id < best_err) { best_err = id; @@ -339,7 +344,7 @@ namespace CPPSPMD_NAME(basisu_kernels_namespace) __m128i vsels = shuffle_epi8(sels.m_value, shuf); storeu_si32((void *)(pSelectors + i), vsels); - *pDistance += reduce_add(min_dist); + *pDistance += reduce_add64(min_dist); if (*pDistance >= early_out_err) return; } @@ -384,13 +389,13 @@ namespace CPPSPMD_NAME(basisu_kernels_namespace) vint dg = base_g - g; vint db = base_b - b; - vint delta_l = dr * 27 + dg * 92 + db * 9; - vint delta_cr = dr * 128 - delta_l; - vint delta_cb = db * 128 - delta_l; + vint delta_l = dr * 14 + dg * 45 + db * 5; + vint delta_cr = dr * 64 - delta_l; + vint delta_cb = db * 64 - delta_l; - vint id = VINT_SHIFT_RIGHT(delta_l * delta_l, 7) + - VINT_SHIFT_RIGHT(VINT_SHIFT_RIGHT(delta_cr * delta_cr, 7) * 26, 7) + - VINT_SHIFT_RIGHT(VINT_SHIFT_RIGHT(delta_cb * delta_cb, 7) * 3, 7); + vint id = VINT_SHIFT_RIGHT(delta_l * delta_l, 5) + + VINT_SHIFT_RIGHT(VINT_SHIFT_RIGHT(delta_cr * delta_cr, 5) * 26, 7) + + VINT_SHIFT_RIGHT(VINT_SHIFT_RIGHT(delta_cb * delta_cb, 5) * 3, 7); return id; } @@ -428,7 +433,7 @@ namespace CPPSPMD_NAME(basisu_kernels_namespace) vint min_dist = min(min(min(dist0, dist1), dist2), dist3); - *pDistance += reduce_add(min_dist); + *pDistance += reduce_add64(min_dist); if (*pDistance > early_out_error) return; } @@ -446,13 +451,13 @@ namespace CPPSPMD_NAME(basisu_kernels_namespace) int dg = base_g - g; int db = base_b - b; - int delta_l = dr * 27 + dg * 92 + db * 9; - int delta_cr = dr * 128 - delta_l; - int delta_cb = db * 128 - delta_l; + int delta_l = dr * 14 + dg * 45 + db * 5; + int delta_cr = dr * 64 - delta_l; + int delta_cb = db * 64 - delta_l; - int id = ((delta_l * delta_l) >> 7) + - ((((delta_cr * delta_cr) >> 7) * 26) >> 7) + - ((((delta_cb * delta_cb) >> 7) * 3) >> 7); + int id = ((delta_l * delta_l) >> 5) + + ((((delta_cr * delta_cr) >> 5) * 26) >> 7) + + ((((delta_cb * delta_cb) >> 5) * 3) >> 7); if (id < best_err) { @@ -515,7 +520,7 @@ namespace CPPSPMD_NAME(basisu_kernels_namespace) vint min_dist = min(min(min(dist0, dist1), dist2), dist3); - *pDistance += reduce_add(min_dist); + *pDistance += reduce_add64(min_dist); if (*pDistance > early_out_error) return; }