[cairo] Meta surface update and new ps surface

[Carl Worth]

On Fri, 17 Jun 2005 15:17:58 -0400, Kristian Høgsberg wrote:
> Using the meta surface I posted a few weeks back, I've been working on a 
> better postscript backend.  I'm attaching a patch with the meta surface 
> and the new postscript backend.  The meta surface hasn't changed much 
> from what I posted earlier; I've updated it to support the new path 
> clipping and fixed a couple of bugs.

Kristian,

You're doing fantastic work here. Keep it up!

I haven't looked at the postscript-related piece yet, (I'll tackle
that in a separate message), but here's some review of
cairo-meta-surface.c, (and much of this already communicated in IRC).

> +static void
> +_meta_pattern_init_copy (cairo_pattern_t *dest, cairo_pattern_t *src)
> +{
> +    if (src)
> +	_cairo_pattern_init_copy (dest, src);
> +    else
> +	dest->ref_count = 0;
> +}
> +
> +static void
> +_meta_pattern_fini (cairo_pattern_t *pattern)
> +{
> +    if (pattern->ref_count > 0)
> +	_cairo_pattern_fini (pattern);
> +}
> +
> +static cairo_pattern_t *
> +_meta_pattern_get (cairo_pattern_t *pattern)
> +{
> +    if (pattern->ref_count > 0)
> +	return pattern;
> +    else
> +	return NULL;
> +}

This allows the meta-surface to treat NULL as a legitimate pattern. I
think that makes sense, but this should happen in
_cairo_pattern_init_copy instead, (and the result should be
communicated with pattern->status == CAIRO_STATUS_NULL_POINTER rather
than returning NULL). So the three functions above should disappear I
think.

> +    command->type = CAIRO_COMMAND_SET_CLIP_REGION;
> +    command->region = region;

Oops. The region should be copied here.

> +    if (path) {
> +	status = _cairo_path_fixed_init_copy (&command->path, path);
> +	if (status) {
> +	    free (command);
> +	    return status;
> +	}
> +	command->path_pointer = &command->path;
> +    } else {
> +	command->path_pointer = NULL;
> +    }

Here's another place where allowing NULL as a legitimate object value
could simplify things. It's certainly not a prerequisite for this
patch, but in general, allowing NULL to "init_copy" would work well
for any object that uses status-based error handling. And it is
consistent with allowing NULL to "reference" and "destroy" which I
recently applied to most objects in the implementation.

> +static cairo_int_status_t
> +_cairo_meta_surface_get_extents (void		   *abstract_surface,
> +				 cairo_rectangle_t *rectangle)
> +{
> +    cairo_meta_surface_t *meta = abstract_surface;
> +
> +    /* Currently this is used for getting the extents of the surface
> +     * before calling cairo_paint().  This is the only this that
> +     * requires the meta surface to have an explicit size.  If paint
> +     * was just a backend function, this would not be necessary. */

This seems like a valid argument, and I think I'd like to see
surface->backend->paint land before the current patch. I was really
hoping to see cairo_meta_surface_t land as a size-less surface.

> +    command = malloc (sizeof (cairo_command_fill_path_t));
> +    if (command == NULL)
> +	return CAIRO_STATUS_NO_MEMORY;
> +
> +    command->type = CAIRO_COMMAND_FILL_PATH;
> +    command->operator = operator;
> +    _meta_pattern_init_copy (&command->pattern.base, pattern);
> +    status = _cairo_path_fixed_init_copy (&command->path, path);

The previous functions all copied the arguments before allocating the
command. All of the functions should be consistent one way or the
other.

> +static const cairo_surface_backend_t cairo_meta_surface_backend = {
> +    _cairo_meta_surface_create_similar,
> +    _cairo_meta_surface_finish,
> +    NULL, /* acquire_source_image */
> +    NULL, /* release_source_image */
> +    NULL, /* acquire_dest_image */
> +    NULL, /* release_dest_image */
> +    NULL, /* clone_similar */

I don't know if it would be useful, but it occurs to me that the
acquire functions could be implemented in terms of replaying against
the image backend. No action needed here though, just thinking out
loud.

> +void
> +_cairo_meta_surface_clear (cairo_surface_t *surface)

I don't see where this function would be useful outside of this
file. It seems to be it should work just as well as:

	static void
	_cairo_meta_surface_fini (cairo_meta_surface_t *surface)

No?

> +cairo_int_status_t
> +_cairo_meta_surface_replay (cairo_surface_t			*surface,
> +			    const cairo_surface_backend_t	*backend,
> +			    void				*closure)
> +{

Couldn't closure just as well be of type cairo_surface_t* rather than
void?

> +	case CAIRO_COMMAND_COMPOSITE:
> +	    if (backend->composite == NULL)
> +		break;

Just giving up here doesn't seem like the right thing.

Don't we really want to call into the same logic as
cairo_surface_composite?

The only problem with doing that directly right now is that
cairo_surface_composite currently accepts a single "cairo_surface_t
*dst" which it uses both to lookup dst->backend->composite and as the
destination pointer to pass to that function. Here, however, we have
backend and closure as separate parameters.

So, something would have to change a bit, but it does seem to me that
we would want to call into the same logic.

> +typedef enum {
> +    CAIRO_COMMAND_COMPOSITE,
> +    CAIRO_COMMAND_FILL_RECTANGLES,
> +    CAIRO_COMMAND_COMPOSITE_TRAPEZOIDS,
> +    CAIRO_COMMAND_COPY_PAGE,
> +    CAIRO_COMMAND_SHOW_PAGE,
> +    CAIRO_COMMAND_SET_CLIP_REGION,
> +    CAIRO_COMMAND_INTERSECT_CLIP_PATH,
> +    CAIRO_COMMAND_SHOW_GLYPHS,
> +    CAIRO_COMMAND_FILL_PATH
> +} cairo_command_type_t;

Seeing those names so close together points out some
inconsistencies. We should probably rename "show_glyphs" to
"composite_glyphs" throughout the backend interface, (though that can
be after this patch). And "fill_rectangles" as "composite_rectangles"
as well?

Once again, this is great work and I'm really excited to see it coming
along.

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050621/588a7bd1/attachment.pgp