Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2023693002:
Handle stroked single line special case in Ganesh (Closed)
|
Created:
4 years, 6 months ago by robertphillips Modified:
4 years, 6 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionHandle stroked single line special case in Ganesh
This CL roughly halves the time spent on the captured stroked lines skp.
On my Linux desktop it boosts the external benchmark from 2618 to 5007.
This is a companion to: https://codereview.chromium.org/2019193002/ (Add new GM to exercise stroked line special case)
The idea is to land the GM first so any regressions are visible.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2023693002
Committed: https://skia.googlesource.com/skia/+/0851d2d04716ad4a7c7a646a5846a81db3d5b925
Patch Set 1 #Patch Set 2 : clean up #
Total comments: 10
Patch Set 3 : Address code review comments #Patch Set 4 : Fix bugs #
Messages
Total messages: 26 (14 generated)
Description was changed from ========== Handle stroked single line special case in Ganesh ========== to ========== Handle stroked single line special case in Ganesh GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2023693002 ==========
Description was changed from ========== Handle stroked single line special case in Ganesh GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2023693002 ========== to ========== Handle stroked single line special case in Ganesh This is a companion to: https://codereview.chromium.org/2019193002/ (Add new GM to exercise stroked line special case) GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2023693002 ==========
robertphillips@google.com changed reviewers: + bsalomon@google.com
Description was changed from ========== Handle stroked single line special case in Ganesh This is a companion to: https://codereview.chromium.org/2019193002/ (Add new GM to exercise stroked line special case) GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2023693002 ========== to ========== Handle stroked single line special case in Ganesh This is a companion to: https://codereview.chromium.org/2019193002/ (Add new GM to exercise stroked line special case) The idea is to land the GM first so any regressions are visible. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2023693002 ==========
Description was changed from ========== Handle stroked single line special case in Ganesh This is a companion to: https://codereview.chromium.org/2019193002/ (Add new GM to exercise stroked line special case) The idea is to land the GM first so any regressions are visible. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2023693002 ========== to ========== Handle stroked single line special case in Ganesh This CL roughly halves the time spent on the captured stroked lines skp. This is a companion to: https://codereview.chromium.org/2019193002/ (Add new GM to exercise stroked line special case) The idea is to land the GM first so any regressions are visible. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2023693002 ==========
Description was changed from ========== Handle stroked single line special case in Ganesh This CL roughly halves the time spent on the captured stroked lines skp. This is a companion to: https://codereview.chromium.org/2019193002/ (Add new GM to exercise stroked line special case) The idea is to land the GM first so any regressions are visible. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2023693002 ========== to ========== Handle stroked single line special case in Ganesh This CL roughly halves the time spent on the captured stroked lines skp. On my Linux desktop it boosts the external benchmark from 2618 to 5007. This is a companion to: https://codereview.chromium.org/2019193002/ (Add new GM to exercise stroked line special case) The idea is to land the GM first so any regressions are visible. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2023693002 ==========
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023693002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023693002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reed@google.com changed reviewers: + reed@google.com
https://codereview.chromium.org/2023693002/diff/20001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): https://codereview.chromium.org/2023693002/diff/20001/src/gpu/SkGpuDevice.cpp... src/gpu/SkGpuDevice.cpp:706: const SkScalar halfWidth = origPaint.getStrokeWidth()/2.0f; Does this version devolve into multiply-by-0.5 ? https://codereview.chromium.org/2023693002/diff/20001/src/gpu/SkGpuDevice.cpp... src/gpu/SkGpuDevice.cpp:707: SkASSERT(halfWidth > 0); Do you not try to trigger your hairline code for thin lines? https://codereview.chromium.org/2023693002/diff/20001/src/gpu/SkGpuDevice.cpp... src/gpu/SkGpuDevice.cpp:727: m.setSinCos(v.fX, -v.fY, mid.fX, mid.fY); Is this really faster than just drawing a quad? I have a working CL that also tries to catch stroked lines, but I was adding a helper to SkStroker that would return a quad (at least for non-round-caps). Was hoping we might share code, but not if that is a lot slower than your rotated rect trick. https://codereview.chromium.org/2023693002/diff/20001/src/gpu/SkGpuDevice.cpp... src/gpu/SkGpuDevice.cpp:759: draw.fMatrix->preservesRightAngles() && origSrcPath.isLine(points)) { May not come up much, but if we just create quads, we don't need to check the matrix for this attribute...
Might also plug into drawPoints, which also generates lines (depending on the enum).
https://codereview.chromium.org/2023693002/diff/20001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): https://codereview.chromium.org/2023693002/diff/20001/src/gpu/SkGpuDevice.cpp... src/gpu/SkGpuDevice.cpp:760: // TODO: to support round capped stroked lines we need RRect batches Re to TODO. Unless we really have to, I strongly prefer to not add more complexity here. Adding another drawing path now for this case is, to me, only justified by the big perf win. drawStrokedLine already destined to get rewritten/removed in the near future, I don't want to pile on more technical debt.
https://codereview.chromium.org/2023693002/diff/20001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): https://codereview.chromium.org/2023693002/diff/20001/src/gpu/SkGpuDevice.cpp... src/gpu/SkGpuDevice.cpp:706: const SkScalar halfWidth = origPaint.getStrokeWidth()/2.0f; On 2016/05/31 12:54:48, reed1 wrote: > Does this version devolve into multiply-by-0.5 ? Done. https://codereview.chromium.org/2023693002/diff/20001/src/gpu/SkGpuDevice.cpp... src/gpu/SkGpuDevice.cpp:707: SkASSERT(halfWidth > 0); On 2016/05/31 12:54:48, reed1 wrote: > Do you not try to trigger your hairline code for thin lines? To the best of my knowledge we only kick into hairline mode when strokeWidth is exactly 0. https://codereview.chromium.org/2023693002/diff/20001/src/gpu/SkGpuDevice.cpp... src/gpu/SkGpuDevice.cpp:727: m.setSinCos(v.fX, -v.fY, mid.fX, mid.fY); On 2016/05/31 12:54:48, reed1 wrote: > Is this really faster than just drawing a quad? I have a working CL that also > tries to catch stroked lines, but I was adding a helper to SkStroker that would > return a quad (at least for non-round-caps). Was hoping we might share code, but > not if that is a lot slower than your rotated rect trick. Doing it this way allows greater batching. A more extensive change could add a GrDrawContext::drawQuad entry point but we would have to ensure the axis-aligned rect draws would also batch with them. So ... for right now, yes, this is faster. https://codereview.chromium.org/2023693002/diff/20001/src/gpu/SkGpuDevice.cpp... src/gpu/SkGpuDevice.cpp:759: draw.fMatrix->preservesRightAngles() && origSrcPath.isLine(points)) { On 2016/05/31 12:54:48, reed1 wrote: > May not come up much, but if we just create quads, we don't need to check the > matrix for this attribute... Right. The skew and perspective cases don't seem to come up much in practice though. https://codereview.chromium.org/2023693002/diff/20001/src/gpu/SkGpuDevice.cpp... src/gpu/SkGpuDevice.cpp:760: // TODO: to support round capped stroked lines we need RRect batches On 2016/05/31 16:47:27, bsalomon wrote: > Re to TODO. Unless we really have to, I strongly prefer to not add more > complexity here. Adding another drawing path now for this case is, to me, only > justified by the big perf win. drawStrokedLine already destined to get > rewritten/removed in the near future, I don't want to pile on more technical > debt. Done. Switched the TODO over to an informational comment.
lgtm
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023693002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023693002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robertphillips@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/2023693002/#ps60001 (title: "Fix bugs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023693002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023693002/60001
Message was sent while issue was closed.
Description was changed from ========== Handle stroked single line special case in Ganesh This CL roughly halves the time spent on the captured stroked lines skp. On my Linux desktop it boosts the external benchmark from 2618 to 5007. This is a companion to: https://codereview.chromium.org/2019193002/ (Add new GM to exercise stroked line special case) The idea is to land the GM first so any regressions are visible. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2023693002 ========== to ========== Handle stroked single line special case in Ganesh This CL roughly halves the time spent on the captured stroked lines skp. On my Linux desktop it boosts the external benchmark from 2618 to 5007. This is a companion to: https://codereview.chromium.org/2019193002/ (Add new GM to exercise stroked line special case) The idea is to land the GM first so any regressions are visible. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2023693002 Committed: https://skia.googlesource.com/skia/+/0851d2d04716ad4a7c7a646a5846a81db3d5b925 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/0851d2d04716ad4a7c7a646a5846a81db3d5b925 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
