[cairo] more pattern rewrite stuff...

[David Reveman]

On Mon, 2005-01-31 at 23:00 -0500, Kristian Høgsberg wrote: 
> David Reveman wrote:
> > On Mon, 2005-01-31 at 15:14 -0500, Kristian Høgsberg wrote: 
> > 
> >>David Reveman wrote:
> >>
> >>>ok, I couldn't stop myself from trying to get this right. This patch
> >>>includes new changes to the pattern system, this time I'm pretty happy
> >>>with how it turned out. 
> >>>
> >>>It removes backend functions set_filter, set_repeat and set_matrix.
> >>>Makes the "pattern <-> gstate" and "pattern <-> backend" connections
> >>>more appropriate and cleans up a lot of stuff. Looking at the diff is
> >>>probably not good enough.
> >>
> >>We discussed this in IRC a couple of days ago, and general consensus was 
> >>that filter, repeat and transform should be moved out of the surface and 
> >>into the pattern.  Owen summarized it here:
> >>
> >>   https://bugs.freedesktop.org/show_bug.cgi?id=2397
> > 
> > 
> > ok, I've done the first step of this then. The the repeat, filter and
> > matrix attributes are still left in the surface only to be picked up by
> > caior_show_surface. Moving filter attribute into the gstate and removing
> > the surface matrix should now be simple, but as it's a quite large
> > change to public API I think it can wait until we get my current patch
> > merged with Owen's fall-back bits. We could also remove the width and
> > height parameters from show_surface, at the same time.
> > 
> > 
> >>>Futher on, we should make the mask surface passed to surface_composite a
> >>>pattern as well, 
> >>
> >>I think we should get this done sooner rather than later.  I can take a 
> >>look at that once you get your patch landed.
> > 
> > good, it shouldn't be to hard once we get the _cairo_pattern_get_surface
> > stuff working well.
> >>> static cairo_int_status_t
> >>>+_cairo_image_surface_change_attributes (cairo_image_surface_t	   *surface,
> >>>+					cairo_surface_attributes_t *attributes)
> >>
> >>So, instead of this, we would set the pixman surface attributes from the 
> >>pattern.
> > 
> > _cairo_image_surface_change_attributes is actually doing this.
> > cairo_surface_attributes_t is basically the same as cairo_pattern_info_t
> > in Owen's patch. Hmm, _cairo_pattern_get_surface returns a surface
> > representing the pattern so I think cairo_surface_attributes_t is more
> > appropriate.
> 
> What I mean is, why not just pass a cairo_pattern_t instead of 
> cairo_surface_attributes_t, since the pattern will contain all the 
> information needed.
> 
> Another point is that these attributes aren't necessarily surface 
> attributes, that's just an artifact^Wimplementation detail of the 
> render/pixman based backends.  For the PDF backend there are no surfaces 
> involved in solid color or gradient patterns.
> 
> Anyway, I guess this will look all different when you get it merged with 
> current CVS.

yeah, I think you're getting this a bit wrong. You'll probably
understand when it gets merged with current CVS (hopefully I'll be able
to get that done sometime this week), but for what it's worth:

_cairo_pattern_get_surface should exist as a convenience function for
backends with render/pixman semantics, giving it a pattern, it returns a
surface for the pattern, right? This surface needs to be updated with
surface attributes (filter, matrix...) to work correctly and these
attributes are filled in by _cairo_pattern_get_surface. This allows
_cairo_pattern_get_surface to return attributes that doesn't match the
pattern and we need to be able to do that as the backends are not
allowed to modify the pattern. PDF-like backends should never call
_cairo_pattern_get_surface.

> 
> >>>@@ -380,33 +387,60 @@
> >>> 				unsigned int		width,
> >>> 				unsigned int		height)
> >>> {
> >>>-    cairo_image_surface_t *dst = abstract_dst;
> >>>-    cairo_image_surface_t *src;
> >>>-    cairo_image_surface_t *mask = (cairo_image_surface_t *) generic_mask;
> >>>-    int x_offset, y_offset;
> >>>-
> >>>+    cairo_surface_attributes_t	attributes;
> >>>+    cairo_image_surface_t	*dst = abstract_dst;
> >>>+    cairo_image_surface_t	*src;
> >>>+    cairo_image_surface_t	*mask = (cairo_image_surface_t *) generic_mask;
> >>>+    cairo_image_surface_t	*mask_alpha = NULL;
> >>>+    cairo_int_status_t		status;
> >>>+    double			alpha = pattern->color.alpha;
> >>>+
> >>>+    if (generic_mask)
> >>>+    {
> >>>+	if (generic_mask->backend != dst->base.backend)
> >>>+	    return CAIRO_INT_STATUS_UNSUPPORTED;
> >>>+    }
> >>>+    else if (pattern->type == CAIRO_PATTERN_SURFACE && alpha != 1.0)
> >>>+    {
> >>>+	mask = mask_alpha = (cairo_image_surface_t *)
> >>>+	    _cairo_surface_create_similar_solid (&dst->base, CAIRO_FORMAT_A8,
> >>>+						 1, 1, &pattern->color);
> >>>+	if (!mask)
> >>>+	    return CAIRO_STATUS_NO_MEMORY;
> >>>+	
> >>>+	cairo_surface_set_repeat (&mask->base, 1);
> >>>+	alpha = 1.0;
> >>>+    }
> >>>+    
> >>>     src = (cairo_image_surface_t *)
> >>>-	_cairo_pattern_get_surface (pattern, &dst->base,
> >>>+	_cairo_pattern_get_surface (pattern, alpha, &dst->base,
> >>> 				    src_x, src_y, width, height,
> >>>-				    &x_offset, &y_offset);
> >>
> >>Why are you reading out the alpha value from the pattern color only to 
> >>pass it back to _cairo_pattern_get_surface, which use it as it used to 
> >>use pattern->color.alpha?  
> > 
> > If no mask surface or eventually mask pattern is passed to composite, we
> > can use the mask to accelerate overall surface alpha. The alpha
> > parameter tells _cairo_pattern_get_surface what alpha to use for surface
> > patterns. Hence, passing alpha=1.0 and _cairo_pattern_get_surface will
> > not do overall alpha for us. 
> 
> Yeah, but why don't you just change the alpha value in the pattern 
> instead of overriding it by this new alpha argument?

Backends are not allow to modify the pattern. Think of the fall-backs
handled in cairo_surface.c. We could save pattern attributes to
temporary variables and restore them, but I like my solution better than
that.

> 
> > The gradient struct will need to have alpha as well. How about this?
> > 
> > struct _cairo_pattern {
> > 	unsigned int ref_count;
> > 
> > 	cairo_extend_t extend;
> > 	cairo_filter_t filter;
> > 	cairo_matrix_t matrix;
> > 
> > 	cairo_pattern_type_t type;
> > 	union {
> > 		double	alpha;
> > 		struct {
> > 			double	alpha;
> > 			double	red, green, blue;
> > 		} color;
> > 		struct {
> > 			double		alpha;
> > 			cairo_surface_t *source;
> > 		} surface;
> > 		struct {
> > 			double			alpha;
> > 			cairo_point_double_t	point0;
> > 			cairo_point_double_t	point0;
> > 			cairo_color_stop_t	*stops;
> > 			int			num_stops;
> > 		} gradient;
> > 	} u;
> > };
> 
> Yeah, that looks pretty good.
> 
> cheers,
> Kristian

-David