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.