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

[Pekka Paalanen]

On Thu, 27 Aug 2015 11:45:22 +0100
"Ben Avison" <bavison at riscosopen.org> wrote:

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

Now that I'm finally getting on track what's going on here, I agree,
this should be done in two steps after we have merged cover-test
upstream:

1. Remove the useless 8e fuzz margins.
2. Change the meaning of the COVER_CLIP_BILINEAR flag so that it is no
   longer safe for fetchers to always fetch a 2x2 pixel block.

Step 1 should be easy to get in.

Step 2 is a little more controversial it seems. Your idea of using
another flag for the transition is a good one, though I wonder if we
should keep the old flag as is, and introduce a new one for the strict
case. If the old flag goes finally unused, we can remove it then. What
remains is the argument of whether we want to do this.

I wonder if step 1 alone will already have a significant impact.

I also think we're going to need the PIXMAN_DISABLE=wholeops thing we
talked about, so that we can force the use of optimized fetchers and
check those don't have problems with cover-test.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/pixman/attachments/20150903/f48e1278/attachment.sig>