SetConsoleScreenBufferInfoEx color palette behavior broken

[PhMajerus]

19H1 has many improvements concerning console colors, but introduced a new bug. (@zadjii-msft is gonna hate me, sorry)
Testing on 19H1 build 18362.1 and comparing with 1809 build 17763.402.

The console was designed to work like a CGA text screen, with modifiable palette like on VGA.
Whenever the palette is changed, the characters already on screen are displayed using the updated palette on its next refresh cycle,
This was how it used to work until Win10 1809...
The new console code doesn't refresh all displayed cells when SetConsoleScreenBufferInfoEx is called.
This is a breaking compatibility change in 19H1, and an undesirable one as some apps might take advantage of the ability to change the palette without having to redraw the whole text screen to change displayed colors. (like how VGA games used colors cycling).

See the following sample code, it outputs text in color # 7 over a background of color # 0.
Since both lines it outputs are using the same colors indexes, they should be shown in the same color, changing the palette applies to the whole screen, including any existing output.

On 19H1, the console screen isn't updated with the new colors until the user either goes into console settings and clicks OK, or tries to resize the window even just a bit, or even just minimizes and restores the window, forcing a repaint.
This is clearly a bug, as minimizing and restoring the window would not change the colors if it was a new intended behavior.

int main()
{
	HANDLE hStdOut = GetStdHandle(STD_OUTPUT_HANDLE);

	CONSOLE_SCREEN_BUFFER_INFOEX csbiex;
	ZeroMemory(&csbiex, sizeof(CONSOLE_SCREEN_BUFFER_INFOEX));
	csbiex.cbSize = sizeof(CONSOLE_SCREEN_BUFFER_INFOEX);
	GetConsoleScreenBufferInfoEx(hStdOut, &csbiex);
	// Make sure we're using color#0 as background and #7 as foreground
	csbiex.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED;

	// Set blue colors scheme.
	csbiex.ColorTable[0] = RGB(128, 0, 0);
	csbiex.ColorTable[7] = RGB(255, 0, 0);
	SetConsoleScreenBufferInfoEx(hStdOut, &csbiex);
	LPCTSTR pszRedString = TEXT("Bright red on dark red\r\n");
	WriteConsole(hStdOut, pszRedString, _tcslen(pszRedString), NULL, NULL);

	// Set red colors scheme.
	csbiex.ColorTable[0] = RGB(0, 0, 128);
	csbiex.ColorTable[7] = RGB(0, 0, 255);
	SetConsoleScreenBufferInfoEx(hStdOut, &csbiex);
	// At this point, the "Bright red on dark red", as well as all background,
	// for the whole screen should have been turned to blue.
	LPCTSTR pszBlueString = TEXT("Bright blue on dark blue\r\n");
	WriteConsole(hStdOut, pszBlueString, _tcslen(pszBlueString), NULL, NULL);

	getchar();
	return 0;
}

Screen on 1809 and below :

Same on 19H1 (18362.1) :

[miniksa]

Yeah this looks like it's messed up. I've filed MSFT: 20990369 onto @zadjii-msft internally. He's out this week, so hopefully we can get him to look next week when he gets back.

[zadjii-msft]

i cri evrytiem

Eventually, I'm going to stop trying to add color features, and everything will just work as it should, for all scenarios. But 19H1 is not that release.

I'll take a look at this later in <insert next release codename>. Thanks for the detailed writeup, as always!

[PhMajerus]

"Eventually, I'm going to stop trying to add color features, and everything will just work as it should, for all scenarios."
Don't lose hope, I've been testing the VT colors extensively and the issues really got reduced with the new implementation, you're almost there.
I'm patiently waiting for that time all the colors stuff works as well, so you can start working on sixel graphics and run into all kind of new problems with the characters cells assumptions in the codebase 😄

About this palette issue, since it seems you properly use the new palette when the screen requires a refresh, how about an InvalidateRect (hWnd, NULL, FALSE) at the end of SetConsoleScreenBufferInfoEx if the palette has changed to trigger a WM_PAINT ?

[miniksa]

@PhMajerus, there is actually an Invalidate All being triggered when the colors are changing. The fact that it is not working quite right is what's making @zadjii-msft cry. It's a tangled web. 😋

[oising]

I'm patiently waiting for that time all the colors stuff works as well, so you can start working on sixel graphics and run into all kind of new problems with the characters cells assumptions in the codebase 😄

SIXEL YASSSSSS

Seriously though, I think that's not going to land with conhost v2. v3 perhaps? :D

[j4james]

As far I can see, this is just missing a call to IRenderTarget::TriggerRedrawAll. There is a redraw trigger in the SCREEN_INFORMATION::SetDefaultAttributes method, but that only happens if the default attributes are changed, which isn't the case here.

