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

Fix Windows 10 support for nearby font loading #12554

Merged
1 commit merged into from
Mar 10, 2022
Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Feb 23, 2022

By replacing IDWriteFontSetBuilder2::AddFontFile with
IDWriteFactory5::CreateFontFileReference and
IDWriteFontSetBuilder1::AddFontFile we add nearby
font loading support for Windows 10, build 15021.

This commit also fixes font fallback for AtlasEngine,
which was crashing during testing.

Finally it fixes a bug in DxEngine, where we only created a "nearby" font
collection if we couldn't find the font in the system collection. This doesn't
fix the bug, if the font is locked or broken in the system collection.

This is related to #11648.

PR Checklist

Validation Steps Performed

  • Build a Debug version of Windows Terminal
  • Put Jetbrains Mono into the writeable AppX directory
  • Jetbrains Mono is present in the settings UI ✅
  • DxEngine works with Jetbrains Mono ✅
  • AtlasEngine works with Jetbrains Mono ✅

@ghost ghost added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Feb 23, 2022
src/renderer/atlas/FontCache.h Outdated Show resolved Hide resolved
Comment on lines +433 to +447
void AtlasEngine::UpdateHyperlinkHoveredId(const uint16_t hoveredId) noexcept
{
_api.hyperlinkHoveredId = hoveredId;
}

#pragma endregion

void AtlasEngine::_resolveAntialiasingMode() noexcept
{
// If the user asks for ClearType, but also for a transparent background
// (which our ClearType shader doesn't simultaneously support)
// then we need to sneakily force the renderer to grayscale AA.
const auto forceGrayscaleAA = _api.antialiasingMode == D2D1_TEXT_ANTIALIAS_MODE_CLEARTYPE && !_api.backgroundOpaqueMixin;
_api.realizedAntialiasingMode = forceGrayscaleAA ? D2D1_TEXT_ANTIALIAS_MODE_GRAYSCALE : _api.antialiasingMode;
}
Copy link
Member Author

@lhecker lhecker Feb 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code isn't new. It's just moved up from down below to group the code diff better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes it difficult to review the diff -- can you point out the part that fixed the fallback issue you mentioned?

Copy link
Member Author

@lhecker lhecker Feb 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, saying that I moved it might've been an overstatement.
I didn't actually move the code. The _updateFont function is new. It's just git diffing it poorly and no builtin algorithm improves this diff.

@lhecker lhecker added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. and removed zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. labels Feb 23, 2022
@lhecker lhecker force-pushed the dev/lhecker/12420-nearby-fonts branch from c6b873f to 9aa6246 Compare February 23, 2022 15:54
src/cascadia/TerminalApp/AppLogic.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalSettingsEditor/ProfileViewModel.cpp Outdated Show resolved Hide resolved
Comment on lines +433 to +447
void AtlasEngine::UpdateHyperlinkHoveredId(const uint16_t hoveredId) noexcept
{
_api.hyperlinkHoveredId = hoveredId;
}

#pragma endregion

