[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
