[cairo] patch for cairo_mask()

[Carl Worth]

On Thu, 28 Apr 2005 22:01:43 -0400, Owen Taylor wrote:
> Here's a patch to implement cairo_mask() and cairo_mask_surface()

Thanks for doing this Owen! Once again, you're preemptively removing
items from my TODO list, so I can't complain. And this did end up
being more work than I expected, so thanks even more.

>  - cairo_paint_[with_]alpha could be done as a simple convenience
>    function in cairo.c on top of cairo_mask(), using a solid
>    pattern.

Excellent. I'm anxious to get that in place, so that we will no longer
have an API hole left from the removal of cairo_set_alpha, (see
cairo-demo/X11/knockout.c which cannot be successfully ported to
current API).

>    (We don't have a public API to create a solid color pattern?)

Yeah, that's something we should probably fix for completeness. If
nothing else, it will make the documentation for the various
cairo_set_source_* functions clear in that they are all convenience
functions which prevent the user from having to create and destroy a
pattern just to set it as the current source.

>  - I made cairo_mask_surface() take surface_x/surface_y, similar
>    to an idea I discussed with Carl recently for 
>    cairo_set_source_surface().

I've had that cairo_set_source, cairo_set_source_surface patch in the
wings for a few days now. I've got all hung up now on trying to decide
some fine details about how libpixman should sample images, (I've long
had off-by-one placement issues in xsvg but I've never know which
layer of the code was in the wrong). I should probably just get the
patch out there along with the tests that demonstrate the sampling
question, (even if I haven't nailed down the right answer).

>  - When generating an intermediate surface for clipping, I currently
>    only constrain the size of the intermediate surface by the
>    clip, not by the size of the mask or source.
[...]
>  - I didn't optimize the case of:
> 
>     - Solid color or gradient pattern
>     - Alpha mask

Neither of those seem sufficient for holding up the patch.

In fact, I don't see much reason to hold the patch up at all. It does
seem to fill the basic requirement, (the new test seems to work fine
to me), and this is new functionality, so even if imperfect, it will
be easier to work with that in CVS.

I do have a few comments below from a brief look at the patch.


> +    if (gstate->surface->level != gstate->surface_level)
> +	return CAIRO_STATUS_BAD_NESTING;
> +    

Oops. This leaked in from an as-of-yet uncommitted patch. Do elide
this to avoid breaking compilability.

>  /**
> + * cairo_mask:
> + * @cr: a cairo context
> + * @pattern: a #cairo_pattern_t
> + *
> + * A drawing operator that paints the current source in the
> + * using the alpha channel of @pattern as a mask. (Opaque
> + * areas of @mask are painted with the source, transparent
> + * areas are not painted.)
> + */

How about:

* A drawing operator that paints the current source
* using the alpha channel of @pattern as a mask. (Opaque
* areas of @pattern are painted with the source, transparent
* areas are not painted.)

> + * A drawing operator that paints the current source in the
> + * using the alpha channel of @surface as a mask. (Opaque

Similarly:

* A drawing operator that paints the current source
 * using the alpha channel of @surface as a mask. (Opaque


> +    /* Some of our drawing is unbounded, so we draw those sections
> +     * to a temporary surface and copy over.
> +     */

That seems like an awful lot of extra work to me. Here's a replacement
body that uses cairo_clip in place of the intermediate surface
shuffling:

static cairo_test_status_t
draw (cairo_t *cr, int width, int height)
{
    int i, j, k;

    for (k = 0; k < ARRAY_SIZE (clip_funcs); k++) {
	for (j = 0; j < ARRAY_SIZE (mask_funcs); j++) {
	    for (i = 0; i < ARRAY_SIZE (pattern_funcs); i++) {
		int  x, y;

		x = i * (WIDTH + PAD) + PAD;
		y = (ARRAY_SIZE (mask_funcs) * k + j) * (HEIGHT + PAD)
		+ PAD;

		/* draw */
		cairo_save (cr);

		cairo_rectangle (cr, x, y, WIDTH, HEIGHT);
		cairo_clip (cr);

		clip_funcs[k] (cr, x, y);
		pattern_funcs[i] (cr, x, y);
		mask_funcs[j] (cr, x, y);

		cairo_restore (cr);
	    }
	}
    }

    return CAIRO_TEST_SUCCESS;
}

Finally, I'd like to also see a smaller test case for cairo_mask
that's easier to read, (ie. no function pointer tables at
least). But, again, no need to wait for that to commit. I'll put one
together after this is in CVS, (I'd like to look at a problem a user
reported in trying to get cairo_scale to affect cairo_mask_surface).

Maybe we want a naming convention to distinguish between these big
somewhat-exhaustive matrix-based tests vs. a more focused tests. I
imagine we'll likely want both flavors for most function calls. Maybe
mask-exhausting.c vs. mask.c?

-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/20050429/b93bd254/attachment.pgp