On Thu, 27 Aug 2015 11:45:22 +0100 "Ben Avison" <bavi...@riscosopen.org> wrote:
> On Thu, 27 Aug 2015 03:55:07 +0100, Siarhei Siamashka > <siarhei.siamas...@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
pgpQsJHTSTg2d.pgp
Description: OpenPGP digital signature
_______________________________________________ Pixman mailing list Pixman@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pixman
