Restrict query bounds for reduce clip to dev bounds

Restrict query bounds for reduce clip to dev bounds

Committed: https://skia.googlesource.com/skia/+/db4758c75fdd30ef58096c685e2d749aacaf3d92

[bsalomon, 2015-11-23 17:04:39]

bsalomon@google.com changed reviewers:
+ robertphillips@google.com

[bsalomon, 2015-11-23 17:04:52]

bsalomon@google.com changed reviewers:
+ joshualitt@google.com

[robertphillips, 2015-11-23 17:22:56]

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:413: // a 1 pixel thickness in device space.
Are we double checking here ?

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/batches/GrAALin...
File src/gpu/batches/GrAALinearizingConvexPathRenderer.cpp (right):

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/batches/GrAALin...
src/gpu/batches/GrAALinearizingConvexPathRenderer.cpp:270: if
(SkPaint::kMiter_Join == geometry.fJoin) {
Shouldn't this be a max ?
Does the miterLimit apply to hairlines (i.e., should we check it for hairlines
too) ?

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/batches/GrTesse...
File src/gpu/batches/GrTessellatingPathRenderer.cpp (right):

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/batches/GrTesse...
src/gpu/batches/GrTessellatingPathRenderer.cpp:1599: // include the path bounds.
// This will increase the size of the resulting geometry but will propagate more
reliable information to the ClipStack (which will need to apply an explicit clip
to the inverse filled geometry).

?

I wonder if adding an extra bit of bloat to the clipBounds (for the inverse fill
case) will make the tesselator happier.

[joshualitt, 2015-11-23 17:26:07]

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/GrClipMaskManag...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/GrClipMaskManag...
src/gpu/GrClipMaskManager.cpp:321: if (devBounds) {
When would we not have devbounds?

[bsalomon, 2015-11-23 17:59:34]

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/GrClipMaskManag...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/GrClipMaskManag...
src/gpu/GrClipMaskManager.cpp:321: if (devBounds) {
On 2015/11/23 17:26:07, joshualitt wrote:
> When would we not have devbounds?

Not sure... was planning to investigate that and possibly put up another CL.

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:413: // a 1 pixel thickness in device space.
On 2015/11/23 17:22:55, robertphillips wrote:
> Are we double checking here ?

Eek, meant kPoints not kLines.

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/batches/GrAALin...
File src/gpu/batches/GrAALinearizingConvexPathRenderer.cpp (right):

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/batches/GrAALin...
src/gpu/batches/GrAALinearizingConvexPathRenderer.cpp:270: if
(SkPaint::kMiter_Join == geometry.fJoin) {
On 2015/11/23 17:22:55, robertphillips wrote:
> Shouldn't this be a max ?
> Does the miterLimit apply to hairlines (i.e., should we check it for hairlines
> too) ?

hairlines don't get joins applied to them. Do you mean max(w, w*miterlimit)? I
was assuming miter limit is >= 1, but will check.

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/batches/GrTesse...
File src/gpu/batches/GrTessellatingPathRenderer.cpp (right):

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/batches/GrTesse...
src/gpu/batches/GrTessellatingPathRenderer.cpp:1599: // include the path bounds.
On 2015/11/23 17:22:55, robertphillips wrote:
> // This will increase the size of the resulting geometry but will propagate
more
> reliable information to the ClipStack (which will need to apply an explicit
clip
> to the inverse filled geometry).

It's not fundamentally for clipping. The current code is just incorrect even
without changes to the clipping code.

> 
> ?
> 
> I wonder if adding an extra bit of bloat to the clipBounds (for the inverse
fill
> case) will make the tesselator happier.

I don't see any artifacts in any of our tests. I can think of cases where it
would simplify things to add pad and cases where it would make it worse (e.g.
the contains a rect where one edge of the rect is along the path bounds.

[joshualitt, 2015-11-23 18:30:51]

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/GrClipMaskManag...
File src/gpu/GrClipMaskManager.cpp (right):

https://codereview.chromium.org/1467253002/diff/40001/src/gpu/GrClipMaskManag...
src/gpu/GrClipMaskManager.cpp:321: if (devBounds) {
On 2015/11/23 17:59:34, bsalomon wrote:
> On 2015/11/23 17:26:07, joshualitt wrote:
> > When would we not have devbounds?
> 
> Not sure... was planning to investigate that and possibly put up another CL.

cool, other than possibly NVPR, everyone would, I hope, have bounds.  It'd be
really nice to just assert that we have devbounds or make it a const ref

[bsalomon, 2015-11-23 18:52:35]

PTAL

[robertphillips, 2015-11-23 18:55:23]

lgtm

[robertphillips, 2015-11-23 18:58:46]

https://codereview.chromium.org/1467253002/diff/60001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/1467253002/diff/60001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:414: if (!paint.isAntiAlias() ||
GrIsPrimTypeLines(primitiveType) ||
yodafy ?

https://codereview.chromium.org/1467253002/diff/60001/src/gpu/batches/GrAALin...
File src/gpu/batches/GrAALinearizingConvexPathRenderer.cpp (right):

https://codereview.chromium.org/1467253002/diff/60001/src/gpu/batches/GrAALin...
src/gpu/batches/GrAALinearizingConvexPathRenderer.cpp:269: w /= 2;
// comment here about miter falling back to bevel ?

[bsalomon, 2015-11-23 19:01:19]

https://codereview.chromium.org/1467253002/diff/60001/src/gpu/GrDrawContext.cpp
File src/gpu/GrDrawContext.cpp (right):

https://codereview.chromium.org/1467253002/diff/60001/src/gpu/GrDrawContext.c...
src/gpu/GrDrawContext.cpp:414: if (!paint.isAntiAlias() ||
GrIsPrimTypeLines(primitiveType) ||
On 2015/11/23 18:58:45, robertphillips wrote:
> yodafy ?

Done.

https://codereview.chromium.org/1467253002/diff/60001/src/gpu/batches/GrAALin...
File src/gpu/batches/GrAALinearizingConvexPathRenderer.cpp (right):

https://codereview.chromium.org/1467253002/diff/60001/src/gpu/batches/GrAALin...
src/gpu/batches/GrAALinearizingConvexPathRenderer.cpp:269: w /= 2;
On 2015/11/23 18:58:46, robertphillips wrote:
> // comment here about miter falling back to bevel ?

Done.

[bsalomon, 2015-11-23 19:01:34]

The CQ bit was checked by bsalomon@google.com

[bsalomon, 2015-11-23 19:01:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1467253002/#ps100001
(title: "Address comments")

[commit-bot: I haz the power, 2015-11-23 19:01:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1467253002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1467253002/100001

[commit-bot: I haz the power, 2015-11-23 19:14:22]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/db4758c75fdd30ef58096c685e2d749aacaf3d92

[scroggo, 2015-11-30 19:37:41]

On 2015/11/23 19:14:22, commit-bot: I haz the power wrote:
> Committed patchset #6 (id:100001) as
> https://skia.googlesource.com/skia/+/db4758c75fdd30ef58096c685e2d749aacaf3d92

Brian, could this have sped up https://perf.skia.org/cl/1004 ?