[Skia] Optimize DrawPath() when knockout is on
Instead of calculating geometric union and subtraction of paths which
may contain many points, composite the objects in pixel-space using
blend modes.
This updates the pdfium_tests hash in DEPS to
https://pdfium-review.googlesource.com/c/pdfium_tests/+/138210
Initially I kept the same structure of doing a union or difference based
on the stroke color and drawing the stroke on the separate layer from
the fill. However when I read ISO 32000-1:2008 I realized I could
simplify it and make the function more accurately follow the spec.
Rendering performance for the pdf in the bug using pdfium_test:
- Before: 68.72s user 0.37s system 99% cpu 1:09.14 total
- After: 0.52s user 0.12s system 99% cpu 0.645 total
Bug: 334774307, 460180742
Change-Id: I7cc2d264bb06182c69d74dc7fcd04fe285d47256
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/137630
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: April Kallmeyer <ask@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
diff --git a/DEPS b/DEPS
index e66cb17..a363f56 100644
--- a/DEPS
+++ b/DEPS
@@ -190,7 +190,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': '79a975f5b013ab1b3b0d1b887d9bdddcd50725e3',
+ 'pdfium_tests_revision': '47d8f8f7f29b30a7abb9b116c93d6034f80e255d',
# Three lines of non-changing comments so that
# the commit queue can handle CLs rolling result_adapter_revision
# and whatever else without interference from each other.
diff --git a/core/fxge/skia/fx_skia_device.cpp b/core/fxge/skia/fx_skia_device.cpp
index 7a5bae2..6529997 100644
--- a/core/fxge/skia/fx_skia_device.cpp
+++ b/core/fxge/skia/fx_skia_device.cpp
@@ -1122,50 +1122,78 @@
if (fill_options.full_cover) {
path_paint.setBlendMode(SkBlendMode::kPlus);
}
+
+ SkPaint stroke_paint = path_paint;
int stroke_alpha = FXARGB_A(stroke_color);
if (stroke_alpha) {
const CFX_GraphStateData stroke_options_copy =
stroke_options ? *stroke_options : CFX_GraphStateData();
- SetupStrokePaint(&path_paint, &stroke_options_copy, transform_matrix,
+ SetupStrokePaint(&stroke_paint, &stroke_options_copy, transform_matrix,
fill_options);
}
SkAutoCanvasRestore scoped_save_restore(canvas_, /*doSave=*/true);
canvas_->concat(transform_matrix);
- bool do_stroke = true;
- if (fill_options.fill_type != CFX_FillRenderOptions::FillType::kNoFill &&
+
+ // Draw the fill (and the stroke if it's a knockout group)
+ const bool is_point_path = IsPathAPoint(path);
+ if (!is_point_path &&
+ fill_options.fill_type != CFX_FillRenderOptions::FillType::kNoFill &&
fill_color) {
- SkPath path_outline;
- const SkPath* fill_path = &path;
- if (stroke_alpha) {
- if (group_knockout_) {
- skpathutils::FillPathWithPaint(path, path_paint, &path_outline);
- if (stroke_color == fill_color &&
- Op(path, path_outline, SkPathOp::kUnion_SkPathOp, &path_outline)) {
- fill_path = &path_outline;
- do_stroke = false;
- } else if (Op(path, path_outline, SkPathOp::kDifference_SkPathOp,
- &path_outline)) {
- fill_path = &path_outline;
- }
- }
+ // Knockout Group is a PDF feature that means the elements of the group
+ // should not affect each other with transparency.
+ //
+ // See section 11.4.6 of in ISO 32000-1:2008:
+ // "At any given point, only the topmost object enclosing the point shall
+ // contribute to the result colour and opacity of the group as a whole"
+ if (stroke_alpha && group_knockout_) {
+ // Draw the knockout group path on a separate layer so blend modes can be
+ // adjusted. When restore() is called path_paint is used to composite the
+ // layer.
+ canvas_->saveLayer(nullptr, &path_paint);
+ SkPaint layer_paint = stroke_paint;
+
+ // kSrc means the top-most object fully overwrites any pixels it draws.
+ // See https://skia.org/docs/user/api/skblendmode_overview/
+ layer_paint.setBlendMode(SkBlendMode::kSrc);
+ layer_paint.setStyle(SkPaint::kFill_Style);
+
+ layer_paint.setColor(fill_color);
+ DrawPathImpl(path, layer_paint);
+
+ // Compute the stroke outline and draw with fill style so that if the
+ // path self-intersects the anti-aliasing pixels won't stack their
+ // transparency.
+ //
+ // Drawing it as a stroke normally already operates in the knockout way
+ // but not for the AA pixels in some cases.
+ SkPath stroke_outline;
+ skpathutils::FillPathWithPaint(path, stroke_paint, &stroke_outline);
+ layer_paint.setColor(stroke_color);
+ DrawPathImpl(stroke_outline, layer_paint);
+
+ return true; // `scoped_save_restore` restores two layers here
}
- path_paint.setStyle(SkPaint::kFill_Style);
- path_paint.setColor(fill_color);
- DrawPathImpl(*fill_path, path_paint);
+
+ // Draw the fill normally if it isn't a knockout group
+ stroke_paint.setStyle(SkPaint::kFill_Style);
+ stroke_paint.setColor(fill_color);
+ DrawPathImpl(path, stroke_paint);
}
- if (stroke_alpha && do_stroke) {
- path_paint.setStyle(SkPaint::kStroke_Style);
- path_paint.setColor(stroke_color);
- if (!path.isLastContourClosed() && IsPathAPoint(path)) {
- DCHECK_GE(path.countPoints(), 1);
- canvas_->drawPoint(path.getPoint(0), path_paint);
- } else if (IsPathAPoint(path) &&
- path_paint.getStrokeCap() != SkPaint::kRound_Cap) {
+
+ // Draw the stroke
+ if (stroke_alpha) {
+ stroke_paint.setStyle(SkPaint::kStroke_Style);
+ stroke_paint.setColor(stroke_color);
+ if (!path.isLastContourClosed() && is_point_path) {
+ CHECK_GE(path.countPoints(), 1);
+ canvas_->drawPoint(path.getPoint(0), stroke_paint);
+ } else if (is_point_path &&
+ stroke_paint.getStrokeCap() != SkPaint::kRound_Cap) {
// Do nothing. A closed 0-length closed path can be rendered only if
// its line cap type is round.
} else {
- DrawPathImpl(path, path_paint);
+ DrawPathImpl(path, stroke_paint);
}
}
return true;
diff --git a/fpdfsdk/fpdf_edit_embeddertest.cpp b/fpdfsdk/fpdf_edit_embeddertest.cpp
index 57e9d78..b78d8bd 100644
--- a/fpdfsdk/fpdf_edit_embeddertest.cpp
+++ b/fpdfsdk/fpdf_edit_embeddertest.cpp
@@ -2697,7 +2697,11 @@
ScopedFPDFBitmap page_bitmap = RenderPage(page);
const char* checksum_3 = []() {
if (CFX_DefaultRenderDevice::UseSkiaRenderer()) {
- return "a5de6ddefcbae60924bebc99347e460b";
+#if BUILDFLAG(IS_APPLE) && defined(ARCH_CPU_ARM64)
+ return "a4b4b739462a471cad72656e7e8c2af8";
+#else
+ return "037f5b38d8b612abf0833eb9ac8adf69";
+#endif
}
return "ff3e6a22326754944cc6e56609acd73b";
}();
@@ -3943,7 +3947,11 @@
TEST_F(FPDFEditEmbedderTest, SaveAndRender) {
const char* checksum = []() {
if (CFX_DefaultRenderDevice::UseSkiaRenderer()) {
- return "edd4aed776c0eaf8c79dd24d9654af95";
+#if BUILDFLAG(IS_APPLE) && defined(ARCH_CPU_ARM64)
+ return "0b8b84f8da13bf8a5bbaff3087685bed";
+#else
+ return "a6e8827e9fda09151765130e2f5531eb";
+#endif
}
return "3c20472b0552c0c22b88ab1ed8c6202b";
}();
diff --git a/testing/SUPPRESSIONS b/testing/SUPPRESSIONS
index 2abdb60..8c8b44b 100644
--- a/testing/SUPPRESSIONS
+++ b/testing/SUPPRESSIONS
@@ -348,6 +348,7 @@
example_042.pdf mac_x86 * * skia
font_1_embedded_font_en_feature.pdf mac_x86 * * skia
font_2_embedded_font_en_size14.pdf mac_x86 * * skia
+transparent1.pdf mac_x86 * * skia
xfermodes.pdf mac_x86 * * skia
xfermodes2.pdf mac_x86 * * skia
xfermodes3.pdf mac_x86 * * skia
@@ -817,4 +818,5 @@
# rendering that fuzzy matching is not enough for it to match. The difference
# looks like it may be due to several rounding errors combining for a bigger
# than normal pixel value difference.
+bug_1430333.in mac_x86 * * skia
bug_1746.in mac_x86 * * skia
diff --git a/testing/SUPPRESSIONS_EXACT_MATCHING b/testing/SUPPRESSIONS_EXACT_MATCHING
index db0a0fc..023eb63 100644
--- a/testing/SUPPRESSIONS_EXACT_MATCHING
+++ b/testing/SUPPRESSIONS_EXACT_MATCHING
@@ -167,6 +167,7 @@
new_textmarkup2.pdf mac * * skia
new_textmarkup8.pdf mac * * skia
path_5_pattern.pdf mac * * skia
+path_9.pdf mac_x86 * * skia
quick_start_guide.pdf mac * * skia
shading_2_15.pdf mac * * skia
shading_2_17.pdf mac * * skia
diff --git a/testing/resources/pixel/bug_1430333_expected_skia.pdf.0.png b/testing/resources/pixel/bug_1430333_expected_skia.pdf.0.png
index e6a1827..509eff5 100644
--- a/testing/resources/pixel/bug_1430333_expected_skia.pdf.0.png
+++ b/testing/resources/pixel/bug_1430333_expected_skia.pdf.0.png
Binary files differ
diff --git a/testing/resources/pixel/bug_1430333_expected_skia_mac.pdf.0.png b/testing/resources/pixel/bug_1430333_expected_skia_mac.pdf.0.png
new file mode 100644
index 0000000..2e892d6
--- /dev/null
+++ b/testing/resources/pixel/bug_1430333_expected_skia_mac.pdf.0.png
Binary files differ
diff --git a/testing/resources/pixel/same_color_knockout_fill_expected_skia.pdf.0.png b/testing/resources/pixel/same_color_knockout_fill_expected_skia.pdf.0.png
deleted file mode 100644
index c0ecfbd..0000000
--- a/testing/resources/pixel/same_color_knockout_fill_expected_skia.pdf.0.png
+++ /dev/null
Binary files differ
diff --git a/testing/resources/pixel/single_point_paths_expected_skia.pdf.0.png b/testing/resources/pixel/single_point_paths_expected_skia.pdf.0.png
index 9034434..12b05e6 100644
--- a/testing/resources/pixel/single_point_paths_expected_skia.pdf.0.png
+++ b/testing/resources/pixel/single_point_paths_expected_skia.pdf.0.png
Binary files differ
