Fix failed filter followed by an affects-transparent-black filter.

Fix failed filter followed by an affects-transparent-black filter.

When an upstream filter returns null, either through failure or clipping, a downstream affects-transparent-black 
filter should still produce non-transparent pixels.

This patch fixes SkColorFilterImageFilter.

Note: this will affect the results of the imagefilterscropexpand GM.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1844593002

Committed: https://skia.googlesource.com/skia/+/6a93fa1a4526ba14782f7f17a73479ca5a4a8e3a

[Stephen White, 2016-03-29 18:03:23]

Description was changed from

==========
Fix clipped-out source followed by an affects-transparent black filter.

Add a unit test, and munge imageblurtiled GM to test this case.

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

to

==========
Fix clipped-out source followed by an affects-transparent black filter.

Add a unit test, and munge imageblurtiled GM to test this case.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Stephen White, 2016-03-29 21:47:02]

Description was changed from

==========
Fix clipped-out source followed by an affects-transparent black filter.

Add a unit test, and munge imageblurtiled GM to test this case.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Fix clipped-out source followed by an affects-transparent black filter.

Add a unit test, and munge imageblurtiled GM to test this case.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Stephen White, 2016-03-29 21:47:02]

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

[Stephen White, 2016-03-29 21:49:45]

Description was changed from

==========
Fix clipped-out source followed by an affects-transparent black filter.

Add a unit test, and munge imageblurtiled GM to test this case.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Fix failed filter followed by an affects-transparent-black filter.

When an upstream filter returns null, either through failure or clipping, a
downstream affects-transparent-black 
filter should still produce non-transparent pixels.

This patch fixes SkColorFilterImageFilter.

Note: this will affect the results of the imagefilterscropexpand GM.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Stephen White, 2016-03-29 21:50:30]

Rob: PTAL. Thanks!

[Stephen White, 2016-03-29 21:52:23]

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

[commit-bot: I haz the power, 2016-03-29 21:52:29]

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

[commit-bot: I haz the power, 2016-03-29 22:04:45]

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

[commit-bot: I haz the power, 2016-03-29 22:04:46]

Dry run: This issue passed the CQ dry run.

[robertphillips, 2016-03-30 17:12:28]

lgtm + nits & dox requests

