Add support for restoring a DECCTR color table report #13139

j4james · 2022-05-22T17:00:01Z

This PR introduces the framework for the DECRSTS sequence which is
used to restore terminal state reports. But to start with, I've just
implemented the DECCTR color table report, which provides a way for
applications to alter the terminal's color scheme.

PR Checklist

Closes Add support for the VT525 restore color table sequence #13132
CLA signed.
Tests added/passed
Documentation updated.
Schema updated.
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

I've added the functions for parsing DEC RGB and HLS color formats into
the Utils class, where we've got all our other color parsing routines,
since this functionality will eventually be needed in other VT protocols
like Sixel and ReGIS.

Since DECRSTS is a DCS sequence, this only works in conhost for now,
or when using the experimental passthrough mode in Windows Terminal.

Validation Steps Performed

I've added a number of unit tests to check that the DECCTR report is
being interpreted as expected. This includes various edge cases (e.g.
omitted and out-of-range parameters), which I have confirmed to match
the color parsing on a real VT240 terminal.

lhecker

I think I realize now, that making VTInt a signed integer might've been a mistake... Thankfully it should be easier to change that since it's just a alias now.

If we need these two new color functions to be performant, we could use a LUT for mapping 0-100 to 0-255. That wouldn't just work for ColorFromRGB100 but also for ColorFromHLS which can be written with integer arithmetic only (without integer division).

j4james · 2022-05-23T22:28:39Z

I think I realize now, that making VTInt a signed integer might've been a mistake...

Why's that? I actually thought a signed int was the right choice. As an example, if you're working with VT coordinates, a typical calculation would be to add or subtract an offset, and then clamp the result within the active margins or buffer range. If that intermediate offset goes negative on a signed int that's not a problem - it's still going to clamp correctly. But if it's unsigned, it's going to wrap, and you're going to end up with the wrong result.

If we need these two new color functions to be performant, we could use a LUT for mapping 0-100 to 0-255.

I like that idea. I am keen to get these functions as fast as possible, mostly for when they're used in sixel. Although HLS colors aren't likely to be used much, so the HLS one is less critical.

That wouldn't just work for ColorFromRGB100 but also for ColorFromHLS which can be written with integer arithmetic only (without integer division).

I've done some initial experiments with a LUT, and it's a big improvement for ColorFromRGB100, but in ColorFromHLS it actually seems somewhat slower. It could just my badly written code, but I went with the floating point implementation in the first place because I found it faster than my integer version, and I suspect the LUT doesn't make up for that loss.

I'll play around some more and see if I can improve things, otherwise I'll just do the LUT for ColorFromRGB100.

lhecker · 2022-05-23T22:46:09Z

Why's that? I actually thought a signed int was the right choice.

It's just... It kind of feels wrong to me at times. For instance your code does std::min(r, 100), which is 100% exactly what I'd do myself. But shouldn't it be std::clamp(r, 0, 100)? Because if r was negative, the computation would result in a negative number and thus in a red value of 255 instead of 0. We know it works, because VTInts are never negative.
So I've been thinking whether it would make sense to make VTInt a uint32_t or maybe even a uint16_t. The former would contractually guarantee that negative numbers can't exist and the latter would even make it possible to freely convert it to both uint32_t or int32_t without any AuditCheck naggings. Hmm... 🤔
But it's not that important really...

I've done some initial experiments with a LUT, and it's a big improvement for ColorFromRGB100, but in ColorFromHLS it actually seems somewhat slower.

Oh nice! I'm glad it works in practice! BTW here's a SO answer with a low-level HLS-RGB routine: https://stackoverflow.com/a/45450486
That post is CC BY-SA 3.0 so we can easily use it here if we choose to do so. I think the last line is supposed to read (r+m)<<16, etc., which makes me question whether the code is correctly tested. But it does seem reasonable.

j4james · 2022-05-24T14:59:52Z

So I've been thinking whether it would make sense to make VTInt a uint32_t or maybe even a uint16_t. The former would contractually guarantee that negative numbers can't exist and the latter would even make it possible to freely convert it to both uint32_t or int32_t without any AuditCheck naggings.

I see your point. My main concern was that we'd find ourselves having to cast to int in a bunch of places, but maybe using uint16_t would make that less of a problem.

BTW here's a SO answer with a low-level HLS-RGB routine: https://stackoverflow.com/a/45450486

They're relying on all the inputs being in the range 0-255, which allows them to do a bunch of bit shifting and masking that isn't applicable in our case. And then there's stuff like using a branch in place of abs which seems slower to me, and the switch in places of the final conditions didn't show any obvious benefit in our code as far as I could tell (it may be better for them because they could use a shift for the division).

So for now I think I'm happy to stick with the current HLS implementation, and just use your LUT idea in the RGB conversion. You're welcome to take a stab at optimising this further after it's merged, although as I said, the HLS routine isn't that critical.

