SkPDF: fix scalar serialization

src/pdf/SkPDFUtils.cpp

Index: src/pdf/SkPDFUtils.cpp
diff --git a/src/pdf/SkPDFUtils.cpp b/src/pdf/SkPDFUtils.cpp
index 108f2c10abd56e6d0ae94b263c89c5c9a5d34dd4..2ebbec5779fd2e2880900ceb939525c06bb38e14 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();
@@ -253,52 +255,70 @@ void SkPDFUtils::ApplyPattern(int objectIndex, SkWStream* content) {
     content->writeText(" scn\n");
 }
 
-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;
+void SkPDFUtils::AppendScalar(SkScalar v, SkWStream* stream) {
+    if (v == SK_FloatInfinity) {
+        v = FLT_MAX;  // nearest finite float.
     }
-
-    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);
+    if (v == SK_FloatNegativeInfinity) {
+        v = -FLT_MAX;  // nearest finite float.
+    }
+    if (!isfinite(v)) {
+        stream->writeText("0");  // Unsupported number in PDF
         return;
     }
-    // Continue to enforce the PDF limits for small floats.
-    if (value < 1.0f/65536 && value > -1.0f/65536) {
-        stream->writeDecAsText(0);
+    // "PDF does not support [numbers] in exponential format (such as 6.02e23)"
+    // Inspired by:
+    // http://www.exploringbinary.com/quick-and-dirty-floating-point-to-decimal-conversion/
+    // Must use double math to keep precision right.
+    double value = static_cast<double>(v);
+    char result[1076];
+    size_t resultIndex = 0;
+    if (value < 0.0) {
+        result[resultIndex++] = '-';
+        value = -value;
+    }
+    double intPart;
+    double fracPart = std::modf(value, &intPart);
+    size_t significantDigits = 0;
+    const size_t maxSignificantDigits = 9; // any less loses precision

[tomhudson, 2016/02/22 20:13:46]

With a 32b signed fixed, can't we have 10 significant digits, 5 on each side of
the decimal point?

[hal.canary, 2016/02/22 22:45:38]

Ah, but we don't have a fixed-point input.  SkScalar is float.

+    SkASSERT(intPart >= 0.0 && fracPart >= 0.0);

[tomhudson, 2016/02/22 20:13:46]

What happened to negative numbers? std::modf() "[D]ecomposes given floating
point value x into integral and fractional parts, each having the same type and
sign as x."

[hal.canary, 2016/02/22 22:45:38]

I already dealt with negative number.

[tomhudson, 2016/02/24 20:10:56]

Acknowledged.

+    if (intPart == 0.0 && fracPart == 0.0) {
+        stream->writeText("0");  // Unsupported number in PDF

[tomhudson, 2016/02/22 20:13:46]

Why is 0 unsupported?

[hal.canary, 2016/02/22 22:45:38]

cut-and-paste error.

         return;
     }
-    // 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 (intPart > 0.0) {

[tomhudson, 2016/02/22 20:13:46]

This code looks like it was hard to write; it should not be so hard to read.

+        char reversed[311];

[tomhudson, 2016/02/22 20:13:46]

What's with the magic number 311? No idea how you came up with it.

[hal.canary, 2016/02/22 22:45:38]

that's a holdover from the double code.  I did some experimentation and found
the correct number for single-precision and then documented why I use that one.

+        size_t reveredIndex = 0;

[tomhudson, 2016/02/22 20:13:46]

Why is this index holy or particularly esteemed? (spelling?)

[hal.canary, 2016/02/22 22:45:38]

done

+        while (intPart > 0.0) {
+            SkASSERT(reveredIndex < sizeof(reversed));
+            int digit = static_cast<int>(std::fmod(intPart, 10.0));
+            SkASSERT(digit >= 0 && digit <= 9);
+            reversed[reveredIndex++] = '0' + digit;
+            intPart = std::floor(intPart / 10.0);
+        }
+        significantDigits = reveredIndex;
+        while (reveredIndex-- > 0) {
+            result[resultIndex++] = reversed[reveredIndex];
+            SkASSERT(resultIndex <= sizeof(result));
+        }
     }
-    if (buffer[len - 1] == '.') {
-        buffer[len - 1] = '\0';
+    if (fracPart > 0 && significantDigits < maxSignificantDigits) {
+        result[resultIndex++] = '.';
+        SkASSERT(resultIndex <= sizeof(result));
+        do {
+            fracPart *= 10.0;
+            fracPart = std::modf(fracPart, &intPart);
+            int digit = static_cast<int>(intPart);
+            result[resultIndex++] = '0' + digit;
+            SkASSERT(digit >= 0 && digit <= 9);
+            SkASSERT(resultIndex <= sizeof(result));
+            if (digit > 0 || significantDigits > 0) {
+                ++significantDigits;
+            }
+        } while (fracPart > 0 && significantDigits < maxSignificantDigits);
+
     }
-    stream->writeText(buffer);
-    return;
-#endif  // SK_ALLOW_LARGE_PDF_SCALARS
+    stream->write(result, resultIndex);
 }
 
 SkString SkPDFUtils::FormatString(const char* cin, size_t len) {