Allow GrReducedClip to take non-integer query bounds

Allow GrReducedClip to take non-integer query bounds

Fixes places where AA bloat was being conflated with geometric
boundaries and updates GrReducedClip to work with non-integer query
bounds. This allows for better clip reduction with AA shared edges.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2160093002

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

[csmartdalton, 2016-07-18 22:18:50]

Description was changed from

==========
Allow GrReducedClip to take non-integer query bounds

Fixes places where AA bloat was being conflated with geometric
boundaries and updates GrReducedClip to work with non-integer query
bounds. This allows for better clip reduction with AA shared edges.

BUG=skia:
==========

to

==========
Allow GrReducedClip to take non-integer query bounds

Fixes places where AA bloat was being conflated with geometric
boundaries and updates GrReducedClip to work with non-integer query
bounds. This allows for better clip reduction with AA shared edges.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2160093002
==========

[csmartdalton, 2016-07-18 22:27:13]

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

[bsalomon, 2016-07-19 13:33:12]

This looks like a good improvement. From the try logs I'm guessing that the fuzz
is somehow leading to a rect being produced with a bottom value that has a
device height rather than device height - 1 which leads us to compute a GL
scissor with a y at -1.

https://codereview.chromium.org/2160093002/diff/20001/include/gpu/GrClip.h
File include/gpu/GrClip.h (right):

https://codereview.chromium.org/2160093002/diff/20001/include/gpu/GrClip.h#ne...
include/gpu/GrClip.h:123: constexpr static SkScalar kClipBoundsFuzz = 1e-3f;
Can we call this tolerance or "tol" rather than fuzz? Just more consistent with
other similar constants.

https://codereview.chromium.org/2160093002/diff/20001/src/gpu/GrReducedClip.cpp
File src/gpu/GrReducedClip.cpp (right):

https://codereview.chromium.org/2160093002/diff/20001/src/gpu/GrReducedClip.c...
src/gpu/GrReducedClip.cpp:351: // TODO: Some way to indicate there are no
clipIBounds?
We could change the InitialState enum to incorporate this perhaps. Or
alternatively we could return a set of 4 bits to the caller. There would be one
bit per edge of clipIBounds, indicating whether the caller must scissor against
that edge. This would allow us to merge draws with different clipIBounds.

E.g. draw 1 needs a right scissor of 20 and the device bounds of draw 2 have a
right edge of 18. Those could batch with a right scissor edge of 20, but today
we don't do that. I think this comes up when you have things that run over the
right or bottom edge of a UI element. So most of the elements need no clip and a
few at the edges need a scissor that all the other draws are inside.

https://codereview.chromium.org/2160093002/diff/20001/src/utils/SkLua.cpp
File src/utils/SkLua.cpp (right):

https://codereview.chromium.org/2160093002/diff/20001/src/utils/SkLua.cpp#new...
src/utils/SkLua.cpp:667: // It absolutely must return these as additional items
on the lua stack in order to be correct.
This was used to gather information about multi-element clips. It isn't used to
actually implement anything by lua other than dumping the element list. I don't
know that anything is necessarily incorrect here.

[csmartdalton, 2016-07-19 15:59:01]

https://codereview.chromium.org/2160093002/diff/20001/include/gpu/GrClip.h
File include/gpu/GrClip.h (right):

https://codereview.chromium.org/2160093002/diff/20001/include/gpu/GrClip.h#ne...
include/gpu/GrClip.h:123: constexpr static SkScalar kClipBoundsFuzz = 1e-3f;
On 2016/07/19 13:33:12, bsalomon wrote:
> Can we call this tolerance or "tol" rather than fuzz? Just more consistent
with
> other similar constants.

Done.

https://codereview.chromium.org/2160093002/diff/20001/src/gpu/GrReducedClip.cpp
File src/gpu/GrReducedClip.cpp (right):

https://codereview.chromium.org/2160093002/diff/20001/src/gpu/GrReducedClip.c...
src/gpu/GrReducedClip.cpp:351: // TODO: Some way to indicate there are no
clipIBounds?
On 2016/07/19 13:33:12, bsalomon wrote:
> We could change the InitialState enum to incorporate this perhaps. Or
> alternatively we could return a set of 4 bits to the caller. There would be
one
> bit per edge of clipIBounds, indicating whether the caller must scissor
against
> that edge. This would allow us to merge draws with different clipIBounds.
> 
> E.g. draw 1 needs a right scissor of 20 and the device bounds of draw 2 have a
> right edge of 18. Those could batch with a right scissor edge of 20, but today
> we don't do that. I think this comes up when you have things that run over the
> right or bottom edge of a UI element. So most of the elements need no clip and
a
> few at the edges need a scissor that all the other draws are inside.

The second idea is interesting. It seems like a lot of this method could be
redone to think about the edges independently and it would clean a few things
up.

I did consider addressing this issue further, but then decided that it might be
better suited for its own CL. Are you ok with leaving it at just comment for
now? This is the same API that was there before.

