Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21161003:
Use Path Ops to generate PDF clips (Closed)
|
Created:
7 years, 4 months ago by ducky Modified:
7 years, 4 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionUse Path Ops to generate PDF clips
Committed: https://code.google.com/p/skia/source/detail?r=10633
Patch Set 1 #Patch Set 2 : Simplify GM, add transforms support #Patch Set 3 : Make more things static, fix warnings when SK_PDF_USE_PATHOPS not enabled #
Total comments: 21
Patch Set 4 : Fix review comments #
Total comments: 6
Patch Set 5 : Use wide-open inverse clip #
Total comments: 4
Patch Set 6 : Improve fallback, simplify code #Patch Set 7 : Fix indents #
Total comments: 4
Patch Set 8 : Improve style #
Messages
Total messages: 14 (0 generated)
This CL changes the clips output for PDF to use Path Ops to generate a vector clip from the clip stack, when available. This is dependent on the Inverse Fills CL, and the code is currently hidden behind a #define guard. A GM is also added that tests the different clip modes. Notes: When enabled, this regresses the paths version of the complexclips2 GM. This seems to be an issue with Path Ops not returning the correct clip path. Other GMs modified by this code look and zoom much better.
https://codereview.chromium.org/21161003/diff/18001/gm/circularclips.cpp File gm/circularclips.cpp (right): https://codereview.chromium.org/21161003/diff/18001/gm/circularclips.cpp#newc... gm/circularclips.cpp:17: nit: {} https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:335: SkPath* outPath) { nit: align https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:391: transform.mapRect(&translatedRect, clipEntry->getRect()); this can fail! https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:394: clipEntry->getPath().transform(transform, &entryPath); this can fail?
https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:335: SkPath* outPath) { indent (in original too). https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:345: static SkPathOp region_op_to_pathops_op(SkRegion::Op op) { SkRegion::Op and SkPathOp have the same value for these. You might want to do compile asserts that this remains true and then just do a cast - it'll be faster. https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:368: // Because of additive operations (especially XOR), starting an empty I don't understand the first part. https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:374: // bounds. The buffer space ensures there is no ambiguity on edge cases. I don't think we should use this buffer space. If you need to, write a gm that clips along the edge to ensure that we get it right. https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:400: SkPathOp op = region_op_to_pathops_op(clipEntry->getOp()); Do you have to check if entryPath has an inverse fill type? You could have a rectangle and then a inverse path inside that... https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:416: if (!Op(clipPath, boundsPath, kIntersect_PathOp, &clipPath)) { Won't this last step take care of any inverse path fill of clipPath ? https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:456: bool needRegion = false; If we have path ops turned on, what do you think of always using it, even if needsRegion would be false.
Most review comments addressed. Still unsure about not leaving a buffer for the clips. https://codereview.chromium.org/21161003/diff/18001/gm/circularclips.cpp File gm/circularclips.cpp (right): https://codereview.chromium.org/21161003/diff/18001/gm/circularclips.cpp#newc... gm/circularclips.cpp:17: On 2013/07/31 18:27:37, edisonn wrote: > nit: {} Done. https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:335: SkPath* outPath) { On 2013/07/31 18:35:17, vandebo wrote: > indent (in original too). Done. https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:345: static SkPathOp region_op_to_pathops_op(SkRegion::Op op) { On 2013/07/31 18:35:17, vandebo wrote: > SkRegion::Op and SkPathOp have the same value for these. You might want to do > compile asserts that this remains true and then just do a cast - it'll be > faster. Done. https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:368: // Because of additive operations (especially XOR), starting an empty On 2013/07/31 18:35:17, vandebo wrote: > I don't understand the first part. Ok, I tried to clarify. https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:374: // bounds. The buffer space ensures there is no ambiguity on edge cases. On 2013/07/31 18:35:17, vandebo wrote: > I don't think we should use this buffer space. If you need to, write a gm that > clips along the edge to ensure that we get it right. I'm concerned about the numerical issues that were brought up in the Path Ops talk, where points might not be exact because of floating point limitations or other accuracy errors. It's probably a very edge case, and if this behavior is a problem, I'm not sure if I can write a GM to demonstrate it without a deeper understanding of how PathOps works. So this just plays it safe. Additionally, SkRegion is a bitmap, which is only an approximation of the clipping path. If there's any filtering going on (like if a path occupying an area of less than half a pixel doesn't make the pixel inside the path - I don't know if this is the case), that may cause the clip bounds to be a tad too small. https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:391: transform.mapRect(&translatedRect, clipEntry->getRect()); On 2013/07/31 18:27:37, edisonn wrote: > this can fail! Fixed, now the transform happens as a path, which hopefully should handle the cases that rect can't. https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:394: clipEntry->getPath().transform(transform, &entryPath); On 2013/07/31 18:27:37, edisonn wrote: > this can fail? Doesn't look like it. SkPath::transform returns void. https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:400: SkPathOp op = region_op_to_pathops_op(clipEntry->getOp()); On 2013/07/31 18:35:17, vandebo wrote: > Do you have to check if entryPath has an inverse fill type? > > You could have a rectangle and then a inverse path inside that... I think PathOps handles inverses. All the GMs (some including inverses) that were affected have turned out fine. https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:416: if (!Op(clipPath, boundsPath, kIntersect_PathOp, &clipPath)) { On 2013/07/31 18:35:17, vandebo wrote: > Won't this last step take care of any inverse path fill of clipPath ? Good point. https://codereview.chromium.org/21161003/diff/18001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:456: bool needRegion = false; On 2013/07/31 18:35:17, vandebo wrote: > If we have path ops turned on, what do you think of always using it, even if > needsRegion would be false. Sure - added. Though, this could potentially preclude optimizations where common parts of the clip stack don't need to be re-emitted for every clip operation. But I don't think that is happening right now, and this makes the code a lot cleaner.
https://codereview.chromium.org/21161003/diff/30001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/21161003/diff/30001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:372: workingBounds.outset(SK_Scalar1, SK_Scalar1); Do you need to constrain the path bounds at the start? Why not just apply the clip stack to a wide open clip, and then bound it by the page size at the end to handle an inverse fill? https://codereview.chromium.org/21161003/diff/30001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:381: clipPath.reset(); Should this also add workingBounds to clipPath ? https://codereview.chromium.org/21161003/diff/30001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:391: clipPath = entryPath; Should you intersect with workingBounds here?
Now with wide-open starting clips! https://codereview.chromium.org/21161003/diff/30001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/21161003/diff/30001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:372: workingBounds.outset(SK_Scalar1, SK_Scalar1); On 2013/08/01 14:56:14, vandebo wrote: > Do you need to constrain the path bounds at the start? Why not just apply the > clip stack to a wide open clip, and then bound it by the page size at the end to > handle an inverse fill? Good point, I didn't realize I could just set the inverse on an empty path and it turns into a wide open fill. https://codereview.chromium.org/21161003/diff/30001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:381: clipPath.reset(); On 2013/08/01 14:56:14, vandebo wrote: > Should this also add workingBounds to clipPath ? Yep. D'oh. Fixed, with new empty clip style. https://codereview.chromium.org/21161003/diff/30001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:391: clipPath = entryPath; On 2013/08/01 14:56:14, vandebo wrote: > Should you intersect with workingBounds here? No longer necessary with new style. But the idea behind the old style was to allow things to be messy / wrong outside workingBounds, then clip it with clipBounds at the end so the parts we are about are correct.
https://codereview.chromium.org/21161003/diff/38001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/21161003/diff/38001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:382: SkAssertResult(clipRegion.getBoundaryPath(&clipPath)); Here we could fall back to the old code. https://codereview.chromium.org/21161003/diff/38001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:388: // Intersect the results with the clip bounds from the region to handle Don't we only need to do this chunk of code if clipPath.isInverseFillType() ? And if it is, should we call calculate_inverse_path ?
Improved fallback and simplified code. https://codereview.chromium.org/21161003/diff/38001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/21161003/diff/38001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:382: SkAssertResult(clipRegion.getBoundaryPath(&clipPath)); On 2013/08/02 15:58:09, vandebo wrote: > Here we could fall back to the old code. Done. https://codereview.chromium.org/21161003/diff/38001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:388: // Intersect the results with the clip bounds from the region to handle On 2013/08/02 15:58:09, vandebo wrote: > Don't we only need to do this chunk of code if clipPath.isInverseFillType() ? > And if it is, should we call calculate_inverse_path ? Good point.
LGTM with nits. https://codereview.chromium.org/21161003/diff/44001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/21161003/diff/44001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:334: static bool calculate_inverse_path(const SkRect& bounds, const SkPath& invPath, nit: put this and handleInversePath() inside the SK_PDF_USE_PATHOPS https://codereview.chromium.org/21161003/diff/44001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:500: currentEntry()->fClipStack = clipStack; nit: this is done in all cases, so just pull it up to line 440.
https://codereview.chromium.org/21161003/diff/44001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/21161003/diff/44001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:334: static bool calculate_inverse_path(const SkRect& bounds, const SkPath& invPath, On 2013/08/05 16:38:08, vandebo wrote: > nit: put this and handleInversePath() inside the SK_PDF_USE_PATHOPS Done. https://codereview.chromium.org/21161003/diff/44001/src/pdf/SkPDFDevice.cpp#n... src/pdf/SkPDFDevice.cpp:500: currentEntry()->fClipStack = clipStack; On 2013/08/05 16:38:08, vandebo wrote: > nit: this is done in all cases, so just pull it up to line 440. Done.
LGTM
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/richardlin@chromium.org/21161003/58001
Failed to apply patch for gm/circularclips.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
The next patch would create the file gm/circularclips.cpp,
which already exists! Skipping patch.
1 out of 1 hunk ignored
Patch: N gm/circularclips.cpp
Index: gm/circularclips.cpp
diff --git a/gm/circularclips.cpp b/gm/circularclips.cpp
new file mode 100644
index
0000000000000000000000000000000000000000..fdf2dfb4f3cf2e745e5e1aa6e0c8f0a722b408bb
--- /dev/null
+++ b/gm/circularclips.cpp
@@ -0,0 +1,78 @@
+/*
+ * Copyright 2013 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "gm.h"
+#include "SkCanvas.h"
+#include "SkPath.h"
+
+namespace skiagm {
+
+class CircularClipsGM : public GM {
+public:
+ CircularClipsGM() {}
+
+protected:
+ virtual SkString onShortName() {
+ return SkString("circular-clips");
+ }
+
+ virtual SkISize onISize() {
+ return SkISize::Make(800, 600);
+ }
+
+ virtual void onDraw(SkCanvas* canvas) {
+ SkRegion::Op ops[] = {
+ SkRegion::kDifference_Op,
+ SkRegion::kIntersect_Op,
+ SkRegion::kUnion_Op,
+ SkRegion::kXOR_Op,
+ SkRegion::kReverseDifference_Op,
+ SkRegion::kReplace_Op,
+ };
+
+ SkScalar x1 = 80, x2 = 120;
+ SkScalar y = 50;
+ SkScalar r = 40;
+
+ SkPath circle1, circle2;
+ circle1.addCircle(x1, y, r, SkPath::kCW_Direction);
+ circle2.addCircle(x2, y, r, SkPath::kCW_Direction);
+ SkRect rect = SkRect::MakeLTRB(x1 - r, y - r, x2 + r, y + r);
+
+ SkPaint fillPaint;
+
+ for (size_t i = 0; i < 4; i++) {
+ circle1.toggleInverseFillType();
+ if (i % 2 == 0) {
+ circle2.toggleInverseFillType();
+ }
+
+ canvas->save();
+ for (size_t op = 0; op < SK_ARRAY_COUNT(ops); op++) {
+ canvas->save();
+
+ canvas->clipPath(circle1, SkRegion::kReplace_Op);
+ canvas->clipPath(circle2, ops[op]);
+
+ canvas->drawRect(rect, fillPaint);
+
+ canvas->restore();
+ canvas->translate(0, 2 * y);
+ }
+ canvas->restore();
+ canvas->translate(x1 + x2, 0);
+ }
+ }
+
+private:
+ typedef GM INHERITED;
+};
+
+//////////////////////////////////////////////////////////////////////////////
+
+DEF_GM( return new CircularClipsGM; )
+}
commit-bot looks like it's really confused. It also seems the CL went in as r10633, about an hour before commit-bot started testing. !?!?!?!? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
