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

[Ben Avison]

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.

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.

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

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

Ben