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

Missing glyph substitution #10472

Closed
alabuzhev opened this issue Jun 21, 2021 · 8 comments · Fixed by #10478
Closed

Missing glyph substitution #10472

alabuzhev opened this issue Jun 21, 2021 · 8 comments · Fixed by #10478
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Impact-Compatibility Sure don't work like it used to. Impact-Correctness It be wrong. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-2 A description (P2) 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.
Milestone

Comments

@alabuzhev
Copy link
Contributor

Why not let Windows substitute the glyphs that are missing in the current font?

  • I'm sure that this should've been discussed already somewhere due to the obviousness of the topic, but I can't find anything similar.

Currently the GDI renderer of conhost/OpenConsole uses PolyTextOutW for drawing.
It looks like PolyTextOutW doesn't try to substitute any missing glyphs, so the only way to see, say, Hiragana is to change the whole font to something like MS Gothic (which is eye-bleeding, to be honest).

However, there are other APIs that perform such substitution, e.g. ExtTextOutW.
Are there any reasons not to use them?

Proposed technical implementation details (optional)

A trivial replace PolyTextOutW -> ExtTextOutW already does the trick (and probably there are alternative / better methods as well):

diff --git a/src/renderer/gdi/paint.cpp b/src/renderer/gdi/paint.cpp
index 9295d3d6..550727d3 100644
--- a/src/renderer/gdi/paint.cpp
+++ b/src/renderer/gdi/paint.cpp
@@ -436,10 +436,22 @@ using namespace Microsoft::Console::Render;
 
     if (_cPolyText > 0)
     {
+#if 1
+        for (size_t i = 0; i != _cPolyText; ++i)
+        {
+            const auto& t = _pPolyText[i];
+            if (!ExtTextOutW(_hdcMemoryContext, t.x, t.y, t.uiFlags, &t.rcl, t.lpstr, t.n, t.pdx))
+            {
+                hr = E_FAIL;
+                break;
+            }
+        }
+#else
         if (!PolyTextOutW(_hdcMemoryContext, _pPolyText, (UINT)_cPolyText))
         {
             hr = E_FAIL;
         }
+#endif
 
         _polyStrings.clear();
         _polyWidths.clear();

Before the change:

image

After the change:

image

It works as expected in other apps, including positioning, clipping, size, etc.:

image

So, any reason not to do that or something similar?

P.S. I'm aware that there's Windows Terminal, where it already works as expected. This is about conhost/OpenConsole.

@alabuzhev alabuzhev added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jun 21, 2021
@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 Jun 21, 2021
@skyline75489 skyline75489 added the Product-Conhost For issues in the Console codebase label Jun 21, 2021
@DHowett
Copy link
Member

DHowett commented Jun 21, 2021

That is an excellent question. I believe we went with PolyTextOutW to reduce the number of times we needed to context-switch into kernel mode to one per buffer instead of one per line (?). @miniksa can keep me honest here, but he's out for a couple days.

If it doesn't impact performance as much as it used to, this seems like a pretty easy win for a problem that's existed since time immemorial. 😄

@alabuzhev
Copy link
Contributor Author

I believe we went with PolyTextOutW...

As far as I can see, conhost.exe was already using PolyTextOutW back in Windows 7 (2009):

dumpbin /imports:gdi32.dll conhost.exe
Dump of file conhost.exe

File Type: EXECUTABLE IMAGE

  Section contains the following imports:

    GDI32.dll
             10003F000 Import Address Table
             100041958 Import Name Table
                     0 time date stamp
                     0 Index of first forwarder reference

                          4F CreateRectRgn
                          22 CombineRgn
                         231 InvertRgn
                          E6 DeleteObject
                          30 CreateCompatibleDC
                          2F CreateCompatibleBitmap
                         277 SelectObject
                         278 SelectPalette
                         2B4 StretchDIBits
                          E3 DeleteDC
                          36 CreateDIBitmap
                         1FD GetObjectW
                          13 BitBlt
                         1CA GetDIBits
                         20D GetStockObject
                         251 PolyPatBlt
                         175 GdiFlush
                         1F6 GetNearestColor
                         285 SetDCBrushColor
                         2A6 SetTextColor
                         27E SetBkColor
                          40 CreateFontIndirectW
                         246 PatBlt
                         21E GetTextExtentPoint32W
                          32 CreateDCW
                         125 EnumFontFamiliesExW
                         28C SetFontEnumeration
                         224 GetTextFaceW
                         1CB GetDeviceCaps
                         27F SetBkMode
                         1C4 GetCurrentObject
                         20A GetRegionData
                         20C GetRgnBox
                    ---> 255 PolyTextOutW
                         2A3 SetSystemPaletteUse
                         25C RealizePalette
                         210 GetStringBitmapW
                          54 CreateSolidBrush
                         1B7 GetCharWidth32W
                          29 CreateBitmap
                         2BA TranslateCharsetInfo
                         27C SetBitmapBits
                         2B3 StretchBlt
                         1A7 GetBitmapBits
                         226 GetTextMetricsW
                         289 SetDIBitsToDevice

...to reduce the number of times we needed to context-switch into kernel mode to one per buffer instead of one per line

I've added this just before the loop:

assert(_cPolyText == 1);

and tried again, with cmd and other apps, outputting quite a few text in large and small blocks, using WriteConsoleW / WriteConsoleOutputW, but that assert never failed. It looks like there's not much to reduce in the first place, if anything :)

I've also tried erasing and filling the whole viewport in a loop and haven't noticed any difference with the stock conhost - it flickers with visually the same frequency. Even the debug version performs acceptably.

@DHowett
Copy link
Member

DHowett commented Jun 21, 2021

Wow, I found the actual changeset in Win7 that switched us to PolyTextOutW. Unfortunately, it switched us from something called NtGdiConsoleTextOut which took a POLYTEXTW (😅)

NtGdiConsoleTextOut dates to 1998. It calls GreConsoleTextOut (1992!), which is documented as:

/******************************Public*Routine******************************\
* BOOL GreConsoleTextOut
*
* Write text with no spacing and alignment options, thereby saving a ton
* of time.
*
* History:
*  Fri 12-Nov-1993 -by- Redacted [redacted]
* Smaller and Faster
*
*  Wed 16-Sep-1992 17:36:17 -by- Redacted [redacted]
* Duplicated GrePolyTextOut, and then deleted the unneeded code.
\**************************************************************************/

It looks like the decision to have a "fast path" for the console text pipeline to get out to GDI was made back in or before NT 3.5. Back then, and actually probably up until Windows 10, we did just send the entire console buffer to GDI every time it changed. Dirty region tracking (and therefore, not having cPolyText > 1) came about only relatively recently in the console's history.

@DHowett
Copy link
Member

DHowett commented Jun 21, 2021

This is probably one of those decisions we never revisited after being told that it was true when we inherited the code. 😄

@alabuzhev
Copy link
Contributor Author

I found the actual changeset in Win7 that switched us to PolyTextOutW. Unfortunately, it switched us from something called NtGdiConsoleTextOut

And, surprisingly, glyph substitution worked before that changeset :)

