Allow negative values in SkBlurImageFilter sigma.

Allow negative values in SkBlurImageFilter sigma.

There are two ways negative sigma values may occur: in
the original filter parameters, or after multiplication 
by a negative scaling CTM. The former case is 
invalid according to the spec, so we continue to check
for it at validation time. In the latter case, we should
interpret it as a horizontal flip in the kernel pixel
access, and simply take the absolute value (since the 
filter kernel is symmetric).

Also refactor all this logic into a single place for the
CPU, GPU and onFilterBounds() paths.

BUG=https://code.google.com/p/chromium/issues/detail?id=409602

Committed: https://skia.googlesource.com/skia/+/32673b99a4fb5d798206eb7665b730ed0b4597a0

[Stephen White, 2014-09-09 15:18:09]

senorblanco@chromium.org changed reviewers:
+ sugoi@google.com

[Stephen White, 2014-09-09 15:18:18]

sugoi@: PTAL. Thanks!

[reed1, 2014-09-09 15:37:38]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2014-09-09 15:37:38]

I think taking abs after the CTM is totally correct.

I don't see how that need extend to allowing negative values in our Create
functions (or when reading a stream). I think being strict there is still a good
idea.

https://codereview.chromium.org/555603002/diff/40001/src/effects/SkBlurImageF...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/555603002/diff/40001/src/effects/SkBlurImageF...
src/effects/SkBlurImageFilter.cpp:31: if (sigma.fX < 0) {
SkScalarAbs()

[sugoi1, 2014-09-09 15:42:39]

sugoi@chromium.org changed reviewers:
+ sugoi@chromium.org

[sugoi1, 2014-09-09 15:42:40]

https://codereview.chromium.org/555603002/diff/40001/src/effects/SkBlurImageF...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/555603002/diff/40001/src/effects/SkBlurImageF...
src/effects/SkBlurImageFilter.cpp:30: sigma.fY = SkMinScalar(sigma.fY,
MAX_SIGMA);
Shouldn't this happen after line 37? What if sigma.fX is a very large negative
number?

Or why not just replace lines 29 to 37 by :
sigma.fX = SkMinScalar(SkScalarAbs(sigma.fX), MAX_SIGMA);
sigma.fY = SkMinScalar(SkScalarAbs(sigma.fY), MAX_SIGMA);

[Stephen White, 2014-09-09 15:48:15]

https://codereview.chromium.org/555603002/diff/40001/src/effects/SkBlurImageF...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/555603002/diff/40001/src/effects/SkBlurImageF...
src/effects/SkBlurImageFilter.cpp:31: if (sigma.fX < 0) {
On 2014/09/09 15:37:38, reed1 wrote:
> SkScalarAbs() 

Done.

[Stephen White, 2014-09-09 15:48:45]

On 2014/09/09 15:37:38, reed1 wrote:
> I think taking abs after the CTM is totally correct.
> 
> I don't see how that need extend to allowing negative values in our Create
> functions (or when reading a stream). I think being strict there is still a
good
> idea.

OK, I've restored that check.

>
https://codereview.chromium.org/555603002/diff/40001/src/effects/SkBlurImageF...
> File src/effects/SkBlurImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/555603002/diff/40001/src/effects/SkBlurImageF...
> src/effects/SkBlurImageFilter.cpp:31: if (sigma.fX < 0) {
> SkScalarAbs()

[Stephen White, 2014-09-09 15:54:00]

New patch up: PTAL. Thanks!

[reed1, 2014-09-09 15:59:54]

lgtm

[Stephen White, 2014-09-09 16:03:59]

https://codereview.chromium.org/555603002/diff/40001/src/effects/SkBlurImageF...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/555603002/diff/40001/src/effects/SkBlurImageF...
src/effects/SkBlurImageFilter.cpp:30: sigma.fY = SkMinScalar(sigma.fY,
MAX_SIGMA);
On 2014/09/09 15:42:39, sugoi1 wrote:
> Shouldn't this happen after line 37? What if sigma.fX is a very large negative
> number?
> 
> Or why not just replace lines 29 to 37 by :
> sigma.fX = SkMinScalar(SkScalarAbs(sigma.fX), MAX_SIGMA);
> sigma.fY = SkMinScalar(SkScalarAbs(sigma.fY), MAX_SIGMA);

Good catch. Fixed.

[Stephen White, 2014-09-09 16:07:39]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2014-09-09 16:08:13]

CQ is trying da patch. Follow status at

https://skia-tree-status.appspot.com/cq/senorblanco@chromium.org/555603002/80001

[commit-bot: I haz the power, 2014-09-09 16:15:12]

Committed patchset #5 (id:80001) as 32673b99a4fb5d798206eb7665b730ed0b4597a0