SkBlurImageFilter returns input when sigma = 0

SkBlurImageFilter returns input when sigma = 0

In the current implementation, a blur filter is always created even in the
case when sigma.fX == 0 && sigma.fY == 0. This CL makes the blur filter
return input in this case.

BUG=568393
Committed: https://skia.googlesource.com/skia/+/467ddc0b24a63ee1525fa18d1dcf62e47975588a

[xidachen, 2015-12-10 03:00:23]

xidachen@chromium.org changed reviewers:
+ junov@chromium.org, senorblanco@chromium.org

[xidachen, 2015-12-10 03:00:24]

PTAL

[reed1, 2015-12-10 13:36:49]

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

[reed1, 2015-12-10 13:36:49]

Interesting general question about our factories:

If we think a given effect (maskfilter, colorfilter, etc.) is a no-op, should we
return null to the caller?

https://codereview.chromium.org/1518643002/diff/1/src/effects/SkDropShadowIma...
File src/effects/SkDropShadowImageFilter.cpp (right):

https://codereview.chromium.org/1518643002/diff/1/src/effects/SkDropShadowIma...
src/effects/SkDropShadowImageFilter.cpp:81: if (sigma.fX != 0 || sigma.fY != 0)
It seems that for the most part, if the sigmas are zero, they will always have
been zero (i.e. at Create factory time).

Perhaps the factory itself should check for zero sigmas, and return an
OffsetImageFilter instead of a DropShaderImageFilter...

https://codereview.chromium.org/1518643002/diff/1/src/effects/SkDropShadowIma...
src/effects/SkDropShadowImageFilter.cpp:86: if (blurFilter)
This check seems unnecessary, since line 87 will execute correctly either way.

[Stephen White, 2015-12-10 14:57:18]

https://codereview.chromium.org/1518643002/diff/1/src/effects/SkDropShadowIma...
File src/effects/SkDropShadowImageFilter.cpp (right):

https://codereview.chromium.org/1518643002/diff/1/src/effects/SkDropShadowIma...
src/effects/SkDropShadowImageFilter.cpp:81: if (sigma.fX != 0 || sigma.fY != 0)
On 2015/12/10 13:36:49, reed1 wrote:
> It seems that for the most part, if the sigmas are zero, they will always have
> been zero (i.e. at Create factory time).
> 
> Perhaps the factory itself should check for zero sigmas, and return an
> OffsetImageFilter instead of a DropShaderImageFilter...

If it's drawing shadow & foreground, an offset may not be enough. It'd need a
composite as well. And you'd have to check for crop rect.

For this particular case, I'd prefer that this was done in
SkBlurImageFilter::Create(); something like:

if (sigma.fX == 0 && sigma.fY == 0 && nullptr == cropRect) {
    return input;
}

Would that work?

Note that we may have to check the call sites in Chrome to ensure they aren't
expecting a non-null pointer (running the blink layout tests should be enough).

[Stephen White, 2015-12-10 14:57:39]

On 2015/12/10 13:36:49, reed1 wrote:
> Interesting general question about our factories:
> 
> If we think a given effect (maskfilter, colorfilter, etc.) is a no-op, should
we
> return null to the caller?

I think we should.

[reed1, 2015-12-10 15:05:04]

Good point about fg/bg.

We already return null for some "no-ops), e.g. xfermode::create(srcover), so I
imagine we could do that same sort of thing here for blurs... (and likely others
like OffsetImageFilter, ColorMatrixImageFilter)

That said, if we need a quick-fix for the current situation, I don't want to
stall this CL for a larger exploration.

[xidachen, 2015-12-10 15:16:41]

I have moved the check to the constructor of SkBlurImageFilter. PTAL.

https://codereview.chromium.org/1518643002/diff/1/src/effects/SkDropShadowIma...
File src/effects/SkDropShadowImageFilter.cpp (right):

