SkPDF: fix scalar serialization

BUG=skia:4868
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1720863003

Review URL: https://codereview.chromium.org/1720863003

4 files changed, 212 insertions, 64 deletions

diff --git a/gm/skbug_4868.cpp b/gm/skbug_4868.cpp
new file mode 100644
index 0000000000..4f33715a43
--- /dev/null
+++ b/gm/skbug_4868.cpp
@@ -0,0 +1,23 @@
+/*
+ * Copyright 2016 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "gm.h"
+
+// clipRect and drawLine should line up exactly when they use the same point.
+// When SkPDF rounds large floats, this doesn't always happen.
+DEF_SIMPLE_GM(skbug_4868, canvas, 32, 32) {
+    canvas->translate(-68.0f, -3378.0f);
+    SkPaint paint;
+    paint.setAntiAlias(true);
+    paint.setStyle(SkPaint::kStroke_Style);
+    canvas->scale(0.56692914f, 0.56692914f);
+    SkRect rc = SkRect::MakeLTRB(158.0f, 5994.80273f, 165.0f, 5998.80225f);
+    canvas->clipRect(rc);
+    canvas->clear(0xFFCECFCE);
+    canvas->drawLine(rc.left(), rc.top(), rc.right(), rc.bottom(), paint);
+    canvas->drawLine(rc.right(), rc.top(), rc.left(), rc.bottom(), paint);
+}
diff --git a/src/pdf/SkPDFUtils.cpp b/src/pdf/SkPDFUtils.cpp
index 108f2c10ab..30d6ee7d68 100644
--- a/src/pdf/SkPDFUtils.cpp
+++ b/src/pdf/SkPDFUtils.cpp
@@ -16,6 +16,8 @@
 #include "SkString.h"
 #include "SkPDFTypes.h"
 
+#include <cmath>
+
 //static
 SkPDFArray* SkPDFUtils::RectToArray(const SkRect& rect) {
     SkPDFArray* result = new SkPDFArray();
@@ -254,51 +256,120 @@ void SkPDFUtils::ApplyPattern(int objectIndex, SkWStream* content) {
 }
 
 void SkPDFUtils::AppendScalar(SkScalar value, SkWStream* stream) {
-    // The range of reals in PDF/A is the same as SkFixed: +/- 32,767 and
-    // +/- 1/65,536 (though integers can range from 2^31 - 1 to -2^31).
-    // When using floats that are outside the whole value range, we can use
-    // integers instead.
-
-#if !defined(SK_ALLOW_LARGE_PDF_SCALARS)
-    if (value > 32767 || value < -32767) {
-        stream->writeDecAsText(SkScalarRoundToInt(value));
-        return;
-    }
+    char result[kMaximumFloatDecimalLength];
+    size_t len = SkPDFUtils::FloatToDecimal(SkScalarToFloat(value), result);
+    SkASSERT(len < kMaximumFloatDecimalLength);
+    stream->write(result, len);
+}
+
+/** Write a string into result, includeing a terminating '\0' (for
+    unit testing).  Return strlen(result) (for SkWStream::write) The
+    resulting string will be in the form /[-]?([0-9]*.)?[0-9]+/ and
+    sscanf(result, "%f", &x) will return the original value iff the
+    value is finite. This function accepts all possible input values.
+
+    Motivation: "PDF does not support [numbers] in exponential format
+    (such as 6.02e23)."  Otherwise, this function would rely on a
+    sprintf-type function from the standard library. */
+size_t SkPDFUtils::FloatToDecimal(float value,
+                                  char result[kMaximumFloatDecimalLength]) {
+    /* The longest result is -FLT_MIN.
+       We serialize it as "-.0000000000000000000000000000000000000117549435"
+       which has 48 characters plus a terminating '\0'. */
+
+    /* section C.1 of the PDF1.4 spec (http://goo.gl/0SCswJ) says that
+       most PDF rasterizers will use fixed-point scalars that lack the
+       dynamic range of floats.  Even if this is the case, I want to
+       serialize these (uncommon) very small and very large scalar
+       values with enough precision to allow a floating-point
+       rasterizer to read them in with perfect accuracy.
+       Experimentally, rasterizers such as pdfium do seem to benefit
+       from this.  Rasterizers that rely on fixed-point scalars should
+       gracefully ignore these values that they can not parse. */
+    char* output = &result[0];
+    const char* const end = &result[kMaximumFloatDecimalLength - 1];
+    // subtract one to leave space for '\0'.
 
-    char buffer[SkStrAppendScalar_MaxSize];
-    char* end = SkStrAppendFixed(buffer, SkScalarToFixed(value));
-    stream->write(buffer, end - buffer);
-    return;
-#endif  // !SK_ALLOW_LARGE_PDF_SCALARS
-
-#if defined(SK_ALLOW_LARGE_PDF_SCALARS)
-    // Floats have 24bits of significance, so anything outside that range is
-    // no more precise than an int. (Plus PDF doesn't support scientific
-    // notation, so this clamps to SK_Max/MinS32).
-    if (value > (1 << 24) || value < -(1 << 24)) {
-        stream->writeDecAsText(value);
-        return;
+    /* This function is written to accept any possible input value,
+       including non-finite values such as INF and NAN.  In that case,
+       we ignore value-correctness and and output a syntacticly-valid
+       number. */
+    if (value == SK_FloatInfinity) {
+        value = FLT_MAX;  // nearest finite float.
     }
-    // Continue to enforce the PDF limits for small floats.
-    if (value < 1.0f/65536 && value > -1.0f/65536) {
-        stream->writeDecAsText(0);
-        return;
+    if (value == SK_FloatNegativeInfinity) {
+        value = -FLT_MAX;  // nearest finite float.
     }
-    // SkStrAppendFloat might still use scientific notation, so use snprintf
-    // directly..
-    static const int kFloat_MaxSize = 19;
-    char buffer[kFloat_MaxSize];
-    int len = SNPRINTF(buffer, kFloat_MaxSize, "%#.8f", value);
-    // %f always prints trailing 0s, so strip them.
-    for (; buffer[len - 1] == '0' && len > 0; len--) {
-        buffer[len - 1] = '\0';
+    if (!std::isfinite(value) || value == 0.0f) {
+        // NAN is unsupported in PDF.  Always output a valid number.
+        // Also catch zero here, as a special case.
+        *output++ = '0';
+        *output = '\0';
+        return output - result;
     }
-    if (buffer[len - 1] == '.') {
-        buffer[len - 1] = '\0';
+    // Inspired by:
+    // http://www.exploringbinary.com/quick-and-dirty-floating-point-to-decimal-conversion/
+
+    if (value < 0.0) {
+        *output++ = '-';
+        value = -value;
+    }
+    SkASSERT(value >= 0.0f);
+
+    // Must use double math to keep precision right.
+    double intPart;
+    double fracPart = std::modf(static_cast<double>(value), &intPart);
+    SkASSERT(intPart + fracPart == static_cast<double>(value));
+    size_t significantDigits = 0;
+    const size_t maxSignificantDigits = 9;
+    // Any fewer significant digits loses precision.  The unit test
+    // checks round-trip correctness.
+    SkASSERT(intPart >= 0.0 && fracPart >= 0.0);  // negative handled already.
+    SkASSERT(intPart > 0.0 || fracPart > 0.0);  // zero already caught.
+    if (intPart > 0.0) {
+        // put the intPart digits onto a stack for later reversal.
+        char reversed[1 + FLT_MAX_10_EXP];  // 39 == 1 + FLT_MAX_10_EXP
+        // the largest integer part is FLT_MAX; it has 39 decimal digits.
+        size_t reversedIndex = 0;
+        do {
+            SkASSERT(reversedIndex < sizeof(reversed));
+            int digit = static_cast<int>(std::fmod(intPart, 10.0));
+            SkASSERT(digit >= 0 && digit <= 9);
+            reversed[reversedIndex++] = '0' + digit;
+            intPart = std::floor(intPart / 10.0);
+        } while (intPart > 0.0);
+        significantDigits = reversedIndex;
+        SkASSERT(reversedIndex <= sizeof(reversed));
+        SkASSERT(output + reversedIndex <= end);
+        while (reversedIndex-- > 0) {  // pop from stack, append to result
+            *output++ = reversed[reversedIndex];
+        }
+    }
+    if (fracPart > 0 && significantDigits < maxSignificantDigits) {
+        *output++ = '.';
+        SkASSERT(output <= end);
+        do {
+            fracPart = std::modf(fracPart * 10.0, &intPart);
+            int digit = static_cast<int>(intPart);
+            SkASSERT(digit >= 0 && digit <= 9);
+            *output++ = '0' + digit;
+            SkASSERT(output <= end);
+            if (digit > 0 || significantDigits > 0) {
+                // start counting significantDigits after first non-zero digit.
+                ++significantDigits;
+            }
+        } while (fracPart > 0.0
+                 && significantDigits < maxSignificantDigits
+                 && output < end);
+        // When fracPart == 0, additional digits will be zero.
+        // When significantDigits == maxSignificantDigits, we can stop.
+        // when output == end, we have filed the string.
+        // Note: denormalized numbers will not have the same number of
+        // significantDigits, but do not need them to round-trip.
     }
-    stream->writeText(buffer);
-    return;
-#endif  // SK_ALLOW_LARGE_PDF_SCALARS
+    SkASSERT(output <= end);
+    *output = '\0';
+    return output - result;
 }
 
 SkString SkPDFUtils::FormatString(const char* cin, size_t len) {
diff --git a/src/pdf/SkPDFUtils.h b/src/pdf/SkPDFUtils.h
index 0aa05a088a..814d77193c 100644
--- a/src/pdf/SkPDFUtils.h
+++ b/src/pdf/SkPDFUtils.h
@@ -58,6 +58,14 @@ public:
     static void DrawFormXObject(int objectIndex, SkWStream* content);
     static void ApplyGraphicState(int objectIndex, SkWStream* content);
     static void ApplyPattern(int objectIndex, SkWStream* content);
+
+    // 3 = '-', '.', and '\0' characters.
+    // 9 = number of significant digits
+    // abs(FLT_MIN_10_EXP) = number of zeros in FLT_MIN
+    static const size_t kMaximumFloatDecimalLength = 3 + 9 - FLT_MIN_10_EXP;
+    // FloatToDecimal is exposed for unit tests.
+    static size_t FloatToDecimal(float value,
+                                 char output[kMaximumFloatDecimalLength]);
     static void AppendScalar(SkScalar value, SkWStream* stream);
     static SkString FormatString(const char* input, size_t len);
 };
diff --git a/tests/PDFPrimitivesTest.cpp b/tests/PDFPrimitivesTest.cpp
index 9ca1bd619a..f618e6f68b 100644
--- a/tests/PDFPrimitivesTest.cpp
+++ b/tests/PDFPrimitivesTest.cpp
@@ -18,6 +18,7 @@
 #include "SkPDFFont.h"
 #include "SkPDFStream.h"
 #include "SkPDFTypes.h"
+#include "SkPDFUtils.h"
 #include "SkReadBuffer.h"
 #include "SkScalar.h"
 #include "SkStream.h"
@@ -199,20 +200,16 @@ static void TestPDFUnion(skiatest::Reporter* reporter) {
     ASSERT_EMIT_EQ(reporter, int42, "42");
 
     SkPDFUnion realHalf = SkPDFUnion::Scalar(SK_ScalarHalf);
-    ASSERT_EMIT_EQ(reporter, realHalf, "0.5");
+    ASSERT_EMIT_EQ(reporter, realHalf, ".5");
 
     SkPDFUnion bigScalar = SkPDFUnion::Scalar(110999.75f);
-#if !defined(SK_ALLOW_LARGE_PDF_SCALARS)
-    ASSERT_EMIT_EQ(reporter, bigScalar, "111000");
-#else
     ASSERT_EMIT_EQ(reporter, bigScalar, "110999.75");
 
-    SkPDFUnion biggerScalar = SkPDFUnion::Scalar(50000000.1);
+    SkPDFUnion biggerScalar = SkPDFUnion::Scalar(50000000.1f);
     ASSERT_EMIT_EQ(reporter, biggerScalar, "50000000");
 
-    SkPDFUnion smallestScalar = SkPDFUnion::Scalar(1.0 / 65536);
-    ASSERT_EMIT_EQ(reporter, smallestScalar, "0.00001526");
-#endif
+    SkPDFUnion smallestScalar = SkPDFUnion::Scalar(1.0f / 65536);
+    ASSERT_EMIT_EQ(reporter, smallestScalar, ".0000152587890");
 
     SkPDFUnion stringSimple = SkPDFUnion::String("test ) string ( foo");
     ASSERT_EMIT_EQ(reporter, stringSimple, "(test \\) string \\( foo)");
@@ -248,34 +245,34 @@ static void TestPDFArray(skiatest::Reporter* reporter) {
     ASSERT_EMIT_EQ(reporter, *array, "[42]");
 
     array->appendScalar(SK_ScalarHalf);
-    ASSERT_EMIT_EQ(reporter, *array, "[42 0.5]");
+    ASSERT_EMIT_EQ(reporter, *array, "[42 .5]");
 
     array->appendInt(0);
-    ASSERT_EMIT_EQ(reporter, *array, "[42 0.5 0]");
+    ASSERT_EMIT_EQ(reporter, *array, "[42 .5 0]");
 
     array->appendBool(true);
-    ASSERT_EMIT_EQ(reporter, *array, "[42 0.5 0 true]");
+    ASSERT_EMIT_EQ(reporter, *array, "[42 .5 0 true]");
 
     array->appendName("ThisName");
-    ASSERT_EMIT_EQ(reporter, *array, "[42 0.5 0 true /ThisName]");
+    ASSERT_EMIT_EQ(reporter, *array, "[42 .5 0 true /ThisName]");
 
     array->appendName(SkString("AnotherName"));
-    ASSERT_EMIT_EQ(reporter, *array, "[42 0.5 0 true /ThisName /AnotherName]");
+    ASSERT_EMIT_EQ(reporter, *array, "[42 .5 0 true /ThisName /AnotherName]");
 
     array->appendString("This String");
     ASSERT_EMIT_EQ(reporter, *array,
-                   "[42 0.5 0 true /ThisName /AnotherName (This String)]");
+                   "[42 .5 0 true /ThisName /AnotherName (This String)]");
 
     array->appendString(SkString("Another String"));
     ASSERT_EMIT_EQ(reporter, *array,
-                   "[42 0.5 0 true /ThisName /AnotherName (This String) "
+                   "[42 .5 0 true /ThisName /AnotherName (This String) "
                    "(Another String)]");
 
     SkAutoTUnref<SkPDFArray> innerArray(new SkPDFArray);
     innerArray->appendInt(-1);
     array->appendObject(innerArray.detach());
     ASSERT_EMIT_EQ(reporter, *array,
-                   "[42 0.5 0 true /ThisName /AnotherName (This String) "
+                   "[42 .5 0 true /ThisName /AnotherName (This String) "
                    "(Another String) [-1]]");
 
     SkAutoTUnref<SkPDFArray> referencedArray(new SkPDFArray);
@@ -287,7 +284,7 @@ static void TestPDFArray(skiatest::Reporter* reporter) {
 
     SkString result = emit_to_string(*array, &catalog);
     ASSERT_EQ(reporter, result,
-              "[42 0.5 0 true /ThisName /AnotherName (This String) "
+              "[42 .5 0 true /ThisName /AnotherName (This String) "
               "(Another String) [-1] 1 0 R]");
 }
 
@@ -310,7 +307,7 @@ static void TestPDFDict(skiatest::Reporter* reporter) {
     SkAutoTUnref<SkPDFArray> innerArray(new SkPDFArray);
     innerArray->appendInt(-100);
     dict->insertObject(n3, innerArray.detach());
-    ASSERT_EMIT_EQ(reporter, *dict, "<</n1 42\n/n2 0.5\n/n3 [-100]>>");
+    ASSERT_EMIT_EQ(reporter, *dict, "<</n1 42\n/n2 .5\n/n3 [-100]>>");
 
     dict.reset(new SkPDFDict);
     ASSERT_EMIT_EQ(reporter, *dict, "<<>>");
@@ -322,26 +319,26 @@ static void TestPDFDict(skiatest::Reporter* reporter) {
     ASSERT_EMIT_EQ(reporter, *dict, "<</n1 24\n/n2 99>>");
 
     dict->insertScalar("n3", SK_ScalarHalf);
-    ASSERT_EMIT_EQ(reporter, *dict, "<</n1 24\n/n2 99\n/n3 0.5>>");
+    ASSERT_EMIT_EQ(reporter, *dict, "<</n1 24\n/n2 99\n/n3 .5>>");
 
     dict->insertName("n4", "AName");
-    ASSERT_EMIT_EQ(reporter, *dict, "<</n1 24\n/n2 99\n/n3 0.5\n/n4 /AName>>");
+    ASSERT_EMIT_EQ(reporter, *dict, "<</n1 24\n/n2 99\n/n3 .5\n/n4 /AName>>");
 
     dict->insertName("n5", SkString("AnotherName"));
-    ASSERT_EMIT_EQ(reporter, *dict, "<</n1 24\n/n2 99\n/n3 0.5\n/n4 /AName\n"
+    ASSERT_EMIT_EQ(reporter, *dict, "<</n1 24\n/n2 99\n/n3 .5\n/n4 /AName\n"
                    "/n5 /AnotherName>>");
 
     dict->insertString("n6", "A String");
-    ASSERT_EMIT_EQ(reporter, *dict, "<</n1 24\n/n2 99\n/n3 0.5\n/n4 /AName\n"
+    ASSERT_EMIT_EQ(reporter, *dict, "<</n1 24\n/n2 99\n/n3 .5\n/n4 /AName\n"
                    "/n5 /AnotherName\n/n6 (A String)>>");
 
     dict->insertString("n7", SkString("Another String"));
-    ASSERT_EMIT_EQ(reporter, *dict, "<</n1 24\n/n2 99\n/n3 0.5\n/n4 /AName\n"
+    ASSERT_EMIT_EQ(reporter, *dict, "<</n1 24\n/n2 99\n/n3 .5\n/n4 /AName\n"
                    "/n5 /AnotherName\n/n6 (A String)\n/n7 (Another String)>>");
 
     dict.reset(new SkPDFDict("DType"));
     ASSERT_EMIT_EQ(reporter, *dict, "<</Type /DType>>");
-    
+
     SkAutoTUnref<SkPDFArray> referencedArray(new SkPDFArray);
     Catalog catalog;
     catalog.numbers.addObject(referencedArray.get());
@@ -435,3 +432,52 @@ DEF_TEST(PDFFontCanEmbedTypeface, reporter) {
     REPORTER_ASSERT(reporter,
                     SkPDFFont::CanEmbedTypeface(portableTypeface, &canon));
 }
+
+
+// test to see that all finite scalars round trip via scanf().
+static void check_pdf_scalar_serialization(
+        skiatest::Reporter* reporter, float inputFloat) {
+    char floatString[SkPDFUtils::kMaximumFloatDecimalLength];
+    size_t len = SkPDFUtils::FloatToDecimal(inputFloat, floatString);
+    if (len >= sizeof(floatString)) {
+        ERRORF(reporter, "string too long: %u", (unsigned)len);
+        return;
+    }
+    if (floatString[len] != '\0' || strlen(floatString) != len) {
+        ERRORF(reporter, "terminator misplaced.");
+        return;  // The terminator is needed for sscanf().
+    }
+    if (reporter->verbose()) {
+        SkDebugf("%15.9g = \"%s\"\n", inputFloat, floatString);
+    }
+    float roundTripFloat;
+    if (1 != sscanf(floatString, "%f", &roundTripFloat)) {
+        ERRORF(reporter, "unscannable result: %s", floatString);
+        return;
+    }
+    if (isfinite(inputFloat) && roundTripFloat != inputFloat) {
+        ERRORF(reporter, "roundTripFloat (%.9g) != inputFloat (%.9g)",
+               roundTripFloat, inputFloat);
+    }
+}
+
+// Test SkPDFUtils::AppendScalar for accuracy.
+DEF_TEST(PDFPrimitives_Scalar, reporter) {
+    SkRandom random(0x5EED);
+    int iterationCount = 512;
+    while (iterationCount-- > 0) {
+        union { uint32_t u; float f; };
+        u = random.nextU();
+        static_assert(sizeof(float) == sizeof(uint32_t), "");
+        check_pdf_scalar_serialization(reporter, f);
+    }
+    float alwaysCheck[] = {
+        0.0f, -0.0f, 1.0f, -1.0f, SK_ScalarPI, 0.1f, FLT_MIN, FLT_MAX,
+        -FLT_MIN, -FLT_MAX, FLT_MIN / 16.0f, -FLT_MIN / 16.0f,
+        SK_FloatNaN, SK_FloatInfinity, SK_FloatNegativeInfinity,
+        -FLT_MIN / 8388608.0
+    };
+    for (float inputFloat: alwaysCheck) {
+        check_pdf_scalar_serialization(reporter, inputFloat);
+    }
+}