Skip to content

Incorrect colors returned by the ReadConsoleOutputAttribute API #5940

Closed

Description

Environment

Windows build number: Version 10.0.18362.719

Steps to reproduce

Compile and run the following C++ code.

#include <windows.h>
#include <conio.h>
#include <stdio.h>

void main()
{
  constexpr auto numColors = 24;
  HANDLE handle = GetStdHandle(STD_OUTPUT_HANDLE);

  // Enable VT mode.
  DWORD mode;
  GetConsoleMode(handle, &mode);
  mode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
  SetConsoleMode(handle, mode);

  printf("\033[m\033[H\033[2J\033[3J"); // clear screen and scrollback
  for (auto i = 0; i < numColors; i++) {
    auto c = i*10 + 8;
    printf("\033[48;2;%d;%d;%dm ", c, c, c);
  }
  printf("\033[m <- Colors produced with VT sequences\n");

  CHAR_INFO info[numColors];
  SMALL_RECT region = { 0, 0, numColors, 1 };
  ReadConsoleOutput(handle, info, { numColors, 1 }, { 0, 0 }, &region);

  for (auto i = 0; i < numColors; i++) {
    SetConsoleTextAttribute(handle, info[i].Attributes);
    printf(" ");
  }
  printf("\033[m <- Colors seen by ReadConsoleOutput\n");

  WORD attr[numColors];
  DWORD read;
  ReadConsoleOutputAttribute(handle, attr, numColors, { 0, 0 }, &read);

  for (auto i = 0; i < numColors; i++) {
    SetConsoleTextAttribute(handle, attr[i]);
    printf(" ");
  }
  printf("\033[m <- Colors seen by ReadConsoleOutputAttribute\n");
}

This test case writes out a line of spaces in varying shades of gray using VT sequences to set the colors with RGB values. It then reads back those colors as legacy attributes using the ReadConsoleOutput and ReadConsoleOutputAttribute APIs, and writes out what they see.

Expected behavior

The two lines rendered with legacy attributes should at least approximate the RGB shades of gray.

Actual behavior

The ReadConsoleOutput API produces reasonable results (given the limitations of the 16 color palette), but the colors returned by the ReadConsoleOutputAttribute API are quite obviously wrong.

image

The problem is that ReadConsoleOutputAttribute is using the TextAttribute::GetLegacyAttributes method, which only works correctly for the first 16 indexed colors. ReadConsoleOutput uses the Settings::GenerateLegacyAttributes method, which produces decent results, although it can be quite slow.

