[cairo] [Pixman] Supersampling - 2nd attempt

[Soeren Sandmann]

Krzysztof Kosiński <tweenk.pl at gmail.com> writes:

> Hello
> 
> This is the second attempt at bitmap supersampling in Pixman.

Some general comments:

I generally like where this is going. It's certainly a large quality
improvement, and the code is well-written. And of course, it fixes
what is probably the biggest image quality issue in pixman.

As I mentioned elsewhere, I don't think there should be any
automatically computed supersampling. While it might be nice to make
image scaling look good without any changes to cairo, I think we are
going to want more fine grained control over the API. Also, simply
changing pixman behavior will cause the X server behavior to change,
which seems undesirable. So by default pixman should still just point
sample.

A question is, what should the API look like? It is probably useful to
make sure we can user other algorithms than regular supersampling in
the future. Suggestions welcome.

Some more specific comments on the code:

- Is it possible to share the supersampling code between the affine
  and projective cases? Maybe the super sampling could be done in an
  inline function that takes other inline functions as arguments.

- The NO_SUPERSAMPLING flag needs to be added all over the place. None
  of the existing fast paths can deal with super sampling. Also, maybe
  it should be renamed to POINT_SAMPLING instead; that would get rid
  of the negative in the name.

- Can we get away with using a simpler algorithm? I can see why you do
  the computation of all the remainders and I appreciate the attention
  to correctness. 

  However, in most cases, the rate_x/rate_y is going to be small
  compared to the fixed point unit of 65536, so it might be possible
  to just compute a "small step" and a "big step":

        normal_step = ux/rate_x
        initial_step = (ux - (rate - 1) * big_step) / 2

  and then just use those instead of the remainder computations.

> +    uxstart = (ux * (rate_x-1) + rate_x) / (2*rate_x);

This seems to compute a uxstart of 

        ux / 2 - ux / (2 * rate_x)

which later gets subtracted from the transformed x coordinate to get
the first subsample coordinate. So far, so good. ux/(2 * rate_x) is
the small initial step inside the transformed pixel, and we do need to
subtract ux/2 to get to the corner of the pixel. But then it seems
that _before_ actually sampling for the first time, you end up adding
ux_frac:

> +		    spx += uxfrac;

which offsets the position by uxfrac.

Also, just to make sure, the "+ rate_x" in the first expression is a
rounding correction, and that is the reason it shouldn't converted to
fixed point? (Adding a fixed point and an integer triggers a red flag
for me, but it might be correct).

Finally, I suspect getting rid of the four per-pixel divisions with
samples is going to be a noticable performance win. It should be
possible to just compute (1/samples) up front and then multiply with
it.


Thanks,
Soren