[cairo] cairo pattern/glitz patches

[David Reveman]

On Fri, 2005-02-25 at 15:32 -0500, Carl Worth wrote: 
> On Fri, 25 Feb 2005 14:09:27 +0100, David Reveman wrote:
> > Possibly, I'm not completely happy with the current public gradient
> > interface. I don't like the way color stops are specified. We should
> > probably give this interface a second thought.
> 
> As you recall, the R, G, B, and A interface of
> cairo_pattern_add_color_stop was something we considered when coming
> up with the replacement for cairo_set_rgb_color.
> 
> There we decided on three different ways to set the source to a single
> color:
> 
> 	cairo_set_source_rgb
> 	cairo_set_source_rgba
> 	cairo_set_source_rgba_premultiplied
> 
> We might want the same three variations for creating color stops.
> 
> > > If that's the 
> > > case, shouldn't we use a construction more like cairo_surface_t?
> 
> I do agree that we should decide on one "best" way to do sub-typing
> and apply it consistently throughout cairo. So, I'm glad the current
> debate is happening.
> 
> And I wouldn't be too opposed to having two different implementations
> in CVS if the understood goal was to change one of them to match the
> other.
> 
> One thing I don't like about the current proposed patch is that it
> changes the public header file for what really should be an
> implementation detail, (which is really a fault in C, not the
> patch). If the outer-level-union-based sub-typing wins, would it make
> sense to hide it in a struct for the sake of cairo.h?
> 
> > This means that we can't allocate a general pattern on the stack. We do
> > this a lot (_cairo_pattern_copy) and I think we like to keep doing
> > this. 
> 
> I don't see this a lot in the code. In CVS I see cairo_pattern_t on
> the stack in four places, but in each instance, the usage of the
> pattern is only with one specific sub-type.

In _cairo_gstate_pattern_init_copy we call _cairo_pattern_init_copy and
_cairo_gstate_pattern_init_copy is always used before a pattern is sent
to the backend. In one of my later patches a use
_cairo_pattern_init_copy for some acceleration hooks (make a copy of the
pattern, modify the copy and pass this copy to sub functions). This is
all done to unknown pattern types.  

> 
> The patch does add a call to _cairo_pattern_init_copy with a pattern
> on the stack, but would calling some _cairo_pattern_clone, (and
> cairo_pattern_destroy), really be that much worse?

Maybe not when _cairo_pattern_create/clone can't fail. However, "copy
pattern, modify pattern and pass modified pattern to sub function" is
the way I've implemented acceleration hooks for patterns (you'll see
more of this with all my patches applied). I think it works well and
it's easy to understand when you read the code. In some cases for the
glitz backend, this means that we're making a total of four sub-copies
of the original pattern for one drawing operation and the number of
copies might grow larger as we add more acceleration hooks. If no one
sees any future performance problems with all these mallocs for every
drawing operation or if someone can come up with a better way to handle
acceleration hooks I don't mind changing the cairo_pattern_t struct to a
cairo_surface_t like struct, but otherwise, I think we should stick to a
union for the pattern.

-David