Work (in progress) to make SkShader immutable.

Work (in progress) to make SkShader immutable.

Rename SkShader to SkShaderGenerator (not positive we should stick
with the new name, but it's a better way to think about the way it
works now). The object will eventually be immutable, and it will
create the object that does the work at draw time: a ShaderImpl.

Replace setContext with a series of calls:

validContext() - returns false if the old setContext call would have.

shaderImplSize() - returns the size needed by the SkShader subclass.

createShaderImpl() - return a new ShaderImpl object, which holds many
of the functions that were previously on SkShader. This should always
succeed if validContext() returned true (I think... need to look
at all shaders just in case). It has a const reference to the
SkShaderGenerator, so data need not be duplicated between the two.

createShaderImpl will place the new object in storage provided by
an SkSmallAllocator, in general.

BUG=skia:1976

[Dominik Grewe, 2014-03-14 16:00:40]

I really prefer this to what we had before. I'm wondering if we can make it more
similar to SkDrawLooper, i.e. have a ShaderContext which simply stores the
mutable data. We could have a |ShaderContext* SkShader::createContext(...)|
which first calls validContext and only creates the object if it returns true
(otherwise NULL). This way you don't have to rely on the user to call both
functions.
It may also allow you to cache some of the values, e.g. the inverted matrix for
the SkShader base class, so you don't have to compute them twice. But I'm not
sure if that's necessary.

https://codereview.chromium.org/198193005/diff/1/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/198193005/diff/1/src/core/SkShader.cpp#newcode53
src/core/SkShader.cpp:53: fTotalInverseClass =
(uint8_t)ComputeMatrixClass(fTotalInverse);
These variables should be temporary rather than member variables right?

https://codereview.chromium.org/198193005/diff/1/src/core/SkShader.cpp#newcod...
src/core/SkShader.cpp:251: SkASSERT(this->contextValid(device, paint, matrix));
Could we just call validContext() here and return NULL if it returns false?

[scroggo, 2014-03-17 15:11:56]

https://codereview.chromium.org/198193005/diff/1/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/198193005/diff/1/src/core/SkShader.cpp#newcode53
src/core/SkShader.cpp:53: fTotalInverseClass =
(uint8_t)ComputeMatrixClass(fTotalInverse);
On 2014/03/14 16:00:40, Dominik Grewe wrote:
> These variables should be temporary rather than member variables right?

Oh yes, of course.

https://codereview.chromium.org/198193005/diff/1/src/core/SkShader.cpp#newcod...
src/core/SkShader.cpp:251: SkASSERT(this->contextValid(device, paint, matrix));
On 2014/03/14 16:00:40, Dominik Grewe wrote:
> Could we just call validContext() here and return NULL if it returns false?

The problem with that is that SkSmallAllocator has no way of knowing that you
did not create an object in the space provided by reserveT, so it will still
call the destructor. One hacky way to solve that is if this function returns
NULL to create a dummy one to call the destructor on. Or maybe SkSmallAllocator
needs a way to just tell it to not call the destructor. That may not be too bad.

[Dominik Grewe, 2014-03-17 16:07:07]

I looked into SkBitmapProcShader. Afaict we can use a similar method there, but
validation is quite complex. Hopefully not too expensive... I think we'd have to
create a temporary SkBitmapProcState and try setting it up via chooseProcs
(possibly we can have a more light-weight validation method that only does as
much setup as needed to verify the parameters).

I'm fairly busy this week myself, but I'll try to upload a WIP patch later this
week. Then we can see what's the best way forward.

https://codereview.chromium.org/198193005/diff/1/src/core/SkShader.cpp
File src/core/SkShader.cpp (right):

https://codereview.chromium.org/198193005/diff/1/src/core/SkShader.cpp#newcod...
src/core/SkShader.cpp:251: SkASSERT(this->contextValid(device, paint, matrix));
On 2014/03/17 15:11:56, scroggo wrote:
> On 2014/03/14 16:00:40, Dominik Grewe wrote:
> > Could we just call validContext() here and return NULL if it returns false?
> 
> The problem with that is that SkSmallAllocator has no way of knowing that you
> did not create an object in the space provided by reserveT, so it will still
> call the destructor. One hacky way to solve that is if this function returns
> NULL to create a dummy one to call the destructor on. Or maybe
SkSmallAllocator
> needs a way to just tell it to not call the destructor. That may not be too
bad.

Good point. Not sure which way is best. If we have a reserveT that doesn't call
a constructor maybe it's okay to add a freeT that makes sure the destructor
isn't called, as you suggested.

[scroggo, 2014-03-19 05:01:01]

On 2014/03/17 16:07:07, Dominik Grewe wrote:
> I looked into SkBitmapProcShader. Afaict we can use a similar method there,
but
> validation is quite complex. Hopefully not too expensive... I think we'd have
to
> create a temporary SkBitmapProcState and try setting it up via chooseProcs
> (possibly we can have a more light-weight validation method that only does as
> much setup as needed to verify the parameters).

If we allow createShaderImpl to return NULL even if validation fails,
SkBitmapProcShader
won't need any major changes.

> 
> I'm fairly busy this week myself, but I'll try to upload a WIP patch later
this
> week. Then we can see what's the best way forward.
> 
> https://codereview.chromium.org/198193005/diff/1/src/core/SkShader.cpp
> File src/core/SkShader.cpp (right):
> 
>
https://codereview.chromium.org/198193005/diff/1/src/core/SkShader.cpp#newcod...
> src/core/SkShader.cpp:251: SkASSERT(this->contextValid(device, paint,
matrix));
> On 2014/03/17 15:11:56, scroggo wrote:
> > On 2014/03/14 16:00:40, Dominik Grewe wrote:
> > > Could we just call validContext() here and return NULL if it returns
false?
> > 
> > The problem with that is that SkSmallAllocator has no way of knowing that
you
> > did not create an object in the space provided by reserveT, so it will still
> > call the destructor. One hacky way to solve that is if this function returns
> > NULL to create a dummy one to call the destructor on. Or maybe
> SkSmallAllocator
> > needs a way to just tell it to not call the destructor. That may not be too
> bad.
> 
> Good point. Not sure which way is best. If we have a reserveT that doesn't
call
> a constructor maybe it's okay to add a freeT that makes sure the destructor
> isn't called, as you suggested.

[Dominik Grewe, 2014-03-19 09:54:27]

On 2014/03/19 05:01:01, scroggo wrote:
> On 2014/03/17 16:07:07, Dominik Grewe wrote:
> > I looked into SkBitmapProcShader. Afaict we can use a similar method there,
> but
> > validation is quite complex. Hopefully not too expensive... I think we'd
have
> to
> > create a temporary SkBitmapProcState and try setting it up via chooseProcs
> > (possibly we can have a more light-weight validation method that only does
as
> > much setup as needed to verify the parameters).
> 
> If we allow createShaderImpl to return NULL even if validation fails,
> SkBitmapProcShader
> won't need any major changes.

But where would you store the SkBitmapProcState? If it's part of the ShaderImpl
you may end up with a half-constructed object (just what we're trying to avoid).
The only other option I see is to allocate a separate object on the stack inside
createShaderImpl and copy that, if created successfully, into ShaderImpl (would
require a deep copy or passing ownership of pointers).

[Dominik Grewe, 2014-03-19 15:06:57]

On 2014/03/19 09:54:27, Dominik Grewe wrote:
> On 2014/03/19 05:01:01, scroggo wrote:
> > On 2014/03/17 16:07:07, Dominik Grewe wrote:
> > > I looked into SkBitmapProcShader. Afaict we can use a similar method
there,
> > but
> > > validation is quite complex. Hopefully not too expensive... I think we'd
> have
> > to
> > > create a temporary SkBitmapProcState and try setting it up via chooseProcs
> > > (possibly we can have a more light-weight validation method that only does
> as
> > > much setup as needed to verify the parameters).
> > 
> > If we allow createShaderImpl to return NULL even if validation fails,
> > SkBitmapProcShader
> > won't need any major changes.
> 
> But where would you store the SkBitmapProcState? If it's part of the
ShaderImpl
> you may end up with a half-constructed object (just what we're trying to
avoid).
> The only other option I see is to allocate a separate object on the stack
inside
> createShaderImpl and copy that, if created successfully, into ShaderImpl
(would
> require a deep copy or passing ownership of pointers).

Or are you okay with half-constructed objects as long as they're never returned?
That would certainly simplify the code a lot and it would avoid having to
recompute things.

[scroggo, 2014-04-17 14:11:45]

On 2014/03/19 15:06:57, Dominik Grewe wrote:
> On 2014/03/19 09:54:27, Dominik Grewe wrote:
> > On 2014/03/19 05:01:01, scroggo wrote:
> > > On 2014/03/17 16:07:07, Dominik Grewe wrote:
> > > > I looked into SkBitmapProcShader. Afaict we can use a similar method
> there,
> > > but
> > > > validation is quite complex. Hopefully not too expensive... I think we'd
> > have
> > > to
> > > > create a temporary SkBitmapProcState and try setting it up via
chooseProcs
> > > > (possibly we can have a more light-weight validation method that only
does
> > as
> > > > much setup as needed to verify the parameters).
> > > 
> > > If we allow createShaderImpl to return NULL even if validation fails,
> > > SkBitmapProcShader
> > > won't need any major changes.
> > 
> > But where would you store the SkBitmapProcState? If it's part of the
> ShaderImpl
> > you may end up with a half-constructed object (just what we're trying to
> avoid).
> > The only other option I see is to allocate a separate object on the stack
> inside
> > createShaderImpl and copy that, if created successfully, into ShaderImpl
> (would
> > require a deep copy or passing ownership of pointers).
> 
> Or are you okay with half-constructed objects as long as they're never
returned?
> That would certainly simplify the code a lot and it would avoid having to
> recompute things.

Replaced with https://codereview.chromium.org/207683004/