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

[Pekka Paalanen]

On Thu, 10 Sep 2015 17:35:03 +0100
"Ben Avison" <bavison at riscosopen.org> wrote:

> On Thu, 10 Sep 2015 12:46:50 +0100, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > 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.
> 
> I sense we're taking a slightly different perspective on the problem
> here. I don't really see these two steps as different in spirit. In the
> first one, the flag calculation was allowing extra space to permit the
> corresponding fast path to be a bit sloppy with its coordinate
> transformations. In the second one, the flag calculation was allowing
> extra space to permit the corresponding fast path to be a bit sloppy with
> loading data that it doesn't need. Apart from meaning that less efficient
> fast paths sometimes get used, this also means a lot of unnecessary cache
> lines get loaded in many cases, which has got to hurt performance.

The difference between these is that the 8e margin is completely
useless already. We can remove it without any other changes. So, we
should do just that. Simple and not controversial at all.

Step 2 is about changing the meaning of a flag and fixing up the code
to correspond. This is more tricky.

Sure, both remove extra space requirements, but they do it on very
different grounds.

> Back to the point at hand, though. I still think documenting the existing
> behaviour is important for the first patch, because it explains why after
> removing the 8e margin, then although we subtract e for the NEAREST case,
> we *don't* do so for the BILINEAR case. Suppose the existing
> implementations had been written to always fetch a 2x2 pixel block, but
> that in the event of the coordinate having fractional part 0, that it was
> right-aligned rather than left-aligned to the relevant source pixel. In
> that case, you *would* have needed to subtract e in the BILINEAR case as
> well as the NEAREST case.

You can leave the comments in the commit message, but please formulate
them to not question the location of the BILINEAR zero-weight pixel.
Instead, say how it has been like it would always remain like that,
since no bit of this patch is preparing for it to change. If this patch
did something extra based on the fact that future patches are expected
to change things wrt. to the zero-weight pixel, then justification for
that extra change would be in place.

If you actually want to document things, then I think
pixman/rounding.txt would be the right place (and another patch). After
all, commit messages are only used to justify the patch, they are not
documentation people usually read.

Curiously enough, that file competely ignores BILINEAR but explains
everything else. I'm not sure if I'm supposed to infer what BILINEAR
does by assuming some particular convolution filter, but I could not
figure out how.

> Perhaps the only way to make this clear is to further split the first
> step into two. To avoid failing cover-test when only one of the two sub-
> step patches was applied, it would have to be in the order:
> 
> Step 1, patch 1: centre the 8e margin over the correct position in the
> NEAREST case
> 
> Step 1, patch 2: remove the 8e margin in both NEAREST and BILINEAR cases

I do not think that would be an improvement. Let's keep it as it is.

> >> 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?
> 
> Assuming you agree with the above, this would then be patch 3 of step 1,
> presumably? None of these are hard code changes, all the argument is
> about the wording of the commit messages, I think. Feel free to compose
> something that you think is appropriate.

Cool, I'll see what I can come up with.


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/20150911/8cd91c4b/attachment.sig>