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

Background color can exceed the height of the last line #17672

Closed
ltrzesniewski opened this issue Aug 6, 2024 · 14 comments · Fixed by #17674
Closed

Background color can exceed the height of the last line #17672

ltrzesniewski opened this issue Aug 6, 2024 · 14 comments · Fixed by #17674
Labels
In-PR This issue has a related PR 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

Comments

@ltrzesniewski
Copy link

Windows Terminal version

1.21.1772.0

Windows build number

10.0.22631.0

Other Software

No response

Steps to reproduce

I am experiencing this because I use Oh My Posh, but this can be reproduced without a custom prompt:

  • Open PowerShell
  • Maximize the terminal window
  • Press enter until the prompt reaches the bottom of the screen
  • Write some text with colored background on the last line:
    Write-Host -BackgroundColor Blue Hello && Start-Sleep 10
    You may also try to add -NoNewline to Write-Host if this doesn't reproduce, but I hope it does 😅

Expected Behavior

The background color should not exceed the height of the last line. This works fine when the window is not maximized:

image

Note the height of the last line is the same as the cursor height.

Actual Behavior

When maximized, the background color exceeds the height of the last line, it is extended a few pixels below the vertical span of the line:

image

Compare the height of the blue block to the cursor height.

Here is what happens after the sleep is over:

image

And writing some text makes a part of the blue bar disappear:

image

Here's what I experience with Oh My Posh:

image


I suppose this is due to the fact that maximizing the window prevents Windows Terminal from ensuring the window height is a multiple of the line size. I first thought this rendering quirk would be quickly noticed, but it's been a while and I'm surprised I couldn't find a similar issue, so here we go.

PS: I was able to reproduce this with other fonts as well.

@ltrzesniewski ltrzesniewski 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 Aug 6, 2024
@DHowett
Copy link
Member

DHowett commented Aug 6, 2024

Ah, yes, this! @lhecker was telling me about this.

You're probably using the Direct2D renderer (by choice or by hardware support, or maybe because you're remoting in!)

When we prepare the background, we render all the colors into a bitmap and then scale it to cover the entire terminal surface. The terminal surface is actually a little larger than the sum of the cells present in the terminal, so there's some excess that gets filled.

He notes:

  • D2D
    • Margins are filled with background color ❌
      • They're filled with the neighboring's cell background color for convenience, as D2D doesn't support D3D11_TEXTURE_ADDRESS_BORDER

@ltrzesniewski
Copy link
Author

ltrzesniewski commented Aug 6, 2024

Ooh, yes I'm using RDP, and setting the renderer to D3D 11 explicitly fixed the issue.

Thanks! 🙂

(I'm closing this since there's a fix, but feel free to reopen if you think a fix in D2D is necessary)

@lhecker
Copy link
Member

lhecker commented Aug 6, 2024

It's probably worth noting that the D2D renderer is only chosen automatically if no hardware GPU is available. If you explicitly force the D3D renderer the application can become laggy, because it's not optimized for software rendering. We'll probably fix this issue soon (DHowett prototyped a fix for this just now).

@DHowett DHowett reopened this Aug 6, 2024
DHowett added a commit that referenced this issue Aug 6, 2024
BackendD2D will now draw one extra cell on all sides when rendering the
background, filled with the expected background color, starting at (-1, -1)
to ensure that cell backgrounds do not bleed over the edges of the
viewport.

Fixes #17672
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Aug 6, 2024
@DHowett
Copy link
Member

DHowett commented Aug 6, 2024

You caught me on a good day. I figured, "this has been bothering me too, maybe I can fix it myself!"

@ltrzesniewski
Copy link
Author

Cool! 😎

FWIW, we've enabled a new setting in some of our WPF apps which turns hardware acceleration on when they're used over RDP, and I remember them actually working better. But maybe I'm comparing different things - WPF going from their software renderer to their DirectX renderer (I'm not sure if they use D2D or D3D, both seem present in the source code) vs WT going from D2D to D3D. I'm not very familiar with this stuff. 😅

I'll double-check this tomorrow and also bench both WT's renderers over RDP, as it would be surprising if our apps gained on perf but WT were slower.

@lhecker
Copy link
Member

lhecker commented Aug 6, 2024

D2D gets transmitted as commands over RDP and rendered on the client side. This makes it pretty much the optimal protocol for rendering. It's less important nowadays, only because RDP has much more efficient H264 codecs and because the hardware & internet got faster.

