Simplify flattening to just write enough to call the factory

Simplify flattening to just write enough to call the factory/public-constructor for the class. We want to *not* rely on private constructors, and not rely on calling through the inheritance hierarchy for either flattening or unflattening(CreateProc).

Refactoring pattern:

1. guard the existing constructor(readbuffer) with the legacy build-flag
2. If you are a instancable subclass, implement CreateProc(readbuffer) to create a new instances from the buffer params (or return NULL).

If you're a shader subclass
1. You must read/write the local matrix if your class accepts that in its factory/constructor, else ignore it.

Committed: https://skia.googlesource.com/skia/+/9fa60daad4d5f54c0dbe3dbcc7608a8f6d721187

[reed1, 2014-07-15 19:31:58]

not finished, but well on its way

[reed1, 2014-07-17 21:06:05]

have now implemented ~3 imagefilters. Those might be worth looking at, to see if
I'm on the right path for applying this pattern to them.

[Stephen White, 2014-07-17 21:29:00]

Direction looks good. Comments are nits.

https://codereview.chromium.org/395603002/diff/40001/include/core/SkColorFilt...
File include/core/SkColorFilter.h (right):

https://codereview.chromium.org/395603002/diff/40001/include/core/SkColorFilt...
include/core/SkColorFilter.h:138: #ifdef SK_SUPPORT_LEGACY_DEEPFLATTENING
Nanonit: Isn't this unflattening? ;)

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

https://codereview.chromium.org/395603002/diff/40001/include/core/SkImageFilt...
include/core/SkImageFilter.h:241: void flattenCommon(SkWriteBuffer&) const;
Now that we have a flattenCommon(), would it make sense to move
Common:unflatten() to SkImageFilter::unflattenCommon() for symmetry? Might
require making Common's members public, but it basically is a struct already.
Just a thought.

https://codereview.chromium.org/395603002/diff/40001/src/effects/SkBlurImageF...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/395603002/diff/40001/src/effects/SkBlurImageF...
src/effects/SkBlurImageFilter.cpp:53: return Create(sigmaX, sigmaY,
common.inputs()[0], &common.cropRect());
Adding a getInput(index) to Common might make this a little less ugly.

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

