CStretchEngine: ensure sum of pixel weights is kFixedPointOne.
When calculating pixel weights using floating point arithmetic and
rounding, there is no guarantee that the sum of the rounding errors
will be zero. Instead, accumulate the error and distribute as we
compute weights. Finally, include those few fractional bits into
the last weight.
Bug: pdfium:1688
Change-Id: I7585731ad6aca1557b46e3a26eb3ddcb0aab59ca
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/82012
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
diff --git a/DEPS b/DEPS
index 1bf9a63..ad72b2f 100644
--- a/DEPS
+++ b/DEPS
@@ -111,7 +111,7 @@
# Three lines of non-changing comments so that
# the commit queue can handle CLs rolling pdfium_tests
# and whatever else without interference from each other.
- 'pdfium_tests_revision': '422552c1cf9795131684aa34973a3873e94c4937',
+ 'pdfium_tests_revision': 'b67d79fef155c0c44be240a4fde07f15adac5a4f',
# Three lines of non-changing comments so that
# the commit queue can handle CLs rolling skia
# and whatever else without interference from each other.
diff --git a/core/fxcodec/jbig2/jbig2_embeddertest.cpp b/core/fxcodec/jbig2/jbig2_embeddertest.cpp
index c897a28..8e52dbf 100644
--- a/core/fxcodec/jbig2/jbig2_embeddertest.cpp
+++ b/core/fxcodec/jbig2/jbig2_embeddertest.cpp
@@ -21,6 +21,6 @@
FPDF_PAGE page = LoadPage(0);
ASSERT_TRUE(page);
ScopedFPDFBitmap bitmap = RenderLoadedPage(page);
- CompareBitmap(bitmap.get(), 691, 432, "24d75af646f8772c5ee7ced260452ae4");
+ CompareBitmap(bitmap.get(), 691, 432, "726c2b8c89df0ab40627322d1dddd521");
UnloadPage(page);
}
diff --git a/core/fxge/dib/cstretchengine.cpp b/core/fxge/dib/cstretchengine.cpp
index a0674cc..15ab57a 100644
--- a/core/fxge/dib/cstretchengine.cpp
+++ b/core/fxge/dib/cstretchengine.cpp
@@ -75,9 +75,7 @@
m_DestMin = dest_min;
- // TODO(tsepez): test results are sensitive to `scale` being a double
- // rather than a float with an initial value no more precise than float.
- const double scale = static_cast<float>(src_len) / dest_len;
+ const double scale = static_cast<double>(src_len) / dest_len;
const double base = dest_len < 0 ? src_len : 0;
const size_t weight_count = static_cast<size_t>(ceil(fabs(scale))) + 1;
m_ItemSizeBytes = PixelWeight::TotalBytesForWeightCount(weight_count);
@@ -89,15 +87,13 @@
m_WeightTablesSizeBytes = dest_range * m_ItemSizeBytes;
m_WeightTables.resize(m_WeightTablesSizeBytes);
- if (options.bNoSmoothing || fabs(static_cast<float>(scale)) < 1.0f) {
+ if (options.bNoSmoothing || fabs(scale) < 1.0f) {
for (int dest_pixel = dest_min; dest_pixel < dest_max; ++dest_pixel) {
PixelWeight& pixel_weights = *GetPixelWeight(dest_pixel);
double src_pos = dest_pixel * scale + scale / 2 + base;
if (bilinear) {
- int src_start =
- static_cast<int>(floor(static_cast<float>(src_pos) - 1.0f / 2));
- int src_end =
- static_cast<int>(floor(static_cast<float>(src_pos) + 1.0f / 2));
+ int src_start = static_cast<int>(floor(src_pos - 0.5));
+ int src_end = static_cast<int>(floor(src_pos + 0.5));
src_start = std::max(src_start, src_min);
src_end = std::min(src_end, src_max - 1);
pixel_weights.SetStartEnd(src_start, src_end, weight_count);
@@ -105,13 +101,13 @@
// Always room for one weight per size calculation.
pixel_weights.m_Weights[0] = kFixedPointOne;
} else {
- pixel_weights.m_Weights[1] = FixedFromFloat(
- static_cast<float>(src_pos - pixel_weights.m_SrcStart - 0.5f));
+ pixel_weights.m_Weights[1] =
+ FixedFromDouble(src_pos - pixel_weights.m_SrcStart - 0.5f);
pixel_weights.m_Weights[0] =
kFixedPointOne - pixel_weights.m_Weights[1];
}
} else {
- int pixel_pos = static_cast<int>(floor(static_cast<float>(src_pos)));
+ int pixel_pos = static_cast<int>(floor(src_pos));
int src_start = std::max(pixel_pos, src_min);
int src_end = std::min(pixel_pos, src_max - 1);
pixel_weights.SetStartEnd(src_start, src_end, weight_count);
@@ -130,7 +126,9 @@
start_i = std::max(start_i, src_min);
end_i = std::min(end_i, src_max - 1);
pixel_weights.SetStartEnd(start_i, end_i, weight_count);
- for (int j = start_i; j <= end_i; ++j) {
+ uint32_t remaining = kFixedPointOne;
+ double rounding_error = 0.0;
+ for (int j = start_i; j < end_i; ++j) {
double dest_start = (j - base) / scale;
double dest_end = (j + 1 - base) / scale;
if (dest_start > dest_end)
@@ -138,11 +136,18 @@
double area_start = std::max(dest_start, static_cast<double>(dest_pixel));
double area_end = std::min(dest_end, static_cast<double>(dest_pixel + 1));
double weight = std::max(0.0, area_end - area_start);
- if (weight == 0 && j == end_i) {
- pixel_weights.RemoveLastWeight();
- break;
- }
- pixel_weights.SetWeightForPosition(j, FixedFromFloat(weight));
+ uint32_t fixed_weight = FixedFromDouble(weight + rounding_error);
+ pixel_weights.SetWeightForPosition(j, fixed_weight);
+ remaining -= fixed_weight;
+ rounding_error =
+ weight - static_cast<double>(fixed_weight) / kFixedPointOne;
+ }
+ // Note: underflow is defined behaviour for unsigned types and will
+ // result in an out-of-range value.
+ if (remaining && remaining <= kFixedPointOne) {
+ pixel_weights.SetWeightForPosition(end_i, remaining);
+ } else {
+ pixel_weights.RemoveLastWeightAndAdjust(remaining);
}
}
return true;
diff --git a/core/fxge/dib/cstretchengine.h b/core/fxge/dib/cstretchengine.h
index eef7fd0..da5792e 100644
--- a/core/fxge/dib/cstretchengine.h
+++ b/core/fxge/dib/cstretchengine.h
@@ -26,6 +26,10 @@
static constexpr uint32_t kFixedPointBits = 16;
static constexpr uint32_t kFixedPointOne = 1 << kFixedPointBits;
+ static inline uint32_t FixedFromDouble(double d) {
+ return static_cast<uint32_t>(FXSYS_round(d * kFixedPointOne));
+ }
+
static inline uint32_t FixedFromFloat(float f) {
return static_cast<uint32_t>(FXSYS_roundf(f * kFixedPointOne));
}
@@ -55,9 +59,12 @@
m_Weights[position - m_SrcStart] = weight;
}
- void RemoveLastWeight() {
+ // NOTE: relies on defined behaviour for unsigned overflow to
+ // decrement the previous position, as needed.
+ void RemoveLastWeightAndAdjust(uint32_t weight_change) {
CHECK_GT(m_SrcEnd, m_SrcStart);
--m_SrcEnd;
+ m_Weights[m_SrcEnd - m_SrcStart] += weight_change;
}
int m_SrcStart;
diff --git a/core/fxge/dib/cstretchengine_unittest.cpp b/core/fxge/dib/cstretchengine_unittest.cpp
index 888a3d9..379d844 100644
--- a/core/fxge/dib/cstretchengine_unittest.cpp
+++ b/core/fxge/dib/cstretchengine_unittest.cpp
@@ -86,8 +86,7 @@
EXPECT_FALSE(engine.GetResampleOptionsForTest().bLossy);
}
-// See https://crbug.com/pdfium/1688
-TEST(CStretchEngine, DISABLED_WeightRounding) {
+TEST(CStretchEngine, WeightRounding) {
FXDIB_ResampleOptions options;
ExecuteStretchTests(options);
}
@@ -98,8 +97,7 @@
ExecuteStretchTests(options);
}
-// See https://crbug.com/pdfium/1688
-TEST(CStretchEngine, DISABLED_WeightRoundingBilinear) {
+TEST(CStretchEngine, WeightRoundingBilinear) {
FXDIB_ResampleOptions options;
options.bInterpolateBilinear = true;
ExecuteStretchTests(options);
diff --git a/fpdfsdk/fpdf_edit_embeddertest.cpp b/fpdfsdk/fpdf_edit_embeddertest.cpp
index 6077395..7ac8ed6 100644
--- a/fpdfsdk/fpdf_edit_embeddertest.cpp
+++ b/fpdfsdk/fpdf_edit_embeddertest.cpp
@@ -3671,7 +3671,7 @@
ScopedFPDFBitmap bitmap(
FPDFImageObj_GetRenderedBitmap(document(), page, obj));
EXPECT_EQ(FPDFBitmap_BGRA, FPDFBitmap_GetFormat(bitmap.get()));
- CompareBitmap(bitmap.get(), 53, 43, "90fa16c2fb2bf8ad3654c2258417664c");
+ CompareBitmap(bitmap.get(), 53, 43, "582ca300e003f512d7b552c7b5b45d2e");
}
// Check the matrix for |obj|.
@@ -3708,7 +3708,7 @@
ScopedFPDFBitmap bitmap(
FPDFImageObj_GetRenderedBitmap(document(), page, obj));
EXPECT_EQ(FPDFBitmap_BGRA, FPDFBitmap_GetFormat(bitmap.get()));
- CompareBitmap(bitmap.get(), 120, 43, "57ed8e15daa535490ff0c8b7640a36b4");
+ CompareBitmap(bitmap.get(), 120, 43, "0824c16dcf2dfcef44b45d88db1fddce");
}
UnloadPage(page);
diff --git a/testing/resources/pixel/bug_591137_expected.pdf.0.png b/testing/resources/pixel/bug_591137_expected.pdf.0.png
index 265e682..74c5c64 100644
--- a/testing/resources/pixel/bug_591137_expected.pdf.0.png
+++ b/testing/resources/pixel/bug_591137_expected.pdf.0.png
Binary files differ
diff --git a/testing/resources/pixel/type3_expected.pdf.0.png b/testing/resources/pixel/type3_expected.pdf.0.png
index 6837bb8..63d9e57 100644
--- a/testing/resources/pixel/type3_expected.pdf.0.png
+++ b/testing/resources/pixel/type3_expected.pdf.0.png
Binary files differ
diff --git a/testing/resources/pixel/xfa_specific/resolve_nodes_0_expected.pdf.0.png b/testing/resources/pixel/xfa_specific/resolve_nodes_0_expected.pdf.0.png
index 72c2e2a..adfabbc 100644
--- a/testing/resources/pixel/xfa_specific/resolve_nodes_0_expected.pdf.0.png
+++ b/testing/resources/pixel/xfa_specific/resolve_nodes_0_expected.pdf.0.png
Binary files differ
