Matrix convolution bounds fix; affectsTransparentBlack fixes.

Matrix convolution bounds fix; affectsTransparentBlack fixes.

Because the convolution kernel is (currently) applied in device space,
there's no way to know which object-space pixels will be touched. So
return false from canComputeFastBounds().

The results from the matrixconvolution GM were actually wrong, since
they were showing edge differences on the clip boundaries, where they
should really only show on crop boundaries. I added a crop to the GM
to keep the results the same (which are useful to test the different
convolution tile modes).

While I was at it, SkImageFilter::affectsTransparentBlack() was
inapplicable on most things except color filters, and its use on
leaf nodes was confusing. So I removed it, and made
SkImageFilter::canComputeFastBounds() virtual instead.

BUG=skia:4630
Committed: https://skia.googlesource.com/skia/+/a544eda5ddea215037b1ada6ba5cfc98f6c8ee15

[Stephen White, 2015-12-04 20:42:19]

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

[Stephen White, 2015-12-04 20:42:20]

reed@: PTAL. Thanks!

[reed1, 2015-12-04 21:01:03]

lgtm w/  request for comment

https://codereview.chromium.org/1500923004/diff/20001/src/effects/SkRectShade...
File src/effects/SkRectShaderImageFilter.cpp (right):

https://codereview.chromium.org/1500923004/diff/20001/src/effects/SkRectShade...
src/effects/SkRectShaderImageFilter.cpp:82: bool
SkRectShaderImageFilter::canComputeFastBounds() const {
nit: a comment explaining why we can't compute fast?

[Stephen White, 2015-12-04 21:39:26]

https://codereview.chromium.org/1500923004/diff/20001/src/effects/SkRectShade...
File src/effects/SkRectShaderImageFilter.cpp (right):

https://codereview.chromium.org/1500923004/diff/20001/src/effects/SkRectShade...
src/effects/SkRectShaderImageFilter.cpp:82: bool
SkRectShaderImageFilter::canComputeFastBounds() const {
On 2015/12/04 21:01:03, reed1 wrote:
> nit: a comment explaining why we can't compute fast?

Done.

[Stephen White, 2015-12-04 21:39:37]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-12-04 21:39:38]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1500923004/#ps40001
(title: "Add a comment to SkRectShaderImageFilter::canComputeFastBounds()")

[commit-bot: I haz the power, 2015-12-04 21:39:44]

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

[commit-bot: I haz the power, 2015-12-04 21:42:25]

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

[commit-bot: I haz the power, 2015-12-04 21:42:25]

Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)

[Stephen White, 2015-12-04 21:44:29]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-12-04 21:44:29]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1500923004/#ps60001
(title: "Win32 fix")

[commit-bot: I haz the power, 2015-12-04 21:44:34]

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

[commit-bot: I haz the power, 2015-12-04 21:57:35]

Description was changed from

==========
Matrix convolution bounds fix; affectsTransparentBlack fixes.

Because the convolution kernel is (currently) applied in device space,
there's no way to know which object-space pixels will be touched. So
return false from canComputeFastBounds().

The results from the matrixconvolution GM were actually wrong, since
they were showing edge differences on the clip boundaries, where they
should really only show on crop boundaries. I added a crop to the GM
to keep the results the same (which are useful to test the different
convolution tile modes).

While I was at it, SkImageFilter::affectsTransparentBlack() was
inapplicable on most things except color filters, and its use on
leaf nodes was confusing. So I removed it, and made
SkImageFilter::canComputeFastBounds() virtual instead.

BUG=skia:
==========

to

==========
Matrix convolution bounds fix; affectsTransparentBlack fixes.

Because the convolution kernel is (currently) applied in device space,
there's no way to know which object-space pixels will be touched. So
return false from canComputeFastBounds().

The results from the matrixconvolution GM were actually wrong, since
they were showing edge differences on the clip boundaries, where they
should really only show on crop boundaries. I added a crop to the GM
to keep the results the same (which are useful to test the different
convolution tile modes).

While I was at it, SkImageFilter::affectsTransparentBlack() was
inapplicable on most things except color filters, and its use on
leaf nodes was confusing. So I removed it, and made
SkImageFilter::canComputeFastBounds() virtual instead.

BUG=skia:

Committed:
https://skia.googlesource.com/skia/+/8705ec80518ef551994b82ca5ccaeb0241d6adec
==========

[commit-bot: I haz the power, 2015-12-04 21:57:36]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/8705ec80518ef551994b82ca5ccaeb0241d6adec

[reed2, 2015-12-05 02:03:15]

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

[reed2, 2015-12-05 02:03:16]

Do we need a chrome guard for this change, given the results from the DEPS roll?

https://codereview.chromium.org/1500133002
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...

[reed2, 2015-12-05 03:28:27]

I think the change in order, where now we call isColorFilterNode before calling
affectsTransparentBlack, might have caused the memory leak

http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-Golo-GPU-GT6...

isColorFilterNode will ref() is parameter. If affectsTransparentBlack returns
false, then the caller will not know to unref the parameter, and we will leak.

https://codereview.chromium.org/1500923004/diff/60001/include/core/SkImageFil...
File include/core/SkImageFilter.h (left):

https://codereview.chromium.org/1500923004/diff/60001/include/core/SkImageFil...
include/core/SkImageFilter.h:205: !this->affectsTransparentBlack() &&
Notice that here, we check for affectsTransparentBlack, and then call
isColorFilterNode...

https://codereview.chromium.org/1500923004/diff/60001/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/1500923004/diff/60001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:391: this->isColorFilterNode(filterPtr) &&
Here, unlike the old version, we call isColorFilterNode *before* we check
affectsTransparentBlack.

[Stephen White, 2015-12-05 13:58:35]

Whoops; thanks for the investigation. Will revert.

[Stephen White, 2015-12-05 13:59:31]

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

The reason for reverting is: Introduced memory leak; pixel changes in Chrome..

[Stephen White, 2015-12-07 15:02:28]

Description was changed from

==========
Matrix convolution bounds fix; affectsTransparentBlack fixes.

Because the convolution kernel is (currently) applied in device space,
there's no way to know which object-space pixels will be touched. So
return false from canComputeFastBounds().

The results from the matrixconvolution GM were actually wrong, since
they were showing edge differences on the clip boundaries, where they
should really only show on crop boundaries. I added a crop to the GM
to keep the results the same (which are useful to test the different
convolution tile modes).

While I was at it, SkImageFilter::affectsTransparentBlack() was
inapplicable on most things except color filters, and its use on
leaf nodes was confusing. So I removed it, and made
SkImageFilter::canComputeFastBounds() virtual instead.

BUG=skia:

Committed:
https://skia.googlesource.com/skia/+/8705ec80518ef551994b82ca5ccaeb0241d6adec
==========

to

==========
Matrix convolution bounds fix; affectsTransparentBlack fixes.

Because the convolution kernel is (currently) applied in device space,
there's no way to know which object-space pixels will be touched. So
return false from canComputeFastBounds().

The results from the matrixconvolution GM were actually wrong, since
they were showing edge differences on the clip boundaries, where they
should really only show on crop boundaries. I added a crop to the GM
to keep the results the same (which are useful to test the different
convolution tile modes).

While I was at it, SkImageFilter::affectsTransparentBlack() was
inapplicable on most things except color filters, and its use on
leaf nodes was confusing. So I removed it, and made
SkImageFilter::canComputeFastBounds() virtual instead.

BUG=skia:

Committed:
https://skia.googlesource.com/skia/+/8705ec80518ef551994b82ca5ccaeb0241d6adec
==========

[Stephen White, 2015-12-07 15:02:35]

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

[commit-bot: I haz the power, 2015-12-07 15:02:38]

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

[commit-bot: I haz the power, 2015-12-07 15:15:01]

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

[commit-bot: I haz the power, 2015-12-07 15:15:02]

