Convert floats to string better in CPDF_Number::WriteTo()
The existing implementation of CPDF_Number::WriteTo() for floats results
in strings that cannot be represented as a float (due to precision
error) and has certain restrictions (constrained within integer min/max
bounds, only a certain number of significant digits). These restrictions
are unnecessary for writing to PDFs and are not part of the PDF
specification. They are also causing slight miscalculations in PDFs.
Change to use WriteFloat() instead, which will write the float value to
string as-is with all necessary digits.
To use WriteFloat() in core/fpdfapi/parser, split it into its own build
target to avoid circular dependencies.
Some tests have floats in PDFs that change slightly with the new string
representation, so modify their checksums or values. The overall PDF
should not have any noticeable change.
Bug: 352496170
Change-Id: I93539341dc2eee9739db02dd8923d5ed857b8601
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/124272
Commit-Queue: Andy Phan <andyphan@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
diff --git a/core/fpdfapi/edit/BUILD.gn b/core/fpdfapi/edit/BUILD.gn
index ddc8bb9..38db4eb 100644
--- a/core/fpdfapi/edit/BUILD.gn
+++ b/core/fpdfapi/edit/BUILD.gn
@@ -7,8 +7,6 @@
source_set("edit") {
sources = [
- "cpdf_contentstream_write_utils.cpp",
- "cpdf_contentstream_write_utils.h",
"cpdf_creator.cpp",
"cpdf_creator.h",
"cpdf_npagetooneexporter.cpp",
@@ -29,6 +27,7 @@
"../../../:pdfium_noshorten_config",
]
deps = [
+ ":contentstream_write_utils",
"../../../constants",
"../../fxcrt",
"../font",
@@ -38,6 +37,18 @@
visibility = [ "../../../*" ]
}
+source_set("contentstream_write_utils") {
+ sources = [
+ "cpdf_contentstream_write_utils.cpp",
+ "cpdf_contentstream_write_utils.h",
+ ]
+ configs += [
+ "../../../:pdfium_strict_config",
+ "../../../:pdfium_noshorten_config",
+ ]
+ deps = [ "../../fxcrt" ]
+}
+
pdfium_unittest_source_set("unittests") {
sources = [
"cpdf_npagetooneexporter_unittest.cpp",
diff --git a/core/fpdfapi/edit/cpdf_contentstream_write_utils.h b/core/fpdfapi/edit/cpdf_contentstream_write_utils.h
index 2440d92..dfebe6b 100644
--- a/core/fpdfapi/edit/cpdf_contentstream_write_utils.h
+++ b/core/fpdfapi/edit/cpdf_contentstream_write_utils.h
@@ -9,7 +9,10 @@
#include "core/fxcrt/fx_coordinates.h"
+// Writes `value` to `stream` as a decimal string in a form suitable for use in
+// a PDF content stream. Does not use scientific notation. Omits leading zeroes.
std::ostream& WriteFloat(std::ostream& stream, float value);
+
std::ostream& WriteMatrix(std::ostream& stream, const CFX_Matrix& matrix);
std::ostream& WritePoint(std::ostream& stream, const CFX_PointF& point);
std::ostream& WriteRect(std::ostream& stream, const CFX_FloatRect& rect);
diff --git a/core/fpdfapi/parser/BUILD.gn b/core/fpdfapi/parser/BUILD.gn
index 858ddaa..94ab93e 100644
--- a/core/fpdfapi/parser/BUILD.gn
+++ b/core/fpdfapi/parser/BUILD.gn
@@ -81,6 +81,7 @@
"../../../constants",
"../../fdrm",
"../../fxcodec",
+ "../edit:contentstream_write_utils",
]
public_deps = [ "../../fxcrt" ]
allow_circular_includes_from = []
diff --git a/core/fpdfapi/parser/cpdf_number.cpp b/core/fpdfapi/parser/cpdf_number.cpp
index 50e626d..f77b9a2 100644
--- a/core/fpdfapi/parser/cpdf_number.cpp
+++ b/core/fpdfapi/parser/cpdf_number.cpp
@@ -6,7 +6,21 @@
#include "core/fpdfapi/parser/cpdf_number.h"
+#include <sstream>
+
+#include "core/fpdfapi/edit/cpdf_contentstream_write_utils.h"
#include "core/fxcrt/fx_stream.h"
+#include "core/fxcrt/fx_string_wrappers.h"
+
+namespace {
+
+ByteString FloatToString(float value) {
+ fxcrt::ostringstream sstream;
+ WriteFloat(sstream, value);
+ return ByteString(sstream);
+}
+
+} // namespace
CPDF_Number::CPDF_Number() = default;
@@ -45,12 +59,17 @@
}
ByteString CPDF_Number::GetString() const {
+ // TODO(crbug.com/352496170): Try using FloatToString() here.
return number_.IsInteger() ? ByteString::FormatInteger(number_.GetSigned())
: ByteString::FormatFloat(number_.GetFloat());
}
bool CPDF_Number::WriteTo(IFX_ArchiveStream* archive,
const CPDF_Encryptor* encryptor) const {
+ // For floats, because this is writing to file, use WriteFloat() instead of
+ // GetString().
return archive->WriteString(" ") &&
- archive->WriteString(GetString().AsStringView());
+ archive->WriteString(
+ (number_.IsInteger() ? GetString() : FloatToString(GetNumber()))
+ .AsStringView());
}
diff --git a/core/fpdfapi/parser/cpdf_number_unittest.cpp b/core/fpdfapi/parser/cpdf_number_unittest.cpp
index bc1b469..2a221a1 100644
--- a/core/fpdfapi/parser/cpdf_number_unittest.cpp
+++ b/core/fpdfapi/parser/cpdf_number_unittest.cpp
@@ -55,33 +55,32 @@
}
{
ByteStringArchiveStream output_stream;
+ // `number` cannot be represented as a float without losing precision.
auto number = pdfium::MakeRetain<CPDF_Number>(38.895285f);
ASSERT_TRUE(number->WriteTo(&output_stream, /*encryptor=*/nullptr));
- // TODO(crbug.com/352496170): Can the output have more precision?
- EXPECT_EQ(" 38.8953", output_stream.str());
+ EXPECT_EQ(" 38.895287", output_stream.str());
}
{
ByteStringArchiveStream output_stream;
+ // `number` cannot be represented as a float without losing precision.
auto number = pdfium::MakeRetain<CPDF_Number>(-77.037232f);
ASSERT_TRUE(number->WriteTo(&output_stream, /*encryptor=*/nullptr));
- // TODO(crbug.com/352496170): Can the output have more precision?
- EXPECT_EQ(" -77.0372", output_stream.str());
+ EXPECT_EQ(" -77.037231", output_stream.str());
}
{
ByteStringArchiveStream output_stream;
auto number =
pdfium::MakeRetain<CPDF_Number>(std::numeric_limits<float>::max());
ASSERT_TRUE(number->WriteTo(&output_stream, /*encryptor=*/nullptr));
- // TODO(crbug.com/352496170): This should be much bigger than INT_MAX.
- EXPECT_EQ(" 2147483647", output_stream.str());
+ EXPECT_EQ(" 340282350000000000000000000000000000000", output_stream.str());
}
{
ByteStringArchiveStream output_stream;
auto number =
pdfium::MakeRetain<CPDF_Number>(std::numeric_limits<float>::min());
ASSERT_TRUE(number->WriteTo(&output_stream, /*encryptor=*/nullptr));
- // TODO(crbug.com/352496170): This should not be 0.
- EXPECT_EQ(" 0", output_stream.str());
+ EXPECT_EQ(" .0000000000000000000000000000000000000117549435",
+ output_stream.str());
}
}
diff --git a/core/fpdfdoc/BUILD.gn b/core/fpdfdoc/BUILD.gn
index 649fe90..fecd518 100644
--- a/core/fpdfdoc/BUILD.gn
+++ b/core/fpdfdoc/BUILD.gn
@@ -83,7 +83,7 @@
]
deps = [
"../../constants",
- "../fpdfapi/edit",
+ "../fpdfapi/edit:contentstream_write_utils",
"../fpdfapi/font",
"../fpdfapi/page",
"../fpdfapi/parser",
diff --git a/fpdfsdk/BUILD.gn b/fpdfsdk/BUILD.gn
index abf055f..1d1acce 100644
--- a/fpdfsdk/BUILD.gn
+++ b/fpdfsdk/BUILD.gn
@@ -70,6 +70,7 @@
"../constants",
"../core/fdrm",
"../core/fpdfapi/edit",
+ "../core/fpdfapi/edit:contentstream_write_utils",
"../core/fpdfapi/font",
"../core/fpdfapi/page",
"../core/fpdfapi/parser",
diff --git a/fpdfsdk/fpdf_annot_embeddertest.cpp b/fpdfsdk/fpdf_annot_embeddertest.cpp
index 147d6fa..21fd3c7 100644
--- a/fpdfsdk/fpdf_annot_embeddertest.cpp
+++ b/fpdfsdk/fpdf_annot_embeddertest.cpp
@@ -1177,17 +1177,17 @@
const char* md5_new_annot = []() {
if (CFX_DefaultRenderDevice::UseSkiaRenderer()) {
#if BUILDFLAG(IS_WIN)
- return "2fb4dde3342a5bec5acb2999bf3add15";
+ return "9ca0a274d1ae0db09ad814ee455dd88c";
#elif BUILDFLAG(IS_APPLE)
- return "0ca554e90329a46fc26f637c5218ebeb";
+ return "71c8fb8eee9720c19851c48745dde152";
#else
- return "6556da390ee2f3ca95ff9a6b8506ca74";
+ return "f522e1262f487cc1976bb3fc585ef469";
#endif
}
#if BUILDFLAG(IS_APPLE)
- return "55dab4f158fdc284e439b88c4306373c";
+ return "e6015f42eb81ed6003224cb2f27dcb51";
#else
- return "cc08493b1f079803930388ecc703be9d";
+ return "2e567a33390cd2ebad9dc33d82a8b054";
#endif
}();
@@ -1403,17 +1403,17 @@
const char* md5_modified_image = []() {
if (CFX_DefaultRenderDevice::UseSkiaRenderer()) {
#if BUILDFLAG(IS_WIN)
- return "1c9c1d10d541a40bf65ea4df514d21aa";
+ return "aeb7cfb0bf6ac2bcdef4412276f3bad4";
#elif BUILDFLAG(IS_APPLE)
- return "78b93a22fa6c5e43dcd857e761146078";
+ return "5beae8949ee6b5c99fe17475e90aea02";
#else
- return "355af7d15348cef766fa5bc6bd1f108b";
+ return "cfa8aa132250a1c0fec505bd13c15916";
#endif
}
#if BUILDFLAG(IS_APPLE)
- return "ce68959f74242d588af7fb82be5ba0ab";
+ return "25bf5ec7c197cf9ff3d12b41fc336b25";
#else
- return "425646a517a23104b9ef22881a19b3e2";
+ return "dcb492d8e32528dd81bb60fa5bc900f8";
#endif
}();
@@ -1655,17 +1655,17 @@
const char* md5 = []() {
if (CFX_DefaultRenderDevice::UseSkiaRenderer()) {
#if BUILDFLAG(IS_WIN)
- return "3b64690a2b112f79513a2229c684f3c2";
+ return "25b2a610cab9d656b9ab5f72746c9e2e";
#elif BUILDFLAG(IS_APPLE)
- return "f54ac1bff1711de04498692beb5650dd";
+ return "0f6501f8e22441630bdd535363c93e59";
#else
- return "e9fcb7a0f6c16c7f6ecab4d2165808f3";
+ return "1814140b1a9a9776546af7894e21d17f";
#endif
}
#if BUILDFLAG(IS_APPLE)
- return "52e93c54796f7f7167edf64e81d12bd7";
+ return "0521eaa52fe2aa43aafd3e4495f63f0b";
#else
- return "5143f9a98beb7b00ff40b89110a1089f";
+ return "5f19ddad9d48f5b7b87ee7d92f577db6";
#endif
}();
diff --git a/fpdfsdk/fpdf_edit_embeddertest.cpp b/fpdfsdk/fpdf_edit_embeddertest.cpp
index b43e209..9025e6b 100644
--- a/fpdfsdk/fpdf_edit_embeddertest.cpp
+++ b/fpdfsdk/fpdf_edit_embeddertest.cpp
@@ -594,24 +594,33 @@
EXPECT_TRUE(FPDFPath_BezierTo(blue_path, 375, 330, 390, 360, 400, 400));
EXPECT_TRUE(FPDFPath_Close(blue_path));
FPDFPage_InsertObject(page, blue_path);
- const char* last_checksum = []() {
- if (CFX_DefaultRenderDevice::UseSkiaRenderer()) {
- return "ed14c60702b1489c597c7d46ece7f86d";
- }
- return "9823e1a21bd9b72b6a442ba4f12af946";
- }();
{
+ const char* blue_path_checksum = []() {
+ if (CFX_DefaultRenderDevice::UseSkiaRenderer()) {
+ return "ed14c60702b1489c597c7d46ece7f86d";
+ }
+ return "9823e1a21bd9b72b6a442ba4f12af946";
+ }();
ScopedFPDFBitmap page_bitmap = RenderPage(page);
- CompareBitmap(page_bitmap.get(), 612, 792, last_checksum);
+ CompareBitmap(page_bitmap.get(), 612, 792, blue_path_checksum);
}
- // Now save the result, closing the page and document
+ // Now save the result, closing the page and document.
EXPECT_TRUE(FPDFPage_GenerateContent(page));
EXPECT_TRUE(FPDF_SaveAsCopy(document(), this, 0));
FPDF_ClosePage(page);
- // Render the saved result
- VerifySavedDocument(612, 792, last_checksum);
+ // Render the saved result. The checksum will change due to floating point
+ // precision error.
+ {
+ const char* last_checksum = []() {
+ if (CFX_DefaultRenderDevice::UseSkiaRenderer()) {
+ return "423b20c18c177e78c93d8b67594e49f1";
+ }
+ return "111c38e9bf9e2ba0a57b875cca596fff";
+ }();
+ VerifySavedDocument(612, 792, last_checksum);
+ }
}
TEST_F(FPDFEditEmbedderTest, ClipPath) {
diff --git a/fpdfsdk/fpdf_save_embeddertest.cpp b/fpdfsdk/fpdf_save_embeddertest.cpp
index e404e79..e3d8947 100644
--- a/fpdfsdk/fpdf_save_embeddertest.cpp
+++ b/fpdfsdk/fpdf_save_embeddertest.cpp
@@ -152,7 +152,7 @@
EXPECT_THAT(GetString(), HasSubstr("36 0 obj"));
EXPECT_THAT(GetString(), Not(HasSubstr("37 0 obj")));
EXPECT_THAT(GetString(), Not(HasSubstr("38 0 obj")));
- EXPECT_EQ(7908u, GetString().size());
+ EXPECT_EQ(7996u, GetString().size());
// Make sure new document renders the same as the old one.
ASSERT_TRUE(OpenSavedDocument());
