[cairo] [PATCH] image: Add public API for the creation of a surface for a pixman_image_t

[Chris Wilson]

On Tue, 20 Mar 2012 15:07:33 +0100, Uli Schlachter <psychon at znc.in> wrote:
> On 20.03.2012 14:12, Chris Wilson wrote:
> > Greater interoperablity with pixman is often requested by users dealing
> > with quirky hardware that prefers niche formats. As the goal is to keep
> > the number of core, well supported cairo_format_t to a minimum, we need
> > an alternative mechanism to support the extensive range of formats
> > supported by pixman. In the future we will also have to look to
> > supporting pixman colorspaces, but for now we only handled RGBA linear
> > compositing and so restrict ourselves to that subset of pixman images.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  src/cairo-image-surface.c      |  126 +++++++++++++++++++++++++++++++++++++++-
> >  src/cairo.h                    |   10 +++
> >  util/cairo-missing/Makefile.am |    2 +-
> >  3 files changed, 134 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/cairo-image-surface.c b/src/cairo-image-surface.c
> > index 2a2d59d..5c682f0 100644
> > --- a/src/cairo-image-surface.c
> > +++ b/src/cairo-image-surface.c
> [...]
> > +cairo_surface_t *
> > +cairo_image_surface_create_for_pixman_image (pixman_format_code_t format,
> > +					     pixman_image_t *image)
> > +{
> > +    int width, height;
> > +    pixman_image_t *copy;
> > +    cairo_surface_t *surface;
> 
> I would suggest adding:
> 
>      if (image == NULL)
>           return _cairo_surface_create_in_error (_cairo_error
> (CAIRO_STATUS_NULL_POINTER));

Done.

> 
> > +    if (! pixman_format_supported_destination (format))
> > +	return _cairo_surface_create_in_error (_cairo_error (CAIRO_STATUS_INVALID_FORMAT));
> > +
> > +    width = pixman_image_get_width (image);
> > +    height = pixman_image_get_width (image);
> > +    if (! _cairo_image_surface_is_size_valid (width, height))
> > +	return _cairo_surface_create_in_error (_cairo_error (CAIRO_STATUS_INVALID_SIZE));
> > +
> > +    copy = pixman_image_create_bits (format, width, height,
> > +				     pixman_image_get_data (image),
> > +				     pixman_image_get_stride (image));
> > +    if (unlikely (copy == NULL))
> > +	return _cairo_surface_create_in_error (_cairo_error (CAIRO_STATUS_NO_MEMORY));
> > +
> > +    surface = _cairo_image_surface_create_for_pixman_image (copy, format);
> > +    if (unlikely (surface->status))
> > +	return surface;
> > +
> > +    pixman_image_set_destroy_function (image,
> > +				       _pixman_image_destroy_notify,
> > +				       surface);
> > +    return surface;
> > +}
> [...]
> > + * Return value: the format code of the image, along with the pointer to
> > + * the pixman image, or 0 if @surface is not an image surface, or if
> [...]
> > + **/
> > +pixman_format_code_t
> > +cairo_image_surface_get_pixman_image (cairo_surface_t *surface,
> > +				      pixman_image_t *image)
> 
> Shouldn't this be "pixman_image_t **image"? Else...
> 
> > +{
> > +    cairo_image_surface_t *image_surface = (cairo_image_surface_t *) surface;
> > +
> > +    if (! _cairo_surface_is_image (surface)) {
> > +	_cairo_error_throw (CAIRO_STATUS_SURFACE_TYPE_MISMATCH);
> > +	return 0;
> > +    }
> > +
> > +    if (surface->finished) {
> > +	_cairo_error_throw (CAIRO_STATUS_SURFACE_FINISHED);
> > +	return 0;
> > +    }
> > +
> > +    *image = image_surface->pixman_image;
> 
> ...this line casts a pixman_image_t* to a pixman_image_t and assigns the result
> to *image. (Does this compile? I assume so, but that makes me wonder why it
> compiles)

Last minute bug to keep the reader busy ;-)

> 
> > +    return image_surface->pixman_format;
> > +}
> > +cairo_format_t
> >  _cairo_format_from_content (cairo_content_t content)
> >  {
> >      switch (content) {
> > diff --git a/src/cairo.h b/src/cairo.h
> > index d23cd10..bb93336 100644
> > --- a/src/cairo.h
> > +++ b/src/cairo.h
> > @@ -42,6 +42,8 @@
> >  #include "cairo-features.h"
> >  #include "cairo-deprecated.h"
> >  
> > +#include <pixman.h>
> 
> This is bad. It means that cairo.h now depends on pixman.h which it didn't do
> before. If you really want to do that, you should add "pixman" to src/cairo.pc.
> Alternatively, we would have to add a cairo-image.h for this which gets its own
> pkg-config file, but I guess that is ugly to use.

Right, I contemplated cairo-image.h but it seems consistent that all the
builtin surfaces are available just through cairo.h. Doing otherwise
would be an annoying API break. cairo.pc should already be pulling in
pixman.pc, but we do so through Requires.private and now that needs to
be Requires. Hmm, probably easiest/safest to do that explicitly.

> 
> >  #ifdef  __cplusplus
> >  # define CAIRO_BEGIN_DECLS  extern "C" {
> >  # define CAIRO_END_DECLS    }
> > @@ -2439,6 +2441,14 @@ cairo_image_surface_create_for_data (unsigned char	       *data,
> >  				     int			height,
> >  				     int			stride);
> >  
> > +cairo_public cairo_surface_t *
> > +cairo_image_surface_create_for_pixman_image (pixman_format_code_t format,
> > +					     pixman_image_t *image);
> > +
> > +cairo_public pixman_format_code_t
> > +cairo_image_surface_get_pixman_image (cairo_surface_t *surface,
> > +				      pixman_image_t *image);
> > +
> >  cairo_public unsigned char *
> >  cairo_image_surface_get_data (cairo_surface_t *surface);
> >  
> > diff --git a/util/cairo-missing/Makefile.am b/util/cairo-missing/Makefile.am
> > index 9078610..d252b74 100644
> > --- a/util/cairo-missing/Makefile.am
> > +++ b/util/cairo-missing/Makefile.am
> > @@ -1,6 +1,6 @@
> >  include $(top_srcdir)/util/cairo-missing/Makefile.sources
> >  
> > -AM_CPPFLAGS = -I$(top_srcdir)/src -I$(top_builddir)/src
> > +AM_CPPFLAGS = -I$(top_srcdir)/src -I$(top_builddir)/src $(CAIRO_CFLAGS)
> 
> I assume this fixes a "pixman.h not found" error and thus is just a band-aid for
> the above problem?

It's an outright compilation failure as cairo.h now includes pixman.h
and so we need to pull in the relevant include path which is in
CAIRO_CFLAGS.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre