SkPDF: Glyph validation change

src/pdf/SkPDFDevice.cpp

Index: src/pdf/SkPDFDevice.cpp
diff --git a/src/pdf/SkPDFDevice.cpp b/src/pdf/SkPDFDevice.cpp
index a36125f38090f26fa574e36a2c2ac0bb31808cb9..455a73de188c75659b745babb9354c5428b2a0e9 100644
--- a/src/pdf/SkPDFDevice.cpp
+++ b/src/pdf/SkPDFDevice.cpp
@@ -32,7 +32,6 @@
 #include "SkSurface.h"
 #include "SkTextFormatParams.h"
 #include "SkTemplates.h"
-#include "SkTypefacePriv.h"
 #include "SkXfermodeInterpretation.h"
 
 #define DPI_FOR_RASTER_SCALE_ONE 72
@@ -973,7 +972,7 @@ public:
         , fDefaultPositioning(defaultPositioning) {
         set_text_transform(origin.x(), origin.y(), textSkewX, fContent);
     }
-    ~GlyphPositioner() { SkASSERT(!fInText);  /* flush first */ }
+    ~GlyphPositioner() {this->flush();  }

[bungeman-skia, 2016/08/25 19:28:43]

nit: probably want a space between '{' and 'this'.

[hal.canary, 2016/08/26 15:27:03]

Done.  And removed the extra space before '}'

     void flush() {
         if (fInText) {
             fContent->writeText("> Tj\n");
@@ -987,21 +986,20 @@ public:
             fWideChars = wideChars;
         }
     }
