Clip in grdrawtarget

Clip in grdrawtarget

NOTREECHECKS=true
BUG=skia:

Committed: https://skia.googlesource.com/skia/+/2c93efeb6f2dd652eb2575c04124d82952f020c7

[joshualitt, 2014-10-31 00:17:17]

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

[joshualitt, 2014-10-31 00:17:17]

I won't check this in until after the branch.

[joshualitt, 2014-11-05 16:46:37]

Please take a look when you have a moment, I plan on trying to land this
tomorrow after the branch.

[bsalomon, 2014-11-05 19:13:45]

https://codereview.chromium.org/685883003/diff/160001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/685883003/diff/160001/include/gpu/GrContext.h...
include/gpu/GrContext.h:874: GrDrawTarget* getDrawTarget();
If this is for testing then I think it should be named in a way that indicates
it is for testing. Also, this should be defined in GrTest.cpp so that we can't
call it in a normal Skia lib build without getting a linker error.

Also, can we just use getTestTarget() for this use case rather than adding this
accessor?

https://codereview.chromium.org/685883003/diff/160001/src/gpu/GrInOrderDrawBu...
File src/gpu/GrInOrderDrawBuffer.cpp (left):

https://codereview.chromium.org/685883003/diff/160001/src/gpu/GrInOrderDrawBu...
src/gpu/GrInOrderDrawBuffer.cpp:179: bool
GrInOrderDrawBuffer::quickInsideClip(const SkRect& devBounds) {
It'll be interesting to see what happens w.r.t. to perf by removing this. I
think invoking GrCMM early gives us the same batching benefit, but we'll have to
see.

https://codereview.chromium.org/685883003/diff/160001/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.chromium.org/685883003/diff/160001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:377: GrGpuGL* gpu =
static_cast<GrGpuGL*>(fContext->getGpu());
Does this have to be the GL subclass?

[joshualitt, 2014-11-05 19:55:47]

feedback inc

https://codereview.chromium.org/685883003/diff/160001/src/gpu/GrInOrderDrawBu...
File src/gpu/GrInOrderDrawBuffer.cpp (left):

https://codereview.chromium.org/685883003/diff/160001/src/gpu/GrInOrderDrawBu...
src/gpu/GrInOrderDrawBuffer.cpp:179: bool
GrInOrderDrawBuffer::quickInsideClip(const SkRect& devBounds) {
On 2014/11/05 19:13:43, bsalomon wrote:
> It'll be interesting to see what happens w.r.t. to perf by removing this. I
> think invoking GrCMM early gives us the same batching benefit, but we'll have
to
> see.

was there a place we were actually calling this?

https://codereview.chromium.org/685883003/diff/160001/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.chromium.org/685883003/diff/160001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:377: GrGpuGL* gpu =
static_cast<GrGpuGL*>(fContext->getGpu());
On 2014/11/05 19:13:43, bsalomon wrote:
> Does this have to be the GL subclass?

Otherwise we can't build the GL program and we need access to the GLCaps.

[bsalomon, 2014-11-05 20:00:22]

https://codereview.chromium.org/685883003/diff/160001/src/gpu/GrInOrderDrawBu...
File src/gpu/GrInOrderDrawBuffer.cpp (left):

https://codereview.chromium.org/685883003/diff/160001/src/gpu/GrInOrderDrawBu...
src/gpu/GrInOrderDrawBuffer.cpp:179: bool
GrInOrderDrawBuffer::quickInsideClip(const SkRect& devBounds) {
On 2014/11/05 19:55:47, joshualitt wrote:
> On 2014/11/05 19:13:43, bsalomon wrote:
> > It'll be interesting to see what happens w.r.t. to perf by removing this. I
> > think invoking GrCMM early gives us the same batching benefit, but we'll
have
> to
> > see.
> 
> was there a place we were actually calling this?

void GrInOrderDrawBuffer::onDraw(const DrawInfo& info) {

    GeometryPoolState& poolState = fGeoPoolStateStack.back();
    const GrDrawState& drawState = this->getDrawState();
    AutoClipReenable acr;

    if (drawState.isClipState() &&
        info.getDevBounds() &&
        this->quickInsideClip(*info.getDevBounds())) {
        acr.set(this->drawState());
    }


It helped a lot with batching rects.

https://codereview.chromium.org/685883003/diff/160001/tests/GLProgramsTest.cpp
File tests/GLProgramsTest.cpp (right):

https://codereview.chromium.org/685883003/diff/160001/tests/GLProgramsTest.cp...
tests/GLProgramsTest.cpp:377: GrGpuGL* gpu =
static_cast<GrGpuGL*>(fContext->getGpu());
On 2014/11/05 19:55:47, joshualitt wrote:
> On 2014/11/05 19:13:43, bsalomon wrote:
> > Does this have to be the GL subclass?
> 
> Otherwise we can't build the GL program and we need access to the GLCaps.

Gotcha, seems like we're pretty close to making this be backend-independent, but
not quite there yet.

[bsalomon, 2014-11-06 20:34:59]

lgtm

[joshualitt, 2014-11-06 20:36:03]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-11-06 20:36:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/685883003/70010

[joshualitt, 2014-11-06 20:56:55]

The CQ bit was unchecked by joshualitt@google.com

[joshualitt, 2014-11-06 20:56:55]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2014-11-06 20:57:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/685883003/70010

[commit-bot: I haz the power, 2014-11-06 20:57:17]

Committed patchset #13 (id:70010) as 2c93efeb6f2dd652eb2575c04124d82952f020c7