https://codereview.chromium.org/395603002/diff/40001/src/effects/SkColorFilte...
src/effects/SkColorFilterImageFilter.cpp:100: if (common.inputCount() > 1 ||
NULL == cf.get()) {
Nit: if we did pass the expected count to Common's constructor (or unflatten),
we wouldn't need to check it here.

[reed1, 2014-07-18 13:40:30]

https://codereview.chromium.org/395603002/diff/40001/include/core/SkColorFilt...
File include/core/SkColorFilter.h (right):

https://codereview.chromium.org/395603002/diff/40001/include/core/SkColorFilt...
include/core/SkColorFilter.h:138: #ifdef SK_SUPPORT_LEGACY_DEEPFLATTENING
On 2014/07/17 21:29:00, Stephen White wrote:
> Nanonit: Isn't this unflattening? ;)

The flattened format is no longer "deep", in that we no longer must pass the
flattening/unflatten call-chain down the inheritance tree.

https://codereview.chromium.org/395603002/diff/40001/src/effects/SkBlurImageF...
File src/effects/SkBlurImageFilter.cpp (right):

https://codereview.chromium.org/395603002/diff/40001/src/effects/SkBlurImageF...
src/effects/SkBlurImageFilter.cpp:53: return Create(sigmaX, sigmaY,
common.inputs()[0], &common.cropRect());
On 2014/07/17 21:29:00, Stephen White wrote:
> Adding a getInput(index) to Common might make this a little less ugly.

good idea.

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

https://codereview.chromium.org/395603002/diff/40001/src/effects/SkColorFilte...
src/effects/SkColorFilterImageFilter.cpp:100: if (common.inputCount() > 1 ||
NULL == cf.get()) {
On 2014/07/17 21:29:00, Stephen White wrote:
> Nit: if we did pass the expected count to Common's constructor (or unflatten),
> we wouldn't need to check it here.

The input-filter appears to be optional, reading the constructor on line 60. Is
that not the case?

[Stephen White, 2014-07-18 14:06:20]

https://codereview.chromium.org/395603002/diff/40001/include/core/SkColorFilt...
File include/core/SkColorFilter.h (right):

https://codereview.chromium.org/395603002/diff/40001/include/core/SkColorFilt...
include/core/SkColorFilter.h:138: #ifdef SK_SUPPORT_LEGACY_DEEPFLATTENING
On 2014/07/18 13:40:29, reed1 wrote:
> On 2014/07/17 21:29:00, Stephen White wrote:
> > Nanonit: Isn't this unflattening? ;)
> 
> The flattened format is no longer "deep", in that we no longer must pass the
> flattening/unflatten call-chain down the inheritance tree.

I misread this as "deflattening". Nix that nit.

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

https://codereview.chromium.org/395603002/diff/40001/src/effects/SkColorFilte...
src/effects/SkColorFilterImageFilter.cpp:100: if (common.inputCount() > 1 ||
NULL == cf.get()) {
On 2014/07/18 13:40:30, reed1 wrote:
> On 2014/07/17 21:29:00, Stephen White wrote:
> > Nit: if we did pass the expected count to Common's constructor (or
unflatten),
> > we wouldn't need to check it here.
> 
> The input-filter appears to be optional, reading the constructor on line 60.
Is
> that not the case?

Yes, it's optional, in the sense that it can be NULL, but the array should be of
size 1. We call getInput(0) unconditionally during processing (as do all the
filters), and getInput() asserts on out-of-bounds. In fact, I think it's quite
important that we make the expectedCount arg of Common::unflatten() non-default,
as it was before, for this reason.

[reed1, 2014-07-18 14:13:37]

On 2014/07/18 14:06:20, Stephen White wrote:
>
https://codereview.chromium.org/395603002/diff/40001/include/core/SkColorFilt...
> File include/core/SkColorFilter.h (right):
> 
>
https://codereview.chromium.org/395603002/diff/40001/include/core/SkColorFilt...
> include/core/SkColorFilter.h:138: #ifdef SK_SUPPORT_LEGACY_DEEPFLATTENING
> On 2014/07/18 13:40:29, reed1 wrote:
> > On 2014/07/17 21:29:00, Stephen White wrote:
> > > Nanonit: Isn't this unflattening? ;)
> > 
> > The flattened format is no longer "deep", in that we no longer must pass the
> > flattening/unflatten call-chain down the inheritance tree.
> 
> I misread this as "deflattening". Nix that nit.
> 
>
https://codereview.chromium.org/395603002/diff/40001/src/effects/SkColorFilte...
> File src/effects/SkColorFilterImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/395603002/diff/40001/src/effects/SkColorFilte...
> src/effects/SkColorFilterImageFilter.cpp:100: if (common.inputCount() > 1 ||
> NULL == cf.get()) {
> On 2014/07/18 13:40:30, reed1 wrote:
> > On 2014/07/17 21:29:00, Stephen White wrote:
> > > Nit: if we did pass the expected count to Common's constructor (or
> unflatten),
> > > we wouldn't need to check it here.
> > 
> > The input-filter appears to be optional, reading the constructor on line 60.
> Is
> > that not the case?
> 
> Yes, it's optional, in the sense that it can be NULL, but the array should be
of
> size 1. We call getInput(0) unconditionally during processing (as do all the
> filters), and getInput() asserts on out-of-bounds. In fact, I think it's quite
> important that we make the expectedCount arg of Common::unflatten()
non-default,
> as it was before, for this reason.

Good catch. I'm correcting that mistake now.

[Stephen White, 2014-07-18 14:17:21]

BTW, I'm wondering if we should remove all the calls to 
buffer.validate(), and simply take the NULL return from 
CreateProc() to mean that we read something invalid, and have 
the calling function in the reader set the buffer invalid, just
once. Seems like that would clean up a lot of code. WDYT?

[reed1, 2014-07-18 14:28:34]

uploaded version that requires specified number of inputs, but those elements
may be NULL.

[reed1, 2014-07-18 14:41:52]

On 2014/07/18 14:17:21, Stephen White wrote:
> BTW, I'm wondering if we should remove all the calls to 
> buffer.validate(), and simply take the NULL return from 
> CreateProc() to mean that we read something invalid, and have 
> the calling function in the reader set the buffer invalid, just
> once. Seems like that would clean up a lot of code. WDYT?

I'm starting to think the same thing.

[Stephen White, 2014-07-18 15:43:32]

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

https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
src/effects/SkColorFilterImageFilter.cpp:99: return Create(cf,
common.inputAt(0), &common.cropRect());
I see you've removed the validate() here (which is good), 
but you've also removed the early return. If the color 
filter fails to read, for example, the cfif Create() function 
will assert. I think we should keep in the early-returns, 
just remove all the validate()s.

Also, Alexis suggests that we might want to do one 
"if (!buffer.isValid()) return NULL;" just before calling each
Create(), since even buffer.readInt() might fail, and
there's no point in constructing an object which we know
will be thrown away anyway.

[sugoi1, 2014-07-18 15:51:58]

On 2014/07/18 15:43:32, Stephen White wrote:
>
https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
> File src/effects/SkColorFilterImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
> src/effects/SkColorFilterImageFilter.cpp:99: return Create(cf,
> common.inputAt(0), &common.cropRect());
> I see you've removed the validate() here (which is good), 
> but you've also removed the early return. If the color 
> filter fails to read, for example, the cfif Create() function 
> will assert. I think we should keep in the early-returns, 
> just remove all the validate()s.
> 
> Also, Alexis suggests that we might want to do one 
> "if (!buffer.isValid()) return NULL;" just before calling each
> Create(), since even buffer.readInt() might fail, and
> there's no point in constructing an object which we know
> will be thrown away anyway.

If there was a way to centralize that check somehow, so that we don't have to
remember to manually call it before each Create() function, that would be great
(Maybe as a function or macro, if necessary, in SkFlattenable).

[reed1, 2014-07-18 16:18:47]

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

https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
src/effects/SkColorFilterImageFilter.cpp:99: return Create(cf,
common.inputAt(0), &common.cropRect());
On 2014/07/18 15:43:32, Stephen White wrote:
> I see you've removed the validate() here (which is good), 
> but you've also removed the early return. If the color 
> filter fails to read, for example, the cfif Create() function 
> will assert. I think we should keep in the early-returns, 
> just remove all the validate()s.
> 
> Also, Alexis suggests that we might want to do one 
> "if (!buffer.isValid()) return <span id="abbrex-spnAbbr_1473"
class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">NULL</span>;" just before calling each
> Create(), since even buffer.readInt() might fail, and
> there's no point in constructing an object which we know
> will be thrown away anyway.

It is a new question about where/when to perform validation on the input
parameters. If we did runtime checks in the Factories (e.g. null-checks,
NaN-checks) in the Factory, we could skip those steps in the CreateProc and only
check for bad-buffers. We would then have all the checks in one place, and our
public factories would be that much saner...

Checking every time before calling a Create isn't wrong it would seem, but I
wonder if it is required. Seems like the final caller of the entire stream
can/should always check the state of the buffer at the end, and can abort the
world if it is invalid. I just want to avoid lots of extra noise in each
CreateProc if we don't absolutely require it. Lets keep thinking on this.

[Stephen White, 2014-07-18 17:05:26]

On 2014/07/18 16:18:47, reed1 wrote:
>
https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
> File src/effects/SkColorFilterImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
> src/effects/SkColorFilterImageFilter.cpp:99: return Create(cf,
> common.inputAt(0), &common.cropRect());
> On 2014/07/18 15:43:32, Stephen White wrote:
> > I see you've removed the validate() here (which is good), 
> > but you've also removed the early return. If the color 
> > filter fails to read, for example, the cfif Create() function 
> > will assert. I think we should keep in the early-returns, 
> > just remove all the validate()s.
> > 
> > Also, Alexis suggests that we might want to do one 
> > "if (!buffer.isValid()) return <span id="abbrex-spnAbbr_1473"
> class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown
> abbrex-AbbrUnknownVisible">NULL</span>;" just before calling each
> > Create(), since even buffer.readInt() might fail, and
> > there's no point in constructing an object which we know
> > will be thrown away anyway.
> 
> It is a new question about where/when to perform validation on the input
> parameters. If we did runtime checks in the Factories (e.g. null-checks,
> NaN-checks) in the Factory, we could skip those steps in the CreateProc and
only
> check for bad-buffers. We would then have all the checks in one place, and our
> public factories would be that much saner...
> 
> Checking every time before calling a Create isn't wrong it would seem, but I
> wonder if it is required. Seems like the final caller of the entire stream
> can/should always check the state of the buffer at the end, and can abort the
> world if it is invalid. I just want to avoid lots of extra noise in each
> CreateProc if we don't absolutely require it. Lets keep thinking on this.

There are two cases, as I see it: buffer.readFoo() fails (e.g., due to a
truncated buffer). The factories can't handle this, because they don't
know about the buffer, so it has to be handled in the CreateProc, perhaps 
with a single call to !buffer.isValid() before the factory call.

The other case is that the parameter is read, but invalid. For things like
out-of-range enums, etc, that should definitely be checked in the factory, 
and not the create proc, but that's not the case today. So in this patch we
should either beef up the factory's resilience, or leave in some of the
checks in the deserialization. Only going part way leaves us vulnerable,
I think.

As a side note, there are probably a couple of checks in the deserialization 
which are superfluous. For example, now that all filters scale their params 
by the CTM, checking for a negative blur radius is not helpful, since it 
could be invalidated by a negative scale in the CTM at runtime. We should 
remove these checks, and ensure that the processing code is doing the checks
instead.

[reed1, 2014-07-18 17:27:28]

On 2014/07/18 17:05:26, Stephen White wrote:
> On 2014/07/18 16:18:47, reed1 wrote:
> >
>
https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
> > File src/effects/SkColorFilterImageFilter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
> > src/effects/SkColorFilterImageFilter.cpp:99: return Create(cf,
> > common.inputAt(0), &common.cropRect());
> > On 2014/07/18 15:43:32, Stephen White wrote:
> > > I see you've removed the validate() here (which is good), 
> > > but you've also removed the early return. If the color 
> > > filter fails to read, for example, the cfif Create() function 
> > > will assert. I think we should keep in the early-returns, 
> > > just remove all the validate()s.
> > > 
> > > Also, Alexis suggests that we might want to do one 
> > > "if (!buffer.isValid()) return <span id="abbrex-spnAbbr_1473"
> > class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown
> > abbrex-AbbrUnknownVisible">NULL</span>;" just before calling each
> > > Create(), since even buffer.readInt() might fail, and
> > > there's no point in constructing an object which we know
> > > will be thrown away anyway.
> > 
> > It is a new question about where/when to perform validation on the input
> > parameters. If we did runtime checks in the Factories (e.g. null-checks,
> > NaN-checks) in the Factory, we could skip those steps in the CreateProc and
> only
> > check for bad-buffers. We would then have all the checks in one place, and
our
> > public factories would be that much saner...
> > 
> > Checking every time before calling a Create isn't wrong it would seem, but I
> > wonder if it is required. Seems like the final caller of the entire stream
> > can/should always check the state of the buffer at the end, and can abort
the
> > world if it is invalid. I just want to avoid lots of extra noise in each
> > CreateProc if we don't absolutely require it. Lets keep thinking on this.
> 
> There are two cases, as I see it: buffer.readFoo() fails (e.g., due to a
> truncated buffer). The factories can't handle this, because they don't
> know about the buffer, so it has to be handled in the CreateProc, perhaps 
> with a single call to !buffer.isValid() before the factory call.
> 
> The other case is that the parameter is read, but invalid. For things like
> out-of-range enums, etc, that should definitely be checked in the factory, 
> and not the create proc, but that's not the case today. So in this patch we
> should either beef up the factory's resilience, or leave in some of the
> checks in the deserialization. Only going part way leaves us vulnerable,
> I think.
> 
> As a side note, there are probably a couple of checks in the deserialization 
> which are superfluous. For example, now that all filters scale their params 
> by the CTM, checking for a negative blur radius is not helpful, since it 
> could be invalidated by a negative scale in the CTM at runtime. We should 
> remove these checks, and ensure that the processing code is doing the checks
> instead.

If we take these two cases in reverse order...

I'm fine with beefing up the checking in the Factories. I think this is good in
general, since it means our constructors can be simpler and we don't have to
carry around checks in other methods (like if my input-ptr is NULL, always
return false for the foo method).

If we have robust Factories, then if readFoo() returns a 0 or random value
because it was corrupted we can blindly send it to the Factory, which will
handle it. If the bufferstream is truncated, we will get a 0 or a randome valuue
as well, which will be "safe" to send to the Factory. In this latter case, the
buffer has been marked as invalid, so the ultimate caller can detect that and
throw away whatever was returned.

[Stephen White, 2014-07-18 17:33:53]

On 2014/07/18 17:27:28, reed1 wrote:
> On 2014/07/18 17:05:26, Stephen White wrote:
> > On 2014/07/18 16:18:47, reed1 wrote:
> > >
> >
>
https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
> > > File src/effects/SkColorFilterImageFilter.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
> > > src/effects/SkColorFilterImageFilter.cpp:99: return Create(cf,
> > > common.inputAt(0), &common.cropRect());
> > > On 2014/07/18 15:43:32, Stephen White wrote:
> > > > I see you've removed the validate() here (which is good), 
> > > > but you've also removed the early return. If the color 
> > > > filter fails to read, for example, the cfif Create() function 
> > > > will assert. I think we should keep in the early-returns, 
> > > > just remove all the validate()s.
> > > > 
> > > > Also, Alexis suggests that we might want to do one 
> > > > "if (!buffer.isValid()) return <span id="abbrex-spnAbbr_1473"
> > > class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown
> > > abbrex-AbbrUnknownVisible">NULL</span>;" just before calling each
> > > > Create(), since even buffer.readInt() might fail, and
> > > > there's no point in constructing an object which we know
> > > > will be thrown away anyway.
> > > 
> > > It is a new question about where/when to perform validation on the input
> > > parameters. If we did runtime checks in the Factories (e.g. null-checks,
> > > NaN-checks) in the Factory, we could skip those steps in the CreateProc
and
> > only
> > > check for bad-buffers. We would then have all the checks in one place, and
> our
> > > public factories would be that much saner...
> > > 
> > > Checking every time before calling a Create isn't wrong it would seem, but
I
> > > wonder if it is required. Seems like the final caller of the entire stream
> > > can/should always check the state of the buffer at the end, and can abort
> the
> > > world if it is invalid. I just want to avoid lots of extra noise in each
> > > CreateProc if we don't absolutely require it. Lets keep thinking on this.
> > 
> > There are two cases, as I see it: buffer.readFoo() fails (e.g., due to a
> > truncated buffer). The factories can't handle this, because they don't
> > know about the buffer, so it has to be handled in the CreateProc, perhaps 
> > with a single call to !buffer.isValid() before the factory call.
> > 
> > The other case is that the parameter is read, but invalid. For things like
> > out-of-range enums, etc, that should definitely be checked in the factory, 
> > and not the create proc, but that's not the case today. So in this patch we
> > should either beef up the factory's resilience, or leave in some of the
> > checks in the deserialization. Only going part way leaves us vulnerable,
> > I think.
> > 
> > As a side note, there are probably a couple of checks in the deserialization

> > which are superfluous. For example, now that all filters scale their params 
> > by the CTM, checking for a negative blur radius is not helpful, since it 
> > could be invalidated by a negative scale in the CTM at runtime. We should 
> > remove these checks, and ensure that the processing code is doing the checks
> > instead.
> 
> If we take these two cases in reverse order...
> 
> I'm fine with beefing up the checking in the Factories. I think this is good
in
> general, since it means our constructors can be simpler and we don't have to
> carry around checks in other methods (like if my input-ptr is NULL, always
> return false for the foo method).
> 
> If we have robust Factories, then if readFoo() returns a 0 or random value
> because it was corrupted we can blindly send it to the Factory, which will
> handle it. If the bufferstream is truncated, we will get a 0 or a randome
valuue
> as well, which will be "safe" to send to the Factory. In this latter case, the
> buffer has been marked as invalid, so the ultimate caller can detect that and
> throw away whatever was returned.

I'm fine with that, as long as we robustify the factories as above. Alexis's 
point was that we are constructing an object unnecessarily, but I'll let him 
take that point further if he wants to.

[sugoi1, 2014-07-18 17:45:02]

On 2014/07/18 17:33:53, Stephen White wrote:
> On 2014/07/18 17:27:28, reed1 wrote:
> > On 2014/07/18 17:05:26, Stephen White wrote:
> > > On 2014/07/18 16:18:47, reed1 wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
> > > > File src/effects/SkColorFilterImageFilter.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
> > > > src/effects/SkColorFilterImageFilter.cpp:99: return Create(cf,
> > > > common.inputAt(0), &common.cropRect());
> > > > On 2014/07/18 15:43:32, Stephen White wrote:
> > > > > I see you've removed the validate() here (which is good), 
> > > > > but you've also removed the early return. If the color 
> > > > > filter fails to read, for example, the cfif Create() function 
> > > > > will assert. I think we should keep in the early-returns, 
> > > > > just remove all the validate()s.
> > > > > 
> > > > > Also, Alexis suggests that we might want to do one 
> > > > > "if (!buffer.isValid()) return <span id="abbrex-spnAbbr_1473"
> > > > class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown
> > > > abbrex-AbbrUnknownVisible">NULL</span>;" just before calling each
> > > > > Create(), since even buffer.readInt() might fail, and
> > > > > there's no point in constructing an object which we know
> > > > > will be thrown away anyway.
> > > > 
> > > > It is a new question about where/when to perform validation on the input
> > > > parameters. If we did runtime checks in the Factories (e.g. null-checks,
> > > > NaN-checks) in the Factory, we could skip those steps in the CreateProc
> and
> > > only
> > > > check for bad-buffers. We would then have all the checks in one place,
and
> > our
> > > > public factories would be that much saner...
> > > > 
> > > > Checking every time before calling a Create isn't wrong it would seem,
but
> I
> > > > wonder if it is required. Seems like the final caller of the entire
stream
> > > > can/should always check the state of the buffer at the end, and can
abort
> > the
> > > > world if it is invalid. I just want to avoid lots of extra noise in each
> > > > CreateProc if we don't absolutely require it. Lets keep thinking on
this.
> > > 
> > > There are two cases, as I see it: buffer.readFoo() fails (e.g., due to a
> > > truncated buffer). The factories can't handle this, because they don't
> > > know about the buffer, so it has to be handled in the CreateProc, perhaps 
> > > with a single call to !buffer.isValid() before the factory call.
> > > 
> > > The other case is that the parameter is read, but invalid. For things like
> > > out-of-range enums, etc, that should definitely be checked in the factory,

> > > and not the create proc, but that's not the case today. So in this patch
we
> > > should either beef up the factory's resilience, or leave in some of the
> > > checks in the deserialization. Only going part way leaves us vulnerable,
> > > I think.
> > > 
> > > As a side note, there are probably a couple of checks in the
deserialization
> 
> > > which are superfluous. For example, now that all filters scale their
params 
> > > by the CTM, checking for a negative blur radius is not helpful, since it 
> > > could be invalidated by a negative scale in the CTM at runtime. We should 
> > > remove these checks, and ensure that the processing code is doing the
checks
> > > instead.
> > 
> > If we take these two cases in reverse order...
> > 
> > I'm fine with beefing up the checking in the Factories. I think this is good
> in
> > general, since it means our constructors can be simpler and we don't have to
> > carry around checks in other methods (like if my input-ptr is NULL, always
> > return false for the foo method).
> > 
> > If we have robust Factories, then if readFoo() returns a 0 or random value
> > because it was corrupted we can blindly send it to the Factory, which will
> > handle it. If the bufferstream is truncated, we will get a 0 or a randome
> valuue
> > as well, which will be "safe" to send to the Factory. In this latter case,
the
> > buffer has been marked as invalid, so the ultimate caller can detect that
and
> > throw away whatever was returned.
> 
> I'm fine with that, as long as we robustify the factories as above. Alexis's 
> point was that we are constructing an object unnecessarily, but I'll let him 
> take that point further if he wants to.

I was just surprised initially, because I thought the main goal was not to call
constructors unnecessarily at all. It can be avoided with a small check before
calling the constructor. If you'd rather construct, then destroy the object in
case of an error, I won't object, but I think we should avoid constructing
objects if we already know we're going to destroy them.

[reed1, 2014-07-18 19:01:26]

On 2014/07/18 17:45:02, sugoi1 wrote:
> On 2014/07/18 17:33:53, Stephen White wrote:
> > On 2014/07/18 17:27:28, reed1 wrote:
> > > On 2014/07/18 17:05:26, Stephen White wrote:
> > > > On 2014/07/18 16:18:47, reed1 wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
> > > > > File src/effects/SkColorFilterImageFilter.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/395603002/diff/60001/src/effects/SkColorFilte...
> > > > > src/effects/SkColorFilterImageFilter.cpp:99: return Create(cf,
> > > > > common.inputAt(0), &common.cropRect());
> > > > > On 2014/07/18 15:43:32, Stephen White wrote:
> > > > > > I see you've removed the validate() here (which is good), 
> > > > > > but you've also removed the early return. If the color 
> > > > > > filter fails to read, for example, the cfif Create() function 
> > > > > > will assert. I think we should keep in the early-returns, 
> > > > > > just remove all the validate()s.
> > > > > > 
> > > > > > Also, Alexis suggests that we might want to do one 
> > > > > > "if (!buffer.isValid()) return <span id="abbrex-spnAbbr_1473"
> > > > > class="abbrex-Abbr abbrex-Abbr_NULL abbrex-AbbrUnknown
> > > > > abbrex-AbbrUnknownVisible">NULL</span>;" just before calling each
> > > > > > Create(), since even buffer.readInt() might fail, and
> > > > > > there's no point in constructing an object which we know
> > > > > > will be thrown away anyway.
> > > > > 
> > > > > It is a new question about where/when to perform validation on the
input
> > > > > parameters. If we did runtime checks in the Factories (e.g.
null-checks,
> > > > > NaN-checks) in the Factory, we could skip those steps in the
CreateProc
> > and
> > > > only
> > > > > check for bad-buffers. We would then have all the checks in one place,
> and
> > > our
> > > > > public factories would be that much saner...
> > > > > 
> > > > > Checking every time before calling a Create isn't wrong it would seem,
> but
> > I
> > > > > wonder if it is required. Seems like the final caller of the entire
> stream
> > > > > can/should always check the state of the buffer at the end, and can
> abort
> > > the
> > > > > world if it is invalid. I just want to avoid lots of extra noise in
each
> > > > > CreateProc if we don't absolutely require it. Lets keep thinking on
> this.
> > > > 
> > > > There are two cases, as I see it: buffer.readFoo() fails (e.g., due to a
> > > > truncated buffer). The factories can't handle this, because they don't
> > > > know about the buffer, so it has to be handled in the CreateProc,
perhaps 
> > > > with a single call to !buffer.isValid() before the factory call.
> > > > 
> > > > The other case is that the parameter is read, but invalid. For things
like
> > > > out-of-range enums, etc, that should definitely be checked in the
factory,
> 
> > > > and not the create proc, but that's not the case today. So in this patch
> we
> > > > should either beef up the factory's resilience, or leave in some of the
> > > > checks in the deserialization. Only going part way leaves us vulnerable,
> > > > I think.
> > > > 
> > > > As a side note, there are probably a couple of checks in the
> deserialization
> > 
> > > > which are superfluous. For example, now that all filters scale their
> params 
> > > > by the CTM, checking for a negative blur radius is not helpful, since it

> > > > could be invalidated by a negative scale in the CTM at runtime. We
should 
> > > > remove these checks, and ensure that the processing code is doing the
> checks
> > > > instead.
> > > 
> > > If we take these two cases in reverse order...
> > > 
> > > I'm fine with beefing up the checking in the Factories. I think this is
good
> > in
> > > general, since it means our constructors can be simpler and we don't have
to
> > > carry around checks in other methods (like if my input-ptr is NULL, always
> > > return false for the foo method).
> > > 
> > > If we have robust Factories, then if readFoo() returns a 0 or random value
> > > because it was corrupted we can blindly send it to the Factory, which will
> > > handle it. If the bufferstream is truncated, we will get a 0 or a randome
> > valuue
> > > as well, which will be "safe" to send to the Factory. In this latter case,
> the
> > > buffer has been marked as invalid, so the ultimate caller can detect that
> and
> > > throw away whatever was returned.
> > 
> > I'm fine with that, as long as we robustify the factories as above. Alexis's

> > point was that we are constructing an object unnecessarily, but I'll let him

> > take that point further if he wants to.
> 
> I was just surprised initially, because I thought the main goal was not to
call
> constructors unnecessarily at all. It can be avoided with a small check before
> calling the constructor. If you'd rather construct, then destroy the object in
> case of an error, I won't object, but I think we should avoid constructing
> objects if we already know we're going to destroy them.

I see your point. I guess its a question of the performance of that unnecessary
build/destroy times the frequency we think it will happen, compared to the
complexity-debt of having to perform the check at every site (and likely we'll
perform the check at the master site as well).

[reed1, 2014-08-18 15:35:18]

this is ready for review.

only known problem is around twopointgradients needing another reversal, so I am
working on that now.

[Stephen White, 2014-08-18 18:17:26]

https://codereview.chromium.org/395603002/diff/260001/include/core/SkImageFil...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/395603002/diff/260001/include/core/SkImageFil...
include/core/SkImageFilter.h:338: #define
SK_IMAGEFILTER_UNFLATTEN_COMMON(expectedCount)      \
This macro is too much magic, IMHO: putting a variable unnamed at
the call site in the caller's scope, and an early return that you
don't see at the call site.

At a minimum, I'd like to see the variable name in the macro:

#define SK_IMAGEFILTER_UNFLATTEN_COMMON(expectedCount, var)
  ....

But to be honest, without a macro, this is only 4 lines of code. I 
would be ok with duplicating those 4 lines in each filter to 
improve readability at the call site, and to make the early-
return explicit.

https://codereview.chromium.org/395603002/diff/260001/src/effects/SkLightingI...
File src/effects/SkLightingImageFilter.cpp (left):

https://codereview.chromium.org/395603002/diff/260001/src/effects/SkLightingI...
src/effects/SkLightingImageFilter.cpp:840: // our caller knows that we take
ownership of the light, so we don't
I think this comment should now be removed, since it's no longer true (we do
ref, and the caller doesn't assume we take ownership).

https://codereview.chromium.org/395603002/diff/260001/src/effects/SkMatrixCon...
File src/effects/SkMatrixConvolutionImageFilter.cpp (right):

https://codereview.chromium.org/395603002/diff/260001/src/effects/SkMatrixCon...
src/effects/SkMatrixConvolutionImageFilter.cpp:98: if
(!buffer.validate(kernelSize.width() * kernelSize.height() == count)) {
Alexis should look at this. It looks like there's a potential for int 
overflow here, from which the old code above protected us.

[sugoi1, 2014-08-18 20:10:17]

On 2014/08/18 18:17:26, Stephen White wrote:
>
https://codereview.chromium.org/395603002/diff/260001/include/core/SkImageFil...
> File include/core/SkImageFilter.h (right):
> 
>
https://codereview.chromium.org/395603002/diff/260001/include/core/SkImageFil...
> include/core/SkImageFilter.h:338: #define
> SK_IMAGEFILTER_UNFLATTEN_COMMON(expectedCount)      \
> This macro is too much magic, IMHO: putting a variable unnamed at
> the call site in the caller's scope, and an early return that you
> don't see at the call site.
> 
> At a minimum, I'd like to see the variable name in the macro:
> 
> #define SK_IMAGEFILTER_UNFLATTEN_COMMON(expectedCount, var)
>   ....
> 
> But to be honest, without a macro, this is only 4 lines of code. I 
> would be ok with duplicating those 4 lines in each filter to 
> improve readability at the call site, and to make the early-
> return explicit.
> 
>
https://codereview.chromium.org/395603002/diff/260001/src/effects/SkLightingI...
> File src/effects/SkLightingImageFilter.cpp (left):
> 
>
https://codereview.chromium.org/395603002/diff/260001/src/effects/SkLightingI...
> src/effects/SkLightingImageFilter.cpp:840: // our caller knows that we take
> ownership of the light, so we don't
> I think this comment should now be removed, since it's no longer true (we do
> ref, and the caller doesn't assume we take ownership).
> 
>
https://codereview.chromium.org/395603002/diff/260001/src/effects/SkMatrixCon...
> File src/effects/SkMatrixConvolutionImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/395603002/diff/260001/src/effects/SkMatrixCon...
> src/effects/SkMatrixConvolutionImageFilter.cpp:98: if
> (!buffer.validate(kernelSize.width() * kernelSize.height() == count)) {
> Alexis should look at this. It looks like there's a potential for int 
> overflow here, from which the old code above protected us.

I haven't had time to review this yet, but I'll do it first thing tomorrow
morning. I really want to review it thoroughly and try to get this in while
keeping clusterfuzz happy.

[reed1, 2014-08-18 22:39:16]

https://codereview.chromium.org/395603002/diff/260001/include/core/SkImageFil...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/395603002/diff/260001/include/core/SkImageFil...
include/core/SkImageFilter.h:338: #define
SK_IMAGEFILTER_UNFLATTEN_COMMON(expectedCount)      \
On 2014/08/18 18:17:26, Stephen White wrote:
> This macro is too much magic, <span id="abbrex-spnAbbr_11835"
class="abbrex-Abbr abbrex-Abbr_IMHO abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">IMHO</span>: putting a variable unnamed at
> the call site in the caller's scope, and an early return that you
> don't see at the call site.
> 
> At a minimum, I'd like to see the variable name in the macro:
> 
> #define SK_IMAGEFILTER_UNFLATTEN_COMMON(expectedCount, var)
>   ....
> 
> But to be honest, without a macro, this is only 4 lines of code. I 
> would be ok with duplicating those 4 lines in each filter to 
> improve readability at the call site, and to make the early-
> return explicit.

A downside of copy/pasting 4 lines everywhere is the chance for a typo, and/or
forgetting to do the early return on failure.
I think the compromise suggestion of passing the var name makes good sense.
Done.

https://codereview.chromium.org/395603002/diff/260001/src/effects/SkLightingI...
File src/effects/SkLightingImageFilter.cpp (left):

https://codereview.chromium.org/395603002/diff/260001/src/effects/SkLightingI...
src/effects/SkLightingImageFilter.cpp:840: // our caller knows that we take
ownership of the light, so we don't
On 2014/08/18 18:17:26, Stephen White wrote:
> I think this comment should now be removed, since it's no longer true (we do
> ref, and the caller doesn't assume we take ownership).

Done.

https://codereview.chromium.org/395603002/diff/260001/src/effects/SkMatrixCon...
File src/effects/SkMatrixConvolutionImageFilter.cpp (right):

https://codereview.chromium.org/395603002/diff/260001/src/effects/SkMatrixCon...
src/effects/SkMatrixConvolutionImageFilter.cpp:98: if
(!buffer.validate(kernelSize.width() * kernelSize.height() == count)) {
On 2014/08/18 18:17:26, Stephen White wrote:
> Alexis should look at this. It looks like there's a potential for int 
> overflow here, from which the old code above protected us.

Done.

[robertphillips, 2014-08-19 15:23:27]

https://codereview.chromium.org/395603002/diff/320001/gm/gradients_2pt_conica...
File gm/gradients_2pt_conical.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/gm/gradients_2pt_conica...
gm/gradients_2pt_conical.cpp:338: canvas->save();
Why are we skipping these cases now ?

https://codereview.chromium.org/395603002/diff/320001/include/core/SkFlattena...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/395603002/diff/320001/include/core/SkFlattena...
include/core/SkFlattenable.h:52: 
typo (declar)

[sugoi1, 2014-08-19 18:46:18]

https://codereview.chromium.org/395603002/diff/320001/gm/gradients_2pt_conica...
File gm/gradients_2pt_conical.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/gm/gradients_2pt_conica...
gm/gradients_2pt_conical.cpp:335: const int count =
gGradCases[fGradCaseType].fCount;
This may have been mentioned earlier in the review, but why is "count" computed
only the be multiplied by 0 later on?

https://codereview.chromium.org/395603002/diff/320001/include/core/SkImageFil...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/395603002/diff/320001/include/core/SkImageFil...
include/core/SkImageFilter.h:342: } while (0)
I understand the use of do { ... } while(0) by itself in a macro, but since you
also have "Common localVar;" at the beginning of this macro, I don't think this
would allow you to do more than just a regular scope { }, since writing
something like:

if (condition)
  SK_IMAGEFILTER_UNFLATTEN_COMMON(...);
else

would fail anyway. Maybe there's another reason for using "do { ... } while(0)"
that you know.

https://codereview.chromium.org/395603002/diff/320001/src/core/SkFilterShader...
File src/core/SkFilterShader.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/core/SkFilterShader...
src/core/SkFilterShader.cpp:33: fShader->unref();
I'm surprised these never crashed on a read error in the constructor. Maybe
these aren't exercised by the fuzzer.

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

https://codereview.chromium.org/395603002/diff/320001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:79: if (!buffer.validate(count >= 0)) {
If it is no longer possible to use an expectedCount with a value of -1, then
this is also checked at line 82, so I think you can remove lines 79 to 81. Also,
we could consider making expectedCount an unsigned value.

If it is still possible to use an expectedCount with a value of -1, then this is
broken, since it will always fail the validation at line 82.

https://codereview.chromium.org/395603002/diff/320001/src/effects/Sk1DPathEff...
File src/effects/Sk1DPathEffect.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/Sk1DPathEff...
src/effects/Sk1DPathEffect.cpp:177: return SkPath1DPathEffect::Create(path,
advance, phase, style);
Hmmm... Wouldn't it be better to validate style here to avoid problems later?

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkAvoidXfer...
File src/effects/SkAvoidXfermode.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkAvoidXfer...
src/effects/SkAvoidXfermode.cpp:36: return Create(color, tolerance, (Mode)mode);
Note: a bad mode could cause an out of bounds memory access in toString(), but I
assume toString() is never going to be called in chrome.

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkColorFilt...
File src/effects/SkColorFilters.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkColorFilt...
src/effects/SkColorFilters.cpp:131: return
SkColorFilter::CreateModeFilter(color, mode);
I think the mode validation might be important here, since the the only test
performed later on is against ILLEGAL_XFERMODE_MODE, which is -1, and the mode
can be queried from asColorMode() and potentially cause issues later on.

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkDiscreteP...
File src/effects/SkDiscretePathEffect.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkDiscreteP...
src/effects/SkDiscretePathEffect.cpp:79: SkScalar segLength =
buffer.readScalar();
Note: Looking at filterPath(), it seems like a very small value or a negative
value (through integer overflow of "n") of fSegLength could cause the "while
(--n >= 0)" to go for a very long time and hang.

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkDisplacem...
File src/effects/SkDisplacementMapEffect.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkDisplacem...
src/effects/SkDisplacementMapEffect.cpp:194: SkScalar scale =
buffer.readScalar();
I think the check for a finite scale should be kept, as this value will be used
to compute an offset to address memory.

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkLayerRast...
File src/effects/SkLayerRasterizer.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkLayerRast...
src/effects/SkLayerRasterizer.cpp:162: 
White space changes?

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkMorpholog...
File src/effects/SkMorphologyImageFilter.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkMorpholog...
src/effects/SkMorphologyImageFilter.cpp:266: return Create(width, height,
common.inputAt(0), &common.cropRect());
Note: negative values of width / height can create an invalid SkRect in
computeFastBounds().

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkOffsetIma...
File src/effects/SkOffsetImageFilter.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkOffsetIma...
src/effects/SkOffsetImageFilter.cpp:98: return Create(offset.x(), offset.y(),
common.inputAt(0), &common.cropRect());
Since it's used for translation, which is eventually used to offset memory
accesses (in the CPU codepath), checking for finite scalars could prevent
errors.

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkTestImage...
File src/effects/SkTestImageFilters.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkTestImage...
src/effects/SkTestImageFilters.cpp:76: return Create(buffer.readScalar(),
common.inputAt(0));
Maybe we could check the validity of the scale value here instead of checking it
every time we call SkDownSampleImageFilter::onFilterImage().

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkTileImage...
File src/effects/SkTileImageFilter.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkTileImage...
src/effects/SkTileImageFilter.cpp:103: return Create(src, dst,
common.inputAt(0));
Is it useful to create an SkTileImageFilter with invalid src/dst rects?

https://codereview.chromium.org/395603002/diff/320001/src/effects/gradients/S...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/gradients/S...
src/effects/gradients/SkGradientShader.cpp:65: return true;
return buffer.isValid() ?

[reed1, 2014-08-19 19:58:06]

https://codereview.chromium.org/395603002/diff/320001/gm/gradients_2pt_conica...
File gm/gradients_2pt_conical.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/gm/gradients_2pt_conica...
gm/gradients_2pt_conical.cpp:338: canvas->save();
On 2014/08/19 15:23:27, robertphillips wrote:
> Why are we skipping these cases now ?

debugging. fixed.

https://codereview.chromium.org/395603002/diff/320001/include/core/SkFlattena...
File include/core/SkFlattenable.h (right):

https://codereview.chromium.org/395603002/diff/320001/include/core/SkFlattena...
include/core/SkFlattenable.h:52: 
On 2014/08/19 15:23:27, robertphillips wrote:
> typo (declar)

Done.

https://codereview.chromium.org/395603002/diff/320001/include/core/SkImageFil...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/395603002/diff/320001/include/core/SkImageFil...
include/core/SkImageFilter.h:342: } while (0)
On 2014/08/19 18:46:16, sugoi1 wrote:
> I understand the use of do { ... } while(0) by itself in a macro, but since
you
> also have "Common localVar;" at the beginning of this macro, I don't think
this
> would allow you to do more than just a regular scope { }, since writing
> something like:
> 
> if (condition)
>   SK_IMAGEFILTER_UNFLATTEN_COMMON(...);
> else
> 
> would fail anyway. Maybe there's another reason for using "do { ... }
while(0)"
> that you know.

True, the presence of the localVar may obviate the need for the do/while.

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

https://codereview.chromium.org/395603002/diff/320001/src/core/SkImageFilter....
src/core/SkImageFilter.cpp:79: if (!buffer.validate(count >= 0)) {
On 2014/08/19 18:46:16, sugoi1 wrote:
> If it is no longer possible to use an expectedCount with a value of -1, then
> this is also checked at line 82, so I think you can remove lines 79 to 81.
Also,
> we could consider making expectedCount an unsigned value.
> 
> If it is still possible to use an expectedCount with a value of -1, then this
is
> broken, since it will always fail the validation at line 82.

Good catch, I think I no longer take -1.

https://codereview.chromium.org/395603002/diff/320001/src/effects/Sk1DPathEff...
File src/effects/Sk1DPathEffect.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/Sk1DPathEff...
src/effects/Sk1DPathEffect.cpp:177: return SkPath1DPathEffect::Create(path,
advance, phase, style);
On 2014/08/19 18:46:16, sugoi1 wrote:
> Hmmm... Wouldn't it be better to validate style here to avoid problems later?

I think there are lots of additional validation checks we could add. I feel this
CL is already huge, and am trying to avoid any changes not germane to this new
Flatten technique. Can we defer this sort of improvement to another CL?

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkAvoidXfer...
File src/effects/SkAvoidXfermode.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkAvoidXfer...
src/effects/SkAvoidXfermode.cpp:36: return Create(color, tolerance, (Mode)mode);
On 2014/08/19 18:46:16, sugoi1 wrote:
> Note: a bad mode could cause an out of bounds memory access in toString(), but
I
> assume toString() is never going to be called in chrome.

A good validation check to add in a future CL.

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkColorFilt...
File src/effects/SkColorFilters.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkColorFilt...
src/effects/SkColorFilters.cpp:131: return
SkColorFilter::CreateModeFilter(color, mode);
On 2014/08/19 18:46:17, sugoi1 wrote:
> I think the mode validation might be important here, since the the only test
> performed later on is against ILLEGAL_XFERMODE_MODE, which is -1, and the mode
> can be queried from asColorMode() and potentially cause issues later on.

Fixed in factory, removed 1st constructor, eliminated ILLEGAL_XFERMODE_MODE

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkDiscreteP...
File src/effects/SkDiscretePathEffect.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkDiscreteP...
src/effects/SkDiscretePathEffect.cpp:79: SkScalar segLength =
buffer.readScalar();
On 2014/08/19 18:46:17, sugoi1 wrote:
> Note: Looking at filterPath(), it seems like a very small value or a negative
> value (through integer overflow of "n") of fSegLength could cause the "while
> (--n >= 0)" to go for a very long time and hang.

Good note for future CL.

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkDisplacem...
File src/effects/SkDisplacementMapEffect.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkDisplacem...
src/effects/SkDisplacementMapEffect.cpp:194: SkScalar scale =
buffer.readScalar();
On 2014/08/19 18:46:17, sugoi1 wrote:
> I think the check for a finite scale should be kept, as this value will be
used
> to compute an offset to address memory.

Moved check into factory itself.

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkLayerRast...
File src/effects/SkLayerRasterizer.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkLayerRast...
src/effects/SkLayerRasterizer.cpp:162: 
On 2014/08/19 18:46:17, sugoi1 wrote:
> White space changes?

Done.

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkMorpholog...
File src/effects/SkMorphologyImageFilter.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkMorpholog...
src/effects/SkMorphologyImageFilter.cpp:266: return Create(width, height,
common.inputAt(0), &common.cropRect());
On 2014/08/19 18:46:17, sugoi1 wrote:
> Note: negative values of width / height can create an invalid SkRect in
> computeFastBounds().

Moved to factory.

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkOffsetIma...
File src/effects/SkOffsetImageFilter.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkOffsetIma...
src/effects/SkOffsetImageFilter.cpp:98: return Create(offset.x(), offset.y(),
common.inputAt(0), &common.cropRect());
On 2014/08/19 18:46:17, sugoi1 wrote:
> Since it's used for translation, which is eventually used to offset memory
> accesses (in the <span id="abbrex-spnAbbr_15374" class="abbrex-Abbr
abbrex-Abbr_CPU abbrex-AbbrUnknown abbrex-AbbrUnknownVisible">CPU</span>
codepath), checking for finite scalars could prevent
> errors.

Moved to factory.

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkTestImage...
File src/effects/SkTestImageFilters.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkTestImage...
src/effects/SkTestImageFilters.cpp:76: return Create(buffer.readScalar(),
common.inputAt(0));
On 2014/08/19 18:46:18, sugoi1 wrote:
> Maybe we could check the validity of the scale value here instead of checking
it
> every time we call SkDownSampleImageFilter::onFilterImage().

Moved to factory.

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkTileImage...
File src/effects/SkTileImageFilter.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/SkTileImage...
src/effects/SkTileImageFilter.cpp:103: return Create(src, dst,
common.inputAt(0));
On 2014/08/19 18:46:18, sugoi1 wrote:
> Is it useful to create an SkTileImageFilter with invalid src/dst rects?

moved to factory

https://codereview.chromium.org/395603002/diff/320001/src/effects/gradients/S...
File src/effects/gradients/SkGradientShader.cpp (right):

https://codereview.chromium.org/395603002/diff/320001/src/effects/gradients/S...
src/effects/gradients/SkGradientShader.cpp:65: return true;
On 2014/08/19 18:46:18, sugoi1 wrote:
> return buffer.isValid() ?

Done.

[reed1, 2014-08-20 01:20:20]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-08-20 01:20:35]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/395603002/420001

[commit-bot: I haz the power, 2014-08-20 01:20:37]

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 a valid reviewer till 2014-08-20 07:20 UTC

[reed1, 2014-08-20 01:21:44]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-08-20 13:23:24]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-08-20 13:24:02]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/395603002/440001

[commit-bot: I haz the power, 2014-08-20 13:24:04]

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 a valid reviewer till 2014-08-20 19:24 UTC

[reed1, 2014-08-20 13:36:45]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-08-20 21:11:15]