-    void writeGlyph(SkScalar x,
-                    SkScalar y,
+    void writeGlyph(SkPoint xy,
                     SkScalar advanceWidth,
                     uint16_t glyph) {
         if (!fDefaultPositioning) {
-            SkScalar xPosition = x - fCurrentMatrixX;
-            SkScalar yPosition = y - fCurrentMatrixY;
+            SkScalar xPosition = xy.x() - fCurrentMatrixX;
+            SkScalar yPosition = xy.y() - fCurrentMatrixY;

[bungeman-skia, 2016/08/25 19:28:43]

This handling seems almost painful to read. Is there any reason why
fCurrentMatrixX/Y and x/yPosition aren't also SkPoint? SkPoint::operator- exists
as expected to do exactly this sort of operation.

[hal.canary, 2016/08/26 15:27:03]

Excellent point.

Done.

I also used SkPoint::operator!=()

             if (xPosition != fXAdvance || yPosition != 0) {
                 this->flush();
                 SkPDFUtils::AppendScalar(xPosition, fContent);
                 fContent->writeText(" ");
                 SkPDFUtils::AppendScalar(-yPosition, fContent);
                 fContent->writeText(" Td ");
-                fCurrentMatrixX = x;
-                fCurrentMatrixY = y;
+                fCurrentMatrixX = xy.x();
+                fCurrentMatrixY = xy.y();
                 fXAdvance = 0;
             }
             fXAdvance += advanceWidth;
@@ -1034,12 +1032,14 @@ static void draw_transparent_text(SkPDFDevice* device,
                                   const void* text, size_t len,
                                   SkScalar x, SkScalar y,
                                   const SkPaint& srcPaint) {
-    SkPaint transparent;
-    if (!SkPDFFont::CanEmbedTypeface(transparent.getTypeface(),
+    sk_sp<SkTypeface> defaultFace = SkTypeface::MakeDefault();
+    if (!SkPDFFont::CanEmbedTypeface(defaultFace.get(),
                                      device->getCanon())) {

[bungeman-skia, 2016/08/25 19:28:43]

nit: Since you're touching this line anyway, will it all fit on one line now?

[hal.canary, 2016/08/26 15:27:03]

Done.

         SkDebugf("SkPDF: default typeface should be embeddable");
         return;  // Avoid infinite loop in release.
     }
+    SkPaint transparent;
+    transparent.setTypeface(std::move(defaultFace));
     transparent.setTextSize(srcPaint.getTextSize());
     transparent.setColor(SK_ColorTRANSPARENT);
     switch (srcPaint.getTextEncoding()) {
@@ -1069,6 +1069,21 @@ static void draw_transparent_text(SkPDFDevice* device,
     }
 }
 
+namespace {
+struct TextObject {
+    SkDynamicMemoryWStream* fOut;
+    TextObject(SkDynamicMemoryWStream* out) : fOut(out) { fOut->writeText("BT\n"); }
+    ~TextObject() { fOut->writeText("ET\n"); }
+};
+} // namespace

[bungeman-skia, 2016/08/25 19:28:43]

It seems that this struct is only used by internalDrawText. Can we just
declare/define it inside that method?

[hal.canary, 2016/08/26 15:27:03]

Done.

+
+static int get_stretch(SkPDFFont* font, const SkGlyphID* glyphs, int index,
+                       int glyphCount, SkGlyphID maxGlyphID, bool multiByte) {
+        SkASSERT(font);
+        return multiByte ? glyphCount - index
+                         : font->countStretch(&glyphs[index], glyphCount - index, maxGlyphID);
+}
+
 void SkPDFDevice::internalDrawText(
         const SkDraw& d, const void* sourceText, size_t sourceByteCount,
         const SkScalar pos[], SkTextBlob::GlyphPositioning positioning,
@@ -1097,11 +1112,11 @@ void SkPDFDevice::internalDrawText(
         SkDebugf("SkPDF: SkTypeface::MakeDefault() returned nullptr.\n");
         return;
     }
-    int typefaceGlyphCount = typeface->countGlyphs();
-    if (typefaceGlyphCount < 1) {
-        SkDebugf("SkPDF: SkTypeface has no glyphs.\n");
+    SkPDFFont::TypefaceInfo faceInfo = SkPDFFont::GetTypefaceInfo(typeface);
+    if (!faceInfo.fGood) {
         return;
     }
+    const SkGlyphID maxGlyphID = faceInfo.fMaxGlyphID;
     if (!SkPDFFont::CanEmbedTypeface(typeface, fDocument->canon())) {
         SkPath path; // https://bug.skia.org/3866
         paint.getTextPath(sourceText, sourceByteCount,
@@ -1112,21 +1127,19 @@ void SkPDFDevice::internalDrawText(
                               offset.x(), offset.y(), paint);
         return;
     }
-    // Always make a copy (1) to validate user-input glyphs and
-    // (2) because we may modify the glyphs in place (for
-    // single-byte-glyph-id PDF fonts).
     int glyphCount = paint.textToGlyphs(sourceText, sourceByteCount, nullptr);
-    if (glyphCount <= 0) { return; }
-    SkAutoSTMalloc<128, SkGlyphID> glyphStorage(SkToSizeT(glyphCount));
-    SkGlyphID* glyphs = glyphStorage.get();
-    (void)paint.textToGlyphs(sourceText, sourceByteCount, glyphs);
+    if (glyphCount <= 0) {
+        return;
+    }
+    SkAutoSTMalloc<128, SkGlyphID> glyphStorage;
+    const SkGlyphID* glyphs = nullptr;
     if (paint.getTextEncoding() == SkPaint::kGlyphID_TextEncoding) {
-        // Validate user-input glyphs.
-        SkGlyphID maxGlyphID = SkToU16(typefaceGlyphCount - 1);
-        for (int i = 0; i < glyphCount; ++i) {
-            glyphs[i] = SkTMin(maxGlyphID, glyphs[i]);
-        }
+        glyphs = (const SkGlyphID*)sourceText;
+        // validate input later.
     } else {
+        glyphStorage.reset(SkToSizeT(glyphCount));
+        (void)paint.textToGlyphs(sourceText, sourceByteCount, glyphStorage.get());
+        glyphs = glyphStorage.get();
         paint.setTextEncoding(SkPaint::kGlyphID_TextEncoding);
     }
 
@@ -1135,89 +1148,90 @@ void SkPDFDevice::internalDrawText(
     SkAutoGlyphCache glyphCache(paint, nullptr, nullptr);
 
     SkPaint::Align alignment = paint.getTextAlign();
+    float alignmentFactor = SkPaint::kLeft_Align   == alignment ?  0.0f :
+                            SkPaint::kCenter_Align == alignment ? -0.5f :
+                            /* SkPaint::kRight_Align */           -1.0f;
     if (defaultPositioning && alignment != SkPaint::kLeft_Align) {
-        SkScalar advance{0};
+        SkScalar advance = 0;
         for (int i = 0; i < glyphCount; ++i) {
             advance += glyphCache->getGlyphIDAdvance(glyphs[i]).fAdvanceX;
         }
-        SkScalar m = alignment == SkPaint::kCenter_Align
-            ? 0.5f * advance : advance;
-        offset -= SkPoint{m, 0};
+        offset.offset(alignmentFactor * advance, 0);
     }
     ScopedContentEntry content(this, d, paint, true);
     if (!content.entry()) {
         return;
     }
     SkDynamicMemoryWStream* out = &content.entry()->fContent;
-    out->writeText("BT\n");
-    if (!this->updateFont(paint, glyphs[0], content.entry())) {
-        SkDebugf("SkPDF: Font error.");
-        out->writeText("ET\n%SkPDF: Font error.\n");
-        return;
+    SkScalar textSize = paint.getTextSize();
+    TextObject textObject(out);  // Must be in scope for this->updateFont().

[bungeman-skia, 2016/08/25 19:28:43]

I'm not sure this name clearly conveys the side effects. Something like
'ScopedTextObject' maybe?

This would actually be a good place to use mtklein's proposed sk_at_scope_exit,
since this isn't actually handling any resources at all.

out->writeText("BT\n");
sk_at_scope_exit(out->writeText("ET\n"));

[hal.canary, 2016/08/26 15:27:03]

Done.

+    int index = 0;
+    while (glyphs[index] > maxGlyphID) {  // Invalid glyphID for this font.
+        ++index;  // Skip this glyphID
+        if (index == glyphCount) {
+            return;  // all glyphIDs were bad.
+        }
     }
-    SkPDFFont* font = content.entry()->fState.fFont;
+    SkPDFFont* font = this->updateFont(
+            typeface, textSize, glyphs[index], content.entry());
+    SkASSERT(font);  // All preconditions for SkPDFFont::GetFontResource are met.
+    if (!font) { return; }
+
     GlyphPositioner glyphPositioner(out,
                                     paint.getTextSkewX(),
                                     font->multiByteGlyphs(),
                                     defaultPositioning,
                                     offset);
-    const SkGlyphID* const glyphsEnd = glyphs + glyphCount;
-
-    while (glyphs < glyphsEnd) {
-        font = content.entry()->fState.fFont;
-        int stretch = font->multiByteGlyphs()
-            ? SkToInt(glyphsEnd - glyphs)
-            : font->glyphsToPDFFontEncodingCount(glyphs, SkToInt(glyphsEnd - glyphs));
-        SkASSERT(glyphs + stretch <= glyphsEnd);
+
+    while (index < glyphCount) {
+        bool multiByte = font->multiByteGlyphs();
+        int stretch = get_stretch(font, glyphs, index, glyphCount, maxGlyphID, multiByte);
+        SkASSERT(index + stretch <= glyphCount);
         if (stretch < 1) {
-            SkASSERT(!font->multiByteGlyphs());
+            SkASSERT(!multiByte);
             // The current pdf font cannot encode the next glyph.
             // Try to get a pdf font which can encode the next glyph.
             glyphPositioner.flush();
-            if (!this->updateFont(paint, *glyphs, content.entry())) {
-                SkDebugf("SkPDF: Font error.");
-                out->writeText("ET\n%SkPDF: Font error.\n");
-                return;
+            // first, validate the next glyph
+            while (glyphs[index] > maxGlyphID) {
+                ++index;  // Skip this glyphID
+                if (index == glyphCount) {
+                    return;  // all remainng glyphIDs were bad.
+                }
             }
-            font = content.entry()->fState.fFont;
+            SkASSERT(index < glyphCount);
+            font = this->updateFont(typeface, textSize, glyphs[index], content.entry());
+            SkASSERT(font);  // preconditions for SkPDFFont::GetFontResource met.
+            if (!font) { return; }
             glyphPositioner.setWideChars(font->multiByteGlyphs());
-            // try again
-            stretch = font->glyphsToPDFFontEncodingCount(glyphs,
-                                                         SkToInt(glyphsEnd - glyphs));
+            // Get stretch for this new font.
+            stretch = get_stretch(font, glyphs, index, glyphCount, maxGlyphID, false);
             if (stretch < 1) {
                 SkDEBUGFAIL("PDF could not encode glyph.");
-                glyphPositioner.flush();
-                out->writeText("ET\n%SkPDF: Font encoding error.\n");
                 return;
             }
         }
-        font->noteGlyphUsage(glyphs, stretch);
-        if (defaultPositioning) {
-            (void)font->glyphsToPDFFontEncoding(glyphs, SkToInt(glyphsEnd - glyphs));
-            while (stretch-- > 0) {
-                glyphPositioner.writeGlyph(0, 0, 0, *glyphs);
-                ++glyphs;
-            }
-        } else {
-            while (stretch-- > 0) {
-                SkScalar advance = glyphCache->getGlyphIDAdvance(*glyphs).fAdvanceX;
-                SkScalar x = *pos++;
-                // evaluate x and y in order!
-                SkScalar y = SkTextBlob::kFull_Positioning == positioning ? *pos++ : 0;
-                SkPoint xy{x, y};
-                if (alignment != SkPaint::kLeft_Align) {
-                    SkScalar m = alignment == SkPaint::kCenter_Align
-                        ? 0.5f * advance : advance;
-                    xy -= SkPoint{m, 0};
+        while (stretch-- > 0) {
+            SkGlyphID gid = glyphs[index];
+            if (gid <= maxGlyphID) {
+                font->noteGlyphUsage(gid);
+                SkGlyphID encodedGlyph = font->glyphToPDFFontEncoding(gid);
+                if (defaultPositioning) {
+                    glyphPositioner.writeGlyph(SkPoint{0, 0}, 0, encodedGlyph);
+                } else {
+                    SkScalar advance = glyphCache->getGlyphIDAdvance(gid).fAdvanceX;
+                    SkPoint xy = SkTextBlob::kFull_Positioning == positioning
+                                 ? SkPoint{pos[2 * index], pos[2 * index + 1]}
+                                 : SkPoint{pos[index], 0};
+                    if (alignment != SkPaint::kLeft_Align) {
+                        xy.offset(alignmentFactor * advance, 0);
+                    }
+                    glyphPositioner.writeGlyph(xy, advance, encodedGlyph);
                 }
-                (void)font->glyphsToPDFFontEncoding(glyphs, 1);
-                glyphPositioner.writeGlyph(xy.x(), xy.y(), advance, *glyphs);
-                ++glyphs;
             }
+            ++index;
         }
     }
-    glyphPositioner.flush();
-    out->writeText("ET\n");
 }
 
 void SkPDFDevice::drawText(const SkDraw& d, const void* text, size_t len,
@@ -1903,26 +1917,28 @@ int SkPDFDevice::addXObjectResource(SkPDFObject* xObject) {
     return result;
 }
 
-bool SkPDFDevice::updateFont(const SkPaint& paint, uint16_t glyphID,
-                             SkPDFDevice::ContentEntry* contentEntry) {
-    SkTypeface* typeface = paint.getTypeface();
+SkPDFFont* SkPDFDevice::updateFont(SkTypeface* typeface,
+                                   SkScalar textSize,
+                                   uint16_t glyphID,
+                                   SkPDFDevice::ContentEntry* contentEntry) {
     if (contentEntry->fState.fFont == nullptr ||
-            contentEntry->fState.fTextSize != paint.getTextSize() ||
+            contentEntry->fState.fTextSize != textSize ||
             !contentEntry->fState.fFont->hasGlyph(glyphID)) {
         int fontIndex = getFontResourceIndex(typeface, glyphID);
         if (fontIndex < 0) {
-            return false;
+            SkDebugf("SkPDF: Font error.");
+            return nullptr;
         }
         contentEntry->fContent.writeText("/");
         contentEntry->fContent.writeText(SkPDFResourceDict::getResourceName(
                 SkPDFResourceDict::kFont_ResourceType,
                 fontIndex).c_str());
         contentEntry->fContent.writeText(" ");
-        SkPDFUtils::AppendScalar(paint.getTextSize(), &contentEntry->fContent);
+        SkPDFUtils::AppendScalar(textSize, &contentEntry->fContent);
         contentEntry->fContent.writeText(" Tf\n");
         contentEntry->fState.fFont = fFontResources[fontIndex];
     }
-    return true;
+    return contentEntry->fState.fFont;
 }
 
 int SkPDFDevice::getFontResourceIndex(SkTypeface* typeface, uint16_t glyphID) {