SkPDF: Glyph validation change

src/pdf/SkPDFDevice.cpp

 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkPDFDevice.h"
 #include "SkAnnotationKeys.h"
 #include "SkBitmapDevice.h"
@@ skipping 14 matching lines @@
 #include "SkPDFResourceDict.h"
 #include "SkPDFShader.h"
 #include "SkPDFTypes.h"
 #include "SkPDFUtils.h"
 #include "SkRasterClip.h"
 #include "SkRRect.h"
 #include "SkString.h"
 #include "SkSurface.h"
 #include "SkTextFormatParams.h"
 #include "SkTemplates.h"
-#include "SkTypefacePriv.h"
 #include "SkXfermodeInterpretation.h"
 
 #define DPI_FOR_RASTER_SCALE_ONE 72
 
 // Utility functions
 
 // If the paint will definitely draw opaquely, replace kSrc_Mode with
 // kSrcOver_Mode.  http://crbug.com/473572
 static void replace_srcmode_on_opaque_paint(SkPaint* paint) {
     if (kSrcOver_SkXfermodeInterpretation
@@ skipping 920 matching lines @@
                     SkPoint origin)
         : fContent(content)
         , fCurrentMatrixX(0.0f)
         , fCurrentMatrixY(0.0f)
         , fXAdvance(0.0f)
         , fWideChars(wideChars)
         , fInText(false)
         , 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");
             fInText = false;
         }
     }
     void setWideChars(bool wideChars) {
         if (fWideChars != wideChars) {
             SkASSERT(!fInText);
             SkASSERT(fWideChars == wideChars);
             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;
         }
         if (!fInText) {
             fContent->writeText("<");
             fInText = true;
         }
         if (fWideChars) {
             SkPDFUtils::WriteUInt16BE(fContent, glyph);
@@ skipping 12 matching lines @@
     bool fInText;
     const bool fDefaultPositioning;
 };
 }  // namespace
 
 static void draw_transparent_text(SkPDFDevice* device,
                                   const SkDraw& d,
                                   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()) {
         case SkPaint::kGlyphID_TextEncoding: {
             // Since a glyphId<->Unicode mapping is typeface-specific,
             // map back to Unicode first.
             size_t glyphCount = len / 2;
             SkAutoTMalloc<SkUnichar> unichars(glyphCount);
             srcPaint.glyphsToUnichars(
                     (const uint16_t*)text, SkToInt(glyphCount), &unichars[0]);
             transparent.setTextEncoding(SkPaint::kUTF32_TextEncoding);
             // TODO(halcanary): deal with case where default typeface
             // does not have glyphs for these unicode code points.
             device->drawText(d, &unichars[0],
                              glyphCount * sizeof(SkUnichar),
                              x, y, transparent);
             break;
         }
         case SkPaint::kUTF8_TextEncoding:
         case SkPaint::kUTF16_TextEncoding:
         case SkPaint::kUTF32_TextEncoding:
             transparent.setTextEncoding(srcPaint.getTextEncoding());
             device->drawText(d, text, len, x, y, transparent);
             break;
         default:
             SkFAIL("unknown text encoding");
     }
 }
 
+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,
         SkPoint offset, const SkPaint& srcPaint) {
     NOT_IMPLEMENTED(srcPaint.getMaskFilter() != nullptr, false);
     if (srcPaint.getMaskFilter() != nullptr) {
         // Don't pretend we support drawing MaskFilters, it makes for artifacts
         // making text unreadable (e.g. same text twice when using CSS shadows).
         return;
     }
     NOT_IMPLEMENTED(srcPaint.isVerticalText(), false);
     if (srcPaint.isVerticalText()) {
         // Don't pretend we support drawing vertical text.  It is not
         // clear to me how to switch to "vertical writing" mode in PDF.
         // Currently neither Chromium or Android set this flag.
         // https://bug.skia.org/5665
         return;
     }
     SkPaint paint = calculate_text_paint(srcPaint);
     replace_srcmode_on_opaque_paint(&paint);
     if (!paint.getTypeface()) {
         paint.setTypeface(SkTypeface::MakeDefault());
     }
     SkTypeface* typeface = paint.getTypeface();
     if (!typeface) {
         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,
                           offset.x(), offset.y(), &path);
         this->drawPath(d, path, srcPaint, &SkMatrix::I(), true);
         // Draw text transparently to make it copyable/searchable/accessable.
         draw_transparent_text(this, d, sourceText, sourceByteCount,
                               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);
     }
 
     bool defaultPositioning = (positioning == SkTextBlob::kDefault_Positioning);
     paint.setHinting(SkPaint::kNo_Hinting);
     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;
+        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);
+                }
             }
-        } 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};
-                }
-                (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,
                            SkScalar x, SkScalar y, const SkPaint& paint) {
     this->internalDrawText(d, text, len, nullptr, SkTextBlob::kDefault_Positioning,
                            SkPoint{x, y}, paint);
 }
 
 void SkPDFDevice::drawPosText(const SkDraw& d, const void* text, size_t len,
                               const SkScalar pos[], int scalarsPerPos,
@@ skipping 665 matching lines @@
     // Assumes that xobject has been canonicalized (so we can directly compare
     // pointers).
     int result = fXObjectResources.find(xObject);
     if (result < 0) {
         result = fXObjectResources.count();
         fXObjectResources.push(SkRef(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) {
     sk_sp<SkPDFFont> newFont(
             SkPDFFont::GetFontResource(fDocument->canon(), typeface, glyphID));
     if (!newFont) {
         return -1;
     }
     int resourceIndex = fFontResources.find(newFont.get());
     if (resourceIndex < 0) {
@@ skipping 209 matching lines @@
 }
 
 sk_sp<SkSpecialImage> SkPDFDevice::makeSpecial(const SkImage* image) {
     return SkSpecialImage::MakeFromImage(SkIRect::MakeWH(image->width(), image->height()),
                                          image->makeNonTextureImage());
 }
 
 sk_sp<SkSpecialImage> SkPDFDevice::snapSpecial() {
     return nullptr;
 }