SkPDF: Subset Type3 (fallback) font

src/pdf/SkPDFFont.cpp

Index: src/pdf/SkPDFFont.cpp
diff --git a/src/pdf/SkPDFFont.cpp b/src/pdf/SkPDFFont.cpp
index ff1dd2d76d594de616b15d074ac19ebe083089fb..b401aed45b10e1e13f61590fe85adf03b7555981 100644
--- a/src/pdf/SkPDFFont.cpp
+++ b/src/pdf/SkPDFFont.cpp
@@ -60,7 +60,7 @@ public:
                    SkTypeface* typeface);
     virtual ~SkPDFType0Font();
     bool multiByteGlyphs() const override { return true; }
-    SkPDFFont* getFontSubset(const SkPDFGlyphSet* usage) override;
+    sk_sp<SkPDFObject> getFontSubset(const SkPDFGlyphSet* usage) override;
 #ifdef SK_DEBUG
     void emitObject(SkWStream*,
                     const SkPDFObjNumMap&,
@@ -108,11 +108,14 @@ public:
     SkPDFType3Font(const SkAdvancedTypefaceMetrics* info,
                    SkTypeface* typeface,
                    uint16_t glyphID);
-    virtual ~SkPDFType3Font();
+    virtual ~SkPDFType3Font() {}
+    void emitObject(SkWStream*,
+                    const SkPDFObjNumMap&,
+                    const SkPDFSubstituteMap&) const override {
+        SkDEBUGFAIL("should call getFontSubset!");
+    }
+    sk_sp<SkPDFObject> getFontSubset(const SkPDFGlyphSet* usage) override;
     bool multiByteGlyphs() const override { return false; }
-
-private:
-    bool populate(uint16_t glyphID);
 };
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -335,7 +338,7 @@ SkScalar scaleFromFontUnits(int16_t val, uint16_t emSize) {
 
 
 void setGlyphWidthAndBoundingBox(SkScalar width, SkIRect box,
-                                 SkWStream* content) {
+                                 SkDynamicMemoryWStream* content) {
     // Specify width and bounding box for the glyph.
     SkPDFUtils::AppendScalar(width, content);
     content->writeText(" 0 ");
@@ -559,7 +562,7 @@ SkPDFFont* SkPDFFont::GetFontResource(SkPDFCanon* canon,
     return font;
 }
 
-SkPDFFont* SkPDFFont::getFontSubset(const SkPDFGlyphSet*) {
+sk_sp<SkPDFObject> SkPDFFont::getFontSubset(const SkPDFGlyphSet*) {
     return nullptr;  // Default: no support.
 }
 
@@ -571,12 +574,13 @@ SkPDFFont::SkPDFFont(const SkAdvancedTypefaceMetrics* info,
     , fFirstGlyphID(1)
     , fLastGlyphID(info ? info->fLastGlyphID : 0)
     , fFontInfo(SkSafeRef(info))
-    , fDescriptor(SkSafeRef(relatedFontDescriptor)) {
-    if (info == nullptr ||
-            info->fFlags & SkAdvancedTypefaceMetrics::kMultiMaster_FontFlag) {
-        fFontType = SkAdvancedTypefaceMetrics::kOther_Font;
-    } else {
-        fFontType = info->fType;
+    , fDescriptor(SkSafeRef(relatedFontDescriptor))
+    , fFontType((!info || info->fFlags & SkAdvancedTypefaceMetrics::kMultiMaster_FontFlag)
+                ? SkAdvancedTypefaceMetrics::kOther_Font
+                : info->fType) {
+    if (0 == fLastGlyphID) {
+        SkAutoResolveDefaultTypeface face(typeface);
+        fLastGlyphID = SkToU16(face->countGlyphs() - 1);
     }
 }
 
@@ -588,23 +592,29 @@ SkPDFFont* SkPDFFont::Create(SkPDFCanon* canon,
                              SkPDFDict* relatedFontDescriptor) {
     SkAdvancedTypefaceMetrics::FontType type =
         info ? info->fType : SkAdvancedTypefaceMetrics::kOther_Font;
-
-    if (info && (info->fFlags & SkAdvancedTypefaceMetrics::kMultiMaster_FontFlag)) {
+    SkAdvancedTypefaceMetrics::FontFlags flags =
+        info ? info->fFlags : SkAdvancedTypefaceMetrics::kEmpty_FontFlag;
+    if (SkToBool(flags & SkAdvancedTypefaceMetrics::kMultiMaster_FontFlag) ||
+        SkToBool(flags & SkAdvancedTypefaceMetrics::kNotEmbeddable_FontFlag)) {
         return new SkPDFType3Font(info, typeface, glyphID);
     }
-    if (type == SkAdvancedTypefaceMetrics::kType1CID_Font ||
-        type == SkAdvancedTypefaceMetrics::kTrueType_Font) {
-        SkASSERT(relatedFontDescriptor == nullptr);
-        return new SkPDFType0Font(info, typeface);
-    }
-    if (type == SkAdvancedTypefaceMetrics::kType1_Font) {
-        return new SkPDFType1Font(info, typeface, glyphID, relatedFontDescriptor);
+    switch (type) {
+        case SkAdvancedTypefaceMetrics::kType1CID_Font:
+        case SkAdvancedTypefaceMetrics::kTrueType_Font:
+            SkASSERT(relatedFontDescriptor == nullptr);
+            SkASSERT(info != nullptr);
+            return new SkPDFType0Font(info, typeface);
+        case SkAdvancedTypefaceMetrics::kType1_Font:
+            SkASSERT(info != nullptr);
+            return new SkPDFType1Font(info, typeface, glyphID, relatedFontDescriptor);
+        case SkAdvancedTypefaceMetrics::kCFF_Font:
+            SkASSERT(info != nullptr);
+            // fallthrough
+        case SkAdvancedTypefaceMetrics::kOther_Font:
+            return new SkPDFType3Font(info, typeface, glyphID);
     }
-
-    SkASSERT(type == SkAdvancedTypefaceMetrics::kCFF_Font ||
-             type == SkAdvancedTypefaceMetrics::kOther_Font);
-
-    return new SkPDFType3Font(info, typeface, glyphID);
+    SkDEBUGFAIL("invalid SkAdvancedTypefaceMetrics::FontType");
+    return nullptr;
 }
 
 const SkAdvancedTypefaceMetrics* SkPDFFont::fontInfo() {
@@ -703,11 +713,11 @@ SkPDFType0Font::SkPDFType0Font(const SkAdvancedTypefaceMetrics* info, SkTypeface
 
 SkPDFType0Font::~SkPDFType0Font() {}
 
-SkPDFFont* SkPDFType0Font::getFontSubset(const SkPDFGlyphSet* subset) {
+sk_sp<SkPDFObject>  SkPDFType0Font::getFontSubset(const SkPDFGlyphSet* subset) {
     if (!canSubset()) {
         return nullptr;
     }
-    SkPDFType0Font* newSubset = new SkPDFType0Font(fontInfo(), typeface());
+    auto newSubset = sk_make_sp<SkPDFType0Font>(fontInfo(), typeface());
     newSubset->populate(subset);
     return newSubset;
 }
@@ -1054,84 +1064,153 @@ bool SkPDFType1Font::populate(int16_t glyphID) {
 // class SkPDFType3Font
 ///////////////////////////////////////////////////////////////////////////////
 
-SkPDFType3Font::SkPDFType3Font(const SkAdvancedTypefaceMetrics* info,
-                               SkTypeface* typeface,
-                               uint16_t glyphID)
-    : SkPDFFont(info, typeface, nullptr) {
-    this->populate(glyphID);
+namespace {
+// returns [0, first, first+1, ... last-1,  last]
+struct SingleByteGlyphIdIterator {
+    SingleByteGlyphIdIterator(SkGlyphID first, SkGlyphID last)
+        : fFirst(first), fLast(last) {
+        SkASSERT(fFirst > 0);
+        SkASSERT(fLast >= first);
+    }
+    struct Iter {
+        void operator++() {
+            fCurrent = (0 == fCurrent) ? fFirst : fCurrent + 1;
+        }
+        SkGlyphID operator*() const { return SkToU16(fCurrent); }

[bungeman-skia, 2016/08/10 15:32:56]

Doesn't this operator normally return a reference? (probably a const reference)

[hal.canary, 2016/08/10 18:59:52]

It doesn't have to, but okay.

Done.

+        bool operator!=(const Iter& rhs) const {
+            return fCurrent != rhs.fCurrent;
+        }
+        const SkGlyphID& fFirst;

[bungeman-skia, 2016/08/10 15:32:56]

It seems odd that this is a reference. If the compiler optimizes it out it makes
no difference, but if not the reference will be larger than the integer. It
seems strange because it makes it look like this fFirst may change based on the
'outer' fFirst changing, but the 'outer' fFirst doesn't change.

[hal.canary, 2016/08/10 18:59:53]

Done.

+        int fCurrent;
+    };
+    Iter begin() const { return Iter{fFirst, 0}; }
+    Iter end() const { return{fFirst, (int)(fLast)+1}; }
+    SkGlyphID fFirst;
+    SkGlyphID fLast;

[bungeman-skia, 2016/08/10 15:32:56]

This is all in a limited scope, but it seems better if these fields are private.
If they're made private I don't have to think about them again (same goes for
fCurrent above). Also, they don't appear to ever change, so it would be good to
mark them const.

[hal.canary, 2016/08/10 18:59:53]

Done.

+};
 }
 
-SkPDFType3Font::~SkPDFType3Font() {}
-
-bool SkPDFType3Font::populate(uint16_t glyphID) {
+static void add_type3_font_info(SkPDFDict* font,
+                                SkTypeface* typeface,
+                                SkScalar emSize,
+                                const SkPDFGlyphSet* subset,
+                                SkGlyphID firstGlyphID,
+                                SkGlyphID lastGlyphID) {
+    SkASSERT(lastGlyphID >= firstGlyphID);
     SkPaint paint;
-    paint.setTypeface(sk_ref_sp(this->typeface()));
-    paint.setTextSize(1000);
+    paint.setTypeface(sk_ref_sp(typeface));
+    paint.setTextSize(emSize);

[bungeman-skia, 2016/08/10 15:32:56]

Set hinting to none?

[hal.canary, 2016/08/10 18:59:52]

Done.

     const SkSurfaceProps props(0, kUnknown_SkPixelGeometry);
-    SkAutoGlyphCache autoCache(paint, &props, nullptr);
-    SkGlyphCache* cache = autoCache.getCache();
-    // If fLastGlyphID isn't set (because there is not fFontInfo), look it up.
-    if (lastGlyphID() == 0) {
-        setLastGlyphID(cache->getGlyphCount() - 1);
-    }
+    SkAutoGlyphCache cache(paint, &props, nullptr);
 
-    adjustGlyphRangeForSingleByteEncoding(glyphID);
-
-    insertName("Subtype", "Type3");
+    font->insertName("Subtype", "Type3");
     // Flip about the x-axis and scale by 1/1000.

[bungeman-skia, 2016/08/10 15:32:56]

comment: scale by em

[hal.canary, 2016/08/10 18:59:52]

Done.

     SkMatrix fontMatrix;
-    fontMatrix.setScale(SkScalarInvert(1000), -SkScalarInvert(1000));
-    this->insertObject("FontMatrix", SkPDFUtils::MatrixToArray(fontMatrix));
+    fontMatrix.setScale(SkScalarInvert(emSize), -SkScalarInvert(emSize));
+    font->insertObject("FontMatrix", SkPDFUtils::MatrixToArray(fontMatrix));
 
     auto charProcs = sk_make_sp<SkPDFDict>();
     auto encoding = sk_make_sp<SkPDFDict>("Encoding");
 
     auto encDiffs = sk_make_sp<SkPDFArray>();
-    encDiffs->reserve(lastGlyphID() - firstGlyphID() + 2);
-    encDiffs->appendInt(1);
+    encDiffs->reserve(lastGlyphID - firstGlyphID + 3);
+    encDiffs->appendInt(0);
 
     auto widthArray = sk_make_sp<SkPDFArray>();
+    widthArray->reserve(lastGlyphID - firstGlyphID + 2);

[bungeman-skia, 2016/08/10 15:32:56]

a short comment about these magic '2' and '3' would be nice here.

[hal.canary, 2016/08/10 18:59:52]

Done.

 
     SkIRect bbox = SkIRect::MakeEmpty();
-    for (int gID = firstGlyphID(); gID <= lastGlyphID(); gID++) {
+
+    sk_sp<SkPDFStream> emptyStream;
+    for (SkGlyphID gID : SingleByteGlyphIdIterator(firstGlyphID, lastGlyphID)) {
+        bool skipGlyph = subset && gID != 0 && !subset->has(gID);
         SkString characterName;
-        characterName.printf("gid%d", gID);
+        SkScalar advance = 0.0f;
+        SkIRect glyphBBox;
+        if (skipGlyph) {
+            characterName.set("g0");
+        } else {
+            characterName.printf("g%X", gID);
+            const SkGlyph& glyph = cache->getGlyphIDMetrics(gID);
+            advance = SkFloatToScalar(glyph.fAdvanceX);
+            glyphBBox = SkIRect::MakeXYWH(glyph.fLeft, glyph.fTop,
+                                          glyph.fWidth, glyph.fHeight);
+            bbox.join(glyphBBox);
+            const SkPath* path = cache->findPath(glyph);
+            if (path && !path->isEmpty()) {
+                SkDynamicMemoryWStream content;
+                setGlyphWidthAndBoundingBox(SkFloatToScalar(glyph.fAdvanceX), glyphBBox,
+                                            &content);
+                SkPDFUtils::EmitPath(*path, SkPaint::kFill_Style, &content);
+                content.writeText("f"); // ::kFill_Style, ::kWinding_FillType
+                charProcs->insertObjRef(
+                    characterName, sk_make_sp<SkPDFStream>(
+                            std::unique_ptr<SkStreamAsset>(content.detachAsStream())));
+            } else {
+                if (!emptyStream) {
+                    emptyStream = sk_make_sp<SkPDFStream>(
+                            std::unique_ptr<SkStreamAsset>(
+                                    new SkMemoryStream((size_t)0)));
+                }
+                charProcs->insertObjRef(characterName, emptyStream);
+            }
+        }
         encDiffs->appendName(characterName.c_str());
+        widthArray->appendScalar(advance);
+    }
 
-        const SkGlyph& glyph = cache->getGlyphIDMetrics(gID);
-        widthArray->appendScalar(SkFloatToScalar(glyph.fAdvanceX));
-        SkIRect glyphBBox = SkIRect::MakeXYWH(glyph.fLeft, glyph.fTop,
-                                              glyph.fWidth, glyph.fHeight);
-        bbox.join(glyphBBox);
-
-        SkDynamicMemoryWStream content;
-        setGlyphWidthAndBoundingBox(SkFloatToScalar(glyph.fAdvanceX), glyphBBox,
-                                    &content);
-        const SkPath* path = cache->findPath(glyph);
-        if (path) {
-            SkPDFUtils::EmitPath(*path, paint.getStyle(), &content);
-            SkPDFUtils::PaintPath(paint.getStyle(), path->getFillType(),
-                                  &content);
+    encoding->insertObject("Differences", std::move(encDiffs));
+    font->insertInt("FirstChar", 0);
+    font->insertInt("LastChar", lastGlyphID - firstGlyphID + 1);
+    font->insertObject("FontBBox", makeFontBBox(bbox, 1000));

[bungeman-skia, 2016/08/10 15:32:56]

Is this 1000 right? Should it be em?

[hal.canary, 2016/08/10 18:59:52]

actually no.  they want the glyph coordinate system.

fixed

+    font->insertName("CIDToGIDMap", "Identity");
+    sk_sp<const SkAdvancedTypefaceMetrics> metrics;
+    if (subset) {
+        SkTDArray<uint32_t> subsetList;
+        for (SkGlyphID gID : SingleByteGlyphIdIterator(firstGlyphID, lastGlyphID)) {
+            if (gID == 0 || subset->has(gID)) {  // Always include glyph 0.
+                subsetList.push(0);
+            }
         }
-        charProcs->insertObjRef(
-                characterName, sk_make_sp<SkPDFStream>(
-                        std::unique_ptr<SkStreamAsset>(content.detachAsStream())));
+        subset->exportTo(&subsetList);
+        metrics = SkPDFFont::GetFontMetricsWithToUnicode(typeface, subsetList.begin(),
+                                                         subsetList.count());
+    } else {
+        metrics = SkPDFFont::GetFontMetricsWithToUnicode(typeface, nullptr, 0);
     }
+    font->insertObjRef("ToUnicode",
+                       SkPDFMakeToUnicodeCmap(metrics->fGlyphToUnicode,
+                                              subset,
+                                              false,
+                                              firstGlyphID,
+                                              lastGlyphID));
+    font->insertObject("Widths", std::move(widthArray));
+    font->insertObject("Encoding", std::move(encoding));
+    font->insertObject("CharProcs", std::move(charProcs));
+}
 
-    encoding->insertObject("Differences", std::move(encDiffs));
+SkPDFType3Font::SkPDFType3Font(const SkAdvancedTypefaceMetrics* info,
+                               SkTypeface* typeface,
+                               uint16_t glyphID)
+    : SkPDFFont(info, typeface, nullptr) {
+    // If fLastGlyphID isn't set (because there is not fFontInfo), look it up.
+    this->setLastGlyphID(SkToU16(typeface->countGlyphs() - 1));
+    this->adjustGlyphRangeForSingleByteEncoding(glyphID);
+}
 
-    this->insertObject("CharProcs", std::move(charProcs));
-    this->insertObject("Encoding", std::move(encoding));
+sk_sp<SkPDFObject> SkPDFType3Font::getFontSubset(const SkPDFGlyphSet* usage) {
+    // All fonts are subset before serialization.
+    // TODO(halcanary): all fonts should follow this pattern.
+    auto font = sk_make_sp<SkPDFDict>("Font");
+    const SkAdvancedTypefaceMetrics* info = this->fontInfo();
+    uint16_t emSize = info && info->fEmSize > 0 ? info->fEmSize : 1000;

[bungeman-skia, 2016/08/10 15:32:56]

Do you sometimes get a bogus em size or no info? Seems like an odd thing to
guess here.

[hal.canary, 2016/08/10 18:59:52]

I do not assume SkAdvancedTypefaceMetrics* is non-null.  if it is null, I end up
here.  Also emSize defaults to zero.  I want to future-proof this code.

+    add_type3_font_info(font.get(), this->typeface(), (SkScalar)emSize, usage,
+                        this->firstGlyphID(), this->lastGlyphID());
+    return font;
+}
 
-    this->insertObject("FontBBox", makeFontBBox(bbox, 1000));
-    this->insertInt("FirstChar", 1);
-    this->insertInt("LastChar", lastGlyphID() - firstGlyphID() + 1);
-    this->insertObject("Widths", std::move(widthArray));
-    this->insertName("CIDToGIDMap", "Identity");
 
-    this->populateToUnicodeTable(nullptr);
-    return true;
-}
+////////////////////////////////////////////////////////////////////////////////
 
 SkPDFFont::Match SkPDFFont::IsMatch(SkPDFFont* existingFont,
                                     uint32_t existingFontID,