bug#41544: 26.3; Possible incorrect results from color-distance

bug#41544: 26.3; Possible incorrect results from color-distance

[Simon Pugnet]

[-- Attachment #1: Type: text/plain, Size: 5715 bytes --]

Hello,

I have noticed some potentially incorrect behaviour from the
`color-distance` elisp function.

As an example, take the following elisp: -
(list
 (color-distance '(0 0 0) '(65535 65535 65535))
 (color-distance '(65535 65535 65535) '(0 0 0))
 (color-distance '(1 2 3) '(4 5 6))
 (color-distance '(4 5 6) '(1 2 3)))

Result: (589568 584970 8 0)

Here, I would expect the first two elements to have the same 
result as
well as the third and fourth. This is because conceptually the 
distance
between colour (1 2 3) and (4 5 6) is the same as the distance 
between
(4 5 6) and (1 2 3), etc.

The problem comes from the `color_distance()` C function. In this
function, values are calculated via bit shifts to perform integer
divisions of 256 (>>8) and 512 (>>9). Take for example the 3rd and 
4th
items above (red channel): -
1 - 4 = -3
4 - 1 = 3
but: -
(1 - 4) >> 8 = -1
(4 - 1) >> 8 = 0

Therefore for negative values, there is a difference of 1 every 
time the
bit shift is performed, which is what leads to the discrepancy 
mentioned
above.

Modifying the function to remove these discrepancies causes the 
results
above to become (584970 584970 0 0) which appear to be more 
sensible.

My apologies in advance if this is in fact the correct behaviour 
of this
function.

Kind regards,

--
Simon Pugnet
https://www.polaris64.net/

---

In GNU Emacs 26.3 (build 2, x86_64-pc-linux-gnu, GTK+ Version 
3.24.14)
 of 2020-03-26, modified by Debian built on lcy01-amd64-020
Windowing system distributor 'The X.Org Foundation', version 
11.0.12008000
System Description:	Ubuntu 20.04 LTS

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Mark set
(589568 584970 8 0)
You can run the command ‘eval-print-last-sexp’ with C-j
Mark activated
kill-line: End of buffer
Making completion list...
You can run the command ‘kill-region’ with C-w

Configured using:
 'configure --build x86_64-linux-gnu --prefix=/usr
 --sharedstatedir=/var/lib --libexecdir=/usr/lib
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --enable-libsystemd --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/26.3/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/26.3/site-lisp:/usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils --build
 x86_64-linux-gnu --prefix=/usr --sharedstatedir=/var/lib
 --libexecdir=/usr/lib --localstatedir=/var/lib
 --infodir=/usr/share/info --mandir=/usr/share/man 
 --enable-libsystemd
 --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/26.3/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/26.3/site-lisp:/usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils --with-x=yes
 --with-x-toolkit=gtk3 --with-toolkit-scroll-bars 'CFLAGS=-g -O2
 -fdebug-prefix-map=/build/emacs-mEZBk7/emacs-26.3+1=. 
 -fstack-protector-strong
 -Wformat -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
 -D_FORTIFY_SOURCE=2' 'LDFLAGS=-Wl,-Bsymbolic-functions 
 -Wl,-z,relro''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS 
GLIB
NOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT 
ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM THREADS LIBSYSTEMD LCMS2

Important settings:
  value of $LANG: en_GB.UTF-8
  value of $XMODIFIERS: @im=none
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt gv
bytecomp byte-compile cconv cl-loaddefs cl-lib dired 
dired-loaddefs
format-spec rfc822 mml easymenu mml-sec password-cache epa derived 
epg
epg-config gnus-util rmail rmail-loaddefs mm-decode mm-bodies 
mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail 
rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils elec-pair 
time-date
mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd 
tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace 
newcomment
text-mode elisp-mode lisp-mode prog-mode register page menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock 
font-lock
syntax facemenu font-core term/tty-colors frame cl-generic cham 
georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech 
european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray 
minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads dbusbind
inotify lcms2 dynamic-setting system-font-setting 
font-render-setting
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 95338 8459)
 (symbols 48 20386 1)
 (miscs 40 50 168)
 (strings 32 28445 1160)
 (string-bytes 1 747812)
 (vectors 16 13900)
 (vector-slots 8 500980 11290)
 (floats 8 51 264)
 (intervals 56 301 25)
 (buffers 992 12))

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

[CC:ing Tom Tromey, who used colour-distance in css-mode]

Ah, where to begin. Clearly a distance metric ought to be symmetric; as you note, this is easily fixed by replacing the shift by division, which has the extra benefit of not relying on the implementation-defined behaviour when right-shifting negative values in C. The extra cost is negligible.

Looking at it a bit closer, it seems a waste to use full 16 bit colour channels only to shift out 8 of them before getting started. I presume this was done partly in order to follow the Riedersma metric more closely, and partly to stay within 32 bit numbers (I note that the code uses the C 'long' type, which is almost always a mistake). Using 64-bit arithmetic costs us next to nothing today, and this solves several problems.

But a darker cloud is on the horizon. Since the Emacs implementation omits the square root of the result, it no longer satisfies the triangle inequality, which is even more fundamental for distance metrics than symmetry. It is good enough for comparison of distances, but they can no longer be added, since it's not a true metric.

In fact, it seems that users of color_distance are confused as well: a comment says

  /* If the distance (as returned by color_distance) between two colors is
     less than this, then they are considered the same, for determining
     whether a color is supported or not.  The range of values is 0-65535.  */

  #define TTY_SAME_COLOR_THRESHOLD  10000

which is a lie, since color_distance can return values well above 65535.
Some uses are very questionable, given that it's the square of a metric:

      int delta_delta
	= (color_distance (&fg_std_color, &bg_std_color)
	   - color_distance (&fg_tty_color, &bg_tty_color));
      if (delta_delta > TTY_SAME_COLOR_THRESHOLD
	  || delta_delta < -TTY_SAME_COLOR_THRESHOLD)

So what to do? Best would be to do the arithmetic on the entire channel values and take the square root at the end; the cost is nothing to worry about on hardware less than 30 years old. Some constants used for comparison would need to be adjusted: the above-mentioned TTY_SAME_COLOR_THRESHOLD (10000), face-near-same-threshold (30000), and a number in css--contrasty-color (292485).  At least the last should probably use the luma norm originally proposed instead (see bug#25525); the use of colour-distance here cannot be right.

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 235 bytes --]

tags 41544 patch
stop

Try the attached patch. A couple of constants used for comparison were recomputed, but since they mostly appeared to have been picked out of thin air, I didn't bother attempting a very precise translation.