The CQ bit was checked by reed@google.com

[reed1, 2014-08-20 21:11:25]

ptal

[commit-bot: I haz the power, 2014-08-20 21:11:27]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/395603002/460001

[commit-bot: I haz the power, 2014-08-20 21:11:29]

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 a valid reviewer till 2014-08-21 03:11 UTC

[Stephen White, 2014-08-20 21:23:15]

https://codereview.chromium.org/395603002/diff/440001/include/core/SkImageFil...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/395603002/diff/440001/include/core/SkImageFil...
include/core/SkImageFilter.h:216: bool unflatten(SkReadBuffer&, int
expectedInputs = -1);
Unrelated to this CL, but we should probably remove the default arg here. -1 is
somewhat dangerous as a default, since most filters do not bounds-check calls to
getInput(). (I think all of the filters now pass both args to this function
anyway.)

https://codereview.chromium.org/395603002/diff/440001/include/core/SkImageFil...
include/core/SkImageFilter.h:223: SkImageFilter*  inputAt(int index) const {
return fInputs[index]; }
This should be named getInput() to match SkImageFilter (or
SkImageFilter::getInput() should be inputAt()).

https://codereview.chromium.org/395603002/diff/440001/src/effects/SkBlurMaskF...
File src/effects/SkBlurMaskFilter.cpp (left):

