From 33eb30fed8aa45c47a65e684e1b65da2b2e1fb91 Mon Sep 17 00:00:00 2001 From: bungeman Date: Tue, 17 Jun 2014 18:12:51 -0700 Subject: [PATCH] Clean up DirectWrite typeface cache matching. The matching code was difficult to follow due to naming issues, and performed some duplicate work which is not wanted. This change will either fix the associated bug or make it possible to track the cause. CQ_EXTRA_TRYBOTS=tryserver.skia:Test-Win7-ShuttleA-HD2000-x86-Release-DirectWrite-Trybot BUG=384529 R=reed@google.com Author: bungeman@google.com Review URL: https://codereview.chromium.org/338923003 --- src/ports/SkFontMgr_win_dw.cpp | 178 +++++++++++++++++++---------------------- 1 file changed, 83 insertions(+), 95 deletions(-) diff --git a/src/ports/SkFontMgr_win_dw.cpp b/src/ports/SkFontMgr_win_dw.cpp index 8633775..eb0989a 100644 --- a/src/ports/SkFontMgr_win_dw.cpp +++ b/src/ports/SkFontMgr_win_dw.cpp @@ -259,11 +259,6 @@ public: memcpy(fLocaleName.get(), localeName, localeNameLength * sizeof(WCHAR)); } - /** Creates a typeface using a typeface cache. */ - SkTypeface* createTypefaceFromDWriteFont(IDWriteFontFace* fontFace, - IDWriteFont* font, - IDWriteFontFamily* fontFamily) const; - protected: virtual int onCountFamilies() const SK_OVERRIDE; virtual void onGetFamilyName(int index, SkString* familyName) const SK_OVERRIDE; @@ -283,16 +278,10 @@ private: HRESULT getByFamilyName(const WCHAR familyName[], IDWriteFontFamily** fontFamily) const; HRESULT getDefaultFontFamily(IDWriteFontFamily** fontFamily) const; - void Add(SkTypeface* face, SkTypeface::Style requestedStyle, bool strong) const { - SkAutoMutexAcquire ama(fTFCacheMutex); - fTFCache.add(face, requestedStyle, strong); - } - - SkTypeface* FindByProcAndRef(SkTypefaceCache::FindProc proc, void* ctx) const { - SkAutoMutexAcquire ama(fTFCacheMutex); - SkTypeface* typeface = fTFCache.findByProcAndRef(proc, ctx); - return typeface; - } + /** Creates a typeface using a typeface cache. */ + SkTypeface* createTypefaceFromDWriteFont(IDWriteFontFace* fontFace, + IDWriteFont* font, + IDWriteFontFamily* fontFamily) const; SkTScopedComPtr fFactory; SkTScopedComPtr fFontCollection; @@ -321,129 +310,128 @@ private: SkTScopedComPtr fFontFamily; }; -static bool are_same(IUnknown* a, IUnknown* b) { +static HRESULT are_same(IUnknown* a, IUnknown* b, bool& same) { SkTScopedComPtr iunkA; - if (FAILED(a->QueryInterface(&iunkA))) { - return false; - } + HRM(a->QueryInterface(&iunkA), "Failed to QI for a."); SkTScopedComPtr iunkB; - if (FAILED(b->QueryInterface(&iunkB))) { - return false; - } + HRM(b->QueryInterface(&iunkB), "Failed to QI for b."); - return iunkA.get() == iunkB.get(); + same = (iunkA.get() == iunkB.get()); + return S_OK; } -static bool FindByDWriteFont(SkTypeface* face, SkTypeface::Style, void* ctx) { +struct ProtoDWriteTypeface { + IDWriteFontFace* fDWriteFontFace; + IDWriteFont* fDWriteFont; + IDWriteFontFamily* fDWriteFontFamily; +}; + +static bool FindByDWriteFont(SkTypeface* cached, SkTypeface::Style, void* ctx) { + DWriteFontTypeface* cshFace = reinterpret_cast(cached); + ProtoDWriteTypeface* ctxFace = reinterpret_cast(ctx); + bool same; + //Check to see if the two fonts are identical. - DWriteFontTypeface* dwFace = reinterpret_cast(face); - IDWriteFont* dwFont = reinterpret_cast(ctx); - if (are_same(dwFace->fDWriteFont.get(), dwFont)) { + HRB(are_same(cshFace->fDWriteFont.get(), ctxFace->fDWriteFont, same)); + if (same) { return true; } - //Check if the two fonts share the same loader and have the same key. - SkTScopedComPtr dwFaceFontFace; - SkTScopedComPtr dwFontFace; - HRB(dwFace->fDWriteFont->CreateFontFace(&dwFaceFontFace)); - HRB(dwFont->CreateFontFace(&dwFontFace)); - if (are_same(dwFaceFontFace.get(), dwFontFace.get())) { + HRB(are_same(cshFace->fDWriteFontFace.get(), ctxFace->fDWriteFontFace, same)); + if (same) { return true; } - UINT32 dwFaceNumFiles; - UINT32 dwNumFiles; - HRB(dwFaceFontFace->GetFiles(&dwFaceNumFiles, NULL)); - HRB(dwFontFace->GetFiles(&dwNumFiles, NULL)); - if (dwFaceNumFiles != dwNumFiles) { + //Check if the two fonts share the same loader and have the same key. + UINT32 cshNumFiles; + UINT32 ctxNumFiles; + HRB(cshFace->fDWriteFontFace->GetFiles(&cshNumFiles, NULL)); + HRB(ctxFace->fDWriteFontFace->GetFiles(&ctxNumFiles, NULL)); + if (cshNumFiles != ctxNumFiles) { return false; } - SkTScopedComPtr dwFaceFontFile; - SkTScopedComPtr dwFontFile; - HRB(dwFaceFontFace->GetFiles(&dwFaceNumFiles, &dwFaceFontFile)); - HRB(dwFontFace->GetFiles(&dwNumFiles, &dwFontFile)); + SkTScopedComPtr cshFontFile; + SkTScopedComPtr ctxFontFile; + HRB(cshFace->fDWriteFontFace->GetFiles(&cshNumFiles, &cshFontFile)); + HRB(ctxFace->fDWriteFontFace->GetFiles(&ctxNumFiles, &ctxFontFile)); //for (each file) { //we currently only admit fonts from one file. - SkTScopedComPtr dwFaceFontFileLoader; - SkTScopedComPtr dwFontFileLoader; - HRB(dwFaceFontFile->GetLoader(&dwFaceFontFileLoader)); - HRB(dwFontFile->GetLoader(&dwFontFileLoader)); - if (!are_same(dwFaceFontFileLoader.get(), dwFontFileLoader.get())) { + SkTScopedComPtr cshFontFileLoader; + SkTScopedComPtr ctxFontFileLoader; + HRB(cshFontFile->GetLoader(&cshFontFileLoader)); + HRB(ctxFontFile->GetLoader(&ctxFontFileLoader)); + HRB(are_same(cshFontFileLoader.get(), ctxFontFileLoader.get(), same)); + if (!same) { return false; } //} - const void* dwFaceFontRefKey; - UINT32 dwFaceFontRefKeySize; - const void* dwFontRefKey; - UINT32 dwFontRefKeySize; - HRB(dwFaceFontFile->GetReferenceKey(&dwFaceFontRefKey, &dwFaceFontRefKeySize)); - HRB(dwFontFile->GetReferenceKey(&dwFontRefKey, &dwFontRefKeySize)); - if (dwFaceFontRefKeySize != dwFontRefKeySize) { + const void* cshRefKey; + UINT32 cshRefKeySize; + const void* ctxRefKey; + UINT32 ctxRefKeySize; + HRB(cshFontFile->GetReferenceKey(&cshRefKey, &cshRefKeySize)); + HRB(ctxFontFile->GetReferenceKey(&ctxRefKey, &ctxRefKeySize)); + if (cshRefKeySize != ctxRefKeySize) { return false; } - if (0 != memcmp(dwFaceFontRefKey, dwFontRefKey, dwFontRefKeySize)) { + if (0 != memcmp(cshRefKey, ctxRefKey, ctxRefKeySize)) { return false; } //TODO: better means than comparing name strings? - //NOTE: .tfc and fake bold/italic will end up here. - SkTScopedComPtr dwFaceFontFamily; - SkTScopedComPtr dwFontFamily; - HRB(dwFace->fDWriteFont->GetFontFamily(&dwFaceFontFamily)); - HRB(dwFont->GetFontFamily(&dwFontFamily)); - - SkTScopedComPtr dwFaceFontFamilyNames; - SkTScopedComPtr dwFaceFontNames; - HRB(dwFaceFontFamily->GetFamilyNames(&dwFaceFontFamilyNames)); - HRB(dwFace->fDWriteFont->GetFaceNames(&dwFaceFontNames)); - - SkTScopedComPtr dwFontFamilyNames; - SkTScopedComPtr dwFontNames; - HRB(dwFontFamily->GetFamilyNames(&dwFontFamilyNames)); - HRB(dwFont->GetFaceNames(&dwFontNames)); - - UINT32 dwFaceFontFamilyNameLength; - UINT32 dwFaceFontNameLength; - HRB(dwFaceFontFamilyNames->GetStringLength(0, &dwFaceFontFamilyNameLength)); - HRB(dwFaceFontNames->GetStringLength(0, &dwFaceFontNameLength)); - - UINT32 dwFontFamilyNameLength; - UINT32 dwFontNameLength; - HRB(dwFontFamilyNames->GetStringLength(0, &dwFontFamilyNameLength)); - HRB(dwFontNames->GetStringLength(0, &dwFontNameLength)); - - if (dwFaceFontFamilyNameLength != dwFontFamilyNameLength || - dwFaceFontNameLength != dwFontNameLength) + //NOTE: .ttc and fake bold/italic will end up here. + SkTScopedComPtr cshFamilyNames; + SkTScopedComPtr cshFaceNames; + HRB(cshFace->fDWriteFontFamily->GetFamilyNames(&cshFamilyNames)); + HRB(cshFace->fDWriteFont->GetFaceNames(&cshFaceNames)); + UINT32 cshFamilyNameLength; + UINT32 cshFaceNameLength; + HRB(cshFamilyNames->GetStringLength(0, &cshFamilyNameLength)); + HRB(cshFaceNames->GetStringLength(0, &cshFaceNameLength)); + + SkTScopedComPtr ctxFamilyNames; + SkTScopedComPtr ctxFaceNames; + HRB(ctxFace->fDWriteFontFamily->GetFamilyNames(&ctxFamilyNames)); + HRB(ctxFace->fDWriteFont->GetFaceNames(&ctxFaceNames)); + UINT32 ctxFamilyNameLength; + UINT32 ctxFaceNameLength; + HRB(ctxFamilyNames->GetStringLength(0, &ctxFamilyNameLength)); + HRB(ctxFaceNames->GetStringLength(0, &ctxFaceNameLength)); + + if (cshFamilyNameLength != ctxFamilyNameLength || + cshFaceNameLength != ctxFaceNameLength) { return false; } - SkSMallocWCHAR dwFaceFontFamilyNameChar(dwFaceFontFamilyNameLength+1); - SkSMallocWCHAR dwFaceFontNameChar(dwFaceFontNameLength+1); - HRB(dwFaceFontFamilyNames->GetString(0, dwFaceFontFamilyNameChar.get(), dwFaceFontFamilyNameLength+1)); - HRB(dwFaceFontNames->GetString(0, dwFaceFontNameChar.get(), dwFaceFontNameLength+1)); + SkSMallocWCHAR cshFamilyName(cshFamilyNameLength+1); + SkSMallocWCHAR cshFaceName(cshFaceNameLength+1); + HRB(cshFamilyNames->GetString(0, cshFamilyName.get(), cshFamilyNameLength+1)); + HRB(cshFaceNames->GetString(0, cshFaceName.get(), cshFaceNameLength+1)); - SkSMallocWCHAR dwFontFamilyNameChar(dwFontFamilyNameLength+1); - SkSMallocWCHAR dwFontNameChar(dwFontNameLength+1); - HRB(dwFontFamilyNames->GetString(0, dwFontFamilyNameChar.get(), dwFontFamilyNameLength+1)); - HRB(dwFontNames->GetString(0, dwFontNameChar.get(), dwFontNameLength+1)); + SkSMallocWCHAR ctxFamilyName(ctxFamilyNameLength+1); + SkSMallocWCHAR ctxFaceName(ctxFaceNameLength+1); + HRB(ctxFamilyNames->GetString(0, ctxFamilyName.get(), ctxFamilyNameLength+1)); + HRB(ctxFaceNames->GetString(0, ctxFaceName.get(), ctxFaceNameLength+1)); - return wcscmp(dwFaceFontFamilyNameChar.get(), dwFontFamilyNameChar.get()) == 0 && - wcscmp(dwFaceFontNameChar.get(), dwFontNameChar.get()) == 0; + return wcscmp(cshFamilyName.get(), ctxFamilyName.get()) == 0 && + wcscmp(cshFaceName.get(), ctxFaceName.get()) == 0; } SkTypeface* SkFontMgr_DirectWrite::createTypefaceFromDWriteFont( IDWriteFontFace* fontFace, IDWriteFont* font, IDWriteFontFamily* fontFamily) const { - SkTypeface* face = FindByProcAndRef(FindByDWriteFont, font); + SkAutoMutexAcquire ama(fTFCacheMutex); + ProtoDWriteTypeface spec = { fontFace, font, fontFamily }; + SkTypeface* face = fTFCache.findByProcAndRef(FindByDWriteFont, &spec); if (NULL == face) { face = DWriteFontTypeface::Create(fFactory.get(), fontFace, font, fontFamily); if (face) { - Add(face, get_style(font), true); + fTFCache.add(face, get_style(font), true); } } return face; -- 2.7.4