Fix premul alpha problems w/matrix convolution filter

Clamp the generated rgb channels to the generated alpha, and re-enable the problematic bench.

[reed1, 2012-09-20 12:37:49]

http://codereview.appspot.com/6541043/diff/1/src/effects/SkMatrixConvolutionI...
File src/effects/SkMatrixConvolutionImageFilter.cpp (right):

http://codereview.appspot.com/6541043/diff/1/src/effects/SkMatrixConvolutionI...
src/effects/SkMatrixConvolutionImageFilter.cpp:100: template<class PixelFetcher>
Does this class support the "applies only to RGB" option?

http://codereview.appspot.com/6541043/diff/1/src/effects/SkMatrixConvolutionI...
src/effects/SkMatrixConvolutionImageFilter.cpp:109: SkScalar k = fKernel[cy *
fKernelSize.fWidth + cx];
Have you timed storing some fixed-point version of the kernel values? That would
let you avoid 4xNxN int-to-float conversions.

You could do the same trick w/ fGain and fBias, to save the float-to-int
conversions, though there are NxN fewer of those...

[Stephen White, 2012-09-20 13:27:46]

http://codereview.appspot.com/6541043/diff/1/src/effects/SkMatrixConvolutionI...
File src/effects/SkMatrixConvolutionImageFilter.cpp (right):

http://codereview.appspot.com/6541043/diff/1/src/effects/SkMatrixConvolutionI...
src/effects/SkMatrixConvolutionImageFilter.cpp:100: template<class PixelFetcher>
On 2012/09/20 12:37:49, reed1 wrote:
> Does this class support the "applies only to RGB" option?

Not yet.  (See the comment about "preserveAlpha" in the first checkin).  I plan
to do it in another patch.

http://codereview.appspot.com/6541043/diff/1/src/effects/SkMatrixConvolutionI...
src/effects/SkMatrixConvolutionImageFilter.cpp:109: SkScalar k = fKernel[cy *
fKernelSize.fWidth + cx];
On 2012/09/20 12:37:49, reed1 wrote:
> Have you timed storing some fixed-point version of the kernel values? That
would
> let you avoid 4xNxN int-to-float conversions.

You mean, and do all the math in fixed?  No, I haven't.  Now that I have a
bench, at least I could.  :)

As an aside, I realize the conversions used to god-awful slow on x87, but aren't
they relatively fast now?  SSE2 has an instruction for it, and for x87, most
libc's that I know of are using that round-twice-and-average hack that avoids
changing the rounding mode and pipeline flush.

[reed1, 2012-09-20 13:58:30]

By default I think we still get x87 code gen on windows at least, unless you
explicitly move this code into an SSE2 file or something. I think all-int math
might just remove the need for that complication, and would work well on ARM as
well.

If you aren't going to do the premul or fixed-point code in this CL, lets create
Issues to track those two things.

[Stephen White, 2012-09-20 14:07:20]

On 2012/09/20 13:58:30, reed1 wrote:
> By default I think we still get x87 code gen on windows at least, unless you
> explicitly move this code into an SSE2 file or something. I think all-int math
> might just remove the need for that complication, and would work well on ARM
as
> well.

IIRC, MSVC's libc ftol() does a runtime SSE2 check and then does the SSE2
instruction or the x87 hack.  Of course, it's still a function call, so it could
be faster (but at least it's not two pipeline flushes.  :) )

> If you aren't going to do the premul or fixed-point code in this CL, lets
create
> Issues to track those two things.

Done.
https://code.google.com/p/skia/issues/detail?id=898
https://code.google.com/p/skia/issues/detail?id=899

[reed1, 2012-09-20 15:41:02]

lgtm

[Stephen White, 2012-09-20 19:10:45]

On 2012/09/20 15:41:02, reed1 wrote:
> lgtm

Landed as r5610; closing.