[-- Attachment #2: 0001-Make-color-distance-into-a-proper-distance-metric-bu.patch --]
[-- Type: application/octet-stream, Size: 4224 bytes --]

From f0eab8ad402baf8c4c790e685e84f3f89b1d6ecd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 29 May 2020 17:07:59 +0200
Subject: [PATCH] Make color-distance into a proper distance metric (bug#41544)

The previous function discarded bits in the arguments, was not
symmetric, and did not obey the triangle equality; the return value
was quadratic in the perceived colour distance instead of linear.

Reported by Simon Pugnet.

* src/xfaces.c (color_distance): Use all 16 bits per channel and
add the missing square root operation.
(TTY_SAME_COLOR_THRESHOLD, syms_of_xfaces):
* lisp/textmodes/css-mode.el (css--contrasty-color):
Update colour distance constants to values appropriate for the new
function.
---
 etc/NEWS                   |  6 ++++++
 lisp/textmodes/css-mode.el |  4 ++--
 src/xfaces.c               | 22 +++++++++++-----------
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 64cf0abbdb..d8470223e5 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -489,6 +489,12 @@ Use macro 'with-current-buffer-window' with action alist entry 'body-function'.
 ** Some libraries obsolete since Emacs 23 have been removed:
 'ledit.el', 'lmenu.el', 'lucid.el and 'old-whitespace.el'.
 
+---
+** The 'color-distance' function is now a proper distance metric.
+It was previously not symmetric, nor did it obey the triangle equality.
+The default value of the 'face-near-same-color-threshold' variable has
+been updated to a number that roughly corresponds to the same distance.
+
 \f
 * Lisp Changes in Emacs 28.1
 
diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 0035c5e7b0..eda739a397 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -1156,8 +1156,8 @@ css--contrasty-color
 with NAME; in particular so that if NAME is used as a background
 color, the returned color can be used as the foreground and still
 be readable."
-  ;; See bug#25525 for a discussion of this.
-  (if (> (color-distance name "black") 292485)
+  ;; See bug#25525 and bug#41544 for a discussion of this.
+  (if (> (color-distance name "black") 138500)
       "black" "white"))
 
 (defcustom css-fontify-colors t
diff --git a/src/xfaces.c b/src/xfaces.c
index 7d7aff95c1..e828c12f09 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -218,6 +218,7 @@ Copyright (C) 1993-1994, 1998-2020 Free Software Foundation, Inc.
 #include <config.h>
 #include <stdlib.h>
 #include "sysstdio.h"
+#include <math.h>
 #include <sys/types.h>
 #include <sys/stat.h>
 
@@ -4356,15 +4357,14 @@ color_distance (Emacs_Color *x, Emacs_Color *y)
 
      See <https://www.compuphase.com/cmetric.htm> for more info.  */
 
-  long r = (x->red   - y->red)   >> 8;
-  long g = (x->green - y->green) >> 8;
-  long b = (x->blue  - y->blue)  >> 8;
-  long r_mean = (x->red + y->red) >> 9;
+  long long r = x->red   - y->red;
+  long long g = x->green - y->green;
+  long long b = x->blue  - y->blue;
+  long long r_mean = (x->red + y->red) >> 1;
 
-  return
-    (((512 + r_mean) * r * r) >> 8)
-    + 4 * g * g
-    + (((767 - r_mean) * b * b) >> 8);
+  return (int) sqrt ((((2 * 65536 + r_mean) * r * r) >> 16)
+                     + 4 * g * g
+                     + (((2 * 65536 + 65535 - r_mean) * b * b) >> 16));
 }
 
 
@@ -4931,9 +4931,9 @@ DEFUN ("face-attributes-as-vector", Fface_attributes_as_vector,
 
 /* If the distance (as returned by color_distance) between two colors is
    less than this, then they are considered the same, for determining
-   whether a color is supported or not.  The range of values is 0-65535.  */
+   whether a color is supported or not.  */
 
-#define TTY_SAME_COLOR_THRESHOLD  10000
+#define TTY_SAME_COLOR_THRESHOLD  25000
 
 #ifdef HAVE_WINDOW_SYSTEM
 
@@ -7008,7 +7008,7 @@ syms_of_xfaces (void)
 
 Lisp programs that change the value of this variable should also
 clear the face cache, see `clear-face-cache'.  */);
-  face_near_same_color_threshold = 30000;
+  face_near_same_color_threshold = 44300;
 
 #ifdef HAVE_WINDOW_SYSTEM
   defsubr (&Sbitmap_spec_p);
-- 
2.21.1 (Apple Git-122.3)

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 29 May 2020 17:17:41 +0200
> Cc: Tom Tromey <tom@tromey.com>, 41544@debbugs.gnu.org
> 
>  /* If the distance (as returned by color_distance) between two colors is
>     less than this, then they are considered the same, for determining
> -   whether a color is supported or not.  The range of values is 0-65535.  */
> +   whether a color is supported or not.  */
>  
> -#define TTY_SAME_COLOR_THRESHOLD  10000
> +#define TTY_SAME_COLOR_THRESHOLD  25000

Did you try what this does to tty-color-approximate and friends,
especially when the terminal supports only 8 or 16 colors?  If not,
please do, we must ensure there are no regressions there.

Thanks.

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

29 maj 2020 kl. 17.36 skrev Eli Zaretskii <eliz@gnu.org>:

> Did you try what this does to tty-color-approximate and friends,
> especially when the terminal supports only 8 or 16 colors?  If not,
> please do, we must ensure there are no regressions there.

tty-color-approximate uses its own metric (square of the unweighted Euclidian RGB distance).
To test the TTY_SAME_COLOR_THRESHOLD, I tried display-supports-face-attributes-p, and it seems to behave in the same way. All tested with TERM=xterm-color, which gives the standard 8 colours.

The translated constants were obtained by generating many random RGB triples and computing distances between all pairs, then finding the value of NEWCONST which minimises the number of cases where

  old-colour-dist(c1,c2) < OLDCONST

and

  new-colour-dist(c1,c2) < NEWCONST

differ in truth value. The computation was also run on the rgb.txt list instead of random colours as an extra check.

bug#41544: 26.3; Possible incorrect results from color-distance

[Tom Tromey]

Mattias> Try the attached patch. A couple of constants used for
Mattias> comparison were recomputed, but since they mostly appeared to
Mattias> have been picked out of thin air, I didn't bother attempting a
Mattias> very precise translation.

Thank you for doing this.

I wasn't sure whether Emacs would accept a patch changing the return
value in this way.

However, my main concern is just whether it still picks reasonably
contrasting colors when editing CSS.  If it does, then that's good
enough for me.

thanks,
Tom

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 1477 bytes --]

29 maj 2020 kl. 19.52 skrev Tom Tromey <tom@tromey.com>:

> However, my main concern is just whether it still picks reasonably
> contrasting colors when editing CSS.  If it does, then that's good
> enough for me.

Thank you for the kind words. I couldn't leave well enough alone, of course. Emacs does this sort of is-this-colour-dark computation in at least 7 different places, with different algorithms:

* max(r,g,b) < 0.5
* r+g+b < 0.5*3
* color-distance(c, "black") < 292485

They aren't really satisfactory: for example, saturated blue (#0000ff) is quite clearly 'dark', yet the first algorithm considers it 'light'. Colour distance isn't quite right either -- the implemented formula is intended to measure distances between colours, not brightness. For example, it considers #ff0000 to be closer than #0000ff to black, but the red is clearly brighter.

I tentatively went with your suggested 0.299r + 0.587g + 0.114g, with a cut-off value of 0.58 to make saturated blue and red 'dark' and green 'light'. This is not a correct luma calculation since there is no gamma correction, but it might do for this purpose.

Proposed patch attached. I found css-mode no worse than before (a tad better, if anything). Perhaps we need to decompress to linear components after all, but at least now there is a single place to do it.

(Should list-colors-display use color-dark-p for the text in its left column, by the way? Or is there a point in not doing so?)


[-- Attachment #2: 0001-Use-a-single-light-dark-colour-predicate.patch --]
[-- Type: application/octet-stream, Size: 11092 bytes --]

From 5d5ef884c47695a39c22d931c4bc44e6a812d7b4 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sun, 31 May 2020 21:12:46 +0200
Subject: [PATCH] Use a single light/dark colour predicate

Add a single predicate, color-dark-p, for deciding whether a colour
is more readable against black or white.  Previously this was done in
different ways in several places, and with worse results.  (Bug#41544)

* lisp/color.el (color-name-to-rgb): Use color-component-max instead
of the error-prone method of (color-values "#ffffffffffff"), which
would give unexpected values or outright fail if called without
an initialised display (as in batch mode).
* lisp/facemenu.el (list-colors-print): Use readable-foreground-color.
(color-dark-p, color-component-max): New functions.
* lisp/term/pc-win.el: Update comment.
* lisp/term/rxvt.el (rxvt-set-background-mode):
* lisp/term/w32console.el (terminal-init-w32console):
* lisp/term/xterm.el (xterm-maybe-set-dark-background-mode):
* lisp/faces.el (readable-foreground-color):
* lisp/frame.el (frame-set-background-mode): Use color-dark-p.
* lisp/textmodes/css-mode.el (css--contrasty-color): Remove.
(css--fontify-region): Use color-dark-p.
---
 lisp/color.el              |  4 +---
 lisp/facemenu.el           |  8 +++-----
 lisp/faces.el              | 34 +++++++++++++++++++++++++---------
 lisp/frame.el              | 10 +++-------
 lisp/term/pc-win.el        |  8 +++-----
 lisp/term/rxvt.el          | 12 +++++-------
 lisp/term/w32console.el    |  5 ++---
 lisp/term/xterm.el         |  5 ++---
 lisp/textmodes/css-mode.el | 14 ++------------
 9 files changed, 46 insertions(+), 54 deletions(-)

diff --git a/lisp/color.el b/lisp/color.el
index 560631ae66..2385874028 100644
--- a/lisp/color.el
+++ b/lisp/color.el
@@ -50,9 +50,7 @@ color-name-to-rgb
 Optional argument FRAME specifies the frame where the color is to be
 displayed.  If FRAME is omitted or nil, use the selected frame.
 If FRAME cannot display COLOR, return nil."
-  ;; `colors-values' maximum value is either 65535 or 65280 depending on the
-  ;; display system.  So we use a white conversion to get the max value.
-  (let ((valmax (float (car (color-values "#ffffffffffff")))))
+  (let ((valmax (float (color-component-max frame))))
     (mapcar (lambda (x) (/ x valmax)) (color-values color frame))))
 
 (defun color-rgb-to-hex  (red green blue &optional digits-per-component)
diff --git a/lisp/facemenu.el b/lisp/facemenu.el
index b10d874b21..119a2ba790 100644
--- a/lisp/facemenu.el
+++ b/lisp/facemenu.el
@@ -621,9 +621,7 @@ list-colors-print
 						 (downcase b))))))
 	(setq color (list color)))
       (let* ((opoint (point))
-	     (color-values (color-values (car color)))
-	     (light-p (>= (apply 'max color-values)
-			  (* (car (color-values "white")) .5))))
+             (fg (readable-foreground-color (car color))))
 	(insert (car color))
 	(indent-to 22)
 	(put-text-property opoint (point) 'face `(:background ,(car color)))
@@ -639,7 +637,7 @@ list-colors-print
 	(insert (propertize
 		 (apply 'format "#%02x%02x%02x"
 			(mapcar (lambda (c) (ash c -8))
-				color-values))
+				(color-values (car color))))
 		 'mouse-face 'highlight
 		 'help-echo
 		 (let ((hsv (apply 'color-rgb-to-hsv
@@ -651,7 +649,7 @@ list-colors-print
 	   opoint (point)
 	   'follow-link t
 	   'mouse-face (list :background (car color)
-			     :foreground (if light-p "black" "white"))
+			     :foreground fg)
 	   'color-name (car color)
 	   'action callback-fn)))
       (insert "\n"))
diff --git a/lisp/faces.el b/lisp/faces.el
index e707f6f4b6..2b9bcb9bcf 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1786,15 +1786,22 @@ defined-colors-with-face-attributes
 
 (defun readable-foreground-color (color)
   "Return a readable foreground color for background COLOR."
-  (let* ((rgb   (color-values color))
-	 (max   (apply #'max rgb))
-	 (black (car (color-values "black")))
-	 (white (car (color-values "white"))))
-    ;; Select black or white depending on which one is less similar to
-    ;; the brightest component.
-    (if (> (abs (- max black)) (abs (- max white)))
-	"black"
-      "white")))
+  (if (color-dark-p (color-name-to-rgb color)) "white" "black"))
+
+(defun color-dark-p (rgb)
+  "Whether RGB is more readable against white than black.
+RGB is a 3-element list (R G B), each component in the range [0,1]."
+  (let ((r (nth 0 rgb))
+        (g (nth 1 rgb))
+        (b (nth 2 rgb)))
+    (unless (<= 0 (min r g b) (max r g b) 1)
+      (error "RGB component not in [0,1]"))
+    ;; Simple heuristic -- correct luma requires gamma correction,
+    ;; which is overkill for this purpose.
+    ;; The cut-off value was designed to make saturated green 'light',
+    ;; but saturated red and blue 'dark'.
+    (< (+ (* r 0.299) (* g 0.587) (* b 0.114))
+       0.58)))
 
 (declare-function xw-color-defined-p "xfns.c" (color &optional frame))
 
@@ -1840,6 +1847,15 @@ color-values
    (t
     (tty-color-values color frame))))
 
+(defun color-component-max (&optional frame)
+  "The highest value of a color component on FRAME.
+If FRAME is omitted or nil, use the selected frame."
+  ;; Right now, NS frames are the only ones having a different maximum
+  ;; colour component value.
+  (if (eq (framep-on-display frame) 'ns)
+      #xff00
+    #xffff))
+
 (defalias 'x-color-values 'color-values)
 
 (declare-function xw-display-color-p "xfns.c" (&optional terminal))
diff --git a/lisp/frame.el b/lisp/frame.el
index 6c2f774709..fed46f333d 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -1181,13 +1181,9 @@ frame-set-background-mode
 		   non-default-bg-mode)
 		  ((not (color-values bg-color frame))
 		   default-bg-mode)
-		  ((>= (apply '+ (color-values bg-color frame))
-		       ;; Just looking at the screen, colors whose
-		       ;; values add up to .6 of the white total
-		       ;; still look dark to me.
-		       (* (apply '+ (color-values "white" frame)) .6))
-		   'light)
-		  (t 'dark)))
+                  ((color-dark-p (color-name-to-rgb bg-color frame))
+                   'dark)
+                  (t 'light)))
 	   (display-type
 	    (cond ((null (window-system frame))
 		   (if (tty-display-color-p frame) 'color 'mono))
diff --git a/lisp/term/pc-win.el b/lisp/term/pc-win.el
index 76a48a86c7..16eb660f00 100644
--- a/lisp/term/pc-win.el
+++ b/lisp/term/pc-win.el
@@ -54,11 +54,9 @@
 ;; DJGPP-compiled Emacs on the same PC.  The names of X colors used to
 ;; define the pixel values are shown as comments to each color below.
 ;;;
-;; If you want to change the RGB values, keep in mind that various pieces
-;; of Emacs think that a color whose RGB values add up to less than 0.6 of
-;; the values for WHITE (i.e. less than 117963) are ``dark'', otherwise the
-;; color is ``light''; see `frame-set-background-mode' in lisp/faces.el for
-;; an example.
+;; If you want to change the RGB values, consider the heuristics in
+;; `color-dark-p' which is used to select a suitably contrasting
+;; foreground or background colour.
 (defvar msdos-color-values
   '(("black"          0     0     0     0)
     ("blue"           1     0     0 52480) ; MediumBlue
diff --git a/lisp/term/rxvt.el b/lisp/term/rxvt.el
index 31e3d6ede4..ebcb13db77 100644
--- a/lisp/term/rxvt.el
+++ b/lisp/term/rxvt.el
@@ -206,13 +206,11 @@ rxvt-set-background-mode
       ;; The next line assumes that rxvt-standard-colors are ordered
       ;; by the color index in the ascending order!
       (setq rgb (car (cddr (nth bg rxvt-standard-colors))))
-      ;; See the commentary in frame-set-background-mode about the
-      ;; computation below.
-      (if (< (apply '+ rgb)
-	     ;; The following line assumes that white is the 15th
-	     ;; color in rxvt-standard-colors.
-	     (* (apply '+ (car (cddr (nth 15 rxvt-standard-colors)))) 0.6))
-	  (set-terminal-parameter nil 'background-mode 'dark)))))
+      ;; The following line assumes that white is the 15th
+      ;; color in rxvt-standard-colors.
+      (let ((comp-max (caddr (nth 15 rxvt-standard-colors))))
+        (when (color-dark-p (mapcar (lambda (c) (/ c comp-max)) rgb))
+	  (set-terminal-parameter nil 'background-mode 'dark))))))
 
 (provide 'term/rxvt)
 
diff --git a/lisp/term/w32console.el b/lisp/term/w32console.el
index 36e9d896c7..b249a4e602 100644
--- a/lisp/term/w32console.el
+++ b/lisp/term/w32console.el
@@ -86,9 +86,8 @@ terminal-init-w32console
     (setq r (nth 2 descr)
 	  g (nth 3 descr)
 	  b (nth 4 descr))
-    (if (< (+ r g b) (* .6 (+ 65535 65535 65535)))
-	(setq bg-mode 'dark)
-      (setq bg-mode 'light))
+    (setq bg-mode (if (color-dark-p (list (/ r 65535) (/ g 65535) (/ b 65535)))
+                      'dark 'light))
     (set-terminal-parameter nil 'background-mode bg-mode))
   (tty-set-up-initial-frame-faces)
   (run-hooks 'terminal-init-w32-hook))
diff --git a/lisp/term/xterm.el b/lisp/term/xterm.el
index 1a727e3933..bf9bcae526 100644
--- a/lisp/term/xterm.el
+++ b/lisp/term/xterm.el
@@ -1120,9 +1120,8 @@ xterm-register-default-colors
     (clear-face-cache)))
 
 (defun xterm-maybe-set-dark-background-mode (redc greenc bluec)
-  ;; Use the heuristic in `frame-set-background-mode' to decide if a
-  ;; frame is dark.
-  (when (< (+ redc greenc bluec) (* .6 (+ 65535 65535 65535)))
+  (when (color-dark-p (mapcar (lambda (c) (/ c 65535.0))
+                              (list redc greenc bluec)))
     (set-terminal-parameter nil 'background-mode 'dark)
     t))
 
diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index eda739a397..2cd99787e8 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -1149,17 +1149,6 @@ css--compute-color
    ;; Evaluate to the color if the name is found.
    ((css--named-color start-point match))))
 
-(defun css--contrasty-color (name)
-  "Return a color that contrasts with NAME.
-NAME is of any form accepted by `color-distance'.
-The returned color will be usable by Emacs and will contrast
-with NAME; in particular so that if NAME is used as a background
-color, the returned color can be used as the foreground and still
-be readable."
-  ;; See bug#25525 and bug#41544 for a discussion of this.
-  (if (> (color-distance name "black") 138500)
-      "black" "white"))
-
 (defcustom css-fontify-colors t
   "Whether CSS colors should be fontified using the color as the background.
 When non-`nil', a text representing CSS color will be fontified
@@ -1199,7 +1188,8 @@ css--fontify-region
 		    (add-text-properties
 		     start (point)
 		     (list 'face (list :background color
-				       :foreground (css--contrasty-color color)
+				       :foreground (readable-foreground-color
+                                                    color)
 				       :box '(:line-width -1))))))))))))
     extended-region))
 
-- 
2.21.1 (Apple Git-122.3)


[-- Attachment #3: Type: text/plain, Size: 2 bytes --]

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sun, 31 May 2020 22:46:07 +0200
> Cc: Simon Pugnet <simon@polaris64.net>, 41544@debbugs.gnu.org,
>         Eli Zaretskii <eliz@gnu.org>
> 
> Proposed patch attached. I found css-mode no worse than before (a tad better, if anything). Perhaps we need to decompress to linear components after all, but at least now there is a single place to do it.
> 
> (Should list-colors-display use color-dark-p for the text in its left column, by the way? Or is there a point in not doing so?)

Please remind me why do we want to make such deep and wide changes in
our color system?  Is that just because we have a minor bug in
color-distance?  Wouldn't it be better to just fix that one bug?

I'm worried because the way our automatic color mapping on
color-challenged TTYs works took some time to get right, and it worked
well for years.  If we want to risk rocking that particular boat som
profoundly, we had better had a very good reason, preferably several
good reasons.

Thanks.

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

1 juni 2020 kl. 18.32 skrev Eli Zaretskii <eliz@gnu.org>:

> Please remind me why do we want to make such deep and wide changes in
> our color system?  Is that just because we have a minor bug in
> color-distance?  Wouldn't it be better to just fix that one bug?

These changes are neither deep nor wide. Perhaps the confusion arose from remeding two almost entirely independent issues in the same discussion: the flaws in color-distance, taken care of by the first patch, and the contrasting colour computation, which was the topic of the message and patch you replied to.

Sorry about conflating the two -- I should have opened a separate bug for the colour contrast mess. Their only point of intersection was, in hindsight, rather coincidental (css-mode).

> I'm worried because the way our automatic color mapping on
> color-challenged TTYs works took some time to get right, and it worked
> well for years.  If we want to risk rocking that particular boat som
> profoundly, we had better had a very good reason, preferably several
> good reasons.

Not quite sure what you are talking about here. You previously asked about the exact value of TTY_SAME_COLOR_THRESHOLD. Were you unsatisfied with my answer, and if so, in what respect?

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Mon, 1 Jun 2020 19:24:45 +0200
> Cc: tom@tromey.com, simon@polaris64.net, 41544@debbugs.gnu.org
> 
> > I'm worried because the way our automatic color mapping on
> > color-challenged TTYs works took some time to get right, and it worked
> > well for years.  If we want to risk rocking that particular boat som
> > profoundly, we had better had a very good reason, preferably several
> > good reasons.
> 
> Not quite sure what you are talking about here. You previously asked about the exact value of TTY_SAME_COLOR_THRESHOLD. Were you unsatisfied with my answer, and if so, in what respect?

I'm just looking at the changes.  I see a change in how colors are
converted to RGB triplets.  I see a change in what colors are
considered dark and light, with a new function which decides that that
is being used for frame background mode and in several lisp/term/
files, including 16-color terminals.  I'm asking why do we want to
make all those changes, which modify very basic aspects of our color
support on many terminals.

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> Date: Mon, 01 Jun 2020 20:35:53 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: tom@tromey.com, simon@polaris64.net, 41544@debbugs.gnu.org
> 
> I'm just looking at the changes.  I see a change in how colors are
> converted to RGB triplets.  I see a change in what colors are
> considered dark and light, with a new function which decides that that
> is being used for frame background mode and in several lisp/term/
> files, including 16-color terminals.  I'm asking why do we want to
> make all those changes, which modify very basic aspects of our color
> support on many terminals.

And then, of course, there are the changes in color-distance itself,
which change the values it returns.  Again, why such significant
changes to fix an otherwise insignificant bug?

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

1 juni 2020 kl. 19.44 skrev Eli Zaretskii <eliz@gnu.org>:

>> I'm just looking at the changes.  I see a change in how colors are
>> converted to RGB triplets.  I see a change in what colors are
>> considered dark and light, with a new function which decides that that
>> is being used for frame background mode and in several lisp/term/
>> files, including 16-color terminals.  I'm asking why do we want to
>> make all those changes, which modify very basic aspects of our color
>> support on many terminals.
> 
> And then, of course, there are the changes in color-distance itself,
> which change the values it returns.  Again, why such significant
> changes to fix an otherwise insignificant bug?

It is difficult to give precise answers to vague complaints. Take one thing at a time: as I wrote, there are two different patches addressing two almost completely different issues. Let's start with the color-distance changes, out of respect for the bug reporter if nothing else.

It is not possible to change a function without changing it. Either we fix it or we don't. The reported bug was about broken symmetry, which is rather embarrassing; as written previously, the first analysis uncovered deeper issues worth fixing, such as loss of precision and (especially) the nonlinearity that causes triangle inequality violation.

The proposed fixes to color-distance, I hope you agree, are straightforward, reasonable and address all these points. Callers have been updated with carefully recomputed comparison constants; I detailed how they were obtained in a previous reply, and test have all been satisfactory.

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Tue, 2 Jun 2020 17:27:46 +0200
> Cc: tom@tromey.com, simon@polaris64.net, 41544@debbugs.gnu.org
> 
> Let's start with the color-distance changes, out of respect for the bug reporter if nothing else.
> 
> It is not possible to change a function without changing it. Either we fix it or we don't. The reported bug was about broken symmetry, which is rather embarrassing; as written previously, the first analysis uncovered deeper issues worth fixing, such as loss of precision and (especially) the nonlinearity that causes triangle inequality violation.
> 
> The proposed fixes to color-distance, I hope you agree, are straightforward, reasonable and address all these points. Callers have been updated with carefully recomputed comparison constants; I detailed how they were obtained in a previous reply, and test have all been satisfactory.

I'd prefer to fix only the symmetry bug (which AFAIU happens because
we use bit shifts on signed integers), without introducing any other
effects on the function's behavior and return values.  AFAIU, such a
fix should not require any changes outside of the function itself.

OK?

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 691 bytes --]

2 juni 2020 kl. 18.14 skrev Eli Zaretskii <eliz@gnu.org>:

> I'd prefer to fix only the symmetry bug (which AFAIU happens because
> we use bit shifts on signed integers), without introducing any other
> effects on the function's behavior and return values.  AFAIU, such a
> fix should not require any changes outside of the function itself.

Very well, it is obviously an improvement. The reason for the current asymmetry was actually that the algorithm discarded the low bits; what about fixing that as well? The improved accuracy amounts to less than 1 % of difference in the return value; no other code needs changing, and we get the symmetry for free. Proposed patch attached.


[-- Attachment #2: 0001-Make-color-distance-symmetric-and-more-accurate.patch --]
[-- Type: application/octet-stream, Size: 3902 bytes --]

From b40a56f8be6add33f7de634f0966887622063e43 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Tue, 2 Jun 2020 22:31:14 +0200
Subject: [PATCH] Make color-distance symmetric and more accurate

* src/xfaces.c (color_distance): Don't throw away the low 8 bits of
the colours, and make the function symmetric (bug41544)
(Fcolor_distance): Add caution about this not being a true metric.
* test/src/xfaces-tests.el: New file.
---
 src/xfaces.c             | 24 +++++++++++++-----------
 test/src/xfaces-tests.el | 27 +++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 11 deletions(-)
 create mode 100644 test/src/xfaces-tests.el

diff --git a/src/xfaces.c b/src/xfaces.c
index 7d7aff95c1..cf155288bd 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -4356,15 +4356,15 @@ color_distance (Emacs_Color *x, Emacs_Color *y)
 
      See <https://www.compuphase.com/cmetric.htm> for more info.  */
 
-  long r = (x->red   - y->red)   >> 8;
-  long g = (x->green - y->green) >> 8;
-  long b = (x->blue  - y->blue)  >> 8;
-  long r_mean = (x->red + y->red) >> 9;
-
-  return
-    (((512 + r_mean) * r * r) >> 8)
-    + 4 * g * g
-    + (((767 - r_mean) * b * b) >> 8);
+  long long r = x->red   - y->red;
+  long long g = x->green - y->green;
+  long long b = x->blue  - y->blue;
+  long long r_mean = (x->red + y->red) >> 1;
+
+  return (((((2 * 65536 + r_mean) * r * r) >> 16)
+           + 4 * g * g
+           + (((2 * 65536 + 65535 - r_mean) * b * b) >> 16))
+          >> 16);
 }
 
 
@@ -4374,7 +4374,9 @@ DEFUN ("color-distance", Fcolor_distance, Scolor_distance, 2, 4, 0,
 or lists of the form (RED GREEN BLUE), each in the range 0 to 65535 inclusive.
 If FRAME is unspecified or nil, the current frame is used.
 If METRIC is specified, it should be a function that accepts
-two lists of the form (RED GREEN BLUE) aforementioned. */)
+two lists of the form (RED GREEN BLUE) aforementioned.
+Despite the name, this is not a true distance metric as it does not satisfy
+the triangle inequality.  */)
   (Lisp_Object color1, Lisp_Object color2, Lisp_Object frame,
    Lisp_Object metric)
 {
@@ -4931,7 +4933,7 @@ DEFUN ("face-attributes-as-vector", Fface_attributes_as_vector,
 
 /* If the distance (as returned by color_distance) between two colors is
    less than this, then they are considered the same, for determining
-   whether a color is supported or not.  The range of values is 0-65535.  */
+   whether a color is supported or not.  */
 
 #define TTY_SAME_COLOR_THRESHOLD  10000
 
diff --git a/test/src/xfaces-tests.el b/test/src/xfaces-tests.el
new file mode 100644
index 0000000000..f08a87a518
--- /dev/null
+++ b/test/src/xfaces-tests.el
@@ -0,0 +1,27 @@
+;;; xfaces-tests.el --- tests for xfaces.c           -*- lexical-binding: t -*-
+
+;; Copyright (C) 2020 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+(require 'ert)
+
+(ert-deftest xfaces-color-distance ()
+  ;; Check symmetry (bug#51455).
+  (should (equal (color-distance "#222222" "#ffffff")
+                 (color-distance "#ffffff" "#222222"))))
+
+(provide 'xfaces-tests)
-- 
2.21.1 (Apple Git-122.3)

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Tue, 2 Jun 2020 22:41:03 +0200
> Cc: tom@tromey.com, simon@polaris64.net, 41544@debbugs.gnu.org
> 
> > I'd prefer to fix only the symmetry bug (which AFAIU happens because
> > we use bit shifts on signed integers), without introducing any other
> > effects on the function's behavior and return values.  AFAIU, such a
> > fix should not require any changes outside of the function itself.
> 
> Very well, it is obviously an improvement. The reason for the current asymmetry was actually that the algorithm discarded the low bits; what about fixing that as well?

Sure, let's make it as accurate as the return value allows.

> The improved accuracy amounts to less than 1 % of difference in the return value; no other code needs changing, and we get the symmetry for free. Proposed patch attached.

Not sure I understand completely: does this patch make the function
symmetric?  If so, what additional improvements did you have in mind
when you mentioned more fixing above?

Thanks.

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

3 juni 2020 kl. 16.24 skrev Eli Zaretskii <eliz@gnu.org>:

> Not sure I understand completely: does this patch make the function
> symmetric?

Yes, the patch both improves the accuracy and makes the function symmetric.

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 3 Jun 2020 17:01:46 +0200
> Cc: tom@tromey.com, simon@polaris64.net, 41544@debbugs.gnu.org
> 
> 3 juni 2020 kl. 16.24 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > Not sure I understand completely: does this patch make the function
> > symmetric?
> 
> Yes, the patch both improves the accuracy and makes the function symmetric.

Then I think we should install it.

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

3 juni 2020 kl. 17.59 skrev Eli Zaretskii <eliz@gnu.org>:

> Then I think we should install it.

Thank you; pushed to master.

Now about the consolidation of the contrast colour predicate (color-dark-p): as described previously in detail, the current code for doing so in various places is unsatisfactory. For example, some of the methods employed classify #00ff00 as a "dark" colour, leading to suboptimal results. (Try typing #00ff00 in css-mode.)

There are other bugs that are annoying in themselves, but need to be fixed in order to make progress. Start Emacs in TTY mode with TERM=xterm-color and evaluate (color-name-to-rgb "blue"). Notice how one of the components is greater than 1 -- this is the unfortunate result of several bad decisions.

The attached patch supersedes the previous one; after staring at colour combinations I came to the conclusion that gamma-expansion is a necessity, but the exact sRGB composite gamma curve isn't quite necessary, and a power of 2.2 is close enough.

It also uses a contrasting text colour for the first column in list-colors-display, which serves as a good demonstration of how the predicate works for the standard named colours.


[-- Attachment #2: 0001-Use-a-single-light-dark-colour-predicate.patch --]
[-- Type: application/octet-stream, Size: 11398 bytes --]

From f328bda216f06dd2daff6098c6d60e128da2ef67 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sun, 31 May 2020 21:12:46 +0200
Subject: [PATCH] Use a single light/dark colour predicate

Add a single predicate, color-dark-p, for deciding whether a colour
is more readable against black or white.  Previously this was done in
different ways in several places, and with worse results.  (Bug#41544)

* lisp/color.el (color-name-to-rgb): Use color-component-max instead
of the error-prone method of (color-values "#ffffffffffff"), which
would give unexpected values or outright fail if called without
an initialised display (as in batch mode).
* lisp/facemenu.el (list-colors-print): Use readable-foreground-color.
(color-dark-p, color-component-max): New functions.
* lisp/term/pc-win.el: Update comment.
* lisp/term/rxvt.el (rxvt-set-background-mode):
* lisp/term/w32console.el (terminal-init-w32console):
* lisp/term/xterm.el (xterm-maybe-set-dark-background-mode):
* lisp/faces.el (readable-foreground-color):
* lisp/frame.el (frame-set-background-mode): Use color-dark-p.
* lisp/textmodes/css-mode.el (css--contrasty-color): Remove.
(css--fontify-region): Use color-dark-p.
---
 lisp/color.el              |  4 +---
 lisp/facemenu.el           | 11 +++++------
 lisp/faces.el              | 36 +++++++++++++++++++++++++++---------
 lisp/frame.el              | 10 +++-------
 lisp/term/pc-win.el        |  8 +++-----
 lisp/term/rxvt.el          | 12 +++++-------
 lisp/term/w32console.el    |  5 ++---
 lisp/term/xterm.el         |  5 ++---
 lisp/textmodes/css-mode.el | 14 ++------------
 9 files changed, 50 insertions(+), 55 deletions(-)

diff --git a/lisp/color.el b/lisp/color.el
index 560631ae66..2385874028 100644
--- a/lisp/color.el
+++ b/lisp/color.el
@@ -50,9 +50,7 @@ color-name-to-rgb
 Optional argument FRAME specifies the frame where the color is to be
 displayed.  If FRAME is omitted or nil, use the selected frame.
 If FRAME cannot display COLOR, return nil."
-  ;; `colors-values' maximum value is either 65535 or 65280 depending on the
-  ;; display system.  So we use a white conversion to get the max value.
-  (let ((valmax (float (car (color-values "#ffffffffffff")))))
+  (let ((valmax (float (color-component-max frame))))
     (mapcar (lambda (x) (/ x valmax)) (color-values color frame))))
 
 (defun color-rgb-to-hex  (red green blue &optional digits-per-component)
diff --git a/lisp/facemenu.el b/lisp/facemenu.el
index b10d874b21..419b76101b 100644
--- a/lisp/facemenu.el
+++ b/lisp/facemenu.el
@@ -621,12 +621,11 @@ list-colors-print
 						 (downcase b))))))
 	(setq color (list color)))
       (let* ((opoint (point))
-	     (color-values (color-values (car color)))
-	     (light-p (>= (apply 'max color-values)
-			  (* (car (color-values "white")) .5))))
+             (fg (readable-foreground-color (car color))))
 	(insert (car color))
 	(indent-to 22)
-	(put-text-property opoint (point) 'face `(:background ,(car color)))
+	(put-text-property opoint (point) 'face `(:background ,(car color)
+                                                  :foreground ,fg))
 	(put-text-property
 	 (prog1 (point)
 	   (insert " ")
@@ -639,7 +638,7 @@ list-colors-print
 	(insert (propertize
 		 (apply 'format "#%02x%02x%02x"
 			(mapcar (lambda (c) (ash c -8))
-				color-values))
+				(color-values (car color))))
 		 'mouse-face 'highlight
 		 'help-echo
 		 (let ((hsv (apply 'color-rgb-to-hsv
@@ -651,7 +650,7 @@ list-colors-print
 	   opoint (point)
 	   'follow-link t
 	   'mouse-face (list :background (car color)
-			     :foreground (if light-p "black" "white"))
+			     :foreground fg)
 	   'color-name (car color)
 	   'action callback-fn)))
       (insert "\n"))
diff --git a/lisp/faces.el b/lisp/faces.el
index e707f6f4b6..5958427946 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1786,15 +1786,24 @@ defined-colors-with-face-attributes
 
 (defun readable-foreground-color (color)
   "Return a readable foreground color for background COLOR."
-  (let* ((rgb   (color-values color))
-	 (max   (apply #'max rgb))
-	 (black (car (color-values "black")))
-	 (white (car (color-values "white"))))
-    ;; Select black or white depending on which one is less similar to
-    ;; the brightest component.
-    (if (> (abs (- max black)) (abs (- max white)))
-	"black"
-      "white")))
+  (if (color-dark-p (color-name-to-rgb color)) "white" "black"))
+
+(defun color-dark-p (rgb)
+  "Whether RGB is more readable against white than black.
+RGB is a 3-element list (R G B), each component in the range [0,1]."
+  (let* ((sr (nth 0 rgb))
+         (sg (nth 1 rgb))
+         (sb (nth 2 rgb))
+         ;; Use the power 2.2 as an approximation to sRGB gamma;
+         ;; it should be good enough for the purpose of this function.
+         (r (expt sr 2.2))
+         (g (expt sg 2.2))
+         (b (expt sb 2.2)))
+    (unless (<= 0 (min r g b) (max r g b) 1)
+      (error "RGB components %S not in [0,1]" rgb))
+    ;; The cut-off value was determined experimentally; see bug#41544.
+    (< (+ (* r 0.299) (* g 0.587) (* b 0.114))
+       (eval-when-compile (expt 0.6 2.2)))))
 
 (declare-function xw-color-defined-p "xfns.c" (color &optional frame))
 
@@ -1840,6 +1849,15 @@ color-values
    (t
     (tty-color-values color frame))))
 
+(defun color-component-max (&optional frame)
+  "The highest value of a color component on FRAME.
+If FRAME is omitted or nil, use the selected frame."
+  ;; Right now, NS frames are the only ones having a different maximum
+  ;; colour component value.
+  (if (eq (framep-on-display frame) 'ns)
+      #xff00
+    #xffff))
+
 (defalias 'x-color-values 'color-values)
 
 (declare-function xw-display-color-p "xfns.c" (&optional terminal))
diff --git a/lisp/frame.el b/lisp/frame.el
index 6c2f774709..fed46f333d 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -1181,13 +1181,9 @@ frame-set-background-mode
 		   non-default-bg-mode)
 		  ((not (color-values bg-color frame))
 		   default-bg-mode)
-		  ((>= (apply '+ (color-values bg-color frame))
-		       ;; Just looking at the screen, colors whose
-		       ;; values add up to .6 of the white total
-		       ;; still look dark to me.
-		       (* (apply '+ (color-values "white" frame)) .6))
-		   'light)
-		  (t 'dark)))
+                  ((color-dark-p (color-name-to-rgb bg-color frame))
+                   'dark)
+                  (t 'light)))
 	   (display-type
 	    (cond ((null (window-system frame))
 		   (if (tty-display-color-p frame) 'color 'mono))
diff --git a/lisp/term/pc-win.el b/lisp/term/pc-win.el
index 76a48a86c7..16eb660f00 100644
--- a/lisp/term/pc-win.el
+++ b/lisp/term/pc-win.el
@@ -54,11 +54,9 @@
 ;; DJGPP-compiled Emacs on the same PC.  The names of X colors used to
 ;; define the pixel values are shown as comments to each color below.
 ;;;
-;; If you want to change the RGB values, keep in mind that various pieces
-;; of Emacs think that a color whose RGB values add up to less than 0.6 of
-;; the values for WHITE (i.e. less than 117963) are ``dark'', otherwise the
-;; color is ``light''; see `frame-set-background-mode' in lisp/faces.el for
-;; an example.
+;; If you want to change the RGB values, consider the heuristics in
+;; `color-dark-p' which is used to select a suitably contrasting
+;; foreground or background colour.
 (defvar msdos-color-values
   '(("black"          0     0     0     0)
     ("blue"           1     0     0 52480) ; MediumBlue
diff --git a/lisp/term/rxvt.el b/lisp/term/rxvt.el
index 31e3d6ede4..ebcb13db77 100644
--- a/lisp/term/rxvt.el
+++ b/lisp/term/rxvt.el
@@ -206,13 +206,11 @@ rxvt-set-background-mode
       ;; The next line assumes that rxvt-standard-colors are ordered
       ;; by the color index in the ascending order!
       (setq rgb (car (cddr (nth bg rxvt-standard-colors))))
-      ;; See the commentary in frame-set-background-mode about the
-      ;; computation below.
-      (if (< (apply '+ rgb)
-	     ;; The following line assumes that white is the 15th
-	     ;; color in rxvt-standard-colors.
-	     (* (apply '+ (car (cddr (nth 15 rxvt-standard-colors)))) 0.6))
-	  (set-terminal-parameter nil 'background-mode 'dark)))))
+      ;; The following line assumes that white is the 15th
+      ;; color in rxvt-standard-colors.
+      (let ((comp-max (caddr (nth 15 rxvt-standard-colors))))
+        (when (color-dark-p (mapcar (lambda (c) (/ c comp-max)) rgb))
+	  (set-terminal-parameter nil 'background-mode 'dark))))))
 
 (provide 'term/rxvt)
 
diff --git a/lisp/term/w32console.el b/lisp/term/w32console.el
index 36e9d896c7..b249a4e602 100644
--- a/lisp/term/w32console.el
+++ b/lisp/term/w32console.el
@@ -86,9 +86,8 @@ terminal-init-w32console
     (setq r (nth 2 descr)
 	  g (nth 3 descr)
 	  b (nth 4 descr))
-    (if (< (+ r g b) (* .6 (+ 65535 65535 65535)))
-	(setq bg-mode 'dark)
-      (setq bg-mode 'light))
+    (setq bg-mode (if (color-dark-p (list (/ r 65535) (/ g 65535) (/ b 65535)))
+                      'dark 'light))
     (set-terminal-parameter nil 'background-mode bg-mode))
   (tty-set-up-initial-frame-faces)
   (run-hooks 'terminal-init-w32-hook))
diff --git a/lisp/term/xterm.el b/lisp/term/xterm.el
index 1a727e3933..bf9bcae526 100644
--- a/lisp/term/xterm.el
+++ b/lisp/term/xterm.el
@@ -1120,9 +1120,8 @@ xterm-register-default-colors
     (clear-face-cache)))
 
 (defun xterm-maybe-set-dark-background-mode (redc greenc bluec)
-  ;; Use the heuristic in `frame-set-background-mode' to decide if a
-  ;; frame is dark.
-  (when (< (+ redc greenc bluec) (* .6 (+ 65535 65535 65535)))
+  (when (color-dark-p (mapcar (lambda (c) (/ c 65535.0))
+                              (list redc greenc bluec)))
     (set-terminal-parameter nil 'background-mode 'dark)
     t))
 
diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 0035c5e7b0..2cd99787e8 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -1149,17 +1149,6 @@ css--compute-color
    ;; Evaluate to the color if the name is found.
    ((css--named-color start-point match))))
 
-(defun css--contrasty-color (name)
-  "Return a color that contrasts with NAME.
-NAME is of any form accepted by `color-distance'.
-The returned color will be usable by Emacs and will contrast
-with NAME; in particular so that if NAME is used as a background
-color, the returned color can be used as the foreground and still
-be readable."
-  ;; See bug#25525 for a discussion of this.
-  (if (> (color-distance name "black") 292485)
-      "black" "white"))
-
 (defcustom css-fontify-colors t
   "Whether CSS colors should be fontified using the color as the background.
 When non-`nil', a text representing CSS color will be fontified
@@ -1199,7 +1188,8 @@ css--fontify-region
 		    (add-text-properties
 		     start (point)
 		     (list 'face (list :background color
-				       :foreground (css--contrasty-color color)
+				       :foreground (readable-foreground-color
+                                                    color)
 				       :box '(:line-width -1))))))))))))
     extended-region))
 
-- 
2.21.1 (Apple Git-122.3)

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 3 Jun 2020 22:08:46 +0200
> Cc: Tom Tromey <tom@tromey.com>, Simon Pugnet <simon@polaris64.net>,
>         41544@debbugs.gnu.org
> 
> Now about the consolidation of the contrast colour predicate (color-dark-p): as described previously in detail, the current code for doing so in various places is unsatisfactory. For example, some of the methods employed classify #00ff00 as a "dark" colour, leading to suboptimal results. (Try typing #00ff00 in css-mode.)

Let's please discuss each problem in detail (I tried to understand
them from the log message you posted, but couldn't find rationale
there).

And in any case, I will prefer solutions that fix any problems
locally, not changes in low-level stuff used in many other places,
because the latter run the risk of introducing new bugs.  As the
problems are quite minor, AFAICT, solving them in unsafe ways is
something to be avoided.

> There are other bugs that are annoying in themselves, but need to be fixed in order to make progress. Start Emacs in TTY mode with TERM=xterm-color and evaluate (color-name-to-rgb "blue"). Notice how one of the components is greater than 1 -- this is the unfortunate result of several bad decisions.

You mean, the component that is 1.0393?  What bad decisions caused
that, what problems does this small deviation causes in itself?  We
should weigh the gravity of the problems we try to solve here with the
potential of breaking working code elsewhere which relies on these
idiosyncrasies.

Thanks.

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

4 juni 2020 kl. 16.07 skrev Eli Zaretskii <eliz@gnu.org>:

> Let's please discuss each problem in detail (I tried to understand
> them from the log message you posted, but couldn't find rationale
> there).
> 
> And in any case, I will prefer solutions that fix any problems
> locally, not changes in low-level stuff used in many other places,
> because the latter run the risk of introducing new bugs.  As the
> problems are quite minor, AFAICT, solving them in unsafe ways is
> something to be avoided.

I think we agree. There should be nothing unsafe here other than the code being replaced, but code should be scrutinised.

> You mean, the component that is 1.0393?  What bad decisions caused
> that, what problems does this small deviation causes in itself?

Yes, this is as good a place to start as any, and the fix for this is a good change on its own. It goes something like this:

1. color-name-to-rgb calls (color-values "#ffffffffffff") as a means to get the range of the colour values.
2. With TERM=xterm-color, there are 8 colours. These are assumed to be the 8 first of xterm-standard-colors (xterm.el).
3. The colour closest to "#ffffffffffff" is "white", with the values (229 229 229), or translated to 16 bit/channel, (#e5e5 #e5e5 #e5e5) which color-values returns.
4. "blue" has the values (0 0 238), or (0 0 #xeeee).
5. Thus color-name-to-rgb returns #xeeee/#xe5e5 for the blue channel, or 1.039, which is a clear bug.

The main problem is trusting "#ffffffffffff" to match a colour with the maximum range. It could be argued that xterm.el shouldn't use subdued colours when only 8 are present; I didn't go far back in XTerm history to find out. Modern XTerm has default colours 0-7 that correspond to the assumptions of Emacs.

Since we already document that the colour channel maximum is either 65535 or 65280 depending on platform, taking the very roundabout way of trying to match a sufficiently white colour and using its components is demonstrably unsafe and error-prone, as well as unnecessarily slow. Hence color-component-max in the patch.

This also fixes a different problem: if the display hasn't been initialised fully, such as when running in batch mode, then (color-values "#ffffffffffff") returns nil, and as we shall see later, it may be useful to be able to call color-name-to-rgb at this stage without crashing.

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Thu, 4 Jun 2020 17:29:06 +0200
> Cc: tom@tromey.com, simon@polaris64.net, 41544@debbugs.gnu.org
> 
> 1. color-name-to-rgb calls (color-values "#ffffffffffff") as a means to get the range of the colour values.

There's some history to that: see bug#25890.

> 2. With TERM=xterm-color, there are 8 colours. These are assumed to be the 8 first of xterm-standard-colors (xterm.el).
> 3. The colour closest to "#ffffffffffff" is "white", with the values (229 229 229), or translated to 16 bit/channel, (#e5e5 #e5e5 #e5e5) which color-values returns.
> 4. "blue" has the values (0 0 238), or (0 0 #xeeee).
> 5. Thus color-name-to-rgb returns #xeeee/#xe5e5 for the blue channel, or 1.039, which is a clear bug.

What bad results does this issue cause in practice?

(Btw, in a GUI session I see (0.0 0.0 1.0), so no problem there.)

> The main problem is trusting "#ffffffffffff" to match a colour with the maximum range.

Why is that a problem, given the color representation we use in Emacs?

> It could be argued that xterm.el shouldn't use subdued colours when only 8 are present; I didn't go far back in XTerm history to find out. Modern XTerm has default colours 0-7 that correspond to the assumptions of Emacs.
> 
> Since we already document that the colour channel maximum is either 65535 or 65280 depending on platform, taking the very roundabout way of trying to match a sufficiently white colour and using its components is demonstrably unsafe and error-prone, as well as unnecessarily slow. Hence color-component-max in the patch.

Sorry, you lost me here.  I don't understand what you are saying here
and what does that have to do with the problem being discussed.

> This also fixes a different problem: if the display hasn't been initialised fully, such as when running in batch mode, then (color-values "#ffffffffffff") returns nil, and as we shall see later, it may be useful to be able to call color-name-to-rgb at this stage without crashing.

This command:

  emacs -batch --eval "(message \"%s\" (color-name-to-rgb \"white\"))"

yields "(1.0 1.0 1.0)" on my system with today's master branch, and
this command:

  emacs -batch --eval "(message \"%s\" (color-values \"#ffffffffffff\"))"

yields "(65535 65535 65535)", so I don't think I understand what
problem you are concerned with here, and how can this cause a crash.
Please elaborate.

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

5 juni 2020 kl. 14.27 skrev Eli Zaretskii <eliz@gnu.org>:

> There's some history to that: see bug#25890.

Thank you very much, that certainly gives some historical perspective!

> What bad results does this issue cause in practice?

The immediate bad result is that color-name-to-rgb returns a value that is (a) wrong and (b) outside the range of legal values for that function. Code calling it expect the value to be (a) correct and (b) within the range of legal values.

> (Btw, in a GUI session I see (0.0 0.0 1.0), so no problem there.)

Of course, but this was specifically in terminals where the colour closest to full white isn't.

>> The main problem is trusting "#ffffffffffff" to match a colour with the maximum range.
> 
> Why is that a problem, given the color representation we use in Emacs?

Because there is not always a matching (white) colour that has the maximum component value. The example I gave was for TERM=xterm-color, where the closest colour is (#xe5e5 #xe5e5 #xe5e5).

Note that this does not mean that the gamut for that terminal is somehow normalised with (0xe5e5 0xe5e5 0xe5e5) as perfect white; it is not. It is still the case that the maximum component value is either #xffff or #xff00; in this case #xffff.

Now, for non-TTY Emacs, we typically have an unlimited number of colours and (color-values "#ffffffffffff") returns the expected value. The same goes for a TTY where "white" is indeed white and not washed-out grey, such as TERM=linux.

> Sorry, you lost me here.  I don't understand what you are saying here
> and what does that have to do with the problem being discussed.

Let me try again:
(1) We know that the maximum colour component value is 65535 or 65280, depending on the platform (display system).
(2) color-name-to-rgb needs the maximum colour component value in order to normalise the result.
(3) color-name-to-rgb currently uses (color-values "#ffffffffffff") to obtain the maximum colour component value, but it is not always correct.
(4) Instead, we can just use 65535 or 65280 right away, which is fast and always correct.

The rest of my argument was merely discarding the possible alternative solution of redefining "white" as (255 255 255) for xterm-color.

> yields "(65535 65535 65535)", so I don't think I understand what
> problem you are concerned with here, and how can this cause a crash.

Sorry, I should have been more specific: this condition is present earlier, in frame-set-background-mode, before the --eval arguments are processed. This only pertains to the main part of the patch, which we have not yet discussed. I cannot fault you for being impatient!

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 5 Jun 2020 17:50:47 +0200
> Cc: tom@tromey.com, simon@polaris64.net, 41544@debbugs.gnu.org
> 
> > What bad results does this issue cause in practice?
> 
> The immediate bad result is that color-name-to-rgb returns a value that is (a) wrong and (b) outside the range of legal values for that function. Code calling it expect the value to be (a) correct and (b) within the range of legal values.

That in itself is not bad, IMO.  When I said "in practice", I meant
practical problems this causes, and that inevitably involves some
callers of that function (and the callers of those callers) that
suffer problems which show on display or cause incorrect decisions to
be made in specific Lisp applications.  What you presented are
theoretical difficulties that IMO don't yet justify any significant
changes on this level, not by themselves.

> > (Btw, in a GUI session I see (0.0 0.0 1.0), so no problem there.)
> 
> Of course, but this was specifically in terminals where the colour closest to full white isn't.

On such terminals we will always have a problem, because "white" (and
"red" and "blue", and in fact any color specified by its name) is not
well-defined.  Their RGB values depend on external factors and
configurations that we cannot control, like X-level configuration of
the first 8 or 16 xterm colors.

IOW, this problem cannot be solved in principle, and we shouldn't even
try.  We currently have a solution that works "well enough" for those
cases, and I see no reason to make any significant changes in what we
arrived at after a long journey (which started during development of
Emacs 21).

> >> The main problem is trusting "#ffffffffffff" to match a colour with the maximum range.
> > 
> > Why is that a problem, given the color representation we use in Emacs?
> 
> Because there is not always a matching (white) colour that has the maximum component value.

This cannot be helped on a TTY.  Using #ffffffffffff is as good an
approximation as any other, and better than some which we tried in the
past.  I see no reason to make any changes due to this theoretical
issue.

> (1) We know that the maximum colour component value is 65535 or 65280, depending on the platform (display system).
> (2) color-name-to-rgb needs the maximum colour component value in order to normalise the result.
> (3) color-name-to-rgb currently uses (color-values "#ffffffffffff") to obtain the maximum colour component value, but it is not always correct.
> (4) Instead, we can just use 65535 or 65280 right away, which is fast and always correct.

This would make the result dependent on the frame, since the TTY type
can be different for different frames.  That would give rise to new
and exciting bugs, because these APIs currently don't accept a FRAME
argument (adding such an argument, while it can be made
backward-compatible, will take eons to propagate to Lisp code).

Again, I see no justification for such a change.  If we think these
minor deviations from theoretically perfect results may confuse
someone, we can document these pitfalls in any number of words we see
fit.

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

[I've left Tom and Simon out of the CC list to spare them the noise.]

6 juni 2020 kl. 09.29 skrev Eli Zaretskii <eliz@gnu.org>:

> That in itself is not bad, IMO.  When I said "in practice", I meant
> practical problems this causes, and that inevitably involves some
> callers of that function (and the callers of those callers) that
> suffer problems which show on display or cause incorrect decisions to
> be made in specific Lisp applications.  What you presented are
> theoretical difficulties that IMO don't yet justify any significant
> changes on this level, not by themselves.

Thank you all the same, but I'd like to fix this bug nevertheless. It is clearly a bug, and I'm one of those writing code calling color-values and thus being affected by it. Of course, if you can show some negative consequence of the suggested fix, then some alternative has to be considered.

Instead of replying point-for-point, which can go on forever, let's try to break the stalemate; we are clearly talking past one another. I'm trying to understand your assumptions, and hope that you will do me the same courtesy.

The values returned from color-values are scaled to a maximum of 65535 for all Emacs displays (except NS). Just because a TTY does not have a 'white' colour with RGB values (65535 65535 65535) does not mean that the scale is somehow different.

In the case of TERM=xterm-color, the brightest colour (confusingly named "white") is (58853 58853 58853). This doesn't mean that 58853 is the maximum colour component value; it just means that the brightest colour is not pure white but something like a 90% grey, ie (0.9 0.9 0.9) in 1-normalised RGB notation.

The method of using (color-values "#ffffffffffff") was a clever trick for obtaining the scale factor without having to know exactly what the maximum was for that frame, since parts of Emacs had different ideas of what range to actually use: it was common for some time to convert from 8 to 16 bit/channel by shifting 8 bits to the left. I've read through bug#25890 and bug#24273, as well as poured over the change history, and it seems very clear where this came from.

However, the back-end code appears much more robust and regular now, and the code can be simplified, as well as avoiding the irregularities occurring with TTYs lacking a pure white colour. Surely there is no harm in that?

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 6 Jun 2020 12:59:53 +0200
> Cc: 41544@debbugs.gnu.org
> 
> Thank you all the same, but I'd like to fix this bug nevertheless. It is clearly a bug, and I'm one of those writing code calling color-values and thus being affected by it. Of course, if you can show some negative consequence of the suggested fix, then some alternative has to be considered.

I have already pointed out the negative consequences: the fix you
proposed changes the behavior and return values of a low-level API
that is used in many places, both directly and indirectly.  Thus, it
runs a high risk of producing bugs and breaking code that works well
enough now.

That is, assuming we are still talking about the last patch you
posted in this matter.

> Instead of replying point-for-point, which can go on forever, let's try to break the stalemate; we are clearly talking past one another. I'm trying to understand your assumptions, and hope that you will do me the same courtesy.

My assumption is that making changes for purely academic and/or
aesthetic reasons is something that we should avoid.  Time and again
such changes just introduce bugs in other places, wasting our time and
scarce resources, and leaving the overall quality of Emacs is none the
better.  I will therefore object to any low-level changes that don't
fix clear-cut practical problems in some important functionality.

> The values returned from color-values are scaled to a maximum of 65535 for all Emacs displays (except NS). Just because a TTY does not have a 'white' colour with RGB values (65535 65535 65535) does not mean that the scale is somehow different.
> 
> In the case of TERM=xterm-color, the brightest colour (confusingly named "white") is (58853 58853 58853). This doesn't mean that 58853 is the maximum colour component value; it just means that the brightest colour is not pure white but something like a 90% grey, ie (0.9 0.9 0.9) in 1-normalised RGB notation.
> 
> The method of using (color-values "#ffffffffffff") was a clever trick for obtaining the scale factor without having to know exactly what the maximum was for that frame, since parts of Emacs had different ideas of what range to actually use: it was common for some time to convert from 8 to 16 bit/channel by shifting 8 bits to the left. I've read through bug#25890 and bug#24273, as well as poured over the change history, and it seems very clear where this came from.
> 
> However, the back-end code appears much more robust and regular now, and the code can be simplified, as well as avoiding the irregularities occurring with TTYs lacking a pure white colour. Surely there is no harm in that?

Here you again lost me, sorry.  You asked for understanding of your
assumptions, but I cannot glean those assumptions from the text above.
I don't even understand what each paragraph above tries to say, and/or
with what argument of mine it attempts to argue.

Specifically, what is there that is the current state of affairs, what
is that _should_be_ the state of affairs in your opinion (a.k.a. "your
assumptions", I presume), and why what we have now is in your opinion
so bad that we must fix it?

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

6 juni 2020 kl. 13.59 skrev Eli Zaretskii <eliz@gnu.org>:

> I have already pointed out the negative consequences: the fix you
> proposed changes the behavior and return values of a low-level API
> that is used in many places, both directly and indirectly. Thus, it
> runs a high risk of producing bugs and breaking code that works well
> enough now.

This is very speculative and hypothetical. Forgive me for being sceptical, but can you come up with a concrete and realistic example of what you think will break?

> That is, assuming we are still talking about the last patch you
> posted in this matter.

We were specifically talking about fixing the bug in color-name-to-rgb, I believe. It is subordinate to the main change, which we have not discussed at all. If you like, we could leave color-name-to-rgb alone, and we will see whether the change is needed when doing the actual work (color-dark-p).

> My assumption is that making changes for purely academic and/or
> aesthetic reasons is something that we should avoid.

That is a rather disparaging way of referring to fixes intended to make code working as advertised. 

> I don't even understand what each paragraph above tries to say, and/or
> with what argument of mine it attempts to argue.

You were saying that #ffffffffffff is as good an approximation as any other, and I was showing that it's not.

> Specifically, what is there that is the current state of affairs, what
> is that _should_be_ the state of affairs

The current state of affairs is that 'color-values' returns an incorrect value in certain cases. This can be fixed by making the code simpler and more robust.

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 6 Jun 2020 15:29:31 +0200
> Cc: 41544@debbugs.gnu.org
> 
> 6 juni 2020 kl. 13.59 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > I have already pointed out the negative consequences: the fix you
> > proposed changes the behavior and return values of a low-level API
> > that is used in many places, both directly and indirectly. Thus, it
> > runs a high risk of producing bugs and breaking code that works well
> > enough now.
> 
> This is very speculative and hypothetical. Forgive me for being sceptical, but can you come up with a concrete and realistic example of what you think will break?

None at this time.  But bitter experience has taught me that they will
almost certainly come.  They always do.

> > My assumption is that making changes for purely academic and/or
> > aesthetic reasons is something that we should avoid.
> 
> That is a rather disparaging way of referring to fixes intended to make code working as advertised. 

If the problem is that the documentation doesn't match the behavior,
it is much easier for me to agree to amend the documentation.  In this
case, I think a Lisp program that interprets the documentation too
literally is making a mistake, but I'm not opposed to make that
clearer in the docs.

> > I don't even understand what each paragraph above tries to say, and/or
> > with what argument of mine it attempts to argue.
> 
> You were saying that #ffffffffffff is as good an approximation as any other, and I was showing that it's not.

Then I'm not convinced, sorry.

> > Specifically, what is there that is the current state of affairs, what
> > is that _should_be_ the state of affairs
> 
> The current state of affairs is that 'color-values' returns an incorrect value in certain cases. This can be fixed by making the code simpler and more robust.

We've made a full circle: I was talking about the effects on the
callers of color-values and color-name-to-rgb, and explicitly asked
that we don't limit ourselves to these functions alone.  If there are
no problems caused to the callers of these functions, then I think we
should leave the code alone.

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 686 bytes --]

6 juni 2020 kl. 15.57 skrev Eli Zaretskii <eliz@gnu.org>:

>> can you come up with a concrete and realistic example of what you think will break?
> 
> None at this time.

That's high praise!

> I think a Lisp program that interprets the documentation too
> literally is making a mistake

I must remember that, a most useful answer!

> , but I'm not opposed to make that
> clearer in the docs.

No, I really don't think we should document the bug.

Since we are making little progress, let's leave color-name-to-rgb unchanged for the moment. We can both change our minds later. It's not strictly required for the introduction and use of color-dark-p; patch updated.


[-- Attachment #2: 0001-Use-a-single-light-dark-colour-predicate.patch --]
[-- Type: application/octet-stream, Size: 10474 bytes --]

From f7693e7a2e6cc65ad40d42c9854539ed85466bae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sun, 31 May 2020 21:12:46 +0200
Subject: [PATCH] Use a single light/dark colour predicate

Add a single predicate, color-dark-p, for deciding whether a colour
is more readable against black or white.  Previously this was done in
different ways in several places, and with worse results.  (Bug#41544)

* lisp/facemenu.el (list-colors-print): Use readable-foreground-color.
(color-dark-p): New function.
* lisp/term/pc-win.el: Update comment.
* lisp/term/rxvt.el (rxvt-set-background-mode):
* lisp/term/w32console.el (terminal-init-w32console):
* lisp/term/xterm.el (xterm-maybe-set-dark-background-mode):
* lisp/faces.el (readable-foreground-color):
* lisp/frame.el (frame-set-background-mode): Use color-dark-p.
* lisp/textmodes/css-mode.el (css--contrasty-color): Remove.
(css--fontify-region): Use color-dark-p.
---
 lisp/facemenu.el           | 11 +++++------
 lisp/faces.el              | 27 ++++++++++++++++++---------
 lisp/frame.el              | 17 ++++++++++-------
 lisp/term/pc-win.el        |  8 +++-----
 lisp/term/rxvt.el          | 12 +++++-------
 lisp/term/w32console.el    |  6 +++---
 lisp/term/xterm.el         |  5 ++---
 lisp/textmodes/css-mode.el | 14 ++------------
 8 files changed, 48 insertions(+), 52 deletions(-)

diff --git a/lisp/facemenu.el b/lisp/facemenu.el
index b10d874b21..419b76101b 100644
--- a/lisp/facemenu.el
+++ b/lisp/facemenu.el
@@ -621,12 +621,11 @@ list-colors-print
 						 (downcase b))))))
 	(setq color (list color)))
       (let* ((opoint (point))
-	     (color-values (color-values (car color)))
-	     (light-p (>= (apply 'max color-values)
-			  (* (car (color-values "white")) .5))))
+             (fg (readable-foreground-color (car color))))
 	(insert (car color))
 	(indent-to 22)
-	(put-text-property opoint (point) 'face `(:background ,(car color)))
+	(put-text-property opoint (point) 'face `(:background ,(car color)
+                                                  :foreground ,fg))
 	(put-text-property
 	 (prog1 (point)
 	   (insert " ")
@@ -639,7 +638,7 @@ list-colors-print
 	(insert (propertize
 		 (apply 'format "#%02x%02x%02x"
 			(mapcar (lambda (c) (ash c -8))
-				color-values))
+				(color-values (car color))))
 		 'mouse-face 'highlight
 		 'help-echo
 		 (let ((hsv (apply 'color-rgb-to-hsv
@@ -651,7 +650,7 @@ list-colors-print
 	   opoint (point)
 	   'follow-link t
 	   'mouse-face (list :background (car color)
-			     :foreground (if light-p "black" "white"))
+			     :foreground fg)
 	   'color-name (car color)
 	   'action callback-fn)))
       (insert "\n"))
diff --git a/lisp/faces.el b/lisp/faces.el
index e707f6f4b6..caa72fbfff 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1786,15 +1786,24 @@ defined-colors-with-face-attributes
 
 (defun readable-foreground-color (color)
   "Return a readable foreground color for background COLOR."
-  (let* ((rgb   (color-values color))
-	 (max   (apply #'max rgb))
-	 (black (car (color-values "black")))
-	 (white (car (color-values "white"))))
-    ;; Select black or white depending on which one is less similar to
-    ;; the brightest component.
-    (if (> (abs (- max black)) (abs (- max white)))
-	"black"
-      "white")))
+  (if (color-dark-p (color-name-to-rgb color)) "white" "black"))
+
+(defun color-dark-p (rgb)
+  "Whether RGB is more readable against white than black.
+RGB is a 3-element list (R G B), each component in the range [0,1]."
+  (let* ((sr (nth 0 rgb))
+         (sg (nth 1 rgb))
+         (sb (nth 2 rgb))
+         ;; Use the power 2.2 as an approximation to sRGB gamma;
+         ;; it should be good enough for the purpose of this function.
+         (r (expt sr 2.2))
+         (g (expt sg 2.2))
+         (b (expt sb 2.2)))
+    (unless (<= 0 (min r g b) (max r g b) 1)
+      (error "RGB components %S not in [0,1]" rgb))
+    ;; The cut-off value was determined experimentally; see bug#41544.
+    (< (+ (* r 0.299) (* g 0.587) (* b 0.114))
+       (eval-when-compile (expt 0.6 2.2)))))
 
 (declare-function xw-color-defined-p "xfns.c" (color &optional frame))
 
diff --git a/lisp/frame.el b/lisp/frame.el
index 6c2f774709..253528da75 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -1156,6 +1156,13 @@ frame-background-mode
 
 (defvar inhibit-frame-set-background-mode nil)
 
+(defun frame--color-name-to-rgb (color frame)
+  "Convert the COLOR string to a list of normalised RGB components.
+Like `color-name-to-rgb', but works even when the display has not yet
+been initialised."
+  (let ((valmax (if (eq (framep-on-display frame) 'ns) 65280.0 65535.0)))
+    (mapcar (lambda (x) (/ x valmax)) (color-values color frame))))
+
 (defun frame-set-background-mode (frame &optional keep-face-specs)
   "Set up display-dependent faces on FRAME.
 Display-dependent faces are those which have different definitions
@@ -1181,13 +1188,9 @@ frame-set-background-mode
 		   non-default-bg-mode)
 		  ((not (color-values bg-color frame))
 		   default-bg-mode)
-		  ((>= (apply '+ (color-values bg-color frame))
-		       ;; Just looking at the screen, colors whose
-		       ;; values add up to .6 of the white total
-		       ;; still look dark to me.
-		       (* (apply '+ (color-values "white" frame)) .6))
-		   'light)
-		  (t 'dark)))
+                  ((color-dark-p (frame--color-name-to-rgb bg-color frame))
+                   'dark)
+                  (t 'light)))
 	   (display-type
 	    (cond ((null (window-system frame))
 		   (if (tty-display-color-p frame) 'color 'mono))
diff --git a/lisp/term/pc-win.el b/lisp/term/pc-win.el
index 76a48a86c7..16eb660f00 100644
--- a/lisp/term/pc-win.el
+++ b/lisp/term/pc-win.el
@@ -54,11 +54,9 @@
 ;; DJGPP-compiled Emacs on the same PC.  The names of X colors used to
 ;; define the pixel values are shown as comments to each color below.
 ;;;
-;; If you want to change the RGB values, keep in mind that various pieces
-;; of Emacs think that a color whose RGB values add up to less than 0.6 of
-;; the values for WHITE (i.e. less than 117963) are ``dark'', otherwise the
-;; color is ``light''; see `frame-set-background-mode' in lisp/faces.el for
-;; an example.
+;; If you want to change the RGB values, consider the heuristics in
+;; `color-dark-p' which is used to select a suitably contrasting
+;; foreground or background colour.
 (defvar msdos-color-values
   '(("black"          0     0     0     0)
     ("blue"           1     0     0 52480) ; MediumBlue
diff --git a/lisp/term/rxvt.el b/lisp/term/rxvt.el
index 31e3d6ede4..5dc754c8e0 100644
--- a/lisp/term/rxvt.el
+++ b/lisp/term/rxvt.el
@@ -206,13 +206,11 @@ rxvt-set-background-mode
       ;; The next line assumes that rxvt-standard-colors are ordered
       ;; by the color index in the ascending order!
       (setq rgb (car (cddr (nth bg rxvt-standard-colors))))
-      ;; See the commentary in frame-set-background-mode about the
-      ;; computation below.
-      (if (< (apply '+ rgb)
-	     ;; The following line assumes that white is the 15th
-	     ;; color in rxvt-standard-colors.
-	     (* (apply '+ (car (cddr (nth 15 rxvt-standard-colors)))) 0.6))
-	  (set-terminal-parameter nil 'background-mode 'dark)))))
+      ;; The following line assumes that white is the 15th
+      ;; color in rxvt-standard-colors.
+      (let ((comp-max (float (caddr (nth 15 rxvt-standard-colors)))))
+        (when (color-dark-p (mapcar (lambda (c) (/ c comp-max)) rgb))
+	  (set-terminal-parameter nil 'background-mode 'dark))))))
 
 (provide 'term/rxvt)
 
diff --git a/lisp/term/w32console.el b/lisp/term/w32console.el
index 36e9d896c7..0e9d7c8b5c 100644
--- a/lisp/term/w32console.el
+++ b/lisp/term/w32console.el
@@ -86,9 +86,9 @@ terminal-init-w32console
     (setq r (nth 2 descr)
 	  g (nth 3 descr)
 	  b (nth 4 descr))
-    (if (< (+ r g b) (* .6 (+ 65535 65535 65535)))
-	(setq bg-mode 'dark)
-      (setq bg-mode 'light))
+    (setq bg-mode (if (color-dark-p
+                       (list (/ r 65535.0) (/ g 65535.0) (/ b 65535.0)))
+                      'dark 'light))
     (set-terminal-parameter nil 'background-mode bg-mode))
   (tty-set-up-initial-frame-faces)
   (run-hooks 'terminal-init-w32-hook))
diff --git a/lisp/term/xterm.el b/lisp/term/xterm.el
index 1a727e3933..bf9bcae526 100644
--- a/lisp/term/xterm.el
+++ b/lisp/term/xterm.el
@@ -1120,9 +1120,8 @@ xterm-register-default-colors
     (clear-face-cache)))
 
 (defun xterm-maybe-set-dark-background-mode (redc greenc bluec)
-  ;; Use the heuristic in `frame-set-background-mode' to decide if a
-  ;; frame is dark.
-  (when (< (+ redc greenc bluec) (* .6 (+ 65535 65535 65535)))
+  (when (color-dark-p (mapcar (lambda (c) (/ c 65535.0))
+                              (list redc greenc bluec)))
     (set-terminal-parameter nil 'background-mode 'dark)
     t))
 
diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 0035c5e7b0..2cd99787e8 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -1149,17 +1149,6 @@ css--compute-color
    ;; Evaluate to the color if the name is found.
    ((css--named-color start-point match))))
 
-(defun css--contrasty-color (name)
-  "Return a color that contrasts with NAME.
-NAME is of any form accepted by `color-distance'.
-The returned color will be usable by Emacs and will contrast
-with NAME; in particular so that if NAME is used as a background
-color, the returned color can be used as the foreground and still
-be readable."
-  ;; See bug#25525 for a discussion of this.
-  (if (> (color-distance name "black") 292485)
-      "black" "white"))
-
 (defcustom css-fontify-colors t
   "Whether CSS colors should be fontified using the color as the background.
 When non-`nil', a text representing CSS color will be fontified
@@ -1199,7 +1188,8 @@ css--fontify-region
 		    (add-text-properties
 		     start (point)
 		     (list 'face (list :background color
-				       :foreground (css--contrasty-color color)
+				       :foreground (readable-foreground-color
+                                                    color)
 				       :box '(:line-width -1))))))))))))
     extended-region))
 
-- 
2.21.1 (Apple Git-122.3)

bug#41544: 26.3; Possible incorrect results from color-distance

[Drew Adams]

(Not following this thread; apologies.)

Noticed this in the patch:

+(defun color-dark-p (rgb)
+  "Whether RGB is more readable against white than black.
+RGB is a 3-element list (R G B), each component in the range [0,1]."

Something seems a bit wrong, here.  Dunno whether
it's the predicate name or the doc string (or both).

The predicate name suggests it's about testing a
color (via RGB) to determine whether it's dark or
light.

The doc string suggests it's specifically about
the readability of _foreground_ text that is of
that color - specifically whether it's more
readable against a white than a black background.

Maybe the predicate name should be changed to
indicate (1) that it's about RGB as a foreground
and (2) it's compared (in terms of contrast or
value or whatever) to a white and black background.

Just a suggestion.  Discovery, based on the
predicate name, will mislead as it is now, I think.

I also think the doc string could say just what it
means by "more readable", i.e. what kind of RGB (or
other) test it uses.

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

6 juni 2020 kl. 20.15 skrev Drew Adams <drew.adams@oracle.com>:

> +(defun color-dark-p (rgb)
> +  "Whether RGB is more readable against white than black.
> +RGB is a 3-element list (R G B), each component in the range [0,1]."

> The predicate name suggests it's about testing a
> color (via RGB) to determine whether it's dark or
> light.
> 
> The doc string suggests it's specifically about
> the readability of _foreground_ text that is of
> that color - specifically whether it's more
> readable against a white than a black background.

Thank you, this actually raises a good point.

The predicate should work with the argument both as a foreground and as a background colour, for selecting a black or white contrasting colour. The assumption is that the same predicate can be used for both, which may be wrong, but absent evidence to the contrary, I think it is a reasonable one to make.

If you are in doubt, see if you can come up with a colour for which it does not hold. For example, if you find a rare shade of beige that when used for text looks better against a white background, but when used as a background prefers black text. I have yet to do so, much less been able to articulate it formally as an algorithm.

I agree that this could be stated more explicitly in the doc string.

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sun, 7 Jun 2020 11:13:01 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, 41544@debbugs.gnu.org
> 
> The predicate should work with the argument both as a foreground and as a background colour, for selecting a black or white contrasting colour. The assumption is that the same predicate can be used for both, which may be wrong, but absent evidence to the contrary, I think it is a reasonable one to make.

IME, it is unreasonable to assume that a pair of colors will be
perceived as "contrasting enough" no matter which of them is
foreground and which background.  Background colors affect larger
portions of display, and therefore a bright background is perceived as
brighter and a dark background as darker, than when the same color is
used as foreground.

bug#41544: 26.3; Possible incorrect results from color-distance

[Drew Adams]

> IME, it is unreasonable to assume that a pair of colors will be
> perceived as "contrasting enough" no matter which of them is
> foreground and which background.  Background colors affect larger
> portions of display, and therefore a bright background is perceived as
> brighter and a dark background as darker, than when the same color is
> used as foreground.

I agree with this.  And as you said earlier, different
eyes see differently (and different devices display
differently).

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

7 juni 2020 kl. 16.30 skrev Eli Zaretskii <eliz@gnu.org>:

>  Background colors affect larger
> portions of display, and therefore a bright background is perceived as
> brighter and a dark background as darker, than when the same color is
> used as foreground.

Yes, but we should ask what that means in terms of a predicate to select between them. Of all comparisons I've done, the latest color-dark-p predicate appears to work fairly well for selecting backgrounds, in the senses that it's never bad.

You are quite right that the optimal predicate may be different, and I'm not at all opposed to having two separate functions. They may be identical initially, but could diverge as we learn more.

Experimentally, it looks like the case of selecting a black/white background is more forgiving: the set of 'ambiguous' colours that contrast well with both black and white greater than when selecting a black/white text.

If you could come up with a colour that strongly prefers a black background but a white foreground, or vice versa, then that would provide us with more insight. If not, it might indicate that those colours are rare.

bug#41544: 26.3; Possible incorrect results from color-distance

[Drew Adams]

> > +(defun color-dark-p (rgb)
> > +  "Whether RGB is more readable against white than black.
> > +RGB is a 3-element list (R G B), each component in the range [0,1]."
> 
> > The predicate name suggests it's about testing a
> > color (via RGB) to determine whether it's dark or
> > light.
> >
> > The doc string suggests it's specifically about
> > the readability of _foreground_ text that is of
> > that color - specifically whether it's more
> > readable against a white than a black background.
> 
> Thank you, this actually raises a good point.
> 
> The predicate should work with the argument both as a foreground and as a
> background colour, for selecting a black or white contrasting colour. The
> assumption is that the same predicate can be used for both, which may be
> wrong, but absent evidence to the contrary, I think it is a reasonable one to
> make.
> 
> If you are in doubt, see if you can come up with a colour for which it does
> not hold. For example, if you find a rare shade of beige that when used for
> text looks better against a white background, but when used as a background
> prefers black text. I have yet to do so, much less been able to articulate it
> formally as an algorithm.
> 
> I agree that this could be stated more explicitly in the doc string.

1. Please do consider stating the behavior more explicitly
in the doc.

2. I don't have any special knowledge or suggestion about
whether the same criteria should be used for light and dark
foreground/background.  I'd think that the comparison would
need to use the complement of the foreground or background,
a priori.  E.g. if a background is 90% light then what
works as a "readable" dark foreground would lead to a light
foreground that is more or less similarly "readable" when
against a 90% dark background.
___

In my own work, when I supply a default foreground or
background for a face, I typically do this:

I start with a light background (my own setup uses LightBlue
- somewhat light), and I pick a color that seems reasonable
enough for the foreground default - by eyeball.  Then I
check it against the default (emacs -Q) background - again,
by eyeball.

Once I've picked a default foreground color for a light
`background-mode', I take its complement (using `hexrgb.el')
as the default foreground for a dark background.  I check
that with emacs -Q.  (I use a light background in my setup,
and I don't spend a lot of energy trying to get a great
default for a dark background.)

In my experience (feedback from users), complementing works
pretty well.  And since, in my setup, I use a background
that's only somewhat light, it gives a pretty good idea of
what works (according to my eyes) for a complemented
(hence somewhat dark) background.

No idea whether any of this info helps you, but it's
what I do.  I don't use color-distance in this endeavor.

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 6 Jun 2020 18:54:41 +0200
> Cc: 41544@debbugs.gnu.org
> 
> Since we are making little progress, let's leave color-name-to-rgb unchanged for the moment. We can both change our minds later. It's not strictly required for the introduction and use of color-dark-p; patch updated.

What practical problem is being solved here?  (Please don't say "this
was done in different ways", that's not a practical problem from my
POV.)

> * lisp/facemenu.el (list-colors-print): Use readable-foreground-color.

I don't mind installing this part, but have the rationale should be
spelled out in the log message.

> --- a/lisp/faces.el
> +++ b/lisp/faces.el
> @@ -1786,15 +1786,24 @@ defined-colors-with-face-attributes
>  
>  (defun readable-foreground-color (color)
>    "Return a readable foreground color for background COLOR."
> -  (let* ((rgb   (color-values color))
> -	 (max   (apply #'max rgb))
> -	 (black (car (color-values "black")))
> -	 (white (car (color-values "white"))))
> -    ;; Select black or white depending on which one is less similar to
> -    ;; the brightest component.
> -    (if (> (abs (- max black)) (abs (- max white)))
> -	"black"
> -      "white")))

What was wrong with the original code?  If it produced sub-optimal
results, please show an example of that.

> +(defun color-dark-p (rgb)
> +  "Whether RGB is more readable against white than black.
> +RGB is a 3-element list (R G B), each component in the range [0,1]."
> +  (let* ((sr (nth 0 rgb))
> +         (sg (nth 1 rgb))
> +         (sb (nth 2 rgb))
> +         ;; Use the power 2.2 as an approximation to sRGB gamma;
> +         ;; it should be good enough for the purpose of this function.
> +         (r (expt sr 2.2))
> +         (g (expt sg 2.2))
> +         (b (expt sb 2.2)))
> +    (unless (<= 0 (min r g b) (max r g b) 1)
> +      (error "RGB components %S not in [0,1]" rgb))
> +    ;; The cut-off value was determined experimentally; see bug#41544.
> +    (< (+ (* r 0.299) (* g 0.587) (* b 0.114))
> +       (eval-when-compile (expt 0.6 2.2)))))

This code's algorithm and rationale should be explained in the
comments before we can discuss whether it's an improvement and why.

> diff --git a/lisp/frame.el b/lisp/frame.el
> index 6c2f774709..253528da75 100644
> --- a/lisp/frame.el
> +++ b/lisp/frame.el
> @@ -1156,6 +1156,13 @@ frame-background-mode

This is a non-starter, sorry.  I'm not interested in changing what is
considered dark and light background of a frame.

> diff --git a/lisp/term/rxvt.el b/lisp/term/rxvt.el
> index 31e3d6ede4..5dc754c8e0 100644
> --- a/lisp/term/rxvt.el
> +++ b/lisp/term/rxvt.el
> @@ -206,13 +206,11 @@ rxvt-set-background-mode

Likewise here.

> diff --git a/lisp/term/w32console.el b/lisp/term/w32console.el
> index 36e9d896c7..0e9d7c8b5c 100644
> --- a/lisp/term/w32console.el
> +++ b/lisp/term/w32console.el
> @@ -86,9 +86,9 @@ terminal-init-w32console
>      (setq r (nth 2 descr)
>  	  g (nth 3 descr)
>  	  b (nth 4 descr))
> -    (if (< (+ r g b) (* .6 (+ 65535 65535 65535)))
> -	(setq bg-mode 'dark)
> -      (setq bg-mode 'light))
> +    (setq bg-mode (if (color-dark-p
> +                       (list (/ r 65535.0) (/ g 65535.0) (/ b 65535.0)))
> +                      'dark 'light))
>      (set-terminal-parameter nil 'background-mode bg-mode))

And here.

> diff --git a/lisp/term/xterm.el b/lisp/term/xterm.el
> index 1a727e3933..bf9bcae526 100644
> --- a/lisp/term/xterm.el
> +++ b/lisp/term/xterm.el
> @@ -1120,9 +1120,8 @@ xterm-register-default-colors
>      (clear-face-cache)))
>  
>  (defun xterm-maybe-set-dark-background-mode (redc greenc bluec)
> -  ;; Use the heuristic in `frame-set-background-mode' to decide if a
> -  ;; frame is dark.
> -  (when (< (+ redc greenc bluec) (* .6 (+ 65535 65535 65535)))
> +  (when (color-dark-p (mapcar (lambda (c) (/ c 65535.0))
> +                              (list redc greenc bluec)))
>      (set-terminal-parameter nil 'background-mode 'dark)

And here.

> -(defun css--contrasty-color (name)
> -  "Return a color that contrasts with NAME.
> -NAME is of any form accepted by `color-distance'.
> -The returned color will be usable by Emacs and will contrast
> -with NAME; in particular so that if NAME is used as a background
> -color, the returned color can be used as the foreground and still
> -be readable."
> -  ;; See bug#25525 for a discussion of this.
> -  (if (> (color-distance name "black") 292485)
> -      "black" "white"))
> -
>  (defcustom css-fontify-colors t
>    "Whether CSS colors should be fontified using the color as the background.
>  When non-`nil', a text representing CSS color will be fontified
> @@ -1199,7 +1188,8 @@ css--fontify-region
>  		    (add-text-properties
>  		     start (point)
>  		     (list 'face (list :background color
> -				       :foreground (css--contrasty-color color)
> +				       :foreground (readable-foreground-color
> +                                                    color)
>  				       :box '(:line-width -1))))))))))))
>      extended-region))

Here, once again I will ask what practical problem is being fixed.

Thanks.

bug#41544: 26.3; Possible incorrect results from color-distance

[Simen Heggestøyl]

Eli Zaretskii <eliz@gnu.org> writes:

>> -(defun css--contrasty-color (name)
>> -  "Return a color that contrasts with NAME.
>> -NAME is of any form accepted by `color-distance'.
>> -The returned color will be usable by Emacs and will contrast
>> -with NAME; in particular so that if NAME is used as a background
>> -color, the returned color can be used as the foreground and still
>> -be readable."
>> -  ;; See bug#25525 for a discussion of this.
>> -  (if (> (color-distance name "black") 292485)
>> -      "black" "white"))
>> -
>>  (defcustom css-fontify-colors t
>>    "Whether CSS colors should be fontified using the color as the background.
>>  When non-`nil', a text representing CSS color will be fontified
>> @@ -1199,7 +1188,8 @@ css--fontify-region
>>  		    (add-text-properties
>>  		     start (point)
>>  		     (list 'face (list :background color
>> -				       :foreground (css--contrasty-color color)
>> +				       :foreground (readable-foreground-color
>> +                                                    color)
>>  				       :box '(:line-width -1))))))))))))
>>      extended-region))
>
> Here, once again I will ask what practical problem is being fixed.

I can't comment on the patch overall, but this part at least seems to
address Richard Copley's complaints in bug#30295. A dark foreground is
now used for #0f0/rgba(0,255,0,0.5) and #5e5 as Richard requested, which
indeed looks more readable to me too.

Maybe the pendulum has swung too far however. For instance, a dark
foreground is now used for #ef716e, which I think was easier to read
with the light foreground used before. Could that be fixed by tweaking
the cut-off values in color-dark-p, perhaps?

-- Simen

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

7 juni 2020 kl. 11.04 skrev Simen Heggestøyl <simenheg@runbox.com>:

> I can't comment on the patch overall, but this part at least seems to
> address Richard Copley's complaints in bug#30295. A dark foreground is
> now used for #0f0/rgba(0,255,0,0.5) and #5e5 as Richard requested, which
> indeed looks more readable to me too.

Thank you, and yes, this was actually one of the motivations.

> Maybe the pendulum has swung too far however. For instance, a dark
> foreground is now used for #ef716e, which I think was easier to read
> with the light foreground used before. Could that be fixed by tweaking
> the cut-off values in color-dark-p, perhaps?

Right; the current predicate should be a significant overall improvement but there are inevitably some colours where the decision isn't quite right. I'll see what can be done about it. It seems to be mainly those somewhat reddish colours that need some work.
Perhaps you could try list-colours-display and see if you can spot a pattern?

I'll be back with a longer reply on the principles later.

bug#41544: 26.3; Possible incorrect results from color-distance

[Simen Heggestøyl]

Mattias Engdegård <mattiase@acm.org> writes:

> Perhaps you could try list-colours-display and see if you can spot a
> pattern?

Hm, that one seems to use a black foreground color for all of the
entries(?).

-- Simen

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

7 juni 2020 kl. 21.23 skrev Simen Heggestøyl <simenheg@runbox.com>:

>> Perhaps you could try list-colours-display and see if you can spot a
>> pattern?
> 
> Hm, that one seems to use a black foreground color for all of the
> entries(?).

Yes, but with the patch posted, it should be using color-dark-p. Please give it a go.

(An updated patch as well as a longer discussion of the principles is upcoming; a bit busy with other things at the moment.)

bug#41544: 26.3; Possible incorrect results from color-distance

[Simen Heggestøyl]

Mattias Engdegård <mattiase@acm.org> writes:

> 7 juni 2020 kl. 21.23 skrev Simen Heggestøyl <simenheg@runbox.com>:
>
>>> Perhaps you could try list-colours-display and see if you can spot a
>>> pattern?
>>
>> Hm, that one seems to use a black foreground color for all of the
>> entries(?).
>
> Yes, but with the patch posted, it should be using color-dark-p. Please give it a go.

Ah. With my screen and pair of eyes, every entry looks readable
there. The worst are, like you said, those darker reddish colors, like
orange3 and LightSalmon3.

-- Simen

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Simen Heggestøyl <simenheg@runbox.com>
> Cc: Mattias Engdegård <mattiase@acm.org>,  Richard Copley
>  <rcopley@gmail.com>, 41544@debbugs.gnu.org
> Date: Sun, 07 Jun 2020 11:04:30 +0200
> 
> I can't comment on the patch overall, but this part at least seems to
> address Richard Copley's complaints in bug#30295. A dark foreground is
> now used for #0f0/rgba(0,255,0,0.5) and #5e5 as Richard requested, which
> indeed looks more readable to me too.
> 
> Maybe the pendulum has swung too far however. For instance, a dark
> foreground is now used for #ef716e, which I think was easier to read
> with the light foreground used before. Could that be fixed by tweaking
> the cut-off values in color-dark-p, perhaps?

We can (and did) tweak the various constants involved in this, more
than once.  The lesson I took from that is that we could never produce
something that will fit all the needs, let alone satisfy all the
users.  There are two main factors here on which we have no control:
the subjective differences between color perception by different
people, and variations in how different terminals and displays show
the same colors.

That is why I object to making changes in this low-level functionality
with the motivation of "fixing them all": I think it's simply
impossible.  We will make it slightly better in some situations and
slightly worse in others.  There's no net win here.

bug#41544: 26.3; Possible incorrect results from color-distance

[Drew Adams]

> There are two main factors here on which we have no control:
> the subjective differences between color perception by different
> people, and variations in how different terminals and displays show
> the same colors.

+1.

bug#41544: 26.3; Possible incorrect results from color-distance

[Simen Heggestøyl]

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> We can (and did) tweak the various constants involved in this, more
> than once.  The lesson I took from that is that we could never produce
> something that will fit all the needs, let alone satisfy all the
> users.  There are two main factors here on which we have no control:
> the subjective differences between color perception by different
> people, and variations in how different terminals and displays show
> the same colors.
>
> That is why I object to making changes in this low-level functionality
> with the motivation of "fixing them all": I think it's simply
> impossible.  We will make it slightly better in some situations and
> slightly worse in others.  There's no net win here.

Maybe the ultimate fix in CSS mode's case could be to display the color
separate from the text. Firefox, for instance, displays the color in a
little circle next to the color code (screenshot attached). I'm not sure
how to implement it in Emacs, though.

-- Simen


[-- Attachment #2: firefox-color-indicator.png --]
[-- Type: image/png, Size: 5558 bytes --]

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 3072 bytes --]

6 juni 2020 kl. 20.27 skrev Eli Zaretskii <eliz@gnu.org>:

> What practical problem is being solved here?

The current predicates for determining whether a colour is light or dark are just bad. We can and should do better, and that's what this is all about.

Let's consider the three saturated colours #ff0000 (red), #00ff00 (green) and #0000ff (blue). Black text looks terrible on blue, as does white on green; black on red isn't good either. This comes as no surprise: the human eye is more sensitive to brightness levels of green than blue, with red somewhere in-between.

(These three colours just serve as illustrative examples; large chunks of the RGB space will share the same properties.)

How do the existing predicates do? Let's call them MAX, AVG and DIST.

MAX: dark iff max(r,g,b) < 0.5

This classifies saturated red and blue as light, which is clearly terrible.

AVG: dark iff (r+g+b) / 3 < 0.6

This classifies saturated green as dark, which is also wrong.

DIST: dark iff (color-distance c "black") ≤ 292485

This one also thinks saturated green is dark. While the approach here looks reasonable at first blush, it's really not: color-distance uses a simplified expression for how dissimilar two colours are, but doesn't do a very good job at determining brightness. For instance, its heavy blue weight makes no sense when used in this way:

 (color-distance "#ff0000" "black") => 162308
 (color-distance "#00ff00" "black") => 260100
 (color-distance "#0000ff" "black") => 194820

This means that we cannot fix DIST by tweaking its cut-off value; it's fundamentally unfit for this purpose.

For a proper solution, we have theory to guide us: determine the perceived brightness of a colour, and classify everything below a cut-off value as dark, others as light. The patch uses a standard expression for relative luminance: Y = 0.2126R + 0.7152G + 0.0722B, where R, G and B are linear colour components. We assume a gamma of 2.2; it is nearly identical to the sRGB gamma curve and the results are almost the same.

With a cut-off of 0.6, this predicate turns out to be quite good: much better than MAX, AVG or DIST at any rate. While not perfect, it's good enough in the sense that for colours where it seems to make the wrong decision, it's a fairly close call, and the colour is quite readable with both black and write as contrast.

I have tested it on several platforms and monitors, including 80x25 VGA hardware text mode, and have yet to find a case where it fails, which definitely cannot be said about the old predicates.

We can still tweak it: mainly the cut-off value (which is just experimentally determined) but also the coefficients and the gamma correction. Although technically valid, they aren't holy.

> This code's algorithm and rationale should be explained in the
> comments before we can discuss whether it's an improvement and why.

Thanks, I have improved this in the new patch.

This patch should also use the right coefficients (see above); I think I got them wrong the first time.


[-- Attachment #2: 0001-Improved-light-dark-colour-predicate-bug-41544.patch --]
[-- Type: application/octet-stream, Size: 10744 bytes --]

From 00c9176cc26853e1314f95bfb540753273740807 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sun, 31 May 2020 21:12:46 +0200
Subject: [PATCH] Improved light/dark colour predicate (bug#41544)

Add a single predicate, color-dark-p, for deciding whether a colour
is more readable against black or white.  Previously this was done in
different ways in several places, and with worse results.

The new predicate compares the relative luminance of the colour to
an experimentally determined cut-off value, and it seems to get it
right in almost all cases, with no value leading to outright bad
results.

* lisp/facemenu.el (list-colors-print): Use readable-foreground-color.
(color-dark-p): New function.
* lisp/term/pc-win.el: Update comment.
* lisp/term/rxvt.el (rxvt-set-background-mode):
* lisp/term/w32console.el (terminal-init-w32console):
* lisp/term/xterm.el (xterm-maybe-set-dark-background-mode):
* lisp/faces.el (readable-foreground-color):
* lisp/frame.el (frame-set-background-mode): Use color-dark-p.
* lisp/textmodes/css-mode.el (css--contrasty-color): Remove.
(css--fontify-region): Use color-dark-p.
---
 lisp/facemenu.el           | 11 +++++------
 lisp/faces.el              | 29 ++++++++++++++++++++---------
 lisp/frame.el              | 16 +++++++++-------
 lisp/term/pc-win.el        |  8 +++-----
 lisp/term/rxvt.el          | 12 +++++-------
 lisp/term/w32console.el    |  6 +++---
 lisp/term/xterm.el         |  5 ++---
 lisp/textmodes/css-mode.el | 14 ++------------
 8 files changed, 49 insertions(+), 52 deletions(-)

diff --git a/lisp/facemenu.el b/lisp/facemenu.el
index b10d874b21..419b76101b 100644
--- a/lisp/facemenu.el
+++ b/lisp/facemenu.el
@@ -621,12 +621,11 @@ list-colors-print
 						 (downcase b))))))
 	(setq color (list color)))
       (let* ((opoint (point))
-	     (color-values (color-values (car color)))
-	     (light-p (>= (apply 'max color-values)
-			  (* (car (color-values "white")) .5))))
+             (fg (readable-foreground-color (car color))))
 	(insert (car color))
 	(indent-to 22)
-	(put-text-property opoint (point) 'face `(:background ,(car color)))
+	(put-text-property opoint (point) 'face `(:background ,(car color)
+                                                  :foreground ,fg))
 	(put-text-property
 	 (prog1 (point)
 	   (insert " ")
@@ -639,7 +638,7 @@ list-colors-print
 	(insert (propertize
 		 (apply 'format "#%02x%02x%02x"
 			(mapcar (lambda (c) (ash c -8))
-				color-values))
+				(color-values (car color))))
 		 'mouse-face 'highlight
 		 'help-echo
 		 (let ((hsv (apply 'color-rgb-to-hsv
@@ -651,7 +650,7 @@ list-colors-print
 	   opoint (point)
 	   'follow-link t
 	   'mouse-face (list :background (car color)
-			     :foreground (if light-p "black" "white"))
+			     :foreground fg)
 	   'color-name (car color)
 	   'action callback-fn)))
       (insert "\n"))
diff --git a/lisp/faces.el b/lisp/faces.el
index f4a9dedd79..d68eb82de6 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1786,15 +1786,26 @@ defined-colors-with-face-attributes
 
 (defun readable-foreground-color (color)
   "Return a readable foreground color for background COLOR."
-  (let* ((rgb   (color-values color))
-	 (max   (apply #'max rgb))
-	 (black (car (color-values "black")))
-	 (white (car (color-values "white"))))
-    ;; Select black or white depending on which one is less similar to
-    ;; the brightest component.
-    (if (> (abs (- max black)) (abs (- max white)))
-	"black"
-      "white")))
+  (if (color-dark-p (color-name-to-rgb color)) "white" "black"))
+
+(defun color-dark-p (rgb)
+  "Whether RGB is more readable against white than black.
+RGB is a 3-element list (R G B), each component in the range [0,1]."
+  (unless (<= 0 (apply #'min rgb) (apply #'max rgb) 1)
+    (error "RGB components %S not in [0,1]" rgb))
+  (let* ((sr (nth 0 rgb))
+         (sg (nth 1 rgb))
+         (sb (nth 2 rgb))
+         ;; Gamma-correct the RGB components to linear values.
+         ;; Use the power 2.2 as an approximation to sRGB gamma;
+         ;; it should be good enough for the purpose of this function.
+         (r (expt sr 2.2))
+         (g (expt sg 2.2))
+         (b (expt sb 2.2))
+         ;; Compute the relative luminance.
+         (y (+ (* r 0.2126) (* g 0.7152) (* b 0.0722))))
+    ;; Compare it to a cut-off value determined experimentally; see bug#41544.
+    (< y (eval-when-compile (expt 0.6 2.2)))))
 
 (declare-function xw-color-defined-p "xfns.c" (color &optional frame))
 
diff --git a/lisp/frame.el b/lisp/frame.el
index 6c2f774709..b46ba3686f 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -1156,6 +1156,12 @@ frame-background-mode
 
 (defvar inhibit-frame-set-background-mode nil)
 
+(defun frame--color-name-to-rgb (color frame)
+  "Convert the COLOR string to a list of normalised RGB components.
+Like `color-name-to-rgb', but works even when the display has not yet
+been initialised."
+  (mapcar (lambda (x) (/ x 65535.0)) (color-values color frame)))
+
 (defun frame-set-background-mode (frame &optional keep-face-specs)
   "Set up display-dependent faces on FRAME.
 Display-dependent faces are those which have different definitions
@@ -1181,13 +1187,9 @@ frame-set-background-mode
 		   non-default-bg-mode)
 		  ((not (color-values bg-color frame))
 		   default-bg-mode)
-		  ((>= (apply '+ (color-values bg-color frame))
-		       ;; Just looking at the screen, colors whose
-		       ;; values add up to .6 of the white total
-		       ;; still look dark to me.
-		       (* (apply '+ (color-values "white" frame)) .6))
-		   'light)
-		  (t 'dark)))
+                  ((color-dark-p (frame--color-name-to-rgb bg-color frame))
+                   'dark)
+                  (t 'light)))
 	   (display-type
 	    (cond ((null (window-system frame))
 		   (if (tty-display-color-p frame) 'color 'mono))
diff --git a/lisp/term/pc-win.el b/lisp/term/pc-win.el
index 76a48a86c7..16eb660f00 100644
--- a/lisp/term/pc-win.el
+++ b/lisp/term/pc-win.el
@@ -54,11 +54,9 @@
 ;; DJGPP-compiled Emacs on the same PC.  The names of X colors used to
 ;; define the pixel values are shown as comments to each color below.
 ;;;
-;; If you want to change the RGB values, keep in mind that various pieces
-;; of Emacs think that a color whose RGB values add up to less than 0.6 of
-;; the values for WHITE (i.e. less than 117963) are ``dark'', otherwise the
-;; color is ``light''; see `frame-set-background-mode' in lisp/faces.el for
-;; an example.
+;; If you want to change the RGB values, consider the heuristics in
+;; `color-dark-p' which is used to select a suitably contrasting
+;; foreground or background colour.
 (defvar msdos-color-values
   '(("black"          0     0     0     0)
     ("blue"           1     0     0 52480) ; MediumBlue
diff --git a/lisp/term/rxvt.el b/lisp/term/rxvt.el
index 31e3d6ede4..5dc754c8e0 100644
--- a/lisp/term/rxvt.el
+++ b/lisp/term/rxvt.el
@@ -206,13 +206,11 @@ rxvt-set-background-mode
       ;; The next line assumes that rxvt-standard-colors are ordered
       ;; by the color index in the ascending order!
       (setq rgb (car (cddr (nth bg rxvt-standard-colors))))
-      ;; See the commentary in frame-set-background-mode about the
-      ;; computation below.
-      (if (< (apply '+ rgb)
-	     ;; The following line assumes that white is the 15th
-	     ;; color in rxvt-standard-colors.
-	     (* (apply '+ (car (cddr (nth 15 rxvt-standard-colors)))) 0.6))
-	  (set-terminal-parameter nil 'background-mode 'dark)))))
+      ;; The following line assumes that white is the 15th
+      ;; color in rxvt-standard-colors.
+      (let ((comp-max (float (caddr (nth 15 rxvt-standard-colors)))))
+        (when (color-dark-p (mapcar (lambda (c) (/ c comp-max)) rgb))
+	  (set-terminal-parameter nil 'background-mode 'dark))))))
 
 (provide 'term/rxvt)
 
diff --git a/lisp/term/w32console.el b/lisp/term/w32console.el
index 36e9d896c7..0e9d7c8b5c 100644
--- a/lisp/term/w32console.el
+++ b/lisp/term/w32console.el
@@ -86,9 +86,9 @@ terminal-init-w32console
     (setq r (nth 2 descr)
 	  g (nth 3 descr)
 	  b (nth 4 descr))
-    (if (< (+ r g b) (* .6 (+ 65535 65535 65535)))
-	(setq bg-mode 'dark)
-      (setq bg-mode 'light))
+    (setq bg-mode (if (color-dark-p
+                       (list (/ r 65535.0) (/ g 65535.0) (/ b 65535.0)))
+                      'dark 'light))
     (set-terminal-parameter nil 'background-mode bg-mode))
   (tty-set-up-initial-frame-faces)
   (run-hooks 'terminal-init-w32-hook))
diff --git a/lisp/term/xterm.el b/lisp/term/xterm.el
index 1a727e3933..bf9bcae526 100644
--- a/lisp/term/xterm.el
+++ b/lisp/term/xterm.el
@@ -1120,9 +1120,8 @@ xterm-register-default-colors
     (clear-face-cache)))
 
 (defun xterm-maybe-set-dark-background-mode (redc greenc bluec)
-  ;; Use the heuristic in `frame-set-background-mode' to decide if a
-  ;; frame is dark.
-  (when (< (+ redc greenc bluec) (* .6 (+ 65535 65535 65535)))
+  (when (color-dark-p (mapcar (lambda (c) (/ c 65535.0))
+                              (list redc greenc bluec)))
     (set-terminal-parameter nil 'background-mode 'dark)
     t))
 
diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 0035c5e7b0..2cd99787e8 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -1149,17 +1149,6 @@ css--compute-color
    ;; Evaluate to the color if the name is found.
    ((css--named-color start-point match))))
 
-(defun css--contrasty-color (name)
-  "Return a color that contrasts with NAME.
-NAME is of any form accepted by `color-distance'.
-The returned color will be usable by Emacs and will contrast
-with NAME; in particular so that if NAME is used as a background
-color, the returned color can be used as the foreground and still
-be readable."
-  ;; See bug#25525 for a discussion of this.
-  (if (> (color-distance name "black") 292485)
-      "black" "white"))
-
 (defcustom css-fontify-colors t
   "Whether CSS colors should be fontified using the color as the background.
 When non-`nil', a text representing CSS color will be fontified
@@ -1199,7 +1188,8 @@ css--fontify-region
 		    (add-text-properties
 		     start (point)
 		     (list 'face (list :background color
-				       :foreground (css--contrasty-color color)
+				       :foreground (readable-foreground-color
+                                                    color)
 				       :box '(:line-width -1))))))))))))
     extended-region))
 
-- 
2.21.1 (Apple Git-122.3)

bug#41544: 26.3; Possible incorrect results from color-distance

[Drew Adams]

Sounds reasonable. Maybe you could attach some
screenshots (pic = 1000 words)?
___

I still have the same comment as before, regarding
the name `color-dark-p' and the doc string, FWIW:

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41544#103

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

8 juni 2020 kl. 16.30 skrev Drew Adams <drew.adams@oracle.com>:

> Sounds reasonable. Maybe you could attach some
> screenshots (pic = 1000 words)?

You could try applying the latest patch, and run list-colors-display. To see other predicates in action, just edit color-dark-p to your heart's desire, and run list-colors-display again, and see which one you think is better.

> I still have the same comment as before, regarding
> the name `color-dark-p' and the doc string

Quite, both naming and documentation can be improved. Those will have to follow the semantics, so that will have to be nailed first.

bug#41544: 26.3; Possible incorrect results from color-distance

[Drew Adams]

> > I still have the same comment as before, regarding
> > the name `color-dark-p' and the doc string
> 
> Quite, both naming and documentation can be improved. Those will have to
> follow the semantics, so that will have to be nailed first.

So this never happened?  The bug was "fixed" without
fixing this mismatch?  Too bad, if so.

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

10 juni 2020 kl. 20.37 skrev Drew Adams <drew.adams@oracle.com>:

> So this never happened?  The bug was "fixed" without
> fixing this mismatch?  Too bad, if so.

I'm not dismissing your concern, but I did amend the doc string prior to pushing to state explicitly that it can be used for either foreground or background colours.

I didn't bother making separate functions since they would probably have been identical anyway, after studying the different cases. Hope that's all right, and if you can argue -- after having used it yourself in your own code -- that there is an important semantic distinction, then the code can be modified accordingly.

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Mon, 8 Jun 2020 15:11:36 +0200
> Cc: 41544@debbugs.gnu.org
> 
> > What practical problem is being solved here?
> 
> The current predicates for determining whether a colour is light or dark are just bad. We can and should do better, and that's what this is all about.
> 
> Let's consider the three saturated colours #ff0000 (red), #00ff00 (green) and #0000ff (blue). Black text looks terrible on blue, as does white on green; black on red isn't good either. This comes as no surprise: the human eye is more sensitive to brightness levels of green than blue, with red somewhere in-between.

Here we already not necessarily agree: at least on some text-mode
terminals some of the above combinations look quite legible to me.

Like I said: individual taste and differences, as well as different
RGB values used by some terminals for the same color names, can and do
wreak havoc on this, so a perfect solution is simply not possible.

>  (color-distance "#ff0000" "black") => 162308
>  (color-distance "#00ff00" "black") => 260100
>  (color-distance "#0000ff" "black") => 194820
> 
> This means that we cannot fix DIST by tweaking its cut-off value; it's fundamentally unfit for this purpose.
> 
> For a proper solution, we have theory to guide us: determine the perceived brightness of a colour, and classify everything below a cut-off value as dark, others as light. The patch uses a standard expression for relative luminance: Y = 0.2126R + 0.7152G + 0.0722B, where R, G and B are linear colour components. We assume a gamma of 2.2; it is nearly identical to the sRGB gamma curve and the results are almost the same.
> 
> With a cut-off of 0.6, this predicate turns out to be quite good: much better than MAX, AVG or DIST at any rate. While not perfect, it's good enough in the sense that for colours where it seems to make the wrong decision, it's a fairly close call, and the colour is quite readable with both black and write as contrast.

Again, I see no practical problems described here, just a theoretical
issue with the particular implementations we have now.  Those
implementations do their job, although they are clearly not perfect.
However, I seed no reason to seek perfection in this case.

> diff --git a/lisp/facemenu.el b/lisp/facemenu.el
> index b10d874b21..419b76101b 100644
> --- a/lisp/facemenu.el
> +++ b/lisp/facemenu.el
> @@ -621,12 +621,11 @@ list-colors-print

I don't think this change is very important, but I don't object to
installing it, since it only affects this single command, whose
purpose is just to display the list of colors.

> +(defun color-dark-p (rgb)
> +  "Whether RGB is more readable against white than black.
> +RGB is a 3-element list (R G B), each component in the range [0,1]."
> +  (unless (<= 0 (apply #'min rgb) (apply #'max rgb) 1)
> +    (error "RGB components %S not in [0,1]" rgb))
> +  (let* ((sr (nth 0 rgb))
> +         (sg (nth 1 rgb))
> +         (sb (nth 2 rgb))
> +         ;; Gamma-correct the RGB components to linear values.
> +         ;; Use the power 2.2 as an approximation to sRGB gamma;
> +         ;; it should be good enough for the purpose of this function.
> +         (r (expt sr 2.2))
> +         (g (expt sg 2.2))
> +         (b (expt sb 2.2))
> +         ;; Compute the relative luminance.
> +         (y (+ (* r 0.2126) (* g 0.7152) (* b 0.0722))))
> +    ;; Compare it to a cut-off value determined experimentally; see bug#41544.
> +    (< y (eval-when-compile (expt 0.6 2.2)))))

IMO, the commentary here doesn't explain enough, and actually begs
more questions than it answers.  What is "gamma-correction", and why
is it pertinent here?  Why is the power 2.2 a "good enough"
approximation here?  What are the other constants, and what is the
meaning of each one of them?  And pointing to the bug number for
rationale of the cut-off value doesn't really help, since the
discussion is very long, so I doubt people will easily find that
rationale.

If these questions cannot be reasonably answered in the comments, how
about pointing to some article or URL where interested readers could
read up on that?

> +(defun frame--color-name-to-rgb (color frame)
> +  "Convert the COLOR string to a list of normalised RGB components.
> +Like `color-name-to-rgb', but works even when the display has not yet
> +been initialised."
> +  (mapcar (lambda (x) (/ x 65535.0)) (color-values color frame)))

I still don't understand why we need this function.  Did you see any
practical problems with using color-name-to-rgb?  Why does it matter
that it needs the display to be initialized?  Would it be enough to
document that it needs the display to be initialized?

>  (defun frame-set-background-mode (frame &optional keep-face-specs)
>    "Set up display-dependent faces on FRAME.
>  Display-dependent faces are those which have different definitions
> @@ -1181,13 +1187,9 @@ frame-set-background-mode
>  		   non-default-bg-mode)
>  		  ((not (color-values bg-color frame))
>  		   default-bg-mode)
> -		  ((>= (apply '+ (color-values bg-color frame))
> -		       ;; Just looking at the screen, colors whose
> -		       ;; values add up to .6 of the white total
> -		       ;; still look dark to me.
> -		       (* (apply '+ (color-values "white" frame)) .6))
> -		   'light)
> -		  (t 'dark)))
> +                  ((color-dark-p (frame--color-name-to-rgb bg-color frame))
> +                   'dark)
> +                  (t 'light)))

As I said before, I don't want to change the default value of
frame-background-mode.  This code has been relatively stable for quite
some time, and the result is customizable if the user doesn't like the
default.  Changing the default value in subtle ways simply risks
annoying users.  There's nothing to gain here, only potential losses.

Same comment for calculation of background-mode frame parameter in the
various lisp/term/*.el files.  I don't want to make those changes.

> --- a/lisp/textmodes/css-mode.el
> +++ b/lisp/textmodes/css-mode.el
> @@ -1149,17 +1149,6 @@ css--compute-color
>     ;; Evaluate to the color if the name is found.
>     ((css--named-color start-point match))))
>  
> -(defun css--contrasty-color (name)
> -  "Return a color that contrasts with NAME.
> -NAME is of any form accepted by `color-distance'.
> -The returned color will be usable by Emacs and will contrast
> -with NAME; in particular so that if NAME is used as a background
> -color, the returned color can be used as the foreground and still
> -be readable."
> -  ;; See bug#25525 for a discussion of this.
> -  (if (> (color-distance name "black") 292485)
> -      "black" "white"))
> -
>  (defcustom css-fontify-colors t
>    "Whether CSS colors should be fontified using the color as the background.
>  When non-`nil', a text representing CSS color will be fontified
> @@ -1199,7 +1188,8 @@ css--fontify-region
>  		    (add-text-properties
>  		     start (point)
>  		     (list 'face (list :background color
> -				       :foreground (css--contrasty-color color)
> +				       :foreground (readable-foreground-color
> +                                                    color)
>  				       :box '(:line-width -1))))))))))))
>      extended-region))

If Tom is okay with this change, I won't object.

Note that AFAIR CSS (and HTML in general) uses 24 BPP colors, whereas
your color-dark-p is AFAICT based on 16-bit color values, not 8-bit.
ISTR there were issues with converting between these two
representations, something to do with whether an 8-bit component
should be converted to a 16-bit component by zero-extending it or by a
left shift (i.e. whether #ff should be mapped to #00ff or to #ff00).
Apologies if I'm confused here.

Thanks.

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 2584 bytes --]

9 juni 2020 kl. 18.20 skrev Eli Zaretskii <eliz@gnu.org>:

>> Let's consider the three saturated colours #ff0000 (red), #00ff00 (green) and #0000ff (blue). Black text looks terrible on blue, as does white on green; black on red isn't good either. This comes as no surprise: the human eye is more sensitive to brightness levels of green than blue, with red somewhere in-between.
> 
> Here we already not necessarily agree: at least on some text-mode
> terminals some of the above combinations look quite legible to me.

Of course there will be some terminals where some of the combinations are legible. But that wasn't the point, the point being that they are less legible that the alternative, and on most terminals substantially so.

> Like I said: individual taste and differences, as well as different
> RGB values used by some terminals for the same color names, can and do
> wreak havoc on this, so a perfect solution is simply not possible.

Nowhere did I claim perfection; let's tuck away the straw men. However, I do think we should strive to do as well as we can, and I think I'm not alone. This is not a matter of individual taste: colour perception is a science.

It is true that Emacs sometimes doesn't know the exact colours used by the terminal, but that is a problem that the old code suffers from as well. Maybe the old predicates are more robust for bad input? Unfortunately, there is no evidence of this at all; I've experimented with many terminals and settings. See further below for a counter-example.

> Again, I see no practical problems described here, just a theoretical
> issue with the particular implementations we have now.  Those
> implementations do their job, although they are clearly not perfect.
> However, I seed no reason to seek perfection in this case.

Again, it is not a matter of perfection but about being better. Or more critically, avoiding being bad. The new predicate almost never gives bad results; the old ones often do.

If you want to argue for the old code, please come up with precise examples of colours for which they avoid a bad decision while colour-dark-p does not. You could also show how the old predicates are better than the new one in a majority of cases. Describe the circumstances (environment, configuration etc) so that they can be reproduced.

Meanwhile, here are two screenshots of Emacs displaying some lisp code, both using XTerm version 328, configured for 256 colours, with TERM=xterm-256color, and the same background, without any Emacs customisation.

First, without the patch:


[-- Attachment #2: without-patch.png --]
[-- Type: image/png, Size: 32146 bytes --]

[-- Attachment #3: Type: text/plain, Size: 25 bytes --]



Next, with the patch:


[-- Attachment #4: with-patch.png --]
[-- Type: image/png, Size: 33507 bytes --]

[-- Attachment #5: Type: text/plain, Size: 2430 bytes --]



This particular background is taken from the XTerm's 256-colour palette used by Emacs, and there are many more in that set exhibiting similar problems. Obviously, with 24-bit colour terminals, there is a large number of colours that cause trouble for the old heuristics.

> IMO, the commentary here doesn't explain enough, and actually begs
> more questions than it answers.  What is "gamma-correction", and why
> is it pertinent here?  Why is the power 2.2 a "good enough"
> approximation here?  What are the other constants, and what is the
> meaning of each one of them?  And pointing to the bug number for
> rationale of the cut-off value doesn't really help, since the
> discussion is very long, so I doubt people will easily find that
> rationale.

Fair enough -- I thought that the programmers who don't already know the theory would immediately look it up, but I've added a link to Wikipedia.

> I still don't understand why we need this function.  Did you see any
> practical problems with using color-name-to-rgb?  Why does it matter
> that it needs the display to be initialized?  Would it be enough to
> document that it needs the display to be initialized?

If we use color-name-to-rgb then we get a crash on start-up with TERM=xterm (for example), as explained before. I agree it's a somewhat artificial function; I've eliminated it in the attached patch.

> As I said before, I don't want to change the default value of
> frame-background-mode.  This code has been relatively stable for quite
> some time, and the result is customizable if the user doesn't like the
> default.  Changing the default value in subtle ways simply risks
> annoying users.  There's nothing to gain here, only potential losses.

Quite the contrary: the new predicate is more robust than the old one, which I have argued with both concrete examples and theory. If you disagree, please supply both: why the AVG predicate is better, and specifically for what colours.
There is nothing to lose here, only potential gains.

Here is another screenshot: it compares the three old predicates with the new one for all colours in an Xterm with TERM=xterm-16color. The left-hand columns show the contrasting decision with each colour as background, the right-hand columns with the same colour as foreground. max, avg and dist refer to the old predicates as per previous message; new is color-dark-p of the patch.


[-- Attachment #6: xterm-16color-predicate-comparison.png --]
[-- Type: image/png, Size: 28060 bytes --]

[-- Attachment #7: Type: text/plain, Size: 119 bytes --]



Here is the same comparison on a terminal configured with the exact colours of a Linux VGA console, TERM=linux:


[-- Attachment #8: linux-console-predicate-comparison.png --]
[-- Type: image/png, Size: 78421 bytes --]

[-- Attachment #9: Type: text/plain, Size: 684 bytes --]



Note how the Emacs's RGB values are far off (especially the brownish shade that it thinks is bright yellow), yet the new predicate does better than all the old ones here.

> Note that AFAIR CSS (and HTML in general) uses 24 BPP colors, whereas
> your color-dark-p is AFAICT based on 16-bit color values, not 8-bit.

No, CSS and Emacs normalise hex colours to the number of digits used, so that #123, #112233 and #111122223333 all represent the same colour. (The Windows-specific colour parser has a bug, as mentioned recently on emacs-devel; I intend to fix that since other colour parsers are buggy in other ways. This is completely unrelated to the current discussion.)


[-- Attachment #10: 0001-Improved-light-dark-colour-predicate-bug-41544.patch --]
[-- Type: application/octet-stream, Size: 10438 bytes --]

From 18e4b6cfa9cebfa70c1cebf52e6c2c9622b86c29 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Sun, 31 May 2020 21:12:46 +0200
Subject: [PATCH] Improved light/dark colour predicate (bug#41544)

Add a single predicate, color-dark-p, for deciding whether a colour
is more readable against black or white.  Previously this was done in
different ways in several places, and with worse results.

The new predicate compares the relative luminance of the colour to
an experimentally determined cut-off value, and it seems to get it
right in almost all cases, with no value leading to outright bad
results.

* lisp/facemenu.el (list-colors-print): Use readable-foreground-color.
(color-dark-p): New function.
* lisp/term/pc-win.el: Update comment.
* lisp/term/rxvt.el (rxvt-set-background-mode):
* lisp/term/w32console.el (terminal-init-w32console):
* lisp/term/xterm.el (xterm-maybe-set-dark-background-mode):
* lisp/faces.el (readable-foreground-color):
* lisp/frame.el (frame-set-background-mode): Use color-dark-p.
* lisp/textmodes/css-mode.el (css--contrasty-color): Remove.
(css--fontify-region): Use color-dark-p.
---
 lisp/facemenu.el           | 11 +++++------
 lisp/faces.el              | 33 ++++++++++++++++++++++++---------
 lisp/frame.el              | 10 +++-------
 lisp/term/pc-win.el        |  8 +++-----
 lisp/term/rxvt.el          | 12 +++++-------
 lisp/term/w32console.el    |  6 +++---
 lisp/term/xterm.el         |  5 ++---
 lisp/textmodes/css-mode.el | 14 ++------------
 8 files changed, 47 insertions(+), 52 deletions(-)

diff --git a/lisp/facemenu.el b/lisp/facemenu.el
index b10d874b21..419b76101b 100644
--- a/lisp/facemenu.el
+++ b/lisp/facemenu.el
@@ -621,12 +621,11 @@ list-colors-print
 						 (downcase b))))))
 	(setq color (list color)))
       (let* ((opoint (point))
-	     (color-values (color-values (car color)))
-	     (light-p (>= (apply 'max color-values)
-			  (* (car (color-values "white")) .5))))
+             (fg (readable-foreground-color (car color))))
 	(insert (car color))
 	(indent-to 22)
-	(put-text-property opoint (point) 'face `(:background ,(car color)))
+	(put-text-property opoint (point) 'face `(:background ,(car color)
+                                                  :foreground ,fg))
 	(put-text-property
 	 (prog1 (point)
 	   (insert " ")
@@ -639,7 +638,7 @@ list-colors-print
 	(insert (propertize
 		 (apply 'format "#%02x%02x%02x"
 			(mapcar (lambda (c) (ash c -8))
-				color-values))
+				(color-values (car color))))
 		 'mouse-face 'highlight
 		 'help-echo
 		 (let ((hsv (apply 'color-rgb-to-hsv
@@ -651,7 +650,7 @@ list-colors-print
 	   opoint (point)
 	   'follow-link t
 	   'mouse-face (list :background (car color)
-			     :foreground (if light-p "black" "white"))
+			     :foreground fg)
 	   'color-name (car color)
 	   'action callback-fn)))
       (insert "\n"))
diff --git a/lisp/faces.el b/lisp/faces.el
index f4a9dedd79..fdc2653ba9 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -1786,15 +1786,30 @@ defined-colors-with-face-attributes
 
 (defun readable-foreground-color (color)
   "Return a readable foreground color for background COLOR."
-  (let* ((rgb   (color-values color))
-	 (max   (apply #'max rgb))
-	 (black (car (color-values "black")))
-	 (white (car (color-values "white"))))
-    ;; Select black or white depending on which one is less similar to
-    ;; the brightest component.
-    (if (> (abs (- max black)) (abs (- max white)))
-	"black"
-      "white")))
+  ;; We use #ffffff instead of "white", because the latter is sometimes
+  ;; less than white.  That way, we get the best contrast possible.
+  (if (color-dark-p (mapcar (lambda (c) (/ c 65535.0)) color))
+      "#ffffff" "black"))
+
+(defun color-dark-p (rgb)
+  "Whether RGB is more readable against white than black.
+RGB is a 3-element list (R G B), each component in the range [0,1]."
+  (unless (<= 0 (apply #'min rgb) (apply #'max rgb) 1)
+    (error "RGB components %S not in [0,1]" rgb))
+  ;; Compute the relative luminance after gamma-correcting (assuming sRGB),
+  ;; and compare to a cut-off value determined experimentally.
+  ;; See https://en.wikipedia.org/wiki/Relative_luminance for details.
+  (let* ((sr (nth 0 rgb))
+         (sg (nth 1 rgb))
+         (sb (nth 2 rgb))
+         ;; Gamma-correct the RGB components to linear values.
+         ;; Use the power 2.2 as an approximation to sRGB gamma;
+         ;; it should be good enough for the purpose of this function.
+         (r (expt sr 2.2))
+         (g (expt sg 2.2))
+         (b (expt sb 2.2))
+         (y (+ (* r 0.2126) (* g 0.7152) (* b 0.0722))))
+    (< y (eval-when-compile (expt 0.6 2.2)))))
 
 (declare-function xw-color-defined-p "xfns.c" (color &optional frame))
 
diff --git a/lisp/frame.el b/lisp/frame.el
index 6c2f774709..d05fbe3152 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -1181,13 +1181,9 @@ frame-set-background-mode
 		   non-default-bg-mode)
 		  ((not (color-values bg-color frame))
 		   default-bg-mode)
-		  ((>= (apply '+ (color-values bg-color frame))
-		       ;; Just looking at the screen, colors whose
-		       ;; values add up to .6 of the white total
-		       ;; still look dark to me.
-		       (* (apply '+ (color-values "white" frame)) .6))
-		   'light)
-		  (t 'dark)))
+                  ((color-dark-p (mapcar (lambda (c) (/ c 65535.0))  bg-color))
+                   'dark)
+                  (t 'light)))
 	   (display-type
 	    (cond ((null (window-system frame))
 		   (if (tty-display-color-p frame) 'color 'mono))
diff --git a/lisp/term/pc-win.el b/lisp/term/pc-win.el
index 76a48a86c7..16eb660f00 100644
--- a/lisp/term/pc-win.el
+++ b/lisp/term/pc-win.el
@@ -54,11 +54,9 @@
 ;; DJGPP-compiled Emacs on the same PC.  The names of X colors used to
 ;; define the pixel values are shown as comments to each color below.
 ;;;
-;; If you want to change the RGB values, keep in mind that various pieces
-;; of Emacs think that a color whose RGB values add up to less than 0.6 of
-;; the values for WHITE (i.e. less than 117963) are ``dark'', otherwise the
-;; color is ``light''; see `frame-set-background-mode' in lisp/faces.el for
-;; an example.
+;; If you want to change the RGB values, consider the heuristics in
+;; `color-dark-p' which is used to select a suitably contrasting
+;; foreground or background colour.
 (defvar msdos-color-values
   '(("black"          0     0     0     0)
     ("blue"           1     0     0 52480) ; MediumBlue
diff --git a/lisp/term/rxvt.el b/lisp/term/rxvt.el
index 31e3d6ede4..5dc754c8e0 100644
--- a/lisp/term/rxvt.el
+++ b/lisp/term/rxvt.el
@@ -206,13 +206,11 @@ rxvt-set-background-mode
       ;; The next line assumes that rxvt-standard-colors are ordered
       ;; by the color index in the ascending order!
       (setq rgb (car (cddr (nth bg rxvt-standard-colors))))
-      ;; See the commentary in frame-set-background-mode about the
-      ;; computation below.
-      (if (< (apply '+ rgb)
-	     ;; The following line assumes that white is the 15th
-	     ;; color in rxvt-standard-colors.
-	     (* (apply '+ (car (cddr (nth 15 rxvt-standard-colors)))) 0.6))
-	  (set-terminal-parameter nil 'background-mode 'dark)))))
+      ;; The following line assumes that white is the 15th
+      ;; color in rxvt-standard-colors.
+      (let ((comp-max (float (caddr (nth 15 rxvt-standard-colors)))))
+        (when (color-dark-p (mapcar (lambda (c) (/ c comp-max)) rgb))
+	  (set-terminal-parameter nil 'background-mode 'dark))))))
 
 (provide 'term/rxvt)
 
diff --git a/lisp/term/w32console.el b/lisp/term/w32console.el
index 36e9d896c7..0e9d7c8b5c 100644
--- a/lisp/term/w32console.el
+++ b/lisp/term/w32console.el
@@ -86,9 +86,9 @@ terminal-init-w32console
     (setq r (nth 2 descr)
 	  g (nth 3 descr)
 	  b (nth 4 descr))
-    (if (< (+ r g b) (* .6 (+ 65535 65535 65535)))
-	(setq bg-mode 'dark)
-      (setq bg-mode 'light))
+    (setq bg-mode (if (color-dark-p
+                       (list (/ r 65535.0) (/ g 65535.0) (/ b 65535.0)))
+                      'dark 'light))
     (set-terminal-parameter nil 'background-mode bg-mode))
   (tty-set-up-initial-frame-faces)
   (run-hooks 'terminal-init-w32-hook))
diff --git a/lisp/term/xterm.el b/lisp/term/xterm.el
index 1a727e3933..bf9bcae526 100644
--- a/lisp/term/xterm.el
+++ b/lisp/term/xterm.el
@@ -1120,9 +1120,8 @@ xterm-register-default-colors
     (clear-face-cache)))
 
 (defun xterm-maybe-set-dark-background-mode (redc greenc bluec)
-  ;; Use the heuristic in `frame-set-background-mode' to decide if a
-  ;; frame is dark.
-  (when (< (+ redc greenc bluec) (* .6 (+ 65535 65535 65535)))
+  (when (color-dark-p (mapcar (lambda (c) (/ c 65535.0))
+                              (list redc greenc bluec)))
     (set-terminal-parameter nil 'background-mode 'dark)
     t))
 
diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 0035c5e7b0..2cd99787e8 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -1149,17 +1149,6 @@ css--compute-color
    ;; Evaluate to the color if the name is found.
    ((css--named-color start-point match))))
 
-(defun css--contrasty-color (name)
-  "Return a color that contrasts with NAME.
-NAME is of any form accepted by `color-distance'.
-The returned color will be usable by Emacs and will contrast
-with NAME; in particular so that if NAME is used as a background
-color, the returned color can be used as the foreground and still
-be readable."
-  ;; See bug#25525 for a discussion of this.
-  (if (> (color-distance name "black") 292485)
-      "black" "white"))
-
 (defcustom css-fontify-colors t
   "Whether CSS colors should be fontified using the color as the background.
 When non-`nil', a text representing CSS color will be fontified
@@ -1199,7 +1188,8 @@ css--fontify-region
 		    (add-text-properties
 		     start (point)
 		     (list 'face (list :background color
-				       :foreground (css--contrasty-color color)
+				       :foreground (readable-foreground-color
+                                                    color)
 				       :box '(:line-width -1))))))))))))
     extended-region))
 
-- 
2.21.1 (Apple Git-122.3)

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 10 Jun 2020 16:51:24 +0200
> Cc: Tom Tromey <tom@tromey.com>, simenheg@runbox.com, 41544@debbugs.gnu.org
> 
>  (defun readable-foreground-color (color)
>    "Return a readable foreground color for background COLOR."

Please make sure the doc string says that the function will return
either the black or the white color, depending on which one will
contrast better with COLOR.  Otherwise it is impossible to know,
without looking at the code, that this function can return only these
two colors.

Other than that, I'm okay with the following parts of your patch:

  . the changes in list-colors-print
  . the addition of color-dark-p and the change in
    readable-foreground-color to use it
  . the replacement of css--contrasty-color with
    readable-foreground-color (assuming Tom doesn't object)

Please don't install anything else.

Thanks.

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

10 juni 2020 kl. 17.08 skrev Eli Zaretskii <eliz@gnu.org>:

> Please make sure the doc string says that the function will return
> either the black or the white color, depending on which one will
> contrast better with COLOR.  Otherwise it is impossible to know,
> without looking at the code, that this function can return only these
> two colors.

A good suggestion -- followed.

> Other than that, I'm okay with the following parts of your patch:
> 
>  . the changes in list-colors-print
>  . the addition of color-dark-p and the change in
>    readable-foreground-color to use it
>  . the replacement of css--contrasty-color with
>    readable-foreground-color (assuming Tom doesn't object)

Thank you, pushed to master. It does not prevent extended use later on.

Would still like a better reason for it than a curt dismissal without any technical argument whatsoever other than a general aversion to change. The proposed code is likely considerably better researched and tested than what it aimed to replace ever was.

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 10 Jun 2020 20:29:37 +0200
> Cc: tom@tromey.com, simenheg@runbox.com, 41544@debbugs.gnu.org
> 
> >  . the changes in list-colors-print
> >  . the addition of color-dark-p and the change in
> >    readable-foreground-color to use it
> >  . the replacement of css--contrasty-color with
> >    readable-foreground-color (assuming Tom doesn't object)
> 
> Thank you, pushed to master. It does not prevent extended use later on.

Thank you.

> Would still like a better reason for it than a curt dismissal without any technical argument whatsoever other than a general aversion to change. The proposed code is likely considerably better researched and tested than what it aimed to replace ever was.

I gave all the reasons I have.  They are serious, at least to me.
They have nothing to do with the quality of your research or of the
resulting code.  They have everything to do with considerations of
stability, and with weighing risks and efforts vs advantages.  If you
don't regard such considerations as technical arguments, so be it.

However, after saying that in so many different ways, I don't see any
point in continuing the argument, as we evidently disagree.  Let's
move on, we both have better things to do.

bug#41544: 26.3; Possible incorrect results from color-distance

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I gave all the reasons I have.  They are serious, at least to me.
> They have nothing to do with the quality of your research or of the
> resulting code.  They have everything to do with considerations of
> stability, and with weighing risks and efforts vs advantages.  If you
> don't regard such considerations as technical arguments, so be it.

I understand your objections to changing this behaviour, but the new,
correct calculations make things a lot better.  Would it be possible to
add the new calculations with a defcustom to say whether to use the new
or old methods?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Mattias Engdegård <mattiase@acm.org>,
>   simenheg@runbox.com,
>   tom@tromey.com,  41544@debbugs.gnu.org
> Date: Tue, 18 Aug 2020 15:44:50 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I gave all the reasons I have.  They are serious, at least to me.
> > They have nothing to do with the quality of your research or of the
> > resulting code.  They have everything to do with considerations of
> > stability, and with weighing risks and efforts vs advantages.  If you
> > don't regard such considerations as technical arguments, so be it.
> 
> I understand your objections to changing this behaviour, but the new,
> correct calculations make things a lot better.

How do they make things better, and what things, exactly?  My
recollection is that this was about consistency, not correctness.
Also, the original discussion was about quite a few separate changes,
some of which were installed, so I don't think I have a clear idea
which parts you allude to.

> Would it be possible to add the new calculations with a defcustom to
> say whether to use the new or old methods?

I don't think defcustom is the proper way of handling differences in
behavior at this low level.

bug#41544: 26.3; Possible incorrect results from color-distance

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> How do they make things better, and what things, exactly?  My
> recollection is that this was about consistency, not correctness.

According to the screen shots, they compute correct colours (based on
fixing the luminance formulas, if I remember correctly (it's been a
couple weeks since I read this thread)).

> Also, the original discussion was about quite a few separate changes,
> some of which were installed, so I don't think I have a clear idea
> which parts you allude to.

My impression was that only a few bug fixes were done, but the rest of
the colour computation was not applied?  That may be wrong -- Mattias?

>> Would it be possible to add the new calculations with a defcustom to
>> say whether to use the new or old methods?
>
> I don't think defcustom is the proper way of handling differences in
> behavior at this low level.

Different ways of computing colours?  That's not very low level.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

18 aug. 2020 kl. 16.10 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> My impression was that only a few bug fixes were done, but the rest of
> the colour computation was not applied?  That may be wrong -- Mattias?

In the end, the only affected code was css-mode and list-colors-display. 'color-dark-p' was made available for code that want to use the same mechanism. Other contrasting-colour computations were left unchanged.

bug#41544: 26.3; Possible incorrect results from color-distance

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> 18 aug. 2020 kl. 16.10 skrev Lars Ingebrigtsen <larsi@gnus.org>:
>
>> My impression was that only a few bug fixes were done, but the rest of
>> the colour computation was not applied?  That may be wrong -- Mattias?
>
> In the end, the only affected code was css-mode and
> list-colors-display. 'color-dark-p' was made available for code that
> want to use the same mechanism. Other contrasting-colour computations
> were left unchanged.

I'm not quite sure I follow you here, but could these other computations
also be fixed, with a defcustom to switched between the two computation
methods?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

[CC list trimmed]

19 aug. 2020 kl. 12.11 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> I'm not quite sure I follow you here, but could these other computations
> also be fixed, with a defcustom to switched between the two computation
> methods?

Technically yes, but is there is a reason for users to adjust this particular behaviour? Users dissatisfied with the outcome of the existing algorithms typically curse and set colours explicitly (or suffer in silence); they are unlikely to set a 'use different algorithm' variable.

To be precise, the computations I suppose we are talking about are located in:

 frame-set-background-mode (frame.el:1184)
 xterm-maybe-set-dark-background-mode (xterm.el:1122)
 rxvt-set-background-mode (rxvt.el:198)
 terminal-init-w32console (w32console.el:89)

which all use the predicate (r+g+b)/3 < 0.6 to determine if a colour is dark.
It is also mentioned in a comment in pc-win.el:57.

I believe it to be suboptimal but have no plans to do anything about it.

bug#41544: 26.3; Possible incorrect results from color-distance

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Technically yes, but is there is a reason for users to adjust this
> particular behaviour? Users dissatisfied with the outcome of the
> existing algorithms typically curse and set colours explicitly (or
> suffer in silence); they are unlikely to set a 'use different
> algorithm' variable.

Well, adding this variable would allow us to experiment with defaulting
it to `new-style' during Emacs 28 and see whether there's any immediate
fallout, and allow switching between the two to see what the effect is
(on various systems).

As Eli points out, these things can be finicky.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: mattiase@acm.org,  simenheg@runbox.com,  tom@tromey.com,
>   41544@debbugs.gnu.org
> Date: Tue, 18 Aug 2020 16:10:44 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > How do they make things better, and what things, exactly?  My
> > recollection is that this was about consistency, not correctness.
> 
> According to the screen shots, they compute correct colours (based on
> fixing the luminance formulas, if I remember correctly (it's been a
> couple weeks since I read this thread)).

My bitter experience with handling colors in Emacs is that changes
frequently fix some use cases and botch others.  So I'd prefer to make
such changes only where the current results are completely
unacceptable (and I'd be surprised if we have such cases nowadays).

> > I don't think defcustom is the proper way of handling differences in
> > behavior at this low level.
> 
> Different ways of computing colours?  That's not very low level.

I think it is.  Think about what you'd say in the doc string of such a
defcustom.  "If non-nil, handle colors correctly, otherwise handle
them incorrectly"?

bug#41544: 26.3; Possible incorrect results from color-distance

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I think it is.  Think about what you'd say in the doc string of such a
> defcustom.  "If non-nil, handle colors correctly, otherwise handle
> them incorrectly"?

It's probably say "If non-nil, use old-style computation".

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: mattiase@acm.org,  simenheg@runbox.com,  tom@tromey.com,
>   41544@debbugs.gnu.org
> Date: Wed, 19 Aug 2020 12:13:00 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I think it is.  Think about what you'd say in the doc string of such a
> > defcustom.  "If non-nil, handle colors correctly, otherwise handle
> > them incorrectly"?
> 
> It's probably say "If non-nil, use old-style computation".

Is that a useful doc string?

bug#41544: 26.3; Possible incorrect results from color-distance

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> It's probably say "If non-nil, use old-style computation".
>
> Is that a useful doc string?

It would then go on to discuss the differences in the two approaches.  I
thought you wanted only the first line.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41544: 26.3; Possible incorrect results from color-distance

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: mattiase@acm.org,  simenheg@runbox.com,  tom@tromey.com,
>   41544@debbugs.gnu.org
> Date: Wed, 19 Aug 2020 17:03:37 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> It's probably say "If non-nil, use old-style computation".
> >
> > Is that a useful doc string?
> 
> It would then go on to discuss the differences in the two approaches.  I
> thought you wanted only the first line.

So you propose to explain that the alternatives change what colors
Emacs considers "dark" and "light" when it needs to decide which
colors will contrast well enough, and let people experiment with these
two alternatives to their taste?  I'm not sure, but maybe that could
be useful to someone.

bug#41544: 26.3; Possible incorrect results from color-distance

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> So you propose to explain that the alternatives change what colors
> Emacs considers "dark" and "light" when it needs to decide which
> colors will contrast well enough, and let people experiment with these
> two alternatives to their taste?  I'm not sure, but maybe that could
> be useful to someone.

I think so.

Mattias?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

20 aug. 2020 kl. 15.08 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> Mattias?

I don't want to revive this discussion and consider the matter settled, but since you asked:

Suppose we add standard-colour-darkness-predicate, say, with color-dark-p being the default and (r+b+g)/3<0.6 the 'traditional' option (or the default, if you prefer). How would the effects of that choice be explained to the user?

A difference is only visible when:

1. a face sensitive to the background mode is used (many standard faces are), and
2. a background colour used that is judged differently by the available predicates

Most reasonable backgrounds are either too light or too dark to pass the latter criterion, but people's idea of what is reasonable varies. For instance, black-on-green text is somewhat readable, but

  emacs -bg green -fg black

will give mostly bad default faces (for instance, the minibuffer prompt is almost invisible). With color-dark-p it becomes workable (attached diff).

Whether this is reason enough to add a defcustom is something I won't comment on. I doubt anyone will change it, whatever the default would be.


[-- Attachment #2: dark.diff --]
[-- Type: application/octet-stream, Size: 2621 bytes --]

diff --git a/lisp/frame.el b/lisp/frame.el
index 081d3010e9..75e7f74209 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -1198,13 +1198,10 @@ frame-set-background-mode
 		   non-default-bg-mode)
 		  ((not (color-values bg-color frame))
 		   default-bg-mode)
-		  ((>= (apply '+ (color-values bg-color frame))
-		       ;; Just looking at the screen, colors whose
-		       ;; values add up to .6 of the white total
-		       ;; still look dark to me.
-		       (* (apply '+ (color-values "white" frame)) .6))
-		   'light)
-		  (t 'dark)))
+		  ((color-dark-p (mapcar (lambda (c) (/ c 65535.0))
+                                         (color-values bg-color frame)))
+                   'dark)
+		  (t 'light)))
 	   (display-type
 	    (cond ((null (window-system frame))
 		   (if (tty-display-color-p frame) 'color 'mono))
diff --git a/lisp/term/rxvt.el b/lisp/term/rxvt.el
index 31e3d6ede4..793512105a 100644
--- a/lisp/term/rxvt.el
+++ b/lisp/term/rxvt.el
@@ -208,10 +208,7 @@ rxvt-set-background-mode
       (setq rgb (car (cddr (nth bg rxvt-standard-colors))))
       ;; See the commentary in frame-set-background-mode about the
       ;; computation below.
-      (if (< (apply '+ rgb)
-	     ;; The following line assumes that white is the 15th
-	     ;; color in rxvt-standard-colors.
-	     (* (apply '+ (car (cddr (nth 15 rxvt-standard-colors)))) 0.6))
+      (if (color-dark-p (mapcar (lambda (c) (/ c 255.0)) rgb))
 	  (set-terminal-parameter nil 'background-mode 'dark)))))
 
 (provide 'term/rxvt)
diff --git a/lisp/term/w32console.el b/lisp/term/w32console.el
index 36e9d896c7..66680af44b 100644
--- a/lisp/term/w32console.el
+++ b/lisp/term/w32console.el
@@ -86,7 +86,7 @@ terminal-init-w32console
     (setq r (nth 2 descr)
 	  g (nth 3 descr)
 	  b (nth 4 descr))
-    (if (< (+ r g b) (* .6 (+ 65535 65535 65535)))
+    (if (color-dark-p (mapcar (lambda (c) (/ c 65535.0)) (list r g b)))
 	(setq bg-mode 'dark)
       (setq bg-mode 'light))
     (set-terminal-parameter nil 'background-mode bg-mode))
diff --git a/lisp/term/xterm.el b/lisp/term/xterm.el
index 1a727e3933..d71297d811 100644
--- a/lisp/term/xterm.el
+++ b/lisp/term/xterm.el
@@ -1122,7 +1122,8 @@ xterm-register-default-colors
 (defun xterm-maybe-set-dark-background-mode (redc greenc bluec)
   ;; Use the heuristic in `frame-set-background-mode' to decide if a
   ;; frame is dark.
-  (when (< (+ redc greenc bluec) (* .6 (+ 65535 65535 65535)))
+  (when (color-dark-p (mapcar (lambda (c) (/ c 65535.0))
+                              (list redc greenc bluec)))
     (set-terminal-parameter nil 'background-mode 'dark)
     t))
 

bug#41544: 26.3; Possible incorrect results from color-distance

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> Suppose we add standard-colour-darkness-predicate, say, with
> color-dark-p being the default and (r+b+g)/3<0.6 the 'traditional'
> option (or the default, if you prefer). How would the effects of that
> choice be explained to the user?
>
> A difference is only visible when:
>
> 1. a face sensitive to the background mode is used (many standard
> faces are), and
> 2. a background colour used that is judged differently by the
> available predicates
>
> Most reasonable backgrounds are either too light or too dark to pass
> the latter criterion, but people's idea of what is reasonable
> varies. For instance, black-on-green text is somewhat readable, but
>
>   emacs -bg green -fg black
>
> will give mostly bad default faces (for instance, the minibuffer
> prompt is almost invisible). With color-dark-p it becomes workable
> (attached diff).

I think that's a perfectly good explanation.  Could perhaps be shortened
a bit in the doc string.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41544: 26.3; Possible incorrect results from color-distance

[Simon Pugnet]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

> Very well, it is obviously an improvement. The reason for the 
> current asymmetry was actually that the algorithm discarded the 
> low bits; what about fixing that as well? The improved accuracy 
> amounts to less than 1 % of difference in the return value; no 
> other code needs changing, and we get the symmetry for free. 
> Proposed patch attached.

Firstly, Mattias and Eli, thank you both for investigating and 
dealing with this bug so quickly and thoroughly.

I just noticed that the patch and commit 
7e8c1a671872ef8e45057f25912594cf548639ab to master both reference 
the bug incorrectly in test/src/xfaces-tests.el. The comment under 
the new test is "Check symmetry (bug#51455)" however the bug ID is 
41544.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

4 juni 2020 kl. 08.15 skrev Simon Pugnet <simon@polaris64.net>:

> I just noticed that the patch and commit 7e8c1a671872ef8e45057f25912594cf548639ab to master both reference the bug incorrectly in test/src/xfaces-tests.el. The comment under the new test is "Check symmetry (bug#51455)" however the bug ID is 41544.

Thanks, fixed.

bug#41544: 26.3; Possible incorrect results from color-distance

[Basil L. Contovounesios]

Mattias Engdegård <mattiase@acm.org> writes:

> Emacs does this sort of is-this-colour-dark computation in at least 7
> different places, with different algorithms:
>
> * max(r,g,b) < 0.5
> * r+g+b < 0.5*3
> * color-distance(c, "black") < 292485

Does the list of 7 places already include net/shr-color.el?

-- 
Basil

bug#41544: 26.3; Possible incorrect results from color-distance

[Mattias Engdegård]

1 juni 2020 kl. 21.46 skrev Basil L. Contovounesios <contovob@tcd.ie>:

> Does the list of 7 places already include net/shr-color.el?

No, and thanks for bringing it to my attention! That code appears considerably more sophisticated than the others, and its task (in shr-color-visible) is somewhat different (not just a decision between black and white as contrasting colour). It's probably better to leave it alone for now.