The reason why the D2D renderer is better however isn't because of RDP, but rather because of the lack of a GPU on your host. Both renderers draw the entire window contents from scratch on every frame. The D3D renderer will do this via WARP if there's no GPU, whereas the D2D renderer will transmit the commands over RDP and use the client's GPU to rasterize it. That makes it a lot cheaper.

Long-term I'm planning to improve our rendering architecture and then we can implement partial invalidation which should make the D3D rendering a lot cheaper, similar to how it's performing in WPF.

@ltrzesniewski
Copy link
Author

Thanks for the info! That makes sense.

@ltrzesniewski
Copy link
Author

@DHowett thanks a lot for the fix! 🙂

@lhecker just to close the parenthesis, if you're interested: I have a Xeon E-2288G on the host, which comes with a P630 sort-of-GPU, but dxdiag still shows the RDP driver:

image

I found this benchmark by @xoofx and here are the results:

  • D2D: 476x94 = 44744 Elapsed: 20,99274015984014ms
  • D3D: 476x94 = 44744 Elapsed: 45,35825394605393ms

So D2D is much faster through RDP but it skips frames, whereas I could see every frame when using D3D.

Since I have a big screen, I also tried reducing the window size to 1/4 of the screen, and D3D feels much better:

  • D2D: 235x45 = 10575 Elapsed: 4,405244155844153ms
  • D3D: 235x45 = 10575 Elapsed: 5,969610189810194ms

I also tried this Doom fire with the smaller (235x45) window size:

  • D2D: 80 fps
  • D3D: 50 fps

So yes, D2D is definitely faster through RDP. As for our WPF app, the HW renderer is a bit faster, but that's not obvious to the human eye. 🙂

@lhecker
Copy link
Member

lhecker commented Aug 7, 2024

That's weird. It's normal that it shows the RDP driver in dxdiag. But the P630 is a modern GPU, and it should be provided to us by DWM when we ask for the primary GPU.

While connected via RDP, could you go to Settings > System > Display > Graphics and search for Windows Terminal? Add it via "Add Microsoft Store app" if it's not there. Then there should be a GPU dropdown:

image

Does the P630 show up there? I tested it via RDP between two of my computers and it never picked D2D via RDP if the server/host had a capable GPU.

@ltrzesniewski
Copy link
Author

WT wasn't in the list, so I added it and set it to high performance. It only shows "GPU: Microsoft Basic Render Driver":

image

Even in this mode, WT still picks D2D when set to auto. I made sure I'm in the high perf power mode.

I checked the Intel driver update tool, but it doesn't show any updates for the graphics driver:

image

Note that I use a remote desktop gateway, so maybe that plays a role?

@lhecker
Copy link
Member

lhecker commented Aug 7, 2024

"Microsoft Basic Render Driver" is WARP. You can safely remove that entry from the settings app again so that it picks the default adapter in the future if you have a proper GPU. Otherwise it may be stuck on WARP forever. To remove it, click "Reset" and then "Remove" below the GPU dropdown.

Are you sure the integrated GPU is even enabled? The ASPEED entry that's listed by the Intel tool is a GPU for remote management (it allow you to see the BIOS over network) and is not an actual proper GPU. The other entry is just WARP again.

Do you ever use that machine physically or only ever via RDP? In case of the latter I suspect that the integrated GPU is indeed simply turned off.

@ltrzesniewski
Copy link
Author

This machine is only ever used through RDP, and I don't have access to the BIOS. I asked the admins to check if the GPU is enabled, but they're quite busy and I didn't receive a response yet. I'll keep you posted.

@ltrzesniewski
Copy link
Author

Well, after patching the BIOS config, the GPU got enabled:

image

And I can confirm that WT now automatically picks D3D over RDP. 🙂

@lhecker
Copy link
Member

lhecker commented Aug 8, 2024

Nice! I'm glad to hear that. It should run a lot smoother now, in particular if the CPU is already busy with other tasks (compilation, etc.).

DHowett added a commit that referenced this issue Aug 13, 2024
BackendD2D will now draw one extra cell on all sides when rendering the
background, filled with the expected background color, starting at (-1,
-1) to ensure that cell backgrounds do not bleed over the edges of the
viewport where the is swapchain but no content.

Fixes #17672

(cherry picked from commit 9007fc2)
Service-Card-Id: 93494324
Service-Version: 1.21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In-PR This issue has a related PR 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants