color-util: simplify hsv_to_rgb, fix rgb_to_hsv negative-hue wrap

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 <noreply@anthropic.com>

diff --git a/src/shared/color-util.c b/src/shared/color-util.c
index 191278595472012f3de1dd3d91eed44ff07dcb3b..ed417a1a2010a3bf7b5a63e8ede037190fb6ae1a 100644 (file)
--- 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 3d00d8719d82ae8169647ed6d9763dab18b7c149..a1369f4ef5231d10faebc83ceeb302867f3fdaa0 100644 (file)
--- 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);
-
-        hsv_to_rgb(0, 100, 100, &r, &g, &b);
-        assert(r == 255 && g == 0 && b == 0);
-
-        hsv_to_rgb(120, 100, 100, &r, &g, &b);
-        assert(r == 0 && g == 255 && b == 0);
-
-        hsv_to_rgb(240, 100, 100, &r, &g, &b);
-        assert(r == 0 && g == 0 && b == 255);
-
+        ASSERT_EQ(r, 255);
+        ASSERT_EQ(g, 255);
+        ASSERT_EQ(b, 255);
+
+        hsv_to_rgb(180, 0, 100, &r, &g, &b);
+        ASSERT_EQ(r, 255);
+        ASSERT_EQ(g, 255);
+        ASSERT_EQ(b, 255);
+
+        /* 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);
+
+        /* 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);
-
-        rgb_to_hsv(1, 0, 0, &h, &s, &v);
-        assert(h >= 359 || h <= 1);
-        assert(s >= 100);
-        assert(v >= 100);
-
-        rgb_to_hsv(0, 1, 0, &h, &s, &v);
-        assert(h >= 119 && h <= 121);
-        assert(s >= 100);
-        assert(v >= 100);
-
-        rgb_to_hsv(0, 0, 1, &h, &s, &v);
-        assert(h >= 239 && h <= 241);
-        assert(s >= 100);
-        assert(v >= 100);
-
+        ASSERT_TRUE(s <= 0);
+        ASSERT_TRUE(v >= 100);
+
+        rgb_to_hsv(0.5, 0.5, 0.5, &h, &s, &v);
+        ASSERT_TRUE(s <= 0);
+        ASSERT_TRUE(v >= 49 && v <= 51);
+
+        /* 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, 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);