diff options
author | Seigo Nonaka <nona@google.com> | 2016-04-11 17:53:34 +0900 |
---|---|---|
committer | Raph Levien <raph@google.com> | 2016-04-11 13:33:35 -0700 |
commit | 6c60831cfce24b0749f50f37231e0a56d8fd4b85 (patch) | |
tree | 6f389a2979b0585148168a6e6ea1ad91ee655272 | |
parent | 29abb82198868908ece4600284fa8b7d3ed73f3b (diff) | |
download | android_frameworks_minikin-6c60831cfce24b0749f50f37231e0a56d8fd4b85.tar.gz android_frameworks_minikin-6c60831cfce24b0749f50f37231e0a56d8fd4b85.tar.bz2 android_frameworks_minikin-6c60831cfce24b0749f50f37231e0a56d8fd4b85.zip |
Fix minikin_unittests
This CL fixes following test cases in minikin_tests
- FontFamilyTest.hasVariationSelectorTest
- HbFontCacheTest.getHbFontLockedTest
- HbFontCacheTest.purgeCacheTest
For the fix of FontFamilyTest.hasVariationSelectorTest, removing virtual
from GetUniqueId() in MinikinFont. After [1], MinikinFont's destructor
started calling purgeHbCache() which calls virtual method,
MinikinFont::GetUniqueId(). Fortunately, the SkTypeface::uniqueID()
returns just internal value, so we can store it at the construction time
and use it instead of calling SkTypeface::uniqueID() every time.
This patch also changes purgeHbFont to purgeHbFontLocked, as all uses of
it were already under global mutex. This change avoids deadlock on
explicit unref, as when invoked by a Java finalizer from the Java object
that holds a reference to the font.
Some of the tests needed to change to using the ref counting protocol
rather than explicitly destructing font objects, as well.
[1] 9afcc6e2bd4d89e4e1deb6e18c3c4daca4e114fd
Bug: 28105730
Bug: 28105688
Change-Id: Ie5983c4869147dacabdca81af1605066cd680b3f
-rw-r--r-- | include/minikin/MinikinFont.h | 8 | ||||
-rw-r--r-- | include/minikin/MinikinFontFreeType.h | 3 | ||||
-rw-r--r-- | include/minikin/MinikinRefCounted.h | 17 | ||||
-rw-r--r-- | libs/minikin/HbFontCache.cpp | 4 | ||||
-rw-r--r-- | libs/minikin/HbFontCache.h | 2 | ||||
-rw-r--r-- | libs/minikin/MinikinFont.cpp | 2 | ||||
-rw-r--r-- | libs/minikin/MinikinFontFreeType.cpp | 6 | ||||
-rw-r--r-- | sample/MinikinSkia.cpp | 5 | ||||
-rw-r--r-- | sample/MinikinSkia.h | 2 | ||||
-rw-r--r-- | tests/FontFamilyTest.cpp | 37 | ||||
-rw-r--r-- | tests/MinikinFontForTest.cpp | 14 | ||||
-rw-r--r-- | tests/MinikinFontForTest.h | 2 | ||||
-rw-r--r-- | tests/how_to_run.txt | 2 |
13 files changed, 58 insertions, 46 deletions
diff --git a/include/minikin/MinikinFont.h b/include/minikin/MinikinFont.h index 0298235..4951514 100644 --- a/include/minikin/MinikinFont.h +++ b/include/minikin/MinikinFont.h @@ -99,6 +99,8 @@ typedef void (*MinikinDestroyFunc) (void* data); class MinikinFont : public MinikinRefCounted { public: + MinikinFont(int32_t uniqueId) : mUniqueId(uniqueId) {} + virtual ~MinikinFont(); virtual float GetHorizontalAdvance(uint32_t glyph_id, @@ -125,12 +127,14 @@ public: return 0; } - virtual int32_t GetUniqueId() const = 0; - static uint32_t MakeTag(char c1, char c2, char c3, char c4) { return ((uint32_t)c1 << 24) | ((uint32_t)c2 << 16) | ((uint32_t)c3 << 8) | (uint32_t)c4; } + + int32_t GetUniqueId() const { return mUniqueId; } +private: + const int32_t mUniqueId; }; } // namespace android diff --git a/include/minikin/MinikinFontFreeType.h b/include/minikin/MinikinFontFreeType.h index 535c249..baa08df 100644 --- a/include/minikin/MinikinFontFreeType.h +++ b/include/minikin/MinikinFontFreeType.h @@ -52,8 +52,6 @@ public: // TODO: provide access to raw data, as an optimization. - int32_t GetUniqueId() const; - // Not a virtual method, as the protocol to access rendered // glyph bitmaps is probably different depending on the // backend. @@ -64,7 +62,6 @@ public: private: FT_Face mTypeface; - int32_t mUniqueId; static int32_t sIdCounter; }; diff --git a/include/minikin/MinikinRefCounted.h b/include/minikin/MinikinRefCounted.h index 74d27fe..603aff0 100644 --- a/include/minikin/MinikinRefCounted.h +++ b/include/minikin/MinikinRefCounted.h @@ -37,6 +37,23 @@ private: int mRefcount_; }; +// An RAII container for reference counted objects. +// Note: this is only suitable for clients which are _not_ holding the global lock. +template <typename T> +class MinikinAutoUnref { +public: + MinikinAutoUnref(T* obj) : mObj(obj) { + } + ~MinikinAutoUnref() { + mObj->Unref(); + } + T& operator*() const { return *mObj; } + T* operator->() const { return mObj; } + T* get() const { return mObj; } +private: + T* mObj; +}; + } #endif // MINIKIN_REF_COUNTED_H
\ No newline at end of file diff --git a/libs/minikin/HbFontCache.cpp b/libs/minikin/HbFontCache.cpp index 9544dc2..3be942d 100644 --- a/libs/minikin/HbFontCache.cpp +++ b/libs/minikin/HbFontCache.cpp @@ -91,8 +91,8 @@ void purgeHbFontCacheLocked() { getFontCacheLocked()->clear(); } -void purgeHbFont(const MinikinFont* minikinFont) { - AutoMutex _l(gMinikinLock); +void purgeHbFontLocked(const MinikinFont* minikinFont) { + assertMinikinLocked(); const int32_t fontId = minikinFont->GetUniqueId(); getFontCacheLocked()->remove(fontId); } diff --git a/libs/minikin/HbFontCache.h b/libs/minikin/HbFontCache.h index 92e465a..449b354 100644 --- a/libs/minikin/HbFontCache.h +++ b/libs/minikin/HbFontCache.h @@ -23,7 +23,7 @@ namespace android { class MinikinFont; void purgeHbFontCacheLocked(); -void purgeHbFont(const MinikinFont* minikinFont); +void purgeHbFontLocked(const MinikinFont* minikinFont); hb_font_t* getHbFontLocked(MinikinFont* minikinFont); } // namespace android diff --git a/libs/minikin/MinikinFont.cpp b/libs/minikin/MinikinFont.cpp index d56ec9c..ef42e9b 100644 --- a/libs/minikin/MinikinFont.cpp +++ b/libs/minikin/MinikinFont.cpp @@ -20,7 +20,7 @@ namespace android { MinikinFont::~MinikinFont() { - purgeHbFont(this); + purgeHbFontLocked(this); } } // namespace android diff --git a/libs/minikin/MinikinFontFreeType.cpp b/libs/minikin/MinikinFontFreeType.cpp index b9ea5d7..4a1b115 100644 --- a/libs/minikin/MinikinFontFreeType.cpp +++ b/libs/minikin/MinikinFontFreeType.cpp @@ -30,8 +30,8 @@ namespace android { int32_t MinikinFontFreeType::sIdCounter = 0; MinikinFontFreeType::MinikinFontFreeType(FT_Face typeface) : + MinikinFont(sIdCounter++), mTypeface(typeface) { - mUniqueId = sIdCounter++; } MinikinFontFreeType::~MinikinFontFreeType() { @@ -72,10 +72,6 @@ const void* MinikinFontFreeType::GetTable(uint32_t tag, size_t* size, MinikinDes return buf; } -int32_t MinikinFontFreeType::GetUniqueId() const { - return mUniqueId; -} - bool MinikinFontFreeType::Render(uint32_t glyph_id, const MinikinPaint& /* paint */, GlyphBitmap *result) { FT_Error error; diff --git a/sample/MinikinSkia.cpp b/sample/MinikinSkia.cpp index c4971bb..e2ecde0 100644 --- a/sample/MinikinSkia.cpp +++ b/sample/MinikinSkia.cpp @@ -7,6 +7,7 @@ namespace android { MinikinFontSkia::MinikinFontSkia(SkTypeface *typeface) : + MinikinFont(typeface->uniqueID()), mTypeface(typeface) { } @@ -67,8 +68,4 @@ SkTypeface *MinikinFontSkia::GetSkTypeface() { return mTypeface; } -int32_t MinikinFontSkia::GetUniqueId() const { - return mTypeface->uniqueID(); -} - } diff --git a/sample/MinikinSkia.h b/sample/MinikinSkia.h index e1d7bf6..6eb9065 100644 --- a/sample/MinikinSkia.h +++ b/sample/MinikinSkia.h @@ -14,8 +14,6 @@ public: const void* GetTable(uint32_t tag, size_t* size, MinikinDestroyFunc* destroy); - int32_t GetUniqueId() const; - SkTypeface *GetSkTypeface(); private: diff --git a/tests/FontFamilyTest.cpp b/tests/FontFamilyTest.cpp index 907f395..194063f 100644 --- a/tests/FontFamilyTest.cpp +++ b/tests/FontFamilyTest.cpp @@ -350,9 +350,9 @@ void expectVSGlyphs(FontFamily* family, uint32_t codepoint, const std::set<uint3 } TEST_F(FontFamilyTest, hasVariationSelectorTest) { - MinikinFontForTest minikinFont(kVsTestFont); - FontFamily family; - family.addFont(&minikinFont); + MinikinAutoUnref<MinikinFontForTest> minikinFont(new MinikinFontForTest(kVsTestFont)); + MinikinAutoUnref<FontFamily> family(new FontFamily); + family->addFont(minikinFont.get()); AutoMutex _l(gMinikinLock); @@ -365,24 +365,24 @@ TEST_F(FontFamilyTest, hasVariationSelectorTest) { const uint32_t kVS20 = 0xE0103; const uint32_t kSupportedChar1 = 0x82A6; - EXPECT_TRUE(family.getCoverage()->get(kSupportedChar1)); - expectVSGlyphs(&family, kSupportedChar1, std::set<uint32_t>({kVS1, kVS17, kVS18, kVS19})); + EXPECT_TRUE(family->getCoverage()->get(kSupportedChar1)); + expectVSGlyphs(family.get(), kSupportedChar1, std::set<uint32_t>({kVS1, kVS17, kVS18, kVS19})); const uint32_t kSupportedChar2 = 0x845B; - EXPECT_TRUE(family.getCoverage()->get(kSupportedChar2)); - expectVSGlyphs(&family, kSupportedChar2, std::set<uint32_t>({kVS2, kVS18, kVS19, kVS20})); + EXPECT_TRUE(family->getCoverage()->get(kSupportedChar2)); + expectVSGlyphs(family.get(), kSupportedChar2, std::set<uint32_t>({kVS2, kVS18, kVS19, kVS20})); const uint32_t kNoVsSupportedChar = 0x537F; - EXPECT_TRUE(family.getCoverage()->get(kNoVsSupportedChar)); - expectVSGlyphs(&family, kNoVsSupportedChar, std::set<uint32_t>()); + EXPECT_TRUE(family->getCoverage()->get(kNoVsSupportedChar)); + expectVSGlyphs(family.get(), kNoVsSupportedChar, std::set<uint32_t>()); const uint32_t kVsOnlySupportedChar = 0x717D; - EXPECT_FALSE(family.getCoverage()->get(kVsOnlySupportedChar)); - expectVSGlyphs(&family, kVsOnlySupportedChar, std::set<uint32_t>({kVS3, kVS19, kVS20})); + EXPECT_FALSE(family->getCoverage()->get(kVsOnlySupportedChar)); + expectVSGlyphs(family.get(), kVsOnlySupportedChar, std::set<uint32_t>({kVS3, kVS19, kVS20})); const uint32_t kNotSupportedChar = 0x845C; - EXPECT_FALSE(family.getCoverage()->get(kNotSupportedChar)); - expectVSGlyphs(&family, kNotSupportedChar, std::set<uint32_t>()); + EXPECT_FALSE(family->getCoverage()->get(kNotSupportedChar)); + expectVSGlyphs(family.get(), kNotSupportedChar, std::set<uint32_t>()); } TEST_F(FontFamilyTest, hasVSTableTest) { @@ -403,12 +403,13 @@ TEST_F(FontFamilyTest, hasVSTableTest) { "Font " + testCase.fontPath + " should have a variation sequence table." : "Font " + testCase.fontPath + " shouldn't have a variation sequence table."); - MinikinFontForTest minikinFont(testCase.fontPath); - FontFamily family; - family.addFont(&minikinFont); - family.getCoverage(); + MinikinAutoUnref<MinikinFontForTest> minikinFont(new MinikinFontForTest(testCase.fontPath)); + MinikinAutoUnref<FontFamily> family(new FontFamily); + family->addFont(minikinFont.get()); + AutoMutex _l(gMinikinLock); + family->getCoverage(); - EXPECT_EQ(testCase.hasVSTable, family.hasVSTable()); + EXPECT_EQ(testCase.hasVSTable, family->hasVSTable()); } } diff --git a/tests/MinikinFontForTest.cpp b/tests/MinikinFontForTest.cpp index 96f3326..66dd4ea 100644 --- a/tests/MinikinFontForTest.cpp +++ b/tests/MinikinFontForTest.cpp @@ -22,8 +22,14 @@ #include <cutils/log.h> -MinikinFontForTest::MinikinFontForTest(const std::string& font_path) : mFontPath(font_path) { - mTypeface = SkTypeface::CreateFromFile(font_path.c_str()); +MinikinFontForTest::MinikinFontForTest(const std::string& font_path) : + MinikinFontForTest(font_path, SkTypeface::CreateFromFile(font_path.c_str())) { +} + +MinikinFontForTest::MinikinFontForTest(const std::string& font_path, SkTypeface* typeface) : + MinikinFont(typeface->uniqueID()), + mTypeface(typeface), + mFontPath(font_path) { } MinikinFontForTest::~MinikinFontForTest() { @@ -55,7 +61,3 @@ const void* MinikinFontForTest::GetTable(uint32_t tag, size_t* size, *destroy = free; return buf; } - -int32_t MinikinFontForTest::GetUniqueId() const { - return mTypeface->uniqueID(); -} diff --git a/tests/MinikinFontForTest.h b/tests/MinikinFontForTest.h index 4686f7a..e527d21 100644 --- a/tests/MinikinFontForTest.h +++ b/tests/MinikinFontForTest.h @@ -24,6 +24,7 @@ class SkTypeface; class MinikinFontForTest : public android::MinikinFont { public: explicit MinikinFontForTest(const std::string& font_path); + MinikinFontForTest(const std::string& font_path, SkTypeface* typeface); ~MinikinFontForTest(); // MinikinFont overrides. @@ -31,7 +32,6 @@ public: void GetBounds(android::MinikinRect* bounds, uint32_t glyph_id, const android::MinikinPaint& paint) const; const void* GetTable(uint32_t tag, size_t* size, android::MinikinDestroyFunc* destroy); - int32_t GetUniqueId() const; const std::string& fontPath() const { return mFontPath; } private: diff --git a/tests/how_to_run.txt b/tests/how_to_run.txt index a135c30..bee367b 100644 --- a/tests/how_to_run.txt +++ b/tests/how_to_run.txt @@ -1,5 +1,5 @@ mmm -j8 frameworks/minikin/tests && adb push $OUT/data/nativetest/minikin_tests/minikin_tests \ /data/nativetest/minikin_tests/minikin_tests && -adb push frameworks/minikin/tests/data /data/nativetest/minikin_tests/data && +adb push frameworks/minikin/tests/data /data/nativetest/minikin_tests/ && adb shell /data/nativetest/minikin_tests/minikin_tests |