Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23712005:
Add bevel-stroke support in GrAARectRenderer (Closed)
|
Created:
7 years, 3 months ago by yunchao Modified:
7 years, 1 month ago CC:
skia-review_googlegroups.com Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAdd bevel-stroke support in GrAARectRenderer
Committed: http://code.google.com/p/skia/source/detail?r=12082
Committed: http://code.google.com/p/skia/source/detail?r=12148
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 7
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 15
Patch Set 8 : remove changes for GM/rects.cpp, and revised the code according to Brian and Rob's suggestions #
Total comments: 1
Patch Set 9 : fix small bugs #Patch Set 10 : fix small bugs #
Total comments: 1
Messages
Total messages: 28 (0 generated)
This can benefit bevel-stroke rasterization through HW-acceleration. Take bench case draw_stroke_rect_bevel for example, the improvement in Linux on my desktop(Intel Quad core i7 2.93GHz, NV GT218) is about 300%, it is about 500% in Android on Nexus 4. This also benefit other bench cases who rastered bevel-stroke rectangle in GPU path. gm testing shows no error introduced.
On 2013/09/13 09:22:44, yunchao wrote: Any suggestion about this？
On 2013/09/13 09:22:44, yunchao wrote: Any suggestion about this？
Do we have GMs that handle interesting mitered rect cases? (e.g. small mitered rects, wide strokes that close the interior, mitered rects with an SkShader attached, etc) https://codereview.chromium.org/23712005/diff/16001/include/gpu/GrContext.h File include/gpu/GrContext.h (right): https://codereview.chromium.org/23712005/diff/16001/include/gpu/GrContext.h#n... include/gpu/GrContext.h:404: bool miterStroke = true); Could we modify this to take an SkStrokeRec rather than strokeWidth and miterStroke?
https://codereview.chromium.org/23712005/diff/16001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): https://codereview.chromium.org/23712005/diff/16001/src/gpu/SkGpuDevice.cpp#n... src/gpu/SkGpuDevice.cpp:642: bool usePath = doStroke && width > 0 && Can the logic about using a path b/c of stroking be pushed down into GrContext::drawRect? That is have GrContext::drawRect take an arbitrary SkStrokeRec and then internally fallback to path rendering or not.
https://codereview.chromium.org/23712005/diff/16001/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/23712005/diff/16001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:400: gpu->createIndexBuffer(miterStroke ? I don't think this is right. This index buffer is lazily created but then reused. I think we would need separate miter and bevel idx buffers.
https://codereview.chromium.org/23712005/diff/16001/include/gpu/GrContext.h File include/gpu/GrContext.h (right): https://codereview.chromium.org/23712005/diff/16001/include/gpu/GrContext.h#n... include/gpu/GrContext.h:404: bool miterStroke = true); On 2013/09/17 14:40:20, bsalomon wrote: > Could we modify this to take an SkStrokeRec rather than strokeWidth and > miterStroke? Other places also called GrContext::drawRect. If we use a SkStrokeRec to call GrContext::drawRect and changed its parameters, many other callers need to construct a SkStrokeRec instance to store the default value of strokeWidth, miterStroke. So It may be better to keep this: simply add miterStroke parameter and give it a default value. https://codereview.chromium.org/23712005/diff/16001/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/23712005/diff/16001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:400: gpu->createIndexBuffer(miterStroke ? On 2013/09/17 15:03:47, bsalomon wrote: > I don't think this is right. This index buffer is lazily created but then > reused. I think we would need separate miter and bevel idx buffers. Yeah, good suggestion. https://codereview.chromium.org/23712005/diff/16001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): https://codereview.chromium.org/23712005/diff/16001/src/gpu/SkGpuDevice.cpp#n... src/gpu/SkGpuDevice.cpp:642: bool usePath = doStroke && width > 0 && On 2013/09/17 14:43:49, bsalomon wrote: > Can the logic about using a path b/c of stroking be pushed down into > GrContext::drawRect? That is have GrContext::drawRect take an arbitrary > SkStrokeRec and then internally fallback to path rendering or not. GrContext::drawRect will not fallback to drawPath. The decision that whether It needs to call drawPath is made in SkGpuDevice. Hope that this answered your questions.
On 2013/09/22 08:36:37, yunchao wrote: > https://codereview.chromium.org/23712005/diff/16001/include/gpu/GrContext.h > File include/gpu/GrContext.h (right): > > https://codereview.chromium.org/23712005/diff/16001/include/gpu/GrContext.h#n... > include/gpu/GrContext.h:404: bool miterStroke = true); > On 2013/09/17 14:40:20, bsalomon wrote: > > Could we modify this to take an SkStrokeRec rather than strokeWidth and > > miterStroke? > > Other places also called GrContext::drawRect. If we use a SkStrokeRec to call > GrContext::drawRect and changed its parameters, many other callers need to > construct a SkStrokeRec instance to store the default value of strokeWidth, > miterStroke. So It may be better to keep this: simply add miterStroke parameter > and give it a default value. > > https://codereview.chromium.org/23712005/diff/16001/src/gpu/GrAARectRenderer.cpp > File src/gpu/GrAARectRenderer.cpp (right): > > https://codereview.chromium.org/23712005/diff/16001/src/gpu/GrAARectRenderer.... > src/gpu/GrAARectRenderer.cpp:400: gpu->createIndexBuffer(miterStroke ? > On 2013/09/17 15:03:47, bsalomon wrote: > > I don't think this is right. This index buffer is lazily created but then > > reused. I think we would need separate miter and bevel idx buffers. > Yeah, good suggestion. > > https://codereview.chromium.org/23712005/diff/16001/src/gpu/SkGpuDevice.cpp > File src/gpu/SkGpuDevice.cpp (right): > > https://codereview.chromium.org/23712005/diff/16001/src/gpu/SkGpuDevice.cpp#n... > src/gpu/SkGpuDevice.cpp:642: bool usePath = doStroke && width > 0 && > On 2013/09/17 14:43:49, bsalomon wrote: > > Can the logic about using a path b/c of stroking be pushed down into > > GrContext::drawRect? That is have GrContext::drawRect take an arbitrary > > SkStrokeRec and then internally fallback to path rendering or not. > > GrContext::drawRect will not fallback to drawPath. The decision that whether It > needs to call drawPath is made in SkGpuDevice. Hope that this answered your > questions. Hi, any suggestion about this patch?
I'd like to see a GM that exercises this code path in various ways: opaque, translucent, miter and bevel, thin in x and thin in y, stroke width wider than rect width and/or height, rotated, skewed, with a SkShader, ... See the GM called "rects" as a starting point. https://codereview.chromium.org/23712005/diff/16001/include/gpu/GrContext.h File include/gpu/GrContext.h (right): https://codereview.chromium.org/23712005/diff/16001/include/gpu/GrContext.h#n... include/gpu/GrContext.h:404: bool miterStroke = true); On 2013/09/22 08:36:37, yunchao wrote: > On 2013/09/17 14:40:20, bsalomon wrote: > > Could we modify this to take an SkStrokeRec rather than strokeWidth and > > miterStroke? > > Other places also called GrContext::drawRect. If we use a SkStrokeRec to call > GrContext::drawRect and changed its parameters, many other callers need to > construct a SkStrokeRec instance to store the default value of strokeWidth, > miterStroke. So It may be better to keep this: simply add miterStroke parameter > and give it a default value. What about void drawRect(const GrPaint&, const SkRect&, const SkMatrix* matrix = NULL, const SkStrokeRec* stroke = NULL); where NULL for stroke rec means fill?
On 2013/09/25 15:26:24, bsalomon wrote: > I'd like to see a GM that exercises this code path in various ways: > > opaque, translucent, miter and bevel, thin in x and thin in y, stroke width > wider than rect width and/or height, rotated, skewed, with a SkShader, ... > > See the GM called "rects" as a starting point. > > https://codereview.chromium.org/23712005/diff/16001/include/gpu/GrContext.h > File include/gpu/GrContext.h (right): > > https://codereview.chromium.org/23712005/diff/16001/include/gpu/GrContext.h#n... > include/gpu/GrContext.h:404: bool miterStroke = true); > On 2013/09/22 08:36:37, yunchao wrote: > > On 2013/09/17 14:40:20, bsalomon wrote: > > > Could we modify this to take an SkStrokeRec rather than strokeWidth and > > > miterStroke? > > > > Other places also called GrContext::drawRect. If we use a SkStrokeRec to call > > GrContext::drawRect and changed its parameters, many other callers need to > > construct a SkStrokeRec instance to store the default value of strokeWidth, > > miterStroke. So It may be better to keep this: simply add miterStroke > parameter > > and give it a default value. > > What about > > void drawRect(const GrPaint&, > const SkRect&, > const SkMatrix* matrix = NULL, > const SkStrokeRec* stroke = NULL); > > where NULL for stroke rec means fill? Hi, I updated the code according to Brian's suggestion, add code into gm/rects to exercises this CL too. please help to review it.
Hi, all, any suggestions about this?
On 2013/10/30 06:54:37, yunchao wrote: > Hi, all, any suggestions about this? Should I separate this CL into two: move the added code in gm/rects.cpp out of this CL, then It is easy to compare the difference with/without this CL?
On 2013/10/30 10:05:54, yunchao wrote: > On 2013/10/30 06:54:37, yunchao wrote: > > Hi, all, any suggestions about this? > > Should I separate this CL into two: move the added code in gm/rects.cpp out of > this CL, then It is easy to compare the difference with/without this CL? Actually that's a good idea. Can you submit just the GM change and then the substantive change? The code looks good to me but I'd like Rob to have a look, too. I have a few small nits I'll publish in a moment.
https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:391: GrIndexBuffer* GrAARectRenderer::aaStrokeRectIndexBuffer(GrGpu* gpu, this can stay as one line (we wrap at 100col) https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:700: //SkScalar width, Please remove rather than commenting out. https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:703: bool miterStroke*/) { and here
Overall lgtm. I would like a big comment somewhere talking about the geometry used for the new draw path. Please see inline for several nits and a question. https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:360: Could you add a comment here (and maybe some ASCII art) documenting the index layout? https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:401: fAAMiterStrokeRectIndexBuffer->updateData(gMiterStrokeAARectIdx, Please align the "sizeof" line with the above "(". https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:415: fAABevelStrokeRectIndexBuffer->updateData(gBevelStrokeAARectIdx, Here too please. https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:747: I think the entire computation of "miterStroke" can fix on one line. https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:755: if (!miterStroke) { inset(0, ry) rather than outset(0, -ry)? https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:765: const SkRect& devOutside, Is there a comment somewhere (maybe the header) as to what 'devOutsideAssist' is, what it is used for and when it is used? https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:773: Make these two with static const (and prefix with a "k") so that it is clear we aren't changing them? Each constant could also be moved into the scope (in the following "if" blocks) where they are only used. https://codereview.chromium.org/23712005/diff/39001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): https://codereview.chromium.org/23712005/diff/39001/src/gpu/SkGpuDevice.cpp#n... src/gpu/SkGpuDevice.cpp:659: How expensive is it to create the SkStrokeRect all the time? Also, don't think we need the final "NULL" on this call to drawRect.
Do you have any performance information?
I revised the CL according to your suggestions, thank you! https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.cpp File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:360: On 2013/10/30 14:36:08, robertphillips wrote: > Could you add a comment here (and maybe some ASCII art) documenting the index > layout? Agree with you. I added a comment about the indices layout and drew a figure too. It is much easier to understand now. https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:755: if (!miterStroke) { On 2013/10/30 14:36:08, robertphillips wrote: > inset(0, ry) rather than outset(0, -ry)? I checked my local code, it is a typo, it should be outset(0, rx). Thank you Rob. https://codereview.chromium.org/23712005/diff/39001/src/gpu/GrAARectRenderer.... src/gpu/GrAARectRenderer.cpp:773: On 2013/10/30 14:36:08, robertphillips wrote: > Make these two with static const (and prefix with a "k") so that it is clear we > aren't changing them? Each constant could also be moved into the scope (in the > following "if" blocks) where they are only used. Yeah, a good suggestion, I will keep this style. https://codereview.chromium.org/23712005/diff/39001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): https://codereview.chromium.org/23712005/diff/39001/src/gpu/SkGpuDevice.cpp#n... src/gpu/SkGpuDevice.cpp:659: On 2013/10/30 14:36:08, robertphillips wrote: > How expensive is it to create the SkStrokeRect all the time? > Also, don't think we need the final "NULL" on this call to drawRect. Agree. create the SkStrokeRec when it is needed to in new CL, instead of creating it every time.
On 2013/10/30 14:36:36, robertphillips wrote: > Do you have any performance information? Hi, According to the bench case draw_stroke_rect_bevel, my original test shows that it is about 3X acceleration on GPU path for bevel-stroke rects for Linux desktop, and 5X acceleration for Android on Nexus 4. However, it is more than 10X acceleration for Linux now! Both show no regression for irrelevant bench cases.
lgtm https://codereview.chromium.org/23712005/diff/179001/src/gpu/GrAARectRenderer... File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/23712005/diff/179001/src/gpu/GrAARectRenderer... src/gpu/GrAARectRenderer.cpp:370: * 4 7 nice!
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/yunchao.he@intel.com/23712005/179001
Message was sent while issue was closed.
Change committed as 12082
Message was sent while issue was closed.
This was reverted in r12091. This CL caused the following GM tests to fail: thinstrokerects strokerects twopointconical linepath cubicclosepath cubicpath quadpath emptypath quadclosepath lineclosepath degeneratesegments rrect ovals strokerect roundrects drawlooper rects nested_aa poly2poly Of these perhaps the most worrying are the rrect and strokerect failures
Message was sent while issue was closed.
There've also been a bunch of Mac & Win perf bot crashes around this CL, wich appear to have cleared after the revert - for example: http://108.170.217.252:10117/builders/Perf-Mac10.7-MacMini4.1-GeForce320M-x86... So there's a chance that it's more than just visual differences.
On 2013/11/01 19:27:32, robertphillips wrote: > This was reverted in r12091. > > This CL caused the following GM tests to fail: > > thinstrokerects > strokerects > twopointconical > linepath > cubicclosepath > cubicpath > quadpath > emptypath > quadclosepath > lineclosepath > degeneratesegments > rrect > ovals > strokerect > roundrects > drawlooper > rects > nested_aa > poly2poly > > Of these perhaps the most worrying are the rrect and strokerect failures reopen this issue. the difference of strokerect has been fixed, I will fix the others tomorrow.
Hi, Brian and Rob, the bug of GM difference and crash problem are fixed, They are basically introduced by a typo below. I rebased the code, all GM outputs w/ vs w/o the patch are exactly the same except rects_gpu.png(about 200+ pixels are different in 1080000 pixels) which is caused by the slightly difference drawRect vs draw rect in path by drawPath. If no objection, I will land it tomorrow. https://codereview.chromium.org/23712005/diff/379001/src/gpu/GrAARectRenderer... File src/gpu/GrAARectRenderer.cpp (right): https://codereview.chromium.org/23712005/diff/379001/src/gpu/GrAARectRenderer... src/gpu/GrAARectRenderer.cpp:904: verts += (outerVertexNum + innerVertexNum) * vsize; variable 'verts' needs to move 2*outerVertexNum+innerVertexNum points from outer AA line to inner AA line to set the color, but it has moved outerVertexNum points before(line 898), so it needs to move outerVertexNum+ innerVertexNum points here. This small bug lead to most difference for gm cases and crashes reported by Florin Malita(buffer overflow).
lgtm
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/yunchao.he@intel.com/23712005/379001
Message was sent while issue was closed.
Change committed as 12148 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
