Re: [PATCH] Support 24-bit terminal colors.

["Rami Ylimäki" <rami.ylimaki@vincit.com>]

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

2016-08-31 17:21 GMT+03:00 Eli Zaretskii <eliz@gnu.org>:

> > From: Rami Ylimäki <rjy@iki.fi>
> > Date: Tue, 30 Aug 2016 23:11:46 +0300
> >
> > Based on previous work by Rüdiger Sonderfeld, Christian Hopps and
> > Charles Strahan.
>
> Some of these people don't have copyright assignments on file, so how
> much of the code is actually taken from what they wrote?
>

The code is mostly based on the original patch [1] from Rüdiger.
[1] https://lists.gnu.org/archive/html/emacs-devel/2013-10/msg00461.html


> I also don't find your assignment, but maybe once you adjudicate the
> comments below, the number of changes will be small enough to accept
> without an assignment.  Nevertheless, I encourage you to start your
> legal paperwork rolling, so that we could continue receiving your
> contributions even if this one doesn't need it.
>

Sure, I can assign copyright. I believe you can send me instructions and
necessary documents to sign?


> > Currently it's not possible to detect whether terminal supports 24-bit
> > colors. Therefore following methods can be used to force 24-bit mode on:
> >
> > emacs -nw --color=rgb
> > emacsclient -nw -F "((tty-color-mode . rgb))"
>
> That is a screw.  Is there really no way of detecting true-color
> support?  Not even a way that's specific to each kind of terminal
> emulator that supports this mode?
>

I sent mail to bug-ncurses about this. I think 24-bit terminal support has
become more widespread and de facto standard since Rüdiger contacted them
so that they might be willing to standardize new terminal capabilities
regarding this.


> Asking the user to specify a command-line argument each time is so
> annoying that I'm tempted to say this feature is not yet ready for a
> complex multi-terminal program such as Emacs, and we should wait for
> some kind of standard to emerge before we dump this on our users.
>
> Can we devise some customizable data structure that stores names of
> terminals that support true-color?  Then users could set this list
> from their ~/.emacs init files, and have any frame open on those
> terminals automatically support 24-bit colors.  (If we go this way,
> the data structure will have to be consulted by startup.el at the
> right place, so that the initial frame is already set up correctly.)
>

Yeah, let's see first whether the terminfo guys are willing to add
xterm-24bit entry and related capabilities first.


> In any case, if we do agree to specify this via a command-line
> argument, the same command-line argument should IMO be supported by
> emacsclient; asking users to use Lisp is less user-friendly, IMO.
>

Ok.

> * lisp/server.el (server-process-filter): Pass 'tty-color-mode frame
> >   parameter from command line to server-create-tty-frame.
> >   (server-create-tty-frame): Pass 'tty-color-mode frame parameter to
> >   make-frame.
>
> This part is IMO wrong: the true-color support is a property of the
> terminal, not of a frame.  So we should use terminal-parameters for
> communicating the support, and each frame on that terminal should
> thereafter automatically be set to use 24-bit colors.
>

Got it.

> diff --git a/lisp/term/tty-colors.el b/lisp/term/tty-colors.el
> > index a886950..b9b69cd 100644
> > --- a/lisp/term/tty-colors.el
> > +++ b/lisp/term/tty-colors.el
> > @@ -764,7 +764,8 @@
> >      (auto . 0)
> >      (ansi8 . 8)
> >      (always . 8)
> > -    (yes . 8))
> > +    (yes . 8)
> > +    (rgb . 16777216))
> >    "An alist of supported standard tty color modes and their aliases.")
>
> I think 'rgb' is too general to be useful mnemonically.  How about
> '24bit' or 'true-color' instead?
>

24bit sounds good to me if we can't avoid the entry.


