2D kernel initial wiring for Guassian

2D kernel initial wiring for Guassian

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/5acfea789d39106dbc706c6ee85cd8f027fce3ed

[joshua.litt, 2014-07-25 20:31:41]

Initial changes.  The kernel seems to marginally outperform the separable case @
3x3, but @ 5x5 its already a wash.

[Stephen White, 2014-07-25 20:43:18]

Some initial comments; will do more extensive review later.

https://codereview.chromium.org/418223009/diff/20001/gm/imageblur2.cpp
File gm/imageblur2.cpp (right):

https://codereview.chromium.org/418223009/diff/20001/gm/imageblur2.cpp#newcode16
gm/imageblur2.cpp:16: #include <stdio.h>
Is this needed? If so, put it with the other #includes above.

https://codereview.chromium.org/418223009/diff/20001/gm/imageblur2.cpp#newcode18
gm/imageblur2.cpp:18: // TODO this surely exists somewhere
Try SK_ARRAY_COUNT.

https://codereview.chromium.org/418223009/diff/20001/gm/imageblur2.cpp#newcode35
gm/imageblur2.cpp:35: class ImageBlur2GM : public GM {
Let's call it BlurImageFilter (like the other image filter GMs).

https://codereview.chromium.org/418223009/diff/20001/src/gpu/effects/GrMatrix...
File src/gpu/effects/GrMatrixConvolutionEffect.cpp (right):

https://codereview.chromium.org/418223009/diff/20001/src/gpu/effects/GrMatrix...
src/gpu/effects/GrMatrixConvolutionEffect.cpp:269: // is dropped here, since we
renormalize the kernel below.
I'm not fond of baking the Gaussian-specific stuff into the 2D convolution.
Could we keep it generic, and have the caller create a Gaussian with it?

[joshua.litt, 2014-07-28 14:57:53]

Feedback incorporated.

Create Gaussian now uses the generic constructor.  This requires an extra copy
of the kernel, which is a bit unfortunate.

On 2014/07/25 20:43:18, Stephen White wrote:
> Some initial comments; will do more extensive review later.
> 
> https://codereview.chromium.org/418223009/diff/20001/gm/imageblur2.cpp
> File gm/imageblur2.cpp (right):
> 
>
https://codereview.chromium.org/418223009/diff/20001/gm/imageblur2.cpp#newcode16
> gm/imageblur2.cpp:16: #include <stdio.h>
> Is this needed? If so, put it with the other #includes above.
> 
>
https://codereview.chromium.org/418223009/diff/20001/gm/imageblur2.cpp#newcode18
> gm/imageblur2.cpp:18: // TODO this surely exists somewhere
> Try SK_ARRAY_COUNT.
> 
>
https://codereview.chromium.org/418223009/diff/20001/gm/imageblur2.cpp#newcode35
> gm/imageblur2.cpp:35: class ImageBlur2GM : public GM {
> Let's call it BlurImageFilter (like the other image filter GMs).
> 
>
https://codereview.chromium.org/418223009/diff/20001/src/gpu/effects/GrMatrix...
> File src/gpu/effects/GrMatrixConvolutionEffect.cpp (right):
> 
>
https://codereview.chromium.org/418223009/diff/20001/src/gpu/effects/GrMatrix...
> src/gpu/effects/GrMatrixConvolutionEffect.cpp:269: // is dropped here, since
we
> renormalize the kernel below.
> I'm not fond of baking the Gaussian-specific stuff into the 2D convolution.
> Could we keep it generic, and have the caller create a Gaussian with it?

[bsalomon, 2014-07-28 15:20:42]

https://codereview.chromium.org/418223009/diff/40001/src/effects/SkGpuBlurUti...
File src/effects/SkGpuBlurUtils.cpp (right):

https://codereview.chromium.org/418223009/diff/40001/src/effects/SkGpuBlurUti...
src/effects/SkGpuBlurUtils.cpp:65: // a 2d gaussian convolution pass
Maybe we should give both these functions symmetrical names? E.g.;

convolve_gaussian_1d()
convolve_gaussian_2d()

or something along those lines.

https://codereview.chromium.org/418223009/diff/40001/src/effects/SkGpuBlurUti...
src/effects/SkGpuBlurUtils.cpp:224: // are okay too but for simplicity only
handle the square case for now
is the squareness enforced here?

https://codereview.chromium.org/418223009/diff/40001/src/effects/SkGpuBlurUti...
src/effects/SkGpuBlurUtils.cpp:234: if (scaleFactorY > 1) {
Can we get here with scaleFactorY > 1? I thought that was only used for large
sigmas.

https://codereview.chromium.org/418223009/diff/40001/src/effects/SkGpuBlurUti...
src/effects/SkGpuBlurUtils.cpp:252: else {
style nit, else goes with }

https://codereview.chromium.org/418223009/diff/40001/src/gpu/effects/GrMatrix...
File src/gpu/effects/GrMatrixConvolutionEffect.cpp (right):

https://codereview.chromium.org/418223009/diff/40001/src/gpu/effects/GrMatrix...
src/gpu/effects/GrMatrixConvolutionEffect.cpp:171: uint32_t key =
encodeXY(m.kernelSize().width(), m.kernelSize().height());
When this was originally written we had a really restricted range bit range for
effect keys. It might time to simplify this to be more straight forward.

https://codereview.chromium.org/418223009/diff/40001/src/gpu/effects/GrMatrix...
src/gpu/effects/GrMatrixConvolutionEffect.cpp:231:
this->setWillNotUseInputColor();
I think if we're using this for mask filter blur then not reading the input
color is now longer valid. We should not set this here and make this effect
multiply its computed color by the input color in emitCode.

https://codereview.chromium.org/418223009/diff/40001/src/gpu/effects/GrMatrix...
File src/gpu/effects/GrMatrixConvolutionEffect.h (right):

https://codereview.chromium.org/418223009/diff/40001/src/gpu/effects/GrMatrix...
src/gpu/effects/GrMatrixConvolutionEffect.h:23: //TODO expand texture domain
class to add ClampToColor and Repeat and then clean this up
It has clamp to black already I believe.

[joshua.litt, 2014-08-11 17:07:55]

feedback incorporated.  I'm going to debug on android.  I wonder if those
failures are related to the utilization of the input color.

[bsalomon, 2014-08-11 17:39:21]

On 2014/08/11 17:07:55, joshua.litt wrote:
> feedback incorporated.  I'm going to debug on android.  I wonder if those
> failures are related to the utilization of the input color.

Might just be imperceptible differences.

[bsalomon, 2014-08-11 17:39:23]

On 2014/08/11 17:07:55, joshua.litt wrote:
> feedback incorporated.  I'm going to debug on android.  I wonder if those
> failures are related to the utilization of the input color.

Might just be imperceptible differences.

[bsalomon, 2014-08-11 18:49:09]

lgtm

[joshua.litt, 2014-08-11 18:59:47]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-08-11 19:00:40]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/joshualitt@chromium.org/418223009/80001

[commit-bot: I haz the power, 2014-08-11 19:00:44]

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

[commit-bot: I haz the power, 2014-08-11 19:00:45]

Failed to apply patch for expectations/gm/ignored-tests.txt:
While running git apply --index -p1;
  error: patch failed: expectations/gm/ignored-tests.txt:37
  error: expectations/gm/ignored-tests.txt: patch does not apply

Patch:       expectations/gm/ignored-tests.txt
Index: expectations/gm/ignored-tests.txt
diff --git a/expectations/gm/ignored-tests.txt
b/expectations/gm/ignored-tests.txt
index
90514b22a0503f5682d81eb7b2867e18830fd244..8565515bd8a2a0d8db1a64d07387db547a4b9521
100644
--- a/expectations/gm/ignored-tests.txt
+++ b/expectations/gm/ignored-tests.txt
@@ -37,3 +37,5 @@
 #batching
 bleed
 
+#joshualitt
+matrixconvolution

[joshua.litt, 2014-08-11 19:09:49]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-08-11 19:10:39]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/joshualitt@chromium.org/418223009/100001

[commit-bot: I haz the power, 2014-08-11 19:19:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)

[commit-bot: I haz the power, 2014-08-11 19:25:42]

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

[commit-bot: I haz the power, 2014-08-11 19:25:45]

Try jobs failed on following builders:
  Build-Win-VS2013-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win-VS2013-x86-Debug-Trybot/builds...)

[joshua.litt, 2014-08-11 20:47:07]

The CQ bit was checked by joshualitt@chromium.org

[commit-bot: I haz the power, 2014-08-11 20:47:31]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/joshualitt@chromium.org/418223009/160001

[commit-bot: I haz the power, 2014-08-11 20:55:39]

Change committed as 5acfea789d39106dbc706c6ee85cd8f027fce3ed