Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22486003:
Fix hairline pathrenderer for Nexus-10. Switches to passing in an offset and using that to compute … (Closed)
|
Created:
7 years, 4 months ago by jvanverth1 Modified:
7 years, 4 months ago CC:
skia-review_googlegroups.com, egdaniel Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionFix hairline pathrenderer for Nexus-10. Switches to using additional
geometry and passing in the coverage value instead.
BUG=
Committed: http://code.google.com/p/skia/source/detail?r=10640
Committed: http://code.google.com/p/skia/source/detail?r=10707
R=bsalomon@google.com, robertphillips@google.com
Committed: https://code.google.com/p/skia/source/detail?r=10769
Patch Set 1 #Patch Set 2 : Changed to use geometry-based method #
Total comments: 2
Patch Set 3 : Fix comment. #Patch Set 4 : Changed fCoverage to fLine.fCoverage; added note for later. #Patch Set 5 : Changed fCoverage to fLine.fCoverage; added note for later. #
Total comments: 8
Patch Set 6 : Minor tweaks for nits/suggestions #Patch Set 7 : Split hairline vertex buffer into buffer for line segments and buffer for quadratics #
Total comments: 26
Patch Set 8 : Fixes based on comments #Patch Set 9 : More view matrix cleanup #
Total comments: 2
Patch Set 10 : Clean up asserts #
Messages
Total messages: 21 (0 generated)
This appears to be slightly slower, but not much (measured on Mac). It does
produce non-crufty results on Nexus-10.
lines_1_BW GPU 10.05 10.87 -0.82 -8.2%
lines_1_AA GPU 7.09 7.62 -0.53 -7.5%
lines GPU 19.03 19.25 -0.22 -1.2%
lines_0_AA GPU 0.00 0.00 +0.00 +0.0%
lines_0_BW GPU 0.00 0.01 -0.01 +0.0%
lines_0.5_AA GPU 7.13 7.04 +0.09 +1.3%
On 2013/08/06 21:54:42, JimVV wrote: > This appears to be slightly slower, but not much (measured on Mac). It does > produce non-crufty results on Nexus-10. > > lines_1_BW GPU 10.05 10.87 -0.82 -8.2% > lines_1_AA GPU 7.09 7.62 -0.53 -7.5% > lines GPU 19.03 19.25 -0.22 -1.2% > lines_0_AA GPU 0.00 0.00 +0.00 +0.0% > lines_0_BW GPU 0.00 0.01 -0.01 +0.0% > lines_0.5_AA GPU 7.13 7.04 +0.09 +1.3% lgtm
Based on Robert's advice, I changed this to use a geometry-based approach
instead. With this it still looks good on Nexus-10 and I get comparable results
to before:
lines_1_AA GPU 7.63 7.67 -0.04 -0.5%
lines GPU 19.02 19.04 -0.02 -0.1%
lines_0.5_AA GPU 8.00 8.00 +0.00 +0.0%
lines_0_AA GPU 0.01 0.01 +0.00 +0.0%
lines_0_BW GPU 0.01 0.01 +0.00 +0.0%
lines_1_BW GPU 10.75 10.75 +0.00 +0.0%
https://codereview.chromium.org/22486003/diff/5001/src/gpu/GrAAHairLinePathRe... File src/gpu/GrAAHairLinePathRenderer.cpp (right): https://codereview.chromium.org/22486003/diff/5001/src/gpu/GrAAHairLinePathRe... src/gpu/GrAAHairLinePathRenderer.cpp:489: SkScalar fCoverage; fLineCoverage? or... struct { SkScalar fCoverage; } fLine; It'd just be nice to keep them labeled with the edge type that uses them.
https://codereview.chromium.org/22486003/diff/5001/src/gpu/GrAAHairLinePathRe... File src/gpu/GrAAHairLinePathRenderer.cpp (right): https://codereview.chromium.org/22486003/diff/5001/src/gpu/GrAAHairLinePathRe... src/gpu/GrAAHairLinePathRenderer.cpp:489: SkScalar fCoverage; On 2013/08/07 20:59:57, bsalomon wrote: > fLineCoverage? or... > struct { > SkScalar fCoverage; > } fLine; > > It'd just be nice to keep them labeled with the edge type that uses them. Done.
lgtm
lgtm + nits/suggestions https://codereview.chromium.org/22486003/diff/14001/src/gpu/GrAAHairLinePathR... File src/gpu/GrAAHairLinePathRenderer.cpp (right): https://codereview.chromium.org/22486003/diff/14001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:44: make this static too? https://codereview.chromium.org/22486003/diff/14001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:86: static - given the name? qIdxBuffer -> lineIdxBuffer? // fill the line index buffer with a canonical set of indices to draw AA line segments https://codereview.chromium.org/22486003/diff/14001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:95: // spine of the segment, and alpha = 0 along the outer edges, represented i.e., https://codereview.chromium.org/22486003/diff/14001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:141: SkAutoTUnref<GrIndexBuffer> lIdxBuffer(lIdxBuf); Make one line?
https://codereview.chromium.org/22486003/diff/14001/src/gpu/GrAAHairLinePathR... File src/gpu/GrAAHairLinePathRenderer.cpp (right): https://codereview.chromium.org/22486003/diff/14001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:44: On 2013/08/08 13:08:54, robertphillips wrote: > make this static too? Done. https://codereview.chromium.org/22486003/diff/14001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:86: On 2013/08/08 13:08:54, robertphillips wrote: > static - given the name? > qIdxBuffer -> lineIdxBuffer? > // fill the line index buffer with a canonical set of indices to draw AA line > segments Done. https://codereview.chromium.org/22486003/diff/14001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:95: // spine of the segment, and alpha = 0 along the outer edges, represented On 2013/08/08 13:08:54, robertphillips wrote: > i.e., Done. https://codereview.chromium.org/22486003/diff/14001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:141: SkAutoTUnref<GrIndexBuffer> lIdxBuffer(lIdxBuf); On 2013/08/08 13:08:54, robertphillips wrote: > Make one line? Done.
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/jvanverth@google.com/22486003/22001
Message was sent while issue was closed.
Change committed as 10640
I've updated the change to split the vertex buffer into two: one for line segments and one for quadratics. There doesn't appear to be any significant performance hit.
Adding Greg to CC so he is aware of this. https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... File src/gpu/GrAAHairLinePathRenderer.cpp (right): https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:489: struct { Would it make sense to just have k,l,m and get rid of fQuadCoord (quads would just use k and l)? https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1176: #if GR_DEBUG #if shouldn't be indented https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1249: // TODO: See whether rendering lines as degenerate quads improves perf Let's axe this comment.. I don't think it makes sense now that the number of edge types is increasing. https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... File src/gpu/GrAAHairLinePathRenderer.h (right): https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.h:14: typedef SkTArray<SkPoint, true> PtArray; Can we nest these in the class?
https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... File src/gpu/GrAAHairLinePathRenderer.cpp (right): https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:500: Do we need one of these for LineVertex now? https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:729: // just make it degenerate and likely offscreen Would this be clearer as a loop over kVertsPerLineSeg? https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1011: const SkMatrix& viewM? https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1013: This comment seems out of date https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1018: viewM.mapRect(devBounds); Do we still need to go out 1 here (can we go out .5)? https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1034: Do we still need toDevice? https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1065: const SkMatrix& viewM? https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1176: #if GR_DEBUG Could this test be a sub routine? https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1258: #if GR_DEBUG Make this a sub routine too?
https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... File src/gpu/GrAAHairLinePathRenderer.cpp (right): https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:489: struct { On 2013/08/12 17:25:43, bsalomon wrote: > Would it make sense to just have k,l,m and get rid of fQuadCoord (quads would > just use k and l)? I think that's a question for Greg. Greg? https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:500: On 2013/08/12 17:27:01, robertphillips wrote: > Do we need one of these for LineVertex now? I'll turn this around and ask: Do we need this for BezierVertex? What's the point of the check? https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:729: // just make it degenerate and likely offscreen On 2013/08/12 17:27:01, robertphillips wrote: > Would this be clearer as a loop over kVertsPerLineSeg? Done. https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1011: On 2013/08/12 17:27:01, robertphillips wrote: > const SkMatrix& viewM? Done. https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1013: On 2013/08/12 17:27:01, robertphillips wrote: > This comment seems out of date Done. https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1018: viewM.mapRect(devBounds); On 2013/08/12 17:27:01, robertphillips wrote: > Do we still need to go out 1 here (can we go out .5)? I think we still do -- the line geometry bounds extends 1 on either side of the line. https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1034: On 2013/08/12 17:27:01, robertphillips wrote: > Do we still need toDevice? Done. https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1065: On 2013/08/12 17:27:01, robertphillips wrote: > const SkMatrix& viewM? Done. https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1176: #if GR_DEBUG On 2013/08/12 17:25:43, bsalomon wrote: > #if shouldn't be indented Done. https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1176: #if GR_DEBUG On 2013/08/12 17:27:01, robertphillips wrote: > Could this test be a sub routine? Done. https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1249: // TODO: See whether rendering lines as degenerate quads improves perf On 2013/08/12 17:25:43, bsalomon wrote: > Let's axe this comment.. I don't think it makes sense now that the number of > edge types is increasing. Done. https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1258: #if GR_DEBUG On 2013/08/12 17:27:01, robertphillips wrote: > Make this a sub routine too? Done. https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... File src/gpu/GrAAHairLinePathRenderer.h (right): https://codereview.chromium.org/22486003/diff/31001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.h:14: typedef SkTArray<SkPoint, true> PtArray; On 2013/08/12 17:25:43, bsalomon wrote: > Can we nest these in the class? Done.
lgtm with minor change noted below. https://codereview.chromium.org/22486003/diff/45001/src/gpu/GrAAHairLinePathR... File src/gpu/GrAAHairLinePathRenderer.cpp (right): https://codereview.chromium.org/22486003/diff/45001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1193: #if GR_DEBUG We can remove the #if now that the call is in an assert. https://codereview.chromium.org/22486003/diff/45001/src/gpu/GrAAHairLinePathR... src/gpu/GrAAHairLinePathRenderer.cpp:1250: #if GR_DEBUG and here
On 2013/08/13 15:14:22, bsalomon wrote: > lgtm with minor change noted below. > > https://codereview.chromium.org/22486003/diff/45001/src/gpu/GrAAHairLinePathR... > File src/gpu/GrAAHairLinePathRenderer.cpp (right): > > https://codereview.chromium.org/22486003/diff/45001/src/gpu/GrAAHairLinePathR... > src/gpu/GrAAHairLinePathRenderer.cpp:1193: #if GR_DEBUG > We can remove the #if now that the call is in an assert. > > https://codereview.chromium.org/22486003/diff/45001/src/gpu/GrAAHairLinePathR... > src/gpu/GrAAHairLinePathRenderer.cpp:1250: #if GR_DEBUG > and here Oh, should we be using SkAssert now?
On 2013/08/13 15:21:38, JimVV wrote: > On 2013/08/13 15:14:22, bsalomon wrote: > > lgtm with minor change noted below. > > > > > https://codereview.chromium.org/22486003/diff/45001/src/gpu/GrAAHairLinePathR... > > File src/gpu/GrAAHairLinePathRenderer.cpp (right): > > > > > https://codereview.chromium.org/22486003/diff/45001/src/gpu/GrAAHairLinePathR... > > src/gpu/GrAAHairLinePathRenderer.cpp:1193: #if GR_DEBUG > > We can remove the #if now that the call is in an assert. > > > > > https://codereview.chromium.org/22486003/diff/45001/src/gpu/GrAAHairLinePathR... > > src/gpu/GrAAHairLinePathRenderer.cpp:1250: #if GR_DEBUG > > and here > > Oh, should we be using SkAssert now? Good point, yes.
lgtm
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/jvanverth@google.com/22486003/50001
Message was sent while issue was closed.
Change committed as 10707
Message was sent while issue was closed.
Committed patchset #10 manually as r10769 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
