SkPDF: fix large-number bug

src/pdf/SkPDFDevice.cpp

Index: src/pdf/SkPDFDevice.cpp
diff --git a/src/pdf/SkPDFDevice.cpp b/src/pdf/SkPDFDevice.cpp
index 6680ea472e9e534c20fa8a920685f55921a643f4..5f0937c3aff4bf1e4d1668f86c44929c31102cc9 100644
--- a/src/pdf/SkPDFDevice.cpp
+++ b/src/pdf/SkPDFDevice.cpp
@@ -41,6 +41,26 @@
 
 // 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) {
@@ -801,6 +821,16 @@ void SkPDFDevice::drawPoints(const SkDraw& d,
             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
@@ -929,6 +959,17 @@ void SkPDFDevice::drawRect(const SkDraw& d,
     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;
@@ -981,6 +1022,17 @@ void SkPDFDevice::drawPath(const SkDraw& d,
                            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;
@@ -1273,6 +1325,16 @@ static void draw_transparent_text(SkPDFDevice* device,
 
 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;
@@ -1334,6 +1396,18 @@ void SkPDFDevice::drawText(const SkDraw& d, const void* text, size_t len,
 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;