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

"Chunked" soft fonts are no longer working correctly #16079

Closed
j4james opened this issue Oct 1, 2023 · 4 comments · Fixed by #16349
Closed

"Chunked" soft fonts are no longer working correctly #16079

j4james opened this issue Oct 1, 2023 · 4 comments · Fixed by #16349
Assignees
Labels
Area-VT Virtual Terminal sequence support In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty

Comments

@j4james
Copy link
Collaborator

j4james commented Oct 1, 2023

Windows Terminal version

1.19.2682.0

Windows build number

10.0.19045.3448

Other Software

No response

Steps to reproduce

  1. Download the VT220 soft font from this gist.
  2. Open a cmd shell in Windows Terminal.
  3. TYPE the soft font file downloaded in step 1.

Expected Behavior

The next prompt that is output should be rendered with that low-res VT220 font, looking something like this:

image

Actual Behavior

It appears that some of the characters are only partially downloaded, and some haven't been downloaded at all, so I'm seeing this:

image

If you look at the contents of the font file, you'll see that it's defined as a series of DECDLD sequences, so it's downloading 24 sets of 4 characters, rather than a single 94 character set. I think that's part of the reason for the failure, because fonts that aren't chunked like that don't seem to have the problem.

I also noticed that it only happens in a cmd shell, but not in PowerShell or a WSL bash shell. My guess is that the other shells are buffering their output in a way that hides the problem. It's also possible there is a timing factor, but the cmd shell failure is easily reproducible for me.

And as far as I can tell, the problem was introduced in PR #15991. The commit prior to 41f7ed7 was working as expected.

@j4james j4james added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Oct 1, 2023
@j4james
Copy link
Collaborator Author

j4james commented Oct 1, 2023

I think I know what the problem is. The new "cork" algorithm causes the VT renderer to flush when the end of the output buffer is reached. This assumes that the VT output is in a ground state, but that's not the case when we're processing a DCS sequence and are in the middle of passing it through to the conpty client. So what's happening is we're flushing a mode ?25 cursor update in the middle of the DCS stream, and that ends the stream early (you can see this in the debug tap).

In theory this could potentially affect all soft fonts, but I think it's more easily triggered by the "chunked" examples I have because they have a line break after each chunk, and the cmd shell's TYPE command probably flushes its output after every line break. Fonts that are written out in a single block will likely only be affected if they're over a certain size. DECCTR palette updates might be affected too, because they also rely on DCS passthrough.

@zadjii-msft zadjii-msft added Product-Conpty For console issues specifically related to conpty Area-VT Virtual Terminal sequence support labels Oct 2, 2023
@zadjii-msft zadjii-msft added this to the Terminal v1.20 milestone Oct 2, 2023
@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 Oct 4, 2023
@lhecker lhecker self-assigned this Nov 16, 2023
@lhecker
Copy link
Member

lhecker commented Nov 16, 2023

I can't figure out how this worked before... VtEngine is still bound into the ~60 "FPS" rendering loop, so why didn't this still randomly happen previously?

@j4james
Copy link
Collaborator Author

j4james commented Nov 16, 2023

I assumed there was a lock of some sort that prevented the VtEngine render thread from doing anything while the state machine was busy processing output. If that's not the case then maybe it was a matter of luck that it worked before, and PR #15991 just made the issue more noticeable. But I wasn't able to reproduce the issue prior to that commit, and it happens all the time now.

@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Nov 21, 2023
microsoft-github-policy-service bot pushed a commit that referenced this issue Nov 21, 2023
This changeset fixes an issue caused by #15991 where "chunked" escape
sequences would get corrupted. The fix is to simply not flush eagerly
anymore. I tried my best to keep the input lag reduction from #15991,
but unfortunately this isn't possible for console APIs.

Closes #16079