So to make sure the redraw is always triggered, we'd need another explicit TriggerRedrawAll call in SetConsoleScreenBufferInfoExImpl, some point after the color table is set. That seems to fix the issue for me.

That said, if the redraw is expensive, you may want to be a bit smarter about it, and only refresh if the color table has actually changed.

[PhMajerus]

@miniksa and @zadjii-msft, do you have any news on this? It's been almost a year and the bug is still present in the latest 20H1 insider build from last week.

Since it seems the conhost side is only updated with feature updates, missing the 20H1 release would make us have to keep this bug for another 6 months.

[zadjii-msft]

Oh yea we stopped work on 20H1 months ago. Unfortunately this was missed since we didn't have it attached to that milestone 😞. This bug looks like it unfortunately was filed just before the Terminal was announced, so it awkwardly missed the transition to our new bug tracking here in the public.

[PhMajerus]

@zadjii-msft and @miniksa, did you have a fix for this or is it still on the backlog?
The issue is still present in the current Windows 10 Insider Preview Dev Channel, build 20175.1000

Just bumping this since it's probably something that can only get fixed in an OS release and we'll be moving to a yearly release soon...

[zadjii-msft]

We don't have any updates on this one. It might have just coincidentally gotten fixed in recent weeks, there was a lot of churn in this area recently. I don't think "September" is necessarily right for the 21H1 milestone, so don't worry about that.

[zadjii-msft]

I just went to go and try and fix this, and this looks fine on main, and at least whatever version of conhost is on my 10.0.22563.1001 build. We must have fixed this one along the way! Sorry I don't have a more specific fix to point at 😕

[j4james]

@zadjii-msft I don't think the underlying issue is actually fixed. There is just a bug in the test case which is now hiding the problem.

One of the weird "features" of the Get/SetConsoleScreenBufferInfoEx APIs, is that the one uses exclusive coordinates while the other is inclusive. If you call GetConsoleScreenBufferInfoEx, and then pass the resulting info back to SetConsoleScreenBufferInfoEx, it's actually going to resize the viewport (which in this case is enough to trigger a full screen paint and force the updated palette to render).

Try adding the following lines after the GetConsoleScreenBufferInfoEx call:

csbiex.srWindow.Right += 1;
csbiex.srWindow.Bottom += 1;

That should prevent the resize, and you'll then see that the screen is still not being repainted correctly. It's not the same as it was before, but it's still not correct.

[zadjii-msft]

WOW okay that is wild. I added some sleeps in there to see what was going on. If you start making a selection after the red color was set, you can see how wild this is.

int __cdecl wmain(int /*argc*/, WCHAR* /*argv[]*/)
{
    HANDLE hStdOut = GetStdHandle(STD_OUTPUT_HANDLE);

    CONSOLE_SCREEN_BUFFER_INFOEX csbiex;
    ZeroMemory(&csbiex, sizeof(CONSOLE_SCREEN_BUFFER_INFOEX));
    csbiex.cbSize = sizeof(CONSOLE_SCREEN_BUFFER_INFOEX);
    GetConsoleScreenBufferInfoEx(hStdOut, &csbiex);
    // Make sure we're using color#0 as background and #7 as foreground
    csbiex.wAttributes = FOREGROUND_BLUE | FOREGROUND_GREEN | FOREGROUND_RED;

    csbiex.srWindow.Right += 1;
    csbiex.srWindow.Bottom += 1;

    // Set blue colors scheme.
    csbiex.ColorTable[0] = RGB(128, 0, 0);
    csbiex.ColorTable[7] = RGB(255, 0, 0);
    SetConsoleScreenBufferInfoEx(hStdOut, &csbiex);

    Sleep(1000);

    LPCWSTR pszRedString = TEXT("Bright red on dark red\r\n");
    WriteConsole(hStdOut, pszRedString, lstrlenW(pszRedString), NULL, NULL);

    Sleep(1000);

    // Set red colors scheme.
    csbiex.ColorTable[0] = RGB(0, 0, 128);
    csbiex.ColorTable[7] = RGB(0, 0, 255);
    SetConsoleScreenBufferInfoEx(hStdOut, &csbiex);

    Sleep(1000);

    // At this point, the "Bright red on dark red", as well as all background,
    // for the whole screen should have been turned to blue.
    LPCWSTR pszBlueString = TEXT("Bright blue on dark blue\r\n");
    WriteConsole(hStdOut, pszBlueString, lstrlenW(pszBlueString), NULL, NULL);

    Sleep(2000);
    return 0;
}

Yea this is still happening. Should be an easy fix. Sorry for being too quick on the trigger 😅