[Pixman] [PATCH pixman] V2 Implement PIXMAN_FILTER_GOOD/BEST as separable filters

[Søren Sandmann]

Hi,

The first patch in this series, which changes the choice of scale for so
that it is based on the bounding box of a transformed ellipse, rather
than a transformed square makes sense to me. As you say, this makes the
filter more invariant under rotations and also less blurry. Also, since
the filter will be smaller, it will generally be faster. The downside is
a bit more aliasing, but overall, this seems like a reasonable change.

However, the rest of the series I don't like at all.

These patches contain three types of changes: (a) sampling vs
reconstruction, (b) changing filter kernels, and (c) changing BEST and
GOOD.

(a) sampling vs reconstruction

We may never agree on this, but for the sake of argument, even if I
agreed that it wasn't useful to separate them, users of pixman could
still work around it by simply choosing PIXMAN_KERNEL_IMPULSE for one of
the kernels. So even if I agreed, it still wouldn't be worth it to make
the pixman_create_separable_convolution() treat the reconstruction
argument as a special case for when the scale factor is less than
one. Users (including cairo) can do this themselves if they like.

I don't buy the argument that having "filter evaluators that know the
size of a pixel, and thus can integrate directly" is much faster because
in the current code, when one of the kernels is PIXMAN_KERNEL_IMPULSE,
the integration is effectively skipped:

    ...
    else if (kernel1 == PIXMAN_KERNEL_IMPULSE)
    {
        assert (width == 0.0);
        return filters[kernel2].func (x2 * scale);
    }
    else if (kernel2 == PIXMAN_KERNEL_IMPULSE)
    {
        assert (width == 0.0);
        return filters[kernel1].func (x1);
    }

and except for BOX, having one of the kernels be KERNEL_IMPULSE is
basically the only thing your code supports.

For BOX, your code implicitly recognizes (even though you will never
admit this :-)) that it *is* indeed useful to have the reconstruction
filter be something other than IMPULSE because your code essentially
uses a BOX reconstruction whenever the filter is given as BOX. But of
course, the existing code can already do this by giving BOX for both
reconstruction and sampling.

I do agree that in the BOX case, your code is both faster and more
accurate than the numerical integration in the existing code. In a sense
this is an optimization that recognizes that the box kernel is
particularly simple and that the convolution of two box kernels is
therefore easy to compute without resorting to quadratures. I'd be open
to adding a special case for BOX/BOX in the existing code, especially if
it can be shown to be performance critical in real scaling applications.

(b) Changing filter kernels

Most of these changes seem gratuitous to me:

- PIXMAN_KERNEL_CUBIC renamed to PIXMAN_KERNEL_MITCHELL. Okay, maybe
  Mitchell would have been a better name since the Cubic filter is
  currently the Mitchell-Netravali filter. But even so, changing it is
  just not worth it.

- PIXMAN_KERNEL_CATMULL_ROM. Maybe it's useful to have this filter even
  though it is indistinguishable from Lanczos2. If so, we can just add
  it. But making the existing LANCZOS2 filter silently be CATMULL_ROM is
  just crazy.

- PIXMAN_KERNEL_NOTCH. More or less the same comment. If this filter is
  useful, we can just add it. Reinterpreting GAUSSIAN is crazy,
  especially since a Gaussian blur is clearly useful on its own.

- PIXMAN_KERNEL_NEAREST is just a renaming of PIXMAN_KERNEL_IMPULSE.

- PIXMAN_KERNEL_TENT is more or less what the existing code calls
  PIXMAN_KERNEL_LINEAR, which you have reinterpreted such that it can no
  longer be used for downscaling.

(c) Changes to BEST and GOOD

Finally, the last patch that changes BEST and GOOD to look better. There
is some argument in favor of doing this since that was the original
point of having these filters. However, I think too many people rely
FAST, GOOD, and BEST to do exactly what they are doing now. In general,
Pixman is too low in the stack for magic to be a good idea, so basically
I think it was a mistake to have FAST/GOOD/BEST in the first place.

(Also, given that it doesn't change pixman-bits-image.c, I don't see how
the patch could possibly work at all, but maybe I'm missing something).

-=-

If I understand correctly, a variant of this series has already been
committed to cairo master. I'd disagree with the patches there as well
but ultimately this is not my call, and there is nothing wrong with
Pixman users generating their own convolution matrices and using them
with PIXMAN_FILTER_SEPARABLE_CONVOLUTION if they don't like the ones
generated by pixman_create_separable_convolution().


Søren