lhecker · 2022-05-24T16:37:12Z

I agree. There's no need to overcomplicate it if it isn't used much. 🙂

BTW, I'm a bit surprised the gsl::at calls inside a noexcept function don't trigger the AuditMode checks. 🤔

j4james · 2022-05-24T18:38:18Z

BTW, I'm a bit surprised the gsl::at calls inside a noexcept function don't trigger the AuditMode checks. 🤔

Oh wow. I've always assumed gsl::at was an unchecked operator[] with the warnings suppressed, in the same vein as gsl::narrow_cast. Otherwise why wouldn't we just use the checked std::array::at method? That at least throws an exception, whereas gsl::at appears to be an app killer. And I guess that's why the audit doesn't complain.

Could I just use an operator[] with a pragma to suppress the warning? I really don't want an unnecessary boundary check there and the optimizer doesn't seem able to avoid it.

DHowett · 2022-05-24T18:41:24Z

Could I just use an operator[] with a pragma to suppress the warning? I really don't want an unnecessary boundary check there and the optimizer doesn't seem able to avoid it.

Eh, honestly that's what til::at is for. It's the "Audit-safe" "I know what I am doing" indexer.

Yes, creating new tools to work around our old tools being poor tools is the name of the game here. 😁 At least til::at's call sites are hand-auditable!

j4james · 2022-05-24T18:44:38Z

Eh, honestly that's what til::at is for. It's the "Audit-safe" "I know what I am doing" indexer.

🤦‍♂️ Doh! That's what I thought I was using! And looking now, there are a bunch of other places in the code where I've used gsl::at which should almost certainly have been til::at. I'm an idiot.

lhecker · 2022-05-24T18:55:06Z

Oh right, gsl::at just std::terminates the application, so that's why the AuditMode checks don't fail... For some reason I thought it's throwing exceptions. 😅

j4james · 2022-05-24T19:18:03Z

OK, I think this is it. I've switched to til::at, and I'm also using std::min<unsigned> to make absolutely sure the parameters will be in range in the unlikely event that someone passes in a negative number.

carlos-zamora

LOVE IT! Thanks!

carlos-zamora · 2022-06-01T20:37:34Z

@msftbot make sure @DHowett signs off on this

ghost · 2022-06-01T20:37:39Z

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @DHowett

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

DHowett

Lovely, as always. Thank you!

DHowett · 2022-06-03T01:49:45Z

src/types/utils.cpp

+    // The color class is expecting components in the range 0 to 255,
+    // so we need to scale our percentage values by 255/100. We can
+    // optimise this conversion with a pre-created lookup table.
+    static constexpr auto scale100To255 = [] {


I love that this is compiled-in :D

DHowett · 2022-06-03T01:50:44Z

src/types/utils.cpp

+// - s - The saturation component of the color (0-100%).
+// Return Value:
+// - The color defined by the given components.
+til::color Utils::ColorFromHLS(const int h, const int l, const int s) noexcept


Eventually, I will file a workitem for moving this stuff into til/color.h itself (not necessarily into the color class, mind). We have about three copies of the HSL/HLS code, and perhaps we can get it down to 1. 😄

Yeah, I'm totally on board with this. I was initially expecting to put it somewhere related to the til::color class - I only went with Utils because that's what we were doing for the XTerm color routines. But as you noted below, these conversions are somewhat unique, and would probably need to be named something like ColorFromDecRGB and ColorFromDecHLS.

DHowett · 2022-06-03T01:51:33Z

src/types/utils.cpp

+
+    // Finally we order the components based on the given hue. But note that the
+    // DEC terminals used a different mapping for hue than is typical for modern
+    // color models. Blue is at 0°, red is at 120°, and green is at 240°.


ah, is this function more like ColorFromDecHLS? Perhaps we should not generalize it... 😄

## Summary of the Pull Request Up to now we haven't supported passing `DCS` sequences over conpty, so any `DCS` operations would just be ignored in Windows Terminal. This PR introduces a mechanism whereby we can selectively pass through operations that could reasonably be handled by the connected terminal without interfering with the regular conpty renderer. For now this is just used to support restoring the `DECCTR` color table report. ## References Support for `DECCTR` was originally added to conhost in PR #13139. ## PR Checklist * [x] Closes #13223 * [x] CLA signed. * [x] Tests added/passed * [ ] Documentation updated. * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments The way this works is we have a helper method in `AdaptDispatch` that `DCS` operations can use to create a passthrough `StringHandler` for the incoming data instead of their usual handler. To make this passthrough process more efficient, the handler buffers the data before forwarding it to conpty. However, it's important that we aren't holding back data if output is paused before the string terminator, so we need to flush the buffer whenever we reach the end of the current write operation. This is achieved by querying a new method in the `StateMachine` that lets us know if we're currently dealing with the last character. Another issue that came up was with the way the `StateMachine` caches sequences that it might later need to forward to conpty. In the case of string sequences like `DCS`, we don't want the actual string data cached here, because that will just waste memory, and can also result in the data being mistakenly output. So I've now disabled that caching when we're in any of the string processing states. ## Validation Steps Performed I've manually confirmed that the `DECCTR` sequence can now update the color table in Windows Terminal. I've also added a new unit test in `ConptyRoundtripTests` to verify the sequence is being passed through successfully.

ghost · 2022-07-06T18:08:45Z

🎉Windows Terminal Preview v1.15.186 has been released which incorporates this pull request.:tada:

Handy links:

This PR introduces the framework for the `DECRQTSR` sequence which is used to query terminal state reports. But for now I've just implemented the `DECCTR` color table report, which provides a way for applications to query the terminal's color scheme. ## References and Relevant Issues This is the counterpart to the the `DECRSTS` sequence, which is used to restore a color table report. That was implemented in PR #13139, but it only became practical to report the color table once conpty passthrough was added in PR #17510. ## Detailed Description of the Pull Request / Additional comments This sequence has the option of reporting the colors as either HLS or RGB, but in both cases the resolution is lower than 24 bits, so the colors won't necessarily round-trip exactly when saving and restoring. The HLS model in particular can accumulate rounding errors over time. ## Validation Steps Performed I've added a basic unit test that confirms the colors are reported as expected for both color models. The color values in these tests were obtained from color reports on a real VT525 terminal. ## PR Checklist - [x] Tests added/passed

j4james added 5 commits May 22, 2022 13:48

Add color conversion routines.

7da4d8d

Hook up the DECRSTS sequence to the dispatch class.

f88e198

Add a handler for restoring the color table.

e7a1b1c

Add some unit tests.

a44a1da

Add terms to spelling bot's dictionary.

8042cf8

ghost added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label May 22, 2022

j4james marked this pull request as ready for review May 22, 2022 18:07

lhecker approved these changes May 23, 2022

View reviewed changes

j4james added 2 commits May 24, 2022 14:13

Use a lookup table for ColorFromRGB100 conversion.

1c21fe9

Add DECRSTS to spelling bot's dictionary.

3577283

Use til::at in place of gsl::at.

d778ff2

carlos-zamora approved these changes Jun 1, 2022

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 1, 2022

DHowett approved these changes Jun 3, 2022

View reviewed changes

ghost merged commit c157f63 into microsoft:main Jun 3, 2022

j4james mentioned this pull request Jun 4, 2022

Restore the DECCTR color table report over conpty #13227

Merged

6 tasks

ghost mentioned this pull request Jul 6, 2022

Add support for the VT525 restore color table sequence #13132

Closed

j4james deleted the feature-decctr branch August 31, 2022 00:43

j4james mentioned this pull request Aug 13, 2024

Add support for querying the DECCTR color table report #17708

Merged

1 task

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for restoring a DECCTR color table report #13139

Add support for restoring a DECCTR color table report #13139

j4james commented May 22, 2022 •

edited by DHowett

Loading

lhecker left a comment

j4james commented May 23, 2022

lhecker commented May 23, 2022 •

edited

Loading

j4james commented May 24, 2022

lhecker commented May 24, 2022

j4james commented May 24, 2022

DHowett commented May 24, 2022

j4james commented May 24, 2022

lhecker commented May 24, 2022

j4james commented May 24, 2022 •

edited

Loading

carlos-zamora left a comment

carlos-zamora commented Jun 1, 2022

ghost commented Jun 1, 2022

DHowett left a comment

DHowett Jun 3, 2022

DHowett Jun 3, 2022

j4james Jun 3, 2022

DHowett Jun 3, 2022

ghost commented Jul 6, 2022

Add support for restoring a DECCTR color table report #13139

Add support for restoring a DECCTR color table report #13139

Conversation

j4james commented May 22, 2022 • edited by DHowett Loading

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

lhecker left a comment

Choose a reason for hiding this comment

j4james commented May 23, 2022

lhecker commented May 23, 2022 • edited Loading

j4james commented May 24, 2022

lhecker commented May 24, 2022

j4james commented May 24, 2022

DHowett commented May 24, 2022

j4james commented May 24, 2022

lhecker commented May 24, 2022

j4james commented May 24, 2022 • edited Loading

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora commented Jun 1, 2022

ghost commented Jun 1, 2022

DHowett left a comment

Choose a reason for hiding this comment

DHowett Jun 3, 2022

Choose a reason for hiding this comment

DHowett Jun 3, 2022

Choose a reason for hiding this comment

j4james Jun 3, 2022

Choose a reason for hiding this comment

DHowett Jun 3, 2022

Choose a reason for hiding this comment

ghost commented Jul 6, 2022

j4james commented May 22, 2022 •

edited by DHowett

Loading

lhecker commented May 23, 2022 •

edited

Loading

j4james commented May 24, 2022 •

edited

Loading