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

[Bill Spitzak]

On 08/10/2014 12:51 PM, Søren Sandmann wrote:
> 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.

The only reason I made the argument work this way was was to simulate 
the useful results from the previous version. If it was not for 
back-compatability I would remove it entirely.

> 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.

Although box is the only one that is fully accurate, almost all my 
functions are simulating the box reconstruction filter. This can be seen 
in their behavior for sizes less than 1, where they recurse and compute 
the values for a filter 2x as wide and average them.

Yes it is using impulse for sizes of 1 and larger. But that is quite 
close for smooth filters (since it is less or equal to the Nyquist 
frequency). Also it is a requirement to get interpolation.

Because of the point the tent filter's impulse sample is not very close 
to the box reconstruction, as the Nyquist frequency is much higher. But 
this cannot be fixed as it will remove all the size advantages of the 
tent filter, and make it not match other software, and not match box at 
size=1.

Although I get what you are trying to do, I still feel it does not work. 
A literal implementation will not produce interpolating filters, and 
will add the width of the reconstruction filter to the result. 
Multiplying two imperfect low-pass filters just makes a worse low-pass 
filter. And since the reconstruction filter has to change depending on 
the size of the filter, a single argument cannot be used for non-affine 
transforms (if they are added in the future).

> (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.

My main concern is that most software uses the term "cubic" for an 
interpolating filter (often Catmull-Rom but sometimes others). It is a 
bad idea to use this term for a non-interpolating kernel.

> - 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.

I'm ok with using LANCZOS2 and not having the Catmull-Rom one.

> - 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.

Gaussian is useful for blur but that is a totally different api (in fact 
most fast blur algorithms cannot do anything other than gaussian).

The current function lacks windowing, so the sharp ends will produce 
visible artifacts. I thought it was easier to reuse the cubic code to 
get something "blurry" rather than fix this.

> - PIXMAN_KERNEL_NEAREST is just a renaming of PIXMAN_KERNEL_IMPULSE.

I was concerned about using a different term than the 
PIXMAN_FILTER_NEAREST. But this probably is not a real problem.

> - 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.

There has to be a way to get the same result as PIXMAN_FILTER_BILINEAR, 
otherwise you cannot match that in one direction while using filtering 
in another, which I think is necessary for smooth transitions in 
distortion. I was worried that using the word "bilinear" for this was 
misleading, as that literally means two linear interpolations, not one. 
But perhaps this is ok, and LINEAR can continue to mean the tent.

> (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.

It seems to work a lot better in pixman than in Cairo. Pixman is already 
doing lots of analysis of the images to select the BILINEAR or NEAREST 
paths so there is very little additional overhead to picking the right 
filter for GOOD/BEST. It cannot do this for arbitrary filter arrays 
because it needs to know if they are interpolating.

The Cairo one does not fix X11 and XCB backends because they use xrender 
and cannot pass the filter array. I made X11 use the image fallback but 
that change was reverted because it slowed things down too much. The 
only fix is either to add the convolution filter api to XRender, or just 
have GOOD/BEST work as expected. I think fixing GOOD/BEST sounds a lot 
easier.

I really doubt anybody is relying on the current behavior of BEST/GOOD. 
They probably expect what this patch is doing. And it is trivial to 
change to BILINEAR if they really wanted the old behavior.

> (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).

It sets FAST_PATH_SEPARABLE_CONVOLUTION_FILTER which makes that code use 
the filter array.

> 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().

I did post an earlier patch that just changed the arguments to 
pixman_create_separable_convolution but it never produced what was 
wanted. I could not find settings that produced a correct box filter.