[Pixman] [PATCH 1/7] Refactor calculation of cover flags

[Ben Avison]

On Thu, 27 Aug 2015 03:55:07 +0100, Siarhei Siamashka <siarhei.siamashka at gmail.com> wrote:
>> -    /* Expand the source area by a tiny bit so account of different rounding that
>> -     * may happen during sampling. Note that (8 * pixman_fixed_e) is very far from
>> -     * 0.5 so this won't cause the area computed to be overly pessimistic.
>> -     */
>> -    transformed.x1 -= 8 * pixman_fixed_e;
>> -    transformed.y1 -= 8 * pixman_fixed_e;
>> -    transformed.x2 += 8 * pixman_fixed_e;
>> -    transformed.y2 += 8 * pixman_fixed_e;
>
> Thanks for spotting this! I think that this code was used to compensate
> matrix multiplication accuracy problems in older versions of pixman.
> But we have already fixed these accuracy problems some time ago:
[...]
> Now we can drop this "8 * pixman_fixed_e" adjustment because there
> is no accuracy loss in the matrix multiplication for affine
> transformations.

Ah, thank you! I couldn't work out what rounding it was that the comment
was referring to, and it seemed to have been quite deliberate. Søren
could only recall the related issue with bilinear scalers reading pixel
pairs where one source pixel might be read from outside the source array
if its weight was going to be 0.

> And it is probably better to just do this simple fix
> with a single patch. There is no need to spread it across multiple
> patches.

> Just as you do it in
>     http://lists.freedesktop.org/archives/pixman/2015-August/003877.html
> this part becomes:
>
> if (pixman_fixed_to_int (transformed.x1 - pixman_fixed_e) >= 0                &&
>     pixman_fixed_to_int (transformed.y1 - pixman_fixed_e) >= 0                &&
>     pixman_fixed_to_int (transformed.x2 - pixman_fixed_e) < image->bits.width &&
>     pixman_fixed_to_int (transformed.y2 - pixman_fixed_e) < image->bits.height)

Glad you agree about the missing pixman_fixed_e offset, which was
disguised by the old 8 * pixman_fixed_e enlargement.

I originally wrote this stuff as a single patch, but I got the impression
that it was too much to digest in one go for most people, hence why I
split it up. Perhaps the compromise is to go for two patches, one to deal
with the 8 * pixman_fixed_e issue, and one to deal with bilinear scaling
with zero-weight pixels just beyond the edges of the image.

For those following the other thread where I'm describing the operation
of the cover-test program, you may want to note that if you multiply both
sides of the above inequalities by pixman_fixed_1, you get

transformed.x1 - pixman_fixed_e >= 0
transformed.y1 - pixman_fixed_e >= 0
transformed.x2 - pixman_fixed_e < image->bits.width * pixman_fixed_1
transformed.y2 - pixman_fixed_e < image->bits.height * pixman_fixed_1

which is equivalent to

transformed.x1 >= pixman_fixed_e
transformed.y1 >= pixman_fixed_e
transformed.x2 <= image->bits.width * pixman_fixed_1
transformed.y2 <= image->bits.height * pixman_fixed_1

which lines up nicely with my description of which coordinates correspond
to which source pixels.

> And [the bilinear case] does not need any changes. At least as far as
> dropping "8 * pixman_fixed_e" is concerned.

Reworking these in the same way, you get

transformed.x1 - pixman_fixed_1 / 2 >= 0
transformed.y1 - pixman_fixed_1 / 2 >= 0
transformed.x2 + pixman_fixed_1 / 2 < image->bits.width
transformed.y2 + pixman_fixed_1 / 2 < image->bits.height

transformed.x1 >= pixman_fixed_1 / 2
transformed.y1 >= pixman_fixed_1 / 2
transformed.x2 < image->bits.width * pixman_fixed_1 - pixman_fixed_1 / 2
transformed.y2 < image->bits.height * pixman_fixed_1 - pixman_fixed_1 / 2

and then my remaining changes basically boil down to arguing that these
last two rows should be <= not <. That could sensibly be a separate patch.

> I believe that neither of FAST_PATH_SAMPLES_COVER_CLIP_NEAREST and
> FAST_PATH_SAMPLES_COVER_CLIP_BILINEAR flags matters for projective
> transformations, but this can be additionally checked just in case.

I did survey all the existing fast paths and fetchers and discovered that
both the cover flags were only being used for affine transforms when I
was trying to work out what the 8 * pixman_fixed_e rounding might have
been referring to.

By check, do you mean we should ensure that the flags are not set for
projective transforms? If so, I see that compute_image_info() is called
before analyze_extent() so it could be achieved by adding an extra test
in analyze_extent() that FAST_PATH_AFFINE_TRANSFORM is set before setting
the cover flags.

Ben