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

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 <[email protected]>
> ---
>  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));

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

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

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

Uli
-- 
"Do you know that books smell like nutmeg or some spice from a foreign land?"
                                                  -- Faber in Fahrenheit 451
--
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo