Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit edea198

Browse files
Ben WagnerSkia Commit-Bot
Ben Wagner
authored and
Skia Commit-Bot
committed
Fix loaders for DW variable fonts.
This fixes several issues introduced with DirectWrite and onMakeClone. The primary issue is that the font file loader and collection loader need to remain registered until the last font using them is no longer in use. Prior to onMakeClone this was just the original font, so unregistration was simple. However, when cloning a new font is brought into existence using the same loaders as the original. All clones need to keep a reference to the loaders so that the last can unregister them. In addition variable fonts complicated the concept of equality of fonts. As a result when variable font support was introduced an Equal method was provided to make comparison easier. Use it when available to avoid issues when comparing two system fonts which differ only by their variation design position. Change-Id: I6aa8d8cc3240f1244d16f661f6fbfd17ff0f7412 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/288159 Reviewed-by: Herb Derby <herb@google.com> Commit-Queue: Ben Wagner <bungeman@google.com>
1 parent 1f5f38e commit edea198

File tree

3 files changed

+68
-36
lines changed

3 files changed

+68
-36
lines changed

‎src/ports/SkFontMgr_win_dw.cpp‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,16 @@ struct ProtoDWriteTypeface {
360360
static bool FindByDWriteFont(SkTypeface* cached, void* ctx) {
361361
DWriteFontTypeface* cshFace = reinterpret_cast<DWriteFontTypeface*>(cached);
362362
ProtoDWriteTypeface* ctxFace = reinterpret_cast<ProtoDWriteTypeface*>(ctx);
363+
364+
// IDWriteFontFace5 introduced both Equals and HasVariations
365+
SkTScopedComPtr<IDWriteFontFace5> cshFontFace5;
366+
SkTScopedComPtr<IDWriteFontFace5> ctxFontFace5;
367+
cshFace->fDWriteFontFace->QueryInterface(&cshFontFace5);
368+
ctxFace->fDWriteFontFace->QueryInterface(&ctxFontFace5);
369+
if (cshFontFace5 && ctxFontFace5) {
370+
return cshFontFace5->Equals(ctxFontFace5.get());
371+
}
372+
363373
bool same;
364374

365375
//Check to see if the two fonts are identical.
@@ -459,7 +469,7 @@ sk_sp<SkTypeface> SkFontMgr_DirectWrite::makeTypefaceFromDWriteFont(
459469
ProtoDWriteTypeface spec = { fontFace, font, fontFamily };
460470
sk_sp<SkTypeface> face = fTFCache.findByProcAndRef(FindByDWriteFont, &spec);
461471
if (nullptr == face) {
462-
face = DWriteFontTypeface::Make(fFactory.get(), fontFace, font, fontFamily);
472+
face = DWriteFontTypeface::Make(fFactory.get(), fontFace, font, fontFamily, nullptr);
463473
if (face) {
464474
fTFCache.add(face);
465475
}
@@ -875,6 +885,10 @@ template <typename T> class SkAutoIDWriteUnregister {
875885
SkAutoIDWriteUnregister(IDWriteFactory* factory, T* unregister)
876886
: fFactory(factory), fUnregister(unregister)
877887
{ }
888+
SkAutoIDWriteUnregister(const SkAutoIDWriteUnregister&) = delete;
889+
SkAutoIDWriteUnregister& operator=(const SkAutoIDWriteUnregister&) = delete;
890+
SkAutoIDWriteUnregister(SkAutoIDWriteUnregister&&) = delete;
891+
SkAutoIDWriteUnregister& operator=(SkAutoIDWriteUnregister&&) = delete;
878892

879893
~SkAutoIDWriteUnregister() {
880894
if (fUnregister) {
@@ -904,7 +918,6 @@ template <typename T> class SkAutoIDWriteUnregister {
904918
sk_sp<SkTypeface> SkFontMgr_DirectWrite::onMakeFromStreamIndex(std::unique_ptr<SkStreamAsset> stream,
905919
int ttcIndex) const {
906920
SkTScopedComPtr<StreamFontFileLoader> fontFileLoader;
907-
// This transfers ownership of stream to the new object.
908921
HRN(StreamFontFileLoader::Create(std::move(stream), &fontFileLoader));
909922
HRN(fFactory->RegisterFontFileLoader(fontFileLoader.get()));
910923
SkAutoIDWriteUnregister<StreamFontFileLoader> autoUnregisterFontFileLoader(
@@ -940,8 +953,10 @@ sk_sp<SkTypeface> SkFontMgr_DirectWrite::onMakeFromStreamIndex(std::unique_ptr<S
940953
if (faceIndex == ttcIndex) {
941954
return DWriteFontTypeface::Make(fFactory.get(),
942955
fontFace.get(), font.get(), fontFamily.get(),
943-
autoUnregisterFontFileLoader.detatch(),
944-
autoUnregisterFontCollectionLoader.detatch());
956+
sk_make_sp<DWriteFontTypeface::Loaders>(
957+
fFactory.get(),
958+
autoUnregisterFontFileLoader.detatch(),
959+
autoUnregisterFontCollectionLoader.detatch()));
945960
}
946961
}
947962
}
@@ -952,7 +967,6 @@ sk_sp<SkTypeface> SkFontMgr_DirectWrite::onMakeFromStreamIndex(std::unique_ptr<S
952967
sk_sp<SkTypeface> SkFontMgr_DirectWrite::onMakeFromStreamArgs(std::unique_ptr<SkStreamAsset> stream,
953968
const SkFontArguments& args) const {
954969
SkTScopedComPtr<StreamFontFileLoader> fontFileLoader;
955-
// This transfers ownership of stream to the new object.
956970
HRN(StreamFontFileLoader::Create(std::move(stream), &fontFileLoader));
957971
HRN(fFactory->RegisterFontFileLoader(fontFileLoader.get()));
958972
SkAutoIDWriteUnregister<StreamFontFileLoader> autoUnregisterFontFileLoader(
@@ -1028,8 +1042,10 @@ sk_sp<SkTypeface> SkFontMgr_DirectWrite::onMakeFromStreamArgs(std::unique_ptr<Sk
10281042

10291043
return DWriteFontTypeface::Make(
10301044
fFactory.get(), fontFace.get(), font.get(), fontFamily.get(),
1031-
autoUnregisterFontFileLoader.detatch(),
1032-
autoUnregisterFontCollectionLoader.detatch());
1045+
sk_make_sp<DWriteFontTypeface::Loaders>(
1046+
fFactory.get(),
1047+
autoUnregisterFontFileLoader.detatch(),
1048+
autoUnregisterFontCollectionLoader.detatch()));
10331049
}
10341050
}
10351051

‎src/ports/SkTypeface_win_dw.cpp‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,21 @@
3232
#include "src/utils/win/SkDWrite.h"
3333
#include "src/utils/win/SkDWriteFontFileStream.h"
3434

35+
DWriteFontTypeface::Loaders::~Loaders() {
36+
// Don't return if any fail, just keep going to free up as much as possible.
37+
HRESULT hr;
38+
39+
hr = fFactory->UnregisterFontCollectionLoader(fDWriteFontCollectionLoader.get());
40+
if (FAILED(hr)) {
41+
SK_TRACEHR(hr, "FontCollectionLoader");
42+
}
43+
44+
hr = fFactory->UnregisterFontFileLoader(fDWriteFontFileLoader.get());
45+
if (FAILED(hr)) {
46+
SK_TRACEHR(hr, "FontFileLoader");
47+
}
48+
}
49+
3550
void DWriteFontTypeface::onGetFamilyName(SkString* familyName) const {
3651
SkTScopedComPtr<IDWriteLocalizedStrings> familyNames;
3752
HRV(fDWriteFontFamily->GetFamilyNames(&familyNames));
@@ -50,10 +65,10 @@ void DWriteFontTypeface::onGetFontDescriptor(SkFontDescriptor* desc,
5065

5166
desc->setFamilyName(utf8FamilyName.c_str());
5267
desc->setStyle(this->fontStyle());
53-
*isLocalStream = SkToBool(fDWriteFontFileLoader.get());
68+
*isLocalStream = SkToBool(fLoaders);
5469
}
5570

56-
void DWriteFontTypeface::onCharsToGlyphs(const SkUnichar uni[], int count,
71+
void DWriteFontTypeface::onCharsToGlyphs(const SkUnichar* uni, int count,
5772
SkGlyphID glyphs[]) const {
5873
fDWriteFontFace->GetGlyphIndices((const UINT32*)uni, count, glyphs);
5974
}
@@ -317,8 +332,7 @@ sk_sp<SkTypeface> DWriteFontTypeface::onMakeClone(const SkFontArguments& args) c
317332
newFontFace.get(),
318333
fDWriteFont.get(),
319334
fDWriteFontFamily.get(),
320-
fDWriteFontFileLoader.get(),
321-
fDWriteFontCollectionLoader.get());
335+
fLoaders);
322336
}
323337

324338
#endif

‎src/ports/SkTypeface_win_dw.h‎

Lines changed: 27 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -38,21 +38,39 @@ static SkFontStyle get_style(IDWriteFont* font) {
3838
}
3939

4040
class DWriteFontTypeface : public SkTypeface {
41+
public:
42+
struct Loaders : public SkNVRefCnt<Loaders> {
43+
Loaders(IDWriteFactory* factory,
44+
IDWriteFontFileLoader* fontFileLoader,
45+
IDWriteFontCollectionLoader* fontCollectionLoader)
46+
: fFactory(SkRefComPtr(factory))
47+
, fDWriteFontFileLoader(SkRefComPtr(fontFileLoader))
48+
, fDWriteFontCollectionLoader(SkRefComPtr(fontCollectionLoader))
49+
{}
50+
Loaders(const Loaders&) = delete;
51+
Loaders& operator=(const Loaders&) = delete;
52+
Loaders(Loaders&&) = delete;
53+
Loaders& operator=(Loaders&&) = delete;
54+
~Loaders();
55+
56+
SkTScopedComPtr<IDWriteFactory> fFactory;
57+
SkTScopedComPtr<IDWriteFontFileLoader> fDWriteFontFileLoader;
58+
SkTScopedComPtr<IDWriteFontCollectionLoader> fDWriteFontCollectionLoader;
59+
};
60+
4161
private:
4262
DWriteFontTypeface(const SkFontStyle& style,
4363
IDWriteFactory* factory,
4464
IDWriteFontFace* fontFace,
4565
IDWriteFont* font,
4666
IDWriteFontFamily* fontFamily,
47-
IDWriteFontFileLoader* fontFileLoader = nullptr,
48-
IDWriteFontCollectionLoader* fontCollectionLoader = nullptr)
67+
sk_sp<Loaders> loaders)
4968
: SkTypeface(style, false)
5069
, fFactory(SkRefComPtr(factory))
51-
, fDWriteFontCollectionLoader(SkSafeRefComPtr(fontCollectionLoader))
52-
, fDWriteFontFileLoader(SkSafeRefComPtr(fontFileLoader))
5370
, fDWriteFontFamily(SkRefComPtr(fontFamily))
5471
, fDWriteFont(SkRefComPtr(font))
5572
, fDWriteFontFace(SkRefComPtr(fontFace))
73+
, fLoaders(std::move(loaders))
5674
{
5775
if (!SUCCEEDED(fDWriteFontFace->QueryInterface(&fDWriteFontFace1))) {
5876
// IUnknown::QueryInterface states that if it fails, punk will be set to nullptr.
@@ -77,8 +95,6 @@ class DWriteFontTypeface : public SkTypeface {
7795
public:
7896
SkTScopedComPtr<IDWriteFactory> fFactory;
7997
SkTScopedComPtr<IDWriteFactory2> fFactory2;
80-
SkTScopedComPtr<IDWriteFontCollectionLoader> fDWriteFontCollectionLoader;
81-
SkTScopedComPtr<IDWriteFontFileLoader> fDWriteFontFileLoader;
8298
SkTScopedComPtr<IDWriteFontFamily> fDWriteFontFamily;
8399
SkTScopedComPtr<IDWriteFont> fDWriteFont;
84100
SkTScopedComPtr<IDWriteFontFace> fDWriteFontFace;
@@ -91,30 +107,15 @@ class DWriteFontTypeface : public SkTypeface {
91107
IDWriteFontFace* fontFace,
92108
IDWriteFont* font,
93109
IDWriteFontFamily* fontFamily,
94-
IDWriteFontFileLoader* fontFileLoader = nullptr,
95-
IDWriteFontCollectionLoader* fontCollectionLoader = nullptr)
110+
sk_sp<Loaders> loaders)
96111
{
97-
return sk_sp<DWriteFontTypeface>(
98-
new DWriteFontTypeface(get_style(font), factory, fontFace, font, fontFamily,
99-
fontFileLoader, fontCollectionLoader));
112+
return sk_sp<DWriteFontTypeface>(new DWriteFontTypeface(
113+
get_style(font), factory, fontFace, font, fontFamily, std::move(loaders)));
100114
}
101115

102116
protected:
103117
void weak_dispose() const override {
104-
// Don't return if these fail, just keep going so this gets disposed.
105-
HRESULT hr;
106-
if (fDWriteFontFileLoader.get()) {
107-
hr = fFactory->UnregisterFontFileLoader(fDWriteFontFileLoader.get());
108-
if (FAILED(hr)) {
109-
SK_TRACEHR(hr, "FontFileLoader");
110-
}
111-
}
112-
if (fDWriteFontCollectionLoader.get()) {
113-
hr = fFactory->UnregisterFontCollectionLoader(fDWriteFontCollectionLoader.get());
114-
if (FAILED(hr)) {
115-
SK_TRACEHR(hr, "FontCollectionLoader");
116-
}
117-
}
118+
fLoaders.reset();
118119

119120
//SkTypefaceCache::Remove(this);
120121
INHERITED::weak_dispose();
@@ -143,6 +144,7 @@ class DWriteFontTypeface : public SkTypeface {
143144
sk_sp<SkData> onCopyTableData(SkFontTableTag) const override;
144145

145146
private:
147+
mutable sk_sp<Loaders> fLoaders;
146148
typedef SkTypeface INHERITED;
147149
};
148150

0 commit comments

Comments
 (0)