terminal: render: defer conversion of TextColor into COLORREF until actual rendering time (avoid ConPTY narrowing bugs)

(just for more info - the inbox telnetd that we ship internally actually uses xterm-ascii, not the WinTelnetEngine. After building the WinTelnetEngine, I discovered that the inbox telnet was capable enough of doing everything the xterm engine was doing, aside from 256 color and utf-8 character encoding. We probably should just delete it at some point.)

I just don’t think we need to differentiate between something like SGR 95 and SGR 38:5:13 if they’re going to produce the exact same color on every terminal.

Are you sure that you won’t ever need it, and that they look exactly the same on every terminal?

For the second eight colors (SGR 90 … 97 vs. 38:5:8 … 38:5:15, and the same with hundreds and forties for background) this might easily be correct, probably it is correct, although I’m not 100% sure. And no one can guarantee that xterm or some other popular terminal will never introduce some difference, e.g. SGR 2 faint getting applied to the first format only.

For the first eight colors (SGR 30 … 37 vs. 38:5:0 … 38:5:7, and the same with forties for the background) you know it’s not true, we’ve just recently talked about it in #5384. Unless you want to hardcode the xterm 331 … 347 behavior forever.

Which means you can perhaps squash the two representation of the second 8 colors, but not the first 8. Where does it take you? A more complex, harder to understand and harder to maintain codebase, with some squeezing logic where it’s not required at all. Different codepath for the two possible representations of the first 8 colors versus the two possible representations of the second 8 colors, not just where they need to differ, but elsewhere too. Harder to debug, as you’ll run into unexpected inconsistencies wherever you peek into the data. Saving 8 possible values out of the little-bit-more-than-2^24 values, still resulting in little-bit-more-than-2^24, that is, not solving anything here. And in some presumably very rare cases, where the two representations of the same color reside next to each other, you might save a few bytes getting transferred. I find it absolutely pointless.

The experience of having already fixed quite a few color handling bugs in VTE tells me that @Undercat is on the right track here. Treat 95 and 38:5:13 differently all the way through, it’s pointless to “optimize” solely for the sake of 8 colors, based on the current knowledge / future assumption that they’ll end up being the same RGB. Such “optimization” is counterproductive, you lose a lot by having more complex architecture and more complex code, while practically win nothing.

IMO squeezing them only made sense if the two different notations of the 16 colors would be synonyms, and would be guaranteed that they’ll always remain so (as in, let’s say, it’s okay to forget whether it was 38:5:13 with colons, or 38;5;13 with semicolons). This is clearly not the case here.

Well, this is a skeleton of what I propose morphing TextColor into (I prototyped it in GNU using a much more compact style because it’s more comfortable for me to design and test things that way, but just imagine it expanded out into Microsoft syntax for the moment).

#include <unistd.h>
#include <stdlib.h>
#include <stdio.h>

unsigned table[256];
unsigned *getDefaultColorTable(int x) { return table; }
unsigned getDefaultColor() { return 0; } // CONSOLE_INFORMATION::GetDefault[Foreground|Background]()

