Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2106953009:
Fix bug where ovals' AA exceed bounds by .5 pixel (Closed)
|
Created:
4 years, 5 months ago by vjiaoblack Modified:
4 years, 5 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionFix bug where ovals' AA exceed bounds by .5 pixel
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2106953009
Committed: https://skia.googlesource.com/skia/+/977996dd0e0c6e6686a980a23e97e6fb5da1a592
Patch Set 1 #
Total comments: 2
Patch Set 2 : Realized that lines of code previous to change were inverse of the change. Deleted them. #
Total comments: 2
Patch Set 3 : In addition to the past change, also outset/expand relevant variables/bounds #
Total comments: 4
Patch Set 4 : Cleaned up code #Messages
Total messages: 22 (9 generated)
Description was changed from ========== Fix bug where ovals' AA exceed bounds by .5 pixel BUG=skia: ========== to ========== Fix bug where ovals' AA exceed bounds by .5 pixel BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2106953009 ==========
vjiaoblack@google.com changed reviewers: + jvanverth@google.com
robertphillips@google.com changed reviewers: + robertphillips@google.com
https://codereview.chromium.org/2106953009/diff/1/src/gpu/GrOvalRenderer.cpp File src/gpu/GrOvalRenderer.cpp (right): https://codereview.chromium.org/2106953009/diff/1/src/gpu/GrOvalRenderer.cpp#... src/gpu/GrOvalRenderer.cpp:946: Hmmm ... can we not just remove the above 2 lines ?
https://codereview.chromium.org/2106953009/diff/1/src/gpu/GrOvalRenderer.cpp File src/gpu/GrOvalRenderer.cpp (right): https://codereview.chromium.org/2106953009/diff/1/src/gpu/GrOvalRenderer.cpp#... src/gpu/GrOvalRenderer.cpp:946: On 2016/06/30 14:10:16, robertphillips wrote: > Hmmm ... can we not just remove the above 2 lines ? Right yeah, sorry. Jim pointed this out while overlooking my commit. Fixed in Patch Set 2.
https://codereview.chromium.org/2106953009/diff/20001/src/gpu/GrOvalRenderer.cpp File src/gpu/GrOvalRenderer.cpp (left): https://codereview.chromium.org/2106953009/diff/20001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:945: yRadius += SK_ScalarHalf; I don't think this is quite right. We need to expand the bounds by 0.5 in each direction (left, top, right, bottom) to make space for 1/2 a pixel on each side of anti-aliasing. Otherwise we'll chop off that 1/2 a pixel. I think what needs to happen is: 1) Leave the inner and outer radii alone. Those are used for the ellipse function so they should stay the original size of the ellipse. 2) When creating the devBounds, expand it by 0.5 in all directions (you can do that with SkRect::outset). 3) Up in EllipseBatch::onPrepareDraws, adjust fOffset so it's 0.5 bigger in each direction than (xRadius, yRadius). See lines 1664 and 1454-1470 to see how this is handled with roundrects.
The CQ bit was checked by vjiaoblack@google.com
The CQ bit was unchecked by vjiaoblack@google.com
https://codereview.chromium.org/2106953009/diff/20001/src/gpu/GrOvalRenderer.cpp File src/gpu/GrOvalRenderer.cpp (left): https://codereview.chromium.org/2106953009/diff/20001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:945: yRadius += SK_ScalarHalf; On 2016/06/30 15:29:19, jvanverth1 wrote: > I don't think this is quite right. We need to expand the bounds by 0.5 in each > direction (left, top, right, bottom) to make space for 1/2 a pixel on each side > of anti-aliasing. Otherwise we'll chop off that 1/2 a pixel. I think what needs > to happen is: > > 1) Leave the inner and outer radii alone. Those are used for the ellipse > function so they should stay the original size of the ellipse. > 2) When creating the devBounds, expand it by 0.5 in all directions (you can do > that with SkRect::outset). > 3) Up in EllipseBatch::onPrepareDraws, adjust fOffset so it's 0.5 bigger in each > direction than (xRadius, yRadius). > > See lines 1664 and 1454-1470 to see how this is handled with roundrects. I've implemented these changes locally, and cross-compared the results between the original (before this series of changes entirely) and new (your proposed changes) versions of the code, along with switching between the diellipse code and the ellipse code. Since the problem originally seemed to be that the GPU-drawn oval was slightly too large, esp noticeably with the hairline ovals (or have I interpreted the bug wrong? I thought it was clear, but I indeed could have misinterpreted it), this current fix does not appear to resolve that problem itself. Actually, (I think due to increasing the fOffset) the hairline ovals became slightly thicker and larger. Are you sure this is the intended fix? I looked at the GM:rrect test, and admittedly, the rounded rects look pretty spot on.
Hey! I've put the code back up with the requested changes from @Jim; please take a look!
https://codereview.chromium.org/2106953009/diff/40001/src/gpu/GrOvalRenderer.cpp File src/gpu/GrOvalRenderer.cpp (right): https://codereview.chromium.org/2106953009/diff/40001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:826: verts[0].fOffset = SkPoint::Make(-xRadius - SK_ScalarHalf, -yRadius - SK_ScalarHalf); I suggest creating two variables, like xMaxOffset and yMaxOffset, set them to xRadius + 0.5 and yRadius + 0.5, and use those here instead. I think that'd be a little clearer. https://codereview.chromium.org/2106953009/diff/40001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:942: // We've extended the outer x radius out half a pixel to antialias. I would delete all these lines.
https://codereview.chromium.org/2106953009/diff/40001/src/gpu/GrOvalRenderer.cpp File src/gpu/GrOvalRenderer.cpp (right): https://codereview.chromium.org/2106953009/diff/40001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:826: verts[0].fOffset = SkPoint::Make(-xRadius - SK_ScalarHalf, -yRadius - SK_ScalarHalf); On 2016/06/30 18:00:06, jvanverth1 wrote: > I suggest creating two variables, like xMaxOffset and yMaxOffset, set them to > xRadius + 0.5 and yRadius + 0.5, and use those here instead. I think that'd be a > little clearer. Done. https://codereview.chromium.org/2106953009/diff/40001/src/gpu/GrOvalRenderer.... src/gpu/GrOvalRenderer.cpp:942: // We've extended the outer x radius out half a pixel to antialias. On 2016/06/30 18:00:06, jvanverth1 wrote: > I would delete all these lines. Done.
lgtm
The CQ bit was checked by vjiaoblack@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 vjiaoblack@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix bug where ovals' AA exceed bounds by .5 pixel BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2106953009 ========== to ========== Fix bug where ovals' AA exceed bounds by .5 pixel BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2106953009 Committed: https://skia.googlesource.com/skia/+/977996dd0e0c6e6686a980a23e97e6fb5da1a592 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/977996dd0e0c6e6686a980a23e97e6fb5da1a592 |
