[cairo] [PATCH] Revert "image: Use convolution filters for sample reconstruction when downscaling"
Bryce W. Harrington
b.harrington at samsung.com
Tue May 13 14:39:31 PDT 2014
On Tue, May 13, 2014 at 11:09:26AM +0200, Uli Schlachter wrote:
> Small amendment to the commit message:
>
> On 13.05.2014 10:15, Uli Schlachter wrote:
> > This reverts commit fb57ea13e04d82866cbc8e86c83261148bb3e231.
> >
> > 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.
>
> More reasons can be found in the mailing list thread titled "Concerns about
> using filters for downscaling":
>
> http://lists.cairographics.org/archives/cairo/2014-March/025104.html
It sounded like some consensus was building in that thread, and I've
been holding off committing anything in anticipation of that.
The patch we currently have in trunk is erroneous; Krzysztof posted a
couple followup patches that should fix issues, but I've not replicated
the improvements using those patches in my own test runs, which is why I
haven't committed them.
I'd hate to see the patch reverted since it's an important one for
Inkscape, but clearly not every t is crossed and i dotted yet.
I look forward to hearing other people's thoughts on what we should do,
both near term and long.
Bryce
> > ---
> > 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);
> > }
> >
> > {
> >
>
>
> --
> "For saving the Earth.. and eating cheesecake!"
> --
> cairo mailing list
> cairo at cairographics.org
> http://lists.cairographics.org/mailman/listinfo/cairo
More information about the cairo
mailing list