enum ColorType { Default = 0, SGR4 = 1, SGR8 = 2, SGR24 = 3, Legacy = 4, RGB = 3 };
struct Color {
  union {
    unsigned u32;
    unsigned char u8;
    struct {
      unsigned R:8;
      unsigned G:8;
      unsigned B:8;
      unsigned type:3;
      unsigned background:1;
    };
    struct {
      unsigned legacy:4;
    };
  };
  Color() : type(ColorType::Default) { }
  Color(unsigned char red, unsigned char green, unsigned char blue, bool bg = false) { set(red, green, blue, bg); }
  Color(unsigned char attr, bool bg = false) { set(attr, bg); }
  Color(unsigned colorref, bool bg = false) { set(colorref, bg); }
  Color(unsigned color, ColorType ct, bool bg = false) { set(color, ct, bg); }
  void set(unsigned char red, unsigned char green, unsigned char blue, bool bg = false) { type = ColorType::SGR24; background = bg; R = red; G = green; B = blue; }
  void set(unsigned char attr, bool bg = false) { type = ColorType::Legacy; background = bg; legacy = bg ? attr >> 4 : attr; }
  void set(unsigned colorref, bool bg = false) { type = ColorType::RGB; background = bg; u32 = colorref; }
  void set(unsigned color, ColorType ct, bool bg = false) { type = ct; background = bg;
    switch (type) {
      case ColorType::Default: return;
      case ColorType::SGR4:
        if ((color >= 30 && color <= 37) || (color >= 90 && color <= 97)) { u8 = color; background = 0; }
        if ((color >= 40 && color <= 47) || (color >= 100 && color <= 107)) { u8 = color - 10; background = 1; }
        return;
      case ColorType::SGR8: u8 = color; return;
      case ColorType::SGR24: u32 &= 0xff000000; u32 |= color & 0xffffff; return;
      case ColorType::Legacy: u8 = background ? color >> 4 : color; return;
  } }
  unsigned asRGB(unsigned *table4, unsigned *table8) {
    switch (type) {
      case ColorType::Default: return getDefaultColor();
      case ColorType::SGR4: return table4[u8 > 37 ? u8 - 90 : u8 - 30];
      case ColorType::SGR8: return table8[u8];
      case ColorType::SGR24: return u32 & 0xffffff;
      case ColorType::Legacy: return table4[legacy];
  } }
  operator unsigned() { return asRGB(getDefaultColorTable(4), getDefaultColorTable(8)); }
  int emitSGR(FILE *target = stdout) {
    switch (type) {
      case ColorType::Default: return fprintf(target, "\x1b[%hhum", background ? 49 : 39);
      case ColorType::SGR4: return fprintf(target, "\x1b[%hhum", background ? u8 + 10 : u8);
      case ColorType::SGR8: return background ? fprintf(target, "\x1b[48;5;%hhum", u8) : fprintf(target, "\x1b[38;5;%hhum", u8);
      case ColorType::SGR24: return background ? fprintf(target, "\x1b[48;2;%hhu;%hhu;%hhum", R, G, B) : fprintf(target, "\x1b[38;2;%hhu;%hhu;%hhum", R, G, B);
      case ColorType::Legacy: return fprintf(target, "\x1b[%hhum", background ? (u8 < 8 ? u8 + 30 : u8 + 90) : (u8 < 8 ? u8 + 40 : u8 + 100));
  } }
};

int main() {
  for (int i = 0; i < 256; ++i) table[i] = i;
  Color color(128, 128, 128);
  printf("%x\n", color.asRGB(0, 0));
  color.emitSGR();
  printf("half-intensity\n");
  Color normal;
  normal.emitSGR();
  printf("full-intensity\n");
  Color red(31, ColorType::SGR4);
  red.emitSGR();
  printf("red text\n");
  Color greenish(40, ColorType::SGR8);
  greenish.emitSGR();
  printf("greenish text\n");
  normal.emitSGR();
  printf("normal again\n");
  Color bluebg(0, 0, 80, true);
  bluebg.emitSGR(); red.emitSGR();
  printf("nightmare\x1b[m\n");
  printf("%x %x %x\n", (unsigned)red, (unsigned)greenish, (unsigned)bluebg);
}

Basically:

• The components are all placed into anonymous bitfield/unions so the data layout is more obvious.
• That data layout is then rearranged to match COLORREF for easy interconversion (simple masking).
• The type metadata enum has been extended to completely support all the various color regimes, which are supported as natively as possible for maximal performance and minimum potential for loss/bugs.
• Type conversion and color look-up is done in one place instead of a dozen.
• It could “easily” be extended to also convert RGB to legacy attribute-color, if there really is a need for that somewhere (???)

It would be necessary to re-plumb TerminalApi.cpp and TerminalDispatchGraphics.cpp in order to record the source type accurately enough (right now, there is a complete reliance on constructor type overloading in order to determine the color regime used by the source, but that is inappropriate and dangerous when you start getting multiple types that are built on single octet data words, in addition to being confusing…better to make it explicit).

Before I even start to shoe-horn this monster into the existing code (which will obviously take more than a day, not including validation), I’d be interested in some feedback. Comments? Suggestions? Flames?

IIRC, I had a super old branch where I tried to get TextColor to store a 256-color index, not just a 16-color index. I’m not sure that this is the right branch, but dev/migrie/b/1223-change-256-table looks like it might be the one I’m thinking of.

That might be able to be combined with the “plumb TextColor through” to get all of 4bit, 8bit, and 24bit colors in the same format they were originally.

I think the solution we had in mind was plumbing our TextColor or TextAttribute structures all the way to the renderer, instead of just the COLORREF, then letting the renderer decide what it wants. Most of the render engines will just want the RGB color, but the VT render engine will actually be able to use the TextColor to differentiate between someone setting a color from the 16-color palette and someone coincidentally using the same RGB value as one of the 16-color values.

But that’s still going to convert an application’s request for 31 into a 24-bit value and make it impossible for a connected terminal to determine that the application emitted 31. It’s just flipping this issue over so that a different set of data is lost, but this time the lost data is the entire 16-color palette, isn’t it?

Getting rid of the 16-color SGR will make it impossible for PTY hosts to redefine the 16-color palette entirely, as the pty driver will translate 31 into, say, 38;2;355;0;0. There’s no way for a pty host to push its view of the palette into the pty driver.