[cairo] new pattern interface and OpenGL patch

[David Reveman]

On Mon, 2004-03-22 at 21:43 -0500, Carl Worth wrote: 
> I think I know now how I want the pattern API to work. I knew that
> things like cairo_surface_set_repeat where wrong, and I had
> recommended reworking them as cairo_set_pattern_repeat, but now I
> think I would instead like:
> 
> 	cairo_pattern_t *
> 	cairo_pattern_create_for_surface (cairo_surface_t *surface);
> 
> 	cairo_status_t
> 	cairo_pattern_set_extend (cairo_pattern_t *pattern, cairo_extend_t extend);
> 
> 	cairo_status_t
> 	cairo_pattern_set_matrix (cairo_pattern_t *pattern, cairo_matrix_t *matrix);
> 
> 	cairo_status_t
> 	cairo_pattern_get_matrix (cairo_pattern_t *pattern, cairo_matrix_t *matrix);
> 
> 	void
> 	cairo_set_pattern (cairo_t *cr, cairo_pattern_t *pattern);
> 
> Then, in this style, the two functions above might be:
> 
> 	cairo_pattern_t *
> 	cairo_pattern_create_linear (cairo_t *cr,
> 				     double x0, double y0,
> 				     double x1, double y1);
> 
> 	cairo_pattern_t *
> 	cairo_pattern_create_radial (cairo_t *cr,
> 				     double center_x, double center_y,
> 				     double radius_dx, double radius_dy,
> 				     double focal_x, double focal_y);
> 
> What do you think of that?

I like it, 
but shouldn't the
cairo_pattern_create_for_surface (cairo_surface_t *surface);
be
cairo_pattern_create_for_surface (cairo_t *cr, 
                                  cairo_surface_t *surface);
just like the linear and radial ones.
I'm asking because I'm currently transforming all points of the linear
and radial patterns with the CTM when they are created and in the same
way I save the inverse CTM when creating a surface pattern.


> Oh, and PostScript provides a (very) slightly different model for the
> radial pattern parameters. Following that model would give:
> 
> 	cairo_pattern_t *
> 	cairo_pattern_create_radial (cairo_t *cr,
> 				     double cx0, double cy0, double radius0,
> 				     double cx1, double cy1, double radius1);
> 
> Having a single radius rather than separate radius_dx/radius_dy values
> matches the style used by cairo_arc. Also, we need a radius for the
> final point as well, right?

I like this model better. Separate radius_dx/radius_dy values are not
needed, the pattern matrix could be used instead.

>     +void
>     +cairo_add_pattern_color_stop (cairo_t *cr, double offset,
>     +                              double red, double green, double blue,
>     +                              double alpha);
> 
> Continuing the trend:
> 
> 	cairo_status_t
> 	cairo_pattern_add_color_stop (cairo_pattern_t *pattern, double offset,
> 				      double red, double green, double blue,
> 				      double alpha);
> 
> Oh, and up until now, I had been avoiding putting red, green, blue,
> and alpha into a single function call, since it raises the question of
> whether the color is represented with premultiplied alpha or not. So,
> we'll have to answer that question here.
> 
> One option would be to finally cave in a declare a cairo_color_t
> structure. Owen argued in the past that applications are going to want
> that anyway, so we should provide it as a convenience.

ok.

> 
>     +
>     +typedef enum {
>     +  CAIRO_SHADER_LINEAR,
>     +  CAIRO_SHADER_GAUSSIAN,
>     +  CAIRO_SHADER_FUNCTION
>     +} cairo_shader_t;
> 
> Could we get away with just using the same cairo_filter_t type for
> this purpose instead?

yes, the NEAREST filter type would actually be useful for linear and
radial patterns. hmm, would the GAUSSIAN filter type make any sense for
surface patterns? It could mean that the surface should be filter using a
gaussian convolution filter.   

> 
>     +/* component order of colors are RGBA and factor is the distance
>     +   from color stop 1 (0.0 < factor < 1.0). */
>     +typedef void (*cairo_shader_function_t) (unsigned char *color1,
>     +                                         unsigned char *color2,
>     +                                         double factor,
>     +                                         unsigned char *result_color);
>     +
>     +/* shoud we introduce an additional function for setting the
>     +   shader function? */
>     +void
>     +cairo_set_pattern_shader (cairo_t *cr,
>     +                          cairo_shader_t shader,
>     +                          cairo_shader_function_t function);
>     +
>     +cairo_shader_t
>     +cairo_current_pattern_shader (cairo_t *cr,
>     +                              cairo_shader_function_t *function);
> 
> Is it important to have function-based filtering here? I'd fell more
> comfortable not adding this unless there is a compelling reason.

Not to me. You're right, it would be much cleaner if we removed it.
Would allow us to use the cairo_filter_t type as mentioned above.

> 
>     +/* Rectangular extents */
>     +void
>     +cairo_stroke_extents (cairo_t *cr, double *x1, double *y1, double
>     *x2, double *y2);
>     +
>     +void
>     +cairo_fill_extents (cairo_t *cr, double *x1, double *y1, double *x2,
>     double *y2);
> 
> These should be quite handy. Thanks.

I should probably mention it as Thomas Hunger asked me about it.
I'm currently tesselating the path and calculate the extents from the
trapezoids.

