Get OffsetImageFilter really working with SkSpecialImages

Get OffsetImageFilter really working with SkSpecialImages
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1695823002

[robertphillips, 2016-02-12 19:49:35]

Description was changed from

==========
Get OffsetImageFilter really working with SkSpecialImages
==========

to

==========
Get OffsetImageFilter really working with SkSpecialImages
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Stephen White, 2016-02-18 17:56:17]

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

[Stephen White, 2016-02-18 17:56:18]

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

https://codereview.chromium.org/1695823002/diff/180001/src/core/SkImageFilter...
src/core/SkImageFilter.cpp:586: if (!src->internal_getBM(&tmpSrc)) {
Would it work to put this chunk of code into the base class implementation of
SkImageFilter::filterImage() instead? Then we wouldn't need the
supportsUnifiedFiltering() virtual.

https://codereview.chromium.org/1695823002/diff/180001/src/effects/SkOffsetIm...
File src/effects/SkOffsetImageFilter.cpp (right):

https://codereview.chromium.org/1695823002/diff/180001/src/effects/SkOffsetIm...
src/effects/SkOffsetImageFilter.cpp:45: // TODO: need to plumb MSAA sampleCount
down to here!
I don't think we should (except for the original primitive drawn by saveLayer,
which doesn't go through this path), but I might be missing a special case.

Maybe we should just remove this comment and keep an eye on the GM results?

https://codereview.chromium.org/1695823002/diff/180001/src/effects/SkOffsetIm...
src/effects/SkOffsetImageFilter.cpp:68: bool
SkOffsetImageFilter::onFilterImageDeprecated(Proxy* proxy, const SkBitmap&
source,
Can we not simply remove this function now? When does this get called?

[robertphillips, 2016-02-19 19:37:59]

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

https://codereview.chromium.org/1695823002/diff/180001/src/core/SkImageFilter...
src/core/SkImageFilter.cpp:586: if (!src->internal_getBM(&tmpSrc)) {
On 2016/02/18 17:56:17, Stephen White wrote:
> Would it work to put this chunk of code into the base class implementation of
> SkImageFilter::filterImage() instead? Then we wouldn't need the
> supportsUnifiedFiltering() virtual.

Done. It had to go in onFilterImage though. This does mean the caching code gets
hit twice, once in SkImageFilter::filterImage and (for those imagefilters that
don't implement their own onFilterImage) once in SkImageFilter::onFilterImage.

https://codereview.chromium.org/1695823002/diff/180001/src/effects/SkOffsetIm...
File src/effects/SkOffsetImageFilter.cpp (right):

https://codereview.chromium.org/1695823002/diff/180001/src/effects/SkOffsetIm...
src/effects/SkOffsetImageFilter.cpp:45: // TODO: need to plumb MSAA sampleCount
down to here!
On 2016/02/18 17:56:18, Stephen White wrote:
> I don't think we should (except for the original primitive drawn by saveLayer,
> which doesn't go through this path), but I might be missing a special case.
> 
> Maybe we should just remove this comment and keep an eye on the GM results?

Done. I agree that all the draws performed by the image filters shouldn't need
MSAA.

https://codereview.chromium.org/1695823002/diff/180001/src/effects/SkOffsetIm...
src/effects/SkOffsetImageFilter.cpp:68: bool
SkOffsetImageFilter::onFilterImageDeprecated(Proxy* proxy, const SkBitmap&
source,
On 2016/02/18 17:56:18, Stephen White wrote:
> Can we not simply remove this function now? When does this get called?

It can still be called when the offset image filter is the root of the DAG
(e.g., from within SkCanvas::internalDrawDevice).

[Stephen White, 2016-02-19 19:53:56]

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

https://codereview.chromium.org/1695823002/diff/180001/src/core/SkImageFilter...
src/core/SkImageFilter.cpp:586: if (!src->internal_getBM(&tmpSrc)) {
On 2016/02/19 19:37:59, robertphillips wrote:
> On 2016/02/18 17:56:17, Stephen White wrote:
> > Would it work to put this chunk of code into the base class implementation
of
> > SkImageFilter::filterImage() instead? Then we wouldn't need the
> > supportsUnifiedFiltering() virtual.
> 
> Done. It had to go in onFilterImage though.

Sorry, that's actually what I meant!

> This does mean the caching code gets
> hit twice, once in SkImageFilter::filterImage and (for those imagefilters that
> don't implement their own onFilterImage) once in SkImageFilter::onFilterImage.

That's probably bearable in the short term.

https://codereview.chromium.org/1695823002/diff/180001/src/effects/SkOffsetIm...
File src/effects/SkOffsetImageFilter.cpp (right):

https://codereview.chromium.org/1695823002/diff/180001/src/effects/SkOffsetIm...
src/effects/SkOffsetImageFilter.cpp:68: bool
SkOffsetImageFilter::onFilterImageDeprecated(Proxy* proxy, const SkBitmap&
source,
On 2016/02/19 19:37:59, robertphillips wrote:
> On 2016/02/18 17:56:18, Stephen White wrote:
> > Can we not simply remove this function now? When does this get called?
> 
> It can still be called when the offset image filter is the root of the DAG
> (e.g., from within SkCanvas::internalDrawDevice).

Could we also switch over the callsites (in SkCanvas and SkGpuDevice) to use
filterImage() in this patch? Then we should be able to remove the legacy path
from each filter as we go. Would that work?

[robertphillips, 2016-02-22 15:21:58]

https://codereview.chromium.org/1695823002/diff/180001/src/effects/SkOffsetIm...
File src/effects/SkOffsetImageFilter.cpp (right):

https://codereview.chromium.org/1695823002/diff/180001/src/effects/SkOffsetIm...
src/effects/SkOffsetImageFilter.cpp:68: bool
SkOffsetImageFilter::onFilterImageDeprecated(Proxy* proxy, const SkBitmap&
source,
I think that should probably be its own patch. We could land the SkImageFilter.*
changes, then that patch, then a revised version of the SkOffsetImageFilter.*
files.

[Stephen White, 2016-02-22 15:42:38]

https://codereview.chromium.org/1695823002/diff/180001/src/effects/SkOffsetIm...
File src/effects/SkOffsetImageFilter.cpp (right):

https://codereview.chromium.org/1695823002/diff/180001/src/effects/SkOffsetIm...
src/effects/SkOffsetImageFilter.cpp:68: bool
SkOffsetImageFilter::onFilterImageDeprecated(Proxy* proxy, const SkBitmap&
source,
On 2016/02/22 15:21:58, robertphillips wrote:
> I think that should probably be its own patch. We could land the
SkImageFilter.*
> changes, then that patch, then a revised version of the SkOffsetImageFilter.*
> files.

OK, just to clarify: you're saying

1) Land the SkImageFilter.* changes from this patch
2) Land the infrastructure patch (SkCanvas/SkGpuDevice)
3) Land the (updated) SkOffsetImageFilter changes from this patch
*) Convert the remaining filters

?

If so, I'm good with that.

[robertphillips, 2016-03-04 18:50:09]

Description was changed from

==========
Get OffsetImageFilter really working with SkSpecialImages
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Get OffsetImageFilter really working with SkSpecialImages
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[robertphillips, 2016-03-04 18:51:38]

Closing in favor of:

https://codereview.chromium.org/1757983002/ (Add SkSpecialImage-based methods to
SkImageFilter)
https://codereview.chromium.org/1762013002/ (Swap over to using
SkImageFilter::filterImage instead of filterImageDeprecated)
https://codereview.chromium.org/1766743003/ (Switch SkOffsetImageFilter over to
new onFilterImage interface)