void AtlasEngine::_resolveAntialiasingMode() noexcept
{
// If the user asks for ClearType, but also for a transparent background
// (which our ClearType shader doesn't simultaneously support)
// then we need to sneakily force the renderer to grayscale AA.
const auto forceGrayscaleAA = _api.antialiasingMode == D2D1_TEXT_ANTIALIAS_MODE_CLEARTYPE && !_api.backgroundOpaqueMixin;
_api.realizedAntialiasingMode = forceGrayscaleAA ? D2D1_TEXT_ANTIALIAS_MODE_GRAYSCALE : _api.antialiasingMode;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes it difficult to review the diff -- can you point out the part that fixed the fallback issue you mentioned?

@lhecker lhecker added the zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. label Feb 23, 2022
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the font exists in the system set, but is unreadable due to a permission error? Would we find a "nearby" version of the same font and load it? That is the core of the issue with Cascadia on upgrade.

src/renderer/base/FontCache.h Outdated Show resolved Hide resolved
src/renderer/base/FontCache.h Outdated Show resolved Hide resolved
@lhecker
Copy link
Member Author

lhecker commented Feb 24, 2022

What if the font exists in the system set, but is unreadable due to a permission error? Would we find a "nearby" version of the same font and load it? That is the core of the issue with Cascadia on upgrade.

With the Windows Font Cache Service enabled I can't get my system in a state where Jetbrains Mono isn't available since it remains cached even after the .ttf file had its ACLs changed. If I disable that service, DirectWrite properly won't support the font at all. In short: I can't get my system to replicate the issue even after changing the ACLs.

In theory this approach should fix the issue, because we append the "nearby fonts" last to the collection, due to which they should take priority over the system fonts. I suspect that the people for whom this doesn't work, mostly used Windows 10, which this PR will fix.

@DHowett DHowett added the Needs-Second It's a PR that needs another sign-off label Feb 25, 2022
src/cascadia/TerminalApp/AppLogic.cpp Outdated Show resolved Hide resolved
src/renderer/base/FontCache.h Outdated Show resolved Hide resolved
src/renderer/dx/DxFontInfo.cpp Outdated Show resolved Hide resolved
src/renderer/dx/lib/dx.vcxproj Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 25, 2022
@lhecker lhecker force-pushed the dev/lhecker/12420-nearby-fonts branch from dbf37a7 to 418ef40 Compare February 28, 2022 19:15
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Feb 28, 2022
@lhecker lhecker force-pushed the dev/lhecker/12420-nearby-fonts branch from 418ef40 to fe55404 Compare February 28, 2022 21:04
@lhecker
Copy link
Member Author

lhecker commented Mar 1, 2022

FYI I made the following changes to the PR:

  • No more caching of the font collection (ergo no invalidation required)
  • DxEngine now always creates a nearby font collection, so that we never try to use the installed Cascadia font
  • Added feature flagging to remove the nearby font loading from conhost, if we ever decide to ship Atlas/DxEngine with conhost
    We should do the same with OpenConsole, however we don't have a macro to control that just yet.

Comment on lines -192 to -195
_weight = DWRITE_FONT_WEIGHT_NORMAL;
_stretch = DWRITE_FONT_STRETCH_NORMAL;
_style = DWRITE_FONT_STYLE_NORMAL;
face = _FindFontFace(dwriteFactory, localeName, false);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_FindFontFace doesn't depend on any of the 3 members above, so this code is redundant and could be removed.

Comment on lines +407 to +409
static constexpr std::array fallbackFaceNames{ static_cast<const wchar_t*>(nullptr), L"Consolas", L"Lucida Console", L"Courier New" };
auto it = fallbackFaceNames.begin();
const auto end = fallbackFaceNames.end();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows Terminal ignores the HRESULT this function returns and will simply crash with a division by 0 error if it wasn't for this new code. This implements font fallback in a similar way as DxEngine does.

In the future Terminal shouldn't ignore the HRESULT and call UpdateFont() with fallback fonts itself until this method succeeds.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future

should we track this work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a short note in #9999.

@@ -122,8 +114,7 @@ void DxFontInfo::SetFromEngine(const std::wstring_view familyName,
// - localeName - Locale to search for appropriate fonts
// Return Value:
// - Smart pointer holding interface reference for queryable font data.
[[nodiscard]] Microsoft::WRL::ComPtr<IDWriteFontFace1> DxFontInfo::ResolveFontFaceWithFallback(gsl::not_null<IDWriteFactory1*> dwriteFactory,
std::wstring& localeName)
[[nodiscard]] Microsoft::WRL::ComPtr<IDWriteFontFace1> DxFontInfo::ResolveFontFaceWithFallback(IDWriteFontCollection* fontCollection, std::wstring& localeName)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved ownership of the nearby font collection over to DxFontRenderData. That way we don't instantiate 4 font collections for the 4 font styles (italic & bold).

fontCollection = nearbyCollection;
}
}
THROW_IF_FAILED(fontCollection->FindFamilyName(_familyName.data(), &familyIndex, &familyExists));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DxEngine will now always search for nearby fonts. The previous approach is inherently flawed, because we first checked if a font exists in the system collection, but those were corrupted. So the only fix is to eagerly use nearby fonts and not rely on MSIX anymore.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k I'm happy the spooky action at a distance is gone

