GPU blit speedup: avoid texture filtering and texture domain when not necessary

Closed with r3421

[junov1, 2012-03-15 18:47:49]

Significant bitmap blit speedup for common use cases in GPU rendering path.

By being more selective about cases that require the use of the texture domain
feature, we can avoid unnecessary overhead from uploading uniforms.

In cases where texture sampling is texel-aligned, we can turn off texture
filtering which is useless in that case.  Switching from bilinear to nearest
should help performance on several low-end embedded GPUs.

[TomH, 2012-03-15 18:53:54]

http://codereview.appspot.com/5836047/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5836047/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1267: if (m.rectStaysRect() && useFiltering) {
Great low-level comments through here, but this higher level conditional isn't
clear to me. You're covering is or is not rect-stays-rect, but otherwise if
!useFiltering none of these flags get set? Why even call the function in the
first place, then?

(Passing booleans to functions to tell them what to do is a mild code smell,
right?)

http://codereview.appspot.com/5836047/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1398: sampler->setTextureDomain(GrRect::MakeEmpty());
As I read the code flow, this will always happen if !useFiltering?

[Stephen White, 2012-03-15 19:25:34]

http://codereview.appspot.com/5836047/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5836047/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:57: // This constant represents the screen alignment
criterion for using texture
You might want to say "in texels" or "in source pixels" (or just "in pixels",
since we're 1:1 anyway).

http://codereview.appspot.com/5836047/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1281: caveats |= kSamplingCaveat_UnalignedSamples;
It seems like UnalignedSamples is already established by this point (without
knowing isSubRect), so we could have separate functions for unaligned samples
and color bleeding, and have the caller check isSubRect to know if they need to
check for color bleeding.

[bsalomon, 2012-03-15 19:38:16]

Maybe a way of addressing Tom and Stephen's comments would be to break the new
function up into two: one that determines bleeding and one that determines
alignment. Then perhaps it would be clearer to get rid of the bitfield and only
call the functions when filtering/issubrect deems it necessary.

I'm wondering if the float->int conversions show up in a profile?

http://codereview.appspot.com/5836047/diff/2001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5836047/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1255: kSamplingCaveat_None = 0,
nit: the usual style in skia would be kNone_SamplingCaveat

[Stephen White, 2012-03-15 19:51:14]

On 2012/03/15 19:38:16, bsalomon wrote:
> Maybe a way of addressing Tom and Stephen's comments would be to break the new
> function up into two: one that determines bleeding and one that determines
> alignment.

Yes, that's what I was trying to suggest (hobbled as usual by my tortuous
grammar).

> Then perhaps it would be clearer to get rid of the bitfield and only
> call the functions when filtering/issubrect deems it necessary.
> 
> I'm wondering if the float->int conversions show up in a profile?
> 
> http://codereview.appspot.com/5836047/diff/2001/src/gpu/SkGpuDevice.cpp
> File src/gpu/SkGpuDevice.cpp (right):
> 
>
http://codereview.appspot.com/5836047/diff/2001/src/gpu/SkGpuDevice.cpp#newco...
> src/gpu/SkGpuDevice.cpp:1255: kSamplingCaveat_None = 0,
> nit: the usual style in skia would be kNone_SamplingCaveat

[junov1, 2012-03-15 20:22:21]

New patch.

[Stephen White, 2012-03-15 20:49:28]

http://codereview.appspot.com/5836047/diff/3/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5836047/diff/3/src/gpu/SkGpuDevice.cpp#newcode1349
src/gpu/SkGpuDevice.cpp:1349: bool needsTextureDomain = false;
I would actually invert this logic:  initialize needsTextureDomain to true here,
then only turn it off when you're sure:

http://codereview.appspot.com/5836047/diff/3/src/gpu/SkGpuDevice.cpp#newcode1357
src/gpu/SkGpuDevice.cpp:1357:
sampler->setFilter(GrSamplerState::kNearest_Filter);
needsTextureDomain = false;

http://codereview.appspot.com/5836047/diff/3/src/gpu/SkGpuDevice.cpp#newcode1363
src/gpu/SkGpuDevice.cpp:1363: needsTextureDomain = true;
... and delete this.  No change in logic, but it feels more conservative that
way.

http://codereview.appspot.com/5836047/diff/3/src/gpu/SkGpuDevice.cpp#newcode1367
src/gpu/SkGpuDevice.cpp:1367: needsTextureDomain &&= srcRect.width() <
bitmap.width() ||
Actually, come to think of it, if you initialize needsTextureDomain to be this
test first (srcRect is a subset), then you could use the short-circuiting && to
possibly skip the call to mayColorBleed().

[junov1, 2012-03-15 21:40:52]

New patch.

[bsalomon, 2012-03-16 13:14:32]

Mostly LGTM. Do you think there would be an advantage to doing the checks
outside of the loop that calls internalDrawBitmap?

http://codereview.appspot.com/5836047/diff/6002/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5836047/diff/6002/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1365: needsTextureDomain &&=
Is there such a thing as &&=?

[junov1, 2012-03-16 14:03:59]

On 2012/03/16 13:14:32, bsalomon wrote:
> Mostly LGTM. Do you think there would be an advantage to doing the checks
> outside of the loop that calls internalDrawBitmap?

Not really because when we are tiling, tiles that encroach the border of the
subrect boundary may require the texture domain feature, while other tiles do
not.  We could however test pixel alignment in drawBitmap instead of
internalDrawBitmap, but I think that would be over-optimizating at the expense
of readability.  Blitting large textures is more likely to be fill rate bound
that CPU bound, so I won't lose any sleep over a bit of per-tile overhead.

> Is there such a thing as &&=?

Nope.  wishful thinking on my part :-)
The last patch I uploaded actually compiles! It also passes gm and unit tests. 
I will run it through layout tests next.

[bsalomon, 2012-03-16 14:17:36]

Some trivial comments below that you can commit with or without as far as I'm
concerned. So LGTM.

http://codereview.appspot.com/5836047/diff/8002/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

http://codereview.appspot.com/5836047/diff/8002/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1272: // So we can assume that sampling is axis aligned
but not texel aligned.
Want to stick an assert here GrAssert(!hasAlignedSamples(srcrect,
transformedRect))?

http://codereview.appspot.com/5836047/diff/8002/src/gpu/SkGpuDevice.cpp#newco...
src/gpu/SkGpuDevice.cpp:1340: bool needsTextureDomain;
very minor comment: would it improve readability to set needsTextureDomain=false
here rather than have the else clause?  I assume that either the overhead of
double setting is not worth worrying about or that the compiler can effectively
turn it into an else.

It would match the flow below with textureDomain set initially empty