[cairo] [PATCH] V3 image: xlib/xcb backends use filtering to match image backend
Bryce Harrington
bryce at osg.samsung.com
Fri Oct 3 13:14:51 PDT 2014
On Fri, Sep 26, 2014 at 06:05:45PM -0700, Bill Spitzak wrote:
> Changes from previous version:
>
> Minor code cleanup
> Added PIXMAN_HAS_GOOD_BEST #define in case code gets into pixman
> For all backends a scale of .5 and integer translate uses bilinear filter
>
> The xlib and xcb backends will use the image fallback if necessary
> to get correct filtering of transformed surfaces. This should
> produce output that matches the image backend.
>
> - Added capability flags:
> CAIRO_RENDER_HAS_FILTER_GOOD
> CAIRO_RENDER_HAS_FILTER_BEST
> CAIRO_XCB_RENDER_HAS_FILTER_GOOD
> CAIRO_XCB_RENDER_HAS_FILTER_BEST
> PIXMAN_HAS_GOOD_BEST
> These indicate if various backends support GOOD/BEST filtering.
> Currently nothing turns these on.
>
> - _cairo_pattern_analyze_filter() changes GOOD to BILINEAR if if thinks
> biliniear is acceptable. This allows all backends to reuse the logic and fits
"bilinear"
> in with the existing changes to NEAREST for integer translate. This does
> assume that BILINEAR is identical to GOOD on for these scales on all backends,
> this appears to be true as all other backends ignore the filter type.
>
> - Removed the equivalent test from _pixman_image_set_properties.
>
> - xlib backend pattern_is_supported changed to return false if GOOD or
> BEST is requested and the backend does not support it.
>
> - xcb backend _pattern_is_supported changed to return false if GOOD or
> BEST is requested and the backend does not support it.
Since we're down to the wire on 1.14.0, I'm going to hold this patch for
1.4.1 since there's a few things I have questions on, and it looks like
there's some areas where this needs further development. Also, this
includes some behavioral changes, which I assume are good but perhaps
need some more discussion?
You might consider breaking this patch into several patches, since this
is making several conceptually distinct changes.
> ---
> src/cairo-image-source.c | 38 ++++++++++++++++++++++++--------------
> src/cairo-pattern.c | 36 +++++++++++++++++++++++++++++++++++-
> src/cairo-xcb-connection.c | 10 ++++++++++
> src/cairo-xcb-private.h | 6 +++++-
> src/cairo-xcb-surface-render.c | 34 ++++++++++++++++------------------
> src/cairo-xlib-private.h | 2 ++
> src/cairo-xlib-source.c | 23 ++++++++++++++---------
> 7 files changed, 106 insertions(+), 43 deletions(-)
>
> diff --git a/src/cairo-image-source.c b/src/cairo-image-source.c
> index b6b6b9f..c3f77ad 100644
> --- a/src/cairo-image-source.c
> +++ b/src/cairo-image-source.c
> @@ -527,7 +527,9 @@ _pixel_to_solid (cairo_image_surface_t *image, int x, int y)
> }
>
> /* ========================================================================== */
> +#define PIXMAN_HAS_GOOD_BEST 0 /* move this to config.h! */
Until we have config rules supporting this, the added defines below
won't actually do anything. This is probably handy for testing during
development, but I'd rather wait until it's actually hooked up.
> +#if ! PIXMAN_HAS_GOOD_BEST
> /* Index into filter table */
> typedef enum
> {
> @@ -885,6 +887,7 @@ create_separable_convolution (int *n_values,
>
> return params;
> }
> +#endif /* ! PIXMAN_HAS_GOOD_BEST */
>
> /* ========================================================================== */
>
> @@ -917,6 +920,7 @@ _pixman_image_set_properties (pixman_image_t *pixman_image,
> else
> {
> pixman_filter_t pixman_filter;
> +#if ! PIXMAN_HAS_GOOD_BEST
> kernel_t kernel;
> double dx, dy;
>
> @@ -935,27 +939,31 @@ _pixman_image_set_properties (pixman_image_t *pixman_image,
> */
> if (! (dx < 0x7FFF)) dx = 0x7FFF;
> if (! (dy < 0x7FFF)) dy = 0x7FFF;
> -
> +#endif
> switch (pattern->filter) {
> case CAIRO_FILTER_FAST:
> pixman_filter = PIXMAN_FILTER_FAST;
> break;
> +#if PIXMAN_HAS_GOOD_BEST
> case CAIRO_FILTER_GOOD:
> pixman_filter = PIXMAN_FILTER_GOOD;
> - if (dx > 1.35 || dy > 1.35) {
> - pixman_filter = PIXMAN_FILTER_SEPARABLE_CONVOLUTION;
> - kernel = KERNEL_BOX;
> - /* Clip the filter size to prevent extreme slowness. This
> - value could be raised if 2-pass filtering is done */
> - if (dx > 16.0) dx = 16.0;
> - /* Match the bilinear filter for dimension scaling up: */
> - else if (dx < 1.0) dx = 1.0;
> - if (dy > 16.0) dy = 16.0;
> - else if (dy < 1.0) dy = 1.0;
> - }
> break;
> case CAIRO_FILTER_BEST:
> pixman_filter = PIXMAN_FILTER_BEST;
> + break;
> +#else
> + case CAIRO_FILTER_GOOD:
> + pixman_filter = PIXMAN_FILTER_SEPARABLE_CONVOLUTION;
> + kernel = KERNEL_BOX;
> + /* Clip the filter size to prevent extreme slowness. This
> + value could be raised if 2-pass filtering is done */
> + if (dx > 16.0) dx = 16.0;
> + if (dy > 16.0) dy = 16.0;
> + /* Match the bilinear filter for scales > .75: */
> + if (dx < 1.35) dx = 1.0;
> + if (dy < 1.35) dy = 1.0;
This should probably be mentioned more explicitly in the changelog that
you're changing the bilinear filtering for scales > 75% rather than 100%
as before. (And maybe this should be broken out as its own patch, to
ease future testing/cherrypicking/reverting needs.)
Also, you're using 1.35 here and a few other places as a constant for
representing a .75 scale. I assume this is 1/75, but that's actually
1.3333, so I'm curious why you picked 1.35 specifically? If it is just
representing 1/75, then might the code be clearer to test against
1./75.?
> + break;
> + case CAIRO_FILTER_BEST:
> pixman_filter = PIXMAN_FILTER_SEPARABLE_CONVOLUTION;
> kernel = KERNEL_CATMULL_ROM; /* LANCZOS3 is better but not much */
> /* Clip the filter size to prevent extreme slowness. This
> @@ -974,6 +982,7 @@ _pixman_image_set_properties (pixman_image_t *pixman_image,
> else dy = 1.0;
> }
> break;
> +#endif /* PIXMAN_HAS_GOOD_BEST */
> case CAIRO_FILTER_NEAREST:
> pixman_filter = PIXMAN_FILTER_NEAREST;
> break;
> @@ -990,6 +999,7 @@ _pixman_image_set_properties (pixman_image_t *pixman_image,
> pixman_filter = PIXMAN_FILTER_BEST;
> }
>
> +#if ! PIXMAN_HAS_GOOD_BEST
> if (pixman_filter == PIXMAN_FILTER_SEPARABLE_CONVOLUTION) {
> int n_params;
> pixman_fixed_t *params;
> @@ -998,9 +1008,9 @@ _pixman_image_set_properties (pixman_image_t *pixman_image,
> pixman_image_set_filter (pixman_image, pixman_filter,
> params, n_params);
> free (params);
> - } else {
> + } else
> +#endif
> pixman_image_set_filter (pixman_image, pixman_filter, NULL, 0);
> - }
> }
>
> {
> diff --git a/src/cairo-pattern.c b/src/cairo-pattern.c
> index e6fdae6..4916ab2 100644
> --- a/src/cairo-pattern.c
> +++ b/src/cairo-pattern.c
> @@ -3339,6 +3339,26 @@ _cairo_pattern_is_clear (const cairo_pattern_t *abstract_pattern)
> }
>
> /**
> + * Will given row of back-translation matrix work with bilinear scale?
> + * This is true for scales larger than 1. Also it was judged acceptable
> + * for scales larger than .75. And if there is integer translation
> + * then a scale of exactly .5 works.
Maybe elaborate on how it was judged? Otherwise it sounds like it was
set arbitrarily.
> + */
> +static int
> +use_bilinear(double x, double y, double t)
> +{
> + /* This is the inverse matrix! */
> + double h = x*x + y*y;
> + if (h < 1.35 * 1.35)
> + return TRUE; /* scale > .75 */
> + if ((h > 3.99 && h < 4.01) /* scale is 1/2 */
> + && !_cairo_fixed_from_double(x*y) /* parallel to an axis */
> + && _cairo_fixed_is_integer (_cairo_fixed_from_double (t)))
> + return TRUE;
> + return FALSE;
> +}
> +
> +/**
> * _cairo_pattern_analyze_filter:
> * @pattern: surface pattern
> * @pad_out: location to store necessary padding in the source image, or %NULL
> @@ -3378,7 +3398,21 @@ _cairo_pattern_analyze_filter (const cairo_pattern_t *pattern,
> * more would be defensive...
> */
> pad = 0.5;
> - optimized_filter = pattern->filter;
> + /* Use BILINEAR for any scale greater than .75 instead
> + * of GOOD. For scales of 1 and larger this is identical,
> + * for the smaller sizes it was judged that the artifacts
> + * were not worse than the artifacts from a box filer.
> + * BILINEAR can also be used if the scale is exactly .5
> + * and the translation in that direction is an integer.
> + */
> + if (pattern->filter == CAIRO_FILTER_GOOD &&
> + use_bilinear (pattern->matrix.xx, pattern->matrix.xy,
> + pattern->matrix.x0) &&
> + use_bilinear (pattern->matrix.yx, pattern->matrix.yy,
> + pattern->matrix.y0))
> + optimized_filter = CAIRO_FILTER_BILINEAR;
> + else
> + optimized_filter = pattern->filter;
> }
> break;
>
> diff --git a/src/cairo-xcb-connection.c b/src/cairo-xcb-connection.c
> index b48add1..2d51e14 100644
> --- a/src/cairo-xcb-connection.c
> +++ b/src/cairo-xcb-connection.c
> @@ -77,6 +77,8 @@ typedef struct _cairo_xcb_xid {
>
> #define XCB_RENDER_HAS_PICTURE_TRANSFORM(surface) XCB_RENDER_AT_LEAST((surface), 0, 6)
> #define XCB_RENDER_HAS_FILTERS(surface) XCB_RENDER_AT_LEAST((surface), 0, 6)
> +#define XCB_RENDER_HAS_FILTER_GOOD(surface) FALSE
> +#define XCB_RENDER_HAS_FILTER_BEST(surface) FALSE
I'm gathering these are just stubs and would need further work to hook
up with pixman if/when it includes the necessary functionality?
> #define XCB_RENDER_HAS_EXTENDED_REPEAT(surface) XCB_RENDER_AT_LEAST((surface), 0, 10)
> #define XCB_RENDER_HAS_GRADIENTS(surface) XCB_RENDER_AT_LEAST((surface), 0, 10)
> @@ -390,6 +392,12 @@ _cairo_xcb_connection_query_render (cairo_xcb_connection_t *connection)
> if (XCB_RENDER_HAS_FILTERS (version))
> connection->flags |= CAIRO_XCB_RENDER_HAS_FILTERS;
>
> + if (XCB_RENDER_HAS_FILTER_GOOD (version))
> + connection->flags |= CAIRO_XCB_RENDER_HAS_FILTER_GOOD;
> +
> + if (XCB_RENDER_HAS_FILTER_BEST (version))
> + connection->flags |= CAIRO_XCB_RENDER_HAS_FILTER_BEST;
> +
> if (XCB_RENDER_HAS_PDF_OPERATORS (version))
> connection->flags |= CAIRO_XCB_RENDER_HAS_PDF_OPERATORS;
>
> @@ -882,6 +890,8 @@ cairo_xcb_device_debug_cap_xrender_version (cairo_device_t *device,
> CAIRO_XCB_RENDER_HAS_COMPOSITE_TRAPEZOIDS |
> CAIRO_XCB_RENDER_HAS_PICTURE_TRANSFORM |
> CAIRO_XCB_RENDER_HAS_FILTERS |
> + CAIRO_XCB_RENDER_HAS_FILTER_GOOD |
> + CAIRO_XCB_RENDER_HAS_FILTER_BEST |
> CAIRO_XCB_RENDER_HAS_PDF_OPERATORS |
> CAIRO_XCB_RENDER_HAS_EXTENDED_REPEAT |
> CAIRO_XCB_RENDER_HAS_GRADIENTS);
> diff --git a/src/cairo-xcb-private.h b/src/cairo-xcb-private.h
> index 2610458..1e1d1ee 100644
> --- a/src/cairo-xcb-private.h
> +++ b/src/cairo-xcb-private.h
> @@ -259,6 +259,8 @@ enum {
> CAIRO_XCB_RENDER_HAS_PDF_OPERATORS = 0x0080,
> CAIRO_XCB_RENDER_HAS_EXTENDED_REPEAT = 0x0100,
> CAIRO_XCB_RENDER_HAS_GRADIENTS = 0x0200,
> + CAIRO_XCB_RENDER_HAS_FILTER_GOOD = 0x0400,
> + CAIRO_XCB_RENDER_HAS_FILTER_BEST = 0x0800,
>
> CAIRO_XCB_HAS_SHM = 0x80000000,
>
> @@ -271,7 +273,9 @@ enum {
> CAIRO_XCB_RENDER_HAS_FILTERS |
> CAIRO_XCB_RENDER_HAS_PDF_OPERATORS |
> CAIRO_XCB_RENDER_HAS_EXTENDED_REPEAT |
> - CAIRO_XCB_RENDER_HAS_GRADIENTS,
> + CAIRO_XCB_RENDER_HAS_GRADIENTS |
> + CAIRO_XCB_RENDER_HAS_FILTER_GOOD |
> + CAIRO_XCB_RENDER_HAS_FILTER_BEST,
> CAIRO_XCB_SHM_MASK = CAIRO_XCB_HAS_SHM
> };
>
> diff --git a/src/cairo-xcb-surface-render.c b/src/cairo-xcb-surface-render.c
> index edfa34c..21a89cc 100644
> --- a/src/cairo-xcb-surface-render.c
> +++ b/src/cairo-xcb-surface-render.c
> @@ -394,11 +394,6 @@ _pattern_is_supported (uint32_t flags,
> if (pattern->type == CAIRO_PATTERN_TYPE_SOLID)
> return TRUE;
>
> - if (! _cairo_matrix_is_integer_translation (&pattern->matrix, NULL, NULL)) {
> - if ((flags & CAIRO_XCB_RENDER_HAS_PICTURE_TRANSFORM) == 0)
> - return FALSE;
> - }
> -
> switch (pattern->extend) {
> default:
> ASSERT_NOT_REACHED;
> @@ -412,18 +407,22 @@ _pattern_is_supported (uint32_t flags,
> }
>
> if (pattern->type == CAIRO_PATTERN_TYPE_SURFACE) {
> - cairo_filter_t filter;
> -
> - filter = pattern->filter;
> - if (_cairo_matrix_is_pixel_exact (&pattern->matrix))
> - {
> - filter = CAIRO_FILTER_NEAREST;
> - }
> -
> - if (! (filter == CAIRO_FILTER_NEAREST || filter == CAIRO_FILTER_FAST)) {
> - if ((flags & CAIRO_XCB_RENDER_HAS_FILTERS) == 0)
> - return FALSE;
> + switch (pattern->filter) {
> + case CAIRO_FILTER_FAST:
> + case CAIRO_FILTER_NEAREST:
> + return (flags & CAIRO_XCB_RENDER_HAS_PICTURE_TRANSFORM) ||
> + _cairo_matrix_is_integer_translation (&pattern->matrix, NULL, NULL);
> + case CAIRO_FILTER_GOOD:
> + return flags & CAIRO_XCB_RENDER_HAS_FILTER_GOOD;
> + case CAIRO_FILTER_BEST:
> + return flags & CAIRO_XCB_RENDER_HAS_FILTER_BEST;
> + case CAIRO_FILTER_BILINEAR:
> + case CAIRO_FILTER_GAUSSIAN:
> + default:
> + return flags & CAIRO_XCB_RENDER_HAS_FILTERS;
Could you explain the above refactoring? I don't grok how you get to
the new code from the old.
> }
> + } else if (pattern->type == CAIRO_PATTERN_TYPE_MESH) {
> + return FALSE;
> } else { /* gradient */
> if ((flags & CAIRO_XCB_RENDER_HAS_GRADIENTS) == 0)
> return FALSE;
> @@ -435,9 +434,8 @@ _pattern_is_supported (uint32_t flags,
> {
> return FALSE;
> }
> + return TRUE;
> }
> -
> - return pattern->type != CAIRO_PATTERN_TYPE_MESH;
> }
>
> static void
> diff --git a/src/cairo-xlib-private.h b/src/cairo-xlib-private.h
> index 822c85b..6d37896 100644
> --- a/src/cairo-xlib-private.h
> +++ b/src/cairo-xlib-private.h
> @@ -373,6 +373,8 @@ _cairo_xlib_font_close (cairo_xlib_font_t *font);
>
> #define CAIRO_RENDER_HAS_PICTURE_TRANSFORM(surface) CAIRO_RENDER_AT_LEAST((surface), 0, 6)
> #define CAIRO_RENDER_HAS_FILTERS(surface) CAIRO_RENDER_AT_LEAST((surface), 0, 6)
> +#define CAIRO_RENDER_HAS_FILTER_GOOD(surface) FALSE
> +#define CAIRO_RENDER_HAS_FILTER_BEST(surface) FALSE
>
> #define CAIRO_RENDER_HAS_EXTENDED_REPEAT(surface) CAIRO_RENDER_AT_LEAST((surface), 0, 10)
> #define CAIRO_RENDER_HAS_GRADIENTS(surface) CAIRO_RENDER_AT_LEAST((surface), 0, 10)
> diff --git a/src/cairo-xlib-source.c b/src/cairo-xlib-source.c
> index 8275da3..81cc028 100644
> --- a/src/cairo-xlib-source.c
> +++ b/src/cairo-xlib-source.c
> @@ -1093,17 +1093,22 @@ pattern_is_supported (cairo_xlib_display_t *display,
> return FALSE;
> }
>
> - if (! CAIRO_RENDER_HAS_PICTURE_TRANSFORM (display)) {
> - if (!_cairo_matrix_is_integer_translation (&pattern->matrix, NULL, NULL))
> - return FALSE;
> - }
> -
> - if (! CAIRO_RENDER_HAS_FILTERS (display)) {
> - /* No filters implies no transforms, so we optimise away BILINEAR */
> + switch (pattern->filter) {
> + case CAIRO_FILTER_FAST:
> + case CAIRO_FILTER_NEAREST:
> + return CAIRO_RENDER_HAS_PICTURE_TRANSFORM (display) ||
> + _cairo_matrix_is_integer_translation (&pattern->matrix, NULL, NULL);
> + case CAIRO_FILTER_GOOD:
> + return CAIRO_RENDER_HAS_FILTER_GOOD (display);
> + case CAIRO_FILTER_BEST:
> + return CAIRO_RENDER_HAS_FILTER_BEST (display);
> + case CAIRO_FILTER_BILINEAR:
> + case CAIRO_FILTER_GAUSSIAN:
> + default:
> + return CAIRO_RENDER_HAS_FILTERS (display);
> }
> -
> - return TRUE;
> }
> +
> cairo_surface_t *
> _cairo_xlib_source_create_for_pattern (cairo_surface_t *_dst,
> const cairo_pattern_t *pattern,
> --
> 1.7.9.5
>
> --
> cairo mailing list
> cairo at cairographics.org
> http://lists.cairographics.org/mailman/listinfo/cairo
Bryce
More information about the cairo
mailing list