Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(430)

Issue 1394503003: fix some pathops bugs found in 1M skps (Closed)

Created:
5 years, 2 months ago by caryclark
Modified:
5 years, 2 months ago
Reviewers:
CC:
reviews_skia.org
Base URL:
https://skia.googlesource.com/skia.git@master
Target Ref:
refs/heads/master
Project:
skia
Visibility:
Public.

Description

Enabling clip stack flattening exercises path ops. Iterating through the 903K skps that represent the imagable 1M top web pages triggers a number of bugs, some of which are addressed here. Some web pages trigger intersecting cubic representations of arc with their conic counterparts. This exposed a flaw in coincident detection that caused an infinite loop. The loop alternatively extended the coincident section and, determining the that the bounds of the curve pairs did not overlap, deleted the extension. Track the number of times the coincident detection is called, and if it exceeds an empirically found limit, assume that the curves are coincident and force it to be so. The loop count limit can be determined by enabling DEBUG_T_SECT_LOOP_COUNT and running all tests. The largest count is reported on completion. Another class of bugs was caused by concident detection duplicating nearly identical points that had been merged earlier. To track these bugs, the 'handle coincidence' code was duplicated as a const debug variety that reported if one of a dozen or so irregularities are present; then it is easier to see when a block of code that fixes one irregularity regresses another. Creating the debug const code version exposed some non-debug code that could be const, and some that was experimental and could be removed. Set DEBUG_COINCIDENCE to track coincidence health and handling. For running on Chrome, DEBUG_VERIFY checks the result of pathops against the same operation using SkRegion to verify that the results are nearly the same. When visualizing the pathops work using tools/pathops_visualizer.htm, set DEBUG_DUMP_ALIGNMENT to see the curves after they've been aligned for coincidence. Other bugs fixed include detecting when a section of a pair of curves have devolved into lines and are coincident. TBR=reed@google.com Committed: https://skia.googlesource.com/skia/+/26ad22ab61539e3d3b6bc5e0da8dcebbd52a53de

Patch Set 1 #

Patch Set 2 : wip; coin debug report #

Patch Set 3 : wip; add more coincident health validation / debugging #

Patch Set 4 : wip; validation works on op tests #

Patch Set 5 : wip; remove allowcoinnear #

Patch Set 6 : wip; skp mybuilder bug isolation #

Patch Set 7 : wip; coin skp fixed #

Patch Set 8 : wip; partial coin fix #

Patch Set 9 : wip; adding worst loop stats #

Patch Set 10 : wip; adding worst loop stats part 2 #

Patch Set 11 : wip; coincident force #

Patch Set 12 : fix various skp-found bugs #

Patch Set 13 : fix skia_test verbose finalize code #

Patch Set 14 : add new test file #

Patch Set 15 : cast array size to int #

Patch Set 16 : init to avoid warning #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2464 lines, -388 lines) Patch
M gyp/pathops_unittest.gypi View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M src/pathops/SkAddIntersections.cpp View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
M src/pathops/SkIntersections.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +15 lines, -0 lines 0 comments Download
M src/pathops/SkOpAngle.h View 1 2 2 chunks +1 line, -1 line 0 comments Download
M src/pathops/SkOpAngle.cpp View 1 2 1 chunk +0 lines, -47 lines 0 comments Download
M src/pathops/SkOpCoincidence.h View 1 2 3 4 5 6 6 chunks +25 lines, -6 lines 0 comments Download
M src/pathops/SkOpCoincidence.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 16 chunks +41 lines, -43 lines 0 comments Download
M src/pathops/SkOpContour.h View 1 2 3 chunks +11 lines, -4 lines 0 comments Download
M src/pathops/SkOpSegment.h View 1 2 3 4 chunks +24 lines, -5 lines 0 comments Download
M src/pathops/SkOpSegment.cpp View 1 2 3 4 5 6 6 chunks +21 lines, -26 lines 0 comments Download
M src/pathops/SkOpSpan.h View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +7 lines, -1 line 0 comments Download
M src/pathops/SkPathOpsCommon.cpp View 1 2 3 4 5 6 1 chunk +31 lines, -3 lines 0 comments Download
M src/pathops/SkPathOpsDebug.h View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +20 lines, -2 lines 0 comments Download
M src/pathops/SkPathOpsDebug.cpp View 1 2 3 4 5 6 7 8 9 10 14 chunks +1180 lines, -4 lines 0 comments Download
M src/pathops/SkPathOpsOp.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +146 lines, -1 line 0 comments Download
M src/pathops/SkPathOpsSimplify.cpp View 2 chunks +4 lines, -1 line 0 comments Download
M src/pathops/SkPathOpsTSect.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 10 chunks +93 lines, -5 lines 0 comments Download
M src/pathops/SkPathOpsTypes.h View 1 2 3 4 5 6 7 8 9 10 5 chunks +23 lines, -1 line 0 comments Download
M src/pathops/SkPathOpsTypes.cpp View 1 2 3 4 5 6 7 8 9 10 2 chunks +4 lines, -0 lines 0 comments Download
A tests/PathOpsCubicConicIntersectionTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +73 lines, -0 lines 0 comments Download
M tests/PathOpsCubicIntersectionTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +16 lines, -1 line 0 comments Download
M tests/PathOpsDebug.cpp View 1 2 3 4 5 6 7 8 9 10 7 chunks +74 lines, -21 lines 0 comments Download
M tests/PathOpsExtendedTest.cpp View 2 chunks +10 lines, -1 line 0 comments Download
M tests/PathOpsOpTest.cpp View 1 2 3 4 5 6 2 chunks +2 lines, -2 lines 0 comments Download
M tests/PathOpsSimplifyTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +31 lines, -0 lines 0 comments Download
M tests/PathOpsSkpTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +217 lines, -4 lines 0 comments Download
M tests/PathOpsTSectDebug.h View 1 2 3 4 5 6 7 2 chunks +20 lines, -0 lines 0 comments Download
M tests/PathOpsTestCommon.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M tests/skia_test.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +4 lines, -0 lines 0 comments Download
M tools/pathops_sorter.htm View 1 2 3 4 5 6 7 8 9 10 1 chunk +5 lines, -6 lines 0 comments Download
M tools/pathops_visualizer.htm View 1 2 3 4 5 6 7 8 9 10 11 15 chunks +361 lines, -202 lines 0 comments Download

Messages

Total messages: 16 (8 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1394503003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1394503003/260001
5 years, 2 months ago (2015-10-16 15:43:46 UTC) #2
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x86_64-Debug-Trybot/builds/3734) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on ...
5 years, 2 months ago (2015-10-16 15:44:44 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1394503003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1394503003/280001
5 years, 2 months ago (2015-10-16 15:49:27 UTC) #6
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86_64-Release-Trybot/builds/3733)
5 years, 2 months ago (2015-10-16 15:50:34 UTC) #8
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1394503003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1394503003/300001
5 years, 2 months ago (2015-10-16 15:53:38 UTC) #10
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 2 months ago (2015-10-16 16:02:35 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1394503003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1394503003/300001
5 years, 2 months ago (2015-10-16 16:02:53 UTC) #15
commit-bot: I haz the power
5 years, 2 months ago (2015-10-16 16:03:42 UTC) #16
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as
https://skia.googlesource.com/skia/+/26ad22ab61539e3d3b6bc5e0da8dcebbd52a53de

Powered by Google App Engine
This is Rietveld 408576698