[Pixman] [PATCH 1/7] Refactor calculation of cover flags
ppaalanen at gmail.com
Thu Sep 3 03:53:26 PDT 2015
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
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 811 bytes
Desc: OpenPGP digital signature
More information about the Pixman