first attempt to plumb fast blur code into skia mask filter

first attempt to plumb fast blur code into skia mask filter


BUG=

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

[humper, 2013-03-11 13:58:17]

On 2013/03/04 22:25:36, humper wrote:

Made necessary plumbing changes to only compute small blur for 9patch; achieves
actual perf gains.

[humper, 2013-03-11 18:20:51]

Fix bug with rect offset, remove unused 9patch bypass path, ifdef for staging
webkit.

On 2013/03/11 13:58:17, humper wrote:
> On 2013/03/04 22:25:36, humper wrote:
> 
> Made necessary plumbing changes to only compute small blur for 9patch;
achieves
> actual perf gains.

[humper, 2013-03-11 18:25:06]

reverted bench changes

On 2013/03/11 18:20:51, humper wrote:
> Fix bug with rect offset, remove unused 9patch bypass path, ifdef for staging
> webkit.
> 
> On 2013/03/11 13:58:17, humper wrote:
> > On 2013/03/04 22:25:36, humper wrote:
> > 
> > Made necessary plumbing changes to only compute small blur for 9patch;
> achieves
> > actual perf gains.

[humper, 2013-03-11 19:00:48]

Adding Ben

[bungeman-skia, 2013-03-11 19:47:41]

Publishing comments on Patch Set 4 for posterity.

https://codereview.chromium.org/12387099/diff/5002/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.chromium.org/12387099/diff/5002/src/effects/SkBlurMask.cpp...
src/effects/SkBlurMask.cpp:1248: radius = (radius + .5f) *2.f;
My eyes... need a space after *

https://codereview.chromium.org/12387099/diff/5002/src/effects/SkBlurMask.cpp...
src/effects/SkBlurMask.cpp:1250: profile_size = compute_profile( radius,
perform_blur ? &profile : NULL );
It feels awkward to have this check here and another check in the function. Can
we separate 'size' from 'profile' since one if a logical part of the blur (or at
least out blur approximation) and the other an implementation detail?

https://codereview.chromium.org/12387099/diff/5002/src/effects/SkBlurMask.cpp...
src/effects/SkBlurMask.cpp:1264: dst->fBounds.set(shadow_left + src.fLeft,
shadow_top + src.fTop, shadow_right + src.fRight, shadow_bottom + src.fBottom);
Nit: This line got really long. A few more below.

https://codereview.chromium.org/12387099/diff/5002/src/effects/SkBlurMask.cpp...
src/effects/SkBlurMask.cpp:1273: if (perform_blur) {
Can we put the else clause under !perform_blur and return true in there, leaving
the rest of the 'meat' unindented?

[humper, 2013-03-11 19:56:14]

all done
On 2013/03/11 19:47:41, bungeman1 wrote:
> Publishing comments on Patch Set 4 for posterity.
> 
> https://codereview.chromium.org/12387099/diff/5002/src/effects/SkBlurMask.cpp
> File src/effects/SkBlurMask.cpp (right):
> 
>
https://codereview.chromium.org/12387099/diff/5002/src/effects/SkBlurMask.cpp...
> src/effects/SkBlurMask.cpp:1248: radius = (radius + .5f) *2.f;
> My eyes... need a space after *
> 
>
https://codereview.chromium.org/12387099/diff/5002/src/effects/SkBlurMask.cpp...
> src/effects/SkBlurMask.cpp:1250: profile_size = compute_profile( radius,
> perform_blur ? &profile : NULL );
> It feels awkward to have this check here and another check in the function.
Can
> we separate 'size' from 'profile' since one if a logical part of the blur (or
at
> least out blur approximation) and the other an implementation detail?
> 
>
https://codereview.chromium.org/12387099/diff/5002/src/effects/SkBlurMask.cpp...
> src/effects/SkBlurMask.cpp:1264: dst->fBounds.set(shadow_left + src.fLeft,
> shadow_top + src.fTop, shadow_right + src.fRight, shadow_bottom +
src.fBottom);
> Nit: This line got really long. A few more below.
> 
>
https://codereview.chromium.org/12387099/diff/5002/src/effects/SkBlurMask.cpp...
> src/effects/SkBlurMask.cpp:1273: if (perform_blur) {
> Can we put the else clause under !perform_blur and return true in there,
leaving
> the rest of the 'meat' unindented?

[bungeman-skia, 2013-03-11 20:03:36]

Comments on Patch Set 6.

https://codereview.chromium.org/12387099/diff/8002/src/effects/SkBlurMask.cpp
File src/effects/SkBlurMask.cpp (right):

https://codereview.chromium.org/12387099/diff/8002/src/effects/SkBlurMask.cpp...
src/effects/SkBlurMask.cpp:1189: profile.
Nit: this was wrapped before, but doesn't need to be now. Nice to have it all on
one line.

https://codereview.chromium.org/12387099/diff/8002/src/effects/SkBlurMask.cpp...
src/effects/SkBlurMask.cpp:1246: SkIPoint *margin, bool perform_blur) {
Maybe use SkMask::CreateMode to avoid awkward 'do what I want' and 'what do I
name this' debates?

https://codereview.chromium.org/12387099/diff/8002/src/effects/SkBlurMask.cpp...
src/effects/SkBlurMask.cpp:1266: dst->fBounds.set(shadow_left + src.fLeft,
shadow_top + src.fTop, shadow_right + src.fRight, shadow_bottom + src.fBottom);
This line is long.

https://codereview.chromium.org/12387099/diff/8002/src/effects/SkBlurMask.cpp...
src/effects/SkBlurMask.cpp:1272: int             sw =
SkScalarFloorToInt(src.width());
I know this used to have this crazy spacing, but I think the reason for it has
been lost to the sands of time.

https://codereview.chromium.org/12387099/diff/8002/src/effects/SkBlurMask.cpp...
src/effects/SkBlurMask.cpp:1282: unsigned int *profile = NULL;
Probably want a blank line above, since this is part of the next block of code
and not associated with the above block.

[bungeman-skia, 2013-03-11 20:12:38]

LGTM (for now... dun dun dun...) There needs to be a format cleanup in these
files at some point, but things look cleaner than when this started.