src/cascadia/TerminalApp/AppLogic.cpp Outdated Show resolved Hide resolved
return fontFiles;
}

inline wil::com_ptr<IDWriteFontCollection> getFontCollection(bool forceUpdate)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm kinda just trusting that this bit of the code will work as expected. I don't totally know the mechanical differences between a IDWriteFactory6 and a IDWriteFactory, or a IDWriteFontSetBuilder1 and a IDWriteFontSetBuilder2, but I'm trusting it works

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW the new code is what IDWriteFontSetBuilder2::AddFontFile does internally. It's implemented in terms of IDWriteFontSetBuilder1::AddFontFile.

@zadjii-msft zadjii-msft linked an issue Mar 9, 2022 that may be closed by this pull request
Comment on lines +407 to +409
static constexpr std::array fallbackFaceNames{ static_cast<const wchar_t*>(nullptr), L"Consolas", L"Lucida Console", L"Courier New" };
auto it = fallbackFaceNames.begin();
const auto end = fallbackFaceNames.end();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future

should we track this work?

if (til::ends_with(p.path().native(), L".ttf"))
{
wil::com_ptr<IDWriteFontFile> fontFile;
if (SUCCEEDED_LOG(factory5->CreateFontFileReference(p.path().c_str(), nullptr, fontFile.addressof())))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it's safe to use these objects on different threads because it is a SHARED-type factory? neato

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DWRITE_FACTORY_TYPE_SHARED only enables the use of the Windows Font Cache service for DirectWrite. It's unspecified whether DirectWrite is thread-safe, but it should be, since Direct2D can be used across threads. I'm gonna ask one of the dwrite devs about this...

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 10, 2022
@ghost
Copy link

ghost commented Mar 10, 2022

Hello @zadjii-msft!

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.

@ghost ghost merged commit f84ccad into main Mar 10, 2022
@ghost ghost deleted the dev/lhecker/12420-nearby-fonts branch March 10, 2022 12:36
DHowett pushed a commit that referenced this pull request Mar 10, 2022
By replacing `IDWriteFontSetBuilder2::AddFontFile` with
`IDWriteFactory5::CreateFontFileReference` and
`IDWriteFontSetBuilder1::AddFontFile` we add nearby
font loading support for Windows 10, build 15021.

This commit also fixes font fallback for AtlasEngine,
which was crashing during testing.

Finally it fixes a bug in DxEngine, where we only created a "nearby" font
collection if we couldn't find the font in the system collection. This doesn't
fix the bug, if the font is locked or broken in the system collection.

This is related to #11648.

## PR Checklist
* [x] Closes #12420
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed
* Build a Debug version of Windows Terminal
* Put Jetbrains Mono into the writeable AppX directory
* Jetbrains Mono is present in the settings UI ✅
* DxEngine works with Jetbrains Mono ✅
* AtlasEngine works with Jetbrains Mono ✅

(cherry picked from commit f84ccad)
DHowett pushed a commit that referenced this pull request Mar 15, 2022
By replacing `IDWriteFontSetBuilder2::AddFontFile` with
`IDWriteFactory5::CreateFontFileReference` and
`IDWriteFontSetBuilder1::AddFontFile` we add nearby
font loading support for Windows 10, build 15021.

This commit also fixes font fallback for AtlasEngine,
which was crashing during testing.

Finally it fixes a bug in DxEngine, where we only created a "nearby" font
collection if we couldn't find the font in the system collection. This doesn't
fix the bug, if the font is locked or broken in the system collection.

This is related to #11648.

* [x] Closes #12420
* [x] I work here
* [x] Tests added/passed

* Build a Debug version of Windows Terminal
* Put Jetbrains Mono into the writeable AppX directory
* Jetbrains Mono is present in the settings UI ✅
* DxEngine works with Jetbrains Mono ✅
* AtlasEngine works with Jetbrains Mono ✅

(cherry picked from commit f84ccad)
@ghost
Copy link

ghost commented Mar 25, 2022

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

Handy links:

ghost pushed a commit that referenced this pull request Apr 14, 2022
The original research for a solution all the way back in #11032 contained an
unfortunate flaw. The nearby font loading code was written under the assumption
that Cascadia is missing in the system font collection, leading to our issues.
Adding nearby fonts last into the collection would thus ensure that we use
the system fonts whenever possible, but only have nearby fonts as a fallback.

This didn't work and we figured that we'd have to always prefer loading nearby
fonts over system fonts. #12554 tried to achieve this, but failed to change
the order in which the font set is built. In order to prefer nearby fonts
over system ones, we have to add the system font collection last.

## PR Checklist

* [x] Closes #11648
* [x] I work here
* [x] Tests added/passed
* [x] Embarrassment for my incompetence

## Validation Steps Performed

* Put Jetbrains Mono into the AppX directory of the Debug build
* Jetbrains Mono shows up in the font selector and is useable

Additionally a more complex mini-test was built:
Using FontForge I've cloned arial.ttf and removed all characters except for
the letter "0". Afterwards I've build a custom font collection the same way
we do it in Terminal, extracted a `FontFace` named "Arial" and called
`IDWriteFont::HasCharacter` for the letter "1".
Loading the system font collection first results in `TRUE` and loading it last
results in `FALSE` (since my custom arial.ttf doesn't have the letter "1").
This confirms that we need to load the system font collection last.
DHowett pushed a commit that referenced this pull request Apr 21, 2022
The original research for a solution all the way back in #11032 contained an
unfortunate flaw. The nearby font loading code was written under the assumption
that Cascadia is missing in the system font collection, leading to our issues.
Adding nearby fonts last into the collection would thus ensure that we use
the system fonts whenever possible, but only have nearby fonts as a fallback.

This didn't work and we figured that we'd have to always prefer loading nearby
fonts over system fonts. #12554 tried to achieve this, but failed to change
the order in which the font set is built. In order to prefer nearby fonts
over system ones, we have to add the system font collection last.

## PR Checklist

* [x] Closes #11648
* [x] I work here
* [x] Tests added/passed
* [x] Embarrassment for my incompetence

## Validation Steps Performed

* Put Jetbrains Mono into the AppX directory of the Debug build
* Jetbrains Mono shows up in the font selector and is useable

Additionally a more complex mini-test was built:
Using FontForge I've cloned arial.ttf and removed all characters except for
the letter "0". Afterwards I've build a custom font collection the same way
we do it in Terminal, extracted a `FontFace` named "Arial" and called
`IDWriteFont::HasCharacter` for the letter "1".
Loading the system font collection first results in `TRUE` and loading it last
results in `FALSE` (since my custom arial.ttf doesn't have the letter "1").
This confirms that we need to load the system font collection last.

(cherry picked from commit eeb8970)
Service-Card-Id: 80641214
Service-Version: 1.13
DHowett pushed a commit that referenced this pull request Apr 21, 2022
The original research for a solution all the way back in #11032 contained an
unfortunate flaw. The nearby font loading code was written under the assumption
that Cascadia is missing in the system font collection, leading to our issues.
Adding nearby fonts last into the collection would thus ensure that we use
the system fonts whenever possible, but only have nearby fonts as a fallback.

This didn't work and we figured that we'd have to always prefer loading nearby
fonts over system fonts. #12554 tried to achieve this, but failed to change
the order in which the font set is built. In order to prefer nearby fonts
over system ones, we have to add the system font collection last.

## PR Checklist

* [x] Closes #11648
* [x] I work here
* [x] Tests added/passed
* [x] Embarrassment for my incompetence

## Validation Steps Performed

* Put Jetbrains Mono into the AppX directory of the Debug build
* Jetbrains Mono shows up in the font selector and is useable

Additionally a more complex mini-test was built:
Using FontForge I've cloned arial.ttf and removed all characters except for
the letter "0". Afterwards I've build a custom font collection the same way
we do it in Terminal, extracted a `FontFace` named "Arial" and called
`IDWriteFont::HasCharacter` for the letter "1".
Loading the system font collection first results in `TRUE` and loading it last
results in `FALSE` (since my custom arial.ttf doesn't have the letter "1").
This confirms that we need to load the system font collection last.

(cherry picked from commit eeb8970)
Service-Card-Id: 80641213
Service-Version: 1.12
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Priority-3 A description (P3) Product-Terminal The new Windows Terminal. zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes.
Projects
None yet
3 participants