Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2482193004:
Tighten SkAAClip Bounds by Path Bounds (Closed)
DescriptionTighten SkAAClip Bounds by Path Bounds
This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. (The quadratic edge drifts across the boundary after an update and sets a dX=0 line segment which triggers blitAntiRect.)
Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966.
Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing...
BUG=chromium:662780, chromium:662862
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004
Committed: https://skia.googlesource.com/skia/+/041da389dd923689041ba5e45cda34901ba4fe8a
Patch Set 1 #Patch Set 2 : Send in the true SkRegion #
Total comments: 1
Patch Set 3 : Rebase #Patch Set 4 : Fix 100 line limit #Messages
Total messages: 37 (23 generated)
Description was changed from ========== Tighten Analytic AA Bounds by Path Bounds BUG=chromium:662780 ========== to ========== Tighten Analytic AA Bounds by Path Bounds BUG=chromium:662780 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ==========
Description was changed from ========== Tighten Analytic AA Bounds by Path Bounds BUG=chromium:662780 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ========== to ========== Tighten Analytic AA Bounds by Path Bounds This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. As per Mike's request, I've sent in the complete path boundary ir and assert whenever we blit something. Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966. Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing... BUG=chromium:662780 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ==========
liyuqian@google.com changed reviewers: + caryclark@google.com, fmalita@chromium.org, reed@google.com
Description was changed from ========== Tighten Analytic AA Bounds by Path Bounds This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. As per Mike's request, I've sent in the complete path boundary ir and assert whenever we blit something. Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966. Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing... BUG=chromium:662780 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ========== to ========== Tighten Analytic AA Bounds by Path Bounds This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. (The quadratic edge drifts across the boundary after an update and sets a dX=0 line segment which triggers blitAntiRect.) As per Mike's request, I've sent in the complete path boundary ir and assert whenever we blit something. Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966. Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing... BUG=chromium:662780 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ==========
The CQ bit was checked by liyuqian@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Tighten Analytic AA Bounds by Path Bounds This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. (The quadratic edge drifts across the boundary after an update and sets a dX=0 line segment which triggers blitAntiRect.) As per Mike's request, I've sent in the complete path boundary ir and assert whenever we blit something. Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966. Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing... BUG=chromium:662780 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ========== to ========== Tighten Analytic AA Bounds by Path Bounds This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. (The quadratic edge drifts across the boundary after an update and sets a dX=0 line segment which triggers blitAntiRect.) As per Mike's request, I've sent in the complete path boundary ir and assert whenever we blit something. Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966. Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing... BUG=chromium:662780, chromium:662862 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've now made sure that SkAAClip will send in the real clip after intersecting the path bounds. (So we don't have to tighten the bounds later down in my analytic AA code.)
Description was changed from ========== Tighten Analytic AA Bounds by Path Bounds This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. (The quadratic edge drifts across the boundary after an update and sets a dX=0 line segment which triggers blitAntiRect.) As per Mike's request, I've sent in the complete path boundary ir and assert whenever we blit something. Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966. Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing... BUG=chromium:662780, chromium:662862 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ========== to ========== Tighten SkAAClip Bounds by Path Bounds This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. (The quadratic edge drifts across the boundary after an update and sets a dX=0 line segment which triggers blitAntiRect.) As per Mike's request, I've sent in the complete path boundary ir and assert whenever we blit something. Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966. Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing... BUG=chromium:662780, chromium:662862 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ==========
Description was changed from ========== Tighten SkAAClip Bounds by Path Bounds This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. (The quadratic edge drifts across the boundary after an update and sets a dX=0 line segment which triggers blitAntiRect.) As per Mike's request, I've sent in the complete path boundary ir and assert whenever we blit something. Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966. Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing... BUG=chromium:662780, chromium:662862 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ========== to ========== Tighten SkAAClip Bounds by Path Bounds This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. (The quadratic edge drifts across the boundary after an update and sets a dX=0 line segment which triggers blitAntiRect.) As per Mike's request, I've tightened the clip sent by SkAAClip and assert that we're within the tightened clip whenever we blit something. Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966. Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing... BUG=chromium:662780, chromium:662862 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ==========
The CQ bit was checked by liyuqian@google.com to run a CQ dry run
can we land the "snug-clip" change on its own, or does it need to be paired with changing AAA code? https://codereview.chromium.org/2482193004/diff/20001/src/core/SkAAClip.cpp File src/core/SkAAClip.cpp (right): https://codereview.chromium.org/2482193004/diff/20001/src/core/SkAAClip.cpp#n... src/core/SkAAClip.cpp:1406: SkRegion realClip(*clip); nit: real doesn't convey much. How about tightClip or snugClip, perhaps with a comment explaining the value.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/09 21:27:42, reed1 wrote: > can we land the "snug-clip" change on its own, or does it need to be paired with > changing AAA code? > > https://codereview.chromium.org/2482193004/diff/20001/src/core/SkAAClip.cpp > File src/core/SkAAClip.cpp (right): > > https://codereview.chromium.org/2482193004/diff/20001/src/core/SkAAClip.cpp#n... > src/core/SkAAClip.cpp:1406: SkRegion realClip(*clip); > nit: real doesn't convey much. How about tightClip or snugClip, perhaps with a > comment explaining the value. I think it can be an independent CL. I'm uploading one with the nit fix soon.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Tighten SkAAClip Bounds by Path Bounds This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. (The quadratic edge drifts across the boundary after an update and sets a dX=0 line segment which triggers blitAntiRect.) As per Mike's request, I've tightened the clip sent by SkAAClip and assert that we're within the tightened clip whenever we blit something. Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966. Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing... BUG=chromium:662780, chromium:662862 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ========== to ========== Tighten SkAAClip Bounds by Path Bounds This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. (The quadratic edge drifts across the boundary after an update and sets a dX=0 line segment which triggers blitAntiRect.) Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966. Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing... BUG=chromium:662780, chromium:662862 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ==========
I've rebased this CL after the landing of https://skia-review.googlesource.com/c/4636/
On 2016/11/10 15:53:05, liyuqian wrote: > I've rebased this CL after the landing of > https://skia-review.googlesource.com/c/4636/ Hi everyone, because of the landing of https://skia-review.googlesource.com/c/4636/ , this CL becomes much shorter and hopefully easier to understand. Could anyone give a quick look at it?
lgtm nit: could you adjust the asserts so they fit in 100 columns?
The CQ bit was checked by liyuqian@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
On 2016/11/11 16:19:46, caryclark wrote: > lgtm > > nit: could you adjust the asserts so they fit in 100 columns? done, thanks
The CQ bit was checked by liyuqian@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from caryclark@google.com Link to the patchset: https://codereview.chromium.org/2482193004/#ps60001 (title: "Fix 100 line limit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by liyuqian@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Tighten SkAAClip Bounds by Path Bounds This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. (The quadratic edge drifts across the boundary after an update and sets a dX=0 line segment which triggers blitAntiRect.) Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966. Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing... BUG=chromium:662780, chromium:662862 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 ========== to ========== Tighten SkAAClip Bounds by Path Bounds This bug chromium:662780 exists after our original fix (https://codereview.chromium.org/2477393002/) because this path (added in unit test) is calling blitAntiRect rather than blitAntiH when the path is drifted across the boundary. (The quadratic edge drifts across the boundary after an update and sets a dX=0 line segment which triggers blitAntiRect.) Note that I didn't assert for the dLeft = dRite = 0 case because the left/right there won't drift after the SkTMin/SkTMax in line 964/966. Theoretically we can revert the relaxation in https://codereview.chromium.org/2477393002/ (that's only a relaxation for analytic AA, not supersampled AA). However, consider that the initial landing of analytic AA is so painful, I decide to revert that relaxation only after our successful landing... BUG=chromium:662780, chromium:662862 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482193004 Committed: https://skia.googlesource.com/skia/+/041da389dd923689041ba5e45cda34901ba4fe8a ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/041da389dd923689041ba5e45cda34901ba4fe8a |
