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

[DHowett-MSFT]

Right now, every time Renderer needs to switch the brush colors, it does the following:

1. get the TextColor from the attribute run
2. get the RGB value of the color from the color table
3. pass that to Engine::UpdateDrawingBrushes

Step 2 is lossy, and is the source of a class of bugs I'll call "ConPTY narrowing" bugs. In addition, there's a step where VtEngine needs to know the color table to do a translation from RGB back into indexed color.

"narrowing" bugs

application output	conpty sends	why
\e[38;2;22;198;12m	\e[92m	RGB(22, 198, 12) matches color index 10 in Campbell
\e[38;5;127m	\e[38;2;175;0;175m	the xterm-256color color palette gets translated into RGB because Campbell only covers 0-16
\e[38;2;12;12;12m	\e[30m	RGB(12, 12, 12) matches index 0 in Campbell and index 0 is the default color (#293)

If Renderer passed the TextColor directly to the Engine, Xterm256Engine could pass through all indexed colors unchanged, and XtermEngine and WinTelnetEngine could flatten to the 16-color space.

• [x] #3076 is caused by us doing early unpacking of colors and boldness; deferring until later will yield an improvement in preservation of underline status.

[zadjii-msft]

On a scale of 1-10, how related to #1223 is this?

I already have a fix for #1223 that involves storing 256-color attributes as an index and looking up their color when we want to render them, not when we write them.

I guess this kinda dovetails with that work nicely. Should we do this for 20H1? If so, I'll send the other PR ASAP and get on this one next.

[ExE-Boss]

Probably quite a bit, since according to the above table, conhost currently converts 256‑color attributes into 24‑bit TrueColor RGB values.

[DHowett-MSFT]

Scale: 10

[Undercat]

I think I can solve part of the problem pretty easily. Enough to take care of issue #5201, at least.

At around line 223 in paint.cpp you have the following as part of _RgbUpdateDrawingBrushes() (reformatted for brevity):

  WORD wFoundColor = 0;
  if (fgChanged) {
    if (fgIsDefault) {
      RETURN_IF_FAILED(_SetGraphicsRenditionDefaultColor(true));
    } else if (::FindTableIndex(colorForeground, ColorTable, cColorTable, &wFoundColor)) {
      RETURN_IF_FAILED(_SetGraphicsRendition16Color(wFoundColor, true));
    } else {
      RETURN_IF_FAILED(_SetGraphicsRenditionRGBColor(colorForeground, true));
    }  _LastFG = colorForeground;
  }
  if (bgChanged) {
    if (bgIsDefault) {
      RETURN_IF_FAILED(_SetGraphicsRenditionDefaultColor(false));
    } else if (::FindTableIndex(colorBackground, ColorTable, cColorTable, &wFoundColor)) {
      RETURN_IF_FAILED(_SetGraphicsRendition16Color(wFoundColor, false));
    } else {
      RETURN_IF_FAILED(_SetGraphicsRenditionRGBColor(colorBackground, false));
    } _LastBG = colorBackground;
  }

The above code prefers 4-bit SGR over 24-bit SGR—even though it claims to be formatting truecolor output, as opposed to _16ColorUpdateDrawingBrushes()—so you effectively assume that the PTY slave maps those 16 primary colors to the same RGB intensities that you do. This is not always going to be the case.

By deleting the middle condition entirely, at least you would be generating SGR in a uniform 24-bit color space. The values sent for the 16 primary colors would, of course, still be encoded with your color table values, instead of whatever the downstream's intensity mapping for the primaries might happen to be, but at least the color space would not exhibit discontinuities related to mismatches between primary color tables.

It only solves “half” the problem, as I said, because you still don't know what SGR labeling may have initially produced the RGB intensity values in your buffer, but I think it’s a very cheap fix for what DHowett called “narrowing bugs.”

Am I missing something?

[DHowett-MSFT]

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.

[Undercat]

No problem. The important issue is to encode in 24 bits. The values being encoded are unimportant. Change the middle condition to...

  WORD wFoundColor = 0;
  if (fgChanged) {
    if (fgIsDefault) {
      RETURN_IF_FAILED(_SetGraphicsRenditionDefaultColor(true));
    } else if (::FindTableIndex(colorForeground, ColorTable, cColorTable, &wFoundColor)) {
      RETURN_IF_FAILED(_SetGraphicsRenditionRGBColor(wFoundColor, true));
    } else {
      RETURN_IF_FAILED(_SetGraphicsRenditionRGBColor(colorForeground, true));
    }  _LastFG = colorForeground;
  }
  if (bgChanged) {
    if (bgIsDefault) {
      RETURN_IF_FAILED(_SetGraphicsRenditionDefaultColor(false));
    } else if (::FindTableIndex(colorBackground, ColorTable, cColorTable, &wFoundColor)) {
      RETURN_IF_FAILED(_SetGraphicsRenditionRGBColor(wFoundColor, false));
    } else {
      RETURN_IF_FAILED(_SetGraphicsRenditionRGBColor(colorBackground, false));
    } _LastBG = colorBackground;
  }

[DHowett-MSFT]

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?

[Undercat]

Absolutely. Like I said, it doesn't solve the issue of recovering the initial SGR labeling used to produce the RGB intensity values from which you render output, but it will solve the issue of relying on the assumption that the slave uses the same RGB values for \e[31m that you do.

For example, if you generate output for foreground RGB(118, 118, 118) now, with the default primary palette, your render routine for VT will generate \e[90m which may not be mapped back to RGB(118, 118, 118) by the slave. They may map the palette colors differently. Practically everyone does, in fact.

With my change, the renderer will produce a 24-bit SGR label and the PTR slave will not be able to change it. So at least the palette will be uniform. Right now, it is only uniform if the slave's primary color palette matches yours.

Effectively, my suggesting converts the “narrowing bug” into a “widening bug”—you still won’t be able to recover true 256-color SGR labels, for instance. You won’t ever generate \e[38;5;243m instead of \e[38;2;118;118;118m, but at least you won’t be generating \e[90m. That is a primary color. The primary colors are mapped by various terminal emulators in a large number of different ways. Nobody can “remap” the 24-bit SGR codes, though.

In other words, the situation you have now is actually the worst of all worlds. It only renders visually correct results if the slave’s primary color palettes match yours. If they don’t match, you wind up with egregious visual artifacting, as can be seen in mintty/mintty#981

[DHowett-MSFT]

I appreciate that, but that correctness introduces a more immediate visual issue: the user’s carefully-crafted 16-color palettes are overridden because the pty driver is sending out 24-bit color. For now, I’m willing to accept “sixteen of the 16,000,000 colors are rendered incorrectly” if the immediate alternative is “all sixteen of the user’s specified colors are lost”. Just picking our trade offs :smile:

[Undercat]

Yes, that is quite true. The best solution, by far, would be able to reconstruct the original SGR labeling used by the console application to produce the color change in the first place.

In order to do that, you would have to send the SGR code down to the renderer: if the renderer happened to be a VT driver, out the door the code would go; if it were a screen painter, though, the SGR would have to be decoded on-the-spot. You would have to push the decode down to your renderers, as you originally suggested...or at least tag colors in your buffer with a “width code”, so the VT drivers would know what flavor of SGR code to generate.

A much more complicated proposition than changing a few characters, for sure! :smile_cat:

[zadjii-msft]

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.

[Undercat]

Unfortunately, TextColor alone would not be enough. Although it could certainly distinguish between 4-bit and 24-bit SGR labelings, it would have no way of recording and reconstructing 8-bit labelings.

I was thinking about hiding information in the high-order octet of COLORREF, since nobody uses it, but certain opaque downstream functions might actually insist that it be zero, like WINAPI SetColor(). Not being able to overload COLORREF means that a fairly large number of changes would be needed to insure that all consumers, like UpdateDrawingBrushes(), digest some new “color record,” from which they could simply extract a COLORREF, if that's all they need.

Do any of you happen to know where the input stream (ANSI SGR escape sequences, in particular) gets decoded into properties, like COLORREFs? I've been poking around, but I don't have enough context to zero in on it with simple searches.

[zadjii-msft]

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.

[DHowett-MSFT]

Doesn't it already store a 256-color index, though? With _meta = ColorType::IsIndex and the union's _index value storing the index 0-255... we should be able to determine if it's being used for <16 and <89.

Unless the issue is that the first 16 colors in the 256-color palette and the traditional 16-color palette are disjunct. If that's the case, Xterm was perhaps crazy for doing that.

[Undercat]

I'm thinking it might actually be best to hijack COLORREF, after all. Stripping the high octet in the screen-painting renderers is no more inconvenient, in practice, than defining a new type to convey the information to them and extracting a COLORREF in situ.

There’s just one problem remaining...I don't have enough bits in a DWORD to unambiguously record all the information required. I can certainly minimize the loss to a trivial level that will be indistinguishable in practice, but it will not reconstruct the input stream with 100% fidelity.

To wit, if you take the high octet and use it store the index value for 8-bit SGR labelings, you can get the 4-bit labelings for free. Although I am unaware of any absolute requirement to do so, virtually everyone treats the first sixteen indices of the VGA color table as duplicates of the CGA color table. This is, in fact, the default SGR behavior.

So, imagine the high octet contains a value less than 17, you would simply emit a 4-bit SGR label; if it contained a value 17 or above, you would emit an 8-bit SGR label; if it was zero, and the 24-bit color in the rest of the word matched the value for "black", you would send \e[30m or \e[40m—if it didn’t match, you would send the 24-bit color. Not perfect, but it avoids using a UINT64 to hold the one extra bit you need to disambiguate the 4-bit and 8-bit cases without resorting to reverse look-up tables for all outputs...which will be costly.

Thoughts?

[Undercat]

I cannot find any example where they are disjoint in practice.

The scheme I describe avoids disambiguating a union on the color data—the 24-bit color is always available. The extra data can just be stripped out or ignored by anyone that doesn't need it.

[egmontkob]

Unless the issue is that the first 16 colors in the 256-color palette and the traditional 16-color palette are disjunct.

The first 8 used to be different in xterm; I mean the colors were the same (i.e. a single 256-color palette, along with OSC 4 for setting it), but the legacy notation (e.g. 31) did convert it to bright counterpart when asked to be bold (1); whereas the 256-color notation (e.g. 38:5:1) did not, it kept the dark color and painted that with bold typeface. VTE copied this behavior.

Xterm stopped doing this in version 331 (I'm not aware of any place where 31 works differently than 38:5:1 (which doesn't imply there isn't any)). In VTE we kept treating them differently.

[zadjii-msft]

@DHowett-MSFT It is capable of storing 8bit in the index there, but doesn't currently. This commit 608c660b from the aforementioned branch changes conhost to actually use the index as a 8bit value, not just a 4bit one. We're currently storing two tables, but ignoring the first 16 of the 256 color table. That's dumb, obviously, but an artifact of the organic growth of RGB color in conhost.

[Undercat]

Mmmm. Interesting.

Well, the “ideal” solution would be to push a UINT64 to the renderers that has complete color information in it, without overlap. I doubt this would cause any overflow to the stack for the relatively modest function calls involved, even on 32-bit platforms. That way, a renderer could immediately access whatever encoding it needed and only one forward look-up would need to be done to decode the color at the time the escape sequence is parsed.

[zadjii-msft]

@Undercat or perhaps 2 32bit TextColor's? 😆

[Undercat]

@zadjii-msft lol.

You could, I suppose, push overlapped (union'd) data down and let the renderers figure out what to do with it for their own purposes. You could then unambiguously record 4-bit, 8-bit and 24-bit SGR labelings just by extending TextColor's metadata to add the 8-bit case and have three encoding cases instead of two. You would have color index look-ups all over the place that way, but then again, who cares? It would, at least, keep the 32-bit DWORD parameter....

[egmontkob]

You would have color index look-ups all over the place that way, but then again, who cares?

If this is not a problem then encoding the union of 256-color index and 16M truecolor plus some special values is trivial in 25 bits. Sounds to me like the best approach. (Also what VTE does by the way.)

Somehow I have the impression that WT devs don't like it, or it's not sufficient for WT's architecture; the resolved color (24 bits) and the origin (256 indices or direct true color = 257 or so possible different values) have to be carried together. And for this you would need just a tiny little more than 32 bits.

If indeed the resolved RGB and the origin of the color needs to be both carried, and squeezed into 32 bits, there are still some tricks to consider.

One idea is to drop the color precision of the R and/or B channel to 7 bits. No one will notice with their bare eyes, only when using some color picker software to check the exact color.

Another idea is not to allow to alter the extra 240 colors, only the basic 16 ones. (All terminals I've checked have the exact same default RGB for these 240 additional entries.) For the additional ones, the RGB value wouldn't be directly stored, but could be derived using a fixed hardwired table.

But you can also relax on this limitation: allow the extra 240 colors to be freely modified, with just one limitation: the last 24 entries (the grayscale ones) must be grayscale. With this limitation all the information can be squeezed into 32 bits. E.g. the high 8 bits are either the index (0..231) or maybe 232 for "default", 233 for "grayscale", in the latter case you have 24 more bits to store the actual grayscale index (4 bits), plus the R=G=B component (8 bits) and still have 12 bits free... which you could use to further loosen the restriction, and allow almost-grayscale colors, where the color components aren't too far away from each other :-)

[Undercat]

@egmontkob That was rather like what I was thinking about originally. There appears to be one additional difficulty, however. There is an extra flag in TextColor used for “default color.”

Really, as long as you’re okay with multiplexing color information (by creating unions that have to be decoded by the consumers), you can directly encode the 4-bit, 8-bit and 24-bit cases and then reconstruct the original SGR input for VT sinks with 100% fidelity.

I'm doodling a test case right now by creating a class (really just a structure with methods) that has assignment overloads for WORD and DWORD that automatically reconstruct the 4-bit legacy attribute color or 24-bit COLORREF data from a union...with a gaggle of methods to write SGR4|8|24 color sequences from the same data. This would minimize changes to the consumers of color data—basically, you would just have to change the color parameter’s formal type from COLORREF to something like Color.

Since the new type would behave like a COLORREF in most situations, fewer changes would need to be made to all the hungry consumers of color data, hopefully making this parameter enhancement (to support differentiable SGR labelings) less apocalyptic, in terms of code-change footprint.

[Undercat]

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?

[j4james]

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?

@Undercat Personally, I'd recommend leaving the TextColor class alone. Fixing this issue requires changes to a number of areas of the code base, including the renderer, the SGR dispatcher, and color table management (I'm working on some of these things at the moment). The TextColor class is the least of the problems, and rewriting that now is just going to cause a lot of pain without actually fixing anything. At most I think it might need an additional ColorType option.

[zadjii-msft]

Holy crap I had this whole big long message typed up and I never sent it to this thread D: That's my bad. Lemme think if I can try and recover some of my thoughts here

Woah, thanks for the contribution here. There's certainly a lot to look at, so I might be reading some of the details wrong here.

I'm still not sure how this is better than TextColor+"change _index to be a 8bit index, instead of just a 4bit one". I'm not sure there's a lot of value in tracking 4bit VT indices separately from legacy ones - since legacy colors were always a 4bit index, we're trying to treat them as effectively the same thing as a VT 4bit index.

It could “easily” be extended to also convert RGB to legacy attribute-color, if there really is a need for that somewhere (???)

There unfortunately is - because the Win32 console API provides a mechanism for reading back the contents of the buffer, including text attributes, but only as legacy attributes, we need to be able to convert the non-legacy attrs back into legacy ones.

Type conversion and color look-up is done in one place instead of a dozen

I'm not sure that Color::asRGB is going to be called in any fewer places than TextColor::GetColor already is

That data layout is then rearranged to match COLORREF for easy interconversion (simple masking).

This is nice, and I’d be interested to see if it could be integrated with TextColor rather than replace it entirely

Sorry again to ghost this thread accidentally 😬

[Undercat]

I changed the "class" name to Color only to make debugging easier. Changing the name would cause all consumers of color data to fail catastrophically when TextColor gets deleted, insuring that all downstream dependencies would be found and updated to conform to the new, extended, API.

This would prevent subtle bugs from being masked by incomplete functional similarities between the old and new color representations, but in the abstract, it really doesn't matter what you call the class/structure—the feature extension that I consider to be sine qua non is adding explicit support for distinguishing 4- and 8-bit SGR labelings from legacy 4-bit color and truecolor.

I distinguish legacy 4-bit color from 4-bit SGR color to avoid very subtle, and fortunately also very obscure, color bugs.

Different screen writers can have different expectations about what color mapping is used for 4-bit color, and TextColor is unable to satisfy both those expectations at the same time. It cannot distinguish between text that is written using the Windows API and text that should be passed-through using SGR. Most of the time, this will not cause a problem, and for those situations where it is a problem, there frankly is no good fix. Converting the Windows 4-bit legacy color values to 24-bit color values would eliminate any color infidelities caused by mismatched color tables, but it assumes that the downstream renderer is 24-bit capable. Still, I'd rather have the option of making the distinction than not having that option...and later needing it. Just my bias.

Of course, this does not seem to be an issue for 8-bit SGR because, as near as I can tell, the Windows console has never supported 8-bit native color. Besides, the upper part of the VGA color table is almost never changed from its canonical values, so it's likely to have the same values everywhere.

Now, as far as color tables are concerned: the new structure does not care how color mappings are represented—such mappings are an altogether different branch of code I haven't dealt with here at all, except as a dependency. The only thing Color attempts to do is encapsulate the color transformations in one place, but you could just as easily generate SGR externally from 4/8/24-bit values and an index key.

Leaving TextColor largely intact would be difficult, but then, in all fairness, shoe-horning a new representation in to replace it would be non-trivial, too.

To minimize changes to TextColor, you'd basically have to modify the input state machine to explicitly record 4- and 8-bit SGR codes; extend TextColor to support those codes in some way, at least by eliminating TextColor's existing dependence on constructor overloading to implicitly differentiate between 4-bit and 24-bit color values (which presently is the only way of initializing color data); then extend the existing color mapping system to provide support for 8-bit color indices, in addition to the 4-bit indices; and finally, modify the SGR output routines (in VtEngine) to explicitly reconstruct 4-bit and 8-bit SGR codes, when appropriate, for VTs.

And, of course, you can then also change whatever readback function @zadjii-msft referred to, above, to reconstruct values from the new color representations (4-bit and 8-bit SGR values). The goal is essentially to avoid using truecolor as a "universal" intermediate for extended colors...a use which presently makes it impossible to regenerate SGR codes with fidelity, and is what got me started on this whole thing in the first place.

I really would try hard to encapsulate all these color representations to avoid leaking different color encodings all over the place, even if it requires separating the SGR parser/generator for colors values from the rest of the SGR parser/renderer. That's just my personal paranoia, though.

Somehow, I managed to spent over an hour looking again for the exact code that parses the CSIs out of the input stream...to no avail. Should have bookmarked it, I suppose, but I didn't think this was going anywhere, since I got no feedback for weeks.... Anyway, I really don't have the time to spend on this right now—especially given the high potential for long and painful iterations raised by the highly-non-local nature of the changes required, and the fact that @j4james is already modifying some of the code involved. The fact that I don't actually have a personal use-case for any of the code involved doesn't help.

Still, I did just outline (in very broad strokes) some of the changes that I feel would be required to support SGR color with any fidelity. Some of you Microsoft natives can probably agree amongst yourselves on how to make the required changes in the course of your other work that already touches upon the relevant code. That would probably be 10x easier than having me attempt to make such changes in relative isolation.

Sorry I couldn't be of more help.

Cheers!

[DHowett-MSFT]

@undercat (sorry again for the delay, and we absolutely appreciate your input on this -- it's one of the ways we're hoping to be "better together" :smile:)

[j4james]

Just to clarify my position, I'm working on some areas of the code that relate to this issue (like the SGR dispatcher), and as part of that I've been experimenting with a partial fix for this issue as well. But that's mostly been to validate the changes I'm making to the API. Fixing this issue is not the focus of my work at the moment, so I don't want to discourage anyone else from taking it on in the meantime (although I think that'll be tricky without other fixes being in place first).

The main point of my previous comment, though, was that I didn't think a TextColor rewrite was necessary, and would entail a lot of work with no real benefit. I don't think there's any value in trying to preserve the exact format of every SGR sequence in the screen buffer, because realistically that's never going to be a perfect representation. And in the long term, clients that need that level of fidelity will be using something like the pass-through mode anyway.

So if we stick to what is actually needed to generate the correct output, rather than trying to reproduce the original input, I think there are only four color types we need to worry about.

1. Default colors - SGR 0, 39, and 49.
2. Indexed colors that can be brightened by SGR 1 - SGR 30 to 37 and 40 to 47.
3. Indexed colors that shouldn't be brightened - SGR 38:5, 48:5, 90 to 97, and 100 to 107.
4. Direct colors - SGR 38:2, 48:2, and at some point potentially the CMY(K) variants as well.

I would probably expect colors set via the console APIs (e.g. SetConsoleTextAttribute) to map to case 3. They're indexed colors, which should be affected by the color table, but they probably shouldn't be brightened by SGR 1.

So as I see it, the only thing missing from the existing TextColor implementation is the ability to distinguish been cases 2 and 3. And for that, all we really need is one more ColorType, and maybe something on the index constructor designating which index variant is being set.

[Undercat]

You absolutely need to store information about the original SGR labeling that led to a color being in the buffer. I cannot emphasize this enough: if you do not know the labeling that produced a color value in the buffer, you cannot guarantee its correct reproduction.

@j4james you need to remember that the terminal "wrapper" may have a different view of the indexed colors than any downstream VT used to actually render the output. They each can have different color tables. When they do, if you continue to generate SGR codes based on the actual color value in your buffer, without knowing what SGR labeling produced it, you have no way of distinguishing between labelings that map to the same color in terminal, but different colors on the VT.

You wind up with a non-invertible many-to-one mapping in terminal that forces you to arbitrarily choose an SGR code to output. Right now, it chooses the shortest possible SGR code. That produces the visual error in mintty/mintty#981 and is why I ran down this rabbit hole to begin with.

You cannot fix this problem without extending TextColor to distinguish the original SGR code that produced a color in the buffer. It's not as simple as differentiating colors based on how other attributes, like brightening, might apply to them.

[j4james]

if you continue to generate SGR codes based on the actual color value in your buffer, without knowing what SGR labeling produced it, you have no way of distinguishing between labelings that map to the same color in terminal, but different colors on the VT.

We definitely won't be generating the SGR codes based on the RGB value if that's what you mean. As I said above, the are four different color types I think we need to retain, and we would generate VT sequences that best represent those types. 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. Mintty certainly doesn't seem to interpret them differently.

It would help if you could provide an example of a specific color sequence that you think we'd be getting wrong without the additional types you're proposing, and also what terminal you'd expect it to fail on.

[egmontkob]

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.

[j4james]

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

Sure enough that I'm not going to encourage a whole lot of unnecessary changes to the code base on the off chance that it might be an issue 10 years from now on some obscure terminal that nobody has heard of. However, if you can come up with a genuine use case that is going to impact users in a meaningful way, please do let me know.

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.

Which is why I'm not suggesting we treat SGR 30 the same as 38:5:0. See my message above. I proposed two index types to distinguish between these two cases. SGR 30 is case 2 (indexed colors that can be brightened), and SGR 38:5:0 would be case 3 (indexed colors that shouldn't be brightened).

Anyway, I can't stress enough how little any of this matters. The difficult part of getting this issue fixed is in places like the dispatcher and the renderers, and then figuring out all the little issues that are going to break once the existing behaviour changes. Adding more color types in the future, if we really need to, is trivial in comparison.

[DHowett-MSFT]

Right- regardless of which particular representation we use and its fidelity, the first step towards closing #2661 is us having a framework for piping attributes around instead of just RGB color values. Once we nail that, we can work on improving the fidelity wherever it is practical.

[egmontkob]

@j4james I admit I slipped through some of the details in your earlier posts, apologies for that. Indeed if you're certain that the status quo won't change, the solution you're proposing is technically correct.

I'm still not a great fan of it architecturally, since some of the decisions (within case 3 mapping 90..97 into the same slots as 38:5:8..15) happen before the colors are stored wherever they are stored (and you lose info that might be useful for debugging etc.) and some other decisions (e.g. whether to actually brighten case 2) when it comes to hitting the screen pixels; that is, the whole logic is split into two vastly different places – whereas all of these could be done at this latter step, which I'd find a much cleaner, simpler architecture.

Anyway, as you say, it indeed really doesn't matter.

[Undercat]

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. Mintty certainly doesn't seem to interpret them differently. [emphasis mine]

They absolutely are not guaranteed to be the same color intensity values on every terminal. They will only be the same color if terminal's internal color table for the primary colors + intensity (i.e., the first 16 colors in 4-bit and 8-bit color) exactly matches the corresponding color table of the VT renderer, whatever it may be.

You need to tag the source of the color so you can follow it all the way through terminal—at least given the way you convert all colors (and every other text property, for that matter) to a non-portable "common ground" internal format right now, and then render everything from that internal format. If you do not tag the origin of the colors, you will be unable to distinguish between \e[90m and \e[38;5;8m and \e[38;5;243m and \e[38;2;118;118;118m because they will all have the same color value in the default Microsoft mapping.

If you distinguish only based on whether a color can be brightened, you separate the first two cases. You do nothing to distinguish them from the last two cases, which should be absolute colors, but in your scheme will wind up being rendered as Microsoft default colors.

Classifying the colors based on the ancillary properties that might apply to them is insufficient. You need enough information to distinguish between the four alternatives I just listed, and the only approach that guarantees a given SGR input stream will produce the same SGR output stream is to tag the colors by their origin.

Now, you could get around this problem by only generating 24-bit SGR colors, but this assumes that the VT renders truecolor (likely) and it also eliminates emitting primary SGR codes (30/90 series) entirely. If you restrict the 24-bit up-conversion to VT SGR renderings only, then all will be well from a color fidelity standpoint...assuming the downstream renderer can handle 24-bit SGR codes. But you will never be able to generate a primary SGR color this way, and since those are the colors that everyone remaps, you will effectively be forcing all renderers to use terminal's colormap.

I already had that discussion with @zadjii-msft a long time ago, but I don't think the implications are being appreciated here.

[DHowett-MSFT]

You need to tag the source of the color so you can follow it all the way through terminal—at least given the way you convert all colors (and every other text property, for that matter) to a non-portable "common ground" internal format right now, and then render everything from that internal format. If you do not tag the origin of the colors, you will be unable to distinguish between \e[90m and \e[38;5;8m and \e[38;5;243m and \e[38;2;118;118;118m because they will all have the same color value in the default Microsoft mapping.

@Undercat given that this entire workitem was filed to capture "stop converting all colors into their 24-bit RGB representations and therefore losing fidelity", I think that most of your concerns are already noted.

What we have, today:

• INPUT: All colors are converted into 24-bit color representations
• OUTPUT: All 24-bit color representations are palettized into 0-16 or sent as 24-bit

This is incorrect. This is what this issue seeks to solve.

What we have, with this issue closed:

• INPUT: All colors are stored as palette-index or 24-bit color with an enumerator indicating what they are.
• OUTPUT: All palette-index or 24-bit color representations are passed through in a format that most closely represents what they were on input.

What we can have in the future, with the architecture this issue lays down:

• INPUT: All colors are stored as palette-index (16 normal), palette-index (8 bright), palette-index (256 xterm), or 24-bit color
• OUTPUT: All colors are emitted as close to INPUT as possible.

All that to say:

When this issue is closed, there will be no dependency on the console host's internal color table and no assumption that a connected terminal will share the same color table.

[Undercat]

OUTPUT: All colors are emitted as close to INPUT as possible.

I can't seem to get through, unfortunately. You have absolutely no way of knowing if a color you output is "close" to its "input value." You don't know what the input color is; you only know what SGR encoding the application asked for.

You convert that SGR code using your internal color tables. Those color tables may not match what is used by an external renderer.

Specifically, if the renderer thinks that \e[90m is rgb(85,85,85) and you put rgb(118,118,118) into the buffer because that's what you think \e[90m's color should be, according to your internal color table, you have already irretrievably lost information that will foreverafter prevent you from recovering and reproducing the VT's notion of what that color should be.

On output, you will generate an SGR value that corresponds "closely" to rgb(118, 118, 118) because "that's what's in the buffer," but its not what the SGR color you received was calling for.

If you do not make the architectural change, you will either do it later or you will ever-after force users to choose their primary palettes to be "close enough" to the Microsoft palette not to introduce visual artifacts.

[DHowett-MSFT]

I think we are both talking past eachother.

What I mean to say is “this issue, #2661, represents the work required to make sure we can forward the SGR values without using the color as an intermediary.”

When this issue is closed, there will be no dependency on the console host's internal color table and no assumption that a connected terminal will share the same color table.

I really do mean this. When 2661 is closed, there will be no dependency on the color values in the console host’s color table.

I’d like some feedback about how I can communicate that point more effectively 😄

[Undercat]

Oh, I see. The operative notion here is "when the issue is closed", not "when the modifications thus far suggested have been implemented."

My bad. It certainly isn't a pressing issue for me, or anything. I just wanted to make sure you understood that attaching non-SGR-specific attributes to the buffer wasn't going to fix the underlying problem.

Cheers!

[DHowett-MSFT]

(The reason I say “as close to input as possible” is that \e[4m\e[4m would likely be “optimized” to, you know, not be repeated. Same with something like “4;24” or anything that overrides or contradicts itself or stuff like that.)

[Undercat]

Gotcha. Visually-idempotent transformations: no problem. 😸

[j4james]

I've been working around the edges of this issue for a while now, but I've got to the point where I'd like to try and finish off my POC and turn it into a PR. Does anyone have any objections to me doing that? Technically this issue is still assigned to @zadjii-msft so I don't want step on any toes.

Also, any suggestions for testing the 16-color xterm and telnet engines? So far I've just been patching the conhost command line in winconpty.cpp to startup with a different mode, but is there a better way? In the case of telnet, it would be nice to test via the telnet client itself if that were possible.

[DHowett]

@j4james you’re more than welcome to. I view my assignments as some hybrid between “would be the person on my team tasked with doing the work eventually” and “is the subject matter expert, and can answer community questions for the implementor.” In this case, I think you might be the current subject matter expert 😉 so conversely, is it okay if I assign you?

The “best” way to test those engines right now is, unfortunately, exactly what you’re doing. If you’d like to test with the telnet engine and the actual telnet client, it’ll be a bit difficult because the telnet server no longer ships with desktop Windows ... hm.

Our coverage story for these engines is very, very shaky. I don’t even know of a use of the non-24bit color xterm engine anywhere in Windows.

If you replace your system’s conhost.exe with OpenConsole (no warranty!) running Windows executables through WSL will launch ConPTY, but even that just uses the Xterm256Engine.

There might be something fun to be done with VTPipeTerm?

[j4james]

In this case, I think you might be the current subject matter expert 😉 so conversely, is it okay if I assign you?

This part of the code was actually a little outside my comfort zone to start with, but I'm far enough along now that I'm reasonably confident I can get it done, so you're welcome to assign to me. With any luck I should have something ready this weekend. Although I should point out that we'll probably still need to address #5952 before we'll want to merge these fixes.

Our coverage story for these engines is very, very shaky. I don’t even know of a use of the non-24bit color xterm engine anywhere in Windows.

Yeah, I was wondering about that too.

If you replace your system’s conhost.exe with OpenConsole (no warranty!) running Windows executables through WSL will launch ConPTY, but even that just uses the Xterm256Engine.

OK. I might try that with a patched version of OpenConsole with one of the other engines hard coded.

There might be something fun to be done with VTPipeTerm?

I did briefly try that, but couldn't get it to work. It kept randomly dropping escape characters (or something to that effect) so half the VT sequences would get printed to the screen instead of being interpreted.

[zadjii-msft]

(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.)

[ghost]

:tada:This issue was addressed in #6506, which has now been successfully released as Windows Terminal Preview v1.2.2022.0.:tada:

Handy links:

• Release Notes
• Store Download