SkPDF: fix large-number bug

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 "SkAnnotation.h"
@@ skipping 23 matching lines @@
 #include "SkSurface.h"
 #include "SkTextFormatParams.h"
 #include "SkTemplates.h"
 #include "SkTypefacePriv.h"
 #include "SkXfermodeInterpretation.h"
 
 #define DPI_FOR_RASTER_SCALE_ONE 72
 
 // Utility functions
 
+static bool excessive_translation(const SkMatrix& m) {
+    const SkScalar kExcessiveTranslation = 8192.0f;
+    return SkScalarAbs(m.getTranslateX()) > kExcessiveTranslation
+        || SkScalarAbs(m.getTranslateY()) > kExcessiveTranslation;
+}
+
+static SkDraw untranslate(const SkDraw& d, SkVector* shiftVector) {
+    SkScalar translateX = d.fMatrix->getTranslateX() / d.fMatrix->getScaleX();
+    SkScalar translateY = d.fMatrix->getTranslateY() / d.fMatrix->getScaleY();
+    SkMatrix translate = *d.fMatrix;
+    translate.preTranslate(-translateX, -translateY);
+    SkASSERT(translate.getTranslateX() <= 1.0);

[tomhudson, 2015/11/16 16:35:30]

Do we want an fabs() here for paranoia?

[hal.canary, 2015/11/16 16:50:17]

done

+    SkDraw drawCopy(d);
+    drawCopy.fMatrix = &translate;

[tomhudson, 2015/11/16 16:35:30]

Uh-oh; returning reference to a local static - this is non-reentrant.
If I'm reading the docs right, it's entirely unsafe, because a static function
is only saying something about the linkage; you're returning a pointer to space
up above the stack.
Even if it were safe (static SkMatrix translate), I'd want a warning about
reentrancy issues, since the current uses of the function look compatible but
it's a nasty thing to hide.

I wish SkDraw.h told us clearly that the structure doesn't own any of the things
it points to, that it's guaranteed to have a default destructor, ...

[hal.canary, 2015/11/16 16:50:18]

Argh!

This is what I get from copy-and-pasting into a function!  It was right, and
then it wasn't/

Fixed.

+    if (shiftVector) {
+        shiftVector->set(translateX, translateY);
+    }
+    return drawCopy;
+}
+
 // 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
         == SkInterpretXfermode(*paint, false)) {
         paint->setXfermode(nullptr);
     }
 }
 
 static void emit_pdf_color(SkColor color, SkWStream* result) {
@@ skipping 740 matching lines @@
 
     if (count == 0) {
         return;
     }
 
     if (SkAnnotation* annotation = passedPaint.getAnnotation()) {
         if (handlePointAnnotation(points, count, *d.fMatrix, annotation)) {
             return;
         }
     }
+    if (excessive_translation(*d.fMatrix)) {
+        // https://bug.skia.org/257 If the translation is too large,
+        // PDF can't exactly represent the float values as numbers.

[tomhudson, 2015/11/16 16:35:30]

Nit: this comment is cut-and-pasted four times. Do we have somewhere we could
put it once, around excessive_translation() or untranslate(), which would still
be sufficiently descriptive? (It does help motivate the 8192.0f magic number.)

These conditionals are all similar, but templating it would be two or three
steps too far.

[hal.canary, 2015/11/16 16:50:18]

done

+        SkVector translate;
+        SkDraw drawCopy = untranslate(d, &translate);
+        SkTArray<SkPoint> pointsCopy(points, count);
+        SkPoint::Offset(&pointsCopy[0], count, translate.x(), translate.y());
+        this->drawPoints(drawCopy, mode, count, &pointsCopy[0], srcPaint);
+        return;  // NOTE: shader behavior will be off.
+    }
 
     // SkDraw::drawPoints converts to multiple calls to fDevice->drawPath.
     // We only use this when there's a path effect because of the overhead
     // of multiple calls to setUpContentEntry it causes.
     if (passedPaint.getPathEffect()) {
         if (d.fClip->isEmpty()) {
             return;
         }
         SkDraw pointDraw(d);
         pointDraw.fDevice = this;
@@ skipping 108 matching lines @@
 }
 
 void SkPDFDevice::drawRect(const SkDraw& d,
                            const SkRect& rect,
                            const SkPaint& srcPaint) {
     SkPaint paint = srcPaint;
     replace_srcmode_on_opaque_paint(&paint);
     SkRect r = rect;
     r.sort();
 
+    if (excessive_translation(*d.fMatrix)) {
+        // https://bug.skia.org/257 If the translation is too large,
+        // PDF can't exactly represent the float values as numbers.
+        SkVector translate;
+        SkDraw drawCopy = untranslate(d, &translate);
+        SkRect rectCopy = rect;
+        rectCopy.offset(translate.x(), translate.y());
+        this->drawRect(drawCopy, rectCopy, srcPaint);
+        return;  // NOTE: shader behavior will be off.
+    }
+
     if (paint.getPathEffect()) {
         if (d.fClip->isEmpty()) {
             return;
         }
         SkPath path;
         path.addRect(r);
         drawPath(d, path, paint, nullptr, true);
         return;
     }
 
@@ skipping 32 matching lines @@
     SkPath  path;
     path.addOval(oval);
     this->drawPath(draw, path, paint, nullptr, true);
 }
 
 void SkPDFDevice::drawPath(const SkDraw& d,
                            const SkPath& origPath,
                            const SkPaint& srcPaint,
                            const SkMatrix* prePathMatrix,
                            bool pathIsMutable) {
+    if (excessive_translation(*d.fMatrix)) {
+        // https://bug.skia.org/257 If the translation is too large,
+        // PDF can't exactly represent the float values as numbers.
+        SkVector translate;
+        SkDraw drawCopy = untranslate(d, &translate);
+        SkPath pathCopy(origPath);
+        pathCopy.offset(translate.x(), translate.y());
+        this->drawPath(drawCopy, pathCopy, srcPaint, prePathMatrix, true);
+        return;  // NOTE: shader behavior will be off.
+    }
+
     SkPaint paint = srcPaint;
     replace_srcmode_on_opaque_paint(&paint);
     SkPath modifiedPath;
     SkPath* pathPtr = const_cast<SkPath*>(&origPath);
 
     SkMatrix matrix = *d.fMatrix;
     if (prePathMatrix) {
         if (paint.getPathEffect() || paint.getStyle() != SkPaint::kFill_Style) {
             if (!pathIsMutable) {
                 pathPtr = &modifiedPath;
@@ skipping 272 matching lines @@
             device->drawText(d, text, len, x, y, transparent);
             break;
         default:
             SkFAIL("unknown text encoding");
     }
 }
 
 
 void SkPDFDevice::drawText(const SkDraw& d, const void* text, size_t len,
                            SkScalar x, SkScalar y, const SkPaint& srcPaint) {
+    if (excessive_translation(*d.fMatrix)) {
+        // https://bug.skia.org/257 If the translation is too large,
+        // PDF can't exactly represent the float values as numbers.
+        SkVector translate;
+        SkDraw drawCopy = untranslate(d, &translate);
+        this->drawText(drawCopy, text, len, x + translate.x(),
+                       y + translate.y(), srcPaint);
+        return;  // NOTE: shader behavior will be off.
+    }
+
     if (!SkPDFFont::CanEmbedTypeface(srcPaint.getTypeface(), fCanon)) {
         // https://bug.skia.org/3866
         SkPath path;
         srcPaint.getTextPath(text, len, x, y, &path);
         this->drawPath(d, path, srcPaint, &SkMatrix::I(), true);
         // Draw text transparently to make it copyable/searchable/accessable.
         draw_transparent_text(this, d, text, len, x, y, srcPaint);
         return;
     }
     SkPaint paint = srcPaint;
@@ skipping 41 matching lines @@
         content.entry()->fContent.writeText(encodedString.c_str());
         consumedGlyphCount += availableGlyphs;
         content.entry()->fContent.writeText(" Tj\n");
     }
     content.entry()->fContent.writeText("ET\n");
 }
 
 void SkPDFDevice::drawPosText(const SkDraw& d, const void* text, size_t len,
                               const SkScalar pos[], int scalarsPerPos,
                               const SkPoint& offset, const SkPaint& srcPaint) {
+    if (excessive_translation(*d.fMatrix)) {
+        // https://bug.skia.org/257 If the translation is too large,
+        // PDF can't exactly represent the float values as numbers.
+        SkVector translate;
+        SkDraw drawCopy = untranslate(d, &translate);
+        SkPoint offsetCopy = offset;
+        SkPoint::Offset(&offsetCopy, 1, translate.x(), translate.y());
+        this->drawPosText(drawCopy, text, len, pos, scalarsPerPos, offsetCopy,
+                          srcPaint);
+        return;  // NOTE: shader behavior will be off.

[tomhudson, 2015/11/16 16:35:30]

Can we quantify and document how much of a shader problem we'll have? Do we
sense whether it'll be seriously breaking? Are you proposing to just accept that
smaller bug now and fix it later if it becomes a problem?

[hal.canary, 2015/11/16 16:50:17]

It's already off (comparing PDF to 8888 in that GM will show you).
Yes.

+    }
+
     if (!SkPDFFont::CanEmbedTypeface(srcPaint.getTypeface(), fCanon)) {
         const SkPoint* positions = reinterpret_cast<const SkPoint*>(pos);
         SkAutoTMalloc<SkPoint> positionsBuffer;
         if (2 != scalarsPerPos) {
             int glyphCount = srcPaint.textToGlyphs(text, len, NULL);
             positionsBuffer.reset(glyphCount);
             for (int  i = 0; i < glyphCount; ++i) {
                 positionsBuffer[i].set(pos[i], 0.0f);
             }
             positions = &positionsBuffer[0];
         }
         SkPath path;
         srcPaint.getPosTextPath(text, len, positions, &path);
         SkMatrix matrix;
         matrix.setTranslate(offset);

[tomhudson, 2015/11/16 16:35:30]

This gets merged into d.fMatrix in drawPath(), *after* the excessive translation
check; looks like a failure case?

         this->drawPath(d, path, srcPaint, &matrix, true);
         // Draw text transparently to make it copyable/searchable/accessable.
         draw_transparent_text(
                 this, d, text, len, offset.x() + positions[0].x(),
                 offset.y() + positions[0].y(), srcPaint);
         return;
     }
 
     SkPaint paint = srcPaint;
     replace_srcmode_on_opaque_paint(&paint);
@@ skipping 1001 matching lines @@
     if (!pdfimage) {
         pdfimage.reset(SkPDFCreateBitmapObject(image));
         if (!pdfimage) {
             return;
         }
         fCanon->addPDFBitmap(image->uniqueID(), pdfimage);
     }
     SkPDFUtils::DrawFormXObject(this->addXObjectResource(pdfimage.get()),
                                 &content.entry()->fContent);
 }