Implement the y-transpose optimization in image (RGBA) blurs. This gives ~38% performance improvem…

Implement the y-transpose optimization in image (RGBA) blurs.  This gives ~38% performance improvement on my Linux box. Also unify the box blur functions with a template.

R=mtklein@google.com, mtklein, reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12171

[Stephen White, 2013-11-05 22:43:23]

Mike & Mike: PTAL. Thanks!

[Stephen White, 2013-11-05 22:52:21]

Note: this will affect the results of the following GMs:  spritebitmap
imagefiltersgraph imagefilterscropped imagefiltersbase colorfilterimagefilter. 
The differences are a max of 2 each in RGBA.

Those tests are already protected by ignored-tests.txt changes from my last CL,
and I'm protecting them in Blink with the same flag as my previous optimization,
so no bots should go red on this change or the Skia roll.

[mtklein, 2013-11-06 13:51:51]

https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
src/effects/SkBlurImageFilter.cpp:45: 
Maybe add a nice big block comment here with some ASCII art?  Every time I look
at the mask blur code that does this my brain goes all inside out.  I sort of
always go through the same questions, "Wait, you can really walk in different
directions?", "how does it all line up?", etc.  It's somewhat surprising you can
do this at all.  (A link to an external description of this trick would be just
fine by me too... no need to rehash this if it's a commonly known trick.)

https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
src/effects/SkBlurImageFilter.cpp:99: if (srcDirection == kY) {
Should be fine to remove this prefetch entirely now?  It'll only be triggered in
the old code paths.

https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
src/effects/SkBlurImageFilter.cpp:109: static void boxBlurX(const SkPMColor*
src, int srcStride, SkPMColor* dst, int kernelSize,
These are going to want to get renames, eh?  X seems a bit overloaded.  boxBlur
and boxBlurTransposed for XY?

[Stephen White, 2013-11-06 15:09:58]

Thanks for your review.

https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
src/effects/SkBlurImageFilter.cpp:45: 
On 2013/11/06 13:51:52, mtklein wrote:
> Maybe add a nice big block comment here with some ASCII art?  Every time I
look
> at the mask blur code that does this my brain goes all inside out.  I sort of
> always go through the same questions, "Wait, you can really walk in different
> directions?", "how does it all line up?", etc.  It's somewhat surprising you
can
> do this at all.  (A link to an external description of this trick would be
just
> fine by me too... no need to rehash this if it's a commonly known trick.)

Done.

https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
src/effects/SkBlurImageFilter.cpp:99: if (srcDirection == kY) {
On 2013/11/06 13:51:52, mtklein wrote:
> Should be fine to remove this prefetch entirely now?  It'll only be triggered
in
> the old code paths.

Actually, it'll still be used in the Y-only path (when kernelSizeX is 0, below).
So I think we should leave it in.

Although another thing we could try is breaking out each of the if-tests above
into separate loops, as is done in the mask blurs. Then the hardware prefetchers
might work better.

https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
src/effects/SkBlurImageFilter.cpp:109: static void boxBlurX(const SkPMColor*
src, int srcStride, SkPMColor* dst, int kernelSize,
On 2013/11/06 13:51:52, mtklein wrote:
> These are going to want to get renames, eh?  X seems a bit overloaded. 
boxBlur
> and boxBlurTransposed for XY?

We'll still need boxBlurY(), and boxBlurTransposed() makes it hard to line up
the parameters in all the calls below. :) How about boxBlurXX, boxBlurXY,
boxBlurYY?

(I'd also like to try reordering the Y-only passes into YX, XX, XY, for which
I'll need boxBlurYX).

[mtklein, 2013-11-06 15:18:56]

lgtm

https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
src/effects/SkBlurImageFilter.cpp:109: static void boxBlurX(const SkPMColor*
src, int srcStride, SkPMColor* dst, int kernelSize,
On 2013/11/06 15:09:59, Stephen White wrote:
> On 2013/11/06 13:51:52, mtklein wrote:
> > These are going to want to get renames, eh?  X seems a bit overloaded. 
> boxBlur
> > and boxBlurTransposed for XY?
> 
> We'll still need boxBlurY(), and boxBlurTransposed() makes it hard to line up
> the parameters in all the calls below. :) How about boxBlurXX, boxBlurXY,
> boxBlurYY?
> 
> (I'd also like to try reordering the Y-only passes into YX, XX, XY, for which
> I'll need boxBlurYX).

Maybe just get rid of these intermediates and call boxBlur<Src,Dst> directly?

https://codereview.chromium.org/59133006/diff/90001/src/effects/SkBlurImageFi...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/59133006/diff/90001/src/effects/SkBlurImageFi...
src/effects/SkBlurImageFilter.cpp:59: *                                        
+---+       +---+       +---+
This is awesome.  Thank you.

[Stephen White, 2013-11-06 15:25:26]

On 2013/11/06 15:18:56, mtklein wrote:
> lgtm
> 
>
https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
> File src/effects/SkBlurImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
> src/effects/SkBlurImageFilter.cpp:109: static void boxBlurX(const SkPMColor*
> src, int srcStride, SkPMColor* dst, int kernelSize,
> On 2013/11/06 15:09:59, Stephen White wrote:
> > On 2013/11/06 13:51:52, mtklein wrote:
> > > These are going to want to get renames, eh?  X seems a bit overloaded. 
> > boxBlur
> > > and boxBlurTransposed for XY?
> > 
> > We'll still need boxBlurY(), and boxBlurTransposed() makes it hard to line
up
> > the parameters in all the calls below. :) How about boxBlurXX, boxBlurXY,
> > boxBlurYY?
> > 
> > (I'd also like to try reordering the Y-only passes into YX, XX, XY, for
which
> > I'll need boxBlurYX).
> 
> Maybe just get rid of these intermediates and call boxBlur<Src,Dst> directly?

Heh.. I tried that (see patch set 6), but I found it less clear.

>
https://codereview.chromium.org/59133006/diff/90001/src/effects/SkBlurImageFi...
> File src/effects/SkBlurImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/59133006/diff/90001/src/effects/SkBlurImageFi...
> src/effects/SkBlurImageFilter.cpp:59: *                                       

> +---+       +---+       +---+
> This is awesome.  Thank you.

[mtklein, 2013-11-06 15:29:08]

On 2013/11/06 15:25:26, Stephen White wrote:
> On 2013/11/06 15:18:56, mtklein wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
> > File src/effects/SkBlurImageFilter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
> > src/effects/SkBlurImageFilter.cpp:109: static void boxBlurX(const SkPMColor*
> > src, int srcStride, SkPMColor* dst, int kernelSize,
> > On 2013/11/06 15:09:59, Stephen White wrote:
> > > On 2013/11/06 13:51:52, mtklein wrote:
> > > > These are going to want to get renames, eh?  X seems a bit overloaded. 
> > > boxBlur
> > > > and boxBlurTransposed for XY?
> > > 
> > > We'll still need boxBlurY(), and boxBlurTransposed() makes it hard to line
> up
> > > the parameters in all the calls below. :) How about boxBlurXX, boxBlurXY,
> > > boxBlurYY?
> > > 
> > > (I'd also like to try reordering the Y-only passes into YX, XX, XY, for
> which
> > > I'll need boxBlurYX).
> > 
> > Maybe just get rid of these intermediates and call boxBlur<Src,Dst>
directly?
> 
> Heh.. I tried that (see patch set 6), but I found it less clear.
> 
> >
>
https://codereview.chromium.org/59133006/diff/90001/src/effects/SkBlurImageFi...
> > File src/effects/SkBlurImageFilter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/59133006/diff/90001/src/effects/SkBlurImageFi...
> > src/effects/SkBlurImageFilter.cpp:59: *                                     
 
> 
> > +---+       +---+       +---+
> > This is awesome.  Thank you.

Maybe "boxBlurStraight " and "boxBlurTranspose"?  (Note the alignment space.)  X
and Y are just too darn similar looking, eh?

[mtklein, 2013-11-06 15:37:28]

On 2013/11/06 15:29:08, mtklein wrote:
> On 2013/11/06 15:25:26, Stephen White wrote:
> > On 2013/11/06 15:18:56, mtklein wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
> > > File src/effects/SkBlurImageFilter.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/59133006/diff/50001/src/effects/SkBlurImageFi...
> > > src/effects/SkBlurImageFilter.cpp:109: static void boxBlurX(const
SkPMColor*
> > > src, int srcStride, SkPMColor* dst, int kernelSize,
> > > On 2013/11/06 15:09:59, Stephen White wrote:
> > > > On 2013/11/06 13:51:52, mtklein wrote:
> > > > > These are going to want to get renames, eh?  X seems a bit overloaded.

> > > > boxBlur
> > > > > and boxBlurTransposed for XY?
> > > > 
> > > > We'll still need boxBlurY(), and boxBlurTransposed() makes it hard to
line
> > up
> > > > the parameters in all the calls below. :) How about boxBlurXX,
boxBlurXY,
> > > > boxBlurYY?
> > > > 
> > > > (I'd also like to try reordering the Y-only passes into YX, XX, XY, for
> > which
> > > > I'll need boxBlurYX).
> > > 
> > > Maybe just get rid of these intermediates and call boxBlur<Src,Dst>
> directly?
> > 
> > Heh.. I tried that (see patch set 6), but I found it less clear.
> > 
> > >
> >
>
https://codereview.chromium.org/59133006/diff/90001/src/effects/SkBlurImageFi...
> > > File src/effects/SkBlurImageFilter.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/59133006/diff/90001/src/effects/SkBlurImageFi...
> > > src/effects/SkBlurImageFilter.cpp:59: *                                   
 
>  
> > 
> > > +---+       +---+       +---+
> > > This is awesome.  Thank you.
> 
> Maybe "boxBlurStraight " and "boxBlurTranspose"?  (Note the alignment space.) 
X
> and Y are just too darn similar looking, eh?

Ugh.  I wasn't thinking about all four combinations.  I have no good names for
those.  I'm happy to stop bikeshedding and go with whichever names you like. 
still lgtm at 7

[Stephen White, 2013-11-07 18:35:15]

Committed patchset #7 manually as r12171 (presubmit successful).