Add support for 24-bit / full RGB color

[dandavison]

Hi, thanks very much for this project. This PR adds support for 24-bit / full RGB ANSI color sequences. I have time available to do further work on this branch if that would help.

Changes

1. Add support for 24-bit ANSI color sequences
2. Add an option allowing the user to request the colors as overlays instead of text properties.
3. Make it possible for the user to specify that 'face rather than 'font-lock-face is used, even when font-lock-mode is active.

I'm using this to add support in magit for handling ANSI colors when the git output is redirected through a diff renderer such as [delta](https://github.com/dandavison/delta). To motivate the new options regarding overlays ~~and faces~~, it might be helpful to show the function that I am currently using in magit to apply `xterm-color` to the output received from git (via `delta`): ```emacs-lisp (defun magit-diff-convert-ansi-colors--xterm-color (beg end) (require 'xterm-color) ;; save-excursion does not work here because xterm-color uses delete-and-extract-region (let ((pos (point))) (save-restriction (narrow-to-region beg end) (let ((inhibit-read-only t) (buffer-read-only nil)) (xterm-color-colorize-buffer 'use-overlays))) (goto-char pos))) ```

How do we know this is correct?

1. I've added a 24-bit color demo to xterm-color-test:

   It looks plausibly correct. Furthermore, the 24-bit colors in the demo are the same as in this shell-script. When the shell script is run in a terminal emulator (iTerm2), it appears the same:

2. What about the rest of the output in xtem-color-test?
   Here are screenshots of the test buffers on the two branches:

3. When xterm-color encounters a 24-bit ANSI sequence, the face cache key is > 32bits. Does that work in Emacs on a 32-bit architecture?
   I wasn't sure what to expect the answer to be here. What I have done is create an i686 ubuntu VM (Linux ubuntu-xenial 4.4.0-177-generic #207-Ubuntu SMP Mon Mar 16 01:15:50 UTC 2020 i686 i686 i686 GNU/Linux). In that VM Emacs 24.5.1 has most-positive-fixnum equal to 536870911 (versus 2305843009213693951 on my laptop). There were no errors when running xterm-color-test in Emacs 24.5.1 in that VM.

4. Any issues byte compiling? xterm-color.el byte-compiled with no errors and appears to work correctly when using the compiled file.

5. Finally, here's a screenshot of the new code handling some more complex patterns of ANSI sequences involving foreground and background colors (this is delta in magit, using xterm-color with this PR):

Has it impacted 256-color performance?

Here's a shell script we can use to investigate that:

  #!/usr/bin/env emacs --script
  (let ((package-load-list '((xterm-color t)))) (package-initialize))
  (require 'xterm-color)
  (let ((n (string-to-number (getenv "XTERM_COLOR_TEST_N_ITERATIONS"))))
    (dolist (i (number-sequence 0 (1- n)))
      (xterm-color-test)))

The experiment I'm doing is looking at the impact of the proposed branch on the existing 256-color processing, so I'm comparing master (4b21b619841c93c4700039a93eb1881beee9248c) against this PR, but with the change to xterm-color-test reverted.

The results show a very small decrease in speed: it looks like each call to xterm-color-test takes approximately 3ms longer.

EDIT: these timings are without byte compiling; see comment below for byte-compiled times.

   xterm-color  (master) for i in 1 2 3 4 5; do XTERM_COLOR_TEST_N_ITERATIONS=100 time bash -c "./time-xterm-color-test  2> /dev/null" ; done
         10.42 real        10.15 user         0.05 sys
         10.32 real        10.07 user         0.05 sys
         10.12 real         9.87 user         0.05 sys
         10.12 real         9.87 user         0.04 sys
         10.11 real         9.88 user         0.04 sys
   xterm-color  (master) git checkout -q 24-bit-color--with-no-change-to-xterm-color-test
   xterm-color  (24-bit-color--with-no-change-to-xterm-color-test) for i in 1 2 3 4 5; do XTERM_COLOR_TEST_N_ITERATIONS=100 time bash -c "./time-xterm-color-test  2> /dev/null" ; done
         10.71 real        10.45 user         0.05 sys
         10.46 real        10.22 user         0.04 sys
         10.45 real        10.22 user         0.05 sys
         10.42 real        10.21 user         0.04 sys
         10.40 real        10.15 user         0.05 sys

[dandavison]

Regarding #10, I did have signed FSF papers in the past, and I am happy to check up on my status there and renew if necessary.

[atomontage]

Thanks for the thorough work Dan! I'll try and carve out some time for review and merge. Out of curiosity, how does using face rather than font-lock-face with font-locking active, come into play in practice?

[dandavison]

I'll try and carve out some time for review and merge.

Thanks!

how does using face rather than font-lock-face with font-locking active, come into play in practice?

Ah, good call, I think you identified a part of the PR that needed revisiting. When using xterm-color to display colors in magit, I was originally having trouble with the faces being volatile: getting wiped out by font-lock in the magit buffer. However, your comment prompted me to double-check that I could reproduce that on the current version, and I cannot. So I think it was a mis-step on my part to have concluded that I should use 'face in conjunction with magit.

So, what I've done is push d858d1952adbfaa66f9fa2d9da77479e6209d785, which reverts the option to specify the face property. It uses a buffer-local variable to prevent us from choosing the face property dynamically twice. I suspect that that latest commit is more similar to what we're going to end up deciding on, and I think it's easier for us to discuss with the smaller diff against master.

I have also force-pushed the branch, because I realized that the commit message on the main commit was very inaccurate, due to a subsequent simplification. (I won't need to force push again).

[dandavison]

I realized that that the benchmarks above were made without byte-compiling. Here are timings of the compiled code (256 color codes only). I think the conclusion is the same -- no difference that users will be able to notice: each call to xterm-color-test is less than 1ms slower on this branch.

 xterm-color  (master) hyperfine 'bash -c "XTERM_COLOR_TEST_N_ITERATIONS=100 ./time-xterm-color-test  2> /dev/null"'
Benchmark #1: bash -c "XTERM_COLOR_TEST_N_ITERATIONS=100 ./time-xterm-color-test  2> /dev/null"
  Time (mean ± σ):      3.501 s ±  0.095 s    [User: 3.336 s, System: 0.040 s]
  Range (min … max):    3.352 s …  3.633 s    10 runs

 xterm-color  (24-bit-color--with-no-change-to-xterm-color-test) hyperfine 'bash -c "XTERM_COLOR_TEST_N_ITERATIONS=100 ./time-xterm-color-test  2> /dev/null"'
Benchmark #1: bash -c "XTERM_COLOR_TEST_N_ITERATIONS=100 ./time-xterm-color-test  2> /dev/null"
  Time (mean ± σ):      3.556 s ±  0.075 s    [User: 3.382 s, System: 0.041 s]
  Range (min … max):    3.463 s …  3.671 s    10 runs

[atomontage]

I pushed a few changes today to master, mostly documentation updates and a parsing improvement (don't signal an error that can desync the state machine). You should be able to easily resolve any conflicts. Thanks again for implementing this.

[dandavison]

Thanks for the very helpful review @atomontage. I've merged master and I believe I've addressed all the points in 85a3e240bd.

I agree with not complicating the code for machines with 32 bit integers. What I've done in 85a3e240bd is disable caching of faces when the current fg/bg is a full RGB ANSI sequence and the integer width is insufficient. Do you think that's reasonable?

EDIT I'm going to push another commit affecting the caching; it's still not correct on 32bit.

Would you like to make an executive decision on which of, and how, the terms "full RGB", "truecolor" and "24-bit" should be used? The code now uses "truecolor" as you requested. However there are a few places, mostly comments, that are bothering me:

;; SGR attributes 38 and 48 are supported in both their 8-bit (256 color) and 24-bit
;; (full RGB) variants.

(defsubst xterm-color--skip-24 (SGR-list)

                  (eq 2 (cl-second SGR-list)))          ; 24-bit FG color
             'xterm-color--skip-24)
            (set-a! +truecolor+)
            (set-truecolor! (cddr SGR-list) xterm-color--current-fg))
    (:match ((and (eq 48 (cl-first SGR-list))
                  (eq 2 (cl-second SGR-list)))          ; 24-bit BG color

   ;; 24-bit colors
   (insert "\n")
   (insert "*  24-bit color ramps\n\n")

;; The face caching scheme requires an integer width of at least 56 bits to cache faces derived
;; from truecolor (i.e. full RGB / 24-bit) ANSI sequences. Caching is therefore disabled on e.g.
;; machines with 32-bit integers.
(defvar xterm-color--cache-truecolor-faces (>= most-positive-fixnum (1- (expt 2 56.0))))

[dandavison]

At the latest commit, 4baa93ba0b2dadbb43e51e2c29e4a08ca1436f8a, 24-bit color support is disabled when the integer width is insufficient. It's disabled in the sense that the state machine will not recognize those ANSI sequences as 24-bit, so for a 32bit Emacs, the behavior is as on master.

[atomontage]

I would use truecolor everywhere (except the function xterm-color--skip-24 which is fine), and truecolor (24-bit) in comments next to relevant code to make the structure ultra obvious.

[dandavison]

OK, 760518e22e543db8e5916477427182c9bb989c6c makes terminology more consistent. It also changes the library description in the first line of the file to

;;; xterm-color.el --- ANSI, XTERM 256, and Truecolor color support -*- lexical-binding: t -*-

Of course please feel free to push to this branch to edit any of this, e.g. capitalization.

[atomontage]

I will do another review pass soon and merge. There's a few spots I'll fix up myself later:

• On 32bit, it should skip over the 24bit specific attribute processing but still process any additional attributes in the same SGR list. Right now it just skips every SGR attribute.

• I don't like the huge horizontal color ramp so I'll play around with different ways to display it.

I'll release 2.0 soon after.

Can you squash all these commits into a single one that I can merge? You can force push again.

Thanks again.

[dandavison]

Squashed. I added one line to the README.

[atomontage]

Did some extra work and cleanup, improved performance and parsing reliability and the ANSI-macro-helpers macro should be a lot easier to read now. There are a few more things I want to commit and then I'll release 2.0.

[dandavison]

Great. That all looks much cleaner, in particular make-color-fg and make-color-bg.

[atomontage]

I pushed more fixes. Basically: splitting the truecolor representation out of the color itself and having a separate attribute/flag to keep track of it doesn't work. It seems like it works and the tests that exist seem to work but it's fundamentally broken.

Example:

printf "\x1b[38;2;100;250;250;42mHELLO\x1b[m\n"

Here, the truecolor "flag" stays on after the 38;2;100;250;250 sequence is parsed, since it's not explicitly being turned off. End result being that the next sequence is totally mishandled. Treating the color as a color and not as extra state solves this issue

I should have caught this earlier since I've been bitten by a similar issue when the AIXTERM pull request came in and I wasn't paying attention. That goes to show you that things that may appear simple, can hide unexpected complexity. You should pause and look at things again and again, whenever you're dealing with non-trivial state machines.

[dandavison]

I see, sorry about that, thanks for working on this so carefully. What do you think, in principle, about adding some ert tests, and a CI build?

[atomontage]

No worries, these are mostly notes to myself. Thanks again, if it weren't for your pull request waking me up from my slumber, there wouldn't be truecolor support!

I will add some more tests but they won't be exhaustive. That's a lot of work as I'll need to test against another state machine that I trust to be correct, and I simply don't have the time. Setting all of this up is outside the scope of what I envisioned for this library. Maybe one day.

[dandavison]

OK, I have another repo with an ert test suite, a Makefile and a .travis.yml config that are already set up and working. I sent you a collaborator invitation (it's private currently) in case you're interested in copying that setup over; I'd be happy to help.

[atomontage]

And of course it's still not correct, my new packing scheme works to keep everything in one integer but not in the cache as there's overlap since one bit of information (is the color present or not?, the variable is set to nil outside the cache if it's not, but it's zeroed in the cache which overlaps with color values) is lost! Even my old 256 color caching scheme was not correct.

I think I have it now, but I'm also in a haze as I'm about to go to sleep. I'll give it another shot tomorrow.