> > +(defun xterm-rgb-support-p ()
> > +  "Check whether 24-bit colors have been forced on."
> > +  (or
> > +   ;; emacs -nw --color=rgb
> > +   (eql (cdr (assoc 'tty-color-mode default-frame-alist)) 'rgb)
> > +   ;; emacsclient -nw -F "((tty-color-mode . rgb))"
> > +   (eql (cdr (assoc 'tty-color-mode (frame-parameters))) 'rgb)))
>
> Not sure why this function is needed.  Why not simply look at the
> number of supported colors, i.e. what display-color-cells returns?


With the changes, xterm-register-default-colors is called when the
auto-detected color count is still in effect. That function is just a dirty
workaround.


> >  (defun xterm-register-default-colors (colors)
> >    "Register the default set of colors for xterm or compatible emulator.
> >
> > @@ -930,6 +938,14 @@ versions of xterm."
> >      ;; are more colors to support, compute them now.
> >      (when (> ncolors 0)
> >        (cond
> > +       ((xterm-rgb-support-p) ; 24-bit xterm
> > +       ;; all named tty colors
> > +       (let ((idx (length xterm-standard-colors)))
> > +         (mapc (lambda (color)
> > +                 (unless (assoc (car color) xterm-standard-colors)
> > +                   (tty-color-define (car color) idx (cdr color))
> > +                   (setq idx (1+ idx))))
> > +               color-name-rgb-alist)))
>
> Why don't you use the RGB spec of the color, encoded in an integer as
> the value of IDX here?  Won't that make your job easier in other
> places, and avoid special-casing the true-color terminal?
>

Good point.


> > +/* Color index bit indicating absence of palette.  */
> > +#define FACE_TTY_NONINDEXED_COLOR ((unsigned long) (1 << 24))
>
> Once again, not sure why this bit is needed, since you have the number
> of colors that can be used for the same purpose.


You are right, it's not needed.


> > +static char *
> > +tparam_color (struct tty_display_info *tty, unsigned long color, bool
> > is_foreground)
> > +{
> > +  const char *ts = is_foreground
> > +                  ? tty->TS_set_foreground
> > +                  : tty->TS_set_background;
> > +  return ts ? tparam (ts, NULL, 0, color, 0, 0, 0) : NULL;
> > +}
> > +
> > +static char *
> > +tparam_rgb (struct tty_display_info *tty, unsigned long color, bool
> > is_foreground)
> > +{
> > +  const char *ts = is_foreground
> > +                  ? tty->TS_set_rgb_foreground
> > +                  : tty->TS_set_rgb_background;
> > +  const int red = (color >> 16) & 0xFF;
> > +  const int green = (color >> 8) & 0xFF;
> > +  const int blue = color & 0xFF;
> > +  return ts ? tparam (ts, NULL, 0, red, green, blue, 0) : NULL;
> > +}
> > +
> > +static void
> > +turn_on_color (struct tty_display_info *tty, unsigned long color, bool
> > is_foreground)
> > +{
> > +  if (face_tty_specified_color (color))
> > +    {
> > +      char *p = color & FACE_TTY_NONINDEXED_COLOR
> > +               ? tparam_rgb (tty, color, is_foreground)
> > +               : tparam_color (tty, color, is_foreground);
> > +      if (p)
> > +       {
> > +         OUTPUT (tty, p);
> > +         xfree (p);
> > +        }
> > +    }
> > +}
> > +
> >  /* Turn appearances of face FACE_ID on tty frame F on.
> >     FACE_ID is a realized face ID number, in the face cache.  */
> >
> > @@ -1930,24 +1967,8 @@ turn_on_face (struct frame *f, int face_id)
> >
> >    if (tty->TN_max_colors > 0)
> >      {
> > -      const char *ts;
> > -      char *p;
> > -
> > -      ts = tty->standout_mode ? tty->TS_set_background :
> > tty->TS_set_foreground;
> > -      if (face_tty_specified_color (fg) && ts)
> > -       {
> > -          p = tparam (ts, NULL, 0, fg, 0, 0, 0);
> > -         OUTPUT (tty, p);
> > -         xfree (p);
> > -       }
> > -
> > -      ts = tty->standout_mode ? tty->TS_set_foreground :
> > tty->TS_set_background;
> > -      if (face_tty_specified_color (bg) && ts)
> > -       {
> > -          p = tparam (ts, NULL, 0, bg, 0, 0, 0);
> > -         OUTPUT (tty, p);
> > -         xfree (p);
> > -       }
> > +      turn_on_color (tty, fg, !tty->standout_mode);
> > +      turn_on_color (tty, bg, tty->standout_mode);
> >      }
> >  }
>
> Is this refactoring really needed?  You now have two extra function
> call levels, in a function that is called _a_lot_ in the inner-most
> loops of the display engine.
>
> AFAIU, the only difference in the true-color case is that the 3 last
> arguments of tparam need to be computed by bitwise operations rather
> than set to zero.  Adding 2 extra levels of indirection just for that
> sounds excessive to me.  I'd keep that inline.
>

Ok.


> > @@ -2067,8 +2089,9 @@ tty_default_color_capabilities (struct
> tty_display_info
> > *tty, bool save)
> >        dupstring (&default_orig_pair, tty->TS_orig_pair);
> >        dupstring (&default_set_foreground, tty->TS_set_foreground);
> >        dupstring (&default_set_background, tty->TS_set_background);
> > +      dupstring (&default_set_rgb_foreground,
> tty->TS_set_rgb_foreground);
> > +      dupstring (&default_set_rgb_background,
> tty->TS_set_rgb_background);
>
> Why do we need 2 new attributes here?  Can't we reuse
> TS_set_foreground and TS_set_background instead?  They seem to be
> unused in the 24-bit color case.
>

Yeah, they aren't needed.


> > -      default_max_pairs = tty->TN_max_pairs;
> [...]
> > @@ -4137,7 +4175,6 @@ use the Bourne shell command 'TERM=...; export
> TERM'
> > (C-shell:\n\
> >         }
>
> >        tty->TN_max_colors = tgetnum ("Co");
> > -      tty->TN_max_pairs = tgetnum ("pa");
> [...]
> > -  /* "pa" -- max. number of color pairs on screen.  Not handled yet.
> > -     Could be a problem if not equal to TN_max_colors * TN_max_colors.
> */
> > -  int TN_max_pairs;
> > -
>
> If deletion of this attribute is not necessary for the patch to work,
> it should be done in a separate commit.
>

It's not needed and is a separate change.


> > +      case 16777216:
> > +       tty->TS_orig_pair = "\033[0m";
> > +       tty->TS_set_foreground = tty->TS_set_background = NULL;
> > +#ifdef TERMINFO
> > +       tty->TS_set_rgb_foreground = "\033[38;2;%p1%d;%p2%d;%p3%dm";
> > +       tty->TS_set_rgb_background = "\033[48;2;%p1%d;%p2%d;%p3%dm";
> > +#else
> > +       tty->TS_set_rgb_foreground = "\033[38;2;%d;%d;%dm";
> > +       tty->TS_set_rgb_background = "\033[48;2;%d;%d;%dm";
> > +#endif
> > +       tty->TN_max_colors = 16777216;
> > +       tty->TN_no_color_video = 0;
> > +        break;
>
> Do all true-color terminals support the same escape sequences for
> 24-bit colors?  Is there any standard documents we could rely upon for
> hard-coding them here?
>

AFAIK, there are no documents. However, it seems [2] that all or almost all
24-bit terminals support those sequences (semicolon separated rgb values in
range 0-255) in practice.
[2] https://gist.github.com/XVilka/8346728


>
> > +static bool
> > +tty_supports_rgb (struct frame *f)
> > +{
> > +  return f->output_method == output_termcap
> > +        && f->output_data.tty->display_info->TS_set_rgb_foreground
> > +        && f->output_data.tty->display_info->TS_set_rgb_background;
> > +}
>
> Once again, why not just look at the number of supported colors?


Yeah.


> > +static bool
> > +parse_and_encode_rgb_list (Lisp_Object rgb_list, XColor *color)
> > +{
> > +  if (!parse_rgb_list (rgb_list, color))
> > +    return false;
> > +
> > +  color->pixel = FACE_TTY_NONINDEXED_COLOR
> > +                | (color->red / 256) << 16
> > +                | (color->green / 256) << 8
> > +                | (color->blue / 256);
> > +  return true;
> > +}
> > +
> > +
> > +static bool
> > +tty_lookup_rgb (Lisp_Object color, XColor *tty_color, XColor *std_color)
> > +{
> > +  if (NILP (Ffboundp (Qtty_color_standard_values)))
> > +    return false;
> > +
> > +  if (!NILP (Ffboundp (Qtty_color_canonicalize)))
> > +    color = call1 (Qtty_color_canonicalize, color);
> > +
> > +  color = call1 (Qtty_color_standard_values, color);
> > +  if (!parse_and_encode_rgb_list (color, tty_color))
> > +    return false;
> > +
> > +  if (std_color)
> > +    *std_color = *tty_color;
> > +
> > +  return true;
> > +}
> >
> >  /* Lookup on frame F the color described by the lisp string COLOR.
> >     The resulting tty color is returned in TTY_COLOR; if STD_COLOR is
> > @@ -844,6 +883,9 @@ tty_lookup_color (struct frame *f, Lisp_Object color,
> > XColor *tty_color,
> >  {
> >    Lisp_Object frame, color_desc;
> >
> > +  if (tty_supports_rgb (f))
> > +    return tty_lookup_rgb (color, tty_color, std_color);
> > +
> >    if (!STRINGP (color) || NILP (Ffboundp (Qtty_color_desc)))
> >      return false;
> >
> > @@ -5707,8 +5749,12 @@ map_tty_color (struct frame *f, struct face *face,
> >           CONSP (def)))
> >      {
> >        /* Associations in tty-defined-color-alist are of the form
> > -        (NAME INDEX R G B).  We need the INDEX part.  */
> > -      pixel = XINT (XCAR (XCDR (def)));
> > +        (NAME INDEX R G B).  We need the (R G B) or INDEX part.  */
> > +      XColor tty_color;
> > +      if (tty_supports_rgb (f) && parse_and_encode_rgb_list (XCDR (XCDR
> > (def)), &tty_color))
> > +       pixel = tty_color.pixel;
> > +      else
> > +       pixel = XINT (XCAR (XCDR (def)));
> >      }
>
> Sorry, I don't understand why this complexity is needed.  AFAIU, a
> true-color terminal can produce color directly from its RGB
> components, without the need to look up that RGB descriptor via
> tty-color-desc.  In addition, support for named colors, such as
> "burlywood1", is still required for these terminals, since most color
> specifications in Emacs are of that variety.  Is my understanding
> correct?
>
> If I'm right, then you could either (a) extend tty-color-desc, such
> that it would cons the (NAME INDEX R G B) list for a true-color
> terminal and hand it back to tty_lookup_color; or (b) if you wanted to
> be more efficient, generate the pixel value directly on the C level by
> parsing the color spec if it is of the form "#RRGGBB", and otherwise
> calling tty-color-desc as usual.  Assuming that tty-color-alist is set
> correctly for the true-color terminal, and stores the RGB values for
> each color, that will do the job, eliminating the need for all these
> support functions: parse_and_encode_rgb_list, tty_supports_rgb, and
> tty_lookup_rgb, and also save you 2 extra calls to Lisp.  Am I missing
> something?
>

Not missing anything. Thanks for detailed review and good advice.


> Thanks.
>

[-- Attachment #2: Type: text/html, Size: 20394 bytes --]