From 7d2925fcde70ed221658e01ffb6f2797061d3520 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Sun, 17 May 2026 11:28:00 +0000 Subject: [PATCH] color-util: simplify hsv_to_rgb, fix rgb_to_hsv negative-hue wrap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In hsv_to_rgb, restructure the conversion around the sector index k = (int)(h/60) and fractional offset f = h/60 - k. The auxiliary x value becomes c * (k & 1 ? 1.0 - f : f) and the six branches turn into a switch on k. This drops the two xfmod() calls that were doing the modulo work, in exchange for a single assert(h >= 0 && h < 360) — all in-tree callers satisfy this and never relied on the wrap. In rgb_to_hsv, the two xfmod() calls were no-ops (their arguments were always within the divisor's magnitude). The trailing xfmod(*ret_h, 360) appeared to be wrapping negative hues from the r-max branch back into [0, 360), but fmod is sign-preserving so it never did. Drop the no-ops and add an explicit +360 wrap so magenta (1, 0, 1) now yields h ≈ 300 instead of -60. Extend the tests to cover all six primary/secondary colors at sector boundaries, all six sector midpoints (to catch any future inversion of the ramp direction), the h-near-360 edge of the last sector, and the rgb_to_hsv negative-wrap path via magenta. Switch the new and existing integer-channel checks to ASSERT_EQ from tests.h; the double-typed h/s/v range checks stay on ASSERT_TRUE since the ASSERT_* comparison macros only support integer types. Co-developed-by: Claude Opus 4.7 --- src/shared/color-util.c | 40 +++++++----- src/test/test-color-util.c | 126 ++++++++++++++++++++++++++++--------- 2 files changed, 120 insertions(+), 46 deletions(-) diff --git a/src/shared/color-util.c b/src/shared/color-util.c index 1912785954..ed417a1a20 100644 --- a/src/shared/color-util.c +++ b/src/shared/color-util.c @@ -32,13 +32,15 @@ void rgb_to_hsv(double r, double g, double b, if (ret_h) { if (delta > 0) { if (r >= max_color) - *ret_h = 60 * fmod((g - b) / delta, 6); + *ret_h = 60 * (g - b) / delta; else if (g >= max_color) *ret_h = 60 * (((b - r) / delta) + 2); - else if (b >= max_color) + else /* b >= max_color */ *ret_h = 60 * (((r - g) / delta) + 4); - *ret_h = fmod(*ret_h, 360); + /* The r-max branch produces (-60, 60); fold the negative half up. */ + if (*ret_h < 0) + *ret_h += 360; } else *ret_h = NAN; } @@ -49,29 +51,33 @@ void hsv_to_rgb(double h, double s, double v, double c, x, m, r, g, b; + assert(h >= 0 && h <= 360); assert(s >= 0 && s <= 100); assert(v >= 0 && v <= 100); assert(ret_r); assert(ret_g); assert(ret_b); - h = fmod(h, 360); c = (s / 100.0) * (v / 100.0); - x = c * (1 - fabs(fmod(h / 60.0, 2) - 1)); m = (v / 100) - c; - if (h >= 0 && h < 60) - r = c, g = x, b = 0.0; - else if (h >= 60 && h < 120) - r = x, g = c, b = 0.0; - else if (h >= 120 && h < 180) - r = 0.0, g = c, b = x; - else if (h >= 180 && h < 240) - r = 0.0, g = x, b = c; - else if (h >= 240 && h < 300) - r = x, g = 0.0, b = c; - else - r = c, g = 0.0, b = x; + /* Split h into sector index k ∈ [0, 6] and fractional offset f ∈ [0, 1) within the sector. + * Within each sector exactly one of {r, g, b} equals c, one equals 0, and the third (x) + * ramps linearly between them — ascending in even sectors, descending in odd. h == 360 is + * the cyclic boundary equivalent to h == 0, and maps to sector 0. */ + double seg = h / 60.0; + int k = (int) seg % 6; + double f = seg - (int) seg; + x = c * (k & 1 ? 1.0 - f : f); + + switch (k) { + case 0: r = c; g = x; b = 0.0; break; + case 1: r = x; g = c; b = 0.0; break; + case 2: r = 0.0; g = c; b = x; break; + case 3: r = 0.0; g = x; b = c; break; + case 4: r = x; g = 0.0; b = c; break; + default: r = c; g = 0.0; b = x; break; + } *ret_r = (uint8_t) ((r + m) * 255); *ret_g = (uint8_t) ((g + m) * 255); diff --git a/src/test/test-color-util.c b/src/test/test-color-util.c index 3d00d8719d..a1369f4ef5 100644 --- a/src/test/test-color-util.c +++ b/src/test/test-color-util.c @@ -6,58 +6,126 @@ TEST(hsv_to_rgb) { uint8_t r, g, b; + /* Black at any hue. */ hsv_to_rgb(0, 0, 0, &r, &g, &b); - assert(r == 0 && g == 0 && b == 0); + ASSERT_EQ(r, 0); + ASSERT_EQ(g, 0); + ASSERT_EQ(b, 0); hsv_to_rgb(60, 0, 0, &r, &g, &b); - assert(r == 0 && g == 0 && b == 0); + ASSERT_EQ(r, 0); + ASSERT_EQ(g, 0); + ASSERT_EQ(b, 0); + /* White: s=0 must ignore hue. */ hsv_to_rgb(0, 0, 100, &r, &g, &b); - assert(r == 255 && g == 255 && b == 255); + ASSERT_EQ(r, 255); + ASSERT_EQ(g, 255); + ASSERT_EQ(b, 255); - hsv_to_rgb(0, 100, 100, &r, &g, &b); - assert(r == 255 && g == 0 && b == 0); + hsv_to_rgb(180, 0, 100, &r, &g, &b); + ASSERT_EQ(r, 255); + ASSERT_EQ(g, 255); + ASSERT_EQ(b, 255); - hsv_to_rgb(120, 100, 100, &r, &g, &b); - assert(r == 0 && g == 255 && b == 0); + /* Pure primary and secondary colors — one per sector boundary. */ + hsv_to_rgb(0, 100, 100, &r, &g, &b); /* red */ + ASSERT_EQ(r, 255); ASSERT_EQ(g, 0); ASSERT_EQ(b, 0); + hsv_to_rgb(60, 100, 100, &r, &g, &b); /* yellow */ + ASSERT_EQ(r, 255); ASSERT_EQ(g, 255); ASSERT_EQ(b, 0); + hsv_to_rgb(120, 100, 100, &r, &g, &b); /* green */ + ASSERT_EQ(r, 0); ASSERT_EQ(g, 255); ASSERT_EQ(b, 0); + hsv_to_rgb(180, 100, 100, &r, &g, &b); /* cyan */ + ASSERT_EQ(r, 0); ASSERT_EQ(g, 255); ASSERT_EQ(b, 255); + hsv_to_rgb(240, 100, 100, &r, &g, &b); /* blue */ + ASSERT_EQ(r, 0); ASSERT_EQ(g, 0); ASSERT_EQ(b, 255); + hsv_to_rgb(300, 100, 100, &r, &g, &b); /* magenta */ + ASSERT_EQ(r, 255); ASSERT_EQ(g, 0); ASSERT_EQ(b, 255); - hsv_to_rgb(240, 100, 100, &r, &g, &b); - assert(r == 0 && g == 0 && b == 255); + /* Sector midpoints. Catches inverted ramp direction (k & 1 ? 1-f : f) in any single + * sector — a regression would shift exactly one channel by ±half-intensity. */ + hsv_to_rgb(30, 100, 100, &r, &g, &b); /* orange — sector 0 */ + ASSERT_EQ(r, 255); ASSERT_EQ(g, 127); ASSERT_EQ(b, 0); + hsv_to_rgb(90, 100, 100, &r, &g, &b); /* chartreuse — sector 1 */ + ASSERT_EQ(r, 127); ASSERT_EQ(g, 255); ASSERT_EQ(b, 0); + hsv_to_rgb(150, 100, 100, &r, &g, &b); /* spring green — sector 2 */ + ASSERT_EQ(r, 0); ASSERT_EQ(g, 255); ASSERT_EQ(b, 127); + hsv_to_rgb(210, 100, 100, &r, &g, &b); /* azure — sector 3 */ + ASSERT_EQ(r, 0); ASSERT_EQ(g, 127); ASSERT_EQ(b, 255); + hsv_to_rgb(270, 100, 100, &r, &g, &b); /* violet — sector 4 */ + ASSERT_EQ(r, 127); ASSERT_EQ(g, 0); ASSERT_EQ(b, 255); + hsv_to_rgb(330, 100, 100, &r, &g, &b); /* rose — sector 5 */ + ASSERT_EQ(r, 255); ASSERT_EQ(g, 0); ASSERT_EQ(b, 127); + /* Just below 360 — ensures (int) seg stays in sector 5 and doesn't wrap to 6. */ + hsv_to_rgb(359.5, 100, 100, &r, &g, &b); + ASSERT_EQ(r, 255); ASSERT_EQ(g, 0); ASSERT_EQ(b, 2); + + /* Exactly 360 — cyclic boundary, equivalent to 0 (red). Callers like color_for_pcr() in + * pcrlock.c can reach this via floating-point arithmetic (e.g. 360.0/23*23 == 360.0). */ + hsv_to_rgb(360, 100, 100, &r, &g, &b); + ASSERT_EQ(r, 255); ASSERT_EQ(g, 0); ASSERT_EQ(b, 0); + + /* Non-trivial s/v: regression check for the multiply-and-cast path. */ hsv_to_rgb(311, 52, 62, &r, &g, &b); - assert(r == 158 && g == 75 && b == 143); + ASSERT_EQ(r, 158); ASSERT_EQ(g, 75); ASSERT_EQ(b, 143); } TEST(rgb_to_hsv) { double h, s, v; + + /* Grayscale: delta == 0, h is NaN, s == 0. */ rgb_to_hsv(0, 0, 0, &h, &s, &v); - assert(s <= 0); - assert(v <= 0); + ASSERT_TRUE(s <= 0); + ASSERT_TRUE(v <= 0); rgb_to_hsv(1, 1, 1, &h, &s, &v); - assert(s <= 0); - assert(v >= 100); + ASSERT_TRUE(s <= 0); + ASSERT_TRUE(v >= 100); - rgb_to_hsv(1, 0, 0, &h, &s, &v); - assert(h >= 359 || h <= 1); - assert(s >= 100); - assert(v >= 100); + rgb_to_hsv(0.5, 0.5, 0.5, &h, &s, &v); + ASSERT_TRUE(s <= 0); + ASSERT_TRUE(v >= 49 && v <= 51); - rgb_to_hsv(0, 1, 0, &h, &s, &v); - assert(h >= 119 && h <= 121); - assert(s >= 100); - assert(v >= 100); + /* Pure primary colors. */ + rgb_to_hsv(1, 0, 0, &h, &s, &v); /* red */ + ASSERT_TRUE(h >= 0 && h <= 1); + ASSERT_TRUE(s >= 100); + ASSERT_TRUE(v >= 100); - rgb_to_hsv(0, 0, 1, &h, &s, &v); - assert(h >= 239 && h <= 241); - assert(s >= 100); - assert(v >= 100); + rgb_to_hsv(0, 1, 0, &h, &s, &v); /* green */ + ASSERT_TRUE(h >= 119 && h <= 121); + ASSERT_TRUE(s >= 100); + ASSERT_TRUE(v >= 100); + rgb_to_hsv(0, 0, 1, &h, &s, &v); /* blue */ + ASSERT_TRUE(h >= 239 && h <= 241); + ASSERT_TRUE(s >= 100); + ASSERT_TRUE(v >= 100); + + /* Pure secondary colors — each exercises a different "max" branch. Magenta exercises + * the negative-hue wrap from the r-max branch (raw value is -60). */ + rgb_to_hsv(1, 1, 0, &h, &s, &v); /* yellow — r-max branch, positive */ + ASSERT_TRUE(h >= 59 && h <= 61); + ASSERT_TRUE(s >= 100); + ASSERT_TRUE(v >= 100); + + rgb_to_hsv(0, 1, 1, &h, &s, &v); /* cyan — g-max branch */ + ASSERT_TRUE(h >= 179 && h <= 181); + ASSERT_TRUE(s >= 100); + ASSERT_TRUE(v >= 100); + + rgb_to_hsv(1, 0, 1, &h, &s, &v); /* magenta — r-max branch, wrapped */ + ASSERT_TRUE(h >= 299 && h <= 301); + ASSERT_TRUE(s >= 100); + ASSERT_TRUE(v >= 100); + + /* Mixed values. */ rgb_to_hsv(0.5, 0.6, 0.7, &h, &s, &v); - assert(h >= 209 && h <= 211); - assert(s >= 28 && s <= 31); - assert(v >= 69 && v <= 71); + ASSERT_TRUE(h >= 209 && h <= 211); + ASSERT_TRUE(s >= 28 && s <= 31); + ASSERT_TRUE(v >= 69 && v <= 71); } DEFINE_TEST_MAIN(LOG_DEBUG);