Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
880643002:
Convex batch (Closed)
|
Created:
5 years, 11 months ago by joshua.litt Modified:
5 years, 10 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@hair_defer Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionConvex batch
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/27f398f04dff306418a142c27175eaa35d21a915
Patch Set 1 #Patch Set 2 : cleaup #Patch Set 3 : more cleanup #
Total comments: 1
Patch Set 4 : test #Patch Set 5 : adding comment #
Total comments: 2
Patch Set 6 : feedback incorporated #Patch Set 7 : feedback inc #Patch Set 8 : cleaup #Messages
Total messages: 19 (6 generated)
joshualitt@google.com changed reviewers: + bsalomon@google.com
joshualitt@google.com changed reviewers: + joshualitt@google.com
On 2015/01/26 20:15:24, joshualitt wrote: ptal
https://codereview.chromium.org/880643002/diff/40001/src/gpu/GrAAConvexPathRe... File src/gpu/GrAAConvexPathRenderer.cpp (right): https://codereview.chromium.org/880643002/diff/40001/src/gpu/GrAAConvexPathRe... src/gpu/GrAAConvexPathRenderer.cpp:894: // This outset was determined experimentally by running skps and gms. It probably could be a yikes!!! That seems really dangerous. I think we need to determine a provable bound based on the vertices that will be computed.
tighter bounding box + only using one gp
https://codereview.chromium.org/880643002/diff/80001/src/gpu/GrAAConvexPathRe... File src/gpu/GrAAConvexPathRenderer.cpp (right): https://codereview.chromium.org/880643002/diff/80001/src/gpu/GrAAConvexPathRe... src/gpu/GrAAConvexPathRenderer.cpp:896: // This outset was determined experimentally by running skps and gms. It probably could be a Ok, can we change this comment to just say that we outset our vertices by 1 pixel and add 1 more pixel for precision? Is it possible to remove the hack that uses "tolDevBounds" when checking the bounds.
On 2015/02/05 18:36:12, bsalomon wrote: > https://codereview.chromium.org/880643002/diff/80001/src/gpu/GrAAConvexPathRe... > File src/gpu/GrAAConvexPathRenderer.cpp (right): > > https://codereview.chromium.org/880643002/diff/80001/src/gpu/GrAAConvexPathRe... > src/gpu/GrAAConvexPathRenderer.cpp:896: // This outset was determined > experimentally by running skps and gms. It probably could be a > Ok, can we change this comment to just say that we outset our vertices by 1 > pixel and add 1 more pixel for precision? Is it possible to remove the hack that > uses "tolDevBounds" when checking the bounds. feedback incorporated. I might have to start batching paths too, but I'd like to get some perf numbers before I invest more time at this stage of the game.
https://codereview.chromium.org/880643002/diff/80001/src/gpu/GrAAConvexPathRe... File src/gpu/GrAAConvexPathRenderer.cpp (right): https://codereview.chromium.org/880643002/diff/80001/src/gpu/GrAAConvexPathRe... src/gpu/GrAAConvexPathRenderer.cpp:816: // Check devBounds I didn't mean to eliminate the check... just the expansion from arg.fDevBounds to tolDevBounds. This assert is still valuable. I'm just hoping that the outset of 2 covers everything.
On 2015/02/05 19:51:33, bsalomon wrote: > https://codereview.chromium.org/880643002/diff/80001/src/gpu/GrAAConvexPathRe... > File src/gpu/GrAAConvexPathRenderer.cpp (right): > > https://codereview.chromium.org/880643002/diff/80001/src/gpu/GrAAConvexPathRe... > src/gpu/GrAAConvexPathRenderer.cpp:816: // Check devBounds > I didn't mean to eliminate the check... just the expansion from arg.fDevBounds > to tolDevBounds. This assert is still valuable. I'm just hoping that the outset > of 2 covers everything. feedback incorporated
If the assert still passes, lgtm
The CQ bit was checked by joshualitt@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/880643002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on client.skia.compile (http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu13.10-Cl...)
New patchsets have been uploaded after l-g-t-m from bsalomon@google.com
The CQ bit was checked by joshualitt@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/880643002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://skia.googlesource.com/skia/+/27f398f04dff306418a142c27175eaa35d21a915 |