Thomas Hunger wrote:
I am not quite sure why you want to determine the extents by tesselating to 
traps first? An interpret_path should do the same thing but in a
fraction of the time since tesselating is quite expensive compared to
interpreting.

My answer:
Yes, you're right tesselation is much more expensive but it's also much
easier to get accurate. Exact regions, independent of stroke width and
such. I thought there might be problems with interpreting when cairo
supports dashed bezier curves and variable stroke width. When it comes
to performance the region calculations for the create_pattern function
are even faster than interpreting as we can calculate the region from
the already tesselated traps. The public API functions are of course a
bit slower as they need to actually tesselate the path on its own (but
we could cache the traps to remove this performance penalty, if
necessary). My tests have also proven that this performance penalty is
almost significant compared to the actual rendering time, even for the
OpenGL backend.


>     +#define CAIRO_GL_OPTION_DOUBLEBUFFER_MASK \
>     +  GLC_FORMAT_OPTION_DOUBLEBUFFER_MASK
>     +#define CAIRO_GL_OPTION_SINGLEBUFFER_MASK \
>     +  GLC_FORMAT_OPTION_SINGLEBUFFER_MASK
>     +#define CAIRO_GL_OPTION_MULTISAMPLE_MASK \
>     +  GLC_FORMAT_OPTION_MULTISAMPLE_MASK
>     +#define CAIRO_GL_OPTION_NO_MULTISAMPLE_MASK \
>     +  GLC_FORMAT_OPTION_NO_MULTISAMPLE_MASK
> 
> Ah, these are the options for the option parameter above? I might
> suggest typedef'ing that int to cairo_gl_option_t so that it would be
> easier to search for these symbolic values within cairo.h.

the 'options' parameter is a bit mask of the above options, so if we should
use a cairo_gl_option_t type for them, we need to replace the 'options' 
parameter with a list of cairo_gl_option_t's or something.  

> 
>     +cairo_status_t
>     +cairo_gl_surface_realize (cairo_surface_t *surface);
> 
> Hmmm... I was a little confused at first when I saw "cairo_gl_surface"
> as opposed to cairo_glx_surface. But I guess what you're doing here is
> providing a function that works for both the GLX and AGL backends.
> 
> Unfortunately, the cairo_surface_t type doesn't provide enough
> information to enforce the association between custom surface
> functions and surface types.
> 
> So, in the absence of good type-checking, I've been trying to at least
> match the function names, eg.:
> 
> 	cairo_surface_t *
> 	cairo_foo_surface_create (...);
> 
> 	cairo_status_t
> 	cairo_foo_surface_bar (cairo_surface_t *surface, ...);
> 
> Where foo is also the name of a backend that can be configured with
> --enable-foo, tested with CAIRO_HAS_FOO_SURFACE, etc.
> 
> So, I don't know what the best thing to do here would be.

We don't actually need to have any cairo_gl_surface* functions in the 
public API. We could remove the realize function or change it into
two functions, a glx one and one agl one.

> 
>     +XVisualInfo *
>     +cairo_glx_find_best_visual_info (Display *dpy,
>     +                                 int screen,
>     +                                 int options);
> 
> What's this function? It seems a bit out of place.

You need the XVisualInfo to create a Window which can be used
with cairo_glx_surface_create_for_window. The 'options' parameter
must be the same for both functions.

I'm sure this can be a bit confusing but it shouldn't be to weird
for you who are familiar with GLX. It's similar to:
glXFindVisualInfo and glXCreateWindow.

> 
>     +cairo_surface_t *
>     +cairo_glx_surface_create_for_window (Display *dpy, /* ... */
> 
> Each of the new surface_create functions look just fine.
> 
>     +typedef struct _cairo_gl_convolution_t {
>     +  double matrix[3][3];
>     +} cairo_gl_convolution_t;
>     +  
>     +cairo_status_t
>     +cairo_gl_surface_set_convolution (cairo_surface_t *surface,
>     +                                  cairo_gl_convolution_t *convolution);
> 
> Hmmm.... would we want a general function to be able to apply
> convolutions to all types of surfaces?

The cairo_filter_t model mentioned earlier might work.

> 
>     +cairo_status_t
>     +cairo_gl_surface_show (cairo_surface_t *surface,
>     +                       int x,
>     +                       int y,
>     +                       unsigned int width,
>     +                       unsigned int height);
>     +
>     +cairo_status_t
>     +cairo_gl_surface_gl_begin (cairo_surface_t *surface);
>     +
>     +cairo_status_t
>     +cairo_gl_surface_gl_end (cairo_surface_t *surface);
> 
> What are the semantics of these functions? I just want to make sure
> that whatever general functionality we need lives in the general
> interface.

surface_show is used for swapping buffers of double buffered surfaces or
flushing drawing operations of single buffered ones. This isn't needed as
we can use copy_page and show_page for this.

gl_begin and gl_end makes it possible to use the OpenGL API
for rendering to a cairo surface. We added it for testing
purposes.

> 
> Phew. Thanks for sticking through all that with me.

well, thank you for taking your time and look at the patches. I'll start
working on the things you've suggested tomorrow.

- David

-- 
David Reveman <c99drn at cs.umu.se>