Dry run: This issue passed the CQ dry run.

[Stephen White, 2015-12-07 15:20:24]

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

[commit-bot: I haz the power, 2015-12-07 15:20:36]

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

[Stephen White, 2015-12-07 15:26:51]

Description was changed from

==========
Matrix convolution bounds fix; affectsTransparentBlack fixes.

Because the convolution kernel is (currently) applied in device space,
there's no way to know which object-space pixels will be touched. So
return false from canComputeFastBounds().

The results from the matrixconvolution GM were actually wrong, since
they were showing edge differences on the clip boundaries, where they
should really only show on crop boundaries. I added a crop to the GM
to keep the results the same (which are useful to test the different
convolution tile modes).

While I was at it, SkImageFilter::affectsTransparentBlack() was
inapplicable on most things except color filters, and its use on
leaf nodes was confusing. So I removed it, and made
SkImageFilter::canComputeFastBounds() virtual instead.

BUG=skia:

Committed:
https://skia.googlesource.com/skia/+/8705ec80518ef551994b82ca5ccaeb0241d6adec
==========

to

==========
Matrix convolution bounds fix; affectsTransparentBlack fixes.

Because the convolution kernel is (currently) applied in device space,
there's no way to know which object-space pixels will be touched. So
return false from canComputeFastBounds().

The results from the matrixconvolution GM were actually wrong, since
they were showing edge differences on the clip boundaries, where they
should really only show on crop boundaries. I added a crop to the GM
to keep the results the same (which are useful to test the different
convolution tile modes).

While I was at it, SkImageFilter::affectsTransparentBlack() was
inapplicable on most things except color filters, and its use on
leaf nodes was confusing. So I removed it, and made
SkImageFilter::canComputeFastBounds() virtual instead.

BUG=skia:4630
==========

[commit-bot: I haz the power, 2015-12-07 15:35:02]

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

[commit-bot: I haz the power, 2015-12-07 15:35:03]

Dry run: This issue passed the CQ dry run.

[Stephen White, 2015-12-07 15:47:52]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-12-07 15:47:53]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/1500923004/#ps100001
(title: "Alternate, more future-proof fix")

[commit-bot: I haz the power, 2015-12-07 15:47:56]

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

[commit-bot: I haz the power, 2015-12-07 15:48:36]

Description was changed from

==========
Matrix convolution bounds fix; affectsTransparentBlack fixes.

Because the convolution kernel is (currently) applied in device space,
there's no way to know which object-space pixels will be touched. So
return false from canComputeFastBounds().

The results from the matrixconvolution GM were actually wrong, since
they were showing edge differences on the clip boundaries, where they
should really only show on crop boundaries. I added a crop to the GM
to keep the results the same (which are useful to test the different
convolution tile modes).

While I was at it, SkImageFilter::affectsTransparentBlack() was
inapplicable on most things except color filters, and its use on
leaf nodes was confusing. So I removed it, and made
SkImageFilter::canComputeFastBounds() virtual instead.

BUG=skia:4630
==========

to

==========
Matrix convolution bounds fix; affectsTransparentBlack fixes.

Because the convolution kernel is (currently) applied in device space,
there's no way to know which object-space pixels will be touched. So
return false from canComputeFastBounds().

The results from the matrixconvolution GM were actually wrong, since
they were showing edge differences on the clip boundaries, where they
should really only show on crop boundaries. I added a crop to the GM
to keep the results the same (which are useful to test the different
convolution tile modes).

While I was at it, SkImageFilter::affectsTransparentBlack() was
inapplicable on most things except color filters, and its use on
leaf nodes was confusing. So I removed it, and made
SkImageFilter::canComputeFastBounds() virtual instead.

BUG=skia:4630

Committed:
https://skia.googlesource.com/skia/+/a544eda5ddea215037b1ada6ba5cfc98f6c8ee15
==========

[commit-bot: I haz the power, 2015-12-07 15:48:37]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/a544eda5ddea215037b1ada6ba5cfc98f6c8ee15