From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.ciao.gmane.io!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#41544: 26.3; Possible incorrect results from color-distance Date: Tue, 09 Jun 2020 19:20:13 +0300 Message-ID: <83wo4g5hz6.fsf@gnu.org> References: <5C4A633D-8222-4439-BE37-9B8674F1DA6D@acm.org> <87r1v2aat3.fsf@tromey.com> <9902865C-01B4-4E50-A433-DBC8B8311234@acm.org> <83tuzueogo.fsf@gnu.org> <6272275C-560C-4437-90F1-2A8294D27019@acm.org> <83o8q2elja.fsf@gnu.org> <83mu5mel4o.fsf@gnu.org> <77F1DDD3-A69F-40ED-902D-74986D5E6596@acm.org> <83y2p5cumz.fsf@gnu.org> <83blm0cjlz.fsf@gnu.org> <83367ccf8w.fsf@gnu.org> <624D7FB8-A836-4A7E-8895-47E867214504@acm.org> <83o8pyc4bq.fsf@gnu.org> <55D73CA5-1EFB-4B0A-8F8B-FDA1D39F51BF@acm.org> <835zc5bsut.fsf@gnu.org> <3BBCFDD4-C14D-4628-91CB-2A0456A96FC7@acm.org> <838sh0abzz.fsf@gnu.org> <83r1us8kw6.fsf@gnu.org> <020DE875-14A8-457A-9AE4-AA0925DB8997@acm.org> <83img48ffx.fsf@gnu.org> <83bllw82xt.fsf@gnu.org> <1B0F31C8-1E11-4527-A053-DD2DE8235F58@acm.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="ciao.gmane.io:159.69.161.202"; logging-data="24198"; mail-complaints-to="usenet@ciao.gmane.io" Cc: simenheg@runbox.com, 41544@debbugs.gnu.org To: Mattias =?UTF-8?Q?Engdeg=C3=A5rd?= , Tom Tromey Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue Jun 09 18:21:22 2020 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jih05-0006D7-RN for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 09 Jun 2020 18:21:21 +0200 Original-Received: from localhost ([::1]:42134 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jih04-0006F2-M2 for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 09 Jun 2020 12:21:20 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:46486) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jigzn-0006Ar-37 for bug-gnu-emacs@gnu.org; Tue, 09 Jun 2020 12:21:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:49239) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jigzl-0008DS-TZ for bug-gnu-emacs@gnu.org; Tue, 09 Jun 2020 12:21:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jigzl-0005Gw-LU for bug-gnu-emacs@gnu.org; Tue, 09 Jun 2020 12:21:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 09 Jun 2020 16:21:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 41544 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 41544-submit@debbugs.gnu.org id=B41544.159171964120172 (code B ref 41544); Tue, 09 Jun 2020 16:21:01 +0000 Original-Received: (at 41544) by debbugs.gnu.org; 9 Jun 2020 16:20:41 +0000 Original-Received: from localhost ([127.0.0.1]:60755 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jigzR-0005FI-BB for submit@debbugs.gnu.org; Tue, 09 Jun 2020 12:20:41 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:38266) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jigzP-0005F4-Td for 41544@debbugs.gnu.org; Tue, 09 Jun 2020 12:20:40 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:44931) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jigzJ-0008AX-D6; Tue, 09 Jun 2020 12:20:33 -0400 Original-Received: from [176.228.60.248] (port=3831 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1jigzI-0003Q6-TA; Tue, 09 Jun 2020 12:20:33 -0400 In-Reply-To: <1B0F31C8-1E11-4527-A053-DD2DE8235F58@acm.org> (message from Mattias =?UTF-8?Q?Engdeg=C3=A5rd?= on Mon, 8 Jun 2020 15:11:36 +0200) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:181793 Archived-At: > From: Mattias Engdegård > 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.