From fb0d396929e534a3686469b474d4f670864aa5ac Mon Sep 17 00:00:00 2001 From: Seigo Nonaka Date: Thu, 17 Sep 2015 17:12:13 +0900 Subject: Extract hb_face_t object cache mechanism from Layout.cpp. This CL does following things: - Extract hb_face_t object cache mechanism from Layout.cpp to be able to use it from other cpp file, especially from FontFamily.cpp. To address Bug 11256006 and Bug 17759267, need to touch hb_face_t from FontFamily. - Make hb_face_t cache mechanism thread-safe. - Add unit tests for HbFaceCache test cases. Bug: 11256006 Bug: 17759267 Change-Id: Ic183634ef34326793bd9a32167236611d0af34d6 --- libs/minikin/Android.mk | 1 + libs/minikin/HbFaceCache.cpp | 119 +++++++++++++++++++++++++++++++++++++ libs/minikin/HbFaceCache.h | 29 +++++++++ libs/minikin/Layout.cpp | 58 +----------------- libs/minikin/MinikinInternal.cpp | 6 ++ libs/minikin/MinikinInternal.h | 3 + tests/Android.mk | 4 +- tests/HbFaceCacheTest.cpp | 123 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 287 insertions(+), 56 deletions(-) create mode 100644 libs/minikin/HbFaceCache.cpp create mode 100644 libs/minikin/HbFaceCache.h create mode 100644 tests/HbFaceCacheTest.cpp diff --git a/libs/minikin/Android.mk b/libs/minikin/Android.mk index 765a3dd..fab5586 100644 --- a/libs/minikin/Android.mk +++ b/libs/minikin/Android.mk @@ -22,6 +22,7 @@ minikin_src_files := \ FontCollection.cpp \ FontFamily.cpp \ GraphemeBreak.cpp \ + HbFaceCache.cpp \ Hyphenator.cpp \ Layout.cpp \ LayoutUtils.cpp \ diff --git a/libs/minikin/HbFaceCache.cpp b/libs/minikin/HbFaceCache.cpp new file mode 100644 index 0000000..fde2fa8 --- /dev/null +++ b/libs/minikin/HbFaceCache.cpp @@ -0,0 +1,119 @@ +/* + * Copyright (C) 2015 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#define LOG_TAG "Minikin" + +#include "HbFaceCache.h" + +#include +#include + +#include +#include "MinikinInternal.h" + +namespace android { + +static hb_blob_t* referenceTable(hb_face_t* face, hb_tag_t tag, void* userData) { + MinikinFont* font = reinterpret_cast(userData); + size_t length = 0; + bool ok = font->GetTable(tag, NULL, &length); + if (!ok) { + return 0; + } + char* buffer = reinterpret_cast(malloc(length)); + if (!buffer) { + return 0; + } + ok = font->GetTable(tag, reinterpret_cast(buffer), &length); +#ifdef VERBOSE_DEBUG + ALOGD("referenceTable %c%c%c%c length=%d %d", + (tag >>24)&0xff, (tag>>16)&0xff, (tag>>8)&0xff, tag&0xff, length, ok); +#endif + if (!ok) { + free(buffer); + return 0; + } + return hb_blob_create(const_cast(buffer), length, + HB_MEMORY_MODE_WRITABLE, buffer, free); +} + +static unsigned int disabledDecomposeCompatibility( + hb_unicode_funcs_t*, hb_codepoint_t, hb_codepoint_t*, void*) { + return 0; +} + +class HbFaceCache : private OnEntryRemoved { +public: + HbFaceCache() : mCache(kMaxEntries) { + mCache.setOnEntryRemovedListener(this); + } + + // callback for OnEntryRemoved + void operator()(int32_t& key, hb_face_t*& value) { + hb_face_destroy(value); + } + + hb_face_t* get(int32_t fontId) { + return mCache.get(fontId); + } + + void put(int32_t fontId, hb_face_t* face) { + mCache.put(fontId, face); + } + + void clear() { + mCache.clear(); + } + +private: + static const size_t kMaxEntries = 100; + + LruCache mCache; +}; + +HbFaceCache* getFaceCacheLocked() { + assertMinikinLocked(); + static HbFaceCache* cache = nullptr; + if (cache == nullptr) { + cache = new HbFaceCache(); + } + return cache; +} + +void purgeHbFaceCacheLocked() { + assertMinikinLocked(); + getFaceCacheLocked()->clear(); +} + +hb_face_t* getHbFaceLocked(MinikinFont* minikinFont) { + assertMinikinLocked(); + if (minikinFont == nullptr) { + return nullptr; + } + + HbFaceCache* faceCache = getFaceCacheLocked(); + const int32_t fontId = minikinFont->GetUniqueId(); + hb_face_t* face = faceCache->get(fontId); + if (face != nullptr) { + return face; + } + + face = hb_face_create_for_tables(referenceTable, minikinFont, nullptr); + faceCache->put(fontId, face); + return face; +} + +} // namespace android diff --git a/libs/minikin/HbFaceCache.h b/libs/minikin/HbFaceCache.h new file mode 100644 index 0000000..8ee38c3 --- /dev/null +++ b/libs/minikin/HbFaceCache.h @@ -0,0 +1,29 @@ +/* + * Copyright (C) 2015 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef MINIKIN_HBFACE_CACHE_H +#define MINIKIN_HBFACE_CACHE_H + +struct hb_face_t; + +namespace android { +class MinikinFont; + +void purgeHbFaceCacheLocked(); +hb_face_t* getHbFaceLocked(MinikinFont* minikinFont); + +} // namespace android +#endif // MINIKIN_HBFACE_CACHE_H diff --git a/libs/minikin/Layout.cpp b/libs/minikin/Layout.cpp index eaa2b7d..65a8a75 100644 --- a/libs/minikin/Layout.cpp +++ b/libs/minikin/Layout.cpp @@ -35,6 +35,7 @@ #include #include "LayoutUtils.h" +#include "HbFaceCache.h" #include "MinikinInternal.h" #include #include @@ -188,22 +189,6 @@ private: static const size_t kMaxEntries = 5000; }; -class HbFaceCache : private OnEntryRemoved { -public: - HbFaceCache() : mCache(kMaxEntries) { - mCache.setOnEntryRemovedListener(this); - } - - // callback for OnEntryRemoved - void operator()(int32_t& key, hb_face_t*& value) { - hb_face_destroy(value); - } - - LruCache mCache; -private: - static const size_t kMaxEntries = 100; -}; - static unsigned int disabledDecomposeCompatibility(hb_unicode_funcs_t*, hb_codepoint_t, hb_codepoint_t*, void*) { return 0; @@ -223,7 +208,6 @@ public: hb_buffer_t* hbBuffer; hb_unicode_funcs_t* unicodeFunctions; LayoutCache layoutCache; - HbFaceCache hbFaceCache; }; ANDROID_SINGLETON_STATIC_INSTANCE(LayoutEngine); @@ -291,30 +275,6 @@ void Layout::setFontCollection(const FontCollection* collection) { mCollection = collection; } -hb_blob_t* referenceTable(hb_face_t* face, hb_tag_t tag, void* userData) { - MinikinFont* font = reinterpret_cast(userData); - size_t length = 0; - bool ok = font->GetTable(tag, NULL, &length); - if (!ok) { - return 0; - } - char* buffer = reinterpret_cast(malloc(length)); - if (!buffer) { - return 0; - } - ok = font->GetTable(tag, reinterpret_cast(buffer), &length); -#ifdef VERBOSE_DEBUG - ALOGD("referenceTable %c%c%c%c length=%d %d", - (tag >>24) & 0xff, (tag>>16)&0xff, (tag>>8)&0xff, tag&0xff, length, ok); -#endif - if (!ok) { - free(buffer); - return 0; - } - return hb_blob_create(const_cast(buffer), length, - HB_MEMORY_MODE_WRITABLE, buffer, free); -} - static hb_position_t harfbuzzGetGlyphHorizontalAdvance(hb_font_t* hbFont, void* fontData, hb_codepoint_t glyph, void* userData) { MinikinPaint* paint = reinterpret_cast(fontData); @@ -342,19 +302,8 @@ hb_font_funcs_t* getHbFontFuncs() { return hbFontFuncs; } -static hb_face_t* getHbFace(MinikinFont* minikinFont) { - HbFaceCache& cache = LayoutEngine::getInstance().hbFaceCache; - int32_t fontId = minikinFont->GetUniqueId(); - hb_face_t* face = cache.mCache.get(fontId); - if (face == NULL) { - face = hb_face_create_for_tables(referenceTable, minikinFont, NULL); - cache.mCache.put(fontId, face); - } - return face; -} - static hb_font_t* create_hb_font(MinikinFont* minikinFont, MinikinPaint* minikinPaint) { - hb_face_t* face = getHbFace(minikinFont); + hb_face_t* face = getHbFaceLocked(minikinFont); hb_font_t* parent_font = hb_font_create(face); hb_ot_font_set_funcs(parent_font); @@ -885,8 +834,7 @@ void Layout::purgeCaches() { AutoMutex _l(gMinikinLock); LayoutCache& layoutCache = LayoutEngine::getInstance().layoutCache; layoutCache.clear(); - HbFaceCache& hbCache = LayoutEngine::getInstance().hbFaceCache; - hbCache.mCache.clear(); + purgeHbFaceCacheLocked(); } } // namespace android diff --git a/libs/minikin/MinikinInternal.cpp b/libs/minikin/MinikinInternal.cpp index 71c8649..5a21ef9 100644 --- a/libs/minikin/MinikinInternal.cpp +++ b/libs/minikin/MinikinInternal.cpp @@ -18,8 +18,14 @@ #include "MinikinInternal.h" +#include + namespace android { Mutex gMinikinLock; +void assertMinikinLocked() { + LOG_FATAL_IF(gMinikinLock.tryLock() == 0); +} + } diff --git a/libs/minikin/MinikinInternal.h b/libs/minikin/MinikinInternal.h index b8430df..34a95bb 100644 --- a/libs/minikin/MinikinInternal.h +++ b/libs/minikin/MinikinInternal.h @@ -29,6 +29,9 @@ namespace android { extern Mutex gMinikinLock; +// Aborts if gMinikinLock is not acquired. Do nothing on the release build. +void assertMinikinLocked(); + } #endif // MINIKIN_INTERNAL_H diff --git a/tests/Android.mk b/tests/Android.mk index 680cbe9..b4d5e91 100644 --- a/tests/Android.mk +++ b/tests/Android.mk @@ -43,12 +43,14 @@ LOCAL_SRC_FILES += \ FontTestUtils.cpp \ MinikinFontForTest.cpp \ GraphemeBreakTests.cpp \ + HbFaceCacheTest.cpp \ LayoutUtilsTest.cpp \ UnicodeUtils.cpp LOCAL_C_INCLUDES := \ $(LOCAL_PATH)/../libs/minikin/ \ + external/harfbuzz_ng/src \ external/libxml2/include \ - external/skia/src/core \ + external/skia/src/core include $(BUILD_NATIVE_TEST) diff --git a/tests/HbFaceCacheTest.cpp b/tests/HbFaceCacheTest.cpp new file mode 100644 index 0000000..1b63073 --- /dev/null +++ b/tests/HbFaceCacheTest.cpp @@ -0,0 +1,123 @@ +/* + * Copyright (C) 2015 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include "HbFaceCache.h" + +#include +#include +#include + +#include "MinikinInternal.h" +#include + +namespace android { +namespace { + +// A mock implementation of MinikinFont. The passed integer value will be +// returned in GetUniqueId(). +class MockMinikinFont : public MinikinFont { +public: + MockMinikinFont(int32_t id) : mId(id) { + } + + virtual bool GetGlyph(uint32_t codepoint, uint32_t *glyph) const { + LOG_ALWAYS_FATAL("MockMinikinFont::GetGlyph is not implemented."); + return false; + } + + virtual float GetHorizontalAdvance( + uint32_t glyph_id, const MinikinPaint &paint) const { + LOG_ALWAYS_FATAL("MockMinikinFont::GetHorizontalAdvance is not implemented."); + return 0.0f; + } + + virtual void GetBounds(MinikinRect* bounds, uint32_t glyph_id, + const MinikinPaint &paint) const { + LOG_ALWAYS_FATAL("MockMinikinFont::GetBounds is not implemented."); + } + + virtual bool GetTable(uint32_t tag, uint8_t *buf, size_t *size) { + LOG_ALWAYS_FATAL("MockMinikinFont::GetTable is not implemented."); + return false; + } + + virtual int32_t GetUniqueId() const { + return mId; + } + +private: + int32_t mId; +}; + +class HbFaceCacheTest : public testing::Test { +public: + virtual void TearDown() { + AutoMutex _l(gMinikinLock); + purgeHbFaceCacheLocked(); + } +}; + +TEST_F(HbFaceCacheTest, getHbFaceLockedTest) { + AutoMutex _l(gMinikinLock); + + MockMinikinFont fontA(1); + MockMinikinFont fontB(2); + MockMinikinFont fontC(2); + + // Never return NULL. + EXPECT_TRUE(getHbFaceLocked(&fontA)); + EXPECT_TRUE(getHbFaceLocked(&fontB)); + EXPECT_TRUE(getHbFaceLocked(&fontC)); + + // Must return same object if same font object is passed. + EXPECT_EQ(getHbFaceLocked(&fontA), getHbFaceLocked(&fontA)); + EXPECT_EQ(getHbFaceLocked(&fontB), getHbFaceLocked(&fontB)); + EXPECT_EQ(getHbFaceLocked(&fontC), getHbFaceLocked(&fontC)); + + // Different object must be returned if the passed minikinFont has different ID. + EXPECT_NE(getHbFaceLocked(&fontA), getHbFaceLocked(&fontB)); + EXPECT_NE(getHbFaceLocked(&fontA), getHbFaceLocked(&fontC)); + + // Same object must be returned if the minikinFont has same Id. + EXPECT_EQ(getHbFaceLocked(&fontB), getHbFaceLocked(&fontC)); +} + +TEST_F(HbFaceCacheTest, purgeCacheTest) { + AutoMutex _l(gMinikinLock); + MockMinikinFont font(1); + + hb_face_t* face = getHbFaceLocked(&font); + EXPECT_TRUE(face); + + // Set user data to identify the face object. + hb_user_data_key_t key; + void* data = (void*)0xdeadbeef; + hb_face_set_user_data(face, &key, data, NULL, false); + EXPECT_EQ(data, hb_face_get_user_data(face, &key)); + + purgeHbFaceCacheLocked(); + + // By checking user data, confirm that the object after purge is different from previously + // created one. Do not compare the returned pointer here since memory allocator may assign + // same region for new object. + face = getHbFaceLocked(&font); + EXPECT_EQ(nullptr, hb_face_get_user_data(face, &key)); +} + +} // namespace +} // namespace android -- cgit v1.2.3