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
diff --git a/gm/skbug_4868.cpp b/gm/skbug_4868.cpp
new file mode 100644
index 0000000..4f33715
--- /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 108f2c1..30d6ee7 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::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.
+ char result[kMaximumFloatDecimalLength];
+ size_t len = SkPDFUtils::FloatToDecimal(SkScalarToFloat(value), result);
+ SkASSERT(len < kMaximumFloatDecimalLength);
+ stream->write(result, len);
+}
-#if !defined(SK_ALLOW_LARGE_PDF_SCALARS)
- if (value > 32767 || value < -32767) {
- stream->writeDecAsText(SkScalarRoundToInt(value));
- return;
- }
+/** 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.
- char buffer[SkStrAppendScalar_MaxSize];
- char* end = SkStrAppendFixed(buffer, SkScalarToFixed(value));
- stream->write(buffer, end - buffer);
- return;
-#endif // !SK_ALLOW_LARGE_PDF_SCALARS
+ 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'. */
-#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;
+ /* 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'.
+
+ /* 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;
}
- stream->writeText(buffer);
- return;
-#endif // SK_ALLOW_LARGE_PDF_SCALARS
+ 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.
+ }
+ 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 0aa05a0..814d771 100644
--- a/src/pdf/SkPDFUtils.h
+++ b/src/pdf/SkPDFUtils.h
@@ -58,6 +58,14 @@
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 9ca1bd6..f618e6f 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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);
+ }
+}