Recognize more path points that form rectangles in CFX_PathData.
Right now, CFX_PathData only recognizes path points with 4 or 5 points
as rectangles. Change it to normalize path points with more points that
form rectangles. Then CFX_PathData can recognize more rectangles, and
AGG can draw them more efficiently. As a result, the slow rendering PDF
that triggered this investigation renders 77 times faster according to
safetynet_measure.py with callgrind.
With the faster rendering for rectangles, AGG draws a few PDFs slightly
differently. Update affected pixel test expectations and update DEPS to
adjust corpus tests expectations affected by this change.
Bug: pdfium:1683
Change-Id: I7042aa3a580bece650ef0b8fb67b091c5d220501
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/80914
Commit-Queue: Lei Zhang <thestig@chromium.org>
Reviewed-by: Hui Yingst <nigi@chromium.org>
diff --git a/DEPS b/DEPS
index 2e23885..0069542 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': '61bf033292709e29941375f45183db852e74977c',
+ 'pdfium_tests_revision': '422552c1cf9795131684aa34973a3873e94c4937',
# 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/fxge/cfx_pathdata.cpp b/core/fxge/cfx_pathdata.cpp
index ca3029f..db9b059 100644
--- a/core/fxge/cfx_pathdata.cpp
+++ b/core/fxge/cfx_pathdata.cpp
@@ -6,8 +6,11 @@
#include "core/fxge/cfx_pathdata.h"
+#include <algorithm>
+#include <iterator>
+
#include "core/fxcrt/fx_system.h"
-#include "third_party/base/check.h"
+#include "third_party/base/check_op.h"
#include "third_party/base/numerics/safe_math.h"
namespace {
@@ -35,12 +38,67 @@
return p1.x != p2.x && p1.y != p2.y;
}
+bool IsRectImpl(const std::vector<FX_PATHPOINT>& points) {
+ if (!IsRectPreTransform(points))
+ return false;
+
+ for (int i = 1; i < 4; i++) {
+ if (XYBothNotEqual(points[i].m_Point, points[i - 1].m_Point))
+ return false;
+ }
+
+ if (XYBothNotEqual(points[0].m_Point, points[3].m_Point))
+ return false;
+
+ return true;
+}
+
CFX_FloatRect CreateRectFromPoints(const CFX_PointF& p1, const CFX_PointF& p2) {
CFX_FloatRect rect(p1.x, p1.y, p2.x, p2.y);
rect.Normalize();
return rect;
}
+bool PathPointsNeedNormalization(const std::vector<FX_PATHPOINT>& points) {
+ return points.size() > 5;
+}
+
+std::vector<FX_PATHPOINT> GetNormalizedPoints(
+ const std::vector<FX_PATHPOINT>& points) {
+ DCHECK(PathPointsNeedNormalization(points));
+
+ if (points[0].m_Point != points.back().m_Point)
+ return {};
+
+ std::vector<FX_PATHPOINT> normalized;
+ normalized.reserve(6);
+ normalized.push_back(points[0]);
+ for (auto it = points.begin() + 1; it != points.end(); ++it) {
+ // Exactly 5 points left. Stop normalizing and take what is left.
+ if (normalized.size() + std::distance(it, points.end()) == 5) {
+ std::copy(it, points.end(), std::back_inserter(normalized));
+ break;
+ }
+
+ // If the line does not move, skip this point.
+ const auto& point = *it;
+ if (point.m_Type == FXPT_TYPE::LineTo && !point.m_CloseFigure &&
+ !normalized.back().m_CloseFigure &&
+ point.m_Point == normalized.back().m_Point) {
+ continue;
+ }
+
+ normalized.push_back(point);
+
+ // Too many points. Not considered as a rectangle.
+ if (normalized.size() > 5)
+ return {};
+ }
+
+ DCHECK_EQ(5u, normalized.size());
+ return normalized;
+}
+
void UpdateLineEndPoints(CFX_FloatRect* rect,
const CFX_PointF& start_pos,
const CFX_PointF& end_pos,
@@ -332,34 +390,32 @@
}
bool CFX_PathData::IsRect() const {
- if (!IsRectPreTransform(m_Points))
- return false;
-
- for (int i = 1; i < 4; i++) {
- if (XYBothNotEqual(m_Points[i].m_Point, m_Points[i - 1].m_Point))
- return false;
- }
-
- if (XYBothNotEqual(m_Points[0].m_Point, m_Points[3].m_Point))
- return false;
-
- return true;
+ if (PathPointsNeedNormalization(m_Points))
+ return IsRectImpl(GetNormalizedPoints(m_Points));
+ return IsRectImpl(m_Points);
}
Optional<CFX_FloatRect> CFX_PathData::GetRect(const CFX_Matrix* matrix) const {
+ bool do_normalize = PathPointsNeedNormalization(m_Points);
+ std::vector<FX_PATHPOINT> normalized;
+ if (do_normalize)
+ normalized = GetNormalizedPoints(m_Points);
+ const std::vector<FX_PATHPOINT>& path_points =
+ do_normalize ? normalized : m_Points;
+
if (!matrix) {
- if (!IsRect())
+ if (!IsRectImpl(path_points))
return pdfium::nullopt;
- return CreateRectFromPoints(m_Points[0].m_Point, m_Points[2].m_Point);
+ return CreateRectFromPoints(path_points[0].m_Point, path_points[2].m_Point);
}
- if (!IsRectPreTransform(m_Points))
+ if (!IsRectPreTransform(path_points))
return pdfium::nullopt;
CFX_PointF points[5];
- for (size_t i = 0; i < m_Points.size(); ++i) {
- points[i] = matrix->Transform(m_Points[i].m_Point);
+ for (size_t i = 0; i < path_points.size(); ++i) {
+ points[i] = matrix->Transform(path_points[i].m_Point);
if (i == 0)
continue;
diff --git a/core/fxge/cfx_pathdata_unittest.cpp b/core/fxge/cfx_pathdata_unittest.cpp
index cecf347..419e542 100644
--- a/core/fxge/cfx_pathdata_unittest.cpp
+++ b/core/fxge/cfx_pathdata_unittest.cpp
@@ -208,10 +208,10 @@
path.AppendPoint({2, 1}, FXPT_TYPE::LineTo);
path.AppendPoint({0, 1}, FXPT_TYPE::LineTo);
path.AppendPoint({0, 0}, FXPT_TYPE::LineTo);
- // TODO(crbug.com/pdfium/1683): Treat this as a rect.
- EXPECT_FALSE(path.IsRect());
+ EXPECT_TRUE(path.IsRect());
Optional<CFX_FloatRect> rect = path.GetRect(nullptr);
- EXPECT_FALSE(rect.has_value());
+ ASSERT_TRUE(rect.has_value());
+ EXPECT_EQ(rect.value(), CFX_FloatRect(0, 0, 2, 1));
path.Clear();
path.AppendPoint({0, 0}, FXPT_TYPE::MoveTo);
@@ -227,10 +227,10 @@
path.AppendPoint({0, 0}, FXPT_TYPE::LineTo);
path.AppendPoint({0, 0}, FXPT_TYPE::LineTo);
path.AppendPoint({0, 0}, FXPT_TYPE::LineTo);
- // TODO(crbug.com/pdfium/1683): Treat this as a rect.
- EXPECT_FALSE(path.IsRect());
+ EXPECT_TRUE(path.IsRect());
rect = path.GetRect(nullptr);
- EXPECT_FALSE(rect.has_value());
+ ASSERT_TRUE(rect.has_value());
+ EXPECT_EQ(rect.value(), CFX_FloatRect(0, 0, 2, 1));
}
TEST(CFX_PathData, NotRect) {
diff --git a/testing/resources/pixel/xfa_specific/dynamic_password_field_background_fill_expected.pdf.0.png b/testing/resources/pixel/xfa_specific/dynamic_password_field_background_fill_expected.pdf.0.png
index d20d7ea..c1cebee 100644
--- a/testing/resources/pixel/xfa_specific/dynamic_password_field_background_fill_expected.pdf.0.png
+++ b/testing/resources/pixel/xfa_specific/dynamic_password_field_background_fill_expected.pdf.0.png
Binary files differ
diff --git a/testing/resources/pixel/xfa_specific/dynamic_password_field_background_fill_expected_win.pdf.0.png b/testing/resources/pixel/xfa_specific/dynamic_password_field_background_fill_expected_win.pdf.0.png
index 734b7e4..2cf7145 100644
--- a/testing/resources/pixel/xfa_specific/dynamic_password_field_background_fill_expected_win.pdf.0.png
+++ b/testing/resources/pixel/xfa_specific/dynamic_password_field_background_fill_expected_win.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 a3d9830..72c2e2a 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
