Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1275553002:
Implement caching of stroked paths in the tessellating path renderer. (Closed)
|
Created:
5 years, 4 months ago by Stephen White Modified:
5 years, 4 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionImplement caching of stroked paths in the tessellating path renderer.
This requires adding the stroke info to the cache key, and doing the
stroking and dashing before rendering as triangles.
BUG=skia:3755
Committed: https://skia.googlesource.com/skia/+/29e0d3f267a03546f236023347cdb4595ece2fd1
Committed: https://skia.googlesource.com/skia/+/b4f9d0ec6cc95ce46f9351fee5adaffcfa729e38
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add a GrStrokeInfo randomizer, and call it from the TessellatingPathBatch test #Patch Set 3 : Add random dashing info to GrTestUtils::GetStrokeInfo #Patch Set 4 : Alphabetize #includes #Patch Set 5 : Fix random dash creation: always create an even number of dashes. #Patch Set 6 : Move key computation and stroking into onDrawPath(), to fix path bounds #Patch Set 7 : Remove now-used stroke info from TessellatingPathBatch #Patch Set 8 : Move key computation back to generateGeometry; pass original path & stroke to batch; use IsStrokeHa… #Patch Set 9 : 100-col fix #Patch Set 10 : Revert to PS5, and adjust the bounds for the stroke #Patch Set 11 : Use IsStrokeHairlineOrEquivalent() instead of stroke->isHairline() #
Total comments: 2
Messages
Total messages: 41 (16 generated)
senorblanco@chromium.org changed reviewers: + bsalomon@chromium.org
senorblanco@chromium.org changed reviewers: + bsalomon@google.com - bsalomon@chromium.org
Brian: PTAL. Thanks!
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275553002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275553002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1275553002/diff/1/src/gpu/GrTessellatingPathR... File src/gpu/GrTessellatingPathRenderer.cpp (right): https://codereview.chromium.org/1275553002/diff/1/src/gpu/GrTessellatingPathR... src/gpu/GrTessellatingPathRenderer.cpp:1644: SkPaint strokePaint; This doesn't look like it is actually initialized to set stroking params.
https://codereview.chromium.org/1275553002/diff/1/src/gpu/GrTessellatingPathR... File src/gpu/GrTessellatingPathRenderer.cpp (right): https://codereview.chromium.org/1275553002/diff/1/src/gpu/GrTessellatingPathR... src/gpu/GrTessellatingPathRenderer.cpp:1644: SkPaint strokePaint; On 2015/08/05 18:53:51, bsalomon wrote: > This doesn't look like it is actually initialized to set stroking params. True.. I was looking for something that would generate me a random SkPaint or random GrStrokeInfo, but didn't find one. Did I miss it, or should I write one?
On 2015/08/05 18:57:39, Stephen White wrote: > https://codereview.chromium.org/1275553002/diff/1/src/gpu/GrTessellatingPathR... > File src/gpu/GrTessellatingPathRenderer.cpp (right): > > https://codereview.chromium.org/1275553002/diff/1/src/gpu/GrTessellatingPathR... > src/gpu/GrTessellatingPathRenderer.cpp:1644: SkPaint strokePaint; > On 2015/08/05 18:53:51, bsalomon wrote: > > This doesn't look like it is actually initialized to set stroking params. > > True.. I was looking for something that would generate me a random SkPaint or > random GrStrokeInfo, but didn't find one. Did I miss it, or should I write one? AFAIK there isn't one but it'd be nice to have it somewhere accessible because we'd probably use it in more places.
On 2015/08/05 19:00:30, bsalomon wrote: > On 2015/08/05 18:57:39, Stephen White wrote: > > > https://codereview.chromium.org/1275553002/diff/1/src/gpu/GrTessellatingPathR... > > File src/gpu/GrTessellatingPathRenderer.cpp (right): > > > > > https://codereview.chromium.org/1275553002/diff/1/src/gpu/GrTessellatingPathR... > > src/gpu/GrTessellatingPathRenderer.cpp:1644: SkPaint strokePaint; > > On 2015/08/05 18:53:51, bsalomon wrote: > > > This doesn't look like it is actually initialized to set stroking params. > > > > True.. I was looking for something that would generate me a random SkPaint or > > random GrStrokeInfo, but didn't find one. Did I miss it, or should I write > one? > > AFAIK there isn't one but it'd be nice to have it somewhere accessible because > we'd probably use it in more places. OK, added a new helper as GrTestUtils::TestStrokeInfo(). PTAL.
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275553002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275553002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by senorblanco@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275553002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275553002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/29e0d3f267a03546f236023347cdb4595ece2fd1
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1276633002/ by senorblanco@chromium.org. The reason for reverting is: Breaking/asserting in Debug on DM..
Message was sent while issue was closed.
PS5 contains the fix for GrTestUtils::TestStrokeInfo(): always create an even number of dashes.
The CQ bit was checked by senorblanco@chromium.org
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/1275553002/#ps80001 (title: "Fix random dash creation: always create an even number of dashes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275553002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275553002/80001
The CQ bit was unchecked by senorblanco@chromium.org
The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275553002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275553002/180001
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 senorblanco@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275553002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275553002/200001
Brian: PTAL. Thanks! Note: this patch took a lot of detours, but you can diff against PS4, which is the one which was previously LGTM'ed, and much closer than the intervening versions. The newest patch fixes these issues: 1) Stroked paths weren't clipped, due to incorrect bounds in the TessellatingPathBatch. 2) Near-hairlines were not considered hairlines (and were rendered with the tessellator).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/08/06 15:54:56, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. lgtm
The CQ bit was checked by senorblanco@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1275553002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1275553002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://skia.googlesource.com/skia/+/b4f9d0ec6cc95ce46f9351fee5adaffcfa729e38
Message was sent while issue was closed.
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1275553002/diff/200001/src/gpu/GrTestUtils.cpp File src/gpu/GrTestUtils.cpp (right): https://codereview.chromium.org/1275553002/diff/200001/src/gpu/GrTestUtils.cp... src/gpu/GrTestUtils.cpp:247: dashInfo.fIntervals = SkNEW_ARRAY(SkScalar, dashInfo.fCount); This you? https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-Golo-GPU-GT...
Message was sent while issue was closed.
https://codereview.chromium.org/1275553002/diff/200001/src/gpu/GrTestUtils.cpp File src/gpu/GrTestUtils.cpp (right): https://codereview.chromium.org/1275553002/diff/200001/src/gpu/GrTestUtils.cp... src/gpu/GrTestUtils.cpp:247: dashInfo.fIntervals = SkNEW_ARRAY(SkScalar, dashInfo.fCount); On 2015/08/06 18:42:03, mtklein_C wrote: > This you? > > https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-Golo-GPU-GT... > https://codereview.chromium.org/1274963003/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