image

Apparently, even NtGdiConsoleTextOut had some extra magic which PolyTextOutW doesn't have.

@DHowett
Copy link
Member

DHowett commented Jun 21, 2021

worked before that changeset

...

shit.

@DHowett
Copy link
Member

DHowett commented Jun 21, 2021

Yeah, we should definitely do this. Thanks @alabuzhev!

@DHowett DHowett added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Impact-Compatibility Sure don't work like it used to. Impact-Correctness It be wrong. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Jun 21, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 21, 2021
@DHowett DHowett added this to the Windows vNext milestone Jun 21, 2021
alabuzhev added a commit to alabuzhev/terminal that referenced this issue Jun 21, 2021
Implementation of microsoft#10472.
ExtTextOutW allows substitution of missing glyphs from other fonts.
@ghost ghost added the In-PR This issue has a related PR label Jun 21, 2021
@ghost ghost closed this as completed in #10478 Jun 22, 2021
ghost pushed a commit that referenced this issue Jun 22, 2021
Replace PolyTextOutW with ExtTextOutW to allow substitution of missing
glyphs from other fonts.

Why not let Windows substitute the glyphs that are missing in the
current font?  Currently the GDI renderer of conhost/OpenConsole uses
`PolyTextOutW` for drawing.  `PolyTextOutW` doesn't try to substitute
any missing glyphs, so the only way to see, say, Hiragana is to change
the _whole font_ to something like MS Gothic (which is eye-bleeding, to
be honest).

A trivial replace `PolyTextOutW` -> `ExtTextOutW` does the trick.

Switch to `PolyTextOutW` happened in Windows 7 along with introduction
of conhost.exe.  Substitution worked in previous Windows versions, where
internal NT interfaces were used.

# Before the change:
![image](https://user-images.githubusercontent.com/11453922/122759189-93ff3900-d291-11eb-89a9-b1d47aa22ed9.png)

# After the change:
![image](https://user-images.githubusercontent.com/11453922/122759316-b4c78e80-d291-11eb-87aa-7cdc2979ae28.png)

Closes #10472
@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 Jun 22, 2021
ghost pushed a commit that referenced this issue Jul 8, 2021
Currently, when the user changes the console codepage (manually or via a script) the GDI engine tries to find and set the "best possible" font. The "best possible" here is charset-wise, it doesn't mean that the font is actually better or more eye-candy for the user.

Example:
- Open cmd
- Set the font to Consolas
- Enter `chcp 932`
- Suddenly, a wild MS Gothic appears!

This kind of makes sense (*"if I'm changing the codepage I probably want to see the national characters"*) but it doesn't happen anywhere else - all other apps just substitute the missing glyphs.

After #10472 / #10478 this magic should finally work here as well. So, do we still need to change the whole font? Terminal doesn't do that after all.

## Validation Steps Performed
Download [932.cmd.txt](https://github.com/microsoft/terminal/files/6697577/932.cmd.txt),
rename to 932.cmd, run it, check if the output is still readable.

Closes #10497
@ghost
Copy link

ghost commented Jul 14, 2021

🎉This issue was addressed in #10478, which has now been successfully released as Windows Terminal Preview v1.10.1933.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-Rendering Text rendering, emoji, complex glyph & font-fallback issues Impact-Compatibility Sure don't work like it used to. Impact-Correctness It be wrong. Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-2 A description (P2) 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