Use ComposColorFilter to collaps hierarchy (when possible).

Use ComposColorFilter to collaps hierarchy (when possible).

Clarify asColorFilter ...
1. Rename to isColorFilterNode for DAG reduction
2. Add asAColorFilter for removing the imagefilter entirely (future use-case)

Need layouttest rebaseline suppression before this can land in chrome...
https://codereview.chromium.org/984023004/


BUG=skia:

Committed: https://skia.googlesource.com/skia/+/cedc36f18b2254c5ee21f6348124886b6db4f4c2

[reed1, 2015-03-05 19:14:40]

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

[reed1, 2015-03-05 19:14:43]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-03-05 19:15:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/982933002/20001

[commit-bot: I haz the power, 2015-03-05 19:15:45]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-03-06 01:15 UTC

[Stephen White, 2015-03-05 19:39:53]

https://codereview.chromium.org/982933002/diff/20001/src/effects/SkColorFilte...
File src/effects/SkColorFilterImageFilter.cpp (right):

https://codereview.chromium.org/982933002/diff/20001/src/effects/SkColorFilte...
src/effects/SkColorFilterImageFilter.cpp:92: if (!this->cropRectIsSet() &&
!this->getInput(0)) {
I think the getInput(0) check here is wrong. We should still be returning a
color filter, even if we have an input (in order to optimize e.g. blur -> cf1 ->
cf2 into blur -> cf3).

[reed1, 2015-03-05 20:05:46]

If I'm a colorfilterimagefilter, but I also have an input, then If I return true
for asColorFilter, I'm telling the caller that I draw the same as just using my
(returned) colorfilter, but that is false, since I have an input which would
also affect the drawing.

If that doesn't make sense, lets try a VC

[reed1, 2015-03-05 20:44:29]

Eureka! I think I understand the disconnect.

I was treating asColorFilter to mean -- I can be replaced w/ a colorfilter (I
draw the same as a colorfilter)

The existing logic wanted it to mean -- I am a colorfilterimagefilter, so I can
be collapsed.

I think I will try new name(s) for the method to make each functionality more
clear.

[Stephen White, 2015-03-05 21:19:13]

On 2015/03/05 20:44:29, reed1 wrote:
> Eureka! I think I understand the disconnect.
> 
> I was treating asColorFilter to mean -- I can be replaced w/ a colorfilter (I
> draw the same as a colorfilter)
> 
> The existing logic wanted it to mean -- I am a colorfilterimagefilter, so I
can
> be collapsed.
> 
> I think I will try new name(s) for the method to make each functionality more
> clear.

Yes, exactly.

It's the difference between saying "this node can be expressed is a color
filter" vs. "the entire DAG can be (or has been) collapsed to a colorfilter".
The only difference is getInput(0) is NULL in the latter, and may not be NULL in
the former.

[reed1, 2015-03-05 22:21:16]

ptal

[Stephen White, 2015-03-06 15:07:16]

LGTM

https://codereview.chromium.org/982933002/diff/40001/include/core/SkImageFilt...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/982933002/diff/40001/include/core/SkImageFilt...
include/core/SkImageFilter.h:160: return this->countInputs() > 0 &&
Nit: since the number of inputs to a CFIF is fixed to 1 at compile time (and
we're now asserting it in isColorFilterNode()), this check seems superfluous.

https://codereview.chromium.org/982933002/diff/40001/include/core/SkImageFilt...
include/core/SkImageFilter.h:297: return false;
Nit: declaring a virtual function inline is useless on a class which cannot be
stack-allocated or aggregated (or whose type can otherwise be known at compile
time), which is the case for image filters which are always created via
factories.

[reed1, 2015-03-06 16:05:51]

https://codereview.chromium.org/982933002/diff/40001/include/core/SkImageFilt...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/982933002/diff/40001/include/core/SkImageFilt...
include/core/SkImageFilter.h:160: return this->countInputs() > 0 &&
On 2015/03/06 15:07:16, Stephen White wrote:
> Nit: since the number of inputs to a CFIF is fixed to 1 at compile time (and
> we're now asserting it in isColorFilterNode()), this check seems superfluous.

In this context, "this" is not known to be CFIF, so I can't assume the number of
inputs, right?

[reed1, 2015-03-06 16:08:00]

https://codereview.chromium.org/982933002/diff/40001/include/core/SkImageFilt...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/982933002/diff/40001/include/core/SkImageFilt...
include/core/SkImageFilter.h:297: return false;
On 2015/03/06 15:07:16, Stephen White wrote:
> Nit: declaring a virtual function inline is useless on a class which cannot be
> stack-allocated or aggregated (or whose type can otherwise be known at compile
> time), which is the case for image filters which are always created via
> factories.

Perhaps it is useless in those cases, but the "requirement" that we only use
factories is not inherent in the class per-se, thus it seems fragile to
implicitly bake that assumption in here.

[Stephen White, 2015-03-06 16:15:49]

https://codereview.chromium.org/982933002/diff/40001/include/core/SkImageFilt...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/982933002/diff/40001/include/core/SkImageFilt...
include/core/SkImageFilter.h:160: return this->countInputs() > 0 &&
On 2015/03/06 16:05:50, reed1 wrote:
> On 2015/03/06 15:07:16, Stephen White wrote:
> > Nit: since the number of inputs to a CFIF is fixed to 1 at compile time (and
> > we're now asserting it in isColorFilterNode()), this check seems
superfluous.
> 
> In this context, "this" is not known to be CFIF, so I can't assume the number
of
> inputs, right?

Whoops, you're right.

I guess I'd like this function better if it was just:

this->isColorFilterNode(filterPtr) && NULL == this->getInput(0);

[reed1, 2015-03-06 16:18:46]

The prev change was reverted (just due to need to suppress layoutchanges (which
were fine), so I'm having to manually rebase that and this change together...

[reed1, 2015-03-06 16:19:46]

On 2015/03/06 16:15:49, Stephen White wrote:
>
https://codereview.chromium.org/982933002/diff/40001/include/core/SkImageFilt...
> File include/core/SkImageFilter.h (right):
> 
>
https://codereview.chromium.org/982933002/diff/40001/include/core/SkImageFilt...
> include/core/SkImageFilter.h:160: return this->countInputs() > 0 &&
> On 2015/03/06 16:05:50, reed1 wrote:
> > On 2015/03/06 15:07:16, Stephen White wrote:
> > > Nit: since the number of inputs to a CFIF is fixed to 1 at compile time
(and
> > > we're now asserting it in isColorFilterNode()), this check seems
> superfluous.
> > 
> > In this context, "this" is not known to be CFIF, so I can't assume the
number
> of
> > inputs, right?
> 
> Whoops, you're right.
> 
> I guess I'd like this function better if it was just:
> 
> this->isColorFilterNode(filterPtr) && NULL == this->getInput(0);

I also thought it was slightly faster to perform the input check first, before
calling the virtual.

[reed1, 2015-03-06 16:39:32]

The CQ bit was checked by reed@google.com

[reed1, 2015-03-06 16:39:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
senorblanco@chromium.org
Link to the patchset: https://codereview.chromium.org/982933002/#ps80001 (title:
"fix unused parameter warning")

[commit-bot: I haz the power, 2015-03-06 16:40:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/982933002/80001

[reed1, 2015-03-06 16:46:30]

The CQ bit was unchecked by reed@google.com

[reed2, 2015-03-08 11:36:07]

The CQ bit was checked by reed@chromium.org

[commit-bot: I haz the power, 2015-03-08 11:37:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/982933002/80001

[commit-bot: I haz the power, 2015-03-08 11:42:56]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/cedc36f18b2254c5ee21f6348124886b6db4f4c2