Make SkBlurImageFilter capable of cropping during blur

Make SkBlurImageFilter capable of cropping during blur (raster path)

SkBlurImageFilter can currently only process a source image 
which is larger than or equal to the destination rect. If
the source image (or crop rect) is smaller, it is padded
out to dest size with transparent black via the 6-param
version of applyCropRect().

Fixing this requires modifying all the flavours of RGBA 
box_blur() to accept a src crop rect.

BUG=skia:4502, skia:4526
CQ_EXTRA_TRYBOTS=client.skia.android:Test-Android-GCC-Nexus5-CPU-NEON-Arm7-Release-Trybot;client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

Committed: https://skia.googlesource.com/skia/+/1b82ceb737c73327412f2e8a91748481e1aec9e4

Committed: https://skia.googlesource.com/skia/+/29a440d2d6f306498a7b54e978e3b034bd625c19

[Stephen White, 2015-10-23 18:36:13]

Description was changed from

==========
Make RGBA CPU blurs capable of handling expanding crop rects.

These were using the 6-param version of applyCropRect() to pad the
source rect with transparent black.

BUG=skia:4502
NOTREECHECKS=true
NOTRY=true
NOPRESUBMIT=true
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Make SkBlurImageFilter capable of cropping during blur

SkBlurImageFilter can currently only process a source image which is larger or
equal to the destination rect. If the source image (or cropped size) is smaller,
it is padded out to full size with transparent black via the 6-param version of
applyCropRect().

BUG=skia:4502
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[Stephen White, 2015-10-23 18:37:11]

Description was changed from

==========
Make SkBlurImageFilter capable of cropping during blur

SkBlurImageFilter can currently only process a source image which is larger or
equal to the destination rect. If the source image (or cropped size) is smaller,
it is padded out to full size with transparent black via the 6-param version of
applyCropRect().

BUG=skia:4502
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Make SkBlurImageFilter capable of cropping during blur

SkBlurImageFilter can currently only process a source image which is larger or
equal to the destination rect. If the source image (or crop rect) is smaller, it
is padded out to dest size with transparent black via the 6-param version of
applyCropRect().

Fixing this requires modifying all the flavours of RGBA  box_blur() to accept a
src crop rect.

BUG=skia:4502
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[Stephen White, 2015-10-23 22:12:37]

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

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

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

[commit-bot: I haz the power, 2015-10-23 22:22:52]

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

[commit-bot: I haz the power, 2015-10-23 22:22:53]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-10-27 13:50:49]

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

[reed1, 2015-10-27 13:50:50]

Does this change warrant additional tests to exercise/validate it?

[Stephen White, 2015-10-27 14:04:14]

On 2015/10/27 13:50:50, reed1 wrote:
> Does this change warrant additional tests to exercise/validate it?

The ARM path may not be fully correct yet (especially box_blur_double).

I'd also like to do some perf runs. We may see a regression in the 
non-cropping case, since there are now more comparisons inside the loop.
(If so, I have a plan to get the perf back, but I'd like to verify correctness
first.)

I think the test coverage is ok (assuming the bots cover all hardware configs).

[reed1, 2015-10-27 14:18:43]

I asked about tests specifically around the new "feature" of the BlurImageFilter
returning a dst-buffer that is larger than its src-buffer. (assuming that is the
feature). Do our current GMs know how to exercise that, or to expect/assert that
the dst-buffer is larger?

[Stephen White, 2015-10-27 14:30:40]

On 2015/10/27 14:18:43, reed1 wrote:
> I asked about tests specifically around the new "feature" of the
BlurImageFilter
> returning a dst-buffer that is larger than its src-buffer. (assuming that is
the
> feature). Do our current GMs know how to exercise that, or to expect/assert
that
> the dst-buffer is larger?

Yes, the imagefilterscropexpand test should exercise this case.

[reed1, 2015-10-27 16:20:47]

sorry, I'm still confused. When I run imagefilterscropexpand with the patch, the
dst-buffer size is still always == src-buffer size. My earlier question was
about testing that blur can now return a dst larger than its src.

[reed1, 2015-10-27 16:26:48]

... I may have a funny version of the CL patched locally... retrying

[reed1, 2015-10-27 16:31:17]

