[Pixman] Supersampling

[Adrian Johnson]

As there has been no update to the supersampling patch posted by
Krzysztof Kosiński in 2010 [1] I have rebased the patch and incorporated
Søren's review comments [2]. The patches are at:

http://cgit.freedesktop.org/~ajohnson/pixman/log/?h=supersampling

My responses to the review comments are below.


On 24/08/10 01:09, Soeren Sandmann wrote:
> 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.

As a result of an irc discussion with Søren the suggested api is:

typedef enum
{
    PIXMAN_SAMPLE_POINT,
    PIXMAN_SAMPLE_BOX
} pixman_sample_t;

void pixman_image_set_sampling (pixman_image_t  *image,
			        pixman_sample_t  sampling);

The default sampling is PIXMAN_SAMPLE_POINT. When sampling is
PIXMAN_SAMPLE_BOX the supersampling rate is automatically computed.

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

I could not figure out a good way to do this.

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

I've renamed it to POINT_SAMPLING and added it where ever it appeared to
be needed.

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

I've removed 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.

I've moved the small step addition to the end of the loop.

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

I don't know. I left it as is.

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

Fixed.

>
>
> Thanks,
> Soren
>
> --
> cairo mailing list
> cairo at cairographics.org
> http://lists.cairographics.org/mailman/listinfo/cairo


[1] http://lists.cairographics.org/archives/cairo/2010-August/020490.html

[2] http://lists.cairographics.org/archives/cairo/2010-August/020655.html