https://codereview.chromium.org/395603002/diff/440001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:535: this->INHERITED::flatten(buffer);
Since SkBlurMaskFilter doesn't define flatten(), this was calling
through to SkFlattenable::flatten(), which has this comment:

void SkFlattenable::flatten(SkWriteBuffer&) const
{
    /*  we don't write anything at the moment, but this allows our subclasses
        to not know that, since we want them to always call INHERITED::flatten()
        in their code.
    */
}

is that no longer true?

https://codereview.chromium.org/395603002/diff/440001/src/effects/SkMergeImag...
File src/effects/SkMergeImageFilter.cpp (right):

https://codereview.chromium.org/395603002/diff/440001/src/effects/SkMergeImag...
src/effects/SkMergeImageFilter.cpp:110: if (!common.unflatten(buffer)) {
Let's explicitly pass -1 here.

[reed1, 2014-08-20 22:04:15]

https://codereview.chromium.org/395603002/diff/440001/include/core/SkImageFil...
File include/core/SkImageFilter.h (right):

https://codereview.chromium.org/395603002/diff/440001/include/core/SkImageFil...
include/core/SkImageFilter.h:216: bool unflatten(SkReadBuffer&, int
expectedInputs = -1);
On 2014/08/20 21:23:15, Stephen White wrote:
> Unrelated to this <span id="abbrex-spnAbbr_35884" class="abbrex-Abbr
abbrex-Abbr_CL abbrex-AbbrInactive abbrex-AbbrInactiveVisible">CL</span>, but we
should probably remove the default arg here. -1 is
> somewhat dangerous as a default, since most filters do not bounds-check calls
to
> getInput(). (I think all of the filters now pass both args to this function
> anyway.)

Acknowledged.

https://codereview.chromium.org/395603002/diff/440001/include/core/SkImageFil...
include/core/SkImageFilter.h:223: SkImageFilter*  inputAt(int index) const {
return fInputs[index]; }
On 2014/08/20 21:23:15, Stephen White wrote:
> This should be named getInput() to match SkImageFilter (or
> SkImageFilter::getInput() should be inputAt()).

Done.

https://codereview.chromium.org/395603002/diff/440001/src/effects/SkBlurMaskF...
File src/effects/SkBlurMaskFilter.cpp (left):

https://codereview.chromium.org/395603002/diff/440001/src/effects/SkBlurMaskF...
src/effects/SkBlurMaskFilter.cpp:535: this->INHERITED::flatten(buffer);
On 2014/08/20 21:23:15, Stephen White wrote:
> Since SkBlurMaskFilter doesn't define flatten(), this was calling
> through to SkFlattenable::flatten(), which has this comment:
> 
> void SkFlattenable::flatten(SkWriteBuffer&) const
> {
>     /*  we don't write anything at the moment, but this allows our subclasses
>         to not know that, since we want them to always call
INHERITED::flatten()
>         in their code.
>     */
> }
> 
> is that no longer true?

Good catch. This is no longer true, so I will remove the comment.

https://codereview.chromium.org/395603002/diff/440001/src/effects/SkMergeImag...
File src/effects/SkMergeImageFilter.cpp (right):

https://codereview.chromium.org/395603002/diff/440001/src/effects/SkMergeImag...
src/effects/SkMergeImageFilter.cpp:110: if (!common.unflatten(buffer)) {
On 2014/08/20 21:23:15, Stephen White wrote:
> Let's explicitly pass -1 here.

Done.

[reed1, 2014-08-20 22:26:43]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-08-20 22:27:19]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/395603002/500001

[commit-bot: I haz the power, 2014-08-20 22:27:21]

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 a valid reviewer till 2014-08-21 04:27 UTC

[commit-bot: I haz the power, 2014-08-21 04:27:19]

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

[commit-bot: I haz the power, 2014-08-21 04:27:21]

No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer from
https://skia.googlesource.com/skia/+/master/CQ_COMMITTERS

[reed1, 2014-08-21 12:28:14]

ptal

[Stephen White, 2014-08-21 13:58:41]

LGTM

[reed1, 2014-08-21 14:50:14]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-08-21 14:51:14]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/reed@google.com/395603002/520001

[commit-bot: I haz the power, 2014-08-21 15:00:10]

Committed patchset #27 (520001) as 9fa60daad4d5f54c0dbe3dbcc7608a8f6d721187

[Stephen White, 2014-08-23 18:08:48]

This change seems to have caused a large performance regression in SVG filters
in Chrome (found by hand-bisecting the skia roll):

https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=android-motoe...

Strange, since it should've been a no-op. I'll investigate further.

[reed1, 2014-08-25 12:22:27]

On 2014/08/23 18:08:48, Stephen White wrote:
> This change seems to have caused a large performance regression in SVG filters
> in Chrome (found by hand-bisecting the skia roll):
> 
>
https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=android-motoe...
> 
> Strange, since it should've been a no-op. I'll investigate further.

Will be interesting to see a profile. Since so much did get re-arranged, perhaps
we're just missing a fast-case we used to take.

[Stephen White, 2014-08-25 14:30:05]

On 2014/08/25 12:22:27, reed1 wrote:
> On 2014/08/23 18:08:48, Stephen White wrote:
> > This change seems to have caused a large performance regression in SVG
filters
> > in Chrome (found by hand-bisecting the skia roll):
> > 
> >
>
https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=android-motoe...
> > 
> > Strange, since it should've been a no-op. I'll investigate further.
> 
> Will be interesting to see a profile. Since so much did get re-arranged,
perhaps
> we're just missing a fast-case we used to take.

