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

[Siarhei Siamashka]

On Thu, 3 Sep 2015 13:53:26 +0300
Pekka Paalanen <ppaalanen at gmail.com> wrote:

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

Thanks, it's good to know that all three of us are in a perfect
agreement here.

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

Right. It is a clean and non-controversial change. Just the cover-test
needs to be pushed first.

And the commit message needs to provide explanations with a strict proof
of correctness (it is not very difficult to formulate). Providing only
a single example of one particular matrix is not enough.

The cover-test is useful, but it is only able to demonstrate that the
code is incorrect when it fails (like, for example, the first revision
of Ben's patches). Still the test does not prove anything if it passes.

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

Yes. Basically with the current patch set, Ben is trying to "sell" us
two different things in one bundle. And if we first get rid of the
"8 * pixman_fixed_e" adjustment, then we can look at the new variant
of the BILINEAR COVER flag in isolation. The NEAREST scaling gets
out of the picture too.

I would also suggest to name the new flag as something like "tight"
cover (a neutral description) and avoid names like "accurate",
"precise", "strict", "better", "correct" or anything of this kind.
We should also have a detailed list of extra limitations imposed on
implementations by using this new flag. So that we can compare the
benefits and disadvantages.

-- 
Best regards,
Siarhei Siamashka