https://codereview.chromium.org/1518643002/diff/1/src/effects/SkDropShadowIma...
src/effects/SkDropShadowImageFilter.cpp:81: if (sigma.fX != 0 || sigma.fY != 0)
On 2015/12/10 14:57:18, Stephen White wrote:
> On 2015/12/10 13:36:49, reed1 wrote:
> > It seems that for the most part, if the sigmas are zero, they will always
have
> > been zero (i.e. at Create factory time).
> > 
> > Perhaps the factory itself should check for zero sigmas, and return an
> > OffsetImageFilter instead of a DropShaderImageFilter...
> 
> If it's drawing shadow & foreground, an offset may not be enough. It'd need a
> composite as well. And you'd have to check for crop rect.
> 
> For this particular case, I'd prefer that this was done in
> SkBlurImageFilter::Create(); something like:
> 
> if (sigma.fX == 0 && sigma.fY == 0 && nullptr == cropRect) {
>     return input;
> }
> 
> Would that work?
> 
> Note that we may have to check the call sites in Chrome to ensure they aren't
> expecting a non-null pointer (running the blink layout tests should be
enough).

Done.

https://codereview.chromium.org/1518643002/diff/1/src/effects/SkDropShadowIma...
src/effects/SkDropShadowImageFilter.cpp:86: if (blurFilter)
On 2015/12/10 13:36:49, reed1 wrote:
> This check seems unnecessary, since line 87 will execute correctly either way.

Done.

[Stephen White, 2015-12-10 16:05:40]

https://codereview.chromium.org/1518643002/diff/40001/include/effects/SkBlurI...
File include/effects/SkBlurImageFilter.h (right):

https://codereview.chromium.org/1518643002/diff/40001/include/effects/SkBlurI...
include/effects/SkBlurImageFilter.h:18: if (0.0f == sigmaX && 0.0f == sigmaY &&
NULL == cropRect)
Skia coding style requires braces on all statement blocks.

Also, prefer 0 for SkScalar zero, and nullptr for NULL.

[xidachen, 2015-12-10 16:17:44]

PS#5 has accommodated for them, thanks for the comments.

https://codereview.chromium.org/1518643002/diff/40001/include/effects/SkBlurI...
File include/effects/SkBlurImageFilter.h (right):

https://codereview.chromium.org/1518643002/diff/40001/include/effects/SkBlurI...
include/effects/SkBlurImageFilter.h:18: if (0.0f == sigmaX && 0.0f == sigmaY &&
NULL == cropRect)
On 2015/12/10 16:05:40, Stephen White wrote:
> Skia coding style requires braces on all statement blocks.
> 
> Also, prefer 0 for SkScalar zero, and nullptr for NULL.

Done.

[Stephen White, 2015-12-10 16:20:18]

https://codereview.chromium.org/1518643002/diff/80001/include/effects/SkBlurI...
File include/effects/SkBlurImageFilter.h (right):

https://codereview.chromium.org/1518643002/diff/80001/include/effects/SkBlurI...
include/effects/SkBlurImageFilter.h:18: if ((0 == sigmaX) && (0 == sigmaY) &&
(nullptr == cropRect))
*braces* are mandatory {}; parentheses () are not. :)

[xidachen, 2015-12-10 16:23:30]

https://codereview.chromium.org/1518643002/diff/80001/include/effects/SkBlurI...
File include/effects/SkBlurImageFilter.h (right):

https://codereview.chromium.org/1518643002/diff/80001/include/effects/SkBlurI...
include/effects/SkBlurImageFilter.h:18: if ((0 == sigmaX) && (0 == sigmaY) &&
(nullptr == cropRect))
On 2015/12/10 16:20:18, Stephen White wrote:
> *braces* are mandatory {}; parentheses () are not. :)

Oops, fixed in PS#6.

[Stephen White, 2015-12-10 16:23:55]

The CQ bit was checked by senorblanco@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-10 16:24:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1518643002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1518643002/100001

[commit-bot: I haz the power, 2015-12-10 16:25:56]

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

[commit-bot: I haz the power, 2015-12-10 16:25:58]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[reed1, 2015-12-10 16:31:30]

the cl comment says we will return null. It should say we will return the input
(which may or may not be null).

other than that, I'm fine w/ the change, assuming we update the callers in
skia/chrome to be able to handle a possible null-return (which technically they
already should, since all of our effect factories *should* be able to return
null (e.g. the receive nonsense input like sigma < 0)

[xidachen, 2015-12-10 16:34:53]

Description was changed from

==========
Change blurFilter = null when sigma = 0

In the current implementation, a blurFilter is always created even in the
case when sigma.fX == 0 && sigma.fY == 0. This CL makes the blurFilter
to be null in this case.

BUG=568393
==========

to

==========
SkBlurImageFilter returns input when sigma = 0

In the current implementation, a blur filter is always created even in the
case when sigma.fX == 0 && sigma.fY == 0. This CL makes the blur filter
return input in this case.

BUG=568393
==========

[Stephen White, 2015-12-10 16:49:23]

From the trybots, it looks like there are some benches and GMs which are
assuming a non-null return.

Please change all instances of the form:

paint.setImageFilter(SkBlurImageFilter::Create(...))->unref();

to

SkAutoTUnref<SkImageFilter> blur(SkBlurImageFilter::Create(...));
paint.setImageFilter(blur.get());

Also, the Blink layout test css3/filters/null-effect-check.html fails with this
change. Looks like you'll have to make some changes over in Chrome before
landing as well.

[xidachen, 2015-12-10 19:12:18]

The CQ bit was checked by xidachen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-10 19:12:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1518643002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1518643002/120001

[commit-bot: I haz the power, 2015-12-10 19:12:23]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-12-11 01:12 UTC

[Stephen White, 2015-12-10 19:28:40]

https://codereview.chromium.org/1518643002/diff/120001/include/effects/SkBlur...
File include/effects/SkBlurImageFilter.h (right):

https://codereview.chromium.org/1518643002/diff/120001/include/effects/SkBlur...
include/effects/SkBlurImageFilter.h:19: return input;
One thing I forgot: since the contract is to return a "new" object, we need to
bump the refcount on the input before we return it (this was causing the layout
test failure in Chrome).

Please add:

SkSafeRef(input);

just before this line.

[xidachen, 2015-12-10 19:29:45]

The CQ bit was unchecked by xidachen@chromium.org

[reed1, 2015-12-10 19:41:17]

fine with me; a couple of unimportant style nits

https://codereview.chromium.org/1518643002/diff/140001/bench/BlurImageFilterB...
File bench/BlurImageFilterBench.cpp (right):

https://codereview.chromium.org/1518643002/diff/140001/bench/BlurImageFilterB...
bench/BlurImageFilterBench.cpp:84: paint.setImageFilter(blur.get());
trivial nit:

paint.setImageFilter(blur);

no need to explicitly call .get()

https://codereview.chromium.org/1518643002/diff/140001/include/effects/SkBlur...
File include/effects/SkBlurImageFilter.h (right):

https://codereview.chromium.org/1518643002/diff/140001/include/effects/SkBlur...
include/effects/SkBlurImageFilter.h:20: return input;
or

return SkSafeRef(input);

[Stephen White, 2015-12-10 19:45:42]

LGTM (will need Mike's too, since it's a .h change).

[reed1, 2015-12-10 19:47:37]

lgtm

[xidachen, 2015-12-10 19:55:48]

The CQ bit was checked by xidachen@chromium.org

[xidachen, 2015-12-10 19:55:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
senorblanco@chromium.org, reed@google.com
Link to the patchset: https://codereview.chromium.org/1518643002/#ps160001
(title: "clean up")

[commit-bot: I haz the power, 2015-12-10 19:55:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1518643002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1518643002/160001

[commit-bot: I haz the power, 2015-12-10 20:08:46]

Description was changed from

==========
SkBlurImageFilter returns input when sigma = 0

In the current implementation, a blur filter is always created even in the
case when sigma.fX == 0 && sigma.fY == 0. This CL makes the blur filter
return input in this case.

BUG=568393
==========

to

==========
SkBlurImageFilter returns input when sigma = 0

In the current implementation, a blur filter is always created even in the
case when sigma.fX == 0 && sigma.fY == 0. This CL makes the blur filter
return input in this case.

BUG=568393

Committed:
https://skia.googlesource.com/skia/+/467ddc0b24a63ee1525fa18d1dcf62e47975588a
==========

[commit-bot: I haz the power, 2015-12-10 20:08:47]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/467ddc0b24a63ee1525fa18d1dcf62e47975588a