[cairo] [cairo-commit] src/cairo-pdf-surface.c src/cairo-pdf-surface-private.h src/cairo-recording-surface.c src/cairo-recording-surface-private.h

[Uli Schlachter]

Hi,

I got two comments about this commit:

On 11.09.2013 13:50, Adrian Johnson wrote:
>  src/cairo-pdf-surface-private.h       |    2 
>  src/cairo-pdf-surface.c               |  125 +++++++++++++++++++++++++---------
>  src/cairo-recording-surface-private.h |    8 ++
>  src/cairo-recording-surface.c         |  113 ++++++++++++++++++++++++++++++
>  4 files changed, 215 insertions(+), 33 deletions(-)
> 
> New commits:
> commit 8addb4798c918000eaa6f6dab138e0abb0efa946
> Author: Adrian Johnson <ajohnson at redneon.com>
> Date:   Sun Apr 8 10:57:23 2012 +0930
> 
>     pdf: avoid making groups a transparency group if not required
>     
>     If the group contains only a combination of clear and opaque alpha and
>     only OPERATOR_OVER is used in the group and to paint the group, a
>     transparency group is not required. This allows the pdf viewer to
>     replay the group in place.
> 
[...]
> +static void
> +_cairo_recording_surface_merge_source_attributes (cairo_recording_surface_t  *surface,
> +						  cairo_operator_t            op,
> +						  const cairo_pattern_t      *source)
> +{
> +    if (op != CAIRO_OPERATOR_OVER)
> +	surface->has_only_op_over = FALSE;
> +
> +    if (source->type == CAIRO_PATTERN_TYPE_SURFACE) {
> +	cairo_surface_pattern_t *surf_pat = (cairo_surface_pattern_t *) source;
> +	cairo_surface_t *surf = surf_pat->surface;
> +	cairo_surface_t *free_me;

free_me should be initialized to NULL (gcc warns about an uninitialized variable
being passed to cairo_surface_destroy()).

> +	if (_cairo_surface_is_snapshot (surf))
> +	    free_me = surf = _cairo_surface_snapshot_get_target (surf);
> +
> +	if (surf->type == CAIRO_SURFACE_TYPE_RECORDING) {
> +	    cairo_recording_surface_t *rec_surf = (cairo_recording_surface_t *) surf;
> +
> +	    if (! _cairo_recording_surface_has_only_bilevel_alpha (rec_surf))
> +		surface->has_bilevel_alpha = FALSE;
> +
> +	    if (! _cairo_recording_surface_has_only_op_over (rec_surf))
> +		surface->has_only_op_over = FALSE;
> +
> +	} else if (surf->type == CAIRO_SURFACE_TYPE_IMAGE) {
> +	    cairo_image_surface_t *img_surf = (cairo_image_surface_t *) surf;
> +
> +	    if (_cairo_image_analyze_transparency (img_surf) == CAIRO_IMAGE_HAS_ALPHA)
> +		surface->has_bilevel_alpha = FALSE;
> +
> +	}

This function does nothing if I pass in e.g. a cairo-gl surface with an alpha
channel. I think that the above "if" needs another else case:

 } else {
    if (!_cairo_pattern_is_clear (source)
     && !_cairo_pattern_is_opaque (source, NULL))
	surface->has_bilevel_alpha = FALSE;
 }

(At least these are the most suitable functions that I found for this...)

> +	cairo_surface_destroy (free_me);
> +	return;
> +
> +    } else if (source->type == CAIRO_PATTERN_TYPE_RASTER_SOURCE) {
> +	cairo_surface_t *image;
> +	cairo_surface_t *raster;
> +
> +	image = cairo_image_surface_create (CAIRO_FORMAT_ARGB32, 1, 1);
> +	raster = _cairo_raster_source_pattern_acquire (source, image, NULL);
> +	cairo_surface_destroy (image);
> +	if (raster) {
> +	    if (raster->type == CAIRO_SURFACE_TYPE_IMAGE) {
> +		if (_cairo_image_analyze_transparency ((cairo_image_surface_t *)raster) == CAIRO_IMAGE_HAS_ALPHA)
> +		    surface->has_bilevel_alpha = FALSE;
> +	    }
> +
> +	    _cairo_raster_source_pattern_release (source, raster);
> +	    if (raster->type == CAIRO_SURFACE_TYPE_IMAGE)
> +		return;
> +	}
> +    }
> +
> +    if (!_cairo_pattern_is_clear (source) && !_cairo_pattern_is_opaque (source, NULL))
> +	surface->has_bilevel_alpha = FALSE;
> +}
[...]

Are my observations correct? Should I commit and push these changes? Should a
suitable test be added to the test suite?

Cheers,
Uli
-- 
A normal person is just someone you don't know well enough yet.
 - Nettie Wiebe