well, I did, but on an updated version, I still see the same confusing result.
src-size is still always equal to dst-size when I run that gm. That seems to not
be showing the new feature/behavior...

[Stephen White, 2015-10-27 17:28:06]

The imagefilterscropexpand GM uses a small crop rect on the input filter, 
and a larger (expanded) crop rect on the blur filter.

If you compare the srcBounds passed to the first call to box_blur_xx to the
dstBounds, you should see different values. This will currently *only* be 
called in the crop rect case above (small cropped input to a larger blur 
output).

[Stephen White, 2015-10-27 17:30:20]

On 2015/10/27 17:28:06, Stephen White wrote:
> The imagefilterscropexpand GM uses a small crop rect on the input filter, 
> and a larger (expanded) crop rect on the blur filter.
> 
> If you compare the srcBounds passed to the first call to box_blur_xx to the
> dstBounds, you should see different values. This will currently *only* be 
> called in the crop rect case above (small cropped input to a larger blur 
> output).

Once https://codereview.chromium.org/1304883004/ and 
https://codereview.chromium.org/1308703007/ land, it'll be called in many more 
cases, since we'll be using input primitives with tight bounds.

[reed1, 2015-10-27 17:41:02]

My earlier statement was:

"I asked about tests specifically around the new "feature" of the
BlurImageFilter returning a dst-buffer that is larger than its src-buffer.
(assuming that is the feature)."

You responded that existing GMs should exercise this.

I think the disconnect is/was I said "BlurImageFilter returning a dst-buffer
that is larger than its src-buffer" and you seem to answer in the affirmative
(i.e. that this feature is being exercised).

It appears that this is not happening (hence my confusion), but existing tests
*are* exercising the low-level implmeentation functions (e.g.
blur_this_or_that), which is the gist of this CL.

Is this new understanding correct, that this CL does *not* change
BlurImageFilter's return size, but does change internal calling conventions for
its implementation helper functions?

[Stephen White, 2015-10-27 18:04:19]

On 2015/10/27 17:41:02, reed1 wrote:
> My earlier statement was:
> 
> "I asked about tests specifically around the new "feature" of the
> BlurImageFilter returning a dst-buffer that is larger than its src-buffer.
> (assuming that is the feature)."
> 
> You responded that existing GMs should exercise this.
> 
> I think the disconnect is/was I said "BlurImageFilter returning a dst-buffer
> that is larger than its src-buffer" and you seem to answer in the affirmative
> (i.e. that this feature is being exercised).
> 
> It appears that this is not happening (hence my confusion), but existing tests
> *are* exercising the low-level implmeentation functions (e.g.
> blur_this_or_that), which is the gist of this CL.
> 
> Is this new understanding correct, that this CL does *not* change
> BlurImageFilter's return size, but does change internal calling conventions
for
> its implementation helper functions?

You're correct -- it doesn't change the output size. That will require the 
aforementioned CLs, since without those, we don't receive the smaller (tight) 
size as input.

imagefilterscropexpand exercises this case:

offset = Offset(..., null, &small_crop_rect)
blur = Blur(... , offset, &large_crop_rect)

Without this change, this is performed by 
SkBlurImageFilter calling applyCropRect(6 params) doing a blit from the
small_crop_rect size
to the large_crop_rect size (padding w/transparent black), and then the blur
pixel loops 
blurring at large_crop_rect size (both input and output size).

With this change, this is performed by
SkBlurImageFilter calling applyCropRect(4 params, no blit), and then the blur
pixel loops
blurring from small_crop_rect src to large_crop_rect dest.

Does that make sense?

[reed1, 2015-10-27 18:35:16]

Thanks.

Perhaps we can reorg the CL summary to focus on change itself : expanding
box_filter_helper... to take another rect.

I'll defer to Rob for detail comments about the code change.

Regarding testing, I agree that the new code will get executed. It is somewhat
hard to see that the new code will *really* facilitate the (future) feature of
returning a larger buffer, when it is never asked to in the current environment.

[Stephen White, 2015-10-27 18:37:31]

