Re: [PATCH] Revert "image: Use convolution filters for sample reconstruction when downscaling"

On Tue, May 13, 2014 at 10:15:12AM +0200, Uli Schlachter wrote:
> This reverts commit fb57ea13e04d82866cbc8e86c83261148bb3e231.

Applied to trunk.
 
> When running cairo-test-suite with the parameter "-a", it also runs each test
> with a non-zero device-offset and device-scaling. The above commit influenced
> the device-scaling results badly. E.g. some test results ended up with a black
> border at the top-most and left-most row that looked like there was an offset of
> "0.5" in drawing the image and thus pixels outside of the image were sampled.
> 
> This can be seen by the influence that this revert has on the results from
> running CAIRO_TEST_TARGET=image ./cairo-test-suite -a:
> 
> Before: 31 Passed, 489 Failed [1 crashed, 8 expected], 31 Skipped
> After: 225 Passed, 295 Failed [1 crashed, 8 expected], 31 Skipped
> 
> Most of the failures that disappeared are from the device-scaling tests.
> 
> With such disastrous results on the test suite, this cannot really be usable for
> real-world applications.
> ---
>  src/cairo-image-source.c | 65 ++++++++----------------------------------------
>  1 file changed, 10 insertions(+), 55 deletions(-)
> 
> The idea here is to get master into a releasable state. Also it appears to me
> that the discussion around this patch stopped without any helpful results.
> 
> What do others (especially Krzysztof) think? What else blocks a new release?
> Who wants to make sure the NEWS entry for this doesn't end up in a release? :)
> 
> 
> diff --git a/src/cairo-image-source.c b/src/cairo-image-source.c
> index 661bc10..c5bd228 100644
> --- a/src/cairo-image-source.c
> +++ b/src/cairo-image-source.c
> @@ -554,42 +554,24 @@ _pixman_image_set_properties (pixman_image_t *pixman_image,
>      }
>      else
>      {
> -	double scale_x, scale_y;
> -	int shrink_x, shrink_y;
>  	pixman_filter_t pixman_filter;
> -	pixman_kernel_t pixman_kernel_sample, pixman_kernel_reconstruct;
> -
> -	/* Compute scale factors as the length of basis vectors transformed by
> -	 * the pattern matrix. These scale factors are from user to pattern space,
> -	 * and as such they are greater than 1.0 for downscaling and less than 1.0
> -	 * for upscaling.
> -	 * TODO: this approach may not be completely correct if the matrix
> -	 * contains a skew component. */
> -	scale_x = hypot (pattern->matrix.xx, pattern->matrix.yx);
> -	scale_y = hypot (pattern->matrix.yx, pattern->matrix.yy);
> -
> -	/* Use convolution filtering if the transformation shrinks the image
> -	 * by more than half a pixel */
> -	shrink_x = (extents->width / scale_x - extents->width) < -0.5;
> -	shrink_y = (extents->height / scale_y - extents->height) < -0.5;
>  
>  	switch (pattern->filter) {
>  	case CAIRO_FILTER_FAST:
> +	    pixman_filter = PIXMAN_FILTER_FAST;
> +	    break;
> +	case CAIRO_FILTER_GOOD:
> +	    pixman_filter = PIXMAN_FILTER_GOOD;
> +	    break;
> +	case CAIRO_FILTER_BEST:
> +	    pixman_filter = PIXMAN_FILTER_BEST;
> +	    break;
>  	case CAIRO_FILTER_NEAREST:
>  	    pixman_filter = PIXMAN_FILTER_NEAREST;
> -	    pixman_kernel_sample = PIXMAN_KERNEL_IMPULSE;
> -	    pixman_kernel_reconstruct = PIXMAN_KERNEL_BOX;
>  	    break;
> -	case CAIRO_FILTER_GOOD:
>  	case CAIRO_FILTER_BILINEAR:
>  	    pixman_filter = PIXMAN_FILTER_BILINEAR;
> -	    pixman_kernel_sample = PIXMAN_KERNEL_BOX;
> -	    pixman_kernel_reconstruct = PIXMAN_KERNEL_LINEAR;
>  	    break;
> -	case CAIRO_FILTER_BEST:
> -	    pixman_filter = PIXMAN_FILTER_BEST;
> -	    pixman_kernel_sample = PIXMAN_KERNEL_LANCZOS3;
> -	    pixman_kernel_reconstruct = PIXMAN_KERNEL_LANCZOS3;
>  	case CAIRO_FILTER_GAUSSIAN:
>  	    /* XXX: The GAUSSIAN value has no implementation in cairo
>  	     * whatsoever, so it was really a mistake to have it in the
> @@ -597,37 +579,10 @@ _pixman_image_set_properties (pixman_image_t *pixman_image,
>  	     * else inventing semantics and providing an actual
>  	     * implementation for it. */
>  	default:
> -	    pixman_filter = PIXMAN_FILTER_BILINEAR;
> -	    pixman_kernel_sample = PIXMAN_KERNEL_BOX;
> -	    pixman_kernel_reconstruct = PIXMAN_KERNEL_LINEAR;
> +	    pixman_filter = PIXMAN_FILTER_BEST;
>  	}
>  
> -	if (pixman_filter != PIXMAN_FILTER_NEAREST && (shrink_x || shrink_y)) {
> -	    pixman_kernel_t sampling_kernel_x, sampling_kernel_y;
> -	    int n_params;
> -	    pixman_fixed_t *params;
> -
> -	    sampling_kernel_x = shrink_x ? pixman_kernel_sample : PIXMAN_KERNEL_IMPULSE;
> -	    sampling_kernel_y = shrink_y ? pixman_kernel_sample : PIXMAN_KERNEL_IMPULSE;
> -
> -	    n_params = 0;
> -	    params = pixman_filter_create_separable_convolution (&n_params,
> -								 scale_x * 65536.0 + 0.5,
> -								 scale_y * 65536.0 + 0.5,
> -								 pixman_kernel_reconstruct,
> -								 pixman_kernel_reconstruct,
> -								 sampling_kernel_x,
> -								 sampling_kernel_y,
> -								 1, 1);
> -
> -	    pixman_image_set_filter (pixman_image,
> -				     PIXMAN_FILTER_SEPARABLE_CONVOLUTION,
> -				     params, n_params);
> -
> -	    free (params);
> -	} else {
> -	    pixman_image_set_filter (pixman_image, pixman_filter, NULL, 0);
> -	}
> +	pixman_image_set_filter (pixman_image, pixman_filter, NULL, 0);
>      }
>  
>      {
> -- 
> 2.0.0.rc0
> 
> -- 
> cairo mailing list
> [email protected]
> http://lists.cairographics.org/mailman/listinfo/cairo
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo