Work in progress to make SkShader immutable.

Work in progress to make SkShader immutable.

Currently, SkShader is modified by each draw call. SkShader::setContext
is called with information about the the current state (device, matrix
paint). Each SkShader subclass caches information about that state,
which is later used by the various flavors of shadeSpan (or a
ShadeProc).

In this CL, the caches are moved from the SkShader to a new object,
called SkShader::Context. setContext, no longer a virtual function, now
creates a Context. It then asks its subclass how much extra space is
needed for drawing. It allocates (or places in existing storage) enough
space for the subclass on the Context, and passes it to the subclass
via onSetContext. The new virtual function onSetContext does the work
of the old setContext, only it places its subclass specific info on
the Context. Now, each version of shadeSpan takes the Context as a
parameter, so the SkShader can get its cached information for drawing.
endContext now takes the Context as a parameter (and deletes it).

The Context is stored on the SkBlitter, which takes care of passing
it to each shadeSpan and finally calling endContext.

BUG=skia:1976

[scroggo, 2014-02-11 22:01:21]

This doesn't build yet (an earlier version did, which included lots of similar
changes for each shader subclass, and no subclass specific data). The
interesting changes are mostly in SkShader. I've modified SkComposeShader to
show how an interesting subclass will treat the Context, and the blitters show
how the new Context object is used.

We can also do some placement tricks to avoid the allocation for the Context,
but I wanted to trim this down to the meat of the approach to see what we think.

The current approach (more details in the CL itself...) writes subclass specific
data to a buffer. Another option might be to have subclasses of
SkShader::Context. In that approach, I imagine setContext would look something
like the following:

SkSubclass : public SkShader {
public:
  virtual Context* setContext(...) SK_OVERRIDE {
    Context* c = INHERITED::setContext(...);
    if (NULL == c) {
      return NULL;
    }
    return new SubclassContext(c, ...);
  }
private:
  class SubclassContext : public Context {...};
};

[Dominik Grewe, 2014-02-12 12:24:56]

On 2014/02/11 22:01:21, scroggo wrote:
> The current approach (more details in the CL itself...) writes subclass
specific
> data to a buffer. Another option might be to have subclasses of
> SkShader::Context. In that approach, I imagine setContext would look something
> like the following:
> 
> SkSubclass : public SkShader {
> public:
>   virtual Context* setContext(...) SK_OVERRIDE {
>     Context* c = INHERITED::setContext(...);
>     if (NULL == c) {
>       return NULL;
>     }
>     return new SubclassContext(c, ...);
>   }
> private:
>   class SubclassContext : public Context {...};
> };

It would seem nicer to have subclasses and simply call the parent class's
constructor in their constructor. But it seems that won't work as easily,
because construction might fail and Sk3DShader seems to have a complex way to
set up its context.
Is there another way we could check for a failed construction? The only case
where I've seen it return false is when the matrix isn't invertible.
Not sure it's possible but it I think it would be a lot cleaner if we could
avoid all of this manual memory management.

[Dominik Grewe, 2014-02-14 12:31:30]

What about this?
Use Context subclasses, so each subclass of SkShader returns the correct type
from setContext() (or NULL if it fails). Instead of building the parent's
context separately first and then passing it on to the class-specific context,
call the parent's context's constructor. Have a valid() method to check if
construction succeeded or not. setContext returns NULL and frees the object if
it failed.

Something like the following. Wdyt?

class SkShader {
public:
  class Context {
  public:
    Context(...); // same parameters as setContext
    virtual bool valid() { return fTotalInverseClass != kInvalid_MatrixClass; }
// for example
  };

  virtual Context* setContext(...) {
    Context* context = SkNEW_ARGS(Context, (...));  // pass arguments directly
into constructor (do logic there)
    if (!context->valid()) {
      SkDELETE(context);
      return NULL;
    }
    return context;
  }
};

class SkSubShader : public SkShader {
public:
  virtual Context* setContext(...) SK_OVERRIDE {
    // Essentially the same as above but with new context class.
    // Maybe use macro or something do avoid code duplication.
  }
private:
  class SubContext : public SkShader::Context {
    SubContext(...) : SkShaderContext(...) {
      if (!INHERITED::valid()) {
        return;
      }
      ...
    }
    // possibly implement own valid() if construction can fail.
  };
};


I think my worry about Sk3DShader is unfounded, because it seems we can simply
store the context of the proxy in the context of Sk3DShader.

[scroggo, 2014-02-14 14:59:48]

On 2014/02/14 12:31:30, Dominik Grewe wrote:
> What about this?
> Use Context subclasses, so each subclass of SkShader returns the correct type
> from setContext() (or NULL if it fails). Instead of building the parent's
> context separately first and then passing it on to the class-specific context,
> call the parent's context's constructor. Have a valid() method to check if
> construction succeeded or not. setContext returns NULL and frees the object if
> it failed.
> 
> Something like the following. Wdyt?
> 
> class SkShader {
> public:
>   class Context {
>   public:
>     Context(...); // same parameters as setContext
>     virtual bool valid() { return fTotalInverseClass != kInvalid_MatrixClass;
}
> // for example
>   };
> 
>   virtual Context* setContext(...) {
>     Context* context = SkNEW_ARGS(Context, (...));  // pass arguments directly
> into constructor (do logic there)
>     if (!context->valid()) {
>       SkDELETE(context);
>       return NULL;
>     }
>     return context;
>   }
> };
> 
> class SkSubShader : public SkShader {
> public:
>   virtual Context* setContext(...) SK_OVERRIDE {
>     // Essentially the same as above but with new context class.
>     // Maybe use macro or something do avoid code duplication.
>   }
> private:
>   class SubContext : public SkShader::Context {
>     SubContext(...) : SkShaderContext(...) {
>       if (!INHERITED::valid()) {
>         return;
>       }
>       ...
>     }
>     // possibly implement own valid() if construction can fail.
>   };
> };
> 
> 
> I think my worry about Sk3DShader is unfounded, because it seems we can simply
> store the context of the proxy in the context of Sk3DShader.

The Sk3DShader works like a couple other shaders, and I agree, we'll just store
their extra information on their sub-Contexts.

In response to your earlier comment about why a shader might return false, there
are a couple of ways besides the non-invertible matrix. SkBitmapProcShader can
return false if its bitmap is not ready to draw/valid, or if chooseProcs()
returns NULL. In addition, SkComposeShader supplies a potentially different
matrix to its subshaders, so they could return false even if the initial matrix
was invertible.

Re: valid, in general we don't like having a way to create an object which is
not (and cannot be made) valid. On the other hand, if we found it to be a help
to performance it might be worth it - especially since this is basically
internal details of shaders. (Right now someone could write a subclass of
SkShader outside of Skia, but I'm guessing we want to put a stop to that, as
long as we're forcing them to use a factory.)

I've been thinking some more about it, and I was thinking we'd use a copy
constructor along with placement new in order to prevent all the temporary
allocations. I'll try to get something up Monday with that approach (today I'll
be enjoying the snow in NC!).

[scroggo, 2014-03-06 19:28:34]

On 2014/02/14 14:59:48, scroggo wrote:
> On 2014/02/14 12:31:30, Dominik Grewe wrote:
> > What about this?
> > Use Context subclasses, so each subclass of SkShader returns the correct
type
> > from setContext() (or NULL if it fails). Instead of building the parent's
> > context separately first and then passing it on to the class-specific
context,
> > call the parent's context's constructor. Have a valid() method to check if
> > construction succeeded or not. setContext returns NULL and frees the object
if
> > it failed.
> > 
> > Something like the following. Wdyt?
> > 
> > class SkShader {
> > public:
> >   class Context {
> >   public:
> >     Context(...); // same parameters as setContext
> >     virtual bool valid() { return fTotalInverseClass !=
kInvalid_MatrixClass;
> }
> > // for example
> >   };
> > 
> >   virtual Context* setContext(...) {
> >     Context* context = SkNEW_ARGS(Context, (...));  // pass arguments
directly
> > into constructor (do logic there)
> >     if (!context->valid()) {
> >       SkDELETE(context);
> >       return NULL;
> >     }
> >     return context;
> >   }
> > };
> > 
> > class SkSubShader : public SkShader {
> > public:
> >   virtual Context* setContext(...) SK_OVERRIDE {
> >     // Essentially the same as above but with new context class.
> >     // Maybe use macro or something do avoid code duplication.
> >   }
> > private:
> >   class SubContext : public SkShader::Context {
> >     SubContext(...) : SkShaderContext(...) {
> >       if (!INHERITED::valid()) {
> >         return;
> >       }
> >       ...
> >     }
> >     // possibly implement own valid() if construction can fail.
> >   };
> > };
> > 
> > 
> > I think my worry about Sk3DShader is unfounded, because it seems we can
simply
> > store the context of the proxy in the context of Sk3DShader.
> 
> The Sk3DShader works like a couple other shaders, and I agree, we'll just
store
> their extra information on their sub-Contexts.
> 
> In response to your earlier comment about why a shader might return false,
there
> are a couple of ways besides the non-invertible matrix. SkBitmapProcShader can
> return false if its bitmap is not ready to draw/valid, or if chooseProcs()
> returns NULL. In addition, SkComposeShader supplies a potentially different
> matrix to its subshaders, so they could return false even if the initial
matrix
> was invertible.
> 
> Re: valid, in general we don't like having a way to create an object which is
> not (and cannot be made) valid. On the other hand, if we found it to be a help
> to performance it might be worth it - especially since this is basically
> internal details of shaders. (Right now someone could write a subclass of
> SkShader outside of Skia, but I'm guessing we want to put a stop to that, as
> long as we're forcing them to use a factory.)
> 
> I've been thinking some more about it, and I was thinking we'd use a copy
> constructor along with placement new in order to prevent all the temporary
> allocations. I'll try to get something up Monday with that approach (today
I'll
> be enjoying the snow in NC!).

Sorry for the radio silence. I spent some time investigating using Context
subclasses,
and then got more focused on how to avoid extra allocations (if you're curious,
see
https://codereview.chromium.org/179343005/), along with other tasks.

In my opinion, the subclass version won't buy us much. We'll still need to do a
bunch
of memory management. After putting some more thought into it, here's how I
imagine
the subclass working:

SkSubclass : public SkShader {
public:
  virtual Context* setContext(..., storage, storageSize) SK_OVERRIDE {
    char myStorage[myStorageSize]; 
    Context* c = INHERITED::setContext(..., myStorage, myStorageSize);
    if (NULL == c) {
      return NULL;
    }
    // construct a new SubclassContext in storage passed in, copying the
information
    // from c
    SubclassContext* subC = placement new SubclassContext( storage, c, ...);
    // Call c's destructor**
    c->~Context();
    return subC;
  }
private:
  class SubclassContext : public Context {...};
};

First, setContext calls inherited which will placement new the parent context in
the
local stack. If NULL is returned, we return NULL. Otherwise, we create the
subclass
context, copying all the data from the parent.

Then it seems like we should call the parent's destructor, since we're done with
it.
But really, if it had anything to do in its destructor, we don't actually want
to do
it. i.e. if it had created a new object, we don't want to delete it, because
subC needs
to have that object too. In theory we could solve this with deep copying or ref
counting,
but both of those are expensive given that we really didn't need the original c
to stick
around (if we could use move semantics here I might be happy with that
solution...). The
time when we actually want to do the destruction is in endContext(). I think
this just
highlights the fact that these Contexts are plain old data rather than classes.

If nothing else, the subclass approach has introduced an extra copy into things
(maybe
that's okay, though).

FWIW, my CL at https://codereview.chromium.org/179343005/ makes this a little
simpler,
but it does not address the copy or the in between struct and class nature of
SubclassContext.

[Dominik Grewe, 2014-03-07 15:52:24]

Yeah, I guess it's not any simpler than what we had before. And with your
initial version we can at least avoid the extra copy and the awkwardness of
context being neither a proper class nor a struct.

I'm still not very keen on all of this manual memory management, but I can't see
an alternative that wouldn't add overhead or may create uninitialized context
objects (though you can argue that the extra context data in fStorage is
uninitialized if something fails...).

[scroggo, 2014-03-13 21:29:32]

On 2014/03/07 15:52:24, Dominik Grewe wrote:
> Yeah, I guess it's not any simpler than what we had before. And with your
> initial version we can at least avoid the extra copy and the awkwardness of
> context being neither a proper class nor a struct.
> 
> I'm still not very keen on all of this manual memory management, but I can't
see
> an alternative that wouldn't add overhead or may create uninitialized context
> objects (though you can argue that the extra context data in fStorage is
> uninitialized if something fails...).

I've been working on this some more, and I'm beginning to agree with you on
using
subclasses. The problem we need to solve is being able to fail on bad data. So
I'm
going to pull that out into a separate function, which gets called first. If the
data won't work, then we can create the context, which should always succeed. I
have the beginnings of a CL up at https://codereview.chromium.org/198193005.

[scroggo, 2014-04-17 14:12:06]

On 2014/03/13 21:29:32, scroggo wrote:
> On 2014/03/07 15:52:24, Dominik Grewe wrote:
> > Yeah, I guess it's not any simpler than what we had before. And with your
> > initial version we can at least avoid the extra copy and the awkwardness of
> > context being neither a proper class nor a struct.
> > 
> > I'm still not very keen on all of this manual memory management, but I can't
> see
> > an alternative that wouldn't add overhead or may create uninitialized
context
> > objects (though you can argue that the extra context data in fStorage is
> > uninitialized if something fails...).
> 
> I've been working on this some more, and I'm beginning to agree with you on
> using
> subclasses. The problem we need to solve is being able to fail on bad data. So
> I'm
> going to pull that out into a separate function, which gets called first. If
the
> data won't work, then we can create the context, which should always succeed.
I
> have the beginnings of a CL up at https://codereview.chromium.org/198193005.

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