## Validation Steps Performed
* `type ascii.com` produces soft font ASCII characters ✅
DHowett pushed a commit that referenced this issue Dec 6, 2023
This changeset fixes an issue caused by #15991 where "chunked" escape
sequences would get corrupted. The fix is to simply not flush eagerly
anymore. I tried my best to keep the input lag reduction from #15991,
but unfortunately this isn't possible for console APIs.

Closes #16079

## Validation Steps Performed
* `type ascii.com` produces soft font ASCII characters ✅

(cherry picked from commit bdf2f6f)
Service-Card-Id: 91283205
Service-Version: 1.19
@lhecker
Copy link
Member

lhecker commented Mar 1, 2024

This assumes that the VT output is in a ground state, but that's not the case when we're processing a DCS sequence and are in the middle of passing it through to the conpty client.

Edit: I see now (and remember). The problem is not that we flush with the state machine is not in ground state, but rather that VtEngine inserts random stuff into ongoing sequences.

My hope was I can remedy #16769 by doing this, but that doesn't work because it's in ground state during newlines:

void VtIo::CorkRenderer(SCREEN_INFORMATION& screenInfo, bool corked) const noexcept
{
    if (!corked && screenInfo.GetStateMachine().IsGroundState())
    {
        LOG_IF_FAILED(ServiceLocator::LocateGlobals().pRender->PaintFrame());
    }
    _pVtRenderEngine->Cork(corked);
}

Edit 2: It's not that type does line-wise writes, no, it writes lines in chunks of 80 characters. cmd folks were once again clearly ahead of the the game.

github-merge-queue bot pushed a commit that referenced this issue Mar 28, 2024
I've found that #16079 was never properly addressed (it still randomly
occurred after even after PR #16349), which later led to the issues
described in #16769 (nushell flickering due to too many flushes).

The crux of the fix is that this brings back the `_noFlushOnEnd` flag
that was removed in PR #15991. This is then combined with a change to
the cork API: An `uncork` on `VtEngine` now only flushes if `_Flush`
got called while it was corked in the first place.

`_noFlushOnEnd` prevents us from flushing in between two "unknown"
VT sequences (like soft fonts or FTCS) which prevents them from being
corrupted. The corking prevents the remaining cases of flushing too
often. Long-term, a proper fix would be to pass through VT unmodified.

Closes #16769
DHowett pushed a commit that referenced this issue Apr 16, 2024
I've found that #16079 was never properly addressed (it still randomly
occurred after even after PR #16349), which later led to the issues
described in #16769 (nushell flickering due to too many flushes).

The crux of the fix is that this brings back the `_noFlushOnEnd` flag
that was removed in PR #15991. This is then combined with a change to
the cork API: An `uncork` on `VtEngine` now only flushes if `_Flush`
got called while it was corked in the first place.

`_noFlushOnEnd` prevents us from flushing in between two "unknown"
VT sequences (like soft fonts or FTCS) which prevents them from being
corrupted. The corking prevents the remaining cases of flushing too
often. Long-term, a proper fix would be to pass through VT unmodified.

Closes #16769

(cherry picked from commit 1ede023)
Service-Card-Id: 91965217
Service-Version: 1.20
DHowett pushed a commit that referenced this issue Apr 16, 2024
I've found that #16079 was never properly addressed (it still randomly
occurred after even after PR #16349), which later led to the issues
described in #16769 (nushell flickering due to too many flushes).

The crux of the fix is that this brings back the `_noFlushOnEnd` flag
that was removed in PR #15991. This is then combined with a change to
the cork API: An `uncork` on `VtEngine` now only flushes if `_Flush`
got called while it was corked in the first place.

`_noFlushOnEnd` prevents us from flushing in between two "unknown"
VT sequences (like soft fonts or FTCS) which prevents them from being
corrupted. The corking prevents the remaining cases of flushing too
often. Long-term, a proper fix would be to pass through VT unmodified.

Closes #16769

(cherry picked from commit 1ede023)
Service-Card-Id: 91965216
Service-Version: 1.19
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 In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants