Introduce a mechanism for passing through DCS data strings #9307

j4james · 2021-02-28T02:04:06Z

This PR introduces a mechanism via which DCS data strings can be passed
through directly to the dispatch method that will be handling them, so
the data can be processed as it is received, rather than being buffered
in the state machine. This also simplifies the way string termination is
handled, so it now more closely matches the behaviour of the original
DEC terminals.

Initial support for DCS sequences was introduced in PR Add initial support for VT DCS sequences #6328.
Handling of DCS (and other) C1 controls was added in PR Preprocess and convert C1 controls to their 7 bit equivalent #7340.
This is a prerequisite for Sixel (Feature Request: sixel graphics support #448) and Soft Font (Support for downloadable "soft fonts" (DRCS) #9164) support.

The way this now works, a DCS sequence is dispatched as soon as the
final character of the VTID is received. Based on that ID, the
OutputStateMachineEngine should forward the call to the corresponding
dispatch method, and its the responsibility of that method to return an
appropriate handler function for the sequence.

From then on, the StateMachine will pass on all of the remaining bytes
in the data string to the handler function. When a data string is
terminated (with CAN, SUB, or ESC), the StateMachine will pass
on one final ESC character to let the handler know that the sequence
is finished. The handler can also end a sequence prematurely by
returning false, and then all remaining data bytes will be ignored.

Note that once a DCS sequence has been dispatched, it's not possible
to abort the data string. Both CAN and SUB are considered valid
forms of termination, and an ESC doesn't necessarily have to be
followed by a \ for the string terminator. This is because the data
string is typically processed as it's received. For example, when
outputting a Sixel image, you wouldn't erase the parts that had already
been displayed if the data string is terminated early.

With this new way of handling the string termination, I was also able to
simplify some of the StateMachine processing, and get rid of a few
states that are no longer necessary. These changes don't apply to the
OSC sequences, though, since we're more likely to want to match the
XTerm behavior for those cases (which requires a valid ST control for
the sequence to be accepted).

Validation Steps Performed

For the unit tests, I've had to make a few changes to some of the
OutputEngineTests to account for the updated StateMachine
processing. I've also added a new StateMachineTest to confirm that the
data strings are correctly passed through to the string handler under
all forms of termination.

To test whether the framework is actually usable, I've been working on
DRCS Soft Font support branched off of this PR, and haven't encountered
any problems. To test the throughput speed, I also hacked together a
basic Sixel parser, and that seemed to perform reasonably well.

Closes #7316

j4james · 2021-02-28T02:51:55Z

I'm leaving this as a draft for now, because it's possible I may find I need to tweak things when I try and use it in different scenarios, but I think it's probably OK as is. If anyone is waiting for this functionality, I'd be happy to make it ready for review.

skyline75489 · 2021-03-05T04:00:03Z

Personally I love to see it land. It opens up all the possibility for DCS sequences. The core devs are kind of busy these days. I'm kindly asking @j4james to have more patience on this one.

zadjii-msft · 2021-03-08T20:35:18Z

FYI we're working on some internal deadlines that's slowing down the works. Thanks always for your patience ☺️

skyline75489 · 2021-03-09T23:25:08Z

src/terminal/parser/IStateMachineEngine.hpp

@@ -20,6 +20,8 @@ namespace Microsoft::Console::VirtualTerminal
    class IStateMachineEngine
    {
    public:
+        using StringHandler = std::function<bool(const wchar_t)>;


I had this question back when I was baking the Sixel implementation. And now I'm trying to make tmux control mode happen, which also uses DCS. The same question emerged: sending the data 1 wchar_t a time might also slow down the parsing process when the DCS sequence is very large, because the handler method was called so frequently that it hurts. Perhaps the soft font feature would not suffer from the performance downgrade, because the length of the sequence is limited?

If your concern is about passing the data over conpty, that's not something I've really tried to cover with this PR. Technically I think it could be done, and for something like tmux cc you could probably buffer a line at a time if that makes a difference, but I don't think it's practical for something like Sixel. I've become more and more convinced that the only way we're going to get some of these more complicated VT operations working reasonably in Windows Terminal is with a rewrite of conpty. The VT data stream really has to be passed through uninterrupted.

I was mainly concerned about the application side, because I'm not using conpty when prototyping tmux cc. After parsing the output from tmux, I choose to send the parsed data over NamedPipe, a nice Win32 feature, to avoid the complicated logic of passing data around. Also using NamedPipe will enable tmux cc under cross-instance scenarios (in the future, after the tab-tearing and all that). However, I think this use case might not be generally feasible for every DCS sequences.

Eventually for a serious implementation, I think I still need to pass all the DCS sequences across the ConPTY layer for WT to handle in a upper layer. Yeah you're probably right about "the VT data stream really has to be passed through uninterrupted". This would save so much effort & improve the overall performance.

I'm still not sure what your concern is regarding the handler. The input is already being processed one character at a time by the state machine, so I wouldn't have thought the overhead of a std::function call would make a significant difference to that process. And what would the alternative be anyway? Buffer the data before passing it through to the handler? That's just another form of overhead - potentially even worse? - and then sequences like Sixel and REGIS wouldn't be able to handle real-time updates.

Maybe I need to hack together a POC of Sixel in conhost to see how well it performs. But I really can't think of a better way of handling this.

My concern is not about using std::function as the mechanism for DCS handler. My concern is mainly about the actual code that handles the DCS data. For example, the sixel parser that parses the sixel data, the network handler that transfers data for tmux cc, etc.

Because the data is sent per wchar_t, the actual code that handles them need to be fast enough, to avoid damaging the performance. But you're right. "Buffering the data before passing it through to the handler" is just another form of overhead. And I myself cannot think of a better way of handling this, either.

Btw, I did eventually hack together a basic Sixel parser branched off of this, and it seemed to perform reasonably well. I was just blitting the resulting image to the screen to test, so obviously there would be more overhead for proper storage and rendering, but the parsing seemed instantaneous. So I don't think the way I've implemented the string handler is going to be a bottleneck.

I mostly agree that the VT parsing has always operated character by character so I don't think this will be the thing to suddenly take it down a notch.

I do know from profiling that we should be looking for more opportunities to bulk dispatch content... but it tends to be elsewhere in the codebase that could use bulk optimization more desperately than here (like the output buffer storage itself) .

We'll cross the bridge when we get to it.

Also I'm excited by

I did eventually hack together a basic Sixel parser branched off of this

j4james · 2021-03-28T23:10:42Z

FYI, I've marked this as ready for review now. I've been using it for a while and haven't felt the need to change anything, so I think it's probably OK. It's not blocking anything for me at the moment, though, so there's no hurry to get it merged.

miniksa · 2021-04-14T17:31:08Z

src/terminal/parser/IStateMachineEngine.hpp

@@ -20,6 +20,8 @@ namespace Microsoft::Console::VirtualTerminal
    class IStateMachineEngine
    {
    public:
+        using StringHandler = std::function<bool(const wchar_t)>;


I mostly agree that the VT parsing has always operated character by character so I don't think this will be the thing to suddenly take it down a notch.

I do know from profiling that we should be looking for more opportunities to bulk dispatch content... but it tends to be elsewhere in the codebase that could use bulk optimization more desperately than here (like the output buffer storage itself) .

We'll cross the bridge when we get to it.

Also I'm excited by

I did eventually hack together a basic Sixel parser branched off of this

src/terminal/parser/OutputStateMachineEngine.cpp

miniksa · 2021-04-14T17:37:26Z

src/terminal/parser/stateMachine.cpp

 // Arguments:
 // - wch - Character to dispatch.
 // Return Value:
 // - <none>
-void StateMachine::_ActionDcsPassThrough(const wchar_t wch)
+void StateMachine::_ActionDcsDispatch(const wchar_t wch)


I love the cleanup in here. I'm fine with fewer states as it's less difficult to mentally juggle.

skyline75489

Excited about the actual usage of DCS sequences 😄

DHowett

Just the case question. Clear for approval otherwise -- just like all your other work, this advances the state of the console significantly. Thank you!

DHowett · 2021-04-28T18:23:51Z

src/terminal/parser/ut_parser/OutputEngineTest.cpp

-        {
-            Log::Comment(L"Escape from DcsTermination");
-            mach._state = StateMachine::VTStates::DcsTermination;
+            mach._dcsStringHandler = [](const auto) { return true; };
            break;
        }
        case 18:


I'm concerned about this -- it looks like we're going from case 16 to 18, but the test data only runs to 17. 😄

Oops. That was a dumb mistake. Thanks for catching that.

ghost · 2021-04-30T19:17:20Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

DHowett

thanks!!

ghost · 2021-05-25T19:09:03Z

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

Handy links:

j4james added 3 commits February 28, 2021 00:48

Introduce a mechanism for handling DCS data strings.

6c0b8e5

Simplify string termination to match DEC terminal behavior.

08c7ed7

Update tests to account for changes in DCS behavior.

a94fbbe

ghost added Area-VT Virtual Terminal sequence support Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase labels Feb 28, 2021

Add test verifying data strings with various terminators.

2cbc54d

j4james force-pushed the dcs-passthrough branch from 454cfc5 to 2cbc54d Compare February 28, 2021 02:31

skyline75489 reviewed Mar 9, 2021

View reviewed changes

j4james marked this pull request as ready for review March 28, 2021 23:04

j4james mentioned this pull request Apr 6, 2021

Support for downloadable "soft fonts" (DRCS) #9164

Closed

miniksa approved these changes Apr 14, 2021

View reviewed changes

skyline75489 approved these changes Apr 15, 2021

View reviewed changes

DHowett requested changes Apr 28, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 28, 2021

Fix incorrect case value in output engine test.

a4a0fa1

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Apr 29, 2021

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 30, 2021

DHowett approved these changes Apr 30, 2021

View reviewed changes

ghost merged commit 2559ed6 into microsoft:main Apr 30, 2021

j4james deleted the dcs-passthrough branch May 6, 2021 14:46

ghost mentioned this pull request May 25, 2021

Send the DCS passthrough sequence to the engine #7316

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a mechanism for passing through DCS data strings #9307

Introduce a mechanism for passing through DCS data strings #9307

j4james commented Feb 28, 2021 •

edited by DHowett

Loading

j4james commented Feb 28, 2021

skyline75489 commented Mar 5, 2021

zadjii-msft commented Mar 8, 2021

skyline75489 Mar 9, 2021

j4james Mar 10, 2021

skyline75489 Mar 10, 2021

skyline75489 Mar 10, 2021 •

edited

Loading

j4james Mar 11, 2021

skyline75489 Mar 11, 2021

j4james Mar 28, 2021

miniksa Apr 14, 2021

j4james commented Mar 28, 2021

miniksa Apr 14, 2021

miniksa Apr 14, 2021

skyline75489 Apr 15, 2021

skyline75489 left a comment

DHowett left a comment

DHowett Apr 28, 2021

j4james Apr 29, 2021

ghost commented Apr 30, 2021

DHowett left a comment

ghost commented May 25, 2021

Introduce a mechanism for passing through DCS data strings #9307

Introduce a mechanism for passing through DCS data strings #9307

Conversation

j4james commented Feb 28, 2021 • edited by DHowett Loading

Validation Steps Performed

j4james commented Feb 28, 2021

skyline75489 commented Mar 5, 2021

zadjii-msft commented Mar 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyline75489 Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j4james commented Mar 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyline75489 left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 30, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

DHowett left a comment

Choose a reason for hiding this comment

ghost commented May 25, 2021

j4james commented Feb 28, 2021 •

edited by DHowett

Loading

skyline75489 Mar 10, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.