https://codereview.chromium.org/1844593002/diff/60001/src/effects/SkColorFilt...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.chromium.org/1844593002/diff/60001/src/effects/SkColorFilt...
src/effects/SkColorFilterImageFilter.cpp:63: if
(fColorFilter->affectsTransparentBlack()) {
// If the color filter affects transparent black the bounds have to be the
entire clip

?

https://codereview.chromium.org/1844593002/diff/60001/src/effects/SkColorFilt...
src/effects/SkColorFilterImageFilter.cpp:71: 
Can we move 'bounds' down here ?

https://codereview.chromium.org/1844593002/diff/60001/src/effects/SkColorFilt...
src/effects/SkColorFilterImageFilter.cpp:91: if
(fColorFilter->affectsTransparentBlack()) {
// The following SkSpecialImage::draw call may not fill the entire canvas. This
drawPaint call insures transparent black gets handled everywhere

?

https://codereview.chromium.org/1844593002/diff/60001/src/effects/SkColorFilt...
src/effects/SkColorFilterImageFilter.cpp:93: } else {
can we move the "TODO:" comment in here ?

https://codereview.chromium.org/1844593002/diff/60001/tests/ImageFilterTest.cpp
File tests/ImageFilterTest.cpp (right):

https://codereview.chromium.org/1844593002/diff/60001/tests/ImageFilterTest.c...
tests/ImageFilterTest.cpp:677: sk_sp<SkColorFilter>
green(SkColorFilter::MakeModeFilter(SK_ColorGREEN, SkXfermode::kSrc_Mode));
SkASSERT(green->affectsTransparentBlack());

[Stephen White, 2016-03-30 19:16:01]

Thanks for your comments. Sadly, I can't land this, due to some Blink changes
which need to land first.

[Stephen White, 2016-03-30 19:16:09]

https://codereview.chromium.org/1844593002/diff/60001/src/effects/SkColorFilt...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.chromium.org/1844593002/diff/60001/src/effects/SkColorFilt...
src/effects/SkColorFilterImageFilter.cpp:93: } else {
On 2016/03/30 17:12:28, robertphillips wrote:
> can we move the "TODO:" comment in here ?

Well, technically it applies to the drawPaint(), since there's overdraw there as
well. I'll generalize the comment.

(If I felt courageous, I'd put a clip around the drawPaint & clear that excludes
the input bounds. That should take care of the overdraw.)

[Stephen White, 2016-04-04 21:50:55]

https://codereview.chromium.org/1844593002/diff/60001/src/effects/SkColorFilt...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.chromium.org/1844593002/diff/60001/src/effects/SkColorFilt...
src/effects/SkColorFilterImageFilter.cpp:63: if
(fColorFilter->affectsTransparentBlack()) {
On 2016/03/30 17:12:28, robertphillips wrote:
> // If the color filter affects transparent black the bounds have to be the
> entire clip
> 
> ?

Done. (Isn't that what the code says? :) )

https://codereview.chromium.org/1844593002/diff/60001/src/effects/SkColorFilt...
src/effects/SkColorFilterImageFilter.cpp:71: 
On 2016/03/30 17:12:28, robertphillips wrote:
> Can we move 'bounds' down here ?

Done.

https://codereview.chromium.org/1844593002/diff/60001/src/effects/SkColorFilt...
src/effects/SkColorFilterImageFilter.cpp:91: if
(fColorFilter->affectsTransparentBlack()) {
On 2016/03/30 17:12:28, robertphillips wrote:
> // The following SkSpecialImage::draw call may not fill the entire canvas.
This
> drawPaint call insures transparent black gets handled everywhere
> 
> ?

Done.

https://codereview.chromium.org/1844593002/diff/60001/src/effects/SkColorFilt...
src/effects/SkColorFilterImageFilter.cpp:93: } else {
On 2016/03/30 19:16:08, Stephen White wrote:
> On 2016/03/30 17:12:28, robertphillips wrote:
> > can we move the "TODO:" comment in here ?
> 
> Well, technically it applies to the drawPaint(), since there's overdraw there
as
> well. I'll generalize the comment.
> 
> (If I felt courageous, I'd put a clip around the drawPaint & clear that
excludes
> the input bounds. That should take care of the overdraw.)

Done.

https://codereview.chromium.org/1844593002/diff/60001/tests/ImageFilterTest.cpp
File tests/ImageFilterTest.cpp (right):

https://codereview.chromium.org/1844593002/diff/60001/tests/ImageFilterTest.c...
tests/ImageFilterTest.cpp:677: sk_sp<SkColorFilter>
green(SkColorFilter::MakeModeFilter(SK_ColorGREEN, SkXfermode::kSrc_Mode));
On 2016/03/30 17:12:28, robertphillips wrote:
> SkASSERT(green->affectsTransparentBlack());

Done.

[Stephen White, 2016-04-04 21:52:36]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2016-04-04 21:52:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1844593002/#ps140001
(title: "More nits")

[commit-bot: I haz the power, 2016-04-04 21:52:39]

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

[commit-bot: I haz the power, 2016-04-05 11:43:49]

Description was changed from

==========
Fix failed filter followed by an affects-transparent-black filter.

When an upstream filter returns null, either through failure or clipping, a
downstream affects-transparent-black 
filter should still produce non-transparent pixels.

This patch fixes SkColorFilterImageFilter.

Note: this will affect the results of the imagefilterscropexpand GM.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Fix failed filter followed by an affects-transparent-black filter.

When an upstream filter returns null, either through failure or clipping, a
downstream affects-transparent-black 
filter should still produce non-transparent pixels.

This patch fixes SkColorFilterImageFilter.

Note: this will affect the results of the imagefilterscropexpand GM.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/6a93fa1a4526ba14782f7f17a73479ca5a4a8e3a
==========

[commit-bot: I haz the power, 2016-04-05 11:43:50]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/6a93fa1a4526ba14782f7f17a73479ca5a4a8e3a