I'd like to have a go at refactoring this code for a number of reasons:

  1. We obviously should be using a consistent legacy color calculation, that produces reasonable results, for all the console APIs.
  2. I think there are a few places in the code that are using TextAttribute::GetLegacyAttributes that don't actually need legacy attributes at all, and could avoid the color narrowing.
  3. I think the GenerateLegacyAttributes implementation could be made faster with a simple table lookup, instead of the current search for the closest HSL match in the color table (I suspect this might be responsible for issues like Performance regression for Conhostv2 wrapped by ConEmu #783 and vim slower to open/close in conhost v2 compared to v1 #3950).

Activity

  1. ghost added
    Needs-TriageIt's a new issue that the core contributor team needs to triage at the next triage meeting
    on May 16, 2020
  2. ghost added
    Needs-Tag-FixDoesn't match tag requirements
    on May 16, 2020
  3. DHowett commented on May 18, 2020

    @DHowett
    Member

    Wow, that's ugly. Thanks!

    You're welcome to do any/all of the things you've proposed. I do not believe we ever made a compatibility contract covering exactly how RGB values get mangled when palettized.

  4. added
    Area-ServerDown in the muck of API call servicing, interprocess communication, eventing, etc.
    Help WantedWe encourage anyone to jump in on these.
    Issue-BugIt either shouldn't be doing this or needs an investigation.
    Product-ConhostFor issues in the Console codebase
    and removed
    Needs-TriageIt's a new issue that the core contributor team needs to triage at the next triage meeting
    on May 18, 2020
  5. ghost removed
    Needs-Tag-FixDoesn't match tag requirements
    on May 18, 2020
  6. ghost added
    In-PRThis issue has a related PR
    on Jun 4, 2020
  7. ghost closed this as completedin #6358on Jun 8, 2020
  8. ghost removed
    In-PRThis issue has a related PR
    on Jun 8, 2020
  9. 2 remaining items

  10. added a commit that references this issue on Sep 5, 2020
  11. vefatica commented on Dec 11, 2020

    @vefatica

    Could this be affecting the current mainstream Windows 10 console (conhost.exe)? I have a VBEEP (visual beep) routine that has worked through a few versions of Windows (back to XP, I believe). It's not working correctly with the current Windows 10. I save the attributes with ReadConsoleOutputAttribute and restore them with WriteConsoleOutputAttribute; they're not restored correctly.

    The routine works fine in WindowsTerminalPreview_1.5.3242.0_x64.

    Thanks.

  12. DHowett commented on Dec 11, 2020

    @DHowett
    Member

    Almost certainly. The code in this repository flows back into Windows, but there hasn’t been a GA release of conhost (to wit: of Windows) in some time.

    With time, all of the fixes in Terminal make it back to conhost.

  13. vefatica commented on Dec 11, 2020

    @vefatica

    Thanks, Dustin. I poked around more and found what my problem is. When text is written with reverse color ESC[7m, ReadConsoleOutputAttribute gets COMMON_LVB_REVERSE_VIDEO (0x4000). Apparently WriteConsoleOutputAttribute knows nothing of COMMON_LVB_REVERSE_VIDEO and gives you just the low byte (black on black). Will this change (someday)?

    I read a disheartening comment on the WriteConsoleOutputAttribute help page. [Of virtual termiinal scenarios ...]

    There is no equivalent to apply color to an area without also emitting text.

    That's not exactly true. WriteConsoleOutputAttribute works as always if you stay away from the new-fangled attributes.

  14. DHowett commented on Dec 11, 2020

    @DHowett
    Member

    In general, combining VT with the traditional APIs is not the key to a winning strategy. Those are locked to their current representations for compatibility reasons[1] and we aren’t introducing new ones. Applications should, by and large, have evolved beyond the need for conhost to store information on their behalf and act as anything more than a pipe.

    1: as an example, we tried to make grid line drawing available to all applications. In so doing, we learned that some versions of the Delphi runtime were using the high bits of the console attributes for storage and those were the same bits that controlled which lines to draw. The result was, predictably[2], random lines all over the screen.

    2: predictable only if you know the ending already

  15. vefatica commented on Dec 11, 2020

    @vefatica

    Why don't I have the problem of my last post when I run my visual beep in Windows Terminal?

  16. zadjii-msft commented on Dec 11, 2020

    @zadjii-msft
    Member

    While the inbox console hasn't been updated in some time, the Terminal ships a side-by-side conhost.exe that has been updated. So the Terminal doesn't have to wait for console bug fixes to flow back into the OS, then back out to Insiders (and GA), it gets bug fixes whenever it ships. Pretty neat!

  17. vefatica commented on Dec 11, 2020

    @vefatica

    I see what's happening in Terminal. If normal attributes are 0x07, using ESC[7m (reverse) and ReadConsoleOutputAttribute I get 0x4007 and it comes back the same if I use that with WriteConsoleOutputAttribute. Is that how COMMON_LVB_REVERSE_VIDEO is intended to work (reverse the colors specified in the low byte)? Can I find documentation of these things anywhere, even in the source?

    I'm not an insider (was for a while) but I've heard one recent change has made it into the insider build, namely tab-stop settings surviving leaving/entering VT output mode.

  18. DHowett commented on Dec 11, 2020

    @DHowett
    Member

    The behavior of COMMON_LVB_REVERSE_VIDEO is documented, but the implication is subtle. It says that it will "reverse [the] foreground and background attribute". This suggests that it will "reverse the colors specified in the low byte".

    one recent change

    Tabs? This was actually, like, the least recent change that made it to inbox. Hundreds of changes on top of that have also made it into inbox.

  19. vefatica commented on Dec 11, 2020

    @vefatica

    What is "inbox"? What is "GA"?

    I saw that description of COMMON_LVB_REVERSE_VIDEO. But looking at how it actually wotks in the current version of Windows I would not have guessed that it had anything to do with the low byte.

  20. DHowett commented on Dec 11, 2020

    @DHowett
    Member

    Sorry, "inbox" is "in windows" (at all) and "GA" is "General Availability", which we use to refer to "The version of Windows that everybody with a supported device can update to without joining any weird programs."

    Since the low byte IS the foreground and background (if you look at the definitions of the color attributes, they occupy the bottom and top four bits of the first byte), these are analogous.

  21. vefatica commented on Dec 11, 2020

    @vefatica

    Since the behavior of the GA console does not conform to more modern standards, may I safely assume that the behavior will (someday) change?

  22. DHowett commented on Dec 11, 2020

    @DHowett
    Member

    Everything in this repository eventually becomes part of Windows. Almost everything that becomes part of Windows eventually gets released (from my team, everything that becomes part of Windows gets released). The behavior of the console available in the current public non-insider build of Windows is representative of a point in time (to wit: about a year ago, maybe more).

    If it's better today in the Terminal, it will be better tomorrow[*] in Windows itself. There's a high, high chance that the fix you're asking about is already part of an insider build. 😄

    * not literally tomorrow.

  23. vefatica commented on Dec 11, 2020

    @vefatica

    There's a high, high chance that the fix you're asking about is already part of an insider build.

    I think you're right. An associate who is a fast-track insider does not have the difficulty that I have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Area-ServerDown in the muck of API call servicing, interprocess communication, eventing, etc.Help WantedWe encourage anyone to jump in on these.Impact-CorrectnessIt be wrong.Issue-BugIt either shouldn't be doing this or needs an investigation.Priority-2A description (P2)Product-ConhostFor issues in the Console codebaseResolution-Fix-CommittedFix is checked in, but it might be 3-4 weeks until a release.

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions