factor out flattening/unflattening of common fields from SkImageFilter

factor out flattening/unflattening of common fields from SkImageFilter

This is a precursor to changing SkImageFilters (and all effects) to unflatten via a factory instead of a constructor. In that world, each subclass of ImageFilter will need to control/initiate the unflattening of the common fields, so they can be extract and passed to their Factory.

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

[Stephen White, 2014-07-16 19:26:11]

Looks ok, but I'd like to see how this would look from a subclass's
deserializing factory function as well.

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

https://codereview.chromium.org/395273002/diff/20001/include/core/SkImageFilt...
include/core/SkImageFilter.h:225: kPreallocated = 4
Personally, I'm not fond of these preallocated arrays of some arbitrary size.
Better to add it when profiling tells you than to do it preemptively, IMHO.

https://codereview.chromium.org/395273002/diff/20001/include/core/SkImageFilt...
include/core/SkImageFilter.h:316: int fInputCount;
Could we simply aggregate a Common instance into SkImageFilter itself, get rid
of these members, and then copy-construct it from the one passed in at
construction time?

[reed1, 2014-07-16 19:37:37]

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

https://codereview.chromium.org/395273002/diff/20001/include/core/SkImageFilt...
include/core/SkImageFilter.h:225: kPreallocated = 4
On 2014/07/16 19:26:11, Stephen White wrote:
> Personally, I'm not fond of these preallocated arrays of some arbitrary size.
> Better to add it when profiling tells you than to do it preemptively, <span
id="abbrex-spnAbbr_20617" class="abbrex-Abbr abbrex-Abbr_IMHO abbrex-AbbrUnknown
abbrex-AbbrUnknownVisible">IMHO</span>.

Calling new is a pretty aweful thing if not needed, and the complexity is
totally isolated to this one place, so my take is that this pattern is safe and
helpful. All of that said, do you know the maximum "required" input count for
your subclasses (i.e. is it 2 or something larger). Just curious.

https://codereview.chromium.org/395273002/diff/20001/include/core/SkImageFilt...
include/core/SkImageFilter.h:316: int fInputCount;
On 2014/07/16 19:26:11, Stephen White wrote:
> Could we simply aggregate a Common instance into SkImageFilter itself, get rid
> of these members, and then copy-construct it from the one passed in at
> construction time?

Perhaps. Will have to do that during or after the big change where we eliminate
the flattening hierarchy stuff I think, since right now the subclasses have to
defer to this class to unflatten the inputs.

[Stephen White, 2014-07-16 19:48:04]

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

https://codereview.chromium.org/395273002/diff/20001/include/core/SkImageFilt...
include/core/SkImageFilter.h:225: kPreallocated = 4
On 2014/07/16 19:37:37, reed1 wrote:
> On 2014/07/16 19:26:11, Stephen White wrote:
> > Personally, I'm not fond of these preallocated arrays of some arbitrary
size.
> > Better to add it when profiling tells you than to do it preemptively, <span
> id="abbrex-spnAbbr_20617" class="abbrex-Abbr abbrex-Abbr_IMHO
abbrex-AbbrUnknown
> abbrex-AbbrUnknownVisible">IMHO</span>.
> 
> Calling new is a pretty aweful thing if not needed, and the complexity is
> totally isolated to this one place, so my take is that this pattern is safe
and
> helpful.

If you mean awful for performance, considering that every filter is going to
allocate an SkBitmap of primitive size, and then touch all those pixels at least
once, allocating a small chunk of n pointers seems pretty insignificant by
comparison.

> All of that said, do you know the maximum "required" input count for
> your subclasses (i.e. is it 2 or something larger). Just curious.

Each subclass knows how many it will need (0-2, IIRC), except for SkMerge which
is variable (-1). Which makes me wonder -- should we just have inputCount as a
construction param for Common?

[reed1, 2014-07-16 20:39:42]

On 2014/07/16 19:48:04, Stephen White wrote:
>
https://codereview.chromium.org/395273002/diff/20001/include/core/SkImageFilt...
> File include/core/SkImageFilter.h (right):
> 
>
https://codereview.chromium.org/395273002/diff/20001/include/core/SkImageFilt...
> include/core/SkImageFilter.h:225: kPreallocated = 4
> On 2014/07/16 19:37:37, reed1 wrote:
> > On 2014/07/16 19:26:11, Stephen White wrote:
> > > Personally, I'm not fond of these preallocated arrays of some arbitrary
> size.
> > > Better to add it when profiling tells you than to do it preemptively,
<span
> > id="abbrex-spnAbbr_20617" class="abbrex-Abbr abbrex-Abbr_IMHO
> abbrex-AbbrUnknown
> > abbrex-AbbrUnknownVisible">IMHO</span>.
> > 
> > Calling new is a pretty aweful thing if not needed, and the complexity is
> > totally isolated to this one place, so my take is that this pattern is safe
> and
> > helpful.
> 
> If you mean awful for performance, considering that every filter is going to
> allocate an SkBitmap of primitive size, and then touch all those pixels at
least
> once, allocating a small chunk of n pointers seems pretty insignificant by
> comparison.
> 
> > All of that said, do you know the maximum "required" input count for
> > your subclasses (i.e. is it 2 or something larger). Just curious.
> 
> Each subclass knows how many it will need (0-2, IIRC), except for SkMerge
which
> is variable (-1). Which makes me wonder -- should we just have inputCount as a
> construction param for Common?

Can, though then we have the same funny problem of constructing an object, and
then have to remember to ask if the object is "valid", which is the pattern
we're trying to avoid for unflattening. Making it a method with a bool seems
more natural to handle failure.

[Stephen White, 2014-07-16 20:53:06]

On 2014/07/16 20:39:42, reed1 wrote:
> On 2014/07/16 19:48:04, Stephen White wrote:
> >
>
https://codereview.chromium.org/395273002/diff/20001/include/core/SkImageFilt...
> > File include/core/SkImageFilter.h (right):
> > 
> >
>
https://codereview.chromium.org/395273002/diff/20001/include/core/SkImageFilt...
> > include/core/SkImageFilter.h:225: kPreallocated = 4
> > On 2014/07/16 19:37:37, reed1 wrote:
> > > On 2014/07/16 19:26:11, Stephen White wrote:
> > > > Personally, I'm not fond of these preallocated arrays of some arbitrary
> > size.
> > > > Better to add it when profiling tells you than to do it preemptively,
> <span
> > > id="abbrex-spnAbbr_20617" class="abbrex-Abbr abbrex-Abbr_IMHO
> > abbrex-AbbrUnknown
> > > abbrex-AbbrUnknownVisible">IMHO</span>.
> > > 
> > > Calling new is a pretty aweful thing if not needed, and the complexity is
> > > totally isolated to this one place, so my take is that this pattern is
safe
> > and
> > > helpful.
> > 
> > If you mean awful for performance, considering that every filter is going to
> > allocate an SkBitmap of primitive size, and then touch all those pixels at
> least
> > once, allocating a small chunk of n pointers seems pretty insignificant by
> > comparison.
> > 
> > > All of that said, do you know the maximum "required" input count for
> > > your subclasses (i.e. is it 2 or something larger). Just curious.
> > 
> > Each subclass knows how many it will need (0-2, IIRC), except for SkMerge
> which
> > is variable (-1). Which makes me wonder -- should we just have inputCount as
a
> > construction param for Common?
> 
> Can, though then we have the same funny problem of constructing an object, and
> then have to remember to ask if the object is "valid", which is the pattern
> we're trying to avoid for unflattening. Making it a method with a bool seems
> more natural to handle failure.

Well, we already kind of have that -- it's trivially valid on construction, but 
might be partially invalid after unflatten() if it fails.

I suppose another way to fix it would be to have a static unflattenCommon() 
function on SkImageFilter: 

bool unflattenCommon(Common* common)

or even

static Common* unflattenCommon()

if we want it to construct it always-valid.

Then we only init or construct the Common when we know what's read is valid.

[reed1, 2014-07-16 21:02:18]

Ah, I think I know what you're suggesting now, and why I structured this
differently.

For a subclass to use this, they want to (in the end) call a public Factory,
which will take the input filters and croprect. Thus the subclass needs to have
those to pass in.

e.g.

