[cairo] patch for cairo_mask()

[Owen Taylor]

On Fri, 2005-04-29 at 21:42 -0400, Carl Worth wrote:

> 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).

That one's not a problem, actually. I did the port of the version
in GTK+ (gtk+/tests/testcairo.c). There are other cases, certainly
where it is needed.

> >    (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 think it generally makes things a lot more consistent, and people
might want to pass around a cairo_pattern_t generically in some 
cases.

> >  - 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).

They do seem like two separate issues.

[...]

> 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.

I'll make sure I break this patch into a separate tree and compile
it by itself before committing.

> >  /**
> > + * 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.)

Yes, "in the" is just a stray here.

[...]

> > +    /* 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;
> }

The point of not just using a clip region is to test the code path
where there is no clipping at all and the composite is bounded by
surface.get_clip_extents(). The difference between that code path
and the clip-region case isn't huge with the current code, but I
could imagine more divergence in the future.

I can drop back to a somewhat simpler version I had before that 
*always* drew to the second surface. It tests almost as much as
the version in my last patch, but is considerably easier to read.

> 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).

Both in cairo-gstate.c and in the backends, operations like mask()
or fill() tend to wander down a spaghetti trail of different code
paths. Something like

 cairo_mask (cr, mask);

is really a shorthand for lots of different operations. All that
a simple test case would test would be cairo.c:cairo_mask() and
*one* of those code paths. I'm not sure which one should be singled
out. 

> 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?

What if we had some helper for doing matrix (or cube) tests? I think
they will generally tend to fall into the category of 

 [ different types of source ] x 
 [ different types of shape ] x
 [ different types of clip ]

Like this one, and being specific to that might make things more
readable, but you could just have:

static void (*pattern_funcs[])(cairo_t *cr) = {
    set_solid_pattern,
    set_translucent_pattern,
    set_gradient_pattern,
    set_image_pattern,
};

[...]

static cairo_test_status_t
draw (cairo_t *cr, int width, int height,
        int i, int j, int k)
{
  clip_funcs[k] (cr2);
  pattern_funcs[i] (cr2);
  mask_funcs[j] (cr2);

  return CAIRO_TEST_SUCCESS;
}

int
main (void)
{
    return cairo_test_3D (&test, draw, 
                          N_ELEMENTS (pattern_funcs),
                          N_ELEMENTS (mask_funcs),
                          N_ELEMENTS (clip_funcs));
}

Which even if it keeps the function pointers seems pretty readable.

I've generally found these tests pretty easy to work with ...
they are easy to extend, and easy to pare down if needed for 
debugging.

Regards,
					Owen


-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050430/e0266cdd/attachment.pgp