On 2015/10/27 18:35:16, reed1 wrote:
> Thanks.
> 
> Perhaps we can reorg the CL summary to focus on change itself : expanding
> box_filter_helper... to take another rect.
> 
> I'll defer to Rob for detail comments about the code change.
> 
> Regarding testing, I agree that the new code will get executed. It is somewhat
> hard to see that the new code will *really* facilitate the (future) feature of
> returning a larger buffer, when it is never asked to in the current
environment.

Thanks for your comments.

Unfortunately, as I suspected, this does introduce a perf regression of 10-30%.
I'm going
to try to ameliorate it, but I think the blur loops could use some refactoring
first.
So I wouldn't worry about reviewing this CL yet.

[Stephen White, 2015-10-28 16:00:08]

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

[commit-bot: I haz the power, 2015-10-28 16:00:16]

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

[commit-bot: I haz the power, 2015-10-28 16:11:31]

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

[commit-bot: I haz the power, 2015-10-28 16:11:32]

Dry run: This issue passed the CQ dry run.

[Stephen White, 2015-10-28 17:28:50]

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

[commit-bot: I haz the power, 2015-10-28 17:29:04]

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

[Stephen White, 2015-10-28 17:37:47]

Description was changed from

==========
Make SkBlurImageFilter capable of cropping during blur

SkBlurImageFilter can currently only process a source image which is larger or
equal to the destination rect. If the source image (or crop rect) is smaller, it
is padded out to dest size with transparent black via the 6-param version of
applyCropRect().

Fixing this requires modifying all the flavours of RGBA  box_blur() to accept a
src crop rect.

BUG=skia:4502
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Make SkBlurImageFilter capable of cropping during blur

SkBlurImageFilter can currently only process a source image which is larger or
equal to the destination rect. If the source image (or crop rect) is smaller, it
is padded out to dest size with transparent black via the 6-param version of
applyCropRect().

Fixing this requires modifying all the flavours of RGBA  box_blur() to accept a
src crop rect.

BUG=skia:4502, skia:4526
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[commit-bot: I haz the power, 2015-10-28 17:40:32]

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

[commit-bot: I haz the power, 2015-10-28 17:40:32]

Dry run: This issue passed the CQ dry run.

[Stephen White, 2015-10-28 18:05:19]

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

[commit-bot: I haz the power, 2015-10-28 18:05:24]

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

[Stephen White, 2015-10-28 18:17:32]

reed@, robertphillips@: This one should be ready for review now.

I've updated box_blur to use separate loops for each condition, ameliorating the
perf regression.

I've also made applyCropRect() responsible for computing srcBounds. That way we
have a single intersect() to remove 
when we want to change to the new filter spec semantics.

[commit-bot: I haz the power, 2015-10-28 18:20:23]

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

[commit-bot: I haz the power, 2015-10-28 18:20:24]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-10-28 18:21:38]

https://codereview.chromium.org/1415653003/diff/170001/include/core/SkImageFi...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/1415653003/diff/170001/include/core/SkImageFi...
include/core/SkImageFilter.h:371: /** Computes source bounds as the src bitmap
bounds offset by srcOffset.
This comment is

1. very dense
2. does not seem to refer to all of the named bounds parameters

Can it be expanded/clarified?

Separately, perhaps this applyCropRect API change should land separately from
the modifications that are box-blur-etc. specific?

[Stephen White, 2015-10-28 19:01:00]

On 2015/10/28 18:21:38, reed1 wrote:
>
https://codereview.chromium.org/1415653003/diff/170001/include/core/SkImageFi...
> File include/core/SkImageFilter.h (right):
> 
>
https://codereview.chromium.org/1415653003/diff/170001/include/core/SkImageFi...
> include/core/SkImageFilter.h:371: /** Computes source bounds as the src bitmap
> bounds offset by srcOffset.
> This comment is
> 
> 1. very dense
> 2. does not seem to refer to all of the named bounds parameters
> 
> Can it be expanded/clarified?

Done.

> Separately, perhaps this applyCropRect API change should land separately from
> the modifications that are box-blur-etc. specific?

Done.

[Stephen White, 2015-10-28 21:08:55]

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

[Stephen White, 2015-10-28 21:08:56]

PTAL.

mtklein@: this includes changes to the NEON box_blur_double() which give it the
perf improvement of https://codereview.chromium.org/1426583004/, as well as
support for the crop rect. Let me know if you'd prefer I break those changes out
into a separate CL.

Rebased onto https://codereview.chromium.org/1410553007 (in CQ now).

[mtklein, 2015-10-28 21:13:51]

On 2015/10/28 at 21:08:56, senorblanco wrote:
> PTAL.
> 
> mtklein@: this includes changes to the NEON box_blur_double() which give it
the perf improvement of https://codereview.chromium.org/1426583004/, as well as
support for the crop rect. Let me know if you'd prefer I break those changes out
into a separate CL.
> 
> Rebased onto https://codereview.chromium.org/1410553007 (in CQ now).

If it's not a burden, it'd be easier to review separately.  I'm happily ignorant
of what you're up to with the crop rect.  If it's annoying to split up, I don't
mind reviewing the whole thing, more slowly.

[Stephen White, 2015-10-28 22:57:42]

Patchset #11 (id:190001) has been deleted

[Stephen White, 2015-10-28 22:57:52]

Patchset #11 (id:210001) has been deleted

[Stephen White, 2015-10-28 22:57:59]

Patchset #11 (id:230001) has been deleted

[Stephen White, 2015-10-28 22:58:07]

Patchset #11 (id:250001) has been deleted

[Stephen White, 2015-10-28 22:58:13]

Patchset #11 (id:2) has been deleted

[Stephen White, 2015-10-28 22:58:17]

Patchset #11 (id:280001) has been deleted

[Stephen White, 2015-10-28 23:07:58]

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

[commit-bot: I haz the power, 2015-10-28 23:08:07]

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

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

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

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

Dry run: This issue passed the CQ dry run.

[Stephen White, 2015-10-29 17:09:19]

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

[commit-bot: I haz the power, 2015-10-29 17:09:23]

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

[commit-bot: I haz the power, 2015-10-29 17:23:35]

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

[commit-bot: I haz the power, 2015-10-29 17:23:36]

Dry run: This issue passed the CQ dry run.

[Stephen White, 2015-10-30 14:11:41]

mtklein@, reed@: Friendly ping.

[Stephen White, 2015-10-30 20:07:00]

Description was changed from

==========
Make SkBlurImageFilter capable of cropping during blur

SkBlurImageFilter can currently only process a source image which is larger or
equal to the destination rect. If the source image (or crop rect) is smaller, it
is padded out to dest size with transparent black via the 6-param version of
applyCropRect().

Fixing this requires modifying all the flavours of RGBA  box_blur() to accept a
src crop rect.

BUG=skia:4502, skia:4526
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Make SkBlurImageFilter capable of cropping during blur (raster path)

SkBlurImageFilter can currently only process a source image which is larger or
equal to the destination rect. If the source image (or crop rect) is smaller, it
is padded out to dest size with transparent black via the 6-param version of
applyCropRect().

Fixing this requires modifying all the flavours of RGBA  box_blur() to accept a
src crop rect.

BUG=skia:4502, skia:4526
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[Stephen White, 2015-10-30 20:07:32]

Description was changed from

==========
Make SkBlurImageFilter capable of cropping during blur (raster path)

SkBlurImageFilter can currently only process a source image which is larger or
equal to the destination rect. If the source image (or crop rect) is smaller, it
is padded out to dest size with transparent black via the 6-param version of
applyCropRect().

Fixing this requires modifying all the flavours of RGBA  box_blur() to accept a
src crop rect.

BUG=skia:4502, skia:4526
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Make SkBlurImageFilter capable of cropping during blur (raster path)

SkBlurImageFilter can currently only process a source image 
which is larger than or equal to the destination rect. If
the source image (or crop rect) is smaller, it is padded
out to dest size with transparent black via the 6-param
version of applyCropRect().

Fixing this requires modifying all the flavours of RGBA 
box_blur() to accept a src crop rect.

BUG=skia:4502, skia:4526
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

[reed1, 2015-11-02 15:05:06]

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

[reed1, 2015-11-02 15:05:07]

defer to mike for the bulk of this, as he understands the core logic better than
I

asking for comments on the different "sections", as there are so many cases in
the loops (no overlap w/ domain, partial, etc.) so a new read can grasp the
macro organization of the code a bit.

https://codereview.chromium.org/1415653003/diff/360001/src/opts/SkBlurImageFi...
File src/opts/SkBlurImageFilter_opts.h (right):

https://codereview.chromium.org/1415653003/diff/360001/src/opts/SkBlurImageFi...
src/opts/SkBlurImageFilter_opts.h:235: for (int y = 0; y < top; y++) {
// fill in with zeros where we're sampling above our domain

https://codereview.chromium.org/1415653003/diff/360001/src/opts/SkBlurImageFi...
src/opts/SkBlurImageFilter_opts.h:257: for (x = 0; x < incrementStart; ++x) {
// fill in with zeros where we're sampling to the left of our domain

https://codereview.chromium.org/1415653003/diff/360001/src/opts/SkBlurImageFi...
src/opts/SkBlurImageFilter_opts.h:287: for (; x < width; ++x) {
// fill in with zeros where we're sampling to the right of our domain

https://codereview.chromium.org/1415653003/diff/360001/src/opts/SkBlurImageFi...
src/opts/SkBlurImageFilter_opts.h:294: for (int y = bottom; y < height; ++y) {
// fill in with zeros where we're sampling below our domain

[Stephen White, 2015-11-02 15:23:14]

Thanks for your review.

https://codereview.chromium.org/1415653003/diff/360001/src/opts/SkBlurImageFi...
File src/opts/SkBlurImageFilter_opts.h (right):

https://codereview.chromium.org/1415653003/diff/360001/src/opts/SkBlurImageFi...
src/opts/SkBlurImageFilter_opts.h:235: for (int y = 0; y < top; y++) {
On 2015/11/02 15:05:07, reed1 wrote:
> // fill in with zeros where we're sampling above our domain

Done.

https://codereview.chromium.org/1415653003/diff/360001/src/opts/SkBlurImageFi...
src/opts/SkBlurImageFilter_opts.h:257: for (x = 0; x < incrementStart; ++x) {
On 2015/11/02 15:05:07, reed1 wrote:
> // fill in with zeros where we're sampling to the left of our domain

Done.

https://codereview.chromium.org/1415653003/diff/360001/src/opts/SkBlurImageFi...
src/opts/SkBlurImageFilter_opts.h:287: for (; x < width; ++x) {
On 2015/11/02 15:05:07, reed1 wrote:
> // fill in with zeros where we're sampling to the right of our domain

Done.

https://codereview.chromium.org/1415653003/diff/360001/src/opts/SkBlurImageFi...
src/opts/SkBlurImageFilter_opts.h:294: for (int y = bottom; y < height; ++y) {
On 2015/11/02 15:05:07, reed1 wrote:
> // fill in with zeros where we're sampling below our domain

Done.

[Stephen White, 2015-11-02 15:23:53]

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

[commit-bot: I haz the power, 2015-11-02 15:24:06]

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

[commit-bot: I haz the power, 2015-11-02 15:34:36]

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

[commit-bot: I haz the power, 2015-11-02 15:34:37]

Dry run: This issue passed the CQ dry run.

[mtklein, 2015-11-02 16:53:20]

Description was changed from

==========
Make SkBlurImageFilter capable of cropping during blur (raster path)

SkBlurImageFilter can currently only process a source image 
which is larger than or equal to the destination rect. If
the source image (or crop rect) is smaller, it is padded
out to dest size with transparent black via the 6-param
version of applyCropRect().

Fixing this requires modifying all the flavours of RGBA 
box_blur() to accept a src crop rect.

BUG=skia:4502, skia:4526
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
==========

to

==========
Make SkBlurImageFilter capable of cropping during blur (raster path)

SkBlurImageFilter can currently only process a source image 
which is larger than or equal to the destination rect. If
the source image (or crop rect) is smaller, it is padded
out to dest size with transparent black via the 6-param
version of applyCropRect().

Fixing this requires modifying all the flavours of RGBA 
box_blur() to accept a src crop rect.

BUG=skia:4502, skia:4526
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot;client.skia.android:Test-Android-GCC-Nexus5-CPU-NEON-Arm7-Release-Trybot
==========

[mtklein, 2015-11-02 16:53:38]

The CQ bit was checked by mtklein@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-02 16:53:50]

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

[mtklein, 2015-11-02 16:55:44]

https://codereview.chromium.org/1415653003/diff/380001/src/opts/SkBlurImageFi...
File src/opts/SkBlurImageFilter_opts.h (right):

https://codereview.chromium.org/1415653003/diff/380001/src/opts/SkBlurImageFi...
src/opts/SkBlurImageFilter_opts.h:127: STORE_SUMS_DOUBLE
Should we not be zeroing here and at 155?

[Stephen White, 2015-11-02 17:08:58]

https://codereview.chromium.org/1415653003/diff/380001/src/opts/SkBlurImageFi...
File src/opts/SkBlurImageFilter_opts.h (right):

https://codereview.chromium.org/1415653003/diff/380001/src/opts/SkBlurImageFi...
src/opts/SkBlurImageFilter_opts.h:127: STORE_SUMS_DOUBLE
On 2015/11/02 16:55:44, mtklein wrote:
> Should we not be zeroing here and at 155?

We are: "sum" is always zero in both cases. It's zero here because we
initialized it to zero at 117 (and the loop above this one does nothing in this
case, since incrementStart > 0), and it's zero at the end because we've
decremented all the pixels off, leaving it at zero.

[reed1, 2015-11-02 17:39:10]

Lets add stephen's reply as a brief comment on those lines?

[mtklein, 2015-11-02 17:40:10]

with those comments, lgtm

[commit-bot: I haz the power, 2015-11-02 17:40:20]

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

[commit-bot: I haz the power, 2015-11-02 17:40:21]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-11-02 17:46:46]

lgtm ditto comments

[Stephen White, 2015-11-02 18:34:49]

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

[commit-bot: I haz the power, 2015-11-02 18:34:56]

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

[Stephen White, 2015-11-02 18:35:00]

On 2015/11/02 17:40:10, mtklein wrote:
> with those comments, lgtm

Done.

[Stephen White, 2015-11-02 18:35:07]

The CQ bit was unchecked by senorblanco@chromium.org

[Stephen White, 2015-11-02 18:35:14]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-11-02 18:35:14]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com,
reed@google.com
Link to the patchset: https://codereview.chromium.org/1415653003/#ps400001
(title: "Added comments per review")

[commit-bot: I haz the power, 2015-11-02 18:35:25]

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

[commit-bot: I haz the power, 2015-11-02 19:09:08]

Committed patchset #16 (id:400001) as
https://skia.googlesource.com/skia/+/1b82ceb737c73327412f2e8a91748481e1aec9e4

[f(malita), 2015-11-02 19:52:18]

On 2015/11/02 19:09:08, commit-bot: I haz the power wrote:
> Committed patchset #16 (id:400001) as
> https://skia.googlesource.com/skia/+/1b82ceb737c73327412f2e8a91748481e1aec9e4

ASAN failures appear, PTAL:

http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...


==24383==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6260000bcd40
at pc 0x7feb8b6d24f7 bp 0x7fff18c13d30 sp 0x7fff18c13d28
READ of size 4 at 0x6260000bcd40 thread T0
    #0 0x7feb8b6d24f6 in void sk_sse41::box_blur<(sk_sse41::BlurDirection)0,
(sk_sse41::BlurDirection)0>(unsigned int const*, int, SkIRect const&, unsigned
int*, int, int, int, int, int)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/opts/SkBlurImageFilter_opts.h:258:13
    #1 0x7feb8b3cbc8e in SkBlurImageFilter::onFilterImage(SkImageFilter::Proxy*,
SkBitmap const&, SkImageFilter::Context const&, SkBitmap*, SkIPoint*) const
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/effects/SkBlurImageFilter.cpp:157:9
    #2 0x7feb8a72fb2e in SkImageFilter::filterImage(SkImageFilter::Proxy*,
SkBitmap const&, SkImageFilter::Context const&, SkBitmap*, SkIPoint*) const
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkImageFilter.cpp:256:9
    #3 0x7feb8a61c213 in SkCanvas::internalDrawDevice(SkBaseDevice*, int, int,
SkPaint const*, bool)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkCanvas.cpp:1358:17
    #4 0x7feb8a6108f3 in SkCanvas::internalRestore()
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkCanvas.cpp:1232:13
    #5 0x7feb8a656897 in AutoDrawLooper::~AutoDrawLooper()
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkCanvas.cpp:511:13
    #6 0x7feb8a637169 in SkCanvas::onDrawBitmap(SkBitmap const&, float, float,
SkPaint const*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkCanvas.cpp:2249:1
    #7 0x7feb8a6215b2 in SkCanvas::drawBitmap(SkBitmap const&, float, float,
SkPaint const*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkCanvas.cpp:1901:5
    #8 0x7feb89eea19b in BlurImageFilterBench::onDraw(int, SkCanvas*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../bench/BlurImageFilterBench.cpp:86:13
    #9 0x7feb89eca62f in Benchmark::draw(int, SkCanvas*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../bench/Benchmark.cpp:58:5
    #10 0x7feb8a03f456 in time(int, Benchmark*, Target*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../bench/nanobench.cpp:213:5
    #11 0x7feb8a03b10d in nanobench_main()
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../bench/nanobench.cpp:1250:34
    #12 0x7feb8a040096 in main
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../bench/nanobench.cpp:1344:12
    #13 0x7feb871ebec4 in __libc_start_main
/build/buildd/eglibc-2.19/csu/libc-start.c:287
    #14 0x7feb89dfd1c9 in _start
(/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/nanobench+0xedc1c9)

0x6260000bcd40 is located 0 bytes to the right of 11328-byte region
[0x6260000ba100,0x6260000bcd40)
allocated by thread T0 here:
    #0 0x7feb89e8bf6b in __interceptor_malloc
(/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/nanobench+0xf6af6b)
    #1 0x7feb8b6d6f27 in sk_malloc_flags(unsigned long, unsigned int)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/ports/SkMemory_malloc.cpp:54:15
    #2 0x7feb8a757a47 in SkMallocPixelRef::NewAllocate(SkImageInfo const&,
unsigned long, SkColorTable*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkMallocPixelRef.cpp:81:18
    #3 0x7feb8a759f67 in SkMallocPixelRef::PRFactory::create(SkImageInfo const&,
unsigned long, SkColorTable*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkMallocPixelRef.cpp:202:12
    #4 0x7feb8a522f1a in SkBitmap::tryAllocPixels(SkImageInfo const&, unsigned
long)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkBitmap.cpp:293:22
    #5 0x7feb8a4523ed in SkBitmap::tryAllocPixels(SkImageInfo const&)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../include/core/SkBitmap.h:256:16
    #6 0x7feb8ab9c51a in SkBitmapDevice::Create(SkImageInfo const&,
SkSurfaceProps const&)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkBitmapDevice.cpp:98:14
    #7 0x7feb8ab9d7ed in SkBitmapDevice::onCreateDevice(SkBaseDevice::CreateInfo
const&, SkPaint const*)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkBitmapDevice.cpp:127:12
    #8 0x7feb8a733c13 in SkImageFilter::DeviceProxy::createDevice(int, int)
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkImageFilter.cpp:627:25
    #9 0x7feb8b4a8418 in
SkOffsetImageFilter::onFilterImage(SkImageFilter::Proxy*, SkBitmap const&,
SkImageFilter::Context const&, SkBitmap*, SkIPoint*) const
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/effects/SkOffsetImageFilter.cpp:48:43
    #10 0x7feb8a72fb2e in SkImageFilter::filterImage(SkImageFilter::Proxy*,
SkBitmap const&, SkImageFilter::Context const&, SkBitmap*, SkIPoint*) const
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkImageFilter.cpp:256:9
    #11 0x7feb8a730338 in SkImageFilter::filterInput(int, SkImageFilter::Proxy*,
SkBitmap const&, SkImageFilter::Context const&, SkBitmap*, SkIPoint*, bool)
const
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkImageFilter.cpp:280:12
    #12 0x7feb8b3c9899 in
SkBlurImageFilter::onFilterImage(SkImageFilter::Proxy*, SkBitmap const&,
SkImageFilter::Context const&, SkBitmap*, SkIPoint*) const
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/effects/SkBlurImageFilter.cpp:76:10
    #13 0x7feb8a72fb2e in SkImageFilter::filterImage(SkImageFilter::Proxy*,
SkBitmap const&, SkImageFilter::Context const&, SkBitmap*, SkIPoint*) const
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/core/SkImageFilter.cpp:256:9


SUMMARY: AddressSanitizer: heap-buffer-overflow
/home/default/storage/skia-repo/buildbot/skiabot-linux-tester-005/build/slave/workdir/build/skia/out/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-ASAN/Debug/../../../src/opts/SkBlurImageFilter_opts.h:258:13
in void sk_sse41::box_blur<(sk_sse41::BlurDirection)0,
(sk_sse41::BlurDirection)0>(unsigned int const*, int, SkIRect const&, unsigned
int*, int, int, int, int, int)

[Stephen White, 2015-11-02 19:55:20]

Yeah, that's not good. Can't repro w/valgrind, but I'll revert for now.

[Stephen White, 2015-11-02 19:56:02]

A revert of this CL (patchset #16 id:400001) has been created in
https://codereview.chromium.org/1428053002/ by senorblanco@chromium.org.

The reason for reverting is: ASAN failures (see
https://codereview.chromium.org/1415653003/).

[Stephen White, 2015-11-02 20:39:07]

Description was changed from

==========
Make SkBlurImageFilter capable of cropping during blur (raster path)

SkBlurImageFilter can currently only process a source image 
which is larger than or equal to the destination rect. If
the source image (or crop rect) is smaller, it is padded
out to dest size with transparent black via the 6-param
version of applyCropRect().

Fixing this requires modifying all the flavours of RGBA 
box_blur() to accept a src crop rect.

BUG=skia:4502, skia:4526
CQ_EXTRA_TRYBOTS=client.skia.android:Test-Android-GCC-Nexus5-CPU-NEON-Arm7-Release-Trybot;client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

Committed:
https://skia.googlesource.com/skia/+/1b82ceb737c73327412f2e8a91748481e1aec9e4
==========

to

==========
Make SkBlurImageFilter capable of cropping during blur (raster path)

SkBlurImageFilter can currently only process a source image 
which is larger than or equal to the destination rect. If
the source image (or crop rect) is smaller, it is padded
out to dest size with transparent black via the 6-param
version of applyCropRect().

Fixing this requires modifying all the flavours of RGBA 
box_blur() to accept a src crop rect.

BUG=skia:4502, skia:4526
CQ_EXTRA_TRYBOTS=client.skia.android:Test-Android-GCC-Nexus5-CPU-NEON-Arm7-Release-Trybot;client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot

Committed:
https://skia.googlesource.com/skia/+/1b82ceb737c73327412f2e8a91748481e1aec9e4
==========

[Stephen White, 2015-11-02 20:42:45]

Relanding w/fixes for ASAN errors.

[Stephen White, 2015-11-02 20:43:41]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-11-02 20:43:41]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com,
reed@google.com
Link to the patchset: https://codereview.chromium.org/1415653003/#ps440001
(title: "More ASAN fixes")

[commit-bot: I haz the power, 2015-11-02 20:43:52]

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

[mtklein, 2015-11-02 20:55:17]

The ASAN bot's pretty quick.  You might want to add it to the CQ bots if you're
unsure whether this fixes everything.

[commit-bot: I haz the power, 2015-11-02 20:55:50]

Committed patchset #18 (id:440001) as
https://skia.googlesource.com/skia/+/29a440d2d6f306498a7b54e978e3b034bd625c19

[Stephen White, 2015-11-02 21:03:47]

On 2015/11/02 20:55:17, mtklein wrote:
> The ASAN bot's pretty quick.  You might want to add it to the CQ bots if
you're
> unsure whether this fixes everything.

Cool, thanks. Didn't realize there was an ASAN trybot.

[mtklein, 2015-11-02 21:10:45]

On 2015/11/02 at 21:03:47, senorblanco wrote:
> On 2015/11/02 20:55:17, mtklein wrote:
> > The ASAN bot's pretty quick.  You might want to add it to the CQ bots if
you're
> > unsure whether this fixes everything.
> 
> Cool, thanks. Didn't realize there was an ASAN trybot.

Yup, all the Skia bots get a twin trybot automatically.  Just tack on '-Trybot'
to the main bot's name.