Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send the DCS passthrough sequence to the engine #7316

Closed
skyline75489 opened this issue Aug 17, 2020 · 9 comments · Fixed by #9307
Closed

Send the DCS passthrough sequence to the engine #7316

skyline75489 opened this issue Aug 17, 2020 · 9 comments · Fixed by #9307
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@skyline75489
Copy link
Collaborator

Description of the new feature/enhancement

This is a followup of #6328 . Right now VT DCS sequences are recoginzed and sliently ignored. This is about actually doing something about it.

See also #120 #448 .

@skyline75489 skyline75489 added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Aug 17, 2020
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 17, 2020
@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Product-Conhost For issues in the Console codebase labels Aug 17, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 17, 2020
@zadjii-msft
Copy link
Member

I'm yanking triage off this one because yea duh we should do this. Thanks for filing!

@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 17, 2020
@zadjii-msft zadjii-msft added this to the Console Backlog milestone Aug 17, 2020
@skyline75489
Copy link
Collaborator Author

I've beening trying to make this work like what we did with OSC, that is collecting all the pass through character into a string named _dcsDataString and send it to engine when the DCS sequences terminates. The implementation is straight forward:

void StateMachine::_ActionDcsPassThrough(const wchar_t wch)
{
    _trace.TraceOnAction(L"DcsPassThrough");
    _trace.TraceOnExecute(wch);
    if (!_isDcsPassingThrough)
    {
        // The first character being passed through is the "final character", which combined with intermediates
        // defines the functionality of the DCS sequence. Here we treat it as part of the identifier.
        _identifier.AddIntermediate(wch);
        _isDcsPassingThrough = true;
    }
    else
    {
        _dcsDataString.push_back(wch);
    }
}
void StateMachine::_EventDcsTermination(const wchar_t wch)
{
    _trace.TraceOnEvent(L"DcsTermination");

    if (_isStringTerminatorIndicator(wch))
    {

        const auto success = _engine->ActionDcsDispatch(_identifier.Finalize(),
                                                        { _parameters.data(), _parameters.size() },
                                                        _dcsDataString);
        if (!success)
        {
            TermTelemetry::Instance().LogFailed(wch);
        }

        _EnterGround();
    }
    else
    {
        _EnterEscape();
        _EventEscape(wch);
    }
}

But when dealing with large sixel image outputs, the string collecting itself takes seconds to complete. I'm thinking maybe a direct pass through to the engine should help with the performance.

@skyline75489
Copy link
Collaborator Author

Note from @j4james originally written in #7578 (comment)

My assumption was that the parsing for the DCS command string would be handled in the dispatch classes - I don't think the state machine should need to know about all the different sequence formats. Once it's identified the operation from the final character, I think it should be streaming the command string straight through to the adapter so it can be processed immediately as the data arrives.

In the case of Sixel graphics, this would allow the adapter to render the image straight to the screen as the pixels are received. I think there are other DCS sequences, like music operations, that would have similar requirements. And yes, OSC might fall into this category too.

For the majority of simple operations that don't require streaming, though, there could be a default handler that just gathers everything up in a string, and passes that along once the string terminator is received.

@j4james
Copy link
Collaborator

j4james commented Sep 15, 2020

So what I had in mind was that the state machine would dispatch any of the command string sequences (DCS, APC, etc.) as soon as it received the "final" character and could generate a full VTID. The dispatch method would not be able to process the command at that point, but based on the id it would return an instance of a consumer class. The state machine would save a copy of that, and pass on every subsequent character to that consumer until it received a string terminator, or some kind of error/abort state. At that point it would notify the consumer that it had reached the end of message and the command could be processed (or aborted).

If the dispatcher didn't recognize the VTID, it could return a kind of null consumer that would just ignore everything without wasting memory collecting bytes that were never going to be used. If it was a simple operation that just wanted the final string, it could return a generic consumer that would handle all the data collection automatically (similar to what is currently done with OSC), and then trigger a specified callback function/lambda with the final content. But for something like Sixel, which needs full control of the data, it would return a custom consumer class that could process each character as it was received.

OSC could potentially also follow this pattern, although the initial dispatching is more complicated because of the way the commands are identified. So perhaps that's something that's best left for a follow-up PR, once we've got the basic concept working. Another reason being it's going require a bunch of refactoring of all the existing OSC sequences, since the command string parsing would now be in the dispatch class rather than the state machine.

@j4james
Copy link
Collaborator

j4james commented Oct 10, 2020

@skyline75489 Just as a follow up to your comment on the Sixel issue, I want to be clear that this DCS pass through thing is not something I'm currently working on, in case you were waiting on me. If nobody else takes it on, I will likely get to it eventually, because there are a whole lot of DCS operations I'd love to implement. But for now I've got other issues I want to tackle first. I also figured you're probably the best person to handle this at the moment, because the Sixel requirements will probably be key to shaping the API.

@skyline75489
Copy link
Collaborator Author

skyline75489 commented Oct 11, 2020

@j4james I hear you loud and clear. Thanks for inspiring comments both in this thread and in #448 . I was stuck with my unfinished sixel implementation about a month ago, so I moved on to those OSC things. I think I'll give DCS/sixel another try when I have time.

@j4james
Copy link
Collaborator

j4james commented Feb 18, 2021

@skyline75489 If you aren't working on this, I'd be happy to take it on now. I've started working on the "soft fonts" feature (issue #9164) which has similar DCS requirements to Sixel, so I thought it would make a good test case for this.

For the "consumer class" that I mentioned in my initial proposal above, I realised we could probably get by with a simple std::function<bool(wchar_t)>, and then the dispatch handler could just return a lambda without necessarily requiring a separate class.

The way it would work, the state machine would just pass through all the characters from the DCS string to the returned function, and could indicate the end of the sequence by sending through an ST character. If we need to differentiate an abort from a successful termination, we could also send through a CAN control character, but I'm not sure that's really necessary.

@skyline75489
Copy link
Collaborator Author

skyline75489 commented Feb 18, 2021 via email

@ghost ghost added the In-PR This issue has a related PR label Feb 28, 2021
@ghost ghost closed this as completed in #9307 Apr 30, 2021
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Apr 30, 2021
ghost pushed a commit that referenced this issue Apr 30, 2021
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 #6328.
* Handling of DCS (and other) C1 controls was added in PR #7340.
* This is a prerequisite for Sixel (#448) and Soft Font (#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
@ghost
Copy link

ghost commented May 25, 2021

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

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants