Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1094293002:
Fix double blend in GrAAConvexPathRenderer (Closed)
|
Created:
5 years, 8 months ago by Chris Dalton Modified:
5 years, 7 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionFix double blend in GrAAConvexPathRenderer
Updates GrAAConvexPathRenderer to not draw the interior fan of a
convex path when it does not exist (i.e when segment count <= 2).
TODO: We should also detect and combine colinear segments in order to
make sure we catch every case.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/3596482cc7b36c9f45f59c304dfb28a841229525
Patch Set 1 #
Total comments: 3
Patch Set 2 : Combine colinear lines #
Total comments: 3
Patch Set 3 : Threshold in pixels #Patch Set 4 : Remove colinear lineTo stuff #
Total comments: 1
Messages
Total messages: 19 (3 generated)
cdalton@nvidia.com changed reviewers: + bsalomon@google.com
This will require an update to the gm expectation for mixed_xfermodes, gpu and gpudft. https://codereview.chromium.org/1094293002/diff/1/src/gpu/GrAAConvexPathRende... File src/gpu/GrAAConvexPathRenderer.cpp (right): https://codereview.chromium.org/1094293002/diff/1/src/gpu/GrAAConvexPathRende... src/gpu/GrAAConvexPathRenderer.cpp:494: if (count > 2) { I decided to err on the side of minimal change, but let me know if you want me to rearrange things so we only write fanPt to the vertex array inside this branch.
https://codereview.chromium.org/1094293002/diff/1/src/gpu/GrAAConvexPathRende... File src/gpu/GrAAConvexPathRenderer.cpp (right): https://codereview.chromium.org/1094293002/diff/1/src/gpu/GrAAConvexPathRende... src/gpu/GrAAConvexPathRenderer.cpp:439: if (count > 2) { Do we need to worry about the case where two colinear lineTo's close off a quadratic? (In other words, the interior is still empty but segment count > 2) Or do they already get collapsed into a single lineTo?
bsalomon@google.com changed reviewers: + robertphillips@google.com
https://codereview.chromium.org/1094293002/diff/1/src/gpu/GrAAConvexPathRende... File src/gpu/GrAAConvexPathRenderer.cpp (right): https://codereview.chromium.org/1094293002/diff/1/src/gpu/GrAAConvexPathRende... src/gpu/GrAAConvexPathRenderer.cpp:439: if (count > 2) { On 2015/04/21 11:49:05, Chris Dalton wrote: > Do we need to worry about the case where two colinear lineTo's close off a > quadratic? (In other words, the interior is still empty but segment count > 2) > Or do they already get collapsed into a single lineTo? SkPath won't notice colinear lines when constructing a path so I think you could have this case. Maybe the empty interior could be checked when computing the center of mass? Adding Rob, since he is looking to rewrite the tessellation for convex paths.
On 2015/04/21 15:50:08, bsalomon wrote: > https://codereview.chromium.org/1094293002/diff/1/src/gpu/GrAAConvexPathRende... > File src/gpu/GrAAConvexPathRenderer.cpp (right): > > https://codereview.chromium.org/1094293002/diff/1/src/gpu/GrAAConvexPathRende... > src/gpu/GrAAConvexPathRenderer.cpp:439: if (count > 2) { > On 2015/04/21 11:49:05, Chris Dalton wrote: > > Do we need to worry about the case where two colinear lineTo's close off a > > quadratic? (In other words, the interior is still empty but segment count > 2) > > Or do they already get collapsed into a single lineTo? > > SkPath won't notice colinear lines when constructing a path so I think you could > have this case. Maybe the empty interior could be checked when computing the > center of mass? > > Adding Rob, since he is looking to rewrite the tessellation for convex paths. Ok, good to know they don't get combined. The shape will only get rasterized water tight without an interior if it has two segments that share the exact same endpoints, so I think segment count is the appropriate check. I could look into combining lineTos in get_segments, or did you say this code is going to be rewritten? In that case maybe what we have already is good enough?
On 2015/04/21 19:53:16, Chris Dalton wrote: > On 2015/04/21 15:50:08, bsalomon wrote: > > > https://codereview.chromium.org/1094293002/diff/1/src/gpu/GrAAConvexPathRende... > > File src/gpu/GrAAConvexPathRenderer.cpp (right): > > > > > https://codereview.chromium.org/1094293002/diff/1/src/gpu/GrAAConvexPathRende... > > src/gpu/GrAAConvexPathRenderer.cpp:439: if (count > 2) { > > On 2015/04/21 11:49:05, Chris Dalton wrote: > > > Do we need to worry about the case where two colinear lineTo's close off a > > > quadratic? (In other words, the interior is still empty but segment count > > 2) > > > Or do they already get collapsed into a single lineTo? > > > > SkPath won't notice colinear lines when constructing a path so I think you > could > > have this case. Maybe the empty interior could be checked when computing the > > center of mass? > > > > Adding Rob, since he is looking to rewrite the tessellation for convex paths. > > Ok, good to know they don't get combined. The shape will only get rasterized > water tight without an interior if it has two segments that share the exact same > endpoints, so I think segment count is the appropriate check. > > I could look into combining lineTos in get_segments, or did you say this code is > going to be rewritten? In that case maybe what we have already is good enough? So what I mean was that there could be any number of line segments that are colinear, so I don't think the count is correct. The tessellation code Rob is working on will make it here eventually but it could be a while, so it is probably worth fixing this now. In center_of_mass() could we return something indicating that the fan can be omitted when the computed area is close to zero? Currently, it takes the average of all the on-contour control points.
On 2015/04/21 20:12:55, bsalomon wrote: > So what I mean was that there could be any number of line segments that are > colinear, so I don't think the count is correct. > > The tessellation code Rob is working on will make it here eventually but it > could be a while, so it is probably worth fixing this now. In center_of_mass() > could we return something indicating that the fan can be omitted when the > computed area is close to zero? Currently, it takes the average of all the > on-contour control points. Right, segment count wouldn't be the correct check unless by the time we got here, colinear lineTos were guaranteed to have been combined into one. So my concern with omitting the fan when area ~= 0 (meaning, the fan is actually just a shared edge) is that we could still end up with overlap along that edge, or dropped pixels. For shared edges, a lot of work goes into the GPU rasterizer to guarantee it finds a water tight edge with no overlap, but you have to you feed it the exact same floating-point endpoints, bit for bit. Otherwise, if you give it endpoints that are colinear but not the same point in space, it's not possible with floating point numerics to always guarantee that perfect edge. This is what would end up happening if we gave the rasterizer colinear lineTos, and it's the reason why drawing the ~empty fan was resulting in overlap before this change. So I think part of fixing the "fan == shared edge" issue is to also combine the colinear lineTos.
On 2015/04/21 21:18:39, Chris Dalton wrote: > On 2015/04/21 20:12:55, bsalomon wrote: > > So what I mean was that there could be any number of line segments that are > > colinear, so I don't think the count is correct. > > > > The tessellation code Rob is working on will make it here eventually but it > > could be a while, so it is probably worth fixing this now. In center_of_mass() > > could we return something indicating that the fan can be omitted when the > > computed area is close to zero? Currently, it takes the average of all the > > on-contour control points. > > Right, segment count wouldn't be the correct check unless by the time we got > here, colinear lineTos were guaranteed to have been combined into one. > > So my concern with omitting the fan when area ~= 0 (meaning, the fan is actually > just a shared edge) is that we could still end up with overlap along that edge, > or dropped pixels. > > For shared edges, a lot of work goes into the GPU rasterizer to guarantee it > finds a water tight edge with no overlap, but you have to you feed it the exact > same floating-point endpoints, bit for bit. Otherwise, if you give it endpoints > that are colinear but not the same point in space, it's not possible with > floating point numerics to always guarantee that perfect edge. This is what > would end up happening if we gave the rasterizer colinear lineTos, and it's the > reason why drawing the ~empty fan was resulting in overlap before this change. > So I think part of fixing the "fan == shared edge" issue is to also combine the > colinear lineTos. I see. That makes sense to me.
This one combines colinear lineTo segments. Even without the combined lineTos, the gms are all deterministic with barriers, so it's up to you guys to decide if it's worth the extra protection against overlap or (very unlikely) dropped pixels. The extra cpu overhead to combine colinear lineTos is small but not zero. This also won't protect against a torture case like: quad microscopic lineTo quad But I'm not worried about that one. The code works with every path that's at least trying to be reasonable. There's also the issue of a redundant fanPt being added to the vertex array when the fan doesn't end up getting drawn. I left it for the sake of simplicity, but let me know if you want me to rearrange things to not add the fanPt if it won't end up being used...
https://codereview.chromium.org/1094293002/diff/20001/src/gpu/GrAAConvexPathR... File src/gpu/GrAAConvexPathRenderer.cpp (right): https://codereview.chromium.org/1094293002/diff/20001/src/gpu/GrAAConvexPathR... src/gpu/GrAAConvexPathRenderer.cpp:226: Seems like we should do this in terms of pixels.
https://codereview.chromium.org/1094293002/diff/20001/src/gpu/GrAAConvexPathR... File src/gpu/GrAAConvexPathRenderer.cpp (right): https://codereview.chromium.org/1094293002/diff/20001/src/gpu/GrAAConvexPathR... src/gpu/GrAAConvexPathRenderer.cpp:226: On 2015/04/24 12:32:55, robertphillips wrote: > Seems like we should do this in terms of pixels. Are we not in device space here?
https://codereview.chromium.org/1094293002/diff/20001/src/gpu/GrAAConvexPathR... File src/gpu/GrAAConvexPathRenderer.cpp (right): https://codereview.chromium.org/1094293002/diff/20001/src/gpu/GrAAConvexPathR... src/gpu/GrAAConvexPathRenderer.cpp:226: On 2015/04/24 14:19:55, bsalomon wrote: > On 2015/04/24 12:32:55, robertphillips wrote: > > Seems like we should do this in terms of pixels. > > Are we not in device space here? Doing it in pixels is fine, it just comes at the cost of an extra sqrt. I also reduced the threshold quite a bit, which causes pixel variance in conicpaths.
On 2015/04/24 18:36:27, Chris Dalton wrote: > https://codereview.chromium.org/1094293002/diff/20001/src/gpu/GrAAConvexPathR... > File src/gpu/GrAAConvexPathRenderer.cpp (right): > > https://codereview.chromium.org/1094293002/diff/20001/src/gpu/GrAAConvexPathR... > src/gpu/GrAAConvexPathRenderer.cpp:226: > On 2015/04/24 14:19:55, bsalomon wrote: > > On 2015/04/24 12:32:55, robertphillips wrote: > > > Seems like we should do this in terms of pixels. > > > > Are we not in device space here? > > Doing it in pixels is fine, it just comes at the cost of an extra sqrt. > > I also reduced the threshold quite a bit, which causes pixel variance in > conicpaths. Thinking about it more, I think I might prefer the threshold to be relative to the length of the line segment rather than pixels-- * For higher order shapes composed of subpixel quadratics, each segment could potentially get combined along the way if the threshold is in pixels, building up a significant error. * The sqrt is pretty expensive, especially for big shapes made up of tiny segments... Either way, I'd like to get this resolved quickly since it's blocking my texture barrier patch. Was there anything else you guys wanted to discuss? Leaving out the interior fan is all it actually needs to fix the gm's, the colinear lineTos were just for completeness.
Verified this resolves the gm issues. https://codereview.chromium.org/1094293002/diff/60001/src/gpu/GrAAConvexPathR... File src/gpu/GrAAConvexPathRenderer.cpp (right): https://codereview.chromium.org/1094293002/diff/60001/src/gpu/GrAAConvexPathR... src/gpu/GrAAConvexPathRenderer.cpp:498: if (count >= 3) { I decided to err on the side of minimal code change, but let me know if you want me to rearrange things so we only write fanPt to the vertex array when we take this branch.
lgtm
The CQ bit was checked by cdalton@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1094293002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/3596482cc7b36c9f45f59c304dfb28a841229525 |
