Avoid copying of font table data

The hb_font_t object holds on to tables of font data, acquired through
the MinikinFont::GetTable interface, which is based on copying data
into caller-owned buffers. Now that we're caching lots of hb_font_t's,
the cost of these buffers is significant.

This patch moves to a different interface, inspired by HarfBuzz's
hb_reference_table API, where the font can provide a pointer to the
actual font data (which will often be mmap'ed, so it doesn't even
consume physical RAM).

Bug: 27860101
Change-Id: Id766ab16a8d342bf7322a90e076e801271d527d4

10 files changed, 137 insertions, 62 deletions

diff --git a/include/minikin/MinikinFont.h b/include/minikin/MinikinFont.h
index 5c0ab0f..0298235 100644
--- a/include/minikin/MinikinFont.h
+++ b/include/minikin/MinikinFont.h
@@ -94,6 +94,9 @@ struct MinikinRect {
 
 class MinikinFontFreeType;
 
+// Callback for freeing data
+typedef void (*MinikinDestroyFunc) (void* data);
+
 class MinikinFont : public MinikinRefCounted {
 public:
     virtual ~MinikinFont();
@@ -104,8 +107,23 @@ public:
     virtual void GetBounds(MinikinRect* bounds, uint32_t glyph_id,
         const MinikinPaint &paint) const = 0;
 
-    // If buf is NULL, just update size
-    virtual bool GetTable(uint32_t tag, uint8_t *buf, size_t *size) = 0;
+    virtual const void* GetTable(uint32_t tag, size_t* size, MinikinDestroyFunc* destroy) = 0;
+
+    // Override if font can provide access to raw data
+    virtual const void* GetFontData() const {
+        return nullptr;
+    }
+
+    // Override if font can provide access to raw data
+    virtual size_t GetFontSize() const {
+        return 0;
+    }
+
+    // Override if font can provide access to raw data.
+    // Returns index within OpenType collection
+    virtual int GetFontIndex() const {
+        return 0;
+    }
 
     virtual int32_t GetUniqueId() const = 0;
 
diff --git a/include/minikin/MinikinFontFreeType.h b/include/minikin/MinikinFontFreeType.h
index a957d12..535c249 100644
--- a/include/minikin/MinikinFontFreeType.h
+++ b/include/minikin/MinikinFontFreeType.h
@@ -48,8 +48,9 @@ public:
     void GetBounds(MinikinRect* bounds, uint32_t glyph_id,
         const MinikinPaint& paint) const;
 
-    // If buf is NULL, just update size
-    bool GetTable(uint32_t tag, uint8_t *buf, size_t *size);
+    const void* GetTable(uint32_t tag, size_t* size, MinikinDestroyFunc* destroy);
+
+    // TODO: provide access to raw data, as an optimization.
 
     int32_t GetUniqueId() const;
 
diff --git a/libs/minikin/FontFamily.cpp b/libs/minikin/FontFamily.cpp
index 88448a1..0f71f51 100644
--- a/libs/minikin/FontFamily.cpp
+++ b/libs/minikin/FontFamily.cpp
@@ -77,15 +77,11 @@ FontFamily::~FontFamily() {
 bool FontFamily::addFont(MinikinFont* typeface) {
     AutoMutex _l(gMinikinLock);
     const uint32_t os2Tag = MinikinFont::MakeTag('O', 'S', '/', '2');
-    size_t os2Size = 0;
-    bool ok = typeface->GetTable(os2Tag, NULL, &os2Size);
-    if (!ok) return false;
-    UniquePtr<uint8_t[]> os2Data(new uint8_t[os2Size]);
-    ok = typeface->GetTable(os2Tag, os2Data.get(), &os2Size);
-    if (!ok) return false;
+    HbBlob os2Table(getFontTable(typeface, os2Tag));
+    if (os2Table.get() == nullptr) return false;
     int weight;
     bool italic;
-    if (analyzeStyle(os2Data.get(), os2Size, &weight, &italic)) {
+    if (analyzeStyle(os2Table.get(), os2Table.size(), &weight, &italic)) {
         //ALOGD("analyzed weight = %d, italic = %s", weight, italic ? "true" : "false");
         FontStyle style(weight, italic);
         addFontLocked(typeface, style);
@@ -165,20 +161,15 @@ const SparseBitSet* FontFamily::getCoverage() {
         const FontStyle defaultStyle;
         MinikinFont* typeface = getClosestMatch(defaultStyle).font;
         const uint32_t cmapTag = MinikinFont::MakeTag('c', 'm', 'a', 'p');
-        size_t cmapSize = 0;
-        if (!typeface->GetTable(cmapTag, NULL, &cmapSize)) {
+        HbBlob cmapTable(getFontTable(typeface, cmapTag));
+        if (cmapTable.get() == nullptr) {
             ALOGE("Could not get cmap table size!\n");
             // Note: This means we will retry on the next call to getCoverage, as we can't store
             //       the failure. This is fine, as we assume this doesn't really happen in practice.
             return nullptr;
         }
-        UniquePtr<uint8_t[]> cmapData(new uint8_t[cmapSize]);
-        if (!typeface->GetTable(cmapTag, cmapData.get(), &cmapSize)) {
-            ALOGE("Unexpected failure to read cmap table!\n");
-            return nullptr;
-        }
         // TODO: Error check?
-        CmapCoverage::getCoverage(mCoverage, cmapData.get(), cmapSize, &mHasVSTable);
+        CmapCoverage::getCoverage(mCoverage, cmapTable.get(), cmapTable.size(), &mHasVSTable);
 #ifdef VERBOSE_DEBUG
         ALOGD("font coverage length=%d, first ch=%x\n", mCoverage.length(),
                 mCoverage.nextSetBit(0));
@@ -198,7 +189,9 @@ bool FontFamily::hasVariationSelector(uint32_t codepoint, uint32_t variationSele
     MinikinFont* minikinFont = getClosestMatch(defaultStyle).font;
     hb_font_t* font = getHbFontLocked(minikinFont);
     uint32_t unusedGlyph;
-    return hb_font_get_glyph(font, codepoint, variationSelector, &unusedGlyph);
+    bool result = hb_font_get_glyph(font, codepoint, variationSelector, &unusedGlyph);
+    hb_font_destroy(font);
+    return result;
 }
 
 bool FontFamily::hasVSTable() const {
diff --git a/libs/minikin/HbFontCache.cpp b/libs/minikin/HbFontCache.cpp
index 7a6b3c1..9544dc2 100644
--- a/libs/minikin/HbFontCache.cpp
+++ b/libs/minikin/HbFontCache.cpp
@@ -30,26 +30,18 @@ namespace android {
 
 static hb_blob_t* referenceTable(hb_face_t* /* face */, hb_tag_t tag, void* userData) {
     MinikinFont* font = reinterpret_cast<MinikinFont*>(userData);
-    size_t length = 0;
-    bool ok = font->GetTable(tag, NULL, &length);
-    if (!ok) {
-        return 0;
+    MinikinDestroyFunc destroy = 0;
+    size_t size = 0;
+    const void* buffer = font->GetTable(tag, &size, &destroy);
+    if (buffer == nullptr) {
+        return nullptr;
     }
-    char* buffer = reinterpret_cast<char*>(malloc(length));
-    if (!buffer) {
-        return 0;
-    }
-    ok = font->GetTable(tag, reinterpret_cast<uint8_t*>(buffer), &length);
 #ifdef VERBOSE_DEBUG
-    ALOGD("referenceTable %c%c%c%c length=%zd %d",
-        (tag >>24)&0xff, (tag>>16)&0xff, (tag>>8)&0xff, tag&0xff, length, ok);
+    ALOGD("referenceTable %c%c%c%c length=%zd",
+        (tag >>24)&0xff, (tag>>16)&0xff, (tag>>8)&0xff, tag&0xff, size);
 #endif
-    if (!ok) {
-        free(buffer);
-        return 0;
-    }
-    return hb_blob_create(const_cast<char*>(buffer), length,
-            HB_MEMORY_MODE_WRITABLE, buffer, free);
+    return hb_blob_create(reinterpret_cast<const char*>(buffer), size,
+            HB_MEMORY_MODE_READONLY, const_cast<void*>(buffer), destroy);
 }
 
 class HbFontCache : private OnEntryRemoved<int32_t, hb_font_t*> {
@@ -105,24 +97,37 @@ void purgeHbFont(const MinikinFont* minikinFont) {
     getFontCacheLocked()->remove(fontId);
 }
 
+// Returns a new reference to a hb_font_t object, caller is
+// responsible for calling hb_font_destroy() on it.
 hb_font_t* getHbFontLocked(MinikinFont* minikinFont) {
     assertMinikinLocked();
+    // TODO: get rid of nullFaceFont
     static hb_font_t* nullFaceFont = nullptr;
     if (minikinFont == nullptr) {
         if (nullFaceFont == nullptr) {
             nullFaceFont = hb_font_create(nullptr);
         }
-        return nullFaceFont;
+        return hb_font_reference(nullFaceFont);
     }
 
     HbFontCache* fontCache = getFontCacheLocked();
     const int32_t fontId = minikinFont->GetUniqueId();
     hb_font_t* font = fontCache->get(fontId);
     if (font != nullptr) {
-        return font;
+        return hb_font_reference(font);
     }
 
-    hb_face_t* face = hb_face_create_for_tables(referenceTable, minikinFont, nullptr);
+    hb_face_t* face;
+    const void* buf = minikinFont->GetFontData();
+    if (buf == nullptr) {
+        face = hb_face_create_for_tables(referenceTable, minikinFont, nullptr);
+    } else {
+        size_t size = minikinFont->GetFontSize();
+        hb_blob_t* blob = hb_blob_create(reinterpret_cast<const char*>(buf), size,
+            HB_MEMORY_MODE_READONLY, nullptr, nullptr);
+        face = hb_face_create(blob, minikinFont->GetFontIndex());
+        hb_blob_destroy(blob);
+    }
     hb_font_t* parent_font = hb_font_create(face);
     hb_ot_font_set_funcs(parent_font);
 
@@ -133,7 +138,7 @@ hb_font_t* getHbFontLocked(MinikinFont* minikinFont) {
     hb_font_destroy(parent_font);
     hb_face_destroy(face);
     fontCache->put(fontId, font);
-    return font;
+    return hb_font_reference(font);
 }
 
 }  // namespace android
diff --git a/libs/minikin/Layout.cpp b/libs/minikin/Layout.cpp
index 3a05acf..9c1d6a8 100644
--- a/libs/minikin/Layout.cpp
+++ b/libs/minikin/Layout.cpp
@@ -99,6 +99,7 @@ struct LayoutContext {
     void clearHbFonts() {
         for (size_t i = 0; i < hbFonts.size(); i++) {
             hb_font_set_funcs(hbFonts[i], nullptr, nullptr, nullptr);
+            hb_font_destroy(hbFonts[i]);
         }
         hbFonts.clear();
     }
diff --git a/libs/minikin/MinikinFontFreeType.cpp b/libs/minikin/MinikinFontFreeType.cpp
index c9eb456..b9ea5d7 100644
--- a/libs/minikin/MinikinFontFreeType.cpp
+++ b/libs/minikin/MinikinFontFreeType.cpp
@@ -41,8 +41,8 @@ MinikinFontFreeType::~MinikinFontFreeType() {
 float MinikinFontFreeType::GetHorizontalAdvance(uint32_t glyph_id,
     const MinikinPaint &paint) const {
     FT_Set_Pixel_Sizes(mTypeface, 0, paint.size);
-	FT_UInt32 flags = FT_LOAD_DEFAULT;  // TODO: respect hinting settings
-	FT_Fixed advance;
+    FT_UInt32 flags = FT_LOAD_DEFAULT;  // TODO: respect hinting settings
+    FT_Fixed advance;
     FT_Get_Advance(mTypeface, glyph_id, flags, &advance);
     return advance * (1.0 / 65536);
 }
@@ -52,18 +52,28 @@ void MinikinFontFreeType::GetBounds(MinikinRect* /* bounds */, uint32_t /* glyph
     // TODO: NYI
 }
 
-bool MinikinFontFreeType::GetTable(uint32_t tag, uint8_t *buf, size_t *size) {
-	FT_ULong ftsize = *size;
-	FT_Error error = FT_Load_Sfnt_Table(mTypeface, tag, 0, buf, &ftsize);
-	if (error != 0) {
-		return false;
-	}
-	*size = ftsize;
-	return true;
+const void* MinikinFontFreeType::GetTable(uint32_t tag, size_t* size, MinikinDestroyFunc* destroy) {
+    FT_ULong ftsize = 0;
+    FT_Error error = FT_Load_Sfnt_Table(mTypeface, tag, 0, nullptr, &ftsize);
+    if (error != 0) {
+        return nullptr;
+    }
+    FT_Byte* buf = reinterpret_cast<FT_Byte*>(malloc(ftsize));
+    if (buf == nullptr) {
+        return nullptr;
+    }
+    error = FT_Load_Sfnt_Table(mTypeface, tag, 0, buf, &ftsize);
+    if (error != 0) {
+        free(buf);
+        return nullptr;
+    }
+    *destroy = free;
+    *size = ftsize;
+    return buf;
 }
 
 int32_t MinikinFontFreeType::GetUniqueId() const {
-	return mUniqueId;
+    return mUniqueId;
 }
 
 bool MinikinFontFreeType::Render(uint32_t glyph_id, const MinikinPaint& /* paint */,
diff --git a/libs/minikin/MinikinInternal.cpp b/libs/minikin/MinikinInternal.cpp
index 175ced8..e00f639 100644
--- a/libs/minikin/MinikinInternal.cpp
+++ b/libs/minikin/MinikinInternal.cpp
@@ -17,6 +17,7 @@
 // Definitions internal to Minikin
 
 #include "MinikinInternal.h"
+#include "HbFontCache.h"
 
 #include <cutils/log.h>
 
@@ -72,4 +73,13 @@ bool isEmojiBase(uint32_t c) {
     }
 }
 
+hb_blob_t* getFontTable(MinikinFont* minikinFont, uint32_t tag) {
+    assertMinikinLocked();
+    hb_font_t* font = getHbFontLocked(minikinFont);
+    hb_face_t* face = hb_font_get_face(font);
+    hb_blob_t* blob = hb_face_reference_table(face, tag);
+    hb_font_destroy(font);
+    return blob;
+}
+
 }
diff --git a/libs/minikin/MinikinInternal.h b/libs/minikin/MinikinInternal.h
index 3d68691..709fb9f 100644
--- a/libs/minikin/MinikinInternal.h
+++ b/libs/minikin/MinikinInternal.h
@@ -19,8 +19,12 @@
 #ifndef MINIKIN_INTERNAL_H
 #define MINIKIN_INTERNAL_H
 
+#include <hb.h>
+
 #include <utils/Mutex.h>
 
+#include <minikin/MinikinFont.h>
+
 namespace android {
 
 // All external Minikin interfaces are designed to be thread-safe.
@@ -38,6 +42,35 @@ bool isEmojiBase(uint32_t c);
 // Returns true if c is emoji modifier.
 bool isEmojiModifier(uint32_t c);
 
+hb_blob_t* getFontTable(MinikinFont* minikinFont, uint32_t tag);
+
+// An RAII wrapper for hb_blob_t
+class HbBlob {
+public:
+    // Takes ownership of hb_blob_t object, caller is no longer
+    // responsible for calling hb_blob_destroy().
+    HbBlob(hb_blob_t* blob) : mBlob(blob) {
+    }
+
+    ~HbBlob() {
+        hb_blob_destroy(mBlob);
+    }
+
+    const uint8_t* get() const {
+        const char* data = hb_blob_get_data(mBlob, nullptr);
+        return reinterpret_cast<const uint8_t*>(data);
+    }
+
+    size_t size() const {
+        unsigned int length = 0;
+        hb_blob_get_data(mBlob, &length);
+        return (size_t)length;
+    }
+
+private:
+    hb_blob_t* mBlob;
+};
+
 }
 
 #endif  // MINIKIN_INTERNAL_H
diff --git a/tests/MinikinFontForTest.cpp b/tests/MinikinFontForTest.cpp
index ed70751..96f3326 100644
--- a/tests/MinikinFontForTest.cpp
+++ b/tests/MinikinFontForTest.cpp
@@ -40,16 +40,20 @@ void MinikinFontForTest::GetBounds(android::MinikinRect* /* bounds */, uint32_t
     LOG_ALWAYS_FATAL("MinikinFontForTest::GetBounds is not yet implemented");
 }
 
-bool MinikinFontForTest::GetTable(uint32_t tag, uint8_t *buf, size_t *size) {
-    if (buf == NULL) {
-        const size_t tableSize = mTypeface->getTableSize(tag);
-        *size = tableSize;
-        return tableSize != 0;
-    } else {
-        const size_t actualSize = mTypeface->getTableData(tag, 0, *size, buf);
-        *size = actualSize;
-        return actualSize != 0;
+const void* MinikinFontForTest::GetTable(uint32_t tag, size_t* size,
+        android::MinikinDestroyFunc* destroy) {
+    const size_t tableSize = mTypeface->getTableSize(tag);
+    *size = tableSize;
+    if (tableSize == 0) {
+        return nullptr;
     }
+    void* buf = malloc(tableSize);
+    if (buf == nullptr) {
+        return nullptr;
+    }
+    mTypeface->getTableData(tag, 0, tableSize, buf);
+    *destroy = free;
+    return buf;
 }
 
 int32_t MinikinFontForTest::GetUniqueId() const {
diff --git a/tests/MinikinFontForTest.h b/tests/MinikinFontForTest.h
index 790348d..4686f7a 100644
--- a/tests/MinikinFontForTest.h
+++ b/tests/MinikinFontForTest.h
@@ -30,7 +30,7 @@ public:
     float GetHorizontalAdvance(uint32_t glyph_id, const android::MinikinPaint &paint) const;
     void GetBounds(android::MinikinRect* bounds, uint32_t glyph_id,
             const android::MinikinPaint& paint) const;
-    bool GetTable(uint32_t tag, uint8_t *buf, size_t *size);
+    const void* GetTable(uint32_t tag, size_t* size, android::MinikinDestroyFunc* destroy);
     int32_t GetUniqueId() const;
 
     const std::string& fontPath() const { return mFontPath; }