SkPDF: refactor font subset: fewer copies

src/pdf/SkPDFFont.cpp

Index: src/pdf/SkPDFFont.cpp
diff --git a/src/pdf/SkPDFFont.cpp b/src/pdf/SkPDFFont.cpp
index 2f7137fd99cefae9e51c36e1b4f66a3b71aa7c39..fe3ae4698bcd798e9c366edc767bb94658b108bc 100644
--- a/src/pdf/SkPDFFont.cpp
+++ b/src/pdf/SkPDFFont.cpp
@@ -584,64 +584,23 @@ static sk_sp<SkPDFStream> generate_tounicode_cmap(
     append_cmap_sections(glyphToUnicode, subset, &cmap, multiByteGlyphs,
                          firstGlyphID, lastGlyphID);
     append_cmap_footer(&cmap);
-    sk_sp<SkData> cmapData(cmap.copyToData());
-    return sk_make_sp<SkPDFStream>(cmapData.get());
+    return sk_make_sp<SkPDFStream>(
+            std::unique_ptr<SkStreamAsset>(cmap.detachAsStream()));
 }
 
-#if defined (SK_SFNTLY_SUBSETTER)
-static void sk_delete_array(const void* ptr, void*) {
-    // Use C-style cast to cast away const and cast type simultaneously.
-    delete[] (unsigned char*)ptr;
-}
-#endif
-
-#if defined(SK_SFNTLY_SUBSETTER)
-static size_t get_subset_font_stream(const char* fontName,
-                                     const SkTypeface* typeface,
-                                     const SkTDArray<uint32_t>& subset,
-                                     SkPDFStream** fontStream) {
-    int ttcIndex;
-    std::unique_ptr<SkStreamAsset> fontData(typeface->openStream(&ttcIndex));
-    SkASSERT(fontData);
-    if (!fontData) {
-        return 0;
-    }
-
-    size_t fontSize = fontData->getLength();
-
-    // Read font into buffer.
-    SkPDFStream* subsetFontStream = nullptr;
-    SkTDArray<unsigned char> originalFont;
-    originalFont.setCount(SkToInt(fontSize));
-    if (fontData->read(originalFont.begin(), fontSize) == fontSize) {
-        unsigned char* subsetFont = nullptr;
-        // sfntly requires unsigned int* to be passed in, as far as we know,
-        // unsigned int is equivalent to uint32_t on all platforms.
-        static_assert(sizeof(unsigned int) == sizeof(uint32_t), "unsigned_int_not_32_bits");
-        int subsetFontSize = SfntlyWrapper::SubsetFont(fontName,
-                                                       originalFont.begin(),
-                                                       fontSize,
-                                                       subset.begin(),
-                                                       subset.count(),
-                                                       &subsetFont);
-        if (subsetFontSize > 0 && subsetFont != nullptr) {
-            SkAutoDataUnref data(SkData::NewWithProc(subsetFont,
-                                                     subsetFontSize,
-                                                     sk_delete_array,
-                                                     nullptr));
-            subsetFontStream = new SkPDFStream(data.get());
-            fontSize = subsetFontSize;
-        }
-    }
-    if (subsetFontStream) {
-        *fontStream = subsetFontStream;
-        return fontSize;
-    }
-    fontData->rewind();
-
-    // Fail over: just embed the whole font.
-    *fontStream = new SkPDFStream(std::move(fontData));
-    return fontSize;
+#ifdef SK_SFNTLY_SUBSETTER
+// if possible, make no copy.
+static sk_sp<SkData> stream_to_data(std::unique_ptr<SkStreamAsset> stream) {
+    SkASSERT(stream);
+    (void)stream->rewind();
+    SkASSERT(stream->hasLength());
+    size_t size = stream->getLength();
+    if (const void* base = stream->getMemoryBase()) {
+        SkData::ReleaseProc proc =
+            [](const void*, void* ctx) { delete (SkStream*)ctx; };
+        return SkData::MakeWithProc(base, size, proc, stream.release());
+    }
+    return SkData::MakeFromStream(stream.get(), size);
 }
 #endif
 
@@ -665,7 +624,7 @@ bool SkPDFGlyphSet::has(uint16_t glyphID) const {
 void SkPDFGlyphSet::exportTo(SkTDArray<unsigned int>* glyphIDs) const {
     fBitSet.exportTo(glyphIDs);
 }
- 
+
 ///////////////////////////////////////////////////////////////////////////////
 // class SkPDFGlyphSetMap
 ///////////////////////////////////////////////////////////////////////////////
@@ -1027,36 +986,54 @@ bool SkPDFCIDFont::addFontDescriptor(int16_t defaultWidth,
 
     switch (getType()) {
         case SkAdvancedTypefaceMetrics::kTrueType_Font: {
-            size_t fontSize = 0;
-#if defined(SK_SFNTLY_SUBSETTER)
-            if (this->canSubset()) {
-                sk_sp<SkPDFStream> fontStream;
-                SkPDFStream* rawStream = nullptr;
-                fontSize = get_subset_font_stream(fontInfo()->fFontName.c_str(),
-                                                  typeface(),
-                                                  *subset,
-                                                  &rawStream);
-                if (0 == fontSize) {
-                    return false;
-                }
-                if (rawStream) {
-                    fontStream.reset(rawStream);
-                    fontStream->insertInt("Length1", fontSize);
-                    descriptor->insertObjRef("FontFile2", std::move(fontStream));
-                    break;
-                }
-            }
-#endif
-            sk_sp<SkPDFSharedStream> fontStream;
-            std::unique_ptr<SkStreamAsset> fontData(
-                    this->typeface()->openStream(nullptr));
-            SkASSERT(fontData);
-            if (!fontData || 0 == fontData->getLength()) {
+            int ttcIndex;
+            std::unique_ptr<SkStreamAsset> fontAsset(
+                    this->typeface()->openStream(&ttcIndex));
+            SkASSERT(fontAsset);
+            if (!fontAsset) {
                 return false;
             }
-            fontSize = fontData->getLength();
+            size_t fontSize = fontAsset->getLength();
             SkASSERT(fontSize > 0);
-            fontStream.reset(new SkPDFSharedStream(fontData.release()));
+            if (fontSize == 0) {
+                return false;
+            }
+
+            #ifdef SK_SFNTLY_SUBSETTER
+            if (this->canSubset() && subset) {
+                sk_sp<SkData> fontData(stream_to_data(std::move(fontAsset)));
+                unsigned char* subsetFont = nullptr;
+                // sfntly requires unsigned int* to be passed in,
+                // as far as we know, unsigned int is equivalent
+                // to uint32_t on all platforms.
+                static_assert(sizeof(unsigned) == sizeof(uint32_t), "");
+                const char* fontName = fontInfo()->fFontName.c_str();
+                // TODO(halcanary): Use ttcIndex, not fontName.
+                int subsetFontSize =
+                    SfntlyWrapper::SubsetFont(fontName,
+                                              fontData->bytes(),
+                                              fontSize,
+                                              subset->begin(),
+                                              subset->count(),
+                                              &subsetFont);
+                SkASSERT(subsetFontSize > 0 || subsetFont == nullptr);
+                if (subsetFontSize > 0) {
+                    SkASSERT(subsetFont != nullptr);
+                    auto subset = sk_make_sp<SkPDFStream>(
+                            SkData::MakeWithProc(
+                                    subsetFont, subsetFontSize,
+                                    [](const void* p, void*) {
+                                        delete[] (unsigned char*)p;
+                                    }, nullptr));
+                    subset->insertInt("Length1", subsetFontSize);
+                    descriptor->insertObjRef("FontFile2", std::move(subset));
+                    break;
+                }
+                // If subsetting fails, fall back to original font data.
+                fontAsset.reset(this->typeface()->openStream(&ttcIndex));

[bungeman-skia, 2016/07/27 19:26:44]

Hopefully it doesn't matter too much, but note that this means 'fontData' and
'fontAsset' are both active here, meaning that there may be two copies / mmaps
of the same data in memory (and the the 'subsetFont' data as well). Indeed, the
lifetime of 'fontData' appears to just be around the call to SubsetFont, it
could go away even before making the SkPDFStream.

I know inlining this made some of this more straight forward, but the five
levels of indentation plus indentation for expression continuation makes this
much more difficult to take in.

[hal.canary, 2016/07/27 19:54:26]

Fixed.  now fontData is out of scope (in another fn) before this call.
nope.
done.
fixed.

+            }
+            #endif  // SK_SFNTLY_SUBSETTER
+            auto fontStream = sk_make_sp<SkPDFSharedStream>(fontAsset.release());
             fontStream->dict()->insertInt("Length1", fontSize);
             descriptor->insertObjRef("FontFile2", std::move(fontStream));
             break;
@@ -1207,7 +1184,7 @@ bool SkPDFType1Font::addFontDescriptor(int16_t defaultWidth) {
         return false;
     }
     SkASSERT(this->canEmbed());
-    auto fontStream = sk_make_sp<SkPDFStream>(fontData.get());
+    auto fontStream = sk_make_sp<SkPDFStream>(std::move(fontData));
     fontStream->insertInt("Length1", header);
     fontStream->insertInt("Length2", data);
     fontStream->insertInt("Length3", trailer);