[cairo] API Shakeup: Making set_source consistent, (renaming cairo_set_rgb_color and cairo_set_pattern, eliminating cairo_set_alpha)

[Owen Taylor]

On Tue, 2005-04-12 at 23:39 -0400, Carl Worth wrote:
> On Mon, 28 Mar 2005 16:02:49 -0500, Carl Worth wrote:
> > On Tue, 15 Feb 2005 19:01:04 -0500, Carl Worth wrote:
> > > 	/* Set the source to be a translucent color everywhere. */
> > > 	void
> > > 	cairo_set_source_rgba (cairo_t *cr,
> > > 			       double r, double g, double b,
> > > 			       double alpha);
> > 
> > So, porting away from cairo_set_alpha might be interesting in a few
> > cases, but I think the improved coherence of the rendering model is
> > worth it.
> 
> Here's a fairly complete patch to implement the cairo_set_source
> changes.
> 
> As discussed above, this eliminates cairo_set_alpha. Calls to
> cairo_set_rgb_color and cairo_set_alpha can easily be replaced with a
> call to cairo_set_source_rgba. However, for things like
> cairo_set_alpha and cairo_show_surface, we don't have a good
> replacement in the API yet.

In general, the changes to the test cases make them look cleaner
and simpler, which is a good sign.

> In spite of this API regression, I want to commit this soon. The
> internal cleanup that results from this patch is really needed for the
> subsequent cairo_paint patch I've been working on. And the cairo_paint
> work is needed for the cairo_paint_alpha function that will provide
> the replacement for cairo_set_alpha/cairo_show_surface.
> 
> So, I think it's about time to start some real backwards-incompatible
> API changes, (which we've been mostly avoiding up until now in the API
> Shakeup).

Sounds good to me. 

> One of the internal changes that happens here is the elimination of
> pattern->alpha. This simplifies the code in several places. I've
> checked things out with the W3C SVG test suite, and I think things are
> pretty good, but I would appreciate some review of the cairo-pattern.c
> changes. In eliminating pattern->alpha I probably eliminated some
> performance optimizations, but we can just put those back into
> cairo_paint_alpha when we get there.

Spent some time looking through the patch - I didn't see any problems
with the cairo-pattern.c changes (or even removed optimizations), other
than the premultiplication problems noted below.

+/* XXX: The calculation of:
+
+               channel * 0xfff
+

0xffff of course

-       _cairo_pattern_init_solid (&pattern.solid, 1.0, 1.0, 1.0);
+       _cairo_color_init_rgba (&opaque_white, 1.0, 1.0, 1.0, 1.0);
+       _cairo_pattern_init_solid (&pattern.solid, &opaque_white);

You used init_rgb() in other similar cases elsewhere.

@@ -401,27 +448,21 @@ _cairo_pattern_get_rgb (cairo_pattern_t
                        double          *green,
                        double          *blue)
 {
-
     if (pattern->type == CAIRO_PATTERN_SOLID)
     {
        cairo_solid_pattern_t *solid = (cairo_solid_pattern_t *)
pattern;

-       *red   = solid->red;
-       *green = solid->green;
-       *blue  = solid->blue;
-    } else
+       *red   = solid->color.red;
+       *green = solid->color.green;
+       *blue  = solid->color.blue;
+    } else {
        *red = *green = *blue = 1.0;
+    }

Hmm, should this be non-premultiplied? But I guess this is used
only by cairo_get_rgb_color() which will be deprecated? (Doesn't
seem to be in your patch)

            if (src->n_stops)
-           {
-               _cairo_pattern_init_solid (&solid,
-                                          src->stops->color.red,
-                                          src->stops->color.green,
-                                          src->stops->color.blue);
-               _cairo_pattern_set_alpha (&solid.base,
-                                         src->stops->color.alpha);
-           }
+               _cairo_color_init_rgba (&color,
+                                       src->stops->color.red,
+                                       src->stops->color.green,
+                                       src->stops->color.blue,
+                                       src->stops->color.alpha);

src->stops->color is premultiplied, right?

+       _cairo_color_get_rgba (&src_solid->color, &red, &green, &blue,
&alpha);
+       alpha *= mask_solid->color.alpha;
+
+       _cairo_color_init_rgba (&combined, red, green, blue, alpha);

The unpremultipication followed by premultiplication is going to give
problems for super-luminescent colors, I think? (if alpha is 0, say)

Perhaps having a _cairo_color_multiply_alpha () would make sense
since this occurs in a couple of places.

In general, I think the number of places where the code does, something
like:

 cairo_color_t opaque_white; 
 _cairo_color_init_rgb (&opaque, 1.0, 1.0, 1.0);
 /* use &opaque_white once */

is ugly. Minimally, _cairo_pattern_init_rgba() could be added, but a
thought I had was to add something along the lines of:

 typedef enum {
   CAIRO_WHITE,
   CAIRO_BLACK,
   CAIRO_TRANSPARENT,
 } cairo_stock_t

 cairo_set_source_stock (cr, CAIRO_WHITE);
 
 _cairo_pattern_init_solid (_cairo_stock_color (CAIRO_WHITE));

The private API could be added without the public API, of course.

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/20050413/666a7c80/attachment.pgp