I'm pretty sure I know what's going on now.

The old deserialization constructor copies the (deserialized) Common's uniqueID
into the SkImageFilter's uniqueID. The new path doesn't. This defeats
SkImageFilter's caching under impl-side painting, since without the ID, the
system has no way of connecting the filters created in Blink with the ones
deserialized in Skia.

Looks like at a minimum, we'll have to add the uniqueID to each filter's
constructor. Then we have two options, basically:

1) Add a uniqueID parameter to each (public) Create() function, and have the
CreateProcs pass common's uniqueID.
2) Change the CreateProc's to call the (private) constructors directly, and pass
the uniqueID there.

I'm in favour of #2, since it doesn't expose the uniqueID to the public API. It
may require duplicating some checks from the
Create() in the CreateProc(), but actually many of them can probably be removed,
since they're ineffective in the face of
a negative-scaling CTM anyway.

[reed1, 2014-08-25 14:40:26]

On 2014/08/25 14:30:05, Stephen White wrote:
> On 2014/08/25 12:22:27, reed1 wrote:
> > On 2014/08/23 18:08:48, Stephen White wrote:
> > > This change seems to have caused a large performance regression in SVG
> filters
> > > in Chrome (found by hand-bisecting the skia roll):
> > > 
> > >
> >
>
https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=android-motoe...
> > > 
> > > Strange, since it should've been a no-op. I'll investigate further.
> > 
> > Will be interesting to see a profile. Since so much did get re-arranged,
> perhaps
> > we're just missing a fast-case we used to take.
> 
> I'm pretty sure I know what's going on now.
> 
> The old deserialization constructor copies the (deserialized) Common's
uniqueID
> into the SkImageFilter's uniqueID. The new path doesn't. This defeats
> SkImageFilter's caching under impl-side painting, since without the ID, the
> system has no way of connecting the filters created in Blink with the ones
> deserialized in Skia.
> 
> Looks like at a minimum, we'll have to add the uniqueID to each filter's
> constructor. Then we have two options, basically:
> 
> 1) Add a uniqueID parameter to each (public) Create() function, and have the
> CreateProcs pass common's uniqueID.
> 2) Change the CreateProc's to call the (private) constructors directly, and
pass
> the uniqueID there.
> 
> I'm in favour of #2, since it doesn't expose the uniqueID to the public API.
It
> may require duplicating some checks from the
> Create() in the CreateProc(), but actually many of them can probably be
removed,
> since they're ineffective in the face of
> a negative-scaling CTM anyway.

Perhaps you could VC with Brian and me to talk about this. It seems that two
"creations" can't hit the same cache entry with uniqueIDs, but if we flattened
the parameters state (as we did for all effects in the old picture world) we
could create a key that would match. In that world we wouldn't need to
read/write uniqueIDs.