[Pixman] [PATCH 1/4] Change conditions for setting FAST_PATH_SAMPLES_COVER_CLIP flags

[Pekka Paalanen]

On Wed, 09 Sep 2015 18:09:04 +0100
"Ben Avison" <bavison at riscosopen.org> wrote:

> On Wed, 09 Sep 2015 11:42:26 +0100, Pekka Paalanen <ppaalanen at gmail.com>
> wrote:
> 
> > On Wed, 9 Sep 2015 09:39:07 +0300
> > Oded Gabbay <oded.gabbay at gmail.com> wrote:
> >
> >> On Fri, Sep 4, 2015 at 5:09 AM, Ben Avison <bavison at riscosopen.org> wrote:
> >> > Many bilinear fast paths currently assume that COVER_CLIP_BILINEAR is
> >> > not set when the transformed upper coordinate corresponds exactly
> >> > to the last row/pixel in source space. This is due to a detail of
> >> > many current implementations - they assume they can always load the
> >> > pixel after the one you get by dividing the coordinate by 2^16 with
> >> > rounding to -infinity.
> >> >
> >> > To avoid causing these implementations to exceed array bounds in
> >> > these cases, the COVER_CLIP_BILINEAR flag retains this feature in
> >> > this patch. Subsequent patches deal with removing this assumption,
> >> > to enable cover fast paths to be used in the maximum number of cases.
> >
> > I'm not sure if these two paragraphs about bilinear are really
> > necessary here. The essence is that we remove the extra margins from
> > both NEAREST (not 8*eps, but 7*eps and 9*eps, see below) and BILINEAR
> > (8*eps), without changing them in any other way.
> >
> > The subsequent patches are still under discussion, and we have to see
> > how they work out.
> 
> I felt that since the point of the patch is about getting the thresholds
> correct to the exact multiple of pixman_fixed_e, and I'd been asked for a
> "strict proof of correctness" in the commit message, then I felt this had
> to be said - after all, I don't think this behaviour is documented
> anywhere else.
> 
> Just witness the confusion about the issue - even Søren posted to say that
> when the coordinate was exactly coincident with a source pixel that the
> pixel with the next-lowest coordinate was loaded and multiplied by 0,
> before correcting himself shortly afterwards to say it was the
> next-highest one.
> 
> I hope I've demonstrated, via three completely different approaches, that
> there's no reason why the upper bound has to be set the way it is - you
> don't even have to sacrifice the ability to do SIMD loads. Even with the
> existing implementations, most of those that do load pixels from the
> next-highest coordinate when we end aligned on a source pixel in the
> horizontal direction, don't do the same in the vertical direction.
> 
> I'm also playing a longer game - but because people get overfaced with
> very long patch series, I'm holding back on some others that build on this
> change. Eventually I re-use the same assembly functions in a context where
> they need to adhere to my tighter limits, but I expect the pool of people
> able to review the assembly code will be pretty small, so the ability to
> demonstrate that they obey the limits when used for a COVER operation is
> very useful.
> 
> Granted, perhaps the last sentence you quoted probably belonged after the
> --- separator, especially if the whole series doesn't get pushed at once,
> but I really want to see all of them to go through. I did originally post
> the whole thing as a single patch, after all (mind you, I've spent a lot
> of time working on bilinear scaling for ARMv6 and ARMv7, and this feels
> like a really tiny and obvious part of it to me now...)

Hi Ben,

you're right, documenting this is important. However, I think this
particular patch is not the best place, and here is why.

When we recently discussed this, both I and Siarhei had the opinion
that this needs to be done in two separate steps:

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.

In that sense, patch 1/4 of this series is step 1. Patches 2, 3 and 4
are step 2, which I assumed to be a follow-up patch series.

We're still pending on cover-test, too, so we're getting quite a lot
ahead of ourselves. Thankfully cover-test is essentially sorted by now.

It seems to me the old maintainers processed each patch series as a
single unit. It makes sense because a series is usually interdependent.
I am more liberal in that, I can accept patches from the top or even
the middle if there are no dependencies. That's why I am looking only
at the step 1 patch right now and making sure it is what we want and
get it merged.

That is why I consider talking about the zero-weight pixel off-topic
for *this one* patch. It is very much on topic on the three other
patches that focus on the definition of COVER in the BILINEAR case,
specifically on the point of zero-weight input pixels.

All in due time, IMHO. At least we are moving now. :-)

> > All the above otherwise looks good to me, but there is one more place
> > that has 8*eps, analyze_extent() in pixman.c has:
> >
> >     if (!compute_transformed_extents (transform, &exp_extents, &transformed))
> > 	return FALSE;
> >    if (!IS_16_16 (transformed.x1 + x_off - 8 * pixman_fixed_e)	||
> > 	!IS_16_16 (transformed.y1 + y_off - 8 * pixman_fixed_e)	||
> > 	!IS_16_16 (transformed.x2 + x_off + 8 * pixman_fixed_e + width)	||
> > 	!IS_16_16 (transformed.y2 + y_off + 8 * pixman_fixed_e + height))
> >     {
> > 	return FALSE;
> >     }
> >
> > Should we be removing these 8*eps too?
> 
> Given what Siarhei said, I think I understand where these came from now.
> I'd agree that the correct thing to do would be to remove the 8*e parts,
> though I think that could safely be in a separate patch.

Very good! That would be patch 2 of step 1. :-)

You're right, making it a separate patch is a good idea as we might
have overlooked something.

Would you like me to do these for step 1?


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/20150910/3b61fc6d/attachment.sig>