SkFlattenable* Subclass::CreateProc(SkReadBuffer& buffer) {
    Common common;
    if (!common.unflatten(buffer, 1) {
        return NULL;
    }
    SkScalar width = buffer.readScalar();
    SkScalar height = buffer.readScalar();
    return Subclass::Create(width, height, buffer.inputs()[0],
&buffer.cropRect());
}

[Stephen White, 2014-07-16 21:15:56]

On 2014/07/16 21:02:18, reed1 wrote:
> Ah, I think I know what you're suggesting now, and why I structured this
> differently.
> 
> For a subclass to use this, they want to (in the end) call a public Factory,
> which will take the input filters and croprect. Thus the subclass needs to
have
> those to pass in.
> 
> e.g.
> 
> SkFlattenable* Subclass::CreateProc(SkReadBuffer& buffer) {
>     Common common;
>     if (!common.unflatten(buffer, 1) {
>         return NULL;
>     }
>     SkScalar width = buffer.readScalar();
>     SkScalar height = buffer.readScalar();
>     return Subclass::Create(width, height, buffer.inputs()[0],
> &buffer.cropRect());
> }


Right, I get that. I was just suggesting:

> SkFlattenable* Subclass::CreateProc(SkReadBuffer& buffer) {
>     Common common;
>     if (!SkImageFilter::unflattenCommon(&common, buffer, 1) {
>         return NULL;
>     }
>     SkScalar width = buffer.readScalar();
>     SkScalar height = buffer.readScalar();
>     return Subclass::Create(width, height, common.inputs(),
> common.cropRect());
> }

To avoid the problem of the partially-initialized Common.

Actually, I guess you could avoid it by not assigning to Common's
member vars until all validation passes. That'd work too. Anyway 
it's not a big deal -- much less of a big deal than a partially-initialized
SkImageFilter.

BTW, if we're gonna go down this road, I'd just make all the
subclass factories to call the constructors directly (since we
know the params are now valid), and pass in the Common:

     return SkNEW_ARGS(Subclass, (common, width, height));

Would that work?

[Stephen White, 2014-07-16 21:16:55]

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

https://codereview.chromium.org/395273002/diff/40001/include/core/SkImageFilt...
include/core/SkImageFilter.h:227: SkImageFilter*  fStorage[kPreallocated];
Could we generalize this to some templated array? I think Blink's Vector class
does this (internally-allocated up to a predefined size).

[reed1, 2014-07-17 12:57:33]

On 2014/07/16 21:15:56, Stephen White wrote:
> On 2014/07/16 21:02:18, reed1 wrote:
> > Ah, I think I know what you're suggesting now, and why I structured this
> > differently.
> > 
> > For a subclass to use this, they want to (in the end) call a public Factory,
> > which will take the input filters and croprect. Thus the subclass needs to
> have
> > those to pass in.
> > 
> > e.g.
> > 
> > SkFlattenable* Subclass::CreateProc(SkReadBuffer& buffer) {
> >     Common common;
> >     if (!common.unflatten(buffer, 1) {
> >         return NULL;
> >     }
> >     SkScalar width = buffer.readScalar();
> >     SkScalar height = buffer.readScalar();
> >     return Subclass::Create(width, height, buffer.inputs()[0],
> > &buffer.cropRect());
> > }
> 
> 
> Right, I get that. I was just suggesting:
> 
> > SkFlattenable* Subclass::CreateProc(SkReadBuffer& buffer) {
> >     Common common;
> >     if (!SkImageFilter::unflattenCommon(&common, buffer, 1) {
> >         return NULL;
> >     }
> >     SkScalar width = buffer.readScalar();
> >     SkScalar height = buffer.readScalar();
> >     return Subclass::Create(width, height, common.inputs(),
> > common.cropRect());
> > }
> 
> To avoid the problem of the partially-initialized Common.
> 
> Actually, I guess you could avoid it by not assigning to Common's
> member vars until all validation passes. That'd work too. Anyway 
> it's not a big deal -- much less of a big deal than a partially-initialized
> SkImageFilter.
> 
> BTW, if we're gonna go down this road, I'd just make all the
> subclass factories to call the constructors directly (since we
> know the params are now valid), and pass in the Common:
> 
>      return SkNEW_ARGS(Subclass, (common, width, height));
> 
> Would that work?

Part of the large Cl pattern is to centralize the checking for
valid-params/specialized-backends into the factories. This means the CreateProc
need not do much validation, since it is going to call the public Factory, which
needs to do those checks anyways. Calling any constuctor directly means having
to duplicate the checking/specialization code that should already exist in the
Factory.

[Stephen White, 2014-07-17 13:04:41]

On 2014/07/17 12:57:33, reed1 wrote:
> On 2014/07/16 21:15:56, Stephen White wrote:
> > On 2014/07/16 21:02:18, reed1 wrote:
> > > Ah, I think I know what you're suggesting now, and why I structured this
> > > differently.
> > > 
> > > For a subclass to use this, they want to (in the end) call a public
Factory,
> > > which will take the input filters and croprect. Thus the subclass needs to
> > have
> > > those to pass in.
> > > 
> > > e.g.
> > > 
> > > SkFlattenable* Subclass::CreateProc(SkReadBuffer& buffer) {
> > >     Common common;
> > >     if (!common.unflatten(buffer, 1) {
> > >         return NULL;
> > >     }
> > >     SkScalar width = buffer.readScalar();
> > >     SkScalar height = buffer.readScalar();
> > >     return Subclass::Create(width, height, buffer.inputs()[0],
> > > &buffer.cropRect());
> > > }
> > 
> > 
> > Right, I get that. I was just suggesting:
> > 
> > > SkFlattenable* Subclass::CreateProc(SkReadBuffer& buffer) {
> > >     Common common;
> > >     if (!SkImageFilter::unflattenCommon(&common, buffer, 1) {
> > >         return NULL;
> > >     }
> > >     SkScalar width = buffer.readScalar();
> > >     SkScalar height = buffer.readScalar();
> > >     return Subclass::Create(width, height, common.inputs(),
> > > common.cropRect());
> > > }
> > 
> > To avoid the problem of the partially-initialized Common.
> > 
> > Actually, I guess you could avoid it by not assigning to Common's
> > member vars until all validation passes. That'd work too. Anyway 
> > it's not a big deal -- much less of a big deal than a partially-initialized
> > SkImageFilter.
> > 
> > BTW, if we're gonna go down this road, I'd just make all the
> > subclass factories to call the constructors directly (since we
> > know the params are now valid), and pass in the Common:
> > 
> >      return SkNEW_ARGS(Subclass, (common, width, height));
> > 
> > Would that work?
> 
> Part of the large Cl pattern is to centralize the checking for
> valid-params/specialized-backends into the factories. This means the
CreateProc
> need not do much validation, since it is going to call the public Factory,
which
> needs to do those checks anyways. Calling any constuctor directly means having
> to duplicate the checking/specialization code that should already exist in the
> Factory.

My thought was that the deserialization factory would have to be signaling
failed
validation to the read buffer anyway (via buffer.validate()) which the normal
factory
wouldn't. But maybe just a NULL returned from the factory fn would do that in
one 
place? If that's what you had in mind, your approach makes more sense.

At any rate, most of my objections are fairly minor. LGTM.

[reed1, 2014-07-17 13:32:35]

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

https://codereview.chromium.org/395273002/diff/40001/include/core/SkImageFilt...
include/core/SkImageFilter.h:227: SkImageFilter*  fStorage[kPreallocated];
On 2014/07/16 21:16:54, Stephen White wrote:
> Could we generalize this to some templated array? I think Blink's Vector class
> does this (internally-allocated up to a predefined size).

Done.

[reed1, 2014-07-17 13:34:02]

You comments clearly point out that we are still in-the-mux of refining our
factory and buffer-validation patterns. After the "big" CL, no doubt we will
continue to iterate on this, and that work may impact how we organize this
common code in imagefilters.

[reed1, 2014-07-17 13:34:16]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-07-17 13:34:53]

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

[commit-bot: I haz the power, 2014-07-17 14:03:14]

Change committed as b959ec7815ae0f65f2aabdeaf280a2a2ee6db955

[Stephen White, 2014-07-18 15:14:55]

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

https://codereview.chromium.org/395273002/diff/60001/src/core/SkImageFilter.c...
src/core/SkImageFilter.cpp:43: int count = buffer.readInt();
I think we should bail if the count is negative here, as it was in the old code.
Otherwise a count of -1 in a filter with variable inputs will pass validation
below.