https://codereview.chromium.org/2160093002/diff/20001/src/utils/SkLua.cpp
File src/utils/SkLua.cpp (right):

https://codereview.chromium.org/2160093002/diff/20001/src/utils/SkLua.cpp#new...
src/utils/SkLua.cpp:667: // It absolutely must return these as additional items
on the lua stack in order to be correct.
On 2016/07/19 13:33:12, bsalomon wrote:
> This was used to gather information about multi-element clips. It isn't used
to
> actually implement anything by lua other than dumping the element list. I
don't
> know that anything is necessarily incorrect here.

Done.

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

https://codereview.chromium.org/2160093002/diff/40001/src/gpu/GrReducedClip.c...
src/gpu/GrReducedClip.cpp:372: stackBounds.round(clipIBounds);
This is what started triggering the assert. For a single hard clip rect, we used
to tighten it to the draw boundaries but now we just fix the scissor to the clip
rect itself. The new way ought to show some improvement with batching.

https://codereview.chromium.org/2160093002/diff/40001/src/gpu/gl/GrGLIRect.h
File src/gpu/gl/GrGLIRect.h (left):

https://codereview.chromium.org/2160093002/diff/40001/src/gpu/gl/GrGLIRect.h#...
src/gpu/gl/GrGLIRect.h:59: SkASSERT(fBottom >= 0);
This started asserting because we aren’t always clamping the scissor down to
draw bounds anymore. A scissor with negative origin is perfectly valid, so
unless there's something else that relies on this assert I think we should just
remove it. (As it was, it asserted if the rect crossed the bottom or left RT
boundaries but not the top or right.)

[csmartdalton, 2016-07-19 16:50:22]

Looks like there were quite a few issues that came up with the golds. We can
hold off on reviews until I get them resolved

[csmartdalton, 2016-07-19 22:23:29]

On 2016/07/19 16:50:22, csmartdalton wrote:
> Looks like there were quite a few issues that came up with the golds. We can
> hold off on reviews until I get them resolved

Golds are resolved. The comments have been addressed and it's otherwise ready to
land.

[bsalomon, 2016-07-20 13:34:29]

lgtm

https://codereview.chromium.org/2160093002/diff/20001/src/gpu/GrReducedClip.cpp
File src/gpu/GrReducedClip.cpp (right):

https://codereview.chromium.org/2160093002/diff/20001/src/gpu/GrReducedClip.c...
src/gpu/GrReducedClip.cpp:351: // TODO: Some way to indicate there are no
clipIBounds?
On 2016/07/19 15:59:01, csmartdalton wrote:
> On 2016/07/19 13:33:12, bsalomon wrote:
> > We could change the InitialState enum to incorporate this perhaps. Or
> > alternatively we could return a set of 4 bits to the caller. There would be
> one
> > bit per edge of clipIBounds, indicating whether the caller must scissor
> against
> > that edge. This would allow us to merge draws with different clipIBounds.
> > 
> > E.g. draw 1 needs a right scissor of 20 and the device bounds of draw 2 have
a
> > right edge of 18. Those could batch with a right scissor edge of 20, but
today
> > we don't do that. I think this comes up when you have things that run over
the
> > right or bottom edge of a UI element. So most of the elements need no clip
and
> a
> > few at the edges need a scissor that all the other draws are inside.
> 
> The second idea is interesting. It seems like a lot of this method could be
> redone to think about the edges independently and it would clean a few things
> up.
> 
> I did consider addressing this issue further, but then decided that it might
be
> better suited for its own CL. Are you ok with leaving it at just comment for
> now? This is the same API that was there before.

I think a separate CL is a good idea.

[csmartdalton, 2016-07-20 18:26:30]

The CQ bit was checked by csmartdalton@google.com

[commit-bot: I haz the power, 2016-07-20 18:26:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 18:37:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 18:37:58]

Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on
master.client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[csmartdalton, 2016-07-22 15:39:49]

The CQ bit was checked by csmartdalton@google.com

[csmartdalton, 2016-07-22 15:39:49]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2160093002/#ps140001
(title: "fix tests on ubuntu")

[commit-bot: I haz the power, 2016-07-22 15:39:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-22 15:59:10]

Description was changed from

==========
Allow GrReducedClip to take non-integer query bounds

Fixes places where AA bloat was being conflated with geometric
boundaries and updates GrReducedClip to work with non-integer query
bounds. This allows for better clip reduction with AA shared edges.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2160093002
==========

to

==========
Allow GrReducedClip to take non-integer query bounds

Fixes places where AA bloat was being conflated with geometric
boundaries and updates GrReducedClip to work with non-integer query
bounds. This allows for better clip reduction with AA shared edges.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2160093002

Committed:
https://skia.googlesource.com/skia/+/cbecb082d15da3c9cc5ac7192b243fd26dc29a57
==========

[commit-bot: I haz the power, 2016-07-22 15:59:11]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/cbecb082d15da3c9cc5ac7192b243fd26dc29a57